Skip to content

fix(ui): strip cookies from the analytics proxy request path - #599

Merged
JSONbored merged 1 commit into
JSONbored:mainfrom
galuis116:fix/analytics-proxy-strip-cookies
Jun 11, 2026
Merged

fix(ui): strip cookies from the analytics proxy request path#599
JSONbored merged 1 commit into
JSONbored:mainfrom
galuis116:fix/analytics-proxy-strip-cookies

Conversation

@galuis116

Copy link
Copy Markdown
Contributor

Closes #597.

handleAnalyticsProxy stripped the response set-cookie but did not strip the request cookie header, so the browser's first-party cookies for gittensory.aethereal.dev (session / GitHub OAuth state) were forwarded to the Umami upstream (tasty.aethereal.dev) on every /stats/script.js and /stats/api/send — breaking the documented "cookieless / strips cookies" guarantee (the #595 commit message explicitly claims it "strips cookies").

Changes

  • Add cookie to STRIP_REQUEST_HEADERS so first-party cookies are never forwarded upstream.
  • Add x-forwarded-for to the strip-list and set it to the trusted cf-connecting-ip only (overwrite, not append) so a client-supplied x-forwarded-for can't spoof the visitor's geolocation.
  • Use null (not undefined) for the no-body fetch so the file typechecks under the root tsconfig once a test imports it.
  • Add test/unit/analytics-proxy.test.ts (first test for this proxy): cookies are not forwarded, set-cookie is not relayed back, a spoofed x-forwarded-for is ignored in favor of cf-connecting-ip, and the path allowlist / 405 behavior holds.

Verification

  • tsc --noEmit clean; analytics-proxy.test.ts 3/3 (the cookie and x-forwarded-for assertions fail on the old code). apps/** is excluded from coverage, so the gate is unaffected.

handleAnalyticsProxy stripped response set-cookie but forwarded the browser's first-party request cookies to the Umami upstream, breaking its documented cookieless/strips-cookies guarantee and leaking session/OAuth cookies to the analytics host. Add cookie and x-forwarded-for to the request strip-list and set x-forwarded-for to the trusted cf-connecting-ip only (no client spoofing). Add the first handleAnalyticsProxy unit test.
@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 #599 is no longer open. No action.

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

@ghost ghost added the gittensory:reviewed 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

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 tightens privacy by stripping cookies and client‑supplied x-forwarded-for headers from analytics proxy requests, and simplifies the forwarding of the trusted client IP. It adds comprehensive unit tests to verify the new behavior. The changes are small, well‑documented, and appear safe.

Suggestions

  • Consider adding a brief comment explaining why null is used for the request body instead of undefined.
  • Verify that the TypeScript type for RequestInit.body accepts null in the project’s tsconfig (or cast as null as any if needed).
  • Optionally add a test case ensuring other stripped request headers (e.g., "keep-alive") are also omitted.

Worth double-checking

  • Make sure the runtime environment (Cloudflare Workers) correctly handles body: null for GET/HEAD requests.
  • Confirm that any downstream services consuming the proxied request do not rely on the original x-forwarded-for header.

Reviewer B · nemotron-3-120b-a12b — recommends ✅ merge
The PR strips the Cookie header and client‑supplied X-Forwarded-For from analytics proxy requests, forwarding only the trusted CF‑Connecting-IP to prevent cookie leakage and IP spoofing. It adds corresponding unit tests that verify cookies are not forwarded, X-Forwarded-For is replaced with the trusted IP, and non‑allowlisted paths/methods are handled correctly. The changes are small, focused, and well‑tested.

Suggestions

  • Consider adding a brief comment explaining why body: null is used instead of undefined (though both are functionally equivalent).
  • If the project has a convention for header‑set constants (e.g., all‑uppercase names), ensure STRIP_REQUEST_HEADERS follows it; otherwise it's fine as is.

Worth double-checking

  • Removing X-Forwarded-For may discard legitimate proxy chained IPs if the deployment ever relies on more than just CF‑Connecting-IP; verify that the analytics setup does not need the full chain.
  • Ensure the response‑header stripping logic (not shown in the diff) still removes Set‑Cookie from upstream responses so the test expectation holds.

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

Route Before After
/ before after

@dosubot dosubot Bot added the lgtm label Jun 11, 2026
@JSONbored
JSONbored merged commit 059686e into JSONbored:main Jun 11, 2026
8 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jun 11, 2026
@JSONbored JSONbored added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jun 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

[Bug]: Umami analytics reverse-proxy forwards first-party request cookies to the upstream, breaking its "cookieless / strips cookies" guarantee

2 participants