fix(security): enforce ACL and workspace boundary on datasource lookups in NewActionServiceCEImpl (GHSA-rqp9-r5fr-4cc8)#41846
Conversation
…p9-r5fr-4cc8) Adds two security tests that verify: 1. validateAction fetches datasource with ACL permission check (not 1-arg findById) 2. updateDatasourcePolicyForPublicAction rejects cross-workspace datasource references
…ps in NewActionServiceCEImpl (GHSA-rqp9-r5fr-4cc8) Replace all 1-arg datasourceService.findById(id) calls with the 2-arg ACL-checked variant using datasourcePermission.getExecutePermission(). This prevents cross-tenant datasource reference injection via client-controlled datasource.id in action create/update DTOs. Additionally: - Add workspace boundary check in updateDatasourcePolicyForPublicAction to prevent cross-workspace anonymous EXECUTE policy grants - Add ACL to applicationService.findById in the policy mutation path - Add ACL with graceful fallback on analytics/hint datasource lookups (lines ~866, ~1136, ~1190, ~1584) to prevent cross-tenant info leaks Closes GHSA-rqp9-r5fr-4cc8
subrata71
added
Security
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDatasource lookups in NewActionServiceCEImpl now use execute-permission-scoped findById across validation, update, hint, and archive flows with fallback to embedded configs on denial. Public-action policy updates verify workspace consistency and load the application with read permission. New tests cover ACL enforcement and cross-workspace rejection. ChangesDatasource Access Control and Policy Authorization
Sequence Diagram(s)sequenceDiagram
participant Client
participant NewActionService
participant DatasourceService
participant ApplicationService
participant InMemoryDatasource
Client->>NewActionService: request validate/update/archive/populateHints
NewActionService->>DatasourceService: findById(datasourceId, executePermission)
alt datasource found
DatasourceService-->>NewActionService: Datasource
else not found / permission denied
NewActionService->>InMemoryDatasource: use embedded datasource/config
end
NewActionService->>NewActionService: enforce workspaceId checks
alt updating public action policy & workspace match
NewActionService->>ApplicationService: findById(appId, readPermission)
ApplicationService-->>NewActionService: Application (isPublic?)
NewActionService->>DatasourceService: save(updatedDatasource)
else workspace mismatch
NewActionService-->>Client: error UNAUTHORIZED_ACCESS
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (2)
1191-1203:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep hint generation best-effort on read paths.
populateHintMessages(...)runs during normal action fetches. With the new ACL lookup, a datasource the caller can't execute now turns the whole action read intoNO_RESOURCE_FOUNDinstead of just skipping hints. This should degrade to an empty/default datasource config rather than failing the request.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java` around lines 1191 - 1203, The current dsConfigMono chain calls datasourceService.findById(..., datasourcePermission.getExecutePermission()) and then switchIfEmpty(Mono.error(...)) which turns a missing/unauthorized datasource into a NO_RESOURCE_FOUND and fails action reads; instead make hint-generation best-effort by returning a default empty DatasourceConfiguration when the datasource is absent or inaccessible. Update the dsConfigMono flow (the datasourceService.findById(...) call used by populateHintMessages and referenced as dsConfigMono/action.getDatasource().getId/datasourcePermission.getExecutePermission) to fall back to Mono.just(new DatasourceConfiguration()) rather than Mono.error(...) (or use onErrorResume for that specific NO_RESOURCE_FOUND error to return the default) so populateHintMessages can proceed without failing the whole read.
621-640:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDon't continue with the request datasource after an ACL miss.
When
findById(..., datasourcePermission.getExecutePermission())returns empty, this path falls back to the client-supplied datasource and then uses it for the workspace/policy flow. That lets an unauthorized datasource still reachupdateDatasourcePolicyForPublicAction(...), and the later boundary check is neutralized oncenewAction.setWorkspaceId(datasource.getWorkspaceId())runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java` around lines 621 - 640, The code continues the datasource flow using the client-supplied datasource after an ACL miss, allowing unauthorized data to reach updateDatasourcePolicyForPublicAction and letting newAction.setWorkspaceId(datasource.getWorkspaceId()) run; change the switchIfEmpty branch in the datasourceMono pipeline (where findById(..., datasourcePermission.getExecutePermission()) is called) to return Mono.empty() after recording the error/setting editActionDTO invalid instead of Mono.just(editActionDTO.getDatasource()), and ensure downstream map/flatMap (the map that calls newAction.setWorkspaceId(...) and the flatMap to updateDatasourcePolicyForPublicAction) will not execute for empty Monos so unauthorized datasources are never used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/NewActionServiceSecurityTest.java`:
- Around line 220-258: The test currently stubs
permissionGroupService.isEntityAccessible(...) to always return true so it never
hits the rejection path; change the stubbing to return true for the action
entity and false for the datasource entity (e.g., use Mockito.doAnswer(...) on
permissionGroupService.isEntityAccessible to inspect the first argument and
return Mono.just(true) when it's an Action/has the action's id and
Mono.just(false) when it's the victimDatasource), then call
newActionService.validateAction(publicAction) and assert it fails with the
UNAUTHORIZED_ACCESS error (use StepVerifier.create(...).expectErrorMatches(e ->
e instanceof AppsmithException && ((AppsmithException)e).getErrorCode() ==
AppsmithError.UNAUTHORIZED_ACCESS).verify()) so the cross-workspace guard is
exercised; reference symbols: permissionGroupService.isEntityAccessible,
victimDatasource, publicAction, newActionService.validateAction,
UNAUTHORIZED_ACCESS.
---
Outside diff comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java`:
- Around line 1191-1203: The current dsConfigMono chain calls
datasourceService.findById(..., datasourcePermission.getExecutePermission()) and
then switchIfEmpty(Mono.error(...)) which turns a missing/unauthorized
datasource into a NO_RESOURCE_FOUND and fails action reads; instead make
hint-generation best-effort by returning a default empty DatasourceConfiguration
when the datasource is absent or inaccessible. Update the dsConfigMono flow (the
datasourceService.findById(...) call used by populateHintMessages and referenced
as
dsConfigMono/action.getDatasource().getId/datasourcePermission.getExecutePermission)
to fall back to Mono.just(new DatasourceConfiguration()) rather than
Mono.error(...) (or use onErrorResume for that specific NO_RESOURCE_FOUND error
to return the default) so populateHintMessages can proceed without failing the
whole read.
- Around line 621-640: The code continues the datasource flow using the
client-supplied datasource after an ACL miss, allowing unauthorized data to
reach updateDatasourcePolicyForPublicAction and letting
newAction.setWorkspaceId(datasource.getWorkspaceId()) run; change the
switchIfEmpty branch in the datasourceMono pipeline (where findById(...,
datasourcePermission.getExecutePermission()) is called) to return Mono.empty()
after recording the error/setting editActionDTO invalid instead of
Mono.just(editActionDTO.getDatasource()), and ensure downstream map/flatMap (the
map that calls newAction.setWorkspaceId(...) and the flatMap to
updateDatasourcePolicyForPublicAction) will not execute for empty Monos so
unauthorized datasources are never used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0554daf9-3586-4aaa-93af-f4f5461b4cf3
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/NewActionServiceSecurityTest.java
subrata71
added
ok-to-test
There was a problem hiding this comment.
Pull request overview
Addresses GHSA-rqp9-r5fr-4cc8 (cross-tenant datasource takeover) by replacing the unprotected 1-arg datasourceService.findById(id) calls in NewActionServiceCEImpl with the ACL-checked 2-arg variant using EXECUTE_DATASOURCES, and adds a workspace-boundary guard plus an ACL-checked application lookup in the public-action datasource policy path.
Changes:
- Replace 1-arg
datasourceService.findByIdwith execute-permission variant invalidateAction, analytics, hint, delete, and archive paths (with graceful fallback on non-validation paths). - Add workspace-equality check and ACL-checked
applicationService.findByIdinupdateDatasourcePolicyForPublicAction. - Add new
NewActionServiceSecurityTestwith two GHSA regression tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java |
Switches 6 datasource lookups to ACL-checked variants, adds workspace boundary check and ACL'd application fetch in the public-action policy mutation path. |
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/NewActionServiceSecurityTest.java |
New Spring Boot security test class with two regression tests for the GHSA fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String actionWorkspaceId = action.getWorkspaceId(); | ||
| if (actionWorkspaceId != null && !actionWorkspaceId.equals(datasource.getWorkspaceId())) { | ||
| return Mono.error(new AppsmithException(AppsmithError.UNAUTHORIZED_ACCESS)); | ||
| } |
| StepVerifier.create(newActionService.validateAction(publicAction)) | ||
| .assertNext(validatedAction -> { | ||
| assertThat(validatedAction).isNotNull(); | ||
| }) | ||
| .verifyComplete(); | ||
|
|
||
| Mockito.verify(datasourceService, never()).save(any(Datasource.class), eq(false)); |
…space check (GHSA-rqp9-r5fr-4cc8) Addresses CodeRabbit and Copilot review comments: 1. switchIfEmpty fallback no longer continues with the client-supplied datasource for workspace/policy operations. Returns a stub Datasource without an ID, preventing updateDatasourcePolicyForPublicAction from running on unverified data. 2. Workspace boundary check moved upstream into the datasourceMono flatMap — before newAction.setWorkspaceId() overwrites the action's workspace. This ensures the check compares the action's original workspace against the datasource's actual workspace. 3. Hint generation (populateHintMessages) is now best-effort: falls back to empty DatasourceConfiguration on permission denial instead of failing the entire action read with NO_RESOURCE_FOUND. 4. Cross-workspace test rewritten to assert UNAUTHORIZED_ACCESS error when action workspace differs from datasource workspace, properly exercising the boundary check.
Failed server tests
|
…to avoid fork regression (GHSA-rqp9-r5fr-4cc8) The workspace boundary check in validateAction's datasourceMono.flatMap() caused UNAUTHORIZED_ACCESS in fork flows where the ActionDTO carries the source workspace while the forked datasource is in the target workspace. Moved the check into updateDatasourcePolicyForPublicAction where it compares the APPLICATION's workspace against the DATASOURCE's workspace. This correctly blocks cross-workspace policy grants (the attack vector) without breaking legitimate fork/import flows: - Fork: application and datasource are both in the target workspace -> OK - Attack: application in attacker's workspace, datasource in victim's -> BLOCKED Also updated the cross-workspace test to exercise the actual policy mutation path with proper isEntityAccessible stubbing.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java`:
- Around line 638-643: The code unconditionally sets
newAction.setWorkspaceId(datasource.getWorkspaceId()) inside
NewActionServiceCEImpl which can overwrite a valid action workspace with null
when the fallback datasource lacks a workspaceId; change the mapping so you only
set/backfill the action workspace when datasource.getWorkspaceId() is non-null
(or non-empty) and otherwise leave newAction.getWorkspaceId() unchanged before
calling updateDatasourcePolicyForPublicAction and
validateAndSaveActionToRepository to avoid orphaning the action.
- Around line 1658-1662: The import path is missing the workspace guard used in
the direct mutation path; add the same workspace comparison
(application.getWorkspaceId() != null && datasource.getWorkspaceId() != null &&
!application.getWorkspaceId().equals(datasource.getWorkspaceId())) that aborts
with AppsmithError.UNAUTHORIZED_ACCESS to the import-side flow—either by
inserting this check in validateActionsBeforeImport(...) before calling
updateDatasourcePoliciesForPublicActions(...) or by centralizing it inside
updateDatasourcePoliciesForPublicActions(...) (which is invoked by
updateDatasourcePolicyForPublicAction and the import flow) so that any call that
loads the application via getApplicationById(...) /
applicationService.findById(...) will enforce the application-vs-datasource
workspace equality before saving datasource execute policies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd47b884-efe9-4631-946e-8c1de320e72f
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/NewActionServiceSecurityTest.java
…dd workspace check to import path (GHSA-rqp9-r5fr-4cc8) - Guard newAction.setWorkspaceId() against null when the ACL fallback datasource has no workspaceId, preventing orphaned actions. - Add workspace boundary check to the import-path updateDatasourcePoliciesForPublicActions, mirroring the guard already present in the single-action updateDatasourcePolicyForPublicAction.
|
Actionable comments posted: 0 |
Review Comments Resolution SummaryAll review comments from CodeRabbit and Copilot have been addressed across the following commits: Already resolved in earlier commits (
|
| .findById( | ||
| updatedActionDTO.getDatasource().getId(), | ||
| datasourcePermission.getExecutePermission()) | ||
| .onErrorResume(e -> Mono.justOrEmpty(updatedActionDTO.getDatasource())); |
| return datasourceService | ||
| .findById( | ||
| action.getDatasource().getId(), datasourcePermission.getExecutePermission()) | ||
| .onErrorResume(e -> Mono.justOrEmpty(action.getDatasource())); |
| if (action.getDatasource() != null && action.getDatasource().getId() != null) { | ||
| return datasourceService.findById(action.getDatasource().getId()); | ||
| return datasourceService | ||
| .findById(action.getDatasource().getId(), datasourcePermission.getExecutePermission()) |
| fallback.setPluginId( | ||
| editActionDTO.getDatasource().getPluginId()); | ||
| fallback.setName( | ||
| editActionDTO.getDatasource().getName()); |
| .switchIfEmpty(Mono.just(new DatasourceConfiguration())) | ||
| .onErrorResume(e -> Mono.just(new DatasourceConfiguration())); |
…dary paths (GHSA-rqp9-r5fr-4cc8) The ACL-checked findById returns Mono.empty() (not an error) when the user lacks EXECUTE_DATASOURCES. Three analytics/archival paths used zipWith/zipWhen which collapsed the entire chain to empty on ACL miss, causing silent data loss: - updateUnpublishedAction: REST controller returned empty response - deleteGivenNewAction: delete chain returned Mono.empty() - archiveGivenNewAction: archive chain returned Mono.empty() Add .switchIfEmpty() fallback before .onErrorResume() on all three paths so the embedded datasource is used for analytics when the ACL lookup yields empty. Also: - Preserve isAutoGenerated and datasourceConfiguration on the validateAction fallback datasource so saved invalid actions retain display-relevant fields. - Add warning log in populateHintMessages onErrorResume to surface data-integrity issues instead of silently swallowing errors.
Copilot Round 2 — All 5 comments addressed (commit 652071f)1.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (1)
632-643:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid persisting the full datasource config on ACL fallback.
When the ACL-scoped lookup misses, this fallback now embeds
datasourceConfigurationinto the action. That bypasses the normal external-datasource-by-reference flow at Lines 690-700 and can expose connection settings to users who can read the action but not the datasource. Keep only display-safe fields here, or redact the config before persisting.Suggested minimal change
Datasource clientDs = editActionDTO.getDatasource(); Datasource fallback = new Datasource(); fallback.setPluginId(clientDs.getPluginId()); fallback.setName(clientDs.getName()); fallback.setIsAutoGenerated(clientDs.getIsAutoGenerated()); - fallback.setDatasourceConfiguration(clientDs.getDatasourceConfiguration()); return Mono.just(fallback); }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java` around lines 632 - 643, In the switchIfEmpty fallback in NewActionServiceCEImpl (the block manipulating editActionDTO and creating Datasource fallback), do not copy or set datasourceConfiguration onto the fallback Datasource; instead construct the fallback with only display-safe fields (e.g., pluginId, name, isAutoGenerated, and optionally id/display fields) and ensure datasourceConfiguration is null or redacted so the full connection settings are not embedded into the action DTO; update any subsequent code that expects a config to rely on the external-datasource-by-reference flow rather than this fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java`:
- Around line 632-643: In the switchIfEmpty fallback in NewActionServiceCEImpl
(the block manipulating editActionDTO and creating Datasource fallback), do not
copy or set datasourceConfiguration onto the fallback Datasource; instead
construct the fallback with only display-safe fields (e.g., pluginId, name,
isAutoGenerated, and optionally id/display fields) and ensure
datasourceConfiguration is null or redacted so the full connection settings are
not embedded into the action DTO; update any subsequent code that expects a
config to rely on the external-datasource-by-reference flow rather than this
fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c2e3476-f2be-4809-8bcc-162be66140e1
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
Description
Fixes GHSA-rqp9-r5fr-4cc8 — Cross-tenant datasource takeover via unprotected findById in NewActionServiceCEImpl.
Vulnerability
NewActionServiceCEImpl.validateAction()used the 1-argumentdatasourceService.findById(id)— which performs no ACL check — to resolve the datasource referenced by an action's DTO. Since thedatasource.idfield is fully client-controlled (overwritten viacopyNestedNonNullPropertiesdeep merge), any authenticated user could reference a datasource in another tenant's workspace.For public apps, the subsequent
updateDatasourcePolicyForPublicAction()method then granted anonymousEXECUTE_DATASOURCESon the foreign datasource, enabling unauthenticated execution of queries against the victim's database using stored credentials.This is the same 1-arg
findByIdanti-pattern as GHSA-rg2x-4v4h-g78w (fixed inAuthenticationServiceCEImpl), applied here to the action create/update path.Fix
Primary — ACL on datasource fetch: Replace
datasourceService.findById(id)withfindById(id, datasourcePermission.getExecutePermission())invalidateAction(). Users must have execute permission on a datasource to reference it in an action.Defense-in-depth — workspace boundary check: Add workspace membership verification in
updateDatasourcePolicyForPublicAction()— reject withUNAUTHORIZED_ACCESSif the datasource's workspace doesn't match the action's workspace. This prevents cross-workspace policy grants even if a future code path omits the ACL check.ACL on application fetch: Replace
applicationService.findById(applicationId)with the ACL-checked variant in the policy mutation path.Hardening — analytics/hint paths: Add ACL with graceful fallback (
.onErrorResume()) to all other 1-argdatasourceService.findById()calls in the same file (post-update analytics, post-delete analytics, hint generation, archive path). These prevented cross-tenant datasource metadata leaks.Affected Code
All changes are in a single file:
NewActionServiceCEImpl.java— 6 call sites fixed.validateAction— primary vulnerabilitydatasourcePermission.getExecutePermission()updateDatasourcePolicyForPublicActionupdateDatasourcePolicyForPublicActionapplicationService.findByIdupdateUnpublishedAction— analyticsdeleteUnpublishedAction— analyticsarchiveGivenNewAction— analyticsCE/EE Strategy
CE-only safe.
NewActionServiceImplis a pass-through with no method overrides. Hourly CE→EE sync will carry these changes cleanly.Testing
NewActionServiceSecurityTest.java:validateAction_shouldFetchDatasourceWithPermissionCheck_GHSA_rqp9— verifies ACL enforcementvalidateAction_shouldRejectCrossWorkspaceDatasourcePolicyGrant_GHSA_rqp9— verifies workspace boundary checkLinear Ticket
APP-15250
Severity
Critical (assessed CVSS 9.1) — CWE-862 (Missing Authorization), CWE-639 (IDOR)
/test all
Summary by CodeRabbit
Bug Fixes
Tests
Important
🟣 🟣 🟣 Your tests are running.
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/27101140319
Commit: d4e6cab
Workflow:
PR Automation test suiteTags:
@tag.AllSpec: ``
Sun, 07 Jun 2026 18:32:02 UTC