Smart ad-unit PR installer (CSP patching + PR-run tracking) - #91
Merged
Merged
Conversation
The "Submit PR to install" flow for ad slots only injected the embed before </body> — it never touched the site's CSP, so on any publisher with a Content-Security-Policy the ad unit was silently blocked (the /ad.js script, the /api/ads/serve fetch, the creative iframe, and its images all get refused). The stats-tracker installer already patches CSP; this brings the ad installer to parity. - Generalize the tracker's CSP machinery: export addSourceToDirective / hasDirective / looksLikeCsp and make findCspPatchTargets take a `patch` fn so both installers share it. - Add patchCspForAds: appends the CrawlProof origin to script-src / script-src-elem / connect-src / img-src, and 'self' to frame-src / child-src (the ad is a same-origin srcdoc iframe). Append-only and conservative, same as the tracker. - installAdEmbed now opens a single PR that both injects the embed and patches every CSP config file, returns cspPaths, and handles the embed-already-present case as a CSP-only PR. - Record the install as a project_pr_runs row (kind=install_ad) so it shows up with the project's other automated PRs; widen the kind check constraint via migration. - Add contract tests for patchCspForAds and installAdEmbed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
vu1nz Security Review0 finding(s) in PR #? No security issues found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The Submit PR to install flow for ad slots was the "dumb" cousin of the stats-tracker installer. It only injected the
<div data-cp-ad>+/ad.jsembed before</body>and never patched the publisher's CSP. On any site shipping aContent-Security-Policy, the ad unit is silently blocked — the/ad.jsscript (script-src), the/api/ads/servefetch (connect-src), the creativesrcdociframe (frame-src), and its artwork (img-src) all get refused. The tracker installer has patched CSP for a while; this brings the ad installer to parity.What changed
addSourceToDirective/hasDirective/looksLikeCspand madefindCspPatchTargetstake apatchfn so the tracker and ad installers reuse the same conservative, append-only scanner.patchCspForAds. Appends the CrawlProof origin toscript-src/script-src-elem/connect-src/img-src, and'self'toframe-src/child-src(the ad renders in a same-originsrcdociframe that inherits the host page's CSP). Only rewrites files that already look like a CSP; never adds a directive that wasn't there.installAdEmbed. Now opens a single PR that both injects the embed and patches every CSP config file, returnscspPaths, and treats "embed already present" as a CSP-only PR.project_pr_runsrow (kind=install_ad) so ad installs show up alongside the project's other automated PRs. Migration widens thekindcheck constraint.tests/contract/install-ad.test.tscoverspatchCspForAdsand the threeinstallAdEmbedpaths (inject+CSP, CSP-only, no-op).Verification
tsc --noEmit: 0 errorsvitest run tests/contract: 331 passed / 7 skipped (incl. 5 new)🤖 Generated with Claude Code