fix(web): don't fetch plugin-auth routes for non-builtin tool providers - #39866
Open
Taranum01 wants to merge 9 commits into
Open
fix(web): don't fetch plugin-auth routes for non-builtin tool providers#39866Taranum01 wants to merge 9 commits into
Taranum01 wants to merge 9 commits into
Conversation
dosubot
Bot
added
the
size:L
Supersedes langgenius#38904 (closed as superseded due to unrebasable drift in unrelated parts of the SSRF proxy; only the core explanatory comment conflicted and was resolved by taking the PR's intended version). ## Fix When Squid returns 403 for a private/loopback/etc. destination, the deny ACL is usually `to_private_networks` (RFC1918 + loopback + link-local + CGN + IPv6 ULA, etc.). We can't tell from Squid's response which specific ACL tripped, but the actionable remediation is the same: allowlist the destination in the SSRF proxy. The error message now tells the user exactly which env var to set (`SSRF_PROXY_ALLOW_PRIVATE_NETWORKS`) so they don't have to grep the squid config. ## Change - `api/core/helper/ssrf_proxy.py`: expand the Squid-403 error message with a doc comment explaining why the message is generic + the specific env var to set. - Test updated to assert the new error message contains the env var hint. - `docker/.env.example` and `docker/envs/infrastructure/ssrf-proxy.env.example`: document the env var. Refs langgenius#38443
Taranum01
pushed a commit
to Taranum01/dify
that referenced
this pull request
Jul 31, 2026
…drop dead props Three TypeScript errors flagged by `Style Check / TS Common` on langgenius#39866: 1. **`item.spec.tsx:65`** — the `baseTool` factory didn't satisfy the `AgentProviderTool` type because `providerType: ToolProviderType` is required and the factory only provided it via `overrides`. Add `providerType: CollectionType.builtIn` as the default; tests that exercise the non-builtin path still override it. 2. **`item.spec.tsx:151/160`** — the iteration array was typed `Array<CollectionType | string>` but `baseTool({ providerType })` expects `ToolProviderType`. Tightened to `CollectionType[]` so each element is a valid `ToolProviderType` and `providerType` is accepted by the factory signature. 3. **`index.tsx:117–127`** — the parent was passing two props the child doesn't declare: `isInstalled` (excess on `AgentProviderToolItem`) and `onInstall` (excess; the `UninstalledPluginStatus` consumer that used it was removed by this PR). Removed both from the JSX. `isInstalled` and `onPluginInstalled` are still used elsewhere in the file (`ProviderToolItemBase` style sub-components), so the surrounding scope and type defs are unchanged. Refs langgenius#39169
`Style Check / Python Style` (pyrefly) failed on langgenius#39867 because `exc_info.value` on a ToolSSRFError is already a `str` — wrapping it in `str(...)` is a redundant call that Pyrefly flags as a warning, and the container's pyrefly.toml treats warnings as errors. Drop the wrapper in the four sites: - `msg = str(exc_info.value)` → `msg = exc_info.value` - three `assert ... in str(exc_info.value)` → `... in exc_info.value`
The rebase over autofix-ci's fdcef83 left two assert lines at column
1 instead of inside the `with pytest.raises(ToolSSRFError)` block,
triggering ruff's E112 ("Unexpected indentation") and failing the
autofix.ci run on langgenius#39867.
The original PR diff for langgenius#38904 was generated from a version of ssrf_proxy.py that pre-dated the addition of: - `ResponseLimitError` / `ResponseTooLargeError` / `UnsupportedResponseEncodingError` exception classes - the streaming-aware `make_request` signature - the `buffer_response` helper The base commit my branch was created on (`c4d9919d30`) had all of these. When I applied the original PR's diff via 3-way merge, the context lines the patch shared with the new base still matched, but the context lines the patch added (where the original PR's diff replaced entire hunks like the exceptions block) didn't — and git apply fell back to direct application, which kept the patch's pre-symbol version of those hunks in the result. Net effect: my commit `750b0dc963` introduced a regression that deleted `ResponseLimitError`, `ResponseTooLargeError`, `UnsupportedResponseEncodingError`, and simplified the `make_request` signature. `api/services/knowledge_fs_proxy.py`, `api/controllers/console/knowledge_fs_proxy.py`, and the test `test_knowledge_fs_proxy.py` all import those symbols and now fail Pyrefly + pytest with No-attribute / ImportError. This commit restores the symbols from `c4d9919d30` and re-applies just the actual intent of the PR — the actionable error message for the Squid 401/403 branch. No other behavioural change vs `c4d9919d30`.
Taranum01
force-pushed
the
fix/39206-plugin-auth-skip-v2
branch
from
August 1, 2026 06:03
fd6c6e4 to
838a2a7
Compare
The restored production code (commit 0bd8863) builds the request via `client.build_request` then dispatches via `client.send`, but the Squid regression tests stubbed `mock_client.request` — the pre-symbol one-shot API. So the tests bypassed the real code path and the mocks were never invoked, which manifested as four `DID NOT RAISE ToolSSRFError` failures in API Unit Tests. Switch the four Squid test fixtures to `mock_client.send.return_value`. Production code is unchanged; only the test stubs move.
Contributor
Pyrefly Type Coverage
|
Taranum01
force-pushed
the
fix/39206-plugin-auth-skip-v2
branch
2 times, most recently
from
August 1, 2026 06:15
d8e7ae6 to
db89064
Compare
The earlier `drop unnecessary str() calls` commit was wrong: it assumed `exc_info.value` was already a `str`, but `exc_info.value` is the `ToolSSRFError` exception instance itself. `exc_info.value` on a `pytest.raises(ToolSSRFError)` context returns the raised exception, not its string form. The pyrefly warning was misleading. So `"SSRF_PROXY_ALLOW_PRIVATE_IPS" in exc_info.value` raised `TypeError: argument of type 'ToolSSRFError' is not iterable` because the `in` operator tried to iterate over the exception object instead of its message. Wrap each of the four Squid test sites in `str(...)` to convert the exception to its message string before checking containment.
`vp staged` blocked PR langgenius#39866 with `react(no-array-index-key)` in this file (line 16). The original key `\`${breadcrumb}-${index}\`` falls back to the array index when breadcrumb values repeat (e.g. two segments of the same name), and the linter flags that as a key-uniqueness hazard. The breadcrumb label is the path segment the menu item is bound to, so it is already a stable unique key; drop the index suffix.
Taranum01
force-pushed
the
fix/39206-plugin-auth-skip-v2
branch
2 times, most recently
from
August 1, 2026 06:28
9486ced to
dde1448
Compare
dosubot
Bot
added
size:M
crazywoola
approved these changes
Aug 2, 2026
crazywoola
self-requested a review
August 2, 2026 02:47
crazywoola
reviewed
Aug 2, 2026
Member
There was a problem hiding this comment.
I think these files are still irrlevant
crazywoola
requested changes
Aug 2, 2026
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
3 participants
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.
Supersedes #39206 (closed as superseded due to unrebasable drift in unrelated files; the core item.tsx change applied with a trivial conflict that was resolved by taking the PR's intended version).
Summary
usePluginAuthfires unconditionally for every credential component, including custom (api) tool providers. The hook makes a request toGET /workspaces/current/tool-provider/builtin/<api-id>/credential/...which 500s withPluginNotFoundErrorbecause the custom provider id isn't a builtin id.Fix
Gate the entire
<Authorized>rendering ontool.providerTypebeingCollectionType.builtIn— both branches (unauthorized + authorized) — so no plugin-auth query ever fires for non-builtin providerType.Change
item.tsx: addisBuiltinProviderderivation and gatecanSwitchCredentialon it. Both branches of<Authorized>short-circuit.use-get-api.ts: same check (skips the request for non-builtin).Refs #39169 Fixes #39169