refactor: extract download token helpers#542
Open
zereight wants to merge 3 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🔇 Additional comments (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughInline AES-256-GCM download token logic is extracted from ChangesDownload Token Utility Extraction
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Missing test:mock registration
- Added test/utils/download-token.test.ts to the test:mock script in package.json alongside other utils tests.
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 8397aae. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@test/utils/download-token.test.ts`:
- Around line 28-31: Add a new test case in the test suite to verify that the
decryptDownloadToken function correctly rejects expired tokens by returning
null. Create a token with a short or manipulated TTL, advance time past the
token's expiry using a time mocking library (such as sinon or jest fake timers),
then call decryptDownloadToken and assert it returns null. Consider that the TTL
constant is set at module load time, so you may need to either use time mocking
to simulate expiry without waiting, or refactor the download-token.ts module to
make the TTL configurable for testing purposes.
In `@utils/download-token.ts`:
- Line 32: The DOWNLOAD_TOKEN_TTL constant assignment needs validation to ensure
the parsed value is a positive number. After parsing the environment variable
with Number.parseInt, add a check to verify the result is not NaN and is greater
than 0. If the parsed value fails validation, provide a safe default value (such
as 300) to prevent the TTL from becoming NaN and causing tokens to never expire.
This validation should occur before assigning the final value to the
DOWNLOAD_TOKEN_TTL constant.
- Around line 54-77: The decryptDownloadToken function returns a
DecryptedDownloadToken object without validating that the required fields header
and token exist in the decrypted payload. Add validation after parsing the JSON
payload to check that both payload.h and payload.t are present and have valid
values, returning null immediately if either required field is missing or
undefined. This ensures the returned object always conforms to the
DecryptedDownloadToken interface contract and prevents type-safety issues
downstream.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f2bf1d47-0d99-4589-acf1-ff69c2a5be5d
📒 Files selected for processing (3)
index.tstest/utils/download-token.test.tsutils/download-token.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Cursor Bugbot Autofix
- GitHub Check: test
- GitHub Check: coverage
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
utils/download-token.ts (3)
1-14: LGTM!
23-29: LGTM!
34-52: LGTM!test/utils/download-token.test.ts (1)
1-26: LGTM!index.ts (2)
129-129: LGTM!
504-504: LGTM!
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Part of #516.
This keeps the first refactor slice small and behavior-preserving:
utils/download-token.tsbuildDownloadUrl()andregisterDownloadProxy()inindex.tsVerification
rtk npm run buildnode --import tsx/esm --test test/utils/download-token.test.tsnode --import tsx/esm --test test/test-remote-downloads.tsNote
Low Risk
Mechanical refactor of token crypto with small decrypt hardening; download proxy behavior stays the same aside from stricter invalid-payload rejection.
Overview
Moves AES-256-GCM download token creation and decryption out of
index.tsintoutils/download-token.ts, withindex.tsimporting the helpers forbuildDownloadUrl()and the download proxy.The new module keeps the same env-driven key (
DOWNLOAD_TOKEN_SECRET) and TTL behavior, and tightens decrypt validation (rejects emptyh/tfields; parsesDOWNLOAD_TOKEN_TTLonly when it is a positive finite number). Addstest/utils/download-token.test.tsfor round-trip, invalid, and expired tokens, and registers that file in thetest:mockscript.Reviewed by Cursor Bugbot for commit b3dbf5a. Bugbot is set up for automated code reviews on this repo. Configure here.