Fix WHOOP sync Cognito refresh and revoked token handling - #1355
Conversation
…ation. Sync was refreshing on every step even when the access token was still valid, and Cognito NotAuthorizedException was surfaced as a misleading password error instead of prompting reconnect. Co-authored-by: Cursor <cursoragent@cursor.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughWHOOP token resolution is extracted from ChangesWHOOP Custom Auth Extraction
Sequence DiagramsequenceDiagram
participant SyncJob as process-sync-job
participant ProviderModel
participant resolveWhoopTokens
participant TokenDB as SyncDatabase
participant WhoopClient
SyncJob->>ProviderModel: providerRequiresStoredTokens(provider)
ProviderModel-->>SyncJob: true (whoop in CUSTOM_AUTH_SYNC_PROVIDER_IDS)
SyncJob->>resolveWhoopTokens: { db, fetchFn, userId }
resolveWhoopTokens->>TokenDB: loadTokens("whoop")
alt token valid + userId parseable from scopes
resolveWhoopTokens-->>SyncJob: WhoopAuthToken (no network)
else token expired
resolveWhoopTokens->>WhoopClient: refreshAccessToken(refreshToken)
alt NotAuthorizedException
resolveWhoopTokens->>TokenDB: deleteTokens("whoop")
resolveWhoopTokens-->>SyncJob: throws RefreshTokenRevokedError
else success
resolveWhoopTokens->>TokenDB: saveTokens("whoop", newTokenSet)
resolveWhoopTokens-->>SyncJob: WhoopAuthToken
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
|
Storybook previews for This comment updates automatically on each PR push. |
Centralize token save/resolve in resolveWhoopTokens, drop dead OAuth authSetup, and treat WHOOP as custom-auth so sync-all and the worker require stored tokens correctly. Co-authored-by: Cursor <cursoragent@cursor.com>
provider-auth-policy importing from src/providers/custom-auth-providers.ts violated no-provider-cross-imports in dependency-cruiser. Co-authored-by: Cursor <cursoragent@cursor.com>
Stryker's copyfile fails on git-tracked symlinks pointing at directories; exclude them via ignorePatterns so mutation test shards can run. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server/src/routers/whoop-auth.ts`:
- Around line 143-148: The saveWhoopAuthTokens function is persisting tokens
without validating that they are non-empty, which creates an inconsistency with
resolveWhoopTokens that treats empty refresh tokens as disconnected. Add
validation to the tRPC input schema before the saveWhoopAuthTokens call to
reject requests where accessToken or refreshToken are empty strings. This
ensures that empty tokens cannot be persisted to the database, maintaining
consistency with the coding guidelines that prohibit empty strings as absent
values.
In `@src/providers/whoop/resolve-tokens.ts`:
- Around line 1-3: The WHOOP token payload obtained from the refresh call at
line 72 is persisted directly to storage in lines 78-84 without runtime
validation. Create a Zod schema that validates the structure and types of the
token response payload (ensuring it matches the expected shape with properties
like access_token, refresh_token, expires_in, etc.), then parse the refreshed
token data through this schema before persisting it to ensure malformed API
responses cannot corrupt stored credentials. Handle schema validation errors by
throwing an appropriate error that prevents the invalid token from being saved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bbc5ab92-b708-40b3-af49-102e32016f22
📒 Files selected for processing (19)
package.jsonpackages/server/src/routers/sync-helpers.tspackages/server/src/routers/sync.test.tspackages/server/src/routers/sync.tspackages/server/src/routers/whoop-auth.tssrc/auth/resolve-tokens.tssrc/jobs/process-sync-job.tssrc/lib/custom-auth-providers.test.tssrc/lib/custom-auth-providers.tssrc/providers/provider-auth-policy.test.tssrc/providers/provider-auth-policy.tssrc/providers/whoop-sync.integration.test.tssrc/providers/whoop.test.tssrc/providers/whoop/provider.tssrc/providers/whoop/resolve-tokens.test.tssrc/providers/whoop/resolve-tokens.tssrc/providers/whoop/sync-orchestrator.tsstryker.ci.config.jsonstryker.config.json
💤 Files with no reviewable changes (1)
- src/providers/provider-auth-policy.test.ts
Organize imports, apply formatting, and replace banned `as SyncDatabase` casts with a typed mock helper in resolve-tokens tests. Co-authored-by: Cursor <cursoragent@cursor.com>
Reject empty access/refresh tokens in whoopAuth.saveTokens input and Zod-parse Cognito refresh payloads before persisting refreshed credentials. Co-authored-by: Cursor <cursoragent@cursor.com>
The suite was passing all 87 files but hitting the 10-minute cap during coverage; also seed WHOOP integration tokens with a stored userId to avoid unnecessary Cognito refreshes. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
NotAuthorizedExceptionduring refresh as a revoked refresh token: delete stored tokens and surfaceRefreshTokenRevokedErrorso the UI prompts reconnect.Test plan
pnpm vitest run src/providers/whoop.test.tspnpm vitest run src/providers/whoop-sync.integration.test.tsMade with Cursor
Summary by cubic
Reuse valid WHOOP Cognito access tokens and centralize token handling so sync stops refreshing on every step. Refresh failures now trigger a clean reconnect, WHOOP uses a custom-auth flow (no developer OAuth), and tokens are validated at the boundaries.
Bug Fixes
Refactors
resolveWhoopTokensandsaveWhoopAuthTokens; extracted WHOOP request logger.src/lib/custom-auth-providers, usedproviderRequiresStoredTokensin the worker, passedCUSTOM_AUTH_PROVIDERStoProviderModel, and allowed custom-auth inprovider-auth-policy; removed the developer OAuth path and related env checks.**/skills/stripe-projectssymlinks in Stryker and extend integration test timeout to 12 minutes.Written for commit f0a1cb0. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests