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)
Firefox
Security
Tracking
()
RESOLVED
FIXED
Firefox 55
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
Comment 1•7 years ago
|
||
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]
Assignee | ||
Comment 2•7 years ago
|
||
I'd like to be assigned to solve this bug. :))
Comment 3•7 years ago
|
||
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•7 years ago
|
||
Okay. Thanks :))
Comment hidden (mozreview-request) |
Comment 6•7 years 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•7 years 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.
Comment 8•7 years ago
|
||
> 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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
Okay !
Comment 16•7 years 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•7 years ago
|
Attachment #8847615 -
Attachment is obsolete: true
Comment 18•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/558e813c8a92
Status: NEW → RESOLVED
Closed: 7 years 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.
Description
•