Fix four production bugs surfaced by cloud usage logs - #135
Merged
Merged
Conversation
1. POST /register OAuth DCR no longer 500s on malformed bodies. 17 hits in production crashed with `Cannot read properties of undefined (reading 'redirect_uris')` — multipart/form-data Server Actions and OAuth clients sending non-JSON bodies were dereferenced directly by @rekog/mcp-nest's ClientService. New OAuthRegisterGuardMiddleware validates content-type + body shape before the controller and returns the RFC 7591 error codes (invalid_client_metadata / invalid_redirect_uri) instead of a 500. 2. EmailService no longer logs verification codes in plaintext. When local SMTP wasn't configured, the service fell back to the external API correctly but also logged "verification code for X: 123456" at warn level. A 6-digit code is short enough to be a genuine credential and showed up readable in any log aggregation. Replaced with a debug line that just states delegation happened. 3. OpenApiParser now accepts YAML and OpenAPI 3.1. `Unexpected token 'o', "openapi: 3"... is not valid JSON` was the second-most-common 5xx in production: users pasted YAML specs and the parser called JSON.parse blindly. Now decodeSpecString tries JSON first, falls back to js-yaml. swagger-parser's validator refuses 3.1.0 outright; route 3.1.0 specs through dereference() instead so $ref still resolves and tools can be extracted, with a clearer error message if anything fails. 4. Cross-tenant tool-name collision on the global /mcp endpoint. When two organisations defined a tool with the same name (3 orgs had `weclapp_list_articles`), `toolRegistry.getTool(name)` could return whichever org's copy was registered first. Added `organizationId` to RegisteredTool, a `getToolForOrg(name, orgId)` lookup, and wired it through the auth handler in mcp-server.service and the executor in dynamic-mcp-tools so authenticated callers on the global endpoint resolve their own org's tool. The per-server /mcp/:serverId endpoint already scoped by connectorIds and was safe; /mcp wasn't. Coverage: +5 unit tests (4 for the OAuth guard, 1 for getToolForOrg). Backend jest: 561 passed, 1 skipped, 0 failed. Smoke test: 12/12 PASS end-to-end.
Merged
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.
After analysing 8 weeks of usage on `cloud.anythingmcp.com` four bugs stand out as repeatedly hit by real users. This PR fixes all four.
1. POST /register crashed for some OAuth clients
17 hits in production threw `TypeError: Cannot read properties of undefined (reading 'redirect_uris')` at `@rekog/mcp-nest`'s `ClientService.registerClient`. Triggers: multipart/form-data Server Actions, browsers navigating to `/register`, OAuth clients sending non-JSON bodies. New `OAuthRegisterGuardMiddleware` returns the RFC 7591 `invalid_client_metadata` / `invalid_redirect_uri` codes with 400 instead of letting the upstream library 500.
2. Verification codes logged in plaintext
`EmailService` warned `verification code for X: 641958` whenever the local SMTP wasn't configured (4 different users in the last week, codes sitting in any log aggregation). The fallback path delivers via Mailgun anyway, so the log line had no operational use. Replaced with a debug line that just records that delegation happened.
3. OpenApiParser refused YAML and OpenAPI 3.1
Two consecutive 5xx for one user trying to import the FAA Aviation Weather spec — first YAML (`Unexpected token 'o'`), then OAS 3.1 (`Unsupported OpenAPI version: 3.1.0`). Now the parser tries JSON first, falls back to `js-yaml`, and routes 3.1 specs through `SwaggerParser.dereference()` (resolves `$ref` without the strict validator). Cleaner error messages on the rare malformed-spec path.
4. Cross-tenant tool-name collision on /mcp
Three organisations had a connector with the same tool name (`weclapp_list_articles` etc). The internal registry's `getTool(name)` returned whichever org registered first when called without scoping. Added `organizationId` to `RegisteredTool` and a new `getToolForOrg(name, orgId)` lookup, wired through the auth handler so authenticated callers on the global `/mcp` endpoint resolve their own org's tool. `/mcp/:serverId` was already safe via connector-id scoping.
Test plan