[FIX]URL: Invalid protocols in HREF links fail silently

VERIFIED FIXED in mozilla1.3alpha

Status

()

Core
Networking
P2
normal
VERIFIED FIXED
16 years ago
13 years ago

People

(Reporter: Gregory McLean, Assigned: bz)

Tracking

({testcase})

Trunk
mozilla1.3alpha
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-P0][Hixie-1.0], URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

16 years ago
Attached html presents a link when clicked on does nothing. No error's no
response on the browser's part at all.

View-Source and paste the url in the url bar will generate a popup with 'invaild
protocol httpf'.

Clicking should maybe produce the same?

Linux build 2001061712 from the blizzard rpms.
(Reporter)

Comment 1

16 years ago
Created attachment 38992 [details]
Simple test case
(Reporter)

Comment 2

16 years ago
Possibly a dup of 46537, if not its in the same family ;)

Comment 3

16 years ago
Greg:
changed summary. This is a really good catch, b/c I'm not where I wanted to be
in testing this stuff... I updated bug 46537 too, but I think we should keep
them separate

Gagan: 
since the errors come back correctly in some entry points, isn't this someone
else's code talking to Necko that needs to trap the error?
Blocks: 61999
Summary: Invaild protocol's in links fail silently. → URL: Invaild protocol's in HREF links fail silently
If I do open link in new window, I get a security manager assertion:

###!!! ASSERTION: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: '(!((rv) &
0x80000000))', file nsScriptSecurityManager.cpp, line 808
###!!! Break: at file nsScriptSecurityManager.cpp, line 808
JavaScript error: 
 line 0: uncaught exception: Load of httpf://www.cnn.com/ denied.

This is because the call to NS_NewURI failed, and something is probably turning
a CAPS failure into a security exception. mstoltz, does that sound right?

I don't know if thats the same cause when just clicking on a link, though. I
don't get that assertion, and don't hit the one at the bottom of CheckLoadUri
either.

Comment 5

16 years ago
In nsGenericElement::TriggerLink(..) we return an error from NS_NewURI(..). We 
then do a CheckLoadURI(..) only if rv succeeds. 

proceed = securityManager->CheckLoadURI(..)

Now, since "proceed" is already initialized to NS_OK,  we call OnLinkClick(..)
// Only pass off the click event if the script security manager
// says it's ok.
    if (NS_SUCCEEDED(proceed))
      handler->OnLinkClick(this, aVerb, absURLSpec.GetUnicode(),
                           aTargetSpec.GetUnicode());
Assignee: neeti → jst
Component: Networking: HTTP → Layout

Updated

16 years ago
Target Milestone: --- → mozilla1.0
Component: Layout → Networking
Keywords: testcase
Summary: URL: Invaild protocol's in HREF links fail silently → URL: Invalid protocol's in HREF links fail silently
Well, clicking on a link which is insecure should _also_ give an error.

e.g., if in a web page there is a link to about:cache, then we should report an
error if the user clicks on it.

See also bug 28586, which  is complaining about the fact that error messages should
be inline (as a web page) instead of dialogs.
Whiteboard: [Hixie-P0][Hixie-1.0]

Comment 7

16 years ago
See also bug 47617, Connection to https needs to tell user to install PSM if 
w/o.

See also bug 84128, Need user-visible message when CheckLoadURI fails (eg, for 
links to file: urls).
Summary: URL: Invalid protocol's in HREF links fail silently → URL: Invalid protocols in HREF links fail silently

Comment 8

16 years ago
Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)
Target Milestone: mozilla1.0 → mozilla1.0.1

Comment 9

16 years ago
+mozilla1.0 - please bring this back to 1.0
Keywords: mozilla1.0
Why would this need to be fixed for mozilla1.0? It doesn't break anything as
long as people do the right thing, it only breaks in cases where we'd break anyway.

Comment 11

16 years ago
This should be fixed ASAP. It's a user or web manaager problem, but Mozilla
should at least say that "httpf" or "zzvkjdfr" is an invalid protocol. This kind
of error handling would tell the user that Mozilla is still working, but the
link itself is broken.

Comment 12

16 years ago
*** Bug 112171 has been marked as a duplicate of this bug. ***

Comment 13

16 years ago
Is this bug a dupe of bug 62744?

Comment 14

16 years ago
Plat: PC --> All, OS: Linux --> All 
Removing URL http://tweetie.comstar.net/test.html, because it is 404.
OS: Linux → All
Hardware: PC → All
Andrew, this is not a dup of bug 62744.
Darin, was this fixed by your checkin to bug 47617 (Connection to https needs to
tell user to install PSM if w/o) ?  Seems that you left the error message as
generic for this purpose.

Comment 17

16 years ago
the fix for bug 47617 only handles toplevel document loads that fail with
NS_ERROR_UNKNOWN_SOCKET_TYPE.  that doesn't apply here.

Comment 18

16 years ago
It's fixed. I don't know when, but as of 0.9.7, at the latest, worksforme on
Windows 98. 

Comment 19

16 years ago
Now using 0.9.7 Build ID: 2002011003, it's broken again.

Comment 20

15 years ago
I don't know if this is covered elsewhere, but data:foo doesn't work either.
(should be data:text/html,foo)

Updated

15 years ago
Keywords: mozilla1.0 → mozilla1.3

Updated

15 years ago
Target Milestone: mozilla1.0.1 → ---

Comment 21

15 years ago
data: has it's own set of bugs. please search Browser:Networking. Properly
written bugs should have "data:" as start of the summary.
*** Bug 181008 has been marked as a duplicate of this bug. ***
mine
Assignee: jst → bzbarsky
Priority: -- → P2
Target Milestone: --- → mozilla1.3alpha
Created attachment 106899 [details] [diff] [review]
Move the error reporting down into DoURILoad
Attachment #106899 - Flags: superreview?(darin)
Attachment #106899 - Flags: review?(adamlock)
Summary: URL: Invalid protocols in HREF links fail silently → [FIX]URL: Invalid protocols in HREF links fail silently

Comment 25

15 years ago
Comment on attachment 106899 [details] [diff] [review]
Move the error reporting down into DoURILoad

so, you're moving the error "trap" into DoURILoad because we only care about
alerting the user about errors to create a channel?1?  i'm not sure i follow.
Right now we have the error trap in a few places:

nsWebShell::EndPageLoad
nsDocShell::IsPrintingOrPP (this one doesn't have much to do with pageload)
nsDocShell::LoadURI (the version that takes a PRUnichar* uri)

I'm taking this last check and moving it down so that callers of both versions
of nsDocShell::LoadURI (the PRUnichar* version and the nsIURI version) as well
as direct callers of nsDocShell::InternalLoad will benefit from it.  I'm leaving
the check for NS_ERROR_MALFORMED_URI in nsDocShell::LoadURI (PRUnichar* version)
because that's the only place it's relevant.  So now we report the following errors:

1)  If we start with a string, we will report NS_ERROR_MALFORMED_URI as needed
2)  Once we have a URI, we will report any error in attempting to create a
    channel (that's a recognized error as far as DisplayLoadError is concerned)
3)  Once we have a channel, I suppose we could have errors in trying to open it.
    In fact, we have some code there to deal with port blocking (see
    nsDocShell::DoChannelLoad).  Perhaps it would be better to put the error
    report around the entire call to DoURILoad in nsDocShell::InternalLoad
4)  Once the channel is open, we will get state change notifications via our
    nsIWebProgressListener interface so we will call EndPageLoad() and its error 
    handling.

So we only need to handle errors up to the point when we have an open channel...
Created attachment 106921 [details] [diff] [review]
So more like this.
Attachment #106899 - Attachment is obsolete: true
Attachment #106899 - Flags: superreview?(darin)
Attachment #106899 - Flags: review?(adamlock)
Attachment #106921 - Flags: superreview?(darin)
Attachment #106921 - Flags: review?(adamlock)
Actually, I should check the return value from the OnStartURIOpen call. 
Consider that fixed; it is in my local tree (using a different nsresult variable
than rv so that I don't clobber rv), like so:

            PRBool abort = PR_FALSE;
            nsresult rv2 = mContentListener->OnStartURIOpen(aURI, &abort);
            if (NS_SUCCEEDED(rv2) && abort) {
                // Hey, they're handling the load for us!  How convenient!
                return NS_OK;
            }

Comment 29

15 years ago
Comment on attachment 106921 [details] [diff] [review]
So more like this.

yeah, that makes a lot more sense to me.  thanks!  assuming adam is okay with
this patch, sr=darin.
Attachment #106921 - Flags: superreview?(darin) → superreview+

Comment 30

15 years ago
Comment on attachment 106899 [details] [diff] [review]
Move the error reporting down into DoURILoad

Shouldn't that DisplayLoadError go inside the NS_ERROR_UNKNOWN_PROTOCOL block?
Adam, did you read the last patch or the previous one?  Did you read Darin's
comments?  The idea here is that we want to catch all errors from DoURILoad and
let ReportLoadError decide whether it wants to report them.

Comment 32

15 years ago
Comment on attachment 106921 [details] [diff] [review]
So more like this.

Sorry, was looking at a stale copy of the bug. r=adamlock
Attachment #106921 - Flags: review?(adamlock) → review+
All good.  ;)  Patch checked in; thanks for the reviews.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Blocks: 147414

Comment 34

15 years ago
VERIFIED: Mozilla 1.3a, Win 98.

If anyone wants to verify for Linux and Mac before I do, click on the URL field
label, and then remove the comment for the plaform you tested on...
URL: httpf:
Whiteboard: [Hixie-P0][Hixie-1.0] → [Hixie-P0][Hixie-1.0] checkmac checklinux
*** Bug 100176 has been marked as a duplicate of this bug. ***

Comment 36

15 years ago
VERIFIED:
Mac OS X, Linux - Mozilla 1.3a.
Status: RESOLVED → VERIFIED
Whiteboard: [Hixie-P0][Hixie-1.0] checkmac checklinux → [Hixie-P0][Hixie-1.0]

Comment 37

13 years ago
*** Bug 140025 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.