Opened 9 years ago

Closed 8 years ago

#2689 closed Bug (fixed)

URLs are encoded incorrectly when connectors pass back URLs for File elements in XML

Reported by: Devin Owned by: Alfonso Martínez de Lizarrondo
Priority: Normal Milestone: FCKeditor 2.6.5
Component: File Browser Version: FCKeditor 2.6.3
Keywords: Confirmed Review+ Cc:

Description

When the file browser is used to fetch files and folders, an optional "url" attribute can be passed back with each "File" element. If the "url" attribute is not passed back with each "File" element, then the resulting URLs are displayed correctly after fetching an image or link.

However, if the URL parameter is specified, then problems occur. The "GetFilesAndFoldersCallback" function will store the URL, assuming that it isn't encoded already (which is incorrect - the data is a URL, after all). Then, in the "OpenFile" function, the URL will be "re-encoded". This becomes a problem when the URL already contains encoded characters.

For example, if one of the parts of the URL contains a space, then the "url" attribute will come back with a '%20' (as any correct URL should). The resulting '%20' will then be re-encoded to read "%2520" in the 'OpenFile' function. Obviously, this would be incorrect.

Attachments (5)

absolute00.png (13.4 KB) - added by Michael Osipov 9 years ago.
absolute01.png (12.7 KB) - added by Michael Osipov 9 years ago.
full00.png (13.6 KB) - added by Michael Osipov 9 years ago.
full01.png (13.7 KB) - added by Michael Osipov 9 years ago.
2689.patch (1.7 KB) - added by Alfonso Martínez de Lizarrondo 9 years ago.
Proposed patch

Download all attachments as: .zip

Change History (20)

comment:1 Changed 9 years ago by Michael Osipov

Please set the Component: Server x to which you are referring.

comment:2 Changed 9 years ago by Devin

The component server is a custom server written by me. None of the connectors you include with your code make use of the "url" attribute. My code makes use of the "url" attribute because there are times when I want the URLs to be absolute URLs without the actual host name (when FCKEditor is being used to edit site HTML), and times when I want the URLs to be absolute URLs including the "http" protocol and the host name (when FCKEditor is being used to edit the HTML portion of e-mail messages).

I can assure you that the URLs coming back from my server are valid, as I've put 'alert' messages in the 'GetFilesAndFoldersCallback' function to ensure I'm getting properly formatted URLs. I can also assure you that the 'OpenFile' function is using the "encodeURI" function to encode the URL regardless of whether or not the URL is already properly encoded.

A better solution to this problem would be to handle the encoding of URLs in the 'GetFilesAndFoldersCallback' function. You could change this statement:

var sFileUrl = oFileUrlAtt != null ? oFileUrlAtt.value : sCurrentFolderUrl + sFileName ;

... to something like this:

var sFileUrl = oFileUrlAtt != null ? oFileUrlAtt.value : encodeURI(sCurrentFolderUrl + sFileName);

... and then change 'OpenFile' so that it doesn't use 'encodeURI'.

Changed 9 years ago by Michael Osipov

Attachment: absolute00.png added

Changed 9 years ago by Michael Osipov

Attachment: absolute01.png added

Changed 9 years ago by Michael Osipov

Attachment: full00.png added

Changed 9 years ago by Michael Osipov

Attachment: full01.png added

comment:3 Changed 9 years ago by Michael Osipov

Well, I don't see the problem yet. The Connectors require to return the path as-is. The File Browser has to handle URI encoding. I just tested your scenarios on my server with an absolute path and an full url path.

Absolute path: , then .

Full URL path: , then .

Seems to me absolutely correct.

comment:4 Changed 9 years ago by Devin

How is your custom connector returning the "url" attribute to the browser? If you're not returning it as a properly encoded URL, then the "url" key is a misnomer.

If you don't accept properly encoded URLs in the "url" attribute, then you are limiting what the developer can do. For example, let's assume that the developer has reason to return a URL with a '#' character embedded in the URL, perhaps because the developer would like the path to be linked to a specific part of an HTML document. If that happens, the pound sign will become encoded and the URL will no longer work as the developer intended.

I'm sure there are other similar scenarios.

comment:5 Changed 9 years ago by Devin

Just to make sure that I'm being clear, I'm talking about returning "File" elements in the "GetFoldersAndFiles" command with url attributes like so:

[beginning xml data]

<Files>

<File name="some file.txt" size="1024" url="http://www.somehost.com/some%20folder/some%20file.txt" />

</Files>

[end xml data]

The "url" attribute is not used by any of your standard connectors.

comment:6 Changed 9 years ago by Michael Osipov

Replying to surfacepatterns:

How is your custom connector returning the "url" attribute to the browser? If you're not returning it as a properly encoded URL, then the "url" key is a misnomer.

Returned as-is. It's not a misnomer because when the URL is exposed to the user, the fiel browser encodes it properly.

If you don't accept properly encoded URLs in the "url" attribute, then you are limiting what the developer can do. For example, let's assume that the developer has reason to return a URL with a '#' character embedded in the URL, perhaps because the developer would like the path to be linked to a specific part of an HTML document. If that happens, the pound sign will become encoded and the URL will no longer work as the developer intended.

Well, you are correct, the # marking a fragment should not be encoded. It shall be encoded if it represents part of a value.

This is a bug in frmresourcelist.html line 90:

function OpenFile( fileUrl )
{
	window.top.opener.SetUrl( encodeURI( fileUrl ).replace( '#', '%23' ) ) ;
	window.top.close() ;
	window.top.opener.focus() ;
}

The replace method is illegal but a file url can never contain a fragment link, this code remains fine.

comment:7 in reply to:  5 Changed 9 years ago by Michael Osipov

Replying to surfacepatterns:

Just to make sure that I'm being clear, I'm talking about returning "File" elements in the "GetFoldersAndFiles" command with url attributes like so:

[beginning xml data]

<Files>

<File name="some file.txt" size="1024" url="http://www.somehost.com/some%20folder/some%20file.txt" />

</Files>

[end xml data]

The "url" attribute is not used by any of your standard connectors.

No connector uses the url. They simply return "http://www.somehost.com/some folder/some file.txt" and the OpenFile method uses it.

comment:8 Changed 9 years ago by Devin

Since you still don't seem to understand, how about an example where the filename contains a '/'? A filesystem implementation doesn't _need_ to use a forward or backward slash as a path delimiter.

Let's assume that a filename contains a slash (example: "Cost/Benefit Analysis.html"). If this file was located at the root of the site "www.costbenefit.com", then a correct URL for this file would be:

http://www.costbenefit.com/Cost%2fBenefit%20Analysis.html

... but FCKEditor wouldn't handle the URL correctly.

comment:9 Changed 9 years ago by Michael Osipov

Well, the example you gave, wouldn't work at all because we enforce file and foldername sanitation. See: http://dev.fckeditor.net/browser/FCKeditor.Java/tags/2.4.1/java-core/src/main/java/net/fckeditor/tool/UtilsFile.java#L52 and http://dev.fckeditor.net/browser/FCKeditor.Java/tags/2.4.1/java-core/src/main/java/net/fckeditor/tool/UtilsFile.java#L72

Every connector has to do this. So you won't get into trouble with slash or back slash.

comment:10 Changed 9 years ago by Devin

Why are you referencing code from FCKEditor.Java when I'm referring to code in FCKEditor?

Can I please get another developer involved in this, one that understands the issue here?

comment:11 in reply to:  10 Changed 9 years ago by Michael Osipov

Replying to surfacepatterns:

Why are you referencing code from FCKEditor.Java when I'm referring to code in FCKEditor?

Because sanitation happens on the server-side. The JavaScript processes the XML and applies encodeURI and that's it. Nothing more.

Can I please get another developer involved in this, one that understands the issue here?

Well, wait unitl someone else answers.

comment:12 Changed 9 years ago by Michael Osipov

Component: GeneralFile Browser
Keywords: Pending added

comment:13 Changed 9 years ago by Alfonso Martínez de Lizarrondo

Keywords: Confirmed Review? added; Pending removed
Milestone: FCKeditor 2.6.5
Owner: set to Alfonso Martínez de Lizarrondo
Status: newassigned

It makes sense to avoid this artificial limitation, and allow the developer of custom connectors to fully use the url attribute.

Changed 9 years ago by Alfonso Martínez de Lizarrondo

Attachment: 2689.patch added

Proposed patch

comment:14 Changed 8 years ago by Martin Kou

Keywords: Review+ added; Review? removed

Tested with spaced file names in PHP connector.

comment:15 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Resolution: fixed
Status: assignedclosed

Fixed with [3872]

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy