Skip to content

Making OAuth token refresh more robust #767

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jul 16, 2025
Merged

Making OAuth token refresh more robust #767

merged 8 commits into from
Jul 16, 2025

Conversation

fomalhautb
Copy link
Contributor

@fomalhautb fomalhautb commented Jul 14, 2025

…oken validity handling in CRUD operations


Important

Enhances OAuth token handling by adding validity checks, marking invalid tokens, and attempting token refreshes in crud.tsx.

  • Behavior:
    • Adds isValid column to OAuthAccessToken and OAuthToken in migration.sql and schema.prisma.
    • In crud.tsx, filters tokens by isValid status and marks them invalid if server-side checks fail.
    • Attempts to refresh tokens if no valid access token is found, logging errors and marking refresh tokens invalid if refresh fails.
  • Error Handling:
    • Uses captureError to log token refresh errors in crud.tsx.
  • Misc:
    • Imports TokenSet and captureError in crud.tsx.

This description was created by Ellipsis for a45b64f. You can customize this summary. It will automatically update as commits are pushed.

Copy link

vercel bot commented Jul 14, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
stack-backend 🛑 Canceled (Inspect) Jul 16, 2025 9:21pm
stack-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 16, 2025 9:21pm
stack-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 16, 2025 9:21pm
stack-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 16, 2025 9:21pm

Copy link

recurseml bot commented Jul 14, 2025

✨ No issues found! Your code is sparkling clean! ✨

🗒️ View all ignored comments in this repo
  • The constraint 'TokenStoreType extends string' is too restrictive. It should likely be 'TokenStoreType extends string | object' to match the condition check in line 113 where TokenStoreType is checked against {}
  • Return type mismatch - the interface declares useUsers() returning ServerUser[] but the Team interface that this extends declares useUsers() returning TeamUser[]
  • There is a syntax error in the super constructor call due to the ellipsis operator used incorrectly. Objects aren't being merged correctly. This syntax usage can lead to runtime errors when trying to pass the merged object to 'super()'. Verify that the intended alterations to the object occur before or outside of the super() call if needed.
  • Throwing an error when no active span is found is too aggressive. The log function should gracefully fallback to console.log or another logging mechanism when there's no active span, since not all execution contexts will have an active span. This makes the code less resilient and could break functionality in non-traced environments.

📚 Relevant Docs

  • Function sets backendContext with a new configuration but doesn't pass 'defaultProjectKeys'. Since defaultProjectKeys is required in the type definition and cannot be updated (throws error if tried to set), this will cause a type error.
  • The schema is using array syntax for pick() which is incorrect for Yup schemas. The pick() method in Yup expects individual arguments, not an array. Should be changed to: emailConfigSchema.pick('type', 'host', 'port', 'username', 'sender_name', 'sender_email')

📚 Relevant Docs

  • Creating a refresh token with current timestamp as expiration means it expires immediately. Should set a future date for token expiration.
  • The 'tools' object is initialized as an empty object, even though 'tools' is presumably expected to contain tool definitions. This could cause the server capabilities to lack necessary tool configurations, thus potentially impacting functionalities that depend on certain tool setups.

📚 Relevant Docs

  • 'STACK_SECRET_SERVER_KEY' is potentially being included in every request header without checking its existence again here. Although it's checked during initialization, this could lead to security issues as it's exposed in all communications where the header is logged or captured.

📚 Relevant Docs

  • When adding 'use client' directive at the beginning, it doesn't check if file.text already contains the 'use client' directive. This could lead to duplicate 'use client' directives if the file already has one.

📚 Relevant Docs

Need help? Join our Discord for support!
https://discord.gg/qEjHQk64Z9

…e on creation and mark tokens as invalid when refreshed
@fomalhautb fomalhautb requested a review from N2D4 July 14, 2025 21:41
@fomalhautb fomalhautb assigned N2D4 and unassigned fomalhautb Jul 14, 2025
@fomalhautb fomalhautb marked this pull request as ready for review July 14, 2025 21:41
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR enhances OAuth token handling by introducing explicit token validity tracking through a new isValid boolean field in both OAuthToken and OAuthAccessToken models. Key changes include:

  1. Added isValid column (defaulting to true) to track token validity states without deletion
  2. Improved token refresh logic that iterates through available refresh tokens
  3. Enhanced error handling during token refresh process

The changes make the token refresh system more resilient by maintaining token history and handling invalidation scenarios (particularly with providers like GitHub that can invalidate tokens server-side) more gracefully.

Confidence score: 4.5/5

  1. This PR is very safe to merge as it adds non-breaking changes with good defaults
  2. The score reflects the well-structured changes, clear migration path, and improved error handling, with a small deduction for potential edge cases in token refresh logic
  3. Focus attention on crud.tsx to ensure the token refresh retry logic doesn't introduce any unintended side effects

3 files reviewed, 3 comments
Edit PR Review Bot Settings | Greptile

Copy link
Contributor

@N2D4 N2D4 left a comment

Choose a reason for hiding this comment

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

LGTM! good work

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, is it possible to E2E test this, or would that be too hard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be a bit annoying to do, I will merge this so it get deployed ASAP and create another PR for the tests

@N2D4 N2D4 assigned fomalhautb and unassigned N2D4 Jul 16, 2025
N2D4 and others added 3 commits July 16, 2025 04:33
…provider_id]/access-token/crud.tsx

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@fomalhautb fomalhautb merged commit f32921f into dev Jul 16, 2025
13 of 17 checks passed
@fomalhautb fomalhautb deleted the token-refresh-fix branch July 16, 2025 21:00
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.

2 participants