[Cedric921]

What

Signing in again on a ChatGPT connection that is still signed in now revokes the refresh token it replaces, the same way sign-out and delete do since #2156.

• flows._save reads the connection's current tokens through revocation.tokens_to_revoke before save_sign_in overwrites them, then commits.
• After the commit it calls revocation.revoke, so the new sign-in stands whatever OpenAI answers.
• The same guards apply: the call is skipped when the sign-in host has been turned off, and when the old tokens can't be decrypted.
• A connection that was signed out has nothing to replace, so nothing more is revoked.

Why

docs/architecture/chatgpt-subscription.md listed it under Known gaps, a line #2156 added at review: Sign in again on a live connection overwrote its tokens without revoking the grant they replace, which left that grant open at OpenAI with nothing on this machine able to end it.

Fixes

No issue. This closes that Known gaps line, which is deleted here. The URL table row and the revocation bullet now name signing in again.

How to test

cd surfsense_local/backend
uv run pytest tests/integration/llm/chatgpt tests/integration/llm/test_connections.py -q
uv run ruff check modules/llm tests/integration/llm/chatgpt
python ../../scripts/check_docs.py

Two new tests in test_sign_in_routes.py:

• signing in again over a live sign-in revokes exactly the replaced refresh token and leaves the new one live. It failed before the change.
• signing in again after a sign-out revokes nothing beyond the sign-out's own revocation.

56 passed.

High-level PR Summary

This PR fixes a security gap where signing in again on an existing ChatGPT connection would overwrite tokens without revoking the replaced OAuth grant at OpenAI. The fix ensures that when a user signs in again over a live connection, the old refresh token is revoked at OpenAI (similar to how sign-out and delete already work), preventing orphaned grants that couldn't be terminated. The implementation reads tokens before overwriting them, commits the new sign-in, then revokes the old tokens as a best-effort operation.

⏱️ Estimated Review Time: 5-15 minutes

💡 Review Order Suggestion

Order	File Path
1	docs/architecture/chatgpt-subscription.md
2	surfsense_local/backend/tests/integration/llm/chatgpt/test_sign_in_routes.py
3	surfsense_local/backend/modules/llm/subscriptions/chatgpt/flows.py

Summary by CodeRabbit

• Bug Fixes
  • Signing in again to a ChatGPT connection now revokes a replaced refresh token when it belongs to a different client and revocation is available. The new sign-in remains active if revocation fails.
  • Signing in again after signing out does not trigger an additional revocation.

[vercel]

@Cedric921 is attempting to deploy a commit to the Rohan Verma's projects Team on Vercel.

A member of the Team first needs to authorize it.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: MODSetter/SurfSense/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: da68a9bf-1146-4146-a1e9-ba996e98f61b

📥 Commits

Reviewing files that changed from the base of the PR and between 61f84f9 and 7ddd5fd.

📒 Files selected for processing (3)

• docs/architecture/chatgpt-subscription.md
• surfsense_local/backend/modules/llm/subscriptions/chatgpt/flows.py
• surfsense_local/backend/tests/integration/llm/chatgpt/test_sign_in_routes.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

Repeat sign-in captures tokens from an existing ChatGPT connection. When the new grant uses a different client and revocation is permitted, the flow revokes the old tokens after it saves the new tokens and marks the flow signed_in. Integration tests and architecture documentation describe the behavior and its conditions.

Changes

ChatGPT repeat sign-in

Layer / File(s)

Summary

Repeat sign-in token revocation surfsense_local/backend/modules/llm/subscriptions/chatgpt/flows.py, surfsense_local/backend/tests/integration/llm/chatgpt/test_sign_in_routes.py, docs/architecture/chatgpt-subscription.md

_save returns captured tokens only when the new grant uses a different client and revocation is permitted. _finish marks the flow signed_in before revoking those tokens. Integration tests cover same-client sign-in, unreadable tokens, replacement of another client's tokens, revocation failure, save ordering, and sign-in after sign-out. The documentation describes the revocation conditions and the same-client limitation.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 7ddd5

No identified issue remains that should block merging the repeat sign-in change.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 7ddd5

Saving the new sign-in before revoking an eligible old grant is a sound improvement. No introduced security regression was established, but cleanup remains best effort, same-client tokens are deliberately retained, and deployed configuration and real-provider behavior remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — Each replacement invocation can send one selected connection's old grant to the configured issuer. Across repeated operations, callers with connection-API authority can select the installation's connection rows; this is not evidence of tenant isolation. The existing sign-out API already exposes connection selection and revocation to the same API authority, so no broader selector or newly request-controlled destination was established.

Trust Boundaries and Controls

• observed — The flow retains callback-state validation, rejects a returning flow that names a different client registration, exchanges the code with the PKCE verifier, and delegates ID-token verification with the client ID and nonce before persistence. Revocation uses stored old tokens rather than callback-supplied token values.
• observed — Unreadable secrets suppress cleanup. The configured non-loopback auth host must have an enabled egress row; denied destinations or URL-parsing errors suppress revocation. Loopback destinations are exempt under the existing policy. Eligibility is checked before commit, not rechecked inside the HTTP helper, so this is a policy snapshot rather than continuous enforcement.

Resilience and Maintainability Implications

