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 #27358

Closed
wants to merge 1 commit into from
Closed

[ads] RichNTT: Desktop #27358

wants to merge 1 commit into from

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Jan 27, 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:

See spec.

Copy link
Contributor

Chromium major version is behind target branch (132.0.6834.83 vs 133.0.6943.27). Please rebase.

@github-actions github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch labels Jan 27, 2025
@tmancey tmancey assigned aseren and tmancey and unassigned tmancey Jan 27, 2025
@tmancey tmancey force-pushed the issues/43512 branch 3 times, most recently from b1f9e91 to c4eb8a4 Compare January 27, 2025 19:55
@github-actions github-actions bot removed the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Jan 27, 2025
@tmancey tmancey force-pushed the issues/43512 branch 7 times, most recently from 57011f8 to 51c6336 Compare January 27, 2025 20:37
@tmancey tmancey force-pushed the issues/43512 branch 4 times, most recently from e822363 to aa3de5a Compare January 27, 2025 21:15
@tmancey tmancey force-pushed the issues/43512 branch 5 times, most recently from e56d308 to 31aa8d5 Compare January 27, 2025 21:35
@tmancey tmancey removed the request for review from kylehickinson January 31, 2025 23:42
@tmancey tmancey reopened this Jan 31, 2025
@tmancey tmancey marked this pull request as draft January 31, 2025 23:44
Copy link
Contributor

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

Here's my review of the PR:

Description

This PR adds support for rich media ads in the New Tab Page (NTP) background. It includes the ability to serve rich media content through an untrusted WebUI frame, handle new media playback events (50% and 75%), and support interaction events. The changes also refactor how new tab page ads are managed, moving away from catalog-based ads to component-based ads.

Possible Issues

  1. The PR modifies path traversal checks and security headers for untrusted content - this needs thorough testing to ensure the sandboxing is effective
  2. Some unit tests were removed or disabled which could reduce test coverage
  3. The changes introduce runtime parsing of dynamic content which could impact performance

Security Hotspots

  1. URL Validation: The MaybeGetFilePathForRequestPath method needs to ensure no path traversal is possible when serving rich media content
  2. CSP Headers: The new Content Security Policy settings for rich media frames need to be restrictive enough to prevent malicious behavior
  3. Message Passing: The untrusted frame communication via postMessage needs proper origin validation
Changes

Changes

  1. ntp_sponsored_rich_media_source.h/cc:
  • Added new source handler for rich media content
  • Implements strict CSP and sandboxing
  • Handles path validation and file serving
  1. ntp_p3a_helper_impl.cc:
  • Added support for new event types (interaction, media_50, media_75)
  • Updated metrics reporting
  1. view_counter_service.cc:
  • Added support for rich media ad events
  • Modified wallpaper handling to support rich media type
  1. UI Components:
  • Added new iframe component for rich media content
  • Updated event handling for media playback tracking
  • Added support for interaction events
sequenceDiagram
    participant Browser
    participant NTPService
    participant RichMediaSource
    participant AdService
    
    Browser->>NTPService: Request new tab page
    NTPService->>RichMediaSource: Init rich media frame
    RichMediaSource->>RichMediaSource: Validate paths & set CSP
    RichMediaSource-->>NTPService: Return sandboxed frame
    NTPService-->>Browser: Display rich media content
    
    Browser->>AdService: Report media events
    AdService->>AdService: Process events (25%,50%,75%,100%)
    AdService-->>Browser: Update metrics
Loading

@tmancey tmancey force-pushed the issues/43512 branch 5 times, most recently from 03bbd97 to 7779370 Compare February 2, 2025 13:07
@tmancey tmancey marked this pull request as ready for review February 3, 2025 13:31
@tmancey tmancey requested review from aseren and kylehickinson and removed request for aseren and kylehickinson February 3, 2025 13:31
@tmancey tmancey marked this pull request as draft February 3, 2025 13:32
@tmancey
Copy link
Collaborator Author

tmancey commented Feb 3, 2025

Closing and will reopen a new PR.

@tmancey tmancey closed this Feb 3, 2025
@tmancey tmancey deleted the issues/43512 branch February 3, 2025 13:33
@tmancey tmancey restored the issues/43512 branch February 3, 2025 13:34
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
Status: Done
Development

Successfully merging this pull request may close these issues.

[ads] RichNTT: Desktop
10 participants