fix: refuse POST redirects in guardRedirects unless FollowInfoRefsRedirect is set by jaysomani · Pull Request #122 · entireio/git-sync

jaysomani · 2026-09-24T02:43:06Z

guardRedirects's CheckRedirect closure now returns http.ErrUseLastResponse for POST redirects unless conn.FollowInfoRefsRedirect is set, matching vanilla git's http.followRedirects=initial default where only the /info/refs GET auto-follows. GET redirect behaviour is unchanged.

Required restructuring NewHTTPConnWithClient so guardRedirects closes over the *HTTPConn directly (reading FollowInfoRefsRedirect live at redirect time) rather than running before the struct exists.

Also corrects TestGuardRedirectsAnchorsOnTheIssuedRequest which modelled a post-adoption cross-site redirect but never set FollowInfoRefsRedirect, making the scenario unreachable under the corrected logic.

On a cross-host POST redirect without the flag, the failure mode changes from a delayed opaque 401 (Authorization stripped mid-flight by the existing sameSite guard) to an immediate clear http 307 error — the intended improvement from the issue.

Fixes #67

…irect is set guardRedirects's CheckRedirect closure now returns http.ErrUseLastResponse for POST redirects unless conn.FollowInfoRefsRedirect is set, matching vanilla git's http.followRedirects=initial default where only the /info/refs GET auto-follows. GET redirect behaviour is unchanged. Required restructuring NewHTTPConnWithClient so guardRedirects closes over the *HTTPConn directly (reading FollowInfoRefsRedirect live at redirect time) rather than running before the struct exists. Also corrects TestGuardRedirectsAnchorsOnTheIssuedRequest which modelled a post-adoption cross-site redirect but never set FollowInfoRefsRedirect, making the scenario unreachable under the corrected logic. On a cross-host POST redirect without the flag, the failure mode changes from a delayed opaque 401 (Authorization stripped mid-flight by the existing sameSite guard) to an immediate clear http 307 error — the intended improvement from the issue. Fixes entireio#67

jaysomani · 2026-09-24T02:44:07Z

@Soph can you review it ?

nodo

Thanks for picking this up! The approach looks right to me: building the HTTPConn first so the redirect policy reads FollowInfoRefsRedirect at redirect time works well, and the update to TestGuardRedirectsAnchorsOnTheIssuedRequest makes sense.

I left two inline comments. The first is the main issue: the check currently only catches 307/308, not 301/302/303.

A few smaller things on lines the diff doesn't touch:

adoptChallengeHost doc comment (smarthttp.go ~L935). It says that with the flag off "the immediate retry still hits the challenger directly (so the current op succeeds…)". That's no longer true for POSTs. A cross-host POST redirect now returns the 3xx instead of a 401, so the credential-helper retry never runs. Step 2 of the EnsureAuthForService comment has the same assumption about the probe. That's the intended behaviour, but the comments should say so.
Error message for the refused redirect. The caller now gets http 307: <url> from httpError, which doesn't say where the server tried to send the request or how to allow it. Adding the Location header and a hint about --source-follow-info-refs-redirect / --target-follow-info-refs-redirect for 3xx responses would make the failure much easier to act on.
FollowInfoRefsRedirect field doc (smarthttp.go ~L231). The field now also decides whether POST redirects are followed. Reusing it rather than adding a second flag is fine, but its doc comment should mention the POST behaviour so the name doesn't mislead.

nodo · 2026-09-28T15:33:37Z

 		if len(via) >= maxRedirects {
 			return fmt.Errorf("stopped after %d redirects", maxRedirects)
 		}
+		if req.Method == http.MethodPost && !conn.FollowInfoRefsRedirect {


Go's http.Client rewrites the method before calling CheckRedirect: on a 301, 302 or 303 it turns a POST into a GET (see redirectBehavior in net/http/client.go). So req.Method is only POST here for 307/308, and the other codes are still followed, now as a GET to git-upload-pack with no body.

I checked with a quick test: same POST, flag unset.

301/302/303 -> followed, final server hit with GET, status 200 307/308 -> not followed, 3xx returned

Checking the method of the request that started the chain avoids this:

if len(via) > 0 && via[0].Method == http.MethodPost && !conn.FollowInfoRefsRedirect { return http.ErrUseLastResponse }

(via[0].Method != http.MethodGet would also work if you'd rather allow only GET redirects by default.)

nodo · 2026-09-28T15:33:37Z

+	}))
+	defer final.Close()
+	start := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, final.URL+"/repo.git/git-upload-pack", http.StatusTemporaryRedirect)


These tests only use 307, which is why the 301/302/303 case above wasn't caught. The three new tests are nearly identical, so one table-driven test over method × status code (301, 302, 303, 307, 308) × flag on/off would cover it with less duplication.

Go's http.Client rewrites POST to GET before calling CheckRedirect on 301/302/303, so req.Method is never POST for those codes. Check via[0].Method (the original request method) instead, which is preserved across all rewrites. Also: - Replace 3 nearly-identical 307-only tests with one table-driven test covering POST x {301,302,303,307,308} x flag-on/off and GET x 301 - Add TestPostRPC_RefusedRedirectErrorNamesLocationAndFlags covering the httpError path with Location header and flag hint - Update FollowInfoRefsRedirect field doc to mention POST gating - Update adoptChallengeHost and EnsureAuthForService comments to reflect that cross-host POST redirects now return 3xx instead of triggering the credential-helper retry

jaysomani · 2026-09-28T16:04:56Z

@nodo done ✅

nodo

Thanks, the via[0].Method fix and the table-driven test look good, and the new error message is much easier to act on.

A few small things left:

docs/protocol.md (~L335). The "Off (default)" bullet says that redirects are followed for the GET and the RPC POSTs go to the original host. It should also say that a POST redirect is now refused and returned as an error unless the flag is set.
FollowInfoRefsRedirect field doc. It ends with "Off by default to preserve behaviour for callers that rely on … a POST redirect surfacing rather than silently following". Refusing POST redirects is new with this PR, so "preserve" is misleading. Something like "Off by default: EndpointURL stays stable and POST redirects surface as errors" would be more accurate.
httpError comment. It says a refused POST redirect is "the only way a redirect status ever reaches this function". That isn't quite true: a 3xx without a Location header, or a caller-supplied CheckRedirect returning http.ErrUseLastResponse, can also get here. It's harmless because the new branch only runs when Location is set, but I'd soften the wording (e.g. "typically").

jaysomani · 2026-10-01T03:42:33Z

@nodo got it done

jaysomani · 2026-10-05T02:49:14Z

Hey @nodo i have fixed the failed lint error now can you run the ci again

Soph

Thanks for doing this, I have left 3 more comments. Especially the redirects for POST behavior we should fix before merging. Thanks!

Soph · 2026-10-05T07:46:06Z

+		// original request that started the chain, so it still says POST
+		// for all five redirect codes; req.Method would only catch
+		// 307/308, which are the only two codes that preserve it.
+		if len(via) > 0 && via[0].Method == http.MethodPost && !conn.FollowInfoRefsRedirect {


Could we keep the default behaviour working for hosts that redirect both /info/refs and RPC POSTs?

With the flag off, we follow the /info/refs redirect but keep sending POSTs to the original host. Previously, if that host returned a 307, Go followed it and replayed the buffered request body; tryHelperRetry handled a cross-host 401 if needed. Now we stop at the redirect, so a fetch that used to work fails unless the user enables the flag.

This also differs from git's http.followRedirects=initial: git uses the URL reached by /info/refs for subsequent POSTs, avoiding the original host's redirect. The auth probe (doServiceProbe) has the same problem here: it stops at the 307 before it can receive a 401 and attach helper credentials.

Could we either adopt the endpoint reached by /info/refs even with the flag off, or allow POST redirects to that endpoint while refusing redirects elsewhere?

For the default I'd go with the narrow option: allow a POST redirect only when it lands on the same host that /info/refs already reached, and keep refusing other redirects unless the flag is set. That keeps the third-host case safe and fixes setups that redirect both paths to the same place. Does that match what you had in mind? I'll hold the change until you confirm.

Soph · 2026-10-05T07:46:06Z

+	// turns "why did my push get a 307" into an actionable answer instead
+	// of a bare status code.
+	if res.StatusCode < http.StatusBadRequest {
+		if location := res.Header.Get("Location"); location != "" {


Could we sanitize and redact Location before including it in the error? It's controlled by the server and could contain control characters or a URL with credentials, like https://user:token@…. Right now we'd copy that straight into logs and --json output.

The other server-provided values in httpError already go through sanitize.Text or redact.Endpoint, so this should get the same treatment.

Also, this early return drops the request URL and diagnostic headers that we'd previously include for a 3xx error. Can we keep those?

Soph · 2026-10-05T07:46:06Z

+	// of a bare status code.
+	if res.StatusCode < http.StatusBadRequest {
+		if location := res.Header.Get("Location"); location != "" {
+			return fmt.Errorf("http %d: server redirected to %s — use --source-follow-info-refs-redirect or --target-follow-info-refs-redirect to follow POST redirects",


Could we show this hint only when our POST redirect guard actually blocks a request? Right now it appears for any 3xx with a Location, including a /info/refs GET blocked by the caller's own CheckRedirect policy. Enabling the flag wouldn't help in that case.

I'd also keep the CLI flag names out of gitproto: library callers using gitsync.Endpoint can't act on that advice, and CLI users get both flags even though conn.Label tells us which side failed.

Maybe we could return a distinct error when the guard fires and let the CLI add the hint for the relevant flag?

nodo reviewed Sep 28, 2026

View reviewed changes

nodo reviewed Sep 29, 2026

View reviewed changes

review comments

d287b04

lint error

d469d5c

Soph reviewed Oct 5, 2026

View reviewed changes

Navigation Menu

Repository navigation

fix: refuse POST redirects in guardRedirects unless FollowInfoRefsRedirect is set - #122

fix: refuse POST redirects in guardRedirects unless FollowInfoRefsRedirect is set#122
jaysomani wants to merge 4 commits into
entireio:mainfrom
jaysomani:fix/redirect-post-follows-only-with-flag

jaysomani commented Sep 24, 2026

Uh oh!

jaysomani commented Sep 24, 2026

Uh oh!

nodo left a comment

Uh oh!

nodo Sep 28, 2026

Uh oh!

nodo Sep 28, 2026

Uh oh!

jaysomani commented Sep 28, 2026

Uh oh!

nodo left a comment

Uh oh!

jaysomani commented Oct 1, 2026

Uh oh!

jaysomani commented Oct 5, 2026

Uh oh!

Soph left a comment

Uh oh!

Soph Oct 5, 2026

Uh oh!

jaysomani Oct 6, 2026

Uh oh!

Soph Oct 5, 2026

Uh oh!

Soph Oct 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Conversation

jaysomani commented Sep 24, 2026

Uh oh!

jaysomani commented Sep 24, 2026

Uh oh!

nodo left a comment

Choose a reason for hiding this comment

Uh oh!

nodo Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

nodo Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

jaysomani commented Sep 28, 2026

Uh oh!

nodo left a comment

Choose a reason for hiding this comment

Uh oh!

jaysomani commented Oct 1, 2026

Uh oh!

jaysomani commented Oct 5, 2026

Uh oh!

Soph left a comment

Choose a reason for hiding this comment

Uh oh!

Soph Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

jaysomani Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

Soph Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

Soph Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants