Skip to content

fix: accept absolute paths in file flags; add dry-run chat membership warning#955

Open
LawyerLyu wants to merge 2 commits into
larksuite:mainfrom
LawyerLyu:fix/absolute-path-and-dryrun-warning
Open

fix: accept absolute paths in file flags; add dry-run chat membership warning#955
LawyerLyu wants to merge 2 commits into
larksuite:mainfrom
LawyerLyu:fix/absolute-path-and-dryrun-warning

Conversation

@LawyerLyu
Copy link
Copy Markdown
Contributor

@LawyerLyu LawyerLyu commented May 19, 2026

Summary

Exception: event consume --output-dir retains its absolute-path restriction — event output files should remain within the working directory.

Changes

  • internal/vfs/localfileio/path.go: safePath() now accepts absolute paths (resolves symlinks, skips cwd restriction); relative path ../ guard unchanged
  • cmd/event/consume.go: explicit filepath.IsAbs guard added to sanitizeOutputDir to maintain its documented restriction
  • shortcuts/im/im_messages_send.go: chat-id dry-run output includes membership warning
  • All tests updated to reflect new behavior

Test plan

  • go test ./... (excluding e2e) passes with no failures
  • lark-cli im +messages-send --chat-id oc_xxx --text hello --dry-run shows NOTE about membership
  • lark-cli im +messages-reply --message-id om_xxx --image /tmp/photo.png --dry-run no longer errors on absolute path
  • lark-cli drive +pull --local-dir /home/user/docs --folder-token xxx --dry-run works with absolute path

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Absolute file and directory paths are now accepted across file operations and CLI directory flags.
    • Dry-run for IM sends adds a descriptive warning when targeting chats.
  • Bug Fixes

    • Path validation now focuses on traversal patterns and reports the relevant flag names in errors.
    • Improved symlink resolution for absolute paths and safer handling of resolved paths.

Review Change Stack

…hip warning

Fixes larksuite#872: --image, --file, --video, --audio and --local-dir flags previously
rejected absolute paths with a confusing "must be a relative path within the
current directory" error. Common CLI tools (gh, gcloud, aws s3 cp, kubectl cp)
all accept absolute paths; OS permissions enforce access when the file is
actually read. Absolute path support is now enabled in safePath() while
relative path traversal (../) protection is preserved.

Fixes larksuite#915: im +messages-send --dry-run validated request shape but gave no
indication that bot/user chat membership is not verified. A chat-id send now
includes a NOTE in dry-run output warning that the real send may still fail
with "Bot/User can NOT be out of the chat".

The --output-dir flag in event consume retains its absolute-path restriction
since event output should stay within the working directory.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added domain/ccm PR touches the ccm domain domain/im PR touches the im domain size/XL Architecture-level or global-impact change labels May 19, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 19, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fb337947-72ed-41e5-9786-5270fc0ffc11

📥 Commits

Reviewing files that changed from the base of the PR and between 157e3d3 and 54542a1.

📒 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 accepts absolute filesystem paths across validators and LocalFileIO by resolving symlinks for absolute inputs; it updates callers and many tests to reflect absolute-path acceptance, while retaining path-traversal protections. It also adds a conditional dry-run warning for chat sends.

Changes

Absolute Path Acceptance Refactoring

