Skip to content

fix(auth): sign in via POST so link prefetches can't use magic links - #584

Open
steadhac wants to merge 1 commit into
GenAI-Security-Project:mainfrom
steadhac:steadhac/fix/magic-link-prefetch
Open

steadhac wants to merge 1 commit into
GenAI-Security-Project:mainfrom
steadhac:steadhac/fix/magic-link-prefetch

Conversation

@steadhac

Copy link
Copy Markdown
Contributor

GET /auth/verify used to mark the magic-link token used and sign in whoever fetched it. Mail scanners, click trackers and link previews fetch links automatically, so they were signed in and the real user saw "Link expired".

  • GET /auth/verify now only renders a confirmation page. Sign-in moves to POST /auth/verify.
  • The token is claimed with a conditional update, so a link cannot be redeemed twice, and the claim is released if the session cannot be created, so a failed sign-in no longer burns the link.
  • SessionMiddleware no longer overwrites a session cookie the route has already set. It re-set the cookie to the temporary session that signing in a returning user had just deleted, leaving them signed out.
  • The auth funnel counts only POSTs to /auth/verify as verifications, since opening a link is now a separate GET.

Fixes #582
Fixes #583
Refs #541

Summary

Fixes two magic-link sign-in bugs:

Changes

File Change
finbot/apps/finbot/auth.py GET /auth/verify now only renders a confirmation page. Sign-in moves to POST /auth/verify. The token is claimed with a conditional update so a link can't be redeemed twice, and the claim is released if the session can't be created, so a failed sign-in no longer burns the link. The session-selection logic is unchanged.
finbot/apps/finbot/templates/auth-confirm.html New "Confirm sign-in" page, styled like auth-error.html.
finbot/core/auth/middleware.py Don't overwrite a session cookie the route has already set.
finbot/core/analytics/queries.py The auth funnel counts only POST /auth/verify as verifications; opening a link is now a separate GET.
tests/integration/auth/test_magic_link_verify.py 12 new tests.

Testing

Automatedtests/integration/auth/test_magic_link_verify.py, 12 tests:

Code Result
This branch 12 passed
Without the middleware change 1 failed — the #583 test
main 11 failed

Every test uses a freshly generated email address, and every retry uses a separate client. A same-client retry is short-circuited by _is_authenticated, and reused addresses take the returning-user path, both of which hide these bugs.

Full suite: 325 passed, 3 failed. The same 3 fail on main (tests/unit/agents/test_specialized_agents.py).

Manual, in two browsers against a local server:

Scenario Before After
Link fetched by curl first, then opened by the user curl gets 303 and a session; user sees "Link expired" curl gets the confirmation page; user signs in
Link opened but not confirmed token used, signed in token unused, still signed out
Same link used from a second browser rejected
Returning user, different browser redirected but signed out signed in
Returning user, session cookie deleted before confirming signed out, two Set-Cookie headers signed in, one Set-Cookie (Max-Age=1209600)

Notes for reviewers

GET /auth/verify used to mark the magic-link token used and sign in whoever
fetched it. Mail scanners, click trackers and link previews fetch links
automatically, so they were signed in and the real user saw "Link expired".

- GET /auth/verify now only renders a confirmation page. Sign-in moves to
  POST /auth/verify.
- The token is claimed with a conditional update, so a link cannot be
  redeemed twice, and the claim is released if the session cannot be
  created, so a failed sign-in no longer burns the link.
- SessionMiddleware no longer overwrites a session cookie the route has
  already set. It re-set the cookie to the temporary session that signing
  in a returning user had just deleted, leaving them signed out.
- The auth funnel counts only POSTs to /auth/verify as verifications, since
  opening a link is now a separate GET.

Fixes GenAI-Security-Project#582
Fixes GenAI-Security-Project#583
Refs GenAI-Security-Project#541
@steadhac

Copy link
Copy Markdown
Contributor Author

@saikishu PR for Fixes #582
Fixes #583

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment