Skip to content

plumbing/transport/http: allow redirects within a custom scheme#2170

Closed
peterdeme wants to merge 1 commit into
go-git:mainfrom
peterdeme:remove-redirect-scheme-validation-main
Closed

plumbing/transport/http: allow redirects within a custom scheme#2170
peterdeme wants to merge 1 commit into
go-git:mainfrom
peterdeme:remove-redirect-scheme-validation-main

Conversation

@peterdeme

@peterdeme peterdeme commented Jun 2, 2026

Copy link
Copy Markdown

Problem

applyRedirect rejects any redirect whose final scheme isn't a hardcoded http or https:

if final.Scheme != "http" && final.Scheme != "https" {
    return nil, fmt.Errorf("http transport: redirect to unsupported scheme %q", final.Scheme)
}

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 on resp.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/https allowlist. The cross-scheme guard immediately below it already does the real work:

if final.Scheme != baseURL.Scheme &&
    (baseURL.Scheme != "http" || final.Scheme != "https") {
    return nil, fmt.Errorf("http transport: redirect changes scheme from %q to %q", baseURL.Scheme, final.Scheme)
}

A redirect can only stay on the base URL's own scheme or upgrade httphttps. 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

checkRedirect deliberately keeps its allowlist. It runs at the net/http layer where only real http/https URLs appear, and it has no cross-scheme guard of its own, so the check still carries SSRF weight there.

The ftp-from-https case that used to assert unsupported scheme now asserts changes scheme (the cross-scheme guard catches it), plus a new case covers a redirect that stays within a custom scheme. The file:// block in redirect_test.go is untouched.


Note

This PR targets main. The equivalent fix for the releases/v5.x line is in #2169.

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 pjbgf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@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:

  1. 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.

  1. Amend plumbing/transport/http/http.go so that checkRedirect supports 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
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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",

@peterdeme peterdeme closed this Jun 9, 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