Closed Bug 1299281 Opened 8 years ago Closed 7 years ago

the cert_domain_link in the certificate error page is inconsistent

Categories

(Firefox :: Security, defect, P5)

defect

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox51 --- affected
firefox55 --- fixed

People

(Reporter: keeler, Assigned: prathiksha, Mentored)

Details

(Whiteboard: [fxprivacy])

Attachments

(1 file, 1 obsolete file)

The certificate error page has a feature where if we're trying to get to www.example.com but the certificate is only valid for example.com (or if we're trying to get to example.com and the certificate is only valid for www.example.com), the advanced section will have a link the user can click to go to the domain that might work. If the certificate is valid for a domain that doesn't look like the one we're trying to visit, though, no href is set so clicking the link doesn't do anything. The problem is that it still looks like a link and is thus confusing.

I see a few options here:

1. We could remove the link functionality entirely (it only works if the certificate has exactly one dns name entry in the subject alternative name extension, and maybe linking the user to places from the certificate error page is not a great idea at all)

2. We could always make the link work (this would at least be consistent)

3. We could style it so it doesn't look like a link when clicking it won't do anything
This can be observed e.g. on https://64.4.250.33/ (which is actually PayPal but the link doesn't work).

I vote for Option 1.). There's enough complexity in the cert error pages and this sounds like the safest option.
Mentor: jhofmann
Priority: -- → P5
Whiteboard: [fxprivacy]
I'd like to be assigned to solve this  bug. :))
Great, thanks!

Considering the lengthy discussion around this in bug 402210 I want to avoid the controversy and first solve point 3) from comment 0 (style it so it doesn't look like a link when no href is set).

We can afterwards make a new bug discussing the general pointlessness (IMO) of this feature.

This is the place where the link is set or not: http://searchfox.org/mozilla-central/source/browser/base/content/aboutNetError.xhtml#479

You can either add some code that e.g. sets a class when the link was not set or implement this purely using CSS selectors.

The corresponding CSS file is here: http://searchfox.org/mozilla-central/source/browser/themes/shared/aboutNetError.css

Feel free to ping me if you have a question or want to bounce ideas.
Assignee: nobody → prathikshaprasadsuman
Okay. Thanks :))
Comment on attachment 8847447 [details]
Bug 1299281 - Style the cert_domain_link in the certificate error page to look like normal text when it does not link to anything.

https://reviewboard.mozilla.org/r/120420/#review122462

This works well, but we can still make it a bit cleaner :)

::: browser/base/content/aboutNetError.xhtml:491
(Diff revision 1)
>          if (thisHost.endsWith("." + okHost))
>            link.href = proto + okHost;
>  
> +        //If the link does not exist
> +        if(!link.href)
> +          link.classList.add("linkDoesNotExist");

It's better to do this purely using CSS, with a selector like #cert_domain_link:not(\[href\]), so you can probably remove this line.
Attachment #8847447 - Flags: review?(jhofmann) → review-
Comment on attachment 8847447 [details]
Bug 1299281 - Style the cert_domain_link in the certificate error page to look like normal text when it does not link to anything.

https://reviewboard.mozilla.org/r/120420/#review122472

::: commit-message-2baef:1
(Diff revision 1)
> +Bug 1299281 - the cert_domain_link in the certificate error page is styled to look like normal text when it does not link to anything. r?johannh

Please capitalize the first letter of the commit message. I also usually prefer commit messages in the style of

Do [thing] (to fix [broken thing])

so in your case

Style the cert_domain_link in the certificate error page to look like normal text when it does not link to anything

But I don't think there's an official guideline for it.
> It's better to do this purely using CSS, with a selector like #cert_domain_link:not(\[href\]), so you can probably remove this line.

Ugh, reviewboard escaped my square brackets. I meant to write #cert_domain_link:not([href])
Comment on attachment 8847447 [details]
Bug 1299281 - Style the cert_domain_link in the certificate error page to look like normal text when it does not link to anything.

https://reviewboard.mozilla.org/r/120420/#review122462

> It's better to do this purely using CSS, with a selector like #cert_domain_link:not(\[href\]), so you can probably remove this line.

Okay. I'll change it and submit. :) Thanks!
(In reply to Johann Hofmann [:johannh] from comment #7)
> Comment on attachment 8847447 [details]
> Bug 1299281 - the cert_domain_link in the certificate error page is styled
> to look like normal text when it does not link to anything.
> 
> https://reviewboard.mozilla.org/r/120420/#review122472
> 
> ::: commit-message-2baef:1
> (Diff revision 1)
> > +Bug 1299281 - the cert_domain_link in the certificate error page is styled to look like normal text when it does not link to anything. r?johannh
> 
> Please capitalize the first letter of the commit message. I also usually
> prefer commit messages in the style of
> 
> Do [thing] (to fix [broken thing])
> 
> so in your case
> 
> Style the cert_domain_link in the certificate error page to look like normal
> text when it does not link to anything
> 
> But I don't think there's an official guideline for it.

Okay. I'll follow this from now on. Sounds better than what I've written. :)
Comment on attachment 8847447 [details]
Bug 1299281 - Style the cert_domain_link in the certificate error page to look like normal text when it does not link to anything.

https://reviewboard.mozilla.org/r/120420/#review122494

Looks good, thanks!

We can skip the try build in this case, it's very unlikely this CSS change breaks anything.

I'll land it directly for you when you upload another version with the minor nit below fixed.

::: browser/themes/shared/aboutNetError.css:171
(Diff revision 2)
> +
> +#cert_domain_link:not([href]) {
> +  color: var(--in-content-page-color);
> +  text-decoration: none;
> +}
> +

Nit: I think we try to avoid trailing newlines in files, so please delete this line.
Attachment #8847447 - Flags: review?(jhofmann) → review+
Okay !
Comment on attachment 8847615 [details]
Bug 1299281 - Style the cert_domain_link in the certificate error page to look like normal text when it does not link to anything.

https://reviewboard.mozilla.org/r/120566/#review122512

I think you committed on top of the existing commit, that's why it added a new attachment instead of overwriting the existing one, please remove this commit with hg strip ... and commit --amend again.
Attachment #8847615 - Flags: review?(jhofmann)
Attachment #8847615 - Attachment is obsolete: true
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/558e813c8a92
Style the cert_domain_link in the certificate error page to look like normal text when it does not link to anything. r=johannh
https://hg.mozilla.org/mozilla-central/rev/558e813c8a92
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: