Opened 9 years ago

Closed 9 years ago

#3815 closed defect (fixed)

context menu does not work on error pages

Reported by: pde Owned by: pde
Priority: Medium Milestone:
Component: HTTPS Everywhere/EFF-HTTPS Everywhere Version:
Severity: Keywords:
Cc: micahlee, jacobske87@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

For instance, per this report from Micah

When you go to a website that doesn't load and instead gives you a
certificate browser, and then click the context menu to disable that
rule, that website's rule doesn't appear in the list.

Or at least, that's what happened to me when I went to torrentfreak.com
yesterday on a freshly installed Firefox in Windows. I think
torrentfreak.com doesn't serve the whole certificate chain and I hadn't
cached the correct intermediary certificate, so it gave me a warning.
When I tried disabling the rule, the TorrentFreak rule wasn't in the
context menu, only Google Search was (which also seems strange).

This should be easy enough to recreate on any website with an HTTPS
Everywhere rule just by using an intercepting proxy that MITMs HTTPS and
serves it's own certificates.</blockquote>

Child Tickets

TicketStatusOwnerSummaryComponent
#4173newjacobske87When disabling a rule for the top-level page URL, reload as HTTP not HTTPSHTTPS Everywhere/EFF-HTTPS Everywhere

Attachments (1)

https-everywhere-latest-3815Patched.xpi (124.8 KB) - added by jacobske87 9 years ago.
Fix for the bug

Download all attachments as: .zip

Change History (7)

comment:1 Changed 9 years ago by pde

Status: newaccepted

The underlying reason for this is that (a) the ApplicableList data structures used to populate the context menu are attached to nsIDOMWindows; (b) a new nsIDOMWindow is created when the error occurs; (c) naively, we don't seem to learn the URI that caused the error from within the error window.

Probably the easiest way forward would be to find a way around (c). Followed by finding a way to bridge across the information loss at (b). Getting (a) to work at all was a nightmare that I do not wish to revisit.

Changed 9 years ago by jacobske87

Fix for the bug

comment:2 Changed 9 years ago by jacobske87

Cc: jacobske87@… added
Status: acceptedneeds_review

I made a fix for this. It seems to work on every page I've tried. Commit is here:

https://github.com/kevinjacobs/HTTPS-Everywhere/commit/d66a1398076832d3fd52f9509268ac1488be3b43#diff-0

Also made a band-aid fix for http://i.imgur.com/Y9y8J.jpg (it was annoying me :))commit here:
https://github.com/kevinjacobs/HTTPS-Everywhere/commit/dbcd1d983f830b845f061c260b250e615d03a98d

For the 3815 bug, this approach is probably more fragile than a proper fix, but MUCH simpler too... I haven't found any domains it doesn't work.

Let me know your thoughts on this approach.. You can enable the Techcrunch, 33bits, bittorrent.org, or any rule that triggers a cert error to test.

comment:3 Changed 9 years ago by jacobske87

Note that when disabling the rule from the context menu, it reloads the page as https again (and you have to manually remove the "s" to go back to http).

Not sure what function is doing the reload, maybe you know of a quick fix for that?

comment:4 Changed 9 years ago by pde

Nice work Kevin, this looks like a good hack solution. Does it need to be specific to about:certerror? Malfunctioning rulesets can also cause network errors (about:neterror), for instance.

I will merge this just as soon as I've finished mastering releases to deal with impending catastrophe from #3882.

As for the reload, it's being performed by reload_window() in toolbar_button.js.

comment:5 Changed 9 years ago by jacobske87

Here's the adjustment for any "about:" page. https://github.com/kevinjacobs/HTTPS-Everywhere/commit/d44e2992d502d16901b788695de539ca2cd455cc Either one should work.

As for the reload_window() function, see my commit and comment here: https://github.com/kevinjacobs/HTTPS-Everywhere/commit/6881d382802e02d041857cc4f7426603b50a8584. Would that type of approach be okay, or do you have a better way in mind? I can create another ticket for this issue if you'd prefer.

comment:6 Changed 9 years ago by pde

Resolution: fixed
Status: needs_reviewclosed

Hi Kevin,

I committed the "about" change. Let's open another ticket for the reload issue. It isn't obvious to me what we should do. Perhaps inverting our HTTPS->HTTP transform in the case of a top level URI change makes sense, but we'd need to remember what transform we had applied. And there are lots of nasty cases. What do we do, for instance, about cookies that we marked as secure? Should we go back and remove that flag?

Note: See TracTickets for help on using tickets.