From fb80c7cdcad5f01f1b07e0ed1604368efb04166c Mon Sep 17 00:00:00 2001 From: bsardo <1168933+bsardo@users.noreply.github.com> Date: Mon, 5 Apr 2021 13:43:44 -0400 Subject: [PATCH 1/5] Clone request headers and delete authorization header in debug mode --- exchange/bidder.go | 13 +++++-------- exchange/bidder_test.go | 18 +++++++++--------- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/exchange/bidder.go b/exchange/bidder.go index 5b6da499093..ec044cb8f3c 100644 --- a/exchange/bidder.go +++ b/exchange/bidder.go @@ -167,10 +167,6 @@ func (bidder *bidderAdapter) requestBid(ctx context.Context, request *openrtb2.B if debugInfo := ctx.Value(DebugContextKey); debugInfo != nil && debugInfo.(bool) { if accountDebugAllowed { if bidder.config.DebugInfo.Allow { - // it's safe to mutate the request headers since from this point on the - // information is only used for debugging. - removeSensitiveHeaders(httpInfo.request.Headers) - seatBid.httpCalls = append(seatBid.httpCalls, makeExt(httpInfo)) } else { debugDisabledWarning := errortypes.Warning{ @@ -331,9 +327,10 @@ func getAssetByID(id int64, assets []nativeRequests.Asset) (nativeRequests.Asset var authorizationHeader = http.CanonicalHeaderKey("authorization") -// removeSensitiveHeaders mutates the http header object to remove sensitive information. -func removeSensitiveHeaders(h http.Header) { - h.Del(authorizationHeader) +func filterHeader(h http.Header) map[string][]string { + clone := h.Clone() + clone.Del(authorizationHeader) + return clone } // makeExt transforms information about the HTTP call into the contract class for the PBS response. @@ -343,7 +340,7 @@ func makeExt(httpInfo *httpCallInfo) *openrtb_ext.ExtHttpCall { if httpInfo != nil && httpInfo.request != nil { ext.Uri = httpInfo.request.Uri ext.RequestBody = string(httpInfo.request.Body) - ext.RequestHeaders = httpInfo.request.Headers + ext.RequestHeaders = filterHeader(httpInfo.request.Headers) if httpInfo.err == nil && httpInfo.response != nil { ext.ResponseBody = string(httpInfo.response.Body) diff --git a/exchange/bidder_test.go b/exchange/bidder_test.go index 7c01cd84c83..a16ade4ec84 100644 --- a/exchange/bidder_test.go +++ b/exchange/bidder_test.go @@ -1029,11 +1029,11 @@ func TestMakeExt(t *testing.T) { } } -func TestRemoveSensitiveHeaders(t *testing.T) { +func TestFilterHeader(t *testing.T) { testCases := []struct { description string given http.Header - expected http.Header + expected map[string][]string }{ { description: "Nil", @@ -1048,33 +1048,33 @@ func TestRemoveSensitiveHeaders(t *testing.T) { { description: "One", given: makeHeader(map[string][]string{"Key1": {"value1"}}), - expected: makeHeader(map[string][]string{"Key1": {"value1"}}), + expected: map[string][]string{"Key1": {"value1"}}, }, { description: "Many", given: makeHeader(map[string][]string{"Key1": {"value1"}, "Key2": {"value2a", "value2b"}}), - expected: makeHeader(map[string][]string{"Key1": {"value1"}, "Key2": {"value2a", "value2b"}}), + expected: map[string][]string{"Key1": {"value1"}, "Key2": {"value2a", "value2b"}}, }, { description: "Authorization Header Omitted", given: makeHeader(map[string][]string{"authorization": {"secret"}}), - expected: http.Header{}, + expected: map[string][]string{}, }, { description: "Authorization Header Omitted - Case Insensitive", given: makeHeader(map[string][]string{"AuThOrIzAtIoN": {"secret"}}), - expected: http.Header{}, + expected: map[string][]string{}, }, { description: "Authorization Header Omitted + Other Keys", given: makeHeader(map[string][]string{"authorization": {"secret"}, "Key1": {"value1"}}), - expected: makeHeader(map[string][]string{"Key1": {"value1"}}), + expected: map[string][]string{"Key1": {"value1"}}, }, } for _, test := range testCases { - removeSensitiveHeaders(test.given) - assert.Equal(t, test.expected, test.given, test.description) + result := filterHeader(test.given) + assert.Equal(t, test.expected, result, test.description) } } From 4cb6019694301e2691900b263385b905ef46d33b Mon Sep 17 00:00:00 2001 From: bsardo <1168933+bsardo@users.noreply.github.com> Date: Mon, 5 Apr 2021 13:44:44 -0400 Subject: [PATCH 2/5] Update TestMakeExt with cases verifying authorization header removal and nil header obj --- exchange/bidder_test.go | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/exchange/bidder_test.go b/exchange/bidder_test.go index a16ade4ec84..9bc9bc795be 100644 --- a/exchange/bidder_test.go +++ b/exchange/bidder_test.go @@ -953,13 +953,13 @@ func TestMakeExt(t *testing.T) { expected: &openrtb_ext.ExtHttpCall{}, }, { - description: "Request & Response - No Error", + description: "Request & Response - No Error with Authorization removal", given: &httpCallInfo{ err: nil, request: &adapters.RequestData{ Uri: "requestUri", Body: []byte("requestBody"), - Headers: makeHeader(map[string][]string{"Key1": {"value1", "value2"}}), + Headers: makeHeader(map[string][]string{"Key1": {"value1", "value2"}, "Authorization": {"secret"}}), }, response: &adapters.ResponseData{ Body: []byte("responseBody"), @@ -974,6 +974,28 @@ func TestMakeExt(t *testing.T) { Status: 999, }, }, + { + description: "Request & Response - No Error with nil header", + given: &httpCallInfo{ + err: nil, + request: &adapters.RequestData{ + Uri: "requestUri", + Body: []byte("requestBody"), + Headers: nil, + }, + response: &adapters.ResponseData{ + Body: []byte("responseBody"), + StatusCode: 999, + }, + }, + expected: &openrtb_ext.ExtHttpCall{ + Uri: "requestUri", + RequestBody: "requestBody", + RequestHeaders: nil, + ResponseBody: "responseBody", + Status: 999, + }, + }, { description: "Request & Response - Error", given: &httpCallInfo{ From e3851cecdf48dade0a0f9956de4034b089f6ce37 Mon Sep 17 00:00:00 2001 From: bsardo <1168933+bsardo@users.noreply.github.com> Date: Mon, 5 Apr 2021 15:31:35 -0400 Subject: [PATCH 3/5] Remove obselete test --- exchange/bidder_test.go | 42 ----------------------------------------- 1 file changed, 42 deletions(-) diff --git a/exchange/bidder_test.go b/exchange/bidder_test.go index 9bc9bc795be..03e1ee87214 100644 --- a/exchange/bidder_test.go +++ b/exchange/bidder_test.go @@ -141,48 +141,6 @@ func TestSingleBidder(t *testing.T) { } } -func TestRequestBidRemovesSensitiveHeaders(t *testing.T) { - server := httptest.NewServer(mockHandler(200, "getBody", "responseJson")) - defer server.Close() - - requestHeaders := http.Header{} - requestHeaders.Add("Content-Type", "application/json") - requestHeaders.Add("Authorization", "anySecret") - - bidderImpl := &goodSingleBidder{ - httpRequest: &adapters.RequestData{ - Method: "POST", - Uri: server.URL, - Body: []byte("requestJson"), - Headers: requestHeaders, - }, - bidResponse: &adapters.BidderResponse{ - Bids: []*adapters.TypedBid{}, - }, - } - - debugInfo := &config.DebugInfo{Allow: true} - ctx := context.Background() - ctx = context.WithValue(ctx, DebugContextKey, true) - - bidder := adaptBidder(bidderImpl, server.Client(), &config.Configuration{}, &metricsConfig.DummyMetricsEngine{}, openrtb_ext.BidderAppnexus, debugInfo) - currencyConverter := currency.NewRateConverter(&http.Client{}, "", time.Duration(0)) - seatBid, errs := bidder.requestBid(ctx, &openrtb2.BidRequest{}, "test", 1, currencyConverter.Rates(), &adapters.ExtraRequestInfo{}, true) - - expectedHttpCalls := []*openrtb_ext.ExtHttpCall{ - { - Uri: server.URL, - RequestBody: "requestJson", - RequestHeaders: map[string][]string{"Content-Type": {"application/json"}}, - ResponseBody: "responseJson", - Status: 200, - }, - } - - assert.Empty(t, errs) - assert.ElementsMatch(t, seatBid.httpCalls, expectedHttpCalls) -} - // TestMultiBidder makes sure all the requests get sent, and the responses processed. // Because this is done in parallel, it should be run under the race detector. func TestMultiBidder(t *testing.T) { From 18bf74efbf66f3adf57fddb208a9d366895640c5 Mon Sep 17 00:00:00 2001 From: bsardo <1168933+bsardo@users.noreply.github.com> Date: Mon, 5 Apr 2021 22:20:53 -0400 Subject: [PATCH 4/5] Re-add deleted tests --- exchange/bidder_test.go | 64 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/exchange/bidder_test.go b/exchange/bidder_test.go index 03e1ee87214..724cd0886ac 100644 --- a/exchange/bidder_test.go +++ b/exchange/bidder_test.go @@ -141,6 +141,48 @@ func TestSingleBidder(t *testing.T) { } } +func TestRequestBidRemovesSensitiveHeaders(t *testing.T) { + server := httptest.NewServer(mockHandler(200, "getBody", "responseJson")) + defer server.Close() + + requestHeaders := http.Header{} + requestHeaders.Add("Content-Type", "application/json") + requestHeaders.Add("Authorization", "anySecret") + + bidderImpl := &goodSingleBidder{ + httpRequest: &adapters.RequestData{ + Method: "POST", + Uri: server.URL, + Body: []byte("requestJson"), + Headers: requestHeaders, + }, + bidResponse: &adapters.BidderResponse{ + Bids: []*adapters.TypedBid{}, + }, + } + + debugInfo := &config.DebugInfo{Allow: true} + ctx := context.Background() + ctx = context.WithValue(ctx, DebugContextKey, true) + + bidder := adaptBidder(bidderImpl, server.Client(), &config.Configuration{}, &metricsConfig.DummyMetricsEngine{}, openrtb_ext.BidderAppnexus, debugInfo) + currencyConverter := currency.NewRateConverter(&http.Client{}, "", time.Duration(0)) + seatBid, errs := bidder.requestBid(ctx, &openrtb2.BidRequest{}, "test", 1, currencyConverter.Rates(), &adapters.ExtraRequestInfo{}, true) + + expectedHttpCalls := []*openrtb_ext.ExtHttpCall{ + { + Uri: server.URL, + RequestBody: "requestJson", + RequestHeaders: map[string][]string{"Content-Type": {"application/json"}}, + ResponseBody: "responseJson", + Status: 200, + }, + } + + assert.Empty(t, errs) + assert.ElementsMatch(t, seatBid.httpCalls, expectedHttpCalls) +} + // TestMultiBidder makes sure all the requests get sent, and the responses processed. // Because this is done in parallel, it should be run under the race detector. func TestMultiBidder(t *testing.T) { @@ -910,6 +952,28 @@ func TestMakeExt(t *testing.T) { }, expected: &openrtb_ext.ExtHttpCall{}, }, + { + description: "Request & Response - No Error", + given: &httpCallInfo{ + err: nil, + request: &adapters.RequestData{ + Uri: "requestUri", + Body: []byte("requestBody"), + Headers: makeHeader(map[string][]string{"Key1": {"value1", "value2"}}), + }, + response: &adapters.ResponseData{ + Body: []byte("responseBody"), + StatusCode: 999, + }, + }, + expected: &openrtb_ext.ExtHttpCall{ + Uri: "requestUri", + RequestBody: "requestBody", + RequestHeaders: map[string][]string{"Key1": {"value1", "value2"}}, + ResponseBody: "responseBody", + Status: 999, + }, + }, { description: "Request & Response - No Error with Authorization removal", given: &httpCallInfo{ From b836b63b8be47d8d375339e22e00cfa23b3514cd Mon Sep 17 00:00:00 2001 From: bsardo <1168933+bsardo@users.noreply.github.com> Date: Mon, 5 Apr 2021 22:36:42 -0400 Subject: [PATCH 5/5] Set filterHeader return type to http.Header --- exchange/bidder.go | 2 +- exchange/bidder_test.go | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/exchange/bidder.go b/exchange/bidder.go index ec044cb8f3c..9707ddcb4fe 100644 --- a/exchange/bidder.go +++ b/exchange/bidder.go @@ -327,7 +327,7 @@ func getAssetByID(id int64, assets []nativeRequests.Asset) (nativeRequests.Asset var authorizationHeader = http.CanonicalHeaderKey("authorization") -func filterHeader(h http.Header) map[string][]string { +func filterHeader(h http.Header) http.Header { clone := h.Clone() clone.Del(authorizationHeader) return clone diff --git a/exchange/bidder_test.go b/exchange/bidder_test.go index 724cd0886ac..41ca420a433 100644 --- a/exchange/bidder_test.go +++ b/exchange/bidder_test.go @@ -1077,7 +1077,7 @@ func TestFilterHeader(t *testing.T) { testCases := []struct { description string given http.Header - expected map[string][]string + expected http.Header }{ { description: "Nil", @@ -1087,32 +1087,32 @@ func TestFilterHeader(t *testing.T) { { description: "Empty", given: http.Header{}, - expected: map[string][]string{}, + expected: http.Header{}, }, { description: "One", given: makeHeader(map[string][]string{"Key1": {"value1"}}), - expected: map[string][]string{"Key1": {"value1"}}, + expected: makeHeader(map[string][]string{"Key1": {"value1"}}), }, { description: "Many", given: makeHeader(map[string][]string{"Key1": {"value1"}, "Key2": {"value2a", "value2b"}}), - expected: map[string][]string{"Key1": {"value1"}, "Key2": {"value2a", "value2b"}}, + expected: makeHeader(map[string][]string{"Key1": {"value1"}, "Key2": {"value2a", "value2b"}}), }, { description: "Authorization Header Omitted", given: makeHeader(map[string][]string{"authorization": {"secret"}}), - expected: map[string][]string{}, + expected: http.Header{}, }, { description: "Authorization Header Omitted - Case Insensitive", given: makeHeader(map[string][]string{"AuThOrIzAtIoN": {"secret"}}), - expected: map[string][]string{}, + expected: http.Header{}, }, { description: "Authorization Header Omitted + Other Keys", given: makeHeader(map[string][]string{"authorization": {"secret"}, "Key1": {"value1"}}), - expected: map[string][]string{"Key1": {"value1"}}, + expected: makeHeader(map[string][]string{"Key1": {"value1"}}), }, }