-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Debug extension bridge url #8347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found some issues that need attention.
| if (!instance) { | ||
| try { | ||
| console.log(`[BridgeOrchestrator#connectOrDisconnect] Connecting...`) | ||
| console.log(`[BridgeOrchestrator#connectOrDisconnect] Connecting to ${options.socketBridgeUrl}...`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Logging the full socketBridgeUrl can leak sensitive information (query params like token/key, or basic-auth). Also handle undefined gracefully. Suggest redacting and adding a fallback.
| console.log(`[BridgeOrchestrator#connectOrDisconnect] Connecting to ${options.socketBridgeUrl}...`) | |
| console.log(`[BridgeOrchestrator#connectOrDisconnect] Connecting to ${(() => { try { const u = new URL(options.socketBridgeUrl ?? ''); u.username = ''; u.password = ''; return u.origin + u.pathname + (u.search ? '?…redacted…' : '') + u.hash; } catch { return options.socketBridgeUrl ?? '(unknown)'; } })()}...`) |
| if (!instance) { | ||
| try { | ||
| console.log(`[BridgeOrchestrator#connectOrDisconnect] Connecting...`) | ||
| console.log(`[BridgeOrchestrator#connectOrDisconnect] Connecting to ${options.socketBridgeUrl}...`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3: The log tag uses "connectOrDisconnect" but this is inside connect(). Consider harmonizing the tag to reduce confusion (follow-up change OK).
|
Closing as stale |
Important
Update log message in
BridgeOrchestratorto includesocketBridgeUrlfor better debugging context.connect()method ofBridgeOrchestratorto includesocketBridgeUrlfor better debugging context.This description was created by
for a421812. You can customize this summary. It will automatically update as commits are pushed.