Layer / File(s) Summary
Core safePath implementation and import updates
cmd/event/consume.go, internal/vfs/localfileio/path.go
safePath now accepts absolute paths, resolves symlinks via resolveNearestAncestor, and returns the resolved path; event consumer imports path/filepath and checks filepath.IsAbs for --output-dir before further validation.
Path validation test updates
internal/validate/path_test.go
Tests updated: absolute paths are now accepted by SafeInputPath/SafeOutputPath; added test asserting symlink-resolved temp absolute paths are returned; flag-name error-message tests now use traversal inputs.
LocalFileIO path tests
internal/vfs/localfileio/path_test.go
Adds TestSafeOutputPath_ReturnsAbsolutePathUnchanged and updates absolute-temp-file tests to assert SafeInputPath accepts and returns symlink-resolved absolute paths; error-message tests now target traversal inputs.
LocalFileIO operation tests
internal/vfs/localfileio/localfileio_test.go
Open, Save, and ResolvePath tests now verify absolute paths are accepted and succeed; error-message consistency tests use traversal paths.
CLI command tests for absolute path acceptance
shortcuts/drive/..., shortcuts/event/processor_test.go, shortcuts/im/validate_media_test.go, shortcuts/sheets/...
Drive pull/push/sync, event route parsing, im media validation, and sheet image tests updated to accept absolute --local-dir, dir:, --file, and image paths, replacing prior rejection tests with acceptance cases; traversal still rejected.
Response handling and resolve input tests
internal/client/response_test.go, internal/cmdutil/resolve_test.go
SaveResponse test now accepts absolute output paths; ResolveInput test switched to use a traversal input case.
Sheet image tests and dry-run description enhancement
shortcuts/sheets/lark_sheets_sheet_write_image_test.go, shortcuts/im/im_messages_send.go
Sheet image tests now allow absolute image paths; ImMessagesSend.DryRun conditionally adds a descriptive note when sending to a chat.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • larksuite/cli#322: Related changes to internal/client/response_test.go and internal/vfs/localfileio/path.go accepting absolute paths and returning resolved paths.
  • larksuite/cli#395: Prior refactor relaxing path-validation/localfileio logic to accept absolute filesystem paths.
  • larksuite/cli#709: Drive shortcut tests and --local-dir validation impacted by this PR's path-safety changes.

Suggested labels

size/L

Suggested reviewers

  • jackie3927
  • kongenpei

Poem

🐰 I hopped from cwd to absolute root,
Resolved each symlink, stayed astute,
Traversals still blocked, safe paths retained,
Now temp files and /tmp are happily named,
A carrot for tests: all green and cute!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 17.07% 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 'fix: accept absolute paths in file flags; add dry-run chat membership warning' clearly and directly summarizes the two main changes: accepting absolute paths in file flags and adding a membership warning to dry-run output.
Description check ✅ Passed The PR description provides a clear summary of the two fixes, explains the motivation and impact, details the code changes across multiple files, and includes a test plan. It aligns well with the required template structure with summary, changes, and test plan sections.
Linked Issues check ✅ Passed The pull request fulfills both linked issues: #872 by accepting absolute paths while preserving traversal protection, and #915 by adding a membership warning to dry-run output for chat sends. All code changes directly address the stated objectives.
Out of Scope Changes check ✅ Passed All changes are within scope: path validation updates align with issue #872, dry-run warning aligns with #915, and event consume maintains its absolute-path restriction as documented. The singular unrelated change to im_messages_send.go's DryRun method (restructuring the fluent builder) is directly tied to adding the membership warning and is necessary to the #915 fix.

✏️ 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.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

❌ Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.77%. Comparing base (315e0ab) to head (157e3d3).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
internal/vfs/localfileio/path.go 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #955      +/-   ##
==========================================
- Coverage   66.78%   66.77%   -0.02%     
==========================================
  Files         564      564              
  Lines       52441    52449       +8     
==========================================
- Hits        35024    35022       -2     
- Misses      14516    14521       +5     
- Partials     2901     2906       +5     

☔ View full report in Codecov by Sentry.
📢 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.

@github-actions
Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@157e3d37f067a88d67e94c843711acb86183cc49

🧩 Skill update

npx skills add LawyerLyu/cli#fix/absolute-path-and-dryrun-warning -y -g

…uences

bidichk linter rejects literal U+202E (RLO), U+2066 (LRI), U+200B (ZWSP),
and U+2028 (LS) in source files. Replace with Go string escape sequences
(‮, ⁦, ​, 
) so the test coverage is unchanged.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@fangshuyu-768 fangshuyu-768 added domain/core CLI framework and core libraries and removed domain/ccm PR touches the ccm domain labels May 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/core CLI framework and core libraries domain/im PR touches the im domain size/XL Architecture-level or global-impact change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clarify that im messages-send --dry-run does not verify chat membership --image / --file flags reject absolute paths but --help doesn't document this

2 participants