Skip to content

fix(agent-tools): validate url credential fields and route tool clients through pinnedFetch - #356

Open
egorvas wants to merge 3 commits into
croffasia:mainfrom
egorvas:fix/agent-tools-url-validation
Open

egorvas wants to merge 3 commits into
croffasia:mainfrom
egorvas:fix/agent-tools-url-validation

Conversation

@egorvas

@egorvas egorvas commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

What

A url credential field is validated when stored, and the outbound tool clients in @repo/agent-tools send through the @repo/net SSRF guard.

  • coerceConfig gives type: 'url' its own branch: the value must parse, use https, and not name a private or local host unless SSRF_ALLOWED_HOSTS lists it (same rules and same env var as repository connections and webhooks); userinfo, query string and fragment are refused. The stored value is the origin plus path with no trailing slash — the path is kept because an OpenAI-compatible endpoint carries /v1 and a Gitea can be served under a sub-path. Both url fields in the catalog (Gitea baseUrl in @repo/agent-tools, LLM provider baseUrl in integrations/catalog.ts) go through it, so a model credential is validated at save time too.
  • @repo/net exports checkHttpUrl, the synchronous subset of assertPublicHttpUrl (no DNS lookup), which vet now also uses internally; the behaviour of assertPublicHttpUrl and pinnedFetch is unchanged.
  • @repo/agent-tools depends on @repo/net, and every client in the package (Gitea, Notion, Firecrawl, Threads, Instagram, Jina, Telegram) calls pinnedFetch instead of global fetch: the host is resolved and pinned per call and a 3xx is returned rather than followed, so a token never leaves the vetted origin. The Gitea client turns a 3xx into a message telling the operator to set the URL Gitea answers on.
  • The agent runtime (resolveModel) runs the stored model base URL through assertPublicHttpUrl before passing it to the provider SDK, so a credential stored before this change, or a host whose address changed since, is caught at run time.
  • PATCH /teams/:teamId/integrations/:credentialId: when the patch changes a url field, every secret field of the schema must be in the patch; otherwise 400 Changing <url label> requires entering <secret label> again. Resending the same url (what the edit form does) is not a change.

Why

A credential's url field reached the runtime as a plain string, and the Gitea client fetched it with global fetch, so a stored URL could address an internal or link-local host and the token would be sent there, redirects included. The model provider baseUrl had the same gap on the run path, and an edit that only changed baseUrl forwarded the stored secret to the new address. Closes the SSRF finding on url-type integration credentials.

How to test

  1. Team settings → Integrations → add Gitea: an http:// URL, a private address (https://10.0.0.5), https://localhost:3000 or a URL with user:pw@ is rejected with a 400 naming the field; https://git.example.com/ is stored as https://git.example.com.
  2. Add "OpenAI-compatible (custom)" with base URL https://llm.example.com/v1/: stored as https://llm.example.com/v1; http://… is rejected.
  3. Edit a Gitea credential and change only the instance URL: 400 asking for the access token again. Enter the token as well: 200.
  4. Set SSRF_ALLOWED_HOSTS=<your private gitea host> on the api: that host (https only) is accepted; any other private host is still refused.
  5. cd packages/agent-tools && bun test, cd packages/net && bun test, cd apps/api && bun test --env-file=../../.env.test src/modules/agents/integrations.

Checklist

  • bun run typecheck passes
  • bun run lint and bun run format:check pass
  • Tests added or updated for the changed behaviour
  • Database schema changed: migration generated with bun run db:generate and committed
  • New environment variables documented in .env.example (no new variable; the existing SSRF_ALLOWED_HOSTS note now lists integration credentials as a path that reads it)
  • Docs updated (README.md or the relevant AGENTS.md) — packages/agent-tools/README.md

🤖 Generated with Claude Code

…ts through pinnedFetch

A `url` credential field (Gitea instance URL, OpenAI-compatible base URL) is
checked against the @repo/net rules when stored: https only, no private or
local host unless SSRF_ALLOWED_HOSTS names it, no userinfo, query or fragment.
The stored value is origin plus path. Every outbound client under
packages/agent-tools sends through pinnedFetch, which vets and pins the host on
every call and returns a redirect instead of following it with the token. The
agent runtime vets the model credential's base URL again before handing it to
the provider SDK, and an update that moves a url field must carry the secret
again.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@egorvas
egorvas requested a review from croffasia as a code owner September 9, 2026 13:58
@croffasia

Copy link
Copy Markdown
Owner

net refactor and pinnedFetch are fine. Blocker is resolveModel: vetting baseUrl on every run breaks self-hosted local models (http://ollama:11434), and SSRF_ALLOWED_HOSTS doesn't help — https is checked before the host. Existing credentials break and can't be re-saved.

Allow http for hosts named in SSRF_ALLOWED_HOSTS, then it's good.

The scheme was checked before the hostname was read, so naming a host could not
admit http to it and a self-hosted model server on http://ollama:11434 had no
way to be configured at all. The host and the allowlist are now resolved first,
and http reaches a named host. Every other scheme is still refused, the resolved
address is still pinned, and a host nobody named is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@egorvas

egorvas commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Done, pushed. check() now reads the hostname and the allowlist first and admits http to a named host. http://ollama:11434/v1 saves and runs with SSRF_ALLOWED_HOSTS=ollama; every other scheme is still refused for it, the resolved address is still pinned, and a host nobody named is unchanged.

Three things worth flagging before you merge.

This is not confined to the model path. The ordering is upstream, not something this PR introduced, and check() is what every outbound path goes through. So the relaxation also reaches webhook targets, attachment import and git connections. Naming a host already let anyone who can create a webhook reach it, so the new part is only that the request may now be cleartext — an api key or a webhook secret included. I put that sentence in the SSRF_ALLOWED_HOSTS comment in .env.example rather than leaving it implicit. If you would rather keep the relaxation narrow, the alternative is a flag on check() set only where the URL is operator configuration, and left off where it is content.

It reverses one of your own tests. does not relax the https requirement for a named host in pinned-fetch.test.ts is on main and predates this branch. I rewrote it as admits http to a named host, and no other scheme and added keeps http off a host that is not named beside it, plus the matching cases in net.test.ts and coerce-config.test.ts. Saying so explicitly in case the rule was load-bearing somewhere I did not look.

One break is left, and it is a config change on upgrade. An instance that already stores http://ollama:11434 still fails until the operator adds SSRF_ALLOWED_HOSTS=ollama. The runs fail loudly rather than silently, but it is an upgrade that needs a hand.

If that last one bothers you, there is a cheaper way out than the allowlist: drop the vetting in resolveModel entirely and keep it at save time only. Looking at it again, the run-time check buys very little. It calls assertPublicHttpUrl(baseUrl) and then hands the same baseUrl to the provider SDK, which resolves the name again on its own connection — so nothing is pinned and a DNS flip between the two defeats it. What it actually catches is a statically private address, which coerceConfig already refuses when the credential is saved. Removing it would leave existing credentials working untouched. Say the word and I will.

Verification: packages/net 21 pass, packages/agent-tools 19 pass, full api suite 1809 pass / 0 fail, format:check, lint, typecheck clean.

@egorvas

egorvas commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Same as #353: the failing lint job comes from main, not from this branch. messages/id/apiKeys.json is missing the five API key expiry strings, so i18n-json/identical-keys errors on main too. #381 fixes it.

…-validation

# Conflicts:
#	packages/net/src/index.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants