Migrate to Manifest V3 — restores the extension for Google Chrome - #74
Open
episanty wants to merge 47 commits into
Open
Migrate to Manifest V3 — restores the extension for Google Chrome#74episanty wants to merge 47 commits into
episanty wants to merge 47 commits into
Conversation
…ked full source from zip file into fullSource directory.
…urces.js. Works, but background_load.js is not mv3 safe.
…r mv3 edits. For now, keeping both files with the same code as at the start.
… a background_load_resources.js file which will require the mv3 edits.
…e per change. Backfills the six existing migration commits with why each was needed, tracks known MV2/MV3 breaking-change categories per file, and records outstanding work so reviewers can verify progress without re-deriving it from the diff. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…service worker. Both files were in the original MV2 background.scripts list and are needed for background_interaction.js's calls to BINParser/BINSchema to resolve. Audited clean of DOM APIs and other MV3-incompatible patterns, so this is a straight importScripts() addition with no source edits to either file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…b.js wiring. Marks checklist items 8 and 11 as advanced, adds items 15-17 for the background_interaction.js work (DOM script-injection rework and the two chrome.* API renames) discovered while scoping that file, and records the outstanding-work lean on the module-state persistence question. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nd_interaction.js.
flushAllGlobals() and loadAdjusterScripts() previously loaded one of the 76
extractors/prefselectors/*.js (+ optional background/preformatters/*.js)
files by appending a <script> tag to the background page's own document and
waiting on its .onload event. A service worker has no document, so this
replaces both with importScripts(chrome.runtime.getURL(...)), which is
synchronous -- the .onload-callback wrapper in loadSiteAdjusters() collapses
into an immediately-invoked function, run right after the (now-blocking)
script load instead of on a later event.
Kept as close to the original behavior as possible:
- Per-source loading stays best-effort (a failed importScripts is caught and
logged, exactly as a load failure previously just left the script's global
unset/stale without interrupting the rest of the flow).
- The returned "attempted" flags mirror the old "was a <script> tag created"
semantics (not "did it succeed"), since that's what parsedData.preformatting
and the onload-vs-not branch were actually keyed on before.
- flushAllGlobals() still only resets BINPrefselector, matching original
behavior -- BINPreformatter was never actually cleared by the old DOM
cleanup either (removing a <script> element doesn't undefine an
already-executed global); left as-is with a comment explaining why, so a
future reader doesn't "fix" it into a behavior change.
Added console.log/console.error around each importScripts() call as a
temporary diagnostic, since this can't be exercised through the real popup
flow yet (injectScript() upstream still uses the MV2-only
chrome.tabs.executeScript and hasn't been migrated). Verify via the service
worker's own devtools console instead:
importScripts(chrome.runtime.getURL('/extractors/prefselectors/wiki.js'));
typeof BINPrefselector // => 'object'
Not yet done: background_interaction.js is still not wired into
service-worker.js's importScripts list (kept out until the remaining
deprecated API calls -- chrome.browserAction.openPopup,
chrome.tabs.executeScript, chrome.extension.getURL -- are also fixed).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Marks checklist items 11 and 15 done, adds the log entry explaining what was kept behavior-equivalent and why, and records how to verify the change in isolation via the service worker console given injectScript() still blocks the real popup flow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…s.js. Addresses checklist item 9: this file still held the original MV2 code, using XMLHttpRequest, which does not exist in a service worker -- it threw immediately on import, which is why service-worker.js failed to register at all (Chrome reported "Service worker registration failed. Status code: 15"). Three of the five resource loads (urlSpecificAdjusterList.json, publisherInfo.json, schemata.json) already fetched JSON and convert mechanically: XMLHttpRequest -> fetch(chrome.runtime.getURL(...)), same try/catch-around-JSON.parse structure and per-file fallback defaults kept as-is. The other two (charTable.xml, journalAbbrevs.xml) needed more thought: DOMParser is not available in this service worker either (confirmed empirically via the service worker's own console before writing this), and tracing their consumption in background_resources.js showed real DOM Element usage downstream (getElementById, getAttribute, getElementsByTagName, .innerHTML) -- not simple text extraction. Rather than hand-roll an XML parser for a data path exercised on every character of every extracted citation, both files are now pre-converted to JSON via nameResources/convert_xml_to_json.py (Python's standard-library xml.etree.ElementTree, not hand-rolled parsing), with the script and its output (charTable.json: 2480 entries, journalAbbrevs.json: 29 letter groups/ 20556 journals) committed together so the conversion can be re-run and diffed. The two consuming call sites in background_resources.js are updated from DOM-Element-method style to plain object/array access (getElementById -> object property, getElementsByTagName+innerHTML -> plain field, querySelector -> Array.find) -- verified against the actual XML that this is behaviorally exact for the current data (entity-decoding included; the handful of XML entities in the source all correspond to character codes already hardcoded/special-cased earlier in convertSpecialChars(), so they never reach the DOM-vs-plain-object code path either way). The exactly-10-calls contract that background_load_core.js's backgroundState/backgroundStateFinal readiness gate depends on is preserved: each of the 5 resource loads still calls BINData.increaseBackgroundState() at the same logical completion points (including the 4 nested chrome.storage.local.get calls chained after charTable.json loads), just inside a .then() instead of an XHR onreadystatechange handler. A failed fetch/parse is caught and logged rather than calling increaseBackgroundState, matching the original's silent non-completion on a non-200 XHR response. service-worker.js's importScripts list is restored to include background_load_resources.js (previously commented out as a temporary diagnostic to unblock service worker registration while checking DOMParser/fetch availability in the console) -- net no diff against the already-committed version. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rite (4b5b973). Marks checklist item 9 done, adds item 18 for the newly-discovered XMLHttpRequest call in background_interaction.js's validateCitationDownload (separate, self-contained, not yet addressed), and records the full rationale for the XML->JSON conversion decision and the readiness-gate constraint it had to preserve. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Script; wire background_interaction.js into the service worker. Addresses checklist item 17. injectScript() is the very first step in the extraction flow (injects the meta-extractor into the target tab), so it blocked reaching everything downstream -- including the importScripts-based site-adjuster loading fixed in c3a55c7 -- through the real popup flow. chrome.scripting.executeScript()'s result array holds {result, frameId, documentId} objects rather than the raw per-frame values the old API gave back, so that's unwrapped inside injectScript() itself (results.map(r => r.result)) -- callers, including loadSiteAdjusters()'s arr[0], keep working unchanged. Left as-is, not "fixed": onError is accepted as a parameter here but never actually invoked, same as sendMsg() elsewhere in this file -- checked, this is a consistent pattern across the file, not an isolated oversight, so no new call to onError was introduced. background_interaction.js is now wired into service-worker.js's importScripts -- previously held back because of items 16/17/18 (this commit resolves 17). Items 16 (chrome.browserAction.openPopup, only reached via the Alt+W/Alt+Q shortcuts) and 18 (XMLHttpRequest in validateCitationDownload, only reached via a specific dynamic-download retry path) remain unfixed but are off the path of a basic extraction, so wiring the file in now is safe for that. Also fixed the new self-check added for this file: BINInteraction's IIFE has no return statement (it runs purely for its listener-registration side effects, unlike the other BIN* modules), so `typeof BINInteraction === 'object'` would have wrongly read as a load failure. Checks chrome.runtime.onMessage.hasListeners() instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ac1ed3). Marks checklist items 2, 8, and 17 done, updates items 10 and 15 to reflect that background_interaction.js is now live in the service worker (raising the urgency of the state-persistence decision and making end-to-end verification the next concrete step), and records the onError/sendMsg consistency check and the BINInteraction self-check fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… importScripts(). Supersedes the c3a55c7 design: testing against a real site (github.com) showed dynamic importScripts() of a new file fails in this environment -- "NetworkError: ... failed to load" -- even for a trivial one-line test file, confirmed with zero async nesting (a flat call typed directly into the service worker console). This contradicts Chrome's documentation that extension service workers can call importScripts() "at any time," so the c3a55c7 approach (fetch the matched site's script on demand when a page is visited) doesn't work regardless of how it's wired into the call chain. Static importScripts() -- the list at the top of service-worker.js -- does work, since that's exactly how the 7 other background files already load. So instead of loading a script on demand, all 156 site-adjuster scripts (76 extractors/prefselectors/*.js + 80 background/preformatters/*.js) are now loaded statically at service worker startup via a generated manifest (background/adjuster_scripts_manifest.js, listed in service-worker.js's importScripts). Combined size is 664K -- a non-issue at startup. This only works because every one of the 156 files declares the exact same shared global (var BINPrefselector = .../var BINPreformatter = ...) -- verified with zero exceptions before proceeding. Loading all of them under that shared name at once would have each subsequent file silently clobber the previous one's global, so this commit's companion (bulk-renamed-in-a separate commit) gives each file its own unique global (BINPrefselector_<name>/BINPreformatter_<name>) via rename_adjuster_globals.py, which also generates the manifest. background_interaction.js's loadAdjusterScripts() changes from "fetch a script by path" to "look up the already-loaded global by name" (self["BINPrefselector_" + name]), assigning the match into the shared BINPrefselector/BINPreformatter bindings the rest of this file and background_parse_bib.js already read from -- nothing downstream of that selection needed to change. loadSiteAdjusters()'s call site simplifies too, since there's no path to construct any more, just names to pass through. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Mechanical, generated companion to 691b3e4: every file in extractors/prefselectors/ (76) and background/preformatters/ (80) gets its single top-level declaration renamed from the shared var BINPrefselector/BINPreformatter to a unique var BINPrefselector_<name>/BINPreformatter_<name>, via rename_adjuster_globals.py -- re-run that script to regenerate this commit's changes rather than hand-editing. Exactly one line changed per file; everything else in each file (the actual selector/formatting logic contributed by the community) is untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Rewrites checklist item 15's row to tell the full story: the c3a55c7 approach didn't survive contact with a real site, and documents the empirical isolation steps that ruled out async-nesting depth and file-specific issues before landing on dynamic importScripts() being unavailable outright. Adds the full log entry and updates outstanding work to point at re-testing against github.com next. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
First clean run since the MV3 migration began: reloaded, revisited the same github.com page that originally surfaced the dynamic-importScripts() failure, re-triggered extraction, no errors, real bibliographic data returned. Confirms checklist items 8, 9, 11, 15, and 17 work together. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…opup. Addresses checklist item 16. Two call sites in the Alt+W/Alt+Q auto-copy/ auto-download command handler -- chrome.browserAction doesn't exist under MV3. chrome.action.openPopup() is promise-based rather than callback-based; converted 1:1 (.then() for the old success branch, .catch() for the old chrome.runtime.lastError branch) to keep the diff easy to verify against the original rather than collapsing it, even though both branches were (and still are) a no-op either way. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Marks checklist item 16 done, records the 1:1 promise-conversion rationale, and updates outstanding work to flag the Alt+W/Alt+Q shortcuts as needing manual testing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tion quirk. Alt+W/Alt+Q now confirmed working after manually binding them in chrome://extensions/shortcuts (they showed "Not set" despite the manifest's suggested_key -- a Chrome-side conflict-skipping behavior, not a code issue). Also hit and resolved a separate, unrelated quirk where Alt+C (_execute_action, handled entirely by Chrome, untouched by this migration) needed to be manually re-bound despite showing as already assigned -- recorded so it doesn't get mistaken for a regression later. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses checklist item 18, the last MV3-blocking API in this file. This is the fallback path that retries a citation download directly from the background context (bypassing the tab) when the content script's own attempt reports status -2. Behavior preserved: - withCredentials -> credentials: 'include'. - XHR's .timeout: on expiry it silently falls through to this function's own non-200 branch (no explicit ontimeout handler existed), so an aborted fetch hitting .catch() -> parseMetaData(null, metaData) lands on the same outcome. Only starts an abort timer for a positive timeout value, matching XHR's own "0/unset means no timeout" default. - .responseText.slice(0,200000) -> response.text() then the same slice. - The exact `status === 200` check (not an .ok/2xx-range check). - Request body sent only when non-empty, same condition as before. Not fixed, deliberately kept: the second setRequestHeader call sets "Content-Type" a second time rather than "Cookie", despite its variable name/comment -- looks like a pre-existing copy/paste bug. Left as-is because "Cookie" is a forbidden header name blocked from script-based setting either way (true for both XHR and fetch), so this line never actually set a cookie regardless -- fixing the header name now would be a first-time behavior change smuggled into what's meant to be a platform migration, not a bugfix pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…te (489a25e). Marks checklist item 18 done -- the last item in the breaking-changes checklist besides item 10 (module-state persistence). Records the preserved-timeout/credentials/status-check rationale and the deliberately unfixed Content-Type/Cookie header bug, and flags manual testing of this path as outstanding (needs a dyn_download site hitting the -2 retry case). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e article. Tested both branches of the -2 retry path against nature.com/articles/s41566-024-01499-8: CORS-blocked-before-permission (caught cleanly, surfaced through the existing "Failed!" + retry/allow UI as designed) and successful-after-permission (real data returned, headers and credentials working correctly). This was the last checklist item needing runtime verification -- everything remaining is item 10 (module-state persistence, a design decision, not yet implemented) and the deferred test matrix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tarts. Addresses checklist item 10, the last open item. A non-persistent service worker can be torn down and restarted between events at any time, silently resetting these two module-level vars -- unlike MV2's persistent background page. That's a real, live risk now that background_interaction.js is wired in: opening the options page and leaving it open for a while (well past a plausible idle-suspend window) before closing it would silently break the backNavigation feature (switching back to the tab you were on before opening options), since optionPageId would read back as null and updateTab()'s tab-switch-back branch would never fire. Persisted to chrome.storage.session (cleared when the browser closes, unlike .local -- appropriate for this ephemeral, per-session bookkeeping; already covered by the existing "storage" permission, no manifest change needed). The two vars stay the synchronous, authoritative in-memory copy for the service worker's current lifetime -- everything that reads them (the backNavigation check, etc.) is unchanged. The only new requirement is that the three functions which read/write them (updateTab, resetTabMemory, openOptionPage) wait on a startup rehydration promise (tabMemoryReady) first, since there's no guarantee it resolves before a listener that just woke the service worker up runs. Also fixed a related gap: openOptionPage() dynamically registers a chrome.tabs.onActivated listener (resetTabMemory) whenever an option-page tab is open, but that registration doesn't itself survive a restart the way the persisted variables now do. Re-added on startup whenever restored state says an option page tab is still open, preserving the invariant that this listener is active exactly when optionPageId is non-null. The other four module-level vars in this file (parsedData, allowAutoCopy, allowAutoDownload, parsedDataContainer) are deliberately left as plain, non-persisted state, per the lean already recorded in MIGRATION.md: the worst case of losing them on a restart is a harmless re-extraction or a momentarily-stale cache, not a silently broken feature. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Marks checklist item 10 done -- the last item in the full breaking-changes checklist. Records the tabMemoryReady rehydration design, the resetTabMemory listener re-registration fix, and flags manual testing (force-terminating the service worker) as the next concrete step, along with revisiting the deferred per-site test matrix now that core functionality is complete. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ionPage(). Corrects a wrong call made in checklist item 14's original audit: that audit concluded chrome.extension.getURL was "not an MV3 blocker, just a deprecated alias." It's not just deprecated -- chrome.extension is unavailable in a service worker at all. Surfaced by actually testing openOptionPage() (clicking "Global Options->" in the popup), which threw "TypeError: chrome.extension.getURL is not a function" and silently did nothing from the user's perspective (the error only appeared in the service worker console, not the popup). Repo-wide grep confirmed this was the only chrome.extension.* call site in the whole Chrome build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…fix (72670bf). Item 14's original audit called this "not a blocker, just deprecated" -- that was wrong, caught only by actually testing openOptionPage(). Rewrites the row to say so plainly rather than quietly fixing the status, and adds the log entry explaining what happened and why. Left as an explicit reminder alongside item 15's earlier importScripts mistake: an audit conclusion based on reasoning-by-analogy needs verifying, not trusting. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…weep. Confirms the backNavigation persistence fix works via a real force-terminate-and-restart test. Also records a comprehensive final grep across all 174 .js files in the Chrome build for every deprecated pattern found during this migration (chrome.extension.*, chrome.browserAction, chrome.tabs.executeScript, XMLHttpRequest, eval, chrome.webRequest) -- came back clean, including in popup/description scripts that were never individually audited the way the background files were. Rewrites outstanding work to reflect that the full 18-item checklist is now done and verified; what remains is breadth (the per-site test matrix, secondary features) rather than known defects. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r list. Deferred until core functionality was solid (per earlier discussion), which it now is -- the full MV2->MV3 breaking-changes checklist is done and manually verified. generate_test_matrix.py recursively walks urlSpecificAdjusterList.json (prefselector/preformatter pairs nest inconsistently across entries -- under "top" for country variants, under "path" for URL-path-dependent cases -- a shallow walk undercounts, as a first attempt at this did: 64 instead of the correct 76) to derive the canonical 76-site list, verified 1:1 against the actual files in extractors/prefselectors/. Flags the 72 sites with a matching background/preformatters/ file as also exercising the dynamic-download retry path (checklist item 18), since those are higher-value to check than plain extraction. Pre-fills the 2 sites already manually verified (github.com, a Nature article). Script is a one-time generator, not an idempotent regenerator -- its docstring warns against blindly re-running it once the table has manual test results in it, since it has no logic to merge those back in. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A stray first REPO_ROOT assignment, immediately overwritten by the correct one -- cosmetic only, confirmed the generated output is byte-identical before and after. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…gration bug found. Squashed from the mv3-migration-test-matrix branch, where this testing happened as ~20 incremental commits kept separate from this branch by request. This single commit brings the finished MIGRATION_TEST_MATRIX.md state back in; the granular history remains on that branch if the step-by-step trail is ever needed. Headline result: of everything tested, only one issue traced back to an actual MV3 migration regression. Every other finding -- stale CSS selectors (jstor, ieee, lww, annualreviews), a shared RIS-format-detection mismatch (bioone, science, taylorandfrancis), assorted missing fields (ascopubs, goodreads, genialokal, pubmed), and various site-access/domain quirks -- was individually confirmed via `git log --follow -p` to predate the migration, i.e. would affect the original MV2 build identically. The one real bug: `background_interaction.js`'s `loadAdjusterScripts()` looks up `self["BINPrefselector_" + prefselectorName]` using the raw, unsanitized name from `urlSpecificAdjusterList.json`, but `rename_adjuster_globals.py`'s `sanitize()` replaces `-` with `_` when constructing each file's actual global name during the `691b3e4`/`5d6d59b` static-preload redesign. The lookup key and the real global name never match for any hyphenated prefselector/preformatter name -- 11 prefselector names and 19 preformatter names affected (including 8 Amazon country variants). Effect ranges from silent degradation to generic-fallback-only extraction, up to an uncaught exception that hangs the popup indefinitely for four sites (springer-link-book, wiley-book, wiley-shop, worldscientific-book). Confirmed via the service worker console during a live test, not just code reading. Fixing this is the next piece of work, now to happen directly on this branch. Current numbers: 64/76 tested (43 clean pass, 16 partial pending the fix above or other follow-up, 5 fail). See MIGRATION_TEST_MATRIX.md's "Outstanding work" section for the full breakdown of untested/deferred/ untestable sites and every issue found. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The lookup key self["BINPrefselector_" + prefselectorName] used the raw name from urlSpecificAdjusterList.json, but rename_adjuster_globals.py's sanitize() replaces "-" with "_" when constructing each file's actual global name (a hyphen isn't a valid JS identifier character). The lookup key and the real global name never matched for any hyphenated prefselector/preformatter name -- confirmed via live testing (SW console showed "no preloaded prefselector script for: worldscientific-book", followed by an uncaught TypeError downstream that hung the popup indefinitely for four sites). Introduced in the 691b3e4/5d6d59b static-preload redesign; unlike everything else found during test-matrix verification, this one is a genuine migration regression, not pre-existing site-adjuster staleness. Fix: apply the identical .replace(/-/g,"_") substitution at lookup time. Verified programmatically against all 30 affected names (11 hyphenated prefselector names, 19 hyphenated preformatter names including 8 Amazon country variants) that this produces an exact match with what rename_adjuster_globals.py's sanitize() generates -- zero mismatches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…the migration log. Updates checklist item 15's row to record the bug found via the test matrix and its fix, adds a chronological log entry summarizing the headline result of the whole test-matrix effort (one real migration regression found and fixed; everything else pre-existing), and rewrites outstanding work to point at re-verifying the affected sites and finishing the remaining untested ones, per MIGRATION_TEST_MATRIX.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ttern ncbi-book and oup-journals now pass cleanly (oup-journals' dynamic citation also succeeds where it previously couldn't). oxford-archive's dynamic citation moved from silent "not available" to an honest "Failed!" now that a real attempt happens. elsevier-book and sciencedirect-book still show a journal field on @book content, joining springer-link-book/wiley-book as the same symptom across 4 independent adjusters -- recorded as a likely shared cause in background_parse_bib.js's book-type handling rather than four coincidental site bugs, not yet investigated. elsevier-book's missing authors is confirmed as a separate, genuine site-specific issue unrelated to the lookup-key fix. 64/76 sites tested: 47 pass, 16 partial/notable, 1 fail. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Said 43 pass / 5 fail; actual table (verified via grep) is 47 pass / 1 fail. Left over from an earlier edit pass before the last 5 re-tests were folded in. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Verification-only pass (no source changes) covering what hadn't been explicitly checked yet: a widened repo-wide deprecated-API grep sweep, service-worker listener-registration wake-race safety, the secondary feature code paths (redirection schemes, citation formats, bibkey formatting, PDF fallback), manifest.json diffed directly against the MV2 baseline, and CSP/inline-content edge cases. No new migration bugs found. Also updates "Outstanding work" to reflect that all 9 sites affected by the 13aeef9 fix with a real test URL are now re-verified, and flags one cosmetic finding (a debug leftover in service-worker.js) addressed next. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
console.log('Pulse heartbeat...') fired on every SW cold start since the
very first commit; had no ongoing diagnostic value.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ample of 2 Verified offline against the real manifest and all 156 source files: the name-derivation logic matches every declared global with zero mismatches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Verification-only pass covering permissions minimality, XSS surface, eval/CSP, message-passing trust boundaries, outbound network requests, chrome.storage contents, and hardcoded secrets. Overall picture is good. One finding: setDOILink() in popup_interaction.js assigns page-extracted URL data (bibFieldData[16]) straight to doiLink.href with no scheme validation, for 13 site-specific prefselectors that override the generic extractor's deliberately-empty citation_url query list. Confirmed pre-existing via git blame against the MV2 baseline, not a migration regression. Likely inert under MV3's CSP + target="_blank", but not defended by the extension's own code -- recorded as outstanding work, not fixed here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…_interaction.js
Read the actual code at every cited location before acting. 3 of 4 findings
confirmed accurate and fixed: dropped injectScript()/sendMsg() error
handlers now actually fire (checked every call site first -- all are
pre-written recovery paths that don't risk a retry loop); the two
redundant-sendResponse() cases now send exactly one response; and
validateCitationDownload()'s credentialed fetch now enforces the
same-origin-or-Springer allowlist its own pre-existing comment already
claimed, but the code never actually applied. The 4th ("High") finding's
underlying observation was real but its severity was overstated given
this extension has no broad host_permissions -- fixed anyway as
defense-in-depth. Sender authentication on handleMessage() (Medium) is
recorded as outstanding, not fixed, since there's no exploitable path
today.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Closes the setDOILink() finding from the general security audit. Scope turned out narrower than originally noted: setRedirectionLink() already validated its link, and 3 of setDOILink()'s 4 href assignments build from a fixed https:// prefix and can't become javascript: URIs regardless of input. Only the citation_url branch (raw page-extracted "url" field, no prefix) needed the guard -- added the same /^http[s]?:\/\// check already used elsewhere in this codebase for the identical purpose. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Chrome has changed extension requirements (specifically, disabling Manifest V2 extensions), which disabled this extension for all Chrome users. This PR is a complete MV2→MV3 migration to fix that — no feature changes, no behavior changes beyond what MV3 itself forces.
Changes are logged with their rationale in
MIGRATION.md, including the one real regression the migration introduced (found via testing, fixed, verified). Manually verified against 64 of 76 site adjusters (MIGRATION_TEST_MATRIX.md): 47 clean, 16 partial (pre-existing issues, not caused by this migration), 1 fail.This has been completely vibe-coded, using Claude Sonnet 5, as responsibly as possible, including thorough testing, ensuring documentation of changes and rationale, and a security audit at the end (with a duplicate audit on ChatGPT).
I hope this is seamlessly reviewable. It works fine on my machine, and I am satisfied that the changes implemented are targeted and functional. Ideally this would be done by a professional human software engineer, but as things stand, this looks like our best shot at re-enabling this critical piece of academic infrastructure for a wide population.