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

Fix doubleclick rtc example page, allowlist json in RTC macro #32740

Merged
merged 1 commit into from Mar 3, 2021

Conversation

powerivq
Copy link
Contributor

data-json attribute does not exist, so it will simply error out. There are publishers misled by this template. TGT macro which uses the json attribute appears to be the correct one.

@calebcordry
Copy link
Member

hmm data-json is explicitly mentioned here https://github.com/ampproject/amphtml/blob/master/extensions/amp-ad-network-doubleclick-impl/doubleclick-rtc.md#available-url-macros are we sure this is the right fix?

@powerivq
Copy link
Contributor Author

@calebcordry That data-json looks stale. Everything else in the same doc only references to json attribute, and there are no signs of data-json any more. Maybe I should allowlist json instead?

@amp-owners-bot
Copy link

Hey @jeffkaufman! These files were changed:

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js
extensions/amp-ad-network-doubleclick-impl/doubleclick-rtc.md

@calebcordry
Copy link
Member

It also seems to be used in doubleclick auto ads, but not sure if that matters for RTC case

'json': this.autoAmpAdsElement_.getAttribute('data-json'),

Maybe @lannka or @zombifier have more context?

@powerivq powerivq changed the title Fix doubleclick rtc example page Fix doubleclick rtc example page, allowlist json in RTC macro Feb 18, 2021
@powerivq
Copy link
Contributor Author

I did a git blame, and it appears data-json was never permitted, contrary to what the doc says. Maybe @keithwrightbos knows more on the history of this?

@powerivq
Copy link
Contributor Author

powerivq commented Mar 2, 2021

Updated the example to use ATTR(json) instead. This way this PR does not change the originally intended behavior.

Copy link
Member

@calebcordry calebcordry left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants