Conversation
…uble-nesting
FeatherQuilld daemon endpoints inconsistently return either a bare
payload ({"ok": true}) or wrap their entire payload in a "data" key
(list/search/pull-jobs return {"data": [...]} or
{"data": {"entries": [...]}}).
WebSpaceFilesController::daemonResponse() always forwarded the full
daemon body into ApiResponse::success(), which itself always wraps
its argument in another "data" key. For endpoints in the second
group this produced a doubly-nested {"data": {"data": [...]}}
response. The frontend reads response.data expecting the actual
list/object and got the wrapper instead, so the file manager showed
"No Files Found" even when the WebSpace had files.
Unwrap the daemon's own "data" key only when it is the sole
top-level key and itself an array, so paginated/list-style responses
collapse to a single envelope while endpoints that pair "data" with
sibling keys (e.g. "ok") or return a bare scalar under "data" (e.g.
files/contents returning file text, which the frontend already reads
as response.data.data) are left untouched.
Found during an internal security review, on top of the existing PR that touches this same controller. WebSpaceFilesController proxies raw, user-controlled path strings (from/to/file/root/link/target/directory/entries/name/destination) straight through to FeatherQuilld, the daemon that actually touches the filesystem. The only defense against `../` traversal, NUL-byte injection, and symlink-target escapes was whatever normalization the daemon itself does - there was no second line of defense in this panel if the daemon ever had a bug there. Added WebSpacePathValidator, a small dependency-free helper, and wired it into every endpoint in this controller that accepts a path: contents, write, createDirectory, rename, copy, copyMany, createSymlink, fingerprints, delete, compress, decompress, chmod, pull, download, upload (both directory and each uploaded file's original name), downloadDirectory, listArchive, extractArchiveSelection (root/file/ destination only - the "entries" array is archive-internal paths, a different concern for the extractor itself), getUploadUrl, and shareFile. Rejects: NUL bytes, backslashes, and any `..` path segment. Filename-only fields (created directory/symlink/uploaded file names, compress archive name) additionally reject any path separator at all, since those never legitimately contain one. Testing: - 18 standalone assertions covering valid paths (plain/nested/leading- slash/root), rejected paths (`../../../etc/passwd`, mid-path traversal, bare `..`, NUL byte, backslash traversal, empty string), and the isSafeFilename/firstInvalid/reject wrappers - all passing. - Re-ran the same assertions inside the live production container (through the real composer autoloader, exercising the actual ApiResponse::error() Response object reject() returns) - all passing. - Live curl against the deployed /files/list endpoint confirms existing auth middleware still runs first and the route wiring is intact (no crash, same INVALID_ACCOUNT_TOKEN response as before deploying this).
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
2 participants
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.
Found during an internal security review while looking at the WebSpace file-manager endpoints touched by #223.
WebSpaceFilesControllerproxies raw, user-controlled path strings (from/to/file/root/link/target/directory/entries/name/destination) straight through to FeatherQuilld, the daemon that actually touches the filesystem. The only defense against../traversal, NUL-byte injection, and symlink-target escapes was whatever normalization the daemon itself does - there was no second line of defense in the panel if the daemon ever had a bug there.This PR is stacked on #223 (targets that branch) since it touches the same controller and that PR isn't merged yet - please merge #223 first, or I can rebase this onto
mainonce it lands, whichever is easier for you.What changed
Added
WebSpacePathValidator, a small dependency-free helper, and wired it into every endpoint in this controller that accepts a path:contents,write,createDirectory,rename,copy,copyMany,createSymlink,fingerprints,delete,compress,decompress,chmod,pull,download,upload(both thedirectoryparam and each uploaded file's original name),downloadDirectory,listArchive,extractArchiveSelection(root/file/destination only - theentriesarray is archive-internal paths, a different concern for the extractor itself),getUploadUrl, andshareFile.Rejects: NUL bytes, backslashes, and any
..path segment. Filename-only fields (created directory/symlink/uploaded file names, compress archive name) additionally reject any path separator at all, since those never legitimately contain one.Testing
../../../etc/passwd, mid-path traversal, bare.., NUL byte, backslash traversal, empty string), and theisSafeFilename/firstInvalid/rejectwrappers - all passing.ApiResponse::error()Response objectreject()returns) - all passing./files/listendpoint confirms existing auth middleware still runs first and the route wiring is intact (no crash, sameINVALID_ACCOUNT_TOKENresponse as before deploying this).