fix(mcp): enforce MCP feature flag on all token auth paths - #42186
Open
hacktron-app[bot] wants to merge 2 commits into
Open
fix(mcp): enforce MCP feature flag on all token auth paths#42186hacktron-app[bot] wants to merge 2 commits into
hacktron-app[bot] wants to merge 2 commits into
Conversation
authenticate() gated the organization MCP feature flag only in the cache path, and did so via contextWrite/.then(validateMCPEnabled()) whose context never propagated to the check, causing it to fail open; the store path skipped the check entirely. Move validateMCPEnabled() to the top of authenticate() so both cache and store paths enforce it on the request's real reactive context, and drop the now-dead contextWrite block and unused imports.
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
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.
Vulnerability (root cause)
McpTokenServiceCEImpl.authenticate(...)is the sink used by the MCP token authentication manager. It should reject authentication when the organization has the MCP feature disabled, but the enablement check (validateMCPEnabled()) was applied incorrectly, allowing a bypass (A07:2025):authenticateFromCache) — context loss / fail-open. The check was written as:contextWriteoperators only decorate the innerMono.just(user)and never reachvalidateMCPEnabled(), which runs downstream of.then(). In EE,getCurrentUserOrganizationId()reads the request context, so it resolved empty,validateMCPEnabled()emitted nothing, and the check failed open — authentication succeeded even with MCP disabled.authenticateFromStore) — the DB-backed fallback never calledvalidateMCPEnabled()at all, bypassing the flag entirely.Fix
Enforce the flag once, at the top of the public
authenticate(...)method, on the caller's real reactive context, before either path runs:validateMCPEnabled()errors withAppsmithExceptionwhen MCP is disabled, which now propagates to the caller for both cache and store paths. The ineffectivecontextWrite(...)/.then(validateMCPEnabled())block is removed fromauthenticateFromCache, along with the imports that became unused (ReactiveSecurityContextHolder,Authentication,UsernamePasswordAuthenticationToken,ORGANIZATION_ID).This matches how
create(...)already gates onvalidateMCPEnabled()and centralizes the check at the single shared entry point.Verification
getCurrentUserOrganization()emits the org,validateMCPEnabled()completes, and the cache/store flow proceeds unchanged (legitimate tokens still authenticate). With MCP disabled,validateMCPEnabled()errors before any token lookup, so both paths are blocked.authenticatehas no EE override;authenticateFromCache/authenticateFromStoreare private with no overrides, so the centralized gate covers CE and EE (McpTokenServiceImplextends the CE impl).should_throwError_whenMCPDisabledCacheHashMatchesAndUserIsEnabledtest, which asserted viaexpectErrorSatisfies(...)but was missing the terminal.verify()(so it never subscribed and passed vacuously — the reason the bug went unnoticed). Addedshould_throwError_whenMCPDisabledAndStoreHashMatchesAndUserIsEnabledcovering the store path. Could not execute the suite: the sandbox has no JDK/Maven available.Automated fix by Hacktron for finding: https://app.hacktron.ai/appsmith-inc/findings/1870355c-c981-48ad-b8b0-c124d304abf0