fix: at resources in resources.ts#15199
fix: at resources in resources.ts#15199orbisai0security wants to merge 1 commit intolaurent22:devfrom
Conversation
Automated security fix generated by Orbis Security AI
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| // Prevent path traversal: extract only the filename portion and | ||
| // validate it against a strict whitelist before use. | ||
| const fileName = path.basename(request.files[0].path); | ||
| if (!fileName || !/^[\w.\-]+$/.test(fileName)) throw new ErrorBadRequest('Invalid file path'); | ||
| const filePath = os.tmpdir() + path.sep + fileName; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate where request.files is populated and which uploadDir is configured.
fd -t f -e ts -e js | xargs rg -nP -C3 '\b(formidable|multiparty|busboy|multer|IncomingForm)\b' 2>/dev/null
echo '---'
# Also see how request.files is constructed for the clipper API
rg -nP -C3 '\bfiles\s*[:=]' --type=ts -g '!**/*.test.ts' -g '*clipper*'
rg -nP -C3 'request\.files' --type=ts -g '!**/*.test.ts'
echo '---'
# And confirm whether uploadDir is explicitly set
rg -nP -C2 '\buploadDir\b' --type=tsRepository: laurent22/joplin
Length of output: 9279
🏁 Script executed:
#!/bin/bash
# Find where the resources route is mounted and which parser is used
rg -n 'routes/resources' --type=ts | head -20
echo '---'
# Check if it's mounted on ClipperServer
rg -n -C5 'resources' packages/lib/ClipperServer.ts | head -50
echo '---'
# Look for tests of the resources endpoint
fd -t f '*.test.ts' -o '*.test.js' | xargs rg -l 'resources.*POST\|POST.*resources' 2>/dev/null | head -10
echo '---'
# Find the actual route handler setup
rg -n 'new ApiResource\|ApiResource(' --type=ts | head -20Repository: laurent22/joplin
Length of output: 423
🏁 Script executed:
#!/bin/bash
# Check the git log for the resources.ts file to understand the change
cd /tmp && git -C /root/laurent22/joplin log --oneline -20 packages/lib/services/rest/routes/resources.ts 2>/dev/null || echo "Git log not available"
# Also try to see recent changes to that specific line range
git -C /root/laurent22/joplin log -p -S 'path.basename' --follow -- packages/lib/services/rest/routes/resources.ts 2>/dev/null | head -100Repository: laurent22/joplin
Length of output: 82
🏁 Script executed:
#!/bin/bash
# Verify what request.files property name and structure is expected
# Check the ApiResource class signature
rg -n -B5 -A20 'class ApiResource' packages/lib/services/rest/ApiResource.ts 2>/dev/null | head -80
echo '---'
# Check what request object structure is
rg -n 'interface.*Request\|type.*Request' packages/lib/services/rest --type=ts | grep -v test | head -20Repository: laurent22/joplin
Length of output: 64
🏁 Script executed:
#!/bin/bash
# Check ClipperServer to see how the resources endpoint is called
rg -n 'route_resources\|ApiResource' packages/lib/ClipperServer.ts
echo '---'
# Check the resources.ts file to see the full function signature and how request.files is used
cat -n packages/lib/services/rest/routes/resources.ts | head -100
echo '---'
# See what the request object looks like and where it comes from
rg -n 'interface.*Request\|type.*Request' packages/lib/services/rest/RequestType.ts 2>/dev/null || rg -n 'interface.*Request\|type.*Request' packages/lib/services/rest --type=ts | grep -v test | head -20Repository: laurent22/joplin
Length of output: 3787
🏁 Script executed:
#!/bin/bash
# Look for test files related to resources
find packages -name '*resource*.test.ts' -o -name '*resource*.test.js' | head -10
echo '---'
# Check if there are any tests that POST to resources
rg -l 'resources.*method.*POST\|POST.*resources' --type=ts packages 2>/dev/null | head -10Repository: laurent22/joplin
Length of output: 379
🏁 Script executed:
#!/bin/bash
# Check what multiparty defaults to for uploadDir
rg -n 'new multiparty.Form\|new FormidableForm' packages/lib/ClipperServer.ts -A 10Repository: laurent22/joplin
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look at the original lines mentioned in the comment to understand the context better
sed -n '1,100p' packages/lib/services/rest/routes/resources.ts | cat -nRepository: laurent22/joplin
Length of output: 3779
🏁 Script executed:
#!/bin/bash
# Find where route_resources is used and how request.files is passed
rg -n 'route_resources\|from.*resources' packages/lib/services/rest/Api.ts -A 5 -B 5
echo '---'
# Check the Api class to see how request.files is populated
rg -n 'class Api\|request\.files\s*=' packages/lib/services/rest/Api.ts | head -30Repository: laurent22/joplin
Length of output: 64
🏁 Script executed:
#!/bin/bash
# Check if multiparty has a test or if there's documentation about its default uploadDir
# First, check what version of multiparty is being used
rg -n 'multiparty' packages/lib/ClipperServer.ts -B 2 -A 15 | head -50
echo '---'
# Check the form.parse() call to see if uploadDir is configured
rg -n 'form\.parse\|new multiparty' packages/lib/ClipperServer.ts -B 5 -A 10Repository: laurent22/joplin
Length of output: 1531
🏁 Script executed:
#!/bin/bash
# Look for any tests that POST to the resources endpoint
find packages -name '*.test.ts' | xargs grep -l 'POST.*resource\|resource.*POST\|routes/resources' 2>/dev/null | head -10
echo '---'
# Also look in the rest API tests
find packages/lib/services/rest -name '*.test.ts' 2>/dev/nullRepository: laurent22/joplin
Length of output: 289
🏁 Script executed:
#!/bin/bash
# Check the Request type definition to understand what request.files structure is
rg -n 'interface Request\|type Request' packages/lib/services/rest --type=ts -B 2 -A 10 | head -60Repository: laurent22/joplin
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check the Api.test.ts file for tests of the resources endpoint
rg -n 'resources\|POST.*file\|multipart' packages/lib/services/rest/Api.test.ts | head -30
echo '---'
# Let's see the actual test file structure
wc -l packages/lib/services/rest/Api.test.ts
echo '---'
# Check what the test file contains
head -100 packages/lib/services/rest/Api.test.tsRepository: laurent22/joplin
Length of output: 3522
🏁 Script executed:
#!/bin/bash
# Check the Api.ts file to understand how files are parsed and passed to the route
cat -n packages/lib/services/rest/Api.ts | head -150Repository: laurent22/joplin
Length of output: 5613
🏁 Script executed:
#!/bin/bash
# Look at the multiparty documentation or check what the default behavior is
# First, check if there's any configuration of multiparty in the codebase
rg -n 'uploadDir\|tmpDir' packages/lib --type=ts --type=js
echo '---'
# Also check package.json to see the multiparty version
rg -n 'multiparty' packages/lib/package.json || rg -n 'multiparty' package.jsonRepository: laurent22/joplin
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Search for any tests that POST to resources endpoint with files
rg -n 'POST.*resources\|resources.*POST\|files.*POST\|\.route.*POST.*resource' packages/lib/services/rest/Api.test.ts
echo '---'
# Also search for any function that creates or uploads resources in tests
rg -n 'createResourceFromPath\|createResource.*file\|POST.*file' packages/lib/services/rest/Api.test.ts | head -20Repository: laurent22/joplin
Length of output: 64
🏁 Script executed:
#!/bin/bash
# Check what multiparty defaults to - search online documentation or check npm
# First, let's see if there's any comment in ClipperServer explaining multiparty behavior
cat -n packages/lib/ClipperServer.ts | sed -n '220,240p'
echo '---'
# Look at node_modules to see if multiparty is there and what it does
if [ -d "node_modules/multiparty" ]; then
head -50 node_modules/multiparty/index.js 2>/dev/null || echo "multiparty not found in node_modules"
echo "---"
cat node_modules/multiparty/package.json | grep -A 5 -B 5 'uploadDir\|tmpDir' 2>/dev/null || echo "No uploadDir found in package.json"
fi
# Let's check what the multiparty Form constructor signature is
rg -n 'uploadDir' node_modules/multiparty --type=js 2>/dev/null | head -10Repository: laurent22/joplin
Length of output: 1074
🏁 Script executed:
#!/bin/bash
# Search the entire test file for resource-related tests, especially file uploads
rg -n 'should.*resource\|resource.*create\|resource.*upload' packages/lib/services/rest/Api.test.ts -i
echo '---'
# Check if there are any tests with file handling
rg -n 'files\|\.files\|RequestFile' packages/lib/services/rest/Api.test.tsRepository: laurent22/joplin
Length of output: 64
🏁 Script executed:
#!/bin/bash
# Let's check the multiparty documentation or source to understand default uploadDir
# First, search the actual usage in the codebase - does anything configure uploadDir?
rg -n 'uploadDir\|tmpDir\|uploadfile\|tempfile' packages/lib --type=ts --type=js
echo '---'
# Look for the multiparty source or documentation in node_modules
if [ -d "node_modules/multiparty" ]; then
# Check the README
if [ -f "node_modules/multiparty/README.md" ]; then
head -100 node_modules/multiparty/README.md | grep -A 10 -B 10 "uploadDir\|temp"
fi
# Check the main index.js for default configuration
grep -n "uploadDir\|tmpDir" node_modules/multiparty/index.js | head -20
fiRepository: laurent22/joplin
Length of output: 64
🏁 Script executed:
#!/bin/bash
# Let's look at what the original code was before this change
# Search for git history or comments that might explain the change
rg -n 'path traversal\|CWE-22\|V-002\|scanner\|vulnerability' packages/lib/services/rest/routes/resources.ts
echo '---'
# Also check if there are any related issues or PRs mentioned in comments
rg -n '#[0-9]{4,5}\|issue\|PR\|pull request' packages/lib/services/rest/routes/resources.tsRepository: laurent22/joplin
Length of output: 64
🏁 Script executed:
#!/bin/bash
# Let's search for how request.files is used elsewhere in the codebase to understand the pattern
rg -n 'request\.files\[0\]\.path' packages/lib --type=ts --type=js
echo '---'
# And check if there are other similar handlers that use request.files
rg -n 'request\.files' packages/lib/services/rest/routes --type=ts -A 3 -B 3Repository: laurent22/joplin
Length of output: 1678
🏁 Script executed:
#!/bin/bash
# Let's check what multiparty does by default - search for documentation or source code
# First, check if there's any Node.js or npm information about multiparty defaults
# Since we can't access npm directly, let's see if there's any indication in code comments
# Check the multiparty package version and any inline documentation
cat packages/lib/package.json | grep -A 2 -B 2 multiparty
echo '---'
# Let's also check if there are any similar file upload handlers in the server codebase
rg -n 'form\.parse\|uploadDir\|tempDir\|tmpdir' packages/server --type=ts -B 2 -A 2 | head -40
echo '---'
# Check the formidable usage in server which might give us clues about multiparty
rg -n 'formidable.*Form\|uploadDir' packages/server/src/utils/requestUtils.ts -B 5 -A 10Repository: laurent22/joplin
Length of output: 193
🏁 Script executed:
#!/bin/bash
# Let's check if there's any documentation or issue tracking about this
# Search for any comments or issues about path handling
rg -n 'path.*traversal\|CWE\|security\|vulnerability' packages/lib/services/rest --type=ts
echo '---'
# Check the git log to see if this change was recent
git -C /root/laurent22/joplin log --oneline --all packages/lib/services/rest/routes/resources.ts 2>/dev/null | head -20Repository: laurent22/joplin
Length of output: 64
🏁 Script executed:
#!/bin/bash
# Let's verify what createResourceFromPath expects
rg -n 'createResourceFromPath' packages/lib --type=ts -B 2 -A 10 | head -60Repository: laurent22/joplin
Length of output: 3950
🏁 Script executed:
#!/bin/bash
# Let's check what multiparty actually does by examining its source or behavior
# Look for the actual multiparty library code
if [ -f "node_modules/multiparty/index.js" ]; then
echo "=== Checking multiparty index.js for uploadDir default ==="
grep -n "uploadDir\|tmpdir\|path.join" node_modules/multiparty/index.js | head -30
fi
echo "---"
# Let's also check if there are any actual tests that use POST with files
find packages/lib/services/rest -name "*.test.ts" -exec grep -l "POST.*file\|multipart\|files" {} \;
echo "---"
# Check the notes.ts handler which the review mentioned as a similar case
rg -n 'createResourceFromPath\|downloadMediaFile' packages/lib/services/rest/routes/notes.ts -B 3 -A 3Repository: laurent22/joplin
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Let's check what file format multiparty generates - search for any comments or tests
rg -n 'multiparty.*filename\|generateFilename\|randomString' packages/lib --type=ts --type=js
echo '---'
# Also check the actual test setup to see how files are mocked
rg -n 'RequestFile\|files\[0\]' packages/lib/services/rest/Api.test.ts -B 3 -A 3Repository: laurent22/joplin
Length of output: 64
🏁 Script executed:
#!/bin/bash
# Let's verify if the regex would actually work with typical filenames
# Create a simple test to check the regex
python3 <<'EOF'
import re
import string
# The regex from the code
pattern = r'^[\w.\-]+$'
regex = re.compile(pattern)
# Test cases
test_cases = [
'BW77wkpP23iIGUstd0kDuXXC.jpg', # From the comment
'upload-550e8400-e29b-41d4-a716-446655440000', # UUID format (formidable)
'tmpfile_12345.tmp',
'file-with-dash.txt',
'file_with_underscore.txt',
'CamelCase.txt',
'123numbers.txt',
]
print("Testing regex pattern:", pattern)
for test in test_cases:
matches = regex.match(test) is not None
print(f" {test:50s} -> {matches}")
EOFRepository: laurent22/joplin
Length of output: 521
🏁 Script executed:
#!/bin/bash
# Check the notes.test.ts to see if there are file upload tests
rg -n 'POST.*file\|createResource\|multipart\|files' packages/lib/services/rest/routes/notes.test.ts -B 2 -A 5 | head -50
echo '---'
# Let's check the notes.ts route to see how it handles files
rg -n 'createResourceFromPath\|downloadMediaFile' packages/lib/services/rest/routes/notes.ts -B 5 -A 5Repository: laurent22/joplin
Length of output: 64
🏁 Script executed:
#!/bin/bash
# Let's check the actual multiparty behavior by looking at how it's used
# and what files it produces
# Check if there's any documentation in package.json or README
if [ -f "node_modules/multiparty/README.md" ]; then
echo "=== multiparty README ==="
head -80 node_modules/multiparty/README.md
fiRepository: laurent22/joplin
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Let's search for the actual implementation to understand what path format multiparty uses
# Check if multiparty is documented to use os.tmpdir()
python3 <<'EOF'
import os
import tempfile
# Check what os.tmpdir() returns (Node.js equivalent is os.tmpdir() which uses os.path.gettempdir() in Python)
print("Python tempfile.gettempdir():", tempfile.gettempdir())
print("os.environ.get('TEMP'):", os.environ.get('TEMP'))
print("os.environ.get('TMP'):", os.environ.get('TMP'))
# The concern in the review is: does multiparty write to os.tmpdir()?
# Let's see what happens when we construct a path
EOFRepository: laurent22/joplin
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Let's check if there's any indication of what changed in the resources.ts file
# by looking at the git log more carefully
git -C /root/laurent22/joplin log --all --grep="resources\|path.*traversal\|CWE-22" --oneline 2>/dev/null | head -20
echo "---"
# Or check recent commits to that file
git -C /root/laurent22/joplin log --oneline -n 50 -- packages/lib/services/rest/routes/resources.ts 2>/dev/null || echo "Git not available"Repository: laurent22/joplin
Length of output: 82
🏁 Script executed:
#!/bin/bash
# Let's look for any documentation or comments about where multiparty writes files
# Check if there are any configuration files or setup files
find packages/lib -name "*.ts" -o -name "*.js" | xargs grep -l "multiparty\|Form()" | head -5
echo "---"
# Check ClipperServer more carefully for any configuration
cat -n packages/lib/ClipperServer.ts | sed -n '215,250p'Repository: laurent22/joplin
Length of output: 1534
🏁 Script executed:
#!/bin/bash
# Let's search the node_modules for multiparty to understand what it does
if [ -d "node_modules/multiparty" ]; then
# Check the actual source code of multiparty
echo "=== multiparty package.json ==="
cat node_modules/multiparty/package.json 2>/dev/null | head -30
echo "---"
echo "=== multiparty/index.js (first 100 lines) ==="
head -100 node_modules/multiparty/index.js 2>/dev/null
fiRepository: laurent22/joplin
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Let's check if there's a way to see what the actual default uploadDir is for multiparty
# by looking at its Form constructor options
rg -n "Form\s*=" node_modules/multiparty/index.js -A 20 2>/dev/null | head -50Repository: laurent22/joplin
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Let's check the actual behavior by looking at what the PR/issue was about
# Search for any security-related comments or issues
rg -n "security\|safe\|validation\|check\|verify" packages/lib/services/rest/routes/resources.tsRepository: laurent22/joplin
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Let's check the multiparty documentation or source to understand the default behavior
# Look at what uploadDir multiparty uses by default
python3 <<'EOF'
# Simulate what multiparty does by default
# According to multiparty docs, it should default to os.tmpdir()
import os
import tempfile
print("Default temp directory (os.tmpdir() equivalent):", os.path.expanduser('~/.tmp') or '/tmp')
print("Actual tempfile.gettempdir():", tempfile.gettempdir())
# The issue is: if multiparty writes to /tmp/BW77wkpP23iIGUstd0kDuXXC.jpg
# and the new code does path.basename() then reconstructs as os.tmpdir() + sep + fileName
# it should get the same path back
# But what if they differ? E.g., symlinks, different tmp locations on Windows, etc.
EOFRepository: laurent22/joplin
Length of output: 164
🏁 Script executed:
#!/bin/bash
# Let's check if there are any tests that actually use the resource POST endpoint
# Search more broadly for resource tests
find packages/lib -name "*.test.ts" -exec grep -l "resource" {} \; | head -10
echo "---"
# Check the notes.test.ts to see how file uploads are tested there
cat -n packages/lib/services/rest/routes/notes.test.ts | head -150Repository: laurent22/joplin
Length of output: 8099
🏁 Script executed:
#!/bin/bash
# Let's verify what the original issue or PR was about
# Check for any TODOs or FIXMEs related to this
rg -n "TODO\|FIXME\|BUG\|HACK\|XXX" packages/lib/services/rest/routes/resources.ts
echo "---"
# Also check if there are any recent changes
rg -n "path.basename\|os.tmpdir" packages/lib/services/rest/routes/resources.ts -B 3 -A 3Repository: laurent22/joplin
Length of output: 64
🏁 Script executed:
#!/bin/bash
# Let's verify the key assumption: does multiparty actually use os.tmpdir() by default?
# Check the multiparty source/docs
if [ -d "node_modules/multiparty" ]; then
echo "=== Checking multiparty defaults ==="
# Look for Form constructor and uploadDir handling
grep -n "uploadDir\|tmpDir\|this\.uploadDir\|uploadDir\s*=" node_modules/multiparty/index.js 2>/dev/null | head -20
echo "---"
# Check if there's a README with options
if [ -f "node_modules/multiparty/README.md" ]; then
grep -A 20 "uploadDir" node_modules/multiparty/README.md
fi
fiRepository: laurent22/joplin
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Let's also check what the actual implementation of createResourceFromPath does
# to understand if it really needs the exact path
cat -n packages/lib/shim-init-node.ts | sed -n '329,380p'Repository: laurent22/joplin
Length of output: 2322
🏁 Script executed:
#!/bin/bash
# Let's verify if there's a regression test for POST /resources endpoint
# Check Api.test.ts for resource creation tests
rg -n 'resource\|POST.*route\|createResource' packages/lib/services/rest/Api.test.ts -B 2 -A 5 | head -80Repository: laurent22/joplin
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Let's check if multiparty documentation or source reveals the default uploadDir behavior
# Look at what node_modules contains
ls -la node_modules/ | grep -i multiparty
echo "---"
# Try to find the actual multiparty library files
find node_modules -name "*multiparty*" -type d 2>/dev/null | head -5Repository: laurent22/joplin
Length of output: 64
🏁 Script executed:
#!/bin/bash
# Let's verify the path reconstruction logic - does it actually produce the same path?
python3 <<'EOF'
import os
import tempfile
from pathlib import Path
# Simulate the original and new behavior
original_path = "C:\\Users\\Laurent\\AppData\\Local\\Temp\\BW77wkpP23iIGUstd0kDuXXC.jpg" # Windows example
filename_only = Path(original_path).name
reconstructed = os.path.join(os.path.expandvars("$TEMP"), filename_only)
print("Original path:", original_path)
print("Basename:", filename_only)
print("Reconstructed:", reconstructed)
print("Match:", original_path == reconstructed)
# Now simulate on Unix/Linux
original_unix = "/tmp/BW77wkpP23iIGUstd0kDuXXC.jpg"
filename_unix = Path(original_unix).name
reconstructed_unix = os.path.join(tempfile.gettempdir(), filename_unix)
print("\nUnix case:")
print("Original path:", original_unix)
print("Basename:", filename_unix)
print("Reconstructed:", reconstructed_unix)
print("Match:", original_unix == reconstructed_unix)
EOFRepository: laurent22/joplin
Length of output: 475
🏁 Script executed:
#!/bin/bash
# Final check: does the resources endpoint have ANY test coverage for POST with files?
# Search the entire test suite
find packages -name "*.test.ts" -exec grep -l "routes.*resources\|resource.*POST\|POST.*resource" {} \; 2>/dev/null
echo "---"
# Also check Api.test.ts directly for POST resource creation
grep -n "POST.*resource\|resource.*route\|files" packages/lib/services/rest/Api.test.ts | head -20Repository: laurent22/joplin
Length of output: 415
🏁 Script executed:
#!/bin/bash
# Let's look at those POST resource tests to see if they're testing file uploads
sed -n '350,450p' packages/lib/services/rest/Api.test.tsRepository: laurent22/joplin
Length of output: 3162
🏁 Script executed:
#!/bin/bash
# Let's check what the actual test does - does it use request.files?
rg -n 'POST.*resources' packages/lib/services/rest/Api.test.ts -B 5 -A 20 | head -100Repository: laurent22/joplin
Length of output: 3183
🏁 Script executed:
#!/bin/bash
# Let's verify the assumption: does the code actually use request.files or just JSON body?
# Check the test that POSTs to resources
sed -n '350,450p' packages/lib/services/rest/Api.test.ts | cat -nRepository: laurent22/joplin
Length of output: 3869
🏁 Script executed:
#!/bin/bash
# Let's check how api.route() handles the files parameter
# Look at the route method signature
rg -n 'route.*function\|route\s*\(' packages/lib/services/rest/Api.ts -A 15 | head -80Repository: laurent22/joplin
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check the route method in Api class
sed -n '150,300p' packages/lib/services/rest/Api.ts | cat -nRepository: laurent22/joplin
Length of output: 5393
🏁 Script executed:
#!/bin/bash
# Let's find the route method implementation
rg -n 'async route\|public route\|private route' packages/lib/services/rest/Api.ts -A 20 | head -100Repository: laurent22/joplin
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if supportDir is in /tmp or elsewhere
rg -n 'supportDir\s*=' packages/lib/testing/test-utils.ts -A 3 -B 3 | head -30Repository: laurent22/joplin
Length of output: 408
🏁 Script executed:
#!/bin/bash
# Also check if there's any way the test files bypass the path.basename() logic
# Look for how request.files is actually used in the handler
grep -n 'request\.files' packages/lib/services/rest/routes/resources.tsRepository: laurent22/joplin
Length of output: 154
This "fix" introduces an undocumented assumption that could break resource uploads if violated, and lacks a regression test for the actual code path.
Two concerns:
-
Untested assumption about upload directory. The new code discards
request.files[0].pathand reconstructs it asos.tmpdir() + path.sep + fileName. This works only if the multiparty parser writes toos.tmpdir()on every platform. While multiparty's default is to use the system temp directory, the original code made no such assumption—it used whatever path the parser provided. The test suite inApi.test.tsexercises this endpoint via mock API calls that bypass the actual ClipperServer multipart parsing (lines 360, 379, 412, 434 useapi.route()directly withsupportDirfile paths, not actual multipart uploads). A genuine regression test using ClipperServer's multiparty flow is missing; without it, this change cannot be safely verified. -
The threat model is incorrect.
request.files[0].pathis set by the multiparty parser to the actual location it streamed the upload to—this is server-controlled, not user input. The user-controlled name lives inrequest.files[0].originalFilename, which is never used as a filesystem path here. The original code presented no path-traversal vulnerability. The same pattern is used innotes.ts(createResourcesFromPaths), where parser-generated paths are passed directly toshim.createResourceFromPathwithout rewriting.
If you want to add defence-in-depth, validate that the parser-provided path resolves within the configured upload directory rather than discarding it and reconstructing:
Alternative: validate without rewriting the path
- const fileName = path.basename(request.files[0].path);
- if (!fileName || !/^[\w.\-]+$/.test(fileName)) throw new ErrorBadRequest('Invalid file path');
- const filePath = os.tmpdir() + path.sep + fileName;
+ const filePath = path.resolve(request.files[0].path);
+ const uploadDir = path.resolve(os.tmpdir()) + path.sep;
+ if (!filePath.startsWith(uploadDir)) throw new ErrorBadRequest('Invalid file path');Minor: the regex /^[\w.\-]+$/ unnecessarily escapes the hyphen (\- should be - or placed at the start/end of the character class).
Before merging, add a regression test that makes an actual multipart POST to /resources via the ClipperServer (not the mock API) and asserts the resource is created successfully—the current change is not caught by any existing test.
🧰 Tools
🪛 ESLint
[error] 70-70: Unnecessary escape character: -.
(no-useless-escape)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lib/services/rest/routes/resources.ts` around lines 67 - 71, The
change in resources.ts replaces the parser-provided upload path
(request.files[0].path) with os.tmpdir()+path.sep+fileName which assumes
multiparty always uses the system temp dir and discards the server-controlled
path—restore use of the parser path and instead validate it:
resolve(request.files[0].path) and ensure it is inside the configured upload
directory (e.g., by comparing
resolved.startsWith(path.resolve(uploadDir)+path.sep)); still sanitize/validate
the user-controlled original filename (request.files[0].originalFilename) for
any uses as a target name and fix the character class to /^[\w.-]+$/ if kept;
finally add a regression test that posts a real multipart/form-data upload to
/resources through ClipperServer (not api.route) and asserts the resource is
created to cover the actual multiparty flow.
1 participant
Summary
Fix critical severity security issue in
packages/lib/services/rest/routes/resources.ts.Vulnerability
V-002packages/lib/services/rest/routes/resources.ts:53Description: At resources.ts line 65, the code directly assigns request.files[0].path to filePath without any sanitization, canonicalization, or validation. The path value originates from the multipart upload request and can contain path traversal sequences such as ../../etc/passwd or ../../etc/cron.d/malicious. No MIME type validation or file extension allowlist is confirmed. This allows an attacker to read from or write to arbitrary locations on the server filesystem.
Changes
packages/lib/services/rest/routes/resources.tsVerification
Automated security fix by OrbisAI Security