-
Notifications
You must be signed in to change notification settings - Fork 845
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 #24076
Conversation
812f05e
to
d33d511
Compare
b38363f
to
813aa29
Compare
components/brave_ads/core/internal/common/url/url_util_internal.cc
Outdated
Show resolved
Hide resolved
components/brave_ads/core/internal/catalog/catalog_url_request_json_reader.cc
Show resolved
Hide resolved
components/brave_ads/core/internal/common/url/url_util_internal.cc
Outdated
Show resolved
Hide resolved
components/brave_ads/core/internal/catalog/catalog_url_request_json_reader.cc
Outdated
Show resolved
Hide resolved
5a93e5b
to
efbbb48
Compare
f258d8d
to
62d387b
Compare
[puLL-Merge] - brave/brave-core@24076 Here is my review of the pull request: DescriptionThis PR makes several changes to improve URL validation and filtering in the Brave Ads codebase. The main motivations appear to be:
Security Hotspots
ChangesChangesurl_util.cc:
url_util_internal.cc:
catalog_url_request_json_reader.cc:
url_util_unittest.cc:
url_util_internal_unittest.cc:
Overall this is a significant security improvement to URL validation in Brave Ads. The changes are well structured and tested. I did not see any major issues or overlooked vulnerabilities. Nice work! Let me know if you have any other questions! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look good to me.
components/brave_ads/core/internal/common/url/url_util_internal.cc
Outdated
Show resolved
Hide resolved
components/brave_ads/core/internal/common/url/url_util_internal.cc
Outdated
Show resolved
Hide resolved
a432a7d
to
b1e7dd5
Compare
ee43ebf
to
1c20375
Compare
…n part of the URL
Resolves brave/brave-browser#38683
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
url_pattern
contains asterisk wildcard(s) in eTLD+1url_pattern
contains question mark wildcard(s) in eTLD+1url_pattern
does not have the same domain and registry as thetarget_url
target_url
contains asterisk wildcard(s) in eTLD+1target_url
contains question mark wildcard(s) in eTLD+1imageUrl
contains asterisk wildcard(s) in eTLD+1imageUrl
contains question mark wildcard(s) in eTLD+1