fix: close an auth bypass, stop tools reporting failure as success, remove unreachable code#59
Merged
Merged
Conversation
Deletions of code that never executes or is never read. No user- or model-visible string changes, so tool output is byte-identical. - server.js: unused `createServer` import; `killExisting` (parsed, never read — the kill at startup is unconditional); `organizedTabs`; `currentTime`/`oneHourAgo` (computed, never used — the archive_old rule is the tab.index heuristic below them) - server.js: the `settings_used` block in formatTabCreateResult. The extension's createTabsBatch never sets that field, so the branch could not be entered. - background.js: `generateTestUrls`, `estimateBatchTime` — no call sites - content.js: `estimateTokenUsage` — no call sites; unused `startTime` - popup.js: `toolsList` — built and never referenced - server.js: strip `ADD:`/`START:` prefixes and a stale port-migration note from comments describing changes rather than code Gate: node --check, build, validate, structure tests, web-ext lint all green; web-ext warning count unchanged at 3.
…ing open
`args.find(a => a.startsWith('--token='))` matches the bare flag, so
`--token=` produced AUTH_TOKEN = "" and requireToken's `if (!AUTH_TOKEN)
return next()` waved every request through. The startup banner is gated on
`if (AUTH_TOKEN)` too, so nothing was printed: `--tunnel --token=` published
an unauthenticated browser-control endpoint with no visible sign. This
defeated the loopback/token boundary added in ea71bb9.
`--http-host=` had the same falsy-vs-nullish bug, binding 0.0.0.0 via
`app.listen(PORT, "")`. Together (`--http-host= --token=`) they opened
remote browser control to the network.
Parse flags through argValue(), which returns null when a flag is absent
and errors when its value is empty, so an empty value can no longer be
mistaken for "not set". Ports go through portValue(): digits only, since
Number() accepts '0x1f' and parseInt() accepts '80abc'. A bad port used to
reach net.listen(NaN), which throws synchronously and — with no .catch on
startServer() — died as an unhandled rejection; startServer() now reports
the real message and exits 1.
Verified: --token= / --http-host= / --port=abc / --port=0x10 each exit with
a named error; default startup unchanged; --http-host=0.0.0.0
--token=mysecret123 binds 0.0.0.0 and returns 401 / 401 / 200 for
missing / wrong / correct bearer tokens.
An MCP client acts on what these tools report, so a failure dressed as an empty result is worse than an error. get_history: the catch returned history_items: [], and formatHistoryResult branches only on that array being empty -- it never reads success/error. A permissions failure rendered as "No history items found" with isError:false. Now rethrows, so it reaches the JSON-RPC error boundary with its message. get_selected_text: four failure paths (tab missing, no active tab, script returned nothing, outer catch) returned objects lacking has_selection, which formatSelectedTextResult renders as "No text selected". Now throw. The genuine no-selection case still returns success with has_selection: false. page_navigate: a failed wait condition returned success:true plus a warning field the formatter never read, so the model was told navigation succeeded. The warning is now surfaced (tab_create already did this). Research workflow: appended "Current page bookmarked for reference" unconditionally after swallowing the bookmark failure into console.warn. Now reports what actually happened. Connection handling: createConnection resolved without waiting for the socket to open and swallowed failures, so handleMCPRequest proceeded into the tool switch anyway -- tab_close and element_click executed for real while the reply was written to a CONNECTING socket and dropped. The model saw only "Tool call timeout" and would reasonably retry a side effect. createConnection now awaits open and rethrows; the Chrome branch reuses a live socket instead of replacing the one the request arrived on; send() throws rather than dropping a frame; fire-and-forget reconnect paths catch explicitly; the popup's reconnect button reports the real outcome. Verified: full gate green (0 errors, same 3 pre-existing web-ext warnings, 19 structure markers unchanged); end-to-end round trip over WS + /sse confirms page_navigate renders the warning instead of success. NOT verified: the connection-lifecycle changes need a manual load in Chrome and Firefox -- no automated coverage exists for them.
Two subsystems that no caller could reach.
content.js -- the enhanced-pattern tier never ran. Its guard read
`bestMethod`, an identifier declared nowhere in the repo, so evaluating it
threw ReferenceError on every page where the anti-detection branch had not
already filled quickMatches. The catch below logged "Enhanced patterns
failed" and fell through to quickViewportScan, so the failure was invisible
and tryEnhancedPatterns never executed once.
The legacy single-phase engine was unreachable for a different reason:
analyzePage only reaches it when `phase` is neither "discover" nor
"detailed", but phase defaults to "discover" and is declared
enum: ["discover", "detailed"] in both the extension's registry and the
server's fallback copy -- so reaching it required a schema-forbidden value.
Removed as one closed set, since legacyAnalysis was the only other caller of
tryEnhancedPatterns: legacyAnalysis, tryEnhancedPatterns, ENHANCED_PATTERNS,
parseIntent, findPatternElements, tryUniversalPatterns, tryPatternDatabase,
detectSite, getUniversalPattern, PATTERN_DATABASE, trySemanticAnalysis,
formatAnalysisResult, and isVisible (superseded everywhere by
isLikelyVisible). The dispatch is now total on the declared enum.
server.js -- the prompts system was unreachable per the MCP spec: the
initialize response declares capabilities: { tools: {} } with no prompts
capability, and prompts/get returned { content: [...] } where the spec
requires { messages: [{ role, content }] }. Removed prompts/list,
prompts/get and the six execute*Workflow functions.
page_analyze output is unchanged: no removed code could execute.
Verified: no dangling references to any removed symbol; gate green (0
errors, same 3 pre-existing web-ext warnings, 19 structure markers); live
round trip over WS + /sse still registers tools, answers tools/list, and
renders page_navigate correctly.
build.js: `case 'validate'` discarded validateAllBuilds()'s promise, so the CI "Validate builds" step passed regardless of the result. Now exits 1 on failure — verified by breaking dist/chrome/manifest.json and confirming a non-zero exit. navigateToUrl: destructured tabs.query() and used activeTab.id with no guard, so page_navigate crashed when no normal window has focus (devtools undocked, all windows minimized). Every other caller of the same query already guards; this one now does too. register frame: `availableTools = message.tools` trusted the peer, and any loopback client could send a register with no tools — leaving availableTools undefined and making every later tools/list throw until restart. Now validated as an array; verified tools/list survives a malformed frame. Divide-by-zero: summarizeResults, summarizePosts and calculateQualityScore divided by results.length/posts.length, which are 0 when an extractor matches nothing — shipping avg_score: NaN and "NaN%" to the model. Dead protocol halves: the background's `action: "test"` handler had no sender and the message it emitted had no server-side branch; the popup's `statusUpdate` listener had no sender (status comes from polling). setupViewportAnalyzer: built an IntersectionObserver that never observed anything and a visibilityMap that was never read — one dead allocation per page load. isLikelyVisible does this work. popup: said 17 tools and omitted page_style while the extension registers 18. Name list and count now derive from one array so they can't disagree; verified the list covers all 18 registered tools. logo.webm: referenced by build.js and popup.html but absent from the repo, so the copy was permanently skipped and the <source> never matched. EXTENSION_ORIGIN: the same regex was inlined in the WebSocket handshake check; now uses the existing constant. Gate green: 0 errors, same 3 pre-existing web-ext warnings, 19 structure markers; live round trip unchanged.
…ect ceiling formatScrollResult read scroll_position, wait_time, element_scrolled and page_dimensions — none of which scrollPage returns. It emits new_position and wait_after, so the position and wait were silently dropped from every scroll result while four branches sat permanently false. Remapped the two that have real counterparts; removed the two that don't. formatTabListResult read result.active_tab, but listTabs returns it under summary. "Active tab" therefore always rendered "None". Now reads result.summary?.active_tab. handlePageStyle destructured intensity and duration without defaults, while the tool schema declares 'medium' and 10. applyEffect gates on `duration > 0`, so effects were never auto-removed and the description rendered "effect for undefineds". Defaults now match the schema. Reconnect: createConnection reset reconnectAttempts to 0 after port discovery, which runs whenever attempts > 2 — so the counter oscillated 0->3->0, backoff never grew past ~40s, and the "Maximum reconnection attempts reached" guard was unreachable. The genuine reset in onopen stays. Version: initialize advertised 2.0.0 and the SSE hello 1.0.0 while package.json said 1.1.1. Single SERVER_VERSION constant; verified initialize now reports 1.1.1, matching package.json. Gate green: 0 errors, same 3 pre-existing web-ext warnings, 19 markers; live round trip unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Output of a repo-wide cleanup pass. It turned up more defects than cruft, so
most of this is fixes; the deletions are the parts that provably could not run.
Net: −1527 / +260 across 7 commits, kept separate so any one can be reverted alone.
Security — please read first
00188f2--token=disabled authentication entirely. The flag is matched withstartsWith('--token='), so a bare--token=yielded an empty token, and the requestguard treats an empty token as "no auth configured" and passes every request through.
The startup banner is gated the same way, so nothing is printed —
--tunnel --token=published an unauthenticated browser-control endpoint with no visible sign. This
defeated the boundary added in #55.
--http-host=had the same falsy-vs-nullish bugand bound all interfaces; together they opened remote browser control to the network.
Verified:
401 / 401 / 200for missing / wrong / correct bearer token, and eachmalformed flag now exits with a named error instead of an unhandled rejection.
Tools no longer report failure as success
8b4cdbaAn MCP client acts on what these tools say, so a failure dressed as an emptyresult is worse than an error.
get_historyisError: falseget_selected_textpage_navigatewarningfield was set and never read)Same commit: a dropped connection no longer executes tools anyway.
createConnectionresolved without waiting for the socket to open and swallowed failures, so
tab_closeand
element_clickran against the browser while the reply went to a still-connectingsocket and was discarded — the model saw a generic "Tool call timeout" and would
reasonably retry a side effect.
Removed — unreachable, not merely unused
9db31eaThe enhanced-pattern tier never executed: its guard readbestMethod, anidentifier declared nowhere in the repo, so it threw on every call and the catch below
swallowed it, silently degrading
page_analyzeto a viewport scan. The legacysingle-phase engine went with it — reaching it required a
phasevalue both toolschemas forbid. The
prompts/*workflows were unreachable per spec: nopromptscapability was ever declared, and
prompts/getreturned{content:[...]}where thespec requires
{messages:[...]}.page_analyzeoutput is unchanged, because none of the removed code could run.Fixes and quick wins
6fe5294CI's "Validate builds" step could not fail (validateAllBuilds()'s promise wasdiscarded) — now exits 1, verified by deliberately breaking a manifest ·
page_navigatecrashed when no window has focus · a malformed
registerframe from any loopback clientbroke
tools/listuntil restart ·NaNshipped to the model from divide-by-zero · twodead protocol halves · a dead
IntersectionObserverper page load · popup said 17 toolsand omitted
page_style.6c859a6page_scrollsilently dropped its position data (formatter read field names thecontent script never emits) ·
tab_listalways said "Active tab: None" ·page_styleeffects never expired (schema defaults weren't applied) · the reconnect give-up guard
was unreachable · three disagreeing version strings unified.
Verification
node --check, build,build.js validate, structure tests andweb-ext lintare green:0 errors, the same 3 pre-existing warnings, 19 structure markers. Behavior was checked by
running it — auth enforcement, flag validation, a live WS +
/sseround trip, themalformed-register guard, and version consistency.
One thing that is NOT verified
The connection-lifecycle changes in
8b4cdba— awaiting socket open,send()throwing,the Chrome socket-reuse guard — need a manual load in Chrome and Firefox. This repo has
no behavioral test coverage, so a green gate proves "still parses", not "still works".
That commit is deliberately separable.
Related and still open: Safety Mode may not work on Firefox at all (
background.jsuses the Chrome callback form of
storage.local.get, and the vendored polyfill throwson over-arity) — worth confirming in the same manual pass.