Skip to content

fix(local): revoke the grant a ChatGPT sign-in replaces - #2174

Open
Cedric921 wants to merge 2 commits into
MODSetter:devfrom
Cedric921:fix/chatgpt-revoke-on-sign-in-again
Open

Cedric921 wants to merge 2 commits into
MODSetter:devfrom
Cedric921:fix/chatgpt-revoke-on-sign-in-again

Conversation

@Cedric921

@Cedric921 Cedric921 commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

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

Need help? Join our Discord

Summary by CodeRabbit

  • Bug Fixes
    • Signing in again to an existing ChatGPT connection now revokes the replaced active grant after the new sign-in is saved, when revocation is eligible. The newly issued grant remains active.
    • Signing in again after signing out does not trigger an additional revocation, since signing out has already ended the previous grant.

@vercel

vercel Bot commented Oct 5, 2026

Copy link
Copy Markdown

@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

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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: e81f6e69-768f-4edb-869c-b8f2f07d9f13
📥 Commits

Reviewing files that changed from the base of the PR and between 9560e8a and 61f84f9.

📒 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
💤 Files with no reviewable changes (1)
  • docs/architecture/chatgpt-subscription.md

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 and revokes them after saving when revocation is permitted. Integration tests cover repeat sign-in with and without an active grant. Architecture documentation describes the updated behavior.

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
The save flow captures tokens from an existing connection and revokes them after commit when permitted. Integration tests cover repeat sign-in with an active grant and after sign-out. The architecture documentation includes repeat sign-in among revocation events and removes the corresponding known-gap statement.

Priority: ⬇️ Low

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 61f84

The replacement sign-in remains successful even if the revocation request fails. No actionable merge-blocking issue was identified.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 61f84

The change closes an existing credential-cleanup gap while preserving the replacement sign-in when revocation fails. No introduced security concern was established. Risk remains low because issuer behavior has only been exercised with a fake service, and revocation remains best effort.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — The new sensitive operation targets the stored refresh token of the selected local ChatGPT connection. The caller selects a connection ID, not a revocation destination; the save path revalidates that the selected row is a ChatGPT connection.

Trust Boundaries and Controls

  • observed — Callback state, returning-client identity, required scope, and ID-token verification are checked before persistence. The new revocation call checks configured-host egress eligibility and skips unreadable old secrets, preserving existing recovery behavior.

Resilience and Maintainability Implications

  • inferred — Issuer cleanup is not atomic with local replacement. Failure or process interruption after commit can leave the old grant open without a durable retry recorded by this flow. This exposure predates the PR: the base implementation never attempted cleanup on repeat sign-in. The PR reduces that exposure but does not guarantee grant termination.

Hardening Proposals

  • proposed — Validate with a real issuer account that revoking the replaced refresh token leaves the returning-user replacement grant usable. This would resolve the documented external-contract uncertainty rather than relying solely on fake-service token independence.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 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 refresh-token grant replaced by a new ChatGPT sign-in.
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.
Full details: Docstring Coverage

Explanation

Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ 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

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

…-revoke-on-sign-in-again

# Conflicts:
#	docs/architecture/chatgpt-subscription.md

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant