Skip to content

Commit 20c735b

Browse files
appsmith-smithes[bot]appsmith-smithes[bot]
authored
fix: resolve git-route application via ACL-checked helper (#42121)
## Description resolve git-route application via ACL-checked helper <!-- Smithes will fill from commit messages --> ### Fix <!-- Smithes will fill from plan — describe the fix, not the vulnerability --> ### CE/EE sync <!-- Smithes will fill from check_ce_ee output --> Merge this PR first. The hourly CE→EE sync propagates it to EE. ### EE counterpart EE PR: appsmithorg/appsmith-ee#9486 EE CI when this PR was opened: **passing** (ci-test|ci-test-limited, ci-test-result|ci-test-limited-result green). ### Tracking Linear: https://linear.app/appsmith/issue/APP-15789 > Additional context for reviewers lives in the linked ticket and the internal > Slack thread, not in this PR. Please keep it that way in review comments too. ## Automation /ok-to-test tags="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/31715681567> > Commit: 43fc70f > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=31715681567&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Thu, 13 Aug 2026 19:18:01 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No --------- Co-authored-by: appsmith-smithes[bot] <appsmith-smithes[bot]@users.noreply.github.com>
1 parent f94ebb4 commit 20c735b

2 files changed

Lines changed: 121 additions & 3 deletions

File tree

‎app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/gitRoute/GitRouteArtifactCE.java‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,19 @@ public GitRouteArtifactCE(
2323

2424
public Mono<Artifact> getArtifact(ArtifactType artifactType, String artifactId) {
2525
return switch (artifactType) {
26-
case APPLICATION ->
27-
applicationRepository
28-
.findById(artifactId)
26+
case APPLICATION -> {
27+
// Resolve through the ACL-aware helper (read permission) rather than the raw,
28+
// unfiltered repository lookup. This git-route resolution runs before the
29+
// downstream, permission-checked business logic; using the checked path here makes
30+
// it fail closed for callers with no access to the target application, before any
31+
// stored Git credential is decrypted or used against the configured remote.
32+
GitArtifactHelper<? extends Artifact> artifactHelper = getArtifactHelper(artifactType);
33+
yield artifactHelper
34+
.getArtifactById(artifactId, artifactHelper.getArtifactReadPermission())
2935
.switchIfEmpty(Mono.error(
3036
new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, artifactType, artifactId)))
3137
.map(app -> (Artifact) app);
38+
}
3239
default -> Mono.error(new AppsmithException(AppsmithError.GIT_ROUTE_HANDLER_NOT_FOUND, artifactType));
3340
};
3441
}

‎app/server/appsmith-server/src/test/java/com/appsmith/server/artifacts/gitRoute/GitRouteArtifactCETest.java‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
package com.appsmith.server.artifacts.gitRoute;
2+
3+
import com.appsmith.server.acl.AclPermission;
4+
import com.appsmith.server.constants.ArtifactType;
5+
import com.appsmith.server.domains.Application;
6+
import com.appsmith.server.domains.Artifact;
7+
import com.appsmith.server.exceptions.AppsmithException;
8+
import com.appsmith.server.git.resolver.GitArtifactHelperResolver;
9+
import com.appsmith.server.repositories.ApplicationRepository;
10+
import com.appsmith.server.services.GitArtifactHelper;
11+
import org.junit.jupiter.api.BeforeEach;
12+
import org.junit.jupiter.api.DisplayName;
13+
import org.junit.jupiter.api.Test;
14+
import org.junit.jupiter.api.extension.ExtendWith;
15+
import org.mockito.Mock;
16+
import org.mockito.junit.jupiter.MockitoExtension;
17+
import reactor.core.publisher.Mono;
18+
import reactor.test.StepVerifier;
19+
20+
import static org.assertj.core.api.Assertions.assertThat;
21+
import static org.mockito.ArgumentMatchers.anyString;
22+
import static org.mockito.Mockito.lenient;
23+
import static org.mockito.Mockito.never;
24+
import static org.mockito.Mockito.verify;
25+
26+
/**
27+
* Authorization coverage for {@link GitRouteArtifactCE#getArtifact}.
28+
*
29+
* <p>The git-route AOP layer resolves the target application through this method before the
30+
* downstream, ACL-checked business logic runs. It must resolve via the ACL-aware
31+
* {@link GitArtifactHelper#getArtifactById(String, AclPermission)} with read permission and fail
32+
* closed for a caller with no access to the target application — never via the raw, unfiltered
33+
* {@link ApplicationRepository#findById(String)} soft-delete lookup, which would let the aspect go
34+
* on to decrypt and use the target application's stored Git deploy-key credential.
35+
*/
36+
@ExtendWith(MockitoExtension.class)
37+
class GitRouteArtifactCETest {
38+
39+
private static final String APP_ID = "victim-app-id";
40+
41+
@Mock
42+
private ApplicationRepository applicationRepository;
43+
44+
@Mock
45+
private GitArtifactHelperResolver gitArtifactHelperResolver;
46+
47+
@SuppressWarnings("rawtypes")
48+
@Mock
49+
private GitArtifactHelper gitArtifactHelper;
50+
51+
private GitRouteArtifact gitRouteArtifact;
52+
53+
@BeforeEach
54+
void setUp() {
55+
gitRouteArtifact = new GitRouteArtifact(applicationRepository, gitArtifactHelperResolver);
56+
}
57+
58+
@Test
59+
@DisplayName("getArtifact(APPLICATION): resolves via the ACL-checked helper with read permission, "
60+
+ "never the raw repository")
61+
@SuppressWarnings("unchecked")
62+
void getArtifact_application_resolvesViaAclCheckedHelper() {
63+
Application app = new Application();
64+
app.setId(APP_ID);
65+
66+
lenient()
67+
.when(gitArtifactHelperResolver.getArtifactHelper(ArtifactType.APPLICATION))
68+
.thenReturn(gitArtifactHelper);
69+
lenient().when(gitArtifactHelper.getArtifactReadPermission()).thenReturn(AclPermission.READ_APPLICATIONS);
70+
lenient()
71+
.doReturn(Mono.just(app))
72+
.when(gitArtifactHelper)
73+
.getArtifactById(APP_ID, AclPermission.READ_APPLICATIONS);
74+
// The raw, unchecked lookup would also succeed — the point is it must NOT be used.
75+
lenient().when(applicationRepository.findById(APP_ID)).thenReturn(Mono.just(app));
76+
77+
StepVerifier.create(gitRouteArtifact.getArtifact(ArtifactType.APPLICATION, APP_ID))
78+
.assertNext(
79+
resolved -> assertThat(((Artifact) resolved).getId()).isEqualTo(APP_ID))
80+
.verifyComplete();
81+
82+
verify(gitArtifactHelper).getArtifactById(APP_ID, AclPermission.READ_APPLICATIONS);
83+
verify(applicationRepository, never()).findById(anyString());
84+
}
85+
86+
@Test
87+
@DisplayName("getArtifact(APPLICATION): fails closed when the caller lacks read access, "
88+
+ "even though the raw repository lookup would have returned the application")
89+
@SuppressWarnings("unchecked")
90+
void getArtifact_application_deniedWithoutReadPermission_doesNotLeakViaRawRepository() {
91+
Application victimApp = new Application();
92+
victimApp.setId(APP_ID);
93+
94+
lenient()
95+
.when(gitArtifactHelperResolver.getArtifactHelper(ArtifactType.APPLICATION))
96+
.thenReturn(gitArtifactHelper);
97+
lenient().when(gitArtifactHelper.getArtifactReadPermission()).thenReturn(AclPermission.READ_APPLICATIONS);
98+
// ACL-checked resolution denies the unrelated caller.
99+
lenient()
100+
.doReturn(Mono.empty())
101+
.when(gitArtifactHelper)
102+
.getArtifactById(APP_ID, AclPermission.READ_APPLICATIONS);
103+
// Raw, unfiltered lookup WOULD hand back the victim's application — this is the leak the fix closes.
104+
lenient().when(applicationRepository.findById(APP_ID)).thenReturn(Mono.just(victimApp));
105+
106+
StepVerifier.create(gitRouteArtifact.getArtifact(ArtifactType.APPLICATION, APP_ID))
107+
.verifyError(AppsmithException.class);
108+
109+
verify(applicationRepository, never()).findById(anyString());
110+
}
111+
}

0 commit comments

Comments
 (0)