admin
f7fc15b17c
Includes: - cli.py path fix (parents[1]) for config/catalog resolution - Library cleanup feature design docs (TODO.md, mockup) - Audit + bug-queue markdowns from May 2026 reliability pass - .gitignore expanded for transient artifacts
159 lines
8.5 KiB
Markdown
159 lines
8.5 KiB
Markdown
# Candidate Findings — Extension SW + content + manifest — audit-snapshot-2026-05-24T15-55Z.md
|
|
|
|
Scope: background.js + content.js + manifest.json
|
|
Required-reading: ext AGENTS.md / mockup / bug-audit-plan.md / project memory
|
|
Auditor: fresh Explore agent (read-only Phase 1)
|
|
|
|
---
|
|
|
|
## Candidate C-1: Race condition in maybeNotifyHostError rate-limiting
|
|
|
|
- File: background.js:188-193
|
|
- Hunch: Concurrent recordRpc calls could trigger multiple notifications within 10min due to get-then-set race.
|
|
- Trace: Two simultaneous host errors invoke recordRpc() → maybeNotifyHostError(). Both read HOST_ALERT_KEY before either writes. Both pass the now-lastTs check and both post.
|
|
- Question for verifier: Does code guarantee only one alert fires per 10min window under concurrent error paths?
|
|
- Contract refs needed: Race condition definition in bug-audit-plan.md; Chrome storage.local atomicity
|
|
|
|
---
|
|
|
|
## Candidate C-2: pending Map orphaned on SW eviction mid-call
|
|
|
|
- File: background.js:90, 124-148, 307-365
|
|
- Hunch: If SW evicts between request send and response, next instance has empty pending Map. Response arrives with no matching req_id.
|
|
- Trace: Send stores pending.set(reqId, {resolve, reject}). SW evicts. New SW has empty pending. Response at line 124-127 finds no match, dropped.
|
|
- Question for verifier: Does keepalive (20s pulse) reliably prevent SW eviction during full 90s timeout on slow remote?
|
|
- Contract refs needed: MV3 SW eviction timing vs NATIVE_CALL_TIMEOUT_MS (90s)
|
|
|
|
---
|
|
|
|
## Candidate C-3: mergeSettings shallow-merge loses missing nested keys
|
|
|
|
- File: background.js:62-76
|
|
- Hunch: Deep-merge is one level, but if stored.triggers is incomplete, Object.assign(dv, sv) loses keys not in sv.
|
|
- Trace: dv={autoPageLoad:true, autoKnownSites:false, …7 keys}. sv={autoPageLoad:true}. Result is only {autoPageLoad:true}.
|
|
- Question for verifier: Does incomplete settings blob correctly populate all missing triggers keys from defaults?
|
|
- Contract refs needed: DEFAULT_SETTINGS.triggers shape vs loaded partial settings
|
|
|
|
---
|
|
|
|
## Candidate C-4: Discord webhook URL regex insufficient
|
|
|
|
- File: background.js:232
|
|
- Hunch: Regex validates only schema+domain, not mandatory ID. URL https://discord.com/api/webhooks/ (no ID) passes validation.
|
|
- Trace: /^https:\/\/(?:discord\.com|discordapp\.com)\/api\/webhooks\//.test(url) matches prefix only.
|
|
- Question for verifier: Should regex enforce numeric ID after /api/webhooks/?
|
|
- Contract refs needed: Discord webhook URL format spec
|
|
|
|
---
|
|
|
|
## Candidate C-5: postDiscordAlert silently swallows all errors
|
|
|
|
- File: background.js:215, 268-272
|
|
- Hunch: .catch(() => {}) suppresses Discord errors with no logging or diagnostics visibility.
|
|
- Trace: Line 215 swallows all exceptions. Function catches fetch errors at 268 and records in lastDiscordSend, but callers don't see them.
|
|
- Question for verifier: Should Discord errors be logged to native RPC log or is silent swallow intentional?
|
|
- Contract refs needed: AGENTS.md or alerting design docs
|
|
|
|
---
|
|
|
|
## Candidate C-6: contextMenu handler doesn't validate tab.id
|
|
|
|
- File: background.js:895-905
|
|
- Hunch: Handler checks if (!tab) but not if tab.id is null. Missing tab.id will fail silently in checkTab.
|
|
- Trace: Line 896 checks tab, not tab.id. Line 898 calls checkTab(tab) which calls extractIdFromTab(tab) which calls chrome.tabs.sendMessage(tab.id).
|
|
- Question for verifier: Can contextMenus.onClicked pass tab with null id?
|
|
- Contract refs needed: Chrome contextMenus API contract
|
|
|
|
---
|
|
|
|
## Candidate C-12: FC2 ID regex minimum 4 digits too strict
|
|
|
|
- File: src/shared/id-extract.js:16-20
|
|
- Hunch: FC2-PPV normalizer requires 4+ digits. Pages with FC2-PPV-123 silently fail to match.
|
|
- Trace: Line 17 regex /\bFC2-?PPV-?(\d{4,})\b/i. Line 19 /\bFC2-(\d{4,})\b/i. Both need 4+ digits minimum.
|
|
- Question for verifier: Is 4-digit minimum intentional or should it be 3+?
|
|
- Contract refs needed: Real FC2 ID formats in AGENTS.md
|
|
|
|
---
|
|
|
|
## Candidate C-15: recordRpc read-modify-write race loses entries
|
|
|
|
- File: background.js:162-169
|
|
- Hunch: Concurrent recordRpc calls uncoordinated. Two errors get same old log, prepend differently, second write overwrites first.
|
|
- Trace: get(NATIVE_LOG_KEY), prepend entry, set. If two calls concurrent, second set overwrites first's entry.
|
|
- Question for verifier: Does Chrome storage.local.set serialize, or can concurrent calls lose entries?
|
|
- Contract refs needed: Chrome storage.local atomicity guarantees
|
|
|
|
---
|
|
|
|
## Candidate C-19: ensureContextMenu not called on SW init
|
|
|
|
- File: background.js:766-782
|
|
- Hunch: Context menu may not recreate after SW eviction if only called on settings changes.
|
|
- Trace: ensureContextMenu defined at line 766. Need to verify it's called at SW startup.
|
|
- Question for verifier: Is ensureContextMenu called during SW initialization?
|
|
- Contract refs needed: MV3 context menu persistence after SW eviction
|
|
|
|
---
|
|
|
|
## Candidate C-20: escapeOverlay function not found in content.js
|
|
|
|
- File: content.js (various lines use escapeOverlay but definition not visible)
|
|
- Hunch: showOverlay calls escapeOverlay at multiple points but function is not defined in the 509-line content.js.
|
|
- Trace: Lines 374-400 call escapeOverlay(...) but no definition found.
|
|
- Question for verifier: Is escapeOverlay defined in content.js or missing?
|
|
- Contract refs needed: content.js full file review; module dependencies
|
|
|
|
---
|
|
|
|
## Additional light candidates (lower priority, but noted):
|
|
|
|
C-7: nativePort assignment race (theory-level, JS atomic in practice)
|
|
C-9: Silent catch in recordActivity (observability trade-off)
|
|
C-11: Content message validation (trusted sender, acceptable design trade-off)
|
|
C-14: keepaliveTimer stale reference (uncommon race, low impact)
|
|
C-18: badgeSpinners leak under heavy load (unlikely, has onRemoved handler)
|
|
|
|
---
|
|
|
|
## Triage Summary
|
|
|
|
**High-priority for verification**: C-1, C-2, C-3, C-4, C-5, C-6, C-12, C-15, C-19, C-20
|
|
**Medium-priority (design review)**: C-7, C-9, C-11, C-14, C-18
|
|
|
|
**Focus areas for verifier**:
|
|
1. Concurrency safety of storage get-then-set patterns
|
|
2. Service worker eviction + pending request handling
|
|
3. Settings merge correctness with partial updates
|
|
4. Input validation in all entry points
|
|
|
|
---
|
|
|
|
## VERIFIER NOTES (appended after Phase 1 verification)
|
|
|
|
### C-2 (SW eviction + orphaned pending) — REFUTED
|
|
First verifier returned CONFIRMED, but did not consult Chrome `connectNative` keepalive contract.
|
|
Re-verifier (with explicit contract ref to https://developer.chrome.com/docs/extensions/develop/concepts/service-workers/lifecycle) returned REFUTED, high confidence.
|
|
Key finding: an open `connectNative` port keeps the MV3 SW alive per docs. If port closes, `onDisconnect` fires and rejects all pending (background.js:139). The orphaned-pending-Map scenario cannot occur under documented Chrome contract. The in-code `pulseKeepalive` is defensive redundancy, not load-bearing.
|
|
Caveat: if Brave is observed diverging from this contract, the symptom could manifest as a Brave-specific bug — would need a Brave-observed SW restart trace while a native port stayed active. NOT verified here.
|
|
Final status: REFUTED. Removed from bugs-extension-bg.md.
|
|
|
|
### C-15 (recordRpc race) — CONFIRMED
|
|
Verifier returned CONFIRMED, high confidence. Promoted to bugs-extension-bg.md as S-1.
|
|
|
|
### CHUNK 3 MODERATE VERIFICATION RESULTS (after stricter prompt)
|
|
|
|
- C-1 (maybeNotifyHostError rate-limit race) — CONFIRMED, M, promoted as M-1 in bugs-extension-bg.md
|
|
- C-3 (mergeSettings shallow-merge) — REFUTED. Auditor misread Object.assign arg order; defaults fill missing keys correctly. Discarded.
|
|
- C-5 (Discord errors swallowed) — PARTIAL, demoted M→L. lastDiscordSend storage write present; only passive UI display missing. Promoted as L-1.
|
|
- C-6 (contextMenu tab.id null) — REFUTED. Chrome contract guarantees non-null tab.id for registered contexts. extractIdFromTab also has defensive null check. Discarded.
|
|
- C-19 (ensureContextMenu post-eviction) — CONFIRMED, M (very high confidence). Promoted as M-2.
|
|
- C-20 (escapeOverlay undefined) — REFUTED. Function defined at content.js:451. Auditor missed it. Discarded.
|
|
|
|
CHUNK 3 CALIBRATION SUMMARY:
|
|
- Severe rejection: 1/2 = 50%
|
|
- Moderate rejection: 3/6 = 50%
|
|
- Combined: 4/8 = 50%
|
|
- Stop condition (>30% rejection) TRIGGERED. Chunk 3 audit should be restarted with tighter framing OR Light candidates should NOT be verified per current pass.
|
|
- Calibration learning: auditor over-claimed by reading code in isolation without checking platform API contracts (Chrome lifecycle, contextMenus, storage atomicity). Stricter verifier prompt with explicit contract requirement caught 3 of 4 false positives.
|