Skip to content

Commit 0a6d925

Browse files
fix: abort on chown failure, recursive TOML null check, reject \r in dotenv values
- _atomic.py: abort atomic replacement when os.chown fails instead of silently installing a caller-owned file the application cannot read. Both atomic_write_text and atomic_write_bytes now raise OSError with a descriptive message when ownership preservation fails. - cli.py TOML null check: recursively scan lists and nested dicts for embedded None values so cross-format fixes like items: [1, null] fail cleanly before tomli_w raises TypeError during serialization. - cli.py dotenv multiline: reject carriage returns (\r) alongside newlines (\n) since universal-newline readers treat \r as a line boundary, truncating the setting and causing perpetual drift. Addresses Codex review: _atomic.py:44 (P1), cli.py:514 (P2), cli.py:562 (P2)
1 parent 43a9b57 commit 0a6d925

2 files changed

Lines changed: 45 additions & 22 deletions

File tree

‎src/configdrift/_atomic.py‎

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -32,16 +32,22 @@ def atomic_write_text(path: Path, text: str, encoding: str = "utf-8") -> None:
3232
fh.write(text)
3333
fh.flush()
3434
os.fsync(fh.fileno())
35-
# Preserve target permissions and ownership during atomic replacement
35+
# Preserve target permissions and ownership during atomic replacement.
36+
# If ownership restoration fails (e.g. unprivileged caller), abort
37+
# the replacement rather than installing a caller-owned file that
38+
# the application cannot read.
3639
if resolved.exists():
40+
st = resolved.stat()
41+
os.chmod(tmp, st.st_mode)
3742
try:
38-
st = resolved.stat()
39-
os.chmod(tmp, st.st_mode)
40-
# Preserve owner so application accounts can still read the
41-
# config after a privileged deployment user runs fix.
4243
os.chown(tmp, st.st_uid, st.st_gid)
43-
except OSError:
44-
pass
44+
except OSError as chown_err:
45+
with contextlib.suppress(OSError):
46+
os.unlink(tmp)
47+
raise OSError(
48+
f"Cannot preserve ownership of {resolved} "
49+
f"(uid={st.st_uid}, gid={st.st_gid}): {chown_err}"
50+
) from chown_err
4551
os.replace(tmp, resolved)
4652
except BaseException:
4753
# Clean up temp file on any failure
@@ -66,14 +72,21 @@ def atomic_write_bytes(path: Path, data: bytes) -> None:
6672
fh.write(data)
6773
fh.flush()
6874
os.fsync(fh.fileno())
69-
# Preserve target permissions and ownership during atomic replacement
75+
# Preserve target permissions and ownership during atomic replacement.
76+
# If ownership restoration fails, abort rather than installing a
77+
# caller-owned file the application cannot read.
7078
if resolved.exists():
79+
st = resolved.stat()
80+
os.chmod(tmp, st.st_mode)
7181
try:
72-
st = resolved.stat()
73-
os.chmod(tmp, st.st_mode)
7482
os.chown(tmp, st.st_uid, st.st_gid)
75-
except OSError:
76-
pass
83+
except OSError as chown_err:
84+
with contextlib.suppress(OSError):
85+
os.unlink(tmp)
86+
raise OSError(
87+
f"Cannot preserve ownership of {resolved} "
88+
f"(uid={st.st_uid}, gid={st.st_gid}): {chown_err}"
89+
) from chown_err
7790
os.replace(tmp, resolved)
7891
except BaseException:
7992
with contextlib.suppress(OSError):

‎src/configdrift/cli.py‎

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -509,10 +509,18 @@ def fix(
509509

510510
# Reject None values before building the TOML dict —
511511
# TOML has no null representation, so tomli_w would
512-
# raise TypeError on serialization.
513-
has_null = any(v is None for v in target_data.values())
514-
if has_null:
515-
null_keys = [k for k, v in target_data.items() if v is None]
512+
# raise TypeError on serialization. Check recursively
513+
# through lists and nested dicts for embedded nulls.
514+
def _has_null(val: Any) -> bool:
515+
if val is None:
516+
return True
517+
if isinstance(val, dict):
518+
return any(_has_null(v) for v in val.values())
519+
if isinstance(val, (list, tuple)):
520+
return any(_has_null(v) for v in val)
521+
return False
522+
null_keys = [k for k, v in target_data.items() if _has_null(v)]
523+
if null_keys:
516524
console.print(
517525
f"[red]Error: TOML does not support null values. "
518526
f"Keys with null: {', '.join(null_keys[:5])}[/red]"
@@ -553,17 +561,19 @@ def fix(
553561
)
554562
failed_targets.append(str(target_path))
555563
continue
556-
# Reject multiline values — newlines would corrupt the
557-
# dotenv file by splitting a single KEY=VALUE across
558-
# multiple physical lines.
564+
# Reject multiline values — newlines (\n) and carriage
565+
# returns (\r) would corrupt the dotenv file by splitting
566+
# a single KEY=VALUE across multiple physical lines.
567+
# Readers using universal-newline handling treat \r as a
568+
# line boundary, truncating the setting.
559569
multiline_keys = [
560570
k for k, v in target_data.items()
561-
if isinstance(v, str) and '\n' in v
571+
if isinstance(v, str) and ('\n' in v or '\r' in v)
562572
]
563573
if multiline_keys:
564574
console.print(
565-
f"[red]Error: dotenv values must not contain newlines. "
566-
f"Keys with newlines: {', '.join(multiline_keys[:5])}[/red]"
575+
f"[red]Error: dotenv values must not contain newlines or "
576+
f"carriage returns. Keys affected: {', '.join(multiline_keys[:5])}[/red]"
567577
)
568578
failed_targets.append(str(target_path))
569579
continue

0 commit comments

Comments
 (0)