the cert_domain_link in the certificate error page is inconsistent

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Security
P5
normal
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: keeler, Assigned: prathiksha, Mentored)

Tracking

Trunk
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox51 affected, firefox55 fixed)

Details

(Whiteboard: [fxprivacy])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

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@mozilla.com
Priority: -- → P5
Whiteboard: [fxprivacy]
(Assignee)

Comment 2

6 months ago
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
(Assignee)

Comment 4

6 months ago
Okay. Thanks :))
Comment hidden (mozreview-request)

Comment 6

5 months ago
mozreview-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/#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 7

5 months ago
mozreview-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])
(Assignee)

Comment 9

5 months ago
mozreview-review-reply
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!
(Assignee)

Comment 10

5 months ago
(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 hidden (mozreview-request)

Comment 12

5 months ago
mozreview-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/#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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

5 months ago
Okay !

Comment 16

5 months ago
mozreview-review
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)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8847615 - Attachment is obsolete: true

Comment 18

5 months ago
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

Comment 19

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/558e813c8a92
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.