chore: use plain app id in key gen #960
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
WalkthroughThe changes primarily involve the simplification of the file key generation process. The previous implementation utilized the Sqids library for encoding and randomization, while the new approach directly concatenates the appId with a user-defined file seed. The updated requirements for the file seed ensure it is unique, URL-safe, and at least 36 characters long, streamlining the overall key generation without the need for additional complexity. Changes
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
More templates
commit: |
📦 Bundle size comparison
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- docs/src/app/(docs)/uploading-files/page.mdx (1 hunks)
- packages/shared/src/crypto.ts (1 hunks)
Additional comments not posted (2)
docs/src/app/(docs)/uploading-files/page.mdx (2)
99-103: LGTM!The requirements for generating a file key are clear and reasonable. Using base64 encoding to ensure the file seed is URL-safe is a good approach.
106-111: LGTM!The
generateKeyfunction correctly implements the requirements for generating a file key. The use of base64 encoding ensures thefileSeedis URL-safe, and the concatenation withappIdgenerates a unique file key.
|
|
||
| // Concatenate them | ||
| return encodedAppId + encodedFileSeed; | ||
| return appId + encodedFileSeed; |
There was a problem hiding this comment.
Tip
Codebase Verification
Update key format validation and related tests
The change in the generateKey function, which now directly concatenates appId and encodedFileSeed without encoding, could have significant implications:
- The key format validation in
packages/shared/src/crypto.ts(key.startsWith(expectedPrefix)) may need to be adjusted to accommodate the new key format. - Tests in
packages/shared/test/crypto.test.tsshould be reviewed and updated to ensure they correctly validate the new key format. - The following operations that use the generated key should be thoroughly tested with the new format:
- URL generation (e.g., in
packages/uploadthing/src/internal/handler.ts) - API calls such as
updateACL,deleteFiles, andgetSignedURL
- URL generation (e.g., in
To ensure system stability and compatibility:
- Review and update the key format validation logic.
- Modify existing tests to account for the new key format.
- Add new tests to verify the compatibility of the new key format with all dependent systems.
- Perform integration testing to ensure that URL generation and API calls work correctly with the new key format.
Analysis chain
Verify the impact of the change in key generation format.
The change in the key generation format, by removing the encoding of appId, could lead to compatibility issues with existing systems or databases that rely on the previous format. Please ensure that this change is thoroughly tested and validated against all dependent systems.
Run the following script to verify the usage of the generateKey function and its return value across the codebase:
Additionally, please ensure that the new key generation format maintains the required level of uniqueness and security. Consider adding tests to validate the uniqueness and security properties of the generated keys.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the `generateKey` function and its return value.
# Test 1: Search for the function usage. Expect: Occurrences of the function call with the expected arguments.
ast-grep --lang typescript --pattern $'generateKey($_, $_, $_)'
# Test 2: Search for the usage of the return value. Expect: Occurrences of the key being used in the expected format.
rg --type typescript -A 5 $'generateKey'
Length of output: 1760
Script:
#!/bin/bash
# Description: Verify the usage of the `generateKey` function and its return value.
# Test 1: Search for the function usage and its return value usage
rg --type ts -A 10 'const key = .*generateKey'
# Test 2: Search for any key format validations or comparisons
rg --type ts 'key\.match|key\.startsWith|key\.endsWith|key\.includes'
# Test 3: Look for any database operations or API calls using the key
rg --type ts 'db\..*\(.*key|api\..*\(.*key'
Length of output: 6754
We decided it was too hard to port the encoding to other languages
so we now do a prefix check on appId in plain text instead of the
encoded value.
ref: https://github.com/pingdotgg/uploadthing-infra/pull/550
Summary by CodeRabbit
New Features
Bug Fixes
verifyKeyfunction, streamlining key management and enhancing security.