bug: sanitize static asset fullUrl and validate stored file extension - #102
Open
devin-ai-integration[bot] wants to merge 1 commit into
Open
devin-ai-integration[bot] wants to merge 1 commit into
devin-ai-integration[bot] wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
StaticAssetServiceImplbuilt an asset'sfullUrlstraight from client input —entityType/entityId, the optionalfileNameproperty and the multipart filename — with only the configurable+,*,%replacement applied, so/,\and..survived intoStaticAsset.fullUrland from there into the filesystem write inStaticAssetStorageServiceImpl.createStaticAssetStorage(asset.use.filesystem.storage=trueis the shipped default): an admin upload named../../../x.jspbecame an arbitrary file write. The extension check was also bypassable, because it validated the tika-detected content type while the file is stored under the filename's extension.Sanitization (all in
StaticAssetServiceImpl):sanitizeFileNameis applied in both entry points (createStaticAssetFromFileandcreateStaticAsset, which is also callable directly),buildAssetURLends withvalidateAssetURL, and the storage sink keeps its ownvalidateFileSystemAssetUrlguard plus canonical-path containment so a pre-existing/importedfullUrlcannot escape the file work area either.Extension validation now runs twice — once on the tika-detected extension and once on the extension the file is actually stored under:
and the shipped
disabled.file.extensionsdefault is widened frompdfto the usual server-executable set (jsp,php,asp,sh,exe,war, ...). Behaviour for extension-less uploads under the deny list is unchanged.Testing
repo1.maven.organdnexus2.broadleafcommerce.orgare blocked on this machine, so the module cannot be built here. The new/changed logic was extracted verbatim into standalone harnesses and run against commons-io/commons-lang3: 28 path-traversal payload checks and 13 extension checks (includingshell.jspwith PNG content,shell.PhP5,../../../x.jsp, and allow-list mode) all behave as expected. Unit tests covering both are added inStaticAssetServiceImplTestandStaticAssetStorageServiceImplTestfor CI.Session: https://app.devin.ai/sessions/868bdc1811d742369bb0b366b00501c0
Devin-Org: engineering
Devin Review