canonical(oauth): app client secret duplicated into every user credential bundle; OAuthProvider too thin for real providers

Part of #13623. Two related shape problems in the OAuth layer.

1. The OAuth app client secret is copied into every user's credential bundle

knowledge/connectors/credential_store.py:294-315, _oauth_bundle():

return {
    "provider": provider,
    "access_token": ...,
    "refresh_token": ...,
    "client_id": client_id,          # line 312
    "client_secret": client_secret,  # line 313
    "token_url": token_url,
}

client_id / client_secret identify the OAuth application — one per provider, operator-owned. They are duplicated into every per-user credential secret so that get_access_token() can refresh without re-reading provider config (the docstring at line 200-205 says exactly this, and it is a reasonable motivation).

The costs:

Rotating the app secret means rewriting every user's stored credential. There is no code path that does this today, so a rotation would silently strand every existing connection: refresh keeps using the old secret and starts failing.
The app secret's blast radius becomes every per-user credential row rather than one config entry.
The same value is encrypted N times under N secrets, so revocation and inventory ("where does this client secret live?") have no single answer.

oauth_flow.py:96 resolve_client_credentials(provider) already resolves app credentials from settings — the refresh path could call it instead of carrying a copy.

Fix: separate the operator-owned OAuth client config from the user-owned credential, and have refresh resolve the client config at use time — the external implementation reviewed for #13623 splits these into two services for exactly this reason. Migration must rewrite existing bundles, and needs the same "never lose data" care as any secrets migration.

2. `OAuthProvider` is thinner than real providers require

knowledge/connectors/oauth_flow.py:39-50 has six fields: name, authorize_url, token_url, client_id_setting, client_secret_setting, default_scopes, extra_authorize_params. PKCE and state are handled globally (generate_pkce line 117, generate_state line 125).

Real providers deviate in ways that currently have nowhere to go except a per-provider code branch. The external implementation reviewed for #13623 carries a declarative auth definition covering the deviations it hit across roughly a thousand live integrations — a ready-made list of what breaks first:

a separate refresh URL distinct from the token URL
tokenEndpointAuthMethod — client_secret_basic vs client_secret_post vs none
token request encoding: form vs JSON
per-grant-type field renames — the authorization-code exchange and the refresh call using different parameter names
non-standard token response envelopes ({code, data: {...}, message} rather than a bare token object)
a non-space scope separator (comma)
extra operator-supplied client-config fields (e.g. a tenant or region required before authorization can start)

Adding these as declarative fields now is cheap. Retrofitting them later, after per-provider if provider == "x" branches have accumulated in oauth_flow.py, is not — and even that implementation carries one such hardcoded provider branch, as evidence of what happens when the declarative model does not cover a case.

Scope

Split operator OAuth client config from user credential; refresh resolves client config at use time
Migration rewrites existing bundles without losing tokens; app-secret rotation becomes a supported operation with a test
Extend OAuthProvider with the deviation fields above, all optional and defaulting to current behaviour
Existing providers keep byte-identical behaviour after the change

