Delete a Conflicting Sudo Timestamp Drop-In on Confirm

[ptr727]

install-tools.sh --sudo-timestamp already detected another sudoers file setting the same timestamp option, but only ever warned about it. A host that already carried a hand-authored drop-in (or one left by a prior run of this script before this fix) ended up with two files asserting the same thing, forever.

configure_sudo_timestamp now classifies each conflicting entry it finds:

• Scoped to this user, and the file sets nothing else: delete it on confirm, after the managed drop-in is proved in place.
• Scoped to a different user, or unscoped: report only, never touched, since that changes something beyond what this run was asked to change.
• Scoped to this user but mixed with unrelated settings, or living in /etc/sudoers itself: die and ask for a manual visudo fix rather than guess which lines are safe to drop from a shared file.

A decline leaves the host unchanged, matching the existing confirm-gate pattern. The cleanup also runs when the managed file already has the right content, so a host that already ran the old script and ended up with both files gets the stray one cleaned up on its next run, not just on a fresh install.

Testing

• bash -n, scripts/prose_lint.py, scripts/repo_gate.py: clean.
• shellcheck (koalaman/shellcheck:stable via Docker, per GOVERNANCE.md): clean.
• Real --dry-run against a live host carrying a genuine pre-existing duplicate (/etc/sudoers.d/pieter-global-timestamp): correctly classified it as a pure, this-user duplicate and staged its deletion.
• Isolated fixture harness (fake SUDOERS_FILE and search paths, stubbed visudo/install, no real root writes), covering:
  • pure duplicate elsewhere -> deleted, managed file written
  • mixed-content elsewhere file -> dies, nothing written
  • already-correct managed file, no duplicate -> clean no-op
  • already-correct managed file plus a stray pure duplicate -> stray cleaned up, managed file left untouched
  • a different user's entry -> reported only, survives, write proceeds

[Copilot]

Pull request overview

This pull request tightens install-tools.sh --sudo-timestamp behavior so that, when a conflicting sudo timestamp drop-in already exists, the script can safely remove a redundant “pure” per-user file after confirmation (instead of only warning), while refusing to edit shared/mixed-content sources.

Changes:

• Add logic to detect whether the managed sudoers drop-in is already current and proceed with cleanup even on no-op installs.
• Classify conflicting timestamp settings and, for “pure” per-user duplicates, delete them only after the managed drop-in is validated/in place.
• Improve confirmation/diagnostic messaging around declines and post-change sudoers parse failures.

Suppressed comments (1)

host-setup/linux/install-tools.sh:1166

• The grep -E "Defaults:${user}..." filter treats $user as an ERE fragment. If the username contains regex metacharacters (notably '.'), the candidate set can include unintended files and then be deleted/blocked incorrectly. Escaping $user before embedding it in the pattern avoids this.

        done < <(grep -E "Defaults:${user}[[:space:]]+timestamp_(type|timeout)=" <<< "$elsewhere" | awk -F: '{print $1}' | sort -u)

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[ptr727]

Answering the suppressed (low-confidence) finding from this review round, 1 of 1 in that block:

