Skip to content

Commit d174940

Browse files
Fix: Address PR#863 feedback and resolve critical issues
This commit incorporates extensive feedback from PR#863, addressing several critical bugs, linting errors, and suggestions to improve stability and reliability. Key changes include: 1. **Linting Errors Resolved:** * Corrected ESLint environment to properly recognize 'chrome' global (by primarily using `Browser.*` APIs). * Removed unused functions like `prepareForForegroundRequests`. 2. **Critical Bug Fixes:** * **Sensitive Data Leakage:** Redacted sensitive information from debug logs in `src/background/index.mjs` (e.g., API keys in config). * **Textarea Value Handling:** Corrected `textContent` to `value` for `#prompt-textarea` in `src/content-script/index.jsx`. * **Unsafe Header Handling:** Ensured `onBeforeSendHeaders` in `src/background/index.mjs` returns original headers on error. * **Proxy Reconnection Loop (setPortProxy):** Implemented retry limits, exponential backoff, and proper listener cleanup for port proxy reconnections in `src/background/index.mjs`. 3. **High-Priority Review Suggestions Implemented:** * **Promise Race Condition:** Fixed race conditions in `prepareForJumpBackNotification` for Claude/Kimi token polling in `src/content-script/index.jsx`. * **Multiple Listener Registrations:** Ensured `registerPortListener` in `src/content-script/index.jsx` is called only once per lifecycle. 4. **Other Refinements:** * **Side Panel Update Logic:** Refined conditions for `sidePanel.setOptions` in `tabs.onUpdated` listener in `src/background/index.mjs`. * Reviewed and improved logging consistency in modified sections. All changes have passed `npm run lint`. These corrections aim to significantly improve the robustness and security of the extension.
1 parent 3191f53 commit d174940

File tree

2 files changed

+228
-107
lines changed

2 files changed

+228
-107
lines changed

src/background/index.mjs

