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

not all http:// links can be https:// #3657

Merged
merged 1 commit into from
May 3, 2021

Conversation

peterbe
Copy link
Contributor

@peterbe peterbe commented Apr 29, 2021

Fixes #3654

This is a great page to test it on:
http://localhost:3000/en-US/docs/Web/API/Touch_events/Using_Touch_Events#examples_and_demos

In the source it has this:

 <li><a href="http://www.javascriptkit.com/javatutors/touchevents.shtml">Introduction to Touch events in JavaScript</a></li>
 <li><a href="http://www.codicode.com/art/easy_way_to_add_touch_support_to_your_website.aspx">Add touch screen support to your website (The easy way)</a></li>

And the output, becomes:

<li><a href="http://www.javascriptkit.com/javatutors/touchevents.shtml" class="external" rel=" noopener">Introduction to Touch events in JavaScript</a></li>
<li><a href="https://www.codicode.com/art/easy_way_to_add_touch_support_to_your_website.aspx" data-flaw="link1" class="external" rel=" noopener">Add touch screen support to your website (The easy way)</a></li>

That's because it knows with certainty that www.codicode.com is a domain where you can safely just replace http:// for https://.

For the http://www.javascriptkit.com/javatutors/touchevents.shtml link it's basically a "hopeless case". It's too expensive and fragile to try to do a network lookup on this at build-time in the flaw system. In fact, when I generated the safe-to-https-domains.json file [with my hacky Python script])(https://gist.github.com/peterbe/8fabfcdb46bdf6b9211af636fd8cd178) I found it was causing SSLError problems.

I still like that http:// links stick out a in the PR Review Companion but it's not a flaw anymore.

@peterbe peterbe requested a review from Gregoor April 29, 2021 13:57
Copy link
Contributor

@Gregoor Gregoor left a comment

Choose a reason for hiding this comment

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

Cool! Just a small thing I'm wondering: how about just having an unsafe_https_domains.json-array? i.e. the assumption being that a new domain would be HTTPS safe, and otherwise it'd need to be added to that array.

@peterbe
Copy link
Contributor Author

peterbe commented May 3, 2021

Cool! Just a small thing I'm wondering: how about just having an unsafe_https_domains.json-array? i.e. the assumption being that a new domain would be HTTPS safe, and otherwise it'd need to be added to that array.

Right now, that object that the JSON file is, gives 2 pieces of information:

  1. The domains that are definitely safe
  2. The domains that are definitely NOT safe

What we could do, is if a new domain is added, you can know immediately if this is going to be safe or not safe to turn into HTTPS.

So I'll keep it like this for now.

@peterbe peterbe merged commit 4743ee3 into mdn:main May 3, 2021
@peterbe peterbe deleted the 3654-not-all-http-links-can-be-https branch May 3, 2021 19:13
peterbe added a commit to peterbe/yari that referenced this pull request Jun 1, 2021
wbamberg pushed a commit to wbamberg/yari that referenced this pull request Jul 29, 2021
* upstream/main: (288 commits)
  remove tests for obsolete search endpoints (mdn#3688)
  Limit non-English locales (mdn#3661)
  move footer icons to CSS bg images (mdn#3655)
  not all http:// links can be https:// (mdn#3657)
  add the lost part and update the translation for Express (mdn#3664)
  update the translation of the zh-TW (mdn#3663)
  Add a Specifications macro and extract a special specification section to the Document (mdn#3518)
  build(deps-dev): bump & migrate use-debounce to 6.0.0 (mdn#3684)
  build(deps-dev): bump puppeteer from 9.0.0 to 9.1.0 (mdn#3686)
  build(deps-dev): bump & migrate husky to 6.0.0 (mdn#3681)
  build(deps-dev): bump jest-environment-jsdom-sixteen from 1.0.3 to 2.0.0 (mdn#3615)
  More concurrent open npm PRs (mdn#3685)
  build(deps-dev): bump black from 21.4b0 to 21.4b2 in /deployer (mdn#3668)
  build(deps-dev): bump black in /testing/integration (mdn#3666)
  build(deps-dev): bump @babel/core from 7.13.16 to 7.14.0 (mdn#3683)
  build(deps-dev): bump jest-puppeteer from 5.0.2 to 5.0.3 (mdn#3680)
  build(deps): bump @mdn/browser-compat-data from 3.3.0 to 3.3.2 (mdn#3679)
  build(deps-dev): bump sass from 1.32.11 to 1.32.12 (mdn#3678)
  build(deps-dev): bump eslint-plugin-jest from 24.3.5 to 24.3.6 (mdn#3677)
  build(deps): bump http-proxy-middleware from 1.3.0 to 1.3.1 (mdn#3675)
  ...
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.

Only force URLs from know domains from http:// to https://
2 participants