🛡️ Sentinel: [security improvement] Prevent sensitive data leak in HTTP client error logging#46
🛡️ Sentinel: [security improvement] Prevent sensitive data leak in HTTP client error logging#46badMade wants to merge 1 commit into
Conversation
Modifies `console.error` statements in the HTTP client and proxy to log safe properties (message, name, status) instead of full Axios/HTTP error objects. This prevents sensitive information like `Authorization` headers or API keys embedded in the request configuration from being leaked to logs. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves security and log clarity by sanitizing error logs in the HttpClient and MCPProxy classes. Instead of logging full error objects, which could potentially leak sensitive information from headers or configurations, the code now logs specific safe properties such as message, name, and status. A review comment identifies a redundant log statement in the MCPProxy tool call handler that should be removed to reduce log noise, as the error information is already captured in a preceding log entry.
| if (error instanceof HttpClientError) { | ||
| console.error('HttpClientError encountered, returning structured error', error) | ||
| console.error('HttpClientError encountered, returning structured error', { | ||
| message: error.message, | ||
| status: error.status, | ||
| }) | ||
| const data = error.data?.response?.data ?? error.data ?? {} |
There was a problem hiding this comment.
This log statement is redundant. The error has already been logged at line 102 with the same information (message, name, and status). Removing this second log will reduce noise in the log stream while still providing the necessary diagnostic information.
if (error instanceof HttpClientError) {
const data = error.data?.response?.data ?? error.data ?? {}1 participant
🛡️ Sentinel: [MEDIUM] Fix sensitive data leak in HTTP client error logging
🚨 Severity: MEDIUM
💡 Vulnerability: When Axios throws an error, the full error object includes the request configuration, which contains HTTP headers. Logging the entire error using
console.error(error)exposes anyAuthorizationheaders, API tokens, or secrets to the log stream.🎯 Impact: If these logs are ingested by a centralized logging system, secrets are leaked in plain text, potentially leading to unauthorized access.
🔧 Fix: Modified the
console.errorstatements insrc/openapi-mcp-server/client/http-client.tsandsrc/openapi-mcp-server/mcp/proxy.tsto log an object containing only safe, destructured properties (e.g.,error.message,error.name,error.response?.status).✅ Verification:
npx vitest run --passWithNoTeststo verify no regressions in tests.message,name, andstatus, omitting the sensitive request configuration completely.PR created automatically by Jules for task 16030927670522045557 started by @badMade