Closed Bug 223815 Opened 21 years ago Closed 21 years ago

a coded character in "base href =" shortens the resulted link by two characters

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: peter.biebricher, Assigned: andreas.otte)

Details

(Keywords: fixed1.4.2)

Attachments

(2 files, 3 obsolete files)

this time it should work
3.02 KB, patch
bzbarsky
: review+
darin.moz
: superreview+
mkaply
: approval1.4.2+
Details | Diff | Splinter Review
Just for reference ... this is the 1.4 version of the patch
4.01 KB, patch
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; de-AT; rv:1.5) Gecko/20031007 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; de-AT; rv:1.5) Gecko/20031007 After <base href="file:///C:/mozilla/linking%2ehtml"> the link <a href="linked%2ehtml" target="_top"> linked page</a> evaluates to file:///C:/mozilllinking.html instead to file:///C:/mozilla/linking.html For each dot coded as %2e the base href is shortend by two characters. This happen also to http:-references. Reproducible: Always Steps to Reproduce: <base target="_self" href="http://booksrv.telekom.de:80/bookmgr-cgi/bookmgr.exe/BOOKS/CBCUG020/1%2e2%2e3"> with link <a href="1%2e2%2e4?SHELF=CBCBS020"><img src="/bookmgr/next.gif" border=0 alt="[Next Topic]"></a> Actual Results: http://booksrv.telekom.de/bookmgr-cgi/bookmgr.exe/BOOKS/CBCUG1.2.4?SHELF=CBCBS020 Expected Results: http://booksrv.telekom.de/bookmgr-cgi/bookmgr.exe/BOOKS/CBCUG020/1.2.4?SHELF=CBCBS020 In Mozilla 1.4 the base href is evaluated correctly.
The URL parser is screwing up the length of the mBasename segment (as well as the path, filepath, etc)... here's what nsStandardURL logging shows: 16384[80a1e50]: nsStandardURL::SetSpec [spec=http://mozilla.org/test/1%e23] 16384[80a1e50]: spec = http://mozilla.org/test/1%e23 16384[80a1e50]: port = -1 16384[80a1e50]: scheme = (0,4) 16384[80a1e50]: authority = (7,11) 16384[80a1e50]: username = (7,-1) 16384[80a1e50]: password = (7,-1) 16384[80a1e50]: hostname = (7,11) 16384[80a1e50]: path = (18,11) 16384[80a1e50]: filepath = (18,11) 16384[80a1e50]: directory = (18,6) 16384[80a1e50]: basename = (24,5) 16384[80a1e50]: extension = (24,-1) 16384[80a1e50]: param = (18,-1) 16384[80a1e50]: query = (18,-1) 16384[80a1e50]: ref = (18,-1) note that the basename length is listed as "5" when it should be "3" (and in fact what's stored in the buffer is "1.3" as far as I can tell). The other lengths are correspondingly off.
Assignee: parser → darin
Severity: normal → major
Status: UNCONFIRMED → NEW
Component: HTML: Parser → Networking
Ever confirmed: true
OS: Windows 2000 → All
QA Contact: benc
Hardware: PC → All
This seems to be related to the security fix I did after 1.4 unescaping %2e to . before doing the coalescing of the path ... investigating.
... or only indirectly. There is a CoalescePath in nsStandardURL.cpp which calls net_CoaleseDirs and then corrects the lengths. There are two instances where we call net_CoalesceDirs directly without correcting the lengths. Previously we did not unescape %2e to . during resolving relative paths, so this omission might have gone unnoticed.
-> andreas (thx for investigating this!)
Assignee: darin → andreas.otte
andreas: can you also add this URL to the set of tests in urlparse.dat? thanks!
Sure ... I already have a changed urlparse.dat in my build for ages now ... adapted to some changed behavior a long time ago. However I'm still building (much changes to catch up) ... then I will do some tests with the change.
I was wrong, this happens not because of the missing calls to CoalescePath but because of another already existing call to CoalescePath. The problem is in CoalescePath itself because it silently assumes the change happens in the directory part (which was previously correct) but with a %2e in the filebasename it happens in the basename. A solution might be to let net_CoalesceDirs only unescape %2e in the directory portion of the url. Any thoughts?
andreas: yeah, that does make good sense. we don't need to worry about .. in the filename anyways.
Hmmm ... I took a look and it will be really ugly to implement this in net_CoalesceDirs ... we simply can't look for the last / and stop there because the slash could be part of the query or the ref. In fact we first need to sort of parse the path.
Just a note - this is IBM BookManager that's instigating this, and makes V1.5 unusable in any environment where you depend on your browser to access IBM Manuals. This is a really serious problem.
Attachment #134261 - Attachment is obsolete: true
Comment on attachment 134612 [details] [diff] [review] Okay, this is an ugly patch that does the job rip it to pieces ...
Attachment #134612 - Flags: superreview?(darin)
Attachment #134612 - Flags: review?(bz-vacation)
Attached patch corrected a big ooops ... (obsolete) — — Splinter Review
Attachment #134612 - Attachment is obsolete: true
Attachment #134612 - Flags: superreview?(darin)
Attachment #134612 - Flags: review?(bz-vacation)
Attachment #134614 - Flags: superreview?(darin)
Attachment #134614 - Flags: review?(bz-vacation)
Comment on attachment 134614 [details] [diff] [review] corrected a big ooops ... forget it ...
Attachment #134614 - Attachment is obsolete: true
Attachment #134614 - Flags: superreview?(darin)
Attachment #134614 - Flags: review?(bz-vacation)
Attachment #134710 - Flags: superreview?(darin)
Attachment #134710 - Flags: review?(bz-vacation)
Comment on attachment 134710 [details] [diff] [review] this time it should work r=me on the mechanics (this will indeed unescape %2E only in the path). Darin's call on whether that's what we want to do...
Attachment #134710 - Flags: review?(bz-vacation) → review+
Comment on attachment 134710 [details] [diff] [review] this time it should work sr=darin
Attachment #134710 - Flags: superreview?(darin) → superreview+
patch checked in for 1.6 beta. thanks andreas!
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
We should consider backporting this to 1.4 and 1.5 ...
Comment on attachment 134710 [details] [diff] [review] this time it should work requesting approval to land this on the 1.4 branch for mozilla 1.4.2 andreas: i don't think there is much interest in future releases based off the 1.5 branch, but i could be wrong. there isn't a request flag for it at any rate :-/
Attachment #134710 - Flags: approval1.4.2?
Yes, right ... but there still is a tinderbox page around for 1.5 ...
Comment on attachment 134710 [details] [diff] [review] this time it should work a=mkaply for 1.4.2
Attachment #134710 - Flags: approval1.4.2? → approval1.4.2+
I will check this in into the 1.4 branch ...
modified patch checked in to the 1.4 branch
adding fixed1.4.2 keyword based on comment #26. thx andreas for checking in the patch!
Keywords: fixed1.4.2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: