-
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
re-add the ability to allow/block individual scripts in Shields (webui part) #17221
Conversation
63d2d85
to
d52ed16
Compare
c49dffa
to
0c7be0e
Compare
d52ed16
to
4ffdd84
Compare
28b10c7
to
c77b2e3
Compare
0c7be0e
to
344cbb4
Compare
c77b2e3
to
e55e091
Compare
A Storybook has been deployed to preview UI for the latest push |
dc72839
to
131a0ed
Compare
befa071
to
8161ddb
Compare
A Storybook has been deployed to preview UI for the latest push |
1c213e4
to
03127ab
Compare
A Storybook has been deployed to preview UI for the latest push |
03127ab
to
689e903
Compare
A Storybook has been deployed to preview UI for the latest push |
689e903
to
4eb1f38
Compare
A Storybook has been deployed to preview UI for the latest push |
4eb1f38
to
c7069a4
Compare
A Storybook has been deployed to preview UI for the latest push |
c7069a4
to
a349b43
Compare
A Storybook has been deployed to preview UI for the latest push |
91f5589
to
a6c5106
Compare
A Storybook has been deployed to preview UI for the latest push |
a6c5106
to
cbb16e6
Compare
A Storybook has been deployed to preview UI for the latest push |
<div className={urlTextClass} onClick={handleTextClick}> | ||
{props.path ? props.path : props.host} | ||
</div> | ||
{props.onPermissionButtonClick && ( |
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.
If onPermissionButtonClick is a required prop, why are we checking if it exists here? If the permission button is conditionally rendering, it would make sense to allow null/undefined values for the prop, which should be consistent throughout the call stack.
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.
it can be undefined as suggested by @fallaciousreasoning above #17221 (comment)
export type PermissionButtonHandler =
((name: string) => void) | undefined
false
was not compiled, undefined
works
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.
the handler was suggested in #17221 (comment)
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.
Changed type of onPermissionButtonClick
and made parameter optional
be2c26d
to
510212d
Compare
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.
Looks good!
A Storybook has been deployed to preview UI for the latest push |
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.
Thanks for that @spylogsster!
510212d
to
ab765c0
Compare
A Storybook has been deployed to preview UI for the latest push |
@@ -78,6 +78,7 @@ class BraveShieldsDataController | |||
void SetIsNoScriptEnabled(bool is_enabled); | |||
void SetIsHTTPSEverywhereEnabled(bool is_enabled); | |||
void AllowScriptsOnce(const std::vector<std::string>& origins); | |||
void BlockAllowedScripts(const std::vector<std::string>& origins); |
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.
block allowed scripts? Can you please add a comment explaining what it does because it makes no sense to block an allowed 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.
I think this should probably be BlockScriptsOnce
to match AllowScriptsOnce
?
I don't see any tests to verify that this actually blocks/allows the individual scripts on the page and I'm not really sure how it can work correctly given that
hasn't changed. The test here https://github.com/brave/brave-core/pull/17152/files#diff-7274b75afeaaeb25be8467441f13ad371269f1ba1c637e3fee2a1714f80c8003R201 is not sufficient because you need two different scripts with the same origin to verify that you are blocking by the full script url and not just the origin. If I'm missing something here and this does work then at the very least the comment needs to be changed, but I'm pretty sure it won't work correctly with more than one script from the same domain |
bool is_origin = origin.Serialize() == script; | ||
base::EraseIf(allowed_scripts_, [is_origin, script, | ||
origin](const std::string& value) { | ||
// scripts array may have both origins or full scripts paths. |
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.
SetAllowScriptsFromOriginsOnce
only matches based on origin so having a full script url in this list seems confusing at best
Resolves brave/brave-browser#28510
block.mp4
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 lint
,npm 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: