Opened 5 years ago

Closed 4 years ago

#8776 closed defect (fixed)

Changes to default ruleset state need to work even if the rule was previously present

Reported by: pde Owned by: micahlee
Priority: Very High Milestone: HTTPS-E 4.0dev7
Component: HTTPS Everywhere/EFF-HTTPS Everywhere Version:
Severity: Keywords:
Cc: schoen, lisacyao, mikeperry Actual Points:
Parent ID: #8774 Points:
Reviewer: Sponsor:

Description

This was something we have worked around until now. But when we fix #8774 we don't want to have to re-name all of the affected rulesets.

Child Tickets

Change History (8)

comment:1 Changed 4 years ago by pde

Owner: changed from pde to micahlee
Status: newassigned

comment:2 Changed 4 years ago by pde

Priority: majorcritical

comment:3 Changed 4 years ago by micahlee

Here's my plan for fixing this. I'm adding a new preference called extensions.https_everywhere.rules_checked_version.

When src/chrome/content/code/HTTPSRules.js gets loaded it will see if the value in rules_checked_version is less than the current version of HTTPS Everywhere. If so, it will recheck all of the rules and then set rules_checked_version to the current HTTPS Everywhere version.

Here's how I can figure out the current version of HTTPS Everywhere: http://stackoverflow.com/questions/1088407/how-can-a-firefox-extension-get-its-own-version-number-programmatically

I don't think it should automatically enable any rules on upgrade, only disable them (so that if people had manually disabled broken rules in the past they don't need to worry about upgrades re-enabling them). It should look at the default_off and platform attributes and, based on those, decide if each rule should be disabled.

This is basically introducing a sort of preference migration system into HTTPS Everywhere.

comment:4 Changed 4 years ago by micahlee

I think we should change how rules are saved in the Firefox preferences. Right now all the rules are saved as bools in extensions.https_everywhere.RULENAME.

Instead I think we should split this up into two branches:

extensions.https_everywhere.default_rules.RULENAME
extensions.https_everywhere.rule_changes.RULENAME

The default_rules prefs branch will always contain the default state of the rule. When opening Firefox and re-parsing the rules, it should update values of default_rules prefs if anything has changed.

The rule_changes prefs branch should only record the status of rules that have been manually toggled by the user. Resetting all of the rules should just delete all of the rules under the rule_changes branch.

When deciding if a rule is active, first it should check rule_changes.RULENAME and use that value if it's there. If it isn't, it should fallback to default_rules.RULENAME.

This will involve migrating preferences. Seems like a big job. I'm wondering if it makes sense to make this a separate ticket and instead just disable rules on upgrading, like I was planning above, and make this a separate bug to do later.

And just for my own reference, here's the appropriate parts of the code to edit:

The code that parses each rule and attempts to add new rules is in the RuleSet function in src/chrome/content/code/HTTPSRules.js.

The code that gets the prefs branch is in src/components/https-everywhere.js in the get_prefs() function, in HTTPSEverywhere.prototype. I'll probably have to figure out all the places that call get_prefs(), and make new functions get_default_rules_prefs() and get_rule_changes_prefs() and make them call the appropriate function.

I still need to dig around more to find the piece of code that decides if a rule is on or not.

comment:5 Changed 4 years ago by schoen

I don't see that we need to keep the default_rules.RULENAME data in the preferences. The default state of the rule can be learned from the rule itself.

Instead, we could start storing only when someone has manually changed the state of a rule (like rule_changes.RULENAME). If any information at all is present for rule_changes.RULENAME, we can comply with that state; otherwise, we can use the default state declared in the rule itself.

comment:6 Changed 4 years ago by micahlee

schoen, good call on not needing to save default rules since we already have them in memory.

I have a working patch. I think it still needs a bit of work, but I've pushed it the ruleset-migration-8776 branch on my github repository: https://github.com/micahflee/https-everywhere/tree/ruleset-migration-8776

Will people here please try it out and help test it? It's kind of a significant change, and HTTPS Everywhere doesn't have unit tests (!), so manually have to be thorough.

This code now ignores all of the ruleset about:config prefs. Instead, when you toggle a rule it saves a new pref in a new namespace, extensions.https_everywhere.rule_toggle.*. If you haven't toggled a rule, it defaults to what's in the xml file.

In this way, we can change default_off and platform attributes of rules whenever we want and it will change the default rule state in people's browsers after they upgrade. The only prefs that will be saved between upgrades are rulesets that people manually enable or disable.

Resetting rulesets to defaults now just deletes the associated extensions.https_everywhere.rule_toggle.* rule, so it falls back to the default.

All of this seems to work fine now with this patch, including the preferences dialog.

Now here's the zinger. With this next upgrade, everyone's saved preferences will get reset back to defaults. Can anyone think of a way around this?

If, on upgrade, we loop through their existing extensions.https_everywhere.* prefs and make new extensions.https_everywhere.rule_toggle.* prefs only if they differ from the default, that would work. Only it would totally defeat the purpose of this ticket: when we add platform="mixedcontent" to rules their new default state will be off (what we want), but this logic would save a new rule_toggle pref that will keep them on.

Right now I'm thinking of attaching a notification box to the top of the browser just for users who are upgrading (new users won't see it) that lets them know that all of their enabled/disabled rules prefs have been reset to default. Is this acceptable? I have a feeling that the majority of our users never toggle any rules on or off, though I don't have data to back that up.

comment:7 Changed 4 years ago by micahlee

Cc: schoen lisacyao mikeperry added

I'm cc'ing schoen, lisacyao, and mikeperry on this ticket. You all might have valuable input on my recent patch (https://trac.torproject.org/projects/tor/ticket/8776#comment:6).

comment:8 Changed 4 years ago by micahlee

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.