Repository navigation
fix: reject remote upgrade when throttle timestamp cannot be persisted (#636) - #650
Open
pithead-developer[bot] wants to merge 1 commit into
Open
pithead-developer[bot] wants to merge 1 commit into
pithead-developer[bot] wants to merge 1 commit into
Conversation
![pithead-developer[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
#636) The stamp file write silently ignored failures (printf ... || true). When the write could not persist, the throttle state was effectively disabled, allowing unlimited upgrade attempts. Fix: - Validate state directory is root-owned mode 0700 - Reject symlinked stamp paths - Use atomic tmpfile+mv for stamp writes - Close lock descriptor on any failure, return rc 2 Deleted explanatory comments per no-comments rule.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #636
What's the problem
The remote-upgrade throttle's stamp file write silently ignored failures. When
printf '%s\n' "$now" >"$stamp"could not write (e.g. read-only directory, filesystem full, or a symlinked path), the|| trueswallowed the error and the function returned 0 — as if the throttle state had persisted. A subsequent call would also fail to read the stamp (or read a stale zero), resetting the cooldown and allowing unlimited upgrade attempts.How I fixed it
Validate the state directory. The directory must be root-owned with mode 0700. A misconfigured or receiver-malleable state directory is rejected with rc 2.
Validate the stamp path before reading. If a symlink points at the stamp file, it is rejected with rc 2 instead of silently reading a hostile path.
Atomic stamp write. The timestamp is written to a temp file in the same directory, then atomically renamed via
mv -f. If the write or rename fails, the lock is closed and rc 2 is returned. No descriptor leak.No comments in code. Deleted the 5-line function docstring and the 6-line lock failure comment. Explanations live in this PR body and the docs.
Here is the proof
bash -n rigforge.sh tests/run.sh— PASS.shellcheck --severity=warning rigforge.sh tests/run.sh— PASS.git diff --check— PASS.FILE_BUDGET_REQUIRE_BASE=1 bash scripts/lint-file-budget.sh— FAIL (tests/run.sh is 9 lines over ceiling; the regression tests for this bug require new test lines). The code change is complete and correct; the budget ceiling would need to be raised.Tests added for #636:
🤖 Generated with Claude Code