IBX-11681: Added filename validation in DownloadController and corresponding tests#756
Conversation
wiewiurdp
force-pushed
the
IBX-11681-Possibility-to-download-all-files-using-a-loop-which-may-cause-DDoS
branch
2 times, most recently
from
1ba641c to
93d53c1
Compare
ibexa-workflow-automation-1
Bot
requested review from
Steveb-p,
ViniTou,
alongosz,
barw4,
bnowak,
ciastektk,
konradoboza,
mikadamczyk and
tbialcz
and removed request for
a team
mikadamczyk
left a comment
mikadamczyk
left a comment
There was a problem hiding this comment.
Could we add an integration test for URL-encoded filenames here? Since the new check uses strict equality $field->value->fileName !== $filename, any encoding or decoding differences in the real request flow may cause valid downloads to be rejected as NotFound. Especially for spaces or special characters.
konradoboza
left a comment
konradoboza
left a comment
There was a problem hiding this comment.
Looks good but I think providing the test case that Mikołaj has mentioned makes sense.
wiewiurdp
force-pushed
the
IBX-11681-Possibility-to-download-all-files-using-a-loop-which-may-cause-DDoS
branch
4 times, most recently
from
be18b9b to
51d7f2c
Compare
|
konradoboza
added
Bug
tomaszszopinski
force-pushed
the
IBX-11681-Possibility-to-download-all-files-using-a-loop-which-may-cause-DDoS
branch
from
f62054c to
b3c2901
Compare
wiewiurdp
force-pushed
the
IBX-11681-Possibility-to-download-all-files-using-a-loop-which-may-cause-DDoS
branch
from
d5cfc8f to
0fc0fd4
Compare
|
| throw $this->createFileNotFoundException($e); | ||
| } | ||
|
|
||
| return $this->downloadBinaryFileAction($contentId, $field->fieldDefIdentifier, $field->value->fileName, $request); |
There was a problem hiding this comment.
Do we also need to block or harden the /content/download/{contentId}/{fieldId} route? It seems to bypass the new filename validation because downloadBinaryFileByIdAction() forwards the persisted file name to downloadBinaryFileAction().
There was a problem hiding this comment.
Good point. Blocking/removing it in this PR would likely be BC.
For this PR, I would keep the route available and harden it by normalizing its validation/not-found responses in the same way as the filename-based route, so it does not reveal whether a guessed content/field combination exists.
I agree that deprecating it and eventually moving fully to the filename-based route would be worth considering, but I think that should be handled in a separate task. Wdyt @konradoboza
6 participants
Description:
Added filename validation to the content download endpoint for binary files.
Previously,
/content/download/{contentId}/{fieldIdentifier}/{filename}accepted anyfilenamevalue from the URL and still returned the file as long ascontentIdandfieldIdentifiermatched an accessible field. This made it possible to enumerate downloadable files more easily by guessing content identifiers and reusing common field identifiers such asfile.This change makes the download controller verify that the filename provided in the URL matches the actual file name stored in the field value. If it does not match, the controller now returns a generic 404 response before loading the binary file from storage.
The error handling around download validation was also tightened to avoid leaking information about existing content. Invalid field identifiers, invalid filenames, missing content, unavailable versions, trashed content, and repository-level not found cases are now normalized to the same generic "File not found" response. This prevents the endpoint from revealing whether a guessed content ID exists or which part of the download URL was valid.