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

[ads] Confirmations should not be sent if the wildcard is in a forbidden part of the URL #38683

Closed
diracdeltas opened this issue May 29, 2024 · 7 comments · Fixed by brave/brave-core#24076
Assignees
Labels
feature/ads OS/Android Fixes related to Android browser functionality OS/Desktop OS/iOS Fixes related to iOS browser functionality priority/P2 A bad problem. We might uplift this to the next planned release. QA/Yes release-notes/exclude security
Projects

Comments

@diracdeltas
Copy link
Member

diracdeltas commented May 29, 2024

Description

See https://bravesoftware.slack.com/archives/CJR5902AW/p1717002819317479?thread_ts=1716931296.694229&cid=CJR5902AW for details. If the view/click/landing/conversion URLs for any ad contains a wildcard, we must validate that the wildcard is in either the URL's path or a subdomain before sending a confirmation event. Wildcards in the "base domain" (eTLD+1) part of the URL are invalid and should not trigger any kind of reporting.

@diracdeltas diracdeltas added security priority/P2 A bad problem. We might uplift this to the next planned release. OS/Desktop labels May 29, 2024
@diracdeltas
Copy link
Member Author

Needs to be done on iOS/Android as well if we support wildcards in conversion/etc URLs on those platforms.

@diracdeltas
Copy link
Member Author

see server side implementation in https://github.com/brave/ads-serve/pull/3957

@diracdeltas
Copy link
Member Author

@tackley Can you confirm what is being done (or going to be done) server-side for third party validations? We should probably copy those in the client as well.

@tmancey tmancey moved this from In progress to Review in Ads Jun 8, 2024
@tmancey tmancey moved this from Review to In progress in Ads Jun 8, 2024
@tackley
Copy link

tackley commented Jun 11, 2024

@tackley Can you confirm what is being done (or going to be done) server-side for third party validations? We should probably copy those in the client as well.

Yes, we will use the same tests cases on both server and client. I'll share them with you for your review once I've got to that point in implementation.

@tmancey tmancey added OS/Android Fixes related to Android browser functionality OS/iOS Fixes related to iOS browser functionality labels Jun 11, 2024
@tmancey
Copy link
Contributor

tmancey commented Jun 11, 2024

@diracdeltas @fmarier @tackley the PR is ready for review and supports desktop, Android and iOS. Thanks

@tmancey
Copy link
Contributor

tmancey commented Jun 11, 2024

@diracdeltas @tackley @fmarier the PR also validates creative set conversions are same site.

@tmancey
Copy link
Contributor

tmancey commented Jun 11, 2024

@diracdeltas we use the same test cases, however, we also support certain brave:// schemes which was approved in a previous privacy review. Thanks

@brave-builds brave-builds added this to the 1.69.x - Nightly milestone Jun 14, 2024
@tmancey tmancey changed the title Ads confirmations should not be sent if the wildcard is in a forbidden part of the URL [ads] Confirmations should not be sent if the wildcard is in a forbidden part of the URL Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/ads OS/Android Fixes related to Android browser functionality OS/Desktop OS/iOS Fixes related to iOS browser functionality priority/P2 A bad problem. We might uplift this to the next planned release. QA/Yes release-notes/exclude security
Projects
Status: Done
Ads
  
In progress
4 participants