fix(gateway): use relative redirects to preserve host and port#5860
fix(gateway): use relative redirects to preserve host and port#5860gustavosbarreto merged 1 commit intomasterfrom
Conversation
Add `absolute_redirect off` to the nginx server block so that redirects (like /admin -> /admin/) generate relative Location headers instead of absolute ones. This fixes the redirect dropping the port when accessing the gateway on non-standard ports.
| {{ end }} | ||
| server_name {{ $cfg.Domain }}; | ||
| server_tokens off; | ||
| absolute_redirect off; |
There was a problem hiding this comment.
[medium] Good fix — absolute_redirect off; is the correct and idiomatic nginx directive for this problem.
One thing to consider: there is a second server block further down (the Enterprise WebEndpoints block, ~line 758) that does not get this directive. The architecture analysis suggests it's probably fine since that block only has a rewrite ... break (internal, no client redirect) + proxy_pass, so nginx shouldn't generate any client-facing redirects there. However, for defensive consistency, you might want to add it there too — it's a no-op if no redirects are generated, but protects against future changes to that block.
Up to you whether to apply it there as well; the current change is correct for the immediate fix.
| {{ end }} | ||
| server_name {{ $cfg.Domain }}; | ||
| server_tokens off; | ||
| absolute_redirect off; |
There was a problem hiding this comment.
[medium — testing] The existing TestMain_smoke test in gateway/main_test.go already spins up the gateway container on a random mapped port (i.e., a non-standard port — the exact scenario this PR fixes). It currently only checks /healthcheck. This is a great opportunity to add a regression test for the redirect behavior:
| absolute_redirect off; | |
| absolute_redirect off; |
The config line itself is fine — the suggestion here is to extend gateway/main_test.go with something like:
// Disable automatic redirect following to inspect the Location header.
noRedirectClient := http.Client{
Timeout: 5 * time.Second,
CheckRedirect: func(req *http.Request, via []*http.Request) error {
return http.ErrUseLastResponse
},
}
adminURL := baseURL + "/admin"
resp, err = noRedirectClient.Get(adminURL)
assert.NoError(t, err)
defer resp.Body.Close()
assert.Equal(t, http.StatusMovedPermanently, resp.StatusCode)
location := resp.Header.Get("Location")
assert.Equal(t, "/admin/", location, "Location header should be relative, not absolute")This would catch regressions if absolute_redirect off is ever accidentally removed.
|
Review Summary: Reviewed PR 5860 across 5 dimensions (Code Quality, Security, Testing, Go/TS Patterns, Architecture). Security: No issues - relative redirects are safer. Architecture: No breaking changes, no cloud/enterprise impact. 2 inline comments posted (medium severity): (1) Consider adding absolute_redirect off to the second server block for consistency, (2) Extend gateway/main_test.go smoke test to verify relative redirects. Overall a clean, well-targeted fix. I have provided feedback as inline comments. When ready for re-review, comment /review. |
Summary
absolute_redirect offto the nginx server block so redirects (like/admin→/admin/) generate relativeLocationheaders instead of absolute oneslocalhost:8088/adminredirected tohttp://localhost/admin/instead of/admin/)Test plan
curl -v localhost:8088/adminand verifyLocationheader is/admin/(relative) instead ofhttp://localhost/admin/(absolute)