fix: add default timeout and handle errors for HaveIBeenPwned API (#39316)

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

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 := pwn.New().CheckPassword(ctx, password, true)
-	count, err := client.CheckPassword(password, true)
 	if err != nil {
 		return ErrIsPwnedRequest{err}
 	}

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/"
+const (
-
+	passwordURL     = "https://api.pwnedpasswords.com/range/"
-// ErrEmptyPassword is an empty password error
+	maxResponseSize = 1 << 20
-var ErrEmptyPassword = errors.New("password cannot be empty")
+)
 
 // Client is a HaveIBeenPwned client
 type Client struct {
-	ctx  context.Context
+	mockTransport http.RoundTripper
-	http *http.Client
 }
 
-// New returns a new HaveIBeenPwned Client
+func New() *Client {
-func New(options ...ClientOption) *Client {
+	return &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
 }
 
 // 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)
+	req := httplib.NewClientRequest(http.MethodGet, fmt.Sprintf("%s%s", passwordURL, prefix))
-	if err != nil {
+	req.SetContext(ctx).SetTransport(c.mockTransport)
-		return -1, nil
+	req.Header("User-Agent", "Gitea "+setting.AppVer)
-	}
 	if padding {
-		req.Header.Add("Add-Padding", "true")
+		req.Header("Add-Padding", "true")
 	}
-
+	resp, err := req.Response()
-	resp, err := c.http.Do(req)
-	if err != nil {
-		return -1, err
-	}
-
-	body, err := io.ReadAll(resp.Body)
 	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 {

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)
+	count, err := client.CheckPassword(ctx, "", false)
-	assert.ErrorIs(t, err, ErrEmptyPassword, "blank input should return ErrEmptyPassword")
+	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)
 }

modules/httplib/request.go

@@ -30,16 +30,10 @@ func DialContextWithTimeout(timeout time.Duration) func(ctx context.Context, net
 	}
 }
 
-func NewRequest(url, method string) *Request {
+func NewClientRequest(method, url string) *ClientRequest {
-	return &Request{
+	return &ClientRequest{
-		url: url,
+		url:    url,
-		req: &http.Request{
+		req:    &http.Request{Method: method, Header: make(http.Header)},
-			Method:     method,
-			Header:     make(http.Header),
-			Proto:      "HTTP/1.1", // FIXME: from legacy httplib, it shouldn't be hardcoded
-			ProtoMajor: 1,
-			ProtoMinor: 1,
-		},
 		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)
 }

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
 	}

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.

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")

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

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()