feat(shell): default Patchright for Playwright via NODE_PATH alias#1266
feat(shell): default Patchright for Playwright via NODE_PATH alias#1266atxtechbro wants to merge 3 commits intomainfrom
Conversation
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
There was a problem hiding this comment.
Thank you for the PR! I have reviewed the changes to implement Patchright as the default backend for Playwright. While the implementation is clever, there are several concerns to address:
- Security: The global NODE_PATH modification approach poses potential security risks through untrusted search paths.
- Robustness: There are opportunities to improve the shell script's robustness through better parameter expansion and path handling.
- Syntax: There's an unnecessary escape character that should be removed.
Consider exploring alternative approaches like npm's package aliasing or project-level configurations that don't require global NODE_PATH modifications. This would provide similar functionality while maintaining better security practices.
| export PATCHRIGHT_MODULE_ROOT="$HOME/.config/node-module-alias/node_modules" | ||
| if [ -d "$PATCHRIGHT_MODULE_ROOT" ]; then |
There was a problem hiding this comment.
🛑 [Security Risk]: Modifying NODE_PATH globally to inject modules can pose security risks1. Consider using project-level configurations or npm's package aliasing instead. This approach could lead to unintended module resolution and potential supply chain attacks.
Footnotes
-
CWE-426: Untrusted Search Path - https://cwe.mitre.org/data/definitions/426.html ↩
| # If a user-level module alias exists at ~/.config/node-module-alias/node_modules, | ||
| # add it to NODE_PATH so that require('playwright') resolves to Patchright. | ||
| # This avoids wrapper scripts and per-repo code changes. | ||
| export PATCHRIGHT_MODULE_ROOT="$HOME/.config/node-module-alias/node_modules" |
There was a problem hiding this comment.
Consider using ${HOME} instead of $HOME for consistent shell parameter expansion and to prevent potential word splitting issues. Also, consider quoting the path to handle spaces in directory names.
| *:$PATCHRIGHT_MODULE_ROOT:*) ;; | ||
| *) export NODE_PATH="$PATCHRIGHT_MODULE_ROOT:${NODE_PATH}" ;; |
There was a problem hiding this comment.
Consider handling the case where NODE_PATH is unset. The current approach might result in a trailing colon when NODE_PATH is empty. A more robust approach would be to use parameter expansion with default values.
| *:$PATCHRIGHT_MODULE_ROOT:*) ;; | |
| *) export NODE_PATH="$PATCHRIGHT_MODULE_ROOT:${NODE_PATH}" ;; | |
| *) export NODE_PATH="${PATCHRIGHT_MODULE_ROOT}${NODE_PATH:+:${NODE_PATH}}" ;; |
Closes #1263