CORS to respect the configured METHODS

[pmlopes]

Motivation:

Currently the list of configured METHODs are only used to populate the preflight headers. This PR ensures that that requests are blocked 403 as per fetch spec: https://fetch.spec.whatwg.org/#http-cors-protocol

Open for discussion:

• should this be configurable?

[pmlopes]

@Stwissel can you have a look and let me know what you think? The spec is vague, should we block or should we make it configurable?

[tsegismont]

@pmlopes can you point to the relevant section of the spec?

Unfortunately in other sources, there is no mention of the status code https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/CORS/Errors

Btw, when this happens, shouldn't an Access-Control-Allow-Methods header be present?

[pmlopes]

The CORS spec is defined here: https://fetch.spec.whatwg.org/#http-cors-protocol

The ambiguity is here: https://fetch.spec.whatwg.org/#http-responses

Ultimately server developers have a lot of freedom in how they handle HTTP responses and these tactics can differ between the response to the CORS-preflight request and the CORS request that follows it:

• They can provide a static response. This can be helpful when working with caching intermediaries. A static response can both be successful and not successful depending on the CORS request. This is okay.
• They can provide a dynamic response, tuned to CORS request. This can be helpful when the response body is to be tailored to a specific origin or a response needs to have credentials and be successful for a set of origins.

The issue that @Stwissel mentioned on discord is that in one hand we send a preflight that states we only allow GET, POST (as an example) but in fact if we don't have this check we don't block Cross Origin Requests for methods other than the configured ones...

The Origin is explicit in the spec METHOD and HEADERS are ambiguous...

Discord: https://discord.com/channels/751380286071242794/751397798271909941/1346839828745949244

Perhaps the idea of having this behavior behind a flag does make sense and allows us to keep the existing behavior by default?

[pmlopes]

To complement the comment above, in MDN we can see:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/CORS/Errors/CORSMethodNotFound

Browser behavior is expected to block requests if the preflight request includes a list of allowed methods.

The issue here is that if I'd refer to the resource as an HTML image or iframe, e.g.:

<img src="https://cors.protected.site/form/submit/endpoint">

Browsers will allow this (even though the image will be "broken" as it is, in fact, invoking a form endpoint).

The assumption that your forms are "safe" falls in this case, as not all requests are controlled by a prefligh call by the browser.

[tsegismont]

Thanks @pmlopes for all the details.

I think it would be better to have a flag (enabled by default) that controls whether requests with invalid methods should be rejected.

I'm still confused about the status code for rejection, because the CORS spec does not talk about it and MDN talks about 415, not 403.

And I think in the response to the rejected request, we should add the list of allowed headers.

[Stwissel]

We found servers playing fast and loose with origin headers, which in the current implementation fails, but typically a server sends data but without the cors headers. Maybe something like:

CorsHandler corsHandler = CorsHandler.create()
        .allowCredentials(true)
        .allowedMethods(allowedMethods)
        .addRelativeOrigins(new ArrayList<>(corsHosts))
        .allowedHeaders(getAllowedCorsHeaders())
         .exposedHeaders(getExposedCorsHeaders())
        .failOnHostMismatch(false) // default is true, only applies to simple requests, not preflights
        .maxAgeSeconds(7200); // 2 hours in seconds

Small addition to the error message: the value of the origin header

[Stwissel]

context
        .fail(403, new VertxException("CORS Rejected - Invalid origin: "+origin, true));

[pmlopes]

The 403 comes from here:

Any other kind of HTTP response is not successful and will either end up not being shared or fail the CORS-preflight request. ... . If server developers wish to denote this explicitly, the 403 status can be used, coupled with omitting the relevant headers.

415 feels odd. This is related to content type mismatches. https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/415

I like the failOnHostMismatch and probable to make it explicit perhaps: failOnMismatch(boolean)

This should secure the simple requests (not pre-flight)

• if origin header present and method not OPTIONS it's a simple request
• if request method not in list of methods fail
• if request headers not in the allowed list fail

This approach would secure from web-based attacks (CSRF/XSS). No more images or iframes hacks to bypass the cors rules.

Of course, if a request has no origin (say cURL) it would not be filtered by this. This is also true for CORS preflights. So acceptable IMHO.

[Stwissel]

The 403 is fine, I'd just would like a way in the log to determine what origin failed. Good for trouble shooting and risk mitigation. Of course needs to be escaped if the origin is played back to the browser

[tsegismont]

@pmlopes please ping me for another review when the PR is ready, and thanks again for looking into this!