-
Notifications
You must be signed in to change notification settings - Fork 868
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
Remove Greaselion dependency from Rewards service (v2) #25949
Conversation
A Storybook has been deployed to preview UI for the latest push |
fb5cc24
to
a3c13cd
Compare
805281a
to
442ee26
Compare
visit->provider = *platform; | ||
} | ||
|
||
rewards_service_->GetPublisherActivityFromUrl(visit->Clone()); |
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.
this should really be encapsulated in something like RewardsService::UpdateCreatorForTab
. I have no idea what is going on in these three calls and this also makes it hard to test vs unit testing a single method on RewardsService. We really shouldn't be exposing these kinds of low level calls in the service.
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.
side note: why is this called GetPublisherActivityFromUrl
if it doesn't actually return anything? Also it doesn't take a URL as a param which is confusing. Separately from this PR this method name should probably change.
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.
oh, I guess an overloaded version of GetPublisherActivityFromUrl
was added in this PR so the name should at least change to GetPublisherActivityFromVisitData
and then change GetPublisherActivityFrom
for both in a separate PR
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.
Yes - these methods are pretty ancient and the names reflect that. I've updated the new method name as suggested, and I'll add an issue to fix the names up more generally.
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.
what about my comment about wrapping these three method calls in a single call to the service? If multiple actions need to be taken in response to something it's best for the service to handle that, not the caller. Although this is not a user action, it should still follow the same principle of having a single call to the service for a given event that occurs outside the component
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.
I'm not suggesting refactoring the entire class, but we can certainly simplify this specific case, no? Just because other code is problematic doesn't mean we should keep doing the same thing when we could at least wrap these calls. It's only adding a couple of methods to RewardsService to move the details inside of it
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.
Filed a follow-up issue here: brave/brave-browser#41832 to address the RewardsService AC-related API issues. It will be more clear what the API should be after the upcoming AC changes have been finalized.
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.
fyi I made a similar comment in this PR to make things more unit testable https://github.com/brave/brave-core/pull/26082/files#r1813441440
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.
unit testing tab helpers is not very practical, but unit testing the service is
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.
I won't block on this as long as we have good test coverage for everything that gets called here, but given that this is new code being added I'm struggling with why we can't just add a new method to the service to wrap these three calls so we can unit test it.
442ee26
to
bafbb4a
Compare
bafbb4a
to
c3ffb61
Compare
c3ffb61
to
e8666ab
Compare
f019ae7
to
445543d
Compare
creator_detection_.DetectCreator( | ||
navigation_handle->GetRenderFrameHost(), | ||
base::BindOnce(&RewardsTabHelper::OnCreatorDetected, | ||
base::Unretained(this), detection_navigation_id_)); |
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.
reported by reviewdog 🐶
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:
- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/chromium-uaf.yaml
Cc @thypon @goodov @iefremov
445543d
to
cd73dc4
Compare
injector_->RequestAsyncExecuteScript( | ||
ISOLATED_WORLD_ID_BRAVE_INTERNAL, base::UTF8ToUTF16(script), | ||
blink::mojom::UserActivationOption::kDoNotActivate, | ||
blink::mojom::PromiseResultOption::kAwait, std::move(callback)); |
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.
reported by reviewdog 🐶
[semgrep] RequestAsyncExecuteScript usages should be vet by the security-team.
References:
- https://github.com/brave/brave-browser/wiki/Security-reviews (point 13)
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/brave-execute-script.yaml
Cc @thypon @diracdeltas @bridiver
} | ||
|
||
export function urlPath(url: string) { | ||
try { return new URL(url, location.href).pathname } |
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.
reported by reviewdog 🐶
[semgrep] Are you using the URL(url, base)
constructor as a security control to limit the origin with base location.href
? The base is ignored whenever url looks like an absolute URL, e.g. when it begins protocol://
. \\\\
or //x.y
. Verify that the URL's origin is as expected rather than relying on the URL constructor.
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/services/url-constructor-base.yaml
Cc @thypon @kdenhartog
} | ||
|
||
export function absoluteURL(url: string) { | ||
try { return new URL(url, location.href).toString() } |
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.
reported by reviewdog 🐶
[semgrep] Are you using the URL(url, base)
constructor as a security control to limit the origin with base location.href
? The base is ignored whenever url looks like an absolute URL, e.g. when it begins protocol://
. \\\\
or //x.y
. Verify that the URL's origin is as expected rather than relying on the URL constructor.
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/services/url-constructor-base.yaml
Cc @thypon @kdenhartog
ExecuteScriptCallback callback) { | ||
CHECK(injector_.is_bound()); | ||
injector_->RequestAsyncExecuteScript( | ||
ISOLATED_WORLD_ID_BRAVE_INTERNAL, base::UTF8ToUTF16(script), |
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.
reported by reviewdog 🐶
[semgrep] Security hotspot found (ISOLATED_WORLD
). A security-team member should analyze the code security for possible vulnerabilities.
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/brave-isolated-world.yaml
Cc @thypon @diracdeltas @bridiver
cd73dc4
to
b650615
Compare
[puLL-Merge] - brave/brave-core@25949 DescriptionThis pull request introduces a new feature for detecting Brave Creators on media platform sites. It removes the existing Greaselion-based implementation and replaces it with a new, more efficient system that uses JavaScript injection to detect creators directly on supported platforms. ChangesChanges
Possible Issues
Security Hotspots
|
Released in v1.73.50 |
Resolves brave/brave-browser#34237
Overview
Currently, Brave Creator "pages" are detected on some social media sites (on desktop) by using a combination of the persistent Brave extension and Greaselion extension content scripts. For performance reasons, we must migrate away from using a persistent background page, and for maintainability reasons (and to allow the feature to work cross-platform in the future) we must migrate away from Greaselion.
With this change, the Rewards service no longer uses Greaselion. Instead, scripts are injected by the
RewardsTabHelper
into the "Brave internal" isolated world (using a newCreatorDetectionScriptInjector
class). The scripts are individually compiled and bundled for each site from TypeScript using our current TypeScript bundling process. They are executed in the "Brave internal" isolated world. In the future, the scripts can be moved out ofbrave-core
and delivered viaComponentUpdater
.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: