Copilot commented on code in PR #1036:
URL:
https://github.com/apache/skywalking-banyandb/pull/1036#discussion_r3013571911
##########
mcp/src/index.ts:
##########
@@ -412,9 +604,41 @@ async function main() {
return;
}
+ if (MCP_AUTH_TOKEN && getAuthorizationToken(req) !== MCP_AUTH_TOKEN) {
+ res.writeHead(401, {
+ 'Content-Type': 'application/json',
+ 'WWW-Authenticate': 'Bearer',
+ });
+ res.end(JSON.stringify({ error: 'Unauthorized' }));
+ return;
+ }
+
+ const clientId = getClientIdentifier(req);
+ if (isRateLimited(clientId, Date.now())) {
+ res.writeHead(429, {
+ 'Content-Type': 'application/json',
+ 'Retry-After': `${Math.ceil(MCP_RATE_LIMIT_WINDOW_MS / 1000)}`,
+ });
+ res.end(JSON.stringify({ error: 'Too many requests' }));
+ return;
+ }
+
let body = '';
- req.on('data', (chunk) => { body += chunk; });
+ let requestTooLarge = false;
+ req.on('data', (chunk: Buffer) => {
+ body += chunk.toString();
+ if (body.length > MCP_MAX_BODY_BYTES) {
+ requestTooLarge = true;
+ res.writeHead(413, { 'Content-Type': 'application/json' });
+ res.end(JSON.stringify({ error: `Request body too large. Limit is
${MCP_MAX_BODY_BYTES} bytes.` }));
+ req.destroy();
+ }
Review Comment:
HTTP request-size enforcement uses `body.length` after `chunk.toString()`,
which counts UTF-16 code units and can diverge from actual byte size. This can
allow bodies larger than `MCP_MAX_BODY_BYTES` (or prematurely reject) and also
does extra allocations. Track bytes via `chunk.length` / `Buffer.byteLength`,
and only decode/append while under the limit (or stream into a buffer) before
parsing JSON.
##########
mcp/src/client/index.ts:
##########
@@ -163,12 +174,12 @@ export class BanyanDBClient {
const propertyResult = response.propertyResult ||
response.result?.propertyResult;
const topnResult = response.topnResult || response.result?.topnResult;
- if (streamResult) {
- // Check if it's an empty result set
- if (streamResult.elements && Array.isArray(streamResult.elements) &&
streamResult.elements.length === 0) {
- return 'Stream Query Result: No data found (empty result set)';
- }
- return `Stream Query Result:\n${JSON.stringify(streamResult, null, 2)}`;
+ if (streamResult) {
+ // Check if it's an empty result set
+ if (streamResult.elements && Array.isArray(streamResult.elements) &&
streamResult.elements.length === 0) {
+ return 'Stream Query Result: No data found (empty result set)';
+ }
Review Comment:
Formatting/indentation in `formatResponse` has an extra indentation level
for the `if (streamResult)` block and its `return`, which is inconsistent with
the rest of the method and likely to fail Prettier/ESLint formatting checks.
Reformat this block to match the surrounding style.
```suggestion
if (streamResult) {
// Check if it's an empty result set
if (streamResult.elements && Array.isArray(streamResult.elements) &&
streamResult.elements.length === 0) {
return 'Stream Query Result: No data found (empty result set)';
}
```
##########
mcp/src/index.ts:
##########
@@ -64,6 +92,59 @@ type QueryHints = {
group?: string;
};
+type RateLimitEntry = {
+ count: number;
+ windowStart: number;
+};
+
+const rateLimitByClient = new Map<string, RateLimitEntry>();
+
+function truncateText(text: string, maxLength: number): string {
+ if (text.length <= maxLength) {
+ return text;
+ }
+
+ return `${text.slice(0, maxLength)}\n... [truncated ${text.length -
maxLength} characters]`;
+}
+
+function getAuthorizationToken(req: IncomingMessage): string {
+ const authHeader = req.headers.authorization;
+ if (!authHeader) {
+ return '';
+ }
+
+ const bearerPrefix = 'Bearer ';
+ return authHeader.startsWith(bearerPrefix) ?
authHeader.slice(bearerPrefix.length).trim() : '';
+}
+
+function isLoopbackHost(host: string): boolean {
+ return host === '127.0.0.1' || host === 'localhost' || host === '::1';
+}
+
+function getClientIdentifier(req: IncomingMessage): string {
+ const forwardedFor = req.headers['x-forwarded-for'];
+ if (typeof forwardedFor === 'string' && forwardedFor.trim()) {
+ return forwardedFor.split(',')[0].trim();
+ }
+
+ return req.socket.remoteAddress || 'unknown';
+}
+
+function isRateLimited(clientId: string, now: number): boolean {
+ const currentEntry = rateLimitByClient.get(clientId);
+ if (!currentEntry || now - currentEntry.windowStart >=
MCP_RATE_LIMIT_WINDOW_MS) {
+ rateLimitByClient.set(clientId, { count: 1, windowStart: now });
+ return false;
+ }
+
+ if (currentEntry.count >= MCP_RATE_LIMIT_MAX_REQUESTS) {
+ return true;
+ }
+
+ currentEntry.count += 1;
+ return false;
+}
Review Comment:
`rateLimitByClient` grows without eviction: once a clientId appears it stays
in the Map forever, which can become an unbounded memory sink in HTTP mode
(especially since `x-forwarded-for` can be spoofed). Consider expiring/deleting
entries when their window elapses (e.g., delete on reset) and/or enforce a
maximum Map size with LRU-style eviction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
