fix: address all 5 open CodeQL code scanning alerts - #3680
Merged
Conversation
Add explicit least-privilege permissions block to the repo-gardening job to satisfy the principle of least privilege and silence CodeQL alert #1 (actions/missing-workflow-permissions). The job only needs to read contents and write to issues and pull-requests; project board and Slack writes go through dedicated secrets that are unaffected.
Replace String.includes() host checks with proper URL parsing so that attacker-controlled strings like 'http://evil.com/playground.wordpress.net' cannot bypass the isPlaygroundWeb / isPlaygroundCli guards.
Pass the script string as an argument to the shell binary rather than interpolating it directly into the exec command. This prevents shell metacharacters in environment-derived values (e.g. paths with spaces) from inadvertently altering the meaning of the command.
…(CodeQL #64) Replace exec() + interpolated command strings with execFile() + explicit args arrays so that paths derived from __dirname (e.g. repoRoot, script paths) are never interpreted by a shell. This prevents breakage or injection when paths contain spaces or shell metacharacters.
…odeQL #70) Split the chained replace() calls into two explicit steps and add a comment explaining that backslashes are intentionally converted to forward slashes first, so no backslash can remain to corrupt a subsequent \" escape sequence. This makes the intent clear and resolves the CodeQL incomplete-sanitization false-positive (js/incomplete-sanitization).
![github-advanced-security[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…th (CodeQL #70) Replace the two chained .replace() calls with a single-pass regex that handles both \ and " in one substitution. This eliminates any ordering ambiguity between the two transforms and resolves the CodeQL js/incomplete-sanitization alert: there is now no intermediate state where a newly introduced backslash could interact with a not-yet-processed double-quote character.
Collaborator
📊 Performance Test ResultsComparing f9adb20 vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
…deQL #64) On Windows, npm is a .cmd batch file and cannot be spawned directly by execFile without shell resolution. Adding shell:true restores that while still passing arguments as an array (not an interpolated string), which is sufficient to prevent shell metacharacter injection in path values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses all 5 open CodeQL code scanning alerts from the security tab.
https://github.com/Automattic/studio/security/code-scanning/1 — Missing workflow permissions (
.github/workflows/gardening.yml): Add explicitpermissionsblock (contents: read,issues: write,pull-requests: write) to therepo-gardeningjob. Follows the principle of least privilege, especially important given the use ofpull_request_target.https://github.com/Automattic/studio/security/code-scanning/49 — Incomplete URL substring check (
tools/benchmark-site-editor/benchmark.ts): ReplaceString.includes('playground.wordpress.net')withnew URL(url).hostname === 'playground.wordpress.net'so that attacker-controlled strings likehttp://evil.com/playground.wordpress.netcan't bypass the guard.https://github.com/Automattic/studio/security/code-scanning/44 — Shell command injection via
exec(tools/compare-perf/utils.ts): ReplacechildProcess.exec(script)withchildProcess.execFile(shell, ['-c', script])so the script string is passed as an argument to the shell rather than interpolated into a shell command. Preserves the existing string-based API for callers that pass multi-word commands.https://github.com/Automattic/studio/security/code-scanning/64 — Shell command injection via path interpolation (
apps/studio/forge.config.ts): Replace theexec(commandString)helper withexecFile(cmd, args[])accepting an explicit args array. All call sites updated to pass['npx', 'tsx', scriptPath, ...args]style arrays — paths are never shell-parsed.https://github.com/Automattic/studio/security/code-scanning/70 — Incomplete string escaping in
toPhpIniPath(apps/cli/lib/native-php/config.ts): Replace the two chained.replace()calls with a single-pass[\\"]/gregex that handles both\and"in one substitution. This removes any ordering ambiguity between the two transforms and gives CodeQL a clear, unambiguous implementation to analyse.Test plan
npm test)