From 98105f31f46d3de58a8f69a2a439cee3f7a5e389 Mon Sep 17 00:00:00 2001 From: kshitijk4poor Date: Thu, 30 Jul 2026 20:40:29 +0500 Subject: [PATCH] fix(file_ops): harden new-file umask chmod for portability Follow-ups on top of #70888's cherry-picked fix: - Replace the $((0666 & ~0$u)) shell arithmetic with POSIX who-less 'chmod "=rw"'. zsh (reachable via _find_bash's $SHELL fallback on bash-less hosts) parses leading-zero constants as decimal and silently chmods a garbage mode (e.g. 0210); the symbolic form is spec-identical across bash/dash/busybox-ash/zsh and degrades to mktemp's 0600 (pre-fix behavior) rather than corrupting perms if chmod rejects it. - Move the new-file chmod after the content stream so the temp file stays owner-writable while cat runs. - Run the chmod on a '[ ! -e "$t" ]' check after cat instead of the stat/else branch, keeping the overwrite path untouched. - Update the stale perms comment #70856 called out (new files did NOT land with default umask perms pre-fix). - Tests: select the atomic-write script by content instead of call order (the previous last-call capture only worked because the bare MagicMock's falsy-exit early return suppressed later execs), assert behavior at explicit umasks 0022/0002/0077 via parametrize, add an overwrite mode-preservation regression guard, and dedupe the real-subprocess env fake into make_real_subprocess_env() shared with TestSearchFilesFallbackHiddenPaths. (webtecnica's email mapping already exists in contributors/emails/ on current main; the PR's check-attribution red was stale-base only.) # Conflicts: # tests/tools/test_file_operations.py --- tests/tools/test_file_operations.py | 103 ++++++++++++++++------------ tools/file_operations.py | 18 +++-- 2 files changed, 73 insertions(+), 48 deletions(-) diff --git a/tests/tools/test_file_operations.py b/tests/tools/test_file_operations.py index bc3b6c4542f..e80938d3ef7 100644 --- a/tests/tools/test_file_operations.py +++ b/tests/tools/test_file_operations.py @@ -242,6 +242,38 @@ def file_ops(mock_env): return ShellFileOperations(mock_env) +def make_real_subprocess_env(cwd: str, include_stderr: bool = False) -> MagicMock: + """Mock env whose execute() runs the command in a real subprocess. + + For tests that need the generated shell scripts to actually run + (search fallback, atomic-write permissions) instead of being + intercepted by a bare MagicMock. ``include_stderr`` folds stderr + into ``output`` for tests that surface shell error text; leave it + off for tests that parse structured stdout (e.g. find results). + """ + env = MagicMock() + env.cwd = cwd + + def execute(command, **kwargs): + completed = subprocess.run( + command, + shell=True, + text=True, + capture_output=True, + input=kwargs.get("stdin_data"), + ) + output = completed.stdout + if include_stderr: + output += completed.stderr + return { + "output": output, + "returncode": completed.returncode, + } + + env.execute = execute + return env + + class TestShellFileOpsHelpers: def test_normalize_read_pagination_clamps_invalid_values(self): assert normalize_read_pagination(offset=0, limit=0) == (1, 1) @@ -393,23 +425,7 @@ class TestSearchPathValidation: class TestSearchFilesFallbackHiddenPaths: def _make_env(self): - env = MagicMock() - env.cwd = "/" - - def execute(command, **kwargs): - completed = subprocess.run( - command, - shell=True, - text=True, - capture_output=True, - ) - return { - "output": completed.stdout, - "returncode": completed.returncode, - } - - env.execute = execute - return env + return make_real_subprocess_env("/") def test_hidden_root_with_hidden_ancestor_includes_files(self, tmp_path, monkeypatch): """Fallback find should include visible files when path is inside hidden root.""" @@ -561,42 +577,41 @@ class _DeletedTestGitBaselineCheck: class TestAtomicWriteNewFilePermissions: """_atomic_write should apply umask-default perms to new files (not 0600).""" - def test_new_file_gets_umask_default_permissions(self, tmp_path): + @pytest.mark.parametrize("test_umask", [0o022, 0o002, 0o077]) + def test_new_file_gets_umask_default_permissions(self, tmp_path, test_umask): """Newly created file should get umask-computed perms, not mktemp's 0600. Uses a real subprocess so the shell script actually runs. """ - env = MagicMock() - env.cwd = str(tmp_path) - - def execute(command, **kwargs): - completed = subprocess.run( - command, - shell=True, - text=True, - capture_output=True, - input=kwargs.get("stdin_data"), - ) - return { - "output": completed.stdout + completed.stderr, - "returncode": completed.returncode, - } - - env.execute = execute - ops = ShellFileOperations(env) + ops = ShellFileOperations(make_real_subprocess_env(str(tmp_path))) dest = tmp_path / "new_file.txt" assert not dest.exists() - result = ops.write_file(str(dest), "test content\n") - assert result.error is None, f"write failed: {result.error}" - assert dest.exists() + old_umask = os.umask(test_umask) + try: + result = ops.write_file(str(dest), "test content\n") + finally: + os.umask(old_umask) - # Compute expected mode: 0666 & ~umask - current_umask = os.umask(0) - os.umask(current_umask) # restore - expected_mode = 0o666 & ~current_umask + assert result.error is None, f"write failed: {result.error}" + assert dest.read_text() == "test content\n" + expected_mode = 0o666 & ~test_umask actual_mode = dest.stat().st_mode & 0o777 assert actual_mode == expected_mode, ( - f"Expected mode {expected_mode:04o} (umask {current_umask:04o}), " + f"Expected mode {expected_mode:04o} (umask {test_umask:04o}), " f"got {actual_mode:04o}" ) + + def test_overwrite_still_preserves_existing_mode(self, tmp_path): + """The new-file branch must not disturb the overwrite path's + mode preservation (e.g. an executable script stays 0755).""" + ops = ShellFileOperations(make_real_subprocess_env(str(tmp_path))) + dest = tmp_path / "existing.sh" + dest.write_text("#!/bin/sh\n") + dest.chmod(0o755) + + result = ops.write_file(str(dest), "#!/bin/sh\necho updated\n") + + assert result.error is None, f"write failed: {result.error}" + assert dest.read_text() == "#!/bin/sh\necho updated\n" + assert dest.stat().st_mode & 0o777 == 0o755 diff --git a/tools/file_operations.py b/tools/file_operations.py index aff82fa0af2..d0d1f53a833 100644 --- a/tools/file_operations.py +++ b/tools/file_operations.py @@ -1005,7 +1005,16 @@ class ShellFileOperations(FileOperations): # - `chmod --reference` is GNU-only, so we read the octal mode with # `stat` (GNU `-c%a` or BSD `-f%Lp`) and `chmod` it explicitly; # silent best-effort — a perms-copy failure must not abort the - # write, the file still lands with default umask perms. + # write (the file then lands at mktemp's 0600, same as pre-fix). + # - brand-new targets get `chmod "=rw"` — the POSIX who-less + # symbolic form, which sets rw minus the process umask (e.g. + # 0644 under umask 022) instead of mktemp's hardcoded 0600 + # (#70856). Deliberately NOT shell arithmetic on `$(umask)`: + # zsh (reachable via _find_bash's $SHELL fallback) parses + # leading-zero constants as decimal and silently computes a + # garbage mode, while `chmod "=rw"` is spec-identical in + # bash/dash/ash/zsh and degrades to 0600 (pre-fix behavior) + # if an exotic chmod rejects it. # - `trap ... EXIT` guarantees the temp is removed on every error # path (cat failure, mv failure, signal) but NOT after a # successful mv (the temp no longer exists by then). @@ -1022,11 +1031,12 @@ class ShellFileOperations(FileOperations): 'if [ -e "$t" ]; then ' 'm="$(stat -c%a "$t" 2>/dev/null || stat -f%Lp "$t" 2>/dev/null || true)"; ' '[ -n "$m" ] && chmod "$m" "$tmp" 2>/dev/null || true; ' - # new file: apply umask-computed default instead of mktemp's 0600 - 'else ' - 'u="$(umask)"; chmod $(printf '"'"'%04o'"'"' $((0666 & ~0$u))) "$tmp" 2>/dev/null || true; ' "fi; " 'cat > "$tmp"; ' + # new file: umask-default perms instead of mktemp's 0600 (#70856). + # Runs AFTER cat so a write-masking umask can't EACCES the stream; + # quoted "=rw" so zsh doesn't =word-expand it. + 'if [ ! -e "$t" ]; then chmod "=rw" "$tmp" 2>/dev/null || true; fi; ' 'mv -f "$tmp" "$t"; ' "trap - EXIT" )