Request Forgery takes relative paths into account

[zealot128]

Passing relative paths into form_for and related / derived helpers led to invalid
token generations, as the tokens did not match the request.path on the
POST endpoint. Variants, such as:

form_for url: "" do

Wouldn't generate a matching csrf-token and led to an InvalidAuthenticityToken.

I've added test + code to handle the common cases, such as:

• ""
• "./"
• "./post_one"
• "post_one"

are now handled according to RFC 3986 5.2 - 5.4.

Not implemented from RFC: double dots are not handled (../../path)

relevant/ fixing issue: #31191

[hiroshi]

Hello Rails team,
Please give your attention to this PR!

I also stumbled upon the bug this PR is designed to fix.
I wasted half a day by digging into actionpack/lib/action_controller/metal/request_forgery_protection.rb to know per_form_csrf_tokens is the culprit... (I learnt a lot about rails csrf protection mechanism though, thanks ;-)

Also, I think it also has a security concern because if user developers of Rails cannot do workaround the bug and decided to turn off protect_from_forgery... Can you imagine?

[dillonwelch]

@eileencodes not sure if you're aware of this PR or the linked issue or if you think it makes sense for either to have the security label.

[duyleekun]

@hiroshi You can just form_for url: request.path as @zealot128 suggested, don't disable it just yet :)

[hiroshi]

You can just form_for url: request.path as @zealot128 suggested, don't disable it just yet :)

The problem is not how to avoid the pitfall. I learnt where the pitfall is at least, but others can fall.
Why don't you just remove the pitfall?

[zealot128]

A collegue ran into this bug again in production, after not receiving after upgrading a older client Rails app to later Rails versions... This bug might slip through test, because by default in Test there is no CSRF protection enabled.

Could a maintainer please check if this could be merged? :)

Pinging @rafaelfranca and @amatsuda because both of you had merges in that code region. Please tell me, if there is a chance to have this merged/fixed.

[tonytonyjan]

hi @zealot128

I wonder if you still suffer the issue in the latest version of Rails.

[zealot128]

hi @zealot128

I wonder if you still suffer the issue in the latest version of Rails.

As the method in the main branch is still the same as of 5 years ago, it would guess it would still be an issue.
In the meantime, I learned to workaround this bug and never use an empty path and always specify url: request.path or similar as the action.

I might try to revive the branch and bring it up to date, but in the last 5 years there was no interest to merge it. So my motivation is quite low. I see, my original PR here also has test files attached, so it would be possible to check those test failures for compliance with the RFC URI spec.

[zealot128]

Thanks @rafaelfranca for finding the time to have a look and merge it! 👍