Skip to content

馃悰 escape cta-image before interpolating into page-attachment style - #40561

Open
mrdevel2002-ui wants to merge 1 commit into
ampproject:mainfrom
mrdevel2002-ui:story-attachment-cta-image-css
Open

mrdevel2002-ui wants to merge 1 commit into
ampproject:mainfrom
mrdevel2002-ui:story-attachment-cta-image-css

Conversation

@mrdevel2002-ui

Copy link
Copy Markdown

The page-attachment outlink/CTA UI renders its cta-image into an inline style string. For single page story ads that value is copied verbatim from the ad creative's meta tags by amp-story-auto-ads (createPageOutlink_ via getStoryAdMetadataFromDoc), so it crosses a trust boundary into the host story page.

Repro: serve a single page story ad whose amp-cta-image meta is https://x/i.png) !important; position: fixed; inset: 0; width: 100vw; height: 100vh; z-index: 2147483647; background: url(//evil. The outlink image chip ends up with those declarations applied, so the creative can overlay the whole story with its own target="_top" outlink.

Cause: style={{backgroundImage: ...url(${openImgAttr}) !important...}} is compiled by #core/dom/jsx (babel-plugin-jsx-style-object + jsxStylePropertyString) into setAttribute('style', ...), which accepts multiple declarations. A ) in the URL closes the url() and the rest becomes new declarations. The same shape appears in renderOutlinkUI, renderInlineUi, and buildOutlink_.

Fix: add cssUrl() in amp-story/1.0/utils.js (emits a quoted url("...") token with ", \ and newlines escaped) and route all three sinks through it, so the value stays a single background-image token regardless of input. Keeping the fix at the sink covers the ad path and any future caller rather than relying on each caller to pre-sanitize. Added a regression test that a position/z-index payload in cta-image is not parsed as separate declarations on the element.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant