From eb037ced448fea78066e3cdf57edfa087d322e47 Mon Sep 17 00:00:00 2001 From: Thomas Cooper Date: Mon, 13 Apr 2026 00:49:31 -0400 Subject: [PATCH 1/2] fix(security): remove self-certify review bypass; fix hidden commits false positive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../finos/gitproxy/db/model/PushRecord.java | 9 ++ .../gitproxy/git/ApprovalPreReceiveHook.java | 77 +++++---- .../gitproxy/git/LocalRepositoryCache.java | 4 +- .../gitproxy/servlet/GitProxyServlet.java | 5 - .../filter/CheckHiddenCommitsFilter.java | 24 ++- .../servlet/filter/PushFinalizerFilter.java | 31 +--- .../servlet/filter/PushStoreAuditFilter.java | 18 --- .../git/ApprovalPreReceiveHookTest.java | 153 ++++++++++++++++++ .../filter/PushFinalizerFilterTest.java | 75 +-------- .../filter/PushStoreAuditFilterTest.java | 34 ---- .../frontend/src/pages/Admin.tsx | 23 +-- .../frontend/src/pages/PushDetail.tsx | 12 +- .../frontend/src/types.ts | 6 + .../controller/ConnectivityController.java | 15 +- .../dashboard/controller/PushController.java | 21 +++ .../ConnectivityErrorClassifierTest.java | 65 +++++--- .../controller/PushControllerTest.java | 67 ++++++++ .../jetty/GitProxyServletRegistrar.java | 2 +- 18 files changed, 410 insertions(+), 231 deletions(-) diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/PushRecord.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/PushRecord.java index 37cda572..161726ca 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/PushRecord.java +++ b/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; } diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/git/ApprovalPreReceiveHook.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/git/ApprovalPreReceiveHook.java index 8305c18c..c4c9141c 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/git/ApprovalPreReceiveHook.java +++ b/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. + * + *

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 commands, String reason) { for (ReceiveCommand cmd : commands) { if (cmd.getResult() == ReceiveCommand.Result.NOT_ATTEMPTED) { diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/git/LocalRepositoryCache.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/git/LocalRepositoryCache.java index 2a80e8b6..22214a8c 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/git/LocalRepositoryCache.java +++ b/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; } diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/GitProxyServlet.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/GitProxyServlet.java index 06ffab45..bd0d1ed0 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/GitProxyServlet.java +++ b/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"; diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/CheckHiddenCommitsFilter.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/CheckHiddenCommitsFilter.java index e856ae28..c0499acc 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/CheckHiddenCommitsFilter.java +++ b/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 allNew = collectAllNewCommits(repository, toCommit); + Set allNew = collectAllNewCommits(repository, toCommit, requestDetails.getCommitFrom()); Set 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. + * + *

{@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 collectAllNewCommits(Repository repo, String toCommit) throws IOException { + private Set collectAllNewCommits(Repository repo, String toCommit, String fromCommit) throws IOException { Set result = new HashSet<>(); try (RevWalk walk = new RevWalk(repo)) { @@ -139,6 +144,19 @@ private Set 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()); } diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/PushFinalizerFilter.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/PushFinalizerFilter.java index ded96fc3..05a29196 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/PushFinalizerFilter.java +++ b/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()); diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/PushStoreAuditFilter.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/PushStoreAuditFilter.java index 6c3df12f..77662ba9 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/PushStoreAuditFilter.java +++ b/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(), diff --git a/git-proxy-java-core/src/test/java/org/finos/gitproxy/git/ApprovalPreReceiveHookTest.java b/git-proxy-java-core/src/test/java/org/finos/gitproxy/git/ApprovalPreReceiveHookTest.java index 3a5422c5..ac0ae805 100644 --- a/git-proxy-java-core/src/test/java/org/finos/gitproxy/git/ApprovalPreReceiveHookTest.java +++ b/git-proxy-java-core/src/test/java/org/finos/gitproxy/git/ApprovalPreReceiveHookTest.java @@ -20,8 +20,10 @@ import org.finos.gitproxy.approval.ApprovalGateway; import org.finos.gitproxy.approval.ApprovalResult; import org.finos.gitproxy.db.PushStore; +import org.finos.gitproxy.db.model.Attestation; import org.finos.gitproxy.db.model.PushRecord; import org.finos.gitproxy.db.model.PushStatus; +import org.finos.gitproxy.permission.RepoPermissionService; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -153,6 +155,157 @@ void blockedPush_gatewayRejects_commandRejected() throws Exception { assertEquals(ReceiveCommand.Result.REJECTED_OTHER_REASON, cmd.getResult()); } + // ── Defense-in-depth: hook re-verifies SELF_CERTIFY perm when approver == pusher ───────────── + + @Test + void selfApproved_alreadyApprovedAtHookStart_noPerm_rejected() throws Exception { + String recordId = UUID.randomUUID().toString(); + repo.getConfig().setString("gitproxy", null, "validationRecordId", recordId); + repo.getConfig().save(); + Attestation att = Attestation.builder() + .pushId(recordId) + .type(Attestation.Type.APPROVAL) + .reviewerUsername("alice") + .build(); + PushRecord record = PushRecord.builder() + .id(recordId) + .status(PushStatus.APPROVED) + .resolvedUser("alice") + .provider("github/github.com") + .url("/owner/repo") + .attestation(att) + .build(); + when(pushStore.findById(recordId)).thenReturn(Optional.of(record)); + RepoPermissionService perms = mock(RepoPermissionService.class); + when(perms.isBypassReviewAllowed("alice", "github/github.com", "/owner/repo")) + .thenReturn(false); + + RevCommit c1 = createCommit("init"); + RevCommit c2 = createCommit("second"); + ReceivePack rp = new ReceivePack(repo); + ReceiveCommand cmd = new ReceiveCommand(c1.getId(), c2.getId(), "refs/heads/main", ReceiveCommand.Type.UPDATE); + + new ApprovalPreReceiveHook(pushStore, approvalGateway, Duration.ofSeconds(5), null, perms) + .onPreReceive(rp, List.of(cmd)); + + assertEquals(ReceiveCommand.Result.REJECTED_OTHER_REASON, cmd.getResult()); + verify(perms).isBypassReviewAllowed("alice", "github/github.com", "/owner/repo"); + } + + @Test + void selfApproved_alreadyApprovedAtHookStart_withPerm_passes() throws Exception { + String recordId = UUID.randomUUID().toString(); + repo.getConfig().setString("gitproxy", null, "validationRecordId", recordId); + repo.getConfig().save(); + Attestation att = Attestation.builder() + .pushId(recordId) + .type(Attestation.Type.APPROVAL) + .reviewerUsername("alice") + .build(); + PushRecord record = PushRecord.builder() + .id(recordId) + .status(PushStatus.APPROVED) + .resolvedUser("alice") + .provider("github/github.com") + .url("/owner/repo") + .attestation(att) + .build(); + when(pushStore.findById(recordId)).thenReturn(Optional.of(record)); + RepoPermissionService perms = mock(RepoPermissionService.class); + when(perms.isBypassReviewAllowed("alice", "github/github.com", "/owner/repo")) + .thenReturn(true); + + RevCommit c1 = createCommit("init"); + RevCommit c2 = createCommit("second"); + ReceivePack rp = new ReceivePack(repo); + ReceiveCommand cmd = new ReceiveCommand(c1.getId(), c2.getId(), "refs/heads/main", ReceiveCommand.Type.UPDATE); + + new ApprovalPreReceiveHook(pushStore, approvalGateway, Duration.ofSeconds(5), null, perms) + .onPreReceive(rp, List.of(cmd)); + + assertEquals(ReceiveCommand.Result.NOT_ATTEMPTED, cmd.getResult()); + } + + @Test + void selfApproved_viaWaitForApproval_noPerm_rejected() throws Exception { + String recordId = UUID.randomUUID().toString(); + repo.getConfig().setString("gitproxy", null, "validationRecordId", recordId); + repo.getConfig().save(); + // Initial fetch returns PENDING (no attestation yet); after approval, returns APPROVED with attestation + // showing the pusher self-approved. + PushRecord pending = PushRecord.builder() + .id(recordId) + .status(PushStatus.PENDING) + .resolvedUser("alice") + .provider("github/github.com") + .url("/owner/repo") + .build(); + Attestation att = Attestation.builder() + .pushId(recordId) + .type(Attestation.Type.APPROVAL) + .reviewerUsername("alice") + .build(); + PushRecord approved = PushRecord.builder() + .id(recordId) + .status(PushStatus.APPROVED) + .resolvedUser("alice") + .provider("github/github.com") + .url("/owner/repo") + .attestation(att) + .build(); + when(pushStore.findById(recordId)).thenReturn(Optional.of(pending)).thenReturn(Optional.of(approved)); + when(approvalGateway.waitForApproval(eq(recordId), any(), any(Duration.class))) + .thenReturn(ApprovalResult.APPROVED); + RepoPermissionService perms = mock(RepoPermissionService.class); + when(perms.isBypassReviewAllowed("alice", "github/github.com", "/owner/repo")) + .thenReturn(false); + + RevCommit c1 = createCommit("init"); + RevCommit c2 = createCommit("second"); + ReceivePack rp = new ReceivePack(repo); + ReceiveCommand cmd = new ReceiveCommand(c1.getId(), c2.getId(), "refs/heads/main", ReceiveCommand.Type.UPDATE); + + new ApprovalPreReceiveHook(pushStore, approvalGateway, Duration.ofSeconds(5), null, perms) + .onPreReceive(rp, List.of(cmd)); + + assertEquals(ReceiveCommand.Result.REJECTED_OTHER_REASON, cmd.getResult()); + verify(perms).isBypassReviewAllowed("alice", "github/github.com", "/owner/repo"); + } + + @Test + void differentApproverThanPusher_noReVerifyNeeded() throws Exception { + // Approver != pusher → defense-in-depth check skipped; push is forwarded. + String recordId = UUID.randomUUID().toString(); + repo.getConfig().setString("gitproxy", null, "validationRecordId", recordId); + repo.getConfig().save(); + Attestation att = Attestation.builder() + .pushId(recordId) + .type(Attestation.Type.APPROVAL) + .reviewerUsername("bob") + .build(); + PushRecord record = PushRecord.builder() + .id(recordId) + .status(PushStatus.APPROVED) + .resolvedUser("alice") + .provider("github/github.com") + .url("/owner/repo") + .attestation(att) + .build(); + when(pushStore.findById(recordId)).thenReturn(Optional.of(record)); + RepoPermissionService perms = mock(RepoPermissionService.class); + + RevCommit c1 = createCommit("init"); + RevCommit c2 = createCommit("second"); + ReceivePack rp = new ReceivePack(repo); + ReceiveCommand cmd = new ReceiveCommand(c1.getId(), c2.getId(), "refs/heads/main", ReceiveCommand.Type.UPDATE); + + new ApprovalPreReceiveHook(pushStore, approvalGateway, Duration.ofSeconds(5), null, perms) + .onPreReceive(rp, List.of(cmd)); + + assertEquals(ReceiveCommand.Result.NOT_ATTEMPTED, cmd.getResult()); + verifyNoInteractions(perms); + } + @Test void blockedPush_gatewayTimesOut_commandRejected() throws Exception { String recordId = UUID.randomUUID().toString(); diff --git a/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/PushFinalizerFilterTest.java b/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/PushFinalizerFilterTest.java index 5befa05d..205bd99c 100644 --- a/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/PushFinalizerFilterTest.java +++ b/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/PushFinalizerFilterTest.java @@ -3,7 +3,6 @@ import static org.finos.gitproxy.git.GitClientUtils.ZERO_OID; 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 static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.*; @@ -26,7 +25,6 @@ import org.finos.gitproxy.db.PushStoreFactory; import org.finos.gitproxy.git.GitRequestDetails; import org.finos.gitproxy.git.HttpOperation; -import org.finos.gitproxy.permission.RepoPermissionService; import org.finos.gitproxy.provider.GitProxyProvider; import org.junit.jupiter.api.Test; @@ -196,31 +194,13 @@ void nullDetails_doesNotThrow() throws Exception { } @Test - void selfCertify_granted_allowsPushAndSetsAttribute() throws Exception { + void selfCertify_perm_doesNotBypassReview() throws Exception { + // The push-time bypass for SELF_CERTIFY was removed: self-certify is enforced exclusively in the dashboard + // (PushController.checkReviewerIdentity). The pre-receive hook re-verifies the per-repo permission as defense + // in depth before forwarding an approved self-review. From the proxy filter chain's perspective, every clean + // push with no prior approval blocks pending review regardless of any SELF_CERTIFY grants. GitRequestDetails details = pendingPushDetailsWithUser("alice", "github"); - RepoPermissionService perms = mock(RepoPermissionService.class); - when(perms.isBypassReviewAllowed("alice", "github/github.com", "/owner/repo")) - .thenReturn(true); - PushFinalizerFilter filter = - new PushFinalizerFilter("http://localhost:8080", mock(ApprovalGateway.class), perms); - HttpServletRequest req = mockPushRequest(details); - FakeResponse fakeResponse = new FakeResponse(); - - filter.doHttpFilter(req, fakeResponse.mock); - - assertEquals(GitRequestDetails.GitResult.ALLOWED, details.getResult()); - verify(req).setAttribute(SELF_CERTIFY_USER_ATTR, "alice"); - assertFalse(fakeResponse.committed.get(), "Self-certify must not send a git error to the client"); - } - - @Test - void selfCertify_notGranted_blocksPendingReview() throws Exception { - GitRequestDetails details = pendingPushDetailsWithUser("alice", "github"); - RepoPermissionService perms = mock(RepoPermissionService.class); - when(perms.isBypassReviewAllowed("alice", "github/github.com", "/owner/repo")) - .thenReturn(false); - PushFinalizerFilter filter = - new PushFinalizerFilter("http://localhost:8080", mock(ApprovalGateway.class), perms); + PushFinalizerFilter filter = new PushFinalizerFilter("http://localhost:8080", mock(ApprovalGateway.class)); FakeResponse fakeResponse = new FakeResponse(); filter.doHttpFilter(mockPushRequest(details), fakeResponse.mock); @@ -229,49 +209,6 @@ void selfCertify_notGranted_blocksPendingReview() throws Exception { assertTrue(fakeResponse.committed.get()); } - @Test - void selfCertify_nullProvider_fallsThroughToReview() throws Exception { - // repoPermissionService non-null, resolvedUser set, but no provider → provider ternary takes null branch - // compound condition: username!=null && provider!=null → false → bypass skipped - GitRequestDetails details = pendingPushDetails(); - details.setResolvedUser("alice"); // so username != null; provider is still null - RepoPermissionService perms = mock(RepoPermissionService.class); - PushFinalizerFilter filter = - new PushFinalizerFilter("http://localhost:8080", mock(ApprovalGateway.class), perms); - FakeResponse fakeResponse = new FakeResponse(); - - filter.doHttpFilter(mockPushRequest(details), fakeResponse.mock); - - assertEquals(GitRequestDetails.GitResult.REVIEW, details.getResult()); - verifyNoInteractions(perms); - } - - @Test - void selfCertify_nullRepoRef_fallsThroughToReview() throws Exception { - // repoPermissionService non-null, provider set, but repoRef is null → path ternary takes null branch → bypass - // skipped - GitRequestDetails details = new GitRequestDetails(); - details.setOperation(HttpOperation.PUSH); - details.setCommitTo("abc123"); - details.setResolvedUser("alice"); - GitProxyProvider provider = mock(GitProxyProvider.class); - when(provider.getName()).thenReturn("github"); - when(provider.getType()).thenReturn("github"); - when(provider.getUri()).thenReturn(java.net.URI.create("https://github.com")); - when(provider.getProviderId()).thenReturn("github/github.com"); - details.setProvider(provider); - // repoRef intentionally not set - RepoPermissionService perms = mock(RepoPermissionService.class); - PushFinalizerFilter filter = - new PushFinalizerFilter("http://localhost:8080", mock(ApprovalGateway.class), perms); - FakeResponse fakeResponse = new FakeResponse(); - - filter.doHttpFilter(mockPushRequest(details), fakeResponse.mock); - - assertEquals(GitRequestDetails.GitResult.REVIEW, details.getResult()); - verifyNoInteractions(perms); - } - @Test void autoApprovalGateway_allowsPushWithoutBlocking() throws Exception { GitRequestDetails details = pendingPushDetails(); diff --git a/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/PushStoreAuditFilterTest.java b/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/PushStoreAuditFilterTest.java index fd61f76b..17e720d5 100644 --- a/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/PushStoreAuditFilterTest.java +++ b/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/PushStoreAuditFilterTest.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 static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.*; @@ -12,7 +11,6 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; 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; @@ -141,38 +139,6 @@ void preApprovedRePush_notPersisted() throws Exception { verifyNoInteractions(pushStore); } - // ---- self-certify bypass stamps automated attestation ---- - - @Test - void selfCertifyBypass_stampsAutomatedAttestation() throws Exception { - FilterChain chain = mock(FilterChain.class); - HttpServletRequest req = pushRequest(pushDetails()); - when(req.getAttribute(SELF_CERTIFY_USER_ATTR)).thenReturn("alice"); - - filter.doFilter(req, mock(HttpServletResponse.class), chain); - - verify(pushStore).save(any(PushRecord.class)); - verify(pushStore) - .approve( - any(), - argThat((Attestation a) -> a.isAutomated() - && "alice".equals(a.getReviewerUsername()) - && a.getReason() != null - && a.getReason().contains("alice"))); - } - - @Test - void noBypasAttribute_noApproveCall() throws Exception { - FilterChain chain = mock(FilterChain.class); - HttpServletRequest req = pushRequest(pushDetails()); - when(req.getAttribute(SELF_CERTIFY_USER_ATTR)).thenReturn(null); - - filter.doFilter(req, mock(HttpServletResponse.class), chain); - - verify(pushStore).save(any(PushRecord.class)); - verify(pushStore, never()).approve(any(), any()); - } - // ---- store failure does not propagate (audit must not break pushes) ---- @Test diff --git a/git-proxy-java-dashboard/frontend/src/pages/Admin.tsx b/git-proxy-java-dashboard/frontend/src/pages/Admin.tsx index f29d41ff..778a2db9 100644 --- a/git-proxy-java-dashboard/frontend/src/pages/Admin.tsx +++ b/git-proxy-java-dashboard/frontend/src/pages/Admin.tsx @@ -62,9 +62,7 @@ function HttpBadge({ http }: { http: HttpResult | null }) { function GitProbeBadge({ label, result }: { label: string; result: GitProbeResult }) { const ok = result.status === 'ok' - const detail = ok - ? `${result.httpStatus} ${ms(result.durationMs)}` - : (result.error ?? 'ERROR') + const detail = ok ? `${result.httpStatus} ${ms(result.durationMs)}` : (result.error ?? 'ERROR') return ( )} - {result.gitProbe.receivePack.status === 'ok' && result.gitProbe.receivePack.contentType && ( -

- push content-type: {result.gitProbe.receivePack.contentType} -
- )} + {result.gitProbe.receivePack.status === 'ok' && + result.gitProbe.receivePack.contentType && ( +
+ push content-type: {result.gitProbe.receivePack.contentType} +
+ )} )} {result.steps && result.steps.length > 0 && } @@ -227,7 +226,9 @@ export function Admin() { const [repoPath, setRepoPath] = useState('') const [targetStatus, setTargetStatus] = useState<'idle' | 'loading' | 'done' | 'error'>('idle') const [targetCheckedAt, setTargetCheckedAt] = useState(null) - const [targetResults, setTargetResults] = useState | null>(null) + const [targetResults, setTargetResults] = useState | null>( + null, + ) const [targetError, setTargetError] = useState(null) useEffect(() => { @@ -391,7 +392,9 @@ export function Admin() {
diff --git a/git-proxy-java-dashboard/frontend/src/pages/PushDetail.tsx b/git-proxy-java-dashboard/frontend/src/pages/PushDetail.tsx index b3b1a903..775c1141 100644 --- a/git-proxy-java-dashboard/frontend/src/pages/PushDetail.tsx +++ b/git-proxy-java-dashboard/frontend/src/pages/PushDetail.tsx @@ -883,13 +883,17 @@ export function PushDetail({ currentUser }: PushDetailProps) { {record.status === 'PENDING' && (() => { const isAdmin = currentUser?.authorities?.includes('ROLE_ADMIN') ?? false - const isSelfCertifier = - currentUser?.authorities?.includes('ROLE_SELF_CERTIFY') ?? false + // canCurrentUserSelfCertify is computed server-side and requires BOTH the + // ROLE_SELF_CERTIFY authority AND a SELF_CERTIFY repo permission row for this push's + // path. A user with the role but no per-repo permission gets `false` here, so the + // self-certify banner stays hidden and the approve button is gated behind the + // standard self-review block. + const canSelfCertify = record.canCurrentUserSelfCertify ?? false const isPusher = !!currentUser?.username && !!record.resolvedUser && currentUser.username === record.resolvedUser - const isSelfReview = isPusher && !isAdmin && !isSelfCertifier + const isSelfReview = isPusher && !isAdmin && !canSelfCertify const canCancel = isAdmin || isPusher const attestationsComplete = attestationQuestions .filter((q) => q.required) @@ -927,7 +931,7 @@ export function PushDetail({ currentUser }: PushDetailProps) {
)} - {isSelfCertifier && isPusher && !isAdmin && ( + {canSelfCertify && isPusher && !isAdmin && (
ℹ diff --git a/git-proxy-java-dashboard/frontend/src/types.ts b/git-proxy-java-dashboard/frontend/src/types.ts index cb0276bd..6ce9abee 100644 --- a/git-proxy-java-dashboard/frontend/src/types.ts +++ b/git-proxy-java-dashboard/frontend/src/types.ts @@ -71,6 +71,12 @@ export interface PushRecord { attestation?: Attestation commits?: Commit[] steps?: Step[] + /** + * Server-computed flag (only set on GET /api/push/{id}): the current authenticated user is the resolved pusher, + * holds ROLE_SELF_CERTIFY, AND has a SELF_CERTIFY repo permission for this push's path. Gates the self-certify + * banner and approve button in the UI. + */ + canCurrentUserSelfCertify?: boolean } export interface Provider { diff --git a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/ConnectivityController.java b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/ConnectivityController.java index 7236f076..1a335368 100644 --- a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/ConnectivityController.java +++ b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/ConnectivityController.java @@ -45,10 +45,9 @@ *
  • TLS — complete the TLS handshake (HTTPS providers only). Reports negotiated protocol and cipher, or the * specific exception class for certificate / SNI failures. *
  • HTTP — send {@code GET /} and record the status code and response time. - *
  • Git probe (targeted check only) — send {@code GET /info/refs?service=git-upload-pack} and - * {@code GET /info/refs?service=git-receive-pack} with {@code User-Agent: git/2.x.x} to a specific repo URL. - * Distinguishes appliances that pass generic HTTP but block git-specific URL patterns, query strings, or - * user-agent strings. + *
  • Git probe (targeted check only) — send {@code GET /info/refs?service=git-upload-pack} and {@code GET + * /info/refs?service=git-receive-pack} with {@code User-Agent: git/2.x.x} to a specific repo URL. Distinguishes + * appliances that pass generic HTTP but block git-specific URL patterns, query strings, or user-agent strings. * * *

    Every step is logged at INFO level. Targeted checks additionally return a structured {@code steps} log in the API @@ -73,8 +72,8 @@ public class ConnectivityController { * optional git probe step. * * @param providerName optional — name of the provider to target; if absent all providers are checked - * @param repoPath optional — repo path (e.g. {@code /owner/repo.git}) appended to the provider base URI for - * the git probe step; requires {@code provider} + * @param repoPath optional — repo path (e.g. {@code /owner/repo.git}) appended to the provider base URI for the git + * probe step; requires {@code provider} */ @GetMapping("/api/admin/connectivity") public Map check( @@ -92,8 +91,8 @@ public Map check( GitProxyProvider provider = providers.getProviders().stream() .filter(p -> p.getName().equals(providerName)) .findFirst() - .orElseThrow(() -> new ResponseStatusException( - HttpStatus.BAD_REQUEST, "Unknown provider: " + providerName)); + .orElseThrow(() -> + new ResponseStatusException(HttpStatus.BAD_REQUEST, "Unknown provider: " + providerName)); log.info("--- Provider: {} ({}) [targeted] ---", provider.getName(), provider.getUri()); List> steps = new ArrayList<>(); Map result = checkProvider(provider, sslContext, steps); diff --git a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PushController.java b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PushController.java index a746d005..1bdc0570 100644 --- a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PushController.java +++ b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PushController.java @@ -122,11 +122,32 @@ public ResponseEntity getById(@PathVariable String id) { .findById(id) .map(record -> { stripDiffContent(record); + record.setCanCurrentUserSelfCertify(computeCanCurrentUserSelfCertify(record)); return ResponseEntity.ok(record); }) .orElse(ResponseEntity.notFound().build()); } + /** + * Computes whether the currently authenticated user is permitted to self-approve this specific push. Mirrors the + * server-side enforcement in {@link #checkReviewerIdentity}: the user must be the resolved pusher, hold the + * {@code ROLE_SELF_CERTIFY} authority (capability gate), and have a matching {@code SELF_CERTIFY} repo permission + * row (per-repo entitlement). Admins do not need self-certify — they can approve anything via the regular path. + */ + private boolean computeCanCurrentUserSelfCertify(PushRecord record) { + Authentication auth = SecurityContextHolder.getContext().getAuthentication(); + if (auth == null) return false; + String reviewer = auth.getName(); + String pusher = record.getResolvedUser(); + if (reviewer == null || pusher == null || !pusher.equals(reviewer)) return false; + boolean hasRole = auth.getAuthorities().stream().anyMatch(a -> "ROLE_SELF_CERTIFY".equals(a.getAuthority())); + if (!hasRole) return false; + return repoPermissionService != null + && record.getProvider() != null + && record.getUrl() != null + && repoPermissionService.isBypassReviewAllowed(reviewer, record.getProvider(), record.getUrl()); + } + /** * Returns the raw unified diff for a push. Separated from the main push record response so that large diffs do not * block page load — the dashboard fetches this lazily and decides whether to render inline or link to the diff --git a/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/ConnectivityErrorClassifierTest.java b/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/ConnectivityErrorClassifierTest.java index a2ba6598..22fe28dd 100644 --- a/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/ConnectivityErrorClassifierTest.java +++ b/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/ConnectivityErrorClassifierTest.java @@ -2,10 +2,10 @@ import static org.junit.jupiter.api.Assertions.assertEquals; +import java.io.IOException; import java.net.ConnectException; import java.net.SocketException; import java.net.SocketTimeoutException; -import java.io.IOException; import javax.net.ssl.SSLHandshakeException; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -17,17 +17,21 @@ class ClassifyTcpError { @Test void socketTimeoutException_returnsTimeout() { - assertEquals("TIMEOUT", ConnectivityErrorClassifier.classifyTcpError(new SocketTimeoutException("timed out"))); + assertEquals( + "TIMEOUT", ConnectivityErrorClassifier.classifyTcpError(new SocketTimeoutException("timed out"))); } @Test void connectException_withRefusedMessage_returnsRefused() { - assertEquals("REFUSED", ConnectivityErrorClassifier.classifyTcpError(new ConnectException("Connection refused"))); + assertEquals( + "REFUSED", + ConnectivityErrorClassifier.classifyTcpError(new ConnectException("Connection refused"))); } @Test void connectException_withResetMessage_returnsReset() { - assertEquals("RESET", ConnectivityErrorClassifier.classifyTcpError(new ConnectException("Connection reset"))); + assertEquals( + "RESET", ConnectivityErrorClassifier.classifyTcpError(new ConnectException("Connection reset"))); } @Test @@ -38,17 +42,21 @@ void connectException_withBlankMessage_returnsRefused() { @Test void socketException_withResetMessage_returnsReset() { - assertEquals("RESET", ConnectivityErrorClassifier.classifyTcpError(new SocketException("Connection reset"))); + assertEquals( + "RESET", ConnectivityErrorClassifier.classifyTcpError(new SocketException("Connection reset"))); } @Test void socketException_withRefusedMessage_returnsRefused() { - assertEquals("REFUSED", ConnectivityErrorClassifier.classifyTcpError(new SocketException("Connection refused"))); + assertEquals( + "REFUSED", ConnectivityErrorClassifier.classifyTcpError(new SocketException("Connection refused"))); } @Test void socketException_withTimedOutMessage_returnsTimeout() { - assertEquals("TIMEOUT", ConnectivityErrorClassifier.classifyTcpError(new SocketException("Connection timed out"))); + assertEquals( + "TIMEOUT", + ConnectivityErrorClassifier.classifyTcpError(new SocketException("Connection timed out"))); } @Test @@ -78,7 +86,8 @@ void directSocketTimeout_returnsTimeout() { @Test void wrappedSocketTimeout_unwrapsAndReturnsTimeout() { // HttpClient wraps the real cause in an outer IOException - IOException wrapper = new IOException("HTTP request failed", new SocketTimeoutException("connect timed out")); + IOException wrapper = + new IOException("HTTP request failed", new SocketTimeoutException("connect timed out")); assertEquals("TIMEOUT", ConnectivityErrorClassifier.classifyNetworkError(wrapper)); } @@ -111,44 +120,56 @@ class ClassifyTlsError { @Test void pkixPathBuilding_returnsCertInvalid() { - assertEquals("TLS_CERT_INVALID", ConnectivityErrorClassifier.classifyTlsError( - new SSLHandshakeException("PKIX path building failed: unable to find valid certification path"))); + assertEquals( + "TLS_CERT_INVALID", + ConnectivityErrorClassifier.classifyTlsError(new SSLHandshakeException( + "PKIX path building failed: unable to find valid certification path"))); } @Test void certificateKeyword_returnsCertInvalid() { - assertEquals("TLS_CERT_INVALID", ConnectivityErrorClassifier.classifyTlsError( - new SSLHandshakeException("Certificate expired"))); + assertEquals( + "TLS_CERT_INVALID", + ConnectivityErrorClassifier.classifyTlsError(new SSLHandshakeException("Certificate expired"))); } @Test void certKeyword_returnsCertInvalid() { - assertEquals("TLS_CERT_INVALID", ConnectivityErrorClassifier.classifyTlsError( - new SSLHandshakeException("self-signed cert in chain"))); + assertEquals( + "TLS_CERT_INVALID", + ConnectivityErrorClassifier.classifyTlsError( + new SSLHandshakeException("self-signed cert in chain"))); } @Test void trustAnchorKeyword_returnsCertInvalid() { - assertEquals("TLS_CERT_INVALID", ConnectivityErrorClassifier.classifyTlsError( - new SSLHandshakeException("No trust anchor found for issuer"))); + assertEquals( + "TLS_CERT_INVALID", + ConnectivityErrorClassifier.classifyTlsError( + new SSLHandshakeException("No trust anchor found for issuer"))); } @Test void trustanchorOneWord_returnsCertInvalid() { - assertEquals("TLS_CERT_INVALID", ConnectivityErrorClassifier.classifyTlsError( - new SSLHandshakeException("trustanchor for certpath not found"))); + assertEquals( + "TLS_CERT_INVALID", + ConnectivityErrorClassifier.classifyTlsError( + new SSLHandshakeException("trustanchor for certpath not found"))); } @Test void generalAlertFromPeer_returnsHandshakeFailed() { - assertEquals("TLS_HANDSHAKE_FAILED", ConnectivityErrorClassifier.classifyTlsError( - new SSLHandshakeException("Received fatal alert: handshake_failure"))); + assertEquals( + "TLS_HANDSHAKE_FAILED", + ConnectivityErrorClassifier.classifyTlsError( + new SSLHandshakeException("Received fatal alert: handshake_failure"))); } @Test void protocolVersionMismatch_returnsHandshakeFailed() { - assertEquals("TLS_HANDSHAKE_FAILED", ConnectivityErrorClassifier.classifyTlsError( - new SSLHandshakeException("No appropriate protocol"))); + assertEquals( + "TLS_HANDSHAKE_FAILED", + ConnectivityErrorClassifier.classifyTlsError(new SSLHandshakeException("No appropriate protocol"))); } } } diff --git a/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/PushControllerTest.java b/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/PushControllerTest.java index c7cd27c6..193749bb 100644 --- a/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/PushControllerTest.java +++ b/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/PushControllerTest.java @@ -175,6 +175,73 @@ void notFound_returns404() { when(pushStore.findById("missing")).thenReturn(Optional.empty()); assertEquals(HttpStatus.NOT_FOUND, controller.getById("missing").getStatusCode()); } + + @Test + void canCurrentUserSelfCertify_falseWhenNotAuthenticated() { + var push = blockedPush("p1", "alice"); + when(pushStore.findById("p1")).thenReturn(Optional.of(push)); + // No SecurityContext authentication set + var resp = controller.getById("p1"); + assertEquals(HttpStatus.OK, resp.getStatusCode()); + assertEquals(false, resp.getBody().isCanCurrentUserSelfCertify()); + } + + @Test + void canCurrentUserSelfCertify_falseWhenViewerIsNotPusher() { + var push = blockedPush("p1", "alice"); + when(pushStore.findById("p1")).thenReturn(Optional.of(push)); + loginAs("bob", false, true); // role granted, but bob is not the pusher + var resp = controller.getById("p1"); + assertEquals(false, resp.getBody().isCanCurrentUserSelfCertify()); + } + + @Test + void canCurrentUserSelfCertify_falseWhenRoleMissing() throws Exception { + var push = blockedPush("p1", "alice"); + when(pushStore.findById("p1")).thenReturn(Optional.of(push)); + loginAs("alice", false, false); // pusher, but no ROLE_SELF_CERTIFY + // Permission service stub intentionally omitted — the role check short-circuits before + // computeCanCurrentUserSelfCertify ever consults the permission service. + repoPermissionService = mock(RepoPermissionService.class); + var field = PushController.class.getDeclaredField("repoPermissionService"); + field.setAccessible(true); + field.set(controller, repoPermissionService); + + var resp = controller.getById("p1"); + assertEquals(false, resp.getBody().isCanCurrentUserSelfCertify()); + } + + @Test + void canCurrentUserSelfCertify_falseWhenPermMissing() throws Exception { + var push = blockedPush("p1", "alice"); + when(pushStore.findById("p1")).thenReturn(Optional.of(push)); + loginAs("alice", false, true); // pusher with role + repoPermissionService = mock(RepoPermissionService.class); + when(repoPermissionService.isBypassReviewAllowed("alice", "github", "github.com/acme/repo.git")) + .thenReturn(false); + var field = PushController.class.getDeclaredField("repoPermissionService"); + field.setAccessible(true); + field.set(controller, repoPermissionService); + + var resp = controller.getById("p1"); + assertEquals(false, resp.getBody().isCanCurrentUserSelfCertify()); + } + + @Test + void canCurrentUserSelfCertify_trueWhenRoleAndPermBothGranted() throws Exception { + var push = blockedPush("p1", "alice"); + when(pushStore.findById("p1")).thenReturn(Optional.of(push)); + loginAs("alice", false, true); // pusher with role + repoPermissionService = mock(RepoPermissionService.class); + when(repoPermissionService.isBypassReviewAllowed("alice", "github", "github.com/acme/repo.git")) + .thenReturn(true); + var field = PushController.class.getDeclaredField("repoPermissionService"); + field.setAccessible(true); + field.set(controller, repoPermissionService); + + var resp = controller.getById("p1"); + assertEquals(true, resp.getBody().isCanCurrentUserSelfCertify()); + } } // ── POST /api/push/{id}/authorise ───────────────────────────────────────────── diff --git a/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/GitProxyServletRegistrar.java b/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/GitProxyServletRegistrar.java index 0c240f87..ad072621 100644 --- a/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/GitProxyServletRegistrar.java +++ b/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/GitProxyServletRegistrar.java @@ -242,7 +242,7 @@ public static void registerCoreFilters( filters.add(new GpgSignatureFilter(GpgConfig.defaultConfig())); filters.add(new ValidationSummaryFilter()); filters.add(new FetchFinalizerFilter()); - filters.add(new PushFinalizerFilter(serviceUrl, approvalGateway, repoPermissionService)); + filters.add(new PushFinalizerFilter(serviceUrl, approvalGateway)); filters.add(new AuditLogFilter()); boolean failFast = configBuilder != null && configBuilder.isFailFast(); From 40255ab9834788280514882e5166e70e022893e4 Mon Sep 17 00:00:00 2001 From: Thomas Cooper Date: Mon, 13 Apr 2026 01:11:41 -0400 Subject: [PATCH 2/2] feat(ui): add tooltip to SELF_CERTIFY role badge on profile page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clarifies that holding the role alone is not sufficient — an admin must also grant the per-repo Self-certify permission before self-approval works. --- .../frontend/src/pages/Profile.tsx | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/git-proxy-java-dashboard/frontend/src/pages/Profile.tsx b/git-proxy-java-dashboard/frontend/src/pages/Profile.tsx index eb41c9a1..630ad3ae 100644 --- a/git-proxy-java-dashboard/frontend/src/pages/Profile.tsx +++ b/git-proxy-java-dashboard/frontend/src/pages/Profile.tsx @@ -145,12 +145,20 @@ export function Profile() { : label === 'SELF_CERTIFY' ? 'bg-amber-100 text-amber-700' : 'bg-gray-100 text-gray-600' + const isSelfCertify = label === 'SELF_CERTIFY' return ( - - {label} + + + {label} + + {isSelfCertify && ( + + This role allows you to self-approve your own pushes, but an admin must also grant the Self-certify permission on each individual repository. + + + )} ) })}