fix(security): enforce ACL permission check in OAuth2 callback datasource lookup#41640
Open
fix(security): enforce ACL permission check in OAuth2 callback datasource lookup#41640
Conversation
…urce lookup Replace the 1-argument findById(id) (no ACL check) with the 2-argument findById(id, aclPermission) overload in getAccessTokenForGenericOAuth2, consistent with all other findById calls in the same class. Before this fix, the OAuth2 callback handler parsed the datasource ID from the client-controlled 'state' parameter and fetched it without any permission check, allowing an authenticated user to access and manipulate OAuth2 credentials for datasources in any workspace. Fixes APP-15028 Advisory: GHSA-rg2x-4v4h-g78w CWE: CWE-862 (Missing Authorization) CVSS: 8.1 (High) Co-authored-by: subratadeypappu <subrata71@users.noreply.github.com>
Contributor
WalkthroughDatasource lookup in the OAuth2 authorization-code callback now enforces ACL by supplying the datasource's edit permission to the datasource service before continuing with state parsing, storage lookup, token exchange, and redirect handling. Changes
Sequence Diagram(s)sequenceDiagram
participant OAuth as OAuth Provider
participant Client as Browser/Client
participant Auth as AuthenticationService
participant DS as DatasourceService
participant Store as StorageRepository
participant TokenEx as Token Exchange
Client->>Auth: Authorization callback (code + state)
Auth->>DS: findById(datasourceId, editPermission)
alt datasource found & permission OK
Auth->>Store: findByDatasourceAndEnv(...)
Store-->>Auth: stored config (clientId/secret/redirectUri)
Auth->>TokenEx: exchange code for token (with stored params)
TokenEx-->>Auth: token or error
Auth->>Client: redirect (success or appsmith_error)
else datasource not found / permission denied
DS-->>Auth: not found / forbidden
Auth-->>Client: complete without redirect (empty Mono)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-authored-by: subratadeypappu <subrata71@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/AuthenticationServiceTest.java`:
- Around line 378-395: Replace the current non-existent-datasource case in
testGetAccessTokenForGenericOAuth2_nonExistentDatasource_returnsEmpty with a
real existing datasource that the caller does NOT have permission to access:
create or mock a Datasource (or its repository response) with the same id used
in the test but with a different workspaceId/owner than the caller, ensure
authenticationService.getAccessTokenForGenericOAuth2(callbackDTO) is invoked
with an AuthorizationCodeCallbackDTO whose state includes that datasource id,
and assert it still returns an empty/complete Mono (no token and no progression)
to verify the ACL denial path; reference the test method
testGetAccessTokenForGenericOAuth2_nonExistentDatasource_returnsEmpty, the
AuthorizationCodeCallbackDTO/state construction, and
authenticationService.getAccessTokenForGenericOAuth2 to locate where to change
the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8716057b-d0b0-4f4e-9f86-9c3dc05dda86
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/AuthenticationServiceCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/solutions/AuthenticationServiceTest.java
...r/appsmith-server/src/test/java/com/appsmith/server/solutions/AuthenticationServiceTest.java
Outdated
Show resolved
Hide resolved
…ource test Address CodeRabbit review: the previous test used a non-existent datasource ID which would return Mono.empty() from findById regardless of ACL enforcement. The new test creates a real datasource, then strips all its ACL policies via datasourceRepository.save(), and verifies the OAuth2 callback returns an empty Mono. This test would FAIL if the 1-arg findById (no ACL) were used instead of the 2-arg findById (with ACL), directly proving the security fix works. Co-authored-by: subratadeypappu <subrata71@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/AuthenticationServiceTest.java (1)
396-421: Consider extracting shared datasource setup into a helper.Both tests duplicate the OAuth2 datasource creation flow. A small test helper would reduce maintenance noise and make future ACL-focused assertions easier to read.
Refactor sketch
+private Datasource createOAuth2Datasource(String workspaceId, String environmentId, String accessTokenUrl) { + Mono<Plugin> pluginMono = pluginService.findByName("Installed Plugin Name"); + Datasource datasource = new Datasource(); + datasource.setName("OAuth2 ACL Test Datasource"); + datasource.setWorkspaceId(workspaceId); + DatasourceConfiguration cfg = new DatasourceConfiguration(); + cfg.setUrl("http://test.com"); + OAuth2 auth = new OAuth2(); + auth.setClientId("ClientId"); + auth.setClientSecret("ClientSecret"); + auth.setAuthorizationUrl("AuthorizationURL"); + auth.setAccessTokenUrl(accessTokenUrl); + auth.setScope(Set.of("Scope1")); + auth.setIsAuthorizationHeader(false); + cfg.setAuthentication(auth); + datasource.setDatasourceStorages(new HashMap<>()); + datasource.getDatasourceStorages() + .put(environmentId, new DatasourceStorageDTO(null, environmentId, cfg)); + + return pluginMono.map(plugin -> { + datasource.setPluginId(plugin.getId()); + return datasource; + }) + .flatMap(datasourceService::create) + .block(); +}Also applies to: 472-497
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/AuthenticationServiceTest.java` around lines 396 - 421, Extract the repeated OAuth2 datasource setup into a reusable test helper (e.g., createOAuth2Datasource or buildOauth2Datasource) that constructs a Datasource, sets name/workspaceId/pluginId (from pluginMono.map(...)), builds DatasourceConfiguration with OAuth2 (clientId, clientSecret, authorizationUrl, accessTokenUrl, scope, isAuthorizationHeader), attaches DatasourceStorageDTO keyed by defaultEnvironmentId, and invokes datasourceService.create (or returns the prepared Datasource for creation). Replace the duplicated blocks in AuthenticationServiceTest (the sections using pluginMono, Datasource, DatasourceConfiguration, OAuth2, datasource.setDatasourceStorages, DatasourceStorageDTO, and datasourceService.create) with calls to that helper to reduce duplication and centralize future ACL-related changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/AuthenticationServiceTest.java`:
- Around line 396-421: Extract the repeated OAuth2 datasource setup into a
reusable test helper (e.g., createOAuth2Datasource or buildOauth2Datasource)
that constructs a Datasource, sets name/workspaceId/pluginId (from
pluginMono.map(...)), builds DatasourceConfiguration with OAuth2 (clientId,
clientSecret, authorizationUrl, accessTokenUrl, scope, isAuthorizationHeader),
attaches DatasourceStorageDTO keyed by defaultEnvironmentId, and invokes
datasourceService.create (or returns the prepared Datasource for creation).
Replace the duplicated blocks in AuthenticationServiceTest (the sections using
pluginMono, Datasource, DatasourceConfiguration, OAuth2,
datasource.setDatasourceStorages, DatasourceStorageDTO, and
datasourceService.create) with calls to that helper to reduce duplication and
centralize future ACL-related changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23733fac-b3b9-4d21-885b-53ba7e8eb1ee
📒 Files selected for processing (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/AuthenticationServiceTest.java
NilanshBansal
approved these changes
Mar 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The OAuth2 callback handler in
AuthenticationServiceCEImpl.getAccessTokenForGenericOAuth2()used the 1-argumentfindById(id)— which performs no ACL check — to fetch the datasource whose ID is embedded in the client-controlledstateparameter. An authenticated user could craft the OAuth2 callbackstateto reference a datasource in another workspace, bypassing authorization and gaining access to that datasource's OAuth2 credentials.This fix replaces the unprotected call with the 2-argument
findById(id, aclPermission)overload usingdatasourcePermission.getEditPermission(), consistent with all three otherfindByIdcalls in the same class (lines ~120, ~402, ~586).Advisory: GHSA-rg2x-4v4h-g78w
CVSS: 8.1 (High) —
CVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:U/C:H/I:H/A:NCWE: CWE-862 (Missing Authorization)
EE issue: https://github.com/appsmithorg/appsmith-ee/issues/8793
Changes
AuthenticationServiceCEImpl.java— single-line fix:findById(splitStates[1])→findById(splitStates[1], datasourcePermission.getEditPermission())AuthenticationServiceTest.java— two new tests:testGetAccessTokenForGenericOAuth2_datasourceWithoutPermission_returnsEmpty— creates a real datasource, strips its ACL policies, then verifies the OAuth2 callback returns an empty Mono (ACL denial). This test would fail if the 1-arg findById (no ACL) were used.testGetAccessTokenForGenericOAuth2_authorizedDatasource_progressesPastAcl— verifies that an authorized datasource passes the ACL gate and progresses to the token exchange stepFixes APP-15028
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
Tests