fix: prevent ambiguous multi-upstream resource routing - #44
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughMCP resource and prompt proxying is available only for standard or sole-upstream configurations. Multi-upstream selection requires explicit names, unavailable capabilities are omitted and reported by ChangesResource and prompt proxy behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant MiftahServer
participant MultiUpstreamProcessManager
participant UpstreamSession
MCPClient->>MiftahServer: Request resource or prompt
MiftahServer->>MultiUpstreamProcessManager: Select sole upstream by name
MultiUpstreamProcessManager->>UpstreamSession: Read resource or get prompt
UpstreamSession-->>MiftahServer: Return result or error
MiftahServer-->>MCPClient: Return redacted result or method-not-found
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mcp/server/miftah-server.ts`:
- Around line 152-172: Wrap the ListResourcesRequestSchema and
ListPromptsRequestSchema handlers in the same try/catch pattern as
ReadResourceRequestSchema, redacting caught error messages with redactSecrets
and this.upstreams.getSecretValues() before rethrowing with the original error
as cause. Add regression coverage verifying secrets in resources/list and
prompts/list failure messages are absent from client-facing errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1d161755-d98f-482a-9abf-a3b88aa7110b
📒 Files selected for processing (7)
README.mddocs/architecture.mddocs/config.mdsrc/mcp/server/miftah-server.tssrc/upstream/multi-upstream-process-manager.tssrc/utils/errors.tstests/multi-upstream.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp/server/miftah-server.ts (1)
150-199: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the repeated redact-on-error wrapper.
All four handlers (
ListResources,ReadResource,ListPrompts,GetPrompt) now duplicate the identical try/catch/redact block, differing only in the upstream call made. This exact duplication is what previously let two of the four handlers silently skip redaction (per the earlier review comment on lines 152-186) — consolidating into a shared helper removes that class of bug going forward.♻️ Proposed refactor
+ private async proxyResourceOrPrompt<T>( + upstreamName: string | undefined, + call: (session: Awaited<ReturnType<typeof this.upstreams.get>>) => Promise<T> + ): Promise<T> { + try { + const session = await this.upstreams.get(this.profiles.current().activeProfile, upstreamName); + return redactSecrets(await call(session), this.upstreams.getSecretValues()); + } catch (error) { + throw new Error( + redactSecrets(error instanceof Error ? error.message : String(error), this.upstreams.getSecretValues()), + { cause: error } + ); + } + } + if (this.resourcePromptProxy.available) { const upstreamName = this.resourcePromptProxy.upstreamName; - this.server.setRequestHandler(ListResourcesRequestSchema, async () => { - try { - const session = await this.upstreams.get(this.profiles.current().activeProfile, upstreamName); - return redactSecrets(await session.listResources(), this.upstreams.getSecretValues()); - } catch (error) { - throw new Error( - redactSecrets(error instanceof Error ? error.message : String(error), this.upstreams.getSecretValues()), - { cause: error } - ); - } - }); + this.server.setRequestHandler(ListResourcesRequestSchema, () => + this.proxyResourceOrPrompt(upstreamName, (session) => session.listResources()) + ); - this.server.setRequestHandler(ReadResourceRequestSchema, async (request) => { - try { - const session = await this.upstreams.get(this.profiles.current().activeProfile, upstreamName); - return redactSecrets(await session.readResource(request.params), this.upstreams.getSecretValues()); - } catch (error) { - throw new Error( - redactSecrets(error instanceof Error ? error.message : String(error), this.upstreams.getSecretValues()), - { cause: error } - ); - } - }); + this.server.setRequestHandler(ReadResourceRequestSchema, (request) => + this.proxyResourceOrPrompt(upstreamName, (session) => session.readResource(request.params)) + ); // ...same pattern for ListPrompts / GetPrompt }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp/server/miftah-server.ts` around lines 150 - 199, Extract the duplicated upstream request error-handling into a shared helper near the handler setup, accepting an operation callback and applying secret redaction to both successful results and caught errors. Refactor ListResources, ReadResource, ListPrompts, and GetPrompt to invoke this helper while retaining their respective session methods and request parameters, ensuring all handlers consistently use the centralized redaction logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/multi-upstream.test.ts`:
- Around line 225-269: Extend the test “redacts secrets from upstream resource
and prompt discovery failures” to also invoke client.readResource and
client.getPrompt with inputs that trigger upstream failures, then assert their
Error messages contain “[REDACTED]” and exclude the secret, matching the
existing listResources/listPrompts assertions. Use the relevant
ReadResourceRequestSchema and GetPromptRequestSchema handlers in
miftah-server.ts to ensure the new cases exercise their redact-on-error paths.
---
Outside diff comments:
In `@src/mcp/server/miftah-server.ts`:
- Around line 150-199: Extract the duplicated upstream request error-handling
into a shared helper near the handler setup, accepting an operation callback and
applying secret redaction to both successful results and caught errors. Refactor
ListResources, ReadResource, ListPrompts, and GetPrompt to invoke this helper
while retaining their respective session methods and request parameters,
ensuring all handlers consistently use the centralized redaction logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4200bb93-1958-443a-8b49-a915bc625606
📒 Files selected for processing (3)
src/mcp/server/miftah-server.tstests/fixtures/fake-upstream.mjstests/multi-upstream.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp/server/miftah-server.ts (1)
171-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate redact-and-rethrow logic across three call sites.
The
redactSecrets(error.message/String(error), this.upstreams.getSecretValues())pattern is repeated inhandleUpstreamTool's catch (Lines 224-228),handleManagement's catch (Lines 290-292), andproxyResourcePrompt's catch (Lines 332-336). Consider extracting a smallprivate redactError(error: unknown): stringhelper to reduce duplication and keep the redaction behavior consistent if it needs to change later.♻️ Proposed helper
+ private redactError(error: unknown): string { + return redactSecrets( + error instanceof Error ? error.message : String(error), + this.upstreams.getSecretValues() + ); + }Then replace the three inline blocks with
this.redactError(error).Also applies to: 236-294, 325-339
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp/server/miftah-server.ts` around lines 171 - 234, Extract a private redactError(error: unknown): string helper on the server class that formats Error messages or unknown values with String(error) and passes them through redactSecrets using this.upstreams.getSecretValues(). Replace the duplicated inline redaction logic in handleUpstreamTool, handleManagement, and proxyResourcePrompt catch blocks with this.redactError(error), preserving each caller’s existing error-code and response behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/mcp/server/miftah-server.ts`:
- Around line 171-234: Extract a private redactError(error: unknown): string
helper on the server class that formats Error messages or unknown values with
String(error) and passes them through redactSecrets using
this.upstreams.getSecretValues(). Replace the duplicated inline redaction logic
in handleUpstreamTool, handleManagement, and proxyResourcePrompt catch blocks
with this.redactError(error), preserving each caller’s existing error-code and
response behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b5a1eb00-3e71-4e8e-9205-7ddd64e97a67
📒 Files selected for processing (3)
src/mcp/server/miftah-server.tstests/fixtures/fake-upstream.mjstests/multi-upstream.test.ts
1 participant
Summary
-32601instead of selecting the first server.miftah_health.Verification
npm testnpm run typechecknpm run lintnpm run buildnpm run check:packTracks #5
Summary by CodeRabbit
miftah_healthnow reports resource/prompt proxy availability (including a reason).