fix: OpenAPI server URL reflects https behind reverse proxy #21
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/openapi-https-server-url"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Problem
The OpenAPI document's
servers[]array advertised the container-local URL (http://localhost:8080). Behind an https-terminating nginx reverse proxy, Scalar's Try it out feature used this URL for API calls — which failed because:https://but the advertised URL washttp://localhost:8080is unreachable from the browserRoot cause
The built-in
AddOpenApi()generates the server URL from the container-local request, not from the forwarded headers. Even though we already haveForwardedHeadersmiddleware (which correctly setsRequest.Scheme = httpsfromX-Forwarded-Proto), the OpenAPI generator doesn't use the request context to build the server URL.Fix
Added
ProxyAwareServerTransformer— anIOpenApiDocumentTransformerthat setsdocument.Serversfrom the forwardedHttpContext:Request.Scheme(set byX-Forwarded-Proto)httpsRequest.Hostumbrel.kagaku.space:8080Request.PathBase(set byX-Forwarded-Prefix)/doujinshiResult: the OpenAPI JSON now advertises
https://umbrel.kagaku.space:8080/doujinshias the server URL, so Scalar's "Try it out" sends requests to the correct external HTTPS endpoint.How it works
Files changed (2 files, +48 / -1)
ProxyAwareServerTransformer.csIOpenApiDocumentTransformer— setsdocument.Serversfrom forwarded HttpContextProgram.csIHttpContextAccessor, add transformer toAddOpenApiNo nginx changes needed —
X-Forwarded-ProtoandX-Forwarded-Prefixare already being sent. All 243 tests pass.Summary
Summary
Coverage
DoujinManager.ApplicationCore - 84.8%
DoujinManager.Infrastructure - 91.6%
pshot
DoujinManager.RestAdapter - 84.2%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 22.3%
🤖 Hermes automated review: minor comments
Reviewed diff
66f0b5d...2de97695(+48/-1, 2 files): newProxyAwareServerTransformer+Program.csregistration. The approach (anIOpenApiDocumentTransformerthat setsdocument.Serversfrom the forwardedHttpContext) is sound and correctly placed. Theif (httpContext is not null)guard andAddHttpContextAccessor()registration are correct. Static security scan clean (no secrets, injection, eval/exec, pickle, SQL formatting). No blocking issues — two minor non-blocking notes below.Minor (non-blocking)
1.
Request.Hostonly carries the external hostname if nginx forwards it —XForwardedHostis not enabledProxyAwareServerTransformer.cs:25—var host = request.Host.Value;The
ForwardedHeadersOptions(Program.cs ~line 55) enablesXForwardedFor | XForwardedProto | XForwardedPrefixbut notXForwardedHost. SoRequest.Hostis not populated fromX-Forwarded-Host; it retains whatever the rawHostheader carries. The scheme and pathBase are handled robustly via explicit forwarded headers, but the host relies on nginx'sproxy_set_header Hostconfig instead:proxy_set_header Host $host;(common) →umbrel.kagaku.space:8080✅ works.Host $proxy_host) →Request.Hostis the container-internal upstream host (e.g.172.x.x.x:8080), and the advertised server URL will be wrong.This is fragile/undocumented. For consistency with the other two components, consider enabling
ForwardedHeaders.XForwardedHostand having nginx sendX-Forwarded-Host: umbrel.kagaku.space:8080, or documenting theproxy_set_header Host $host;nginx requirement (PR #19's DEPLOYMENT.md §9 is the natural place).2.
ProxyAwareServerTransformerat 0% test coverage (per CI)No unit test for the new transformer. The logic is simple, but a test would lock in the
{scheme}://{host}{pathBase}assembly and thehttpContext is nullno-op branch. Note: sincedocument.Serversis derived from request headers, a directly-exposed container port would let an attacker spoof theHostheader to inject a malicious server URL into the OpenAPI doc (the container port must not be published to untrusted networks — already noted in the #19 review's trust-all-proxy note; this PR doesn't change that posture).Verification
2de97695(line 83.6%, branch 49.5%; coverage generated 06/29 16:27 UTC, after the head commit). Theci.ymltest job posts coverage only on successful build+test, so build+test passed. Local build/test skipped per CI-evidence policy.git diffbetween base66f0b5dand head2de97695.Automated daily review. I never merge PRs.
0706948f849cbf79ef5d