AMP4Email: Set default "amp-allowed-url-macros" and "amp-action-whitelist" #27561
Conversation
|
Re: bundle size, a couple ideas to consider:
|
|
@choumx Thanks for the suggestions here--decided to delegate the allowed actions to the extensions. Looks like this resolved the bundle size check. To clarify on the first option: email is currently the only format for which the meta tags are provided and processed, which is why the change that overrides them should also make them safe to remove. Am I correct in that understanding? |
I don't think there's a format-specific restriction on usage of these meta tags. But they're invalid AMP, so they can't be used by publishers and must be injected by viewers. So we know they're safe to remove as long as the relevant viewers are informed -- in this case, those are email providers. |
src/service/action-impl.js
Outdated
| */ | ||
| addToWhitelist(tagOrTarget, method) { | ||
| addToWhitelist(tagOrTarget, methods, opt_forFormat) { | ||
| // TODO: When it becomes possible to getFormat(), |
There was a problem hiding this comment.
Minor: Can we add a WG for this TODO?
There was a problem hiding this comment.
Would wg-runtime be appropriate here? 😅
…list" (ampproject#27561) * Allow no URL macros for email * Set default actions allowlist for email * Update comments for variable-source * Add missing actions * Add unit tests * Check existence of `hasAttribute` * Delegate allowed actions to components * Remove queryWhitelist method * Remove action-white-list meta from tests * Don't make every component check email format * Move action allowlist tests to separate file * Remove unparseable allowlist entries * Use getActionInvocation helper method * Add component-based tests and associated fixes * Stub maybeAddToEmailWhitelist in test-amp-form * Remove extra line * Do not allow duplicate actions * Remove "amp-allowed-url-macros" meta * Remove validator cases for meta tags * Stub hasAttribute in test-url-replacements * Fix type annotation * Replace maybeAddToEmailWhitelist with opts * Annotate void return * Fixes to unit tests * Take Arrays of methods, formats, in addToWhitelist * Replace use of hasOwn * Add warning for ignored whitelist entries * Move action unit tests to component unit tests * Move allowlist unit tests back to test-action.js * Fix warning args * Delete test-action-allowlist.js * Remove unnecessary arg * Remove validator tests for meta * Assert valid whitelist entries * Replace `invoke_` with `execute` in unit tests. * Stub user().error * Use sandbox.spy/stub * Add wg-runtime to TODO comment * (empty)
This change hardcodes default allowlists for URL replacements and AMP actions for the AMP4Email format and no longer explicitly from the
amp-allowed-url-macrosandamp-action-whitelistmeta tags. This also introduces an allowlist-specific unit tests for AMP actions and affected components, as well as removes the validator tests which asserts the invalidity of the meta tag names.Fixes #27094