fix: convert Custom OAuth handshake failures to handled Meteor.Errors to prevent 500 crash (fixes #35649)#39391
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 9b89833 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughA patch release fix for Custom OAuth login error handling. The changes modify the OAuth token validation flow to throw proper Meteor.Error exceptions with appropriate HTTP status codes (401/400) instead of returning generic 500 Internal Server Errors when authentication handshakes fail. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…n handshake failure
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/oauth/oauth.js (1)
54-59: Consider preserving error context for different failure modes.The catch block converts all errors to
error-invalid-access-token, but the underlyinghandleAccessTokenRequestcan fail for distinct reasons: network failures, OAuth provider 5xx errors, token validation failures, or JSON parsing errors. Conflating these may complicate debugging for integrators.A potential improvement would be to differentiate error codes based on failure type, or at minimum preserve the original error as the
cause:♻️ Optional: Preserve error context
let oauthResult; try { oauthResult = await service.handleAccessTokenRequest(options); } catch (err) { - throw new Meteor.Error('error-invalid-access-token', err.message); + throw new Meteor.Error('error-invalid-access-token', err.message, { cause: err }); }That said, the current implementation achieves the PR's goal of preventing 500 errors while returning actionable error messages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/lib/server/oauth/oauth.js` around lines 54 - 59, The catch currently maps every failure from service.handleAccessTokenRequest to 'error-invalid-access-token', losing failure context; update the catch to inspect the thrown error (e.g., err.name, err.statusCode, or err.code) and map to more specific Meteor.Error codes for network/provider/validation/parsing failures (or at minimum include the original error as the cause/detail), then rethrow a Meteor.Error that includes that specific code and the original error message/stack in its details so callers can distinguish failure modes while preserving the existing non-500 behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/app/lib/server/oauth/oauth.js`:
- Around line 54-59: The catch currently maps every failure from
service.handleAccessTokenRequest to 'error-invalid-access-token', losing failure
context; update the catch to inspect the thrown error (e.g., err.name,
err.statusCode, or err.code) and map to more specific Meteor.Error codes for
network/provider/validation/parsing failures (or at minimum include the original
error as the cause/detail), then rethrow a Meteor.Error that includes that
specific code and the original error message/stack in its details so callers can
distinguish failure modes while preserving the existing non-500 behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d42e518a-8d00-474f-ace6-38e304d48256
📒 Files selected for processing (2)
.changeset/six-forks-remain.mdapps/meteor/app/lib/server/oauth/oauth.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/lib/server/oauth/oauth.js
🧠 Learnings (4)
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
.changeset/six-forks-remain.md
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
.changeset/six-forks-remain.md
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
.changeset/six-forks-remain.md
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
.changeset/six-forks-remain.md
🔇 Additional comments (4)
.changeset/six-forks-remain.md (1)
1-5: LGTM!The changeset accurately documents the bug fix with the appropriate
patchsemver level. The description clearly communicates the behavior change from 500 to proper 401/400 error responses.apps/meteor/app/lib/server/oauth/oauth.js (3)
33-35: LGTM!Good change to use
Meteor.Errorwith a specific error code instead of a genericError. This ensures the API middleware can return a structured response instead of a 500.
37-41: LGTM!The explicit service configuration check with a descriptive
Meteor.Erroris a good addition. This provides clear feedback when the OAuth service isn't properly configured.
43-52: LGTM!Good defensive check ensuring the service is registered with Meteor's OAuth system. Returning an error object (rather than throwing) follows Meteor's
registerLoginHandlerconvention for authentication failures.
1 participant
Proposed changes
This PR resolves an Internal Server Error (500) that occurs during the Custom OAuth login flow via the REST API.
The issue was caused by the use of generic Error objects and unhandled promise rejections in the OAuth login handler. When an invalid token or a connection failure occurred, the server threw a standard JavaScript Error, which the API middleware interprets as a 500 status.
screenshot :
Key Changes:
Refactored Accounts.registerLoginHandler in oauth.js to catch errors during handleAccessTokenRequest.
Replaced generic Error and Accounts.ConfigError with Meteor.Error to ensure the API returns a structured 401 Unauthorized or 400 Bad Request response.
Added specific error codes (error-invalid-service, error-service-not-configured) for better client-side debugging.
Issue(s)
Closes #35649
Steps to test or reproduce
Enable Custom OAuth in Settings and point it to a non-existent URL (e.g., http://localhost:9999).
Attempt to login via REST API:
Bash
curl -X POST http://localhost:3000/api/v1/login
-H "Content-Type: application/json"
-d '{ "serviceName": "bugHunt", "accessToken": "test", "expiresIn": 3600 }'
Expected Behavior (Fixed): Returns 401 Unauthorized with {"success":false,"error":"error-invalid-access-token"}.
Actual Behavior (Before): Returns 500 Internal Server Error.
Further comments
I chose to wrap the service.handleAccessTokenRequest in a try/catch block because this is the primary point of failure for network-related OAuth handshakes. By returning a Meteor.Error here, we provide a consistent error interface for all OAuth providers, not just custom ones.
Summary by CodeRabbit