From a82e2b9d7c273db12c7e870f755d953e8b1202e2 Mon Sep 17 00:00:00 2001 From: carfeii Date: Fri, 4 Sep 2026 14:23:56 +0800 Subject: [PATCH] Fix code injection via unescaped spec-derived strings in generated clients 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: '' })` 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. --- rollup.config.mjs | 1 + src/templates/client.hbs | 8 ++++---- src/templates/core/OpenAPI.hbs | 4 ++-- src/templates/exportService.hbs | 16 ++++++++-------- src/utils/registerHandlebarHelpers.ts | 18 ++++++++++++++++++ 5 files changed, 33 insertions(+), 14 deletions(-) diff --git a/rollup.config.mjs b/rollup.config.mjs index b25ca8442..968c34fa7 100644 --- a/rollup.config.mjs +++ b/rollup.config.mjs @@ -38,6 +38,7 @@ const handlebarsPlugin = () => ({ enumerator: true, escapeComment: true, escapeDescription: true, + escapeSingleQuotedString: true, camelCase: true, }, }); diff --git a/src/templates/client.hbs b/src/templates/client.hbs index 601f27d8f..49e9a58bf 100644 --- a/src/templates/client.hbs +++ b/src/templates/client.hbs @@ -27,8 +27,8 @@ import { {{{name}}}{{{@root.postfix}}} } from './services/{{{name}}}{{{@root.pos { provide: OpenAPI, useValue: { - BASE: OpenAPI?.BASE ?? '{{{server}}}', - VERSION: OpenAPI?.VERSION ?? '{{{version}}}', + BASE: OpenAPI?.BASE ?? '{{{escapeSingleQuotedString server}}}', + VERSION: OpenAPI?.VERSION ?? '{{{escapeSingleQuotedString version}}}', WITH_CREDENTIALS: OpenAPI?.WITH_CREDENTIALS ?? false, CREDENTIALS: OpenAPI?.CREDENTIALS ?? 'include', TOKEN: OpenAPI?.TOKEN, @@ -61,8 +61,8 @@ export class {{{clientName}}} { constructor(config?: Partial, HttpRequest: HttpRequestConstructor = {{{httpRequest}}}) { this.request = new HttpRequest({ - BASE: config?.BASE ?? '{{{server}}}', - VERSION: config?.VERSION ?? '{{{version}}}', + BASE: config?.BASE ?? '{{{escapeSingleQuotedString server}}}', + VERSION: config?.VERSION ?? '{{{escapeSingleQuotedString version}}}', WITH_CREDENTIALS: config?.WITH_CREDENTIALS ?? false, CREDENTIALS: config?.CREDENTIALS ?? 'include', TOKEN: config?.TOKEN, diff --git a/src/templates/core/OpenAPI.hbs b/src/templates/core/OpenAPI.hbs index 7b9560a26..e027349c8 100644 --- a/src/templates/core/OpenAPI.hbs +++ b/src/templates/core/OpenAPI.hbs @@ -18,8 +18,8 @@ export type OpenAPIConfig = { }; export const OpenAPI: OpenAPIConfig = { - BASE: '{{{server}}}', - VERSION: '{{{version}}}', + BASE: '{{{escapeSingleQuotedString server}}}', + VERSION: '{{{escapeSingleQuotedString version}}}', WITH_CREDENTIALS: false, CREDENTIALS: 'include', TOKEN: undefined, diff --git a/src/templates/exportService.hbs b/src/templates/exportService.hbs index d6bccbbeb..fd03b9ea0 100644 --- a/src/templates/exportService.hbs +++ b/src/templates/exportService.hbs @@ -88,39 +88,39 @@ export class {{{name}}}{{{@root.postfix}}} { {{/equals}} {{/if}} method: '{{{method}}}', - url: '{{{path}}}', + url: '{{{escapeSingleQuotedString path}}}', {{#if parametersPath}} path: { {{#each parametersPath}} - '{{{prop}}}': {{{name}}}, + '{{{escapeSingleQuotedString prop}}}': {{{name}}}, {{/each}} }, {{/if}} {{#if parametersCookie}} cookies: { {{#each parametersCookie}} - '{{{prop}}}': {{{name}}}, + '{{{escapeSingleQuotedString prop}}}': {{{name}}}, {{/each}} }, {{/if}} {{#if parametersHeader}} headers: { {{#each parametersHeader}} - '{{{prop}}}': {{{name}}}, + '{{{escapeSingleQuotedString prop}}}': {{{name}}}, {{/each}} }, {{/if}} {{#if parametersQuery}} query: { {{#each parametersQuery}} - '{{{prop}}}': {{{name}}}, + '{{{escapeSingleQuotedString prop}}}': {{{name}}}, {{/each}} }, {{/if}} {{#if parametersForm}} formData: { {{#each parametersForm}} - '{{{prop}}}': {{{name}}}, + '{{{escapeSingleQuotedString prop}}}': {{{name}}}, {{/each}} }, {{/if}} @@ -132,11 +132,11 @@ export class {{{name}}}{{{@root.postfix}}} { body: {{{parametersBody.name}}}, {{/equals}} {{#if parametersBody.mediaType}} - mediaType: '{{{parametersBody.mediaType}}}', + mediaType: '{{{escapeSingleQuotedString parametersBody.mediaType}}}', {{/if}} {{/if}} {{#if responseHeader}} - responseHeader: '{{{responseHeader}}}', + responseHeader: '{{{escapeSingleQuotedString responseHeader}}}', {{/if}} {{#if errors}} errors: { diff --git a/src/utils/registerHandlebarHelpers.ts b/src/utils/registerHandlebarHelpers.ts index 88f47c19b..49b27c1e3 100644 --- a/src/utils/registerHandlebarHelpers.ts +++ b/src/utils/registerHandlebarHelpers.ts @@ -101,6 +101,24 @@ export const registerHandlebarHelpers = (root: { return value.replace(/\\/g, '\\\\').replace(/`/g, '\\`').replace(/\${/g, '\\${'); }); + // Several templates interpolate spec-derived strings (the request path, + // a server URL, a parameter/header/media-type name, a schema + // type/format/pattern) directly into a single-quoted JS string literal, + // e.g. `url: '{{{path}}}'`. Without escaping, a single quote in that + // value closes the string literal early and the remainder is evaluated + // as live JS: a spec path of `/x'+require('fs').writeFileSync(...)+'` + // becomes `url: '/x'+require('fs').writeFileSync(...)+'',`, which + // executes whenever the generated code runs (as soon as the containing + // module is imported, for module-level fields like a server URL, or on + // every call, for per-request fields like the path). Escape backslashes + // and single quotes so the value can only ever be interpreted as string + // data. + Handlebars.registerHelper('escapeSingleQuotedString', function (value: unknown): string { + return String(value ?? '') + .replace(/\\/g, '\\\\') + .replace(/'/g, "\\'"); + }); + Handlebars.registerHelper('camelCase', function (value: string): string { return camelCase(value); });