Repository navigation
tig-cli: keep container claims in a private, user-owned directory (SonarCloud S5443) - #49
devin-ai-integration[bot] wants to merge 2 commits into
Conversation
Claims fell back to a shared, world-writable TMPDIR or /tmp, where another user could pre-create the claim directory or a symlink in its place to plant or delete claims. Fall back via tempfile.gettempdir(), create the directory 0700, and only use it when it is a real directory owned by this user, tightening its permissions if they are open. Co-Authored-By: jason.han <hanhuijun@gmail.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
A claim directory that was open to others may hold claims planted before it was closed; only trust regular files the user owns. Co-Authored-By: jason.han <hanhuijun@gmail.com>
Project Requirements: PASSPR #49 hardens the tig-cli container claim directory (tig_cli/spec.py Claim) against tampering by other local users in the shared temporary directory, and adds targeted regression tests. Documentation parity: pass (internal, not user-facing). Broad deployability: pass (stdlib-only, portable POSIX). Test coverage: pass (regression tests for each attack condition, run in CI).
Automated pre-check hints
Evaluated at Criteria: |
There was a problem hiding this comment.
Devin Review found 1 finding. Devin automatically fixed it.
Fixed by Devin (1)
- Planted claims survive directory repair — claimed_containers() now lstat()s each claim and only trusts regular files owned by the user, so claims planted while the directory was open are ignored.
Summary
Addresses SonarCloud
python:S5443("publicly writable directories") alerts #1–#7 ondevelop@ ca85348.The real issue was
Claim.directory()(the per-process container claims that protect containers from reaping). WithoutXDG_RUNTIME_DIRit fell back to$TMPDIRor/tmp, then didmkdir(parents=True, exist_ok=True)with default (umask) permissions and trusted whatever was there. Another local user could pre-create/tmp/tig-claims-<uid>(or a symlink there) and then plant claims, which would block reaping, or delete them, which would get a live container reaped.acquire()andclaimed_containers()both go through_private_directory. If the directory can't be trusted, they behave the same way they already did when the directory couldn't be used (no claim is written and no claims are read).tempfile.gettempdir()still readsTMPDIRfirst, so the resolved path doesn't change for existing users. An existing/tmp/tig-claims-<uid>the user already owns is tightened to0700instead of being rejected.Alert mapping
spec.py:664(or "/tmp")tempfile.gettempdir(), and the directory is created0700and checked for ownership, type and mode.spec.py:663(os.environ.get("TMPDIR"))gettempdir()still readsTMPDIR).spec.py:604Mount("/tmp/.X11-unix", "/tmp/.X11-unix")/tmp/.X11-unix. tig creates and writes nothing here; the X server/OS owns the directory.spec.py:316_x_socket()→/tmp/.X11-unix/X<n>x_server_identity()canos.statit to tell when the X server has changed. Nothing is created, opened or written.container.py:558source == "/tmp/.X11-unix"I didn't pull the X11 literal into a shared constant just to clear the alerts. Sonar would flag the constant's line instead, and the code would be no safer.
Tests
New tests in
tig-cli/tests/test_container.py:0700and owned by the user;0777) directory the user already owns is tightened to0700;XDG_RUNTIME_DIR, the fallback istempfile.gettempdir(), which honoursTMPDIR.pytest tests/ -m "not integration"result: 424 passed.Link to Devin session: https://nasa-jpl-demo.devinenterprise.com/sessions/5ca13307d6b54a03a22f909f6815bb1d
Open in Devin Desktop: https://nasa-jpl-demo.devinenterprise.com/desktop/session/5ca13307d6b54a03a22f909f6815bb1d?variant=devin
Requested by: @HuiJun
🟢 Devin fixed the finding · Follow Devin's work