Skip to content
This repository was archived by the owner on Nov 6, 2023. It is now read-only.

Removing rulesets which are preloaded in the major browsers#8128

Merged
cooperq merged 3 commits into
EFForg:masterfrom
Hainish:hsts-removal
Jan 20, 2017
Merged

Removing rulesets which are preloaded in the major browsers#8128
cooperq merged 3 commits into
EFForg:masterfrom
Hainish:hsts-removal

Conversation

@Hainish

@Hainish Hainish commented Jan 7, 2017

Copy link
Copy Markdown
Member

No description provided.

@gloomy-ghost

Copy link
Copy Markdown
Collaborator

Are we still checking STS headers? #7081 which removes mathiasbynens.be was closed due to the missing header, but it's being removed again in this PR......

@gloomy-ghost

Copy link
Copy Markdown
Collaborator
ERROR src/chrome/content/rules/RememberTheMilk.xml: Not enough tests (0 vs 1) for <Rule from '^http://api\.rememberthemilk\.com/' to 'https://api.rememberthemilk.com/'>
ERROR src/chrome/content/rules/RememberTheMilk.xml: Not enough tests (0 vs 1) for <Rule from '^http://s([1-4])\.rtmcdn\.net/' to 'https://s\g<1>.rtmcdn.net/'>
ERROR src/chrome/content/rules/Ixquick.xml: Not enough tests (0 vs 1) for <Rule from '^http://ixquick\.com/' to 'https://ixquick.com/'>
ERROR src/chrome/content/rules/Ixquick.xml: Not enough tests (0 vs 2) for <Rule from '^http://([^@/:]*)\.ixquick\.com/' to 'https://\g<1>.ixquick.com/'>
ERROR src/chrome/content/rules/Ixquick.xml: Not enough tests (0 vs 1) for <Rule from '^http://ixquick-proxy\.com/' to 'https://ixquick-proxy.com/'>
ERROR src/chrome/content/rules/Ixquick.xml: Not enough tests (0 vs 2) for <Rule from '^http://([^@/:]*)\.ixquick-proxy\.com/' to 'https://\g<1>.ixquick-proxy.com/'>
ERROR src/chrome/content/rules/Ixquick.xml: Not enough tests (0 vs 1) for <Rule from '^http://startpage\.com/' to 'https://startpage.com/'>
ERROR src/chrome/content/rules/Ixquick.xml: Not enough tests (0 vs 2) for <Rule from '^http://([^@/:]*)\.startpage\.com/' to 'https://\g<1>.startpage.com/'>
ERROR src/chrome/content/rules/Ixquick.xml: Not enough tests (0 vs 2) for <Rule from '^http://([^@/:]*)\.startingpage\.com/' to 'https://\g<1>.startingpage.com/'>
ERROR src/chrome/content/rules/Tweakers.net.xml: Not enough tests (0 vs 2) for <Rule from '^http://((?:gathering|secure|www)\.)?tweakers\.net/' to 'https://\g<1>tweakers.net/'>
ERROR src/chrome/content/rules/WePay.xml: Not enough tests (0 vs 4) for <Rule from '^http://((?:stage|static|support|t|www)\.)?wepay\.com/' to 'https://\g<1>wepay.com/'>
ERROR src/chrome/content/rules/WordPress.xml: Not enough tests (1 vs 2) for <Rule from '^http://wordpress\.(com|org)/' to 'https://wordpress.\g<1>/'>
ERROR src/chrome/content/rules/WordPress.xml: Not enough tests (0 vs 3) for <Rule from '^http://([^/:@\.]+)\.(blog|files)\.wordpress\.com/' to 'https://\g<1>.\g<2>.wordpress.com/'>
ERROR src/chrome/content/rules/WordPress.xml: Not enough tests (0 vs 2) for <Rule from '^http://(\w+)\.blog\.files\.wordpress\.com/' to 'https://\g<1>-blog.files.wordpress.com/'>
ERROR src/chrome/content/rules/WordPress.xml: Not enough tests (0 vs 2) for <Rule from '^http://s\.wordpress\.(com|org)/' to 'https://www.wordpress.\g<1>/'>
ERROR src/chrome/content/rules/WordPress.xml: Not enough tests (0 vs 2) for <Rule from '^http://s\d\.wordpress\.(com|org)/' to 'https://secure.wordpress.\g<1>/'>
ERROR src/chrome/content/rules/WordPress.xml: Not enough tests (0 vs 3) for <Rule from '^http://([^/:@\.]+)\.wordpress\.(com|org)/' to 'https://\g<1>.wordpress.\g<2>/'>
ERROR src/chrome/content/rules/WordPress.xml: Not enough tests (0 vs 1) for <Rule from '^http://s\.stats\.wordpress\.com/' to 'https://stats.wordpress.com/'>
ERROR src/chrome/content/rules/WordPress.xml: Not enough tests (0 vs 3) for <Rule from '^http://([^/:@\.]+)\.(trac|svn)\.wordpress\.org/' to 'https://\g<1>.\g<2>.wordpress.org/'>
ERROR src/chrome/content/rules/WordPress.xml: Not enough tests (0 vs 3) for <Rule from '^http://(i[012]|s\d*)\.wp\.com/' to 'https://\g<1>.wp.com/'>
ERROR src/chrome/content/rules/WordPress.xml: Not enough tests (0 vs 6) for <Exclusion pattern '^http://(?:[^/:@\.]+\.)?wordpress\.com/latex\.php(?:\?.*)?$'>
ERROR src/chrome/content/rules/WordPress.xml: Not enough tests (0 vs 1) for <Exclusion pattern '^http://s-plugins\.wordpress\.org/'>
ERROR src/chrome/content/rules/ThePirateBay.xml: Not enough tests (0 vs 1) for <Rule from '^http://www\.proxybay\.la/' to 'https://proxybay.la/'>
ERROR src/chrome/content/rules/GoogleAPIs.xml: Not enough tests (0 vs 3) for <Rule from '^http://(click|ssl|www)\.google-analytics\.com/' to 'https://\g<1>.google-analytics.com/'>
ERROR src/chrome/content/rules/LuxSci.xml: Not enough tests (0 vs 3) for <Rule from '^http://(?:(securesend\.|webmail\.|xpress\.)|www\.)?luxsci\.com/' to 'https://\g<1>luxsci.com/'>
ERROR src/chrome/content/rules/AgileBits.xml: Not enough tests (0 vs 1) for <Rule from '^http:' to 'https:'>
ERROR src/chrome/content/rules/Torproject.xml: Not enough tests (0 vs 2) for <Rule from '^http://([^/:@\.]+\.)?torproject\.org/' to 'https://\g<1>torproject.org/'>

Also we might need to remove domains from complicated rules and securecookies (if any)

@Hainish Hainish force-pushed the hsts-removal branch 2 times, most recently from 1cd5a92 to c0b3c40 Compare January 9, 2017 23:20
@Hainish

Hainish commented Jan 9, 2017

Copy link
Copy Markdown
Member Author

@gloomy-ghost I've changed the script to only remove rulesets which include the preload directive. This makes the script slower, but I agree it makes sense to remove rulesets only if we're sure that Chromium won't remove them from the preload list in the near future.

@ivysrono

Copy link
Copy Markdown
Contributor

In my opinion, it is better that: move the rule covered by preload to certain branch, scan them periodic.

@Hainish

Hainish commented Jan 10, 2017

Copy link
Copy Markdown
Member Author

We're unlikely to maintain a separate branch specifically for preloaded rulesets that have been removed. Too much overhead, not enough benefit.

@Hainish

Hainish commented Jan 10, 2017

Copy link
Copy Markdown
Member Author

This still needs some work, changing title to reflect this.

@Hainish Hainish changed the title Removing rulesets which are preloaded in the major browsers [WIP] Removing rulesets which are preloaded in the major browsers Jan 10, 2017
@Hainish Hainish changed the title [WIP] Removing rulesets which are preloaded in the major browsers Removing rulesets which are preloaded in the major browsers Jan 12, 2017
@Hainish

Hainish commented Jan 12, 2017

Copy link
Copy Markdown
Member Author

ready for review again. cc @gloomy-ghost @jeremyn

@Hainish

Hainish commented Jan 13, 2017

Copy link
Copy Markdown
Member Author

My suggestion is to ignore the fetch test errors for now, since this PR only removes targets, and reviewing the fetch errors will be diving into the rabbit hole.

@Hainish Hainish mentioned this pull request Jan 13, 2017
@Hainish

Hainish commented Jan 13, 2017

Copy link
Copy Markdown
Member Author

I don't expect a comprehensive review for this, just a spot-check to make sure things look okay.

@jeremyn

jeremyn commented Jan 13, 2017

Copy link
Copy Markdown
Contributor

Mega.xml still has a test for https://cms.mega.nz even though you removed the target for *.mega.nz. Also it looks like https://cms.mega.nz has an invalid certificate, which Travis reports.

@Hainish

Hainish commented Jan 13, 2017

Copy link
Copy Markdown
Member Author

Good catch, though a fix for this in the prune script may get kind of complicated. We'd have to make sure each test url matches only targets which are removed and no others. For now, I'll just remove that particular test.

@Hainish

Hainish commented Jan 13, 2017

Copy link
Copy Markdown
Member Author

I see a bunch of these, going through them now.

@Hainish

Hainish commented Jan 13, 2017

Copy link
Copy Markdown
Member Author

@jeremyn look good now?

@Hainish

Hainish commented Jan 13, 2017

Copy link
Copy Markdown
Member Author

This is blocking #8119, if we can get this merged today that would be good, then I can cut a release on Monday.

@Hainish

Hainish commented Jan 18, 2017

Copy link
Copy Markdown
Member Author

@cooperq can you look through this?

@funkydude

funkydude commented Jan 18, 2017

Copy link
Copy Markdown
Contributor

The problem you will inevitably encounter here is that people tend to create rules for websites when they don't see a rule already in existence.

To accompany this you would want the UI to show which domains are preloaded. For example, clicking the button in Chrome which then shows the menu of what rules are active, should also show preloaded domains below that. You can submit a check to hstspreload.org on click.

This would definitely end up saving wasted time on unnecessary PRs.

@cooperq

cooperq commented Jan 18, 2017

Copy link
Copy Markdown
Contributor

Looks like one of the tests failed, should I be concerned aboutthat?

@Hainish

Hainish commented Jan 18, 2017

Copy link
Copy Markdown
Member Author

@cooperq the fetch test is bound to fail with this many files modified, we can safely ignore that.

@jeremyn

jeremyn commented Jan 19, 2017

Copy link
Copy Markdown
Contributor

@cooperq @Hainish Travis ended here with

The job exceeded the maximum time limit for jobs, and has been terminated.

I agree about ignoring some fetch errors for the purpose of this bulk change, but it's maybe another thing to ignore that the tests didn't actually complete.

@Hainish

Hainish commented Jan 19, 2017

Copy link
Copy Markdown
Member Author

@jeremyn with this many file changes they are bound to fail both by the accrual of the individual chances of random outages as well as by travis max timeouts

@jeremyn

jeremyn commented Jan 19, 2017

Copy link
Copy Markdown
Contributor

@Hainish You could split the deletions into smaller files that wouldn't time out. Removing preloaded rulesets shouldn't be urgent anyway.

@Hainish

Hainish commented Jan 20, 2017

Copy link
Copy Markdown
Member Author

@funkydude you'd have to provide the visual indication of HSTS preload protection in a privacy-preserving way. This means that we couldn't query for a specific domain in the preload list at the time of access, we'd have to fetch all the preloaded domains and look for inclusion from within the extension. Since this fetch is far from cheap, we'd have to get the preload list and store it upon browser startup (still kind of expensive) or first download of the addon (better), ensuring that if the browser is restarted the preload list corresponds to the current version of the browser. At this point, we'd be able to present that indicator, but keep in mind that we would again have to be loading all the HSTS preloaded domains into memory, which kind of defeats one of the major purposes of removing them in the first place.

@Hainish

Hainish commented Jan 20, 2017

Copy link
Copy Markdown
Member Author

@jeremyn this PR only removes targets. Simply removing a target from a ruleset should never introduce a fetch test failure in itself, if the ruleset was otherwise passing. If a file is modified at all, all the targets within that ruleset are tested. This means that it must have been one of the other targets that started failing since last this file was modified and tested.

Removing targets that are preloaded within the supported browsers is not introducing these errors, and it's outside the scope of this PR to fix rulesets which are failing otherwise. Therefore it is safe to ignore the errors in the fetch test.

Let me know if you see a flaw in this reasoning.

@cooperq

cooperq commented Jan 20, 2017

Copy link
Copy Markdown
Contributor

lgtm

@cooperq cooperq merged commit 7e621be into EFForg:master Jan 20, 2017
@Hainish

Hainish commented Jan 20, 2017

Copy link
Copy Markdown
Member Author

@funkydude I think the proper way to handle PRs which add preloaded domains is to run an additional test which fails if, after running the hsts-prune utility on the added/modified files, these files are modified any further.

@funkydude

Copy link
Copy Markdown
Contributor

@Hainish I agree that a check should be run on PRs, however that wouldn't save the time wasted by the user submitting it.

@Hainish

Hainish commented Jan 20, 2017

Copy link
Copy Markdown
Member Author

@funkydude I don't think we can ever guarantee that a user won't have wasted their time with a PR, since we can't ever guarantee that a PR will be merged

@Hainish

Hainish commented Jan 20, 2017

Copy link
Copy Markdown
Member Author

@funkydude #8226 introduces a new test, as described in #8128 (comment)

@Hainish

Hainish commented Jan 20, 2017

Copy link
Copy Markdown
Member Author

The removal of eff.org in this PR is actually causing some failures in our test suite for PRs that modify the core codebase: #8226

I'll have to fix this next week

@jeremyn

jeremyn commented Jan 22, 2017

Copy link
Copy Markdown
Contributor

@Hainish It's moot since this has been merged, but since you asked, this PR doesn't just remove targets. I pointed out in an earlier comment that you left in a test and you later added a commit to remove tests, securecookies, and rules. It may conceptually just remove targets but it's actually doing more. Maybe we had terrible luck and the XML parsing/regex/whatever you're using completely mangled every rule after the Travis time out. We hope not but that's what the tests are for.

Was this PR the result of running your new hsts-prune process on the existing rulesets?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants