feat: embedded MCP server with user-scoped token authentication (#15383)#41980
feat: embedded MCP server with user-scoped token authentication (#15383)#41980salevine wants to merge 1 commit 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. |
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 server and package
Token lifecycle and UI
Deployment and CI
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant UserProfile
participant AppsmithAPI
participant McpServer
User->>UserProfile: Create MCP token
UserProfile->>AppsmithAPI: POST /v1/users/mcp-tokens
AppsmithAPI-->>UserProfile: Token metadata and secret
User->>McpServer: MCP request with bearer token
McpServer->>AppsmithAPI: Authenticate token and access app data
AppsmithAPI-->>McpServer: Authenticated result
McpServer-->>User: MCP response
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 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}
|
|
||
| return userMcpTokenRepository | ||
| .findByTokenIdAndDeletedAtIsNull(tokenId) | ||
| .filter(storedToken -> passwordEncoder.matches(hashToken(token), storedToken.getTokenHash())) |
There was a problem hiding this comment.
The newly introduced Model Context Protocol (MCP) token authentication mechanism uses Spring Security's default PasswordEncoder (which uses the computationally expensive BCrypt algorithm) to verify API tokens on every authenticated request.
BCrypt is designed to protect low-entropy user passwords from brute-force attacks by consuming significant CPU time (typically 50-100ms per check). However, MCP tokens generated by Appsmith already contain high entropy (a UUID and a cryptographically secure 256-bit random secret), making brute-force attacks mathematically impossible and key-stretching algorithms like BCrypt completely redundant.
An attacker can exploit this to perform a highly effective Denial of Service (DoS) attack. By generating active MCP tokens for their own account and sending a flood of concurrent API requests containing these tokens, the attacker forces the server to repeatedly execute passwordEncoder.matches(...). Because BCrypt is extremely CPU-intensive, a small volume of concurrent requests will completely saturate the JVM's CPU threads, rendering the entire Appsmith instance unresponsive to all users.
Steps to Reproduce
- Log in to Appsmith as any standard user.
- Generate an MCP token by sending a POST request to
/api/v1/users/mcp-tokens. - Extract the
tokenId(UUID) from the response. - Send a high volume of concurrent requests to any API endpoint (e.g.,
/api/v1/users/me) with anAuthorizationheader containing the validtokenIdbut an incorrect secret (e.g.,Authorization: Bearer mcp_<tokenId>.invalid_secret). - Observe that the server's CPU usage spikes to 100%, and the Appsmith instance becomes completely unresponsive to all users.
# 1. Register or log in to a low-privileged account on Appsmith.
# 2. Create an MCP token via POST /api/v1/users/mcp-tokens to get a valid token ID (UUID).
# 3. Flood the server with requests using the valid token ID but an incorrect secret:
for i in {1..100}; do
curl -H "Authorization: Bearer mcp_<valid_token_id>.<wrong_secret>" https://<appsmith-instance>/api/v1/users/me &
done
waitFix with AI
A security vulnerability was found by Hacktron.
File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserMcpTokenServiceCEImpl.java
Lines: 90
Severity: high
Vulnerability: Denial of Service (DoS) via CPU Exhaustion in MCP Token Authentication
Description:
The newly introduced Model Context Protocol (MCP) token authentication mechanism uses Spring Security's default `PasswordEncoder` (which uses the computationally expensive BCrypt algorithm) to verify API tokens on every authenticated request.
BCrypt is designed to protect low-entropy user passwords from brute-force attacks by consuming significant CPU time (typically 50-100ms per check). However, MCP tokens generated by Appsmith already contain high entropy (a UUID and a cryptographically secure 256-bit random secret), making brute-force attacks mathematically impossible and key-stretching algorithms like BCrypt completely redundant.
An attacker can exploit this to perform a highly effective Denial of Service (DoS) attack. By generating active MCP tokens for their own account and sending a flood of concurrent API requests containing these tokens, the attacker forces the server to repeatedly execute `passwordEncoder.matches(...)`. Because BCrypt is extremely CPU-intensive, a small volume of concurrent requests will completely saturate the JVM's CPU threads, rendering the entire Appsmith instance unresponsive to all users.
Proof of Concept:
**Steps to Reproduce**
1. Log in to Appsmith as any standard user.
2. Generate an MCP token by sending a POST request to `/api/v1/users/mcp-tokens`.
3. Extract the `tokenId` (UUID) from the response.
4. Send a high volume of concurrent requests to any API endpoint (e.g., `/api/v1/users/me`) with an `Authorization` header containing the valid `tokenId` but an incorrect secret (e.g., `Authorization: Bearer mcp_<tokenId>.invalid_secret`).
5. Observe that the server's CPU usage spikes to 100%, and the Appsmith instance becomes completely unresponsive to all users.
```bash
# 1. Register or log in to a low-privileged account on Appsmith.
# 2. Create an MCP token via POST /api/v1/users/mcp-tokens to get a valid token ID (UUID).
# 3. Flood the server with requests using the valid token ID but an incorrect secret:
for i in {1..100}; do
curl -H "Authorization: Bearer mcp_<valid_token_id>.<wrong_secret>" https://<appsmith-instance>/api/v1/users/me &
done
wait
```
Affected Code:
.filter(storedToken -> passwordEncoder.matches(hashToken(token), storedToken.getTokenHash()))
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.
salevine
added
ok-to-test
Failed server tests
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
| mcp-build: | ||
| name: mcp-build | ||
| needs: [file-check] | ||
| if: success() && needs.file-check.outputs.runId == 0 | ||
| uses: ./.github/workflows/mcp-build.yml | ||
| secrets: inherit | ||
| with: | ||
| pr: ${{fromJson(needs.file-check.outputs.pr)}} | ||
|
|
||
| build-docker-image: | ||
| needs: [file-check, client-build, server-build, rts-build] | ||
| needs: [file-check, client-build, server-build, rts-build, mcp-build] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix runId comparison type inconsistency.
Line 121 uses needs.file-check.outputs.runId == 0 (unquoted integer), while all other jobs in this file (lines 112, 130, 140) use needs.file-check.outputs.runId == '0' (quoted string). GitHub Actions outputs are strings, so the unquoted comparison may fail to match. Align with the existing pattern.
🔧 Proposed fix
mcp-build:
name: mcp-build
needs: [file-check]
- if: success() && needs.file-check.outputs.runId == 0
+ if: success() && needs.file-check.outputs.runId == '0'
uses: ./.github/workflows/mcp-build.yml📝 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.
| mcp-build: | |
| name: mcp-build | |
| needs: [file-check] | |
| if: success() && needs.file-check.outputs.runId == 0 | |
| uses: ./.github/workflows/mcp-build.yml | |
| secrets: inherit | |
| with: | |
| pr: ${{fromJson(needs.file-check.outputs.pr)}} | |
| build-docker-image: | |
| needs: [file-check, client-build, server-build, rts-build] | |
| needs: [file-check, client-build, server-build, rts-build, mcp-build] | |
| mcp-build: | |
| name: mcp-build | |
| needs: [file-check] | |
| if: success() && needs.file-check.outputs.runId == '0' | |
| uses: ./.github/workflows/mcp-build.yml | |
| secrets: inherit | |
| with: | |
| pr: ${{fromJson(needs.file-check.outputs.pr)}} | |
| build-docker-image: | |
| needs: [file-check, client-build, server-build, rts-build, mcp-build] |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 118-125: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[warning] 122-122: secrets unconditionally inherited by called workflow (secrets-inherit): this reusable workflow
(secrets-inherit)
🤖 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/build-client-server.yml around lines 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.
| - name: Lint | ||
| run: yarn lint | ||
|
|
||
| - name: Check formatting | ||
| run: yarn prettier | ||
|
|
||
| - name: Lint | ||
| run: yarn lint |
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.
| - 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.
| await esbuild.build({ | ||
| entryPoints: ["src/server.ts"], | ||
| bundle: true, | ||
| format: "esm", | ||
| minify: true, | ||
| platform: "node", | ||
| sourcemap: true, | ||
| target: `node${process.versions.node}`, | ||
| outdir: "dist/bundle", | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check esbuild documentation or source for accepted target formats
cat app/client/packages/mcp/package.json | jq '.dependencies.esbuild'
# Also check if build output shows any warnings about target formatRepository: appsmithorg/appsmith
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- build.js ---'
cat -n app/client/packages/mcp/build.js
echo
echo '--- package tree ---'
git ls-files app/client/packages/mcp | sed 's#^`#-` #'
echo
echo '--- search esbuild references ---'
rg -n --hidden --glob '!**/node_modules/**' 'esbuild' app/client/packages/mcp . | head -n 200Repository: appsmithorg/appsmith
Length of output: 8831
🌐 Web query:
esbuild target option full semver node20.11.0 accepted documentation
💡 Result:
esbuild does not support full semantic versioning (semver) for the target option [1]. The --target flag accepts specific environment values (e.g., node14, chrome80, es2020) rather than arbitrary version strings like 20.11.0 [2][3][4]. Specifically regarding Node.js, esbuild uses major version numbers to determine which JavaScript syntax features are supported [5]. Using a target like node20.11.0 is not a valid syntax; you should use the major version identifier, such as --target=node20 [2][3]. esbuild maps these major version targets to the specific syntax features available in those environments [5]. If you require finer-grained control over specific syntax features, you can use the --supported flag to manually enable or disable support for individual language features [3][4].
Citations:
- 1: https://stackoverflow.com/questions/79948555/where-can-i-find-a-list-of-valid-values-for-esbuilds-target-property
- 2: TypeScript support evanw/esbuild#923
- 3: https://manpages.debian.org/trixie/esbuild/esbuild.1.en.html
- 4: https://manpages.debian.org/bookworm/esbuild/esbuild.1.en.html
- 5: Import attributes should be enabled for
node18and >=node20targets evanw/esbuild#3778
Use a major-only Node target for esbuild. process.versions.node includes minor/patch segments, so this builds node20.11.0, which esbuild does not accept for target. Use node${process.versions.node.split(".")[0]} or a fixed node20 value instead.
🤖 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/build.js` around lines 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.
| "Bearer user-token", | ||
| ), | ||
| ).toBe(true); | ||
| for (const callIndex of [5, 6]) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix ESLint padding-line-between-statements failure.
The pipeline fails on yarn lint because a blank line is required before the for statement at line 59. As per coding guidelines, client code must pass ESLint validation.
🔧 Proposed fix
).toBe(true);
for (const callIndex of [5, 6]) {📝 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.
| for (const callIndex of [5, 6]) { | |
| ).toBe(true); | |
| for (const callIndex of [5, 6]) { |
🧰 Tools
🪛 GitHub Actions: Build MCP Workflow / 0_build.txt
[error] 59-59: ESLint: 'Expected blank line before this statement' (padding-line-between-statements). Step failed during 'yarn lint'.
🪛 GitHub Actions: Build MCP Workflow / build
[error] 59-59: ESLint (padding-line-between-statements): Expected blank line before this statement.
🪛 GitHub Check: build
[failure] 59-59:
Expected blank line before this statement
🤖 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.test.ts` at 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.
Sources: Coding guidelines, Pipeline failures
| const fullArtifact = (fetchFn.mock.calls[5][1]?.body as FormData).get( | ||
| "file", | ||
| ) as File; | ||
| const partialArtifact = (fetchFn.mock.calls[6][1]?.body as FormData).get( | ||
| "file", | ||
| ) as File; | ||
|
|
||
| await expect( | ||
| fullArtifact.text(), | ||
| ).resolves.toBe( | ||
| JSON.stringify({ application: { name: "New application" } }), | ||
| ); | ||
| await expect(partialArtifact.text()).resolves.toBe( | ||
| JSON.stringify({ widgets: {} }), | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix Prettier formatting issues flagged by the build.
Lines 69 and 76 have Prettier violations: a blank line is expected before line 69, and the await expect at line 76 should be collapsed to a single line.
🔧 Proposed fixes
}
const fullArtifact = (fetchFn.mock.calls[5][1]?.body as FormData).get(
"file",
) as File;
const partialArtifact = (fetchFn.mock.calls[6][1]?.body as FormData).get(
"file",
) as File;
- await expect(
- fullArtifact.text(),
- ).resolves.toBe(
+ await expect(fullArtifact.text()).resolves.toBe(
JSON.stringify({ application: { name: "New application" } }),
);🧰 Tools
🪛 GitHub Check: build
[failure] 76-76:
Replace ⏎······fullArtifact.text(),⏎···· with fullArtifact.text()
[failure] 69-69:
Expected blank line before this statement
🤖 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.test.ts` around lines 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.
Sources: Coding guidelines, Linters/SAST tools
| import { z } from "zod"; | ||
|
|
||
| const MAX_ID_LENGTH = 128; | ||
| export const MAX_ARTIFACT_BYTES = 1024 * 1024; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Formatting failures are breaking the "build" CI check.
The build check flags Lines 14, 33, 120, and 297 for Prettier formatting violations — these are current CI failures, not just style nits, and should be fixed before merge.
Also applies to: 33-33, 120-120, 297-297
🧰 Tools
🪛 GitHub Check: build
[failure] 14-14:
Expected blank line before this statement
🤖 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 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.
Sources: Linters/SAST tools, Pipeline failures
| async function request<T>(path: string, init?: RequestInit): Promise<T> { | ||
| const isMultipart = init?.body instanceof FormData; | ||
| const response = await fetchFn(`${apiBaseUrl}${path}`, { | ||
| ...init, | ||
| headers: { | ||
| Authorization: `Bearer ${token}`, | ||
| ...(isMultipart ? {} : { "Content-Type": "application/json" }), | ||
| ...init?.headers, | ||
| }, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Appsmith API request failed (${response.status})`); | ||
| } | ||
|
|
||
| return ((await response.json()) as ApiResponse<T>).data; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Outbound backend requests have no timeout.
request() calls fetchFn with no AbortController/timeout. If the Appsmith backend hangs or is slow, MCP requests (and the sessions/resources they hold) can block indefinitely, degrading availability of the MCP process.
⏱️ Add a request timeout
async function request<T>(path: string, init?: RequestInit): Promise<T> {
const isMultipart = init?.body instanceof FormData;
const response = await fetchFn(`${apiBaseUrl}${path}`, {
...init,
+ signal: AbortSignal.timeout(15_000),
headers: {
Authorization: `Bearer ${token}`,
...(isMultipart ? {} : { "Content-Type": "application/json" }),
...init?.headers,
},
});📝 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.
| async function request<T>(path: string, init?: RequestInit): Promise<T> { | |
| const isMultipart = init?.body instanceof FormData; | |
| const response = await fetchFn(`${apiBaseUrl}${path}`, { | |
| ...init, | |
| headers: { | |
| Authorization: `Bearer ${token}`, | |
| ...(isMultipart ? {} : { "Content-Type": "application/json" }), | |
| ...init?.headers, | |
| }, | |
| }); | |
| if (!response.ok) { | |
| throw new Error(`Appsmith API request failed (${response.status})`); | |
| } | |
| return ((await response.json()) as ApiResponse<T>).data; | |
| } | |
| async function request<T>(path: string, init?: RequestInit): Promise<T> { | |
| const isMultipart = init?.body instanceof FormData; | |
| const response = await fetchFn(`${apiBaseUrl}${path}`, { | |
| ...init, | |
| signal: AbortSignal.timeout(15_000), | |
| headers: { | |
| Authorization: `Bearer ${token}`, | |
| ...(isMultipart ? {} : { "Content-Type": "application/json" }), | |
| ...init?.headers, | |
| }, | |
| }); | |
| if (!response.ok) { | |
| throw new Error(`Appsmith API request failed (${response.status})`); | |
| } | |
| return ((await response.json()) as ApiResponse<T>).data; | |
| } |
🤖 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 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.
| transport = new StreamableHTTPServerTransport({ | ||
| sessionIdGenerator: randomUUID, | ||
| onsessioninitialized: (id) => { | ||
| sessions.set(id, { | ||
| expiresAt: now() + sessionTtlMs, | ||
| token, | ||
| username, | ||
| transport: transport!, | ||
| }); | ||
| }, | ||
| }); |
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.
| function reportProcessFailure(kind: string) { | ||
| process.stderr.write(`Appsmith MCP ${kind}\n`); | ||
| process.exitCode = 1; | ||
| } | ||
|
|
||
| process.once("uncaughtException", () => reportProcessFailure("process failure")); | ||
| process.once("unhandledRejection", () => reportProcessFailure("process rejection")); | ||
|
|
||
| httpServer.listen(port, "127.0.0.1", () => { | ||
| process.stderr.write(`Appsmith MCP listening on 127.0.0.1:${port}\n`); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Process doesn't actually exit after an uncaught exception/rejection.
Setting process.exitCode only affects the exit code used when the process naturally exits — it does not terminate it. Since the HTTP server keeps the event loop alive, this MCP process will keep serving requests indefinitely after an uncaught exception/rejection, in a potentially corrupted state, and supervisord (per mcp.conf) never gets the chance to restart it. Also fixes the prettier formatting flagged by the build check.
🛑 Proposed fix: exit after fatal errors
function reportProcessFailure(kind: string) {
process.stderr.write(`Appsmith MCP ${kind}\n`);
- process.exitCode = 1;
+ process.exit(1);
}
-process.once("uncaughtException", () => reportProcessFailure("process failure"));
-process.once("unhandledRejection", () => reportProcessFailure("process rejection"));
+process.once("uncaughtException", () => {
+ reportProcessFailure("process failure");
+});
+process.once("unhandledRejection", () => {
+ reportProcessFailure("process rejection");
+});📝 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.
| function reportProcessFailure(kind: string) { | |
| process.stderr.write(`Appsmith MCP ${kind}\n`); | |
| process.exitCode = 1; | |
| } | |
| process.once("uncaughtException", () => reportProcessFailure("process failure")); | |
| process.once("unhandledRejection", () => reportProcessFailure("process rejection")); | |
| httpServer.listen(port, "127.0.0.1", () => { | |
| process.stderr.write(`Appsmith MCP listening on 127.0.0.1:${port}\n`); | |
| }); | |
| function reportProcessFailure(kind: string) { | |
| process.stderr.write(`Appsmith MCP ${kind}\n`); | |
| process.exit(1); | |
| } | |
| process.once("uncaughtException", () => { | |
| reportProcessFailure("process failure"); | |
| }); | |
| process.once("unhandledRejection", () => { | |
| reportProcessFailure("process rejection"); | |
| }); | |
| httpServer.listen(port, "127.0.0.1", () => { | |
| process.stderr.write(`Appsmith MCP listening on 127.0.0.1:${port}\n`); | |
| }); |
🧰 Tools
🪛 GitHub Check: build
[failure] 14-14:
Replace ·reportProcessFailure("process·rejection") with ⏎··reportProcessFailure("process·rejection"),⏎
[failure] 13-13:
Replace ·reportProcessFailure("process·failure") with ⏎··reportProcessFailure("process·failure"),⏎
🤖 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/server.ts` around lines 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ેન
Source: Linters/SAST tools
| private String extractTokenId(String token) { | ||
| if (!token.startsWith(TOKEN_PREFIX)) { | ||
| return null; | ||
| } | ||
|
|
||
| int separatorIndex = token.indexOf('.', TOKEN_PREFIX.length()); | ||
| if (separatorIndex == -1) { | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| return UUID.fromString(token.substring(TOKEN_PREFIX.length(), separatorIndex)) | ||
| .toString(); | ||
| } catch (IllegalArgumentException exception) { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and nearby symbols.
git ls-files 'app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserMcpTokenServiceCEImpl.java'
echo '---'
ast-grep outline app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserMcpTokenServiceCEImpl.java --view expanded
echo '---'
# Show the relevant section with line numbers.
sed -n '1,260p' app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserMcpTokenServiceCEImpl.java
echo '---'
# Find authenticate call sites / manager.
rg -n "authenticate\\(String token\\)|McpTokenAuthenticationManager|extractTokenId\\(" app/server/appsmith-server/src/main/javaRepository: appsmithorg/appsmith
Length of output: 7243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/managers/McpTokenAuthenticationManager.java
echo '---'
sed -n '1,260p' app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java
echo '---'
rg -n "UserMcpTokenServiceCE|authenticate\\(" app/server/appsmith-server/src/main/java/com/appsmith/server/authentication app/server/appsmith-server/src/main/java/com/appsmith/server/configurationsRepository: appsmithorg/appsmith
Length of output: 16720
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/managers/McpTokenAuthenticationManager.java
echo '---'
sed -n '1,260p' app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java
echo '---'
rg -n "McpTokenAuthenticationManager|authenticate\\(" app/server/appsmith-server/src/main/java/com/appsmith/server/authentication app/server/appsmith-server/src/main/java/com/appsmith/server/configurationsRepository: appsmithorg/appsmith
Length of output: 17282
Guard extractTokenId against null tokens. McpTokenAuthenticationManager passes the raw credentials through, so a missing token currently throws synchronously at token.startsWith(...) instead of falling back to Mono.empty().
🤖 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 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.
1 participant
What & why
Adds an opt-in, 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
Warning
Tests have not run on the HEAD f07f6bf yet
Fri, 10 Jul 2026 21:31:53 UTC