chore: merge auto-fix formatting (#13630)

fix(oauth): resolve the app client secret at refresh time, not from t…

Partial delivery landed: PR #13757 (squashed as f76e780) resolves the app client secret at refresh time instead of reading the copy embedded in each user's credential bundle, with an unknown/unconfigured-provider fallback so no migration is required.

Remaining, so this stays open:

Migration rewrites existing bundles without losing tokens
Extend OAuthProvider with the deviation fields, all optional
Existing providers keep byte-identical behaviour (covered only for the refresh path so far)

Closure gate 3 — partial delivery never closes.

Landed — PR #13757 (`f76e78005`)

The refresh path no longer trusts the client secret stored in each user's credential bundle. _resolve_app_credentials reads the operator's current OAuth app credentials at refresh time and falls back to the stored copy only when the provider is unknown or lookup fails, so rotating the app secret no longer silently breaks every existing connector.

Verified in the base branch:

$ git show origin/Dev_new_gui:autobot-backend/knowledge/connectors/credential_store.py | grep -c _resolve_app_credentials
2

Still open on this issue — not delivered

Migration of already-stored secrets. Existing bundles keep their duplicated copy. It is now unused on the refresh path, but it is still a secret at rest in a second place. Removing it is a data migration.
Declarative OAuthProvider fields. The provider model is still thinner than real providers need (per-provider scopes, audience, extra token params).

Both are owner decisions rather than mechanical follow-ups, so the issue stays open rather than being closed on partial delivery.

Session ending (weekly token limit, owner call to drop concurrency). Not started -- no branch checks, no code reading done on this issue yet. Handing off as-is so the next session doesn't assume any progress here.

Context that may help: this issue was in a batch of 15 open security issues with unlanded branch work, dispatched by autobot-ai-df (queue coordinator) as part of a repo-wide stale-branch audit. Before touching any assigned branch, verify supersession first (compare each touched file's blob at branch tip against origin/main -- several branches in this same batch turned out to be stale pre-merge duplicates of already-merged PRs). Also check autobot-ai-df's trap.json scan (revival hazard: a branch whose removed lines cite an issue number lower than a later fix to the same file) before reviving anything -- this issue may or may not be on it, worth checking fresh.

Part 1 code is on the fix branch, not pushed yet. A note on main: #13757 already made refresh prefer the configured app credentials over the copy stored in each user's credential.

New user OAuth bundles no longer carry the app's client_id/client_secret.
Refresh resolves them from provider config through resolve_client_credentials.
Legacy bundles still refresh. When config yields nothing, refresh falls back to the stored copy and logs a WARNING (no values). With neither source, it raises a clear error.
Tests:
- the rotation test: a legacy bundle uses the config secret, not its stale copy;
- the fallback warning, with no secret in the logs;
- no copy in new bundles.
A guard forbids a user bundle literal that holds both access_token and client_secret.
Visible behaviour change: a bundle created after this change, on a provider whose config is later removed, fails refresh instead of using a stored copy. That is the intended single source.

Proposed migration: human approval required (#17038). No agent runs it. Strip the legacy app-credential copies from existing connector_oauth_token secrets:

Preconditions: this change is deployed, and every provider those bundles reference has its operator OAuth app configured. Otherwise stripping breaks refresh. The WARNING log lines name the providers still relying on copies.
Back up the bundles (encrypted export or DB snapshot). Dry-run first, printing counts only (bundles per provider), no values.
Strip: per secret, through the secrets service, remove only client_id and client_secret and leave tokens, expiry, scope, provider and token_url untouched. Re-mirror to the vault if the mirror is enabled.
Verify:
- a SHA-256 of {access_token, refresh_token, provider, token_url} per secret matches before and after;
- one forced refresh per provider succeeds;
- no "legacy credential bundle" WARNING remains;
- a count of bundles still holding client_secret is 0.
Rollback: restore the backup. The fallback still handles restored legacy bundles.

Part 2 (declarative OAuthProvider fields) isn't started; it's a feature, not part of this de-duplication.

Owner decision (2026-10-10): the strip migration is approved, to run through the #17038 approval queue exactly as proposed: preconditions checked, backup, dry run with counts only, strip only client_id/client_secret, token-hash comparison before and after, a forced refresh per provider, rollback by restore.

fix(connectors): OAuth bundle strip takes the refresh lease per bundl…

fix(connectors): the OAuth bundle strip refuses to execute without a …

fix(connectors): the OAuth bundle strip mirrors inside the refresh le…

Navigation Menu

Repository navigation

canonical(oauth): app client secret duplicated into every user credential bundle; OAuthProvider too thin for real providers #13630

1. The OAuth app client secret is copied into every user's credential bundle

2. `OAuthProvider` is thinner than real providers require

Scope

Landed — PR #13757 (`f76e78005`)

Still open on this issue — not delivered

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

canonical(oauth): app client secret duplicated into every user credential bundle; OAuthProvider too thin for real providers #13630

Description

1. The OAuth app client secret is copied into every user's credential bundle

2. OAuthProvider is thinner than real providers require

Scope

Activity

mrveiss commented on Aug 8, 2026

mrveiss commented on Aug 10, 2026

Landed — PR #13757 (f76e78005)

Still open on this issue — not delivered

mrveiss commented on Sep 20, 2026

mrveiss commented on Oct 10, 2026

mrveiss commented on Oct 10, 2026

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

2. `OAuthProvider` is thinner than real providers require

Landed — PR #13757 (`f76e78005`)