Skip to content

fix: strip access token via redirect#9170

Merged
mscolnick merged 2 commits intomainfrom
ms/strip-access-token-via-redirect
Apr 13, 2026
Merged

fix: strip access token via redirect#9170
mscolnick merged 2 commits intomainfrom
ms/strip-access-token-via-redirect

Conversation

@mscolnick
Copy link
Copy Markdown
Contributor

@mscolnick mscolnick commented Apr 13, 2026

Defense in depth for the edit-mode access_token. Previously the token stayed in window.location.search until the frontend's cleanupAuthQueryParams ran.

  • In the / route, when the query param is still present (auth has already validated it and promoted it to a session cookie), 303 redirect to the same URL with the token removed. The Set-Cookie rides the redirect, so the browser lands on a clean authenticated URL before any notebook-controlled content is parsed.
  • Add Referrer-Policy: no-referrer and X-Content-Type-Options: nosniff on HTML page responses to block Referer leakage and MIME sniffing.

Copilot AI review requested due to automatic review settings April 13, 2026 19:36
@vercel
Copy link
Copy Markdown

vercel bot commented Apr 13, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
marimo-docs Ready Ready Preview, Comment Apr 13, 2026 8:14pm

Request Review

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the root HTML endpoint (GET /) by stripping access_token from the query string via a redirect after successful authentication, reducing the chance the plaintext token is leaked (e.g., via history or referrers).

Changes:

  • Add a 303 redirect on / when access_token is present, redirecting to the same path with the token removed.
  • Add HTML response security headers (Referrer-Policy: no-referrer, X-Content-Type-Options: nosniff) to both the redirect and the final HTML response.
  • Add endpoint tests covering token stripping, parameter preservation, invalid-token behavior, and the new headers.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
marimo/_server/api/endpoints/assets.py Implements access-token stripping redirect for / and applies HTML security headers.
tests/_server/api/endpoints/test_assets.py Adds regression tests validating redirect behavior, header presence, and invalid-token handling.

Two test-side touchpoints hit `/?access_token=...` and broke when the
server started 303-redirecting to strip the token:

- `_try_fetch` used `urllib.request.urlopen`, which follows redirects
  but does not preserve cookies. Swap to an `HTTPCookieProcessor`-backed
  opener so the session cookie set on the redirect response is replayed
  on the follow-up GET and the client lands on the authenticated page
  instead of the login screen.
- `test_auth_by_query` asserted a direct 200 with `Set-Cookie`. It now
  explicitly does not follow the redirect, asserts the 303 + Location +
  Set-Cookie on the redirect response, and then verifies that the
  cookie authenticates the follow-up request.
akshayka
akshayka previously approved these changes Apr 13, 2026
@mscolnick mscolnick added the bug Something isn't working label Apr 13, 2026
@mscolnick mscolnick merged commit 727dc67 into main Apr 13, 2026
55 of 63 checks passed
@mscolnick mscolnick deleted the ms/strip-access-token-via-redirect branch April 13, 2026 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants