CORS requests not working as expected when OPA is being used

[nicolaracco]

My stac-auth-proxy instance is configured with an upstream that handles CORS requests, and is integrated with OPA for filtering.
I'm currently experiencing issues during CORS requests to stac-auth-proxy (which should rely on the upstream response). After breaking down the flow, here’s what I observed when performing an authenticated OPTIONS request to https://stac-auth-proxy/collections/something:

• The OPTIONS request bypasses AuthEnforceMiddleware, as expected, and proceeds to call OPA.
• Depending on what OPA returns, there are three possible scenarios — none of which results in correct CORS handling from my experiments:
  • OPA returns void (the response is empty):
    • When the OPA policy returns nothing, the Cql2BuildFilterMiddleware raises an error.
  • OPA returns null:
    • The Cql2BuildFilterMiddleware catches a ValidationError and aborts the request with a 502.
    • Subsequently, AddProcessingTimeHeaderMiddleware also raises an error because its call to call_next fails with a RuntimeError: "No response returned".
  • OPA returns an always-true expression (for example "1=1")
    • stac-auth-proxy forwards the OPTIONS request upstream, which correctly returns an empty response body.
    • However, Cql2ValidateResponseBodyMiddleware raises a ParseError because it’s expecting a response body whenever a CQL2 filter is present

From my perspective, unless I’m missing something fundamental, there seem to be several potential improvement points:

• Cql2ValidateResponseBodyMiddleware should not assume a body is always present. If the response body is empty, the middleware should handle it gracefully.
• Cql2BuildFilterMiddleware should not abort the request immediately. Instead, it could raise a proper HTTPException so downstream middleware doesn’t break.
• Cql2BuildFilterMiddleware should allow empty filter results and null values to mean “no filter to apply”. Apart from OPTIONS requests, it could prove useful to avoid applying filters when they are not needed.

What do you think about these potential changes?

[alukach]

Thanks for the well thought-out ticket, @nicolaracco!

At a high level, it sounds like what you're saying is that the STAC Auth Proxy experiences issues when a CQL2 filter factory returns an invalid CQL2 filter expression. In your case, this is None or maybe '' (I'm not exactly sure what the difference is between OPA's void and null in terms of a JSON response). As stated in the docs¹, a filter factory must return a valid CQL2 dictionary or string. This extends to OPA².

I think you're correct that we should more robustly handle situations when these requirements are not met by CQL2 factories.

The OPTIONS request bypasses AuthEnforceMiddleware, as expected, and proceeds to call OPA.

This makes sense, but I wonder if the last part of your statement is actually be undesirable behavior. Is there any reason why we would actually want to run the CQL2 filters on OPTIONS requests? I think that the response to an OPTIONS request should not be dependent on the content filters (even if a 404 is to be returned on a GET, I think the OPTIONS request should still return a 204). I'm admittedly not certain about this.

stac-auth-proxy/src/stac_auth_proxy/middleware/Cql2BuildFilterMiddleware.py

Lines 75 to 91 in 6396563

	filter_builder = self._get_filter(request.url.path)
	if not filter_builder:
	return await self.app(scope, receive, send)
	filter_expr = await filter_builder(
	{
	"req": {
	"path": request.url.path,
	"method": request.method,
	"query_params": dict(request.query_params),
	"path_params": requests.extract_variables(request.url.path),
	"headers": dict(request.headers),
	},
	**scope["state"],
	}
	)
	cql2_filter = Expr(filter_expr)

Cql2BuildFilterMiddleware should allow empty filter results and null values to mean “no filter to apply”.

While I understand the convenience of this, we're currently pretty strict about the CQL2 filters and thus suggest that a filter factory should instead return 1=1. That seems unergonomic, but I worry that permitting None could open up implementors to make mistakes and accidentally expose sensitive data. I'm not totally certain that is the right way to go, but that's where my thinking is at right now.

Footnotes

1. https://github.com/developmentseed/stac-auth-proxy/blob/v0.10.1/docs/user-guide/record-level-auth.md?plain=1#L66-L72 ↩

2. https://github.com/developmentseed/stac-auth-proxy/blob/v0.10.1/docs/user-guide/record-level-auth.md?plain=1#L165 ↩