New Tab Page Sponsored Image Clickbox #17356
Conversation
github-actions
bot
added
the
CI/storybook-url
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| export default function SponsoredImageClickArea(props: { sponsoredImageUrl: string, onClick: () => void }) { | ||
| return ( | ||
| <Link href={props.sponsoredImageUrl} rel="noreferrer noopener" onClick={props.onClick}> |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Detected a variable used in an anchor tag with the 'href' attribute. A malicious actor may be able to input the 'javascript:' URI, which could cause cross-site scripting (XSS). It is recommended to disallow 'javascript:' URIs within your application.
Source: https://semgrep.dev/r/typescript.react.security.audit.react-href-var.react-href-var
Cc @brave/sec-team @fmarier @thypon
There was a problem hiding this comment.
@fallaciousreasoning can we clean up the link to only accept https?:// schemes?
There was a problem hiding this comment.
While it would be nice to validate at some point in here that the URL scheme is https:// or brave:// and ignore anything else, it's not a huge deal since these URLs are manually entered by us and therefore trusted.
There was a problem hiding this comment.
We're already using this link on the page, and as @fmarier mentioned, the source of the URLs are trusted, so I don't know if it's worth adding that check here.
Maybe, in future, we could introduce a Link component which does a bunch of security checks on the URL? Maybe the one in Leo would be a good target for this, as then we'd get the benefits everywhere Leo is used?
Could be worth filing an issue on https://github.com/brave/leo/issues/new?
There was a problem hiding this comment.
Actually @simonhong, is it okay that I have the noreferrer here? How do sponsored sites track clicks?
There was a problem hiding this comment.
@fallaciousreasoning why an issue in Leo and not in brave-browser?
It seems like a good practice to just use a SafeLink^(possible name) component in brave-core
There was a problem hiding this comment.
I think, ideally, that component should exist in Leo, so we can use it in other Brave projects, if the need arises
There was a problem hiding this comment.
Actually @simonhong, is it okay that I have the
noreferrerhere? How do sponsored sites track clicks?
I think noreferrer is fine. When onClickLogo() is clicked, it notifies to browser with current sponsored images's metadata. BTW, we are using onClickLogo() for notifying about user's logo clicking.
Not sure, this could be used here. @tmancey @aseren
There was a problem hiding this comment.
As we do the same as for the Logo click (i.e. open the same URL), I think it is fine to call onClickLogo() when an image area is clicked.
Should we also add rel="noreferrer noopener" to the Logo click anchor?
|
|
||
| export default function SponsoredImageClickArea(props: { sponsoredImageUrl: string, onClick: () => void }) { | ||
| return ( | ||
| <Link href={props.sponsoredImageUrl} rel="noreferrer noopener" onClick={props.onClick}> |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Detected a variable used in an anchor tag with the 'href' attribute. A malicious actor may be able to input the 'javascript:' URI, which could cause cross-site scripting (XSS). It is recommended to disallow 'javascript:' URIs within your application.
Source: https://semgrep.dev/r/typescript.react.security.audit.react-href-var.react-href-var
Cc @brave/sec-team @fmarier @thypon
|
A Storybook has been deployed to preview UI for the latest push |
* New Tab Page Sponsored Image Clickbox (#17356) * Fixed presubmit error --------- Co-authored-by: Brian Clifton <brian@clifton.me>
Resolves: brave/brave-browser#28736
Adds a click box to new tab page sponsored images. It slots in between the NTP grid and the widgets on the right. If widgets are showing and the screen is in one column mode, we don't add the clickbox (but if the widgets aren't showing we do).
The box has a fair bit of padding (to prevent misclicks) so it's at some screen sizes it's small to non-existent.
Note: This is only present when we have a sponsored background. (red border for illustration only)
Screencast.from.2023-02-24.11-05-45.webm
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run lint,npm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: