Skip to content

Fix bugs and hardening issues found by the code audit - #239

Merged
lyrixx merged 1 commit into
mainfrom
audit-fixes
Sep 7, 2026
Merged

lyrixx merged 1 commit into
mainfrom
audit-fixes

Conversation

@lyrixx

@lyrixx lyrixx commented Sep 7, 2026

Copy link
Copy Markdown
Member
  • 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.

- 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.
@lyrixx
lyrixx merged commit 29ebc87 into main Sep 7, 2026
5 checks passed
@lyrixx
lyrixx deleted the audit-fixes branch September 7, 2026 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant