Skip to content

fix(ui): strip credentials from analytics proxy - #598

Closed
JSONbored wants to merge 2 commits into
mainfrom
codex/fix-analytics-proxy-credential-leak
Closed

fix(ui): strip credentials from analytics proxy#598
JSONbored wants to merge 2 commits into
mainfrom
codex/fix-analytics-proxy-credential-leak

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • The first-party analytics proxy was unintentionally forwarding sensitive first-party credentials (e.g. Cookie, Authorization, Proxy-Authorization) to the upstream Umami host.
  • That forwarding can leak HttpOnly cookies or authorization headers to the analytics service and its logs, so the proxy must fail closed for credentials while preserving telemetry.
  • The change aims to remove credential forwarding with minimal impact to the proxy behaviour and site analytics.

Description

  • Add authorization, cookie, and proxy-authorization to the inbound header denylist in apps/gittensory-ui/src/lib/analytics-proxy.ts so they are not forwarded upstream.
  • Build the proxied request RequestInit (upstreamRequest) and attach the buffered body only when present to avoid passing an explicit undefined body to fetch while preserving GET/HEAD behaviour.
  • Add a unit test test/unit/analytics-proxy.test.ts that verifies analytics headers and x-forwarded-for are preserved while Authorization, Cookie, and Proxy-Authorization are not forwarded.

Testing

  • Ran the new unit test with npx vitest run test/unit/analytics-proxy.test.ts --reporter verbose and it passed.
  • Ran the full TypeScript check with npm run typecheck and it completed successfully.
  • Ran the UI workspace typecheck with npm run ui:typecheck and it completed successfully.

Codex Task

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jun 11, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui 6d7095b Commit Preview URL

Branch Preview URL
Jun 11 2026, 10:13 AM

@dosubot dosubot Bot added the size:S label Jun 11, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@ghost

ghost commented Jun 11, 2026

Copy link
Copy Markdown

Note

Gittensory Gate skipped

PR closed before full evaluation. No late first comment was created.

Signal Result Evidence Action
Gate result ⚠️ Skipped #598 is no longer open. No action.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

@ghost

ghost commented Jun 11, 2026

Copy link
Copy Markdown

reviewbot · advisory review

Reviewed 2 changed file(s) — two independent AI reviewers.

Suggested action:Safe to merge — both reviewers found no blocking issues.

Reviewer A · gpt-oss-120b — recommends ✅ merge
The PR hardens the analytics proxy by stripping first‑party credential headers (Authorization, Cookie, Proxy‑Authorization) and refactors the fetch call to use a pre‑built request object. A unit test verifies the new stripping behavior. The changes are small, well‑scoped, and pass the added test.

Suggestions

  • Add a test case to confirm that header stripping works regardless of header name casing (e.g., 'Authorization' vs 'authorization').
  • Consider adding a test for a GET request to ensure no body is attached and the proxy still forwards correctly.

Worth double-checking

  • If the header stripping logic is case‑sensitive, the new headers might still be forwarded on some browsers.
  • The upstreamRequest.body is set to an ArrayBuffer; ensure this is compatible with all environments used by the app.

Reviewer B · nemotron-3-120b-a12b — recommends ✅ merge
The PR improves security by preventing forwarding of Authorization, Cookie, and Proxy‑Authorization headers to the analytics upstream, refactors the fetch call to buffer the request body once, and adds a unit test verifying that those credentials are stripped while other headers (e.g., cf‑connecting‑ip, x‑forwarded‑for) are forwarded correctly. The change is straightforward and low‑risk.

Suggestions

  • Verify that the STRIP_REQUEST_HEADERS set is indeed used elsewhere in the file to filter headers (if not, the addition will be ineffective).
  • Consider adding a test case for a GET request (no body) to ensure the same header stripping applies.
  • Add a brief comment near the STRIP_REQUEST_HEADERS definition explaining why these specific headers are hop‑by‑hop or credentials‑related.

Worth double-checking

  • If the header‑filtering logic that references STRIP_REQUEST_HEADERS is missing or broken, the added headers would still be forwarded.
  • The test relies on the current behavior of merging x‑forwarded‑for and cf‑connecting‑ip; ensure the upstream logic that combines them is unchanged.
  • Buffering the body with arrayBuffer() is fine for tiny payloads, but confirm that analytics events remain small enough to avoid memory issues.

Before = production · After = this PR's preview deploy.

Route Before After
/ before after

@JSONbored JSONbored closed this Jun 11, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jun 11, 2026
@JSONbored
JSONbored deleted the codex/fix-analytics-proxy-credential-leak branch June 11, 2026 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant