fix(starlark): address the review on the ownership repair

Findings from the automated review of #604.

Symlinks (CWE-59, the serious one). A root chown that follows links is a
privilege-escalation primitive: anyone able to write in starlark-apps could
point a link at a root-owned file and have the repair hand it over. Entries
are now read with os.lstat, symlinks are skipped outright, and the chown
passes follow_symlinks=False. Descendants are processed before the directory
itself, so the container does not change hands while its contents are still
being walked.

install_app() caught PermissionError in its broad handler and returned
False, which both routes report as a generic install failure -- the exact
shape of the bug this PR exists to fix, since the caller could not tell
"this app is broken" from "this process cannot write here". PermissionError
is now re-raised; every other failure still returns False.

The test fixtures skipped on bare Exception, which would have turned a
syntax error or NameError in the plugin into a green run. They now skip only
for a named absent dependency and re-raise anything else.

Also fixed the _Stat stub that failed in CI but passed locally: it carried
only st_uid/st_gid, and pathlib reads st_mode while walking. It now wraps
the real stat result and overrides ownership alone.

NOT taken: the CodeQL "information exposure through an exception" finding on
the hint response. Dropping `details` would contradict this package's
documented rule -- "if it returns 5xx, it says why" -- which
test_no_api_v3_handler_discards_its_exception enforces with an allowance
that may shrink and never grow. The Starlark routes are the ones that policy
was written for: they answered 500 with no detail for three releases.
describe_exception already redacts credentials and truncates. Keeping the
detail is the deliberate trade-off, so the finding is declined rather than
silently worked around.

Verified on hdpi with the updated code: a symlink to /etc/shadow planted in
starlark-apps was skipped while the directory was handed back, and
/etc/shadow stayed root:shadow.

Mutation-checked all three behaviours. The symlink test was vacuous on the
first attempt -- the link already had the target owner, so it was skipped
for the wrong reason and the mutation passed. It now forces the link to look
like it needs handing over, and fails when the check is removed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9
This commit is contained in:
ChuckBuilds
2026-09-21 18:09:28 -04:00
co-authored by Claude Opus 5
parent 965d509864
commit a864c3223c
3 changed files with 143 additions and 16 deletions
+22 -3
View File
@@ -10,6 +10,7 @@ API Version: 1.0.0
import json
import os
import re
import stat
import time
import fcntl
from pathlib import Path
@@ -501,15 +502,26 @@ class StarlarkAppsPlugin(BasePlugin):
# directory is correct and there is nobody to hand it to.
return
for path in (apps_dir, *apps_dir.rglob("*")):
# Deepest first, with the directory itself last. Handing over the
# container before its contents would briefly let a local user rename
# entries underneath a repair that is still running.
descendants = sorted(apps_dir.rglob("*"),
key=lambda p: len(p.parts), reverse=True)
for path in (*descendants, apps_dir):
try:
st = path.stat()
st = os.lstat(path)
except OSError:
continue
if stat.S_ISLNK(st.st_mode):
# Never hand over a link's target. Anyone able to write in
# this directory could otherwise point a symlink at a
# root-owned file and have this give it away -- the whole
# point of the loop is that it runs as root.
continue
if st.st_uid == owner.st_uid and st.st_gid == owner.st_gid:
continue
try:
chown(path, owner.st_uid, owner.st_gid)
chown(path, owner.st_uid, owner.st_gid, follow_symlinks=False)
except OSError as e:
self.logger.warning(
"Could not hand %s to uid %s: %s -- installs from the web "
@@ -1065,6 +1077,13 @@ class StarlarkAppsPlugin(BasePlugin):
self.logger.info(f"Installed Starlark app: {app_id} (sanitized: {safe_app_id})")
return True
except PermissionError:
# Deliberately not folded into the False below. A False here is
# reported as a generic install failure, which is how the
# directory-ownership bug stayed invisible: the caller could not
# tell "this app is broken" from "this process cannot write here".
# The routes turn this into a message that names the fix.
raise
except Exception as e:
self.logger.error(f"Error installing app {app_id}: {e}")
return False