Closed
Bug 423247
Opened 15 years ago
Closed 15 years ago
Certificate error for iframe is shown as a dialog instead of an error page, making it impossible to add an exception
Categories
(Core :: Security: PSM, defect, P2)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
People
(Reporter: pedrosoriarodriguez, Assigned: johnath)
References
()
Details
(Keywords: regression, testcase)
Attachments
(3 files, 1 obsolete file)
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4) Gecko/2008030318 Firefox/3.0b4 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4) Gecko/2008030318 Firefox/3.0b4 Using Ffox 3, beta4 on Linux Ffox 3 normally displays a warning when the SSL cert of the server cannot be verified or trusted. This warning includes the option to add an exception. The bug I am reporting is that the option to add an exception is not shown in a particular case. Reproducible: Always Steps to Reproduce: 1. Go to www.renfe.es (spanish ralways) 2. There's a pull down menu with the text "Principales Estaciones Origen" (departure train stations). In that menu, select any entry (for instance, "Madrid") 3. Then a warning dialog window comes up, with the following message: w1.renfe.es:443 uses an invalid security certificate. The certificate is not trusted because the issuer certificate is not trusted. (Error code: sec_error_untrusted_issuer) Actual Results: A warning about sec_error_untrusted_issuer is shown, without option to add an exception. User is locked out from using the website (destination train stations do not get to load up) Expected Results: Expected: a warning about sec_error_untrusted_issuer, with an option to add an exception. The destination train station list should load after selecting the departure train station. User should be able add exception, and continue to use the website. I will attach two screen shows: one with expected results (from another webpage) and one showing the bug. I believe the problem may be at the website, rather than Firefox. On this webpage (www.renfe.es), the website seems to load a page through SSL on the background, while the main page is not SSL-protected. I wonder if this case is not treated by FFox3 as expected. On other websites where sec_error_untrusted_issuer happens, the "Secure Connection Failed" message comes with the option to add an exception.
Reporter | ||
Comment 1•15 years ago
|
||
Reporter | ||
Comment 2•15 years ago
|
||
Comment 3•15 years ago
|
||
Here's a reduced teestcase: data:text/html,<iframe src="https://w1.renfe.es/"></iframe> I think Firefox should show an error page inside the iframe, since that's what happens if the server is simply unreachable. Is there a reason it shows a dialog, or is that just a bug?
Assignee: nobody → kengert
Blocks: https-error-pages
Status: UNCONFIRMED → NEW
Component: General → Security: PSM
Ever confirmed: true
Keywords: regression,
testcase
OS: Linux → All
Product: Firefox → Core
QA Contact: general → psm
Hardware: PC → All
Summary: sec_error_untrusted_issuer, no option to add exception. → Certificate error for iframe is shown as a dialog instead of an error page, making it impossible to add an exception
Version: unspecified → Trunk
Updated•15 years ago
|
Flags: blocking1.9?
Comment 4•15 years ago
|
||
dupe of bug 399876 (where Kai is already working on) ?
Comment 5•15 years ago
|
||
Yes, though that bug's summary is less specific than this one. I think we should probably separate the "warn for blocked loads due to invalid cert errors" and "display error pages in subframes" issues that have been conflated in that bug. Perhaps this bug can cover landing attachment 284998 [details] [diff] [review].
![]() |
||
Comment 6•15 years ago
|
||
That attachment should just get checked in, imo. I'm not sure why it hasn't been, exactly...
Updated•15 years ago
|
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Comment 7•15 years ago
|
||
(In reply to comment #6) > That attachment should just get checked in, imo. I'm not sure why it hasn't > been, exactly... Because we had agreed that complete silent is bad, because it makes it imppossible to diagnose what's failing. It has been proposed to log an error to the console, but until now we have no patch to do that.
Comment 8•15 years ago
|
||
I think not being able to see what's wrong on pages that use hidden iframes is better than getting an alert and not being able to add exceptions for pages that use visible frames, so preventing that patch from landing because we haven't figured out how to do error console reporting seems like a net loss.
Assignee | ||
Comment 9•15 years ago
|
||
(In reply to comment #7) > (In reply to comment #6) > > That attachment should just get checked in, imo. I'm not sure why it hasn't > > been, exactly... > > Because we had agreed that complete silent is bad, because it makes it > imppossible to diagnose what's failing. > > It has been proposed to log an error to the console, but until now we have no > patch to do that. Yeah, but I wouldn't want the user experience to suffer for that, if we expect that to be difficult, and bug 399876 comment 19 makes it sound like logging in a consistently helpful way might be. (In reply to comment #8) > I think not being able to see what's wrong on pages that use hidden iframes is > better than getting an alert and not being able to add exceptions for pages > that use visible frames, so preventing that patch from landing because we > haven't figured out how to do error console reporting seems like a net loss. And I think there's no reason not to keep that bug open to solve the more general problem (including logging!) while at the same time using this bug to fix the specific problem of frames. To that end, I've attached your patch from 399876 over here, and will give bz a review request, since the other one had no official review. Kai - you indicated in bug 399876 comment 20 that you felt basically okay with landing this patch, though you pointed out that the work was not all done. Are you okay with this approach that has us landing the patch here, and keeping that bug alive to track the question of other resource types, and logging?
Attachment #309989 -
Flags: review?(bzbarsky)
![]() |
||
Comment 10•15 years ago
|
||
> Because we had agreed that complete silent is bad
For subresource errors? We had? I must have missed it.... It's not ideal, but it's better than the status quo, in my opinion.
![]() |
||
Comment 11•15 years ago
|
||
Comment on attachment 309989 [details] [diff] [review] Kai's patch from 399876, applies cleanly >Index: security/manager/ssl/src/nsNSSIOLayer.cpp >+ nsCOMPtr<nsIDocShellTreeItem> rootItem; That doesn't need to be declared in this outermost scope, right? Please push the decl down to where it's used. >+ if (rootItem) It won't be null. I wouldn't bother checking. Especially since do_QI is null-safe, but even if it weren't, a treeitem always has a root item of same type. With those two nits fixed, r=bzbarsky.
Attachment #309989 -
Flags: review?(bzbarsky) → review+
Comment 12•15 years ago
|
||
(In reply to comment #9) > Kai - you indicated in bug 399876 comment 20 that you felt basically okay with > landing this patch, though you pointed out that the work was not all done. Are > you okay with this approach that has us landing the patch here, and keeping > that bug alive to track the question of other resource types, and logging? Ok, thanks.
Comment 13•15 years ago
|
||
(In reply to comment #10) > > Because we had agreed that complete silent is bad > > For subresource errors? We had? I must have missed it.... It's not ideal, > but it's better than the status quo, in my opinion. bug 399876 comment 8, your second comment. you propose to log to the error console. And in bug 399876 comment 21 and 23 you tried to assist me to get the logging done. But let's not nit pick on who said what. I hope we can agree that logging detected errors for diagnosing is a good thing, and that we should do it if we can do it. And I'm fine to postpone it to a later time, as Gavin and Johnathan proposed.
Assignee | ||
Comment 14•15 years ago
|
||
(In reply to comment #11) > (From update of attachment 309989 [details] [diff] [review]) > >Index: security/manager/ssl/src/nsNSSIOLayer.cpp > >+ nsCOMPtr<nsIDocShellTreeItem> rootItem; > > That doesn't need to be declared in this outermost scope, right? Please push > the decl down to where it's used. Moved inside the |if (item)| block. > >+ if (rootItem) > > It won't be null. I wouldn't bother checking. Especially since do_QI is > null-safe, but even if it weren't, a treeitem always has a root item of same > type. Removed the check, and changed the assert text to be slightly more meaningful.
Assignee | ||
Comment 15•15 years ago
|
||
Comment on attachment 310018 [details] [diff] [review] Address review comments In principle, I need review from a PSM peer, and Kai's out since this is really his patch. Tagging Bob, feel free to punt to WTC if that's a better play.
Attachment #310018 -
Flags: review?(rrelyea)
Comment 16•15 years ago
|
||
Comment on attachment 310018 [details] [diff] [review] Address review comments r+ Verified this patch incorporates the review comments from bz.
Attachment #310018 -
Flags: review?(rrelyea) → review+
Assignee | ||
Comment 17•15 years ago
|
||
Checking in security/manager/ssl/src/nsNSSIOLayer.cpp; /cvsroot/mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp,v <-- nsNSSIOLayer.cpp new revision: 1.153; previous revision: 1.152 done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Flags: in-testsuite?
Comment 18•15 years ago
|
||
I tried to verify this bug, but I stumbled on another problem : bug 423833
Assignee | ||
Comment 19•15 years ago
|
||
(In reply to comment #18) > I tried to verify this bug, but I stumbled on another problem : bug 423833 Yep, that's a bug alright! :) In the meantime, we can verify this fix using a url like: data:text/html,<html><frameset cols="500,500"><frame src="http://google.com"><frame src="https://kuix.de:8443"></frameset></html>
You need to log in
before you can comment on or make changes to this bug.
Description
•