Fix mcp registry auth issue - #3132
Conversation
📝 WalkthroughWalkthroughThe REST API test suite adds MCP registry coverage, CSRF-enabled page mutations, ignored test reports, and explicit JSON 401 handling for unauthenticated requests. ChangesMCP registry and authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
portals/api-portal/it/rest-api/mcp-registry/mcp-registry.spec.js (1)
112-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the version list structurally instead of stringifying the whole body.
JSON.stringify(versions.body)).toContain('1.0.0')also passes when1.0.0appears in an unrelated field, for example inside a remote URL or a schema version. The neighbouring test at Line 120 already shows the shape isserver.version, so assert on the list entries.♻️ Tighter assertion
const versions = await client.raw() .get(`${client.BASE_PATH}${REGISTRY}/servers/${encodeName(name)}/versions`); expect(versions.status).toBe(200); - expect(JSON.stringify(versions.body)).toContain('1.0.0'); + expect(versions.body.servers.map((s) => s.server.version)).toContain('1.0.0');Confirm the field name for the versions payload before applying the diff.
🤖 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 `@portals/api-portal/it/rest-api/mcp-registry/mcp-registry.spec.js` around lines 112 - 115, Update the versions assertion in the mcp-registry test to inspect the payload’s version-list entries structurally, using the confirmed server.version field, rather than stringifying the entire response body. Assert that an entry has version “1.0.0” while preserving the existing status check.
🤖 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 `@portals/api-portal/it/rest-api/mcp-registry/mcp-registry.spec.js`:
- Around line 253-256: Update the tampering assertion in the MCP registry test
to use a server field that survives reads, such as description, instead of
server.title. Change the hijack payload to assign a distinct description value
and assert the retrieved description is not that value, preserving the test’s
verification that unauthenticated PUT attempts leave the server unchanged.
- Around line 163-166: Update the duplicate validation in the upsert test to
inspect the complete registry result instead of only the first 100-item page.
Follow metadata.nextCursor across subsequent requests, aggregate servers before
filtering by name, and preserve the expectation that exactly one matching server
exists.
In `@portals/api-portal/src/middlewares/ensureAuthenticated.js`:
- Around line 100-101: Update the mTLS branch in the authentication middleware
to store the result of req.socket.getPeerCertificate(true) and require a
non-empty certificate using cert && Object.keys(cert).length > 0 before calling
enforceMTLS. Ensure enforceMTLS returns the same JSON error response used by
enforceSecurity for unauthenticated requests, preserving the expected 401
behavior when no client certificate is provided.
---
Nitpick comments:
In `@portals/api-portal/it/rest-api/mcp-registry/mcp-registry.spec.js`:
- Around line 112-115: Update the versions assertion in the mcp-registry test to
inspect the payload’s version-list entries structurally, using the confirmed
server.version field, rather than stringifying the entire response body. Assert
that an entry has version “1.0.0” while preserving the existing status check.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d265c654-6fec-4eb6-9106-ca12bb73027b
📒 Files selected for processing (4)
portals/api-portal/.gitignoreportals/api-portal/it/rest-api/mcp-registry/mcp-registry.spec.jsportals/api-portal/it/rest-api/support/client.jsportals/api-portal/src/middlewares/ensureAuthenticated.js
| // The upsert must not leave a duplicate behind. | ||
| const list = await client.raw().get(`${client.BASE_PATH}${REGISTRY}/servers?limit=100`); | ||
| const matches = list.body.servers.filter((s) => s.server.name === name); | ||
| expect(matches).toHaveLength(1); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The duplicate check depends on the server fitting inside one 100-item page.
The registry database is shared across the suite and across organizations, as the comment at Line 146 states. ?limit=100 returns one page only. If the org holds more than 100 servers, the upserted name can fall on a later page. matches is then 0 and the test fails even though the upsert behaved correctly. A duplicate on a later page would also go unseen.
Page through metadata.nextCursor until the cursor is absent, or scope the list request to the name if the registry supports a name filter.
🤖 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 `@portals/api-portal/it/rest-api/mcp-registry/mcp-registry.spec.js` around
lines 163 - 166, Update the duplicate validation in the upsert test to inspect
the complete registry result instead of only the first 100-item page. Follow
metadata.nextCursor across subsequent requests, aggregate servers before
filtering by name, and preserve the expectation that exactly one matching server
exists.
| // And the server is untouched by either attempt. | ||
| const check = await client.raw().get(target); | ||
| expect(check.status).toBe(200); | ||
| expect(check.body.server.title).not.toBe('Hijacked'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This assertion cannot detect tampering while the title defect stands.
The pinned test at Line 183 establishes that title is always undefined on a read. expect(check.body.server.title).not.toBe('Hijacked') therefore passes whether or not the unauthenticated PUT was rejected. It proves nothing today, and it silently starts proving something only after the title round trip is fixed.
Assert on a field that does survive the read, for example description, and send a distinct value in the hijack payload.
🤖 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 `@portals/api-portal/it/rest-api/mcp-registry/mcp-registry.spec.js` around
lines 253 - 256, Update the tampering assertion in the MCP registry test to use
a server field that survives reads, such as description, instead of
server.title. Change the hijack payload to assign a distinct description value
and assert the retrieved description is not that value, preserving the test’s
verification that unauthenticated PUT attempts leave the server unchanged.
| } else if (typeof req.socket?.getPeerCertificate === 'function' && req.socket.getPeerCertificate(true)) { | ||
| enforceMTLS(req, res, next); | ||
| return enforceMTLS(req, res, next); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Determine whether the API portal terminates TLS itself and how mTLS is enabled.
rg -nP -C5 '\b(https\.createServer|requestCert|rejectUnauthorized|mtls|mTLS)\b' portals/api-portal/src --type=js
fd -t f -e js 'server|app' portals/api-portal/src --exec rg -nP -C4 '\blisten\s*\('Repository: wso2/api-platform
Length of output: 15078
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ensureAuthenticated outline =="
ast-grep outline portals/api-portal/src/middlewares/ensureAuthenticated.js || true
echo
echo "== relevant ensureAuthenticated lines =="
sed -n '1,140p' portals/api-portal/src/middlewares/ensureAuthenticated.js | cat -n
echo
echo "== server listen lines =="
sed -n '130,170p' portals/api-portal/src/server.js | cat -n
echo
echo "== config docs mentioning https enabled/keyFile/certFile =="
rg -n "server\.https|https\.enabled|certFile|keyFile|rejectUnauthorized|requestCert" . -g '!node_modules' -g '!dist' -g '!build' | head -200
echo
echo "== mcp-registry test around 229 =="
sed -n '200,250p' $(fd -t f "mcp-registry\.spec\.js" . | head -1) | cat -nRepository: wso2/api-platform
Length of output: 27923
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== enforceMTLS definition =="
sed -n '396,430p' portals/api-portal/src/middlewares/ensureAuthenticated.js | cat -n
echo
echo "== Node getPeerCertificate behavior probe (if node available) =="
node - <<'JS' || true
const tls = require('tls');
const { Duplex } = require('stream');
class NoCertDuplex extends Duplex {
constructor() {
super();
this._readableState = {};
this._writableState = {};
}
_read() {}
_write(_chunk, _encoding, _callback = () => {}) {}
getPeerCertificate() { return {}; }
}
const req = { socket: new NoCertDuplex(), body: {}, headers: {}, params: { orgName: 'org' }, session: {} };
console.log('getPeerCertificate:', JSON.stringify(req.socket.getPeerCertificate()));
console.log('keys.length:', Object.keys(req.socket.getPeerCertificate()).length);
console.log('condition true when non-null empty object:', typeof req.socket.getPeerCertificate === 'function' && req.socket.getPeerCertificate());
JS
echo "== config defaults/readers for server.https fields =="
sed -n '1,70p' portals/api-portal/src/config/configDefaults.js | cat -n
echo
sed -n '1,60p' portals/api-portal/configs/config-template.toml | cat -n
echo
rg -n -C3 "APIP_AP_SERVER_HTTPS_ENABLED|https.enabled|certFile|keyFile" portals/api-portal -g '!node_modules' | head -120Repository: wso2/api-platform
Length of output: 19005
Gate mTLS on a non-empty client certificate.
config.toml defaults enabled = true, and the Docker docker-entrypoint.sh default for APIP_AP_SERVER_HTTPS_ENABLED is also true, so TLS can terminate in the Node process. req.socket.getPeerCertificate(true) returns an empty object for TLS connections without client certificates, and an empty object is truthy; the request then enters enforceMTLS and receives the 403 plain-text response instead of the JSON 401 used by enforceSecurity and the MCP registry write tests. Change the branch to require cert && Object.keys(cert).length > 0, and return the same JSON error from enforceMTLS.
🤖 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 `@portals/api-portal/src/middlewares/ensureAuthenticated.js` around lines 100 -
101, Update the mTLS branch in the authentication middleware to store the result
of req.socket.getPeerCertificate(true) and require a non-empty certificate using
cert && Object.keys(cert).length > 0 before calling enforceMTLS. Ensure
enforceMTLS returns the same JSON error response used by enforceSecurity for
unauthenticated requests, preserving the expected 401 behavior when no client
certificate is provided.
1 participant
Purpose
$subject
Approach
Fixed the redirect url of mcp registry in unauthenticated scenario
Add tests for mcp registry