Static Frames Validation Security Fix #736
Conversation
🦋 Changeset detectedLatest commit: 1d2dab8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded@rygine has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
Deploying xmtp-js-docs with
|
| Latest commit: |
1d2dab8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1ee6ca8e.xmtp-js.pages.dev |
| Branch Preview URL: | https://coda-frames-check.xmtp-js.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
sdks/node-sdk/src/Client.ts (3)
175-180: Add JSDoc documentation and input validationThe method implementation looks correct, but could benefit from:
- JSDoc documentation explaining the purpose, parameters, and return value
- Input validation for the Ethereum address format
+ /** + * Checks if an address is authorized for a given inbox + * @param inboxId - The ID of the inbox to check + * @param address - The Ethereum address to verify + * @returns Promise resolving to true if the address is authorized + */ async isAddressAuthorized( inboxId: string, address: string, ): Promise<boolean> { + if (!address.match(/^0x[0-9a-fA-F]{40}$/)) { + throw new Error('Invalid Ethereum address format'); + } return this.#innerClient.isAddressAuthorized(inboxId, address); }
182-187: Add JSDoc documentation and input validationThe method implementation looks correct, but could benefit from:
- JSDoc documentation explaining the purpose, parameters, and return value
- Basic validation for the installationId parameter
+ /** + * Checks if an installation is authorized for a given inbox + * @param inboxId - The ID of the inbox to check + * @param installationId - The installation ID as a byte array + * @returns Promise resolving to true if the installation is authorized + */ async isInstallationAuthorized( inboxId: string, installationId: Uint8Array, ): Promise<boolean> { + if (!installationId?.length) { + throw new Error('Installation ID cannot be empty'); + } return this.#innerClient.isInstallationAuthorized(inboxId, installationId); }
175-187: Consider documenting the authorization flowThese methods are part of a larger authorization system and are used by the frames-validator. Consider adding documentation that explains:
- The overall authorization flow
- How these methods fit into the larger picture
- Common usage patterns and examples
This would help developers understand when and how to use these methods effectively.
Consider adding this documentation in:
- The class-level JSDoc
- A separate authorization flow document
- Code examples in the README
packages/frames-validator/src/validation.ts (3)
31-31: Include missing variable in destructuring assignmentYou're destructuring
deserializeProtoMessage(messageBytes)into several variables. Ensure that all necessary variables returned bydeserializeProtoMessageare included in the assignment. IfmessageTypeor other relevant variables are returned by the function but not destructured here, consider adding them for completeness.
59-61: Consistent error messages for unauthorized accessThe error message "Unable to associate wallet address with inbox" might not clearly convey that the wallet address is not authorized. Consider rephrasing it to make the issue more explicit.
Suggested change:
-throw new Error("Unable to associate wallet address with inbox"); +throw new Error("Wallet address is not authorized to access the inbox");
59-61: Handle potential errors fromisAddressAuthorizedmethodSimilar to the previous authorization check, wrap the call to
Client.isAddressAuthorizedin a try-catch block to handle any unexpected errors.Apply this change:
+try { const isMember = await Client.isAddressAuthorized(inboxId, walletAddress); if (!isMember) { throw new Error("Wallet address is not authorized to access the inbox"); } +} catch (error) { + throw new Error(`Address authorization check failed: ${error.message}`); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
packages/frames-validator/package.json(1 hunks)packages/frames-validator/src/validation.ts(2 hunks)sdks/node-sdk/package.json(2 hunks)sdks/node-sdk/src/Client.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- sdks/node-sdk/package.json
🔇 Additional comments (3)
packages/frames-validator/package.json (1)
43-43: Verify the impact of node-sdk upgrade
The version bump of @xmtp/node-sdk to ^0.0.28 aligns with the PR's authorization changes. However, since this is a pre-1.0 version, we should be extra cautious about potential breaking changes.
Let's verify the changelog and any potential breaking changes:
Consider adding a test plan in the PR description to verify that the new authorization functionality works as expected with this version of the SDK.
✅ Verification successful
Version bump is safe to proceed
The code analysis shows that @xmtp/node-sdk is primarily used for authorization-related functionality in the validation process. The upgrade aligns with the PR's focus on authorization changes, and the codebase has comprehensive tests covering both V2 and V3 authentication flows. The main usage is through Client.isInstallationAuthorized and Client.isAddressAuthorized methods, which are well-tested in the validation test suite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the changelog and git history for breaking changes
# Check the latest release notes
gh release view v0.0.28 --repo xmtp/xmtp-js
# Look for any breaking change indicators in recent commits
git log --grep="BREAKING CHANGE" --grep="breaking change" -i HEAD~50..HEAD
Length of output: 63
Script:
#!/bin/bash
# Let's verify the actual usage of @xmtp/node-sdk in the codebase
# to understand the impact of the upgrade
# Find files that import or require @xmtp/node-sdk
rg "@xmtp/node-sdk" -l
# Look for specific usage patterns
ast-grep --pattern 'import $_ from "@xmtp/node-sdk"'
# Check package-lock.json for any dependency conflicts
fd package-lock.json --exec cat {} | jq '.dependencies."@xmtp/node-sdk"' 2>/dev/null
# Check if there are any tests that might be affected
rg "@xmtp/node-sdk" --type test
Length of output: 891
Script:
#!/bin/bash
# Let's check the specific usage in validation.ts and its tests
# to understand the impact on authorization-related code
# Check the validation.ts file where the SDK is used
cat packages/frames-validator/src/validation.ts
# Check the corresponding test file
cat packages/frames-validator/src/validation.test.ts
# Check if there are any recent changes to these files
git diff HEAD~10 HEAD -- packages/frames-validator/src/validation.ts packages/frames-validator/src/validation.test.ts
Length of output: 11273
sdks/node-sdk/src/Client.ts (1)
175-187: Verify error handling and usage patterns
The new authorization methods look well-integrated, but we should verify:
- Error handling propagation from the inner client
- Usage patterns in the frames-validator
✅ Verification successful
Based on the results, I can now generate the final response since I have sufficient information about both error handling patterns and usage in the frames-validator:
Error handling and usage patterns are properly implemented
The implementation is correct and well-integrated:
- Error handling follows the same pattern as other methods in the Client class - direct propagation from inner client, which is consistent with simple getter methods like
accountAddress(),inboxId(), etc. - The frames-validator properly uses these methods with explicit error handling:
- Throws specific error messages when authorization checks fail
- Used in critical validation flows for both installation and wallet address verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check error handling patterns in other similar methods
rg -A 2 "this\.#innerClient\." sdks/node-sdk/src/Client.ts
# Check usage patterns in frames-validator
rg -A 5 "isAddressAuthorized|isInstallationAuthorized" packages/frames-validator/src/
Length of output: 3146
packages/frames-validator/src/validation.ts (1)
51-57: Verify correctness of isInstallationAuthorized parameters
Ensure that the parameters inboxId and installationId are in the correct order as expected by the Client.isInstallationAuthorized method. Passing them in the wrong order could lead to incorrect authorization checks.
Run the following script to verify the parameter order in the method definition:
✅ Verification successful
Parameters are correctly ordered in isInstallationAuthorized method
The verification shows that the parameters in the method call match the method definition in sdks/node-sdk/src/Client.ts:
- Method call:
Client.isInstallationAuthorized(inboxId, installationId) - Method definition:
async isInstallationAuthorized(inboxId: string, installationId: Uint8Array)
The parameters are being passed in the correct order as expected by the method signature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the parameter order of `isInstallationAuthorized`
# Search for the method definition
rg -A 5 'static async isInstallationAuthorized\('
Length of output: 51
Script:
#!/bin/bash
# Let's try a broader search for the Client class and its methods
ast-grep --pattern 'class Client {
$$$
isInstallationAuthorized($$$) {
$$$
}
$$$
}'
# Also search for any file containing Client class
fd -e ts -e js Client
Length of output: 830
Script:
#!/bin/bash
# Let's also try searching for the method name without the static keyword
rg "isInstallationAuthorized" -A 5
Length of output: 1107
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/frames-validator/src/validation.ts (4)
52-58: Enhance installation authorization error handlingWhile the authorization check is good, consider adding more context to the error message to aid debugging.
- throw new Error("Installation not a member of association state"); + throw new Error(`Installation ${installationId} not authorized for inbox ${inboxId}`);
60-63: Add error context for wallet authorizationSimilarly, enhance the error message for wallet authorization failures.
- throw new Error("Unable to associate wallet address with inbox"); + throw new Error(`Wallet address ${walletAddress} not authorized for inbox ${inboxId}`);
Line range hint
69-73: Enhance signature verification error handlingThe signature verification is correct, but consider adding more context to aid debugging.
if (!valid) { - throw new Error("Invalid signature"); + throw new Error(`Invalid installation signature for installation ${installationId}`); }
52-63: Add debug logging for security operationsConsider adding debug logging for security-relevant operations to aid in troubleshooting and audit trails.
+ console.debug(`Verifying installation ${installationId} for inbox ${inboxId}`); const authorized = await client.isInstallationAuthorized( inboxId, installationId, ); if (!authorized) { + console.warn(`Installation authorization failed for ${installationId}`); throw new Error("Installation not a member of association state"); } + console.debug(`Verifying wallet ${walletAddress} for inbox ${inboxId}`); const isMember = await client.isAddressAuthorized(inboxId, walletAddress); if (!isMember) { + console.warn(`Wallet authorization failed for ${walletAddress}`); throw new Error("Unable to associate wallet address with inbox"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
packages/frames-client/package.json(1 hunks)packages/frames-validator/src/validation.ts(3 hunks)sdks/node-sdk/src/Client.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdks/node-sdk/src/Client.ts
🔇 Additional comments (2)
packages/frames-client/package.json (2)
73-73: Verify the version update and document the changes
The update to @xmtp/node-sdk@^0.0.28 reintroduces authorization methods, but being a pre-1.0 version, we should ensure stability.
Let's verify the version and changelog:
Consider adding a comment in the PR description documenting why this specific version is required, particularly noting the reintroduction of isAddressAuthorized and isInstallationAuthorized methods.
✅ Verification successful
Version update to @xmtp/node-sdk@^0.0.28 is appropriate and recent
The version update is verified to be the latest available version (0.0.28), published just recently on November 26, 2024. This aligns perfectly with the PR's authorization-related changes, and the version bump from 0.0.27 (published on November 23, 2024) represents the most current stable release.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check latest version and changelog for @xmtp/node-sdk
# Check latest version on npm
echo "Latest version on npm:"
npm view @xmtp/node-sdk version
# Check for recent versions and their publish dates
echo -e "\nRecent versions and publish dates:"
npm view @xmtp/node-sdk time --json | jq 'with_entries(select(.key|test("^0.0.2[7-9]$")))'
# Check for any security advisories
echo -e "\nChecking for security advisories:"
npm audit @xmtp/node-sdk --json || true
Length of output: 1069
73-73: Verify peer dependency compatibility
The package has a peer dependency on @xmtp/xmtp-js (>9.3.1). Let's ensure the new node-sdk version is compatible.
✅ Verification successful
Let me try another approach to verify the dependencies.
Peer dependency compatibility verified
The package.json shows that @xmtp/node-sdk is in devDependencies, not in dependencies, and the project already has @xmtp/xmtp-js@^12.0.0 as a devDependency which satisfies the peer dependency requirement of >9.3.1. Since both packages are in devDependencies, there's no compatibility concern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check compatibility between node-sdk and xmtp-js
# Get node-sdk's dependencies
echo "Checking @xmtp/[email protected] dependencies:"
npm view @xmtp/[email protected] peerDependencies --json || true
# Check if current xmtp-js version (^12.0.0) satisfies any constraints
echo -e "\nCurrent xmtp-js version in package.json:"
jq '.dependencies["@xmtp/xmtp-js"]' package.json
Length of output: 308
Script:
#!/bin/bash
# Let's check the actual package.json content first
echo "Content of packages/frames-client/package.json:"
cat packages/frames-client/package.json
echo -e "\nChecking @xmtp/[email protected] all dependencies:"
npm view @xmtp/[email protected] dependencies peerDependencies --json || true
Length of output: 2901
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/frames-validator/src/validation.ts (2)
52-61: Consider memoizing the client instanceCreating a new wallet and client instance for each validation call could impact performance. Consider memoizing these instances or moving them to a higher scope.
Example implementation:
+let cachedClient: Client | null = null; + +async function getOrCreateClient(): Promise<Client> { + if (cachedClient) return cachedClient; + const randomWallet = Wallet.createRandom(); + const encryptionKey = getRandomValues(new Uint8Array(32)); + cachedClient = await Client.create( + { + getAddress: () => randomWallet.address, + signMessage: async (message: string) => + getBytes(await randomWallet.signMessage(message)), + }, + encryptionKey, + ); + return cachedClient; +}
Line range hint
18-22: Document behavioral changesWhile the function signature remains the same, the internal validation behavior has changed significantly. This needs to be documented to help users understand the new authorization requirements.
Consider:
- Adding JSDoc comments explaining the new validation process
- Creating a migration guide for existing users
- Documenting the new error scenarios
Example JSDoc:
/** * Validates a frames post request using XMTP authorization. * @param data - The frames request data * @param env - Optional XMTP environment * @throws {Error} When installation or wallet address authorization fails * @throws {Error} When required fields are missing or invalid * @returns {Promise<XmtpValidationResponse>} */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/frames-validator/src/validation.ts(3 hunks)
🔇 Additional comments (2)
packages/frames-validator/src/validation.ts (2)
33-35:
Validate extracted fields before use
The inboxId and installationId are used in authorization checks without validation. These fields should be validated before use to prevent runtime errors.
Add validation checks:
installationId,
installationSignature,
inboxId,
} = deserializeProtoMessage(messageBytes);
+ if (!inboxId || !installationId) {
+ throw new Error("Missing required fields: inboxId or installationId");
+ }64-75:
Enhance error handling for authorization checks
The authorization checks should handle potential errors from the client methods and provide more informative error messages.
Implement proper error handling:
- const authorized = await client.isInstallationAuthorized(
- inboxId,
- installationId,
- );
- if (!authorized) {
- throw new Error("Installation not a member of association state");
- }
-
- const isMember = await client.isAddressAuthorized(inboxId, walletAddress);
-
- if (!isMember) {
- throw new Error("Unable to associate wallet address with inbox");
- }
+ try {
+ const authorized = await client.isInstallationAuthorized(
+ inboxId,
+ installationId,
+ );
+ if (!authorized) {
+ throw new Error("Installation not a member of association state");
+ }
+
+ const isMember = await client.isAddressAuthorized(inboxId, walletAddress);
+ if (!isMember) {
+ throw new Error("Unable to associate wallet address with inbox");
+ }
+ } catch (error) {
+ throw new Error(`Authorization check failed: ${error.message}`);
+ }
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
packages/frames-validator/src/validation.ts (3)
16-16: Remove commented out codeThis commented line appears to be unused development code. If it's not needed, it should be removed to maintain code cleanliness.
-// let seed = Mnemonic.fromEntropy(randomBytes(16), "", LangEn.wordlist()).computeSeed();
70-77: Consider implementing typed errorsTo improve error handling for consumers, consider implementing specific error types:
// Add at the top of the file export class ValidationError extends Error { constructor(message: string, public code: string) { super(message); this.name = 'ValidationError'; } } // Then update the error throws if (!authorized) { throw new ValidationError( "Installation not a member of association state", "UNAUTHORIZED_INSTALLATION" ); } if (!isMember) { throw new ValidationError( "Unable to associate wallet address with inbox", "UNAUTHORIZED_ADDRESS" ); }
Line range hint
20-24: Document behavior changesWhile the function signature remains the same, the internal validation behavior has changed significantly. Consider:
- Adding JSDoc comments to document the new validation process
- Updating the README with migration notes
- Bumping the major version number due to the behavioral changes
/** * Validates a frames post request using XMTP authorization. * @param data - The frames request data to validate * @param env - Optional XMTP environment configuration * @throws {ValidationError} When installation or address authorization fails * @throws {Error} When signature or data validation fails * @since 2.0.0 Now performs XMTP authorization checks */sdks/node-sdk/src/Client.ts (1)
458-474: Consider adding input validation for network optionsWhile the host resolution logic is correct, consider adding validation for the network options to provide better error messages when invalid options are provided.
Example implementation:
private static resolveHost(networkOptions: NetworkOptions): string { if (networkOptions.apiUrl) { return networkOptions.apiUrl; } const env = networkOptions.env || "dev"; if (!(env in ApiUrls)) { throw new Error(`Invalid environment: ${env}`); } return ApiUrls[env]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
packages/frames-validator/src/validation.ts(4 hunks)sdks/node-sdk/package.json(1 hunks)sdks/node-sdk/src/Client.ts(3 hunks)
🔇 Additional comments (7)
sdks/node-sdk/package.json (1)
56-56: LGTM! Verify version compatibility.
The dependency update to @xmtp/node-bindings@^0.0.26 looks good and aligns with the broader changes mentioned in the PR summary.
Let's verify version compatibility across the ecosystem:
✅ Verification successful
Version compatibility check completed - All versions are aligned
The version check shows:
@xmtp/node-bindingsis only used innode-sdkat version^0.0.26@xmtp/node-sdkis consistently used at version^0.0.28across dependent packages (frames-validatorandframes-client)
This indicates proper version alignment across the ecosystem, with no conflicting or outdated versions found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for version consistency across related packages
# Find all package.json files
echo "Checking @xmtp/node-bindings versions across the repository:"
rg -A 1 '"@xmtp/node-bindings":\s*"[^"]*"' --type json
echo -e "\nChecking @xmtp/node-sdk versions across the repository:"
rg -A 1 '"@xmtp/node-sdk":\s*"[^"]*"' --type json
Length of output: 768
packages/frames-validator/src/validation.ts (3)
1-5: LGTM: Import changes align with new authorization requirements
The new imports support the enhanced authorization logic using the XMTP client and wallet functionality.
66-77: LGTM: Robust authorization checks implemented
The dual authorization checks provide comprehensive validation:
- Installation authorization ensures valid installation
- Address authorization verifies wallet association
35-38: Verify validity of inboxId
Ensure that inboxId extracted from the deserialized message is valid before using it in authorization checks.
+ if (!inboxId) {
+ throw new Error("Invalid inbox ID");
+ }sdks/node-sdk/src/Client.ts (3)
18-19: LGTM: Import statements are correctly added
The new bindings are properly imported and appropriately aliased to avoid naming conflicts with the class methods.
274-286: LGTM: Instance methods are well-implemented
The new authorization methods are properly implemented:
- Clear method signatures with appropriate types
- Consistent with the class's pattern of delegating to the inner client
- Follow async/await pattern correctly
274-286: Verify the integration with frames-validator
Let's verify how these authorization methods are being used in the frames-validator package.
Also applies to: 458-474
✅ Verification successful
Integration with frames-validator is properly implemented
The authorization methods are correctly used in the frames-validator package for validating frame posts:
isInstallationAuthorizedis used to verify installation membershipisAddressAuthorizedis used to verify wallet address association with the inbox
The integration is working as expected with proper error handling and validation flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the new authorization methods are used in the frames-validator package
# Search for usage of authorization methods
echo "Searching for usage of authorization methods..."
rg -A 5 "isAddressAuthorized|isInstallationAuthorized" --type typescript
# Check if the frames-validator package imports the Client class
echo "Checking imports in frames-validator..."
rg -A 5 "from.*@xmtp/node-sdk.*Client" --type typescript
Length of output: 397
Script:
#!/bin/bash
# Let's try searching without file type restriction since typescript extension might not be recognized
echo "Searching for usage of authorization methods..."
rg "isAddressAuthorized|isInstallationAuthorized"
echo -e "\nChecking imports from node-sdk..."
rg "from.*@xmtp/node-sdk.*Client"
echo -e "\nLet's also check for any .ts files in the repository..."
fd "\.ts$"
echo -e "\nChecking package.json in frames-validator..."
fd "package.json" --exec grep -l "@xmtp/node-sdk" {}
Length of output: 12177
Script:
#!/bin/bash
# Let's check the specific implementation in frames-validator
echo "Checking the implementation in frames-validator/validation.ts..."
cat packages/frames-validator/src/validation.ts
echo -e "\nChecking the dependency version in frames-validator's package.json..."
cat packages/frames-validator/package.json
Length of output: 6661
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/frames-validator/src/validation.ts (1)
1-1: Remove unused importsThe imports
getRandomValuesandrandomBytesfrom 'crypto' are not used in the current implementation.-import { getRandomValues, randomBytes } from "crypto";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
packages/frames-client/package.json(1 hunks)packages/frames-validator/package.json(1 hunks)packages/frames-validator/src/validation.ts(3 hunks)sdks/node-sdk/package.json(2 hunks)sdks/node-sdk/src/Client.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/frames-client/package.json
- packages/frames-validator/package.json
- sdks/node-sdk/package.json
- sdks/node-sdk/src/Client.ts
🔇 Additional comments (3)
packages/frames-validator/src/validation.ts (3)
33-36:
Validate inboxId before use
The inboxId extracted from the deserialized message should be validated before use in authorization checks.
installationId,
installationSignature,
inboxId,
} = deserializeProtoMessage(messageBytes);
+ if (!inboxId) {
+ throw new Error("Invalid inbox ID");
+ }53-69: Verify authorization changes impact
Let's verify the usage of these new authorization methods across the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response:
Authorization changes are properly encapsulated
The authorization methods isInstallationAuthorized and isAddressAuthorized are only used in two locations:
- Core implementation in
sdks/node-sdk/src/Client.ts - Current validation logic in
packages/frames-validator/src/validation.ts
The validateFramesPost function is only called from:
- Test files (
validation.test.ts) - Validator implementation (
validator.ts)
These changes are well-contained within the validation flow and don't require modifications elsewhere in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of authorization methods and potential impacts
# Check for direct usage of isInstallationAuthorized and isAddressAuthorized
echo "Checking for direct usage of authorization methods:"
rg "isInstallationAuthorized|isAddressAuthorized" --type ts -A 2
# Check for validateFramesPost callers that might be affected
echo "Checking for validateFramesPost callers:"
rg "validateFramesPost\(" --type ts -A 2
Length of output: 3640
53-60:
Add error handling for isInstallationAuthorized
The async call to isInstallationAuthorized should be wrapped in a try-catch block to handle potential errors gracefully.
- const authorized = await Client.isInstallationAuthorized(
- { env },
- inboxId,
- installationId,
- );
- if (!authorized) {
- throw new Error("Installation not a member of association state");
- }
+ try {
+ const authorized = await Client.isInstallationAuthorized(
+ { env },
+ inboxId,
+ installationId,
+ );
+ if (!authorized) {
+ throw new Error("Installation not a member of association state");
+ }
+ } catch (error) {
+ throw new Error(`Installation authorization check failed: ${error.message}`);
+ }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/frames-validator/src/validation.ts (1)
51-67: Consider combining authorization checks for better performanceThe current implementation makes two separate async calls for authorization. Consider combining these checks in the Client class for better performance.
Suggestion for the Client class:
// In Client class static async validateFrameAuthorization( env: XmtpEnv, inboxId: string, installationId: string, walletAddress: string ): Promise<void> { const [isInstallationAuth, isAddressAuth] = await Promise.all([ this.isInstallationAuthorized(env, inboxId, installationId), this.isAddressAuthorized(env, inboxId, walletAddress) ]); if (!isInstallationAuth || !isAddressAuth) { throw new Error("Authorization failed"); } }Then update the validation code to use this combined check:
- const authorized = await Client.isInstallationAuthorized( - { env }, - inboxId, - installationId, - ); - if (!authorized) { - throw new Error("Installation not a member of association state"); - } - - const isMember = await Client.isAddressAuthorized( - { env }, - inboxId, - walletAddress, - ); - - if (!isMember) { - throw new Error("Unable to associate wallet address with inbox"); - } + try { + await Client.validateFrameAuthorization( + { env }, + inboxId, + installationId, + walletAddress + ); + } catch (error) { + console.error("Frame authorization failed:", error); + throw new Error("Authorization failed"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/frames-validator/src/validation.ts(3 hunks)
🔇 Additional comments (3)
packages/frames-validator/src/validation.ts (3)
51-58:
Add error handling for isInstallationAuthorized
The async call to isInstallationAuthorized should be wrapped in a try-catch block to prevent unhandled rejections from exposing internal details.
Apply this change:
- const authorized = await Client.isInstallationAuthorized(
- { env },
- inboxId,
- installationId,
- );
- if (!authorized) {
- throw new Error("Installation not a member of association state");
- }
+ try {
+ const authorized = await Client.isInstallationAuthorized(
+ { env },
+ inboxId,
+ installationId,
+ );
+ if (!authorized) {
+ throw new Error("Authorization failed");
+ }
+ } catch (error) {
+ // Log the actual error internally but throw a generic message
+ console.error("Installation authorization failed:", error);
+ throw new Error("Authorization failed");
+ }31-34:
Validate inboxId before authorization checks
The inboxId from the deserialized message is used in authorization checks without validation. A null or undefined inboxId could lead to security vulnerabilities.
Add validation before the authorization checks:
inboxId,
} = deserializeProtoMessage(messageBytes);
+ if (!inboxId) {
+ throw new Error("Authorization failed"); // Generic error for security
+ }Likely invalid or redundant comment.
60-67:
Use generic error messages for security
Current error messages like "Unable to associate wallet address with inbox" could leak internal implementation details. Use generic error messages for security failures.
Apply this change:
- const isMember = await Client.isAddressAuthorized(
- { env },
- inboxId,
- walletAddress,
- );
-
- if (!isMember) {
- throw new Error("Unable to associate wallet address with inbox");
- }
+ try {
+ const isMember = await Client.isAddressAuthorized(
+ { env },
+ inboxId,
+ walletAddress,
+ );
+ if (!isMember) {
+ throw new Error("Authorization failed");
+ }
+ } catch (error) {
+ // Log the actual error internally but throw a generic message
+ console.error("Wallet authorization failed:", error);
+ throw new Error("Authorization failed");
+ }Likely invalid or redundant comment.
rygine
force-pushed
the
coda/frames-check
branch
from
db3c086 to
ae0a28c
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Chores
@xmtp/frames-validatorand@xmtp/frames-clientto the latest version.Tests