Fix code injection via unescaped spec-derived strings in generated clients - #2808
Open
carfeii wants to merge 1 commit into
Open
Fix code injection via unescaped spec-derived strings in generated clients#2808carfeii wants to merge 1 commit into
carfeii wants to merge 1 commit into
Conversation
…ients
Several Handlebars templates interpolate OpenAPI-spec-derived strings
directly into a single-quoted JS string literal in the generated output,
with no escaping:
- exportService.hbs: the request path (`url: '{{{path}}}'`), each path/
query/header/cookie parameter's wire name (`'{{{prop}}}': ...`), the
request-body media type, and the response header name.
- client.hbs / core/OpenAPI.hbs: the first server URL and the API version
(`BASE: '{{{server}}}'`, `VERSION: '{{{version}}}'`), both emitted into
core/OpenAPI.ts, which every generated service file imports.
None of `path` (openApi.paths key, getServices.ts), `server`
(openApi.servers[0].url, getServer.ts), `version` (openApi.info.version),
or `prop` (parameter.name, getOperationParameter.ts/
getOperationRequestBody.ts) are sanitized before reaching these templates.
A single quote in any of them closes the string literal early; the
remainder becomes live JS. Two PoCs:
- A path of `/users/'+require('fs').writeFileSync('/tmp/pwned','x')+'`
generates a service method that runs the injected code every time it's
called (`__request(OpenAPI, { url: '<injected>' })` is a fresh
expression evaluated on each call).
- A `servers[0].url` of `https://x'+require('fs').writeFileSync(...)+'`
generates `core/OpenAPI.ts` (always emitted, always imported by every
service file) with the injected code in a module-level `const`
assignment, so it runs on *import*, before any generated method is
even called.
Both verified against this exact codebase: generated the client, compiled
it with tsc, and confirmed the injected code executed via a marker file
written to disk (a plain module `require()` for the second case, no
network call or method invocation needed).
Fix: escape backslash and single-quote characters (registerHandlebarHelpers.ts's
new `escapeSingleQuotedString` helper, registered as a known Handlebars
helper in rollup.config.mjs) before emitting `path`, `server`, `version`,
`prop`, the request-body media type, and the response header name into
their single-quoted string literals. Re-ran both PoCs against the fixed
build: the payload now round-trips as inert string data (the generated
`fetch()` call fails cleanly on the resulting garbage URL; the module
imports without executing anything).
Deliberately scoped to the sinks I traced end-to-end to a raw,
unsanitized spec field; did not touch `pattern` (already escaped in
getPattern.ts) or the enum `name`/`value` fields (already sanitized to a
safe identifier / pre-quoted-and-escaped in getEnum.ts), to avoid
double-escaping regressions on fields that are already safe.
Full unit suite (`npm test`): 50/50 suites, 80/80 tests, 285/285
snapshots, unchanged from before this change - the escaping is a no-op
for every existing (benign) fixture. E2E suite: the browser-driven specs
(angular/xhr/fetch/babel) all fail identically before and after this
change in this sandbox with "No usable sandbox" (no Chrome sandbox
available in this container, unrelated to the fix); no test failure in
either run comes from an assertion, only from the browser launch itself.
Author
|
Fixes #2809. |
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
1 participant
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.
See the linked issue for the vulnerability report and impact.
Root cause
Several templates interpolate OpenAPI-spec-derived strings directly into a single-quoted JS string literal in the generated output, with no escaping: the request path (
url: '{{{path}}}'inexportService.hbs), each path/query/header/cookie parameter's wire name, the request-body media type, the response header name, and the first server URL / API version (client.hbs,core/OpenAPI.hbs). None of the corresponding parser values (getServices.ts'surl,getServer.ts's return value,openApi.info.version,getOperationParameter.ts'sparameter.name) are sanitized before reaching these templates. A single quote in any of them closes the string literal early and the remainder is evaluated as live JS.Fix
escapeSingleQuotedStringHandlebars helper (registerHandlebarHelpers.ts), registered as a known helper inrollup.config.mjs.path,server,version,prop(parameter name), request-bodymediaType,responseHeader.pattern(already escaped ingetPattern.ts) or the enumname/valuefields (already sanitized/pre-escaped ingetEnum.ts), to avoid double-escaping a value that's already safe.Testing
npm test: 50/50 suites, 80/80 tests, 285/285 snapshots pass, identical to before this change (the new escaping is a no-op for every existing benign fixture).npm run test:e2e: the browser-driven specs (angular/xhr/fetch/babel) fail identically before and after this change in the sandbox I used ("No usable sandbox", no Chrome sandbox available in this container) - not a regression from this change, every failure is a browser-launch error, not a test assertion failure.