Add the Ability to filter file uploads based on file extensions #6043
Conversation
| assertNotNull("Matched unlist extension", checkExtension("my test.notListed", mockProps)); | ||
| assertNotNull("Combined multiple extension matched incorrectly", checkExtension("multi.a_v.tar", mockProps)); | ||
| assertNotNull("Multi-multi extension matched unexpectedly", checkExtension("multi.not.tar.gz", mockProps)); | ||
| assertNotNull("No extension matched unexpectedly", checkExtension("No extension", mockProps)); |
There was a problem hiding this comment.
I hadn't considered files with no extension. I guess it's OK to reject them all at this point.
|
|
||
| if (FileUtil.isAllowedFileName(name) != null) | ||
| { | ||
| throw new IOException("The file extension is not allowed."); |
There was a problem hiding this comment.
This may reject the name unrelated to the exception. Let's use the message this call returns instead of hard-coding the error message
Sigmonia
force-pushed
the
24.11_fb_fileExtensions
branch
from
3a252ce to
ef580e1
Compare
| if (appProps.getAllowedExtensions().isEmpty()) | ||
| return null; | ||
|
|
||
| if (extensionChecker == null) |
There was a problem hiding this comment.
When does this get cleared? It seems very sticky.
There was a problem hiding this comment.
It only gets changed in setExtensionChecker which is called when the WritableAppProps.setAllowedFileExtensions is called when the new Allow List prop is saved or when the server is starting up
There was a problem hiding this comment.
I see. The pattern confused me. I would have expected a getExtensionCheck() that wrapped the null check.
|
Do we want to allow folder names to have "." in them? Disallowing doesn't seem to be related to the original goal of the feature. |
| String notAllowedMsg = FileUtil.isAllowedFileName(name); | ||
| if (StringUtils.isNotBlank(notAllowedMsg)) | ||
| { | ||
| throw new IOException(notAllowedMsg); |
There was a problem hiding this comment.
throw DavException in the dav controller to get the file browser to display an expected UI. Maybe use 409 SC_CONFLICT? or 406 (SC_NOT_ACCEPTABLE)
- fix regex quoting - modified checker methods to be testable adjusted description text
- Move isInvalidFilenameBlocked check - Subclass Multipart Resolver so we can more easily check the file extension - fix typos and messaging
Updated error message read the text passed
Sigmonia
force-pushed
the
24.11_fb_fileExtensions
branch
from
5bd71ea to
b8ea9af
Compare
Rationale
Secure Issue 51524: Allowlist for acceptable types of file upload
Related PRs
Changes