Closed Bug 405862 Opened 17 years ago Closed 17 years ago

Override netErrorApp.dtd to let users add cert exceptions from the error page

Categories

(SeaMonkey :: Security, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0a1

People

(Reporter: kairo, Assigned: kairo)

References

Details

Attachments

(1 file, 1 obsolete file)

Once bug 403800 has landed, we should add access to the cert exception dialog to SeaMonkey cert error pages, just like Firefox did in bug 401575, but with overriding just netErrorApp.dtd and not the the whole netError.dtd
Depends on: 401575
This is a port of the part of bug 401575 that changes the securityOverride, so that SeaMonkey users get the same access to adding security exceptions :)
Attachment #290715 - Flags: superreview?(neil)
Attachment #290715 - Flags: review?(neil)
BTW, if you need URLs with cert problems for testing, try of of those:
https://amazon.com
https://gmail.com
Comment on attachment 290715 [details] [diff] [review]
port patch from 401575 to SeaMonkey

Shifting sr to jag as Neil wants his input anyways.
Attachment #290715 - Flags: superreview?(neil) → superreview?(jag)
Comment on attachment 290715 [details] [diff] [review]
port patch from 401575 to SeaMonkey

>+  // Attach a listener to watch for "command" events bubbling up from error
>+  // pages.  This lets us fix bugs like 401575 which require error page UI to
>+  // do privileged things, without letting error pages have any privilege
>+  // themselves.
>+  gBrowser.addEventListener("command", BrowserOnCommand, false);
You can do this in navigator xul - just add oncommand="BrowserOnCommand(event);" to the <tabbrowser>.

>+      if (ot == errorDoc.getElementById('exceptionDialogButton')) {
Why not simply compare ot.id directly?

>+        var params = { location : content.location.href,
>+                       exceptionAdded : false };
Wrong location - errorDoc might be a frame.

>+          content.location.reload();
And again.

>+      else if (ot == errorDoc.getElementById('getMeOutOfHereButton')) {
I don't want a get me out of here button. (FYI there's a bug in your code.)

>+# the following files are suite-specific overrides
>+* locale/@AB_CD@/global/netErrorApp.dtd                                     (%chrome/overrides/netErrorApp.dtd)
This isn't how you do an override. (It won't work with XULrunner.)

>+netError.xhtml) because it exposes functionality specific to firefox. -->
                                                               ^^^^^^^

What about error pages in the sidebar?
Attachment #290715 - Flags: review?(neil) → review-
I copied almost all code for this from FF, so all bugs in "my" code are probably bugs in FF code as well, and probably should be fixed on both sides then.

(In reply to comment #4)
> >+  gBrowser.addEventListener("command", BrowserOnCommand, false);
> You can do this in navigator xul - just add
> oncommand="BrowserOnCommand(event);" to the <tabbrowser>.

Hmm, OK, adding the comment there is not that easy, I guess, but I can do that.

> >+      if (ot == errorDoc.getElementById('exceptionDialogButton')) {
> Why not simply compare ot.id directly?

Perhaps because this also ensures that this is a really existing DOM element?
Just guessing, as I only copied the code.

> >+        var params = { location : content.location.href,
> >+                       exceptionAdded : false };
> Wrong location - errorDoc might be a frame.
> 
> >+          content.location.reload();
> And again.

Ugh. How do I get the correct frame then? BTW, this bug is in FF as well, in this case, so we should backport whatever fix we have for that.

> >+      else if (ot == errorDoc.getElementById('getMeOutOfHereButton')) {
> I don't want a get me out of here button. (FYI there's a bug in your code.)

Why don't you want that button? I think it's a really good idea to have there. And I don't want to have the exception button as the primary thing to attract the user, as he should really only click it if he is really sure that it is a good idea.

> >+# the following files are suite-specific overrides
> >+* locale/@AB_CD@/global/netErrorApp.dtd                                     (%chrome/overrides/netErrorApp.dtd)
> This isn't how you do an override. (It won't work with XULrunner.)

This is how FF does it. How to do it in the right way?

> >+netError.xhtml) because it exposes functionality specific to firefox. -->
>                                                                ^^^^^^^

Oops, too much c&p there ;-)

> What about error pages in the sidebar?

