Skip to content

fix(core): register only credential-shaped env names for integrations - #44066

Open
chandlerm923 wants to merge 1 commit into
anomalyco:devfrom
chandlerm923:fix/provider-env-credential-filter
Open

fix(core): register only credential-shaped env names for integrations#44066
chandlerm923 wants to merge 1 commit into
anomalyco:devfrom
chandlerm923:fix/provider-env-credential-filter

Conversation

@chandlerm923

Copy link
Copy Markdown

Issue for this PR

Closes #44065

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

ModelsDevPlugin (packages/core/src/plugin/models-dev.ts) registers every env var
name models.dev lists for a provider, verbatim, as one env auth method. Connection
resolution takes the first set var as the credential. For providers where models.dev
lists a non-secret setup value before the key — cloudflare-workers-ai
(CLOUDFLARE_ACCOUNT_IDCLOUDFLARE_API_KEY), snowflake-cortex, databricks,
neon — the non-key value wins and gets sent as the bearer token, so a standard setup
gets a deterministic 401.

#43077 hit the same bug for google-vertex and fixed it by hardcoding a per-provider
environmentNames() filter on the v2 branch (azure + google-vertex only). This PR
generalizes that instead of adding more per-provider cases: credentialEnvNames()
keeps only names ending in _API_KEY/_TOKEN/_PAT when any of the provider's env
names match that shape, and falls back to the full list otherwise (so providers with
unfamiliar naming still register their env var same as before).

How did you verify your code works?

  • Added a cloudy fixture provider (CLOUDY_ACCOUNT_ID, CLOUDY_API_KEY) to
    packages/core/test/plugin/fixtures/models-dev.json and asserted only the
    _API_KEY name gets registered (packages/core/test/plugin/models-dev.test.ts)
  • bun test test/plugin/models-dev.test.ts — 2 pass
  • bun run typecheck in packages/core — clean
  • bun test (full packages/core suite) has 2 pre-existing failures in
    snapshot.test.ts unrelated to this change — reproduced identically with this
    diff stashed out, so confirmed pre-existing (git/hg sandbox issue in this
    environment, not caused by this PR)

Screenshots / recordings

Not a UI change.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

models.dev lists provider env vars in setup order, so non-secret values
(account id, host, base url) that precede the actual key get registered
as the env auth method and can win connection resolution's first-set-wins
pick. Filter registered names to *_API_KEY/*_TOKEN/*_PAT when any match,
falling back to the full list otherwise.

Fixes anomalyco#44065
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

  1. packages/core/src/plugin/models-dev.ts:19 — The credential-shape heuristic can drop legitimate credentials whose names don't end in _API_KEY/_TOKEN/_PAT (e.g. SESSION_COOKIE, SECRET_KEY, _SERVICE_ACCOUNT) whenever at least one other name matches — the full-list fallback only fires when nothing matches. Why it matters: such providers would silently lose their env-based auth method and connection resolution fails at runtime. Suggestion: keep a small allowlist of known non-suffix credential names (or a models.dev metadata flag marking the actual credential), and/or emit a warning when filtering removes all plausible secrets.

  2. packages/core/src/plugin/models-dev.ts:14 — Suffix matching is case-sensitive, but a handful of providers in models.dev list lowercase env names. Why it matters: those credentials get filtered out despite being credential-shaped. Suggestion: normalize before comparing, e.g. name.toUpperCase().endsWith(suffix).

  3. packages/core/src/plugin/models-dev.ts:150 — When multiple credential-shaped names exist (_API_KEY and _TOKEN), selection still depends on the "first set var wins" setup-order assumption; the fix narrows candidates but doesn't guarantee the primary key is picked. Why it matters: a secondary/refresh token could be sent as the bearer key. Suggestion: rank matches explicitly (prefer _API_KEY > _TOKEN > _PAT) instead of relying on array order.

  4. packages/core/test/plugin/models-dev.test.ts:161 — Coverage only exercises the filtered path (Cloudy). Why it matters: regressions in the two subtle paths would go unnoticed. Suggestion: add cases asserting (a) the fallback returns the full list unchanged when no name is credential-shaped, and (b) original candidate ordering is preserved.

— automated review (ox-alpha, round2)

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.

V2 runner sends a non-credential env var as the bearer API key for env-configured providers

2 participants