Copilot commented on code in PR #36684:
URL: https://github.com/apache/superset/pull/36684#discussion_r2761670614
##########
superset-frontend/playwright.config.ts:
##########
@@ -117,10 +120,19 @@ export default defineConfig({
// Web server setup - disabled in CI (Flask started separately in workflow)
webServer: process.env.CI
? undefined
- : {
- command: 'curl -f http://localhost:8088/health',
- url: 'http://localhost:8088/health',
- reuseExistingServer: true,
- timeout: 5000,
- },
+ : (() => {
+ // Support custom base URL (e.g., http://localhost:9012/app/prefix/)
+ const baseUrl =
+ process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:8088';
+ // Extract origin (scheme + host + port) for health check
+ // Health endpoint is always at /health regardless of app prefix
+ const healthUrl = new URL('/health', new URL(baseUrl).origin).href;
Review Comment:
The shell command construction uses single quotes to prevent shell injection
via `PLAYWRIGHT_BASE_URL`, but the URL validation is insufficient. An attacker
could still inject shell commands by closing the single quotes and adding
malicious commands.
For example, `PLAYWRIGHT_BASE_URL="http://localhost:8088' ; rm -rf / ; echo
'"` would result in command execution.
Consider using a more secure approach:
1. Validate that `PLAYWRIGHT_BASE_URL` is a valid URL format before using it
2. Use Node.js child_process spawn with an array of arguments instead of
shell command string
3. Or validate that the URL doesn't contain single quotes or other shell
metacharacters
Example validation:
```typescript
const baseUrl = process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:8088';
// Validate URL format
try {
new URL(baseUrl);
} catch {
throw new Error('Invalid PLAYWRIGHT_BASE_URL');
}
// Ensure no shell metacharacters
if (baseUrl.includes("'") || baseUrl.includes(';') || baseUrl.includes('`'))
{
throw new Error('PLAYWRIGHT_BASE_URL contains invalid characters');
}
```
```suggestion
// Validate baseUrl to avoid shell injection via PLAYWRIGHT_BASE_URL
let parsedBaseUrl: URL;
try {
parsedBaseUrl = new URL(baseUrl);
} catch {
throw new Error('Invalid PLAYWRIGHT_BASE_URL');
}
// Ensure no dangerous shell metacharacters are present
if (
baseUrl.includes("'") ||
baseUrl.includes(';') ||
baseUrl.includes('`')
) {
throw new Error('PLAYWRIGHT_BASE_URL contains invalid characters');
}
// Extract origin (scheme + host + port) for health check
// Health endpoint is always at /health regardless of app prefix
const healthUrl = new URL('/health', parsedBaseUrl.origin).href;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
