Skip to content

fix(security): restrict permissions on service tmp dir and log file#2502

Open
ColinM-sys wants to merge 2 commits intoNVIDIA:mainfrom
ColinM-sys:fix/services-tmp-dir-permissions
Open

fix(security): restrict permissions on service tmp dir and log file#2502
ColinM-sys wants to merge 2 commits intoNVIDIA:mainfrom
ColinM-sys:fix/services-tmp-dir-permissions

Conversation

@ColinM-sys
Copy link
Copy Markdown
Contributor

@ColinM-sys ColinM-sys commented Apr 27, 2026

`/tmp/nemoclaw-services-{name}/` was created with the default 0o755
mode, making `cloudflared.log` world-readable. The log contains the
public `*.trycloudflare.com` tunnel URL — any local user on a
multi-user system could read it and use the URL to probe the dashboard
from anywhere on the internet.

Set the directory to 0o700 and the log file to 0o600 so only the
owning user can access them.

Signed-off-by: ColinM-sys cmcdonough@50words.com

Summary by CodeRabbit

  • Bug Fixes
    • PID directories and per-service log files are now created with explicit restrictive filesystem permissions to prevent unauthorized access.
    • Permission settings are applied deterministically (not relying on system defaults) to ensure the intended access restrictions are always enforced.

/tmp/nemoclaw-services-{name}/ was created with the default 0o755 mode,
making cloudflared.log world-readable. The log contains the public
*.trycloudflare.com tunnel URL, which any local user on a multi-user
system could read and use to probe the dashboard.

Set the directory to 0o700 and the log file to 0o600 so only the
owning user can access them.

Signed-off-by: ColinM-sys <cmcdonough@50words.com>
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented Apr 27, 2026

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 22317738-4371-4c12-93e6-78badc6a919f

📥 Commits

Reviewing files that changed from the base of the PR and between 903ba55 and a47c85b.

📒 Files selected for processing (1)
  • src/lib/services.ts
✅ Files skipped from review due to trivial changes (1)
  • src/lib/services.ts

📝 Walkthrough

Walkthrough

The PR updates src/lib/services.ts to set explicit filesystem permissions when creating PID directories and per-service log files: directories are created/chmod'd to 0o700, log files opened/fchmod'd to 0o600. Lines changed: +6/-2.

Changes

Cohort / File(s) Summary
Explicit Filesystem Permissions
src/lib/services.ts
Ensure PID directory is created with 0o700 and chmodSync(pidDir, 0o700) is applied; open per-service log file with mode 0o600 and call fchmodSync(logFd, 0o600) to enforce descriptor permissions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 I nudge the code with careful paws,
I set the doors to safe applause,
0o700 for the PID den tight,
0o600 keeps logs out of sight,
A hop, a guard, and all feels right. 🥕🔐

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% 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 describes the main security fix: restricting permissions on the service temporary directory and log file to prevent unauthorized access.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/lib/services.ts`:
- Line 126: The log file is opened with openSync(logFile, "w", 0o600) but that
mode only applies on create; if cloudflared.log already exists its permissions
can remain world-readable—after calling openSync and assigning to logFd add a
call to fchmodSync(logFd, 0o600) to force owner-only permissions; locate the
openSync usage that assigns to logFd in src/lib/services.ts and invoke
fchmodSync(logFd, 0o600) immediately after opening (keep the same logFile and
logFd symbols).
- Around line 63-65: The code only sets mode in mkdirSync which doesn’t change
permissions on existing pidDir or log files; update the pid directory and any
service log/pid file handling to explicitly call fs.chmodSync after
creation/existence check so permissions are enforced—e.g., after using mkdirSync
or when you compute pidDir (refer to the pidDir variable and the
mkdirSync/existsSync block), call chmodSync(pidDir, 0o700); likewise, after
creating/opening service files (where openSync is used) call
chmodSync(logPathOrPidFile, 0o600) to tighten permissions for existing objects.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c08043df-dfe3-43d8-8597-43fd2f5d28cf

📥 Commits

Reviewing files that changed from the base of the PR and between 6f7f0c6 and 903ba55.

📒 Files selected for processing (1)
  • src/lib/services.ts

Comment thread src/lib/services.ts
Comment thread src/lib/services.ts
…rs and logs

The previous commit set the mode in mkdirSync and openSync, but those
only apply at creation time. If the pid directory or log file already
exists from a prior run the permissions were never updated.

Add chmodSync(pidDir, 0o700) unconditionally after the existence check,
and fchmodSync(logFd, 0o600) immediately after openSync so permissions
are enforced regardless of whether the objects were just created.

Signed-off-by: ColinM-sys <cmcdonough@50words.com>
@wscurran wscurran added bug Something isn't working security Something isn't secure priority: high Important issue that should be resolved in the next release labels Apr 27, 2026
@wscurran
Copy link
Copy Markdown
Contributor

✨ Thanks for submitting this pull request that proposes a way to fix a security bug by restricting permissions on the service tmp dir and log file, making the cloudflared.log file no longer world-readable.

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

Labels

bug Something isn't working priority: high Important issue that should be resolved in the next release security Something isn't secure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants