Skip to content

fix: forward request headers when checking existing - #201

Open
vincentsarago wants to merge 1 commit into
mainfrom
fix/forward-headers-in-validate-transactions
Open

fix: forward request headers when checking existing#201
vincentsarago wants to merge 1 commit into
mainfrom
fix/forward-headers-in-validate-transactions

Conversation

@vincentsarago

Copy link
Copy Markdown
Member

disclaimer: This PR was mostly done by Claude but I reviewed all the steps

I hit an issue when trying to create/update/delete collections (using PUT requests) where I got 502 errors 👇

re-running ingest_data.py against an already-seeded database hits a 502 Bad Gateway on the PUT (update) fallback. That's in the external stac-auth-proxy dependency, not this repo — Cql2ValidateTransactionMiddleware._fetch_existing (Cql2ValidateTransactionMiddleware.py:123-129) does an internal GET to fetch the existing record for update-validation but never forwards the caller's Authorization header, so it 401s against a private endpoint and that gets mapped to a 502. Worth a heads-up since your ingest script is idempotent-by-design (POST-then-PUT-on-409) and will hit this on every re-run

@vincentsarago
vincentsarago requested a review from alukach July 28, 2026 02:36
@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown

Claude finished @vincentsarago's task in 2m 2s —— View job


Claude's Task List

  • Gather context (diff, files, CLAUDE.md)
  • Review correctness / security
  • Apply ponytail-review for over-engineering
  • Post final review

✅ No blocking issues — safe to merge.

The fix is correct and well-scoped: EnforceAuthMiddleware runs before Cql2ValidateTransactionMiddleware in the middleware stack (app.py — last-added-runs-first), so the caller is already authenticated by the time this middleware's internal _fetch_existing GET fires. Forwarding the caller's Authorization/Cookie headers to that internal call is therefore appropriate, not a privilege-escalation concern. The header-stripping list (host, content-length, content-type, transfer-encoding) correctly prevents the original bodied PUT/PATCH's Content-Length/Content-Type from leaking onto the bodyless internal GET, which the regression test (test_does_not_forward_incoming_content_length) confirms.

Non-blocking observations:

  • Cql2ValidateTransactionMiddleware.py:25-27_forward_headers builds a dict via {k: v for k, v in headers.items() ...}. Starlette's Headers.items() can yield repeated keys for multi-valued headers (e.g. multiple Forwarded/X-Forwarded-For from an upstream proxy); the dict comprehension silently keeps only the last occurrence. Unlikely to matter for the Authorization/Cookie case this PR targets, so not blocking.
  • Cql2ValidateTransactionMiddleware.py:133-139_fetch_existing's headers parameter is typed Optional[Headers] = None with a ... if headers else None fallback, but both call sites (_handle_update, _handle_delete) always pass a real Headers object now — the None path is unreachable in practice.

Simplify (ponytail)

  • Cql2ValidateTransactionMiddleware.py:133-139 — drop the now-dead Optional[...] = None default and the if headers else None ternary on _fetch_existing; make headers: Headers required and pass it straight to _forward_headers, since every real caller already supplies it.

💰 Estimated review cost: $0.75 · 2m01s · 21 turns

@github-actions github-actions Bot added the fix label Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant