Remove logging of potentially sensitive information#155
Conversation
Rifdhan
commented
May 19, 2026
Rifdhan
commented
- Update tests
- Remove some additional dead code
![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 focuses on sanitizing logs and telemetry by removing potentially sensitive data and unnecessary debug statements across several files, including handlers.ts, streaming-utils.ts, and thoughtspot-service.ts. It also refactors TestMCPServer in the test suite to remove an unused storage parameter. Feedback from the reviewer highlights a potential runtime error when accessing properties on potentially null objects and suggests preserving minimal debugging context, such as error codes, in logs and spans to maintain observability without compromising security.
- Update tests - Remove some additional dead code
Rifdhan
force-pushed
the
remove-sensitive-logging
branch
from
4a30116 to
dac5dd0
Compare
|
@copilot Please review PR |
Reviewed the PR changes in |
| "generation_number: ", | ||
| generation_number, | ||
| "instanceUrl: ", | ||
| (this.client as any).instanceUrl, |
There was a problem hiding this comment.
The only PII I see here is instanceUrl, which we can replace with tenantId
There was a problem hiding this comment.
I feel this log is not useful anyway, we don't need to debug anything with this flow and it is also in the old V1 tool so I feel we can just remove it
| throw new Error(`Failed to store token (Status: ${storeResponse.status}): ${errorText}`); | ||
| } | ||
|
|
||
| console.log('Redirecting to:', responseData.redirectTo); |
There was a problem hiding this comment.
Should we add tenantId here? Redirecting to tenantId
There was a problem hiding this comment.
This is logged in the end user's browser while going through OAuth, I don't think we have any need to log it at all
