From 3bc8bd49f677be096deac51280df9d127c5ebedd Mon Sep 17 00:00:00 2001 From: "tao.chen" <93983997+taochen-ct@users.noreply.github.com> Date: Mon, 13 Jul 2026 16:24:40 +0800 Subject: [PATCH] =?UTF-8?q?httpclient:=20=E8=AE=B0=E5=BD=95=20RFC=209110?= =?UTF-8?q?=20redirect=20gap,=20deferred=20test=20=E9=94=81=E4=BD=8F?= =?UTF-8?q?=E5=BD=93=E5=89=8D=E8=A1=8C=E4=B8=BA?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 代码 review 发现 DoWithRedirect 有两个 RFC 9110 §15.4 标准违反, 但当前不修: 1. 303 See Other 应该把 method 切到 GET, 当前保持原 method (例如 POST → 307 → 继续 POST). YARN/SHS 当前不用 303, gap 未触发. 2. 307 Temporary Redirect / 308 Permanent Redirect 收到 POST 时 应该原 method + body 重发, 当前 body 强制置 nil. YARN amContainerLogs 是 GET 触发 307, gap 未触发. 不修原因: 没有真实用户报告. 修 307 body 重发要 buffer 起来 + 重写 Content-Length, 容易出 subtle bug (multipart/trailer). 修复成本 vs 收益不成比例. 新增 TestDoWithRedirect_DeferredRFC9110Gaps (2 subtest) 锁定当前 行为, 未来要修必须主动改测试 → 强制 review 行为变更. 同时给 redirect.go 加 TODO(redirect) 注释详细说明两个 gap 和修法方向. subtest 修正一个错位: 303 的 body 丢失不是 gap (RFC 规定 303 必须 丢 body, 跟 method 错没错无关), gap 只是 method 保留 vs 切 GET. 已对应调整注释和断言. 未提交: .env (gitignored 内容之外有意未提交) Co-Authored-By: Claude --- internal/httpclient/redirect.go | 9 +++ internal/httpclient/redirect_test.go | 84 ++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) 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) + } + }) +}