plumbing/transport/http: allow redirects within a custom scheme#2170
Closed
peterdeme wants to merge 1 commit into
Closed
plumbing/transport/http: allow redirects within a custom scheme#2170peterdeme wants to merge 1 commit into
peterdeme wants to merge 1 commit into
Conversation
applyRedirect rejected any redirect whose final scheme wasn't a hardcoded http or https, which makes redirect handling unusable for callers that register their own transport under a custom scheme via transport.Register. Such a transport may rewrite the request URL to a real backend internally and surface the original custom-scheme URL back on resp.Request.URL, at which point the allowlist trips even though the scheme never actually changed. The hardcoded allowlist is redundant here: the cross-scheme guard right below it already blocks any switch to a foreign scheme (the only permitted transition is an http->https upgrade), so a redirect can never escalate to a protocol the caller didn't start on. Dropping the allowlist keeps the SSRF protection intact (https can still never become file:// or gopher://) while letting same-scheme custom redirects through. checkRedirect is left as is: it runs at the net/http layer where only real http/https URLs appear, and it lacks the cross-scheme guard, so its allowlist still does meaningful work there.
pjbgf
requested changes
Jun 8, 2026
pjbgf
left a comment
Member
There was a problem hiding this comment.
@peterdeme thanks for looking into this. Please review the changes below and once they are done we will have to reflect the same into #2169.
Note that this does not completely resolve the problem you are trying to fix. To that effect:
- Add a new test in
plumbing/transport/http/redirect_test.go:
func TestRedirectCustomScheme(t *testing.T) {
t.Parallel()
base, backend := setupSmartServer(t)
prepareRepo(t, fixtures.Basic().One(), base, "basic.git")
rl := test.ListenTCP(t)
raddr := rl.Addr().(*net.TCPAddr)
backendHost := fmt.Sprintf("localhost:%d", backend.Port)
redirectServer := &http.Server{
Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
target := "private://" + backendHost + "/basic.git" + r.URL.Path[len("/redirected-repo"):]
if r.URL.RawQuery != "" {
target += "?" + r.URL.RawQuery
}
http.Redirect(w, r, target, http.StatusMovedPermanently)
}),
}
done := make(chan struct{})
go func() {
defer close(done)
require.ErrorIs(t, redirectServer.Serve(rl), http.ErrServerClosed)
}()
t.Cleanup(func() {
require.NoError(t, redirectServer.Close())
<-done
})
baseTr := &http.Transport{}
baseTr.RegisterProtocol("private", &schemeRewriteRoundTripper{delegate: baseTr})
tr := NewTransport(Options{Client: &http.Client{Transport: baseTr}})
endpoint := &url.URL{
Scheme: "private",
Host: fmt.Sprintf("localhost:%d", raddr.Port),
Path: "/redirected-repo",
}
session, err := tr.Handshake(context.Background(), &transport.Request{
URL: endpoint,
Command: transport.UploadPackService,
})
require.NoError(t, err)
defer session.Close()
refs, err := session.GetRemoteRefs(context.Background())
require.NoError(t, err)
require.NotNil(t, refs)
st := memory.NewStorage()
req := &transport.FetchRequest{}
req.Wants = append(req.Wants, plumbing.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5"))
require.NoError(t, session.Fetch(context.Background(), st, req))
}This should fail based on the current changes.
- Amend
plumbing/transport/http/http.goso thatcheckRedirectsupports custom schemes:
func checkRedirect(req *http.Request, via []*http.Request, policy RedirectPolicy) error {
if len(via) != 0 {
prev := via[len(via)-1]
if prev.URL != nil && prev.URL.Scheme == "https" && req.URL.Scheme == "http" {
return fmt.Errorf("http transport: redirect downgrades scheme to %s", redactedURL(req.URL))
}
}
switch policy {
case FollowRedirects:
case NoFollowRedirects:
return fmt.Errorf("http transport: redirects disabled to %s", redactedURL(req.URL))
case FollowInitialRedirects:
if !isInitialRequest(req) {
return fmt.Errorf("http transport: redirect on non-initial request to %s", redactedURL(req.URL))
}
default:
return fmt.Errorf("http transport: invalid redirect policy %q", policy)
}
if req.URL.Scheme != "http" && req.URL.Scheme != "https" {
// Permit custom schemes only when the redirect stays on the same
// scheme as the previous hop. A caller that registered a transport
// for a custom scheme (e.g. via http.Transport.RegisterProtocol)
// can then follow same-scheme redirects, while a switch to an
// unrelated scheme (file://, gopher://) remains blocked.
var prevScheme string
if len(via) != 0 && via[len(via)-1].URL != nil {
prevScheme = via[len(via)-1].URL.Scheme
}
if req.URL.Scheme != prevScheme {
return fmt.Errorf("redirect to unsupported scheme %q", req.URL.Scheme)
}
}
if len(via) >= 10 {
return fmt.Errorf("http transport: too many redirects")
}
return nil
}| @@ -96,9 +98,6 @@ func applyRedirect(resp *http.Response, baseURL *url.URL) (*url.URL, error) { | |||
| return baseURL, nil | |||
| } | |||
|
|
|||
Member
There was a problem hiding this comment.
This change adds a regression. Instead, let's expand to capture custom schemes:
Suggested change
| if final.Scheme != "http" && final.Scheme != "https" && final.Scheme != baseURL.Scheme { | |
| return nil, fmt.Errorf("redirect to unsupported scheme %q", final.Scheme) | |
| } |
Comment on lines
+125
to
+128
| name: "rejects redirect to a foreign scheme", | ||
| baseURL: "https://example.com/repo.git", | ||
| finalURL: "ftp://evil.com/repo.git/info/refs", | ||
| wantErr: "unsupported scheme", | ||
| wantErr: "changes scheme", |
Member
There was a problem hiding this comment.
Suggested change
| name: "rejects redirect to a foreign scheme", | |
| baseURL: "https://example.com/repo.git", | |
| finalURL: "ftp://evil.com/repo.git/info/refs", | |
| wantErr: "unsupported scheme", | |
| wantErr: "changes scheme", | |
| name: "custom scheme to unrelated scheme is blocked", | |
| baseURL: "https://example.com/repo.git", | |
| finalURL: "ftp://evil.com/repo.git/info/refs", | |
| wantErr: "unsupported scheme", |
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.
Problem
applyRedirectrejects any redirect whose final scheme isn't a hardcodedhttporhttps:This makes redirect handling unusable for callers that register their own transport under a custom scheme via
transport.Register. A transport like that can rewrite the request URL to a real backend internally and then surface the original custom-scheme URL back onresp.Request.URL, at which point the allowlist trips even though the scheme never actually changed.This breaks our Spacelift VCS agent, which addresses internal repositories through a
private://scheme.Fix
Drop only the hardcoded
http/httpsallowlist. The cross-scheme guard immediately below it already does the real work:A redirect can only stay on the base URL's own scheme or upgrade
http→https. It can never escalate to a protocol the caller didn't start on, so the SSRF protection is preserved:https://→file:///gopher://→ still blocked (scheme change)http://→https://→ still allowed (upgrade)private://→private://→ now allowed (same scheme)private://→https://→ still blocked (can't escalate)Note
checkRedirectdeliberately keeps its allowlist. It runs at the net/http layer where only realhttp/httpsURLs appear, and it has no cross-scheme guard of its own, so the check still carries SSRF weight there.The
ftp-from-httpscase that used to assertunsupported schemenow assertschanges scheme(the cross-scheme guard catches it), plus a new case covers a redirect that stays within a custom scheme. Thefile://block inredirect_test.gois untouched.Note
This PR targets
main. The equivalent fix for thereleases/v5.xline is in #2169.