Closed Bug 110508 Opened 23 years ago Closed 23 years ago

URL: doing SetSpec() with a string with a query doesn't do the right thing

Categories

(Core :: Networking, defect, P1)

Product:
Core
Component:
Networking
Platform:
x86
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.7

People

(Reporter: sspitzer, Assigned: darin.moz)

References

Details

(Keywords: testcase)

Attachments

(1 file, 2 obsolete files)

v1.0 patch
628 bytes, patch
sspitzer
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review 
doing SetSpec() with a string with a query doesn't do the right thing

call set spec with a valid url that has a query string.

calls to the url after that (like GetFilePath()) will return the filepath, plus 
the escaped query string.

this caused http://bugzilla.mozilla.org/show_bug.cgi?id=110478
*** Bug 109412 has been marked as a duplicate of this bug. ***
Copying my comment from bug 110478:

This is actually correct behaviour. ? isn't special in a file url. Before this,
it was impossible to open a file url containing a ? character.

This was bug 102603 (also see bug 59002). I mentioned it to darin, but I thought
he'd decided not to fix this the first time arround.

How did mailnews previously work with a local folder containing ? in the name?

The escaping is probably cosmetic. Note that spaces have been escaped, too. I'm
not sure whether that counts as correct behaviour or not, but it is correct that
the filepath includes the ?. File urls do not have a query term.
No longer blocks: 110502
Bradley, mailnews is not using file url but mailbox url for which we know how to 
deal with the query part. With the new url parser, we seems not able anymore to 
correctly differentiate the path part from the query part! When you query the 
url path, you get the query with it!
see my comments + patch:
http://bugzilla.mozilla.org/show_bug.cgi?id=110478#c31
Attached patch v1.0 patch — — Splinter Review 
query string should start with first '?' in the path, not the last :/
Severity: normal → critical
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.7
i'll also add a testcase to urlparse.dat for regression testing.
bbaetz, seth: can you guys review this patch?  thx!
*** Bug 110502 has been marked as a duplicate of this bug. ***
*** Bug 110582 has been marked as a duplicate of this bug. ***
bugs 110502 and 110582 are not dups of this bug.

the reason fwd inline and edit as new are failing is because IsValidScheme() is 
returning false when our spec has '_' in the scheme.

for example, here's the uri we're passing to CreateStartupUrl():

"mailbox_message://sspitzer@mail.meer.net/Inbox#9210143fetchCompleteMessage=true
"

I've got a fix, I'll attach it for darin and bbaetz to review.
hmm, looking at http://www.ietf.org/rfc/rfc2396.txt

scheme        = alpha *( alpha | digit | "+" | "-" | "." )

and http://www.w3.org/Addressing/rfc1738.txt

; the scheme is in lower case; interpreters should use case-ignore
scheme         = 1*[ lowalpha | digit | "+" | "-" | "." ]

'_' is not a legal character in a URI or a URL scheme.

I'll go look into fixing the mailnews schemes.



interesting... RFC2396 says:

scheme        = alpha *( alpha | digit | "+" | "-" | "." )

meaning that '_' isn't a valid element of an URI scheme.  portions of the old
code didn't even support '_' as an URI scheme (see
nsURLHelpers.cpp:ExtractURLScheme).

however, i can't think of any problems that would arise if we did allow '_' as
part of an URI scheme, so i think the patch is fine.

cc'ing andreas
I'll hold off on converting from "foo_message://" to "foo-message://" until the 
jury (bbaetz, andreas, darin) make a decision.
The spec says its bad, and I presume its just a couple of deinfes to change?

Whilst this isn't exposed externally, its probably not a bad idea to change the
scheme.
this will get edit as new, and fwd inline working again.
Attachment #58303 - Attachment is obsolete: true
Comment on attachment 58310 [details] [diff] [review]
change foo_message -> foo-message

obsolete, moved to bug #110502

let's let his bug just cover the original issue, so only darin's patch is
relevant.
Attachment #58310 - Attachment is obsolete: true
Comment on attachment 58190 [details] [diff] [review]
v1.0 patch

r=sspitzer on darin's patch.
Attachment #58190 - Flags: review+
*** Bug 110671 has been marked as a duplicate of this bug. ***
*** Bug 110706 has been marked as a duplicate of this bug. ***
Comment on attachment 58190 [details] [diff] [review]
v1.0 patch

bbaetz r='d this patch in bug 110478.
Attachment #58190 - Flags: superreview+
*** Bug 110721 has been marked as a duplicate of this bug. ***
Judging from the dupes, just because strings like this are invalid doesn't mean
that people don't rely on it....
One change between the old and new urlparser is that we are now checking the
scheme for vaild chars while parsing it. Previously we only did that while
resolving a relative url for example to figure out if we got an absolute or
relative url to resolve. I see no imminent danger in having _ allowed in schemes
but on the other side we have some bugs begging us to do more checks on urls, to
help deceiding which text in mail-messages to url-ify. I think, to help these
bugs we should stay with the check as it is now and change the schemes according
to the rfc.
This is so stupid, I can't even check my e-mail on netscape servers, so just fix
this thing a.s.a.p! 
Found another testcase; setting as URL.
(http://msdn.microsoft.com/downloads/default.asp?url=/downloads/sample.asp?url=/msdn-files/027/001/777/msdncompositedoc.xml)

Neil: Enthusiasm is appreciated, but pestering the Mozilla developers is not
going to get this fixed any faster.
URL: http://msdn.microsoft.com/downloads/d...
Keywords: testcase
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
*** Bug 110823 has been marked as a duplicate of this bug. ***
*** Bug 110926 has been marked as a duplicate of this bug. ***
Alex, it just seems that the patch wasn't tested, before it was added to the
mozilla trunk, and this wasn't the first time that happened, nor will it be the
last :)
*** Bug 110952 has been marked as a duplicate of this bug. ***
*** Bug 110834 has been marked as a duplicate of this bug. ***
*** Bug 111202 has been marked as a duplicate of this bug. ***
*** Bug 111277 has been marked as a duplicate of this bug. ***
*** Bug 112326 has been marked as a duplicate of this bug. ***
VERIFIED:
Mozilla 1.1, Mac OS X and Win 98.
Status: RESOLVED → VERIFIED
Summary: doing SetSpec() with a string with a query doesn't do the right thing → URL: doing SetSpec() with a string with a query doesn't do the right thing
Whiteboard: checklinux
VERIFIED: Mozilla 1.4/Linux 
used: /cgi-bin/printenv?query1?query2 on any apache server
URL: http://msdn.microsoft.com/downloads/d...
Whiteboard: checklinux
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: