fix: Allow OPTIONS method on every path

[celian-garcia]

Hello,
We are making cross origin requests from the browser, and it seems the browser executes some OPTIONS requests on top.

The CORS specification is implemented in the browser and server, allowing web applications to make cross-origin requests. The specification also defines how a browser should handle these requests and how servers should respond.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/OPTIONS#preflighted_requests_in_cors

In CORS, a preflight request is sent with the OPTIONS method so that the server can respond if it is acceptable to send the request. In this example, we will request permission for these parameters:

So I'm proposing this PR to let pass these OPTIONS queries.

[celian-garcia]

Converting it back to draft as it seems the OPTIONS proxy query doesn't have the cors response headers while
OPTIONS and GET upstream queries do, and GET proxy queries do as well.

proxy OPTIONS query

curl -iv -X OPTIONS "http://localhost:4444/api/v1/query"
* Host localhost:4444 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:4444...
* Connected to localhost (::1) port 4444
> OPTIONS /api/v1/query HTTP/1.1
> Host: localhost:4444
> User-Agent: curl/8.7.1
> Accept: */*
> 
* Request completely sent off
< HTTP/1.1 200 OK
HTTP/1.1 200 OK
< Date: Wed, 05 Feb 2025 09:04:35 GMT
Date: Wed, 05 Feb 2025 09:04:35 GMT
< Content-Length: 0
Content-Length: 0
< 

upstream OPTIONS queries

curl -iv -X OPTIONS "http://localhost:19000/api/v1/query"
* Host localhost:19000 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:19000...
* Connected to localhost (::1) port 19000
> OPTIONS /api/v1/query HTTP/1.1
> Host: localhost:19000
> User-Agent: curl/8.7.1
> Accept: */*
> 
* Request completely sent off
< HTTP/1.1 204 No Content
HTTP/1.1 204 No Content
< Access-Control-Allow-Headers: Accept, Accept-Encoding, Authorization, Content-Type, Origin
Access-Control-Allow-Headers: Accept, Accept-Encoding, Authorization, Content-Type, Origin
< Access-Control-Allow-Methods: GET, OPTIONS
Access-Control-Allow-Methods: GET, OPTIONS
< Access-Control-Allow-Origin: *
Access-Control-Allow-Origin: *
< Access-Control-Expose-Headers: Date
Access-Control-Expose-Headers: Date
< Date: Wed, 05 Feb 2025 08:24:17 GMT
Date: Wed, 05 Feb 2025 08:24:17 GMT
< Vary: Accept-Encoding
Vary: Accept-Encoding
< 

* Connection #0 to host localhost left intact

Edit: I found the issue. The browser executes an OPTIONS request on /api/v1/query path, with no query in params. The query handler doesn't like when no query in params... so I updated the enforceMethods to execute the passthrough in case of OPTIONS method, which is far less intrusive.

=> proxy OPTIONS query

curl -iv -X OPTIONS "http://localhost:4444/api/v1/query"
* Host localhost:4444 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:4444...
* Connected to localhost (::1) port 4444
> OPTIONS /api/v1/query HTTP/1.1
> Host: localhost:4444
> User-Agent: curl/8.7.1
> Accept: */*
> 
* Request completely sent off
< HTTP/1.1 204 No Content
HTTP/1.1 204 No Content
< Access-Control-Allow-Headers: Accept, Accept-Encoding, Authorization, Content-Type, Origin
Access-Control-Allow-Headers: Accept, Accept-Encoding, Authorization, Content-Type, Origin
< Access-Control-Allow-Methods: GET, OPTIONS
Access-Control-Allow-Methods: GET, OPTIONS
< Access-Control-Allow-Origin: *
Access-Control-Allow-Origin: *
< Access-Control-Expose-Headers: Date
Access-Control-Expose-Headers: Date
< Date: Wed, 05 Feb 2025 09:11:21 GMT
Date: Wed, 05 Feb 2025 09:11:21 GMT
< Vary: Accept-Encoding
Vary: Accept-Encoding
< 

* Connection #0 to host localhost left intact

[Nexucis]

That sounds reasonable to me. But I don't know much about this code, @simonpasquier are you ok with this change ?

[squat]

@celian-garcia could you please include tests so that we document our intentions here? Otherwise I'm sure that passing CORS requests will regress without anyone noticing since we are not validating.

[celian-garcia]

@celian-garcia could you please include tests so that we document our intentions here? Otherwise I'm sure that passing CORS requests will regress without anyone noticing since we are not validating.

Thanks @squat, good idea indeed! Adding the tests I realized that it's working only if I'm using the StaticLabelEnforcer, via labelValues argument. It works in my case but if some people want to use the query param enforcer, they will face an issue as the browser don't set query param in their OPTIONS queries.

[squat]

@celian-garcia can you elaborate?

If the solution is just passing through all OPTIONS requests, why does it matter if the query is in the query parameters or not? Why would this be a problem for CORS users not using the StaticLabelEnforcer?

I'm still a little bit paranoid that this might lead to a leak.

[celian-garcia]

Hi @squat sorry, I checked twice and the query string is actually sent by the OPTIONS request as well. I thought wrongly that the browser was removing query params but that's not true.

So no issue. Enforcers will well reject bad request and accept good requests, being any http method.

[celian-garcia]

@squat another concern? This is really allowing only the OPTIONS requests to passthrough here. Used only for CORS and supported already by thanos, prometheus and alertmanager.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/OPTIONS

[celian-garcia]

@squat @simonpasquier ?

[celian-garcia]

@squat can we have an update? We're running from our fork here and I would like to align

[celian-garcia]

@simonpasquier you merged a dep last week. Do you have an opinion on that? 🙂

[squat]

@celian-garcia now I'm more confused:

Hi @squat sorry, I checked twice and the query string is actually sent by the OPTIONS request as well. I thought wrongly that the browser was removing query params but that's not true.
So no issue. Enforcers will well reject bad request and accept good requests, being any http method.

If query parameters are sent, then why do we have to skip enforcement and perform a passthrough for OPTIONS requests at all? Instead, can we actually enforce the label on all requests? I think a good solution would be to add OPTIONS to the list of allowed methods for every endpoint and enforce labels on all of the requests.

The model I would like to follow is that this proxy prevents Prometheus from ever seeing any HTTP requests with unenforced labels. I think this is the most important invariant for this project.

[celian-garcia]

@celian-garcia now I'm more confused:

Hi @squat sorry, I checked twice and the query string is actually sent by the OPTIONS request as well. I thought wrongly that the browser was removing query params but that's not true.
So no issue. Enforcers will well reject bad request and accept good requests, being any http method.

If query parameters are sent, then why do we have to skip enforcement and perform a passthrough for OPTIONS requests at all? Instead, can we actually enforce the label on all requests? I think a good solution would be to add OPTIONS to the list of allowed methods for every endpoint and enforce labels on all of the requests.

The model I would like to follow is that this proxy prevents Prometheus from ever seeing any HTTP requests with unenforced labels. I think this is the most important invariant for this project.

Query params are passed yes, but not the POST body. So I'd expect underlying function like r.query to not work well.

[celian-garcia]

If I trust the specs, it's even planned to use body to transmit more detailed demand. A reason more to not go into the r.query function.

A client that generates an OPTIONS request containing a payload body
MUST send a valid Content-Type header field describing the
representation media type. Although this specification does not
define any use for such a payload, future extensions to HTTP might
use the OPTIONS body to make more detailed queries about the target
resource.
https://datatracker.ietf.org/doc/html/rfc7231#section-4.3.7

[squat]

Query params are passed yes, but not the POST body.

That's fine, I think we should still make sure to enforce labels on queries in query parameters IFF they are there and ignore if the query parameter contain no query and the body is empty.

I'm open to hearing @simonpasquier's input here, especially regarding the invariant of exposing Prometheus upstreams only to enforced queries.

[SuperQ]

This needs a rebase