BetterErrors / better_errors

Better error page for Rack apps
MIT License
6.88k stars 436 forks source link

Fix CSRF_TOKEN_COOKIE_NAME wrong reference to VERSION constant #481

Closed peaonunes closed 4 years ago

peaonunes commented 4 years ago

It might relate to discussion at https://github.com/BetterErrors/better_errors/issues/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: https://github.com/BetterErrors/better_errors/blob/22b94ff3ec3a580e48eadb4007804aed76868905/better_errors.gemspec#L5-L13 And see: https://github.com/BetterErrors/better_errors/blob/a31224a9bf5b71ffb9ff08104261837230181b41/lib/better_errors/middleware.rb#L166-L169

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

lukasz-wojcik commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

peaonunes commented 4 years ago

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 😄