Closed Bug 283730 (Shortcut) Opened 20 years ago Closed 19 years ago

"Save As" dialog tries to overwrite link/shortcut (.lnk) file instead of opening the directory/folder

Categories

(Core Graveyard :: File Handling, defect)

Product:
Core Graveyard
Component:
File Handling
Version:
1.7 Branch
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: super_egi, Assigned: emaijala+moz)

References

Details

(Keywords: fixed-aviary1.0.5, fixed1.7.9, regression)

Attachments

(2 files, 6 obsolete files)

Backout patch
680 bytes, patch
Details | Diff | Splinter Review
alternate version of YAAP
2.10 KB, patch
emaijala+moz
: review+
dougt
: superreview+
asa
: approval-aviary1.0.5+
asa
: approval1.7.9+
asa
: approval1.8b3+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.7.6) Gecko/20050224 Firefox/1.0.2 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.7.6) Gecko/20050224 Firefox/1.0.2 When I try to save a file and select a lnk file that links to a folder, it asks if it should overwrite the lnk file. The other versions of Firefox go to the folder instead. Reproducible: Always Steps to Reproduce: 1. File - Save file as... 2. double click a link to a folder Actual Results: It tried to overwrite the link and made it unusable. Expected Results: It should have gone to the folder instead of overwriting the link.
This is fallout from bug 271732.
Depends on: 271732
Keywords: regression
*** Bug 283780 has been marked as a duplicate of this bug. ***
*** Bug 283720 has been marked as a duplicate of this bug. ***
Version: unspecified → 1.0 Branch
*** Bug 283801 has been marked as a duplicate of this bug. ***
Summary: save file - folder link: tries to overwrite link → save file - folder link/shortcut: tries to overwrite link instead of opening the directory
Discussion of this bug on Mozillazine forums: http://forums.mozillazine.org/viewtopic.php?t=228991
Is this present on the trunk? If not, then I guess there's not a great chance of it being fixed. If it is, then this is a relatively annoying regression from 1.0 and I'm sure many would appreciate a fix for 1.0.2.
Flags: blocking-aviary1.1?
Yes, it is present in the latest trunk Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050306 Firefox/1.0+
Nominating in case there's some simple way to distinguish links that point at a file vs links that point at a folder. Not a Firefox bug, Core file handling
Assignee: bugs → file-handling
Component: Download Manager → File Handling
Flags: blocking-aviary1.0.2?
Product: Firefox → Core
QA Contact: aebrahim-bmo → ian
Version: 1.0 Branch → 1.7 Branch
*** Bug 285179 has been marked as a duplicate of this bug. ***
Summary: save file - folder link/shortcut: tries to overwrite link instead of opening the directory → save file - folder link/shortcut: tries to overwrite link (.lnk) instead of opening the directory
*** Bug 285280 has been marked as a duplicate of this bug. ***
*** Bug 285877 has been marked as a duplicate of this bug. ***
Why not just look at the target of the shortcut and then use GetFileAttributesEx()?
Nominating for 1.7.6 too, since it's busted there....
Flags: blocking1.8b2?
Flags: blocking1.7.6?
what platforms? This was fixed to fix a possible security exploit 271732. Between the two, i would rather have this bug than the other. Maybe it doesn't have to be a dichotomy.
I am of the exact opposite opinion. It seems like the other bug is a Windows issue and not a Mozilla/Firefox issue. I don't like having the usefulness of shortcuts removed on my platform because if you save a file two times in the same place, it might overwrite another file on *some* subversion of my platform. BTW - What happened to mozilla.org's position that Windows security bugs are Windows problems, and not the application's issue? See bugs related to storing email encrypted, etc.
Summary: save file - folder link/shortcut: tries to overwrite link (.lnk) instead of opening the directory → "Save As" dialog tries to overwrite link/shortcut (.lnk) file instead of opening the directory/folder
*local* security issues are windows problems. As a browser we're responsible for security issues we bring in from the outside world.
Does Firefox create the Save As dialog and do the navigation, or does Windows? Seems to me you can't have your cake (hand off the file handling to the Windows common dialog), and eat it too (take responsibility for what Windows then does with that file). At least try not to let people who do not use Windows on a daily basis be the ones to decide the fate of issues affecting Windows.
(In reply to comment #18) > Does Firefox create the Save As dialog and do the navigation, or does Windows? windows (via GetSaveFileName); i.e. firefox tells windows "show a filepicker and give me back a filename"
I ask because if the Windows file system chooses to overwrite the target of a link when a link is overwritten with another, then that is a Windows file system issue. Although it is noble to try and protect users from malicious people, there has to be a line somewhere. At some point everyone is going to have to realize that no matter what you do, people will do unwise things and there is nothing you can do to stop it. The sooner that is acknowledged, the better off we'll be. My personal opinion is that if you're going to click OK to download an unknown file two times, and then OK the overwrite of an existing file, then there is no protecting you anyway. Let's not break this functionality for everyone to try and save some poor souls who cannot be helped anyway.
*** Bug 286484 has been marked as a duplicate of this bug. ***
(In reply to comment #20) See nsFilePicker.cpp, about line 235: else if (mMode == modeSave) { ofn.Flags |= OFN_NOREADONLYRETURN | OFN_NODEREFERENCELINKS; result = nsToolkit::mGetSaveFileName(&ofn); "OFN_NODEREFERENCELINKS" !
or really see comment 1!
*** Bug 286801 has been marked as a duplicate of this bug. ***
*** Bug 286778 has been marked as a duplicate of this bug. ***
*** Bug 286928 has been marked as a duplicate of this bug. ***
Flags: blocking1.7.7?
I tested it with an older version and the IE and understand the decision. But couldn't it be made configurable? Unfortunately there seems to be no way on Windows for a better solution, or escaped anything my notice?
Is there a way to update certain files of Firefox so the security holes are fixed, but this bug is gone? I'm not going to download a pif-file anyway, so i'd rather have <a href="show_bug.cgi?id=271732" title="VERIFIED FIXED - COMMAND.COM is overwritten by downloading the pif file">bug 271732</a>.
Not only do you have to download a PIF file, but you have to do it on Windows ME, you have to overwrite another PIF file with the same target, and you have to click OK on a dialog box that asks you if you want to overwrite command.com. Keep that in mind when assessing the security risk of the "bug" that's fix is responsible for this bug.
You're misrepresenting the situation - it's true that for pif files the issue only happens with certain platforms, but for lnk files (which is the issue for this bug anyway) the problem exists in all Windows versions. Nobody is arguing that the risk isn't low - it says "Risk: Low" right at the top of the security advisory. If something is going to get fixed here, someone needs to come up with a solution that doesn't involve reintroducing that low risk - ranting about it isn't going to get anything done, and bugzilla certainly isn't the place to discuss where to draw the line between usability and protecting users from themselves.
The risk isn't any more than the risk of a user selecting an existing system file directly to overwrite. Why not just disallow replacing any files at all? I'm being facetious of course. The reason for posting arguments against (or for) a bug in the bug are for other people that wonder in later. They will not be privy to expired discussions on newsgroups or at MozillaZine.
> expired discussions on newsgroups news.mozilla.org does not expire anything
*** Bug 287614 has been marked as a duplicate of this bug. ***
*** Bug 287717 has been marked as a duplicate of this bug. ***
To fix bug #271732, something else was broken. This worked with Mozilla 1.7.5 and does not work with Mozilla 1.7.6. Thus, this bug is a regression and should have a priority higher than Normal. I have an extensive directory structure spread over two physical hard drives. To simplify navigation, I make extensive use of shortcuts. When I go to download a file, I can usually reach the preferred destination for the downloaded file by selecting a shortcut from wherever I am. This no longer works. Instead, I must first try to remember on which drive the destination resides and then navigate downward from there. What a *#@#!! bother!
*** Bug 279070 has been marked as a duplicate of this bug. ***
(In reply to comment #35) ... > I have an extensive directory structure spread over two physical hard drives. > To simplify navigation, I make extensive use of shortcuts. When I go to > download a file, I can usually reach the preferred destination for the > downloaded file by selecting a shortcut from wherever I am. This no longer > works. Instead, I must first try to remember on which drive the destination > resides and then navigate downward from there. What a *#@#!! bother! ... I strongly agree! Why not give users who are willing to take responsibility for their own security, a choice (in about:config maybe? where most windows users never look) so we are able to take action, and make links work like links rather than files.
I have an alternative way to fix bug 271732 that wouldn't cause this bug. I suggest we back out the fix for bug 271732, and do the following instead: Before opening a save dialog, we look at the default file path. If it is a shortcut filename (pif, lnk, url), we use an empty default filename instead, requiring the user to select a filename. This would prevent users from accidentally saving shortcuts.
(In reply to comment #38) I realy don't care if I save a file as an shortcut. The bigest problem is that you can't use the shortcuts to navigate.
Not blocking 1.7.7 or 1.0.3 security releases
Flags: blocking1.7.7?
Flags: blocking1.7.7-
Flags: blocking1.7.6?
Flags: blocking-aviary1.0.3?
Flags: blocking-aviary1.0.3-
bug 288769 is a duplicate of this bug but I don't can't seem to find a way how I can change this in the other bug report. Is this bug being worked on? It's quite annoying and simply a wrong behaviour which should be fixed.
*** Bug 288769 has been marked as a duplicate of this bug. ***
This is totally infuriating and needs to be fixed ASAP! Sure it isn't security, but it's such a fundamental behavior of all GUIs that it surely will make the list of "reasons Firefox is not ready for prime time." Strangely, I don't think I have this problem with the Download dialog; is there some reason they should work differently??
Attached patch Potential Fix. (obsolete) — — Splinter Review
Dan, what do you think? Maybe we should throw a dialog if this happens? Right now we just silently fail which might be okay since this case isn't going to be hit very often.
Assignee: file-handling → dougt
Flags: blocking1.7.8?
Flags: blocking-aviary1.0.4?
*** Bug 289739 has been marked as a duplicate of this bug. ***
I hate to just add to the noise complaining about this bug, because I can't really offer a programming solution, but since the severity is still only listed as normal, I think perhaps another complaint is not out of order. It seems like poor judgement was excercised in fixing what seemed to be a small minor potential security issue by seriously disabling a GUI feature that every other program written manages to use without problems. I can only vote for this bug once, but I just want to make sure the powers that be who assign priority to these things are paying attention. This is an absolute make or break issue for me and I'm sure for a lot of people. I will be reinstalling Thunderbird 1.0 because this bug is such a major inconvenience, and if I didn't still have the old installer, I'd switch to any other email program, OE included, without hesitation. Okay...I'm done ranting. Thanks to everyone who actually contributes code. I was really happy with both Firefox and Thunderbird before this bug cropped up.
Comment on attachment 179887 [details] [diff] [review] Potential Fix. You should check that length > 4 and do a case-insensitive comparison. Could you also use wchar_t instead of unsigned short? The idea is excellent and the current situation is unbearable.
Just a user here, cant offer much expertise, but I do have a strange factoid to report. Mostly I use 2 computers, one with WinXP home and WinXp Pro on the other (FF 1.0.2- showing this bug). Also, I have an Win2K machine that is used mostly as redundant storage. I am not big on Betas, but because this mis-treatment of links is so irritating, I went to the W2K box and desperately tried one of the FF 1.0.3 candidates I found pointers to on the Mozillazine forum (cant find the link right now, no matter). Anyway, I tried it for a few hours, and THE .LNK FLAW WAS FIXED! Nothing else seemed broken, so I installed the same FF 1.0.3 on an XP machine--THE .LNK FLAW WAS BACK! Now curious, I put FF 1.0.2 on an NT machine. It has the LNK flaw, sort-of. It gives no warning, just inserts the clicked folder link in the filename field. But if you follow through, (open or save, I forget which) it does not save the document with "xxx.lnk" as the file name, instead, it opens the "xxx" folder as the new save-location! IE 6 has the same behavior as FF in the NT enviroment. In WinXP if the overwrite warning is ignored, the contents of "xxx.lnk" is replaced by the file being saved. So, here I have 3 different folder-shortcut treatments in in 3 different windows versions, with the same FF. Hate to be long winded, but this behavior is hugely irritating, just thought that the Windows-version-dependent results might help track down the cause.
UNCA MICK comments about a new behavior of this bug. I have another program that has this new behavior. It is ImageFox from ACD Systems. This program has a "favourite folders" feature. It adds several new buttons to the open/save windows. When you click the favourites button, it takes you to a folder managed by ImageFox that contains links to other folders, so you can go directly to the folder you want. The bug is when you click on this button, you loose the filename you're trying to save and it gets replaced by the location of this favorite folder. This happens with ANY program and in both the open and save windows (but it really has no relevance in the open windows). the curious thing about this is that if you are using, lets say, Photoshop and you click on a link at the save window, it takes you to the refered folder as expected, but if you click on the ImageFox favourites button, the described bad behavior happens. So, the point to note here is that a feature of this add-in, when activated, causes a correctly working open/save window to behave incorrectly. It only happens in Windows XP (Both Home and Pro) so I think it could be somewhat related with the bug we're discussing here and so give any clue about its solution.
(In reply to comment #50) The bug is still in Firefox 1.03 beta
The bug will be in every version of Firefox henceforth until this bug is marked as fixed. There is no reason to waste time downloading new builds just to see if they secretly fixed it.
I got this same problem when I "upgraded" to 1.0.1 and then 1.0.2, using Win XP sp1 - really rather annoying since it (still) works as intended in IE - In my book this BREAKS a major functionality in Firefox (saving/downloading pages). So, no more recommending it to clients or coworkers for a while.
I experienced the bug using Mozilla 1.7.6 it's not in 1.7.5 so I revereted back to that build.
*** Bug 289900 has been marked as a duplicate of this bug. ***
What's the status on this bug? It seems pretty major to me... must block for 1.1!
*** Bug 290447 has been marked as a duplicate of this bug. ***
I can not re-create this issue in Firefox 1.0.3 on windows 2000 SP 4. It traverses all symlinks perfectly fine. and as far as I cam remember it always has traversed symlinks as I always have a set of symlinks on the desktop going to network shares (mapped and non-mapped) and vm host-guest paths on the system. (Vmware install of win2k). Could this be a Windows XP specific bug?
Are you talking about "junctions"? They are the only thing that could be considered "symlinks" on Windows 2000. WIndows XP has hard links, but I don't believe these exist in WIndows 2000. Junctions and hard links will always work perfectly fine because they are at the file system level. Shortcuts are simply files that contain a path in them. When you double-click a shortcut, Windows goes to the path that is listed in the LNK file.
I can verify that 1.0.3 still has the bug. I agree with Caleb that it is quite major - breaking a fundamental ability to navigate in Windows. As Jerry Baker says, Windows shortcuts are files with type .lnk and a path content - they are not fs-internal unix-style symbolic links, or Microsoft network shares. Since 1.01, Firefox has decided to write over these rather than traverse them. From some very confused-sounding talk on the related bug report, some people think that this is a smart way to work around a very obscure security issue which actually requires that you write over links. Please, please fix it - the way it is makes it very awkward to save web pages to different places in a knowledge base. It also makes Firefox look like it doesn't work in Windows to anyone who is used to saving webpages.
Attached file Firefox 1.0.3 patch (obsolete) —
This patch will change the second hax entry 10 00 8D 85 to 00 00 8D 85 and so i hope it will fix this bug
What do i do with those attachments in order to install them?
Will this patch work for Mozilla 1.7.6? Because the bug is there too.
Will the patch work with 1.0.2 spanish version? There isn't still a spanish 1.0.3 version. And, in case the spanish version 1.0.3 is released, would the patch work with it or is just for the english version?
Comment on attachment 180936 [details] Firefox 1.0.3 patch this is a binary patch, not a code patch, so correcting type and flag
Attachment #180936 - Attachment is patch: false
Attachment #180936 - Attachment mime type: text/plain → application/octet-stream
This patch worked fine for me, thanks a lot, bigdady
What does the patch actually *do*? Do you have source code? Does it just undo the fix for bug 271732?
Attachment #180936 - Attachment description: This patch will fix the bug (i hope) → Firefox 1.0.3 patch
Attachment #180936 - Attachment is patch: true
Attachment #180936 - Attachment mime type: application/octet-stream → text/plain
Attachment #180936 - Attachment is patch: false
Attachment #180936 - Attachment mime type: text/plain → application/octet-stream
(In reply to comment #67) This patch does a little bit haxing, like i said it changes the second hax entry 10 00 8D 85 to 00 00 8D 85. If you want to know more http://forums.mozillazine.org/viewtopic.php?p=1288710#1288710
(In reply to comment #68) > (In reply to comment #67) > > This patch does a little bit haxing, like i said it changes the second hax entry > 10 00 8D 85 to 00 00 8D 85. If you want to know more > http://forums.mozillazine.org/viewtopic.php?p=1288710#1288710 Based on my interpretation of the post in the thread, your patch simply reintroduces the security hole. Is that correct?
(In reply to comment #69) Truly I don't know, I just made a patch using the manual in the forum so... I belive that it doesn't reintroduce the security hole, but ain't sure.
If you view the fact that files can be overwritten by clicking on a dialog box that asks you if you want to overwrite a file a security hole, then yes, this patch reintroduces the "you can overwrite a file by telling the computer you want to overwrite the file" security bug.
(In reply to comment #60) > I can verify that 1.0.3 still has the bug. I agree with Caleb that it is quite > major - breaking a fundamental ability to navigate in Windows. How can such an obvious bug - which seems to require a simple change to a flag to fix it - be permitted to propagate through 3 versions of Firefox (1.0.1, 1.0.2, 1.0.3)? This is starting to make Internet Explorer look good by comparison. Could we at least see the status move from "new" to "assigned"?? This is a bug that EVERY Windows user will hit if they use shortcuts to navigate their directory trees. It needs to be fixed - and it is not just a cosmetic bug.
Agreed. Can't we change the severity to Major or Critial? This should have been fixed long ago.
Attached patch patch v.2 (obsolete) — — Splinter Review
including ere's comments.
Attachment #179887 - Attachment is obsolete: true
(In reply to comment #74) > Created an attachment (id=181139) [edit] > patch v.2 > > including ere's comments. A: Does this patch ("patch v.2") fix the regression so it won't be in 1.0.4 (or even currenly in nightlies) ?? B: Is the .exe ("Firefox 1.0.3") patch safe to run? C: what does "ere's" mean? D: Is there really a point to making patches for this regression, or should the people who "fixed" Bug 271732 just make a less sloppy bugfix for their "bug"?
(In reply to comment #75) > C: what does "ere's" mean? It's the genitive case of "Ere" > D: Is there really a point to making patches for this regression, or should the > people who "fixed" Bug 271732 just make a less sloppy bugfix for their "bug"? I would really advise you to compare the authors of the patches for this bug and that bug.
Comment on attachment 180936 [details] Firefox 1.0.3 patch Obsoleting so as not to imply that downloading and running random executables from bugzilla attachments is a good idea.
Attachment #180936 - Attachment is obsolete: true
(In reply to comment #74) > Created an attachment (id=181139) [edit] > patch v.2 > > including ere's comments. Hmm, Doug you did not seem to use any review flags on the patch ....
(In reply to comment #76) > It's the genitive case of "Ere" then what is an / who is "ere"? (http://www.google.com/search?q=define:ere wasn't much help) > I would really advise you to compare the authors > of the patches for this bug and that bug. Well, then my question still stands -- shouldn't the other bug's patch be updated so that this regression is fixed, AND the other bug gets fixed? It seem (to me) that this bug is an error in the implementation of the other bug's patch, therefore the fix should occur on that bug's patch instead. In fact, this bug (regression) is probably invalid if you look at it that way. We really just want a more sophisticated solution to the other bug, which still allows users to use their .lnk shortcuts which point to folders.
(In reply to comment #79) > (In reply to comment #76) > > It's the genitive case of "Ere" > then what is an / who is "ere"? > (http://www.google.com/search?q=define:ere wasn't much help) See comment 47.
I can't believe the noise in this bug. Please read the entire thing before commenting!
(In reply to comment #81) > I can't believe the noise in this bug. Please read the entire thing before > commenting! I did.
On second though, my answer of "I did." is not complete enough. I brought up some really good points to think about. It is apparent that this bug is a big deal, and it's unbelievable to me that it even exists. I asked my questions (A-D) in order to be able to more fully explain this bug over in the mozillazine forums. I wanted clarification on things that confused me, because I figured that the other 80 people who want this bug fixed would appreciate the clarification. I build websites -- that's *my* interest in Mozilla. I do not code, so I cannot offer that -- what I can offer is to increase communication. Writing my questions and clarifications off as "noise" I didn't initially, but now do take as an insult. While I appreciate your effort in coding on this project, your comment I found to be quite rude. I understand the problem of "bugspam", and write every comment focused on the question "is this post even worth reading?" -- I apologize if *you* found them to have zero value. The implementation of the other bug's fix *was not* completely thought out (as is apparent by the results of the patch). Adding more *thinking* to this bug in in order.
(In reply to comment #79) > It seem (to me) that this bug is an error in the implementation of the other > bug's patch, therefore the fix should occur on that bug's patch instead. In > fact, this bug (regression) is probably invalid if you look at it that way. This is normally how we handle regressions: leave the old bug closed and track the regression separately. The exception is when we immediately back out the original fix, but that's not what we've done here.
Comment on attachment 181139 [details] [diff] [review] patch v.2 r=dveditz
Attachment #181139 - Flags: review+
(In reply to comment #83) > On second though, my answer of "I did." is not complete enough. And apparently inaccurate since you didn't notice Ere's comment, even when you had a question about "including Ere's comments" which should have prompted you to look for them. > I brought up some really good points to think about. Not really. A: yes, obviously we think it fixes the regression. That's why it's a patch. B: Of course it's not "safe", it's a binary executable from some random bugzilla commenter. Only you know what your risk tolerance is, and judging the poster's reliability is a personal judgement. C: failure to read in context noted above D: Does it matter *where* the patches are attached? Would it even make sense to do it elsewhere since there are 100 people watching for the fix *here*? > Writing my questions and clarifications off as "noise" [...] > I found to be quite rude. Your comments were neither about the problem nor the patch, they were procedural nitpicks at best. And, frankly, you set the tone by insulting the programmer. > Adding more *thinking* to this bug is in order. Would love to see some.
(In reply to comment #86) > D: Does it matter *where* the patches are attached? Would it even make sense to > do it elsewhere since there are 100 people watching for the fix *here*? No, where it is attached doesn't matter. The development philosophy behind checking things in that break other things and then chasing down all the symptoms rather than addressing the single root cause is questionable at best. Why not just test the checkin first? If it's broke, fix it first. > nitpicks at best. And, frankly, you set the tone by insulting the programmer. If saying that a patch was not well thought out is insulting, I would suggest that the best way to avoid such insult is not to check in patches that aren't well thought out.
this use case was considered when that patch was created, see bug 271732 comment 34
Assignee: dougt → nobody
Comment on attachment 181139 [details] [diff] [review] patch v.2 Ere, I made the changes that you requested. I think this patch is good-to-go. Note that if you do get into the case of download a shortcut twice, the second time will fail without any user feedback. I think this is okay since (a) this case is unlikely, (b) we prevent the serious security hole cited above, and (c) getting a UI change now is going to be difficult.
Attachment #181139 - Flags: superreview?(emaijala)
(In reply to comment #86) > (In reply to comment #83) > > On second though, my answer of "I did." is not complete enough. > > ... you didn't notice Ere's comment, even when you > had a question about "including Ere's comments" which should have prompted you > to look for them. my response (and all future ones) in the mozillazine thread
(In reply to comment #88) > this use case was considered when that patch was created, see bug 271732 comment 34 Yes it was, and then it was dismissed with the comment, "You can still double-click on shortcuts to folders in the dialog" (bug 271732 comment 35). After that, nobody bothered testing it, or if they did they did not post the results. That's what is disconcerting about this whole thing: checkins can be made that break the fundamental features of an OS and not only is it not backed out, but it takes a month of bickering to even have it addressed at all. There *really* needs to be a prohibitions on platform-specific checkins by people who do not use that platform as their primary work environment, because they do not seem to understand the ramifications.
jerry, the position was that it was better to break some functionality to protect the user against a security hole. We fixed it quick to make the release, and now we are looking at better solutions.
That what is represented in that bug is even a security hole is up for debate. I have been seriously contemplating filing another security bug that it is possible to browse to any given folder and overwrite another file simply by answering OK to one dialog asking you if you really want to overwrite that file. ... even files in the system folder :::GASP:::. I don't see how having to OK a dialog asking you to overwrite a file represents any sort of security hole. I think it represents a hole in something, but not security.
jerry, that is a nice thought and all, but it really doesn't help this bug, does it?
I was making the point that none of what anyone says was addressed was actually addressed. It was mentioned and then summarily ignored, but not addressed. The fix that caused this regression is a fix for a non-issue in the first place (that was the point of my facetious bug about being able to overwrite any file). I can't think of any place more relevant to point that out than in a bug caused by the fallout from that fix.
Just to clarify -- the security hole which we thought was important enough to break this function was that you can construct a file such that if you download it twice in the same place, the second download could replace _any_ file on your system. The confirmation dialog would suggest that you were replacing one file, but in fact, it would relace another. The way this bug was reported sounds like a regression cause by our fix for the security hole. I found that there was an inconsistent way that windows handled OFN_NODEREFERENCELINKS. I hope this is clear. if you would like to talk about bug fixing philosophy, security bug considerations, or anything that doesn't directly influence this patch, please move your discussion to the mozillaZine formus or to the public newsgroups. Regards, Doug
You are incorrect. The dialog specified the file that is *actually* replaced. The whole thing is a non-issue. And yes, I'm well aware of mozilla.org's position that you should take relevant discussions somewhere where the developers and drivers can more easily ignore it.
No Jerry.... you are incorrect. The security hole specied one file and replace aonther. Please do not lead yourself to believe that developers don't want feedback... They want feedback, just not incorrect, loud, or inconsiderate feedback.
Is the patch correctly? I think that |ofn.lpstrFile| has reference path of shortcut file. It doesn't have shortcut file path. So, I think that |ofn.lpstrFile| doesn't have ".lnk", ".pif" and ".url" always.
*** Bug 291579 has been marked as a duplicate of this bug. ***
Comment on attachment 181139 [details] [diff] [review] patch v.2 I'm not a super-reviewer, but let's just pretend dveditz did this and vice versa.
Attachment #181139 - Flags: superreview?(emaijala) → superreview+
Masayuki, the problem we are trying to fix is that if a user overwrites an existing shortcut, the overwrite will incorrectly dereference the link and destroy the target of that shortcut. In this case, |ofn.lpstrFile| will have one of the extensions we are looking for. Please confirm.
(In reply to comment #102) > Masayuki, the problem we are trying to fix is that if a user overwrites an > existing shortcut, the overwrite will incorrectly dereference the link and > destroy the target of that shortcut. In this case, |ofn.lpstrFile| will have > one of the extensions we are looking for. Please confirm. That is incorrect. If you select an existing shortcut, ofn.lpstrFile will have the path of the referenced file (not the shortcut). However, this patch will prevent the user from creating the shortcut in the first place (when the shortcut doesn't already exist).
yoni, go read the security bug and attempt to reproduce it.
Jerry -- Dan Veditz confirmed what you are seeing, so maybe we both are correct. He used XP. I think that an issue is that the API is inconsistent between XP and other system such as SE.
If an website shows following step, we cannot fix bug 271732. 1. Download an zip file that is include shortcut file(e.g., "foo.lnk"). 2. Unzip the zip file. 3. The zip file has old version file. Please download "foo.lnk" and overwrite it. Current patch cannot stop to download both shortcut file.
That is correct. You cannot write anything in C, C++, JavaScript, or any other code that I am aware of that will alter the behavior of people. There are just some things that are beyond the control of the programmer.
(Partially in reply to comment #99) I have verified that “patch v.2” is NOT correct. On Windows XP, when the user selects a link to a file in the file save dialog, the windows returns the file pointed to by the link. There is no way for the caller to determine that the file was selected by selecting a link, and not the original file. Therefore, the code in the patch designed to catch this will never trigger. While I do see the potential danger of the original “security hole”, I firmly believe that it is time to write it off, and simply return to the old behavior. This “security hole” exists in every windows application (including IE). There is no reasonable solution (One could, I suppose, do some really hairy stuff, involving overriding the standard windows Save dialog, but even then the solution would be partial at best, and very bug prone). Not being able to use links for navigation (as it stands now) is clearly unacceptable. Speaking as a professional developer with over 10 years windows programming experience, I can tell you that this ‘small’ issue will eventually lead to significant numbers of users abandoning this software. Remove the OFN_NODEREFERENCELINKS flag now.
dan -- your call.
Assignee: nobody → dveditz
We have evangelism for Web pages, but is there evangelism for operating systems? That seems to me to be the appropriate place for the bug that caused this regression.
If the patch doesn't work for you, open firefox.exe with a hex editor (for example http://www.chmaas.handshake.de/delphi/freeware/xvi32/xvi32.htm ) and search for "4D 80 00 80 10 00 8D 85", then change it to "4D 80 00 80 00 00 8D 85". It's not me who found that out, i got it from http://forums.mozillazine.org/viewtopic.php?p=1387539#1387539 I have no idea how to publish patches here, so all i can do is write this.
Is it feasible to compare the directory that is currently open in the "Save As ..." dialog with the path returned by the dialog? That way, if they do not match, we could refuse to write. At least this would cover all cases except for where the file is in the same directory as the one in which you are saving, but then that's not much different from directly overwriting the file.
it could just be the case that we claim this inconsistency is a windows bug and security holes based on this bug are Microsoft's fault. Afterall, if I overwrite a shortcut twice, i do not expect the destination/target to ever be touched.
If it's relatively easy to check the current directory and compare it with the path of the filename we are trying to save, I think it would be an ideal solution. I can't think of a case where you would be saving a file to a different directory than the currently displayed directory EXCEPT when the security hole is in effect. The one caveat would be to make sure that the comparison and PASS FAIL does not occur until the Save button is pressed, or else shortcut navigation might break.
Just make sure I can type in a path into the save as dialog and hit save without having to navigate to that directory...
I have to say that I had never thought of doing that, nor have I seen it done in all my years in IT. It always seemed much faster to do two clicks to the root drive than it was to even type C:\. I suspect that what you want to do would not work were my suggestion to be implemented.
So, if you don't mind being a bit hackish, ignore the check if the filename box contains a backslash. It's an invalid character in filenames on Windows, and so it either has to be a directory or an invalid filename. In the former case, we can bypass the check because I don't think the LNK/PIF file thing will prefill the filename box. The latter case would be handled by whatever error checking Windows usually does on the filename.
What I had in mind is something like: if(TARGET_DIR != CURRENT_DIR) { if(strstr(TEXTBOX_VALUE,"\") /* Save */ else /* Fail */ } else /* Save */
A lot of speculation, but little actual information. (In reply to comment #112) and (In reply to comment #119) et al. Their strategy is to compare the current directory (once the save dialog has closed), to the file path of the file selected. This will not work. The directory is set to the folder containing the item pointed to the link, not the folder the user was in when they selected the link. I have verified this with a test program (on Windows XP). On the other hand, comment #113 is on the right path. IMHO the only way to do this will be to install a hook in the SaveDialog, then it is relatively straightforward to catch the CDN_FILEOK message. At that point CDM_GETSPEC will return the actual name of the file that the user has selected. This will be the link, and the extension can be checked for “.lnk” etc, as in patch v.2. Note that this all takes place in the hook procedure. I have verified that this works correctly with a small test program (again on Windows XP.) To implement this will require a reasonably experienced Windows API developer, and a lot of care. It is more than just a 3 line fix. This will also need to be checked extensively on all Windows platforms, since this is one area (File dialog hooks and overrides) that can vary between versions of Windows.(I am unfortunately, not able to take on this task at this time.) However, I stick by my original statement. The “security hole” exists in every Windows application that I know of. If no-one can be found to fix this reliably and immediately, I think the correct course would be to simply remove the OFN_NODEREFERENCELINKS and live with the minor “security hole” for now. You will loose users over not being able to follow links.
*** Bug 292277 has been marked as a duplicate of this bug. ***
Oh my god this problem is driving me crazy! This WASN't happening in Mozilla 1.7 and now I'm tempted to revert back just so this failure of .lnk resolution doesn't continue to make me want to kill myself.
Supporting comment #120: It isn't necessary to make Mozilla/Firefox the only application "fixing" this Windows bug and pay for this with a very annoying loss of usability. In reply to comment #116: Of course it is possible to enter a qualified file name, including the path; I just tried it. But this is not the same, since it requires a lot of work of the user [s]he tried to get rid off creating the symlink. I can't believe this bug is still marked as NEW. Please fix this bug for the next release. Whoever feels the security threat as so important that fixing it is worth a big usability loss, could decide so out of his own free will by setting a non-default setting; but in my opinion, it would be sufficient to return to the pre-"fix" state.
Ok, so what if the "fix" is coded in but such that it requires a setting in the prefs.js to be entered by hand ? That way the "security issue" is "fixed" out of the box by default, but a quick hand edit of the prefs.js could allow that to be changed. I think that sounds like the best of both worlds.
I shouldn't have to fix the browser after I install it.
How about this: Check the default file name extension and if it's .lnk, .url or .pif, check that the file name the save dialog returned has the same extension. If not, and a file with that name already exists, warn the user or fail. In pseudocode: if (extensionOf(mDefault) == '.pif' || '.url' || '.pif') && (extensionOf(ofn.lpstrFile) != extensionOf(mDefault)) && (fileExists(ofn.lpstrFile)) return NS_ERROR_FAILURE;
(In reply to comment #125) > I shouldn't have to fix the browser after I install it. You'd rather it stay broken with no option to fix? This bug has persisted through two version upgrades. They're not just going to revert to the original "unsecure" handling of links from earlier versions. If an optional setting that isn't selected by default is considered safer simply because one has to be aware enough of what it does to enable it and therefore more palateable, then by all means I'm all for it. As I understand it, the original security flaw isn't a problem if you know not to download .pif files.
I think Ian meant that it should be fixed in a way that doesn't require changing a pref. I like the suggestion in comment 126. It's limited to PIF, LNK, and URL files, and it is the most narrow way I can think of at the moment to stop the security bug while allowing the usefulness of shortcuts.
How about just a "Download File Blacklist" where you'd get a warning every time you try to download files of certain types (.lnk, .pif, ect.). It could have certain files in the list by default, and users could add or remove them if they want.
Attached patch Alternate patch v1 (obsolete) — — Splinter Review
Dan, what do you think about this approach?
Attachment #182604 - Flags: review?(dveditz)
You need to put the warning string into a DTD file so that it can be translated to the various languages Mozilla/Firefox is released under. I'm not sure which is the correct DTD file for errors from the file handler.
I know, but this is sort of a minimal patch. Translatability can be added later.
I agree the Ere's approach. I think that the security hole is opened again by the patch. But the hole is very small. If we can get last opened folder in save dialog, we can *close* the hole... # the folder and the result path is not equal, # the user selected the shortcut file.
*** Bug 292846 has been marked as a duplicate of this bug. ***
Blocks: majorbugs
Attachment #182604 - Flags: superreview?(dveditz)
Attachment #182604 - Flags: review?(dveditz)
Attachment #182604 - Flags: review?(dougt)
I send out a mail to the security list and no one seam to object about backing out the fix completely. This was the text of the email: *271732* COMMAND.COM is overwritten by downloading the pif file We are considering backing out a patch that prevented the arbitrary overwriting of any file on the users systems. An attack would require that you "Save-As" a shortcut file (.lnk, .pif, .url, etc.) to the same place twice. The first file saved will go to the expected location. However, the second time you "Save-As" with the expectation to replace the file, Windows dereferences the shortcut. Using this method, an attack can either destroy existing files or add new files which will execute (saving to the Startup directory). The reason for this is the following: Internet Explorer, and all of windows, suffers from this vulnerability. In IE 6 Running Windows XP SP2, Dan showed that a similar attack was possible. Significant usability issues. The fix was to disable shortcut dereferecing in the save-as dialogs. If Ere has a real fix that is good.
Well, I wouldn't have created that patch if I didn't believe it's good, but I don't oppose to backing out the original patch either. Whatever the decision, it would be about time to fix this issue in a way or another.
Ere's approach could fix the security hole while keeping the browser usable. That would be much better than backing out the original patch and leaving the browser vulnerable. Ere, does your patch prevent the attack from being used to overwrite "C:\Documents and Settings\All Users\Start Menu\Programs\Accessories\WordPad.lnk" or create a file in the Startup directory? I'm not familiar with how this code works, but the comment in the patch makes it sound like the patch only prevents files with non-shortcut extensions from being overwritten.
I made it so that you can overwrite a .lnk with another .lnk, but I can easily change that too if desired.
Attached file Firefox 1.0.4 Patch (obsolete) —
This patch will change the second (hax) entry 10 00 8D 85 in to 00 00 8D 85 in file firefox.exe
Attachment #183364 - Attachment description: A patch to alter the "Save As" bug in the most recent version of Firefox → Firefox 1.0.4 Patch
Still broken in Firefox 1.0.4.
(In reply to comment #140) > Still broken in Firefox 1.0.4. Did anyone say it was fixed? "Generalissimo Francisco Franco is still dead."
No, but the latest patch here does appear to fix 1.0.4. I don't know whether it reopens the security "hole" or not but at least the expected save as behaviour is back.
Comment on attachment 183364 [details] Firefox 1.0.4 Patch Obsoleting per comment #77
Attachment #183364 - Attachment is obsolete: true
Flags: blocking1.8b3?
Flags: blocking1.8b2?
Flags: blocking1.8b2-
Flags: blocking1.7.8?
*** Bug 293536 has been marked as a duplicate of this bug. ***
I hope this isn't noise, or too redundant: All we want is to be able to 'click-thru' shortcuts to folders. I have not seen a comment from anyone that said they wanted to dereference any other type of shortcut during a SAVE. Countless other programs allow this in their Save dialogs. I dont understand why the fix can't just allow only dereferencing of folder shortcuts, but not file shortcuts.
Because Windows doesn't offer such a choice. You either dereference or you don't -- the target type doesn't matter.
Since shortcut files (.lnk) are usually only meaninful on a local machine (and I've never heard of anyone downloading one) why don't we just make them part of a list of files Mozilla/Firefox will not download? If someone needs to work-around it they could just zip up the shortcut for people to download. This is a simple and effective solution. (hopefully)
(In reply to comment #147) > I've never heard of anyone downloading [a .lnk file]) Yeah, I didn't either. Funny idea... I didn't hear about .pif files ("program information files") since Windows for Workgroups, long ago (anyone left using this dinosaur? ... and if [s]he did, would [s]he try to save a .pif file? And if [s]he did, would [s]he try to save the same .pif file twice, without having a look at the first downloaded version?! And if [s]he did, would [s]he do it using Mozilla? And if [s]he did, would [s]he blame Mozilla for his/her own stupidity? And if [s]he did: Would it bother the oak if a hog would scratch it's back? And would, finally, this very single hog outweight the miriads of users who wonder why Mozilla doesn't behave like any sane application, disallowing the user to follow his/her own soft links?!) Nobody offers a .url file for download; everyone would wonder, "why would anyone offer such a file for download; everyone would create a bookmark/'favorite' for an ordinary link if [s]he needed it". Thus, IF someone thinks it be necessary to take security precautions: make a confirmation dialog pop up whenever someone tries to download a .lnk, .pif or .url file to a Windows system. Or simply remove the default file name for those, like suggested in comment #38. But, please: allow users to use their soft links, and make this VERY ANNOYING BUG *block* the next non-security release.
(In reply to comment #148) > (In reply to comment #147) > > I've never heard of anyone downloading [a .lnk file]) > > Yeah, I didn't either. Funny idea... > > I didn't hear about .pif files ("program information files") since Windows for > Workgroups, long ago (anyone left using this dinosaur? ... and if [s]he did, > would [s]he try to save a .pif file? And if [s]he did, would [s]he try to save > the same .pif file twice, without having a look at the first downloaded > version?! And if [s]he did, would [s]he do it using Mozilla? And if [s]he did, > would [s]he blame Mozilla for his/her own stupidity? And if [s]he did: Would it > bother the oak if a hog would scratch it's back? And would, finally, this very > single hog outweight the miriads of users who wonder why Mozilla doesn't behave > like any sane application, disallowing the user to follow his/her own soft links?!) > > Nobody offers a .url file for download; everyone would wonder, "why would anyone > offer such a file for download; everyone would create a bookmark/'favorite' for > an ordinary link if [s]he needed it". > > Thus, IF someone thinks it be necessary to take security precautions: make a > confirmation dialog pop up whenever someone tries to download a .lnk, .pif or > .url file to a Windows system. Or simply remove the default file name for those, > like suggested in comment #38. But, please: allow users to use their soft links, > and make this VERY ANNOYING BUG *block* the next non-security release. YES! GREAT comment! :-] *applause*
I vote for comment #38. Please fix it at least for Firefox 1.1.
*** Bug 294345 has been marked as a duplicate of this bug. ***
Comment on attachment 182604 [details] [diff] [review] Alternate patch v1 >+ ::MessageBox(ofn.hwndOwner, >+ "Cannot overwrite a file of another type with a shortcut file.", "Error", MB_ICONERROR); The error string needs to be localizable. > MessageBox(ofn.hwndOwner, >- 0, > "The filepicker was unexpectedly closed by Windows.", >+ "Error", > MB_ICONERROR); Or not? Is there a lot of this kind of thing in widget-land? This doesn't cover the hypothetical case where the first .lnk references a file that *doesn't* exist in order to put a file into a startup location. Hm..
Do we really want to go in the direction of attempting to anticipate and code for every possible permutation of known Windows API security issues? I don't think it's possible. I think we're faced with an old-fashioned trade off here: we either break functionality, or allow the security "issue" to occur to some extent.
(In reply to comment #138) > I made it so that you can overwrite a .lnk with another .lnk, but I can easily > change that too if desired. Please do.
No longer blocks: majorbugs
*** Bug 295967 has been marked as a duplicate of this bug. ***
(In reply to comment #148) > (In reply to comment #147) > > I've never heard of anyone downloading [a .lnk file]) > > Yeah, I didn't either. Funny idea... > > I didn't hear about .pif files ("program information files") since Windows for > Workgroups, long ago (anyone left using this dinosaur? ... and if [s]he did, > would [s]he try to save a .pif file? And if [s]he did, would [s]he try to save > the same .pif file twice, without having a look at the first downloaded > version?! And if [s]he did, would [s]he do it using Mozilla? And if [s]he did, > would [s]he blame Mozilla for his/her own stupidity? And if [s]he did: Would it > bother the oak if a hog would scratch it's back? And would, finally, this very > single hog outweight the miriads of users who wonder why Mozilla doesn't behave > like any sane application, disallowing the user to follow his/her own soft links?!) > > Nobody offers a .url file for download; everyone would wonder, "why would anyone > offer such a file for download; everyone would create a bookmark/'favorite' for > an ordinary link if [s]he needed it". > > Thus, IF someone thinks it be necessary to take security precautions: make a > confirmation dialog pop up whenever someone tries to download a .lnk, .pif or > .url file to a Windows system. Or simply remove the default file name for those, > like suggested in comment #38. But, please: allow users to use their soft links, > and make this VERY ANNOYING BUG *block* the next non-security release. I agree but do not have the skills to fix it. PLEASE fix this. It will make my life, and others much better!
the last patch for firefox 1.04 seems to work perfect!
the last patch for firefox 1.04 seems to work perfect!
Phew, that was one annoying bug/security fix. Last patch works for me. I cannot understand why Firefox was deployed with such an annoying limitation. Thanks! Rufer
Comment on attachment 182604 [details] [diff] [review] Alternate patch v1 in comment 152, dan mentions that this patch doesn't solve a case where the first lnk file can be pointing at a non-existent file. We need to solve that case as well if we hope to address this hole. + nsCOMPtr<nsILocalFile> file(do_GetService(NS_LOCAL_FILE_CONTRACTID)); localfile's are objects, and isn't a service. we need to use createInstance here. Doug
Attachment #182604 - Flags: review?(dougt) → review-
I'm starting to lean to just backing out bug 271732 and calling it a windows problem.
Flags: blocking1.8b3?
Flags: blocking1.8b3+
Flags: blocking1.7.9+
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1+
Flags: blocking-aviary1.0.5?
Flags: blocking-aviary1.0.5+
(In reply to comment #160) > + nsCOMPtr<nsILocalFile> > file(do_GetService(NS_LOCAL_FILE_CONTRACTID)); D'oh. I'm fairly sure I just copied that from somewhere else.. but might have been my mistake too. Anyway, I'm all for backing out the whole thing.
dougt: Do you have a fix for this or is dveditz's idea a better approach for this? Should we backout that other change?
*** Bug 297050 has been marked as a duplicate of this bug. ***
The problem is that the shortcut is not <b>followed</b>. The shortcut should be followed not an overwrite warning shown.
The actual problem is that *if* the shortcut was followed, Firefox *might* overwrite a file if you download a shortcut file and save it as another shortcut file. Which is a windows bug, not a Firefox bug.
Has anyone from mozilla.org tried contacting Microsoft to see about working with them on resolving this bug, or at least to notify them of the bug?
Dan, your final verdict?
Attached patch Backout patch — — Splinter Review
We're going to back out bug 271732 and leave the problem up to MS as per Dan's decision.
Assignee: dveditz → emaijala
Attachment #181139 - Attachment is obsolete: true
Attachment #182604 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #186504 - Flags: superreview?(dveditz)
Attachment #186504 - Flags: review?(dveditz)
Attachment #186504 - Flags: approval1.8b3?
dveditz: let's get the reviews here and give the a= for the branches.
Dan asked me my thoughts on backing this out. I suggested that we just add a preference such that if you needed this behavior, you could just set a pref. This would fix majority of advanced users as they would have a way to enable this functionality.
(In reply to comment #171) > Dan asked me my thoughts on backing this out. I suggested that we just add a > preference such that if you needed this behavior, you could just set a pref. > This would fix majority of advanced users as they would have a way to enable > this functionality. That's unacceptable... this is not "advanced user" functionality... this is basic Windows functionality that should be (and has been in the past) enabled by default!
*** Bug 297964 has been marked as a duplicate of this bug. ***
*** Bug 291842 has been marked as a duplicate of this bug. ***
(In reply to comment #172) > That's unacceptable... this is not "advanced user" functionality... this is > basic Windows functionality that should be (and has been in the past) enabled by > default! I strongly agree that links should be followed by default. Otherwise this would continue to bite legions of users who might or might not find out about the preference. How about this: Whenever someone attempts to save something totally insane (like a .lnk, .url or .pif file), pop up a confirmation window which informs about the "security threat", tells about the precautions to take (don't save something to the same location twice Without Knowing Exactly What You Are Doing). Whoever happens to need to frequently save files with such names might suppress this confirmation by enabling a non-default switch, maybe using a "don't show this message again" checkbox.
Attachment #182604 - Flags: superreview?(dveditz)
Shortcuts are not just for advanced folks and they need to work right out of the box without any tweaking of settings (which I would consider much more advanced). Besides, I don't want to have a setting that would hinder the usability so much when there are multiple viable alternatives. Anyway, this is yet another approach I like for its simplicity. Let's not dereference shortcuts if the user is saving one. Otherwise, no threat. Please? The only downside I can see is the different behavior when saving a shortcut, but this should be very rarely encountered and actually quite logical.
Attachment #186543 - Flags: superreview?(dveditz)
Attachment #186543 - Flags: review?(dougt)
I was just working on basically the same thing, and since mine fixes a couple of additional bugs I'll attach it anyway. (I also have a working patch for the pref alternative, but I think we're all pretty well agreed that sucks). - Should the default filename exceed the filebuffer we correctly don't overwrite anything, but it's unterminated and the OS will read off into space (not a big deal). - An evil attacker could end their filename with some combination of dots and spaces. It'll still save as a .lnk but defeat naive extension-checking schemes.
Attachment #186546 - Flags: superreview?(dougt)
Attachment #186546 - Flags: review?(emaijala)
Comment on attachment 186543 [details] [diff] [review] YAAP - Yet Another Alternate Patch I like it, but it can be defeated by "foo.lnk..."
Attachment #186543 - Flags: superreview?(dveditz) → superreview-
Comment on attachment 186546 [details] [diff] [review] alternate version of YAAP Nice catches. r=emaijala
Attachment #186546 - Flags: review?(emaijala) → review+
Attachment #186543 - Attachment is obsolete: true
Attachment #186543 - Flags: review?(dougt)
Comment on attachment 186546 [details] [diff] [review] alternate version of YAAP change to the comment: // extra space and dots fool us To state that they do NOT fool us (since we know about them and are trimming them out) r=me lets do it.
Attachment #186546 - Flags: superreview?(dougt) → superreview+
Attachment #186504 - Flags: approval1.8b3?
Attachment #186546 - Flags: approval1.8b3?
Attachment #186546 - Flags: approval1.7.9?
Attachment #186546 - Flags: approval-aviary1.0.5?
Attachment #186546 - Flags: approval1.8b3?
Attachment #186546 - Flags: approval1.8b3+
Attachment #186546 - Flags: approval1.7.9?
Attachment #186546 - Flags: approval1.7.9+
Attachment #186546 - Flags: approval-aviary1.0.5?
Attachment #186546 - Flags: approval-aviary1.0.5+
Fix checked in to trunk, aviary1.0.1 and mozilla1.7 branches
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Keywords: fixed-aviary1.0.5, fixed1.7.9
Resolution: --- → FIXED
Ere: I think attachment 186546 [details] [diff] [review] is buggy. > + // Don't follow shortcuts when saving a shortcut, this can be used > + // to trick users (bug 271732) > + NS_ConvertUTF16toUTF8 ext(mDefault); > + ext.Trim(" .", PR_FALSE, PR_TRUE); // extra space and dots fool us Why you convert the extension UTF16 to UTF8? I think that we MUST convert UTF16 to Native. You should use NS_CopyNativeToUnicode instead of NS_ConvertUTF16toUTF8. See this patch. https://bugzilla.mozilla.org/attachment.cgi?id=166672
Oops. Sorry. NS_CopyNativeToUnicode -> NS_CopyUnicodeToNative See https://bugzilla.mozilla.org/attachment.cgi?id=166569
(In reply to comment #182) > I think attachment 186546 [details] [diff] [review] [edit] is buggy. > > + NS_ConvertUTF16toUTF8 ext(mDefault); > > + ext.Trim(" .", PR_FALSE, PR_TRUE); // extra space and dots fool us > > Why you convert the extension UTF16 to UTF8? > I think that we MUST convert UTF16 to Native. No, we don't have to. |ext| is only used to check whether |mDefault| ends with '.url/.lnk/.???' so that it doesn't matter whether it's UTF-8 or 'ANSI' string. Actually, UTF-8 is better.
Attachment #186504 - Flags: superreview?(dveditz)
Attachment #186504 - Flags: review?(dveditz)
*** Bug 298792 has been marked as a duplicate of this bug. ***
*** Bug 292983 has been marked as a duplicate of this bug. ***
*** Bug 298812 has been marked as a duplicate of this bug. ***
*** Bug 298941 has been marked as a duplicate of this bug. ***
*** Bug 299152 has been marked as a duplicate of this bug. ***
*** Bug 299388 has been marked as a duplicate of this bug. ***
Alias: Shortcut
v.fixed on aviary with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.9) Gecko/20050706 Firefox/1.0.5
*** Bug 306194 has been marked as a duplicate of this bug. ***
The actual problem is that *if* the shortcut was followed, Firefox *might* overwrite a file if you download a shortcut file and save it as another shortcut file. Which is a windows bug, not a Firefox bug. http://www.seslichatailesi.com
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: