Repository navigation
fix(security): add path containment to tenant export (#136-137) - #849
Conversation
…#136-137 storagePath from the database was used in path.join() without validation, allowing path traversal if a DB record is compromised. Now uses resolvePathWithinSync() from @pagespace/lib/security to validate both source and destination paths. Also hardens validateChecksums() in migration-utils.ts against crafted manifest bundles. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds path-traversal protection to tenant export and checksum validation by resolving and validating storage paths before accessing or exporting files; unsafe paths are skipped and reported as invalid in checksum validation. Tests cover malicious storagePath entries to ensure they're excluded or flagged. Changes
Sequence Diagram(s)mermaid Export->>Resolver: resolve srcPath (fileStorage, storagePath) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/tenant-export.ts`:
- Line 22: The import and uses of resolvePathWithinSync are vulnerable to
symlink escapes; replace the sync resolver with the async symlink-aware resolver
(resolvePathWithin) from '@pagespace/lib/security' and update the two call sites
that validate targets before copyFile/checksum operations (the checks around the
copyFile and checksum logic) to await resolvePathWithin instead of using
resolvePathWithinSync so path containment is enforced against symlinks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 232cf7ac-6c34-465c-85cb-6fc58d04bc14
📒 Files selected for processing (3)
scripts/__tests__/tenant-export.test.tsscripts/lib/migration-utils.tsscripts/tenant-export.ts
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…tainment Switch from resolvePathWithinSync to resolvePathWithin in tenant-export and migration-utils. The async resolver verifies symlinks via realpath(), preventing symlink escapes that the sync version cannot detect. Also adds a validateChecksums path-traversal test to tenant-import tests, verifying that crafted manifest entries with traversal paths are flagged as INVALID_PATH. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
storagePathfrom the database was used inpath.join()without containment validation inscripts/tenant-export.ts, enabling path traversal if a DB record is compromised (e.g.storagePath = '../../etc/passwd')resolvePathWithin()from@pagespace/lib/security— the same async, symlink-aware utility used throughout the processor/upload pipelinevalidateChecksums()inscripts/lib/migration-utils.tsagainst crafted manifest bundles (defense-in-depth)Files Changed
scripts/tenant-export.ts— async path containment on storagePath before file read/write (symlink-aware via realpath)scripts/lib/migration-utils.ts— async path containment on entry.path in validateChecksums()scripts/__tests__/tenant-export.test.ts— 2 new integration tests for traversal rejectionscripts/__tests__/tenant-import.test.ts— 1 new test for validateChecksums INVALID_PATH on traversalFollow-up
scripts/tenant-validate.tslines 201-202 has the identical vulnerability — separate PRTest plan
../../etc/passwdand../../../etc/shadowstoragePaths are rejectedtest_file_blob_001/data.txt) still worksresolvePathWithinfor symlink escape protection (per CodeRabbit review)🤖 Generated with Claude Code
Summary by CodeRabbit