fix: escape spec-controlled strings in generated template literals and object keys - #3692
Conversation
📝 WalkthroughWalkthroughThis PR hardens generated key, route, server URL, and Zod default output escaping. It updates query invalidation route wiring, adds ChangesGenerated route and key escaping
Zod output escaping
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OpenAPISpec
participant getRoute
participant getBaseUrl
participant getFullRoute
participant GeneratedCode
OpenAPISpec->>getRoute: raw route
getRoute->>getRoute: escape static segments and parse parameters
OpenAPISpec->>getBaseUrl: server URL and variables
getBaseUrl->>getBaseUrl: substitute variables and escape URL
getRoute-->>getFullRoute: escaped route
getBaseUrl-->>getFullRoute: escaped base URL
getFullRoute-->>GeneratedCode: generated route
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
@orval/angular
@orval/axios
@orval/core
@orval/effect
@orval/fetch
@orval/hono
@orval/mcp
@orval/mock
orval
@orval/query
@orval/solid-start
@orval/swr
@orval/zod
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/zod/src/index.ts (1)
636-657: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winObject default keys are not escaped — code injection via spec-controlled defaults.
The PR fixes template-literal injection in string/array defaults via
formatDefaultValue, but the object-default path (lines 631–658) still interpolates raw${key}into generated code. A malicious spec withdefault: { "a: 1}; require('child_process').execSync('id'); const x = {b": "value" }produces valid, executable generated TypeScript:export const fooDefault = { a: 1}; require('child_process').execSync('id'); const x = {b: "value" as const, };This is the same class of injection the PR aims to fix. Apply
JSON.stringify(key)here for consistency with the property-key escaping done at lines 1334, 1687, and 1791.🔒 Proposed fix: escape object default keys via JSON.stringify
const entries = Object.entries(schema.default) .map(([key, value]) => { if (isString(value)) { - return `${key}: ${JSON.stringify(value)} as const`; + return `${JSON.stringify(key)}: ${JSON.stringify(value)} as const`; } if (Array.isArray(value)) { const arrayItems = value.map((item) => isString(item) ? `${JSON.stringify(item)} as const` : `${item}`, ); - return `${key}: [${arrayItems.join(', ')}]`; + return `${JSON.stringify(key)}: [${arrayItems.join(', ')}]`; } if ( value === null || value === undefined || isNumber(value) || isBoolean(value) ) - return `${key}: ${value}`; + return `${JSON.stringify(key)}: ${value}`; }) .join(', ');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zod/src/index.ts` around lines 636 - 657, The object-default generation in the schema default formatter still interpolates raw object keys, which leaves generated TypeScript vulnerable to injection. Update the object-default path in the default-value builder (the code that maps over Object.entries(schema.default)) to escape keys with JSON.stringify, matching the existing property-key escaping used elsewhere in the module. Keep the rest of the value formatting logic in place, and ensure the change is applied wherever default object entries are serialized into code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/getters/route.test.ts`:
- Around line 262-267: The route escaping coverage is incomplete in getRoute
path tests: the current case only exercises backticks, but it should also
explicitly verify a raw ${...} segment is escaped and not re-interpreted as
interpolation. Update the getRoute/getRoutePath test to include a ${evil} path
segment alongside the existing backtick case, and adjust the route
generation/parsing logic so escaped braces from jsesc are preserved as literals
rather than being converted into live params or interpolation.
---
Outside diff comments:
In `@packages/zod/src/index.ts`:
- Around line 636-657: The object-default generation in the schema default
formatter still interpolates raw object keys, which leaves generated TypeScript
vulnerable to injection. Update the object-default path in the default-value
builder (the code that maps over Object.entries(schema.default)) to escape keys
with JSON.stringify, matching the existing property-key escaping used elsewhere
in the module. Keep the rest of the value formatting logic in place, and ensure
the change is applied wherever default object entries are serialized into code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 554cbcf1-8acc-43ae-b40e-15fd1c76b083
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
packages/core/package.jsonpackages/core/src/getters/keys.test.tspackages/core/src/getters/keys.tspackages/core/src/getters/route.test.tspackages/core/src/getters/route.tspackages/query/src/mutation-generator.tspackages/zod/package.jsonpackages/zod/src/index.tspackages/zod/src/zod.test.ts
|
I will review this weekend! Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/getters/route.test.ts (1)
215-228: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake the template-token assertions parity-aware.
(?<!\\)only checks the immediately preceding character, so a token preceded by an even number of backslashes can still slip through. Use a helper that checks backslash parity before`and${instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/getters/route.test.ts` around lines 215 - 228, The escape assertions in getFullRoute’s tests only use a single-character negative lookbehind, so they miss cases where a token is preceded by an even number of backslashes. Update the checks in route.test.ts to use a helper that verifies backslash parity before backticks and ${, and apply it to the existing getFullRoute expectations so the assertions stay correct even with multiple escapes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/getters/route.ts`:
- Around line 37-43: The escaped template-literal check in route parsing is
returning too early in `getRoute`/the brace-scanning logic, which prevents later
valid OpenAPI params from being converted. Update the `${...}` guard so it skips
only the escaped template tag and then continues scanning the remaining suffix,
ensuring legitimate `{param}` segments later in the same path are still
processed by the same route parsing flow.
- Around line 193-201: The route chunk emitter in the splitter/map logic still
treats escaped placeholder text as executable template syntax and only escapes
single quotes in static segments, which can break generated single-quoted
strings. Update the route serialization in the getter around the split/map
handling to treat escaped placeholders as literal text and use a proper
JavaScript string escaper for every non-template chunk before wrapping it in
quotes.
---
Nitpick comments:
In `@packages/core/src/getters/route.test.ts`:
- Around line 215-228: The escape assertions in getFullRoute’s tests only use a
single-character negative lookbehind, so they miss cases where a token is
preceded by an even number of backslashes. Update the checks in route.test.ts to
use a helper that verifies backslash parity before backticks and ${, and apply
it to the existing getFullRoute expectations so the assertions stay correct even
with multiple escapes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85b14228-b1f3-4a97-b061-18953b2d6187
📒 Files selected for processing (2)
packages/core/src/getters/route.test.tspackages/core/src/getters/route.ts
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
@wadakatu if you don't mind reviewing this it would be great these are all security related I would like to release this weekend if possible |
…iterals
Unescaped spec values (servers[].url, path segments, schema defaults)
were baked into generated template literals, allowing code injection via
backtick or ${ in an attacker-controlled OpenAPI spec.
Uses jsesc with quotes: 'backtick' to escape at three boundaries:
- getRoute(): raw OpenAPI path before param processing
- getFullRoute(): resolved server URL after variable substitution
- formatDefaultValue(): schema default values in zod generation
Addresses: GHSA-88f2-fpv8-89q2, GHSA-w727-8j6c-2rj4,
GHSA-3575-w9fc-c2j6, GHSA-2h9g-j24r-h63g, GHSA-8j6p-r8jg-mxqh,
GHSA-p4cg-3328-rvfg
Unescaped schema property and parameter names were emitted as
double-quoted keys in zod.object({...}), allowing computed property key
injection via " in the name.
Replaces "${key}" with JSON.stringify(key) at all 5 render sites.
Addresses: GHSA-6437-gxhq-pqv8, GHSA-653q-5476-x79g, GHSA-6mr6-jvcr-2f25
…eralEscape Unescaped schema property names were wrapped in single quotes by getKey(), allowing computed property key injection via ' in the name in MSW mock output. Wraps the key body with jsStringLiteralEscape before quoting. Addresses: GHSA-2w86-xfrc-g85r
mutation-generator.ts passed a raw spec path prefix to getFullRoute, bypassing getRoute's escaping. Wrap with getRoute to close the gap. Add test proving JSON.stringify-wrapped object keys are safe against backtick injection (backtick is harmless inside double-quoted strings).
- Escape object-default keys with JSON.stringify in zod schema defaults
(same fix as zod.object keys)
- Add test verifying ${globalThis.X} in path segments is not
re-interpreted as interpolation
- Update existing test expectations for quoted object keys
…ingle quotes in getRouteAsArray
getRoutePath: after jsesc escapes ${ to \${, the remaining {evil} was
mistaken for an OpenAPI path param and re-converted to ${evil}. Add
early-return when { is preceded by $.
getRouteAsArray: segments wrapped in single-quoted strings without
escaping '. A spec path containing ' would break out. Now escapes
single quotes at both wrap sites.
…rray
getRoutePath: skip past ${...} block and continue processing remaining
suffix so later {param} segments are still converted.
getRouteAsArray: add (?<!\) to split/match regexes so jsesc-escaped
${...} is treated as literal text, preventing standalone backslash
from breaking single-quote wrapping.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/getters/route.ts`:
- Around line 199-208: Update getRouteAsArray’s single-quoted segment handling
to escape backslashes before escaping single quotes, so raw route inputs such as
foo\bar produce valid string literals. Verify callers passing raw routes,
including Vue/framework route helpers, while preserving template-tag handling
and avoiding double-escaping already processed inputs.
- Around line 37-49: The ${...} detection in getRoutePath must only skip escaped
template expressions, not literal $ before an OpenAPI parameter. Update the
braceIdx guard in getRoutePath to verify the preceding character is a backslash
(matching the jsesc-escaped \${ sequence), while preserving normal /foo${petId}
handling as /foo$${petId}.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4ceb3ea4-3211-4e0f-903b-f605e5c4b821
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
packages/core/package.jsonpackages/core/src/getters/keys.test.tspackages/core/src/getters/keys.tspackages/core/src/getters/route.test.tspackages/core/src/getters/route.tspackages/query/src/mutation-generator.tspackages/zod/package.jsonpackages/zod/src/index.tspackages/zod/src/zod.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/core/src/getters/keys.test.ts
- packages/core/package.json
- packages/zod/package.json
- packages/core/src/getters/route.test.ts
- packages/query/src/mutation-generator.ts
- packages/zod/src/index.ts
- packages/zod/src/zod.test.ts
| // Don't treat ${...} as a path param — OpenAPI params use {param}, not | ||
| // ${param}. After jsesc boundary escaping, ${ becomes \${, but the { is | ||
| // still visible to the regex below and would be misinterpreted as a param. | ||
| // Skip past the ${...} block and continue processing the remaining suffix. | ||
| const braceIdx = path.indexOf('{'); | ||
| if (braceIdx > 0 && path[braceIdx - 1] === '$') { | ||
| const closeIdx = path.indexOf('}', braceIdx); | ||
| if (closeIdx === -1) return path; | ||
| const rest = path.slice(closeIdx + 1); | ||
| return hasParam(rest) | ||
| ? `${path.slice(0, closeIdx + 1)}${getRoutePath(rest)}` | ||
| : path; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
npm i jsesc --no-save >/dev/null 2>&1
node -e '
const jsesc = require("jsesc");
console.log(jsesc("/foo${petId}", { quotes: "backtick", wrap: false }));
'Repository: orval-labs/orval
Length of output: 1005
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant implementation and dependency locations.
git ls-files 'packages/core/src/getters/route.ts' 'packages/core/src/**/*.ts' 'package.json' 'packages/*/package.json' | sed -n '1,120p'
echo
echo '--- route.ts ---'
cat -n packages/core/src/getters/route.ts | sed -n '1,220p'
echo
echo '--- jsesc references ---'
rg -n "jsesc|getRoutePath|getRoute" packages/core/src package.json packages/*/package.jsonRepository: orval-labs/orval
Length of output: 18474
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- route.test.ts around jsesc / ${ coverage ---'
sed -n '250,310p' packages/core/src/getters/route.test.ts | cat -n
echo
echo '--- route-extended.test.ts ---'
sed -n '1,140p' packages/core/src/getters/route-extended.test.ts | cat -n
echo
echo '--- search for literal $ + param cases ---'
rg -n '\$\{petId\}|\\\$\{petId\}|\$.*\{petId\}' packages/core/src/getters/route*.test.ts packages/core/src/getters/route.tsRepository: orval-labs/orval
Length of output: 10392
🌐 Web query:
jsesc template literal escapes ${ to \${ backtick quotes wrap false documentation
💡 Result:
In the jsesc library, when using the 'backtick' option for quotes, the dollar sign character (
Citations:
- 1: https://github.com/mathiasbynens/jsesc/
- 2: https://github.com/mathiasbynens/jsesc/blob/main/README.md
🌐 Web query:
jsesc backtick mode escape ${ sequence \${ exact output
💡 Result:
The jsesc library's 'backtick' quote mode is designed to escape backticks (`) within a string so that the output is safe to use inside a JavaScript template literal [1][2]. Regarding the specific sequence '${', jsesc does not treat this as a special sequence requiring escaping in 'backtick' mode because '${' is not a reserved character or sequence that breaks a standard JavaScript template literal (unlike the backtick itself). When you run: jsesc('${test}', { 'quotes': 'backtick' }); The output is: '${test}' The library escapes characters that would terminate the string or conflict with the specified quote character (in this case, the backtick). Since '$' and '{' do not require escaping to be valid within a template literal, they are returned as-is [1][3].
Citations:
- 1: https://github.com/mathiasbynens/jsesc/
- 2: https://github.com/mathiasbynens/jsesc/blob/main/README.md
- 3: https://github.com/mathiasbynens/javascript-string-escape
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- getRoute usages ---'
rg -n "\bgetRoute\(" packages/core/src
echo
echo '--- surrounding consumers ---'
sed -n '1,220p' packages/core/src/getters/route.test.ts | cat -nRepository: orval-labs/orval
Length of output: 8434
Handle literal $ before a path param separately packages/core/src/getters/route.ts:41-48 The path[braceIdx - 1] === '$' guard also matches real paths like /foo${petId}, so the emitted template drops the literal $ and should stay /foo$${petId} instead of /foo${petId}. Tighten the check to only skip escaped \${...} sequences.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/getters/route.ts` around lines 37 - 49, The ${...}
detection in getRoutePath must only skip escaped template expressions, not
literal $ before an OpenAPI parameter. Update the braceIdx guard in getRoutePath
to verify the preceding character is a backslash (matching the jsesc-escaped \${
sequence), while preserving normal /foo${petId} handling as /foo$${petId}.
There was a problem hiding this comment.
@melloware we might find more edge cases like this because orval uses template literals instead of string concatenation. I think this has to be fixed in v9.
There was a problem hiding this comment.
@aqeelat That's a reasonable call — given the getRoutePath/getRouteAsArray escaping logic increasingly leans on heuristics to distinguish literal $/{/} from generated template syntax, a broader architectural fix (e.g., moving away from raw template-literal string building toward a safer construction method) makes sense to scope for v9 rather than patching heuristics incrementally in this security PR.
Would you like me to open a follow-up issue to track this class of edge cases (literal $ before path params, and similar template-literal ambiguity) for the v9 effort? I can link this PR and comment as context.
ヽ(・ω・)ﾉ
| return [`'${segment.replaceAll("'", "\\'")}'`]; | ||
| } | ||
| // Split by template tags, keeping the delimiters | ||
| // Split by template tags, keeping the delimiters. | ||
| // (?<!\\) prevents matching \${...} (jsesc-escaped) as a template tag. | ||
| return segment | ||
| .split(/(\$\{.+?\})/g) | ||
| .split(/(?<!\\)(\$\{.+?\})/g) | ||
| .filter(Boolean) | ||
| .map((part) => { | ||
| const match = /^\$\{(.+?)\}$/.exec(part); | ||
| return match ? match[1] : `'${part}'`; | ||
| const match = /^(?<!\\)\$\{(.+?)\}$/.exec(part); | ||
| return match ? match[1] : `'${part.replaceAll("'", "\\'")}'`; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all callers and inspect whether the argument is getRoute/getFullRoute output.
rg -nP --type=ts -C3 '\bgetRouteAsArray\s*\(' packagesRepository: orval-labs/orval
Length of output: 5634
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## route.ts\n'
sed -n '1,260p' packages/core/src/getters/route.ts
printf '\n## route docs/tests references\n'
rg -n "getRouteAsArray|getRoute|getFullRoute" packages/core/src/getters/route.ts packages/core/src/getters/route*.test.ts packages/query/src -C 2
printf '\n## direct call sites and surrounding context\n'
sed -n '170,210p' packages/query/src/frameworks/vue.ts
sed -n '60,95p' packages/query/src/frameworks/index.ts
sed -n '350,390p' packages/query/src/mutation-generator.tsRepository: orval-labs/orval
Length of output: 29810
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## getQueryKeyRouteString callers\n'
rg -n "getQueryKeyRouteString\(" packages -C 3
printf '\n## route construction around query generator\n'
sed -n '300,430p' packages/query/src/mutation-generator.ts
printf '\n## route construction around framework adapter usage\n'
sed -n '1,220p' packages/query/src/framework-adapter.tsRepository: orval-labs/orval
Length of output: 15604
Escape backslashes for raw getRouteAsArray inputs, or document a pre-escaped-only contract.
packages/query/src/frameworks/vue.ts and packages/query/src/frameworks/index.ts pass raw routes here, while packages/query/src/mutation-generator.ts passes getFullRoute(getRoute(...), ...). That means the single-quoted branch can still see raw \ from spec paths, so a segment like foo\bar would emit a malformed string literal.
🧰 Tools
🪛 OpenGrep (1.23.0)
[ERROR] 207-207: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/getters/route.ts` around lines 199 - 208, Update
getRouteAsArray’s single-quoted segment handling to escape backslashes before
escaping single quotes, so raw route inputs such as foo\bar produce valid string
literals. Verify callers passing raw routes, including Vue/framework route
helpers, while preserving template-tag handling and avoiding double-escaping
already processed inputs.
|
@coderabbitai use the assertive profile when reviewing this PR |
|
reviews:
profile: "assertive"Once that's set, I'll pick it up automatically for future reviews on this PR (and repo-wide). If you'd like, I can kick off a fresh full review right now once the config change is in place — just let me know and I'll trigger ``. Given this PR touches security-sensitive escaping logic (jsesc, template literals, computed keys), the assertive profile should help surface more edge cases for ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/getters/route.ts (1)
126-147: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEscape server URLs before embedding in backticks
jsesc(..., { quotes: 'backtick' })leaves${...}intact, so a spec-controlled server URL can still become template-literal interpolation in the generated client. Escape${here too, or keep these URLs out of backtick strings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/getters/route.ts` around lines 126 - 147, Escape every server URL’s template interpolation markers before returning from the URL resolution logic in the route getter, including both the no-variables branch and the resolved `url` branch. Update the `jsesc` handling around the server URL so `${` cannot be interpreted as template-literal interpolation in generated clients, while preserving existing URL escaping and variable substitution behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/core/src/getters/route.ts`:
- Around line 126-147: Escape every server URL’s template interpolation markers
before returning from the URL resolution logic in the route getter, including
both the no-variables branch and the resolved `url` branch. Update the `jsesc`
handling around the server URL so `${` cannot be interpreted as template-literal
interpolation in generated clients, while preserving existing URL escaping and
variable substitution behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee66daf9-11d9-4079-94f8-3db651000609
📒 Files selected for processing (2)
packages/core/src/getters/route.test.tspackages/core/src/getters/route.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/getters/route.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/getters/route.ts`:
- Around line 36-54: Update esc and getRoutePath to explicitly neutralize every
${ sequence before generated route strings are returned; jsesc backtick escaping
is insufficient. Ensure both the ${...} branch and all ordinary path fragments
pass through the same escaping logic, while preserving legitimate {param}
processing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fedff305-3877-4267-99e0-c03328d5ea9a
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
packages/core/package.jsonpackages/core/src/getters/keys.test.tspackages/core/src/getters/keys.tspackages/core/src/getters/route.test.tspackages/core/src/getters/route.tspackages/query/src/mutation-generator.tspackages/zod/package.jsonpackages/zod/src/index.tspackages/zod/src/zod.test.ts
| const esc = (str: string) => jsesc(str, { quotes: 'backtick', wrap: false }); | ||
|
|
||
| const getRoutePath = (path: string): string => { | ||
| // Don't treat ${...} as an OpenAPI path param — the $ makes it literal text, | ||
| // not a {param} template. Escape the ${...} block and continue processing | ||
| // any legitimate {param} segments after it. | ||
| const braceIdx = path.indexOf('{'); | ||
| if (braceIdx > 0 && path[braceIdx - 1] === '$') { | ||
| const closeIdx = path.indexOf('}', braceIdx); | ||
| if (closeIdx === -1) return esc(path); | ||
| const before = esc(path.slice(0, closeIdx + 1)); | ||
| const rest = path.slice(closeIdx + 1); | ||
| return hasParam(rest) | ||
| ? `${before}${getRoutePath(rest)}` | ||
| : `${before}${esc(rest)}`; | ||
| } | ||
|
|
||
| const matches = /([^{]*){?([\w*_-]*)}?(.*)/.exec(path); | ||
| if (!matches?.length) return path; // impossible due to regexp grouping here, but for TS | ||
| if (!matches?.length) return esc(path); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify jsesc v3 behavior with ${ in backtick mode
npm i jsesc@^3.0.0 --no-save >/dev/null 2>&1
node -e '
const jsesc = require("jsesc");
const inputs = ["${evil}", "foo${bar}", "price$", "back`tick"];
for (const s of inputs) {
const out = jsesc(s, { quotes: "backtick", wrap: false });
console.log(JSON.stringify(s), "=>", JSON.stringify(out), "hasUnescaped${:", /(?<!\\)\$\{/.test(out));
}
'Repository: orval-labs/orval
Length of output: 1005
🌐 Web query:
jsesc backtick mode ${ escaping documentation source
💡 Result:
In the jsesc library, the 'backtick' mode is specifically designed to prepare strings for inclusion within JavaScript template literals (which use backticks). When you set the 'quotes' option to 'backtick', jsesc escapes any existing backticks () in the input string as \ [1][2]. Regarding the ${ sequence, it is important to note that jsesc does not specifically target or "escape" the ${ sequence itself as a special character combination. Template literals in JavaScript treat ${ as the start of an expression interpolation [3]. Because jsesc focuses on character-level escaping for string literals, the ${ sequence remains intact in the output. If you are placing the output of jsesc inside a template literal where you do not want ${ to be interpreted as an interpolation, you must manually handle or escape the $ sign or the curly braces according to your specific template usage, as jsesc's backtick mode only ensures the backticks themselves are safe [1][4]. Example usage for backtick mode: jsesc('Lorem ipsum ${variable}', { 'quotes': 'backtick' }); // Result: \\Lorem\ ipsum ${variable} [1][2]
Citations:
- 1: https://github.com/mathiasbynens/jsesc/
- 2: https://github.com/mathiasbynens/jsesc
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals
- 4: https://www.pkgstats.com/pkg:jsesc
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== route.ts around the helper =="
sed -n '1,120p' packages/core/src/getters/route.ts | cat -n
echo
echo "== search for jsesc-related tests and ${ assertions =="
rg -n "jsesc|\\$\\{|backtick|esc\\(" packages/core test . -g '!**/node_modules/**' || trueRepository: orval-labs/orval
Length of output: 4802
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files mentioning getRoutePath/getRoute/esc =="
rg -n "getRoutePath|getRoute\\(|jsesc\\(|\\$\\{" packages/core -g '!**/node_modules/**'
echo
echo "== tests around route generation =="
fd -a 'route' packages/core test . -t f | sed -n '1,40p'Repository: orval-labs/orval
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '210,320p' packages/core/src/getters/route.test.ts | cat -nRepository: orval-labs/orval
Length of output: 4286
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== jsesc version pin =="
rg -n '"jsesc"|jsesc@' package.json package-lock.json pnpm-lock.yaml yarn.lock packages -g '!**/node_modules/**' || trueRepository: orval-labs/orval
Length of output: 446
packages/core/src/getters/route.ts:36-54 — Escape ${ before generating template literals
jsesc in backtick mode does not neutralize ${...}, so spec-controlled paths can still turn into live interpolation in the generated route string. Escape ${ explicitly here, or post-process the escaped output before returning.
🧰 Tools
🪛 OpenGrep (1.23.0)
[ERROR] 53-53: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/getters/route.ts` around lines 36 - 54, Update esc and
getRoutePath to explicitly neutralize every ${ sequence before generated route
strings are returned; jsesc backtick escaping is insufficient. Ensure both the
${...} branch and all ordinary path fragments pass through the same escaping
logic, while preserving legitimate {param} processing.
|
@coderabbitai Both branches (lines 127 and 147) already call |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline packages/core/src/getters/route.ts --items all --type function --match 'getBaseUrl|esc'
printf '\n--- getBaseUrl implementation ---\n'
rg -n -A55 -B8 'function getBaseUrl|const getBaseUrl|export const getBaseUrl' packages/core/src/getters/route.ts
printf '\n--- jsesc dependency ---\n'
rg -n -C2 '"jsesc"' packages/core/package.jsonLength of output: 5154
✏️ Learnings added
|
Summary
Fixes 10 draft security advisories caused by unescaped spec-controlled strings baked into generated code. All fixes use
jsesc(the same library Babel uses for code generation) orJSON.stringify/jsStringLiteralEscapedepending on the output context.Three attack surfaces addressed
1. URL template literals
servers[].urland path segments were baked into generated template literals without escaping backtick or${. A malicious OpenAPI spec could inject arbitrary code that executes when the generated client is called.Fix:
jsescwithquotes: 'backtick'at two boundaries in@orval/core:getRoute()— escapes raw OpenAPI path before param processinggetFullRoute()— escapes resolved server URL after variable substitutionAlso fixes
mutation-generator.tswhich passed a raw spec path prefix togetFullRoute, bypassinggetRoute's escaping.2. Zod schema defaults
Schema
defaultvalues were emitted as backtick template literals (export const xDefault = \${value}`) without escaping. A malicious default likev${globalThis.X()}w` would execute at import time.Fix: New
formatDefaultValue()in@orval/zodusesjsescwithquotes: 'backtick', wrap: truefor string and array-of-string defaults.3. Computed property key injection
Schema property/parameter names were emitted as object keys without escaping quotes. A
"or'in the name could break out and inject a computed property key[expr]that executes at import/call time.Fix:
zod.object({...})keys:JSON.stringify(key)at 5 render sites (escapes"and wraps in double quotes)jsStringLiteralEscapeingetKey()(escapes'and\in single-quoted keys)Test coverage
${injection in server URLs, path segments, string defaults, array defaults, enum defaultsDependency
Adds
jsesc(^3.0.0) as a direct dependency of@orval/coreand@orval/zod.Follow-up (separate PR)
The
getRoute/getFullRouteescaping contract is documented but not type-enforced. A follow-up PR will introduce aSafeRoutebranded type so the compiler prevents accidental double-escaping or bypassed escaping.Summary by CodeRabbit
${…}sequences)..default(...)code generation with safer string/default handling and standardized JSON-quoted object property keys.jsescruntime + type support in core and Zod for consistent escaping.