improvement(vault): new error throw and improvement #2734
Closed
+222
−23
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR improves error handling for OAuth token refresh failures and adds date filtering capabilities to Google Vault tools.
Major Changes:
- Enhanced error messages for Google Workspace session control (
invalid_rapt) and revoked credentials (invalid_grant) - Added date filtering (
startTime,endTime) and search terms to Google Vault export and hold operations - Propagates detailed error messages from OAuth refresh failures to end users
Critical Issues Found:
- Breaking Change:
getOAuthTokenandrefreshAccessTokenIfNeededinutils.tsnow throw errors instead of returningnull, but 30+ existing callers expectnulland useif (!accessToken)checks. This will cause unhandled exceptions in webhook services, cron jobs, and API routes. - Missing
nullchecks inoauth.tswhen usingtypeof errorData === 'object'(which is true fornullin JavaScript)
Google Vault Improvements:
- Successfully adds date/time filtering for exports and holds
- Properly handles corpus-specific query construction (MAIL/GROUPS for date filtering, DRIVE for shared drives)
- Removed unused
timeZoneparameter - Minor UX issue: Date filtering fields show for all corpus types even though holds only support them for MAIL and GROUPS
Confidence Score: 1/5
- This PR contains a critical breaking change that will cause runtime errors across the application
- The change from returning null to throwing errors in
getOAuthTokenandrefreshAccessTokenIfNeededbreaks backward compatibility with 30+ callers that expect null checks. This will cause unhandled promise rejections in production, affecting webhooks, cron jobs, and API endpoints. The improved error messages are good, but the implementation breaks existing code. apps/sim/app/api/auth/oauth/utils.tsrequires immediate attention - the error handling changes will break multiple services.apps/sim/lib/oauth/oauth.tsneeds null safety fixes for typeof checks.
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| apps/sim/app/api/auth/oauth/token/route.ts | 3/5 | Improved error message propagation from token refresh failures, now surfaces specific error details to users |
| apps/sim/app/api/auth/oauth/utils.ts | 1/5 | CRITICAL: Changed error handling from returning null to throwing errors, breaking backward compatibility with 30+ callers expecting null checks |
| apps/sim/lib/oauth/oauth.ts | 2/5 | Added specific error messages for Google Workspace session control and invalid_grant errors; null check missing in typeof checks |
| apps/sim/blocks/blocks/google_vault.ts | 4/5 | Added date filtering and search terms for exports/holds with AI-assisted input generation; minor issue with corpus-specific field visibility |
Sequence Diagram
sequenceDiagram
participant Client
participant API as OAuth API Route
participant Utils as OAuth Utils
participant Lib as OAuth Library
participant Provider as Google OAuth
Client->>API: Request access credentials
API->>Utils: getOAuthToken or refreshAccessTokenIfNeeded
Utils->>Lib: refreshOAuthToken with provider details
Lib->>Provider: POST refresh request
alt Session Control Error (RAPT)
Provider-->>Lib: Error response with invalid_rapt subtype
Lib-->>Utils: throw Error with reconnect instructions
Utils-->>API: propagate error
API-->>Client: 401 or 403 with user-friendly message
else General Invalid Grant
Provider-->>Lib: Error response with invalid_grant
Lib-->>Utils: throw Error requesting reconnection
Utils-->>API: propagate error
API-->>Client: 401 or 403 with reconnect message
else Success
Provider-->>Lib: New credentials with expiry
Lib-->>Utils: Return refreshed credentials
Utils->>Utils: Update database
Utils-->>API: Return valid credentials
API-->>Client: Success response
end
Note over Utils: BREAKING CHANGE in utils.ts<br/>Changed from returning null to throwing errors
Note over API: Most callers expect null checks<br/>Will crash without try-catch blocks
Comment on lines
257
to
267
| } catch (error) { | ||
| // Re-throw the error to propagate detailed error messages (e.g., session expiry instructions) | ||
| logger.error(`[${requestId}] Error refreshing token for credential`, { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| stack: error instanceof Error ? error.stack : undefined, | ||
| providerId: credential.providerId, | ||
| credentialId, | ||
| userId: credential.userId, | ||
| }) | ||
| return null | ||
| throw error | ||
| } |
Contributor
There was a problem hiding this comment.
Changing return null to throw error breaks backward compatibility. Many callers expect refreshAccessTokenIfNeeded to return null on failure and check if (!accessToken). This change will cause unhandled exceptions in:
apps/sim/lib/webhooks/utils.server.ts:2129,2816,2902apps/sim/lib/webhooks/provider-subscriptions.ts:57,190apps/sim/app/api/tools/gmail/labels/route.ts:70apps/sim/app/api/tools/sharepoint/sites/route.ts:53- Plus 20+ more files
Either revert to returning null, or wrap all callers in try-catch blocks.
Comment on lines
+1197
to
+1200
| if ( | ||
| typeof errorData === 'object' && | ||
| (errorData.error_subtype === 'invalid_rapt' || | ||
| errorData.error_description?.includes('reauth related error')) |
Contributor
There was a problem hiding this comment.
typeof errorData === 'object' is true for null in JavaScript. Should add && errorData !== null to prevent null pointer exceptions when accessing errorData.error_subtype
Suggested change
| if ( | |
| typeof errorData === 'object' && | |
| (errorData.error_subtype === 'invalid_rapt' || | |
| errorData.error_description?.includes('reauth related error')) | |
| if ( | |
| typeof errorData === 'object' && | |
| errorData !== null && | |
| (errorData.error_subtype === 'invalid_rapt' || | |
| errorData.error_description?.includes('reauth related error')) |
Comment on lines
+1207
to
+1210
| if ( | ||
| typeof errorData === 'object' && | ||
| errorData.error === 'invalid_grant' && | ||
| !errorData.error_subtype |
Contributor
There was a problem hiding this comment.
typeof errorData === 'object' is true for null in JavaScript. Should add && errorData !== null
Suggested change
| if ( | |
| typeof errorData === 'object' && | |
| errorData.error === 'invalid_grant' && | |
| !errorData.error_subtype | |
| if ( | |
| typeof errorData === 'object' && | |
| errorData !== null && | |
| errorData.error === 'invalid_grant' && | |
| !errorData.error_subtype |
Comment on lines
+162
to
+169
| // Date filtering for exports and holds (holds only support MAIL and GROUPS corpus) | ||
| { | ||
| id: 'startTime', | ||
| title: 'Start Time', | ||
| type: 'short-input', | ||
| placeholder: 'YYYY-MM-DDTHH:mm:ssZ', | ||
| condition: { field: 'operation', value: ['create_matters_export', 'create_matters_holds'] }, | ||
| wandConfig: { |
Contributor
There was a problem hiding this comment.
Date filtering fields show for all corpus types, but the comment states "holds only support MAIL and GROUPS corpus" for date filtering. Consider adding corpus-based conditions:
Suggested change
| // Date filtering for exports and holds (holds only support MAIL and GROUPS corpus) | |
| { | |
| id: 'startTime', | |
| title: 'Start Time', | |
| type: 'short-input', | |
| placeholder: 'YYYY-MM-DDTHH:mm:ssZ', | |
| condition: { field: 'operation', value: ['create_matters_export', 'create_matters_holds'] }, | |
| wandConfig: { | |
| { | |
| id: 'startTime', | |
| title: 'Start Time', | |
| type: 'short-input', | |
| placeholder: 'YYYY-MM-DDTHH:mm:ssZ', | |
| condition: { | |
| field: 'operation', | |
| value: ['create_matters_export', 'create_matters_holds'] | |
| }, |
Or if holds truly need corpus restriction, the condition should be more complex to check corpus for holds operations.
Contributor
Additional Comments (1)
Either revert to returning |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Added better error handling for vault specifically. Google Cloud by default has a limit for token expiration, and they throw invalid_grant after the token expires, and the only workaround to this is disconneting and reconnecting credential, or, as an admin, disabling reauth for Sim.
Also added additional params for dates
Type of Change
Testing
Tested manually. Don't merge yet (will remove this once the token has manually expired after an hour)
Checklist