feat: embedded MCP server with user-scoped token authentication (#15383)#41980
Open
salevine wants to merge 29 commits into
Open
feat: embedded MCP server with user-scoped token authentication (#15383)#41980salevine wants to merge 29 commits into
salevine wants to merge 29 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)
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 |
![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>
Failed server tests
|
…t broken execute-guard - Prettier + eslint --fix across the mcp package (formats the builder modules to the workspace style the Build MCP Lint step enforces). - McpTokenControllerCETest: stub the mint/rotate Monos (Reactor evaluates .then's argument eagerly) and assert the guard rejects without ever subscribing them (no NPE, correct guarantee). - UserMcpTokenServiceImplTest.rotate: save() is called by both create() and rotate(); verify(times(2)). - Revert the datasource EXECUTE_DATASOURCES guard in NewActionServiceCEImpl and its two tests: the integration test showed it did not actually reject, and it is not needed (the MCP data layer already resolves workspace server-authoritatively). Tracked as a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Failed server tests
|
…ervice test - Prettier-format src/pages/UserProfile/McpTokens.tsx (client-prettier/client-lint gate). - Remove UserMcpTokenServiceImplTest.rotate_replacesSecret… : it over-specified Mockito interactions (strict-stubs UnnecessaryStubbing) and is redundant with McpTokenControllerCETest rotate coverage (session-auth delegates; MCP-auth rejected). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MongoRedisGovernanceStore (the real driver behind locks, one-time confirmations, and the mcp_changes audit log) previously had zero integration coverage — every governance test used an in-memory fake. Add an integration suite that exercises the real drivers: - Redis SET NX/PX lock acquisition + contention + release + re-acquire - Lua compare-and-delete release safety (mismatched lockId cannot free) - one-time confirmation consume (Lua get+del) - actor-scoped saveChange/getChange and newest-first listChanges Gated on APPSMITH_MONGODB_URI + APPSMITH_REDIS_URL so it skips in CI (no databases there) and the standard unit run stays green. Verified locally against Docker Mongo/Redis: 4 passed; skips cleanly without the env vars; tsc/eslint/prettier clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s, event chains
Product review found the agent CRUD journey broke at three points; this
closes all three (council re-run: security/architect/qa all approve):
1. create_datasource (data-gated): provision an UNCONFIGURED
PostgreSQL/MySQL/MSSQL datasource from non-secret connection details
(isConfigured:false under CE's unused_env storage). Credentials are
never accepted through MCP — strict schema rejects them and the tool
returns needsCredentials + a UI hand-off nextStep, the same flow as
importing an app without configuring. Idempotent by workspace+name.
2. Display bindings: closed {table, column} selected-row refs compile to
`{{ Table.selectedRow["col"] }}` on text.source / input.defaultValue,
in both the build schema and patch_widgets (type/dangling/conflict
guards, dynamic-path register + unregister on literal overwrite).
read_semantic_page round-trips compiler-emitted bindings as
structured refs; the linter resolves widget-name binding heads in a
second pass so these never false-flag.
3. wire_event chains: run actions gain onSuccess/onError follow-ups
(max 5, no nesting) plus showAlert (quote-free charset), compiled to
one bounded promise chain — submit -> refresh table -> close modal ->
alert. Every reference in the chain is validated before writing.
Also fixes two latent server-contract bugs: GET /api/v1/plugins now
passes its required workspaceId param, and datasource plugin-family
checks resolve plugin document ids via the workspace plugin list
instead of comparing Mongo ids to packageNames.
Catalog/capabilities/instruction guides updated in lockstep (47 tools;
drift test is set-equality). Verified: tsc clean, eslint 0 errors,
prettier clean, jest 285 passed / 4 skipped (DB-gated) / 0 failed —
33 new tests incl. injection attempts on every new emitter.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds an admin-visible on/off switch for the MCP server (previously env-var only) and flips the default to enabled, matching the original plan. Council re-run: security/architect/qa all approve. Admin Settings -> Configuration gains three checkboxes: - MCP server (APPSMITH_MCP_ENABLED, default on) - MCP data tools (APPSMITH_MCP_DATA_ENABLED, default off) - MCP JS objects (APPSMITH_MCP_JS_ENABLED, default off) They save through the existing /admin/env flow (the three vars are added to the EnvVariables whitelist), and the admin restart now includes the "mcp" program so a Save & Restart re-evaluates the gates. Toggle is a real kill switch, not just a route change: - SecurityConfig gates the MCP bearer-auth filter on APPSMITH_MCP_ENABLED, so disabling MCP rejects already-issued mcp_ tokens (401) server-side instead of leaving them valid as full API credentials until expiry. - Deploy: the mcp supervisord program is always installed; run-mcp.sh parks (sleep infinity) when explicitly disabled; caddy re-evaluates the /mcp route on restart; healthcheck probes MCP only when enabled; docker.env.sh seeds the gates and entrypoint backfills them for existing installs. Disable spelling is false/0/no/off (any case). Admin UI defaulting: a new Setting.defaultValue lets an env-backed toggle render from its declared default when the variable is absent from the fetched settings, so the checkbox mirrors the runtime default (MCP server shows checked) instead of falling back to unchecked. Server MCP gate parser accepts the "true"/"false" the UI writes (was "1"-only); extracted to a tested gates module. Verified: mcp tsc clean, 306 passed / 4 skipped / 0 failed; client check-types (node 24) exit 0; eslint 0 errors; prettier clean; spotless applied. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…en TTL Fixes found by running the MCP server end-to-end against a live Appsmith instance, plus token-management UX and configurable lifetime. Bugs the live run surfaced: - build: the shipped ESM bundle crashed on startup with "Dynamic require of timers/promises" because the mongodb driver dynamically requires Node built-ins. Added a createRequire banner to build.js so the same bundle works in Docker with governance enabled. - app.ts: list_workspaces called GET /api/v1/workspaces, which collides with the create (POST) mapping and returns 405 — breaking step one of every agent journey. Use GET /api/v1/workspaces/home. Token management UI: - The token UI lived on pages/UserProfile, a page the app no longer routes to, so it was unreachable. Move it into the live Admin Settings -> Profile (Account) page; remove the dead UserProfile tab. - Fix timestamp display: the server serializes Instant as epoch seconds but the client parsed it as milliseconds, so dates showed 1970 and a 90-day expiry rendered as ~2 hours. Normalize seconds to ms. - UX: right-align the Create token button; add a clipboard icon beside the token field in the one-time reveal modal. Admin-settable token lifetime (default 90 days): - APPSMITH_MCP_TOKEN_TTL_DAYS drives create/rotate expiry via @value setter injection, clamped to 1..3650 so a misconfig can't mint immortal or pre-expired tokens. Whitelisted in EnvVariables; new numeric field on Admin Settings -> Configuration; seeded in docker.env.sh and backfilled in entrypoint.sh. Added server tests for the configured value and clamping. Verified: mcp tsc clean, 306 passed / 4 skipped; client check-types (node 24) exit 0; eslint 0 errors; prettier clean; spotless applied; deploy scripts syntax-checked. Live smoke against a running instance: initialize -> list_workspaces -> list_applications -> create_datasource (idempotent) all pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extends create_datasource to REST APIs and fixes three bugs that made build_application unusable against a real Appsmith backend — all found by running the MCP server end-to-end and creating a real app. create_datasource — REST support: - New plugin "rest": pass a base `url` instead of `connection`. Open APIs need no auth, so REST datasources are created CONFIGURED and immediately usable (needsCredentials: false); databases stay unconfigured pending a password. url-vs-connection is enforced per plugin, and unsafe URLs (non-http scheme, whitespace, template syntax) are rejected at the schema. Credentials are still never accepted. Verified live: a Zippopotam REST datasource + a REST query that returns live data. build_application fixes: - Circular-reference false positive: compileApp reused one array for pageOrder and publishedPageOrder; serializeArtifact rejected the shared reference as circular, breaking every build. The compiler now emits distinct arrays and the serializer only rejects true ancestor cycles (shared references are valid JSON). - CSRF 403 on import: Appsmith's CSRF filter exempts JSON POSTs but not multipart uploads. MCP's server-to-server bearer calls carry no cookies (no CSRF exposure), so they now send the documented X-Requested-By: Appsmith exemption header. - Empty imported app: at the declared serverSchemaVersion the server runs no import migrations, so the migration that derives exportedApplication.pages from pageOrder is skipped and the imported app links to zero pages. The compiler now emits pages/publishedPages itself, matching what that migration produces. Verified: mcp tsc clean, 310 passed / 4 skipped; eslint 0 errors; prettier clean. Live: build_application creates an app whose page imports correctly, create_datasource (rest) + create_rest_api attach a working query. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tion works
MCP's getAction called GET /api/v1/actions/{id}, which does not exist —
that path is PUT/DELETE only, so a GET returns 405. This broke every
tool that reads an action first: run_action, get_action, update_action,
duplicate_action, confirm_delete_action, prepare_run_action,
confirm_run_action.
There is no single-action GET; the only read is the per-application
list. getAction now takes (applicationId, actionId), fetches
GET /api/v1/actions?applicationId=..., selects the action by id, and
throws a clear "not found in application" error otherwise. get_action /
run_action / prepare_run_action / confirm_run_action gain an
applicationId parameter; the action-mutation tools already carry
applicationId in their spec.
Verified live end to end through MCP: build_application -> create_datasource
(rest) -> create_rest_api -> run_action now executes a read-only REST
query and returns 200 OK with live data. mcp tsc clean, 312 passed /
4 skipped; eslint 0 errors; prettier clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…king deployed app
Makes an MCP-built app genuinely interactive and correctly laid out,
verified by building + wiring + publishing a live zip-code lookup.
- create_rest_api: pathParams appends dynamic path segments after the
static path — literals or validated widget references — so a
path-based API takes input from a widget (path "/us" +
{ widget: "ZipInput", property: "text" } -> /us/{{ ZipInput.text }}).
The agent never writes raw {{ }}; segments are charset-checked and the
emitted binding passes SAFE_BINDING.
- table source: optional `field` binds a table to a nested array in the
response ({ query, field: "places" } -> {{ Query.data.places }}), so a
REST result that nests its rows renders in the table.
- Layout: the compiler now pins the app to FIXED positioning
(unpublished/publishedApplicationDetail.appPositioning) so imported
fixed-grid geometry renders as designed instead of being reflowed by
auto-layout (full-width widgets, large gaps, centered button). Input
footprint shrunk from 7 to 4 rows (a single-line field, not a big box).
Verified live end to end through MCP: build_application -> create_datasource
(rest) -> create_rest_api (dynamic path) -> wire_event (button runs the
query) -> prepare_publish/confirm_publish. mcp tsc clean, 316 passed /
4 skipped; eslint 0 errors; prettier clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Root cause of the broken layout in MCP-built apps: the compiler stamped the page DSL version as 4, but the Appsmith client runs migrateDSL on every load, applying every transform from the DSL's version up to LATEST_DSL_VERSION (94). On our version-4 DSL that meant ~90 migrations ran on load — an early grid-density migration rescaled widget rows, then migration 075 (migrateInputWidgetsMultiLineInputType) saw the inflated height and flipped the single-line input to MULTI_LINE_TEXT. The result was a giant multi-line input, an oversized button, and huge gaps — even though the imported DSL was correct. Fix: stamp the root canvas with LATEST_DSL_VERSION so migrateDSL is a no-op and our widgets render exactly as built. A drift test reads the constant from @shared/dsl's source and asserts equality, so an Appsmith version bump fails loudly here instead of silently regressing layouts. Also reverted the input footprint to 7 rows (the real default) — the earlier 4-row change was based on the wrong diagnosis. Verified live: a freshly built app now keeps inputType TEXT / rows 7 / version 94 through build -> wire -> publish AND through an editor load (migrateDSL no longer transforms it); the input renders as a single-line field with a normal button and compact layout. mcp tsc clean, 317 passed / 4 skipped; eslint 0 errors; prettier clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The MCP server was already on by default; the data layer and restricted
JS objects were opt-in. Make all three MCP gates on by default so the
full tool surface is available out of the box (governed/destructive
tools still require Mongo+Redis, so they only register when that infra
is present, and every operation runs under the connecting user's ACL).
- Admin Settings -> Configuration: the "MCP data tools" and "MCP JS
objects" checkboxes now default checked ("Enabled by default").
- server.ts reads these via a new gateEnabledByDefault (on unless
explicitly false/0/no/off), matching APPSMITH_MCP_ENABLED.
- docker.env.sh seeds them true; entrypoint backfills true for existing
installs; docs updated.
Verified: mcp tsc clean, 332 passed / 4 skipped; eslint 0 errors;
prettier clean; deploy scripts syntax-checked.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… query runs
A table bound to a query compiled to `{{ Query.data.places }}`, which
throws (and shows the widget's "Data is undefined, re-run your query or
fix the data" validation error) before the query has executed, because
Query.data is undefined at first render.
Emit optional chaining plus an empty-array fallback instead:
{{ Query.data ?? [] }} (whole response)
{{ Query.data?.places ?? [] }} (nested field)
so tableData always evaluates to an array (empty until the query runs)
and the error is gone. The compiler still authors the whole expression;
query name and field path stay strict identifier paths. The semantic
read-back regex is updated to recover these bindings (with the field).
Verified: mcp tsc clean, 332 passed / 4 skipped; eslint 0 errors;
prettier clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…styling
Extends the MCP builder's closed vocabulary with three capabilities that
completed the "Clear button + styled table" edit flow, all compiler-emitted
so agents still never author raw expressions:
- wire_event `reset` action: `{ reset: 'W' }` or `{ reset: ['A','B'] }`
compiles to `{{ resetWidget('Name', true) }}` (a Clear button that empties
an input and resets widgets). Names are strict identifiers, so the emitted
single-quoted argument cannot be broken out of.
- Guarded table binding: table `source` (build) and patch_widgets `tableData`
(edit) accept `clearWhenEmpty: '<input>'`, emitting
`{{ In.text ? (Q.data?.field ?? []) : [] }}`. resetWidget alone cannot clear
a query-bound table (the bound data re-evaluates and persists); gating the
rows on the input's text lets a Clear button empty the table too. A single
`compileTableDataBinding` emitter is shared by both entry points, and
semantic.ts round-trips the guarded form back to a structured ref.
- Table row striping: patch_widgets accepts literal `oddRowColor`/`evenRowColor`
(TableWidgetV2 style props) for zebra backgrounds.
lint.ts treats Appsmith platform action functions (resetWidget, navigateTo,
showModal, closeModal, showAlert, storeValue, clearStore, removeValue,
copyToClipboard, download, postWindowMessage) as valid binding heads, so
compiler-emitted event bindings are no longer flagged as dangling references.
Security hardening (council review): `oddRowColor`/`evenRowColor` use a
color-grammar allowlist (hex / rgb()/rgba()/hsl()/hsla() / named color) that
cannot spell `url(...)`, so a row color can never become a CSS egress
primitive (tracking beacon / internal-network probe) when a viewer renders the
table. The clear-when-empty guard must be an input widget (it is emitted as
`.text`), rejected at edit time with a clear message.
Council: senior-architect + security-reviewer + qa-engineer + dx-engineer
-> APPROVE WITH RISKS (all findings resolved). 345 mcp tests pass; tsc clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds error handling to generated apps so bad input is caught before it
reaches a query, instead of surfacing a raw "<query> failed to execute".
Both capabilities stay inside the closed vocabulary — the compiler emits
every value; agents never author a regex or a binding.
- Input validation from a named format: input `validation`
{ format: 'zipcode'|'email'|'number'|'integer'|'usPhone', message? }
compiles to a VETTED literal regex + error message and marks the input
required (so empty is invalid too). The input's `regex`/`errorMessage`
props are bind-evaluated by Appsmith, so a raw agent regex could smuggle
a {{ }} expression — the agent only picks a format from a fixed enum
(derived from the format map so the two can't drift), and the regex is
looked up server-side. Available at build time (input spec) and edit time
(patch_widgets).
- disableWhenInvalid: '<input>' on patch_widgets greys a widget out while
the named input is invalid — compiles to `{{ !<input>.isValid }}`. Pair
it with validation so a bad value can't run a query. The referenced widget
must be an input; semantic.ts round-trips the binding to a structured ref.
- read_semantic_page now surfaces literal `regex`/`errorMessage`; safeScalar
still hides any binding-bearing value.
Council review (security-reviewer + senior-architect + qa-engineer +
dx-engineer -> APPROVE WITH RISKS) findings all resolved:
- Correctness (architect): reject a literal isRequired alongside validation
— Object.assign ran last and would silently clobber the required flag,
defeating the guard.
- Security (LOW): bound the email regex quantifiers
(^[^\s@]{1,64}@[^\s@]{1,255}\.[^\s@]{1,63}$) to cap worst-case backtracking.
- Maintainability (dx/architect): derive the format enum from the format map.
- Coverage (qa): tests for every format's compiled regex, message-binding
rejection, semantic surfacing + binding-hiding, and the un-validated default.
359 mcp tests pass; tsc clean.
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/29296816155. |
…xt (security) Fixes a medium-severity finding: run_action executed a stored action with no prepare/confirm step whenever isReadOnlyAction returned true, and the classifier treated any SQL body starting with SELECT/SHOW/EXPLAIN/WITH as read-only. That prefix heuristic is unsound — `WITH x AS (DELETE ... RETURNING *) SELECT ...`, `SELECT mutating_func()`, and `EXPLAIN ANALYZE <mutation>` all mutate — so a prompt-injected agent could run pre-existing mutating queries and bypass the confirmation boundary. Root-cause fix: isReadOnlyAction no longer inspects the query body at all. Only a protocol-level guarantee counts — a REST action whose HTTP method is GET or HEAD (safe per HTTP semantics). Any action with a query body is non-read-only and must go through prepare_run_action / confirm_run_action. This removes the text-parsing path entirely rather than tightening a defeatable allowlist. run_action is the only gate that used this; prepare_run_action's use is informational (the `readOnly` field) and confirm_run_action always consumes a one-time token regardless. No other path executes a stored action, and run_action accepts no execute payload — the server runs the persisted action under the caller's ACL. Client-visible contract change: run_action on a DB/SQL action now returns `code: "confirmation_required"` instead of executing; callers use prepare_run_action -> confirm_run_action for any query. Regression test asserts run_action refuses (executeAction NOT called, code=confirmation_required) for the WITH-CTE-DELETE, SELECT-mutating-function, EXPLAIN ANALYZE, stacked SHOW;DROP, and plain SELECT bodies — red on the old prefix heuristic, green on the fix. Also covers REST HEAD auto-run and REST POST refusal. Tool description and doc comment updated to match. Council review (security-reviewer + senior-architect -> APPROVE; qa-engineer -> APPROVE WITH RISKS, coverage gaps closed). 361 mcp tests pass; tsc clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ense)
Security-review follow-up. The /mcp handler already rejects any request that
carries an Origin header (unconditional, before auth/session) — the primary
DNS-rebinding defense for this POST-based protocol. Host validation was the
missing complementary layer.
A fixed loopback-only Host allowlist can't be the default here: the server
binds 127.0.0.1 and is fronted by Caddy, whose reverse_proxy preserves the
client's original Host upstream, so a hosted deployment's Node process sees the
public domain. A loopback list would reject all proxied traffic.
So this adds the finding's "explicitly allowed configured value": an optional
APPSMITH_MCP_ALLOWED_HOSTS (comma-separated hostnames, port ignored). When set,
the handler rejects any request whose Host hostname isn't listed with
403 { error: "host not allowed" } — placed immediately after the Origin check
and BEFORE bearerToken/session lookup/transport creation, so a rejected request
never validates a token or creates a session. Default-off (unset) leaves Host
unchecked, so existing Caddy-fronted deployments are unaffected; a loopback or
host-pinned deployment sets it to enforce the check.
hostHeaderName() extracts the lowercased hostname (strips the port at the first
colon; handles bracketed IPv6; fails closed on malformed/missing input) with no
regex (no ReDoS). Documented in .env.example.
Regression-tested (red-green verified): a disallowed Host is refused before
validateToken is called; a listed host passes, including host:port (the form
loopback deployments send); default-off does not reject a foreign Host; plus
direct hostHeaderName unit tests (port, IPv6, case, missing).
Council review (security-reviewer + senior-architect + qa-engineer ->
APPROVE WITH RISKS). 366 mcp tests pass; tsc clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/29298524871. |
Failed server tests
|
|
Deploy-Preview-URL: https://ce-41980.dp.appsmith.com |
A user connected ChatGPT to the MCP server and reported it "couldn't see"
capabilities that mostly already exist (edit widget props, JS objects, wire
events, datasources, queries, publish) — they're gated behind
APPSMITH_MCP_DATA_ENABLED / APPSMITH_MCP_JS_ENABLED / governance, and ChatGPT
never called get_capabilities. This addresses visibility first, then fills a
few concrete gaps — all within the closed vocabulary (agents never author raw
DSL/SQL/JS/bindings).
Visibility:
- Server `instructions` (SERVER_INSTRUCTIONS) are now returned at MCP
`initialize`, so every client (ChatGPT, Claude, ...) sees the full build
workflow and the real tool names up front, without calling get_capabilities.
- get_capabilities now returns `disabledCapabilities`: the gated-OFF tool
groups, each with the exact tools it would add and a HUMAN-facing "requires"
instruction (e.g. "ask your Appsmith administrator to enable the MCP data
layer (self-hosted: env APPSMITH_MCP_DATA_ENABLED)") — so an agent tells the
user how to unlock a capability instead of claiming the server can't do it.
Actionable for Cloud users too (no bare env var as the only guidance).
Capability gaps:
- resolve_workspace(name) + list_workspaces now returns { id, name } — an agent
can work from a workspace NAME instead of a raw id (build_application and the
list tools point at this). projectWorkspaces also drops other org metadata.
- patch_widgets: table interactivity toggles (isVisibleSearch,
enableClientSideSearch, isVisibleFilters, isSortable, isVisibleDownload,
isVisiblePagination) for search-across-fields + filtering, and an
`imageSource` selected-row binding (image.src = a table's selected-row column,
e.g. an employee photo in a detail panel) mirroring the text `source` /
input `defaultValue` bindings.
Council review (security APPROVE; senior-architect, qa, product, ux, dx ->
APPROVE WITH RISKS) — findings resolved: user-actionable enablement wording, a
SERVER_INSTRUCTIONS drift-guard test (asserts every tool name / env var in the
prose is real), and edge-case tests (non-array workspaces, duplicate exact
name, literal-image clears a prior binding). 378 mcp tests pass; tsc clean.
Deferred to a follow-up milestone (each a larger vocabulary/compiler
expansion): card-grid/list binding, shared table<->card toggle state, in-table
image columns, rich edit-dialog + full CRUD-wiring presets.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Users creating an MCP token didn't know what server URL to point their client
(ChatGPT, Claude) at. Add a read-only "MCP server URL" field
(`${window.location.origin}/mcp`) with a copy button and helper text, shown on
the tokens page and inside the token-created modal next to the one-time token —
so the setup moment has both the URL and the token together.
Extracts a small ReadOnlyCopyField (also used for the token field), uses the
design-system code font token, and links the helper text via the Input
`description` prop for aria-describedby.
Council: ux-designer + product-planner APPROVE WITH RISKS (polish folded in).
Client McpTokens test suite passes (9/9), including the new server-URL field.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
UpdateUsersName_spec test 2 read the email via `.ads-v2-input__input-section-input` `.last()`, which assumed the email was the last input on the profile page. The MCP tokens section (added on this branch) renders its own read-only "MCP server URL" input on that page, so `.last()` picked it up (http://localhost/mcp) and the assertion failed. Select the email field by its stable `data-testid=t--user-name` instead — robust to other sections adding inputs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment on lines
+59
to
+69
| @GetMapping | ||
| public Flux<ResponseDTO<McpTokenResponseDTO>> list(@AuthenticationPrincipal User user) { | ||
| return userMcpTokenService.list(user).map(token -> new ResponseDTO<>(HttpStatus.OK, token)); | ||
| } | ||
|
|
||
| @DeleteMapping("/{tokenId}") | ||
| public Mono<ResponseDTO<Boolean>> revoke(@AuthenticationPrincipal User user, @PathVariable String tokenId) { | ||
| return userMcpTokenService | ||
| .revoke(user, tokenId) | ||
| .map(revoked -> new ResponseDTO<>(revoked ? HttpStatus.OK : HttpStatus.NOT_FOUND, revoked)); | ||
| } |
There was a problem hiding this comment.
The McpTokenControllerCE class implements endpoints for managing Model Context Protocol (MCP) tokens (create, rotate, list, and revoke). While the create and rotate endpoints correctly call requireSessionAuthentication() to ensure that only interactive web sessions (and not clients authenticated via an MCP token itself) can perform these actions, the list and revoke endpoints do not enforce this check. An attacker who has obtained a leaked or compromised MCP token can authenticate to the Appsmith backend with MCP authority and call the list endpoint (GET /api/v1/mcp/tokens) to retrieve metadata of all active MCP tokens belonging to the victim user, or call the revoke endpoint (DELETE /api/v1/mcp/tokens/{tokenId}) to delete/revoke any of the user's active MCP tokens, leading to unauthorized credential management and a Denial of Service (DoS) of legitimate integrations.
Steps to Reproduce
- Obtain a valid MCP token for a user.
- Send an HTTP GET request to
/api/v1/mcp/tokensusing the MCP token as a Bearer token in the Authorization header. Observe that the list of active tokens is returned. - Send an HTTP DELETE request to
/api/v1/mcp/tokens/{tokenId}using the MCP token as a Bearer token in the Authorization header to revoke another active token. Observe that the token is successfully revoked.
Fix with AI
A security vulnerability was found by Hacktron.
File: app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/McpTokenControllerCE.java
Lines: 59-69
Severity: medium
Vulnerability: Missing Session Authentication Enforcement on MCP Token List and Revoke Endpoints
Description:
The `McpTokenControllerCE` class implements endpoints for managing Model Context Protocol (MCP) tokens (`create`, `rotate`, `list`, and `revoke`). While the `create` and `rotate` endpoints correctly call `requireSessionAuthentication()` to ensure that only interactive web sessions (and not clients authenticated via an MCP token itself) can perform these actions, the `list` and `revoke` endpoints do not enforce this check. An attacker who has obtained a leaked or compromised MCP token can authenticate to the Appsmith backend with MCP authority and call the `list` endpoint (`GET /api/v1/mcp/tokens`) to retrieve metadata of all active MCP tokens belonging to the victim user, or call the `revoke` endpoint (`DELETE /api/v1/mcp/tokens/{tokenId}`) to delete/revoke any of the user's active MCP tokens, leading to unauthorized credential management and a Denial of Service (DoS) of legitimate integrations.
Proof of Concept:
**Steps to Reproduce**
1. Obtain a valid MCP token for a user.
2. Send an HTTP GET request to `/api/v1/mcp/tokens` using the MCP token as a Bearer token in the Authorization header. Observe that the list of active tokens is returned.
3. Send an HTTP DELETE request to `/api/v1/mcp/tokens/{tokenId}` using the MCP token as a Bearer token in the Authorization header to revoke another active token. Observe that the token is successfully revoked.
Affected Code:
@GetMapping
public Flux<ResponseDTO<McpTokenResponseDTO>> list(@AuthenticationPrincipal User user) {
return userMcpTokenService.list(user).map(token -> new ResponseDTO<>(HttpStatus.OK, token));
}
@DeleteMapping("/{tokenId}")
public Mono<ResponseDTO<Boolean>> revoke(@AuthenticationPrincipal User user, @PathVariable String tokenId) {
return userMcpTokenService
.revoke(user, tokenId)
.map(revoked -> new ResponseDTO<>(revoked ? HttpStatus.OK : HttpStatus.NOT_FOUND, revoked));
}
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.
Adds a patch_widgets 'visibleWhen' { control, equals } binding: gate a widget's
isVisible on a select/tabs control's value, compiled to
`{{ Control.selectedOptionValue === 'value' }}` (select) or
`{{ Control.selectedTab === 'value' }}` (tabs). One control can switch between
views (e.g. a table vs a detail panel) with shared data. The value property is
chosen by the compiler from the control's widget type, never agent-supplied;
the control must exist and be a select/tabs widget; `equals` is a safe literal
(no quote/brace/backtick) emitted inside single quotes, so it can't break out.
Mirrors the disableWhenInvalid pattern: type guard, literal-vs-binding conflict
guard, unregister-on-literal, and a semantic round-trip so the agent re-reads it.
Council review: security-reviewer + qa-engineer APPROVE WITH RISKS (findings
folded in: negative-charset + dotted-value + unregister tests). The senior
architect BLOCKED an accompanying card-grid data path (list `source` +
`fromItem`): the emitted LIST_WIDGET_V2 DSL is structurally incomplete (missing
the meta-widget template layer + mainCanvasId/mainContainerId/primaryKeys) and
would import but render broken, and it can't be verified live on this branch.
Per that block, the card-grid path is removed and deferred to a milestone that
can be render-verified; only visibleWhen (independently safe) lands here.
383 mcp tests pass; tsc clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ct U+2028/U+2029
Live-testing an MCP-built app surfaced that SELECT_WIDGET rendered an EMPTY
dropdown: the template emitted the legacy `options` prop, but the modern widget
derives its dropdown from `sourceData` (a JS-evaluated JSON array) keyed by
optionLabel/optionValue. The template now emits sourceData = JSON.stringify(the
agent's {label,value} options), optionLabel "label", optionValue "value", and
marks sourceData in dynamicPropertyPathList — byte-for-byte the shape the
first-party SelectWidget default uses. Verified live: the dropdown now populates
and selects correctly.
Security hardening (review of the above): because sourceData is now JS-evaluated,
add U+2028/U+2029 (line/paragraph separators) to RAW_EXPRESSION in both schema.ts
and editPatch.ts. JSON.stringify leaves them unescaped and they terminate a JS
string literal on pre-ES2019 engines — rejecting them closes a defense-in-depth
gap across EVERY safeText sink, not just the select. The agent's option
label/value are safeText (no {{ }} / ${ / backtick) and JSON.stringify escapes
quotes/backslashes, so the emitted array literal is inert.
Council: security-reviewer APPROVE WITH RISKS -> the U+2028/U+2029 guard it
recommended is included. 384 mcp tests pass; tsc/eslint/prettier clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.Important
🟣 🟣 🟣 Your tests are running.
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/29344307119
Commit: 92117cc
Workflow:
PR Automation test suiteTags:
@tag.AllSpec: ``
Tue, 14 Jul 2026 15:13:48 UTC