Harden HTTP Response Body Handling #4489

SyntaxNode · 2025-08-19T04:22:55Z

No description provided.

SyntaxNode · 2025-08-19T04:23:22Z

adapters/audienceNetwork/facebook.go

@@ -433,7 +433,7 @@ func Builder(bidderName openrtb_ext.BidderName, config config.Adapter, server co
 	return bidder, nil
 }

-func (a *adapter) MakeTimeoutNotification(req *adapters.RequestData) (*adapters.RequestData, []error) {
+func (a *adapter) MakeTimeoutNotification(req *adapters.RequestData) (*adapters.RequestData, error) {


Not directly related. Updated the function signature since only a single error is used.

github-actions · 2025-08-19T04:23:39Z

Code coverage summary

Note:

Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
Coverage summary encompasses all commits leading up to the latest one, 2f114c1

audienceNetwork

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:45:	MakeRequests			100.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:67:	buildRequests			76.9%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:121:	makeAuthID			100.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:128:	modifyRequest			89.5%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:169:	modifyImp			100.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:217:	extractPlacementAndPublisher	85.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:268:	modifyImpCustom			75.9%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:328:	MakeBids			96.3%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:387:	resolveBidType			75.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:397:	resolveImpType			77.8%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:419:	Builder				100.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:436:	MakeTimeoutNotification		90.0%
total:										(statements)			87.5%

SyntaxNode · 2025-08-19T04:24:20Z

analytics/agma/sender.go

+		defer func() {
+			if _, err := io.Copy(io.Discard, resp.Body); err != nil {
+				glog.Errorf("[agmaAnalytics] Draining response body failed: %v", err)
+			}


The response body must be fully read and closed to ensure connection reuse. Discarding the body is especially needed when the body content is ignored / not used.

SyntaxNode · 2025-08-19T04:25:00Z

floors/fetcher.go

@@ -238,13 +238,13 @@ func (f *PriceFloorFetcher) fetchAndValidate(config config.AccountFloorFetch) (*
 	}

 	if len(floorResp) > (config.MaxFileSizeKB * 1024) {
-		glog.Errorf("Recieved invalid floor data from URL: %s, reason : floor file size is greater than MaxFileSize", config.URL)
+		glog.Errorf("Received invalid floor data from URL: %s, reason : floor file size is greater than MaxFileSize", config.URL)


Unrelated typo spotted while editing this file.

github-actions · 2025-08-19T04:33:29Z

Code coverage summary

Note:

Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
Coverage summary encompasses all commits leading up to the latest one, 5d01bb2

audienceNetwork

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:45:	MakeRequests			100.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:67:	buildRequests			76.9%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:121:	makeAuthID			100.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:128:	modifyRequest			89.5%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:169:	modifyImp			100.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:217:	extractPlacementAndPublisher	85.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:268:	modifyImpCustom			75.9%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:328:	MakeBids			96.3%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:387:	resolveBidType			75.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:397:	resolveImpType			77.8%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:419:	Builder				100.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:436:	MakeTimeoutNotification		90.0%
total:										(statements)			87.5%

hhhjort · 2025-08-19T13:00:40Z

analytics/pubstack/config.go

 	"net/http"
 	"net/url"
 	"time"

 	"github.com/docker/go-units"
+	"github.com/golang/glog"
 )

 func fetchConfig(client *http.Client, endpoint *url.URL) (*Configuration, error) {
 	res, err := client.Get(endpoint.String())


Do we want to add a timeout to this fetch while we are here?

I agree we should. I don't know the pubstack analytics module well enough to implement it. Will reach out to pubstack to implement.

Discussed offline. I will reach out to pubstack to see what a reasonable timeout would be and we can update it in another PR.

hhhjort · 2025-08-19T13:01:38Z

analytics/pubstack/eventchannel/sender.go

@@ -27,7 +28,12 @@ func NewHttpSender(client *http.Client, endpoint string) Sender {
 		if err != nil {


Do we want to add a timeout just above to the http request?

I agree we should. I don't know the pubstack analytics module well enough to implement it. Will reach out to pubstack to implement.

hhhjort · 2025-08-19T13:03:04Z

currency/rate_converter.go

@@ -52,14 +52,20 @@ func (rc *RateConverter) fetch() (*Rates, error) {
 	if err != nil {


Do we want to add a timeout to the http request just above?

Yes. Good catch. I added a configurable timeout with a default of 60 seconds. In test from my machine, latency is around 30ms. Since this isn't part of the real time auction flow, happy to set this to a higher limit considering there is no limit today. Hosts can reduce if they find it necessary or we can revisit the default later if there are no concerns right now.

Discussed offline. We can address this in a follow-up PR.

hhhjort · 2025-08-19T13:09:38Z

Looks good other than optionally adding timeouts to the calls that have no timeout context.

guscarreon · 2025-08-19T14:46:11Z

floors/fetcher.go

+		if _, err := io.Copy(io.Discard, httpResp.Body); err != nil {
+			glog.Errorf("Error while draining fetched floor response body: %v", err)
+		}
+		httpResp.Body.Close()


Is Body.Close() neccessary? We do call Close() on this body in unpackResponse()s caller function FetchRequests(...)

80 func (fetcher *HttpFetcher) FetchRequests(ctx context.Context, requestIDs []string, impIDs []string) (requestData map[string]json.RawMessage, impData map[string]json.RawMessage, errs []error) { 81 *-- 8 lines: if len(requestIDs) == 0 && len(impIDs) == 0 {--------------------------------------------------------------------------------------------------------- 89 90 httpResp, err := ctxhttp.Do(ctx, fetcher.client, httpReq) 91 if err != nil { 92 return nil, nil, []error{err} 93 } 94 defer httpResp.Body.Close() 95 requestData, impData, errs = unpackResponse(httpResp) 96 return 97 } stored_requests/backends/http_fetcher/fetcher.go

Do we get a panic for calling Close() twice?

Not sure I follow. unpackResponse does not close the body.

Line 94 inside FetchRequests(...) defers the call to httpResp.Body.Close(). Is it ok if unpackResponse(httpResp) calls httpResp.Body.Close() and we do another httpResp.Body.Close() before FetchRequests(...) goes out of scope?

I got this one wrong @SyntaxNode. Nevermind

github-actions · 2025-08-19T16:08:35Z

Code coverage summary

Note:

Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
Coverage summary encompasses all commits leading up to the latest one, 618b307

audienceNetwork

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:45:	MakeRequests			100.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:67:	buildRequests			76.9%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:121:	makeAuthID			100.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:128:	modifyRequest			89.5%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:169:	modifyImp			100.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:217:	extractPlacementAndPublisher	85.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:268:	modifyImpCustom			75.9%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:328:	MakeBids			96.3%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:387:	resolveBidType			75.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:397:	resolveImpType			77.8%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:419:	Builder				100.0%
github.com/prebid/prebid-server/v3/adapters/audienceNetwork/facebook.go:436:	MakeTimeoutNotification		90.0%
total:										(statements)			87.5%

hhhjort

LGTM

harden http response body handling

2f114c1

SyntaxNode commented Aug 19, 2025

View reviewed changes

hardened captcha response handling

5d01bb2

SyntaxNode added the do not port label Aug 19, 2025

VeronikaSolovei9 previously approved these changes Aug 19, 2025

View reviewed changes

hhhjort reviewed Aug 19, 2025

View reviewed changes

guscarreon reviewed Aug 19, 2025

View reviewed changes

add rate converter http timeout

618b307

SyntaxNode dismissed VeronikaSolovei9’s stale review via 618b307 August 19, 2025 16:07

hhhjort approved these changes Aug 19, 2025

View reviewed changes

guscarreon approved these changes Aug 19, 2025

View reviewed changes

bsardo merged commit 4e44ab4 into prebid:master Aug 19, 2025
6 checks passed

		@@ -27,7 +28,12 @@ func NewHttpSender(client *http.Client, endpoint string) Sender {
		if err != nil {

		@@ -52,14 +52,20 @@ func (rc RateConverter) fetch() (Rates, error) {
		if err != nil {

Harden HTTP Response Body Handling #4489

Harden HTTP Response Body Handling #4489

Uh oh!

Conversation

SyntaxNode commented Aug 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 19, 2025

Code coverage summary

audienceNetwork

Uh oh!

SyntaxNode Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 19, 2025

Code coverage summary

audienceNetwork

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hhhjort commented Aug 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guscarreon Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 19, 2025

Code coverage summary

audienceNetwork

Uh oh!

hhhjort left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SyntaxNode Aug 19, 2025 •

edited

Loading

guscarreon Aug 19, 2025 •

edited

Loading