host-setup/linux/install-tools.sh:1166 (as of the round's head, 6fe87b8): "The grep -E "Defaults:${user}..." filter treats $user as an ERE fragment. If the username contains regex metacharacters (notably '.'), the candidate set can include unintended files and then be deleted/blocked incorrectly. Escaping $user before embedding it in the pattern avoids this."

Fixed in f9195fd: same root cause as the finding answered on the resolved thread above (sudo_timestamp_file_is_pure's own grep had the identical problem). Added sudo_timestamp_user_re, a pure-bash ERE escaper, and applied it at both call sites. Covered by a new fixture scenario where the target username collides via regex with an unrelated user's own file (john.doe vs john_doe), confirming the unrelated file now survives.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

host-setup/linux/install-tools.sh:1182

• The candidate-file extractor only matches when timestamp_type/timeout is the first Defaults entry after the username. If the timestamp option appears later in the list (for example Defaults:user !authenticate, timestamp_type=global), the file will not be classified as unsafe and the script can proceed while leaving a conflicting setting behind. Match any ordering by allowing .* between the user scope and the timestamp assignment.

        done < <(grep -E "Defaults:${user_re}[[:space:]]+timestamp_(type|timeout)=" <<< "$elsewhere" | awk -F: '{print $1}' | sort -u)

host-setup/linux/install-tools.sh:1120

• sudo_timestamp_file_is_pure can classify a file as "pure" even when it contains other Defaults on the same line (for example Defaults:user timestamp_type=global,requiretty) because the allowlist regex only checks the timestamp prefix. Also, a grep read error currently falls back to treating the file as pure, which could delete a file that couldn't be inspected. Tighten the allowlist to accept only timestamp_type/timeout assignments (optionally comma-separated) and treat grep failure as not pure/unsafe.

This issue also appears on line 1182 of the same file.

    local user="$1" file="$2" other user_re
    user_re=$(sudo_timestamp_user_re "$user")
    other=$("${SUDO[@]}" grep -vE "^[[:space:]]*(#.*)?\$|^[[:space:]]*Defaults:${user_re}[[:space:]]+timestamp_(type|timeout)=" \
        "$file" 2> /dev/null) || other=""
    [[ -z $other ]]

[ptr727]

Answering both suppressed (low-confidence) findings from this review round, 2 of 2 in that block:

host-setup/linux/install-tools.sh:1182 (as of the round's head, f9195fd): "The candidate-file extractor only matches when timestamp_type/timeout is the first Defaults entry after the username... Match any ordering by allowing .* between the user scope and the timestamp assignment."

Fixed in 238bd3d: widened the elsewhere classifier's grep to Defaults:${user_re}[[:space:]]+.*timestamp_(type|timeout)=, matching the timestamp assignment anywhere after the required username-boundary whitespace (the boundary itself stays, so a user like john still can't match inside johnson). Covered by a new fixture scenario with Defaults:user !authenticate, timestamp_type=global, now correctly recognized and refused as unsafe to auto-delete, plus a regression scenario proving the john/johnson boundary still holds.

host-setup/linux/install-tools.sh:1120 (same round, same head): "sudo_timestamp_file_is_pure can classify a file as 'pure' even when it contains other Defaults on the same line... a grep read error currently falls back to treating the file as pure... Tighten the allowlist to accept only timestamp_type/timeout assignments (optionally comma-separated) and treat grep failure as not pure/unsafe."

Fixed in 238bd3d: the allowlist is now anchored at both ends (^...$) and accepts only comma-separated timestamp_type/timestamp_timeout assignments, so Defaults:user timestamp_type=global,requiretty no longer passes as a prefix match. Also reads grep's actual exit status now instead of collapsing "no disallowed line" and "read error" into the same fallback; only exit 1 counts as pure, everything else (including a read failure) is treated as not-pure. Covered by two new fixture scenarios: a same-line unrelated option (now refused) and a simulated grep read error on the purity check directly (now correctly reported as not pure).

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

host-setup/linux/install-tools.sh:1187

• The early-return condition ignores unsafe_files. If a mixed-content drop-in sets this user's timestamp option (so it ends up in unsafe_files) and the managed file is already current, the function returns here and never hits the safety die, leaving an unsafe/conflicting configuration unreported.

Include unsafe_files in the early-return guard so mixed-content files for this user still trigger the later error path.

    if [[ $own_current == true && ${#delete_files[@]} -eq 0 ]]; then

[ptr727]

Answering the suppressed (low-confidence) finding from this review round, 1 of 1 in that block:

host-setup/linux/install-tools.sh:1187 (as of the round's head, 238bd3d): "The early-return condition ignores unsafe_files. If a mixed-content drop-in sets this user's timestamp option (so it ends up in unsafe_files) and the managed file is already current, the function returns here and never hits the safety die, leaving an unsafe/conflicting configuration unreported. Include unsafe_files in the early-return guard so mixed-content files for this user still trigger the later error path."

Fixed in 758a964: added && ${#unsafe_files[@]} -eq 0 to the early-return guard, so that combination (already-correct managed file, no pure duplicate, but a standing mixed-content conflict for this user) now falls through to the same warn-and-die path every other unsafe conflict takes, instead of silently returning success. Covered by a new fixture scenario: pre-seed a correct managed file, add a mixed-content file for the same user, run again, confirm it dies rather than reporting "already carries exactly this."

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

host-setup/linux/install-tools.sh:1120

• sudo_timestamp_file_is_pure claims it allows "blank lines and comments", but the allow regex only treats full-line comments as allowable. A drop-in like Defaults:<user> timestamp_type=global # managed will be classified as impure and land in unsafe_files, causing the script to die even though the file still "sets nothing else" beyond a comment. Consider allowing an optional trailing #... comment on otherwise-allowed Defaults lines so safe duplicates with inline comments can still be auto-removed.

    user_re=$(sudo_timestamp_user_re "$user")
    allow="^[[:space:]]*(#.*)?\$|^[[:space:]]*Defaults:${user_re}[[:space:]]+timestamp_(type|timeout)=[^,[:space:]]+(,[[:space:]]*timestamp_(type|timeout)=[^,[:space:]]+)*[[:space:]]*\$"
    "${SUDO[@]}" grep -vE "$allow" "$file" > /dev/null 2>&1 && status=0 || status=$?

[ptr727]

Answering the suppressed (low-confidence) finding from this review round, 1 of 1 in that block:

host-setup/linux/install-tools.sh:1120 (as of the round's head, 758a964): "sudo_timestamp_file_is_pure claims it allows 'blank lines and comments', but the allow regex only treats full-line comments as allowable. A drop-in like Defaults:<user> timestamp_type=global # managed will be classified as impure... Consider allowing an optional trailing #... comment on otherwise-allowed Defaults lines so safe duplicates with inline comments can still be auto-removed."

Two parts, two verdicts:

Fixed in 0353478: the finding is right that the comment overclaimed. Narrowed "aside from blank lines and comments" to "aside from a blank line or a full-line comment," matching what the regex actually accepts.

No change needed on widening the regex to also accept a trailing inline comment on an otherwise-safe Defaults line. Declining, not deferring: every fix across this PR's 4 review rounds moved the same direction, tighten the allowlist, refuse rather than guess, treat an unhandled shape as unsafe rather than assume it is fine (the SC1087 escaping, the anchored allowlist, the read-error fail-closed change, and the unsafe_files early-return gap were all exactly this). Widening the regex to accept an authoring style like Defaults:user timestamp_type=global # managed moves the opposite way for a case that doesn't arise from this feature's own output (sudo_timestamp_content never writes a trailing comment) and isn't how the one real drop-in this PR was built against (/etc/sudoers.d/pieter-global-timestamp, verified on a live host during development) is authored either. A full-line comment already passes. If a real drop-in with a trailing inline comment shows up later, that's a concrete case to widen the allowlist for then, rather than speculative permissiveness added now.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

host-setup/linux/install-tools.sh:1119

• In sudo_timestamp_file_is_pure, the allowlist regex uses \$ at line ends (e.g. (#.*)?\$). In POSIX ERE (grep -E), \$ matches a literal $, not the end-of-line anchor. That will make the allowlist fail to match normal lines, so files that are actually "pure" will be treated as impure/unsafe and trigger the manual-fix die path.

    allow="^[[:space:]]*(#.*)?\$|^[[:space:]]*Defaults:${user_re}[[:space:]]+timestamp_(type|timeout)=[^,[:space:]]+(,[[:space:]]*timestamp_(type|timeout)=[^,[:space:]]+)*[[:space:]]*\$"

[ptr727]

Answering the suppressed (low-confidence) finding from this review round, 1 of 1 in that block:

host-setup/linux/install-tools.sh:1119 (as of the round's head, 0353478): "the allowlist regex uses \$ at line ends... In POSIX ERE (grep -E), \$ matches a literal $, not the end-of-line anchor. That will make the allowlist fail to match normal lines, so files that are actually 'pure' will be treated as impure/unsafe and trigger the manual-fix die path."

Disproven, verified against the running shell rather than argued from the source text alone: allow= is a double-quoted bash assignment, and bash's own quote processing strips a backslash before $ there before grep ever sees the string. I diffed the stored value of allow built with \$ against the same string built with a bare $, byte for byte identical output. I also ran the actual allow regex from this function against a genuinely pure two-line file: 0 lines failed to match (exit 1, correctly classified pure). Every one of the 11 fixture scenarios already covering this PR exercises this exact regex, including several that require a file to be classified pure to pass, and all still pass.

That said, the finding correctly named a real source of confusion, a \$ inside a double-quoted string reads as "escaped-for-grep" to a human or a static analyzer that doesn't trace through bash's own quote processing, and it produced exactly that misreading here. Fixed in 38115d4: dropped the now-provably-redundant backslash and added a comment stating why a bare $ is correct in this position, so the same misreading doesn't recur.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

host-setup/linux/install-tools.sh:1107

• In sudo_timestamp_user_re, the case pattern list includes '*' and '?'. In a case statement these are glob wildcards, so this branch matches every character and escapes everything (e.g. 1 becomes \1), which can turn digits into regex backreferences and break matching/classification.

        case "$c" in
            '.' | '[' | $'\\' | '^' | '$' | '(' | ')' | '*' | '+' | '?' | '{' | '}' | '|') out+="\\$c" ;;
            *) out+="$c" ;;

[ptr727]

Answering the suppressed (low-confidence) finding from this review round, 1 of 1 in that block:

host-setup/linux/install-tools.sh:1107 (as of the round's head, 38115d4): "the case pattern list includes '*' and '?'. In a case statement these are glob wildcards, so this branch matches every character and escapes everything (e.g. 1 becomes \1), which can turn digits into regex backreferences and break matching/classification."

Disproven, verified against the running shell: quoting a glob metacharacter in a case pattern suppresses its wildcard meaning, the same rule as [[ $x == '*' ]] testing for a literal asterisk rather than matching anything. I ran a direct probe of this exact case block against '*' '?' '1' 'a' '.' '+': the digit 1 and the letter a both fell through to the catch-all (no escaping), only the six real metacharacters hit the escape branch. I also ran sudo_timestamp_user_re on real usernames: user123 -> user123 (untouched), john.doe -> john\.doe, weird*user -> weird\*user, a?b -> a\?b, exactly the intended behavior. If the '*'/'?' in this pattern list were unquoted, the finding's concern would be exactly right, quoting is what prevents it, not an accident that happens to work. No change made.