refactor: extract encryptTokenPair utility#584
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new exported helper ChangesToken Pair Encryption Consolidation
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as Router\n(handleCreateSession / handleSessionWsToken)
participant Crypto as Crypto\n(encryptTokenPair)
participant Metrics as Metrics
Client->>Router: POST /create-session or WS token request (scmToken, scmRefreshToken)
Router->>Metrics: start timer (optional)
Router->>Crypto: encryptTokenPair(scmToken, scmRefreshToken, TOKEN_ENCRYPTION_KEY)
Crypto-->>Router: { accessTokenEncrypted, refreshTokenEncrypted } or throws
Router->>Metrics: stop timer (optional)
Router-->>Client: 200 with encrypted tokens OR 500 on encryption error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Review rate limit: 6/8 reviews remaining, refill in 7 minutes and 54 seconds.Comment |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![open-inspect[bot]](https://avatars.githubusercontent.com/u/2492022?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR extracts a shared encryptTokenPair helper and rewires the two router call sites to use it. The helper itself is straightforward and the new unit tests are useful, but the handleCreateSession refactor changes existing error-handling semantics in a way that looks like a behavioral regression.
- PR Title and number:
refactor: extract encryptTokenPair utility(#584) - Author:
@ColeMurray - Files changed count: 3
- Additions/deletions:
+96 / -38
Critical Issues
- [Functionality]
packages/control-plane/src/router.ts:930-handleCreateSessionused to degrade gracefully when refresh-token encryption failed: it logged a warning and still created the session without refresh support. After this refactor, any failure insideencryptTokenPair()causes a 500 response and aborts session creation. Because the PR is positioned as a refactor/extraction, this looks like an unintended behavior change. Preserving the prior split policy here would avoid rejecting otherwise valid session creation requests.
Suggestions
- [Testing]
packages/control-plane/src/router.ts:924- Once the create-session behavior is restored, a targeted test around the router path would help pin the intended asymmetry: access-token encryption failure is fatal, refresh-token encryption failure is non-fatal.
Nitpicks
- Nit:
packages/control-plane/src/auth/crypto.ts:113- The doc comment says the helper returnsnullfor tokens that were not provided(undefined/empty), but the signature only acceptsstring | undefined. If empty strings are intentionally part of the contract, a small test would make that explicit; otherwise the comment could just sayundefined/falsy inputs.
Positive Feedback
- The extracted helper removes duplicated encryption flow and keeps the return shape explicit at the call sites.
- The new
encryptTokenPairunit tests cover the main success cases plus an invalid-key failure path. handleSessionWsTokenremains easy to read after the refactor, especially with the early return when no encryption key is configured.
Questions
None.
Verdict
Request Changes: the handleCreateSession refactor currently changes refresh-token encryption failures from a warning-only degradation into a hard 500.
|
Good catch on the behavior change. We reviewed this and are comfortable with it — Keeping both fatal simplifies the contract: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/control-plane/src/router.ts (1)
927-955:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve caller-provided SCM tokens during enrichment.
encryptTokenPaircan now return one encrypted value and onenull. The existingif (!scmTokenEncrypted)fallback then overwrites the whole pair, so a refresh-only or access-only request can lose the token the caller actually supplied. Backfill each field independently instead of gating on the access-token result.🔧 Suggested fix
- if (!scmTokenEncrypted) { - scmTokenEncrypted = enrichment.accessTokenEncrypted ?? null; - scmRefreshTokenEncrypted = enrichment.refreshTokenEncrypted ?? null; - scmTokenExpiresAt = enrichment.tokenExpiresAt; - } + scmTokenEncrypted ??= enrichment.accessTokenEncrypted ?? null; + scmRefreshTokenEncrypted ??= enrichment.refreshTokenEncrypted ?? null; + scmTokenExpiresAt ??= enrichment.tokenExpiresAt;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/control-plane/src/router.ts` around lines 927 - 955, The enrichment block currently replaces both token fields only when scmTokenEncrypted is falsy, which overwrites a caller-supplied counterpart if encryptTokenPair returned one encrypted value and one null; change the logic in the resolveGitHubEnrichment handling so you backfill each token independently (e.g., set scmTokenEncrypted only if it's null/undefined using enrichment.accessTokenEncrypted, set scmRefreshTokenEncrypted only if it's null/undefined using enrichment.refreshTokenEncrypted, and similarly backfill scmTokenExpiresAt from enrichment.tokenExpiresAt) instead of wrapping both assignments in the single if (!scmTokenEncrypted) branch; keep resolveGitHubEnrichment, scmTokenEncrypted, scmRefreshTokenEncrypted, and scmTokenExpiresAt as the referenced symbols to locate and update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/control-plane/src/router.ts`:
- Around line 1723-1740: The current catch in the encrypt block swallows
encryption failures and returns {scmTokenEncrypted: null,
scmRefreshTokenEncrypted: null}, which can overwrite valid tokens in the Session
DO; instead, change the error handling in the encryptTokenPair call so that if
env.TOKEN_ENCRYPTION_KEY is present and encryptTokenPair throws, the error is
rethrown (or propagate the original error) rather than returning nulls. Locate
the block that calls encryptTokenPair and replace the catch-return-null behavior
(and its logger.error usage) with logic that logs the error and then throws (or
rethrows) the error so the request fails upstream; keep the early return of
nulls only for the case when env.TOKEN_ENCRYPTION_KEY is falsy.
---
Outside diff comments:
In `@packages/control-plane/src/router.ts`:
- Around line 927-955: The enrichment block currently replaces both token fields
only when scmTokenEncrypted is falsy, which overwrites a caller-supplied
counterpart if encryptTokenPair returned one encrypted value and one null;
change the logic in the resolveGitHubEnrichment handling so you backfill each
token independently (e.g., set scmTokenEncrypted only if it's null/undefined
using enrichment.accessTokenEncrypted, set scmRefreshTokenEncrypted only if it's
null/undefined using enrichment.refreshTokenEncrypted, and similarly backfill
scmTokenExpiresAt from enrichment.tokenExpiresAt) instead of wrapping both
assignments in the single if (!scmTokenEncrypted) branch; keep
resolveGitHubEnrichment, scmTokenEncrypted, scmRefreshTokenEncrypted, and
scmTokenExpiresAt as the referenced symbols to locate and update.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: af1b471a-0081-4a2d-aea3-30168470705b
📒 Files selected for processing (3)
packages/control-plane/src/auth/crypto.test.tspackages/control-plane/src/auth/crypto.tspackages/control-plane/src/router.ts
Make handleSessionWsToken consistent with handleCreateSession: if TOKEN_ENCRYPTION_KEY is present and encryptTokenPair throws, return 500 instead of silently continuing with null tokens.
|
Agreed — updated in c127b10. The "overwrite valid tokens" concern doesn't actually apply here (the DO's ws-token handler checks |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
1 participant
Summary
encryptTokenPair(accessToken, refreshToken, encryptionKey)utility inauth/crypto.tsthat encrypts an access/refresh token pair, returningnullfor unprovided tokens and throwing on encryption failurerouter.ts(handleCreateSessionandhandleSessionWsToken) with calls to the shared utilityhandleCreateSessioncatches and returns 500,handleSessionWsTokencatches and continues with null tokensTest plan
encryptTokenPaircovering: both undefined, access-only, refresh-only, both provided (with round-trip decryption verification), and invalid key (throws)Summary by CodeRabbit
Bug Fixes
Tests