Stop forwarding Authorization header to third-party integration APIs#833
Stop forwarding Authorization header to third-party integration APIs#833aram356 wants to merge 2 commits into
Conversation
The integration API proxies forwarded the publisher site's `Authorization`
header (e.g. a staging basic-auth credential) to the third-party upstream.
The Lockr API rejects any request carrying an unexpected `Authorization`
with `{"success":false,"error":{"code":400,"message":"Invalid request"}}`,
which short-circuits the SDK's `getAIMSettings()` init — no settings data,
no page-view, no identity tokens — on any deployment behind site auth.
Forwarding it also leaks the publisher credential to the third party.
Auditing the other integrations found the same copy-paste in permutive and
didomi; sourcepoint already omits it intentionally; datadome, prebid, and
gpt never forwarded it. Stop forwarding `Authorization` in lockr, permutive,
and didomi to match sourcepoint.
Add a regression test asserting the Lockr proxy forwards the POST body and
content-type but strips `Authorization`, and record outgoing request bodies
in the stub HTTP client so the test can assert on them.
aram356
force-pushed
the
test-lockr-proxy-forwards-post-body
branch
from
7866e22 to
663caf2
Compare
aram356
changed the title
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed the Trusted Server PR that strips Authorization from Lockr, Permutive, and Didomi integration proxies. The direction looks correct and CI is green. I am approving as requested, with inline notes for a remaining Lockr cookie-forwarding credential-leak risk and follow-up test coverage/test-infrastructure hardening.
| from: &HeaderMap<HeaderValue>, | ||
| to: &mut HeaderMap<HeaderValue>, | ||
| ) -> Result<(), Report<TrustedServerError>> { | ||
| // NOTE: `Authorization` is intentionally NOT forwarded. It carries the |
There was a problem hiding this comment.
Lockr still forwards publisher cookies upstream
This header allowlist now correctly avoids forwarding Authorization, but the same proxy path still calls copy_cookie_header() below, which forwards the inbound publisher Cookie header after removing only consent cookie names. Because /integrations/lockr/api/... is first-party, browsers attach publisher cookies automatically; session/auth/staging cookies can still be sent to Lockr, which is the same credential-leak class this PR is addressing.
Suggestion: Omit Cookie entirely for Lockr if Lockr does not require it, or replace copy_cookie_header() with an explicit Lockr cookie allowlist. Do not forward unparseable cookie headers unchanged, and add a regression test with Cookie: session_id=secret; euconsent-v2=tcf asserting unrelated publisher cookies do not reach the upstream.
There was a problem hiding this comment.
Fixed. Lockr now forwards no Cookie header at all — copy_cookie_header() is removed (it also forwarded unparseable cookie headers verbatim). Verified the upstream does not need it: the successful direct request carried no Cookie, the SDK passes its first-party cookie data in the request body (firstPartyCookies), and Permutive/Didomi already forward no cookies. Regression test lockr_proxy_forwards_body_and_strips_publisher_credentials sends Cookie: session_id=secret; euconsent-v2=tcf and asserts no Cookie reaches the upstream.
|
|
||
| /// Copy relevant request headers for proxying. | ||
| fn copy_request_headers(&self, from: &HeaderMap<HeaderValue>, to: &mut HeaderMap<HeaderValue>) { | ||
| // `Authorization` is intentionally NOT forwarded: it carries the |
There was a problem hiding this comment.
Add regression coverage for Permutive authorization stripping
This security-sensitive behavior changed for Permutive too, but the new regression coverage only exercises Lockr. A future allowlist edit could reintroduce the Authorization credential leak here without failing tests.
Suggestion: Add a focused Permutive proxy test that sends Authorization to the first-party route and asserts the recorded upstream request omits it while preserving required headers/body.
There was a problem hiding this comment.
Added permutive_proxy_strips_authorization: sends Authorization through the first-party route and asserts the recorded upstream request omits it while still forwarding user-agent.
| ); | ||
| } | ||
|
|
||
| // `Authorization` is intentionally NOT forwarded: it carries the |
There was a problem hiding this comment.
Add regression coverage for Didomi authorization stripping
This security-sensitive behavior changed for Didomi too, but the new regression coverage only exercises Lockr. A future allowlist edit could reintroduce the Authorization credential leak here without failing tests.
Suggestion: Add a focused Didomi proxy test that sends Authorization to the first-party route and asserts the recorded upstream request omits it while preserving required headers/body.
There was a problem hiding this comment.
Added copy_headers_strips_authorization: passes Authorization into copy_headers and asserts it is not present on the proxy request while user-agent is preserved.
|
|
||
| // Capture the outgoing request body so tests can assert it is forwarded. | ||
| let (_, body) = request.request.into_parts(); | ||
| let body_bytes = body |
There was a problem hiding this comment.
Do not hide request-body capture failures in StubHttpClient
into_bytes_bounded(...).await.map(...).unwrap_or_default() records an empty body if collection fails or exceeds the limit, then still returns the queued response. Tests using recorded_request_bodies() cannot distinguish an intentionally empty body from a capture failure.
Suggestion: Propagate the collection error as PlatformError::HttpClient with context, e.g. change_context(PlatformError::HttpClient).attach("failed to capture StubHttpClient request body")?, instead of defaulting to Vec::new().
There was a problem hiding this comment.
Fixed. Replaced unwrap_or_default() with change_context(PlatformError::HttpClient).attach("failed to capture StubHttpClient request body")?, so a capture failure now surfaces as a send error instead of being recorded as an empty body.
…, surface stub body-capture errors - Lockr: stop forwarding the publisher `Cookie` header to the upstream. Under the first-party proxy the browser attaches publisher session/auth cookies to `/integrations/lockr/api/...`; forwarding them (minus only consent names) is the same credential-leak class as `Authorization`. The SDK already sends the identity cookie data it needs in the request body, so no `Cookie` is required. Removes the now-unused `copy_cookie_header` (which also forwarded unparseable cookie headers verbatim). - Add Permutive and Didomi regression tests asserting `Authorization` is not forwarded upstream, so a future allowlist edit can't silently reintroduce the leak. - Extend the Lockr test to assert the `Cookie` header is stripped too. - StubHttpClient: propagate request-body capture failures instead of recording an empty body, so tests can't mistake a capture failure for an empty body.
3 participants
Root cause (verified with a local
fastly compute servereproduction)The integration API proxies forward the publisher site's
Authorizationheader (e.g. a staging basic-auth credential) to the third-party upstream. The Lockr API rejects any request carrying an unexpectedAuthorization:{"success":false,"error":{"code":400,"message":"Invalid request"}}This short-circuits the SDK's
getAIMSettings()init (which gates onsuccess && data) — no settings data, nopvStatus, nopage-view, no identity tokens — on any deployment served behind site auth. It also leaks the publisher credential to the third party.How it was confirmed
success; adding only anAuthorizationheader flips it to the exact400 Invalid request. Missing Origin returns500, missing body/content-type returns400from a different cause — both ruled out.fastly compute serve(legacy path, lockr enabled) reproduced the400; debug logging showed the proxy puttingauthorizationon the outbound wire. Removing it → upstream no longer returns the400.This corrects two earlier misdiagnoses (Origin — PR #831, closed; POST body — the original framing of this PR). The body and content-type were always forwarded correctly.
Fix
Stop forwarding
Authorizationin lockr, permutive, and didomi.Audit of all integration proxies:
Authorization?Tests
lockr_proxy_forwards_body_and_strips_authorization: asserts the proxy forwards the POST body + content-type but stripsAuthorization. Extends the stub HTTP client to record outgoing request bodies.cargo test -p trusted-server-core— 1455 passed;fmt+clippyclean.Fixes #832