EZP-30956: Added raw URL encoding of download file name

[glye]

Question	Answer
JIRA issue	EZP-30956
Bug/Improvement	yes
New feature	no
Target version	6.x/7.x
BC breaks	no
Tests pass	yes
Doc needed	maybe

If you have uploaded a file with a name beginning with a left parenthesis ( character, trying to download it with the route ez_content_download:
/content/download/{contentId}/{fieldIdentifier}/{filename}

...means the filename gets interpreted as a view parameter. So the router thinks there are only 4 parameters, and it will actually match the route ez_content_download_field_id:
/content/download/{contentId}/{fieldId}

...which fails since the field identifier isn't a valid field id. This happens regardless of whether the filename is url encoded or not in the request.

Fixing it by rawurlencode() of the filename in the ContentDownloadRouteReferenceListener. Download using the redirect route (ez_content_download_field_id) works with this patch. This redirects to ez_content_download. However, calling ez_content_download directly still does not work with such filenames, and leads to the same error as before the patch. So the fix is incomplete, if we intend for direct use to be possible.

TODO:

• Implement feature / fix a bug.
• Implement tests.
• Fix new code according to Coding Standards ($ composer fix-cs).
• Ask for Code Review.

[ViniTou]

Outside scope of possible BC breaks, is there any reason for this being static function? And should it not target 7.5?

[glye]

@ViniTou Static: Why not?
7.5: Why, when the bug affects all versions afaik? (Damn, I meant to make the PR against 6.7, I see now it's master, I'll change that.)

[ViniTou]

Static: Why not?

Well, for me it should be other way around - why static.

And, yeah, 6.7 is also good :)

[glye]

Made non-static and rebased on 6.7. Also expanded the PR description.
Squash to trigger test run on 6.7

[glye]

Ensure the filename filtering can't result in an empty string, which would break the route generator.

[alongosz]

Sorry for the long delay.

1. Please add test coverage for the use case (unit test case for this listener already exists).

2. Please change your PR title & commit messages to reflect what changed in the codebase (past simple), not what the bug is.

3. Do we need to filter anything else? I have included ( at the beginning of names, and / anywhere, since that won't work either unless we specifically configure the route to accept it.

   Definitely. See the diff comment.

[glye]

@alongosz @ViniTou Ready for re-review. Switched to doing rawurlencode. Added tests, which pass. Updated PR/commit messages.

NB: Download using the redirect route (ez_content_download_field_id) works with this patch. This redirects to ez_content_download. However, calling ez_content_download directly still does not work with such filenames in my test, and leads to the same error as before the patch. So the fix is incomplete, if we intend for direct use to be possible.

[alongosz]

However, calling ez_content_download directly still does not work with such filenames in my test, and leads to the same error as before the patch. So the fix is incomplete, if we intend for direct use to be possible.

How can I reproduce locally this issue?

[glye]

• Upload a BinaryFile named (Foo)bar.txt. Before applying the fix, view the content, and click download. This fails.
• Save the URL, which will be like /content/download/53/file/(Foo)bar.png
• Apply the fix.
• Click to download again, this works.
• Load the URL you saved, this fails.
• Try URL encoding the URL you saved, it still fails. /content/download/53/file/%28Foo%29bar.png

[ITernovtsii]

I think this fix will work better #2899

[glye]

Hi @ITernovtsiy, and thanks! I like your fix, it looks like a good addition, but it does not replace the url encoding fix, I think. We may need both. Url encoding is the "proper" way to fix the download problem, but more robust view parameter code would also be good.

[ITernovtsii]

@glye are you sure we actually need to do the encoding? I'm able to download /content/download/53/file/(Foo)bar.png file with just named parameters fix.

[glye]

@ITernovtsiy Yes, it works, but this relies on the happy coincidence that the filename is the last parameter of the route. If it wasn't, then a file named (foo) would break it. That's why it's "proper" to encode. It should be done in a way that doesn't depend on "luck", so that custom routes inspired by it can also work.

[ITernovtsii]

@glye if it will be the case, I would change the named parameter check to handle it properly.
And, will not have URL like %2528Foo%2529bar.png.

IMHO, double encoding here looks like a dirty hack over the actual problem with false-positive named parameter detection.
Even if you'll build your own route with file name inside it. Like something/(filename)/something-else. And, you'll have an issue caused by named parameters.
It will not mean that you did something wrong, it will mean that named parameters implementation is not smart enough to understand that (filename)/something-else is your route, not a named parameter.

[alongosz]

The point here is to serve the file using the name it was uploaded as. So I agree with @glye that we may need both.

[ITernovtsii]

@alongosz maybe I'm missing something (and sorry for being so protective).
With disposition header defined here, it will preserve file name with or without rawurlencode.
https://github.com/ezsystems/ezpublish-kernel/blob/7.5/eZ/Publish/Core/MVC/Symfony/Controller/Content/DownloadController.php#L64

In case if for some reason Content-Disposition: header will not be defined and with rawurlencode in place, file will be saved as %28Foo%29bar.png, which is not original file name.
can be tested with this PR applied and this code replacement:

//$response->setContentDisposition($contentDisposition, $filename);
$response->headers->set('Content-Disposition', 'attachment');

[glye]

In case if for some reason Content-Disposition: header will not be defined and with rawurlencode in place, file will be saved as %28Foo%29bar.png, which is not original file name.

In what case can this happen?

To be clear, what I really want is to remove the filename parameter from the route, and download the file using the name it was originally uploaded with. We already have this information. But the problem there is backwards compatibility.

[ITernovtsii]

In what case can this happen?

There is no such use case, it was just an example of the fact the URL will not represent original file name after this change.
%28Foo%29bar.png is correct, but %2528Foo%2529bar.png is not.

[glye]

Both examples represent the original file name, while neither is equal to it. The important thing is that the file is correctly saved. So long as named parameters are not defined/implemented in an unambiguous way (there seems to be no strict specification for them), I think it's too fragile to merge only one of these two PRs. I want both a fix for the current bug, and future stability. But @ezsystems/engineering-team decides.

[ITernovtsii]

@glye
nope, results are different in the controller for these two.
with (Foo)bar.png file uploaded in CMS

In downloadBinaryFileAction - https://github.com/ezsystems/ezpublish-kernel/blob/7.5/eZ/Publish/Core/MVC/Symfony/Controller/Content/DownloadController.php#L44

• with %28Foo%29bar.png in URL, $filename === '(Foo)bar.png' is true
• with %2528Foo%2529bar.png in URL, $filename === '(Foo)bar.png' is false

The important thing is that the file is correctly saved.

It is correctly saved without rawurlencode (and %28Foo%29bar.png in URL).

I want both a fix for the current bug, and future stability.

Completely agree here, and that is the main reason why I don't think it's a good idea to have $filename = '%28Foo%29bar.png'; inside the controller, as it's a potential place for bugs (for developers who expect original filename, and wasn't aware of double-encoding).

Yes, let's @ezsystems/engineering-team decide :)

[glye]

I don't want to have a further semantic discussion about what we mean by "represent"... The important thing is that the file is correctly saved, which afaik it is with or without rawurlencode. So both are "correct". The encoding makes the whole concept less fragile, avoiding any ambiguity with named parameters. I'm open for other ways of achieving that goal.

[adamwojs]

Closing PR as obsolete. eZ Platform 2.5 has reached EOM, so please reopen PR in ezsystems/ezplatform-kernel (bug fixes) or ibexa/core (features/improvements) if issue is still valid for v3.3 / v4.x and you are willing to work on it.