Skip to content

fix(file-transfer): expand Windows-style tilde policy globs#101698

Closed
VectorPeak wants to merge 2 commits into
openclaw:mainfrom
VectorPeak:fix/file-transfer-windows-tilde-v2
Closed

fix(file-transfer): expand Windows-style tilde policy globs#101698
VectorPeak wants to merge 2 commits into
openclaw:mainfrom
VectorPeak:fix/file-transfer-windows-tilde-v2

Conversation

@VectorPeak

@VectorPeak VectorPeak commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes a Windows file-transfer policy edge case where home-relative globs written with Windows path separators, such as ~\Downloads\**, were treated as literal glob text instead of being expanded to the user's home directory.

The file-transfer policy already supports home-relative patterns through ~/... and bare ~. However, Windows users naturally tend to write local paths with backslashes, and ~\... looked like the Windows equivalent of ~/... in configuration. Before this change, that form was not recognized by expandTilde(), so the policy matcher received a pattern that still began with a literal ~\ prefix.

That created two confusing outcomes:

  • allowReadPaths or allowWritePaths entries such as ~\Downloads\** could fail to match files that are actually under the user's home directory.
  • denyPaths entries such as ~\Downloads\**\*.pem could fail to enforce the intended hard-deny rule, because the deny glob never resolved to the real home-directory path.

In practice, a Windows operator could configure a policy that appears to allow or deny the correct local folder, while file-transfer policy evaluation behaves differently because only the forward-slash tilde form was expanded.

Why This Change Was Made

This change extends the existing tilde expansion helper to recognize ~\ as the Windows-style equivalent of ~/.

The fix is intentionally small and stays inside the shared policy normalization path. normalizeGlobs() is already used before matching allowReadPaths, allowWritePaths, and denyPaths, so updating expandTilde() gives all three policy surfaces the same home-relative behavior without duplicating logic or changing the matcher itself.

Existing behavior is preserved for:

  • ~/... home-relative globs
  • bare ~
  • absolute paths
  • normal minimatch semantics
  • Windows path separator normalization during matching
  • deny-before-allow policy precedence

The change only makes a previously unexpanded Windows-style home prefix behave like the already-supported POSIX-style home prefix. This keeps the policy model consistent while avoiding a broader rewrite of path handling or glob evaluation.

User Impact

Windows users can now configure file-transfer policy globs using familiar Windows separators, for example:

~\Downloads\**

and have them resolve against the same home directory target as:

~/Downloads/**

This also makes Windows-style hard-deny rules behave as configured, for example ~\Downloads\**\*.pem now denies matching files under the user's Downloads directory instead of remaining a non-matching literal pattern.

Live Windows Behavior Proof

Redacted live proof was run on Windows against the production file-transfer policy evaluator, not a mock or an isolated helper test. The proof calls evaluateFilePolicy(), which is the shared policy path used by file-transfer node-invoke checks before file fetch/list/write execution. That path normalizes configured globs through normalizeGlobs(), expands home prefixes through expandTilde(), and then applies deny-before-allow matching.

Windows file-transfer policy live proof
platform=win32
node=v24.15.0
home=<USER_HOME>
config.allowReadPaths=["~\\Downloads\\**"]
config.denyPaths=["~\\Downloads\\**\\*.pem"]
case=ALLOW Windows-style home glob
path=<USER_HOME>\Downloads\shot.png
decision=matched-allow
ok=true
case=DENY Windows-style hard-deny glob
path=<USER_HOME>\Downloads\key.pem
decision=POLICY_DENIED
ok=false

This proves both requested behaviors from a real Windows run:

  • ~\Downloads\** resolves as a home-relative allow glob and permits a matching non-denied file.
  • ~\Downloads\**\*.pem resolves as a home-relative deny glob and still wins over the broader allow rule.

Compatibility / Security-Boundary Note

This PR intentionally changes how existing file-transfer policy strings beginning with ~\ are interpreted. Before this fix, those entries were effectively literal non-matches; after this fix, they become active home-relative policy globs.

That is the intended Windows policy interpretation: a stored config entry such as ~\Downloads\** already expresses operator intent to target the user's Downloads directory. The change does not add a new wildcard class or bypass deny precedence; it only makes the Windows-style home prefix behave like the already-supported ~/ home prefix.

Because this is a file-access policy surface, the PR includes both allow and hard-deny proof. The deny proof demonstrates that broad Windows-style allow rules still remain constrained by denyPaths after expansion.

Evidence

Live proof command:

PNPM_CONFIG_MODULES_DIR=D:\ZXY\Github\openclaw\node_modules node --import tsx --input-type=module <redacted inline policy proof script>

Focused validation passed using the repository Vitest runner with hydrated dependencies:

PNPM_CONFIG_MODULES_DIR=D:\ZXY\Github\openclaw\node_modules node scripts/run-vitest.mjs run extensions/file-transfer/src/shared/policy.test.ts

Test Files  1 passed (1)
Tests       32 passed (32)

Diff hygiene also passed for the touched files:

git diff --check -- extensions/file-transfer/src/shared/policy.ts extensions/file-transfer/src/shared/policy.test.ts

The added regression cases cover both directions:

allowReadPaths: ["~\\Downloads\\**"]
path: <home>/Downloads/shot.png
result: matched allow

denyPaths: ["~\\Downloads\\**\\*.pem"]
path: <home>/Downloads/key.pem
result: POLICY_DENIED

AI-assisted: Yes, prepared with Codex GPT-5.5 xhigh review assistance.

@openclaw-barnacle openclaw-barnacle Bot added plugin: file-transfer size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 7, 2026
@clawsweeper

clawsweeper Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 16, 2026, 7:18 AM ET / 11:18 UTC.

Summary
The branch makes file-transfer policy globs beginning with ~\ expand relative to the home directory and adds read-allow and hard-deny regression tests.

PR surface: Source 0, Tests +29. Total +29 across 2 files.

Reproducibility: yes. at source level. The current helper demonstrably leaves ~\ unexpanded, while the supplied Windows production-evaluator output proves the corrected allow and deny outcomes.

Review metrics: 1 noteworthy metric.

  • Policy syntax reinterpretation: 1 existing prefix form activated. Stored ~\... entries move from non-matching text to active file-access authorization rules.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Obtain file-transfer owner confirmation that existing ~\... policy entries should become active authorization rules.

Risk before merge

  • [P1] Existing stored policy strings beginning with ~\ change from literal non-matches into active allow or deny rules during upgrade.
  • [P1] Because the altered interpretation governs file access, maintainers must explicitly confirm that Windows-style tilde syntax is an intended authorization contract rather than an unsupported configuration form.

Maintainer options:

  1. Confirm and land the syntax contract (recommended)
    Explicitly accept ~\ as supported home-relative policy syntax and merge with the existing allow and deny proof.
  2. Keep the existing interpretation
    Pause or close the PR and document that home-relative policy globs must use ~/.

Next step before merge

  • No automated repair is needed; the remaining action is owner acceptance or rejection of the authorization-policy reinterpretation.

Maintainer decision needed

  • Question: Should existing file-transfer policy entries beginning with ~\ become active home-relative authorization rules?
  • Rationale: The code is mechanically sound and well-proven, but automation cannot decide whether previously non-matching stored authorization strings should gain this supported meaning.
  • Likely owner: steipete — He is the strongest available decision owner from the canonical file-transfer authorization review and merge history.
  • Options:
    • Accept Windows tilde syntax (recommended): Treat ~\ as equivalent to ~/ and explicitly accept the stored-policy reinterpretation.
    • Require portable slash syntax: Keep ~\ literal and document ~/ as the only supported home-relative policy form.

Security
Cleared: The patch adds no dependency or supply-chain surface and preserves hard-deny precedence; the intentional authorization reinterpretation is captured as maintainer merge risk rather than a code defect.

Review details

Best possible solution:

Define ~\ as supported home-relative file-transfer policy syntax, retain the shared normalization path and allow/deny regression coverage, and land only after the file-transfer owner confirms that contract.

Do we have a high-confidence way to reproduce the issue?

Yes at source level. The current helper demonstrably leaves ~\ unexpanded, while the supplied Windows production-evaluator output proves the corrected allow and deny outcomes.

Is this the best way to solve the issue?

Yes, conditionally. Updating the existing shared normalization helper is the narrowest maintainable fix; only acceptance of the stored-policy interpretation remains unresolved.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against d5eebf02e238.

Label changes

Label justifications:

  • P2: This is a bounded Windows-specific policy bug with limited plugin scope but meaningful authorization semantics.
  • merge-risk: 🚨 compatibility: Existing stored ~\... policy entries change meaning without a migration or deprecation window.
  • merge-risk: 🚨 security-boundary: The new interpretation can activate allow and deny rules that govern node file access.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): Redacted Windows live output exercises the production evaluator and shows both the newly matching allow rule and the hard-deny result after the change.
  • proof: sufficient: Contributor real behavior proof is sufficient. Redacted Windows live output exercises the production evaluator and shows both the newly matching allow rule and the hard-deny result after the change.
Evidence reviewed

PR surface:

Source 0, Tests +29. Total +29 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 1 1 0
Tests 1 29 0 +29
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 30 1 +29

What I checked:

Likely related people:

  • omarshahine: The canonical file-transfer implementation history attributes the policy evaluator and early hardening work to Omar Shahine. (role: introduced behavior; confidence: high; commits: a25d59ccbe61, c2d127feb169, d578a68a7de6; files: extensions/file-transfer/src/shared/policy.ts, extensions/file-transfer/src/shared/node-invoke-policy.ts)
  • steipete: The original file-transfer authorization work was maintainer-sponsored and merged through review activity from steipete, making him the strongest available owner for the policy-contract choice. (role: merger and likely decision owner; confidence: high; commits: fb6ca67, b712aa4, 3d9ff00; files: extensions/file-transfer/src/shared/policy.ts, extensions/file-transfer/src/shared/node-invoke-policy.ts, src/gateway/server-methods/nodes.ts)
  • vincentkoc: Recent current-main refactoring touched the file-transfer policy module and adjacent plugin type surface. (role: recent area contributor; confidence: medium; commits: 407443264c11, fdc7892a6e5a; files: extensions/file-transfer/src/shared/policy.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (5 earlier review cycles)
  • reviewed 2026-07-07T13:57:04.615Z sha 9659e19 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-07T14:10:19.624Z sha 9659e19 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-07T14:25:50.674Z sha 90919ff :: needs maintainer review before merge. :: none
  • reviewed 2026-07-07T14:48:52.487Z sha 90919ff :: needs maintainer review before merge. :: none
  • reviewed 2026-07-07T15:07:26.341Z sha 283ca30 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels Jul 7, 2026
@VectorPeak

VectorPeak commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

I added the requested redacted live Windows file-transfer proof to the PR body.

Summary of the added proof:

  • Windows runtime: platform=win32
  • Production policy path: evaluateFilePolicy() from extensions/file-transfer/src/shared/policy.ts
  • ~\Downloads\** allowed <USER_HOME>\Downloads\shot.png with decision=matched-allow
  • ~\Downloads\**\*.pem denied <USER_HOME>\Downloads\key.pem with decision=POLICY_DENIED
  • Added compatibility/security-boundary note explaining that existing stored ~\... policy entries intentionally become active home-relative globs after this fix.

@clawsweeper

clawsweeper Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@VectorPeak

VectorPeak commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Re-triggering after rewriting the PR body as UTF-8 without a leading BOM so the proof checker can parse the authored ## What Problem This Solves section. The body now includes the redacted Windows live proof, compatibility/security-boundary note, focused tests, and diff hygiene.

@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 7, 2026
@VectorPeak
VectorPeak force-pushed the fix/file-transfer-windows-tilde-v2 branch from 9659e19 to 90919ff Compare July 7, 2026 14:15
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 7, 2026
@VectorPeak

VectorPeak commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Branch refreshed on latest upstream/main and the PR body now ties the live Windows proof directly to the production evaluateFilePolicy() path:

  • latest head: 90919fface4263aa64a703d9a8620d089506d6e3
  • local focused validation passed: node scripts/run-vitest.mjs run extensions/file-transfer/src/shared/policy.test.ts (32 passed)
  • git diff --check -- extensions/file-transfer/src/shared/policy.ts extensions/file-transfer/src/shared/policy.test.ts passed
  • latest Real behavior proof check is passing
  • the remaining compatibility/security-boundary risk is now explicitly documented as the maintainer decision to accept ~\... as Windows home-relative file-transfer policy syntax

Two current red checks (label and security-sensitive-guard) both fail while reading PR labels via GitHub API with the same 422 (Could not resolve to a node with the global id...); they do not report a patch/test failure. I attempted to rerun failed jobs, but GitHub requires repository admin rights for that rerun.

@clawsweeper

clawsweeper Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@BradGroux BradGroux mentioned this pull request Jul 8, 2026
Draft
@VectorPeak
VectorPeak force-pushed the fix/file-transfer-windows-tilde-v2 branch from 283ca30 to c682efe Compare July 16, 2026 09:28
@VectorPeak @chatgpt-codex-connector
fix(file-transfer): expand Windows-style tilde globs
48ad39f 
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
@VectorPeak
VectorPeak force-pushed the fix/file-transfer-windows-tilde-v2 branch from c682efe to 48ad39f Compare July 16, 2026 10:12
@VectorPeak

VectorPeak commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Closing this for now since it has not received human maintainer/reviewer engagement. Happy to revisit or reopen if maintainers want to take this direction forward.

@VectorPeak VectorPeak closed this Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. plugin: file-transfer proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@VectorPeak