Opened 8 years ago

Closed 7 years ago

#2689 closed Bug (fixed)

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

Reported by: surfacepatterns Owned by: alfonsoml
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 mosipov 7 years ago.
absolute01.png (12.7 KB) - added by mosipov 7 years ago.
full00.png (13.6 KB) - added by mosipov 7 years ago.
full01.png (13.7 KB) - added by mosipov 7 years ago.
2689.patch (1.7 KB) - added by alfonsoml 7 years ago.
Proposed patch

Download all attachments as: .zip

Change History (20)

comment:1 Changed 7 years ago by mosipov

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

comment:2 Changed 7 years ago by surfacepatterns

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 7 years ago by mosipov

Changed 7 years ago by mosipov

Changed 7 years ago by mosipov

Changed 7 years ago by mosipov

comment:3 Changed 7 years ago by mosipov

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 7 years ago by 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.

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 follow-up: Changed 7 years ago by 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.

comment:6 Changed 7 years ago by mosipov

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 7 years ago by mosipov

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 7 years ago by surfacepatterns

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 7 years ago by mosipov

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 follow-up: Changed 7 years ago by surfacepatterns

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 7 years ago by mosipov

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 7 years ago by mosipov

  • Component changed from General to File Browser
  • Keywords Pending added

comment:13 Changed 7 years ago by alfonsoml

  • Keywords Confirmed Review? added; Pending removed
  • Milestone set to FCKeditor 2.6.5
  • Owner set to alfonsoml
  • Status changed from new to assigned

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

Changed 7 years ago by alfonsoml

Proposed patch

comment:14 Changed 7 years ago by martinkou

  • Keywords Review+ added; Review? removed

Tested with spaced file names in PHP connector.

comment:15 Changed 7 years ago by alfonsoml

  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed with [3872]

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