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

CHANGELOG.md

@@ -8,6 +8,26 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged.
 
 ## Unreleased
 
+### Connecting an account survives a vendor that is down
+
+Finishing a connection used to end on a blank server error if the vendor could not be reached at the
+moment you were sent back, whether that was a refused connection, a name that would not resolve, or
+fifteen seconds of silence. It now ends where every other failed connect already ended: back on
+Connected accounts with a note, and nothing stored. Pressing Connect for a vendor this deployment
+has not introduced itself to yet behaves the same way, answering 502 rather than a server error, and
+that message now covers a vendor that could not be reached as well as one that turned the
+registration down.
+
+A connection whose grant cannot be written to the vault ends the same way, rather than on the blank
+error it used to give somebody who had just finished consenting.
+
+Because the person is told the same thing whatever went wrong, the server log is now where the
+difference lives. Three lines to look for: `oauth-token-endpoint-unreachable` and
+`oauth-registration-endpoint-unreachable` name the vendor and the cause, and
+`oauth-connection-not-recorded` says a consent completed and could not be kept. A fourth,
+`oauth-token-endpoint-unusable`, means the fault is this deployment's catalogue rather than the
+vendor.
+
 ### Coworkers are made in a wizard and managed in a dialog
 
 Creating a coworker is now a three-step wizard — who it is, who may see it, then where it runs,

server/src/plugins/oauth.ts

@@ -323,20 +323,81 @@ export async function redeemAuthorizationCode(input: {
   // A public (DCR) client proves itself with PKCE, and some vendors refuse an unexpected empty field.
   if (input.clientSecret) params.set("client_secret", input.clientSecret);
 
-  const response = await fetch(input.tokenUrl, {
-    method: "POST",
-    headers: { "content-type": "application/x-www-form-urlencoded" },
-    body: params,
+  /*
+   * Our own catalogue, told apart from their outage before a request is attempted.
+   *
+   * `fetch` refuses a malformed URL by throwing the same kind of error a refused connection does,
+   * so without this the two would reach the same catch and read as the same sentence. They are not
+   * the same thing: one is a vendor having a bad day and the other is an endpoint in this
+   * deployment's own catalogue that nobody can ever connect through. The person gets the ordinary
+   * refusal either way, because there is nothing else to give them, and the log is where the
+   * difference has to survive.
+   *
+   * Checking it here is also what leaves the catch below covering only the transport, rather than
+   * quietly standing in for a mistake in a frozen literal.
+   */
+  if (!URL.canParse(input.tokenUrl)) {
+    console.error(
+      JSON.stringify({
+        type: "oauth-token-endpoint-unusable",
+        tokenUrl: input.tokenUrl,
+        note: "This vendor's token endpoint is not a usable address, so nobody can connect it until the catalogue is fixed.",
+      }),
+    );
+    return null;
+  }
+
+  /*
+   * A vendor that could not be reached at all, which the refusal below cannot see.
+   *
+   * `!response.ok` needs a response, and there is none when the connection is refused, the name does
+   * not resolve, TLS will not agree, or the timeout on this request fires. Unguarded, that rejection
+   * escaped a function whose whole contract is to refuse quietly, and it escaped at the worst
+   * available moment: the callback has no failure handler above it, so somebody who had just
+   * consented at the vendor got a bare 500 with no Location instead of Settings telling them it did
+   * not work. The same reasoning as the defensive read further down, one step earlier in the request.
+   *
+   * Nothing is read off the error. Which of those it was is a fact about the vendor's infrastructure
+   * on a route that answers an unauthenticated caller, and the person is told the same sentence
+   * either way.
+   */
+  let response: Response;
+  try {
+    response = await fetch(input.tokenUrl, {
+      method: "POST",
+      headers: { "content-type": "application/x-www-form-urlencoded" },
+      body: params,
+      /*
+       * A redirect is a refusal, not a detour to be followed.
+       *
+       * `tokenUrl` is pinned in the catalogue because this request carries a client secret and an
+       * authorization code, and following a 302 would hand both to whatever address the answer named.
+       * Manual leaves the 3xx as the response, which is not `ok`, so it falls into the refusal below.
+       */
+      redirect: "manual",
+      signal: AbortSignal.timeout(15_000),
+    });
+  } catch (error) {
     /*
-     * A redirect is a refusal, not a detour to be followed.
+     * Logged, because refusing quietly to the person must not mean refusing quietly to the
+     * deployment. Until this, an unreachable token endpoint reached Hono's default handler, which
+     * prints the error before its 500; catching it without a line here would have bought the
+     * redirect by making a vendor outage look exactly like nobody trying to connect.
      *
-     * `tokenUrl` is pinned in the catalogue because this request carries a client secret and an
-     * authorization code, and following a 302 would hand both to whatever address the answer named.
-     * Manual leaves the 3xx as the response, which is not `ok`, so it falls into the refusal below.
+     * The status is what a person sees and this is what an operator sees, and only the second one
+     * says which vendor and why. Neither the code nor the client secret is in a transport error:
+     * they are in the request body, which never got sent.
      */
-    redirect: "manual",
-    signal: AbortSignal.timeout(15_000),
-  });
+    console.error(
+      JSON.stringify({
+        type: "oauth-token-endpoint-unreachable",
+        tokenUrl: input.tokenUrl,
+        note: "A person's consent could not be redeemed. They were sent back to Settings with a failure.",
+        error: String(error),
+      }),
+    );
+    return null;
+  }
 
   if (!response.ok) return null;
 
@@ -388,21 +449,55 @@ export async function registerDynamicClient(input: {
   registrationUrl: string;
   redirectUri: string;
 }): Promise<{ clientId: string; clientSecret: string } | null> {
-  const response = await fetch(input.registrationUrl, {
-    method: "POST",
-    headers: { "content-type": "application/json" },
-    body: JSON.stringify({
-      redirect_uris: [input.redirectUri],
-      grant_types: ["authorization_code", "refresh_token"],
-      response_types: ["code"],
-      token_endpoint_auth_method: "none",
-      client_name: "OpenBot",
-    }),
-    // The registration endpoint is pinned in the catalogue, so a redirect is somebody else deciding
-    // where this deployment introduces itself. Left as the response, which is not `ok`.
-    redirect: "manual",
-    signal: AbortSignal.timeout(15_000),
-  });
+  // The same catalogue check the redemption makes, and for the same reason: a registration endpoint
+  // that is not an address is this deployment's mistake, not the vendor's outage, and only the log
+  // can tell them apart.
+  if (!URL.canParse(input.registrationUrl)) {
+    console.error(
+      JSON.stringify({
+        type: "oauth-registration-endpoint-unusable",
+        registrationUrl: input.registrationUrl,
+        note: "This vendor's registration endpoint is not a usable address, so this deployment can never introduce itself.",
+      }),
+    );
+    return null;
+  }
+
+  // A vendor that could not be reached at all — see `redeemAuthorizationCode`, which states the same
+  // gap in full. This one lands on an administrator pressing Connect rather than on somebody
+  // mid-consent, and null is what turns it into the 502 that route already writes for a vendor that
+  // would not register us.
+  let response: Response;
+  try {
+    response = await fetch(input.registrationUrl, {
+      method: "POST",
+      headers: { "content-type": "application/json" },
+      body: JSON.stringify({
+        redirect_uris: [input.redirectUri],
+        grant_types: ["authorization_code", "refresh_token"],
+        response_types: ["code"],
+        token_endpoint_auth_method: "none",
+        client_name: "OpenBot",
+      }),
+      // The registration endpoint is pinned in the catalogue, so a redirect is somebody else deciding
+      // where this deployment introduces itself. Left as the response, which is not `ok`.
+      redirect: "manual",
+      signal: AbortSignal.timeout(15_000),
+    });
+  } catch (error) {
+    // Logged for the same reason the redemption above is: the caller's 502 tells an administrator
+    // to check the vendor's status, and this is the line that says the vendor could not be reached
+    // at all rather than that it turned us down.
+    console.error(
+      JSON.stringify({
+        type: "oauth-registration-endpoint-unreachable",
+        registrationUrl: input.registrationUrl,
+        note: "This deployment could not introduce itself to the vendor. Connect answered 502.",
+        error: String(error),
+      }),
+    );
+    return null;
+  }
   if (!response.ok) return null;
   // A 200 is not a promise of JSON — see `redeemAuthorizationCode`. A body that will not parse is
   // the vendor answering with something other than a client, which is this function's null.

server/src/plugins/routes.ts

@@ -415,7 +415,7 @@ export function createPluginRoutes(
       if (entry.auth.clientRegistration === "dynamic") {
         return context.json(
           {
-            error: `${entry.title} refused this deployment's registration. Try again, and check the vendor's status if it persists.`,
+            error: `${entry.title} would not register this deployment, or could not be reached. Try again, and check the vendor's status if it persists.`,
           },
           502,
         );
@@ -514,12 +514,36 @@ export function createPluginRoutes(
     });
     if (!grant) return context.redirect(failed);
 
-    await store.recordConnection({
-      serverId: state.serverId,
-      userId: state.userId,
-      refreshToken: grant.refreshToken,
-      scope: grant.scope,
-    });
+    /*
+     * The last thing that can fail, answered the same way as everything before it.
+     *
+     * A vault that will not take the grant is this deployment's problem, not the person's, and they
+     * have already done their part at the vendor. Unhandled, this threw past the handler and gave
+     * them the bare 500 that every other failure on this route was written to avoid, on the one
+     * path where they had most reason to think it had worked.
+     *
+     * Told, because unlike the refusals above this one is nobody's fault but ours, and the person's
+     * sentence deliberately says nothing about which failure it was. The refresh token is not
+     * logged: it is the one thing here worth stealing, and the row it belonged to was never written.
+     */
+    try {
+      await store.recordConnection({
+        serverId: state.serverId,
+        userId: state.userId,
+        refreshToken: grant.refreshToken,
+        scope: grant.scope,
+      });
+    } catch (error) {
+      console.error(
+        JSON.stringify({
+          type: "oauth-connection-not-recorded",
+          serverId: state.serverId,
+          note: "A person consented and the grant could not be stored. They were sent back to Settings with a failure and will have to connect again.",
+          error: String(error),
+        }),
+      );
+      return context.redirect(failed);
+    }
 
     return context.redirect(
       connectedAccountsUrlFor(

server/tests/plugin-connect-route.test.ts

@@ -82,7 +82,15 @@ describe("connecting a dynamically registered vendor", () => {
     expect(url.searchParams.get("client_id")).toBe("dyn-1");
   });
 
-  test("a refused registration answers 502, naming the vendor", async () => {
+  /**
+   * One answer for two states, because there is one thing to do about either.
+   *
+   * A vendor that turned this deployment down and a vendor that could not be reached both leave
+   * `ensureOAuthClient` with no client, and the person pressing Connect has the same next step
+   * whichever it was. The sentence says both rather than naming the wrong one confidently; which it
+   * actually was is in the log, where it is of use to somebody who can act on it.
+   */
+  test("a registration that produced no client answers 502, naming the vendor", async () => {
     const ensureCalls: { serverId: string; by: string }[] = [];
     const hono = app({
       oauthClientFor: async () => null,
@@ -101,7 +109,7 @@ describe("connecting a dynamically registered vendor", () => {
     expect(ensureCalls.length).toBe(1);
     const body = (await response.json()) as { error: string };
     expect(body.error).toBe(
-      "Notion refused this deployment's registration. Try again, and check the vendor's status if it persists.",
+      "Notion would not register this deployment, or could not be reached. Try again, and check the vendor's status if it persists.",
     );
   });
 });