fix(binding): skip unix mode audit on windows

[Zhang-986]

Summary

Fix Windows exec provider binding failures caused by treating synthesized POSIX mode bits as real NTFS permissions. On Windows, Go reports writable regular files as mode 0666 based on file attributes, which made secure exec providers fail the world-writable audit even when the file was not broadly writable by ACL.

Changes

• Keep the existing absolute path, non-directory, trusted directory, and symlink checks for secure paths.
• Skip the Unix group/world permission-bit audit on Windows, where those bits do not represent ACL permissions.
• Add a regression test for the Windows synthetic 0666 mode case reported by lark-channel-bridge exec provider wrappers.
• Trade-off: on Windows the inapplicable POSIX mode audit is skipped, while absolute-path, non-directory, symlink, and trusted-dir checks still run; real NTFS ACL auditing can be a follow-up enhancement.

Test Plan

• Unit tests pass: GOTOOLCHAIN=go1.23.12 make unit-test
• Targeted secure-path tests: go test ./internal/binding -run TestAssertSecurePath -count=1
• Related packages: go test ./internal/binding ./internal/transport -count=1
• Race check: go test -race ./internal/binding -count=1
• Windows compile check, including the Windows-only regression test: GOOS=windows GOARCH=amd64 go test -c ./internal/binding -o /tmp/larksuite-cli-binding.test.exe
• Static checks: go vet ./..., gofmt -l ., git diff --check
• Lint on this branch diff: go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=upstream/main
• Manual Windows CLI flow was not run locally; this change is covered by secure-path unit tests and Windows test binary compilation.

Related Issues

• Fixes Windows 平台 exec provider 安全审计误判导致 lark-cli 绑定失败 #1475

Summary by CodeRabbit

• Bug Fixes

  • Fixed file permission validation on Windows to skip POSIX permission mode checks, which don't apply to Windows filesystems. Windows now only verifies file accessibility.
  • Unix systems continue to enforce strict permission validation for secure file access.

• Tests

  • Added test coverage for Windows file permission handling.

[coderabbitai]

📝 Walkthrough

Walkthrough

Splits auditFilePermissions from a single runtime-branched function into two build-tag-separated files: audit_unix.go enforces POSIX world/group writable and readable mode checks, while audit_windows.go performs only a vfs.Stat existence check and skips all POSIX mode-bit auditing. The shared audit.go has its runtime.GOOS variable and Windows branch removed. A new Windows-tagged test validates the bypass behavior.

Changes

Windows POSIX permission audit bypass

Layer / File(s)	Summary
Platform-split auditFilePermissions implementation internal/binding/audit.go, internal/binding/audit_unix.go, internal/binding/audit_windows.go	Removes the auditRuntimeGOOS runtime variable and Windows early-return branch from the shared audit.go; introduces audit_unix.go that enforces POSIX world/group writable and readable bit checks via vfs.Stat, and audit_windows.go that stat-checks the path and returns a wrapped error on failure while skipping all POSIX mode checks.
Windows bypass test internal/binding/audit_windows_test.go	Adds TestAssertSecurePath_WindowsIgnoresSyntheticUnixPermissionBits (Windows build tag), which writes a temp .cmd file with Unix-style permission bits and asserts AssertSecurePath succeeds with the resolved path.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 On Windows, the bits told a fib,
"World-writable!" cried the audit lib.
Now two files stand, by build tag split—
Unix checks modes, Windows just stat.
No more false alarms for .cmd wit! 🎉

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: skipping Unix mode audits on Windows.
Description check	✅ Passed	The description includes all required template sections (Summary, Changes, Test Plan, Related Issues) with comprehensive details and completed test checkboxes.
Linked Issues check	✅ Passed	The PR fully addresses issue #1475 by implementing the recommended fix: skipping Unix group/world permission-bit audits on Windows while preserving other security checks.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the Windows exec provider security audit issue; no out-of-scope modifications detected.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

_{✏️ 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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Pull request overview

Fixes Windows exec-provider binding failures by avoiding Unix permission-bit audits on Windows, where Go’s synthesized POSIX mode bits don’t reflect NTFS ACL permissions.

Changes:

• Skip group/world permission-bit checks in AssertSecurePath on Windows while retaining other secure-path checks (absolute path, not-a-directory, trusted dir, symlink handling, owner UID logic).
• Add a regression test covering the “synthetic 0666” Windows-mode scenario.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
internal/binding/audit.go	Skips Unix permission-bit audit on Windows to prevent false “world-writable” failures.
internal/binding/audit_test.go	Adds a regression test to ensure Windows (or simulated Windows) ignores synthetic POSIX mode bits.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[evandance]

Thanks for the fix — the diagnosis is right: on Windows Go synthesizes mode bits from the read-only attribute, so the world-writable check misfires; the early return is scoped to just the permission-bit audit, and the non-Windows path and the new regression test both look correct.

One change before merge: this package already expresses its other Unix-only check (file ownership) as a build-tagged no-op on Windows (audit_unix.go / audit_windows.go). Please land this POSIX-mode-audit skip the same way — as OS-compiled functions — rather than a runtime GOOS check plus a test-only package-level variable in the shared file. In a security-audit module, keeping the OS branch compile-time and avoiding a mutable global is more consistent with the existing structure.

Please also add a line to the PR description noting the trade-off: on Windows the inapplicable POSIX mode audit is skipped, but the absolute-path, non-directory, symlink, and trusted-dir checks still run; real NTFS ACL auditing can be a follow-up enhancement.

With those two addressed, I have no other blocking comments at the code level; merge will still need a sync with main and a green CI run.

[github-actions]

🚀 PR Preview Install Guide

🧰 CLI update

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

🧩 Skill update

npx skills add Zhang-986/cli#codex/windows-exec-audit-permissions -y -g

[Zhang-986]

Addressed the requested changes:

• Moved the POSIX mode audit into build-tagged OS functions, matching the existing owner UID split.
• Removed the runtime GOOS branch and test-only mutable package variable from the shared audit file.
• Added a Windows-only regression test for the synthetic mode-bit case.
• Updated the PR description with the Windows trade-off note.
• Rebases the PR branch onto current upstream/main.

Local checks run:

• go test ./internal/binding -run TestAssertSecurePath -count=1
• go test ./internal/binding ./internal/transport -count=1
• go test -race ./internal/binding -count=1
• GOOS=windows GOARCH=amd64 go test -c ./internal/binding -o /tmp/larksuite-cli-binding.test.exe
• git diff --check

[coderabbitai]

Actionable comments posted: 2

🤖 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/binding/audit_unix.go`:
- Around line 36-40: The error handling in the auditFilePermissions function
when vfs.Stat fails uses fmt.Errorf which downgrades error classification and
loses type information. Replace the fmt.Errorf call that wraps the vfs.Stat
error with errs.NewInternalError(errs.SubtypeFileIO, ...) to use a typed error,
and chain the original error using .WithCause(err) to preserve the underlying
cause while maintaining proper error classification for command-facing
validation.

In `@internal/binding/audit_windows.go`:
- Around line 21-24: The error handling in the auditFilePermissions function on
the vfs.Stat call is using fmt.Errorf instead of the typed error contract
required for command-facing failures. Replace the fmt.Errorf call with
errs.NewInternalError(errs.SubtypeFileIO, ...) to provide a descriptive message,
and chain it with .WithCause(err) to preserve the underlying stat error for
debugging purposes.

🪄 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: 752f8099-282f-4902-a408-d61a6681d386

📥 Commits

Reviewing files that changed from the base of the PR and between 373da58 and f114038.

📒 Files selected for processing (4)

• internal/binding/audit.go
• internal/binding/audit_unix.go
• internal/binding/audit_windows.go
• internal/binding/audit_windows_test.go

💤 Files with no reviewable changes (1)

• internal/binding/audit.go

✅ Files skipped from review due to trivial changes (1)

• internal/binding/audit_windows_test.go

[evandance]

Thanks for the quick reshape — the build-tag split is exactly right and the non-Windows path is unchanged. LGTM.