Closed
Bug 290381
Opened 19 years ago
Closed 19 years ago
"Save Link As..." doubles the extension if the link contains an escaped hash/pound (#) sign encoded as %23
Categories
(Core Graveyard :: File Handling, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: troy, Assigned: bugs.caleb)
References
()
Details
(Keywords: fixed1.8, Whiteboard: fixed1.8 for toolkit)
Attachments
(4 files, 7 obsolete files)
269 bytes,
text/html
|
Details | |
1.49 KB,
patch
|
bugs.caleb
:
review+
neil
:
superreview+
asa
:
approval1.8b5+
|
Details | Diff | Splinter Review |
1.34 KB,
patch
|
mconnor
:
review+
|
Details | Diff | Splinter Review |
1.35 KB,
patch
|
mtschrep
:
approval1.8b5+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050317 Firefox/1.0.2 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050317 Firefox/1.0.2 There is a proof of concept at http://www.easynews.com/mozillabug/ . Right-click and save link as on the second link. The dialog shows a doubled extension on the jpg file as you're about to save it. Reproducible: Always Steps to Reproduce: 1. Create an HTML page with a %23 in a link target. 2. Do a Save Link As... on that link. Actual Results: Filename extension is doubled (.jpg.jpg), but can be manually corrected before saving. It seems that the extension may be being guessed based off the mime type since some part of the code may be dropping the part after the decoded # sign before validating the extension. Expected Results: Should have presented the link as-is and not doubled the extension.
Comment 1•19 years ago
|
||
confirming with win2k3 and Mozilla 20050414..
Assignee: bugs → file-handling
Status: UNCONFIRMED → NEW
Ever confirmed: true
Product: Firefox → Core
QA Contact: aebrahim-bmo → ian
Version: unspecified → Trunk
Also confirming w/ Mozilla 1.7.5 on XP Pro SP2. Where's my free gigs ?? :o)
Comment 3•19 years ago
|
||
also confirmed on linux kernel 2.6 kde 3.3 firefox 1.01 I could use some free gigs too as about a gig was wasted yesterday by a bug in pan :(
Comment 5•19 years ago
|
||
f the extention is being guessed off of the MIME type, try the proof of concept by naming an mpeg file as .jpg given the above example. If the sample tries to save as file.jpg.mpg or file.mpg.jpg, then that's probably the case
Comment 6•19 years ago
|
||
Can't open the same page in a new window either. Both Opera and IE do that.
Comment 7•19 years ago
|
||
Proposed fix for bug. Should work all platforms.
Comment 8•19 years ago
|
||
Owain, want to patch the non-Firefox version of that file too, since the bug's in Core?
(In reply to comment #7) > Created an attachment (id=181042) [edit] > patch to contentAreaUtils.js > > Proposed fix for bug. Should work all platforms. Can you please set review flags on your patch? The sooner it gets a review the more chances that it gets into 1.1!
Updated•19 years ago
|
Attachment #181042 -
Flags: review+
Comment 10•19 years ago
|
||
See comment 8. If people are insisting on making this a Firefox-only bug, please move it to the right product.
Assignee | ||
Comment 11•19 years ago
|
||
(In reply to comment #10) > See comment 8. If people are insisting on making this a Firefox-only bug, > please move it to the right product. OK. I suppose that it's better to fix the problem at the core rather to band-aid it in Firefox. But if no such solution is available in a timely manner, this bug will sneak into another release I suppose. Btw Owain, by setting the review flags, I meant requesting for review, and not setting it to + (unless, of course, you have the permissions to do so).
Comment 12•19 years ago
|
||
Caleb, I have no idea what you're talking about. Please read comment 8 and the patch again. It's the right approach; it just needs to be done in two places.
Assignee | ||
Comment 13•19 years ago
|
||
(In reply to comment #12) > Caleb, I have no idea what you're talking about. Please read comment 8 and the > patch again. It's the right approach; it just needs to be done in two places. Sorry, I've misunderstood :) So this patch can go as is and the bug can remain open for work to be done on the other part of it then ?
Attachment #181042 -
Flags: approval-aviary1.1a2?
Comment 14•19 years ago
|
||
Comment on attachment 181042 [details] [diff] [review] patch to contentAreaUtils.js he's not a reviewer! :P
Attachment #181042 -
Flags: review+
Updated•19 years ago
|
Attachment #181042 -
Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
Comment 15•19 years ago
|
||
IF the patch gets reviewed by someone competent who decides that they really don't care about keeping the two files in sync (which is probably about nobody, since we're trying to sync them up), yeah.
Comment 16•19 years ago
|
||
Mike: i might be ;-) Caleb: Sorry, i'm new to bugzilla and that was the only "review" option i could find. Boris/all: i'm quite willing to fix the mozilla trunk but it won't be until monday. Only reason i didn't was because i thought the moz equivalent was in xpfe C++ code and i don't have the build tools. So um, wait if you can.
Attachment #181042 -
Flags: approval-aviary1.1a2+
Assignee | ||
Comment 17•19 years ago
|
||
I'm requesting blocking for 1.8b4 since this bug could really piss off people downloading lots of images that contain these urlencoded chars. (Like, say, from a gallery)
Flags: blocking1.8b4?
Comment 18•19 years ago
|
||
same fix for mozilla suite.
Comment 19•19 years ago
|
||
Comment on attachment 188288 [details] [diff] [review] patch for mozilla trunk could you diff with more context, like 6 or 8? also, I'm not sure there's need to keep historical information like this in comments ("no longer use nsIURL") Does this do the right thing for "foo.exe.."?
Comment 20•19 years ago
|
||
Comment on attachment 188288 [details] [diff] [review] patch for mozilla trunk >+ var i = aFile.lastIndexOf("."); >+ if(i < 0) >+ return aFile; This isn't quite correct; we want to treat ".foo" as having no extension. >- if (url.fileExtension != aDefaultExtension) { >+ if (aFile.substr(i+1) != aDefaultExtension) { Nit: spaces around + > return aFile + "." + aDefaultExtension; I was trying to think of some way of doing "ends with aDefaultExtension" in JavaScript but the best I could come up with was aDefaultExtension = "." + aDefaultExtension; if (aFile.slice(1).slice(-aDefaultExtension.length) != aDefaultExtension) return aFile + aDefaultExtension;
Updated•19 years ago
|
Whiteboard: has patch, needs review → [no l10n impact] has patch, needs review
Updated•19 years ago
|
Attachment #188288 -
Flags: review?(mconnor)
Comment 21•19 years ago
|
||
Comment on attachment 188288 [details] [diff] [review] patch for mozilla trunk see Neil's comment.
Attachment #188288 -
Flags: review?(mconnor)
Assignee | ||
Comment 22•19 years ago
|
||
Here're a bunch of links you can test the upcoming patch on.
Assignee | ||
Comment 23•19 years ago
|
||
Updated patch to address Neil's comments.
Assignee: file-handling → bugs.caleb
Attachment #181042 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #190520 -
Flags: review?(mconnor)
Comment 24•19 years ago
|
||
Comment on attachment 190520 [details] [diff] [review] toolkit patch v1.0 >+ if(i <= 0 || i == aFile.length-1) Nit: space before ( and before and after - (although I wonder why you added that second test) >+ aDefaultExtension = "." + aDefaultExtension; >+ if (aFile.slice(1).slice(-aDefaultExtension.length) != aDefaultExtension) >+ return aFile + aDefaultExtension; I didn't mean you to make the change like this. Hopefully mconnor will decide it's unreadable anyway ;-)
Assignee | ||
Comment 25•19 years ago
|
||
(In reply to comment #24) > (From update of attachment 190520 [details] [diff] [review] [edit]) > >+ if(i <= 0 || i == aFile.length-1) > Nit: space before ( and before and after - > (although I wonder why you added that second test) I added the second test so that files such as "bleh." are also considered not to have an extension. If you check out the testcase, without the patch ".bleh" and "bleh." get appended with ".html", with this patch they don't get any extra extension at all. > >+ aDefaultExtension = "." + aDefaultExtension; > >+ if (aFile.slice(1).slice(-aDefaultExtension.length) != aDefaultExtension) > >+ return aFile + aDefaultExtension; > I didn't mean you to make the change like this. > Hopefully mconnor will decide it's unreadable anyway ;-) The previous "method" worked perfectly, and so does this one, I've only changed it because I thought you wanted it this way ;-)
Assignee | ||
Comment 26•19 years ago
|
||
Here's an updated toolkit patch.
Attachment #190520 -
Attachment is obsolete: true
Attachment #190527 -
Flags: review?(mconnor)
Attachment #190520 -
Flags: review?(mconnor)
Assignee | ||
Comment 27•19 years ago
|
||
Neil says he doesn't know (yet) whether he wants "blah." to mean "this file has
no extensions".
If you compare the testcase (attachment 190519 [details]) with and without the patch, you
get this:
With:
bleh.jpg -> bleh.jpg
bleh#3.jpg -> bleh#3.jpg
bleh. -> bleh.
.bleh - > .bleh
bleh.exe.. -> bleh.exe..
bleh -> bleh (this test isn't included in the testcase)
Without
bleh.jpg -> bleh.jpg
bleh#3.jpg -> bleh#3.jpg.jpg
bleh. -> bleh..htm
.bleh - > .bleh.htm
bleh.exe.. -> bleh.exe...htm
bleh -> bleh.htm (this test isn't included in the testcase)
Comment 28•19 years ago
|
||
(In reply to comment #27) >bleh.exe.. -> bleh.exe.. This is bad, because Windows ignores the trailing .s and saves it as bleh.exe so you need to preferably save bleh.exe.htm although I would accept bleh.exe...htm
Comment 29•19 years ago
|
||
the code should probably ignore all trailing spaces and dots on windows, maybe like: aFile = aFile.replace(/[ .]+/, "");
Attachment #190527 -
Attachment is obsolete: true
Attachment #190527 -
Flags: review?(mconnor)
Assignee | ||
Comment 30•19 years ago
|
||
This patch removes trailing spaces and dots on windows, and appends the default extension for invalid files. With (On win32, other platforms do different things): bleh.jpg -> bleh.jpg bleh#3.jpg -> bleh#3.jpg bleh. -> bleh .bleh - > .bleh.htm (Win32 Only - The only reason I append the default extension in this case is because ".filename" is invalid on win32) bleh.exe.. -> bleh.exe.htm bleh -> bleh (this test isn't included in the testcase) Without bleh.jpg -> bleh.jpg bleh#3.jpg -> bleh#3.jpg.jpg bleh. -> bleh..htm .bleh - > .bleh.htm bleh.exe.. -> bleh.exe...htm bleh -> bleh.htm (this test isn't included in the testcase)
Attachment #188288 -
Attachment is obsolete: true
Attachment #191038 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 31•19 years ago
|
||
Comment on attachment 191038 [details] [diff] [review] xpfe patch v1.2 >+#ifdef XP_WIN In XPFE you need to use if (/Win/.test(navigator.platform)) >+ if (i <= 0 || i == aFile.length - 1) >+ return aFile; >+#endif >+ if (aFile.substr(i + 1) != aDefaultExtension) Thinking about this, I'm not sure this is the right test. On all platforms I think you should use if (i <= 0 || aFile.substr(i + 1) != aDefaultExtension) return aFile + "." + aDefaultExtension;
Attachment #191038 -
Flags: review?(neil.parkwaycc.co.uk) → review-
Assignee | ||
Comment 32•19 years ago
|
||
(In reply to comment #31) > (From update of attachment 191038 [details] [diff] [review] [edit]) > >+ if (i <= 0 || i == aFile.length - 1) > >+ return aFile; > >+#endif > >+ if (aFile.substr(i + 1) != aDefaultExtension) > Thinking about this, I'm not sure this is the right test. On all platforms I > think you should use > if (i <= 0 || aFile.substr(i + 1) != aDefaultExtension) > return aFile + "." + aDefaultExtension; You're probably right. I tried to butcher the filename as least as possible, but it seems that "htm" is passed as the default file extension to the file picker when you Right Click > Save As (since we have no content-type/disposition), so even if I leave "bleh" as it is, it will still come out as "bleh.htm", and same goes for every other file that has an "invalid" extension. Does the same thing happen for Linux/etc..? Because I think it would suck to just append .htm to every file not ending with .htm.
Assignee | ||
Comment 33•19 years ago
|
||
OK Neil, this patch should do what you want (always append the default extension... just as it did before).
Attachment #191038 -
Attachment is obsolete: true
Attachment #191318 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 34•19 years ago
|
||
Comment on attachment 191318 [details] [diff] [review] xpfe patch v1.3 Nit: inconsitent bracing styles, your new block includes them but you removed them from the block you changed. r=me with that fixed.
Attachment #191318 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Assignee | ||
Comment 35•19 years ago
|
||
Nits picked, carrying over r+.
Attachment #191318 -
Attachment is obsolete: true
Attachment #191332 -
Flags: review+
Attachment #191332 -
Flags: approval1.8b4?
Assignee | ||
Comment 36•19 years ago
|
||
A toolkit version of the patch.
Attachment #191333 -
Flags: review?(mconnor)
Comment 37•19 years ago
|
||
Comment on attachment 191332 [details] [diff] [review] xpfe patch v1.4 You need sr= for anything inside xpfe/
Attachment #191332 -
Flags: approval1.8b4?
Attachment #191332 -
Flags: superreview?(cbiesinger)
Comment 38•19 years ago
|
||
Comment on attachment 191333 [details] [diff] [review] toolkit patch v1.4 >Index: contentAreaUtils.js >=================================================================== >+#ifdef XP_WIN >+ // Remove trailing dots and spaces on windows >+ if (/Win/.test(navigator.platform)) >+ aFile = aFile.replace(/[\s.]+$/, ""); >+#endif >+ No reason to check navigator.platform here...
Attachment #191333 -
Flags: review?(mconnor) → review-
Assignee | ||
Comment 39•19 years ago
|
||
I hate myself.
Attachment #191333 -
Attachment is obsolete: true
Attachment #191336 -
Flags: review?(mconnor)
Comment 40•19 years ago
|
||
Comment on attachment 191332 [details] [diff] [review] xpfe patch v1.4 sorry, I'm not a super-reviewer...
Attachment #191332 -
Flags: superreview?(cbiesinger)
Comment 41•19 years ago
|
||
Comment on attachment 191332 [details] [diff] [review] xpfe patch v1.4 however, the patch looks right to me, so r=biesi; maybe neil can upgrade his r to an sr ;)
Updated•19 years ago
|
Attachment #191332 -
Flags: superreview+
Assignee | ||
Comment 42•19 years ago
|
||
Comment on attachment 191332 [details] [diff] [review] xpfe patch v1.4 Thanks! Requesting approval for a simple fix.
Attachment #191332 -
Flags: approval1.8b4?
Updated•19 years ago
|
Attachment #191332 -
Flags: approval1.8b4? → approval1.8b4+
Updated•19 years ago
|
Flags: blocking1.8b4? → blocking1.8b4-
Comment 43•19 years ago
|
||
Comment on attachment 191336 [details] [diff] [review] toolkit patch v1.4.99 let's get this on trunk ASAP
Attachment #191336 -
Flags: review?(mconnor) → review+
Assignee | ||
Comment 44•19 years ago
|
||
Assignee | ||
Comment 45•19 years ago
|
||
Comment on attachment 196119 [details] [diff] [review] unbitrotted toolkit (trunk) can someome please check this patch in the trunk?
Updated•19 years ago
|
Whiteboard: [no l10n impact] has patch, needs review → [no l10n impact][checkin needed]
Comment 46•19 years ago
|
||
Trunk: mozilla/toolkit/content/contentAreaUtils.js; new revision: 1.78;
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: [no l10n impact][checkin needed] → [no l10n impact]
Target Milestone: --- → mozilla1.9alpha
Comment 47•19 years ago
|
||
Nobody seems to have checked attachment 191332 [details] [diff] [review] in anywhere (I guess its branch approval is now out of date?) so I checked it in on the trunk at least.
Updated•19 years ago
|
Attachment #191332 -
Flags: approval1.8b4+ → approval1.8b5?
Updated•19 years ago
|
Attachment #196119 -
Flags: approval1.8b5?
Updated•19 years ago
|
Flags: blocking1.8b5- → blocking1.8b5+
Comment 48•19 years ago
|
||
Comment on attachment 196119 [details] [diff] [review] unbitrotted toolkit (trunk) Approved for 1.8b5 per bug meeting
Attachment #196119 -
Flags: approval1.8b5? → approval1.8b5+
Comment 49•19 years ago
|
||
1.8 Branch: Checking in contentAreaUtils.js; /cvsroot/mozilla/toolkit/content/contentAreaUtils.js,v <-- contentAreaUtils.js new revision: 1.77.2.1; previous revision: 1.77 done
Whiteboard: [no l10n impact] → fixed1.8 for toolkit
Updated•19 years ago
|
Attachment #191332 -
Flags: approval1.8b5? → approval1.8b5+
Comment 50•19 years ago
|
||
...and attachment 191332 [details] [diff] [review] is now checked in on the branch too.
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•