fix(oauth): apply manual-token + requiresClientSecret + app-not-found checks uniformly (#30251 follow-up)#30255
Merged
Conversation
… validations to apply uniformly Three follow-up fixes from review of #30251: - CLI: provider serializer emits `authUrl` (camelCase) but `connect.ts` was reading `auth_url`, making the manual-token guard dead code for BYO-mode providers like slack_channel and telegram. - Daemon: `handleOAuthConnectStart` previously ran the manual-token check, app-not-found error, and `requiresClientSecret` check only inside `if (!clientId)`. Callers passing `--client-id` bypassed all three. Hoisted them above the branch split so they apply uniformly, and added the missing app-not-found `NotFoundError` to the explicit-clientId branch. - Cleanup: removed dead `PlatformConnectionEntry` export from `cli/commands/oauth/shared.ts` (zero importers in the repo). Adds 7 new tests in `oauth-connect-routes.test.ts` covering every validation path the refactor touches. Existing tests now mock `oauth-store` so they no longer depend on an unseeded DB returning null.
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #30251 addressing unaddressed Codex/Devin review feedback. Three concrete fixes:
1.
connect.tsreads wrong field name — manual-token guard was dead code (🔴 Devin, 🟡 Codex P2)The IPC route serializer emits the provider's authorize URL as
authUrl(camelCase), but the CLI was readingauth_url(snake_case).authorizeUrlwas alwaysundefined, so the manual-token check at line 303 (if (authorizeUrl === "urn:manual-token")) was unreachable. For BYO-mode manual-token providers likeslack_channelandtelegram, the CLI silently fell through to the daemon instead of returning the intended actionable error.One-character fix:
auth_url→authUrl.2. Daemon route bypassed manual-token + app-not-found + requiresClientSecret checks when
clientIdwas explicitly supplied (🟡 Codex P2, 🟡 Devin)handleOAuthConnectStarthad three validations that only ran insideif (!clientId):NotFoundError.authorizeUrl === "urn:manual-token"→BadRequestError.requiresClientSecret&& missing secret →BadRequestError.Callers passing
--client-idskipped all three. For manual-token providers this let the OAuth browser flow start when it could never succeed; for providers requiring a secret it produced a cryptic downstream error instead of an actionable one; for unregistered client IDs the call silently continued instead of failing fast.Refactor: hoist the three checks above the branch split so they apply uniformly to both
!clientIdand explicit-clientIdcallers. Also added the missing app-not-foundNotFoundErrorto the explicit-clientIdbranch.3. Dead
PlatformConnectionEntrytype inshared.ts(🚩 Devin)The interface was exported from
shared.tsbut had zero importers anywhere in the repo —oauth-commands-routes.tskeeps its own local copy. Removed the dead export.Test coverage
7 new tests in
oauth-connect-routes.test.tscovering every validation path the refactor touches:NotFoundError.clientIdis explicitly supplied (the bug).clientIdis supplied.clientIdwith no registered app →NotFoundError(the bug).clientIdand no registered app →BadRequestError.client_secretwhenrequiresClientSecretis true →BadRequestError.clientId+ stored secret in keyring succeeds without caller-supplied secret.Existing tests now mock
oauth-storeto avoid relying on the unseeded DB returningnull. The removed"missing clientId throws BadRequestError"test was redundant — it was implicitly testing "no app registered" by way of an unseeded DB, which the new explicit "no clientId and no registered app" test covers properly.19/19 tests pass in
oauth-connect-routes.test.ts.Findings explicitly NOT addressed
readBodyDatacan read from daemon's stdin or filesystem" (oauth-commands-routes.ts:606-619). Devin's own analysis notes the practical risk is low (daemon stdin is/dev/null, route requires policy enforcement), and the CLI never exercises thedatafield (onlyparsed_data). Worth a separate scoping conversation rather than a defensive patch here.Bot tags
@codex review
@devin review