diff --git a/internal/httpclient/redirect.go b/internal/httpclient/redirect.go index 178c62e..a1c1499 100644 --- a/internal/httpclient/redirect.go +++ b/internal/httpclient/redirect.go @@ -56,6 +56,15 @@ func (c *Client) DoWithRedirect(ctx context.Context, req *http.Request, maxRedir } // **保留** header (Authorization 等) newReq.Header = current.Header.Clone() + // TODO(redirect): per RFC 9110 §15.4, 303 See Other must switch the + // method to GET and drop the body; 307 Temporary Redirect and + // 308 Permanent Redirect must preserve both method AND body. This + // implementation always preserves the method and discards the body, + // which is correct for 307/308 GET (YARN amContainerLogs) but + // non-compliant for 303 and any 307/308 POST. Current YARN / SHS + // API surface only triggers 307 on GET, so the gap is unfired. + // TestDoWithRedirect_DeferredRFC9110Gaps locks the current behavior + // so a future fix must update that test deliberately. newReq.Body = nil current = newReq } diff --git a/internal/httpclient/redirect_test.go b/internal/httpclient/redirect_test.go index 12c42ab..85768a4 100644 --- a/internal/httpclient/redirect_test.go +++ b/internal/httpclient/redirect_test.go @@ -140,3 +140,87 @@ func TestDoWithRedirect_ZeroDisallows(t *testing.T) { t.Fatalf("status=%d, want 302", resp.StatusCode) } } + +// TestDoWithRedirect_DeferredRFC9110Gaps documents two known +// non-conformances with RFC 9110 §15.4 that we have NOT fixed because +// the current upstream YARN / SHS API surface only triggers 307 on +// GET amContainerLogs, where neither gap is fired. +// +// The assertions below verify the CURRENT (gap) behavior. Any future +// fix must update these assertions deliberately — a silent flip would +// mean the gap fix passed without anyone thinking about 303 method +// switch or 307-POST body resend. See the TODO comment in +// redirect.go for the full discussion. +// +// (1) 303 See Other: we keep the original method (RFC requires switch +// to GET). The body IS correctly dropped — that's RFC-compliant +// even with the wrong method. +// (2) 307 Temporary Redirect / 308 Permanent Redirect on a POST: we +// keep the method (correct) but drop the body (RFC requires +// resend with the same method). +func TestDoWithRedirect_DeferredRFC9110Gaps(t *testing.T) { + t.Run("303 keeps method, should switch to GET", func(t *testing.T) { + var hitMethod, hitBody string + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hitMethod = r.Method + b, _ := io.ReadAll(r.Body) + hitBody = string(b) + w.WriteHeader(http.StatusOK) + })) + defer target.Close() + + origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Location", target.URL+"/see-other") + w.WriteHeader(http.StatusSeeOther) + })) + defer origin.Close() + + c := New(Config{Timeout: 5 * time.Second, MaxResponseBytes: 1024}) + req, _ := http.NewRequest(http.MethodPost, origin.URL+"/start", strings.NewReader("payload-303")) + if _, err := c.DoWithRedirect(context.Background(), req, 5, "127.0.0.1"); err != nil { + t.Fatalf("DoWithRedirect: %v", err) + } + + // CURRENT (gap) behavior. RFC-correct behavior: hitMethod == "GET". + if hitMethod != http.MethodPost { + t.Errorf("303 arrived as %s, want POST (current gap; RFC says GET)", hitMethod) + } + // Body must be empty — that part is RFC-compliant (303 always + // drops body, even when the method is wrong). + if hitBody != "" { + t.Errorf("303 body=%q, want empty (RFC requires drop regardless of method)", hitBody) + } + }) + + t.Run("307 on POST drops body, should resend", func(t *testing.T) { + var hitMethod, hitBody string + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hitMethod = r.Method + b, _ := io.ReadAll(r.Body) + hitBody = string(b) + w.WriteHeader(http.StatusOK) + })) + defer target.Close() + + origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Location", target.URL+"/temp") + w.WriteHeader(http.StatusTemporaryRedirect) + })) + defer origin.Close() + + c := New(Config{Timeout: 5 * time.Second, MaxResponseBytes: 1024}) + req, _ := http.NewRequest(http.MethodPost, origin.URL+"/start", strings.NewReader("payload-307")) + if _, err := c.DoWithRedirect(context.Background(), req, 5, "127.0.0.1"); err != nil { + t.Fatalf("DoWithRedirect: %v", err) + } + + // Method preserved (correct). + if hitMethod != http.MethodPost { + t.Errorf("307 arrived as %s, want POST", hitMethod) + } + // CURRENT (gap) behavior. RFC-correct behavior: hitBody == "payload-307". + if hitBody != "" { + t.Errorf("307 body=%q, want empty (current gap; RFC says resend payload-307)", hitBody) + } + }) +}