fix(security): remove self-certify review bypass; fix hidden commits false positive#148
Merged
coopernetes merged 2 commits intomainfrom Apr 13, 2026
Merged
Conversation
…false positive Discovered while testing live config reload: force-pushing amended commits was going through the proxy unchallenged. Root cause was a self-certify bypass introduced in 3ed0d4a (feat: add SELF_CERTIFY operation, Apr 10) and shipped in v1.0.0-beta.2. The bypass was a secondary effect: with review skipped entirely, each force push advanced the local clone's cached ref without a fetch, causing the next legitimate incremental push to be falsely rejected by checkHiddenCommits (commitFrom was reachable from the new tip but sat above the stale ref boundary, so RevWalk included it in allNew but not in introduced). Self-certify bypass (PushFinalizerFilter + ApprovalPreReceiveHook): The filter chain was calling isBypassReviewAllowed() and immediately setting result=ALLOWED, forwarding the push without review. This is wrong: self-certify means a user may approve their own pushes in the dashboard, not that they skip the queue. The filter has no Spring Security context to check ROLE_SELF_CERTIFY anyway. Both the filter bypass block and the equivalent hook block are removed. Pushes always land in REVIEW. The hook adds a verifySelfApprovalEntitled() defense-in-depth check: after waitForApproval returns APPROVED, if approver == pusher it re-verifies the per-repo SELF_CERTIFY permission (role was already enforced by PushController at approval time; hook can only verify the perm row). Hidden commits false positive (CheckHiddenCommitsFilter): collectAllNewCommits() built its RevWalk boundary from local clone refs, which were stale after a forwarded push advanced the upstream. Fix: always mark commitFrom as uninteresting before walking. commitFrom is the git client's assertion of the current remote tip — correct by definition regardless of clone staleness. JGit propagates the UNINTERESTING flag to all ancestors of commitFrom, covering force-push parent chains too. LocalRepositoryCache: always call refreshIfStale() on cache hits, not only when credentials are supplied. Dashboard / API: - PushController.getById: add transient canCurrentUserSelfCertify flag (pusher == current user AND ROLE_SELF_CERTIFY AND per-repo permission) - PushDetail.tsx: replace role-only isSelfCertifier check with record.canCurrentUserSelfCertify so banner and button match the gate the API actually enforces - SELF_CERTIFY_USER_ATTR constant and PushStoreAuditFilter reference removed (dead after bypass removal) Tests: drop four bypass tests in PushFinalizerFilterTest and two SELF_CERTIFY_USER_ATTR tests in PushStoreAuditFilterTest. Add four ApprovalPreReceiveHookTest cases for the re-fetch path and the approver-not-pusher fast path. Add five PushControllerTest$GetById cases for canCurrentUserSelfCertify. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Clarifies that holding the role alone is not sufficient — an admin must also grant the per-repo Self-certify permission before self-approval works.
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
1 participant
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.
Summary
PushFinalizerFilterandApprovalPreReceiveHookwere auto-approving pushes from users with theSELF_CERTIFYrepo permission, bypassing review entirely. Self-certify is a dashboard-only flow — pushes must always enter the review queue. Introduced in 3ed0d4a, shipped in v1.0.0-beta.2.CheckHiddenCommitsFilterwas rejecting legitimate incremental pushes because the local clone's refs were stale after forwarded pushes. Fix: markcommitFromas an explicit RevWalk boundary — the git client's assertion of the current remote tip is correct regardless of clone staleness.PushController.getByIdnow computescanCurrentUserSelfCertifyserver-side (pusher == current user ANDROLE_SELF_CERTIFYAND per-repo permission).PushDetail.tsxreplaced the role-only check with this field so the UI banner and button match what the API enforces.ApprovalPreReceiveHookre-verifies the per-repoSELF_CERTIFYpermission afterwaitForApprovalreturns when approver == pusher.Test plan
SELF_CERTIFYrole + permission — push lands in PENDING, not auto-approvedSELF_CERTIFYrole but no per-repo permission — dashboard 403s on approveApprovalPreReceiveHookTest,PushFinalizerFilterTest,PushControllerTest