Skip to content

tig-cli: keep container claims in a private, user-owned directory (SonarCloud S5443) - #49

Open
devin-ai-integration[bot] wants to merge 2 commits into
developfrom
devin/1794421872-secure-claim-dir
Open

devin-ai-integration[bot] wants to merge 2 commits into
developfrom
devin/1794421872-secure-claim-dir

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Addresses SonarCloud python:S5443 ("publicly writable directories") alerts #1–#7 on develop @ ca85348.

The real issue was Claim.directory() (the per-process container claims that protect containers from reaping). Without XDG_RUNTIME_DIR it fell back to $TMPDIR or /tmp, then did mkdir(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.

# before
base = XDG_RUNTIME_DIR or os.environ.get("TMPDIR") or "/tmp"
directory.mkdir(parents=True, exist_ok=True)

# after
base = XDG_RUNTIME_DIR or tempfile.gettempdir()    # tempfile imported lazily, only on fallback
Claim._private_directory(create)                   # mkdir(mode=0o700); lstat; must be a real dir
                                                   # owned by os.getuid(); chmod 0700 if group/other bits set;
                                                   # otherwise None -> no claim / no claims trusted
claimed_containers()                               # also skips claim entries that aren't regular files
                                                   # owned by the user (planted while the dir was open)

acquire() and claimed_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 reads TMPDIR first, so the resolved path doesn't change for existing users. An existing /tmp/tig-claims-<uid> the user already owns is tightened to 0700 instead of being rejected.

Alert mapping

Alert Location Resolution
#6 spec.py:664 (or "/tmp") Fixed. Fallback is now tempfile.gettempdir(), and the directory is created 0700 and checked for ownership, type and mode.
#5 spec.py:663 (os.environ.get("TMPDIR")) Fixed. Same change (gettempdir() still reads TMPDIR).
#3, #4 spec.py:604 Mount("/tmp/.X11-unix", "/tmp/.X11-unix") Not changed, recommend dismissing as False positive. This bind-mounts the host X server's socket directory so VICAR GUI tools can draw. X11 clients look for the socket at exactly this path, so both host and container paths have to be /tmp/.X11-unix. tig creates and writes nothing here; the X server/OS owns the directory.
#1, #2 spec.py:316 _x_socket() → /tmp/.X11-unix/X<n> Not changed, recommend dismissing as False positive. This only builds the path of the local display's socket so x_server_identity() can os.stat it to tell when the X server has changed. Nothing is created, opened or written.
#7 container.py:558 source == "/tmp/.X11-unix" Not changed, recommend dismissing as False positive. This is a string comparison that leaves the X11 socket mount out of the list of writable mounts. No filesystem access happens.

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:

  • the claim directory is created 0700 and owned by the user;
  • an open (0777) directory the user already owns is tightened to 0700;
  • a symlink in place of the directory is not trusted: nothing is written through it and planted claims are ignored;
  • a directory owned by another user is not trusted;
  • claim entries that are not regular files owned by the user are ignored;
  • with no XDG_RUNTIME_DIR, the fallback is tempfile.gettempdir(), which honours TMPDIR.

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

Devin Review

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>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

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>
@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

Project Requirements: PASS

PR #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).

Requirement Verdict Notes
1. Documentation parity Pass Not user-facing. The PR hardens the internal container-claim state directory: it is now created 0700, and symlinked, foreign-owned or foreign-file entries are refused. It also replaces the hand-rolled TMPDIR-or-/tmp fallback with tempfile.gettempdir(), which still honors TMPDIR and behaves the same in practice. No CLI option, config key, env var, error message or exit code changes. The claim directory location is not documented anywhere (tig-cli/README.md only describes the reaping guarantees, which still hold), so no docs need updating.
2. Broad deployability Pass Generic POSIX code using only the stdlib (os.lstat, stat, os.chmod, tempfile). It works on Linux and macOS with every supported runtime. No site-specific paths, no new dependencies, and from __future__ import annotations keeps the Path | None annotation compatible with Python 3.9. The os.getuid() dependency already existed before this PR.
3. Test coverage Pass The fix hardens a shared, world-writable fallback claim directory. Six new unit tests in tig-cli/tests/test_container.py cover each triggering condition: the directory is created 0700 and owned by the user; a user-owned directory opened to 0777 is re-closed; a symlink in place of the directory is refused, nothing is written through it and planted claims are ignored; a directory owned by another uid (simulated with a mocked getuid) is not trusted; claim files that are foreign-owned or symlinks are ignored; and the fallback to tempfile.gettempdir()/TMPDIR works. The symlink and foreign-owner tests would fail against the old code. The tests are deterministic, mock rather than need a runtime, and run in CI through test.yml (path filter tig-cli/**, Python 3.9-3.12 on Ubuntu and macOS). Locally all 114 tests in test_container.py pass. Minor untested paths are mkdir or chmod raising OSError, which are non-critical degrade-to-no-claim branches.
Automated pre-check hints
  • tig-cli/src/ changed but neither tig-cli/README.md nor docs/ changed (requirement 1).

Evaluated at f061f3b · Devin session · workflow run

Criteria: .github/project-requirements/criteria.md. A fail blocks the merge; a maintainer can waive it by adding the requirements-waived label.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

View findings in Devin Review · Follow Devin's work

Devin Review

This branch has not been deployed

No deployments
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.

1 participant