Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CORS policy for https://prebid.adnxs.com/pbs/v1/cookie_sync #1198

Closed
wants to merge 2 commits into from

Conversation

guscarreon
Copy link
Contributor

@guscarreon guscarreon commented Feb 21, 2020

A lot of errors related to publisher integrations have been reported because the apparent lack of the Access-Control-Allow-Origin CORS header that is basically a whitelist of the URLs where clients allow to pull content from other source than the origin. In golang's Go CORS handler library, not specifying any value inside the AllowedOrigins array of the Options field, should default to a "*" value which, according to the https://github.com/rs/cors documentation, should suffice to make the header Access-Control-Allow-Origin default to "*". This pull request will explicitly set the Access-Control-Allow-Origin to an "*" value in order to avoid any No 'Access-Control-Allow-Origin' header is present on the requested source errors on the client side.
BEFORE:

(dlv) break router/router.go:309
Breakpoint 1 set at 0x17fe5e4 for github.com/prebid/prebid-server/router.SupportCORS() ./router.go:309
(dlv) c
> github.com/prebid/prebid-server/router.SupportCORS() ./router.go:309 (hits goroutine(8):1 total:1) (PC: 0x17fe5e4)
   304:                 AllowOriginFunc: func(string) bool {
   305:                         return true
   306:                 },
   307:                 //AllowedOrigins: []string{"*", "https://prebid.adnxs.com/pbs/v1/cookie_sync"},
   308:                 AllowedHeaders: []string{"Origin", "X-Requested-With", "Content-Type", "Accept"}})
=> 309:         return c.Handler(handler)
   310: }
   311:
   312: type defReq struct {
   313:         Ext defExt `json:"ext"`
   314: }
(dlv) p c
*github.com/rs/cors.Cors {
        Log: *log.Logger nil,
        allowedOrigins: []string len: 0, cap: 0, nil,
        allowedWOrigins: []github.com/rs/cors.wildcard len: 0, cap: 0, nil,
        allowOriginFunc: github.com/prebid/prebid-server/router.SupportCORS.func1,
        allowedHeaders: []string len: 5, cap: 8, [
                "Origin",
                "X-Requested-With",
                "Content-Type",
                "Accept",
                "Origin",
        ],
        allowedMethods: []string len: 3, cap: 3, ["GET","POST","HEAD"],
        exposedHeaders: []string len: 0, cap: 0, [],
        maxAge: 0,
        allowedOriginsAll: false,
        allowedHeadersAll: false,
        allowCredentials: true,
        optionPassthrough: false,}
(dlv)
before.txt

AFTER

Type 'help' for list of commands.
(dlv) break router/router.go:309
Breakpoint 1 set at 0x17fe672 for github.com/prebid/prebid-server/router.SupportCORS() ./router.go:309
(dlv) c
> github.com/prebid/prebid-server/router.SupportCORS() ./router.go:309 (hits goroutine(9):1 total:1) (PC: 0x17fe672)
   304:                 AllowOriginFunc: func(string) bool {
   305:                         return true
   306:                 },
   307:                 AllowedOrigins: []string{"*"},
   308:                 AllowedHeaders: []string{"Origin", "X-Requested-With", "Content-Type", "Accept"}})
=> 309:         return c.Handler(handler)
   310: }
   311:
   312: type defReq struct {
   313:         Ext defExt `json:"ext"`
   314: }
(dlv) p c
*github.com/rs/cors.Cors {
        Log: *log.Logger nil,
        allowedOrigins: []string len: 0, cap: 0, nil,
        allowedWOrigins: []github.com/rs/cors.wildcard len: 0, cap: 0, nil,
        allowOriginFunc: github.com/prebid/prebid-server/router.SupportCORS.func1,
        allowedHeaders: []string len: 5, cap: 8, [
                "Origin",
                "X-Requested-With",
                "Content-Type",
                "Accept",
                "Origin",
        ],
        allowedMethods: []string len: 3, cap: 3, ["GET","POST","HEAD"],
        exposedHeaders: []string len: 0, cap: 0, [],
        maxAge: 0,
        allowedOriginsAll: true,
        allowedHeadersAll: false,
        allowCredentials: true,
        optionPassthrough: false,}
(dlv)
after.txt

Difference:

╰─➤  diff before.txt after.txt
4c4
< > github.com/prebid/prebid-server/router.SupportCORS() ./router.go:309 (hits goroutine(8):1 total:1) (PC: 0x17fe5e4)
---
> > github.com/prebid/prebid-server/router.SupportCORS() ./router.go:309 (hits goroutine(9):1 total:1) (PC: 0x17fe672)
8c8
<    307:                 //AllowedOrigins: []string{"*", "https://prebid.adnxs.com/pbs/v1/cookie_sync"},
---
>    307:                 AllowedOrigins: []string{"*"},
32c32
<         allowedOriginsAll: false,
---
>         allowedOriginsAll: true,

Copy link
Collaborator

@hhhjort hhhjort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change itself should be good. Was there testing that this header was not being set to "*", or is that just a guess at the root cause?

Copy link
Contributor

@SyntaxNode SyntaxNode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should close this PR. The proposed fix will break cookie sync in pretty much every browser.

@@ -304,6 +304,7 @@ func SupportCORS(handler http.Handler) http.Handler {
AllowOriginFunc: func(string) bool {
return true
},
AllowedOrigins: []string{"*"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually can't do this. It will break in browsers. See: rs/cors#55

Copy link
Collaborator

@hhhjort hhhjort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't do this based on Scott's findings.

@SyntaxNode
Copy link
Contributor

This PR would cause browsers to block the cookie sync endpoint responses via CORS.

@SyntaxNode SyntaxNode closed this Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants