Skip to content

Commit 2a31579

Browse files
coopernetesclaude
coopernetes
and
claude
authored
fix: move per-request credentials from shared Repository config to PushContext (#177)
* fix: move per-request credentials from shared Repository config to PushContext closes #176 Repository objects are cached and shared across concurrent pushes to the same upstream in LocalRepositoryCache. Writing per-request values (pushUser, pushToken, repoSlug, pushId, resolvedUser, scmUsername, validationRecordId, upstreamUser) to db.getConfig() raced under concurrent load — request A's token could be overwritten by request B before A's hooks had a chance to read it. Fix: store all per-request transient values on PushContext, which is created fresh per request in StoreAndForwardReceivePackFactory. upstreamUrl stays in repo config as it is per-repo (not per-request) and is intentionally persisted to disk. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: apply spotless formatting --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 97b345d commit 2a31579

14 files changed

Lines changed: 152 additions & 131 deletions

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

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -45,30 +45,40 @@ public class ApprovalPreReceiveHook implements PreReceiveHook {
4545
private final Duration timeout;
4646
private final String serviceUrl;
4747
private final RepoPermissionService repoPermissionService;
48+
private final PushContext pushContext;
4849

4950
public ApprovalPreReceiveHook(PushStore pushStore, ApprovalGateway approvalGateway) {
50-
this(pushStore, approvalGateway, DEFAULT_TIMEOUT, null, null);
51+
this(pushStore, approvalGateway, DEFAULT_TIMEOUT, null, null, null);
5152
}
5253

5354
public ApprovalPreReceiveHook(PushStore pushStore, ApprovalGateway approvalGateway, String serviceUrl) {
54-
this(pushStore, approvalGateway, DEFAULT_TIMEOUT, serviceUrl, null);
55+
this(pushStore, approvalGateway, DEFAULT_TIMEOUT, serviceUrl, null, null);
5556
}
5657

5758
public ApprovalPreReceiveHook(
5859
PushStore pushStore,
5960
ApprovalGateway approvalGateway,
6061
String serviceUrl,
6162
RepoPermissionService repoPermissionService) {
62-
this(pushStore, approvalGateway, DEFAULT_TIMEOUT, serviceUrl, repoPermissionService);
63+
this(pushStore, approvalGateway, DEFAULT_TIMEOUT, serviceUrl, repoPermissionService, null);
64+
}
65+
66+
public ApprovalPreReceiveHook(
67+
PushStore pushStore,
68+
ApprovalGateway approvalGateway,
69+
String serviceUrl,
70+
RepoPermissionService repoPermissionService,
71+
PushContext pushContext) {
72+
this(pushStore, approvalGateway, DEFAULT_TIMEOUT, serviceUrl, repoPermissionService, pushContext);
6373
}
6474

6575
public ApprovalPreReceiveHook(PushStore pushStore, ApprovalGateway approvalGateway, Duration timeout) {
66-
this(pushStore, approvalGateway, timeout, null, null);
76+
this(pushStore, approvalGateway, timeout, null, null, null);
6777
}
6878

6979
public ApprovalPreReceiveHook(
7080
PushStore pushStore, ApprovalGateway approvalGateway, Duration timeout, String serviceUrl) {
71-
this(pushStore, approvalGateway, timeout, serviceUrl, null);
81+
this(pushStore, approvalGateway, timeout, serviceUrl, null, null);
7282
}
7383

7484
public ApprovalPreReceiveHook(
@@ -77,21 +87,31 @@ public ApprovalPreReceiveHook(
7787
Duration timeout,
7888
String serviceUrl,
7989
RepoPermissionService repoPermissionService) {
90+
this(pushStore, approvalGateway, timeout, serviceUrl, repoPermissionService, null);
91+
}
92+
93+
public ApprovalPreReceiveHook(
94+
PushStore pushStore,
95+
ApprovalGateway approvalGateway,
96+
Duration timeout,
97+
String serviceUrl,
98+
RepoPermissionService repoPermissionService,
99+
PushContext pushContext) {
80100
this.pushStore = pushStore;
81101
this.approvalGateway = approvalGateway;
82102
this.timeout = timeout;
83103
this.serviceUrl = serviceUrl;
84104
this.repoPermissionService = repoPermissionService;
105+
this.pushContext = pushContext;
85106
}
86107

87108
@Override
88109
public void onPreReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
89110
OutputStream msgOut = rp.getMessageOutputStream();
90111

91-
// Read the validation record ID stored by PushStorePersistenceHook.validationResultHook
92-
String validationRecordId = rp.getRepository().getConfig().getString("gitproxy", null, "validationRecordId");
112+
String validationRecordId = pushContext != null ? pushContext.getValidationRecordId() : null;
93113
if (validationRecordId == null) {
94-
log.warn("No validationRecordId in repo config - skipping approval gate");
114+
log.warn("No validationRecordId in push context - skipping approval gate");
95115
return;
96116
}
97117

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ public class BitbucketCredentialRewriteHook implements GitProxyHook {
3030
private static final int ORDER = 148;
3131

3232
private final BitbucketProvider provider;
33+
private final PushContext pushContext;
3334

3435
@Override
3536
public int getOrder() {
@@ -43,12 +44,11 @@ public String getName() {
4344

4445
@Override
4546
public void onPreReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
46-
var repo = rp.getRepository();
47-
String pushEmail = repo.getConfig().getString("gitproxy", null, "pushUser");
48-
String pushToken = repo.getConfig().getString("gitproxy", null, "pushToken");
47+
String pushEmail = pushContext.getPushUser();
48+
String pushToken = pushContext.getPushToken();
4949

5050
if (pushEmail == null || pushToken == null) {
51-
log.debug("No push credentials in repo config — skipping Bitbucket upstream username resolution");
51+
log.debug("No push credentials in context — skipping Bitbucket upstream username resolution");
5252
return;
5353
}
5454

@@ -61,7 +61,7 @@ public void onPreReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
6161
}
6262

6363
String upstreamUser = identity.get().login();
64-
repo.getConfig().setString("gitproxy", null, "upstreamUser", upstreamUser);
64+
pushContext.setUpstreamUser(upstreamUser);
6565
log.debug("Stored Bitbucket upstream username '{}' for push email '{}'", upstreamUser, pushEmail);
6666
}
6767
}

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

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -64,10 +64,9 @@ public CheckUserPushPermissionHook(
6464

6565
@Override
6666
public void onPreReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
67-
var config = rp.getRepository().getConfig();
68-
String pushUser = config.getString("gitproxy", null, "pushUser");
69-
String pushToken = config.getString("gitproxy", null, "pushToken");
70-
String repoSlug = config.getString("gitproxy", null, "repoSlug");
67+
String pushUser = pushContext.getPushUser();
68+
String pushToken = pushContext.getPushToken();
69+
String repoSlug = pushContext.getRepoSlug();
7170

7271
if (identityResolver == null) {
7372
log.debug("No identity resolver configured (open mode), skipping permission check");
@@ -140,13 +139,13 @@ public void onPreReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
140139
user.getUsername(),
141140
providerId,
142141
repoSlug);
143-
config.setString("gitproxy", null, "resolvedUser", user.getUsername());
142+
pushContext.setResolvedUser(user.getUsername());
144143
if (provider != null && user.getScmIdentities() != null) {
145144
user.getScmIdentities().stream()
146145
.filter(id -> provider.getProviderId().equalsIgnoreCase(id.getProvider()))
147146
.map(org.finos.gitproxy.user.ScmIdentity::getUsername)
148147
.findFirst()
149-
.ifPresent(scmUser -> config.setString("gitproxy", null, "scmUsername", scmUser));
148+
.ifPresent(pushContext::setScmUsername);
150149
}
151150
pushContext.addStep(PushStep.builder()
152151
.stepName("checkUserPermission")

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,10 +68,10 @@ public void onPostReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
6868
Repository repo = rp.getRepository();
6969

7070
// If BitbucketCredentialRewriteHook resolved an upstream username, use it instead of the push email.
71-
String upstreamUser = repo.getConfig().getString("gitproxy", null, "upstreamUser");
71+
String upstreamUser = pushContext.getUpstreamUser();
7272
CredentialsProvider effectiveCreds = credentials;
7373
if (upstreamUser != null) {
74-
String pushToken = repo.getConfig().getString("gitproxy", null, "pushToken");
74+
String pushToken = pushContext.getPushToken();
7575
effectiveCreds = new UsernamePasswordCredentialsProvider(upstreamUser, pushToken != null ? pushToken : "");
7676
log.debug("Using Bitbucket upstream username '{}' for forwarding credentials", upstreamUser);
7777
}

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,8 @@ public void onPreReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
7474
return;
7575
}
7676

77-
var config = rp.getRepository().getConfig();
78-
String pushUser = config.getString("gitproxy", null, "pushUser");
79-
String pushToken = config.getString("gitproxy", null, "pushToken");
77+
String pushUser = pushContext.getPushUser();
78+
String pushToken = pushContext.getPushToken();
8079

8180
if (pushUser == null || pushUser.isEmpty()) {
8281
log.debug("No push user in repo config — skipping identity verification");

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

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,40 @@
44
import java.util.Collections;
55
import java.util.List;
66
import java.util.Optional;
7+
import lombok.Data;
78
import org.finos.gitproxy.db.model.PushStep;
89

910
/**
10-
* Shared context for accumulating {@link PushStep} records across pre-receive hooks within a single push. Hooks write
11-
* steps (diffs, scan results, etc.) to this context, and the persistence hook reads them when saving the final record.
11+
* Per-request context shared across all pre/post-receive hooks within a single push. Carries both accumulated
12+
* {@link PushStep} records (diffs, scan results, etc.) and transient per-request values that must not be stored on the
13+
* shared cached {@link org.eclipse.jgit.lib.Repository} config.
14+
*
15+
* <p>All fields are written once (by {@link StoreAndForwardReceivePackFactory} or early hooks) and read by later hooks.
16+
* No synchronisation is needed because hooks in a single push execute sequentially on the same thread.
1217
*/
18+
@Data
1319
public class PushContext {
1420

1521
private final List<PushStep> steps = new ArrayList<>();
1622

23+
// Per-request credentials — written by StoreAndForwardReceivePackFactory before any hook runs.
24+
private String pushUser;
25+
private String pushToken;
26+
private String repoSlug;
27+
28+
// Resolved upstream username for Bitbucket pushes — written by BitbucketCredentialRewriteHook.
29+
private String upstreamUser;
30+
31+
// Push record ID — written by PushStorePersistenceHook.preReceiveHook().
32+
private String pushId;
33+
34+
// Identity resolved by CheckUserPushPermissionHook — written after permission check passes.
35+
private String resolvedUser;
36+
private String scmUsername;
37+
38+
// Validation record ID — written by PushStorePersistenceHook.validationResultHook().
39+
private String validationRecordId;
40+
1741
/** Add a step to the context. */
1842
public void addStep(PushStep step) {
1943
steps.add(step);

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

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -80,8 +80,7 @@ public void setAutoApproval(boolean autoApproval) {
8080
public PreReceiveHook preReceiveHook() {
8181
return (ReceivePack rp, Collection<ReceiveCommand> commands) -> {
8282
String pushId = UUID.randomUUID().toString();
83-
// Store push ID in repo config so post-receive can find it
84-
rp.getRepository().getConfig().setString("gitproxy", null, "pushId", pushId);
83+
pushContext.setPushId(pushId);
8584

8685
try {
8786
PushRecord record = buildInitialRecord(pushId, rp, commands);
@@ -102,14 +101,14 @@ public PreReceiveHook preReceiveHook() {
102101
*/
103102
public PreReceiveHook validationResultHook(ValidationContext validationContext) {
104103
return (ReceivePack rp, Collection<ReceiveCommand> commands) -> {
105-
String pushId = rp.getRepository().getConfig().getString("gitproxy", null, "pushId");
104+
String pushId = pushContext != null ? pushContext.getPushId() : null;
106105
if (pushId == null) return;
107106

108-
// Re-read resolvedUser and scmUsername here — both are set by CheckUserPushPermissionHook
109-
// (order 150), which runs after preReceiveHook(), so they were not available when the
110-
// RECEIVED record was written. validationResultHook fires after all validation hooks complete.
111-
String resolvedUserLate = rp.getRepository().getConfig().getString("gitproxy", null, "resolvedUser");
112-
String scmUsernameLate = rp.getRepository().getConfig().getString("gitproxy", null, "scmUsername");
107+
// Read resolvedUser and scmUsername from pushContext — both are set by CheckUserPushPermissionHook
108+
// (order 150) after preReceiveHook() ran, so they were not available when the RECEIVED record
109+
// was written. validationResultHook fires after all validation hooks complete.
110+
String resolvedUserLate = pushContext.getResolvedUser();
111+
String scmUsernameLate = pushContext.getScmUsername();
113112

114113
try {
115114
pushStore.findById(pushId).ifPresent(initial -> {
@@ -156,9 +155,7 @@ public PreReceiveHook validationResultHook(ValidationContext validationContext)
156155
record.setBlockedMessage(validationContext.getIssues().size() + " validation issue(s) found");
157156
record.setSteps(allSteps);
158157
pushStore.save(record);
159-
rp.getRepository()
160-
.getConfig()
161-
.setString("gitproxy", null, "validationRecordId", record.getId());
158+
if (pushContext != null) pushContext.setValidationRecordId(record.getId());
162159
log.debug(
163160
"Saved validation result record: id={}, status=REJECTED (auto-rejected)",
164161
record.getId());
@@ -221,7 +218,7 @@ public PreReceiveHook validationResultHook(ValidationContext validationContext)
221218
}
222219

223220
pushStore.save(record);
224-
rp.getRepository().getConfig().setString("gitproxy", null, "validationRecordId", record.getId());
221+
if (pushContext != null) pushContext.setValidationRecordId(record.getId());
225222
log.debug(
226223
"Saved validation result record: id={}, status=PENDING (awaiting review)", record.getId());
227224
});
@@ -239,7 +236,7 @@ public PreReceiveHook validationResultHook(ValidationContext validationContext)
239236
*/
240237
public PostReceiveHook postReceiveHook() {
241238
return (ReceivePack rp, Collection<ReceiveCommand> commands) -> {
242-
String pushId = rp.getRepository().getConfig().getString("gitproxy", null, "pushId");
239+
String pushId = pushContext != null ? pushContext.getPushId() : null;
243240
if (pushId == null) return;
244241

245242
try {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,9 @@ public RepositoryUrlRuleHook(
4141

4242
@Override
4343
public void onPreReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
44-
String repoSlug = rp.getRepository().getConfig().getString("gitproxy", null, "repoSlug");
44+
String repoSlug = pushContext.getRepoSlug();
4545
if (repoSlug == null || repoSlug.isBlank()) {
46-
log.warn("No repoSlug in repo config — cannot evaluate URL rules, blocking push (fail-closed)");
46+
log.warn("No repoSlug in push context — cannot evaluate URL rules, blocking push (fail-closed)");
4747
blockPush(rp, commands, "Repository path unavailable");
4848
return;
4949
}

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

Lines changed: 13 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -157,35 +157,27 @@ public ReceivePack create(HttpServletRequest req, Repository db)
157157
creds = extractBasicAuth(req);
158158
}
159159

160-
// Store push credentials in repo config so hooks can read them for identity resolution.
161-
// pushToken is the PAT/password — stored only in-process repo config, never persisted to disk.
160+
// Per-request shared contexts — created before credentials are extracted so they can be
161+
// stored on pushContext rather than the shared cached Repository config.
162+
var validationContext = new ValidationContext();
163+
var pushContext = new PushContext();
164+
165+
// Store per-request credentials on pushContext, not the shared Repository config.
166+
// Writing to db.getConfig() would race with concurrent pushes to the same cached repo.
162167
String[] userPass = extractUserPass(req);
163-
String pushUser = userPass != null ? userPass[0] : null;
164-
String pushToken = userPass != null ? userPass[1] : null;
165-
if (pushUser != null) {
166-
db.getConfig().setString("gitproxy", null, "pushUser", pushUser);
167-
}
168-
if (pushToken != null) {
169-
db.getConfig().setString("gitproxy", null, "pushToken", pushToken);
170-
}
168+
pushContext.setPushUser(userPass != null ? userPass[0] : null);
169+
pushContext.setPushToken(userPass != null ? userPass[1] : null);
171170

172-
// Store the repo slug (/owner/repo) so permission hooks can read it without re-parsing the URL.
173171
String pathInfo = req.getPathInfo();
174172
if (pathInfo != null) {
175-
// pathInfo is e.g. /owner/repo.git — strip .git suffix
176173
String slug = pathInfo.replaceAll("\\.git$", "");
177-
// Normalise to /owner/repo (at most two path segments after leading /)
178174
String[] segments = slug.split("/", 4);
179175
if (segments.length >= 3) {
180176
slug = "/" + segments[1] + "/" + segments[2];
181177
}
182-
db.getConfig().setString("gitproxy", null, "repoSlug", slug);
178+
pushContext.setRepoSlug(slug);
183179
}
184180

185-
// Per-request shared contexts
186-
var validationContext = new ValidationContext();
187-
var pushContext = new PushContext();
188-
189181
// Persistence hook (records push to database)
190182
var persistenceHook = pushStore != null ? new PushStorePersistenceHook(pushStore, provider) : null;
191183
if (persistenceHook != null) {
@@ -247,7 +239,7 @@ public ReceivePack create(HttpServletRequest req, Repository db)
247239
new GpgSignatureHook(gpgConfig, validationContext, pushContext),
248240
new SecretScanningHook(secretScanConfig, validationContext, pushContext)));
249241
if (provider instanceof BitbucketProvider bitbucketProvider) {
250-
validationHooks.add(new BitbucketCredentialRewriteHook(bitbucketProvider));
242+
validationHooks.add(new BitbucketCredentialRewriteHook(bitbucketProvider, pushContext));
251243
}
252244
validationHooks.sort(Comparator.comparingInt(GitProxyHook::getOrder));
253245

@@ -257,7 +249,8 @@ public ReceivePack create(HttpServletRequest req, Repository db)
257249
hooks.add(persistenceHook.preReceiveHook());
258250
hooks.addAll(validationHooks);
259251
hooks.add(persistenceHook.validationResultHook(validationContext));
260-
hooks.add(new ApprovalPreReceiveHook(pushStore, approvalGateway, serviceUrl, repoPermissionService));
252+
hooks.add(new ApprovalPreReceiveHook(
253+
pushStore, approvalGateway, serviceUrl, repoPermissionService, pushContext));
261254
preHooks = hooks.toArray(PreReceiveHook[]::new);
262255
} else {
263256
preHooks = validationHooks.toArray(PreReceiveHook[]::new);

0 commit comments

Comments
 (0)