Hmm, good question, I guess they currently get failing buttons. Would probably be better to move the function to utilityOverlay.js or such then...
(In reply to comment #5)
> > >+        var params = { location : content.location.href,
> > >+                       exceptionAdded : false };
> > Wrong location - errorDoc might be a frame.
> > 
> > >+          content.location.reload();
> > And again.
> 
> Ugh. How do I get the correct frame then? BTW, this bug is in FF as well, in
> this case, so we should backport whatever fix we have for that.

Bah, bz mentioned this in the Firefox bug, but we didn't fix it correctly. Use errorDoc.location. Can you file a bug on johnath, too?
(In reply to comment #6)
> Use errorDoc.location. 

Will do, thanks.

> Can you file a bug on johnath, too?

Done. bug 407369
(In reply to comment #5)
>>>+      else if (ot == errorDoc.getElementById('getMeOutOfHereButton')) {
>>I don't want a get me out of here button. (FYI there's a bug in your code.)
>Why don't you want that button? I think it's a really good idea to have there.
Because jag and I think it looks unprofessional and panic-inducing.

>And I don't want to have the exception button as the primary thing to attract
>the user, as he should really only click it if he is really sure that it is a
>good idea.
Well then maybe we should provide something harder to click. A tools menuitem? Or maybe a notification ;-)

>>>+# the following files are suite-specific overrides
>>>+* locale/@AB_CD@/global/netErrorApp.dtd                                     (%chrome/overrides/netErrorApp.dtd)
>>This isn't how you do an override. (It won't work with XULrunner.)
>This is how FF does it. How to do it in the right way?
(On one line, but obviously Bugzilla is going to wrap this...)
% override chrome://global/locale/netErrorApp.dtd chrome://communicator/locale/netErrorApp.dtd
(In reply to comment #8)
> (In reply to comment #5)
> >>I don't want a get me out of here button. (FYI there's a bug in your code.)
> >Why don't you want that button? I think it's a really good idea to have there.
> Because jag and I think it looks unprofessional and panic-inducing.

Panic is probably right for a normal user when he encounters a certificate error.

> >And I don't want to have the exception button as the primary thing to attract
> >the user, as he should really only click it if he is really sure that it is a
> >good idea.
> Well then maybe we should provide something harder to click. A tools menuitem?
> Or maybe a notification ;-)

A notification or a menuitem would both be completely out of the read flow of someone looking at this page. The whole reason for providing this at the bottom of the error page is that it follows the natural flow of reading the page.
If we do this in a notification bar, people would just not read the page and click the button there right away.
And doing security exceptions is a quite scary thing to do, so we really want people to know what they are doing when they do this.

> % override chrome://global/locale/netErrorApp.dtd
> chrome://communicator/locale/netErrorApp.dtd

Ah, sure, will use that. We probably should get FF to do the same.
(In reply to comment #4)
> What about error pages in the sidebar?

Just like for frames (bug 399876), we don't show an error page but a error dialog for cert errors in sidebars. I hope the patch for 399876 will fix those as well though, and I'll attach a patch in a minute that should let us make this approach work on them as well.
This is a patch with all the above nits fixed.
This approach is probably the farthest I can take this myself. If we want a completely different approach to getting people to the exceptions dialog, I'm probably unable to do the patch (I not sure I even fully understand the JS I'm copying here). This patch brings us up to the same way of dealing with this as Firefox is using for their in many cases probably even less educated users, so I think we should be safe in doing the same in UI terms basically.
Attachment #290715 - Attachment is obsolete: true
Attachment #292776 - Flags: superreview?
Attachment #292776 - Flags: review?(neil)
Attachment #290715 - Flags: superreview?(jag)
Comment on attachment 292776 [details] [diff] [review]
patch, v2: fix nits from v1

>+    if (ot == errorDoc.getElementById('exceptionDialogButton')) {
I still think that ot.id == 'exceptionDialogButton' should work here.

>+      var params = { location : errorDoc.location.href,
>+                      exceptionAdded : false };
Nit: Indentation botched.

>+      window.openDialog('chrome://pippki/content/exceptionDialog.xul',
>+                        '','chrome,centerscreen,modal', params);
Nit: space after '',

>+        url = pref.getComplexValue("browser.startup.homepage",
>+                                    Components.interfaces.nsIPrefLocalizedString).data;
>+        // If url is a pipe-delimited set of pages, just take the first one.
>+        if (url.indexOf("|") != -1)
>+          url = url.split("|")[0];
We don't need this because we save our home pages differently.
Attachment #292776 - Flags: review?(neil) → review+
Attachment #292776 - Flags: superreview? → superreview?(jag)
Comment on attachment 292776 [details] [diff] [review]
patch, v2: fix nits from v1

I'd move vars |ot| and |errorDoc| before the if so you can .test(ot.documentURI)

sr=jag
Attachment #292776 - Flags: superreview?(jag) → superreview+
Checked in with all nits fixed (and a short IRC discussion with jag).
I renamed errorDoc to ownerDoc in the process, as after moving it for addressing comment 13, it's not necessarily pointing to and error document any more.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
So it was decided to use the getMeOutOfHereButton after all?
(In reply to comment #15)
> So it was decided to use the getMeOutOfHereButton after all?

Yes, but with a more helpful text. We discussed that on IRC before I did the final patch.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: