Closed Bug 30817 Opened 25 years ago Closed 24 years ago

Should not resolve aliases on LINKed stylesheet with empty HREF

Categories

(Core :: XPCOM, defect, P3)

Product:
Core
Component:
XPCOM
Platform:
PowerPC
Mac System 8.5
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: pierre, Assigned: attinasi)

Details

Attachments

(1 file)

testcase to store as a local html file
85 bytes, text/html
Details 
Reproduced with latest bits on the Mac.

- Put some aliases to remote AppleShare volumes on your desktop (then unmount 
these volumes)
- Store the attached testcase as a local file on your desktop. The testcase 
contains the following code:
        <html>
        <link rel="stylesheet" href="">
        <body>
            Hello world wide web!
        </body>
        </html>
- Load the testcase in Mozilla
==> If the volumes don't require a password, they wil be automatically mounted. 
If the volumes require a password, the AppleShare password dialog is displayed.

The bug comes from the empty HREF in the LINKed stylesheet (an empty HREF is 
invalid of course)

I recognize that the bug as described is not very common. Basically, it happens 
when you have aliases to remote volumes in the same directory as a local file 
that contains invalid HTML code but... I think it reflects a flaw in the way we 
handle empty URLs in the file system and it may raise questions from a security 
standpoint.
Summary: Resolve aliases on LINKed stylesheet with empty HREF → Should not resolve aliases on LINKed stylesheet with empty HREF
Doug ?
Assignee: dp → dougt
setting milestone.
Target Milestone: M16
rick, could you lend a hand?
rickg?
Assignee: dougt → rickg
Pierre: I don't get your meaning. Can you please restate the problem?
Assignee: rickg → pierre
Rick- The only thing you need to know is that when the HREF is empty some pretty 
bad things happen on the Mac. I think that my description was fairly accurate but 
if you're not a Mac person, you are unlikely to understand what's happening. Here 
it is again:
- Using the Chooser, mount some volumes from a remote AppleShare server that 
requires a password (ie. a server where you don't log into as Guest).
- In the Finder, select these volumes then do "File|Make Alias". Leave the 
aliases on the desktop.
- Put the volumes away (ie. drag them to the trash)
- Save the attached testcase on your desktop as an HTML file.
- Launch Mozilla
- In a browser window, load the HTML file above.
===> The Finder asks for your password in order to mount the volumes that you 
have just put away. This is bad for two reasons:
1) The testcase triggered a piece of code that scans the entire directory where 
the HTML file is stored. It doesn't make any sense.
2) We should never resolve an alias that requires to mount a volume, except as a 
direct response to an action from the user (for instance, using an alias for an 
email attachment).

It is very common for Mac users to keep aliases to their favorite AppleShare 
volumes on the desktop. It is of course much less common to load in the browser a 
local file that has the kind of code you can see in the testcase.

I don't see why dougt reassigned that problem to you. If he needs help from you 
(let's say to detect when the HREF is empty) then he should be more specific. 
Reassigned back to him.
Assignee: pierre → dougt
pierre
I have no idea why an empty href would cause any problems.  In fact, I am not
sure what expected or intented behavior is.  That is the reason I asked rick to
comment on this bug.

We have 2 problems:
(1) An empty HREF should be ignored. It could be rickg's bug.
(2) We have somewhere, probably in XPCOM, a piece of code that crawls through a 
directory and resolves all the aliases, including the AppleShare aliases. This 
code is evil and I detected its presence because it is triggered by (1).

If we fix (1) first, you won't be able to see (2). The course of action I propose 
is to look into (2) - understand why we have that code and when it should be used 
- then reassign to rickg in order to fix (1).

Simon, it sounds like nsLocalFileMac is doing the wrong thing and is always 
resolving.  Do you want me to assign this to you as part as your performance 
work?
OK, I'll take this
Assignee: dougt → sfraser
Status: NEW → ASSIGNED
What happens here is this.Say the sample HTML file is at "Development:testing". 
The file transport tries to open an input stream for the stylesheet using the 
path ""Development:testing:", so nsILocalFile gives it back a spec for the  
directory. nsLocalFileSystem::GetInputStream() then tests that it is a directory, 
and indexifies it in that case. Indexing through the directory then causes those 
aliases to be resolved, which puts up the login dialogs.

I'm not sure where the fix should be. I think some higher-level code should check 
to see if they are using an empty string to fire off the stylesheet request. Or 
perhaps the file channel should know somewhere if it is dealing with a file or a 
directory, and test whether the file exists.

The fix for (1) above would appear to be to check for an empty aHref in 
HTMLContentSink::ProcessStyleLink().

(2) is a side-effect of nsLocalFileSystem (in nsFileTransport.cpp) assuming that 
if it gets passed a directory, then the caller wants a directory index (with MIME 
Type "application/http-index-format"). I've copied warren on this bug to see if 
this is correct. The alias resolution happens as result of the IsHidden() call in 
nsDirectoryIndexStream::Read().

I don't think we should change any behaviour in nsLocalFileMac for this bug.

Giving the bug to warren to fix, or comment on the directory indexing assumtions.
Assignee: sfraser → warren
Status: ASSIGNED → NEW
I was wondering if indexing a directory instead of reading a file could represent 
a security breach. We have seen several breaches in the past where nasty things 
could have happened if the intruder had had knowledge of the file names on the 
local volume.

Moving to M17 which is now considered part of beta2.
Target Milestone: M16 → M17
I'm not sure what what info you want here. Yes, we do create a directory index 
(with MIME type "application/http-index-format") when directories are passed to 
the file transport. No, it's not a security problem, because access to the file: 
protocol is restricted by the security system already.
Assignee: warren → sfraser
Reassign to rickg. HTMLContentSink::ProcessStyleLink() needs to be fixed to deal 
with empty HREFs.
Assignee: sfraser → rickg
Ok -- since this involves 1) the sink; and 2) the style system, I'll ask mark to 
address this.
Assignee: rickg → attinasi
Status: NEW → ASSIGNED
Looks easy enough...
Fix ready for review: checking for empty HREF before processing the link.
No longer processing style links if the HREF is empty. (nsHTMLContentSink.cpp)
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
For the record, an empty HREF attribute is in general perfectly valid, since an
empty URL is a valid relative URL (see section 5.2 of RFC 1808) that is exactly
equivalent to the base URL.  However, in this case (a rare one) it doesn't make
much sense, because the same document is unlikely to be both a stylesheet and a
document containing markup (unless the server is depending on Accept headers
that we probably don't send).
Interesting, David. Does that mean that an empty HREF where a relative URL is 
allowed should always cause the base-URL to be loaded? If so then I think we may 
have a bug in the way we resolve relative URL's. A link like <A HREF="">Test</A> 
does not cause the current document to be loaded, at least not when the document 
is loaded from a file.

Interestingly, I did see a comment in nsFormFrame.cpp acknowledging that:
    
    // Necko's MakeAbsoluteURI doesn't reuse the baseURL's rel path if it is
    // passed a zero length rel path.
http://lxr.mozilla.org/seamonkey/source/layout/html/forms/src/nsFormFrame.cpp#74
4

I agree that it does not matter for this bug, however if ther is a problem in 
the resolution of relative URLs we should probably fix it for the anchor case, 
no?
- Per last comments, age of bug, and no reopen - Marking Verified/Fixed.  Please 
reopen if still a problem.
Status: RESOLVED → VERIFIED
Tracy: the age of the bug and the fact that no one reopens it is certainly not a 
valid reason for marking a bug as verified. If that was the case, we could add a 
script in Bugzilla to automatically close the bugs after a certain time and get 
rid of the QA contact altogether (talk about job security...) Well, I checked and 
the bug is fixed.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: