tao.chenandClaude
3bc8bd49f6
代码 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 <noreply@anthropic.com>
72 lines
2.1 KiB
Go
72 lines
2.1 KiB
Go
package httpclient
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"fmt"
|
|
"net/http"
|
|
)
|
|
|
|
const defaultMaxRedirects = 5
|
|
|
|
// DoWithRedirect 手动跟随 3xx,跨主机跳转保留 Authorization header。
|
|
// maxRedirects == 0 禁止 redirect(等同 Do)
|
|
// maxRedirects < 0 用 defaultMaxRedirects
|
|
func (c *Client) DoWithRedirect(ctx context.Context, req *http.Request, maxRedirects int, allowedHosts ...string) (*http.Response, error) {
|
|
if maxRedirects == 0 {
|
|
return c.Do(ctx, req, allowedHosts...)
|
|
}
|
|
if maxRedirects < 0 {
|
|
maxRedirects = defaultMaxRedirects
|
|
}
|
|
|
|
current := req
|
|
remaining := maxRedirects
|
|
for {
|
|
resp, err := c.Do(ctx, current, allowedHosts...)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
if resp.StatusCode < 300 || resp.StatusCode >= 400 {
|
|
return resp, nil
|
|
}
|
|
|
|
// 3xx
|
|
if remaining == 0 {
|
|
_ = resp.Body.Close()
|
|
return nil, errors.New("httpclient: too many redirects")
|
|
}
|
|
remaining--
|
|
|
|
loc := resp.Header.Get("Location")
|
|
if loc == "" {
|
|
_ = resp.Body.Close()
|
|
return nil, fmt.Errorf("httpclient: %d %s without Location header", resp.StatusCode, resp.Status)
|
|
}
|
|
next, err := current.URL.Parse(loc)
|
|
if err != nil {
|
|
_ = resp.Body.Close()
|
|
return nil, fmt.Errorf("httpclient: parse redirect Location %q: %w", loc, err)
|
|
}
|
|
_ = resp.Body.Close()
|
|
|
|
newReq, err := http.NewRequestWithContext(ctx, current.Method, next.String(), nil)
|
|
if err != nil {
|
|
return nil, fmt.Errorf("httpclient: build redirect request: %w", err)
|
|
}
|
|
// **保留** 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
|
|
}
|
|
}
|