From b2e11ddb37811d44725799e4574616230650bf4f Mon Sep 17 00:00:00 2001 From: afishcalledwander <327599756+afishcalledwander@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:41:57 -0400 Subject: [PATCH] fix: add default timeout and handle errors for HaveIBeenPwned API (#39316) Co-authored-by: silverwind Co-authored-by: wxiaoguang --- modules/auth/password/pwn.go | 3 +- modules/auth/password/pwn/pwn.go | 85 +++++++++------------------ modules/auth/password/pwn/pwn_test.go | 34 ++++++++--- modules/httplib/request.go | 32 ++++------ modules/lfstransfer/backend/util.go | 2 +- modules/private/hook.go | 2 +- modules/private/internal.go | 6 +- modules/private/request.go | 4 +- modules/public/vitedev.go | 2 +- 9 files changed, 74 insertions(+), 96 deletions(-) diff --git a/modules/auth/password/pwn.go b/modules/auth/password/pwn.go index 3539f250a8c..b56ea52dcda 100644 --- a/modules/auth/password/pwn.go +++ b/modules/auth/password/pwn.go @@ -38,8 +38,7 @@ func IsPwned(ctx context.Context, password string) error { return nil } - client := pwn.New(pwn.WithContext(ctx)) - count, err := client.CheckPassword(password, true) + count, err := pwn.New().CheckPassword(ctx, password, true) if err != nil { return ErrIsPwnedRequest{err} } diff --git a/modules/auth/password/pwn/pwn.go b/modules/auth/password/pwn/pwn.go index a4043996fa4..badc417aa15 100644 --- a/modules/auth/password/pwn/pwn.go +++ b/modules/auth/password/pwn/pwn.go @@ -14,67 +14,31 @@ import ( "strconv" "strings" + "gitea.dev/modules/httplib" "gitea.dev/modules/setting" ) -const passwordURL = "https://api.pwnedpasswords.com/range/" - -// ErrEmptyPassword is an empty password error -var ErrEmptyPassword = errors.New("password cannot be empty") +const ( + passwordURL = "https://api.pwnedpasswords.com/range/" + maxResponseSize = 1 << 20 +) // Client is a HaveIBeenPwned client type Client struct { - ctx context.Context - http *http.Client + mockTransport http.RoundTripper } -// New returns a new HaveIBeenPwned Client -func New(options ...ClientOption) *Client { - client := &Client{ - ctx: context.Background(), - http: http.DefaultClient, - } - - for _, opt := range options { - opt(client) - } - - return client -} - -// ClientOption is a way to modify a new Client -type ClientOption func(*Client) - -// WithHTTP will set the http.Client of a Client -func WithHTTP(httpClient *http.Client) func(pwnClient *Client) { - return func(pwnClient *Client) { - pwnClient.http = httpClient - } -} - -// WithContext will set the context.Context of a Client -func WithContext(ctx context.Context) func(pwnClient *Client) { - return func(pwnClient *Client) { - pwnClient.ctx = ctx - } -} - -func newRequest(ctx context.Context, method, url string, body io.ReadCloser) (*http.Request, error) { - req, err := http.NewRequestWithContext(ctx, method, url, body) - if err != nil { - return nil, err - } - req.Header.Add("User-Agent", "Gitea "+setting.AppVer) - return req, nil +func New() *Client { + return &Client{} } // CheckPassword returns the number of times a password has been compromised // Adding padding will make requests more secure, however is also slower // because artificial responses will be added to the response // For more information, see https://www.troyhunt.com/enhancing-pwned-passwords-privacy-with-padding/ -func (c *Client) CheckPassword(pw string, padding bool) (int64, error) { +func (c *Client) CheckPassword(ctx context.Context, pw string, padding bool) (int64, error) { if pw == "" { - return -1, ErrEmptyPassword + return -1, errors.New("password cannot be empty") } sha := sha1.New() @@ -82,25 +46,30 @@ func (c *Client) CheckPassword(pw string, padding bool) (int64, error) { enc := hex.EncodeToString(sha.Sum(nil)) prefix, suffix := enc[:5], enc[5:] - req, err := newRequest(c.ctx, http.MethodGet, fmt.Sprintf("%s%s", passwordURL, prefix), nil) - if err != nil { - return -1, nil - } + req := httplib.NewClientRequest(http.MethodGet, fmt.Sprintf("%s%s", passwordURL, prefix)) + req.SetContext(ctx).SetTransport(c.mockTransport) + req.Header("User-Agent", "Gitea "+setting.AppVer) if padding { - req.Header.Add("Add-Padding", "true") + req.Header("Add-Padding", "true") } - - resp, err := c.http.Do(req) - if err != nil { - return -1, err - } - - body, err := io.ReadAll(resp.Body) + resp, err := req.Response() if err != nil { return -1, err } defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return -1, fmt.Errorf("unexpected status code %d from HaveIBeenPwned API", resp.StatusCode) + } + + body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize+1)) + if err != nil { + return -1, err + } + if len(body) > maxResponseSize { + return -1, fmt.Errorf("response from HaveIBeenPwned API exceeds %d bytes", maxResponseSize) + } + for pair := range strings.SplitSeq(string(body), "\n") { parts := strings.Split(pair, ":") if len(parts) != 2 { diff --git a/modules/auth/password/pwn/pwn_test.go b/modules/auth/password/pwn/pwn_test.go index 4b760fdf323..3462a397a58 100644 --- a/modules/auth/password/pwn/pwn_test.go +++ b/modules/auth/password/pwn/pwn_test.go @@ -26,36 +26,52 @@ func (mockTransport) RoundTrip(req *http.Request) (*http.Response, error) { "/range/5617b": "FD4CB34F0378BCB15D23F6FFD28F0775C9E:3\r\nFDF342FCD8C3611DAE4D76E8A992A3E4169:4\r\nFE81480327C992FE62065A827429DD1318B:0", "/range/79082": "FDF342FCD8C3611DAE4D76E8A992A3E4169:4\r\nFE81480327C992FE62065A827429DD1318B:0\r\nAFEF386F56EB0B4BE314E07696E5E6E6536:0", } + if req.URL.Path == "/range/b6b47" { // sha1("ratelimited") prefix + return &http.Response{Request: req, StatusCode: http.StatusTooManyRequests, Body: io.NopCloser(strings.NewReader("rate limited"))}, nil + } + if req.URL.Path == "/range/76eff" { + return &http.Response{Request: req, StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader(strings.Repeat("0", maxResponseSize+1)))}, nil + } if resp, ok := respMap[req.URL.Path]; ok { - return &http.Response{Request: req, Body: io.NopCloser(strings.NewReader(resp))}, nil + return &http.Response{Request: req, StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader(resp))}, nil } return nil, errors.New("unsupported path") } func TestPassword(t *testing.T) { - client := New(WithHTTP(&http.Client{Transport: mockTransport{}})) + ctx := t.Context() + client := New() + client.mockTransport = mockTransport{} - count, err := client.CheckPassword("", false) - assert.ErrorIs(t, err, ErrEmptyPassword, "blank input should return ErrEmptyPassword") + count, err := client.CheckPassword(ctx, "", false) + assert.ErrorContains(t, err, "password cannot be empty") assert.EqualValues(t, -1, count) - count, err = client.CheckPassword("pwned", false) + count, err = client.CheckPassword(ctx, "pwned", false) assert.NoError(t, err) assert.EqualValues(t, 1, count) - count, err = client.CheckPassword("notpwned", false) + count, err = client.CheckPassword(ctx, "notpwned", false) assert.NoError(t, err) assert.EqualValues(t, 0, count) - count, err = client.CheckPassword("paddedpwned", true) + count, err = client.CheckPassword(ctx, "paddedpwned", true) assert.NoError(t, err) assert.EqualValues(t, 1, count) - count, err = client.CheckPassword("paddednotpwned", true) + count, err = client.CheckPassword(ctx, "paddednotpwned", true) assert.NoError(t, err) assert.EqualValues(t, 0, count) - count, err = client.CheckPassword("paddednotpwnedzero", true) + count, err = client.CheckPassword(ctx, "paddednotpwnedzero", true) assert.NoError(t, err) assert.EqualValues(t, 0, count) + + count, err = client.CheckPassword(ctx, "ratelimited", false) + assert.Error(t, err) + assert.EqualValues(t, -1, count) + + count, err = client.CheckPassword(ctx, "oversized", false) + assert.ErrorContains(t, err, "exceeds") + assert.EqualValues(t, -1, count) } diff --git a/modules/httplib/request.go b/modules/httplib/request.go index 8542a57d367..b47d2b1b5fc 100644 --- a/modules/httplib/request.go +++ b/modules/httplib/request.go @@ -30,16 +30,10 @@ func DialContextWithTimeout(timeout time.Duration) func(ctx context.Context, net } } -func NewRequest(url, method string) *Request { - return &Request{ - url: url, - req: &http.Request{ - Method: method, - Header: make(http.Header), - Proto: "HTTP/1.1", // FIXME: from legacy httplib, it shouldn't be hardcoded - ProtoMajor: 1, - ProtoMinor: 1, - }, +func NewClientRequest(method, url string) *ClientRequest { + return &ClientRequest{ + url: url, + req: &http.Request{Method: method, Header: make(http.Header)}, params: map[string]string{}, // ATTENTION: from legacy httplib, callers must pay more attention to it, it will cause annoying bugs when the response takes a long time @@ -47,7 +41,7 @@ func NewRequest(url, method string) *Request { } } -type Request struct { +type ClientRequest struct { url string req *http.Request params map[string]string @@ -57,38 +51,38 @@ type Request struct { } // SetContext sets the request's Context -func (r *Request) SetContext(ctx context.Context) *Request { +func (r *ClientRequest) SetContext(ctx context.Context) *ClientRequest { r.req = r.req.WithContext(ctx) return r } // SetTransport sets the request transport, if not set, will use httplib's default transport with environment proxy support // ATTENTION: the http.Transport has a connection pool, so it should be reused as much as possible, do not create a lot of transports -func (r *Request) SetTransport(transport http.RoundTripper) *Request { +func (r *ClientRequest) SetTransport(transport http.RoundTripper) *ClientRequest { r.transport = transport return r } -func (r *Request) SetReadWriteTimeout(readWriteTimeout time.Duration) *Request { +func (r *ClientRequest) SetReadWriteTimeout(readWriteTimeout time.Duration) *ClientRequest { r.readWriteTimeout = readWriteTimeout return r } // Header set header item string in request. -func (r *Request) Header(key, value string) *Request { +func (r *ClientRequest) Header(key, value string) *ClientRequest { r.req.Header.Set(key, value) return r } // Param adds query param in to request. // params build query string as ?key1=value1&key2=value2... -func (r *Request) Param(key, value string) *Request { +func (r *ClientRequest) Param(key, value string) *ClientRequest { r.params[key] = value return r } // Body adds request raw body. It supports string, []byte and io.Reader as body. -func (r *Request) Body(data any) *Request { +func (r *ClientRequest) Body(data any) *ClientRequest { if r == nil { return nil } @@ -114,7 +108,7 @@ func (r *Request) Body(data any) *Request { // Response executes request client and returns the response. // Caller MUST close the response body if no error occurs. -func (r *Request) Response() (*http.Response, error) { +func (r *ClientRequest) Response() (*http.Response, error) { var paramBody string if len(r.params) > 0 { var buf bytes.Buffer @@ -160,6 +154,6 @@ func (r *Request) Response() (*http.Response, error) { return client.Do(r.req) } -func (r *Request) GoString() string { +func (r *ClientRequest) GoString() string { return fmt.Sprintf("%s %s", r.req.Method, r.url) } diff --git a/modules/lfstransfer/backend/util.go b/modules/lfstransfer/backend/util.go index 32264450487..07d402b6bb6 100644 --- a/modules/lfstransfer/backend/util.go +++ b/modules/lfstransfer/backend/util.go @@ -127,7 +127,7 @@ func isInternalLFSURL(s string) bool { return true } -func newInternalRequestLFS(ctx context.Context, internalURL, method string, headers map[string]string, body any) *httplib.Request { +func newInternalRequestLFS(ctx context.Context, internalURL, method string, headers map[string]string, body any) *httplib.ClientRequest { if !isInternalLFSURL(internalURL) { return nil } diff --git a/modules/private/hook.go b/modules/private/hook.go index 9e47f10fc95..85d9dfa37e0 100644 --- a/modules/private/hook.go +++ b/modules/private/hook.go @@ -83,7 +83,7 @@ type HookProcReceiveRefResult struct { HeadBranch string } -func newInternalRequestAPIForHooks(ctx context.Context, hookName, ownerName, repoName string, opts HookOptions) *httplib.Request { +func newInternalRequestAPIForHooks(ctx context.Context, hookName, ownerName, repoName string, opts HookOptions) *httplib.ClientRequest { reqURL := setting.LocalURL + fmt.Sprintf("api/internal/hook/%s/%s/%s", hookName, url.PathEscape(ownerName), url.PathEscape(repoName)) req := newInternalRequestAPI(ctx, reqURL, "POST", opts) // This "timeout" applies to http.Client's timeout: A Timeout of zero means no timeout. diff --git a/modules/private/internal.go b/modules/private/internal.go index fe74378eb3d..146b4d97990 100644 --- a/modules/private/internal.go +++ b/modules/private/internal.go @@ -89,7 +89,7 @@ var internalAPITransport = sync.OnceValue(func() http.RoundTripper { } }) -func NewInternalRequest(ctx context.Context, url, method string) *httplib.Request { +func NewInternalRequest(ctx context.Context, url, method string) *httplib.ClientRequest { if setting.InternalToken == "" { log.Fatal(`The INTERNAL_TOKEN setting is missing from the configuration file: %q. Ensure you are running in the correct environment or set the correct configuration file with -c.`, setting.CustomConf) @@ -99,14 +99,14 @@ Ensure you are running in the correct environment or set the correct configurati log.Fatal("Invalid internal request URL: %q", url) } - return httplib.NewRequest(url, method). + return httplib.NewClientRequest(method, url). SetContext(ctx). SetTransport(internalAPITransport()). Header("X-Real-IP", getClientIP()). Header("X-Gitea-Internal-Auth", "Bearer "+setting.InternalToken) } -func newInternalRequestAPI(ctx context.Context, url, method string, body ...any) *httplib.Request { +func newInternalRequestAPI(ctx context.Context, url, method string, body ...any) *httplib.ClientRequest { req := NewInternalRequest(ctx, url, method) if len(body) == 1 { req.Header("Content-Type", "application/json") diff --git a/modules/private/request.go b/modules/private/request.go index 057940496ac..fb209a8f4a2 100644 --- a/modules/private/request.go +++ b/modules/private/request.go @@ -52,7 +52,7 @@ func (re responseError) Error() string { // * If the "res" is a struct pointer, the response will be parsed as JSON // * If the "res" is ResponseText pointer, the response will be stored as text in it // * If the "res" is responseCallback pointer, the callback function should set the ResponseExtra fields accordingly -func requestJSONResp[T any](req *httplib.Request, res *T) (ret *T, extra ResponseExtra) { +func requestJSONResp[T any](req *httplib.ClientRequest, res *T) (ret *T, extra ResponseExtra) { resp, err := req.Response() if err != nil { extra.UserMsg = "Internal Server Connection Error" @@ -118,7 +118,7 @@ func requestJSONResp[T any](req *httplib.Request, res *T) (ret *T, extra Respons // requestJSONClientMsg sends a request to the gitea server, server only responds text message status=200 with "success" body // If the request succeeds (200), the argument clientSuccessMsg will be used as ResponseExtra.UserMsg. -func requestJSONClientMsg(req *httplib.Request, clientSuccessMsg string) ResponseExtra { +func requestJSONClientMsg(req *httplib.ClientRequest, clientSuccessMsg string) ResponseExtra { _, extra := requestJSONResp(req, &ResponseText{}) if extra.HasError() { return extra diff --git a/modules/public/vitedev.go b/modules/public/vitedev.go index df7c0e1d5e1..a93eac5af28 100644 --- a/modules/public/vitedev.go +++ b/modules/public/vitedev.go @@ -124,7 +124,7 @@ func IsViteDevMode() bool { return false } - req := httplib.NewRequest(viteDevServerBaseURL+"/web_src/js/__vite_dev_server_check", "GET") + req := httplib.NewClientRequest(http.MethodGet, viteDevServerBaseURL+"/web_src/js/__vite_dev_server_check") resp, _ := req.Response() if resp != nil { _ = resp.Body.Close()