Skip to content

Commit 10d4bca

Browse files
jawwad-aliclaude
andauthored
fix(integrations): guard _sha256 against unreadable managed files (#3376)
manifest.py::_sha256 does an unguarded open(). check_modified() and uninstall() both call it on a readable-but-unopenable regular file (e.g. permission denied) without catching OSError, so 'specify integration upgrade/uninstall/switch' surface a raw PermissionError traceback. Guard both call sites: in check_modified() treat an unreadable file as modified (consistent with the adjacent symlink / non-regular-file handling); in uninstall() treat it as skipped and preserve it (mirroring the existing path.unlink() OSError guard just below). The force short-circuit is unchanged. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent f1a8d8f commit 10d4bca

2 files changed

Lines changed: 56 additions & 4 deletions

File tree

src/specify_cli/integrations/manifest.py

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,14 @@ def check_modified(self) -> list[str]:
309309
if abs_path.is_symlink() or not abs_path.is_file():
310310
modified.append(rel)
311311
continue
312-
if _sha256(abs_path) != expected_hash:
312+
try:
313+
changed = _sha256(abs_path) != expected_hash
314+
except OSError:
315+
# Unreadable regular file (e.g. permission denied): treat as
316+
# modified, consistent with the symlink / non-regular-file
317+
# handling above, rather than letting the OSError escape.
318+
changed = True
319+
if changed:
313320
modified.append(rel)
314321
return modified
315322

@@ -358,9 +365,17 @@ def uninstall(
358365
skipped.append(path)
359366
continue
360367
else:
361-
if not force and _sha256(path) != expected_hash:
362-
skipped.append(path)
363-
continue
368+
if not force:
369+
try:
370+
matches = _sha256(path) == expected_hash
371+
except OSError:
372+
# Unreadable: can't verify it's ours, so preserve it
373+
# (mirrors the path.unlink() OSError guard below).
374+
skipped.append(path)
375+
continue
376+
if not matches:
377+
skipped.append(path)
378+
continue
364379
try:
365380
path.unlink()
366381
except OSError:

tests/integrations/test_manifest.py

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -481,3 +481,40 @@ def test_rejects_inside_root_dotdot_with_explicit_message(self, tmp_path):
481481
m = IntegrationManifest("test", tmp_path)
482482
with pytest.raises(ValueError, match=r"canonical|'\.\.' segments"):
483483
m.record_existing("dir/../file.txt")
484+
485+
486+
class TestManifestUnreadableFile:
487+
"""A managed file that is unreadable (e.g. PermissionError) must not crash
488+
check_modified()/uninstall() — the CLI handlers surfaced a raw traceback."""
489+
490+
def _mk(self, tmp_path):
491+
m = IntegrationManifest("test", tmp_path)
492+
m.record_file("sub/f.md", "content")
493+
return m
494+
495+
def test_check_modified_treats_unreadable_as_modified(self, tmp_path, monkeypatch):
496+
m = self._mk(tmp_path)
497+
498+
def raise_perm(_path):
499+
raise PermissionError("unreadable")
500+
501+
monkeypatch.setattr(
502+
"specify_cli.integrations.manifest._sha256", raise_perm
503+
)
504+
# Before the fix this raised PermissionError.
505+
assert m.check_modified() == ["sub/f.md"]
506+
507+
def test_uninstall_preserves_unreadable_file(self, tmp_path, monkeypatch):
508+
m = self._mk(tmp_path)
509+
510+
def raise_perm(_path):
511+
raise PermissionError("unreadable")
512+
513+
monkeypatch.setattr(
514+
"specify_cli.integrations.manifest._sha256", raise_perm
515+
)
516+
removed, skipped = m.uninstall(force=False)
517+
# Can't verify ownership => preserve, don't crash and don't delete.
518+
assert removed == []
519+
assert (tmp_path / "sub" / "f.md") in skipped
520+
assert (tmp_path / "sub" / "f.md").exists()

0 commit comments

Comments
 (0)