Skip to content

SDK path on NPM fix#7

Merged
tschoem merged 3 commits intomainfrom
npm-deploy
Sep 4, 2025
Merged

SDK path on NPM fix#7
tschoem merged 3 commits intomainfrom
npm-deploy

Conversation

@tschoem
Copy link
Copy Markdown
Contributor

@tschoem tschoem commented Sep 3, 2025

  • SDK path was not working for npm deploy
  • added utf8 encoding for string parsing
  • removed path from sanitization

- SDK path was not working for npm deploy
- added utf8 encoding for string parsing
- removed path from sanitization
Comment thread index.mjs Fixed
Comment thread index.mjs Fixed
Comment thread index.mjs Fixed
Copy link
Copy Markdown

@abdul-qadir92 abdul-qadir92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved - Excellent Security and Modernization Updates

This PR represents a significant improvement with enhanced security, robust error handling, and modern ES module support.

🔒 Security Enhancements (Major)

  • Path Traversal Protection: Comprehensive file path validation prevents directory traversal attacks
  • File Type Validation: Only allows .side files with proper extension checking
  • Containment Checks: Ensures file access stays within working directory using path.relative()
  • Safe Path Resolution: Uses pathToFileURL and URL for secure path handling

🛠 Technical Improvements

  • ES Module Compatibility: Updated to use named exports { rimrafSync } and { globSync }
  • Enhanced SDK Resolution: Multi-candidate fallback system with debug logging
  • Robust Error Handling: Comprehensive try-catch blocks with detailed error messages
  • UTF-8 Encoding: Explicit encoding for reliable JSON file parsing
  • Node 18+ Support: Added engines field and confirmed compatibility

🧪 Test Results

Tested on Node 18.20.0 with real .side file and browserstack.yml:

  • ✅ File processing with security validation
  • ✅ BrowserStack integration successful (3 platforms)
  • ✅ SDK resolution working with debug output
  • ✅ Cleanup functionality working
  • ✅ Cross-platform test execution working

📊 Version & Dependencies

  • Version: Bumped to 2.4.0 (appropriate for major security updates)
  • Dependencies: Updated browserstack-node-sdk, selenium-webdriver, and other packages
  • Engines: Added "node": ">=18" requirement

🎯 Production Ready

This branch is production-ready with:

  • Security-first approach
  • Comprehensive error handling
  • Modern ES module support
  • Enhanced debugging capabilities

Minor Notes (non-blocking):

  • Still uses --timeouts instead of --timeout (but works fine)
  • Could consider updating to { program } import for commander (optional)

Verdict

APPROVED - This is a critical security and modernization update that should be merged immediately. The security enhancements alone make this essential for production use.

Ready to merge! 🚀

@tschoem tschoem merged commit 5f97a34 into main Sep 4, 2025
7 checks passed
@tschoem tschoem deleted the npm-deploy branch September 4, 2025 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants