fix(swagger): context-correct escaping for attribute and JS string values - #506
Open
FumingPower3925 wants to merge 3 commits into
Open
fix(swagger): context-correct escaping for attribute and JS string values#506FumingPower3925 wants to merge 3 commits into
FumingPower3925 wants to merge 3 commits into
Conversation
…g values The generated UI pages interpolated developer configuration (SpecURL, OAuth2 fields, DocExpansion) with Go's %q, which is the wrong escaper for both embedding contexts it was used in: - HTML attribute value (Scalar `data-url`): %q escapes `"` as `\"`, which HTML does not honour, and leaves `&` raw, so a SpecURL containing `"` terminates the attribute. Now rendered as `data-url="%s"` with html.EscapeString. - JavaScript string literal inside an inline <script> (Swagger UI `url:`/`docExpansion:`/`oauth2RedirectUrl:`/initOAuth fields and `Redoc.init(...)`): %q leaves `</script>` intact (terminating the block) and emits escapes JS lacks (`\a`, `\x`, `\U`). Now rendered via json.Marshal, which escapes `<`, `>`, `&`, U+2028 and U+2029. The existing html.EscapeString(json.Marshal(options)) for the Scalar data-configuration attribute is unchanged. Ordinary inputs render byte-identically (verified by diffing the served pages for seven plain configs across all three renderers, with and without AssetsPath). Not exploitable today (all values are startup configuration and the page is built once in New()); this is hardening surfaced by the CodeQL go/unsafe-quoting triage. Regression tests build every renderer with a hostile value containing `"`, `'`, `</script>`, `&`, BEL and U+2028, with and without AssetsPath, and assert that script tags stay balanced, the data-url attribute holds no raw quotes/angle brackets and unescapes to the original, and each JS string literal is valid JSON that round-trips to the original value. Fixes #504
CodeQL flagged the fmt.Sprintf page builders three times (go/unsafe-quoting #12/#13/#14, all false positives: the interpolated values were already escaped by json.Marshal or html.EscapeString, which CodeQL does not model as quote sanitizers). Rather than dismiss the alerts, remove the pattern: the Swagger UI, Scalar and ReDoc pages are now html/template templates, so the context-correct escaper is chosen by the template engine - RCDATA for <title>, URL attribute for href/src/data-url, plain attribute for data-configuration, JSON string literal inside <script>. The hand-rolled jsString helper and every quoted %s placeholder are gone; marshalOptions remains only to produce the JSON text for the Scalar attribute, and ReDoc's options map is marshalled by html/template itself. Bool and int literals (deepLinking, persistAuthorization, defaultModelsExpandDepth, usePkceWithAuthorizationCodeGrant) are passed as template.JS so html/template does not space-pad them; they are never configuration strings. Rendering 648 ordinary configurations (3 renderers x 4 asset sources x 3 spec sources x 3 option sets x 6 UI configs) with the previous and the new builder yields byte-identical pages. One behaviour is stricter: Scalar's data-url is a URL-typed attribute to html/template, so characters invalid in a URL are percent-encoded and schemes other than http/https are neutralised. TestScalarEscapesDataURLAttribute now decodes HTML entities then percent-encoding (what the browser does before fetching), TestScalarNeutralisesUnsafeSpecURLScheme pins the scheme rule and Config.SpecURL documents it.
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
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.
What
middleware/swagger/swagger.gointerpolated developer configuration (SpecURL,UI.Title,UI.DocExpansion,UI.OAuth2RedirectURL, theOAuth2fields,Options,AssetsPath) into the generated UI pages withfmt.Sprintf, using Go's%qfor the attribute and JS-string values.%qis the wrong escaper for both embedding contexts. This PR replaces the three hand-built pages withhtml/template— Go's context-aware escaper — so every value is escaped for the exact context it lands in:html/template)<title>html.EscapeStringhref/srcasset URLshtml.EscapeString(AssetsPath only)data-url%qdata-configurationhtml.EscapeString(json.Marshal(options))url:/docExpansion:/oauth2RedirectUrl:/ui.initOAuth({...})values, ReDocRedoc.init(<url>, ...)%q<,>,&, U+2028 and U+2029 escapedjson.MarshaltextBool and int literals (
deepLinking,persistAuthorization,defaultModelsExpandDepth,usePkceWithAuthorizationCodeGrant) are passed astemplate.JSso the template does not space-pad them; they are never configuration strings. The pages are still built once inNew(); a template execution error (impossible for these inputs) panics there like any other startup misconfiguration.Why
%qescapes"as\", which HTML does not honour, and leaves&raw — aSpecURLcontaining"terminates the attribute.<script>,%qdoes not neutralise</script>(the HTML parser closes the block regardless of quoting) and emits escapes JavaScript lacks (\a,\x,\U0001xxxx).html/templateis the standard library's answer to exactly this problem, and — unlikejson.Marshalandhtml.EscapeString— CodeQL models it as a sanitizer.Not exploitable today: every value is startup configuration (
swagger.Config), never request data. This is the hardening the issue asks for.CodeQL
The first revision of this PR (a hand-rolled
jsString+html.EscapeStringfed intofmt.Sprintf) tripped three new criticalgo/unsafe-quotingalerts (#12, #13, #14: "If this JSON value contains a double quote, it could break out of the enclosing quotes"). They were false positives —json.Marshalemits\"inside the JS literal andhtml.EscapeStringemits"/'inside the attribute — but CodeQL does not model either as a quote sanitizer, so after merge they would have sat onmainas three open critical alerts, the same shape as the previously dismissed #10/#11. Thehtml/templaterevision removes the pattern instead of dismissing the alerts: noSprintfwith a quoted placeholder remains in the package.Result on this revision (77c0a21): the CodeQL check concludes
success— "No new alerts in code changed by this pull request" — and #12, #13 and #14 are in statefixedon the PR ref (not dismissed), so nothing lands onmainand nothing needs triage after merge.Fail-first evidence
middleware/swagger/escaping_test.gobuilds every renderer, with and withoutAssetsPath, using a hostile valuex"y'z</script>&+ BEL (U+0007) + U+2028 +end, and asserts that<script/</script>tags stay balanced, thedata-urlattribute contains no raw"/'/</>/space/control/U+2028 and decodes back to the original, and every JS string literal is valid JSON that round-trips to the original value.The test file at this revision, run against
main'sswagger.go:The raw interpolations that produced those failures (note the
\a, the unescaped</script>and the\"inside an HTML attribute):(
above stands for the raw U+2028 byte sequence in the page.)With the fix, all escaping tests pass, and the full package passes under
-race:Ordinary inputs are unchanged
TestPlainConfigUnchangedpasses on both the unfixed and the fixed code and pins the exacturl: "/swagger/spec",docExpansion: "list",oauth2RedirectUrl: "...",ui.initOAuth({...}),data-url="https://example.com/openapi.json"andRedoc.init("/swagger/spec", {}, ...)renderings; every pre-existing assertion inswagger_test.go(deepLinking: true,defaultModelsExpandDepth: -1,clientId: "my-client", ...) is untouched and passes.In addition, I rendered 648 ordinary configurations — 3 renderers × 4 asset sources (CDN,
/assets,/assets/,https://cdn.example.com/ui) × 3 spec sources (SpecContent, absoluteSpecURLwith a query string, relativeSpecURL) × 3 option sets (nil, empty, nested theme/expandResponses/hideDownloadButton/scrollYOffset) × 6 UI configurations (defaults, full customisation, OAuth2 with redirect, partial OAuth2, empty OAuth2, PKCE-only) — with theSprintfbuilder and thehtml/templatebuilder;diff -rreports the pages byte-identical.Two intentional differences, for unusual inputs only:
SpecURLcontaining&,<or>renders as&/</>inside the JS literals — semantically identical to the browser, and required to neutralise</script>;data-urlis a URL-typed attribute tohtml/template: characters that are not valid in a URL (quotes, angle brackets, spaces, controls, U+2028) are percent-encoded before HTML escaping, and a scheme other thanhttp/https(relative URLs are fine) is neutralised. Both are what a browser does before fetching.Config.SpecURLdocuments the scheme rule andTestScalarNeutralisesUnsafeSpecURLSchemepins it; Swagger UI and ReDoc receiveSpecURLas a JS string, unchanged.Checks
gofmt -l middleware/swagger/clean,go vet ./...clean,go build ./...ok,golangci-lint run ./middleware/swagger/...0 issuesgo test -race -count=1 ./middleware/swagger/okgo/unsafe-quoting) arefixedon the PR refFixes #504