-
Notifications
You must be signed in to change notification settings - Fork 204
Updated SSR tests app #2961
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Updated SSR tests app #2961
Conversation
| // Generate certificates | ||
| console.log('🔐 Generating certificates...'); | ||
| execSync( | ||
| `mkcert -key-file ${path.join(certDir, 'localhost-key.pem')} -cert-file ${path.join(certDir, 'localhost.pem')} localhost 127.0.0.1`, |
Check warning
Code scanning / CodeQL
Shell command built from environment values Medium
absolute path
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 3 hours ago
In general, to fix this kind of problem you should avoid passing a single shell command string to APIs that invoke a shell (exec, execSync) when that string contains dynamic data. Instead, call the binary directly and pass dynamic values as separate arguments using execFile/execFileSync (or spawn/spawnSync), which bypass shell parsing.
Here, we can keep using mkcert but switch the certificate generation call on lines 31–34 from execSync(<string>, ...) to execFileSync('mkcert', [<args>], ...). The arguments will include -key-file, the key path, -cert-file, the cert path, and the hostnames. This preserves existing functionality (same mkcert options, same stdio behavior) while eliminating shell interpretation of certDir and the joined paths.
Concretely in tools/cli/setupSSRCerts.js:
- Change the import on line 2 from
const { execSync } = require('child_process');to import bothexecSyncandexecFileSync, since we still needexecSyncfor the simple literal commands (mkcert -version,mkcert -install) and will useexecFileSyncfor the dynamic-argument call. - Replace the
execSync(...template literal...)block starting at line 31 with anexecFileSync('mkcert', [ ... ], { stdio: 'inherit' })call, building the two file paths viapath.join(certDir, ...)` as now, but keeping them as plain string arguments instead of embedded in a shell command.
No other methods or dependencies are required beyond adding execFileSync to the existing child_process destructuring.
-
Copy modified line R2 -
Copy modified lines R31-R40
| @@ -1,5 +1,5 @@ | ||
| /* eslint-disable */ | ||
| const { execSync } = require('child_process'); | ||
| const { execSync, execFileSync } = require('child_process'); | ||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
|
|
||
| @@ -28,8 +28,16 @@ | ||
|
|
||
| // Generate certificates | ||
| console.log('🔐 Generating certificates...'); | ||
| execSync( | ||
| `mkcert -key-file ${path.join(certDir, 'localhost-key.pem')} -cert-file ${path.join(certDir, 'localhost.pem')} localhost 127.0.0.1`, | ||
| execFileSync( | ||
| 'mkcert', | ||
| [ | ||
| '-key-file', | ||
| path.join(certDir, 'localhost-key.pem'), | ||
| '-cert-file', | ||
| path.join(certDir, 'localhost.pem'), | ||
| 'localhost', | ||
| '127.0.0.1', | ||
| ], | ||
| { stdio: 'inherit' }, | ||
| ); | ||
|
|
size-limit report 📦
|
Description
Validation
Validation performed:
Local testing, UI tests
Unit Tests added:
No new functionality for teams-js
Additional Requirements
Change file added:
Yes