fix: 2nd-pass audit — wire log.js, enable PVR, fix popup URL check, harden test execSync#135
Merged
Merged
Conversation
…arden test execSync A self-confirming "done" report from the prior session passed an adversarial second-pass and lost four concrete findings. Same model, same code, real defects. This commit fixes all four; the rest of the audit (Major/Minor) is reported separately. C-1. src/lib/log.js was dead code with false documentation. The original modernization PR (#129) shipped log.js but never added it to manifest.json `content_scripts.js`, and scripts/build-bundle.js only bundles entries from that list. So: - The content-script bundle did not include log.js - `window._skillbridgeLog` did not exist at runtime - The chrome.storage.onChanged listener added in the simplify pass (#132) was code that never executed - CONTRIBUTING.md told contributors to "use it" — anyone following that guidance would hit a ReferenceError on first call - Two PR descriptions claimed it was shipped functionality The fix wires it correctly: `src/lib/log.js` is now listed in manifest.json `content_scripts.js` immediately after `browser-polyfill.js` so subsequent scripts can call `window._skillbridgeLog.createLogger`. Bundled size: 237.1 KB → 122.7 KB (unchanged headroom). Added 13 new tests in tests/log.test.js covering: module surface, createLogger type-guard, prefix format, default threshold silencing, severity routing, multi-arg passthrough, storage.onChanged registration, level-from-storage live update, unknown-level rejection, non-extension-context graceful no-op, AND a manifest- wiring regression guard that asserts log.js stays in the content_scripts list. CONTRIBUTING.md updated to reflect the actual shape (`window._skillbridgeLog.createLogger`). C-2. SECURITY.md promised Private Vulnerability Reporting but the feature was disabled. `gh api .../private-vulnerability-reporting` returned `{"enabled": false}` while SECURITY.md confidently directed reporters to "Report a vulnerability" UI that didn't exist. A would- be reporter falling back to a public issue would defeat the privacy intent. Enabled PVR via `gh api -X PUT .../private-vulnerability- reporting`; SECURITY.md text is now true. C-3. src/popup/popup.js:8 had a CodeQL HIGH alert (js/incomplete-url-substring-sanitization, never triaged by the prior session). `tab?.url?.includes('skilljar.com')` matches `evil.skilljar.com.attacker.example/` and `prefix-skilljar.com/`. Replaced with an `isSkilljarHost` helper that parses the URL and checks `hostname === 'skilljar.com' || endsWith('.skilljar.com')`. Throws are caught (invalid URL → false). C-4. tests/{academy-courses,dict-coverage}-checker.test.js had three CodeQL MEDIUM alerts (js/shell-command-injection-from-environment) on `execSync(\`node ${SCRIPT}\`, ...)` calls. The SCRIPT path is __dirname-derived so there's no real injection vector, but the array form documents that. Switched to `spawnSync(process.execPath, [SCRIPT], ...)`; semantics preserved, all 17 self-tests still pass. Pre-flight: 411/411 jest (+13 from log.test.js), eslint clean, prettier clean, `npm run build:bundle` produces 122.7 KB content bundle that now actually includes log.js. Out of scope for this PR (reported back to the session, awaiting decision): - Major: branch protection currently has `required_pull_request_reviews: null` (0 reviews). CODEOWNERS routing is decorative without enforcement. Solo-maintainer context; toggle is intentional but worth surfacing. - Major: academy-courses-drift cron has been paused since PR #132 due to selector drift; issue #126 open since 2026-05-16. Pre-existing tech debt, not a regression introduced here. - Minor: mnao305/chrome-extension-upload v6.0.0 available (we pin v5.0.0). Dependabot PR #130 covers it.
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.
Why this PR exists
The prior session reported all modernization items "done". An adversarial second-pass — same model, same code, adversarial mindset — found four concrete defects under "previously declared done". This PR fixes them.
Critical findings — fixed in this PR
C-1.
src/lib/log.jswas 100% dead code with false documentationThe original PR (#129) shipped
src/lib/log.jsbut never added it tomanifest.jsoncontent_scripts.js.scripts/build-bundle.js:17only bundles entries from that list, so log.js was not in the runtime bundle. Yet:chrome.storage.onChangedlistener inside log.js — that listener could never fire.Fix: wired log.js into
manifest.jsoncontent_scripts.js immediately after browser-polyfill.js. Added 13 contract tests intests/log.test.jsincluding a manifest-wiring regression guard. CONTRIBUTING.md text now matches the actual API shape.C-2. SECURITY.md promised Private Vulnerability Reporting; feature was off
gh api .../private-vulnerability-reportingreturned{"enabled": false}. SECURITY.md confidently directed reporters to the "Report a vulnerability" UI button — which didn't exist. Fix: enabled PVR via API; SECURITY.md text is now true.C-3. CodeQL HIGH-severity alert never triaged —
src/popup/popup.js:8tab?.url?.includes('skilljar.com')matchesevil.skilljar.com.attacker.example/andprefix-skilljar.com/. Fix: extractedisSkilljarHost(url)that parses URL and checkshostname === 'skilljar.com' || endsWith('.skilljar.com').C-4. CodeQL MEDIUM x3 —
execSyncinterpolation in test filestests/{academy-courses,dict-coverage}-checker.test.js—js/shell-command-injection-from-environment. Fix: switched tospawnSync(process.execPath, [SCRIPT])array-args form.Pre-flight
npm test— 411/411 (was 398, +13 from newtests/log.test.js)eslint,prettier,npm run build:bundle— cleangh api .../private-vulnerability-reporting→enabled: trueMajor findings — awaiting decision (NOT in this PR)
required_pull_request_reviews: null. Solo-maintainer context, but CODEOWNERS is decorative without enforcement.Minor
mnao305/chrome-extension-uploadv5.0.0 → v6.0.0; covered by Dependabot PR chore(deps): bump the actions group across 1 directory with 7 updates #130.Out-of-scope (audited, intentional)
background.test.jsmockedfetch+toHaveBeenCalledTimes(N)— legitimate contract test of retry policy, not mock-tautology.src/bridge/puter.jsemptycatch {}— vendored 3rd-party, integrity-checked by maintenance.yml.src/lib/page-bridge.jsdoes not consume log.js — different execution world (page main world). Documented in log.js header.Test plan