Repository navigation
fix(swagger-ui): preserve $ characters when building swagger-ui-init.js - #4119
kamilmysliwiec merged 2 commits into
Conversation
Pass the swagger-ui-init.js options through a replace callback so $`, $&, $', $n, and $$ in OpenAPI JSON (especially descriptions) are inserted verbatim. This matches the existing buildSwaggerHTML and buildJSInitOptions fixes. Co-authored-by: Christian Angelo M Sulit <csulit@users.noreply.github.com>
|
Friendly bump — CI is green on this. It aligns Happy to adjust if maintainers prefer a different approach. Thanks for taking a look. |
micalevisk
left a comment
There was a problem hiding this comment.
Thanks for the PR! LGTM — approving with a couple of non-blocking nits inline.
Verification
- Bug reproduced. I checked out this branch, reverted only the one-line fix, and the new test fails exactly as described: the
$`in the description splices the template prefix into the serialized document, sowindow.onload = function() {…restarts in the middle of the JSON andswagger-ui-init.jsno longer parses. With the fix restored the test passes. - No regressions. Locally on
4cde8e0: unit tests 519/519, e2e 133/133,tsc -p tsconfig.build.jsonclean. CI is green as well. - Not a breaking change. The output is byte-identical for any document that has no
$replacement pattern in it; for documents that do, the previous output was a corrupted script, so nothing could have been relying on it.
Correctness / patterns
- The
() => valuereplacer is the same approach already merged for this bug class in #3896 (buildJSInitOptions) and #3946 (buildSwaggerHTML), so this is consistent with the codebase. I confirmed both references in the description are accurate. - This was the last string-overload
.replace()with user-controlled input underlib/swagger-ui/, so the swagger-ui rendering path is now fully covered. - Test quality is good: asserting on
JSON.stringify(description), compiling withnew Script()(compile-only, nothing is executed) and checking thatwindow.onloadshows up once covers the three distinct ways this breaks.
History
I looked for earlier attempts at this. There is no open issue to link (so Issue Number: N/A is right), no competing open PR, and no previous PR/issue on $ replacement patterns that was closed as "won't fix" / "not planned" — the two earlier PRs for this same class of bug were merged without objection.
Possible follow-up (out of scope here)
lib/plugin/utils/plugin-utils.ts (replaceImportPath) does typeReference.replace(importPath, relativePath) where the replacement is a filesystem-derived string, so it is technically the same bug class. It would only trigger for a path containing $&, $$, $` or $', which is very unlikely in practice, so this is low priority and should not block this PR.
Unrelated to this PR: prettier --check flags the import { … } from './constants.js' line in lib/swagger-ui/swagger-ui.ts, but that line is already unformatted on master, so no action needed here.
| // The serialized OpenAPI document is user-controlled (e.g. descriptions). | ||
| // Pass the replacement through a callback rather than as a string so that | ||
| // `$$`, `$&`, "$`", "$'" and `$1`–`$9` sequences are inserted verbatim | ||
| // instead of being interpreted as replacement patterns (which would silently | ||
| // corrupt swagger-ui-init.js). |
There was a problem hiding this comment.
nit (non-blocking): this is now the third copy of nearly the same 5-line explanation (helpers.ts, buildSwaggerHTML below, and here). Since the full rationale already lives a few lines down in this same file, we could keep this one short and point to it. This also drops the non-ASCII en dash in `$1`–`$9` (the sibling comments use a plain -).
| // The serialized OpenAPI document is user-controlled (e.g. descriptions). | |
| // Pass the replacement through a callback rather than as a string so that | |
| // `$$`, `$&`, "$`", "$'" and `$1`–`$9` sequences are inserted verbatim | |
| // instead of being interpreted as replacement patterns (which would silently | |
| // corrupt swagger-ui-init.js). | |
| // `jsInitOptions` embeds the user-controlled OpenAPI document, so it is passed | |
| // through a callback for the same reason as in `buildSwaggerHTML` below: "$" | |
| // replacement patterns must be inserted verbatim. |
|
|
||
| expect(script).toContain(JSON.stringify(description)); | ||
| expect(() => new Script(script)).not.toThrow(); | ||
| expect(script.match(/window\.onload/g)).toHaveLength(1); |
There was a problem hiding this comment.
optional (non-blocking): customOptions.swaggerOptions is serialized into the same <% swaggerOptions %> placeholder, so it was affected by the same bug and is fixed by the same line. A second case would lock that in too, e.g.:
it('inserts swaggerOptions values with "$" replacement-pattern characters verbatim', () => {
const name = "v1 $& $' $` $$";
const script = buildSwaggerInitJS(
{ openapi: '3.0.0', info: { title: 'Test', version: '1.0.0' }, paths: {} },
{ swaggerOptions: { urls: [{ url: '/docs-json', name }] } }
);
expect(script).toContain(JSON.stringify(name));
expect(() => new Script(script)).not.toThrow();
});Fine to leave as is — the existing test already fails without the fix.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
buildSwaggerInitJSinjects the serialized OpenAPI document intoswagger-ui-init.jswith:String-form
String.prototype.replaceinterprets$`` /$&/$'/$n/$$in the replacement. A response description such as ``^[a-z][a-z0-9_]{1,63}$`` therefore corrupts the generated script (SyntaxErrorin the browser), so/docsrenders blank while/docs-json` stays valid.This is the same class of bug already fixed for
buildSwaggerHTMLin #3946 and forbuildJSInitOptionsin #3896. The HTML path in this file already uses() => valuereplacers for that reason; the init-JS path was left on the string overload.Issue Number: N/A
What is the new behavior?
buildSwaggerInitJSnow passes a callback replacer so$sequences in the OpenAPI JSON (especially descriptions) are inserted verbatim.swagger-ui-init.jsremains syntactically valid and the template does not restart mid-document.Does this PR introduce a breaking change?
Other information
Regression coverage in
test/swagger-ui/swagger-ui.spec.tsfeeds a document whose description contains$``,$&,$',$1, and$$`, then asserts:JSON.stringify(description))new Script(script)fromnode:vm)window.onloadappears once (template does not restart)