Skip to content

[Fix] Authenticate model provider API keys before saving them - #1053

Open
daniel-lxs wants to merge 1 commit into
developfrom
claude/cool-ritchie-ec195b
Open

[Fix] Authenticate model provider API keys before saving them#1053
daniel-lxs wants to merge 1 commit into
developfrom
claude/cool-ritchie-ec195b

Conversation

@daniel-lxs

Copy link
Copy Markdown
Member

What changed

Connecting a hosted model provider persisted the API key without ever authenticating it. The only network call in the save path was model discovery, gated on provider.dynamicModels, which is set on just four providers (github-copilot, litellm, ollama, vllm). Every hosted provider, Anthropic and OpenAI included, contacted nothing at all on save. A typo, a revoked key, or a key pasted into the wrong provider saved cleanly, reported the provider connected, and first surfaced as a task failing at run time, which reads as a Roomote fault rather than a rejected credential. Even for the four dynamic providers the key was committed inside a transaction that closed before discovery ran, and the failure came back as a "couldn't list models" discovery error.

Both save paths, the setup wizard and Models settings, now probe the provider with one bounded authenticated GET before anything is written:

  • Covered: anthropic, openai, google, xai, moonshotai, openrouter, togetherai.
  • Unchanged: providers resolving an operator-supplied endpoint (LiteLLM, Ollama, vLLM, OpenAI-compatible), the OAuth providers, and Bedrock/Azure, whose credential shapes differ.
  • The endpoint and auth-header shape come from the provider's existing INFERENCE_GATEWAY_PROVIDERS descriptor (upstreamBaseUrl + authHeader), so the only new per-provider data is a path. Adding a provider later is a table entry, not a code change.
  • Validation runs before the transaction opens, so a rejected key leaves nothing behind.
  • Tri-state result, matching the Azure DevOps validator: only a rejection from the provider blocks the save. A timeout, a 429, or a 5xx is unknown and lets the save through, so a provider having a bad day cannot wedge settings.
  • Failure copy leads with the provider's own words and names the field: Anthropic rejected the API key (ANTHROPIC_API_KEY), status 401: "invalid x-api-key" Check the value and save it again.

Key resolution is submitted value, then runtime env, then stored value, using || rather than ?? because the settings form posts an empty string for fields already satisfied by a runtime env var. A key missing everywhere skips the probe and still gets the existing required-field error.

Follows the shape of validateTeamsBotCredentials (bounded timeout, save fails with the error attributed to the credential field) and validateAdoToken (tri-state, provider message quoted).

How it was tested

Unit tests for the new module cover the probe URL and auth header per provider, the rejection message, the non-blocking unknown cases, and the skip for endpoint providers; the save-path test asserts nothing is written when the provider rejects the key. 99 tests pass in apps/web/src/trpc/commands/task-models/.

Each of the seven endpoints was also probed live with a bogus key to confirm it exists and rejects:

provider status body
anthropic 401 invalid x-api-key
openai 401 Incorrect API key provided
google 400 API key not valid.
xai 400 Incorrect API key provided.
moonshotai 401 Invalid Authentication
openrouter 401 Missing Authentication header
togetherai 401 Unauthorized

Google and xAI reject with 400 rather than 401, which is why the probe table has a per-provider rejectionStatuses escape hatch, set for those two only. Each probe is a fixed GET with no body, so a 400 there is about the key and not the request. OpenRouter uses /api/v1/key rather than /api/v1/models, which is public and would call any string valid; it was confirmed to actually read the header (three distinct 401 bodies for malformed, well-formed-unknown, and absent).

Not verified: that a valid key returns 200 on each of the seven, which needs real keys.

pnpm lint:fast, pnpm check-types:fast, and pnpm knip are clean. setup-new/launch-lifecycle.test.ts fails 11 tests on a stale local test DB schema, reproduced identically on a stashed tree, unrelated to this change.

Checklist

  • The PR title follows the repo convention
  • This PR is small and scoped to one change
  • pnpm lint and pnpm check-types pass locally
  • I added tests
  • I removed secrets, tokens, private keys, and customer data from code, logs, and screenshots
  • I ran pnpm changeset

Connecting a hosted provider persisted the key without ever calling the
provider: the save path only made a network call for the four providers
with dynamic model discovery, and even those committed the key before
discovery ran. A typo or a revoked key saved cleanly and first surfaced
as a failed task run, attributed to Roomote rather than the credential.

Probe the provider with one bounded authenticated GET before anything is
written, on both the setup wizard and Models settings save paths. The
endpoint and auth header come from the provider's inference-gateway
descriptor, so adding a provider is a table entry. Only a rejection from
the provider blocks the save; a timeout, rate limit, or outage is
reported as unverified and lets the save through.
@roomote-community

roomote-community Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

No code issues found. See task

Reviewed 7a1760f

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.

1 participant