Skip to content

🐛 Allow http protocol for noscript > img fallbacks for parity with amp-img#21686

Merged
Gregable merged 5 commits intoampproject:masterfrom
westonruter:allow/noscript-img-with-http-protocol
Aug 19, 2019
Merged

🐛 Allow http protocol for noscript > img fallbacks for parity with amp-img#21686
Gregable merged 5 commits intoampproject:masterfrom
westonruter:allow/noscript-img-with-http-protocol

Conversation

@westonruter
Copy link
Copy Markdown
Member

@westonruter westonruter commented Apr 2, 2019

This addresses an issue discovered in the AMP plugin. See ampproject/amp-wp#2051. To repeat that PR's description:

We discovered an issue when a site is served via HTTP instead of HTTPS. An amp-img allows for its image to use the http protocol, so this is valid AMP:

<amp-img src="http://example.com/bison-1024x668.jpg" srcset="http://example.com/bison-1024x668.jpg 1024w, http://example.com/bison-300x196.jpg 300w, http://example.com/bison-768x501.jpg 768w, http://example.com/bison.jpg 1200w" sizes="(max-width: 767px) 89vw, (max-width: 1000px) 54vw, (max-width: 1071px) 543px, 580px" width="1024" height="668" layout="intrinsic"></amp-img>

With ampproject/amp-wp#1861 there was introduced noscript > img fallbacks for serving when a user with JS disabled accesses a (canonical) AMP page. This results in markup like:

<amp-img src="http://example.com/bison-1024x668.jpg" srcset="http://example.com/bison-1024x668.jpg 1024w, http://example.com/bison-300x196.jpg 300w, http://example.com/bison-768x501.jpg 768w, http://example.com/bison.jpg 1200w" sizes="(max-width: 767px) 89vw, (max-width: 1000px) 54vw, (max-width: 1071px) 543px, 580px" width="1024" height="668" layout="intrinsic">
  <noscript>
    <img src="http://example.com/bison-1024x668.jpg" srcset="http://example.com/bison-1024x668.jpg 1024w, http://example.com/bison-300x196.jpg 300w, http://example.com/bison-768x501.jpg 768w, http://example.com/bison.jpg 1200w" sizes="(max-width: 767px) 89vw, (max-width: 1000px) 54vw, (max-width: 1071px) 543px, 580px" width="1024" height="668">
  </noscript>
</amp-img>

It turns out that this actually is invalid AMP:

image

The src and srcset attributes are flagged as errors because—perhaps unintentionally—the noscript > img element does not allow the http protocol.

Perhaps the noscript > img omitted the http protocol since it was also omitted for iframe, video, and audio. But amp-iframe, amp-video, and amp-audio all only allow https to begin with. The amp-img is unique in that it also allows http. So this PR ensures that its noscript > img fallback element also allows http.

@twifkak
Copy link
Copy Markdown
Member

twifkak commented Apr 3, 2019

@Gregable: It looks like you explicitly disallowed http URLs on img tags inside noscript, long ago, in commit cee1a3b (cl/114347806). Do you know why (or know how to find out)? Does this requirement still hold?

@Gregable
Copy link
Copy Markdown
Member

Gregable commented Apr 3, 2019

Currently the AMP Cache doesn't rewrite <img src> URLs (or at least my quick look at the code seems to indicate as such), so if http were allowed, we'd end up with mixed-mode warnings (only when javascript is disabled? unsure) which is not desired.

I don't see any reason why we shouldn't rewrite <img src> URLs to point to the AMP Cache, but we do need to order these changes. I think we never implemented it as it's super unlikely that the amp cache would be loaded without javascript.

@twifkak
Copy link
Copy Markdown
Member

twifkak commented Apr 3, 2019 •
edited
Loading

Mixed mode warnings would only appear when JS is disabled. I only tested this on Chrome. But per the spec:

https://html.spec.whatwg.org/multipage/parsing.html#parsing-main-inhead
https://html.spec.whatwg.org/multipage/parsing.html#parsing-main-inbody

A start tag whose tag name is "noscript", if the scripting flag is enabled
  Follow the generic raw text element parsing algorithm.

So the contents of the <noscript> wouldn't even be tokenized (up until the next </noscript>).

This was referenced Apr 13, 2019
Closed
Merged
@mrjoro
Copy link
Copy Markdown
Member

mrjoro commented May 15, 2019

Is this PR still active?

@westonruter
Copy link
Copy Markdown
Member Author

westonruter commented May 15, 2019

Still something I'd like to see happen. Otherwise we need this workaround which won't work if a site is indeed not available over HTTPS: ampproject/amp-wp#2126

Granted, the frequency of this happening will lessen going into the future, as more sites move to HTTPS and more JS is available on more bots.

@Gregable
Copy link
Copy Markdown
Member

Gregable commented May 15, 2019

The rewrite is now in the AMP cache, from my read of the code, so this should be fine to commit now.

@Gregable Gregable self-requested a review May 15, 2019 22:12
@mrjoro
Copy link
Copy Markdown
Member

mrjoro commented Aug 8, 2019

It looks like this PR was approved; is it still okay to merge it?

@twifkak
Copy link
Copy Markdown
Member

twifkak commented Aug 15, 2019

@Gregable probably knows best.

Gregable
Gregable reviewed Aug 15, 2019
View reviewed changes
@Gregable
Copy link
Copy Markdown
Member

Gregable commented Aug 15, 2019

Spotted one small suggestion and it probably needs to be sync'ed with changes before merging since travis hasn't run for 5 months.

@westonruter
Merge branch 'master' of github.com:ampproject/amphtml into allow/nos…
54ebf90 
…cript-img-with-http-protocol

* 'master' of github.com:ampproject/amphtml: (1326 commits)
  Fix and enable e2e tests for AMPHTML ads FIE rendering mode (ampproject#23995)
  🏗 Update WorkerDOM to 0.17.0 (ampproject#24024)
  Make DocInfo.pageViewId64 async (ampproject#23998)
  🐛 Updates amp-sidebar in amp-story  (ampproject#23956)
  Revert "Revert "📖Update documentation for carousel 0.2 (ampproject#23840)" (ampproject#23967)" (ampproject#24016)
  🔥 Revert "📈 Initial StorySpec Implementation (ampproject#23030)" (ampproject#24013)
  Extension skeleton code for payment widgets (ampproject#23045)
  🏗🐛 Don't call `travisBuildNumber()` in the global scope (ampproject#24021)
  Remove suppressTypes from amp-mustache. (ampproject#23993)
  🐛 Move `terser` from `dependencies` to `devDependencies` (ampproject#24018)
  Revert "Revert "Set the new loaders experiment to 1% of traffic. (ampproject#23780)" (ampproject#23963)" (ampproject#24014)
  SwG release 0.1.22.63 (ampproject#23997)
  Resolve navTiming variable earlier if possible (ampproject#23580)
  🏗 Don't run all the runtime tests for validator-only changes (ampproject#24010)
  Collect document ready signal (ampproject#23981)
  Validator rollup (ampproject#24000)
  Remove flaky story branching test. (ampproject#23994)
  Include amp-base-carousel in amp-carousel's build. (ampproject#23984)
  Partial validator rollup (ampproject#23996)
  amp-bind: Rate-limit history operations (ampproject#23938)
  ...
@westonruter
Remove comment about why noscript>img allows http protocol
bba1bb3
@westonruter
Revert "Remove comment about why noscript>img allows http protocol"
479d9ed 
This reverts commit bba1bb3.
@westonruter
Remove obsolete allow_relative:true comment regarding noscript>img src
6d60cb7
@westonruter
Copy link
Copy Markdown
Member Author

westonruter commented Aug 17, 2019

Spotted one small suggestion and it probably needs to be sync'ed with changes before merging since travis hasn't run for 5 months.

Done! Also merged latest changes from master.

@Gregable Gregable merged commit 57fe554 into ampproject:master Aug 19, 2019
westonruter added a commit to westonruter/amphtml that referenced this pull request Aug 20, 2019
@westonruter
Merge branch 'master' of github.com:ampproject/amphtml into add/lando
a89302a 
* 'master' of github.com:ampproject/amphtml: (32 commits)
  ✨ Make tweet id a bindable attribute (ampproject#23953)
  🏗 Update Local AMP extension to allow custom base URLs (ampproject#24029)
  🏗 Improve serving from non-localhost host (ampproject#24066)
  Preventing half pixels. (ampproject#24050)
  Update callout-vendors.js (ampproject#23218)
  🏗 Fixes to `check-package-manager.js` (ampproject#24060)
  Rename AMP_MODE to __AMP_MODE. (ampproject#24052)
  Story media performance metrics. (ampproject#23962)
  Updating Story amp-sidebar width documentation. (ampproject#23894)
  Fixes race condition in amp-video-iframe (ampproject#24033)
  Rename ampExtendedElements to __AMP_EXTENDED_ELEMENTS (ampproject#24056)
  🏗🚮 Enable property inlining (ampproject#24053)
  ✨amp-ads: Added optional params for Directadvert network (ampproject#23724)
  <amp-experiment> style mutation fix and improvment (ampproject#23669)
  🐛 Allow http protocol for noscript > img fallbacks for parity with amp-img (ampproject#21686)
  🏗 Refactor transform-log-asserts (ampproject#24028)
  Automatically preconnect to source origins on page loads. (ampproject#24045)
  Support visibility API in the ampdoc (ampproject#23799)
  Amphtml visual tests should use relative path against root (ampproject#24042)
  FIX: check all fields' dirtiness on AMP form init (ampproject#23978)
  ...
twifkak added a commit to twifkak/amphtml that referenced this pull request Aug 21, 2019
@twifkak
cl/264409259 Revision bump for ampproject#21686
de03471
@twifkak twifkak mentioned this pull request Aug 21, 2019
Merged
twifkak added a commit that referenced this pull request Aug 21, 2019
@twifkak
Validator rollup (#24106)
73ac632 
* cl/263666436 Introduce `<style amp-custom-length-check>`

* cl/263843720 Mark `name` attribute of <a> deprecated for AMP for Email

* cl/263909610 Revision bump for #23045

* cl/264265855 Revision bump for #24016

* cl/264409259 Revision bump for #21686
@westonruter westonruter mentioned this pull request Sep 13, 2019
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Gregable Gregable Gregable approved these changes

@twifkak twifkak Awaiting requested review from twifkak

Assignees

No one assigned

Labels

cla: yes WG: caching

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@westonruter @twifkak @Gregable @mrjoro @honeybadgerdontcare @googlebot @dreamofabear