Fix panic observed in setSiteImplicitly function (#3053)

* Issue_3052: Fix panic observed in setSiteImplicitly function * Move the site.nil check at top
prebid · Sep 5, 2023 · 94dc670 · 94dc670
1 parent 4c10fac
commit 94dc670
Show file tree

Hide file tree

Showing 3 changed files with 63 additions and 33 deletions.
diff --git a/endpoints/openrtb2/auction.go b/endpoints/openrtb2/auction.go
@@ -6,7 +6,6 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"github.com/prebid/prebid-server/privacy"
 	"io"
 	"net/http"
 	"net/url"
@@ -15,6 +14,8 @@ import (
 	"strings"
 	"time"
 
+	"github.com/prebid/prebid-server/privacy"
+
 	"github.com/buger/jsonparser"
 	"github.com/gofrs/uuid"
 	"github.com/golang/glog"
@@ -1966,8 +1967,12 @@ func setAuctionTypeImplicitly(r *openrtb_ext.RequestWrapper) {
 }
 
 func setSiteImplicitly(httpReq *http.Request, r *openrtb_ext.RequestWrapper) {
+	if r.Site == nil {
+		r.Site = &openrtb2.Site{}
+	}
+
 	referrerCandidate := httpReq.Referer()
-	if referrerCandidate == "" && r.Site != nil && r.Site.Page != "" {
+	if referrerCandidate == "" && r.Site.Page != "" {
 		referrerCandidate = r.Site.Page // If http referer is disabled and thus has empty value - use site.page instead
 	}
 
@@ -1981,17 +1986,13 @@ func setSiteImplicitly(httpReq *http.Request, r *openrtb_ext.RequestWrapper) {
 		}
 	}
 
-	if r.Site != nil {
-		if siteExt, err := r.GetSiteExt(); err == nil && siteExt.GetAmp() == nil {
-			siteExt.SetAmp(&notAmp)
-		}
+	if siteExt, err := r.GetSiteExt(); err == nil && siteExt.GetAmp() == nil {
+		siteExt.SetAmp(&notAmp)
 	}
+
 }
 
 func setSitePageIfEmpty(site *openrtb2.Site, sitePage string) {
-	if site == nil {
-		site = &openrtb2.Site{}
-	}
 	if site.Page == "" {
 		site.Page = sitePage
 	}

diff --git a/endpoints/openrtb2/auction_test.go b/endpoints/openrtb2/auction_test.go
@@ -965,80 +965,109 @@ func TestImplicitDNTEndToEnd(t *testing.T) {
 func TestReferer(t *testing.T) {
 	testCases := []struct {
 		description             string
-		givenSitePage           string
-		givenSiteDomain         string
-		givenPublisherDomain    string
 		givenReferer            string
 		expectedDomain          string
 		expectedPage            string
 		expectedPublisherDomain string
+		bidReq                  *openrtb_ext.RequestWrapper
 	}{
 		{
 			description:             "site.page/domain are unchanged when site.page/domain and http referer are not set",
 			expectedDomain:          "",
 			expectedPage:            "",
 			expectedPublisherDomain: "",
+			bidReq: &openrtb_ext.RequestWrapper{BidRequest: &openrtb2.BidRequest{
+				Site: &openrtb2.Site{
+					Publisher: &openrtb2.Publisher{},
+				},
+			}},
 		},
 		{
 			description:             "site.page/domain are derived from referer when neither is set and http referer is set",
 			givenReferer:            "https://test.somepage.com",
 			expectedDomain:          "test.somepage.com",
 			expectedPublisherDomain: "somepage.com",
 			expectedPage:            "https://test.somepage.com",
+			bidReq: &openrtb_ext.RequestWrapper{BidRequest: &openrtb2.BidRequest{
+				Site: &openrtb2.Site{
+					Publisher: &openrtb2.Publisher{},
+				},
+			}},
 		},
 		{
 			description:             "site.domain is derived from site.page when site.page is set and http referer is not set",
-			givenSitePage:           "https://test.somepage.com",
 			expectedDomain:          "test.somepage.com",
 			expectedPublisherDomain: "somepage.com",
 			expectedPage:            "https://test.somepage.com",
+			bidReq: &openrtb_ext.RequestWrapper{BidRequest: &openrtb2.BidRequest{
+				Site: &openrtb2.Site{
+					Page:      "https://test.somepage.com",
+					Publisher: &openrtb2.Publisher{},
+				},
+			}},
 		},
 		{
 			description:             "site.domain is derived from http referer when site.page and http referer are set",
-			givenSitePage:           "https://test.somepage.com",
 			givenReferer:            "http://test.com",
 			expectedDomain:          "test.com",
 			expectedPublisherDomain: "test.com",
 			expectedPage:            "https://test.somepage.com",
+			bidReq: &openrtb_ext.RequestWrapper{BidRequest: &openrtb2.BidRequest{
+				Site: &openrtb2.Site{
+					Page:      "https://test.somepage.com",
+					Publisher: &openrtb2.Publisher{},
+				},
+			}},
 		},
 		{
 			description:             "site.page/domain are unchanged when site.page/domain and http referer are set",
-			givenSitePage:           "https://test.somepage.com",
-			givenSiteDomain:         "some.domain.com",
 			givenReferer:            "http://test.com",
 			expectedDomain:          "some.domain.com",
 			expectedPublisherDomain: "test.com",
 			expectedPage:            "https://test.somepage.com",
+			bidReq: &openrtb_ext.RequestWrapper{BidRequest: &openrtb2.BidRequest{
+				Site: &openrtb2.Site{
+					Domain:    "some.domain.com",
+					Page:      "https://test.somepage.com",
+					Publisher: &openrtb2.Publisher{},
+				},
+			}},
 		},
 		{
 			description:             "Publisher domain shouldn't be overrwriten if already set",
-			givenSitePage:           "https://test.somepage.com",
-			givenSiteDomain:         "",
-			givenPublisherDomain:    "differentpage.com",
 			expectedDomain:          "test.somepage.com",
 			expectedPublisherDomain: "differentpage.com",
 			expectedPage:            "https://test.somepage.com",
+			bidReq: &openrtb_ext.RequestWrapper{BidRequest: &openrtb2.BidRequest{
+				Site: &openrtb2.Site{
+					Domain: "",
+					Page:   "https://test.somepage.com",
+					Publisher: &openrtb2.Publisher{
+						Domain: "differentpage.com",
+					},
+				},
+			}},
+		},
+		{
+			description:             "request.site is nil",
+			givenReferer:            "http://test.com",
+			expectedDomain:          "test.com",
+			expectedPublisherDomain: "test.com",
+			expectedPage:            "http://test.com",
+			bidReq:                  &openrtb_ext.RequestWrapper{BidRequest: &openrtb2.BidRequest{}},
 		},
 	}
 
 	for _, test := range testCases {
 		httpReq := httptest.NewRequest("POST", "/openrtb2/auction", strings.NewReader(validRequest(t, "site.json")))
 		httpReq.Header.Set("Referer", test.givenReferer)
 
-		bidReq := &openrtb_ext.RequestWrapper{BidRequest: &openrtb2.BidRequest{
-			Site: &openrtb2.Site{
-				Domain:    test.givenSiteDomain,
-				Page:      test.givenSitePage,
-				Publisher: &openrtb2.Publisher{Domain: test.givenPublisherDomain},
-			},
-		}}
-
-		setSiteImplicitly(httpReq, bidReq)
+		setSiteImplicitly(httpReq, test.bidReq)
 
-		assert.NotNil(t, bidReq.Site, test.description)
-		assert.Equal(t, test.expectedDomain, bidReq.Site.Domain, test.description)
-		assert.Equal(t, test.expectedPage, bidReq.Site.Page, test.description)
-		assert.Equal(t, test.expectedPublisherDomain, bidReq.Site.Publisher.Domain, test.description)
+		assert.NotNil(t, test.bidReq.Site, test.description)
+		assert.Equal(t, test.expectedDomain, test.bidReq.Site.Domain, test.description)
+		assert.Equal(t, test.expectedPage, test.bidReq.Site.Page, test.description)
+		assert.Equal(t, test.expectedPublisherDomain, test.bidReq.Site.Publisher.Domain, test.description)
 	}
 }
 

diff --git a/endpoints/openrtb2/sample-requests/invalid-whole/no-site-or-app.json b/endpoints/openrtb2/sample-requests/invalid-whole/no-site-or-app.json
@@ -19,5 +19,5 @@
     ]
   },
   "expectedReturnCode": 400,
-  "expectedErrorMessage": "Invalid request: request.site or request.app must be defined, but not both.\n"
+  "expectedErrorMessage": "Invalid request: request.site should include at least one of request.site.id or request.site.page.\n"
 }