feat: embedded MCP server with user-scoped token authentication (#15383)#41980
Open
salevine wants to merge 57 commits into
Open
feat: embedded MCP server with user-scoped token authentication (#15383)#41980salevine wants to merge 57 commits into
salevine wants to merge 57 commits into
Conversation
…15383) Add an opt-in, same-instance Streamable HTTP MCP endpoint that acts as the calling Appsmith user. MCP clients authenticate with a user-scoped bearer token; the Node service forwards that token to existing /api/v1 endpoints so Spring Security reconstructs the real user and existing workspace/app/page ACLs authorize every operation. No privileged/internal credential is used. Server (CE, EE-overridable via *CE base + thin concrete subclass split): - UserMcpToken domain/repository/service + McpTokenController for create/list/ revoke of user-scoped tokens (SHA-256 pre-hash then bcrypt at rest, plaintext shown once, max 10 active tokens/user). - Bearer AuthenticationWebFilter (mcp_ prefix) reconstructs the token owner; invalid/revoked/disabled tokens return 401. - Migration076 creates the userMcpToken indexes (auto-index-creation is off). Node service (app/client/packages/mcp): - Streamable HTTP transport, loopback bind, /health endpoint, request body cap, per-request token revalidation, per-session token binding, and per-user + global session caps. - Tools: list_workspaces, list_applications, get_application_context, and import_application_artifact / import_partial_application_artifact (validated artifact upload through the existing import APIs). Client: - MCP token management UI in the user profile (create / copy-once / revoke). Deploy/CI: - Opt-in APPSMITH_MCP_ENABLED gate (default off) for supervisord autostart and the Caddy /mcp route; Dockerfile copy, mcp-build workflow, route health test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
Property Pane
|
Whoops! Looks like you're using an outdated method of running the Cypress suite. |
Contributor
|
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:
WalkthroughAdds an MCP server, secure user token lifecycle, User Profile token management, backend authentication, Docker runtime support, and CI workflows that build and package MCP artifacts. ChangesMCP app builder and server
Token lifecycle and UI
Deployment and CI
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
salevine
added
ok-to-test
Failed server tests
|
![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: 11
🧹 Nitpick comments (4)
app/client/packages/mcp/src/app.ts (1)
67-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
objectKeysfrom@appsmith/utilsinstead ofObject.keys.Static analysis flags this per the repo's internal lint rule for consistent object-key handling.
🤖 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/client/packages/mcp/src/app.ts` at line 67, Replace the Object.keys call in the artifact emptiness check with the repository’s objectKeys utility imported from `@appsmith/utils`, while preserving the existing condition and behavior.Source: Linters/SAST tools
app/client/src/pages/UserProfile/McpTokens.test.tsx (1)
9-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFull-module mock of
McpTokenApiskips coverage oflist()'s response normalization.Mocking
McpTokenApi.listdirectly (rather than mockingApi.get) means this suite never exercises the array/response-unwrapping logic inside the reallist()implementation — see the concern raised inMcpTokenApi.ts.Also applies to: 32-38
🤖 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/client/src/pages/UserProfile/McpTokens.test.tsx` around lines 9 - 16, Replace the full-module mock of McpTokenApi with a mock of the underlying Api.get request, while retaining mocks for create and revoke as needed, so tests invoke the real McpTokenApi.list implementation and cover its array/response-unwrapping normalization logic..github/workflows/mcp-build.yml (1)
42-60: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider
persist-credentials: falseon checkout.zizmor flags all three checkout steps for credential persistence in the git config, which could be exfiltrated by any subsequent step/dependency script in this job.
🔒 Disable credential persistence
- name: Checkout the merged pull-request commit if: inputs.pr != 0 uses: actions/checkout@v4 with: fetch-tags: true ref: refs/pull/${{ inputs.pr }}/merge + persist-credentials: falseApply similarly to the other two checkout steps.
🤖 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 @.github/workflows/mcp-build.yml around lines 42 - 60, All three checkout steps persist GitHub credentials in the local Git config. Add persist-credentials: false to the with configuration of the checkout steps identified by “Checkout the merged pull-request commit,” “Checkout the specified branch,” and “Checkout the head commit.”Source: Linters/SAST tools
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java (1)
196-199: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider setting a stateless authentication success handler on the MCP filter.
AuthenticationWebFilterdefaults toWebSessionServerAuthenticationSuccessHandler, which creates a WebSession on each successful MCP token authentication. For bearer-token (stateless) auth, this is unnecessary session overhead. Set a no-op orSavedRequestServerAuthenticationSuccessHandlerto keep MCP auth stateless.♻️ Proposed fix
mcpTokenAuthenticationWebFilter.setServerAuthenticationConverter(mcpTokenAuthenticationConverter); mcpTokenAuthenticationWebFilter.setAuthenticationFailureHandler(failureHandler); +mcpTokenAuthenticationWebFilter.setAuthenticationSuccessHandler( + new ServerAuthenticationSuccessHandler() { + `@Override` + public Mono<Void> onAuthenticationSuccess(WebFilterExchange webFilterExchange, Authentication authentication) { + return webFilterExchange.getChain().filter(webFilterExchange.getExchange()); + } + });🤖 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/configurations/SecurityConfig.java` around lines 196 - 199, Configure a stateless authentication success handler on the AuthenticationWebFilter created in SecurityConfig for MCP token authentication, replacing the default WebSessionServerAuthenticationSuccessHandler; use an appropriate no-op or SavedRequestServerAuthenticationSuccessHandler while retaining the existing failure handler.
🤖 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 @.github/workflows/build-client-server.yml:
- Around line 118-128: Update the mcp-build job’s if condition to compare
needs.file-check.outputs.runId with the quoted string '0', matching the runId
comparisons used by the other jobs in this workflow.
In @.github/workflows/mcp-build.yml:
- Around line 84-91: Remove the duplicate “Lint” step running yarn lint from the
workflow, keeping only one lint invocation alongside the existing formatting
check.
In `@app/client/packages/mcp/build.js`:
- Around line 3-12: Update the esbuild configuration in the build script to
derive the target from only the major Node version, such as by splitting
process.versions.node before constructing the target string; alternatively use a
fixed major-only value like node20. Ensure the target passed in the
esbuild.build call is accepted by esbuild.
In `@app/client/packages/mcp/src/app.test.ts`:
- Around line 294-298: Update the mockResolvedValueOnce object in the “fails
safely when session token revalidation fails” test to Prettier’s multiline
object-literal format, preserving its existing username and isAnonymous values.
- Line 59: Insert a blank line immediately before the for loop iterating over
callIndex in app.test.ts, preserving the required
padding-line-between-statements ESLint formatting.
- Around line 69-83: Fix the Prettier formatting in the test around the
fullArtifact and partialArtifact assertions: add the required blank line before
the fullArtifact declaration and collapse the fullArtifact.text() await expect
assertion to one line, matching the project’s formatting rules.
In `@app/client/packages/mcp/src/app.ts`:
- Line 14: Fix the Prettier formatting violations in app.ts at the declarations
and code associated with MAX_ARTIFACT_BYTES and the flagged lines 33, 120, and
297; run Prettier on the file and verify the build formatting check passes.
- Around line 143-159: Update the request function to enforce a finite timeout
for every fetchFn call. Create an AbortController, schedule it to abort after
the configured timeout, pass its signal into the fetch options while preserving
any caller-provided signal behavior, and clear the timeout in a finally block so
completed requests do not retain timers.
- Around line 448-458: Add Origin/Host validation for the /mcp endpoint before
creating or handling the StreamableHTTPServerTransport in the surrounding
request handler. Reject requests whose Origin or Host is not an explicitly
allowed local/ configured value, using middleware or equivalent request checks
rather than transport defaults; ensure rejected requests do not create sessions
or reach MCP processing.
In `@app/client/packages/mcp/src/server.ts`:
- Around line 8-18: Update reportProcessFailure to terminate the MCP process
after recording the failure: retain the stderr message, then call
process.exit(1) rather than only setting process.exitCode. Keep the
uncaughtException and unhandledRejection handlers wired to this function, and
apply the repository’s Prettier formatting to the affected code.</codeેન
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserMcpTokenServiceCEImpl.java`:
- Around line 112-128: Update extractTokenId in UserMcpTokenServiceCEImpl to
handle a null token before calling startsWith, returning null for null or
invalid credentials so McpTokenAuthenticationManager can fall back to
Mono.empty() instead of throwing.
---
Nitpick comments:
In @.github/workflows/mcp-build.yml:
- Around line 42-60: All three checkout steps persist GitHub credentials in the
local Git config. Add persist-credentials: false to the with configuration of
the checkout steps identified by “Checkout the merged pull-request commit,”
“Checkout the specified branch,” and “Checkout the head commit.”
In `@app/client/packages/mcp/src/app.ts`:
- Line 67: Replace the Object.keys call in the artifact emptiness check with the
repository’s objectKeys utility imported from `@appsmith/utils`, while preserving
the existing condition and behavior.
In `@app/client/src/pages/UserProfile/McpTokens.test.tsx`:
- Around line 9-16: Replace the full-module mock of McpTokenApi with a mock of
the underlying Api.get request, while retaining mocks for create and revoke as
needed, so tests invoke the real McpTokenApi.list implementation and cover its
array/response-unwrapping normalization logic.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java`:
- Around line 196-199: Configure a stateless authentication success handler on
the AuthenticationWebFilter created in SecurityConfig for MCP token
authentication, replacing the default
WebSessionServerAuthenticationSuccessHandler; use an appropriate no-op or
SavedRequestServerAuthenticationSuccessHandler while retaining the existing
failure handler.
🪄 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: 3f3c434d-959f-49df-8c12-f94b0bd1a5e6
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (56)
.github/workflows/ad-hoc-docker-image.yml.github/workflows/build-client-server-count.yml.github/workflows/build-client-server.yml.github/workflows/build-docker-image.yml.github/workflows/docs/test-build-docker-image.md.github/workflows/github-release.yml.github/workflows/mcp-build.yml.github/workflows/on-demand-build-docker-image-deploy-preview.yml.github/workflows/playwright-e2e.yml.github/workflows/pr-cypress.yml.github/workflows/test-build-docker-image.ymlDockerfileapp/client/packages/mcp/.env.exampleapp/client/packages/mcp/.eslintignoreapp/client/packages/mcp/build.jsapp/client/packages/mcp/build.shapp/client/packages/mcp/jest.config.cjsapp/client/packages/mcp/package.jsonapp/client/packages/mcp/src/app.test.tsapp/client/packages/mcp/src/app.tsapp/client/packages/mcp/src/server.tsapp/client/packages/mcp/start-server.shapp/client/packages/mcp/tsconfig.jsonapp/client/src/api/McpTokenApi.tsapp/client/src/ce/constants/messages.tsapp/client/src/pages/UserProfile/McpTokens.test.tsxapp/client/src/pages/UserProfile/McpTokens.tsxapp/client/src/pages/UserProfile/index.tsxapp/server/appsmith-server/src/main/java/com/appsmith/server/authentication/converters/McpTokenAuthenticationConverter.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/authentication/managers/McpTokenAuthenticationManager.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/authentication/tokens/McpTokenAuthentication.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/controllers/McpTokenController.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/McpTokenControllerCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/domains/UserMcpToken.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/UserMcpTokenCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/dtos/McpTokenResponseDTO.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration076AddUserMcpTokenIndexes.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/repositories/UserMcpTokenRepository.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/UserMcpTokenRepositoryCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/UserMcpTokenService.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/UserMcpTokenServiceImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserMcpTokenServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserMcpTokenServiceCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/authentication/McpTokenAuthenticationWebFilterTest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/authentication/converters/McpTokenAuthenticationConverterTest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/authentication/managers/McpTokenAuthenticationManagerTest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/UserMcpTokenServiceImplTest.javacontributions/ServerSetup.mddeploy/docker/fs/opt/appsmith/caddy-reconfigure.mjsdeploy/docker/fs/opt/appsmith/entrypoint.shdeploy/docker/fs/opt/appsmith/healthcheck.shdeploy/docker/fs/opt/appsmith/run-mcp.shdeploy/docker/fs/opt/appsmith/templates/supervisord/application_process/mcp.confdeploy/docker/route-tests/common/mcp-health.hurlscripts/local_testing.sh
Comment on lines
+84
to
+91
| - name: Lint | ||
| run: yarn lint | ||
|
|
||
| - name: Check formatting | ||
| run: yarn prettier | ||
|
|
||
| - name: Lint | ||
| run: yarn lint |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Duplicate yarn lint step.
Lint runs at Line 85 and again at Line 91, wasting CI time for no benefit.
🧹 Remove the duplicate step
- name: Check formatting
run: yarn prettier
- - name: Lint
- run: yarn lint
-
- name: Run unit tests📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Lint | |
| run: yarn lint | |
| - name: Check formatting | |
| run: yarn prettier | |
| - name: Lint | |
| run: yarn lint | |
| - name: Lint | |
| run: yarn lint | |
| - name: Check formatting | |
| run: yarn prettier |
🤖 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 @.github/workflows/mcp-build.yml around lines 84 - 91, Remove the duplicate
“Lint” step running yarn lint from the workflow, keeping only one lint
invocation alongside the existing formatting check.
Comment on lines
+448
to
+458
| transport = new StreamableHTTPServerTransport({ | ||
| sessionIdGenerator: randomUUID, | ||
| onsessioninitialized: (id) => { | ||
| sessions.set(id, { | ||
| expiresAt: now() + sessionTtlMs, | ||
| token, | ||
| username, | ||
| transport: transport!, | ||
| }); | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n '"`@modelcontextprotocol/sdk`"' app/client/packages/mcp/package.jsonRepository: appsmithorg/appsmith
Length of output: 1992
🌐 Web query:
@modelcontextprotocol/sdk 1.12.0 StreamableHTTPServerTransport enableDnsRebindingProtection allowedOrigins
💡 Result:
In the @modelcontextprotocol/sdk, StreamableHTTPServerTransport provides DNS rebinding protection to secure HTTP-based servers running on localhost [1][2][3]. The following configuration options control this feature: enableDnsRebindingProtection (boolean, default: false): Must be explicitly set to true to enable the protection mechanism [1][4][5]. allowedHosts (string[], optional): A list of allowed Host header values [1][4][5]. If not specified, host validation is disabled [1][5]. allowedOrigins (string[], optional): A list of allowed Origin header values [1][4][5]. If not specified, origin validation is disabled [1][5]. When enabled, the transport validates incoming request headers against the provided allowedHosts and allowedOrigins lists [4]. If the headers do not match, the request is rejected [4]. Security Note: DNS rebinding protection is disabled by default for backwards compatibility [1][2][6]. It is highly recommended to enable this protection when running MCP servers locally without authentication [2][3]. For easier implementation, users are encouraged to use createMcpExpressApp, which enables this protection by default when binding to localhost [2][3]. Servers using the stdio transport are not affected by this vulnerability [2]. Failure to configure these settings correctly on unauthenticated local servers may allow malicious websites to perform DNS rebinding attacks [2][7].
Citations:
- 1: https://ts.sdk.modelcontextprotocol.io/v2/api/@modelcontextprotocol/server/server/streamableHttp.html
- 2: GHSA-w48q-cv73-mx4w
- 3: https://ts.sdk.modelcontextprotocol.io/documents/server.html
- 4: https://github.com/modelcontextprotocol/typescript-sdk/blob/e74a358728991216391995e8daa5d0573614abc5/src/server/streamableHttp.ts
- 5: https://cdn.jsdelivr.net/npm/@modelcontextprotocol/sdk@1.20.2/dist/esm/server/streamableHttp.d.ts
- 6: While using Streamable HTTP transport, can't use async callback for tools modelcontextprotocol/typescript-sdk#1106
- 7: https://nvd.nist.gov/vuln/detail/CVE-2025-66414
🌐 Web query:
site:github.com modelcontextprotocol sdk enableDnsRebindingProtection allowedOrigins StreamableHTTPServerTransport changelog
💡 Result:
In the Model Context Protocol (MCP) SDKs (such as the TypeScript and Kotlin SDKs), enableDnsRebindingProtection, allowedHosts, and allowedOrigins are configuration options for the StreamableHTTPServerTransport to defend against DNS rebinding attacks [1][2][3][4]. When enableDnsRebindingProtection is set to true, the transport validates incoming HTTP requests to ensure they originate from trusted sources [1][5]. Specifically, if configured, the transport checks the Host and Origin headers of incoming requests against the allowedHosts and allowedOrigins lists, respectively [1][2][3]. If a header does not match an entry in the corresponding allow-list, the request is rejected, typically with a 403 Forbidden status [1][5]. Key details regarding these settings: - Purpose: They act as a security layer for servers, particularly those running on loopback interfaces, to prevent malicious websites from using DNS rebinding to send unauthorized requests to local MCP servers [2][3][5]. - Default behavior: For backwards compatibility, enableDnsRebindingProtection defaults to false in many implementations [1]. Operators are encouraged to enable it and explicitly define allowedHosts and allowedOrigins for production or remote-accessible deployments [2][3]. - Non-browser clients: When allowedOrigins is configured, the validator requires an Origin header to be present [2]. Consequently, non-browser clients (such as curl) must include an appropriate Origin header in their requests to bypass this check [2]. These options are standard features in the current versions of the MCP TypeScript SDK (e.g., within WebStandardStreamableHTTPServerTransport and related classes) and the Kotlin SDK [6][4][7][8]. There is no specific single "changelog" entry for these settings as they were introduced as part of the transport security hardening effort within the SDKs [3][5].
Citations:
- 1: https://github.com/modelcontextprotocol/typescript-sdk/blob/e74a358728991216391995e8daa5d0573614abc5/src/server/streamableHttp.ts
- 2: https://github.com/jeanibarz/knowledge-base-mcp-server/blob/main/docs/rfcs/008-remote-transport.md
- 3: Harden HTTP transport: enable DNS-rebinding protection and default to loopback bind brave/brave-search-mcp-server#314
- 4: https://github.com/modelcontextprotocol/kotlin-sdk/releases
- 5: fix: enable DNS rebinding protection in HTTP transport dynatrace-oss/dynatrace-mcp#540
- 6: https://github.com/modelcontextprotocol/kotlin-sdk/blob/main/kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/StreamableHttpServerTransport.kt
- 7: https://github.com/modelcontextprotocol/typescript-sdk/blob/95d28cbae98e57df1b987b656a555777de976545/packages/server/src/server/streamableHttp.ts
- 8: https://github.com/modelcontextprotocol/typescript-sdk/blob/2c0c481cb9dbfd15c8613f765c940a5f5bace94d/packages/server/src/server/streamableHttp.ts
🌐 Web query:
npm @modelcontextprotocol/sdk StreamableHTTPServerTransport allowedOrigins enableDnsRebindingProtection
💡 Result:
In the Model Context Protocol (MCP) TypeScript SDK, allowedOrigins and enableDnsRebindingProtection are configuration options for StreamableHTTPServerTransport (and its underlying WebStandardStreamableHTTPServerTransport) designed to provide security against DNS rebinding and unauthorized cross-origin requests [1][2][3]. However, please note that these specific properties have been marked as deprecated in recent versions of the SDK [1]. The official guidance is to use external middleware for host and origin validation instead of relying on these built-in transport options [4][1]. Configuration Details: - allowedOrigins: A list of strings representing allowed Origin header values [1][2]. If configured, incoming requests with a present but disallowed Origin header are rejected with a 403 error [4][1]. If left unspecified, origin validation is disabled [1][2]. - enableDnsRebindingProtection: A boolean flag that, when set to true, enables DNS rebinding protection [1][2][3]. It requires allowedHosts and/or allowedOrigins to be configured to function effectively [1][2]. It defaults to false for backwards compatibility [1][2][3]. Security Context: Running an HTTP-based MCP server on localhost without proper authentication can expose it to vulnerabilities [5]. If you are running an MCP server locally, it is recommended to enable these protections (or implement equivalent middleware) and explicitly configure allowedHosts (e.g., to include ['127.0.0.1', 'localhost']) and/or allowedOrigins to secure your server against malicious access attempts [6]. For implementation, modern applications should prefer mounting framework-specific middleware or using the framework-agnostic helpers (e.g., originValidationResponse) provided by @modelcontextprotocol/server to guard your endpoints before requests reach the transport handler [4].
Citations:
- 1: https://github.com/modelcontextprotocol/typescript-sdk/blob/2c0c481cb9dbfd15c8613f765c940a5f5bace94d/packages/server/src/server/streamableHttp.ts
- 2: https://cdn.jsdelivr.net/npm/@modelcontextprotocol/sdk@1.20.2/dist/esm/server/streamableHttp.d.ts
- 3: https://github.com/modelcontextprotocol/typescript-sdk/blob/e74a358728991216391995e8daa5d0573614abc5/src/server/streamableHttp.ts
- 4: https://github.com/modelcontextprotocol/TypeScript-sdk/blob/main/docs/server.md
- 5: https://nvd.nist.gov/vuln/detail/CVE-2025-66414
- 6: While using Streamable HTTP transport, can't use async callback for tools modelcontextprotocol/typescript-sdk#1106
Add Origin/Host validation on /mcp. The bearer token reduces exposure, but this localhost HTTP endpoint still needs a rebinding guard; use middleware or equivalent checks instead of relying on transport defaults.
🤖 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/client/packages/mcp/src/app.ts` around lines 448 - 458, Add Origin/Host
validation for the /mcp endpoint before creating or handling the
StreamableHTTPServerTransport in the surrounding request handler. Reject
requests whose Origin or Host is not an explicitly allowed local/ configured
value, using middleware or equivalent request checks rather than transport
defaults; ensure rejected requests do not create sessions or reach MCP
processing.
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/29135322682. |
Add the high-level page-spec app-builder that compiles to a validated Appsmith import artifact, and harden the MCP surface following an adversarial red-team pass. App-builder (app/client/packages/mcp/src/builder): - Zod page/app/edit spec + best-effort placement, curated widget templates, 64-column auto-layout, spec -> import-artifact compiler with depth and total-widget caps, presets, and a capability catalog. - New tools: get_capabilities, list_presets, get_preset, validate_app_spec (dry-run), build_application, edit_page (append-only, best-effort placement). - Removed the raw artifact-import tools (an arbitrary-content injection surface) in favour of the validated compiler. Security hardening (from the red-team review): - Atomic session admission so concurrent initializes cannot bypass the per-user / global session caps (whole-instance DoS). - Stop crash-looping: guard fire-and-forget transport.close(), log instead of process.exit on unhandledRejection, supervisord startsecs=5. - Do not evict a session on token mismatch (targeted DoS via a leaked session id). - Bound inbound sockets (requestTimeout / headersTimeout / maxConnections). - Token expiry (90d) enforced at authentication; block minting new MCP tokens from an MCP-authenticated principal (no post-revocation persistence). Tests: 39 Node tests (compiler, placement, edit, tool wiring, concurrent session caps, mismatch-no-evict) and Java unit tests for token expiry and the auth marker. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/29160630878. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
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/services/ce/UserMcpTokenServiceCEImpl.java (1)
38-47: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winExpired tokens still consume the active-token quota.
countByUserIdAndDeletedAtIsNullcounts non-deleted tokens regardless ofexpiresAt, so a user who accumulates expired (but un-revoked) tokens can be blocked from creating new ones atMAX_ACTIVE_TOKENS_PER_USERuntil they manually revoke. Consider excluding expired tokens from the count or purging them.🤖 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/services/ce/UserMcpTokenServiceCEImpl.java` around lines 38 - 47, Update the active-token quota check in UserMcpTokenServiceCEImpl.create to exclude tokens whose expiresAt is in the past, using an expiry-aware repository query or purging expired tokens before counting. Preserve the existing deleted-token filtering and MAX_ACTIVE_TOKENS_PER_USER limit for currently valid tokens.
🧹 Nitpick comments (1)
docs/plans/2026-07-11-mcp-app-builder-design.md (1)
82-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language specifier to the fenced code block.
Markdownlint MD040 flags code blocks without a language. Use
textorplaintextfor the directory structure.📝 Proposed fix
-``` +```text src/builder/ schema.ts Zod schema: page spec + app spec + edit spec + placement templates.ts curated getDefaults per widget (text,input,select,button,image,table,container) layout.ts vertical auto-placement on the 64-col grid; placement resolution compile.ts pageSpec[] -> import artifact; edit merge into existing DSL; depth/total-widget caps presets.ts form, table-detail, card-grid, crud capabilities.ts machine-readable widget/preset catalog</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/plans/2026-07-11-mcp-app-builder-design.mdaround lines 82 - 90, Update
the fenced directory-structure block in the design document to include a text or
plaintext language specifier, preserving its contents and formatting.</details> <!-- cr-comment:v1:ff970ee68c5d25f17169e84b --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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/client/packages/mcp/src/app.test.ts:
- Around line 297-304: Update the excluded-tool assertion in the test around
names so it verifies every listed tool is absent, rather than only rejecting the
case where all are present. Use expect.not.arrayContaining with the excluded
tool names or add individual not.toContain checks, while preserving the existing
tool-list validation.In
@app/client/packages/mcp/src/builder/compile.ts:
- Around line 285-301: The placement update after adding a node only grows the
targeted inner canvas, leaving its enclosing CONTAINER_WIDGET height stale for
inside edits. In the resolvePlacement inside-edit flow, after updating
placement.canvas.bottomRow, also update the parent container’s bottomRow to
encompass the child canvas’s new extent, preserving existing root-canvas sizing
behavior.In
@app/client/packages/mcp/src/builder/schema.ts:
- Around line 20-25: Update placementSchema to enforce mutual exclusivity
between its optional after and inside fields with a refinement, while continuing
to allow either field alone or neither field. Keep the existing non-empty string
validation and strict object behavior unchanged.In
@app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/managers/McpTokenAuthenticationManagerTest.java:
- Around line 99-104: Remove the unconditional unknown-source rate-limit stub
from the shared manager() helper. Stub
RateLimitConstants.BUCKET_KEY_FOR_MCP_AUTHENTICATION with "unknown" only in
tests that invoke that lookup, or mark the shared stub lenient, while preserving
the existing manager construction.
Outside diff comments:
In
@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserMcpTokenServiceCEImpl.java:
- Around line 38-47: Update the active-token quota check in
UserMcpTokenServiceCEImpl.create to exclude tokens whose expiresAt is in the
past, using an expiry-aware repository query or purging expired tokens before
counting. Preserve the existing deleted-token filtering and
MAX_ACTIVE_TOKENS_PER_USER limit for currently valid tokens.
Nitpick comments:
In@docs/plans/2026-07-11-mcp-app-builder-design.md:
- Around line 82-90: Update the fenced directory-structure block in the design
document to include a text or plaintext language specifier, preserving its
contents and formatting.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `a1429438-95f0-4da8-aec5-f218de3fea14` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between f07f6bf587b22f2c50a023f716837e70d80bcf29 and b55ba0aecbf5bae02c3f98acdaff88861624b3a8. </details> <details> <summary>📒 Files selected for processing (31)</summary> * `.github/workflows/build-client-server.yml` * `.github/workflows/mcp-build.yml` * `app/client/packages/mcp/build.js` * `app/client/packages/mcp/src/app.test.ts` * `app/client/packages/mcp/src/app.ts` * `app/client/packages/mcp/src/builder/builder.test.ts` * `app/client/packages/mcp/src/builder/capabilities.ts` * `app/client/packages/mcp/src/builder/compile.ts` * `app/client/packages/mcp/src/builder/layout.ts` * `app/client/packages/mcp/src/builder/presets.ts` * `app/client/packages/mcp/src/builder/schema.ts` * `app/client/packages/mcp/src/builder/templates.ts` * `app/client/packages/mcp/src/server.ts` * `app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/converters/McpTokenAuthenticationConverter.java` * `app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/managers/McpTokenAuthenticationManager.java` * `app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/tokens/McpTokenAuthentication.java` * `app/server/appsmith-server/src/main/java/com/appsmith/server/constants/RateLimitConstants.java` * `app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/McpTokenControllerCE.java` * `app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/UserMcpTokenCE.java` * `app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/McpTokenResponseDTO.java` * `app/server/appsmith-server/src/main/java/com/appsmith/server/ratelimiting/RateLimitConfig.java` * `app/server/appsmith-server/src/main/java/com/appsmith/server/ratelimiting/ce/RateLimitServiceCE.java` * `app/server/appsmith-server/src/main/java/com/appsmith/server/ratelimiting/ce/RateLimitServiceCEImpl.java` * `app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserMcpTokenServiceImpl.java` * `app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserMcpTokenServiceCEImpl.java` * `app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/McpTokenAuthenticationWebFilterTest.java` * `app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/converters/McpTokenAuthenticationConverterTest.java` * `app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/managers/McpTokenAuthenticationManagerTest.java` * `app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserMcpTokenServiceImplTest.java` * `deploy/docker/fs/opt/appsmith/templates/supervisord/application_process/mcp.conf` * `docs/plans/2026-07-11-mcp-app-builder-design.md` </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (8)</summary> * deploy/docker/fs/opt/appsmith/templates/supervisord/application_process/mcp.conf * app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/converters/McpTokenAuthenticationConverterTest.java * app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/UserMcpTokenCE.java * app/client/packages/mcp/build.js * app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/converters/McpTokenAuthenticationConverter.java * app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/McpTokenControllerCE.java * app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/McpTokenAuthenticationWebFilterTest.java * .github/workflows/build-client-server.yml </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Failed server tests
|
…on-reservation leak From adversarial + UX + architect reviews: - healthcheck.sh: only health-check the mcp supervisord program when it exists, so a container with MCP disabled (the default) is not reported unhealthy fleet-wide. - Surface token expiry in the profile UI (list rows + created-token modal) — the server already returned expiresAt; the client was discarding it, so agents would die silently at 90 days. Fail closed on a null expiry server-side. - Release the MCP session reservation at onsessioninitialized (registration) instead of only in finally, so a stalled initialize SSE stream cannot pin the reservation and saturate the session caps. - Exclude packages/mcp from the client tsconfig (it has its own tsc, like rts) and use as-unknown-as casts in McpTokenApi so the client check-types passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserMcpTokenServiceCEImpl.java (1)
38-65: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoffCount-then-save is a TOCTOU on the active-token cap.
countByUserIdAndDeletedAtIsNullfollowed bysaveisn't atomic, so concurrentcreaterequests for the same user can each pass the check and push the active count pastMAX_ACTIVE_TOKENS_PER_USER. Low blast radius (a soft safety cap), so fine to defer — flagging for awareness.🤖 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/services/ce/UserMcpTokenServiceCEImpl.java` around lines 38 - 65, Update UserMcpTokenServiceCEImpl.create so enforcing MAX_ACTIVE_TOKENS_PER_USER is atomic with token creation, preventing concurrent requests from passing the count check and exceeding the active-token cap. Replace the separate countByUserIdAndDeletedAtIsNull-then-save flow with the repository or transaction-level mechanism used for atomic enforcement, while preserving the existing error and response behavior.
🤖 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 `@deploy/docker/fs/opt/appsmith/healthcheck.sh`:
- Around line 3-8: Preserve health checks for supervisor-managed mongo and redis
while making MCP checks conditional. In the health-check logic around the
processes list and supervisorctl status invocation, revert to querying
unfiltered supervisor status so absent MCP remains naturally excluded, or
conditionally add mongo and redis alongside mcp if retaining filtering; ensure
the existing mongo and redis branches remain reachable.
- Line 5: Update the health-check branch that currently matches “server” to
match “backend”, aligning it with the backend entry in the processes list so the
HTTP endpoint probe runs in addition to the RUNNING check; alternatively,
support both names without changing the existing probe behavior.
---
Nitpick comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserMcpTokenServiceCEImpl.java`:
- Around line 38-65: Update UserMcpTokenServiceCEImpl.create so enforcing
MAX_ACTIVE_TOKENS_PER_USER is atomic with token creation, preventing concurrent
requests from passing the count check and exceeding the active-token cap.
Replace the separate countByUserIdAndDeletedAtIsNull-then-save flow with the
repository or transaction-level mechanism used for atomic enforcement, while
preserving the existing error and response behavior.
🪄 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: 02f59e36-1a8a-4bd3-bbc4-72dc6e52768c
📒 Files selected for processing (8)
app/client/packages/mcp/src/app.tsapp/client/src/api/McpTokenApi.tsapp/client/src/ce/constants/messages.tsapp/client/src/pages/UserProfile/McpTokens.test.tsxapp/client/src/pages/UserProfile/McpTokens.tsxapp/client/tsconfig.jsonapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserMcpTokenServiceCEImpl.javadeploy/docker/fs/opt/appsmith/healthcheck.sh
✅ Files skipped from review due to trivial changes (1)
- app/client/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (5)
- app/client/src/ce/constants/messages.ts
- app/client/src/api/McpTokenApi.ts
- app/client/src/pages/UserProfile/McpTokens.test.tsx
- app/client/src/pages/UserProfile/McpTokens.tsx
- app/client/packages/mcp/src/app.ts
CI server-unit-tests failures: - CsrfTest: the MCP AuthenticationWebFilter, added at AUTHENTICATION order with a match-any matcher, collided with the form-login filter and broke POST /api/v1/login (No provider found for UsernamePasswordAuthenticationToken -> 500). Restrict the filter to only engage for 'Bearer mcp_' requests so it never touches other auth flows. - McpTokenAuthenticationManagerTest: make the shared rate-limit stub lenient (tests that reject non-MCP / rate-limited requests never reach it) and wrap the failure-path tryIncreaseCounter in Mono.defer so it is not eagerly evaluated on the success path (which NPE'd on the unstubbed mock). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Failed server tests
|
Failed server tests
|
- SecurityConfig: add the MCP AuthenticationWebFilter BEFORE the AUTHENTICATION order instead of AT it. Two AuthenticationWebFilters at the same order break form-login's manager wiring, causing POST /api/v1/login to 500 (CsrfTest failure). - McpTokens.test.tsx: assert the full monospace font-family stack that actually renders; toHaveStyle requires the exact value, not a prefix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Failed server tests
|
Root cause of the CsrfTest failure: McpTokenAuthenticationManager was the only ReactiveAuthenticationManager @component in the server, so Spring Boot auto-wired it as the global default authentication manager. Form-login (which sets no explicit manager) then used it and rejected UsernamePasswordAuthenticationToken with 'No provider found' -> POST /api/v1/login returned 500. Fix: drop @component from the manager and construct it directly in SecurityConfig for the MCP filter only, so the context has no global ReactiveAuthenticationManager bean and form-login resolves its default (UserDetailsService-based) manager as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…gent-UX fixes
Field report (ChatGPT building a ZIP-lookup app): the agent could not build an
"append each lookup to a table + Clear button" app, was misled into a rejected
patch_widgets call by our own docs, and the user got no link to the created app.
Store accumulation (closed vocabulary — agents still never author JS):
- wire_event verbs appendToStore { key, query, field?, fields? } and
clearStoreKey { key }, emitting storeValue('<key>', [].concat(
appsmith.store.<key> ?? [], <expr> ?? []), false). [].concat flattens
array-returning queries; persist=false keeps accumulation session-only (the
Appsmith store persists per-APP to localStorage, so persisted rows would leak
across users of a shared browser).
- fields projection ([{ as, path: ["places", 0, "state"] }]) reaches
space-keyed and array-indexed API values via JSON.stringify-escaped bracket
access, with binding-syntax ({{ }}, ${ }, backtick, U+2028/29) rejected
outright because Appsmith's {{ }} extraction is text-level.
- storeKey schema: ^[A-Za-z_][A-Za-z0-9_]*$, max 64, denylist of
Object.prototype own names + "prototype" (bare store[key]= assignment would
otherwise allow prototype corruption); reused for the unquoted `as` row key.
- Statement-list primary (2-5 statements, at most one run) so a Clear button
can clearStoreKey + reset the input in one click; a sibling appendToStore of
a query the same list runs is rejected (run is async — it would read the
previous response; belongs in onSuccess).
- Table binding { store: "<key>" } -> {{ appsmith.store.<key> ?? [] }} on the
build/edit/patch paths; the list widget keeps its query-only contract.
- Lint warning when a store-bound table's key is never written on the page.
- The exact originating ZIP-lookup app ships as a worked recipe
(appsmith://recipe/zip-lookup).
Auto-publish + URLs:
- build_application deploys the app it just created (brand-new: nothing
deployed to clobber; re-publish stays governed prepare/confirm with the git
refusal). Governed: recorded via gov.execute (operation auto_publish).
Always: structured appsmith_mcp_auto_publish telemetry on success and
failure. Publish failure degrades to a coarse-class warning, never fails the
create. Publish does not make the app public (workspace ACL applies).
- Result now carries applicationId, pages, editorUrl and viewerUrl — absolute
only from a trusted origin (APPSMITH_MCP_PUBLIC_ORIGIN env, validated, or
X-Forwarded-Proto/Host strictly validated + allowlist-gated at initialize),
root-relative on any doubt; omitted when slugs are missing.
- SERVER_INSTRUCTIONS/recipes teach publish-LAST: wire first, re-publish, hand
the viewer link at the end; on ungoverned deployments relay the editorUrl.
Agent-UX fixes (the reported "schema doesn't match" bug):
- The CRUD recipe and bindings guide told agents to patch a table's binding
with source:{query}, which the patch path rejects (source there is a
selected-row ref; the correct prop is tableData) — corrected everywhere, and
get_capabilities gained a patchSpec section documenting the patch vocabulary.
- patch_widgets and wire_event now expose their real zod schemas as the MCP
inputSchema instead of an opaque record; drift-tested.
Design: docs/plans/2026-07-15-mcp-m5-accumulation-and-deploy-design.md.
Council: design (architect, security, product) and code (security — 4 binding
conditions verified, adversarial refutation failed; architect — 5 conditions
verified; qa — independent suite run) all APPROVE WITH RISKS; non-blocking
follow-ups recorded in project risks.
Tests: 595 passed / 4 pre-existing skips (71 new across events, schema,
editPatch, lint, gates, app, instructions). tsc, ESLint (both paths),
Prettier clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
The run_action tool allows execution of stored actions without a confirmation step if the action is classified as read-only by isReadOnlyAction. This classification relies on two conditions: (1) the protocol-level method (actionConfiguration.httpMethod) is "GET" or "HEAD", and (2) the action is host-restricted (i.e., its pluginType is "DB"). However, the code does not verify that the action's plugin type actually supports or enforces HTTP methods. Because the Appsmith backend does not strip or validate the httpMethod field for DB plugin actions, an attacker with developer privileges (or a malicious collaborator) can set actionConfiguration.httpMethod to "GET" on a mutating database action (e.g., a SQL query containing DROP TABLE or DELETE). When this action is evaluated by the MCP server, isReadOnlyAction will incorrectly classify it as read-only, allowing the AI agent to execute arbitrary mutating database queries via the run_action tool without the mandatory confirmation gate (prepare_run_action / confirm_run_action).
Steps to Reproduce
- Create or update a database action (e.g., PostgreSQL query with body "DROP TABLE users;") on the Appsmith backend, but manually inject
"httpMethod": "GET"into itsactionConfigurationvia the Appsmith REST API. - Call the MCP
run_actiontool directly with the targetapplicationIdandactionId. - Observe that the query executes immediately and returns the result, completely bypassing the mandatory confirmation gate (
prepare_run_action/confirm_run_action).
Fix with AI
A security vulnerability was found by Hacktron.
File: app/client/packages/mcp/src/app.ts
Lines: 1026-1039
Severity: medium
Vulnerability: Confirmation Gate Bypass for Database Actions via httpMethod Spoofing
Description:
The `run_action` tool allows execution of stored actions without a confirmation step if the action is classified as read-only by `isReadOnlyAction`. This classification relies on two conditions: (1) the protocol-level method (`actionConfiguration.httpMethod`) is "GET" or "HEAD", and (2) the action is host-restricted (i.e., its `pluginType` is "DB"). However, the code does not verify that the action's plugin type actually supports or enforces HTTP methods. Because the Appsmith backend does not strip or validate the `httpMethod` field for DB plugin actions, an attacker with developer privileges (or a malicious collaborator) can set `actionConfiguration.httpMethod` to "GET" on a mutating database action (e.g., a SQL query containing `DROP TABLE` or `DELETE`). When this action is evaluated by the MCP server, `isReadOnlyAction` will incorrectly classify it as read-only, allowing the AI agent to execute arbitrary mutating database queries via the `run_action` tool without the mandatory confirmation gate (`prepare_run_action` / `confirm_run_action`).
Proof of Concept:
**Steps to Reproduce**
1. Create or update a database action (e.g., PostgreSQL query with body "DROP TABLE users;") on the Appsmith backend, but manually inject `"httpMethod": "GET"` into its `actionConfiguration` via the Appsmith REST API.
2. Call the MCP `run_action` tool directly with the target `applicationId` and `actionId`.
3. Observe that the query executes immediately and returns the result, completely bypassing the mandatory confirmation gate (`prepare_run_action` / `confirm_run_action`).
Affected Code:
function isReadOnlyAction(action: unknown): boolean {
const config = (
actionSource(action) as {
actionConfiguration?: { httpMethod?: unknown };
} | null
)?.actionConfiguration;
const method = config?.httpMethod;
const protocolReadOnly =
typeof method === "string" &&
["GET", "HEAD"].includes(method.toUpperCase());
return protocolReadOnly && !isExternalEgressAction(action);
}
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), !accepted_risk <reason>, or !fixed (resolved). Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/29501506816. |
|
Deploy-Preview-URL: https://ce-41980.dp.appsmith.com |
…s, patchSpec move doc
Three fixes from watching a real ChatGPT session drive the MCP:
- Dead sessions now answer 404, not 400. The Streamable HTTP spec requires 404
for a request whose Mcp-Session-Id the server no longer recognizes (expired,
evicted, restarted) — that status is the defined signal on which a compliant
client transparently re-initializes. Our 400 stranded ChatGPT mid-task: it
surfaced "tool calls returning 400s" instead of reconnecting, which with the
15-minute idle TTL and evict-oldest displacement made long conversations
reliably break. Requests with NO session id (other than initialize) keep the
spec's 400.
- Every layout mutation result (edit_page / patch_widgets / wire_event via
commitLayout) now carries a deployNote stating the change landed in the EDIT
copy only and how to make it live — prepare_publish/confirm_publish when
governance is available, otherwise "open the app in the editor and click
Deploy". Agents skim instructions once but read tool results every call, so
the publish-last guidance is now also in-band; users no longer get a "done!"
while the deployed app still shows the previous version.
- get_capabilities patchSpec misdocumented the move operation as
{ kind: 'move', name, placement } — the schema takes parent and/or
position { topRow, leftColumn }. A ChatGPT session followed the doc and
concluded the server "rejects valid patch operations". Corrected, with a
note that same-topRow positions widgets side by side.
Tests: 597 passed / 4 pre-existing skips in the package (dead-session
assertions moved to 404; new tests pin the 400-vs-404 split and both
deployNote wordings). Full app/client workspace suite also green (3629).
tsc, ESLint (package path), Prettier clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/29515519855. |
|
Deploy-Preview-URL: https://ce-41980.dp.appsmith.com |
… discipline
Field report: MCP-authored layouts could write widgets on top of each other
(the MCP writes DSL directly, bypassing the editor's reflow), and nothing
stopped modal-on-modal stacking. The lint detected overlap post-write; nothing
prevented it.
Occupancy + repair (prevent, not just detect):
- builder/occupancy.ts: per-canvas occupancy model sharing the lint's intersect
predicate; deterministic pushdown (nearestFreePosition) with per-canvas
column counts; numeric hygiene (isNumber reads, safe-int + MAX_ROW clamped
writes — fixes moveToPosition's unguarded span math); detached overlays
(modals) excluded from occupancy and pair sets; mobile row mirrors synced.
- patch move: colliding positions are REPAIRED to the nearest free spot
(requestedPosition + applied position recorded, top-level note) or rejected
with colliders + nearest free position under strict: true. Reparenting lands
occupancy-aware and always records the position.
- NEW resize op { kind: "resize", name, rows?, columns?, strict? }: grid
units; growth cascade-pushes below-siblings; shrinking a container below its
children rejects with the executable minimum; MODAL rows translate to the px
height prop.
- Delta overlap gate in commitLayout (edit_page/patch_widgets/wire_event):
a mutation that INTRODUCES an overlapping sibling pair is rejected
(overlap_introduced + truncated pairs + ready-to-apply suggestedFix);
pre-existing (human-made) overlaps stay editable warnings.
- Container-fit cascade: CONTAINER/FORM/TABS auto-grow, pushing below-siblings
recursively up the ancestor chain; modal-body overflow warns (runtime
scrolls) with an executable resize fix.
- Overlap/clipped lint warnings now carry suggestedFix payloads (sequential,
simulated-occupancy, schema-valid) — inspect_page hands agents literal
repair operations for existing messes too.
Modal discipline:
- Structural: a modal can never nest inside another modal's subtree — rejected
on build, edit, and patch move (delta-aware: pre-existing hand-authored
nesting stays editable).
- Event graph (wire_event + lint): close-aware showModal analysis — an action
that also closes its host modal is a wizard/back-nav TRANSITION, never a
stack. Stacking depth 2 warns (confirm-over-edit stays legal); depth 3+ and
stacking cycles reject with the chain. New edges derive structurally from
the parsed action (emitter<->parser coupling test); pre-existing DSL is
scanned fail-open with linear regexes over trigger props only (50KB cap),
and the excluded-binding count is surfaced per page.
- Hardenings from the code council: single quotes rejected in appendToStore
path elements (closes a closesHost-poisoning vector), DFS depth/chain caps
against dense hostile graphs, trigger-key dedupe, MAX_ROW clamps on lint fix
rows and root-canvas growth.
Design: docs/plans/2026-07-16-mcp-m6-canvas-awareness-design.md. Councils:
design (architect, security, product) and code (security, architect, qa) —
all APPROVE WITH RISKS; every pre-commit condition applied, incl. four pinning
tests (build/edit nested-modal glue, the commitLayout gate rejection envelope
tripped end-to-end via a MAX_ROW-saturated canvas, closesHost coupling
round-trip). Follow-ups tracked in project risks.
Tests: 670 passed / 4 pre-existing skips (+73). tsc, ESLint (both paths),
Prettier clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/29535306843. |
|
Deploy-Preview-URL: https://ce-41980.dp.appsmith.com |
…tation-confirmed commits
The MCP could edit a git-connected app but never finish the job: publish
refuses git apps, nothing could commit, and agents left dirty shared branches.
Ground truth verified against the server source: Appsmith's commit API ALWAYS
pushes (no push flag exists), branch creation pushes the new ref immediately,
and each branch is its own application document (no server-side checkout —
humans' editor views are never affected by agent branch work).
read_git_status + the branch gate (BREAKING for agents editing git apps):
- read_git_status (always-on): whitelist projection — connection, branch,
clean/dirty with modified-entity COUNTS, protected branches, remote HOST
only; never gitAuth/keys/full URL. compareRemote defaults false and is
always sent explicitly (the server default is true).
- Every mutating tool targeting an application now takes `branch`; on a
git-connected app it is REQUIRED and must equal the app's branch
(git_branch_required / git_branch_changed carry the current branch — one
retry recovers). The gate FAILS CLOSED (git_state_unknown) where the old
advisory warning failed open. Confirm-tools gate BEFORE consuming their
one-time token. Includes confirm_rollback (a rollback re-applies a layout).
Per-session gate cache never caches errors; not-connected only for 30s.
create_branch (governed): reserved mcp/ namespace (byte-exact prefix,
[A-Za-z0-9_-]{1,60}, bare "mcp" rejected), at most 5 mcp/ branches per app,
audited; PUSHES the new ref under the instance deploy key (documented, remote
CI note); returns the NEW branched applicationId — subsequent work targets it.
prepare_commit / confirm_commit (governed): because commit implies push, the
MCP only ever commits on mcp/ branches — verified by a fresh, fail-closed read
at prepare AND at confirm (never the gate cache). One-time confirmation binds
app + branch + message + content revision; drift refuses, including a
post-approval re-check. Messages: single-line printable (C0/DEL and Unicode
bidi controls rejected), <=200 chars, non-strippable "[mcp] " marker (leading
"[" rejected); author/committer never cross the wire; isAmendCommit pinned
false. INVALID_GIT_CONFIGURATION maps to "set your git author profile".
Result carries the handoff: branch, branch-scoped editor URL, merge/cleanup
steps. 120s budget for the commit call.
Elicitation (MCP spec 2025-06-18): when the client declares the capability,
confirm_commit prompts the HUMAN directly ("Commit ALL current changes on
branch mcp/<x> ... and PUSH?"); only an explicit accept proceeds; decline/
cancel/timeout never consume the token; at most 3 prompts per confirmation,
then it is invalidated. App names/branches/messages are sanitized (bidi/
control/quote) before interpolation into the prompt. Elicitation is UX on top
of the token flow — no server rule relaxes when it is declared; on
non-elicitation clients prepare returns explicit relay text (documented as
advisory). Progress notifications keep timeout-resetting clients alive while
the human deliberates.
Design: docs/plans/2026-07-16-mcp-m7-git-awareness-design.md (rev 2).
Councils: design (product, architect APPROVE WITH RISKS; security BLOCKED →
APPROVE WITH RISKS after the commit-implies-push correction) and code
(security — all reserved checkpoints + 4 binding conditions verified, no
adversarial construction succeeded; architect — 57-tool gate matrix, found
and fixed the confirm_rollback gap; qa — 10/10 clean full runs, root-caused
and de-flaked the elicitation-timeout test). Follow-ups tracked in project
risks (parameterized gate-matrix test, cap-5 race, per-session elicitation
bound, progress-pinger test).
Tests: 740 passed / 4 pre-existing skips (+70: git.test.ts 31, git-commit
36 incl. real-SDK-client elicitation over InMemoryTransport, drift tests).
tsc, ESLint (0 errors), Prettier clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… hardening
Builds on M7's commit-elicitation machinery, generalizing it and closing a
batch of council follow-ups.
Elicitation for every destructive operation:
- Extract confirm_commit's bespoke elicitation into ONE shared layer
(elicitDestructiveApproval + sendElicitationPrompt + a single session-local
attempts map and budget) and apply it to confirm_delete_page,
confirm_delete_action, confirm_delete_js_object, confirm_publish,
confirm_rollback, confirm_run_action. When the client declares the
elicitation capability the HUMAN is prompted directly with honest,
digest-granular scope ("Delete the page and EVERYTHING on it?", "Deploy to
everyone with access?"); only an explicit accept proceeds. Every rule is
unchanged from the token flow — decline/cancel/timeout never consume the
token, at most 3 prompts per confirmation then invalidation, all
interpolations pass through promptSafe. Non-elicitation clients keep the
prepare-relay-text posture (now added to the six prepare_* twins).
- Pre-prompt ownership peek (security F1): a forged/expired/cross-session
confirmationId is refused (confirmation_invalid) via a non-consuming
peekConfirmation + confirmationBelongsTo(actorId) BEFORE any prompt, so it
can never raise a spoofed approval dialog or burn the session budget —
matching confirm_commit's pre-check.
- Session-total elicitation budget (20) so a prepare→3-prompts→re-prepare loop
cannot fatigue the human indefinitely.
Field-hardening bundle:
- get_guide tool: serves the instruction guides/recipes to tools-only clients
(ChatGPT can't read MCP resources) from the single INSTRUCTION_DOCS registry.
- Build/version stamp on /health and get_capabilities (package version +
optional APPSMITH_MCP_BUILD_TIME) so "which build is this instance" is
answerable.
- create_branch cap-5 race fix: the refs list is re-read inside the governance
mutate under the entity lock, so concurrent creates cannot overshoot.
- Injectable elicitation progress interval; APPSMITH_MCP_PUBLIC_ORIGIN added to
.env.example.
- Tests: M5 fast-follow (governed publish-failure warning, publishFailureClass
classes, bare import response, one continuous build->wire->publish pipeline)
and the M7 gate matrix over all 10 pattern-copied gate call sites.
Council: security, architect, qa — all APPROVE WITH RISKS; F1 (flagged by
security AND architect) and the architect's timeout-precedence test applied
pre-commit. Follow-ups tracked in project risks. Two suite flakes root-caused
and fixed (elicitation-timeout wall-clock answer; gate-matrix per-row session
churn) — 6 consecutive clean full runs.
Tests: 783 passed / 4 pre-existing skips. tsc, ESLint (both paths, 0 errors),
Prettier clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…o from app.ts app.ts had grown past 6,200 lines. Pure code-movement of closure-free helpers into three cohesive modules; behavior-identical (all 783 tests pass with ZERO test-file edits): - src/git/projections.ts — GitState/GitMetadataProjection, gitStateOf, remoteHostOf, gitMetadataOf, gitEntityCount, projectGitStatus, gitBranchNames, fingerprintBranchList, GitGateCache (+ its TTL constant), gitEditWarning. - src/prompt/hygiene.ts — commit-message hygiene (commitMessageProblem + the marker/max constants; the U+202x bidi and template-syntax rejection regex is moved byte-identically), promptSafe, truncateForPrompt. - src/buildInfo.ts — MCP_BUILD_INFO and the package.json version read. app.ts imports these and re-exports the previously-public names (GitGateCache, GIT_GATE_NOT_CONNECTED_TTL_MS, MCP_COMMIT_MARKER, MCP_COMMIT_MESSAGE_MAX, commitMessageProblem, MCP_BUILD_INFO, GitState) so every `from "./app.js"` import keeps resolving. app.ts: 6236 -> 5938 lines. Deliberately NOT extracted this round: buildMcpServer and all tool registrations / confirm_* handlers / the elicitation layer — they close over server-scope locals and would need a dependency-injected factory (and would churn tests), which is out of scope for a maintainability movement. Datasource and action projections are the next clean targets for a future round. Architect review: APPROVE (regex byte-identity verified with od -c; no import cycle; re-exports complete). Security/QA re-review skipped as proportionate — no logic, signature, or surface change; the 783-test pass with zero test edits is the regression gate. tsc, ESLint (both paths, 0 errors), Prettier, and the esbuild bundle all clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
/ok-to-test tags="@tag.All" |
|
Whoops! Looks like you're using an outdated method of running the Cypress suite. |
…_server # Conflicts: # app/server/appsmith-server/src/main/java/com/appsmith/server/constants/RateLimitConstants.java # app/server/appsmith-server/src/main/java/com/appsmith/server/ratelimiting/RateLimitConfig.java
…outId, route contract
First real end-to-end run against a live Appsmith server surfaced three
integration bugs the entire unit suite missed because the affected API calls
are mocked. A pure mock cannot catch a wrapper that encodes a wrong route.
F1 — getApplication hit a dead route (GET /api/v1/applications/{id} → 405).
ApplicationControllerCE has no @GetMapping("/{id}"), so getApplication threw on
every call. The branch gate (gateGitState) is fail-CLOSED, so this made it
REFUSE every mutation with git_state_unknown — the entire editing surface was
broken against the real server. Redefined the single getApplication wrapper to
source the `.application` sub-object from the working pages DTO
(GET /api/v1/pages?applicationId=...&mode=EDIT, already a 200). This auto-fixes
all five consumers; the fail-open (advisory) vs fail-closed (gate/commit/
publish) split is preserved by construction — no consumer edited. The wrapper
returns `.application` (not the whole DTO, which would drop gitApplicationMetadata
and silently disable the gate) and never passes raw git metadata to the agent —
read_git_status stays a host-only projection.
F2 — build_application now returns editorUrl/viewerUrl by sourcing page slugs
from the pages DTO (reuses applicationUrlsFromPages + the governed path's
existing getApplicationPages read; extra read only on the ungoverned branch).
F3 — layoutId is now surfaced (new getPage wrapper → layouts[0].id) in
build_application (default page) and read_pages (per page), so the authoring
tools are usable after a fresh build. read_pages' per-page fan-out is bounded to
a small concurrency pool so a many-page app degrades to a few waves, not an
N-wide burst of full-DSL reads.
F5 — new route-contract test (routeContract.test.ts): instruments the real
createAppsmithApi with a recording fetch, captures every wrapper's (verb, path),
and asserts each is served by a real Spring controller route (parsed from the
Java source). Catches the whole M9 bug class. It immediately found a SECOND
405: updateActionCollection used PUT against the PATCH-only /{id} route
(edit_js_object would have failed live) — fixed to PATCH, matching both the
server route and the existing request builder.
Hardening from the code council:
- The fail-closed gate now treats a 2xx with no application object as
unreadable → git_state_unknown (shared unreadableGitState), closing the last
residual F1 edge where a malformed 200 could read a git app as not-connected.
Verified: check-types clean, ESLint 0 errors, 796 unit tests green (3/3 runs),
and a live end-to-end harness run against the real server 16/16 (patch_widgets
applies, read_git_status → connected:false, build returns an editorUrl,
read_pages layoutId accepted by read_semantic_page).
Council: senior-architect + security-reviewer + qa-engineer + performance-engineer
→ APPROVE WITH RISKS (security clean APPROVE). Tracked follow-ups (non-blocking):
a pre-existing supertest socket-race flake in app.test.ts's M2 block; a
server-side lightweight layoutId route to drop the getPage over-fetch; and the
MCP-auth rate-limit shared-bucket hardening (F4, Java-side).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019hPCEAQ998veVY29Mhac2s
The GitNexus code-intelligence index is a local, machine-specific artifact that should not be tracked. Ignore it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019hPCEAQ998veVY29Mhac2s
There was a problem hiding this comment.
The newly introduced Model Context Protocol (MCP) governance store and coordinator implement instance-wide audit logging and change tracking. However, McpChangeRecord does not contain any organization or tenant identifier (e.g., organizationId).
In a multi-tenant Appsmith Enterprise Edition (EE) deployment, multiple organizations (tenants) share the same database and backend. In Appsmith, isSuperUser is defined as any user who has AclPermission.MANAGE_ORGANIZATION on their own organization. Consequently, in a multi-tenant EE deployment, every tenant-level administrator is treated as a super-user (isSuperUser: true) by the backend.
When a tenant-level administrator connects to the MCP server, they are granted isAdmin privileges. The listAllChanges and getAnyChange methods in MongoRedisGovernanceStore retrieve records globally from the mcp_changes collection without filtering by the caller's organization. As a result, an administrator of one organization can view the complete change history, query details, and layout modifications of all other organizations on the entire instance, breaking the tenant isolation boundary of Appsmith EE.
Steps to Reproduce
- Configure a multi-tenant Appsmith Enterprise Edition (EE) instance.
- Create two separate tenants/organizations (e.g., Tenant A and Tenant B).
- Create an administrator user for Tenant A.
- Enable the MCP server and governance tools.
- Authenticate as the Tenant A administrator and verify that
/api/v1/users/mereturnsisSuperUser: true(since they have MANAGE_ORGANIZATION permissions on Tenant A). - Invoke the MCP server's
list_all_changesorget_any_changetool. - Observe that the returned list contains change records, query details, and metadata belonging to Tenant B and other tenants on the instance.
Fix with AI
A security vulnerability was found by Hacktron.
File: app/client/packages/mcp/src/governance/store.ts
Lines: 171-180
Severity: medium
Vulnerability: Cross-Tenant Information Disclosure / Tenant Isolation Bypass in MCP Governance Audit Tools
Description:
The newly introduced Model Context Protocol (MCP) governance store and coordinator implement instance-wide audit logging and change tracking. However, `McpChangeRecord` does not contain any organization or tenant identifier (e.g., `organizationId`).
In a multi-tenant Appsmith Enterprise Edition (EE) deployment, multiple organizations (tenants) share the same database and backend. In Appsmith, `isSuperUser` is defined as any user who has `AclPermission.MANAGE_ORGANIZATION` on their own organization. Consequently, in a multi-tenant EE deployment, every tenant-level administrator is treated as a super-user (`isSuperUser: true`) by the backend.
When a tenant-level administrator connects to the MCP server, they are granted `isAdmin` privileges. The `listAllChanges` and `getAnyChange` methods in `MongoRedisGovernanceStore` retrieve records globally from the `mcp_changes` collection without filtering by the caller's organization. As a result, an administrator of one organization can view the complete change history, query details, and layout modifications of all other organizations on the entire instance, breaking the tenant isolation boundary of Appsmith EE.
Proof of Concept:
**Steps to Reproduce**
1. Configure a multi-tenant Appsmith Enterprise Edition (EE) instance.
2. Create two separate tenants/organizations (e.g., Tenant A and Tenant B).
3. Create an administrator user for Tenant A.
4. Enable the MCP server and governance tools.
5. Authenticate as the Tenant A administrator and verify that `/api/v1/users/me` returns `isSuperUser: true` (since they have MANAGE_ORGANIZATION permissions on Tenant A).
6. Invoke the MCP server's `list_all_changes` or `get_any_change` tool.
7. Observe that the returned list contains change records, query details, and metadata belonging to Tenant B and other tenants on the instance.
Affected Code:
async getAnyChange(id: string): Promise<McpChangeRecord | undefined> {
return (await this.changes.findOne({ id })) ?? undefined;
}
async listAllChanges(limit: number): Promise<McpChangeRecord[]> {
// No actorId filter — the whole mcp_changes collection for this single-org INSTANCE, newest-first. Records carry
// no org field, so a multi-org (EE) deployment must add an organizationId predicate before enabling the admin
// tool, or one org's super-admin would see every org's change history (tracked EE follow-up).
return this.changes.find({}).sort({ createdAt: -1 }).limit(limit).toArray();
}
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), !accepted_risk <reason>, or !fixed (resolved). Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/29608390077. |
|
Deploy-Preview-URL: https://ce-41980.dp.appsmith.com |
… confirms Field feedback: claude.ai declares the elicitation capability but users never see the approval dialog, and every failure surfaced as 'the user did not approve' — misleading the agent and making broken clients undiagnosable. Non-accept results now carry a reason (declined | cancelled | timeout | accepted_without_confirm | client_error) with per-reason error copy that never blames the user for a client-side failure, plus a sanitized client-reported detail string (control characters flattened, 300-char cap, untrusted-marked) on client errors. Approval semantics, attempt counters, session budget, and token lifecycle are unchanged; reason is advisory diagnostics only. Council-reviewed (architect, security, QA, DX): security re-verified the detail sanitization and approval posture; all findings resolved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N19EppQVZ55hwByVy4HEX8
Milestone A of the approval-flow feedback work: - client_error outcomes refund the prompt charges and degrade the session to the documented relay posture, so a client that declares elicitation but cannot render it no longer dead-ends every destructive operation - an explicit accept now counts as approval even when the client UI omits the optional confirm boolean (bare accept/decline UIs); confirm=false still refuses, malformed values still classify as non-accept - prompt copy now frames accepting as approval (consent-frame condition from security review), centralized for all seven confirm tools - per-outcome telemetry (tool, outcome, client name/version) via an injectable logSink, default stderr - operator knobs: APPSMITH_MCP_DISABLE_ELICITATION forces the relay posture; APPSMITH_MCP_ELICITATION_TIMEOUT_MS overrides the wait (parsed in gates.ts, warn-and-clamp at a 10-minute ceiling) - README documents the reason vocabulary, degradation, and knobs, pinned by docs-drift tests Council-reviewed (architect, security, QA): all conditions applied and re-verified by security; strict-elicitation mode tracked as follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N19EppQVZ55hwByVy4HEX8
The scalar sibling of the table query binding — 'show the temperature
from this API in a label' no longer requires a table workaround (the
field feedback that motivated this: a day-of-week readout was built as
a REST query + table + button because text had no query path).
- queryFieldRefSchema ({query, field?}) + compileQueryFieldBinding
emitting {{ Q.data?.field ?? "" }} — same closed-vocabulary and
charset discipline as every other emitter
- text source widens to selected-row | query-field; image gains source
(build + patch paths, mutual exclusion with literals)
- read_semantic_page reports the new shape (and now also reads back the
image prop, closing a pre-existing gap)
- responseFieldPath tightened to dot-separated identifiers (a trailing
or doubled dot compiled to a JS syntax error) per security review
- bindings guide teaches scalar readouts; the old table bullet is
requalified so the guide no longer contradicts itself
Council-reviewed (security, architect, QA): unanimous approve-with-risks;
all requested fixes and tests applied.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N19EppQVZ55hwByVy4HEX8
…er events Closes the lifecycle-event gap from the capability audit: onOptionChange, onSubmit, onCheckChange, onChange, and onDateSelected join the closed event vocabulary (EVENTS_BY_TYPE + enum), riding the existing generic trigger mechanism unchanged. Widget-type/event pairs verified against the canonical widget sources; cross-type misuse still rejects. Page-load execution needs no event: the server derives on-page-load actions from widget bindings at every layout save, so query-bound widgets already run on load — now stated in the tool copy. Council-reviewed (security APPROVE, architect APPROVE, QA approve-with- risks — closed-enum pin and select end-to-end test added per QA). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N19EppQVZ55hwByVy4HEX8
'Show me the day of the week' is now one widget with zero queries:
text accepts a computed 'value' token from a closed vocabulary —
- { now: { format } }: the agent picks a NAMED preset (dayOfWeek, date,
dateShort, time, dateTime, isoDate, monthYear); the compiler owns the
moment format literal, so no free-form format string is ever emitted
- { count: { query, field? } }: row count of a query's response
- { concat: [...] }: literals (JSON-escaped at compile time — fuzz-
verified inert by security review across 78k hostile inputs),
selected-row refs, and query-field refs joined into one string
Build + patch paths with exactly-one-of text/source/value; concat
selected-row parts get the dangling-table guard on the patch path;
read_semantic_page reports now/count (reverse-map gated so hand-
authored moment formats stay hidden) while concat stays hidden by the
no-raw-bindings default, which the guide now states explicitly.
Council-reviewed (security, architect, QA): unanimous approve-with-
risks; every requested guard, test, and doc landed — including a
table-driven compile+read-back round-trip over all named formats.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N19EppQVZ55hwByVy4HEX8
…ored guidance
Three council-reviewed additions closing the remaining capability-audit
and security-follow-up items:
- visibleWhen gains two predicate forms alongside { control, equals }:
{ rowSelected: table } -> {{ T.selectedRowIndex !== -1 }} (show detail
panels/action buttons only with a selection) and { notEmpty: input }
-> {{ !!I.text }}; both guarded against dangling/wrong-type targets,
both read back semantically with isVisible-only scoping (pinned)
- APPSMITH_MCP_STRICT_ELICITATION: in-band approval prompts REQUIRED —
non-elicitation clients refuse with elicitation_required, client_error
never degrades to the relay posture, strict wins over the disable knob
(loud startup warning on the contradictory pair), and refunded client
errors are capped at 3 per session before prompt attempts stop
(security Risk 1 from this review, fixed in-session)
- the bindings guide now warns that read-back hides hand-written
property bindings on ANY human-built page (and that no count reports
them — inspect_page counts trigger bindings only), so agents confirm
before overwriting props on hand-authored apps
Council (security, architect, QA): unanimous approve-with-risks; every
requested fix, test, and doc landed, including the strict happy-path
test and catalog/tool-copy advertising of the new predicates.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N19EppQVZ55hwByVy4HEX8
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/29684205123. |
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
1 participant
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.
What & why
Adds an opt-out, same-instance Streamable HTTP MCP server that lets an MCP client (e.g. an AI agent) act as a specific Appsmith user. The client authenticates with a user-scoped bearer token; the Node service forwards that token to the existing
/api/v1endpoints, so Spring Security reconstructs the real user and the existing workspace/app/page ACLs authorize every operation. No privileged or instance-wide credential is used.Resolves #15383.
How it works
Server (CE, EE-overridable)
UserMcpTokendomain + repository + service +McpTokenControllerfor create / list / revoke of user-scoped tokens. Every layer follows the CE-base + thin concrete-subclass split (*CE/*CEImpl) so EE can override.AuthenticationWebFilter(only engages for themcp_prefix) reconstructs the token owner. Invalid / revoked / disabled-user tokens return 401.Migration076creates theuserMcpTokenindexes (the instance runs withauto-index-creation=false, so@Indexedalone is inert).Node service (
app/client/packages/mcp)/healthendpoint, request-body size cap, per-request token revalidation, per-session token binding (constant-time compare), and per-user + global session caps.list_workspaces,list_applications,get_application_context, andimport_application_artifact/import_partial_application_artifact— writes go through the existing validated import / partial-import APIs (no raw DSL/Mongo writes).Client
Rollout / deploy
APPSMITH_MCP_ENABLED=1gates both the supervisord autostart and the Caddy/mcproute; a disabled instance never starts the Node process and returns 404 for/mcp. Existing instances are unaffected until an admin opts in and a user deliberately issues + uses a token.mcp-buildCI workflow, and a/mcp/healthroute test.Testing
mcp_/ anonymous → 401), session binding & revalidation, TTL/expiry, per-user (429) and global (503) caps, artifact validation, malformed/oversized body handling. All pass.check-types, ESLint (0 errors), and the MCP package typecheck locally.Security review
Reviewed by a multi-agent council (architecture, security, QA, data-migration, DX, UX, product). Key hardening landed from that review: 401 (not 500) on bad tokens, the CE/EE split, the index migration, the
mcp_-prefix + non-anonymous/megate (closes an unauthenticated-session DoS), per-user session cap, and the opt-in deploy gate.Follow-ups (tracked, not blocking an opt-in ship)
🤖 Generated with Claude Code
Automation
/ok-to-test tags="@tag.All"
Summary by CodeRabbit
Summary
/mcpand/mcp/healthrouting, included in Docker images/packaging workflows.Warning
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/29670092123
Commit: abe9735
Cypress dashboard.
Tags: @tag.All
Spec:
It seems like no tests ran 😔. We are not able to recognize it, please check workflow here.
Sun, 19 Jul 2026 02:31:10 UTC