Fix bugs and hardening issues found by the code audit - #239
Merged
Merged
Conversation
- ReferenceBag::getFirstBranch() mixed up two different arrays (reset($this->branches) then current($this->references)) and could return a non-branch reference, or even throw, when no branch exists. It now correctly returns the first branch or false. - Tag::getBodyMessage() used array_shift()+array_pop() instead of the two array_shift() used by Commit::getBodyMessage(), leaving a stray leading newline (or truncating the last line) for annotated tags using the standard "subject\n\nbody" convention. The body extraction now handles both the blank-line-separated and non-separated forms. - Repository::run() can return null instead of throwing when debug=false and the git command fails, but several call sites fed that null straight into trim()/explode()/Diff::parse() or a non-nullable return type. Depending on the call site this silently produced wrong data (Tag::isAnnotated() reporting true for a lightweight tag, PushReference::isForce() defaulting to a coin flip) or crashed with a TypeError (Blob::getContent()). All call sites in Commit, Tag, Blob, Reference, PushReference, Repository and WorkingCopy now check for null and fail predictably. - Repository::shell() interpolated the repository path into the shell command without escaping it (only the env vars were escaped); it is now passed through escapeshellarg(). - Hooks::set() created hook scripts as 0777 (world-writable); they are now created 0700, readable/writable/executable by the owner only. - Replaced the untyped array<string, mixed> "data bag" backing Commit and Reference\Tag with real typed properties, validated at the one point external data enters (setData()/the parser). This removes the Commit.php and Reference\Tag.php entries from the PHPStan baseline (89 -> 51 ignored errors) without resorting to assert()/@var casts. - Minor: fixed the isSuccessFul() casing typo in Admin.php, and dropped composer.json's leftover minimum-stability: dev (dependency resolution is unaffected, verified with composer update --dry-run). Added regression tests for the two bugs, the debug=false fixes, and the new hook permissions.
2 tasks
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.
ReferenceBag::getFirstBranch() mixed up two different arrays
(reset($this->branches) then current($this->references)) and could
return a non-branch reference, or even throw, when no branch exists.
It now correctly returns the first branch or false.
Tag::getBodyMessage() used array_shift()+array_pop() instead of the
two array_shift() used by Commit::getBodyMessage(), leaving a stray
leading newline (or truncating the last line) for annotated tags
using the standard "subject\n\nbody" convention. The body extraction
now handles both the blank-line-separated and non-separated forms.
Repository::run() can return null instead of throwing when
debug=false and the git command fails, but several call sites fed
that null straight into trim()/explode()/Diff::parse() or a
non-nullable return type. Depending on the call site this silently
produced wrong data (Tag::isAnnotated() reporting true for a
lightweight tag, PushReference::isForce() defaulting to a coin
flip) or crashed with a TypeError (Blob::getContent()). All call
sites in Commit, Tag, Blob, Reference, PushReference, Repository and
WorkingCopy now check for null and fail predictably.
Repository::shell() interpolated the repository path into the shell
command without escaping it (only the env vars were escaped);
it is now passed through escapeshellarg().
Hooks::set() created hook scripts as 0777 (world-writable); they are
now created 0700, readable/writable/executable by the owner only.
Replaced the untyped array<string, mixed> "data bag" backing Commit
and Reference\Tag with real typed properties, validated at the one
point external data enters (setData()/the parser). This removes the
Commit.php and Reference\Tag.php entries from the PHPStan baseline
(89 -> 51 ignored errors) without resorting to assert()/@var casts.
Minor: fixed the isSuccessFul() casing typo in Admin.php, and
dropped composer.json's leftover minimum-stability: dev (dependency
resolution is unaffected, verified with composer update --dry-run).
Added regression tests for the two bugs, the debug=false fixes, and
the new hook permissions.