Skip to content

Fix review issues for caching and encoding - #4093

Merged
martincostello merged 1 commit into
masterfrom
escaping-fixes
Aug 3, 2026
Merged

martincostello merged 1 commit into
masterfrom
escaping-fixes

Conversation

@martincostello

Copy link
Copy Markdown
Collaborator

Resolve a number of code correctness issued identified by scanning the repository's code with GitHub Copilot and Claude Code.

  • Discard a request path base that a browser resolves to another authority (//host, /\host) instead of emitting it as servers[].url, and mark path-base-derived documents private so a shared cache cannot replay one client's prefix to another.
  • Use a distinct ETag per content-coding and send Vary: Accept-Encoding, so a conditional request is no longer answered 304 for a representation the client never held.
  • Escape SwaggerDocumentUrlsPath before interpolating it into a regular expression.
  • Emit the serialized config as a JavaScript object literal instead of splicing it into JSON.parse('...') to ensure things are encoded correctly.
  • Escape the . in the index.(html|js) route patterns, select the file case-insensitively, serve the embedded assets only for GET and HEAD, and require a segment boundary after the route prefix.

- Discard a request path base that a browser resolves to another authority (`//host`, `/\host`) instead of emitting it as `servers[].url`, and mark path-base-derived documents `private` so a shared cache cannot replay one client's prefix to another.
- Use a distinct ETag per content-coding and send `Vary: Accept-Encoding`, so a conditional request is no longer answered `304` for a representation the client never held.
- Escape `SwaggerDocumentUrlsPath` before interpolating it into a regular expression.
- Emit the serialized config as a JavaScript object literal instead of splicing it into `JSON.parse('...')` to ensure things are encoded correctly.
- Escape the `.` in the `index.(html|js)` route patterns, select the file case-insensitively, serve the embedded assets only for GET and HEAD, and require a segment boundary after the route prefix.
@codecov

codecov Bot commented Aug 3, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.16%. Comparing base (0ca5050) to head (085a6da).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4093      +/-   ##
==========================================
+ Coverage   95.08%   95.16%   +0.08%     
==========================================
  Files         111      111              
  Lines        4046     4077      +31     
  Branches      820      826       +6     
==========================================
+ Hits         3847     3880      +33     
+ Misses        199      197       -2     
Flag Coverage Δ
Linux 95.16% <100.00%> (+0.08%) ⬆️
Windows ?
macOS 95.16% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@martincostello
martincostello marked this pull request as ready for review August 3, 2026 16:06
Copilot AI review requested due to automatic review settings August 3, 2026 16:06

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

The new TestSite.CreateServer helper starts an IHost without a deterministic disposal path, risking leaked hosts and test flakiness.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR tightens security and caching correctness for Swashbuckle’s embedded UI/static-asset middleware and Swagger document responses, and adds integration coverage to prevent regressions.

Changes:

  • Hardened route matching and template placeholder substitution for SwaggerUI/ReDoc (regex escaping, safe interpolation, case-insensitive file selection, method restrictions, segment-boundary enforcement).
  • Fixed cache negotiation correctness for embedded assets (ETag varies per content-coding, Vary: Accept-Encoding, avoid incorrect 304 Not Modified across representations).
  • Hardened Swagger document base-path handling (discard cross-authority path-base patterns; mark path-base-derived documents as Cache-Control: private) and added integration tests for these behaviors.
File summaries
File Description
test/Swashbuckle.AspNetCore.IntegrationTests/TestSite.cs Adds a helper to spin up ad-hoc TestServer instances for new middleware-focused integration tests.
test/Swashbuckle.AspNetCore.IntegrationTests/SwaggerUIIntegrationTests.cs Adds tests for ETag/Vary correctness, literal document-urls route matching, route/file handling, method restrictions, and encoding safety.
test/Swashbuckle.AspNetCore.IntegrationTests/SwaggerIntegrationTests.cs Adds tests for discarding cross-origin path-base values and cache behavior when path base is request-derived.
test/Swashbuckle.AspNetCore.IntegrationTests/ReDocIntegrationTests.cs Adds tests validating SpecUrl/config encoding safety and non-string-spliced config emission.
src/Swashbuckle.AspNetCore.SwaggerUI/SwaggerUIMiddleware.cs Fixes index route regex, escapes document-urls path in regex, improves pipeline fallthrough, case-insensitive file selection, safer placeholder replacement, and HTML-encodes title.
src/Swashbuckle.AspNetCore.SwaggerUI/index.js Stops splicing JSON into string literals; emits config/interceptors as JS literals to avoid string-termination injection.
src/Swashbuckle.AspNetCore.Swagger/SwaggerMiddleware.cs Centralizes base-path derivation, discards network-path references, and marks path-base-derived docs as private for shared-cache safety.
src/Swashbuckle.AspNetCore.ReDoc/ReDocMiddleware.cs Mirrors SwaggerUI hardening: regex fixes, pipeline fallthrough, safer placeholder replacement, title encoding, and JSON-literal config/spec url emission.
src/Swashbuckle.AspNetCore.ReDoc/index.js Switches to JS-literal arguments rather than JSON.parse('...') to avoid string-splice risks.
src/Shared/EmbeddedResourceProvider.cs Fixes representation-specific ETag handling, adds Vary: Accept-Encoding, enforces route-prefix segment boundary, and publishes cached resource content atomically.
Review details
  • Files reviewed: 10/10 changed files
  • Comments generated: 1
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Comment thread test/Swashbuckle.AspNetCore.IntegrationTests/TestSite.cs
@martincostello
martincostello merged commit 4fa2d81 into master Aug 3, 2026
16 checks passed
@martincostello
martincostello deleted the escaping-fixes branch August 3, 2026 17:04
This was referenced Oct 7, 2026
This was referenced Oct 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants