Add stack connection and smoke test automation scripts #4567
Conversation
via chrome
called by test-stack to restart server
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4567 +/- ##
========================================
Coverage 39.44% 39.44%
========================================
Files 852 852
Lines 36847 36847
Branches 6011 5760 -251
========================================
Hits 14536 14536
Misses 21793 21793
Partials 518 518
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds automation scripts for connecting to a Quilt stack and running smoke tests for the catalog application. The scripts enable developers to automatically authenticate with a stack, generate configuration files, and validate the catalog's functionality through automated browser testing.
Key changes:
- Stack connection script that automates authentication and configuration generation using Chrome DevTools
- Comprehensive smoke test harness with Chrome browser automation for capturing errors and performance metrics
- New npm scripts for build configuration, testing, and integrated workflows
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| catalog/package.json | Adds new npm scripts and Chrome automation dependencies |
| catalog/internals/scripts/connect-stack.js | Implements stack connection with automated authentication and config generation |
| catalog/internals/scripts/test-stack.js | Provides comprehensive browser testing with error capture and reporting |
| catalog/internals/scripts/connect-stack.md | Documents the stack connection workflow and usage patterns |
| catalog/CHANGELOG.md | Records the addition of stack connection and testing automation |
Files not reviewed (1)
- catalog/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
catalog/internals/scripts/test-stack.js:1
- Using
eval()poses a security risk as it can execute arbitrary code. Consider usingJSON.parse()or a safer parsing method for credential data.
#!/usr/bin/env node
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Summary
This PR adds automation scripts for connecting to Quilt stacks and running smoke tests locally. The implementation includes Chrome DevTools integration for automated credential extraction and webpack dev server integration.
Key Changes:
connect-stack.js: Automation script that discovers stack URLs, handles authentication via Chrome, and generates dev configurationtest-stack.js: Smoke testing harness with automated browser testing capabilities- New npm scripts:
build:config,start:test,start:connectedfor streamlined development workflow - Comprehensive documentation explaining the connection workflow
Security Concerns:
- Use of
eval()for parsing credentials creates code injection vulnerability - Potential command injection in browser launching logic
- Chrome automation dependencies (
chrome-launcher,chrome-remote-interface) expand attack surface
Overall Assessment:
The scripts provide valuable development automation but contain security vulnerabilities that should be addressed before production use.
Confidence Score: 2/5
- This PR contains security vulnerabilities that could lead to code execution
- Score reflects critical security issues: eval() usage enables code injection and command execution risks exist. While the functionality is valuable for development workflows, the security vulnerabilities need immediate attention
- catalog/internals/scripts/connect-stack.js requires immediate security fixes for eval() and command injection
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| catalog/internals/scripts/connect-stack.js | 2/5 | New automation script with security vulnerabilities: eval() usage and potential command injection |
| catalog/internals/scripts/test-stack.js | 4/5 | Smoke test automation script with Chrome integration - appears well-structured with proper validation |
| catalog/package.json | 4/5 | Added new Chrome automation dependencies and npm scripts - dependencies appear legitimate |
Sequence Diagram
sequenceDiagram
participant Dev as Developer
participant Script as connect-stack.js
participant CLI as quilt3 CLI
participant Chrome as Chrome Browser
participant Stack as Quilt Stack
participant Webpack as Webpack Dev Server
Dev->>Script: npm run start:connected
Script->>CLI: quilt3 config
CLI-->>Script: Stack URL
Script->>Chrome: Launch browser with DevTools
Chrome->>Stack: Navigate to /login
Dev->>Stack: Complete authentication
Stack-->>Chrome: Store tokens in localStorage
Chrome-->>Script: Extract tokens via CDP
Script->>Stack: Fetch /config.json
Stack-->>Script: Stack configuration
Script->>Script: Generate config.js with auth
Script->>Webpack: Start dev server
Webpack-->>Dev: Catalog with stack connection
5 files reviewed, 3 comments
| // Use eval to parse JavaScript object (not just JSON) | ||
| let credentialsData; | ||
| try { | ||
| credentialsData = eval('(' + credentialsMatch[1] + ')'); |
There was a problem hiding this comment.
logic: Using eval() creates a security vulnerability. Replace with JSON.parse() or a safe parser.
| credentialsData = eval('(' + credentialsMatch[1] + ')'); | |
| credentialsData = JSON.parse(credentialsMatch[1]); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: catalog/internals/scripts/connect-stack.js
Line: 443:443
Comment:
logic: Using `eval()` creates a security vulnerability. Replace with `JSON.parse()` or a safe parser.
```suggestion
credentialsData = JSON.parse(credentialsMatch[1]);
```
How can I resolve this? If you propose a fix, please make it concise.| const loginUrl = `${this.stackUrl}/login`; | ||
| const openCommand = process.platform === 'darwin' ? 'open' : | ||
| process.platform === 'win32' ? 'start' : 'xdg-open'; | ||
| exec(`${openCommand} "${loginUrl}"`); |
There was a problem hiding this comment.
logic: Potential command injection vulnerability. The loginUrl should be validated or escaped.
| exec(`${openCommand} "${loginUrl}"`); | |
| exec(`${openCommand} ${JSON.stringify(loginUrl)}`); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: catalog/internals/scripts/connect-stack.js
Line: 298:298
Comment:
logic: Potential command injection vulnerability. The `loginUrl` should be validated or escaped.
```suggestion
exec(`${openCommand} ${JSON.stringify(loginUrl)}`);
```
How can I resolve this? If you propose a fix, please make it concise.
fiskus
left a comment
fiskus
left a comment
There was a problem hiding this comment.
It is a very good, wished and worthful endeavor.
However, there are few things, I'd like to change:
- I think "quiltdata/quilt/catalog" and even "quiltdata/quilt" is already bloated, and would be benefited from extracting some parts to standalone repos. This script doesn't use any resources from catalog codebase, so it can easily be placed into standalone repo, e2e repo or meta repo.
- I don't know for sure, but I think with a large level of confidence, there are test launchers, that work out of the box with fewer lines of code necessary to run them.
In other words,
- I would like to move this code to somewhere else: re-use existing repo, or create some test repo
- You can re-use existing tools to run headless web tests
|
|
||
| - `npm run connect:stack`: Run the connection script | ||
| - `npm run start:connected`: Connect to stack and start dev server | ||
| - `npm run start`: Start dev server (expects config to already be generated) |
There was a problem hiding this comment.
npm start is used for a different task already, and it is not defined in this PR.
I mean, it starts catalog dev server, not the "dev server" from this PR.
| ### Files Generated/Modified | ||
|
|
||
| - `static-dev/config.js`: Generated configuration file (gitignored) | ||
| - `.catalog-config.json`: Alternative config location (gitignored) |
| - `static-dev/config.js`: Generated configuration file (gitignored) | ||
| - `.catalog-config.json`: Alternative config location (gitignored) | ||
|
|
||
| Both files are added to `.gitignore` to prevent accidental commits of sensitive configuration data. |
There was a problem hiding this comment.
static-dev is git-ignored, not specific files in it
|
I was unable to run scripts, highly likely because Chrome is not installed the expected way (it is installed).
|
| * Test Stack Script | ||
| * | ||
| * This script automates testing of the catalog by: | ||
| * 1. Starting the webpack dev server (npm run start) |
There was a problem hiding this comment.
I think, it shouldn't run the dev server.
I didn't read the code, but I'd expect this script tests any test stack: localhost:3000, or nightly.quilttest.com, by specifying the host
| @@ -0,0 +1,142 @@ | |||
| <!-- markdownlint-disable MD013 --> | |||
There was a problem hiding this comment.
Let's not ignore lint, especially in a new file
|
tbh i'm not quite sure what's the value of this, given all the complexity. |
Summary
build:config,start:test,start:connected) with required dependenciesTesting