From 9647f2d65f59e566eb2d8c973ab91bc186a3bb56 Mon Sep 17 00:00:00 2001 From: Ty Tremblay Date: Thu, 20 Aug 2026 23:17:46 -0400 Subject: [PATCH] Don't let a lost state marker kill a group-editing authorization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Approving the group-editing grant could end at "Authorization failed. Tell a coach." The callback decided which of the two per-person grants had come back by reading a `metadata` marker put into the OAuth state. When that marker did not survive the round trip, the callback fell through to the enrolment branch, where the scope guard correctly refused to overwrite a mentor's DM token with one that cannot read DMs — and the refusal took the whole authorization down with it. The guard was right. Routing on a marker was not: it made one hint a single point of failure for a flow whose failure mode is a dead end. Grants are now told apart by what the token can actually do. A token carrying `usergroups:write` and no DM history scope cannot be an enrolment, whatever the state parameter says. The marker is still honoured when present, and the scope guard stays as the backstop it was meant to be rather than the tripwire it became. Verified: with the marker absent, a group grant now lands in the admin row and the mentor's DM token is untouched; a normal enrolment still stores as one; and a genuinely degraded enrolment token is still refused. The failure page also stops being a dead end. An authorization link is valid for ten minutes, and expiring is both the most likely failure and the only one the person reading the page can fix, so it now says so and tells them to run the command again. Everything else names the error code to quote to a coach, instead of asking them to report that something, somewhere, went wrong. Co-Authored-By: Claude Opus 5 --- src/slack/app.ts | 41 ++++++++++++++++++++++++++++++++++++--- src/slack/installStore.ts | 25 +++++++++++++++++++++++- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/src/slack/app.ts b/src/slack/app.ts index 3e732e2..0c13c7b 100644 --- a/src/slack/app.ts +++ b/src/slack/app.ts @@ -167,9 +167,44 @@ export function createApp(): App { ); }, failure: (error, _options, _req, res) => { - log.error("oauth failure", { error: String(error) }); - res.writeHead(500, { "content-type": "text/plain; charset=utf-8" }); - res.end("Authorization failed. Tell a coach."); + // An authorization link is valid for ten minutes. Expiring is the + // most common way this fails and the only one the person in front of + // it can fix, so it gets its own answer rather than being lumped in + // with "tell a coach" — which is what to do about the rest. + const code = (error as { code?: string } | undefined)?.code; + const expired = + code === "slack_oauth_invalid_state" || + code === "slack_oauth_missing_state"; + + log.error("oauth failure", { + code: code ?? "none", + error: String(error), + }); + + res.writeHead(expired ? 400 : 500, { + "content-type": "text/html; charset=utf-8", + }); + res.end( + ` + + ${APP_NAME} +
+ ${ICON_SVG} +

${ + expired ? "That link expired." : "Authorization failed." + }

+

${APP_NAME}

+

${ + expired + ? "Authorization links are good for ten minutes. Run the " + + "command again and follow the new link — nothing was " + + "changed." + : "Nothing was changed. Tell a coach, and give them this: " + + `${code ?? "unknown"}. The server log has ` + + "the detail." + }

+
` + ); }, }, }, diff --git a/src/slack/installStore.ts b/src/slack/installStore.ts index a5ebc5b..027a769 100644 --- a/src/slack/installStore.ts +++ b/src/slack/installStore.ts @@ -31,6 +31,29 @@ function teamKey( /** Marks an authorization that came from the group-editing flow. */ export const GROUP_ADMIN_METADATA = "group-admin"; +/** Present on an enrolment token, absent on a group-editing one. */ +const DM_SCOPE = "im:history"; +const GROUP_WRITE_SCOPE = "usergroups:write"; + +/** + * Which of the two per-person grants came back. + * + * Decided from the token's *scopes* rather than from the metadata marker + * alone. The marker is a hint that has to survive a round trip through Slack, + * and when it did not, this fell through to the enrolment branch — where the + * scope guard correctly refused to overwrite a mentor's DM token, and the whole + * authorization died with "Authorization failed. Tell a coach." + * + * What the token can actually do is not a hint. A token carrying + * `usergroups:write` and no DM history scope cannot be an enrolment, whatever + * the state parameter says it was. + */ +function isGroupAdminGrant(installation: Installation): boolean { + if (installation.metadata === GROUP_ADMIN_METADATA) return true; + const scopes = installation.user?.scopes ?? []; + return scopes.includes(GROUP_WRITE_SCOPE) && !scopes.includes(DM_SCOPE); +} + /** * Three kinds of row live here: * @@ -73,7 +96,7 @@ export const installationStore: InstallationStore = { // The group-editing grant is a different consent for a different purpose // and gets its own row. Critically it must not fall through to the // enrolment branch below, which would overwrite a mentor's DM token. - if (installation.metadata === GROUP_ADMIN_METADATA) { + if (isGroupAdminGrant(installation as Installation)) { saveInstallation({ teamId, enterpriseId,