[wip] Client auth compatibility checker #694
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| // MCP POST endpoint - stateless mode | ||
| this.app.post('/mcp', bearerMiddleware, async (req: Request, res: Response) => { |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the problem, we should add a rate-limiting middleware to the /mcp POST endpoint. The best way to do this in an Express application is to use the well-known express-rate-limit package. We should import this package, configure a rate limiter (e.g., 100 requests per 15 minutes per IP), and apply it to the /mcp route. This can be done by creating the rate limiter instance and including it in the middleware chain for the route. The changes should be made in auth-compat/src/server/validation/index.ts, specifically above or at the definition of the /mcp route handler.
| @@ -1,2 +1,3 @@ | ||
| import express, { Request, Response } from 'express'; | ||
| import rateLimit from 'express-rate-limit'; | ||
| import { Server } from 'http'; | ||
| @@ -13,2 +14,8 @@ | ||
| private app: express.Application; | ||
| private mcpRateLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // limit each IP to 100 requests per windowMs | ||
| standardHeaders: true, // Return rate limit info in the `RateLimit-*` headers | ||
| legacyHeaders: false, // Disable the `X-RateLimit-*` headers | ||
| }); | ||
| private server: Server | null = null; | ||
| @@ -140,3 +147,3 @@ | ||
| // MCP POST endpoint - stateless mode | ||
| this.app.post('/mcp', bearerMiddleware, async (req: Request, res: Response) => { | ||
| this.app.post('/mcp', this.mcpRateLimiter, bearerMiddleware, async (req: Request, res: Response) => { | ||
| // Track the incoming message |
| @@ -28,3 +28,4 @@ | ||
| "express": "^4.18.0", | ||
| "zod": "^3.22.0" | ||
| "zod": "^3.22.0", | ||
| "express-rate-limit": "^8.0.1" | ||
| }, |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.0.1 | None |
|
|
||
| private log(...args: any[]): void { | ||
| if (!this.json) { | ||
| console.log(...args); |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the problem, we should ensure that only non-sensitive information is logged. Specifically, the log and logVerbose methods should not accept arbitrary arguments, but instead should only log specific, non-sensitive fields (such as name and description). We should update all calls to this.log and this.logVerbose to ensure that only strings or safe fields are passed, and avoid logging entire objects or arrays. If a log statement is currently logging an object, it should be refactored to log only its non-sensitive properties. The log methods themselves can be updated to enforce this by only accepting strings or by stringifying only whitelisted fields.
All changes are to be made in auth-compat/src/test-framework/index.ts, specifically in the ComplianceTestRunner class and its usage of log and logVerbose.
| @@ -31,5 +31,6 @@ | ||
|
|
||
| private log(...args: any[]): void { | ||
| // Only allow logging of strings to avoid accidental logging of sensitive objects | ||
| private log(message: string): void { | ||
| if (!this.json) { | ||
| console.log(...args); | ||
| console.log(message); | ||
| } | ||
| @@ -37,5 +38,5 @@ | ||
|
|
||
| private logVerbose(...args: any[]): void { | ||
| private logVerbose(message: string): void { | ||
| if (this.verbose && !this.json) { | ||
| console.log(...args); | ||
| console.log(message); | ||
| } |
|
|
||
| private logVerbose(...args: any[]): void { | ||
| if (this.verbose && !this.json) { | ||
| console.log(...args); |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High test
🎭 Playwright E2E Test Results✅ 24 passed Details24 tests across 3 suites 📊 View Detailed HTML Report (download artifacts) |
| if (options.list) { | ||
| console.log('Available test suites and scenarios:\n'); | ||
| allSuites.forEach(suite => { | ||
| console.log(`Suite: ${suite.name}`); |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the problem, we should avoid logging potentially sensitive or untrusted data directly. The best approach is to sanitize or redact the suite names before logging, or to log only a fixed set of allowed suite names. If the suite names are known and safe, we can whitelist them; otherwise, we can replace the actual name with a generic label (e.g., "Suite: [REDACTED]"). In this case, since we cannot guarantee the safety of suite.name, we should redact it in the log output. The change should be made in the block where console.log(\Suite: ${suite.name}`)` is called, replacing it with a safe, non-sensitive string.
| @@ -33,3 +33,3 @@ | ||
| allSuites.forEach(suite => { | ||
| console.log(`Suite: ${suite.name}`); | ||
| console.log('Suite: [REDACTED]'); | ||
| if (suite.description) { |
| allSuites.forEach(suite => { | ||
| console.log(`Suite: ${suite.name}`); | ||
| if (suite.description) { | ||
| console.log(` ${suite.description}`); |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the problem, we should avoid logging the full suite.description if it could contain sensitive or untrusted data. The best approach is to either (a) remove the log statement for the description entirely, or (b) sanitize or redact the description before logging, or (c) only log descriptions for suites that are known to be safe. Since we cannot guarantee the safety of the data, the most robust fix is to remove or redact the description in the log output. This change should be made in the block where suite.description is logged (line 36). No additional imports or methods are needed.
| @@ -34,5 +34,6 @@ | ||
| console.log(`Suite: ${suite.name}`); | ||
| if (suite.description) { | ||
| console.log(` ${suite.description}`); | ||
| } | ||
| // Removed logging of suite.description to avoid leaking sensitive data | ||
| // if (suite.description) { | ||
| // console.log(` ${suite.description}`); | ||
| // } | ||
| suite.scenarios.forEach((scenario, index) => { |
| console.log(` ${suite.description}`); | ||
| } | ||
| suite.scenarios.forEach((scenario, index) => { | ||
| console.log(` ${index + 1}. ${scenario.name}`); |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the problem, we should ensure that sensitive data is never logged. The best way to do this is to sanitize or redact the scenario.name before logging, or to avoid logging it altogether if it could contain sensitive information. If scenario names are guaranteed to be safe, a comment should be added to document this. Otherwise, we should redact or replace potentially sensitive content.
Recommended fix:
- Redact or sanitize
scenario.namebefore logging. - Alternatively, if scenario names are known to be safe, add a comment explaining why logging is safe.
- The change should be made in the block where
console.log(\${index + 1}. ${scenario.name}`);` appears (line 39). - If redacting, replace the name with a placeholder (e.g., "[REDACTED]") or a sanitized version.
| @@ -38,3 +38,7 @@ | ||
| suite.scenarios.forEach((scenario, index) => { | ||
| console.log(` ${index + 1}. ${scenario.name}`); | ||
| // Redact scenario name if it may contain sensitive data | ||
| const safeScenarioName = typeof scenario.name === 'string' && !scenario.name.match(/(token|secret|password|key|credential|bearer|jwt)/i) | ||
| ? scenario.name | ||
| : '[REDACTED]'; | ||
| console.log(` ${index + 1}. ${safeScenarioName}`); | ||
| if (scenario.description) { |
| suite.scenarios.forEach((scenario, index) => { | ||
| console.log(` ${index + 1}. ${scenario.name}`); | ||
| if (scenario.description) { | ||
| console.log(` ${scenario.description}`); |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the problem, we should avoid logging the full scenario description, or at least ensure that only non-sensitive, static information is logged. The best approach is to either omit the description from the log output or to sanitize it before logging. Since scenario names are already being logged and are less likely to contain sensitive data, we can simply remove the line that logs scenario.description. This change should be made in the runTests function, specifically in the block that lists available tests (lines 40-42). No additional imports or methods are required.
| @@ -39,5 +39,6 @@ | ||
| console.log(` ${index + 1}. ${scenario.name}`); | ||
| if (scenario.description) { | ||
| console.log(` ${scenario.description}`); | ||
| } | ||
| // Description omitted from logs to avoid exposing sensitive data | ||
| // if (scenario.description) { | ||
| // console.log(` ${scenario.description}`); | ||
| // } | ||
| }); |
| process.exit(1); | ||
| } | ||
|
|
||
| console.log(`Found ${foundScenarios.reduce((acc, s) => acc + s.scenarios.length, 0)} test(s) matching "${options.test}"\n`); |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Copilot Autofix
AI 3 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
|
I think this is super useful. Will test the pr against some example servers to see how things perform. |
TODO