fix(security): Appsmith App Viewer datasource configuration leak via import helper (GHSA-93mf-9h52-gfxp)#41764
Conversation
…sting (GHSA-93mf-9h52-gfxp)
DatasourceImportableServiceCEImpl.getEntitiesPresentInWorkspace() was
calling getAllByWorkspaceIdWithStorages with null permission, bypassing
ACL checks. This allowed App Viewer users to retrieve sensitive
datasource configuration (internal URLs, Bearer tokens, API keys) via
GET /api/v1/applications/import/{workspaceId}/datasources.
Inject DatasourcePermission and enforce READ_DATASOURCES permission on
this method, consistent with the standard datasource listing endpoint.
The null permission in importEntities() is intentional — that pipeline
has its own authorization layer (edit/create permission checks).
subrata71
added
the
Security
WalkthroughConstructor injection of Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImplTest.java (1)
246-267: Consider also covering the integration boundary (controller → service).The unit test confirms
getEntitiesPresentInWorkspacepasses the read permission, but the original PoC was againstGET /api/v1/applications/import/{workspaceId}/datasources. A small integration/controller test that calls the endpoint as an App Viewer and asserts the response is empty/forbidden would close the loop end-to-end and guard against future regressions in the controller wiring.🤖 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/datasources/importable/DatasourceImportableServiceCEImplTest.java` around lines 246 - 267, Add an integration/controller test that exercises the actual endpoint GET /api/v1/applications/import/{workspaceId}/datasources (the controller method that forwards to importService.getEntitiesPresentInWorkspace) to verify the permission boundary end-to-end: set up a test user with the App Viewer role (or mock the security context accordingly), call the endpoint with a workspaceId, and assert the HTTP response is either 403 Forbidden or an empty body as expected; ensure the test verifies the controller invoked importService.getEntitiesPresentInWorkspace and that no higher-privilege behavior occurs when only READ_DATASOURCES is granted.
🤖 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/datasources/importable/DatasourceImportableServiceCEImplTest.java`:
- Around line 246-267: Add an integration/controller test that exercises the
actual endpoint GET /api/v1/applications/import/{workspaceId}/datasources (the
controller method that forwards to importService.getEntitiesPresentInWorkspace)
to verify the permission boundary end-to-end: set up a test user with the App
Viewer role (or mock the security context accordingly), call the endpoint with a
workspaceId, and assert the HTTP response is either 403 Forbidden or an empty
body as expected; ensure the test verifies the controller invoked
importService.getEntitiesPresentInWorkspace and that no higher-privilege
behavior occurs when only READ_DATASOURCES is granted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb9e40f5-0e49-4c5c-9fea-9e616697ac56
📒 Files selected for processing (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImplTest.java
| @Override | ||
| public Flux<Datasource> getEntitiesPresentInWorkspace(String workspaceId) { | ||
| return datasourceService.getAllByWorkspaceIdWithStorages(workspaceId, null); | ||
| return datasourceService.getAllByWorkspaceIdWithStorages(workspaceId, datasourcePermission.getReadPermission()); |
There was a problem hiding this comment.
What is this call used for, do we need this for some internal operation? IMO as long as we are not showing this to user or comparing it, this should be fine?
There was a problem hiding this comment.
This method is not internal-only — it feeds directly into the controller endpoint GET /api/v1/applications/import/{workspaceId}/datasources (see ApplicationControllerCE.getUnConfiguredDatasource() at line 286).
The call chain is:
getEntitiesPresentInWorkspace(workspaceId)→Flux<Datasource>- →
ImportServiceCEImpl.findDatasourceByArtifactId()filters by datasource IDs used in the artifact - →
ApplicationControllerCE.getUnConfiguredDatasource()wraps inResponseDTO<List<Datasource>>and returns it with@JsonView(Views.Public.class)
The full Datasource objects including datasourceConfiguration (URLs, Bearer tokens, API keys) are serialized to the HTTP response. This is exactly the vulnerability described in the GHSA — an App Viewer who cannot list datasources via the normal API (GET /api/v1/datasources) can use this endpoint to read the same data without READ_DATASOURCES permission.
The null permission in importEntities() (line 117, same file) is a different story — that path is behind import-specific authorization checks and doesn't expose data to HTTP responses.
There was a problem hiding this comment.
could we create two separate methods, one internal, and one which controller can see through.
The idea is that we might have workflows, which should not require permission (sometimes for internal gatekeeping or something)
2 participants
Summary
fix(security): Appsmith App Viewer datasource configuration leak via import helper (GHSA-93mf-9h52-gfxp)
DatasourceImportableServiceCEImpl.getEntitiesPresentInWorkspace()was callinggetAllByWorkspaceIdWithStorages(workspaceId, null)— passingnullas the ACL permission, which bypasses authorization entirely. Changed to enforcedatasourcePermission.getReadPermission()(READ_DATASOURCES), consistent with the standard datasource listing endpoint.DatasourcePermissioninto bothDatasourceImportableServiceCEImplandDatasourceImportableServiceImplconstructors (Spring auto-wires the bean).should_enforceReadPermission_when_getEntitiesPresentInWorkspace_isCalled) annotated with GHSA ID, verifying thatREAD_DATASOURCESpermission is always passed andnullis never used.Vulnerability
DatasourceImportableServiceCEImpl.getEntitiesPresentInWorkspace()Exposure Analysis
App Vieweraccess to a workspace. This is the lowest privilege role — no admin, developer, or owner access required.Authorization: Bearer <token>), custom properties (e.g., API keys), and connection metadata. This is a confidentiality breach of backend infrastructure credentials.Fix
DatasourceImportableServiceCEImpl.getEntitiesPresentInWorkspace()(line 602) calleddatasourceService.getAllByWorkspaceIdWithStorages(workspaceId, null). WhenAclPermissionisnull, the MongoDB repository layer skips ACL filtering and returns all matching documents, bypassing the policy-based permission model.DatasourcePermissioninto the service and replacenullwithdatasourcePermission.getReadPermission()(AclPermission.READ_DATASOURCES). This is the same permission used by the standardGET /api/v1/datasources?workspaceId=endpoint — consistent, minimal, and at the correct layer.importEntities()in the same class (line 117) also usesnullpermission — this is intentional because the import pipeline has its own authorization layer (edit permission on existing datasources, create permission on workspace). (2)DatasourceForkableServiceCEImpl.getExistingEntitiesInTarget()also usesnull— fork operations require higher-level permissions upstream.getEntitiesPresentInWorkspace()automatically inherit the permission enforcement.Test plan
getAllByWorkspaceIdWithStorages.*null— remaining instances are in authorized internal pipelinesmvn compile -DskipTests)CE/EE sync
CE-only safe: the modified files (
DatasourceImportableServiceCEImpl.java,DatasourceImportableServiceImpl.java) are CE files that EE inherits. The hourly CE→EE sync will propagate the fix. The EE repo already has identical changes as uncommitted modifications, confirming alignment.Disclosure
Follow-ups
Fixes APP-15189
Fixes https://linear.app/appsmith/issue/APP-15045/security-high-app-viewer-datasource-configuration-leak-via-import
Summary by CodeRabbit
Bug Fixes
Tests
Automation
/ok-to-test tags="@tag.All"
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/24981307163
Commit: 54b3c7f
Cypress dashboard.
Tags:
@tag.AllSpec:
Mon, 27 Apr 2026 08:09:14 UTC