fix(security): remove self-certify review bypass; fix hidden commits false positive

git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/PushRecord.java

@@ -114,4 +114,13 @@ public class PushRecord {
 
     /** Attestation record if the push was manually reviewed. */
     private Attestation attestation;
+
+    /**
+     * Transient flag computed by the dashboard {@code PushController#getById} endpoint indicating whether the currently
+     * authenticated user is permitted to self-approve this specific push (i.e. they are the resolved pusher, have
+     * {@code ROLE_SELF_CERTIFY}, and have a {@code SELF_CERTIFY} repo permission for this push's path). Not persisted
+     * to the database. Used by the frontend to gate the self-certify banner and approve button.
+     */
+    @Builder.Default
+    private boolean canCurrentUserSelfCertify = false;
 }

git-proxy-java-core/src/main/java/org/finos/gitproxy/git/ApprovalPreReceiveHook.java

@@ -103,6 +103,12 @@ var record = pushStore.findById(validationRecordId).orElse(null);
 
         // Safety net: already approved before this hook ran (race condition or re-push)
         if (record.getStatus() == PushStatus.APPROVED) {
+            if (!verifySelfApprovalEntitled(record)) {
+                String reason = "Self-approved push rejected: no SELF_CERTIFY permission for this repository";
+                sendAndFlush(rp, msgOut, color(RED, "" + sym(CROSS_MARK) + "  " + reason));
+                rejectAll(commands, reason);
+                return;
+            }
             sendAndFlush(rp, msgOut, color(GREEN, "" + sym(HEAVY_CHECK_MARK) + "  Push already approved - forwarding"));
             return;
         }
@@ -115,34 +121,6 @@ var record = pushStore.findById(validationRecordId).orElse(null);
                 return;
             }
 
-            // Trusted contributor bypass: auto-approve without human review
-            if (repoPermissionService != null) {
-                String username = record.getUser();
-                String provider = record.getProvider();
-                String path = record.getUrl();
-                if (username != null
-                        && provider != null
-                        && path != null
-                        && repoPermissionService.isBypassReviewAllowed(username, provider, path)) {
-                    pushStore.approve(
-                            validationRecordId,
-                            Attestation.builder()
-                                    .pushId(validationRecordId)
-                                    .type(Attestation.Type.APPROVAL)
-                                    .reviewerUsername(username)
-                                    .reason("Self-certified by " + username)
-                                    .automated(true)
-                                    .build());
-                    sendAndFlush(
-                            rp,
-                            msgOut,
-                            color(
-                                    GREEN,
-                                    "" + sym(HEAVY_CHECK_MARK) + "  Self-certified — push approved automatically"));
-                    return;
-                }
-            }
-
             sendAndFlush(
                     rp, msgOut, color(YELLOW, "" + sym(WARNING) + "  Push requires review. Waiting for approval..."));
             sendAndFlush(rp, msgOut, color(CYAN, "" + sym(KEY) + "  Push ID: " + validationRecordId));
@@ -172,8 +150,16 @@ var record = pushStore.findById(validationRecordId).orElse(null);
             }
 
             switch (result) {
-                case APPROVED ->
+                case APPROVED -> {
+                    var approvedRecord = pushStore.findById(validationRecordId).orElse(null);
+                    if (approvedRecord != null && !verifySelfApprovalEntitled(approvedRecord)) {
+                        String reason = "Self-approved push rejected: no SELF_CERTIFY permission for this repository";
+                        sendAndFlush(rp, msgOut, color(RED, "" + sym(CROSS_MARK) + "  " + reason));
+                        rejectAll(commands, reason);
+                        return;
+                    }
                     sendAndFlush(rp, msgOut, color(GREEN, "" + sym(HEAVY_CHECK_MARK) + "  Push approved by reviewer"));
+                }
                 case REJECTED -> {
                     var updated = pushStore.findById(validationRecordId).orElse(null);
                     String reason = updated != null && updated.getAttestation() != null
@@ -200,6 +186,39 @@ var record = pushStore.findById(validationRecordId).orElse(null);
         }
     }
 
+    /**
+     * Defense in depth: if the approver is the pusher, re-verify that a {@code SELF_CERTIFY} repo permission still
+     * exists for the pusher on this push's path. {@link org.finos.gitproxy.dashboard.controller.PushController#approve}
+     * already enforces this at approval time, but re-checking here protects against future code paths or bugs that mark
+     * a record APPROVED without going through that gate.
+     *
+     * <p>The {@code ROLE_SELF_CERTIFY} role check is intentionally NOT performed at the hook layer — it requires Spring
+     * Security context (only available in the dashboard) and may live in IdP-derived authorities that aren't persisted
+     * to the user store. The hook re-verifies only the per-repo permission, which is the more granular authoritative
+     * gate.
+     *
+     * @return {@code true} if the push may proceed; {@code false} if the approver was the pusher and no
+     *     {@code SELF_CERTIFY} permission row exists.
+     */
+    private boolean verifySelfApprovalEntitled(org.finos.gitproxy.db.model.PushRecord record) {
+        if (repoPermissionService == null) return true;
+        Attestation att = record.getAttestation();
+        if (att == null) return true;
+        String pusher = record.getResolvedUser();
+        String approver = att.getReviewerUsername();
+        if (pusher == null || approver == null || !pusher.equals(approver)) return true;
+        if (record.getProvider() == null || record.getUrl() == null) return true;
+        boolean entitled = repoPermissionService.isBypassReviewAllowed(pusher, record.getProvider(), record.getUrl());
+        if (!entitled) {
+            log.warn(
+                    "Self-approval rejected at hook: pusher={} provider={} path={} has no SELF_CERTIFY permission",
+                    pusher,
+                    record.getProvider(),
+                    record.getUrl());
+        }
+        return entitled;
+    }
+
     private void rejectAll(Collection<ReceiveCommand> commands, String reason) {
         for (ReceiveCommand cmd : commands) {
             if (cmd.getResult() == ReceiveCommand.Result.NOT_ATTEMPTED) {

git-proxy-java-core/src/main/java/org/finos/gitproxy/git/LocalRepositoryCache.java

@@ -122,9 +122,7 @@ public Repository getOrClone(String remoteUrl, CredentialsProvider credentials)
         CachedRepository cached = cache.get(cacheKey);
         if (cached != null && cached.isValid()) {
             log.debug("Using cached repository for: {}", remoteUrl);
-            if (credentials != null) {
-                refreshIfStale(cached, cacheKey, credentials);
-            }
+            refreshIfStale(cached, cacheKey, credentials);
             cached.repository.incrementOpen();
             return cached.repository;
         }

git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/GitProxyServlet.java

@@ -23,11 +23,6 @@ public class GitProxyServlet extends AsyncProxyServlet.Transparent {
     public static final String GIT_REQUEST_ATTR = "gitproxy.gitRequest";
     public static final String ERROR_ATTR = "gitproxy.error";
     public static final String PRE_APPROVED_ATTR = "gitproxy.preApproved";
-    /**
-     * Request attribute set by {@code PushFinalizerFilter} when a self-certify grant was applied. Value is the resolved
-     * proxy username.
-     */
-    public static final String SELF_CERTIFY_USER_ATTR = "gitproxy.selfCertifyUser";
     /** Request attribute holding the UUID of the original APPROVED push record for a transparent-proxy re-push. */
     public static final String APPROVED_PUSH_ID_ATTR = "gitproxy.approvedPushId";
 

git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/CheckHiddenCommitsFilter.java

@@ -85,7 +85,7 @@ public void doHttpFilter(HttpServletRequest request, HttpServletResponse respons
                 return;
             }
 
-            Set<String> allNew = collectAllNewCommits(repository, toCommit);
+            Set<String> allNew = collectAllNewCommits(repository, toCommit, requestDetails.getCommitFrom());
 
             Set<String> hidden = new HashSet<>(allNew);
             hidden.removeAll(introduced);
@@ -115,9 +115,14 @@ public void doHttpFilter(HttpServletRequest request, HttpServletResponse respons
 
     /**
      * Collect all commits reachable from {@code toCommit} that are not reachable from any existing ref in the upstream
-     * clone. These correspond to the commits that were new to the upstream in this pack.
+     * clone or from {@code fromCommit} (the old branch tip). These correspond to the commits that were genuinely new
+     * and not already present at the upstream.
+     *
+     * <p>{@code fromCommit} is marked uninteresting explicitly because the local clone's refs may be stale (e.g. after
+     * a recently forwarded push the remote ref has advanced but the cache hasn't been re-fetched). Without this,
+     * commits reachable only from the old tip — but not from any cached ref — would be falsely flagged as hidden.
      */
-    private Set<String> collectAllNewCommits(Repository repo, String toCommit) throws IOException {
+    private Set<String> collectAllNewCommits(Repository repo, String toCommit, String fromCommit) throws IOException {
         Set<String> result = new HashSet<>();
 
         try (RevWalk walk = new RevWalk(repo)) {
@@ -139,6 +144,19 @@ private Set<String> collectAllNewCommits(Repository repo, String toCommit) throw
                 }
             }
 
+            // Always mark commitFrom (old branch tip) as uninteresting — it is already at the
+            // upstream by definition, even when the local clone's refs are stale.
+            if (fromCommit != null && !fromCommit.isBlank() && !fromCommit.matches("^0+$")) {
+                try {
+                    ObjectId fromId = repo.resolve(fromCommit);
+                    if (fromId != null) {
+                        walk.markUninteresting(walk.parseCommit(fromId));
+                    }
+                } catch (Exception e) {
+                    log.debug("Could not resolve commitFrom {} as uninteresting boundary — skipping", fromCommit);
+                }
+            }
+
             for (RevCommit commit : walk) {
                 result.add(commit.getName());
             }

git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/PushFinalizerFilter.java

@@ -20,7 +20,6 @@
 import org.finos.gitproxy.approval.ApprovalGateway;
 import org.finos.gitproxy.git.GitRequestDetails;
 import org.finos.gitproxy.git.HttpOperation;
-import org.finos.gitproxy.permission.RepoPermissionService;
 
 /**
  * Terminal filter for push operations that determines the final result:
@@ -47,18 +46,11 @@ public class PushFinalizerFilter extends AbstractGitProxyFilter {
 
     private final String serviceUrl;
     private final ApprovalGateway approvalGateway;
-    private final RepoPermissionService repoPermissionService;
 
     public PushFinalizerFilter(String serviceUrl, ApprovalGateway approvalGateway) {
-        this(serviceUrl, approvalGateway, null);
-    }
-
-    public PushFinalizerFilter(
-            String serviceUrl, ApprovalGateway approvalGateway, RepoPermissionService repoPermissionService) {
         super(ORDER, Set.of(HttpOperation.PUSH));
         this.serviceUrl = serviceUrl;
         this.approvalGateway = approvalGateway;
-        this.repoPermissionService = repoPermissionService;
     }
 
     /**
@@ -123,23 +115,12 @@ public void doHttpFilter(HttpServletRequest request, HttpServletResponse respons
             return;
         }
 
-        // Trusted contributor bypass: auto-approve without human review
-        if (repoPermissionService != null) {
-            String username = details.getResolvedUser();
-            String provider =
-                    details.getProvider() != null ? details.getProvider().getProviderId() : null;
-            String path = details.getRepoRef() != null ? details.getRepoRef().getSlug() : null;
-            if (username != null
-                    && provider != null
-                    && path != null
-                    && repoPermissionService.isBypassReviewAllowed(username, provider, path)) {
-                request.setAttribute(SELF_CERTIFY_USER_ATTR, username);
-                details.setResult(GitRequestDetails.GitResult.ALLOWED);
-                return;
-            }
-        }
-
-        // First push that passed validation - block pending review (dashboard/ServiceNow mode)
+        // First push that passed validation - block pending review (dashboard/ServiceNow mode).
+        // Self-certify is intentionally NOT enforced here: the role check requires Spring Security context
+        // which the proxy filter chain does not have. Self-approval is gated entirely in the dashboard
+        // (PushController.checkReviewerIdentity), where both ROLE_SELF_CERTIFY and the SELF_CERTIFY repo
+        // permission are required. The pre-receive hook re-verifies the per-repo permission as defense in
+        // depth before forwarding an approved self-review.
         details.setResult(GitRequestDetails.GitResult.REVIEW);
         String pushId = details.getId().toString();
         String summary = buildValidationSummary(details.getSteps());

git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/PushStoreAuditFilter.java

@@ -2,7 +2,6 @@
 
 import static org.finos.gitproxy.servlet.GitProxyServlet.GIT_REQUEST_ATTR;
 import static org.finos.gitproxy.servlet.GitProxyServlet.PRE_APPROVED_ATTR;
-import static org.finos.gitproxy.servlet.GitProxyServlet.SELF_CERTIFY_USER_ATTR;
 
 import jakarta.servlet.*;
 import jakarta.servlet.http.HttpServletRequest;
@@ -11,7 +10,6 @@
 import lombok.extern.slf4j.Slf4j;
 import org.finos.gitproxy.db.PushRecordMapper;
 import org.finos.gitproxy.db.PushStore;
-import org.finos.gitproxy.db.model.Attestation;
 import org.finos.gitproxy.db.model.PushRecord;
 import org.finos.gitproxy.git.GitRequestDetails;
 import org.finos.gitproxy.git.HttpOperation;
@@ -56,22 +54,6 @@ private void persistIfPush(ServletRequest request, ServletResponse response) {
             PushRecord record = PushRecordMapper.fromRequestDetails(requestDetails);
             pushStore.save(record);
 
-            // If a self-certify grant was applied, stamp the automated attestation now that
-            // the record exists. PushFinalizerFilter already set the result to ALLOWED so the
-            // saved status is APPROVED.
-            String bypassUser = (String) httpRequest.getAttribute(SELF_CERTIFY_USER_ATTR);
-            if (bypassUser != null) {
-                pushStore.approve(
-                        record.getId(),
-                        Attestation.builder()
-                                .pushId(record.getId())
-                                .type(Attestation.Type.APPROVAL)
-                                .reviewerUsername(bypassUser)
-                                .reason("Self-certified by " + bypassUser)
-                                .automated(true)
-                                .build());
-            }
-
             log.info(
                     "Persisted push record: id={}, repo={}, status={}",
                     record.getId(),