feat(mobile): prevent 3rd party access our packages #17119
Conversation
WalkthroughThis pull request integrates a new security mechanism into the codebase by introducing the ✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
suite-native/app/metro.config.js (2)
41-48: Enhance error reporting for better security monitoring.The security check implementation effectively prevents unauthorized access. Consider these enhancements:
- Include the matched scope in the error message
- Add logging for security monitoring
- if ( - TREZOR_PACKAGES_SCOPES.some(scope => moduleName.includes(scope)) && - context.originModulePath.includes('node_modules/') - ) { + const matchedScope = TREZOR_PACKAGES_SCOPES.find(scope => moduleName.includes(scope)); + if (matchedScope && context.originModulePath.includes('node_modules/')) { + console.warn(`Security violation detected: ${context.originModulePath} attempted to access ${matchedScope}`); throw new Error( - `SECURITY ALERT: Some 3rd party package is using our interal packages or Connect! Please check this file: ${context.originModulePath}`, + `SECURITY ALERT: Package at ${context.originModulePath} attempted to access protected scope ${matchedScope}`, ); }
9-9: Fix typo in variable name.There's a typo in the variable name:
additionalSOurceExtsshould beadditionalSourceExts.-const additionalSOurceExts = process.env.RN_SRC_EXT ? process.env.RN_SRC_EXT.split(',') : []; +const additionalSourceExts = process.env.RN_SRC_EXT ? process.env.RN_SRC_EXT.split(',') : [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
suite-native/app/metro.config.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (2)
suite-native/app/metro.config.js (2)
12-13: LGTM! Well-defined package scopes.The constant clearly defines the protected package scopes that should not be accessed by third-party packages.
41-48:✅ Verification successful
Verify protection against path traversal attacks.
While the implementation blocks direct access, we should verify protection against potential bypass attempts:
- Path traversal in module names
- Alternate import methods
🏁 Script executed:
#!/bin/bash # Search for potential bypass patterns in the codebase # Check for relative imports of protected packages rg -g '!node_modules' "from '\.\..*@(trezor|suite-common|suite-native)" # Check for dynamic imports rg -g '!node_modules' "require\(.*@(trezor|suite-common|suite-native)" # Check for import() statements rg -g '!node_modules' "import\(.*@(trezor|suite-common|suite-native)"Length of output: 3086
Security validation passed for import checks.
After reviewing the code and searching for any relative path traversal or alternative import patterns that could bypass the current check, no evidence of any bypass was found. The static and dynamic import/require usages for the protected packages appear only in controlled parts of the codebase (e.g., internal tests and examples) and do not invoke the critical metro.config.js check. The existing logic insuite-native/app/metro.config.js—which throws an error when a protected module is imported from within anode_modulespath—remains effective.
- No relative path traversal patterns (e.g., using
../) were found attempting to access protected packages.- Dynamic import and require statements for protected modules exist only in expected contexts and are not bypassing the node_modules check.
Nodonisko
force-pushed
the
feat/more-secure-bundler
branch
from
c851f55 to
835e11f
Compare
Nodonisko
force-pushed
the
feat/more-secure-bundler
branch
from
835e11f to
f6b0955
Compare
|
🚀 Expo preview is ready!
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/bundler-security/src/WebpackSecurityPlugin.ts (2)
4-4: Consider renaming import for clarity.The
checkSecurityViolationmethod is imported from a file namedmetroSecureResolver, which might cause confusion since this plugin is part of a Webpack-specific solution. A separate filename or function name could help reflect that it’s also used by Webpack.
7-9: Consider using Webpack's built-in logging or capturing errors in the compilation object.Currently, the plugin uses
console.errorto display security errors. While this is functional, you might leverage Webpack’s logging or the compilation error mechanism (such ascompilation.errors.push(new Error(...))) to integrate more naturally with build pipelines.packages/bundler-security/src/metroSecureResolver.js (2)
1-5: Consider using TypeScript for better type safety.While the comment explains why CommonJS is used, consider creating TypeScript declarations (
.d.tsfile) to provide better type information for consumers.
32-46: Enhance error message with security context.The error message could be more informative about the security implications.
Consider this improvement:
if (isViolation) { throw new Error( - `SECURITY ALERT: Some 3rd party package is trying to import internal packages or Connect!\n` + + `SECURITY VIOLATION: Unauthorized access attempt detected!\n` + + `Third-party packages are not allowed to import internal @trezor packages or use relative paths.\n` + `Module: ${moduleName}\n` + `Normalized: ${normalizedPath}\n` + `From file: ${originModulePath}`, ); }packages/bundler-security/tests/metroSecureResolver.test.ts (1)
3-75: Add test cases for additional security scenarios.While the test coverage is good, consider adding these security-critical test cases:
- Windows-style paths
- Path traversal attempts with dots and slashes
- URL-encoded paths
- Unicode path characters
Here's an example of additional test cases:
it('should detect violation with Windows-style paths', () => { const result = checkSecurityViolation({ moduleName: '..\\..\\@trezor\\connect', originModulePath: 'path\\to\\node_modules\\external-lib\\index.js', }); expect(result.isViolation).toBe(true); }); it('should detect violation with URL-encoded paths', () => { const result = checkSecurityViolation({ moduleName: '%2e%2e/%2e%2e/@trezor/connect', originModulePath: 'path/to/node_modules/external-lib/index.js', }); expect(result.isViolation).toBe(true); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
packages/bundler-security/jest.config.js(1 hunks)packages/bundler-security/package.json(1 hunks)packages/bundler-security/src/WebpackSecurityPlugin.ts(1 hunks)packages/bundler-security/src/constants.js(1 hunks)packages/bundler-security/src/index.ts(1 hunks)packages/bundler-security/src/metroSecureResolver.js(1 hunks)packages/bundler-security/tests/metroSecureResolver.test.ts(1 hunks)packages/bundler-security/tsconfig.json(1 hunks)packages/connect-iframe/package.json(1 hunks)packages/connect-iframe/tsconfig.json(1 hunks)packages/connect-iframe/webpack/base.webpack.config.ts(2 hunks)packages/suite-build/configs/base.webpack.config.ts(2 hunks)packages/suite-build/package.json(1 hunks)packages/suite-build/tsconfig.json(1 hunks)packages/suite-desktop-core/tsconfig.lib.json(1 hunks)scripts/generatePackage.js(1 hunks)suite-native/app/metro.config.js(2 hunks)suite-native/app/package.json(1 hunks)suite-native/app/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/bundler-security/tsconfig.json
- packages/bundler-security/jest.config.js
- packages/bundler-security/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- suite-native/app/metro.config.js
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: test
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: transport-e2e-test
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (13)
packages/bundler-security/src/index.ts (1)
1-1: Export statement looks good.Exporting the plugin here centralizes access to it and maintains clean module boundaries.
packages/bundler-security/src/constants.js (2)
1-2: Clear scoping strategy.Defining scopes and package paths up front is a straightforward way to manage internal references. This improves maintainability and consistency across the codebase.
4-7: Validate correctness of the regex pattern.The regex enforces two or more “../” segments before matching the package paths. If you intend to block any relative path referencing these packages, confirm that the rule covers single “../” references as well.
Would you like a script to check for single “../” references in your codebase to ensure the regex meets your security needs?
scripts/generatePackage.js (1)
9-10: LGTM!The change to named imports improves code clarity and maintainability.
packages/suite-build/configs/base.webpack.config.ts (1)
164-164:✅ Verification successful
Verify WebpackSecurityCheckPlugin configuration.
The plugin is initialized without any configuration options. Please verify if this is the intended behavior.
🌐 Web query:
What configuration options are available for WebpackSecurityCheckPlugin in @trezor/bundler-security?💡 Result:
Based on the provided search results, there is no direct mention of
WebpackSecurityCheckPluginor its configuration options in@trezor/bundler-security. The available information focuses on:
Trezor's security architecture
- Passphrase wallets as an additional security layer ([4][6][10])
- Hardware wallet setup best practices ([8][10])
- Seed phrase protection mechanisms ([6][10])
General Webpack plugin configurations
- Common patterns for using plugins like
DefinePluginandHtmlBundlerPlugin([1][3][5][9])- Tree-shaking optimizations with Sentry ([7])
- Multi-plugin composition strategies ([1][5])
For Trezor-specific bundler configurations:
- The
common-config-webpack-pluginsuite shows modular plugin architecture ([1])- Trezor Connect changelog indicates TypeScript integration and blockchain methods ([2])
To get details about
@trezor/bundler-securityconfigurations:
- Check Trezor's official developer documentation
- Review
package.jsondependencies in Trezor Connect repository ([2])- Look for security-specific webpack plugins in Trezor's GitHub org
The search results don't contain explicit references to
WebpackSecurityCheckPlugin, suggesting it might be part of Trezor's internal build tooling rather than a publicly documented configuration.Citations:
- 1: https://github.com/namics/webpack-config-plugins/blob/master/README.md
- 2: https://github.com/trezor/connect/blob/develop/CHANGELOG.md
- 3: https://www.npmjs.com/package/html-bundler-webpack-plugin
- 4: https://trezor.io/learn/a/standard-passphrase-wallets
- 5: https://webpack.js.org/configuration/plugins/
- 6: https://privacypros.io/trezor/one-review/
- 7: https://docs.sentry.io/platforms/javascript/configuration/tree-shaking/
- 8: https://www.youtube.com/watch?v=whFsn8KxFkA
- 9: https://github.com/webpack/docs/wiki/configuration
- 10: https://trezor.io/learn/a/security-safety-in-trezor
Plugin Initialization Verified: No Additional Configuration Needed
The current usage of
WebpackSecurityCheckPluginwithout any configuration options appears to be intended. A web search for configuration options in@trezor/bundler-securitydid not reveal any available settings, which suggests that the default instantiation is correct.
- File:
packages/suite-build/configs/base.webpack.config.ts, Line 164- No documented configuration options were found; ensure that this aligns with internal guidelines if any exist.
packages/connect-iframe/webpack/base.webpack.config.ts (1)
6-6:❓ Verification inconclusive
Verify WebpackSecurityCheckPlugin configuration.
The security plugin is correctly imported and integrated into the webpack configuration. However, consider whether any configuration options should be provided to customize the security checks based on project requirements.
Run this script to check if the plugin accepts any configuration options:
Also applies to: 111-111
🏁 Script executed:
#!/bin/bash # Description: Check WebpackSecurityCheckPlugin documentation and usage ast-grep --pattern 'class WebpackSecurityCheckPlugin { constructor($_) { $$$ } $$$ }'Length of output: 97
Action Required: Verify Security Plugin Configuration
The WebpackSecurityCheckPlugin is imported and inserted early in the plugins array, which is a good practice for security checks. However, the verification script did not return any information about its constructor or configuration options. Please manually verify whether the plugin supports configurable options to tailor its security checks, and update the configuration if necessary.
packages/suite-build/tsconfig.json (1)
9-9: LGTM!The bundler-security package reference is correctly added to the TypeScript project references.
packages/connect-iframe/tsconfig.json (1)
12-12: LGTM!The bundler-security package reference is correctly added to the TypeScript project references.
packages/suite-desktop-core/tsconfig.lib.json (1)
8-73:❓ Verification inconclusive
LGTM! Verify module resolution.
The TypeScript project references are correctly added with proper relative paths. This comprehensive list of references will help enforce proper module resolution and prevent unauthorized access.
Run this script to verify that all referenced packages exist:
🏁 Script executed:
#!/bin/bash # Description: Verify all referenced packages exist while IFS= read -r path; do if [[ -d "$path" ]]; then echo "✓ Found: $path" else echo "✗ Missing: $path" fi done < <(sed -n 's/.*"path": "\([^"]*\)".*/\1/p' packages/suite-desktop-core/tsconfig.lib.json)Length of output: 197
LGTM – Module resolution appears correct, but manual verification is needed.
The package references in
packages/suite-desktop-core/tsconfig.lib.jsonare properly formatted and consistent. However, the automated verification script failed due to a process substitution error (/dev/fd/63: No such file or directory). Please run the following revised script manually to ensure that all referenced package directories exist:#!/bin/bash # Revised verification: writes paths to a temporary file to avoid process substitution issues. tempfile=$(mktemp) sed -n 's/.*"path": "\([^"]*\)".*/\1/p' packages/suite-desktop-core/tsconfig.lib.json > "$tempfile" while IFS= read -r path; do if [[ -d "$path" ]]; then echo "✓ Found: $path" else echo "✗ Missing: $path" fi done < "$tempfile" rm "$tempfile"Ensure all paths output “✓ Found” to confirm that module resolution is working as intended.
packages/connect-iframe/package.json (1)
22-22: Bundler Security Dev Dependency Addition.
The new dev dependency@trezor/bundler-securityhas been added, which is key for enabling webpack’s security checks. Ensure that its usage (e.g. viaWebpackSecurityCheckPluginin the webpack configuration) is consistent with your overall security strategy.packages/suite-build/package.json (1)
26-26: Bundler Security Dependency Integration.
The dependency@trezor/bundler-securityhas been successfully added to the dependencies section. This integration should enable enhanced security checks during the build process. Verify that the corresponding webpack configurations in this package (and possibly elsewhere in the repo) are updated accordingly.suite-native/app/tsconfig.json (1)
75-77: New TypeScript Reference for Bundler Security.
A new reference to the bundler-security package has been added ("path": "../../packages/bundler-security"). This ensures that its types are correctly included and compiled. Please double-check that the relative path is correct given your monorepo structure.suite-native/app/package.json (1)
82-82: Bundler Security Dependency Added.
The dependency@trezor/bundler-securityis now included in the dependencies section, supporting enhanced security features (e.g., via Metro’s resolver or other integrations). Make sure that the related Metro configuration (wheremetroSecureResolveris imported) aligns with this addition.
| @@ -5,6 +5,10 @@ import webpack from 'webpack'; | |||
| import { BundleAnalyzerPlugin } from 'webpack-bundle-analyzer'; | |||
|
|
|||
| // Get Suite App version from the Suite package.json | |||
| import { WebpackSecurityCheckPlugin } from '@trezor/bundler-security'; | |||
|
|
|||
| console.log('WebpackSecurityCheckPlugin', WebpackSecurityCheckPlugin); | |||
There was a problem hiding this comment.
Remove debug console.log statement.
Debug logging of security-related components should be removed in production code.
-console.log('WebpackSecurityCheckPlugin', WebpackSecurityCheckPlugin);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log('WebpackSecurityCheckPlugin', WebpackSecurityCheckPlugin); |
|
looks good and safe (only build time if I get it right). |
There was a problem hiding this comment.
I just called yarn refs, not sure why it's different than develop but it looks correct now.
There was a problem hiding this comment.
interesting it seems that desktop build fails now because of this... if I remove it work fine, but it should not be possible to have this in develop because yarn refs must not produce diff on develop... I guess it's refs scripts is fucked up
Nodonisko
force-pushed
the
feat/more-secure-bundler
branch
from
f6b0955 to
e0cf237
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/bundler-security/src/metroSecureResolver.js (1)
16-32:⚠️ Potential issueEnhance security check robustness.
The security check logic has potential edge cases that could be exploited:
- Path traversal attacks using complex relative paths
- Inconsistent handling of Windows-style paths
Apply this diff to improve security:
const checkSecurityViolation = ({ moduleName, originModulePath }) => { - const normalizedPath = path.normalize(moduleName); + // Normalize to posix paths and resolve any directory traversal + const normalizedPath = path.posix.normalize(moduleName.replace(/\\/g, '/')); + const normalizedOrigin = path.posix.normalize(originModulePath.replace(/\\/g, '/')); - const isNodeModules = originModulePath.includes('node_modules'); + const isNodeModules = normalizedOrigin.includes('node_modules'); const isScopedPackage = TREZOR_PACKAGES_SCOPES.some(scope => normalizedPath.includes(scope)); const isRelativePath = RELATIVE_PATH_REGEX.test(normalizedPath); return { isViolation: (isScopedPackage || isRelativePath) && isNodeModules, normalizedPath, }; };
🧹 Nitpick comments (1)
packages/bundler-security/src/metroSecureResolver.js (1)
26-26: Add explanation forisRelativePathcheck.The
isRelativePathcheck is crucial for security as it prevents third-party packages from accessing internal packages through relative path traversal (e.g.,../../../packages/connect). Consider adding a more detailed comment:+ // isRelativePath detects attempts to access internal packages through relative path traversal + // e.g., '../../../packages/connect' or '../../scripts/../packages/connect' const isRelativePath = RELATIVE_PATH_REGEX.test(normalizedPath);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
packages/bundler-security/jest.config.js(1 hunks)packages/bundler-security/package.json(1 hunks)packages/bundler-security/src/WebpackSecurityPlugin.ts(1 hunks)packages/bundler-security/src/constants.js(1 hunks)packages/bundler-security/src/index.ts(1 hunks)packages/bundler-security/src/metroSecureResolver.js(1 hunks)packages/bundler-security/tests/metroSecureResolver.test.ts(1 hunks)packages/bundler-security/tsconfig.json(1 hunks)packages/connect-iframe/package.json(1 hunks)packages/connect-iframe/tsconfig.json(1 hunks)packages/connect-iframe/webpack/base.webpack.config.ts(2 hunks)packages/suite-build/configs/base.webpack.config.ts(2 hunks)packages/suite-build/package.json(1 hunks)packages/suite-build/tsconfig.json(1 hunks)packages/suite-desktop-core/tsconfig.lib.json(1 hunks)scripts/generatePackage.js(1 hunks)suite-native/app/metro.config.js(2 hunks)suite-native/app/package.json(1 hunks)suite-native/app/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/bundler-security/src/index.ts
- suite-native/app/tsconfig.json
- packages/connect-iframe/tsconfig.json
- packages/bundler-security/tsconfig.json
- packages/suite-build/tsconfig.json
- packages/bundler-security/package.json
- packages/connect-iframe/package.json
- suite-native/app/package.json
- packages/bundler-security/jest.config.js
- packages/connect-iframe/webpack/base.webpack.config.ts
- packages/suite-build/configs/base.webpack.config.ts
- packages/suite-build/package.json
- packages/bundler-security/src/constants.js
- scripts/generatePackage.js
- suite-native/app/metro.config.js
- packages/bundler-security/src/WebpackSecurityPlugin.ts
- packages/suite-desktop-core/tsconfig.lib.json
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build
- GitHub Check: transport-e2e-test
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (6)
packages/bundler-security/src/metroSecureResolver.js (3)
1-4: LGTM! Well-structured imports.Good separation of concerns by keeping constants in a separate file and using CommonJS format as required by Metro.
34-51: LGTM! Clear and informative error handling.The error message provides excellent context by including the module name, normalized path, and origin file. This will help developers quickly identify and fix security violations.
53-56: LGTM! Clean exports.Both functions are properly exported for external use.
packages/bundler-security/tests/metroSecureResolver.test.ts (3)
4-4: Follow team's test description format.As per team convention noted in Notion, use this format:
- it('should detect violation when accessing @trezor scoped package from node_modules', () => { + it('detects violation when accessing @trezor scoped package from node_modules', () => {
1-75: LGTM! Comprehensive test coverage.The test suite thoroughly covers:
- Direct @trezor package imports from node_modules
- Various relative path patterns including complex traversal
- Valid imports from non-node_modules locations
- Regular node_modules imports
3-75: LGTM! Well-organized test structure.Tests are logically organized:
- Security violation scenarios first
- Allowed scenarios later
- Clear test data and assertions
Nodonisko
force-pushed
the
feat/more-secure-bundler
branch
2 times, most recently
from
4be2f5d to
01432ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/bundler-security/src/metroSecureResolver.js (1)
17-18: 🛠️ Refactor suggestionRemove redundant path normalization.
Based on the previous feedback, path normalization is already handled by webpack before reaching these security checks.
- // Normalize path to remove some crazy relative paths like `../../scripts/..packages/connect` etc. - const normalizedPath = path.normalize(moduleName); + const normalizedPath = moduleName;
🧹 Nitpick comments (2)
packages/bundler-security/src/metroSecureResolver.js (2)
6-10: Add property descriptions to the type definition.Consider enhancing the JSDoc type definition with descriptions for each property to improve code documentation.
/** * @typedef {Object} SecurityCheckParams - * @property {string} moduleName - * @property {string} originModulePath + * @property {string} moduleName - The name/path of the module being imported + * @property {string} originModulePath - The absolute path of the file making the import */
22-26: Improve comment clarity and fix typo.The comments could be clearer and there's a typo in "standarts".
- // prevent standarts imports like `from '@trezor/connect'` or `require('@trezor/connect')` etc. + // Prevent standard imports of internal packages (e.g., `from '@trezor/connect'` or `require('@trezor/connect')`) const isScopedPackage = TREZOR_PACKAGES_SCOPES.some(scope => normalizedPath.includes(scope)); - // 3rd party package can import Connect from node_modules using relative path like `../../../packages/connect` - // check tests for more examples + // Detect attempts by 3rd party packages to import internal packages using relative paths + // (e.g., `../../../packages/connect`). See tests for more examples.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
packages/bundler-security/jest.config.js(1 hunks)packages/bundler-security/package.json(1 hunks)packages/bundler-security/src/WebpackSecurityPlugin.ts(1 hunks)packages/bundler-security/src/constants.js(1 hunks)packages/bundler-security/src/index.ts(1 hunks)packages/bundler-security/src/metroSecureResolver.js(1 hunks)packages/bundler-security/tests/metroSecureResolver.test.ts(1 hunks)packages/bundler-security/tsconfig.json(1 hunks)packages/connect-iframe/package.json(1 hunks)packages/connect-iframe/tsconfig.json(1 hunks)packages/connect-iframe/webpack/base.webpack.config.ts(2 hunks)packages/suite-build/configs/base.webpack.config.ts(2 hunks)packages/suite-build/package.json(1 hunks)packages/suite-build/tsconfig.json(1 hunks)packages/suite-desktop-core/tsconfig.lib.json(1 hunks)scripts/generatePackage.js(1 hunks)suite-native/app/metro.config.js(2 hunks)suite-native/app/package.json(1 hunks)suite-native/app/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/suite-build/package.json
- packages/connect-iframe/tsconfig.json
- suite-native/app/tsconfig.json
- packages/bundler-security/src/index.ts
- packages/bundler-security/tsconfig.json
- packages/suite-build/tsconfig.json
- packages/bundler-security/jest.config.js
- packages/connect-iframe/package.json
- packages/bundler-security/package.json
- suite-native/app/metro.config.js
- packages/connect-iframe/webpack/base.webpack.config.ts
- packages/bundler-security/tests/metroSecureResolver.test.ts
- packages/bundler-security/src/WebpackSecurityPlugin.ts
- packages/suite-build/configs/base.webpack.config.ts
- packages/bundler-security/src/constants.js
- scripts/generatePackage.js
- suite-native/app/package.json
- packages/suite-desktop-core/tsconfig.lib.json
🧰 Additional context used
🧠 Learnings (1)
packages/bundler-security/src/metroSecureResolver.js (1)
Learnt from: Nodonisko
PR: trezor/trezor-suite#17119
File: packages/bundler-security/src/metroSecureResolver.js:0-0
Timestamp: 2025-02-21T10:52:39.712Z
Learning: In the bundler-security package, path normalization is already handled by webpack before reaching our security checks, so additional normalization in the security check functions is unnecessary.
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
- GitHub Check: build-web
- GitHub Check: build-web
🔇 Additional comments (3)
packages/bundler-security/src/metroSecureResolver.js (3)
1-4: LGTM! Clear explanation of CommonJS usage.The comment explaining Metro's limitations with TypeScript and ES modules is helpful for future maintainers.
34-51: LGTM! Clear error message and good separation of concerns.The function provides a detailed error message that will help developers identify and fix security violations.
53-56: LGTM! Clean exports.The exports follow the CommonJS pattern consistently.
Nodonisko
force-pushed
the
feat/more-secure-bundler
branch
2 times, most recently
from
45296de to
a6ce364
Compare
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13497027156 |
trezor-ci
force-pushed
the
feat/more-secure-bundler
branch
from
a6ce364 to
61bbfcf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/bundler-security/src/metroSecureResolver.js (1)
16-32: 🛠️ Refactor suggestionUnify normalization approach for all paths.
You currently normalize
moduleNamebut notoriginModulePath. For strict cross-platform consistency (particularly on Windows paths), consider applying a similar normalization approach tooriginModulePathbefore checking for"node_modules".const checkSecurityViolation = ({ moduleName, originModulePath }) => { + const normalizedOrigin = path.normalize(originModulePath); const normalizedPath = path.normalize(moduleName); - const isNodeModules = originModulePath.includes('node_modules'); + const isNodeModules = normalizedOrigin.includes('node_modules'); // ... };
🧹 Nitpick comments (3)
packages/bundler-security/src/metroSecureResolver.js (3)
6-10: Clarify JSDoc usage.The JSDoc block is well-structured. Ensure that TypeScript or IDE tooling is indeed utilizing these JSDoc definitions for type validation, if desired.
22-22: Fix minor spelling typo.“standarts” should be “standards.”
- // prevent standarts imports + // prevent standard imports
34-51: Ensure helpful error context.Throwing an error is appropriate. However, consider adding more guidance for developers on how to resolve the violation (e.g., recommended alternative import paths).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
packages/bundler-security/jest.config.js(1 hunks)packages/bundler-security/package.json(1 hunks)packages/bundler-security/src/WebpackSecurityPlugin.ts(1 hunks)packages/bundler-security/src/constants.js(1 hunks)packages/bundler-security/src/index.ts(1 hunks)packages/bundler-security/src/metroSecureResolver.js(1 hunks)packages/bundler-security/tests/metroSecureResolver.test.ts(1 hunks)packages/bundler-security/tsconfig.json(1 hunks)packages/connect-iframe/package.json(1 hunks)packages/connect-iframe/tsconfig.json(1 hunks)packages/connect-iframe/webpack/base.webpack.config.ts(2 hunks)packages/suite-build/configs/base.webpack.config.ts(2 hunks)packages/suite-build/package.json(1 hunks)packages/suite-build/tsconfig.json(1 hunks)scripts/generatePackage.js(1 hunks)suite-native/app/metro.config.js(2 hunks)suite-native/app/package.json(1 hunks)suite-native/app/tsconfig.json(1 hunks)suite-native/state/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- suite-native/state/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/bundler-security/tsconfig.json
- packages/suite-build/tsconfig.json
- suite-native/app/package.json
- suite-native/app/tsconfig.json
- packages/suite-build/package.json
- packages/connect-iframe/tsconfig.json
- packages/suite-build/configs/base.webpack.config.ts
- packages/bundler-security/jest.config.js
- packages/bundler-security/package.json
- packages/connect-iframe/package.json
- scripts/generatePackage.js
- suite-native/app/metro.config.js
- packages/bundler-security/src/WebpackSecurityPlugin.ts
- packages/connect-iframe/webpack/base.webpack.config.ts
- packages/bundler-security/src/index.ts
- packages/bundler-security/src/constants.js
- packages/bundler-security/tests/metroSecureResolver.test.ts
🧰 Additional context used
🧠 Learnings (1)
packages/bundler-security/src/metroSecureResolver.js (1)
Learnt from: Nodonisko
PR: trezor/trezor-suite#17119
File: packages/bundler-security/src/metroSecureResolver.js:0-0
Timestamp: 2025-02-21T10:52:39.712Z
Learning: In the bundler-security package, path normalization is already handled by webpack before reaching our security checks, so additional normalization in the security check functions is unnecessary.
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: build-web
- GitHub Check: test
🔇 Additional comments (2)
packages/bundler-security/src/metroSecureResolver.js (2)
1-5: Looks good for Metro bundler integration.This approach to keep the file in plain CommonJS aligns well with Metro’s requirements. Importing and referencing constants is also straightforward.
53-56: Module export structure is consistent.Exporting both functions together is straightforward and follows CommonJS norms.
Nodonisko
force-pushed
the
feat/more-secure-bundler
branch
from
61bbfcf to
eb76d5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/bundler-security/src/metroSecureResolver.js (2)
16-32: Consider stricter matching for Trezor scopes.The check uses
normalizedPath.includes(scope). This can accidentally match substrings like@trezor/connect-somethingelse. If that’s intended, keep it; otherwise, consider using a more precise check, for examplenormalizedPath.startsWith(scope + '/').Additionally, revisit whether path normalization is required for Metro. Per your own learning notes, you mentioned that path normalization may already be handled in another step. If that still applies in Metro, you might remove the extra normalization to reduce complexity.
34-52: Ensure actionable error messages for developers.Throwing an error is valid for security violations; however, consider adding remediation steps or a link to documentation in the error message so developers know how to address the violation if triggered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
packages/bundler-security/jest.config.js(1 hunks)packages/bundler-security/package.json(1 hunks)packages/bundler-security/src/WebpackSecurityPlugin.ts(1 hunks)packages/bundler-security/src/constants.js(1 hunks)packages/bundler-security/src/index.ts(1 hunks)packages/bundler-security/src/metroSecureResolver.js(1 hunks)packages/bundler-security/tests/metroSecureResolver.test.ts(1 hunks)packages/bundler-security/tsconfig.json(1 hunks)packages/connect-iframe/package.json(1 hunks)packages/connect-iframe/tsconfig.json(1 hunks)packages/connect-iframe/webpack/base.webpack.config.ts(2 hunks)packages/suite-build/configs/base.webpack.config.ts(2 hunks)packages/suite-build/package.json(1 hunks)packages/suite-build/tsconfig.json(1 hunks)scripts/generatePackage.js(1 hunks)suite-native/app/metro.config.js(2 hunks)suite-native/app/package.json(1 hunks)suite-native/app/tsconfig.json(1 hunks)suite-native/state/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/connect-iframe/tsconfig.json
- suite-native/app/tsconfig.json
- suite-native/state/tsconfig.json
- packages/suite-build/tsconfig.json
- packages/bundler-security/tsconfig.json
- suite-native/app/package.json
- packages/bundler-security/package.json
- packages/connect-iframe/webpack/base.webpack.config.ts
- packages/suite-build/configs/base.webpack.config.ts
- scripts/generatePackage.js
- packages/connect-iframe/package.json
- packages/suite-build/package.json
- packages/bundler-security/src/constants.js
- suite-native/app/metro.config.js
- packages/bundler-security/src/WebpackSecurityPlugin.ts
- packages/bundler-security/jest.config.js
- packages/bundler-security/src/index.ts
- packages/bundler-security/tests/metroSecureResolver.test.ts
🧰 Additional context used
🧠 Learnings (1)
packages/bundler-security/src/metroSecureResolver.js (1)
Learnt from: Nodonisko
PR: trezor/trezor-suite#17119
File: packages/bundler-security/src/metroSecureResolver.js:0-0
Timestamp: 2025-02-21T10:52:39.712Z
Learning: In the bundler-security package, path normalization is already handled by webpack before reaching our security checks, so additional normalization in the security check functions is unnecessary.
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: transport-e2e-test
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: test
🔇 Additional comments (3)
packages/bundler-security/src/metroSecureResolver.js (3)
1-3: Code readability note.The opening comment clarifies why this file uses plain JavaScript with CommonJS for Metro bundler compatibility. It's helpful context and aligns with Metro's requirements.
6-11: Great use of JSDoc typedef.Defining
SecurityCheckParamswith clear property types improves readability and maintainability.
53-56: Modular export is concise and clear.Exporting both methods under a single object is a neat approach for typical CommonJS usage.
Description
This should add basic protection agains 3rd party packages access our internal libs or Connect which they have no reason to do unless it's something malicious.
Related Issue
Resolve
Screenshots: