From 8d67477326cfd451f70b2000235837381cc43db2 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Thu, 6 Aug 2026 16:17:15 +0200 Subject: [PATCH 1/3] List and apply a ticket's Trac attachments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pull requests are only half of "the work already on a ticket". On many tickets — good-first-bugs especially — the patch a contributor wants to try is a .diff attached to the ticket, not a PR. This adds that half, under the PR list, loaded on demand. Trac serves the attachment list only to a real browser: everything else meets the proof-of-work interstitial. So "Show Trac attachments" opens the real ticket in an embedded window where the contributor clears the challenge once, scrapes the #attachments block, and closes — the window is a means, not the UI. Applying an attachment downloads it through that same challenge-passing session and hands it to the existing preview → apply engine, so an attachment, a PR and a chosen file are one path from the preview onward. Verified end to end against the live ticket #37578: the window passes the challenge unattended, the parser reads the real markup (four attachments, with authors and absolute timestamps, the .txt correctly marked not-a-patch), and the raw-attachment download is authorised by the session cookie (a real dashboard.php diff comes back). Shape and safeguards: - The parser is a pure, dependency-free module (src/trac-attachments.cjs), regex over the #attachments HTML like core's grunt-patch-wordpress, unit tested with a fixture that matches the live markup. It never emits an off-host URL, so a poisoned ticket page cannot get an attacker link in front of the user. - The window (src/trac-view.js) is the app's first remote, untrusted content, so it is locked down: contextIsolation, no nodeIntegration, sandbox, a dedicated persist:trac partition, and NO preload — the page cannot reach the app; only the #attachments HTML crosses back, read by the main process via executeJavaScript. Navigation is pinned to core.trac.wordpress.org against both will-navigate and will-redirect. The attachment fetch re-checks the host before sending the session cookie. - Reading is on demand, not on link: opening a Trac window can surface the challenge, so it happens when the contributor asks, not for every ticket. The persistent session means the challenge is passed once, not per open. httpGet (src/github-prs.js) gains partition/useSessionCookies so the attachment fetch reuses the challenge session rather than duplicating the net helper. Refs #109, #11, part of #110. Co-Authored-By: Claude Opus 4.8 --- src/github-prs.js | 35 +++++--- src/main.js | 25 ++++++ src/preload.js | 4 + src/renderer/index.jsx | 123 ++++++++++++++++++++++++++- src/trac-attachments.cjs | 117 ++++++++++++++++++++++++++ src/trac-view.js | 149 +++++++++++++++++++++++++++++++++ test/ipc-wiring.test.cjs | 19 ++++- test/trac-attachments.test.cjs | 124 +++++++++++++++++++++++++++ 8 files changed, 578 insertions(+), 18 deletions(-) create mode 100644 src/trac-attachments.cjs create mode 100644 src/trac-view.js create mode 100644 test/trac-attachments.test.cjs diff --git a/src/github-prs.js b/src/github-prs.js index c042445..4bfdace 100644 --- a/src/github-prs.js +++ b/src/github-prs.js @@ -30,28 +30,39 @@ const REQUEST_TIMEOUT_MS = 15000; * is data the caller classifies — only on a transport failure or timeout. * Modelled on the never-reject readiness probe in main.js. * - * The `deps` seam (net client and timers) exists only so the response, - * transport-error, timeout, and settle-once paths can be exercised without the - * network or a real 15s wait; production callers pass nothing and get Electron's - * `net` and the global timers. + * `opts` carries both test doubles and request options. The doubles (net client + * and timers) exist only so the response, transport-error, timeout, and + * settle-once paths can be exercised without the network or a real 15s wait; + * production callers pass none and get Electron's `net` and the global timers. + * `partition` + `useSessionCookies` let a caller ride a specific session's + * cookies — the Trac attachment fetch reuses the session that passed the + * proof-of-work challenge, so its `_hcc` cookie authorises the download; net + * does not send session cookies unless asked, hence the explicit flag. * - * @param {string} url - * @param {Object} [headers] - * @param {Object} [deps] + * @param {string} url + * @param {Object} [headers] + * @param {Object} [opts] + * @param {string} [opts.partition] + * @param {boolean} [opts.useSessionCookies] * @return {Promise<{status: number, headers: Object, body: string}>} */ -function httpGet(url, headers = {}, deps = {}) { +function httpGet(url, headers = {}, opts = {}) { // Required lazily, not at module load: requiring `electron` outside Electron // resolves the binary and can spawn its installer on a cold checkout, and the // standalone tests inject their own client and must never reach it. - const netImpl = deps.net || require('electron').net; - const setTimeoutImpl = deps.setTimeout || setTimeout; - const clearTimeoutImpl = deps.clearTimeout || clearTimeout; + const netImpl = opts.net || require('electron').net; + const setTimeoutImpl = opts.setTimeout || setTimeout; + const clearTimeoutImpl = opts.clearTimeout || clearTimeout; return new Promise((resolve, reject) => { let settled = false; const finish = (fn, arg) => { if (!settled) { settled = true; fn(arg); } }; - const request = netImpl.request({ method: 'GET', url }); + const requestOptions = { method: 'GET', url }; + if (opts.partition) { + requestOptions.partition = opts.partition; + requestOptions.useSessionCookies = opts.useSessionCookies !== false; + } + const request = netImpl.request(requestOptions); request.setHeader('User-Agent', USER_AGENT); for (const [key, value] of Object.entries(headers)) request.setHeader(key, value); diff --git a/src/main.js b/src/main.js index 5926618..1b9bd83 100644 --- a/src/main.js +++ b/src/main.js @@ -32,6 +32,7 @@ const { ensureAutocrlf, readTrunkInfo, collectDirtyFiles, discardChanges, update const { applyPatchToDir } = require('./patch-apply'); const { parsePatchFiles, planApply } = require('./patch-plan.cjs'); const { fetchLinkedPrs, fetchPrDiff } = require('./github-prs'); +const { openAndScrape, fetchAttachment } = require('./trac-view'); const { openExternalUrl, ALLOWED_URL_SCHEMES } = require('./external-url'); const { deleteRegisteredSite } = require('./site-registry'); const { getStore } = require('./settings-store'); @@ -520,6 +521,30 @@ ipcMain.handle('git:fetch-pr-diff', async (_e, number) => { } }); +// Trac attachments (#109/#11). Read on demand: opening a real Trac window can +// show the proof-of-work challenge, so it happens when the contributor asks, +// not on every ticket. See src/trac-view.js for the window and scrape. +ipcMain.handle('trac:list-attachments', async (_e, sitePath) => { + try { + const s = await getStore(); + const ticketId = ((s.get('siteMeta') || {})[sitePath] || {}).tracTicket; + if (!ticketId) return { ok: true, status: 'no-ticket', items: [] }; + const result = await openAndScrape(ticketId); + return { ok: true, ...result }; + } catch (e) { + logError('trac:list-attachments', String(e && e.stack ? e.stack : e)); + return { ok: false, status: 'error', items: [], error: String(e) }; + } +}); + +ipcMain.handle('trac:fetch-attachment', async (_e, url) => { + try { + return await fetchAttachment(url); + } catch (e) { + return { ok: false, error: String(e) }; + } +}); + // --- Applying someone else's patch (#11) --- the diff mechanics live in // src/patch-apply.js; these handlers add IPC plumbing and electron-store writes. diff --git a/src/preload.js b/src/preload.js index fb64d6e..669e783 100644 --- a/src/preload.js +++ b/src/preload.js @@ -85,6 +85,10 @@ contextBridge.exposeInMainWorld('api', { listTicketPatches: (sitePath) => ipcRenderer.invoke('git:list-ticket-patches', sitePath) , fetchPrDiff: (number) => ipcRenderer.invoke('git:fetch-pr-diff', number) +, + listTracAttachments: (sitePath) => ipcRenderer.invoke('trac:list-attachments', sitePath) +, + fetchTracAttachment: (url) => ipcRenderer.invoke('trac:fetch-attachment', url) , applyPatch: async (sitePath, options, onLog, onDone) => { const { applyId } = await ipcRenderer.invoke('git:apply-patch', sitePath, options); diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index 21963a1..bb53e3d 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -818,6 +818,11 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit const [ticketPatches, setTicketPatches] = useState(null); const [ticketPatchesLoading, setTicketPatchesLoading] = useState(false); const [fetchingPr, setFetchingPr] = useState(null); + // Trac attachments (#11): loaded on demand, since opening a real Trac window + // can surface the proof-of-work challenge. null until the user asks. + const [tracAttachments, setTracAttachments] = useState(null); + const [tracAttachmentsLoading, setTracAttachmentsLoading] = useState(false); + const [fetchingAttachment, setFetchingAttachment] = useState(null); // Trunk update path (#94) const [trunkDate, setTrunkDate] = useState(null); const [updateIncomplete, setUpdateIncomplete] = useState(false); @@ -1654,13 +1659,18 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit useEffect(() => { if (!tracTicket) { setTicketPatches(null); + // Attachments are per-ticket and loaded on demand; a stale list from the + // previous ticket must not linger. + setTracAttachments(null); loadedTicketRef.current = null; return; } if (!isActive || loadedTicketRef.current === tracTicket) return; - // Marked loaded before the fetch resolves, on purpose: a failed initial - // fetch is not retried on every re-activation (which could keep spending a - // rate-limited quota) — the Refresh button is the retry. + // A new ticket on the active site: drop any attachments the previous one + // loaded, then fetch its PRs. Marked loaded before the fetch resolves, on + // purpose: a failed initial fetch is not retried on every re-activation + // (which could keep spending a rate-limited quota) — Refresh is the retry. + setTracAttachments(null); loadedTicketRef.current = tracTicket; loadTicketPatches(); }, [tracTicket, isActive, loadTicketPatches]); @@ -1691,6 +1701,45 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit } }; + // Opens the real Trac ticket (the user clears the challenge once if shown), + // scrapes its attachment list, and shows it in-app. On demand, not on link. + const loadTracAttachments = async () => { + setApplyError(''); + setTracAttachmentsLoading(true); + try { + const res = await window.api.listTracAttachments(sitePath); + setTracAttachments(res && res.ok ? res : { status: 'error', items: [] }); + } catch { + setTracAttachments({ status: 'error', items: [] }); + } finally { + setTracAttachmentsLoading(false); + } + }; + + // Downloads an attachment through the challenge-passing session and hands it + // to the same preview the PR and file paths use. + const previewAttachment = async (att) => { + setApplyError(''); + setFetchingAttachment(att.url); + try { + const res = await window.api.fetchTracAttachment(att.url); + if (!res || !res.ok) { + setApplyError(res?.error || `Could not download ${att.filename}.`); + return; + } + const preview = await window.api.previewPatch(sitePath, res.text); + if (!preview || !preview.ok) { + setApplyError(preview?.error || 'Could not read that patch.'); + return; + } + setApplyPreview({ ...preview, label: att.filename, text: res.text }); + } catch (e) { + setApplyError(String(e)); + } finally { + setFetchingAttachment(null); + } + }; + const runApply = ({ reverse = false } = {}) => { const state = terminalStateRef.current; if (state.running) { @@ -2286,7 +2335,7 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit
- See the work that already exists on this ticket before adding your own. Trac attachments are not listed yet — open the ticket for those. + See the work that already exists on this ticket before adding your own.
{ticketPatchesLoading && !ticketPatches ? ( @@ -2331,6 +2380,72 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit ) : null} + +
+
+
Trac attachments
+ {tracAttachments ? ( + + ) : null} +
+
+ Patch files are sometimes attached on Trac instead of a PR. Reading them opens the ticket so you can pass its human-check once. +
+ + {!tracAttachments && !tracAttachmentsLoading ? ( +
+ +
+ ) : null} + + {tracAttachmentsLoading ? ( +
Opening the ticket on Trac…
+ ) : null} + + {tracAttachments && tracAttachments.status === 'no-attachments' ? ( +
This ticket has no attachments.
+ ) : null} + + {tracAttachments && (tracAttachments.status === 'challenge-timeout' || tracAttachments.status === 'error') ? ( +
+ {tracAttachments.status === 'challenge-timeout' + ? 'Trac’s human-check did not complete in time. Try again, and click “I am human” if it appears.' + : 'Could not read the attachments from Trac.'} +
+ ) : null} + + {tracAttachments && tracAttachments.items && tracAttachments.items.length ? ( +
+ {tracAttachments.items.map((att) => ( +
+
+
+ +
+
+ {[att.author && `by ${att.author}`, att.dateText, att.sizeText].filter(Boolean).join(' · ')} +
+
+ {att.applyable ? ( + + ) : ( + not a patch + )} +
+ ))} +
+ ) : null} +
) : ( <> diff --git a/src/trac-attachments.cjs b/src/trac-attachments.cjs new file mode 100644 index 0000000..06ca233 --- /dev/null +++ b/src/trac-attachments.cjs @@ -0,0 +1,117 @@ +'use strict'; + +/** + * Reading the attachment list off a Trac ticket page (issue #109 / #11). + * + * On many tickets — good-first-bugs especially — the work a contributor wants + * to try is a `.diff` attached to the ticket, not a pull request. Trac serves + * that list only inside a real browser (everything else meets the proof-of-work + * interstitial), so the embedded view scrapes the `#attachments` block's HTML + * and hands it here to turn into rows. + * + * Parsing is done by regex over the HTML string, not with DOM selectors, + * precisely so it can be unit tested under `node --test` without a browser — + * the same approach core's own `grunt-patch-wordpress` takes. The Trac + * attachment markup has been stable for years, which is what makes this + * tractable; the fragile part is contained here and covered by fixtures. + * + * Each attachment appears as a `
…
` inside the block, carrying a link to + * `/attachment/ticket//` (and usually a raw link too), the author, + * a date, and a size. Missing pieces degrade to empty strings rather than + * dropping the row — a filename with a working download link is useful even + * without its metadata. + */ + +/** + * The canonical raw download URL for an attachment path. Trac serves the file + * itself under `raw-attachment`; the `attachment` path is the HTML view. Same + * transform core's grunt-patch-wordpress uses. + * + * @param {string} pathOrUrl + * @return {string} + */ +const TRAC_HOST = 'core.trac.wordpress.org'; + +function toRawUrl(pathOrUrl) { + const abs = pathOrUrl.startsWith('http') ? pathOrUrl : `https://${TRAC_HOST}${pathOrUrl}`; + return abs.replace('/attachment/ticket/', '/raw-attachment/ticket/'); +} + +/** + * @param {string} chunk HTML of one `
…
` (plus its `
` if present). + * @param {string} id + * @return {{filename: string, url: string, author: string, dateText: string, sizeText: string, applyable: boolean}|null} + */ +function parseOne(chunk, id) { + // The attachment link names the file. Accept both the view and raw forms; + // the id guard keeps stray links (e.g. to other tickets) out. + const link = new RegExp(`href="((?:https?://[^"]+)?/(?:raw-)?attachment/ticket/${id}/([^"?]+))"`).exec(chunk); + if (!link) return null; + const url = toRawUrl(link[1]); + // The parser must never emit an off-host URL: an absolute href on another + // host would pass the id-shaped path check, and the filename is rendered as + // an openExternal link. Rejecting the row here means a poisoned ticket page + // cannot get an attacker URL in front of the user. + try { + if (new URL(url).hostname !== TRAC_HOST) return null; + } catch { + return null; + } + const filename = decodeURIComponent(link[2]); + + // Author: the trac-author anchor, or its text. Falls back to empty. + const authorMatch = /class="trac-author[^"]*"[^>]*>([^<]+)]*>([^<]+) so each attachment's metadata stays with its link. The + // leading segment before the first
(heading) yields no link and drops. + const chunks = html.split(/]/i); + const seen = new Set(); + const rows = []; + for (const chunk of chunks) { + const row = parseOne(chunk, id); + if (!row || seen.has(row.filename)) continue; + seen.add(row.filename); + rows.push(row); + } + return rows; +} + +module.exports = { toRawUrl, parseAttachments }; diff --git a/src/trac-view.js b/src/trac-view.js new file mode 100644 index 0000000..6091604 --- /dev/null +++ b/src/trac-view.js @@ -0,0 +1,149 @@ +'use strict'; + +/** + * The embedded Trac ticket view (issue #109 / #11). + * + * Trac answers non-browser clients with a proof-of-work interstitial, so the + * only way to read a ticket's attachment list is a real Chromium window where + * the user clears the challenge once. This opens such a window, waits for the + * real ticket page, scrapes the `#attachments` block, and closes — the window + * is a means, not the UI. The scraped list is parsed by the pure + * trac-attachments.cjs module and shown natively in the app. + * + * Security: the window renders remote, untrusted content, so it gets no preload + * (the page cannot reach the app), runs sandboxed with context isolation, and + * is pinned to core.trac.wordpress.org. The only thing that crosses back is the + * `#attachments` HTML, read by the main process via executeJavaScript. A + * downloaded attachment is likewise untrusted and flows through the same apply + * engine (#11), which defends against path traversal. + */ + +const { BrowserWindow, session } = require('electron'); +const { parseAttachments } = require('./trac-attachments.cjs'); +const { httpGet } = require('./github-prs'); + +const TRAC_HOST = 'core.trac.wordpress.org'; +const TRAC_PARTITION = 'persist:trac'; +const USER_AGENT = 'WordPress-Contributor-Toolkit (+https://github.com/WordPress/experimental-wp-dev-env)'; +// How long to wait for the ticket page to appear. The hashcash runs +// automatically in a few seconds; the extra headroom covers the escalated +// "I am human" checkbox, which needs a human click. +const READY_TIMEOUT_MS = 90000; +const POLL_MS = 800; + +function ticketUrl(id) { + return `https://${TRAC_HOST}/ticket/${id}`; +} + +/** + * Locks a window's webContents to the Trac host: no popups, no navigating away. + * + * @param {import('electron').WebContents} wc + */ +function pinToTrac(wc) { + wc.setWindowOpenHandler(() => ({ action: 'deny' })); + const stayOnTrac = (event, url) => { + try { + if (new URL(url).hostname !== TRAC_HOST) event.preventDefault(); + } catch { + event.preventDefault(); + } + }; + // will-navigate covers link clicks and script navigation; will-redirect + // covers HTTP 3xx and , which do not fire will-navigate and + // would otherwise move this pinned window off the Trac origin. + wc.on('will-navigate', stayOnTrac); + wc.on('will-redirect', stayOnTrac); +} + +/** + * Opens the ticket, waits for the real page (showing the window only if the + * challenge needs the user), scrapes the attachment list, and closes. + * + * @param {number|string} ticketId + * @return {Promise<{status: string, items: Array, error?: string}>} + */ +async function openAndScrape(ticketId) { + const id = String(ticketId).replace(/[^0-9]/g, ''); + if (!id) return { status: 'error', items: [], error: 'No ticket number' }; + + const tracSession = session.fromPartition(TRAC_PARTITION); + const win = new BrowserWindow({ + width: 1000, + height: 800, + show: false, + title: `Trac #${id}`, + webPreferences: { + contextIsolation: true, + nodeIntegration: false, + sandbox: true, + partition: TRAC_PARTITION + } + }); + pinToTrac(win.webContents); + // A persistent User-Agent that identifies the app, on this session only. + tracSession.setUserAgent(USER_AGENT); + + let shown = false; + const showOnce = () => { + // Once the challenge needs interaction, the window has to be visible. + if (!shown && !win.isDestroyed()) { shown = true; win.show(); } + }; + + try { + await win.loadURL(ticketUrl(id)); + + // Poll until the real ticket page is present. The challenge page has no + // #ticket; when the hashcash (or the user) clears it, Trac reloads to + // the real page and #ticket appears. + const deadline = Date.now() + READY_TIMEOUT_MS; + let ready = false; + while (Date.now() < deadline) { + if (win.isDestroyed()) return { status: 'closed', items: [] }; + const hasTicket = await win.webContents.executeJavaScript('!!document.querySelector("#ticket")').catch(() => false); + if (hasTicket) { ready = true; break; } + showOnce(); + await new Promise((r) => setTimeout(r, POLL_MS)); + } + + if (!ready) { + return { status: 'challenge-timeout', items: [] }; + } + + const html = await win.webContents + .executeJavaScript('(document.querySelector("#attachments") || {}).outerHTML || ""') + .catch(() => ''); + const items = parseAttachments(html, id); + return { status: items.length ? 'ok' : 'no-attachments', items }; + } catch (e) { + return { status: 'error', items: [], error: String(e && e.message ? e.message : e) }; + } finally { + if (!win.isDestroyed()) win.destroy(); + } +} + +/** + * Downloads one attachment through the challenge-passing session, so its cookie + * authorises the request. + * + * @param {string} url A raw-attachment URL on the Trac host. + * @return {Promise<{ok: true, text: string}|{ok: false, error: string}>} + */ +async function fetchAttachment(url) { + let parsed; + try { parsed = new URL(url); } catch { return { ok: false, error: 'Invalid attachment URL' }; } + if (parsed.hostname !== TRAC_HOST) return { ok: false, error: 'Only core.trac.wordpress.org attachments are allowed' }; + + let res; + try { + res = await httpGet(url, { Accept: 'text/plain' }, { partition: TRAC_PARTITION, useSessionCookies: true }); + } catch (e) { + return { ok: false, error: String(e && e.message ? e.message : e) }; + } + if (res.status !== 200) { + return { ok: false, error: `Trac returned ${res.status} — try opening the ticket again to pass the check.` }; + } + return { ok: true, text: res.body }; +} + +module.exports = { openAndScrape, fetchAttachment, ticketUrl }; diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index b896e10..68f2620 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -1157,6 +1157,19 @@ test('git:list-ticket-patches returns no-ticket without calling github-prs when assert.deepEqual(fetchLinkedPrs.calls, []); }); +// --- Trac attachments (#109 / #11) --------------------------------------- + +test('trac:fetch-attachment goes through trac-view', async () => { + const fetchAttachment = spy(async () => ({ ok: true, text: 'DIFF' })); + const main = loadMain({ stubs: { ...silentLogging(), './trac-view': { fetchAttachment, openAndScrape: async () => ({}) } } }); + const url = 'https://core.trac.wordpress.org/raw-attachment/ticket/1/a.diff'; + + const result = await main.invoke('trac:fetch-attachment', url); + + assert.deepEqual(fetchAttachment.calls, [[url]]); + assert.deepEqual(result, { ok: true, text: 'DIFF' }); +}); + // --- the harness's own guard --------------------------------------------- // Requiring the real `electron` package is not a harmless fallback: its @@ -1209,7 +1222,8 @@ const WIRED = new Set([ 'git:preview-patch', 'git:apply-patch', 'git:fetch-pr-diff', - 'git:list-ticket-patches' + 'git:list-ticket-patches', + 'trac:fetch-attachment' ]); // Channels with no module to reach: they read or write electron-store, drive a @@ -1247,7 +1261,8 @@ const NO_DELEGATION = new Map([ // Channels that do delegate, but whose call sits behind something this harness // cannot stand in for yet. Each one is a known hole, not an oversight. const NOT_REACHABLE = new Map([ - ['wordpress:setup', 'calls ensureAutocrlf and readTrunkInfo only after cloning wordpress-develop over the network'] + ['wordpress:setup', 'calls ensureAutocrlf and readTrunkInfo only after cloning wordpress-develop over the network'], + ['trac:list-attachments', 'reads electron-store for the ticket before it can open the Trac window'] ]); const CLASSIFIED = [...WIRED, ...NO_DELEGATION.keys(), ...NOT_REACHABLE.keys()]; diff --git a/test/trac-attachments.test.cjs b/test/trac-attachments.test.cjs new file mode 100644 index 0000000..b409721 --- /dev/null +++ b/test/trac-attachments.test.cjs @@ -0,0 +1,124 @@ +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert'); +const { toRawUrl, parseAttachments } = require('../src/trac-attachments.cjs'); + +// A representative #attachments block, modelled on WordPress Trac's markup and +// the real attachments on ticket #37578 (three .diff files plus a .txt). Built +// from the documented structure; hardened against live markup during the manual +// pass. Kept verbatim so the test breaks loudly if the real markup drifts. +const BLOCK = `
+

Attachments (4)

+
+
+
+ + 37578-01.diff + (1.7 KB) - added by Jonnyauk 10 years ago. +
+
New filters for Dashboard Recent Activity widget.
+
+ + good first bug 1.txt + (606 bytes) - added by vedantsonone1234 16 months ago. +
+
notes
+
+ + 37578.diff + (1.7 KB) - added by pmbaldha 15 months ago. +
+
Added a patch using the existing filter.
+
+ + 37578-1.diff + (1.8 KB) - added by pmbaldha 15 months ago. +
+
PHPDoc Comment update.
+
+
+
`; + +test('toRawUrl: the view path becomes the raw download path (issue #11)', () => { + assert.strictEqual( + toRawUrl('/attachment/ticket/37578/37578.diff'), + 'https://core.trac.wordpress.org/raw-attachment/ticket/37578/37578.diff' + ); + // An already-raw or absolute URL is left as the raw form. + assert.strictEqual( + toRawUrl('https://core.trac.wordpress.org/raw-attachment/ticket/37578/37578.diff'), + 'https://core.trac.wordpress.org/raw-attachment/ticket/37578/37578.diff' + ); +}); + +test('parseAttachments: every attachment is found once, with a raw download URL (issue #11)', () => { + const rows = parseAttachments(BLOCK, 37578); + assert.deepStrictEqual(rows.map((r) => r.filename), [ + '37578-01.diff', 'good first bug 1.txt', '37578.diff', '37578-1.diff' + ]); + // Deduped despite each attachment carrying both a raw and a view link. + assert.strictEqual(rows.length, 4); + assert.strictEqual(rows[0].url, 'https://core.trac.wordpress.org/raw-attachment/ticket/37578/37578-01.diff'); +}); + +test('parseAttachments: only .diff/.patch are applyable (issue #11)', () => { + const rows = parseAttachments(BLOCK, 37578); + const byName = Object.fromEntries(rows.map((r) => [r.filename, r.applyable])); + assert.strictEqual(byName['37578.diff'], true); + assert.strictEqual(byName['37578-1.diff'], true); + assert.strictEqual(byName['good first bug 1.txt'], false); +}); + +test('parseAttachments: author, size and an absolute date are extracted (issue #11)', () => { + const rows = parseAttachments(BLOCK, 37578); + const diff = rows.find((r) => r.filename === '37578.diff'); + assert.strictEqual(diff.author, 'pmbaldha'); + assert.strictEqual(diff.sizeText, '1.7 KB'); + // Absolute timestamp from the title, not the "15 months ago" relative text. + assert.strictEqual(diff.dateText, '05/15/2025 09:30:00 AM'); +}); + +test('parseAttachments: an encoded filename is decoded (issue #11)', () => { + const rows = parseAttachments(BLOCK, 37578); + assert.ok(rows.some((r) => r.filename === 'good first bug 1.txt'), 'the %20 spaces decode'); +}); + +// A row missing its metadata should still be usable — the filename and a working +// download link are the load-bearing parts. +test('parseAttachments: a bare attachment link still yields a row (issue #11)', () => { + const rows = parseAttachments( + '', + 62281 + ); + assert.strictEqual(rows.length, 1); + assert.strictEqual(rows[0].filename, '62281.diff'); + assert.strictEqual(rows[0].url, 'https://core.trac.wordpress.org/raw-attachment/ticket/62281/62281.diff'); + assert.strictEqual(rows[0].author, ''); + assert.strictEqual(rows[0].applyable, true); +}); + +// A poisoned ticket page could carry an absolute href on another host that +// still matches the id-shaped path. It must not become a row — the filename is +// rendered as an openExternal link. +test('parseAttachments: an off-host absolute attachment href is rejected (issue #11)', () => { + const rows = parseAttachments( + '
x.diff
', + 37578 + ); + assert.deepStrictEqual(rows, []); +}); + +test('parseAttachments: links to other tickets are ignored (issue #11)', () => { + const rows = parseAttachments( + '', + 37578 + ); + assert.deepStrictEqual(rows, []); +}); + +test('parseAttachments: an empty or attachment-less block yields nothing, not a throw (issue #11)', () => { + assert.deepStrictEqual(parseAttachments('', 37578), []); + assert.deepStrictEqual(parseAttachments(null, 37578), []); + assert.deepStrictEqual(parseAttachments('

No attachments.

', 37578), []); +}); From f0f13d3b941d9bf6ec3c266e94b77ce99181bb6c Mon Sep 17 00:00:00 2001 From: JuanMa Date: Fri, 7 Aug 2026 11:25:34 +0200 Subject: [PATCH 2/3] Pin Trac URLs to the exact HTTPS origin, not just the host (Copilot #139 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The embedded Trac view carries a session cookie earned by clearing the proof-of-work, so a same-host http downgrade could leak it on an untrusted network. A new pure secureTracUrl(url) helper accepts only https://core.trac.wordpress.org/… and returns the normalized href: - parseOne drops any attachment link that is off-host OR plaintext (#2). - pinToTrac blocks navigation/redirects that are not the secure origin (#1). - fetchAttachment validates and sends the normalized href — the address that passed the check is the one fetched with the cookie (#3). The helper is unit-tested, which also covers the security core of the window-glue testing gap (#5); the rest of that glue is a follow-up. Co-Authored-By: Claude Opus 4.8 --- src/trac-attachments.cjs | 36 +++++++++++++++++++++++----------- src/trac-view.js | 21 ++++++++++---------- test/trac-attachments.test.cjs | 23 +++++++++++++++++++++- 3 files changed, 58 insertions(+), 22 deletions(-) diff --git a/src/trac-attachments.cjs b/src/trac-attachments.cjs index 06ca233..a459080 100644 --- a/src/trac-attachments.cjs +++ b/src/trac-attachments.cjs @@ -37,6 +37,23 @@ function toRawUrl(pathOrUrl) { return abs.replace('/attachment/ticket/', '/raw-attachment/ticket/'); } +/** + * The normalized href iff the URL is exactly the secure Trac origin. Host alone + * is not enough: the embedded view carries a session cookie earned by clearing + * Trac's proof-of-work, and an http downgrade on the same host would leak it on + * an untrusted network. Returns null for anything that is not + * `https://core.trac.wordpress.org/…`, so callers fail closed. + * + * @param {string} url + * @return {string|null} + */ +function secureTracUrl(url) { + let parsed; + try { parsed = new URL(url); } catch { return null; } + if (parsed.protocol !== 'https:' || parsed.hostname !== TRAC_HOST) return null; + return parsed.href; +} + /** * @param {string} chunk HTML of one `
…
` (plus its `
` if present). * @param {string} id @@ -47,16 +64,13 @@ function parseOne(chunk, id) { // the id guard keeps stray links (e.g. to other tickets) out. const link = new RegExp(`href="((?:https?://[^"]+)?/(?:raw-)?attachment/ticket/${id}/([^"?]+))"`).exec(chunk); if (!link) return null; - const url = toRawUrl(link[1]); - // The parser must never emit an off-host URL: an absolute href on another - // host would pass the id-shaped path check, and the filename is rendered as - // an openExternal link. Rejecting the row here means a poisoned ticket page - // cannot get an attacker URL in front of the user. - try { - if (new URL(url).hostname !== TRAC_HOST) return null; - } catch { - return null; - } + // The parser must never emit an off-host or plaintext URL: an absolute href + // on another host — or an http downgrade of this one — would pass the + // id-shaped path check, and the filename is rendered as an openExternal link + // (and later fetched with the session cookie). Rejecting the row here means a + // poisoned ticket page cannot get such a URL in front of the user. + const url = secureTracUrl(toRawUrl(link[1])); + if (!url) return null; const filename = decodeURIComponent(link[2]); // Author: the trac-author anchor, or its text. Falls back to empty. @@ -114,4 +128,4 @@ function parseAttachments(attachmentsHtml, ticketId) { return rows; } -module.exports = { toRawUrl, parseAttachments }; +module.exports = { toRawUrl, parseAttachments, secureTracUrl }; diff --git a/src/trac-view.js b/src/trac-view.js index 6091604..55b49b2 100644 --- a/src/trac-view.js +++ b/src/trac-view.js @@ -19,7 +19,7 @@ */ const { BrowserWindow, session } = require('electron'); -const { parseAttachments } = require('./trac-attachments.cjs'); +const { parseAttachments, secureTracUrl } = require('./trac-attachments.cjs'); const { httpGet } = require('./github-prs'); const TRAC_HOST = 'core.trac.wordpress.org'; @@ -43,11 +43,10 @@ function ticketUrl(id) { function pinToTrac(wc) { wc.setWindowOpenHandler(() => ({ action: 'deny' })); const stayOnTrac = (event, url) => { - try { - if (new URL(url).hostname !== TRAC_HOST) event.preventDefault(); - } catch { - event.preventDefault(); - } + // Pinned to the exact https Trac origin, not just the host: a redirect or + // to http://core.trac.wordpress.org would otherwise keep + // this window — and its session cookie — on a plaintext origin. + if (!secureTracUrl(url)) event.preventDefault(); }; // will-navigate covers link clicks and script navigation; will-redirect // covers HTTP 3xx and , which do not fire will-navigate and @@ -130,13 +129,15 @@ async function openAndScrape(ticketId) { * @return {Promise<{ok: true, text: string}|{ok: false, error: string}>} */ async function fetchAttachment(url) { - let parsed; - try { parsed = new URL(url); } catch { return { ok: false, error: 'Invalid attachment URL' }; } - if (parsed.hostname !== TRAC_HOST) return { ok: false, error: 'Only core.trac.wordpress.org attachments are allowed' }; + // Validate to the exact https Trac origin and send the normalized address, + // not the caller's string: the request rides the session cookie, so the URL + // fetched has to be the one that passed the check. + const safe = secureTracUrl(url); + if (!safe) return { ok: false, error: 'Only https core.trac.wordpress.org attachments are allowed' }; let res; try { - res = await httpGet(url, { Accept: 'text/plain' }, { partition: TRAC_PARTITION, useSessionCookies: true }); + res = await httpGet(safe, { Accept: 'text/plain' }, { partition: TRAC_PARTITION, useSessionCookies: true }); } catch (e) { return { ok: false, error: String(e && e.message ? e.message : e) }; } diff --git a/test/trac-attachments.test.cjs b/test/trac-attachments.test.cjs index b409721..6132023 100644 --- a/test/trac-attachments.test.cjs +++ b/test/trac-attachments.test.cjs @@ -2,7 +2,7 @@ const test = require('node:test'); const assert = require('node:assert'); -const { toRawUrl, parseAttachments } = require('../src/trac-attachments.cjs'); +const { toRawUrl, parseAttachments, secureTracUrl } = require('../src/trac-attachments.cjs'); // A representative #attachments block, modelled on WordPress Trac's markup and // the real attachments on ticket #37578 (three .diff files plus a .txt). Built @@ -109,6 +109,27 @@ test('parseAttachments: an off-host absolute attachment href is rejected (issue assert.deepStrictEqual(rows, []); }); +// Same host, but http:// — a downgrade of the origin that carries the session +// cookie. It must not become a row. +test('parseAttachments: a same-host http (non-https) attachment href is rejected (issue #11)', () => { + const rows = parseAttachments( + '
x.diff
', + 37578 + ); + assert.deepStrictEqual(rows, []); +}); + +test('secureTracUrl: accepts only the exact https Trac origin (issue #11)', () => { + assert.strictEqual( + secureTracUrl('https://core.trac.wordpress.org/raw-attachment/ticket/1/a.diff'), + 'https://core.trac.wordpress.org/raw-attachment/ticket/1/a.diff' + ); + assert.strictEqual(secureTracUrl('http://core.trac.wordpress.org/raw-attachment/ticket/1/a.diff'), null); + assert.strictEqual(secureTracUrl('https://evil.example.com/raw-attachment/ticket/1/a.diff'), null); + assert.strictEqual(secureTracUrl('https://core.trac.wordpress.org.evil.com/a.diff'), null); + assert.strictEqual(secureTracUrl('not a url'), null); +}); + test('parseAttachments: links to other tickets are ignored (issue #11)', () => { const rows = parseAttachments( '', From 9388f5c10eb3da63bc119c710e0015af5dd7be4d Mon Sep 17 00:00:00 2001 From: JuanMa Date: Fri, 7 Aug 2026 11:27:21 +0200 Subject: [PATCH 3/3] Drop stale Trac scrapes and show the closed state (Copilot #139 review) - A scrape can run up to 90s; a generation bumped on every ticket change lets loadTracAttachments ignore a result (and its loading-flag cleanup) that resolves after the ticket moved on, so the old ticket's attachments can no longer appear under a new one (#4). - The attachments panel now renders the 'closed' outcome (the user shut the Trac window) instead of a blank panel, and surfaces the error detail when present (suppressed finding). Co-Authored-By: Claude Opus 4.8 --- src/renderer/index.jsx | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index bb53e3d..294b6de 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -1660,21 +1660,32 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit if (!tracTicket) { setTicketPatches(null); // Attachments are per-ticket and loaded on demand; a stale list from the - // previous ticket must not linger. + // previous ticket must not linger, and a scrape dropped by the generation + // bump below must not leave a stuck spinner. setTracAttachments(null); + setTracAttachmentsLoading(false); loadedTicketRef.current = null; return; } if (!isActive || loadedTicketRef.current === tracTicket) return; // A new ticket on the active site: drop any attachments the previous one - // loaded, then fetch its PRs. Marked loaded before the fetch resolves, on - // purpose: a failed initial fetch is not retried on every re-activation - // (which could keep spending a rate-limited quota) — Refresh is the retry. + // loaded (and clear its loading flag, so a scrape dropped by the generation + // bump cannot leave a stuck spinner with no button to recover), then fetch + // its PRs. Marked loaded before the fetch resolves, on purpose: a failed + // initial fetch is not retried on every re-activation (which could keep + // spending a rate-limited quota) — Refresh is the retry. setTracAttachments(null); + setTracAttachmentsLoading(false); loadedTicketRef.current = tracTicket; loadTicketPatches(); }, [tracTicket, isActive, loadTicketPatches]); + // A Trac scrape can run up to 90s. Bump a generation on every ticket change so + // a scrape that resolves after the ticket has moved on is dropped, rather than + // shown under the wrong ticket or clearing a newer request's loading flag. + const scrapeGenRef = useRef(0); + useEffect(() => { scrapeGenRef.current += 1; }, [tracTicket]); + // Fetches a PR's diff and drops into the same preview the file picker uses, // so applying a PR and applying a downloaded patch are one path from here on. const previewPr = async (pr) => { @@ -1704,15 +1715,18 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit // Opens the real Trac ticket (the user clears the challenge once if shown), // scrapes its attachment list, and shows it in-app. On demand, not on link. const loadTracAttachments = async () => { + const gen = scrapeGenRef.current; setApplyError(''); setTracAttachmentsLoading(true); try { const res = await window.api.listTracAttachments(sitePath); + if (gen !== scrapeGenRef.current) return; // ticket changed mid-scrape; drop the stale result setTracAttachments(res && res.ok ? res : { status: 'error', items: [] }); } catch { + if (gen !== scrapeGenRef.current) return; setTracAttachments({ status: 'error', items: [] }); } finally { - setTracAttachmentsLoading(false); + if (gen === scrapeGenRef.current) setTracAttachmentsLoading(false); } }; @@ -2410,11 +2424,13 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit
This ticket has no attachments.
) : null} - {tracAttachments && (tracAttachments.status === 'challenge-timeout' || tracAttachments.status === 'error') ? ( + {tracAttachments && (tracAttachments.status === 'challenge-timeout' || tracAttachments.status === 'error' || tracAttachments.status === 'closed') ? (
- {tracAttachments.status === 'challenge-timeout' - ? 'Trac’s human-check did not complete in time. Try again, and click “I am human” if it appears.' - : 'Could not read the attachments from Trac.'} + {(() => { + if (tracAttachments.status === 'challenge-timeout') return 'Trac’s human-check did not complete in time. Try again, and click “I am human” if it appears.'; + if (tracAttachments.status === 'closed') return 'The Trac window was closed before the attachments finished loading. Click “Show Trac attachments” to try again.'; + return `Could not read the attachments from Trac.${tracAttachments.error ? ` (${tracAttachments.error})` : ''}`; + })()}
) : null}