Fix Beachfront data race condition

[mansinahar]

No description provided.

[SyntaxNode]

@guscarreon discovered the json tests did not catch this case due to lack of test coverage. Can you please add a test for this condition?

[mansinahar]

I confirmed that without these changes, the following JSON test fails with a data race condition as expected. Thanks @guscarreon

[mansinahar]

@muncha We found a race condition in the Beachfront adapter code. This PR is to fix that. Can you please take a look as well? Thanks!

[guscarreon]

LGTM. Waiting for @muncha's review to approve

[guscarreon]

@muncha is appCopy.Domain = fmt.Sprintf("%s.%s", chunks[len(chunks)-(len(chunks)-1)], chunks[0]) (adapters/beachfront/beachfront.go line 453) correct? Shouldn't the domain here be "domain.us" instead?

[muncha]

@guscarreon @mansinahar
Yes it should. Looks like a copy paste error.
LGTM - thanks for catching that.

[guscarreon]

@muncha line 53 below gets asserted against your adapter's MakeRequests(request *openrtb2.BidRequest, reqInfo *adapters.ExtraRequestInfo). Therefore, if we change adapters/beachfront/beachfronttest/exemplary/adm-video-app.json's line 53 to "domain.us", the test will fail. This issue should be addressed by correcting adapters/beachfront/beachfront.go's line 453 shown below:

447         if bfReqs[i].Request.App != nil && bfReqs[i].Request.App.Domain == "" && bfReqs[i].Request.App.Bundle != "" {
448             if bfReqs[i].Request.App.Bundle != "" {
449                 var chunks = strings.Split(strings.Trim(bfReqs[i].Request.App.Bundle, "_"), ".")
450
451                 if len(chunks) > 1 {
452                     appCopy := *bfReqs[i].Request.App
453                     appCopy.Domain = fmt.Sprintf("%s.%s", chunks[len(chunks)-(len(chunks)-1)], chunks[0])
454                     bfReqs[i].Request.App = &appCopy
455                 }
456             }
457         }
adapters/beachfront/beachfront.go

My team mate @mansinahar is writting the correction and will be included as part of this pull request for you to review and approve

[mansinahar]

@muncha I am guessing this needs to be something like what you do for site on L442, right? The getDomain method strips out the protocol and the path. If it is, then I can update the PR to use the getDomain method for app bundle too.

Also, if it would be easier for you to push a commit on this PR for this then please feel free to do so. Just lemme know.

[muncha]

Hi all, sorry for the delay. I'm pulling this branch and working on it now. To specifically answer @mansinahar -
"the current code resolves the app bundle “some.domain.us/some/page.html” to app domain as “domain.some”, which doesn’t seem right to me"

Nope, not right. It should be domain.us.

[mansinahar]

Perfect! Thank you so much @muncha. Feel free to push a commit on this PR or close this and open a new one that addresses both the data race and the app domain resolution. Let us know what works best for you :)

[muncha]

@mansinahar
I'd rather push to this PR. I'm not sure I know how to do that, though. I've got your fork as a remote, and I've got beachfront-data-race cloned. So, I fix it, then what? Do I push it to /beachfront/prebid-server with the same branch name and there is a hook that will alert this PR? Or more to it than that?

[mansinahar]

I believe you should just be able to push it to this branch on my remote doing something like:

git push <my-remote-name> <local-branch-name>:<beachfront-data-race>

In case you run into issues with permissions and such then lemme know

There's also a github gist on this topic that might be useful: https://gist.github.com/elfrank/c08256de9c15e41e1781

[muncha]

@mansinahar
This was my bad. I said LGTM above, not realizing the problem.

In that test it's sending a page url in for App.Bundle, but that's not what a bundle is supposed to be:
(/go/pkg/mod/github.com/mxm!cherry/openrtb/v15@v15.0.0/openrtb2/app.go)

// Description:
//   A platform-specific application identifier intended to be
//   unique to the app and independent of the exchange. On
//   Android, this should be a bundle or package name (e.g.,
//   **com.foo.mygame**). On iOS, it is typically a numeric ID
Bundle string `json:"bundle,omitempty"

So really, the test is bad, not the Bundle processing:

• Valid: an android bundle : { "bundle": "us.domain.some" } the domain is set to "domain.us".
• Valid: a numeric ID it passes through unharmed, which is what it should do. { "bundle": "id1234567890" }
• Invalid `{ "bundle": "some.domain.us/some/page.html"}, GIGO, "domain.some"
  • To check for that, I would need to check if
    • The passed bundle starts with a TLD, therefore a valid android bundle.
    • Or the passed bundle value matches a regex something like /^id[0-9]+/ therefore a valid iOS bundle

I'm not sure that I should check for that, though. What would a windows phone send? I'm sure I could find out, but I think that users would know the correct bundle format to send.

Does that seem reasonable?

Also - permission denied on your remote.

[muncha]

@mansinahar
I can't push to your remote, but I did go ahead an push that up to beachfront's, so you should be able to pull and merge.

https://github.com/beachfront/prebid-server/tree/beachfront-data-race

[SyntaxNode]

@muncha Friendly ping. There's a question for you in the latest conversation.

[mansinahar]

@muncha thanks for the clarification. I pushed your commits on this branch but I believe this test will fail because based on the malformed bundle above, the domain here will get set to: domain.some. It might be worth it to add some validation for the app bundle in the code in order to avoid this

[muncha]

@mansinahar
Please try again with the most recent commit, which is the one that I thought was up there, but apparently it was not.

[muncha]

@mansinahar
"It might be worth it to add some validation for the app bundle in the code in order to avoid this"
Agreed.

Just Googled a bit, and the Android bundle is not required to start with a TLD, which might be an expensive thing to validate completely without false positives. Say someone has an app developed by the Vatican - .va has to pass, but what about .lu?

It's much simpler than that.

Maybe this suggests some validation at an application level?

[mansinahar]

@muncha Well, Prebid Server just passes the app bundle information as is. In this case the beachfront adapter is using that to resolve the app domain and if sending an invalid app domain would cause the request to be marked invalid on your end then I'd say it'd be the beachfront adapter's responsibility to do that validation. If this won't affect anything on your side and you're okay with an invalid domain being sent in case of an invalid app bundle then the way you have the code right now should be fine.

[muncha]

@mansinahar
Cool. I'll check what effect an invalid domain may have on our ad server end. I think it is not a problem, but if it is I'll put an issue in and get on it.

[muncha]

@mansinahar ...or just add it in here if needed, if you think that's better.

[muncha]

@mansinahar
I went ahead and did a bundle validation branch off of this one:
https://github.com/beachfront/prebid-server/tree/validateBundle

It seems to do what it needs to do. Maybe you want to merge it in here, maybe I do a PR once this is merged?

I still need to find out from beachfront if I should delete seemingly invalid bundle values or just pass it on, plus a test or two.

[muncha]

@mansinahar
They say if I can't figure out what it is, pass it anyway. validateBundle branch is now updated accordingly.

[mansinahar]

@muncha So let's get this one in and you can do a follow-up PR for validation as needed. We are cutting out a tag today and I would like to get this in if possible

[guscarreon]

LGTM