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] RichNTT: Desktop #27466

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[ads] RichNTT: Desktop #27466

wants to merge 1 commit into from

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Feb 3, 2025

Resolves brave/brave-browser#43512

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@tmancey tmancey self-assigned this Feb 3, 2025
@tmancey tmancey requested a review from a team as a code owner February 3, 2025 13:35
@github-actions github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Feb 3, 2025
@tmancey tmancey marked this pull request as draft February 3, 2025 13:36
@tmancey tmancey force-pushed the issues/43512 branch 3 times, most recently from 33bb3af to f126167 Compare February 3, 2025 16:43
@tmancey tmancey marked this pull request as ready for review February 3, 2025 17:00
@tmancey
Copy link
Collaborator Author

tmancey commented Feb 3, 2025

@ShivanKaul @aseren is working on adding the missing tests. Thanks

Copy link
Collaborator

@zenparsing zenparsing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Front-end 👍

components/brave_new_tab_ui/containers/newTab/index.tsx Outdated Show resolved Hide resolved
@tmancey tmancey requested a review from bridiver February 7, 2025 19:48
@aseren aseren force-pushed the issues/43512 branch 2 times, most recently from 4a3f422 to a52fb89 Compare February 7, 2025 20:09
@tmancey
Copy link
Collaborator Author

tmancey commented Feb 7, 2025

Shivan has closed security/privacy review. cc @ShivanKaul

@tmancey tmancey force-pushed the issues/43512 branch 7 times, most recently from bbbc3fd to 8197d6a Compare February 7, 2025 21:33
@aseren aseren requested a review from a team as a code owner February 7, 2025 22:37
@tmancey tmancey enabled auto-merge (squash) February 7, 2025 22:50
@tmancey tmancey disabled auto-merge February 7, 2025 22:51
@tmancey tmancey enabled auto-merge (squash) February 7, 2025 22:55
@aseren aseren dismissed Brandon-T’s stale review February 7, 2025 23:03

Agreed in comments this is ok as images are small

Co-authored-by: Terry Mancey <[email protected]>
Copy link
Contributor

github-actions bot commented Feb 8, 2025

[puLL-Merge] - brave/brave-core@27466

Here's my review of the PR:

Description

This PR adds support for rich media sponsored backgrounds in the New Tab Page (NTP). It allows for more interactive and dynamic sponsored content to be displayed as backgrounds, including HTML/CSS/JS content. The PR includes new handling for rich media ads, content security policy (CSP) enforcement, and proper sandboxing of this content.

Possible Issues

  1. Rich media backgrounds could potentially impact performance of the New Tab Page since they require loading and executing additional content
  2. Need to ensure proper monitoring and metrics are in place to detect any issues with rich media backgrounds in production

Security Hotspots

  1. Resource Loading: The GetContentSecurityPolicy implementation in ntp_sponsored_rich_media_source.cc is critical for preventing unauthorized resource loading. Any changes to this need careful review.
  2. Path Traversal: The path sanitization logic in ntp_sponsored_source_util.cc helps prevent directory traversal attacks. The implementation looks solid but this is a key security control.
  3. Message Passing: The postMessage communication between rich media content and the browser needs proper origin verification in sponsored_rich_media_background.tsx.
Changes

Changes

By filename:

browser/ntp_background/BUILD.gn:

  • Added new test files and dependencies

browser/ntp_background/ntp_sponsored_rich_media_browsertest.cc:

  • Added new browser tests for rich media content loading and interaction

browser/ntp_background/ntp_sponsored_rich_media_with_csp_violation_browsertest.cc:

  • Added tests to verify CSP blocks unauthorized resource loading

components/brave_new_tab_ui/:

  • Added rich media background support to NTP UI
  • Added message passing for rich media events
  • Updated background types and handling

components/ntp_background_images/browser/:

  • Added new rich media source handler
  • Added event handling for rich media ads
  • Added utilities for path validation
  • Updated data structures to support rich media content
sequenceDiagram
    participant NTP as New Tab Page
    participant Handler as RichMediaHandler
    participant Service as BackgroundService
    participant Source as RichMediaSource
    
    NTP->>Service: Request background
    Service->>Source: Load rich media content
    Source->>Source: Validate paths & CSP
    Source->>NTP: Return content
    NTP->>Handler: Initialize rich media
    Note over NTP,Handler: Secure iframe sandbox
    Handler->>Service: Report events
    Service->>Service: Process metrics
Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/storybook-url Deploy storybook and provide a unique URL for each build puLL-Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ads] RichNTT: Desktop
9 participants