Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CSRF_TOKEN_COOKIE_NAME wrong reference to VERSION constant #481

Merged

Conversation

peaonunes
Copy link
Contributor

It might relate to discussion at #479

After upgrading to 2.8.2 I started experiencing the following error:

11:58:02 rails.1   | /Users/my-user/.rvm/gems/ruby-2.6.6/gems/better_errors-2.8.2/lib/better_errors/middleware.rb:43:in `<class:Middleware>‘: uninitialized constant BetterErrors::Middleware::VERSION (NameError)
11:58:02 rails.1   | 	from /Users/my-user/.rvm/gems/ruby-2.6.6/gems/better_errors-2.8.2/lib/better_errors/middleware.rb:28:in `<module:BetterErrors>'
11:58:02 rails.1   | 	from /Users/my-user/.rvm/gems/ruby-2.6.6/gems/better_errors-2.8.2/lib/better_errors/middleware.rb:7:in `<top (required)>'
11:58:02 rails.1   | 	from /Users/my-user/.rvm/gems/ruby-2.6.6/gems/better_errors-2.8.2/lib/better_errors.rb:9:in `<top (required)>'

I might have found the issue. I quickly looked at other places using the gem version and saw that they are using BetterErrors::VERSION instead of VERSION.

See:

Gem::Specification.new do |s|
s.name = "better_errors"
s.version = BetterErrors::VERSION
s.authors = ["Charlie Somerville"]
s.email = ["[email protected]"]
s.description = %q{Provides a better error page for Rails and other Rack apps. Includes source code inspection, a live REPL and local/instance variable inspection for all stack frames.}
s.summary = %q{Better error page for Rails and other Rack apps}
s.homepage = "https://github.com/BetterErrors/better_errors"
s.license = "MIT"

And see:
def no_errors_page
"<h1>No errors</h1><p>No errors have been recorded yet.</p><hr>" +
"<code>Better Errors v#{BetterErrors::VERSION}</code>"
end

Replace the VERSION constant reference in an attempt to solve the issue. Please let me know if that's alright 😄

@manuelmeurer manuelmeurer mentioned this pull request Oct 2, 2020
@lukasz-wojcik
Copy link

lukasz-wojcik commented Oct 2, 2020

This fix does not seem to work (tried it locally). I think it is because middleware.rb is loaded before the version.rb and at the time Middleware class references VERSION there is no such constant. This would work in runtime but constant is referenced at load time. What helped locally is to require_relative 'version' and then use the constant.

EDIT
you could move this require https://github.com/BetterErrors/better_errors/blob/master/lib/better_errors.rb#L13 to the top so that BetterErrors::VERSION is defined when loadingmiddleware.rb

@peaonunes
Copy link
Contributor Author

This fix does not seem to work (tried it locally). I think it is because middleware.rb is loaded before the version.rb and at the time Middleware class references VERSION there is no such constant. This would work in runtime but constant is referenced at load time. What helped locally is to require_relative 'version' and then use the constant.

EDIT
you could move this require https://github.com/BetterErrors/better_errors/blob/master/lib/better_errors.rb#L13 to the top so that BetterErrors::VERSION is defined when loadingmiddleware.rb

Good point! It really is a static definition so the version constant must be loaded already. It should be pretty safe to move the require up since the file does not depend on anything else. Done it here: 90789f9

@gavindidrichsen
Copy link

I'm seeing the same issue in 2.82 and have tested this PR fix locally. It works great and solves my problem. When can you get this PR merged?

Cheers!

@peaonunes
Copy link
Contributor Author

I'm seeing the same issue in 2.82 and have tested this PR fix locally. It works great and solves my problem. When can you get this PR merged?

Cheers!

Good to know! I do not have permissions to merge in, but @RobinDaugherty should be able to do so.

@RobinDaugherty
Copy link
Member

Thank you @peaonunes! Even though #480 also fixed the issue, I'm merging yours as well because it changes the order of require calls, which is also a good change to make.

@RobinDaugherty RobinDaugherty merged commit d577691 into BetterErrors:master Oct 3, 2020
@peaonunes
Copy link
Contributor Author

Thank you @peaonunes! Even though #480 also fixed the issue, I'm merging yours as well because it changes the order of require calls, which is also a good change to make.

I have not even realised the #480 I kinda did everything on a rush yesterday. Glad I helped 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants