Skip to content

fix(vfs): reject Windows absolute paths cross-platform#1401

Merged
liangshuo-1 merged 2 commits into
larksuite:mainfrom
xu91102:fix/vfs-cross-platform-absolute-paths
Jun 13, 2026
Merged

fix(vfs): reject Windows absolute paths cross-platform#1401
liangshuo-1 merged 2 commits into
larksuite:mainfrom
xu91102:fix/vfs-cross-platform-absolute-paths

Conversation

@xu91102

@xu91102 xu91102 commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Background

Path safety validation is intended to reject absolute paths for user-provided input/output paths. The current check only used filepath.IsAbs, which is platform-specific. On Unix/macOS it does not treat Windows-style absolute paths such as C:\Users\agent\secret.txt, C:/Users/agent/secret.txt, \Users\agent\secret.txt, or \\server\share\secret.txt as absolute, so those values could pass the initial absolute-path rejection and be interpreted as relative paths.

Changes

  • Add a platform-independent absolute-path detector in internal/vfs/localfileio.safePath.
  • Reject Unix absolute paths, Windows drive-rooted paths, rooted backslash paths, and UNC paths before cleaning/joining with the current working directory.
  • Add regression cases for Windows-style absolute paths in both SafeOutputPath and SafeInputPath.

Verification

  • Reproduced before fix: go test -timeout=2m ./internal/vfs/localfileio -run '^TestSafeOutputPath_RejectsPathTraversalAndDangerousInput$' failed for Windows absolute path cases.
  • go test -count=1 -timeout=2m ./internal/vfs/localfileio -run '^TestSafeOutputPath_RejectsPathTraversalAndDangerousInput$': passed
  • go test -count=1 -timeout=2m ./internal/vfs/localfileio -run 'TestSafe(Output|Upload)Path_Rejects': passed
  • go test -count=1 -timeout=3m ./internal/vfs/localfileio: passed
  • go test -count=1 -timeout=4m ./internal/vfs/... ./internal/validate: passed
  • go test -count=1 -timeout=2m ./shortcuts/im -run '^TestValidateMediaFlagPath$': passed
  • gofmt -l internal/vfs/localfileio/path.go internal/vfs/localfileio/path_test.go: no output
  • git diff --check origin/main...HEAD: passed

Risk and rollback

Risk is low and constrained to path validation. The behavior becomes stricter for Windows-style absolute paths on all platforms, matching existing CLI path-safety messaging. Rollback is reverting this PR.

Summary by CodeRabbit

  • Bug Fixes
    • Strengthened validation for --output and --file to more reliably reject absolute and Windows drive-qualified paths, including UNC and root-style inputs.
    • Improved handling of whitespace or oddly formatted path inputs so they are correctly treated as invalid when absolute.
    • Clarified error messaging when a provided path is rejected to better guide corrective action.

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 56230600-e2b9-4fd8-be7c-953e76ab939e

📥 Commits

Reviewing files that changed from the base of the PR and between a5118dd and b891900.

📒 Files selected for processing (1)
  • internal/vfs/localfileio/path_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/vfs/localfileio/path_test.go

📝 Walkthrough

Walkthrough

This PR hardens path validation in the VFS localfileio package by adding early detection of absolute paths. A new isAbsolutePath helper identifies absolute paths (including Windows drive formats), and safePath invokes this check before path normalization. Tests were extended to cover Windows-style absolute paths.

Changes

Absolute Path Validation

Layer / File(s) Summary
Absolute path detection and safePath integration
internal/vfs/localfileio/path.go
New isAbsolutePath helper detects leading /, leading \, or Windows drive patterns (colon-separated drive letter). safePath now calls isAbsolutePath on the raw input before filepath.Clean to reject absolute/drive-qualified paths early with an updated error message.
Windows absolute path test coverage
internal/vfs/localfileio/path_test.go
Test cases added and reorganized into table-driven tests to verify rejection of Windows drive-letter absolute paths (e.g., C:\ and C:/), rooted backslash paths, and UNC paths for both output and upload/safe input validations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

size/M

Suggested reviewers

  • liangshuo-1

Poem

🐰 I sniffed the path and gave a hop,
Absolute drives make me stop,
With slashes, backslashes, letters too,
I check the start before I clean through,
Small and safe — that's what we do.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: rejecting Windows absolute paths in a cross-platform manner within the VFS module.
Description check ✅ Passed The description is well-structured with Background, Changes, Verification, and Risk sections that thoroughly explain the motivation, implementation, testing approach, and risk assessment.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added the size/M Single-domain feat or fix with limited business impact label Jun 11, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/vfs/localfileio/path_test.go`:
- Around line 37-40: The test currently adds Windows-absolute cases only for
SafeOutputPath; add the same four cases for SafeInputPath as well to cover the
shared safePath behavior. Update the table of test cases in path_test.go so that
for each of the entries {"absolute path windows drive",
`C:\Users\agent\secret.txt`, true}, {"absolute path windows drive slash",
"C:/Users/agent/secret.txt", true}, {"absolute path windows rooted",
`\Users\agent\secret.txt`, true}, {"absolute path windows unc",
`\\server\share\secret.txt`, true} you create equivalent assertions invoking
SafeInputPath (in addition to the existing SafeOutputPath checks) so both
SafeInputPath and SafeOutputPath are validated against the regression in
safePath.
🪄 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: 967361ce-4458-4ecc-8b5d-463a74bb044a

📥 Commits

Reviewing files that changed from the base of the PR and between e64610f and e68cecc.

📒 Files selected for processing (2)
  • internal/vfs/localfileio/path.go
  • internal/vfs/localfileio/path_test.go

Comment thread internal/vfs/localfileio/path_test.go
@CLAassistant

CLAassistant commented Jun 11, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@xu91102
xu91102 force-pushed the fix/vfs-cross-platform-absolute-paths branch from a5118dd to b891900 Compare June 11, 2026 08:17
@codecov

codecov Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.75%. Comparing base (e64610f) to head (b891900).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
internal/vfs/localfileio/path.go 83.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1401   +/-   ##
=======================================
  Coverage   72.75%   72.75%           
=======================================
  Files         730      730           
  Lines       69034    69044   +10     
=======================================
+ Hits        50228    50236    +8     
- Misses      15034    15035    +1     
- Partials     3772     3773    +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@liangshuo-1
liangshuo-1 merged commit 751092c into larksuite:main Jun 13, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants