fix(mail): restrict --output-dir to current working directory#376
fix(mail): restrict --output-dir to current working directory#376haidaodashushu wants to merge 7 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
domain/mail
Previously, mail +watch --output-dir accepted absolute paths (e.g. /etc, /tmp) and home directory paths (~/), allowing writes to arbitrary locations. Since mail content is sender-controlled, this posed a risk of writing attacker-influenced data to sensitive system directories. Now all --output-dir values go through validate.SafeOutputPath which: - Rejects absolute paths and ~ expansion - Resolves .. and symlinks - Enforces the result stays under CWD
Greptile SummaryThis PR fixes a path traversal/arbitrary write vulnerability in The removal of the post- Confidence Score: 5/5Safe to merge — cleanly eliminates the arbitrary-write vulnerability with no new issues introduced. All remaining items are informational. The tilde rejection, absolute-path blocking, path-traversal prevention, and symlink-escape detection are all correctly implemented and well-tested. The removal of the post-MkdirAll EvalSymlinks is intentional and correct since SafeOutputPath already returns a fully-resolved path. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["--output-dir flag value"] --> B{outputDir != ""}
B -- "empty" --> Z["skip path handling"]
B -- "non-empty" --> C{starts with ~?}
C -- "yes" --> D["❌ ErrValidation:\n'does not support ~ expansion'"]
C -- "no" --> E["validate.SafeOutputPath(outputDir)"]
E --> F{absolute path?}
F -- "yes" --> G["❌ rejected by SafeOutputPath"]
E --> H{path traversal\nor symlink escape?}
H -- "yes" --> I["❌ rejected by SafeOutputPath"]
E --> J["✅ returns resolved absolute\npath within CWD"]
J --> K["outputDir = safePath"]
K --> L["vfs.MkdirAll(outputDir, 0700)"]
L --> M["proceed with mail watching,\nwrites stay inside CWD"]
Reviews (7): Last reviewed commit: "fix(mail): remove redundant post-mkdir S..." | Re-trigger Greptile |
haidaodashushu
force-pushed
the
fix/mail-watch-output-dir-security
branch
from
3e34929 to
9e5a99d
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@716dade4fe0ce5a74ff860fa6798cc1c52d1cbda🧩 Skill updatenpx skills add larksuite/cli#fix/mail-watch-output-dir-security -y -g |
SafeOutputPath treats ~/x as a literal relative path, silently creating a directory named "~" under CWD. Reject ~ prefixed paths with a clear error message instead.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/mail/mail_watch.go`:
- Around line 199-206: The validation branch that rejects ~ and the call to
validate.SafeOutputPath currently return raw errors; wrap these user-input
validation errors with the package-level sentinel output.ErrValidation so
callers see them as validation failures. Specifically, in the block that returns
the "~ expansion" error and in the error path after calling
validate.SafeOutputPath(outputDir), replace the plain errors with wrapped errors
using output.ErrValidation (e.g., return fmt.Errorf("%w: <message or err>",
output.ErrValidation) or equivalent), preserving the original error text from
SafeOutputPath while tagging it with output.ErrValidation.
- Around line 198-199: The current check only rejects exactly "~" and "~/..."
but misses other tilde-prefixed forms; update the conditional in mail_watch.go
that inspects outputDir (the same place that later calls SafeOutputPath) to
reject any outputDir that begins with a tilde character (i.e., when the first
rune == '~' or strings.HasPrefix(outputDir, "~")), including forms with
backslashes and user expansions like "~user/..." or "~\output"; when detected,
return the same fmt.Errorf("--output-dir does not support ~ expansion; use a
relative path like ./output instead").
🪄 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: f724bfc9-a084-4e72-9e0c-6a9bca03b9bb
📒 Files selected for processing (1)
shortcuts/mail/mail_watch.go
- Broaden ~ check from "~ || ~/" to "~" prefix, covering ~user/path forms - Use output.ErrValidation for consistent error type (exit code 2)
SafeOutputPath validates before MkdirAll, but an attacker could replace the newly created directory with a symlink between mkdir and the first write. Add EvalSymlinks after MkdirAll and re-verify the resolved path is still under CWD. Also broaden ~ rejection to all tilde-prefixed paths (~user/path) and use output.ErrValidation for consistent error types.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shortcuts/mail/mail_watch.go (1)
204-207:⚠️ Potential issue | 🟠 MajorWrap
SafeOutputPathfailures as validation errors.At Line 206, returning raw
errcan lose validation error typing/exit semantics for bad user input. Keep this branch consistently tagged asoutput.ErrValidation.Suggested fix
safePath, err := validate.SafeOutputPath(outputDir) if err != nil { - return err + return output.ErrValidation("unsafe --output-dir: %s", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_watch.go` around lines 204 - 207, The call to validate.SafeOutputPath currently returns the raw err (safePath, err := validate.SafeOutputPath(outputDir) ... return err), which loses validation error typing; change the error return to wrap the underlying error with output.ErrValidation (e.g., return fmt.Errorf("%w: %v", output.ErrValidation, err)) so callers can detect validation failures; update imports to include fmt if needed and apply this wrapping wherever validate.SafeOutputPath's error is returned in the mail_watch.go flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/mail/mail_watch.go`:
- Around line 216-227: The code currently calls filepath.EvalSymlinks and
os.Getwd (around the outputDir handling) which violates the repository's
forbidigo rules; remove the direct filepath.EvalSymlinks and os.Getwd calls and
instead, after creating the directory (the existing MkdirAll step), re-validate
the original raw output path using the repository helper validate.SafeOutputPath
(preserving the TOCTOU re-check). Concretely: stop resolving symlinks into
resolved/canonicalCwd and computing rel; after MkdirAll for outputDir, call
validate.SafeOutputPath(outputDir) (or the appropriate validate function used in
this repo) and return an output.ErrValidation on failure, referencing the same
output.ErrValidation message flow as before.
---
Duplicate comments:
In `@shortcuts/mail/mail_watch.go`:
- Around line 204-207: The call to validate.SafeOutputPath currently returns the
raw err (safePath, err := validate.SafeOutputPath(outputDir) ... return err),
which loses validation error typing; change the error return to wrap the
underlying error with output.ErrValidation (e.g., return fmt.Errorf("%w: %v",
output.ErrValidation, err)) so callers can detect validation failures; update
imports to include fmt if needed and apply this wrapping wherever
validate.SafeOutputPath's error is returned in the mail_watch.go flow.
🪄 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: fb459e77-60cc-4f0a-9515-867014ba3c2e
📒 Files selected for processing (1)
shortcuts/mail/mail_watch.go
Replace direct os.Getwd and filepath.EvalSymlinks calls with a second SafeOutputPath call after MkdirAll. This satisfies the forbidigo lint rule (no direct os/filepath calls in shortcuts/) while maintaining the same TOCTOU protection.
SafeOutputPath rejects absolute paths, but after the first call outputDir was already resolved to an absolute path. Pass the original relative path to the second SafeOutputPath call so it can properly re-validate after MkdirAll.
The second SafeOutputPath call after MkdirAll provided no real TOCTOU protection: mail +watch is long-running, so the directory could be replaced at any point during the session, not just between mkdir and the check. The first SafeOutputPath already validates and resolves the path; one call is sufficient.
chanthuang
left a comment
chanthuang
left a comment
There was a problem hiding this comment.
LGTM. 核心安全修复完整：~ 显式拒绝 + 所有路径统一走 SafeOutputPath，代码比之前更简洁。合入时建议 squash commits。
2 participants
Summary
~expansion support frommail +watch --output-dirvalidate.SafeOutputPath, enforcing CWD containmentSecurity issue
--output-dirpreviously accepted absolute paths (e.g./etc,/tmp/evil) and home directory expansion (~/). Since mail content is sender-controlled, this allowed potential writes of attacker-influenced data to sensitive locations.Test plan
/tmp/evil) → rejected~/evil) → rejected./../../etc) → rejected./mail-output/../../../) → rejected./mail-output) → allowedSummary by CodeRabbit