Skip to content

fix(mcp): stamp oauth2_flow=authorization_code when persisting a DCR client registration - #32283

Merged
tin-berri merged 1 commit into
litellm_internal_stagingfrom
litellm_mcp_dcr_oauth2_flow_stamp
Jul 6, 2026
Merged

fix(mcp): stamp oauth2_flow=authorization_code when persisting a DCR client registration#32283
tin-berri merged 1 commit into
litellm_internal_stagingfrom
litellm_mcp_dcr_oauth2_flow_stamp

Conversation

@tin-berri

@tin-berri tin-berri commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Relevant issues

First of a short sequence that persists oauth2_flow at every write site so the legacy field-shape inference in _resolve_oauth2_flow can eventually be deleted. Follow-ups: write-side persistence for UI/REST creates and edits, an app-level backfill for legacy null rows plus resolved-flow reads for the dashboard, then deletion of the inference

Behavior

When an admin completes the interactive OAuth/DCR flow for a server, the row is permanently marked oauth2_flow=authorization_code alongside the registered client_id, so a DCR-registered interactive server can no longer be mistaken for M2M when endpoint discovery fails at registry load

Linear ticket

N/A

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have added meaningful tests
  • My PR passes all CI/CD checks (e.g., lint, format, unit tests)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have requested a Greptile review by commenting @greptileai and received a Confidence Score of at least 4/5 before requesting a maintainer review

Delays in PR merge?

If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).

Screenshots / Proof of Fix

Reproduction on a live proxy backed by Postgres (master key sk-1234)

  1. Start the proxy
python litellm/proxy/proxy_cli.py --config litellm/proxy/dev_config.yaml --detailed_debug --reload --use_v2_migration_resolver 2>&1 | tee litellm.log
  1. Go to http://localhost:4000/ui/?page=mcp-servers, add an interactive OAuth server that supports dynamic client registration (a Linear MCP works: url https://mcp.linear.app/mcp, transport Streamable HTTP, auth type OAuth, leave client id/secret blank so the gateway runs DCR), then open the server's tools tab and complete the Authorize browser flow as a proxy admin

  2. Read the row back and confirm the flow was stamped alongside the registered client

curl -s http://localhost:4000/v1/mcp/server -H "Authorization: Bearer sk-1234" \
  | jq '.[] | select(.alias=="linear") | {auth_type, oauth2_flow, token_url}'

Before this change oauth2_flow stays null after the authorize flow; after it the read-back shows "oauth2_flow": "authorization_code"

Type

🐛 Bug Fix

Changes

Only the gateway-managed interactive flow reaches _persist_dcr_client_registration (the public /register routes never pass persist_credentials, so client-driven and delegate registrations never persist), which means every row that persist writes is an authorization_code server by definition. The flow was not recorded, leaving the row as client credentials + token_url with no persisted authorization_url and a null oauth2_flow: exactly the shape the legacy M2M inference in _resolve_oauth2_flow matches. The row normally classifies correctly anyway because endpoint discovery backfills authorization_url in memory before the inference runs, but on any transient discovery failure at registry build the server flips to client_credentials for that load and per-user traffic routes to the M2M path

This stamps oauth2_flow="authorization_code" in the same partial update that persists the registered client_id, so a DCR-registered interactive server classifies correctly without depending on discovery succeeding. Rows persisted before this change stay null until the follow-up backfill; they keep today's discovery-dependent behavior, no worse

The existing regression test for the DCR persist now also asserts the stamp, so removing it fails the test


Note

Medium Risk
Touches MCP OAuth persistence and flow classification; wrong values could misroute token/egress behavior, but the change is a single explicit field on an already-interactive-only code path with test coverage.

Overview
When the gateway persists a dynamic client registration (RFC 7591) result for an interactive MCP OAuth server, it now also sets oauth2_flow="authorization_code" on the same UpdateMCPServerRequest as the registered client_id / secret / token_url.

Previously that persist left oauth2_flow null while the row looked like legacy M2M (credentials + token_url, no stored authorization_url). _resolve_oauth2_flow could then infer client_credentials if endpoint discovery failed during registry build, sending per-user traffic down the wrong OAuth path.

The DCR persist regression test now asserts oauth2_flow == "authorization_code" alongside the existing credential checks.

Reviewed by Cursor Bugbot for commit 15a840e. Bugbot is set up for automated code reviews on this repo. Configure here.

…client registration

Only the gateway-managed interactive flow reaches this persist (the public /register
routes never pass persist_credentials), so the row it writes is authorization_code by
definition. It was not recorded, which left the row as client creds + token_url with
no persisted authorization_url and a null oauth2_flow: exactly the shape the legacy
M2M inference in _resolve_oauth2_flow matches. The row normally survives because
endpoint discovery backfills authorization_url in memory before the inference runs,
but on any transient discovery failure at registry build the server flips to
client_credentials for that load, routing per-user traffic to the M2M path

Stamping the flow at the write site makes the classification explicit and permanent,
so a DCR-registered interactive server no longer depends on discovery succeeding to
classify correctly. First step of persisting oauth2_flow at every write site so the
legacy inference can eventually be deleted
@tin-berri

Copy link
Copy Markdown
Contributor Author

@greptileai

@greptile-apps

greptile-apps Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a classification bug where a DCR-registered interactive OAuth server could be misidentified as a client-credentials server when endpoint discovery fails at registry build time. The fix is a one-line addition that stamps oauth2_flow="authorization_code" in the partial update written by _persist_dcr_client_registration.

  • Production fix: oauth2_flow="authorization_code" is unconditionally added to the UpdateMCPServerRequest in _persist_dcr_client_registration; the field already exists on the model and the value is definitionally correct for every row this function writes (only the interactive gateway flow reaches this path).
  • Test update: The existing DCR persist regression test gains one new assertion (update_data.oauth2_flow == "authorization_code") and an expanded docstring explaining the failure scenario; no mock behavior is changed or weakened.

Confidence Score: 5/5

Safe to merge — a minimal, targeted write to a single field in an isolated persist function with no impact on other code paths.

The change is a one-line addition to an already-isolated persist helper that is only reachable from the interactive gateway DCR flow. The field being set (oauth2_flow) already exists on UpdateMCPServerRequest, the literal value "authorization_code" is the only value valid for this code path by construction, and the accompanying test strengthens rather than relaxes existing coverage. Rows written before this change are unaffected; they retain their current discovery-dependent classification behavior until a separate backfill is applied.

No files require special attention.

Important Files Changed

Filename Overview
litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py Single-line addition stamps oauth2_flow="authorization_code" in the UpdateMCPServerRequest inside _persist_dcr_client_registration; the field is already declared on that model and the value is always correct for this code path.
tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py Docstring update and one new assertion (oauth2_flow == "authorization_code") added to the existing DCR persist regression test; strengthens coverage without modifying any mock behavior.

Reviews (1): Last reviewed commit: "fix(mcp): stamp oauth2_flow=authorizatio..." | Re-trigger Greptile

@greptile-apps

greptile-apps Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a silent misclassification of DCR-registered MCP servers by stamping oauth2_flow="authorization_code" at persist time, so the row carries an explicit flow regardless of whether OAuth endpoint discovery succeeds later.

  • _persist_dcr_client_registration now sets oauth2_flow="authorization_code" in the UpdateMCPServerRequest; the field is a valid Optional[Literal["client_credentials", "authorization_code"]] on the type and the hardcoded value is correct since only the gateway-managed interactive flow reaches this code path.
  • Regression test is strengthened with an additional assertion verifying the new field, and the docstring is updated to explain the failure mode being guarded against.

Confidence Score: 5/5

Safe to merge — a one-line addition to a single non-critical persist helper with no path changes and a strengthened regression test.

The change touches exactly one call site, the field already exists on the request type with the correct Literal constraint, and the only callers of the persist helper are already gated on persist_credentials=True which is only set by the interactive gateway flow. The test update adds an assertion rather than removing one.

No files require special attention.

Important Files Changed

Filename Overview
litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py Adds oauth2_flow="authorization_code" to the UpdateMCPServerRequest in _persist_dcr_client_registration; the field is a valid Literal on the request type and the hardcoded value is correct because only the gateway-managed interactive flow reaches this persist path.
tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py Extends an existing regression test with a new assert update_data.oauth2_flow == "authorization_code" assertion and an updated docstring; strengthens rather than weakens coverage.

Reviews (2): Last reviewed commit: "fix(mcp): stamp oauth2_flow=authorizatio..." | Re-trigger Greptile

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@tin-berri

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 15a840e. Configure here.