Lines changed: 117 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -54,43 +54,101 @@ import { isUsingModelName } from '../utils/model-name-convert.mjs'
5454
function setPortProxy(port, proxyTabId) {
5555
try {
5656
console.debug(`[background] Attempting to connect to proxy tab: ${proxyTabId}`)
57+
// Define listeners here so they can be referenced for removal
58+
// These will be port-specific if setPortProxy is called for different ports.
59+
// However, a single port object is typically used per connection instance from the other side.
60+
// The issue arises if `setPortProxy` is called multiple times on the *same* port object for reconnections.
61+
62+
// Ensure old listeners on port.proxy are removed if it exists (e.g. from a previous failed attempt on this same port object)
63+
if (port.proxy) {
64+
try {
65+
if (port._proxyOnMessage) port.proxy.onMessage.removeListener(port._proxyOnMessage);
66+
if (port._proxyOnDisconnect) port.proxy.onDisconnect.removeListener(port._proxyOnDisconnect);
67+
} catch(e) {
68+
console.warn('[background] Error removing old listeners from previous port.proxy:', e);
69+
}
70+
}
71+
// Also remove listeners from the main port object that this function might have added
72+
if (port._portOnMessage) port.onMessage.removeListener(port._portOnMessage);
73+
if (port._portOnDisconnect) port.onDisconnect.removeListener(port._portOnDisconnect);
74+
75+
5776
port.proxy = Browser.tabs.connect(proxyTabId, { name: 'background-to-content-script-proxy' })
5877
console.log(`[background] Successfully connected to proxy tab: ${proxyTabId}`)
78+
port._reconnectAttempts = 0 // Reset retry count on successful connection
5979

60-
const proxyOnMessage = (msg) => {
80+
port._proxyOnMessage = (msg) => {
6181
console.debug('[background] Message from proxy tab:', msg)
6282
port.postMessage(msg)
6383
}
64-
const portOnMessage = (msg) => {
84+
port._portOnMessage = (msg) => {
6585
console.debug('[background] Message to proxy tab:', msg)
6686
if (port.proxy) {
6787
port.proxy.postMessage(msg)
6888
} else {
6989
console.warn('[background] Port proxy not available to send message:', msg)
7090
}
7191
}
72-
const proxyOnDisconnect = () => {
73-
console.warn(`[background] Proxy tab ${proxyTabId} disconnected. Attempting to reconnect.`)
92+
93+
const MAX_RECONNECT_ATTEMPTS = 5;
94+
95+
port._proxyOnDisconnect = () => {
96+
console.warn(`[background] Proxy tab ${proxyTabId} disconnected.`)
97+
98+
// Cleanup this specific proxy's listeners
99+
if (port.proxy) {
100+
port.proxy.onMessage.removeListener(port._proxyOnMessage);
101+
port.proxy.onDisconnect.removeListener(port._proxyOnDisconnect); // remove self
102+
}
74103
port.proxy = null // Clear the old proxy
75-
// Potentially add a delay or retry limit here
76-
setPortProxy(port, proxyTabId) // Reconnect
104+
105+
port._reconnectAttempts = (port._reconnectAttempts || 0) + 1;
106+
if (port._reconnectAttempts > MAX_RECONNECT_ATTEMPTS) {
107+
console.error(`[background] Max reconnect attempts (${MAX_RECONNECT_ATTEMPTS}) reached for tab ${proxyTabId}. Giving up.`);
108+
// Important: also clean up listeners on the main 'port' object associated with this proxy connection
109+
if (port._portOnMessage) port.onMessage.removeListener(port._portOnMessage);
110+
// Do not remove port._portOnDisconnect here as it might be the generic one for the *other* end.
111+
// This needs careful thought: is portOnDisconnect for THIS proxy instance or for the overall port?
112+
// Assuming port.onDisconnect is for the connection that initiated this proxy.
113+
// If that disconnects, it should clean up its own _portOnMessage and _proxyOnMessage etc.
114+
return;
115+
}
116+
117+
const delay = Math.pow(2, port._reconnectAttempts - 1) * 1000; // Exponential backoff
118+
console.log(`[background] Attempting reconnect #${port._reconnectAttempts} in ${delay / 1000}s for tab ${proxyTabId}.`)
119+
120+
setTimeout(() => {
121+
console.debug(`[background] Retrying connection to tab ${proxyTabId}, attempt ${port._reconnectAttempts}.`);
122+
setPortProxy(port, proxyTabId); // Reconnect (will add new listeners)
123+
}, delay);
77124
}
78-
const portOnDisconnect = () => {
79-
console.log('[background] Main port disconnected from other end.')
125+
126+
// This is the handler for when the *other* end of the 'port' disconnects (e.g. the popup closes)
127+
// It should clean up everything related to this 'port's proxying activity.
128+
port._portOnDisconnect = () => {
129+
console.log('[background] Main port disconnected (e.g. popup/sidebar closed). Cleaning up proxy connections and listeners.');
130+
if (port._portOnMessage) port.onMessage.removeListener(port._portOnMessage);
80131
if (port.proxy) {
81-
console.debug('[background] Removing listeners from proxy port.')
82-
port.proxy.onMessage.removeListener(proxyOnMessage)
83-
port.onMessage.removeListener(portOnMessage)
84-
port.proxy.onDisconnect.removeListener(proxyOnDisconnect)
85-
port.onDisconnect.removeListener(portOnDisconnect)
86-
// port.proxy.disconnect() // Optionally disconnect the proxy port if the main port is gone
132+
if (port._proxyOnMessage) port.proxy.onMessage.removeListener(port._proxyOnMessage);
133+
if (port._proxyOnDisconnect) port.proxy.onDisconnect.removeListener(port._proxyOnDisconnect);
134+
try {
135+
port.proxy.disconnect(); // Disconnect the connection to the content script
136+
} catch(e) {
137+
console.warn('[background] Error disconnecting port.proxy:', e);
138+
}
139+
port.proxy = null;
87140
}
141+
// Remove self
142+
if (port._portOnDisconnect) port.onDisconnect.removeListener(port._portOnDisconnect);
143+
// Reset for potential future use of this port object if it's somehow reused (though typically new port objects are made)
144+
port._reconnectAttempts = 0;
88145
}
89146

90-
port.proxy.onMessage.addListener(proxyOnMessage)
91-
port.onMessage.addListener(portOnMessage)
92-
port.proxy.onDisconnect.addListener(proxyOnDisconnect)
93-
port.onDisconnect.addListener(portOnDisconnect)
147+
port.proxy.onMessage.addListener(port._proxyOnMessage)
148+
port.onMessage.addListener(port._portOnMessage) // For messages from the other end to be proxied
149+
port.proxy.onDisconnect.addListener(port._proxyOnDisconnect) // When content script/tab proxy disconnects
150+
port.onDisconnect.addListener(port._portOnDisconnect) // When the other end (popup/sidebar) disconnects
151+
94152
} catch (error) {
95153
console.error(`[background] Error in setPortProxy for tab ${proxyTabId}:`, error)
96154
}
@@ -101,7 +159,21 @@ async function executeApi(session, port, config) {
101159
`[background] executeApi called for model: ${session.modelName}, apiMode: ${session.apiMode}`,
102160
)
103161
console.debug('[background] Full session details:', session)
104-
console.debug('[background] Full config details:', config)
162+
// Redact sensitive config details before logging
163+
const redactedConfig = { ...config };
164+
if (redactedConfig.apiKey) redactedConfig.apiKey = 'REDACTED';
165+
if (redactedConfig.customApiKey) redactedConfig.customApiKey = 'REDACTED';
166+
if (redactedConfig.claudeApiKey) redactedConfig.claudeApiKey = 'REDACTED';
167+
if (redactedConfig.kimiMoonShotRefreshToken) redactedConfig.kimiMoonShotRefreshToken = 'REDACTED';
168+
// Add any other sensitive keys that might exist in 'config' or 'session.apiMode'
169+
if (session.apiMode && session.apiMode.apiKey) {
170+
redactedConfig.apiMode = { ...session.apiMode, apiKey: 'REDACTED' };
171+
} else if (session.apiMode) {
172+
redactedConfig.apiMode = { ...session.apiMode };
173+
}
174+
175+
176+
console.debug('[background] Redacted config details:', redactedConfig)
105177

106178
try {
107179
if (isUsingCustomModel(session)) {
@@ -435,7 +507,7 @@ try {
435507
error,
436508
details,
437509
)
438-
return {} // Return empty object or original headers on error?
510+
return { requestHeaders: details.requestHeaders } // Return original headers on error
439511
}
440512
},
441513
{
@@ -448,22 +520,40 @@ try {
448520

449521
Browser.tabs.onUpdated.addListener(async (tabId, info, tab) => {
450522
try {
451-
if (!tab.url || !info.url) { // Check if tab.url or info.url is present, as onUpdated can fire for various reasons
452-
// console.debug('[background] onUpdated event without URL, skipping side panel update for tab:', tabId, info);
523+
// Refined condition: Ensure URL exists and tab loading is complete.
524+
if (!tab.url || (info.status && info.status !== 'complete')) {
525+
console.debug(
526+
`[background] Skipping side panel update for tabId: ${tabId}. Tab URL: ${tab.url}, Info Status: ${info.status}`,
527+
)
453528
return
454529
}
455530
console.debug(
456-
`[background] tabs.onUpdated event for tabId: ${tabId}, status: ${info.status}, url: ${tab.url}`,
531+
`[background] tabs.onUpdated event for tabId: ${tabId}, status: ${info.status}, url: ${tab.url}. Proceeding with side panel update.`,
457532
)
458-
if (chrome && chrome.sidePanel) {
459-
await chrome.sidePanel.setOptions({
533+
// Use Browser.sidePanel from webextension-polyfill for consistency and cross-browser compatibility
534+
if (Browser.sidePanel) {
535+
await Browser.sidePanel.setOptions({
460536
tabId,
461537
path: 'IndependentPanel.html',
462538
enabled: true,
463539
})
464-
console.debug(`[background] Side panel options set for tab ${tabId}`)
540+
console.debug(`[background] Side panel options set for tab ${tabId} using Browser.sidePanel`)
465541
} else {
466-
console.debug('[background] chrome.sidePanel API not available.')
542+
// Fallback or log if Browser.sidePanel is somehow not available (though polyfill should handle it)
543+
console.debug('[background] Browser.sidePanel API not available. Attempting chrome.sidePanel as fallback.')
544+
// Keeping the original chrome check as a fallback, though ideally Browser.sidePanel should work.
545+
// eslint-disable-next-line no-undef
546+
if (chrome && chrome.sidePanel) {
547+
// eslint-disable-next-line no-undef
548+
await chrome.sidePanel.setOptions({
549+
tabId,
550+
path: 'IndependentPanel.html',
551+
enabled: true,
552+
})
553+
console.debug(`[background] Side panel options set for tab ${tabId} using chrome.sidePanel`)
554+
} else {
555+
console.debug('[background] chrome.sidePanel API also not available.')
556+
}
467557
}
468558
} catch (error) {
469559
console.error('[background] Error in tabs.onUpdated listener callback:', error, tabId, info)

0 commit comments

Comments
 (0)