Feature/merge costrae#182
Conversation
| }) | ||
| .catch(() => { }) | ||
| } | ||
| const restore = last ? bashPath(last).replace(/"/g, '\\"') : "" |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
In general, when embedding an arbitrary string into a shell command inside double quotes, you must escape both the double quotes themselves and backslashes (and, if desired, other metacharacters) so that the shell interprets the content literally. Relying on .replace for only one character leads to incomplete escaping: a sequence like \" or \ at the end of the string can be interpreted in unexpected ways.
The best targeted fix here is to update the construction of restore on line 964 so that it escapes backslashes before escaping double quotes. We can do this by chaining two .replace calls with global regexes: first replace(/\\/g, '\\\\') to turn each \ into \\, then replace(/"/g, '\\"') to safely embed into a double-quoted string. This preserves existing functionality (double quotes are still escaped) while adding the missing protection. No external libraries are needed; we can use built-in JavaScript string replacement with regular expressions.
Concretely, in packages/opencode/src/tool/bash.ts, change the line
const restore = last ? bashPath(last).replace(/"/g, '\\"') : ""to
const restore = last
? bashPath(last)
.replace(/\\/g, "\\\\")
.replace(/"/g, '\\"')
: ""This ensures any backslashes in the path are preserved correctly when the string is passed to cd "${restore}" ..., preventing malformed commands or potential injection arising from unescaped backslashes.
| @@ -961,7 +961,11 @@ | ||
| }) | ||
| .catch(() => { }) | ||
| } | ||
| const restore = last ? bashPath(last).replace(/"/g, '\\"') : "" | ||
| const restore = last | ||
| ? bashPath(last) | ||
| .replace(/\\/g, "\\\\") | ||
| .replace(/"/g, '\\"') | ||
| : "" | ||
| const note = restore ? ` (restored working directory: ${last})` : "" | ||
| if (restore) { | ||
| await next |
| .catch(() => { }) | ||
| } | ||
| if (last) { | ||
| const restore = bashPath(last).replace(/"/g, '\\"') |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
In general, when embedding arbitrary strings into shell commands, avoid hand-rolled escaping. Prefer using a proper quoting routine or a well-tested library that correctly handles all special characters, including backslashes and quotes. If you must escape manually, ensure that backslashes are escaped before quotes so that an input like \" doesn’t turn into an unescaped quote in the final string.
For this specific case, the smallest fix that preserves existing behavior is to extend the current replacement chain so that it also escapes backslashes. A correct order is: first replace each backslash \ with a double backslash \\, then escape double quotes. Doing it in this order prevents newly added backslashes from interfering with subsequent escaping of quotes. Concretely, in packages/opencode/src/tool/bash.ts, at line 1175, change:
const restore = bashPath(last).replace(/"/g, '\\"')to:
const restore = bashPath(last).replace(/\\/g, "\\\\").replace(/"/g, '\\"')This keeps the rest of the logic intact and ensures both backslashes and double quotes are escaped before the value is interpolated into the cd command. No new imports or helpers are needed.
| @@ -1172,7 +1172,7 @@ | ||
| .catch(() => { }) | ||
| } | ||
| if (last) { | ||
| const restore = bashPath(last).replace(/"/g, '\\"') | ||
| const restore = bashPath(last).replace(/\\/g, "\\\\").replace(/"/g, '\\"') | ||
| await next | ||
| .run({ | ||
| command: `cd "${restore}" 2>/dev/null || true`, |
| } | ||
| const needs = state.cwd ? state.cwd !== expected : true | ||
| if (needs) { | ||
| const restore = bashPath(expected).replace(/"/g, '\\"') |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
In general, when interpolating untrusted or variable paths into shell commands, avoid manual string concatenation and escaping where possible; instead, use APIs that take argument arrays, or use a robust escaping function that correctly handles all relevant meta-characters (including backslashes). If you must escape manually for a double‑quoted string in a POSIX shell, you need to escape \, ", `, and $ at a minimum.
The single best minimal fix here, without changing overall behavior, is to replace the current .replace(/"/g, '\\"') with an escaping function that properly escapes backslashes first and then double quotes (and optionally other special characters) so the result can safely be embedded inside double quotes. Since we must only modify the shown snippet, we can implement a small helper function right above its first use within this file, and use it to compute restore. For example, define escapeForDoubleQuotes that replaces each backslash \ with \\ and each double quote " with \". Then update line 1238 so that restore is computed as escapeForDoubleQuotes(bashPath(expected)). No new imports are needed; we can implement the helper using built-in string methods.
Concretely:
- In
packages/opencode/src/tool/bash.ts, near theensureReadyfunction (before its definition or at least before line 1238), add a local helper:
const escapeForDoubleQuotes = (s: string): string =>
s.replace(/\\/g, "\\\\").replace(/"/g, '\\"')- Replace line 1238 so that it calls this helper instead of calling
.replacedirectly. The rest of theensureReadylogic remains unchanged.
| @@ -1201,6 +1201,9 @@ | ||
| return { ok: true, cwd: normalizeWin(text), error: "" } | ||
| } | ||
|
|
||
| const escapeForDoubleQuotes = (s: string): string => | ||
| s.replace(/\\/g, "\\\\").replace(/"/g, '\\"') | ||
|
|
||
| const ensureReady = async () => { | ||
| const state = { session: null as BashSession | null, notice: "", cwd: "", error: "" } | ||
| const loop = { count: 0 } | ||
| @@ -1235,7 +1238,7 @@ | ||
| } | ||
| const needs = state.cwd ? state.cwd !== expected : true | ||
| if (needs) { | ||
| const restore = bashPath(expected).replace(/"/g, '\\"') | ||
| const restore = escapeForDoubleQuotes(bashPath(expected)) | ||
| await state.session | ||
| .run({ | ||
| command: `cd "${restore}" 2>/dev/null || cd "${restore}" || exit 1`, |
adef4b0 to
befef7c
Compare
…encode into feature/merge-costrae
Issue for this PR
Closes #
Type of change
What does this PR do?
Please provide a description of the issue, the changes you made to fix it, and why they work. It is expected that you understand why your changes work and if you do not understand why at least say as much so a maintainer knows how much to value the PR.
If you paste a large clearly AI generated description here your PR may be IGNORED or CLOSED!
How did you verify your code works?
Screenshots / recordings
If this is a UI change, please include a screenshot or recording.
Checklist
If you do not follow this template your PR will be automatically rejected.