@mateo-berri mateo-berri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; thanks!

@tin-berri
tin-berri merged commit f5ea72b into litellm_internal_staging Jul 6, 2026
127 checks passed
@tin-berri
tin-berri deleted the litellm_mcp_dcr_oauth2_flow_stamp branch July 6, 2026 22:03
tin-berri added a commit that referenced this pull request Jul 6, 2026
Rows created before the write-side stamps carry a null oauth2_flow and rely on
read-time field-shape inference, which cannot tell a DCR-registered interactive
server (client creds + token_url, no persisted authorization_url) from an M2M
server unless endpoint discovery succeeds first; on a transient discovery failure
those servers flip to client_credentials for that registry load

The backfill classifies each null oauth2 row once, at rest, ordered by signal
strength: per-user token rows (only the interactive flow mints them, so this is
definitive and catches the DCR-trap cohort), then a persisted authorization_url,
then a persisted registration_url (DCR implies interactive; this covers
registered-but-never-signed-in rows), then the M2M credential shape mirroring the
legacy inference, else the interactive default that matches how
needs_user_oauth_token treats a null flow. Every stamp is logged with the rule
that fired and written with updated_by=oauth2_flow_backfill for auditability

Runs in _init_mcp_servers_in_db before the registry load so the first build of
the boot classifies from the column, is isolated so a failure cannot block server
loading, and is idempotent: a healed fleet exits after one indexed query. This
unblocks deleting the read-time inference for DB rows in the follow-up

Third step of the oauth2_flow persistence sequence, after #32283 and #32288
tin-berri added a commit that referenced this pull request Jul 7, 2026
…s config-only plus a logged backstop

With every DB write site stamping oauth2_flow (#32283, #32288) and the startup
backfill healing legacy null rows, the DB build no longer needs to re-derive the
flow from field shape. build_mcp_server_from_table now reads the column verbatim
via _explicit_oauth2_flow: unknown or null values resolve to None, which
needs_user_oauth_token already treats as interactive, so an unstamped row degrades
to the safe default instead of guessing M2M from a shape that a DCR-registered
interactive server shares whenever discovery is down

Field-shape inference survives in exactly two places. config.yaml-loaded servers
keep it at load time: they are rebuilt from the config on every boot, so there is
no row to backfill and load-time resolution is their write-time stamp. And the
request-time backstop in _get_allowed_mcp_servers keeps a not-yet-backfilled M2M
row blocking caller Authorization forwarding (the P1 property); it now logs a
warning whenever it actually fires, which is the fire-rate signal for deleting it
once deployments have booted past the backfill

Regression tests pin that the DB build does not infer M2M from the credential
shape and reads an explicit column value verbatim

Fourth step of the oauth2_flow persistence sequence, stacked on the backfill
tin-berri added a commit that referenced this pull request Jul 7, 2026
Rows created before the write-side stamps carry a null oauth2_flow and rely on
read-time field-shape inference, which cannot tell a DCR-registered interactive
server (client creds + token_url, no persisted authorization_url) from an M2M
server unless endpoint discovery succeeds first; on a transient discovery failure
those servers flip to client_credentials for that registry load

The backfill classifies each null oauth2 row once, at rest, ordered by signal
strength: per-user token rows (only the interactive flow mints them, so this is
definitive and catches the DCR-trap cohort), then a persisted authorization_url,
then a persisted registration_url (DCR implies interactive; this covers
registered-but-never-signed-in rows), then the M2M credential shape mirroring the
legacy inference, else the interactive default that matches how
needs_user_oauth_token treats a null flow. Every stamp is logged with the rule
that fired and written with updated_by=oauth2_flow_backfill for auditability

Runs in _init_mcp_servers_in_db before the registry load so the first build of
the boot classifies from the column, is isolated so a failure cannot block server
loading, and is idempotent: a healed fleet exits after one indexed query. This
unblocks deleting the read-time inference for DB rows in the follow-up

Third step of the oauth2_flow persistence sequence, after #32283 and #32288
tin-berri added a commit that referenced this pull request Jul 7, 2026
…s config-only plus a logged backstop

With every DB write site stamping oauth2_flow (#32283, #32288) and the startup
backfill healing legacy null rows, the DB build no longer needs to re-derive the
flow from field shape. build_mcp_server_from_table now reads the column verbatim
via _explicit_oauth2_flow: unknown or null values resolve to None, which
needs_user_oauth_token already treats as interactive, so an unstamped row degrades
to the safe default instead of guessing M2M from a shape that a DCR-registered
interactive server shares whenever discovery is down

Field-shape inference survives in exactly two places. config.yaml-loaded servers
keep it at load time: they are rebuilt from the config on every boot, so there is
no row to backfill and load-time resolution is their write-time stamp. And the
request-time backstop in _get_allowed_mcp_servers keeps a not-yet-backfilled M2M
row blocking caller Authorization forwarding (the P1 property); it now logs a
warning whenever it actually fires, which is the fire-rate signal for deleting it
once deployments have booted past the backfill

Regression tests pin that the DB build does not infer M2M from the credential
shape and reads an explicit column value verbatim

Fourth step of the oauth2_flow persistence sequence, stacked on the backfill
tin-berri added a commit that referenced this pull request Jul 7, 2026
…s config-only plus a logged backstop

With every DB write site stamping oauth2_flow (#32283, #32288) and the startup
backfill healing legacy null rows, the DB build no longer needs to re-derive the
flow from field shape. build_mcp_server_from_table now reads the column verbatim
via _explicit_oauth2_flow: unknown or null values resolve to None, which
needs_user_oauth_token already treats as interactive, so an unstamped row degrades
to the safe default instead of guessing M2M from a shape that a DCR-registered
interactive server shares whenever discovery is down

Field-shape inference survives in exactly two places. config.yaml-loaded servers
keep it at load time: they are rebuilt from the config on every boot, so there is
no row to backfill and load-time resolution is their write-time stamp. And the
request-time backstop in _get_allowed_mcp_servers keeps a not-yet-backfilled M2M
row blocking caller Authorization forwarding (the P1 property); it now logs a
warning whenever it actually fires, which is the fire-rate signal for deleting it
once deployments have booted past the backfill

Regression tests pin that the DB build does not infer M2M from the credential
shape and reads an explicit column value verbatim

Fourth step of the oauth2_flow persistence sequence, stacked on the backfill
tin-berri added a commit that referenced this pull request Jul 7, 2026
…32290)

* feat(mcp): startup backfill stamping oauth2_flow on legacy null rows

Rows created before the write-side stamps carry a null oauth2_flow and rely on
read-time field-shape inference, which cannot tell a DCR-registered interactive
server (client creds + token_url, no persisted authorization_url) from an M2M
server unless endpoint discovery succeeds first; on a transient discovery failure
those servers flip to client_credentials for that registry load

The backfill classifies each null oauth2 row once, at rest, ordered by signal
strength: per-user token rows (only the interactive flow mints them, so this is
definitive and catches the DCR-trap cohort), then a persisted authorization_url,
then a persisted registration_url (DCR implies interactive; this covers
registered-but-never-signed-in rows), then the M2M credential shape mirroring the
legacy inference, else the interactive default that matches how
needs_user_oauth_token treats a null flow. Every stamp is logged with the rule
that fired and written with updated_by=oauth2_flow_backfill for auditability

Runs in _init_mcp_servers_in_db before the registry load so the first build of
the boot classifies from the column, is isolated so a failure cannot block server
loading, and is idempotent: a healed fleet exits after one indexed query. This
unblocks deleting the read-time inference for DB rows in the follow-up

Third step of the oauth2_flow persistence sequence, after #32283 and #32288

* fix(mcp): backfill leaves the ambiguous M2M shape unstamped instead of guessing client_credentials

The credential shape (client_id + client_secret + token_url, no interactive signal) is
shared by real M2M servers and DCR-registered interactive servers nobody has signed
into: the DCR persist writes creds and token_url but not authorization_url or
registration_url. Stamping client_credentials from that shape permanently mislabeled
the interactive cohort, and once explicit the value is authoritative, so per-user
traffic would run on the proxy's stored client credential with no discovery rescue
and no backstop (it only guards null rows)

The backfill now stamps only what it can prove. Interactive signals keep stamping
authorization_code; the ambiguous shape is left null with an actionable warning naming
the server and the fix (set oauth2_flow via the dashboard or PUT /v1/mcp/server). A
true M2M row keeps working per-request through the security backstop while the warning
nags; an interactive row keeps its Authorize button (null renders interactive), and one
completed sign-in creates the per-user token that stamps it authorization_code at the
next boot. Mirrors the config-level rule: M2M is asserted by a human, never guessed

Raised by review on the PR

* perf(mcp): batch the backfill stamps into one update_many per flow value

The per-row update loop issued one DB round-trip per legacy row at startup; rows
sharing a stamped value now go out as a single update_many, so the DB cost is
constant in fleet size. Per-row logging keeps the rule that fired for each server

Raised by review on the PR

* fix(mcp): backfill stamps only rows still null at write time and counts only real OAuth token rows as sign-in proof

Two review findings. The batched update_many matched on server_id alone, so an
explicit oauth2_flow set between the backfill's read and its write (an admin PUT or
a sign-in's DCR stamp landing in the boot window) would be overwritten with the
inferred value; the where clause now also requires oauth2_flow to still be null, so
an explicit value can never be clobbered under any interleaving

And the per_user_tokens rule counted any LiteLLM_MCPUserCredentials row as proof of
an interactive sign-in, but that table doubles as BYOK storage for user-supplied API
keys; a BYOK-flavored row would have stamped an M2M-shaped server authorization_code.
The rule now counts only rows whose payload decodes as a type oauth2 token via the
existing _decode_oauth_payload discriminator, so bare keys, undecodable rows, and
stale leftovers from a BYOK-to-oauth2 auth switch prove nothing

Raised by review on the PR
tin-berri added a commit that referenced this pull request Jul 7, 2026
…s config-only plus a logged backstop

With every DB write site stamping oauth2_flow (#32283, #32288) and the startup
backfill healing legacy null rows, the DB build no longer needs to re-derive the
flow from field shape. build_mcp_server_from_table now reads the column verbatim
via _explicit_oauth2_flow: unknown or null values resolve to None, which
needs_user_oauth_token already treats as interactive, so an unstamped row degrades
to the safe default instead of guessing M2M from a shape that a DCR-registered
interactive server shares whenever discovery is down

Field-shape inference survives in exactly two places. config.yaml-loaded servers
keep it at load time: they are rebuilt from the config on every boot, so there is
no row to backfill and load-time resolution is their write-time stamp. And the
request-time backstop in _get_allowed_mcp_servers keeps a not-yet-backfilled M2M
row blocking caller Authorization forwarding (the P1 property); it now logs a
warning whenever it actually fires, which is the fire-rate signal for deleting it
once deployments have booted past the backfill

Regression tests pin that the DB build does not infer M2M from the credential
shape and reads an explicit column value verbatim

Fourth step of the oauth2_flow persistence sequence, stacked on the backfill
tin-berri added a commit that referenced this pull request Jul 7, 2026
…nfig; inference reduced to the request-time backstop (#32292)

* refactor(mcp): read oauth2_flow verbatim from DB rows; inference stays config-only plus a logged backstop

With every DB write site stamping oauth2_flow (#32283, #32288) and the startup
backfill healing legacy null rows, the DB build no longer needs to re-derive the
flow from field shape. build_mcp_server_from_table now reads the column verbatim
via _explicit_oauth2_flow: unknown or null values resolve to None, which
needs_user_oauth_token already treats as interactive, so an unstamped row degrades
to the safe default instead of guessing M2M from a shape that a DCR-registered
interactive server shares whenever discovery is down

Field-shape inference survives in exactly two places. config.yaml-loaded servers
keep it at load time: they are rebuilt from the config on every boot, so there is
no row to backfill and load-time resolution is their write-time stamp. And the
request-time backstop in _get_allowed_mcp_servers keeps a not-yet-backfilled M2M
row blocking caller Authorization forwarding (the P1 property); it now logs a
warning whenever it actually fires, which is the fire-rate signal for deleting it
once deployments have booted past the backfill

Regression tests pin that the DB build does not infer M2M from the credential
shape and reads an explicit column value verbatim

Fourth step of the oauth2_flow persistence sequence, stacked on the backfill

* feat(mcp): deprecation warning when config-level M2M is inferred rather than declared

A config.yaml oauth2 server whose credential shape decides client_credentials without
an explicit oauth2_flow now logs a warning at load pointing the admin at the explicit
declaration. First rung of the deprecation ladder: the docs make oauth2_flow the
recommended path, the warning surfaces configs still relying on inference, and a
future breaking release can turn it into a config validation error, at which point
config-level shape inference dies entirely. Interactive omissions stay silent since
the default matches inference there and nothing load-bearing is being guessed

* feat(mcp)!: require explicit oauth2_flow for config-defined oauth2 servers

A config.yaml server with auth_type oauth2 must now declare its flow; the load
raises a config validation error naming both values and what each means:
oauth2_flow: client_credentials for machine-to-machine (the proxy mints a shared
token at token_url using client_id/client_secret) or
oauth2_flow: authorization_code for interactive (per-user tokens via browser
sign-in, including delegate_auth_to_upstream)

This replaces the load-time shape inference for config servers entirely. The
credential shape is genuinely ambiguous (a DCR-registered interactive server
carries client creds + token_url with no authorization_url, identical to M2M),
so the config asserts the answer instead of the proxy guessing it. With this,
field-shape inference survives in exactly one place: the request-time security
backstop, which is telemetry-gated for deletion

BREAKING CHANGE: config-defined oauth2 MCP servers without oauth2_flow fail
proxy startup with the error above. Add the one line to the server block; the
error text says exactly which value to pick

* test(mcp): pin the verbatim read for authorization_code alongside client_credentials

Raised by review on the PR

* fix(mcp): fail closed on the anonymous delegate gate for unstamped M2M-shaped servers

Reading oauth2_flow verbatim (this PR) changed has_client_credentials from True to
False for a legacy null-flow row that still carries the M2M credential shape. That
value is what the anonymous upstream-delegate gate checks before skipping LiteLLM
auth entirely, so an M2M-shaped delegate server that was never stamped would newly
pass the gate: an unauthenticated caller could get it selected and then list/read
upstream data using the client credentials the request-time backstop re-infers,
running as LiteLLM's service account. This reopens the hole the gate's existing
'never delegate for M2M' guard was written to close

The gate now resolves the flow (column first, shape fallback) instead of reading the
bare column, mirroring the request-time backstop in _get_allowed_mcp_servers: both
fail closed on the ambiguous M2M shape and are removed together once no null rows
remain. A pure-PKCE delegate server (no stored credentials) resolves to a non-M2M
flow and keeps its bypass, so the common delegate case is unaffected

Tests: an unstamped M2M-shaped delegate server is denied the bypass (mutation-checked
against the bare-column regression), and a pure-PKCE delegate server still bypasses

Raised by review on the PR

* fix(mcp): centralize the request-time oauth2_flow backstop across every security site

Reading oauth2_flow verbatim made has_client_credentials unreliable for legacy null
rows, and the backstop that compensates was applied at only one reader. Review found
three more consequences of that per-site approach:

- the anonymous-delegate allowlist in get_allowed_mcp_servers read the bare column, so
  an unstamped M2M-shape delegate server was surfaced to anonymous callers (High)
- call_mcp_tool resolved allowed ids into MCPServer objects without the backstop, so a
  null-flow M2M-shape row kept has_client_credentials false on tool execution during a
  backfill gap, though the listing path was covered (High)
- the request-time warning claimed the startup backfill would stamp the row next boot,
  but the backfill deliberately leaves the ambiguous M2M shape unstamped (Low)

Rather than patch each site, introduce two helpers on MCPServerManager that are the
single choke point for request-time resolution: effective_oauth2_flow(server) for the
enum/boolean decisions (allowlist filter, anonymous-delegate gate) and
resolve_oauth2_flow_for_request(server) for the egress object copy (listing and tool
call). Both fail closed on the M2M shape and leave stamped rows and pure-PKCE rows
untouched. The gate now shares effective_oauth2_flow instead of its inline resolution,
and the corrected warning lives once inside resolve_oauth2_flow_for_request, so deleting
the whole transitional layer later is a single-site change.

Tests: helper unit coverage (stamped verbatim, null M2M-shape resolves, pure-PKCE stays
None, stamped/pure-PKCE return the same object, corrected warning text), the anonymous
allowlist excludes an unstamped M2M-shape delegate server, and the call path resolves
the flow like the listing path. The two security-integration tests are mutation-checked
against the bare-column regression.

Raised by review on the PR
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