Skip to content

fix(auth): tighten Windows credentials ACL (#104)#253

Open
naufalfx805-source wants to merge 2 commits into
TestSprite:mainfrom
naufalfx805-source:fix/windows-credential-permissions
Open

fix(auth): tighten Windows credentials ACL (#104)#253
naufalfx805-source wants to merge 2 commits into
TestSprite:mainfrom
naufalfx805-source:fix/windows-credential-permissions

Conversation

@naufalfx805-source

@naufalfx805-source naufalfx805-source commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Closes #104.

This stops the Windows credentials path from silently relying on POSIX chmod(0o600), which Node cannot enforce on Windows ACLs.

Changes:

  • On win32, ensureRestrictiveMode() now runs icacls with an args array: icacls <path> /inheritance:r /grant:r <USERNAME>:F.
  • The POSIX chmod path remains unchanged for non-Windows hosts.
  • If Windows ACL tightening cannot run, the CLI emits an explicit warning instead of silently pretending chmod protected the file.
  • Adds unit coverage for the Windows ACL path using injected platform/spawn dependencies, so CI can verify this from any OS.

Verification

  • npm test -- src/lib/credentials.test.ts -> 24 passed, 1 skipped
  • npm run lint
  • npm run typecheck
  • npm run build
  • git diff --check

Summary by CodeRabbit

  • Bug Fixes
    • Improved credential-file permission tightening across platforms.
    • Added Windows support to apply restrictive ACLs to the current user when creating credentials.
    • Automatically warns when restrictive permissions cannot be applied (e.g., user lookup or permission command failure).
    • Kept the existing secure permission behavior for Unix-like systems.
  • Tests
    • Added Windows-specific coverage for successful ACL tightening and warning scenarios.

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ac1af55a-9256-4219-b423-6e84bfca113f

📥 Commits

Reviewing files that changed from the base of the PR and between 06b450e and b906cf2.

📒 Files selected for processing (1)
  • src/lib/credentials.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/lib/credentials.ts

Walkthrough

Credential permission tightening now handles Windows through icacls ACL commands with injectable execution and warning callbacks, while retaining Unix chmod behavior. Tests cover successful Windows ACL invocation and failure warnings.

Changes

Credential permission enforcement

Layer / File(s) Summary
Platform-aware permission tightening
src/lib/credentials.ts
ensureRestrictiveMode accepts injectable runtime options, applies restrictive Windows ACLs through icacls, reports failures, and preserves Unix mode tightening.
Windows permission behavior tests
src/lib/credentials.test.ts
Tests verify Windows ACL arguments and warning behavior when ACL tightening cannot execute.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: zeshi-du

Sequence Diagram(s)

sequenceDiagram
  participant CredentialsWriter
  participant ensureRestrictiveMode
  participant spawnSync
  participant WarningHandler

  CredentialsWriter->>ensureRestrictiveMode: tighten credentials file permissions
  ensureRestrictiveMode->>spawnSync: execute icacls with restrictive ACL arguments
  spawnSync-->>ensureRestrictiveMode: return execution status
  ensureRestrictiveMode->>WarningHandler: warn when username detection or ACL execution fails
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.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
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: tightening Windows credential ACLs.
Linked Issues check ✅ Passed The PR tightens Windows ACLs with icacls, warns when enforcement can't run, and preserves the POSIX chmod path.
Out of Scope Changes check ✅ Passed The changes stay focused on Windows credential permission handling and related tests.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages.


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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Hackathon] Credential file is not owner-only on Windows (0o600 is a no-op there)

1 participant