Conversation
51dd5a4 to
1b591ec
Compare
jeremyevans
left a comment
There was a problem hiding this comment.
We allow overriding to GET, so not allowing overriding to QUERY seems weird. QUERY is supposed to be idempotent, basically GET but with a body. If you don't CSRF check GET, you shouldn't need to CSRF check QUERY.
In terms of CORS-safelisting, only GET, HEAD, and POST are safelisted, and we allow overriding to other non-safelisted methods.
In terms of upside, if you are making a request using a normal HTTP form to an endpoint that is idempotent and expects QUERY (relying on Rack::MethodOverride to convert browser requests), that wouldn't work after this change.
Maybe I'm missing something, but either I don't understand the reasoning, or I don't agree with it.
|
I agree with Jeremy's concern here. A form using Frameworks should use |
|
It does seem weird at first blush! But GET being in the list is the point, not the counterexample. Every override target today is one of two kinds:
In neither case can
QUERY is the first method in that empty cell: exempt AND preflight-protected. So it's the first where tunneling does change treatment. A plain form POST becomes a request the framework trusts on the grounds that forms can't produce it. The natural Since the problem is specific to the form-param route, IMO Samuel's split is the precise fix. Updating the PR to suit. |
Frameworks CSRF-exempt QUERY on the strength of two structural facts: HTML forms cannot emit it, and cross-origin fetch() QUERY is never CORS-safelisted, so it always preflights. The _method parameter is reachable from a plain form POST, so overriding to QUERY there falsifies the first fact for any framework that checks the post-override method — the natural way to write the check. The X-HTTP-Method-Override header carries no such risk: custom headers always force a CORS preflight, and header tunneling is the standing tool for infrastructure that rejects unfamiliar methods. So split the policy by channel: the parameter channel defaults to allowed_overrides minus QUERY, and the new allowed_param_overrides keyword makes the parameter policy explicit for apps that want it wider or narrower.
1b591ec to
c70f697
Compare
_method form-parameter overrides
|
Rescoped to the form-param/header split.
Parameter policy defaults to |
jeremyevans
left a comment
There was a problem hiding this comment.
I disagree that we should block _method=query. You haven't provided an explanation about why CSRF-exempt, Preflight-protected verbs shouldn't allow overrides using a parameter. No evidence is provided to back up: "A plain form POST becomes a request the framework trusts on the grounds that forms can't produce it."
Frameworks shouldn't automatically trust a QUERY request any more or less than a GET request, even if it is preflight protected. QUERY handling should be idempotent, just as GET handling should be. If a framework uses rack.methodoverride.original_method for CSRF exemptions, then all this change does is actively remove the ability to emulate QUERY via a POST form.
Samuel stated: "A form using _method=query is a legitimate way to reach a safe, idempotent, body-bearing endpoint." Even if we want to split the parameter and header policies, we should still allow QUERY by default for both. The split would allow the user to only turn it off for either header or parameter instead of forcing both. I don't think the complexity for the split is worth it.
|
The claim is mechanical, not empirical:
And it's already live: in Roda, For Sinatra folks, rack-protection's Both are reasonable code and both demonstrate the concern here. Keying on the post-override method is the natural and near-universal pattern, which is exactly why the default matters. On trusting QUERY like GET: a forged GET adds no capability. Cross-origin GET is already unpreflighted. A forged QUERY reaches endpoints that the browser promises are preflight-gated. Fetch could have CORS-safelisted QUERY (safe, idempotent) and deliberately didn't. Emulation/override remains available via the header and per-app |
QUERY is generally CSRF exempted because it is read only/idempotent (like GET), not because HTML forms cannot send it. HTML forms can send both GET and POST. GET is CSRF exempted as it is read only/idempotent, POST is not CSRF exempted as it is not. HTML forms cannot send PUT, and PUT is generally not CSRF exempted.
Roda doesn't use the
I think this discussion has shown that using the post-override method is a bad idea. CSRF protection should use |
|
We're in agreement that the exemption tracks safety/idempotence, then, and that keying CSRF on the post-override method is wrong. So native QUERY's exemption isn't in dispute. The only thing on the table is the tunneled The asymmetry with GET is why the tunnel is different: GET and native QUERY are safe to exempt for opposite reasons. GET's exemption never leaned on a preflight. An attacker can already forge cross-origin GETs freely, so exempting one grants nothing, and GET writes nothing. Native QUERY is the mirror image. The preflight is what makes it unforgeable cross-origin, and that gate is the whole reason exempting it is safe. A That leaves us with one question: where does the fix belong? A CSRF layer that keys on If the concern is layering (why CSRF policy lives in a method-rewriting middleware), that's worth treating directly. Samuel's param/header split keeps this as a general channel policy rather than a QUERY special-case, and we can document |
|
We've established that the proposed change breaks a valid use case. We've also established that CSRF protection should look at the original method and not the overridden method if Rack::MethodOverride is used. Your argument is that we cannot fix all CSRF protection, and it's worth breaking the valid use case in order to protect against cases where CSRF protection uses the overridden method. Your other argument is that QUERY is a more serious issue than GET as it has a body. For XSS vulnerabilities, in general you can fit them in the query string limits and don't need to use a request body. For amplification attacks, this doesn't seem like a significant vector for most applications. Most (maybe all) applications that would be vulnerable to an amplification attack with this would also be vulnerable to an amplification attack without it. You can look at the Rack::MethodOverride implementation to see that it parses the entire request body before even making a decision on whether to override the method. Not to mention that the body parsing is done and cached before the method override. This results in POST -> GET overrides retaining their body parameters ( require 'rack'
require 'stringio'
app = proc do |env|
r = Rack::Request.new(env)
p [r.request_method, r.params]
end
Rack::MethodOverride.new(app).call(
"REQUEST_METHOD" => "POST",
"rack.input" => StringIO.new("_method=get&body_param=value")
)
# => ["GET", {"_method" => "get", "body_param" => "value"}]I think the proper fix is at the CSRF protection layer. Fixing the issue there does not break the valid use case. Your point about the lack of documentation is well taken. The fix there is to document the security issues with Rack::MethodOverride, recommend against its use, and describe the changes needed to mitigate the damage, such as using |
#2474 added QUERY to
MethodOverride's override targets. Before that ships in a release, this excludes QUERY from the_methodform-parameter channel while keeping it available via theX-HTTP-Method-Overrideheader — the split @samuel-williams-shopify suggested.Frameworks CSRF-exempt QUERY on the strength of two structural facts: HTML forms cannot emit it, and cross-origin
fetch()QUERY is never CORS-safelisted, so it always preflights. The_methodparameter is reachable from a plain form POST, so overriding to QUERY there falsifies the first fact for any framework that checks the post-override method — the natural way to write the check.The header channel carries no such risk: custom headers always force a CORS preflight, and header tunneling is the standing tool for infrastructure that rejects unfamiliar methods (Puma, for example, accepts only the standard eight by default). So the policy splits by channel: the parameter channel defaults to
allowed_overrides - ["QUERY"], and a newallowed_param_overrides:keyword makes the parameter policy explicit for apps that want it wider or narrower.Since #2474 and the override keywords are both unreleased, this is a no-op relative to the last release.
/cc @seanpdoyle