Skip to content

Send somebody back to Settings when the vendor cannot be reached at all - #345

Open
beardthelion wants to merge 2 commits into
CopilotKit:mainfrom
beardthelion:fix/oauth-callback-unreachable-vendor
Open

Send somebody back to Settings when the vendor cannot be reached at all#345
beardthelion wants to merge 2 commits into
CopilotKit:mainfrom
beardthelion:fix/oauth-callback-unreachable-vendor

Conversation

@beardthelion

Copy link
Copy Markdown
Contributor

What this changes

GET /api/plugins/oauth/callback says of itself that "every failure ends the same way: back at
Settings with a word about what happened, and nothing written". One failure did not. Redeeming the
authorization code turns a refusal into null by asking !response.ok, and that question needs a
response to ask. There is none when the connection is refused, the name does not resolve, TLS will
not agree, or the fifteen-second AbortSignal.timeout fires. The rejection went straight through a
function whose whole contract is to refuse quietly, nothing above it catches anything, and
server/src installs no app.onError, so somebody who had just consented at the vendor got a bare
500 with no Location instead of the page that would have told them what happened.

registerDynamicClient had the same hole against the same promise one step earlier in the flow, so
it is fixed alongside. Neither route needed changing: both already do the right thing with null,
and both were already tested doing it.

Three things travelled with the fix.

Caught is not the same as unnoticed. Until now these rejections reached Hono's default handler,
which prints the error on its way to the 500. Buying the redirect by dropping that would have made a
vendor outage look exactly like nobody trying to connect. Both paths now log which vendor and why.
The person still sees the one sentence every other refusal produces, which is deliberate on an
endpoint that tells an unauthenticated caller nothing about how far it got.

A malformed endpoint is our fault, not theirs. fetch refuses an unusable URL by throwing the
same kind of error a refused connection does, so the new catch would have read the two as the same
thing. They are checked apart before the request is attempted: same refusal to the person, a
different line in the log, and the catch left covering only the transport.

The last thing that can fail got the same answer as the rest. Writing the grant to the vault was
still unguarded, which is the identical shape one step later, on the one path where somebody has
most reason to think it worked. It now ends at Settings too. The refresh token is not logged.

The 502 an administrator sees when Connect produces no client now says the vendor "would not
register this deployment, or could not be reached", because after this change both states reach it
and there is one thing to do about either.

Where it runs

  • New state that outlives a request? None. Both changed functions are stateless over one
    request, and the new code paths only decide what to return and what to print.
  • What happens on the second replica? The same thing. Every added branch is a pure function
    of that request and the vendor's answer to it, with nothing read or written between processes.
  • Anything serialised? Nothing new. The one place this touches serialisation is
    ensureOAuthClient, which already holds its advisory lock across the registration call. A
    transport failure now returns null inside that lock instead of throwing out of it, so the
    transaction rolls back and commits no client either way.
  • Anything fanned out to a browser? No. The callback answers the one browser it redirects.
  • New listener, port, or schedule? None.

Boundary and audit

  • Every acting call still goes through the gateway. This touches none: the OAuth connect flow is
    a person authorising their own account, not a Bot acting.
  • New refusals and new failures each write a row. The audit rows here are unchanged, because the
    new paths all end before anything is stored, and a trail row for a connection that was never
    made would claim something that did not happen. What they do now write is a log line each, so
    a failure that is deliberately invisible to the person is still visible to whoever runs the
    deployment.
  • Nothing new is trusted from the client. Both endpoint URLs are frozen catalogue literals, the
    person's identity still comes from the sealed state rather than the request, and the new
    validation narrows what is accepted rather than widening it.

Changelog

  • A line in CHANGELOG.md under Unreleased.

Proof

CI does not run on this branch, so this is what was run locally and what came back.

The bug, before the fix. Driving the real route with an unreachable vendor:

connection failure -> 500 null
timeout            -> 500 null
html 200           -> 302 https://app.example/settings/connected-accounts?connected=failed

The third line is the neighbouring case that was already handled, included to show the failure was
specific to the transport rather than to the function.

End to end after the fix, over a real socket, with real failures rather than stubs. A real
server holding the real routes, and a vendor that genuinely cannot be reached:

[1] real ECONNREFUSED against a closed port      -> null in 4 ms
[2] real DNS failure                             -> null
[3] real 15s AbortSignal.timeout, against a
    server that accepts and never answers        -> null after 15 s
[4] real HTTP request to the callback route:
      status:   302
      location: https://app.example/settings/connected-accounts?connected=failed
      body:     0 bytes
      rows written: 0

Each of those printed its log line, including the one that took the full fifteen seconds:

oauth-token-endpoint-unreachable | http://127.0.0.1:1/token        | Error: Unable to connect...
oauth-token-endpoint-unreachable | https://no-such-host.invalid/.. | Error: Unable to connect...
oauth-token-endpoint-unreachable | http://127.0.0.1:32993/token    | TimeoutError: The operation timed out.
oauth-token-endpoint-unreachable | https://mcp.notion.com/token    | Error: Unable to connect...

Tests. Eleven added across four files, every one of them watched fail before the change and pass
after. They were then checked for being load-bearing rather than merely green, by mutation:

Mutation Result
Revert both try/catch blocks 6 RED
Catch only TypeError, rethrow the rest the timeout tests RED
Remove signal: AbortSignal.timeout(15_000) 3 RED
Neutralise the token-endpoint log call 1 RED
Make registerDynamicClient rethrow instead of refuse the store integration test RED

The last one matters because that test drives the real registerDynamicClient through
ensureOAuthClient rather than the injected stub every other test in that suite uses, so the null
it asserts on is produced by the shipped function.

Checks. bun run typecheck clean across app, server and worker. bun run lint clean over 516
files. bun run format:check clean. bun test with a migrated PostgreSQL: 2157 pass, 23 skip, 0
fail
across 176 files.

What is not covered. The recovery path in plugins/store.ts that re-registers after a vendor
evicts a client is reached by two separately executed tests meeting at a value rather than by one
that runs the whole path: the new integration test proves a transport failure produces null from
the real function, and an existing test proves null there rethrows the original refusal. The
fifteen seconds are exercised in the probe above but not in the suite, where the deadline is checked
by asserting the signal is armed and unfired rather than by waiting it out.

Fixes #344

The callback says every failure ends the same way: back at Settings with a word
about what happened, and nothing written. One did not. Redeeming the code turns a
refusal into null by asking `!response.ok`, and that question needs a response.
There is none when the connection is refused, the name does not resolve, TLS will
not agree, or the fifteen-second timeout fires, so the rejection went straight
through a function whose whole contract is to refuse quietly. Nothing above it
catches anything, so somebody who had just consented at the vendor got a bare 500
with no Location instead of the page that would have told them.

registerDynamicClient had the same hole against the same promise, one step
earlier in the flow, and is fixed with it. Neither route needed changing: both
already do the right thing with null, and both were already tested doing it.

Caught is not the same as unnoticed, so both log the cause. Until now the
framework's own handler printed these on the way to the 500, and buying the
redirect by dropping that would have made a vendor outage look exactly like
nobody trying to connect. The person sees the ordinary failure, indistinguishable
from every other refusal on an endpoint that deliberately tells an unauthenticated
caller nothing; the deployment sees which vendor and why.
…wers the others

Review of the redemption fix turned up the same shape in three more places on the same
flow, all of them reachable and none of them saying so.

Writing the grant to the vault was still unguarded, which is the identical failure one
step later and on the worst possible step: somebody who has just finished consenting at
the vendor, told by a blank error that something went wrong at the end of the one part
they did correctly. It ends at Settings now, like everything before it. The refresh token
stays out of the log; the row it belonged to was never written.

A malformed endpoint was being read as a vendor outage, because `fetch` refuses an
unusable URL by throwing the same kind of error a refused connection does. Both are
checked before the request is attempted. The person is told the same thing either way,
since there is nothing else to tell them, but a catalogue somebody can fix and a vendor
nobody can are worth different lines to whoever is reading them.

Connect's 502 said the vendor refused this deployment's registration. After the fix a
vendor that could not be reached arrives at the same branch, so the sentence says both
rather than naming the wrong one confidently.

The timeout tests now read the abort signal instead of ignoring it. They would have passed
against code that had dropped the deadline entirely, which is the one thing a timeout test
is there to notice. And the registration refusal gets a test that runs the real function
through the store rather than the stub the rest of that suite injects, so the null the
store's branches are written for is one the shipped code produced.
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.

OAuth callback answers a bare 500 when the vendor's token endpoint cannot be reached

1 participant