• inferred — Cancellation while the worker saves can allow the replacement to commit without the coroutine receiving the captured old tokens and dispatching revocation. Process interruption can similarly lose this in-memory cleanup obligation. No durable recovery mechanism is present in the changed flow. These cases leave a preexisting orphan-grant exposure unremediated rather than proving that the PR worsens it.

Hardening Proposals

• proposed — Consider cancellation-resilient post-commit cleanup and explicit interruption testing. If restart recovery requires retaining cleanup credentials, protect them with the existing secret-storage controls, minimize retention, and recheck current egress permission before retrying. This would strengthen the best-effort guarantee, not remediate an established new regression.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: revoking the grant that a ChatGPT sign-in replaces.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 2 files. (1 skipped: 1 …
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[MODSetter]

Reviewed this. The shape matches sign-out and delete:

• the old tokens are read before the save;
• the change commits;
• the old grant is revoked outside the transaction;
• a failed revoke is logged without the token and the sign-in stands.

Two things before it can merge.

1. Since #2192, the token this revokes belongs to the same client as the new sign-in

When this was written, the authorize URL always registered a new dynamic client, so the replaced token belonged to a different client. On dev, signing in again over a live connection takes the returning-user path instead:

• _returning reads the live tokens;
• the authorize request sends the issued client with id_token_hint;
• a callback for any other client is refused.

So every revocation this PR makes now targets the same user, client and resource as the session it just created. RFC 7009 §2.1 lets a server revoke the whole grant when one refresh token is revoked, and Auth0, for one, does exactly that when its "Refresh Token Revocation Deletes Grant" setting is on. Nothing we have shows which way OpenAI goes. If it revokes the grant, signing in again shows "signed in", and then the next refresh gets invalid_grant and signs the person out within the hour. The fake revokes one token at a time, so the tests can't tell.

What to change

• Revoke only when the replaced token's client_id differs from the new one's.
• Keep a reworded Known gaps line: a same-client sign-in again leaves the old refresh token to expire on its own.

2. The guards have no tests on the sign-in-again path

Two changes that break it both pass all 20 tests in test_sign_in_routes.py:

• Reading the tokens directly: replace revocation.tokens_to_revoke(...) with read_tokens(...). A sign-in-again over tokens this install can't decrypt then never settles, because UnreadableSecretError escapes _save.
• Revoking too early: move revocation.revoke(replaced) above session.commit().

What to change

Add three tests:

• Lost key: overwrite the connection's oauth_ciphertext with bytes this install can't decrypt, sign in again, and assert the flow ends signed_in with no revocation.
• Failed revocation: the revoke answers 503. Assert the flow ends signed_in, the new tokens are stored, and "Could not revoke" is logged without the token.
• Order: spy on revocation.revoke and, inside it, read the stored token from a fresh session. It must already be the new one.

Not blocking:

• The dialog can call a finished sign-in "cancelled". signed_in is set only after _save returns, and _save waits for the revoke (up to REVOKE_TIMEOUT). A dialog closed in that window reports a cancelled sign-in that was actually saved. To fix it, return replaced from _save, set signed_in, then revoke with asyncio.to_thread.
• chatgpt-subscription.md covers this only partly. Line 42 now names signing in again, but still says "the sign-out stands" and that a lost key "never blocks the sign-out or delete". Step 6 (line 30) doesn't say that signed_in waits for the revoke.

Push the fixes to this branch and it will be reviewed again.

[Cedric921]

Both fixed, and both non-blocking points too, in the latest push.

1. Same client since feat(local): call each remote model on its own route, and run the agent on a ChatGPT plan #2192.
   • _save now returns the replaced tokens only when their client_id differs from the new sign-in's, so the returning path revokes nothing.
   • Known gaps gets: "Signing in again as the same client leaves the replaced refresh token to expire on its own: whether revoking it would end OpenAI's whole grant is unknown."
2. Guards on the sign-in-again path. The different-client case is reached through the routes: a sign-in started over unreadable tokens registers anew, and another client's tokens are stored before the browser comes back. New tests:
   • Lost key: sign in again over undecryptable oauth_ciphertext. It ends signed_in with no revocation.
   • Failed revocation: with the revoke answering 503, the flow ends signed_in, the new tokens are stored, and "Could not revoke" is logged without the old token.
   • Order: a spy on revocation.revoke reads the stored token from a fresh session, and it is already the new one.
   • Same client: signing in again as the same client revokes nothing. It failed before the fix.
   • Different client: the other client's token is revoked, with its own client_id.

• Non-blocking, also done: _finish sets signed_in as soon as _save returns, then revokes with asyncio.to_thread. A dialog closed during the revoke can no longer report a saved sign-in as cancelled.
• Docs:
  • Step 6 says the revocation runs after signed_in.
  • The revocation bullet is split: sign-out and delete ("the sign-out or delete stands") from signing in again (another client only, after signed_in, same guards).

tests/integration/llm/chatgpt and test_connections.py: 62 passed. Ruff and check_docs.py are clean.

[MODSetter]

Merged into dev, thanks. Revoking only another client's tokens is the right call after #2192. The lost-key, failed-revocation and order tests each catch the change they're there for, and signed_in now comes before the revoke. Two small things, neither blocking:

1. No test checks that signed_in is set before the revoke runs. Moving the revoke back above flow.status = "signed_in" still passes all 24 tests. A revoke spy that blocks on a threading.Event while the test asserts the flow already reads signed_in would pin it.
2. The may_revoke egress guard on the sign-in-again path is untested. Dropping it also passes all 24.