fix(cli): fall back to a writable state dir when $HOME is unwritable#805
Open
albcui wants to merge 1 commit into
Open
fix(cli): fall back to a writable state dir when $HOME is unwritable#805albcui wants to merge 1 commit into
albcui wants to merge 1 commit into
Conversation
Contributor
|
`nemo services run` writes per-instance lock/descriptor state under $XDG_STATE_HOME (default ~/.local/state/nmp). When the nmp-api entrypoint runs under a uid that does not own $HOME -- a common Kubernetes/OpenShift hardening pattern (arbitrary runAsUser) -- that location is unwritable and the command crashes with PermissionError before starting. Resolve the base dir by actually creating its `instances/` dir, falling back to a per-uid dir under the system temp dir when the preferred XDG/$HOME location can't be written. Selecting by a real mkdir (rather than an advisory os.access probe) is correct even where the two disagree -- NFS root-squash, SELinux -- and even if the fallback itself is unwritable. The result is memoized so reads and writes agree on one directory, and the temp fallback is created owner-only (0700) so the descriptor (pid/port/config path) and service log are not world-readable on a shared /tmp. Regenerate the vendored SDK CLI copy. Add regression tests for fallback resolution, memoization, the owner-only temp dir, the no-usable-tempdir case, and the no-writable-location error. Signed-off-by: Albert Cui <albcui@nvidia.com>
albcui
force-pushed
the
albcui/harden-nemo-services-state-dir-fallback
branch
from
July 21, 2026 00:23
c3cf273 to
d87108d
Compare
albcui
marked this pull request as ready for review
July 21, 2026 00:23
Contributor
📝 WalkthroughWalkthroughChangesState directory resolution
Sequence Diagram(s)sequenceDiagram
participant Caller
participant _base_state_dir
participant Filesystem
participant Logger
Caller->>_base_state_dir: request base state directory
_base_state_dir->>Filesystem: probe preferred instances directory
Filesystem-->>_base_state_dir: writable or unavailable
_base_state_dir->>Filesystem: probe per-UID fallback directory
Filesystem-->>_base_state_dir: writable fallback
_base_state_dir->>Logger: emit fallback warning
_base_state_dir-->>Caller: return memoized directory
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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
`@packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/services/_process.py`:
- Around line 141-143: Update the warning in _preferred_base_state_dir()’s
state-directory fallback path to remove the unsupported _NMP_STATE_DIR override
from the message, leaving only configuration variables that actually affect
resolution.
- Around line 104-105: Remove the contextlib.suppress around os.chmod for the
fallback nmp-state directory so EPERM or other chmod failures reject the
candidate and prevent using an unsecured path. Update the fallback handling
around this chmod operation to continue safely without writing descriptors or
logs there, and add a regression test covering a pre-created squatter-controlled
directory whose chmod fails.
🪄 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: d9a315a0-f5e6-4f51-b7eb-a00e0eef4ebd
⛔ Files ignored due to path filters (2)
sdk/python/nemo-platform/src/nemo_platform/cli/commands/services/_process.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/commands/test_services_process.pyis excluded by!sdk/**
📒 Files selected for processing (2)
packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/services/_process.pypackages/nemo_platform_ext/tests/cli/commands/test_services_process.py
Comment on lines
+104
to
+105
| with contextlib.suppress(OSError): | ||
| os.chmod(base.parent, 0o700) # the `nmp-state-<uid>` dir under the temp root |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject fallback directories that cannot be secured.
A different user can pre-create /tmp/nmp-state-<uid> as writable. chmod then fails with EPERM, is suppressed, and descriptors/logs may be written under the squatter-controlled path. Let the chmod failure reject this fallback candidate and add a squatting regression test.
Also applies to: 139-147
🤖 Prompt for 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.
In
`@packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/services/_process.py`
around lines 104 - 105, Remove the contextlib.suppress around os.chmod for the
fallback nmp-state directory so EPERM or other chmod failures reject the
candidate and prevent using an unsecured path. Update the fallback handling
around this chmod operation to continue safely without writing descriptors or
logs there, and add a regression test covering a pre-created squatter-controlled
directory whose chmod fails.
Comment on lines
+141
to
+143
| logger.warning( | ||
| "State directory %s is not writable; using %s instead. Set XDG_STATE_HOME " | ||
| "or _NMP_STATE_DIR to a writable path to control where instance state lives.", |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not advertise an ignored override.
_preferred_base_state_dir() reads only XDG_STATE_HOME and $HOME; _NMP_STATE_DIR will not change resolution. Remove it from this warning or implement it.
🤖 Prompt for 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.
In
`@packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/services/_process.py`
around lines 141 - 143, Update the warning in _preferred_base_state_dir()’s
state-directory fallback path to remove the unsupported _NMP_STATE_DIR override
from the message, leaving only configuration variables that actually affect
resolution.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
nemo services runwrites per-instance lock/descriptor state under$XDG_STATE_HOME(default~/.local/state/nmp). When thenmp-apientrypoint runs under a uid that does not own$HOME-- a common Kubernetes/OpenShift hardening pattern (arbitraryrunAsUser) -- that location is unwritable and the command crashes with PermissionError before starting.Make
_base_state_dir()resilient: probe writability non-destructively and fall back to a per-uid dir under the system temp dir when theXDG/$HOMElocation cannot be written. The probe tolerates an unsearchable parent (a $HOME created mode 0700 for a different uid raises EACCES on stat) so it never propagates the very error the fallback exists to avoid.Summary by CodeRabbit