fix: route MCP provider credential lookup away from builtin auth (fix #40612) - #40686
Open
Sskift wants to merge 1 commit into
Open
fix: route MCP provider credential lookup away from builtin auth (fix #40612)#40686Sskift wants to merge 1 commit into
Sskift wants to merge 1 commit into
Conversation
…ggenius#40612) getProviderCredentialType classified any provider with stored team_credentials as api-key, before checking provider.type. An MCP provider with team_credentials was therefore routed into the plugin-auth UI path, where useGetApi builds a builtin-credential URL for the MCP server id and the backend 500s. Scope the team_credentials check to builtin providers only so MCP providers resolve to no credential type and never mount the plugin-auth path. Co-Authored-By: Claude <noreply@anthropic.com>
5 tasks
|
I reproduced #40612 on a self-hosted Dify 1.16.0 deployment and locally verified this branch ( Focused test: The provider-catalog change prevents an MCP provider with stored team credentials from being classified as a plugin |
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.
Summary
Fixes #40612.
getProviderCredentialType(inweb/features/agent-v2/agent-detail/configure/tool-provider-catalog.ts) classified any tool provider with storedteam_credentialsas credential typeapi-key, checkingteam_credentialsbeforeprovider.type. An MCP provider that had stored team credentials was therefore misclassified asapi-key, which routed it into the plugin-auth UI path and setcredentialVariant: 'unauthorized'.UnauthorizedCredentialStatusthen mountsusePluginAuth, whoseuseGetApialways builds URLs like/workspaces/current/tool-provider/builtin/<provider>/credential/info. For an MCP server,<provider>is the MCP server id (not a builtin/plugin provider id), so the backend returns 500.Root cause
The
team_credentialscheck ingetProviderCredentialTypewas not scoped to builtin providers, so any provider type (includingmcp) with stored team credentials resolved to'api-key'.Fix
Scope the
team_credentials→'api-key'classification toCollectionType.builtInonly, mirroring the existingoauth2branch. MCP (andcustom/workflow/model) providers now resolve to no credential type, so they no longer enter the plugin-auth path.Testing
web/features/agent-v2/agent-detail/configure/__tests__/tool-provider-catalog.spec.tscoveringgetProviderCredentialTypefor builtin (api-key / oauth2) and MCP (with/without team_credentials) providers.'api-key'and the test failed (expected 'api-key' to be undefined).pnpm --config.verifyDepsBeforeRun=false test configure→ 26 files, 379 tests passed.Note: this addresses the primary root cause (credential-type misclassification). The related hardening suggested in the issue — adding an
isBuiltinProviderguard on theUnauthorizedCredentialStatusrender path inprovider-tool/item.tsxso non-builtin providers short-circuit beforeusePluginAuthmounts — was intentionally left out of this PR to keep the change atomic and scoped to the actual 500 trigger; it can be a follow-up if maintainers want defense-in-depth.From Claude