Skip to content

Fix bugs and harden against XSS across the extension#340

Merged
vict0rsch merged 13 commits intorefactor-es-modulesfrom
fix-security-review
Apr 13, 2026
Merged

Fix bugs and harden against XSS across the extension#340
vict0rsch merged 13 commits intorefactor-es-modulesfrom
fix-security-review

Conversation

@vict0rsch
Copy link
Copy Markdown
Owner

TL;DR

Fix a collection of bugs (scoping, logic, infinite loops) and harden the extension against XSS by escaping all user-controlled data injected into HTML via a new escapeHtml utility.

Breaking changes 🔥

None.

Changes

  • XSS hardening: Add escapeHtml() utility and apply it across all innerHTML/template-literal injection points in popup templates, options page, content script, bib matcher, and shared utils. Covers paper titles, authors, tags, notes, code links, venue names, URLs, and error messages.
  • Download sanitization: Replace ad-hoc character stripping with a proper filename sanitizer (reserved chars, control chars, path traversal, length limit) and validate pdfUrl is https?:// before downloading.
  • Sender validation: Reject chrome.runtime.onMessage messages not originating from the extension itself.
  • PAT redaction: Stop logging the GitHub Personal Access Token in plaintext; log [REDACTED] instead.
  • Code injection → func API: Replace chrome.scripting.executeScript({ code: ... }) with the safer { func, args } form (MV3-compatible).
  • Infinite loop fix: Bound the PDF title-setting loop (makeTitle) with a local iteration counter instead of the uncapped while(1).
  • Bug fixes: Fix Array.prototype.last mutating the original array via .reverse(); fix mergePapers keeping the newer addDate instead of the oldest; fix prepareOverwriteData .map() destructuring; fix implicit globals (major, minor, pwcMatch, abstractH2); fix OpenReview regex capturing only the last character; remove dead code after early return in fetchPWCData; add missing done callback default in addOrUpdatePaper; replace undefined error() calls with logError() in parsers; add null guards on OpenReview URL regex matches.
  • autoTag regex safety: Wrap user-provided regex in try/catch to prevent crashes on invalid patterns while preserving regex matching behavior.

Review hints

  • escapeHtml in src/shared/js/utils/functions.js is the central utility — verify it covers the five OWASP-recommended characters (& < > " ').
  • The cutAuthors(escapeHtml(paper.author)) call order in templates.js means entity-encoded strings are measured for truncation length — acceptable tradeoff but worth noting.
  • The autoTagPaper change in parsers.js keeps new RegExp() with a try/catch instead of switching to .includes(), preserving regex UX as documented in the options UI.
  • The sender.id check in background.js silently drops external messages — callers get undefined responses, which existing code handles.

The .last() implementation called this.reverse() which mutates the
array in place. Every call permanently reversed the array, corrupting
data when called more than once. Use index-based access instead.

Made-with: Cursor
- Fix regex capturing only last character of OpenReview IDs by moving
  the + quantifier inside the capture group.
- Remove unreachable dead code in fetchPWCData (after early return).
- Replace undefined setTitleCode/setFaviconCode with MV3-compatible
  chrome.scripting.executeScript using func + args.
- Remove unused identifier variable in pushSyncPapers.

Made-with: Cursor
- data.js: add const to major/minor in versionToSemantic (implicit globals)
- parsers.js: replace undefined error() with logError() in extractCrossrefData
- content_script.js: add const to abstractH2 in huggingfacePapers
- options.js: add i parameter to validateImportPaper, add const to pwcMatch

Made-with: Cursor
- Fix inverted addDate comparison in syncMerge: use < instead of >
  to keep the oldest add date as intended by the comment.
- Add done() to default contentScriptCallbacks to prevent TypeError
  when callers don't provide it.

Made-with: Cursor
Object.entries().map((k, v) => ...) was using the index as v instead
of the value. Fix to destructure as ([k, v]) and filter empty arrays.

Made-with: Cursor
Replace while(1) with a bounded loop (max 20 iterations) to prevent
the function from running indefinitely for the lifetime of the tab.

Made-with: Cursor
- Add escapeHtml utility to functions.js for shared use.
- Escape paper.venue and bibtex year in makePaperMemoryHTMLDiv,
  displayPaperVenue, and huggingfacePapers.
- Validate codeLink protocol and escape in displayPaperCode to
  prevent javascript: URI injection.
- Escape paper.id in updateCompleteSecretHTML content attribute.

Made-with: Cursor
- templates.js: escape paper.title, note, codeLink, author, and tag
  names before HTML insertion in getMemoryItemHTML and getPopupEditFormHTML.
- options.js: escape title/authors in getAutoTagHTML value attributes,
  paper.title in addPreprintUpdate, URLs in import feedback, update
  content values in startMatching, and error objects in sync feedback.

Made-with: Cursor
- functions.js: escape URL and title in copyHyperLinkToClipboard,
  escape error stack lines in stringifyError.
- data.js: escape validation warning keys and values in
  prepareOverwriteData.
- bibMatcher.js: escape paper titles, citation keys, venues, and
  sources in updateMatchedTitles and matchBibliography status output.

Made-with: Cursor
…nd.js

- Validate sender.id matches extension ID on all messages to reject
  messages from external extensions/web pages.
- Improve filename sanitization to strip all filesystem-unsafe chars,
  collapse consecutive dots, and limit length.
- Validate pdfUrl protocol before initiating download.

Made-with: Cursor
- Redact GitHub PAT from console log output in options.js.
- Replace regex-based autoTag matching with case-insensitive string
  includes to eliminate ReDoS risk from user-supplied patterns.

Made-with: Cursor
The WXT migration removed pre-committed bundles, so
dist/chrome-mv3 must be built before Puppeteer can load the extension.

Made-with: Cursor
@vict0rsch vict0rsch merged commit ff8fb9b into refactor-es-modules Apr 13, 2026
8 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant