diff --git a/.gitignore b/.gitignore index c3e324b..9fc0ba5 100644 --- a/.gitignore +++ b/.gitignore @@ -49,9 +49,26 @@ pnpm-debug.log* Thumbs.db desktop.ini -# Testing +# Testing — kept local, never published (CLAUDE.md rule 1.12). +# Tests still run on the dev machine via `npm test`; they just don't +# ship in the repo, which holds only what builds and runs the product. coverage/ .nyc_output/ +tests/ +__tests__/ +__mocks__/ +__fixtures__/ +*.test.ts +*.test.tsx +*.spec.ts +*.spec.tsx +vitest.config.ts +vitest.workspace.ts +# Scratch / debug helpers +debug-*.mjs +debug-*.ts +scratch-*.mjs +scratch-*.ts # Cache .cache/ diff --git a/apps/desktop/src/main/credentials.ts b/apps/desktop/src/main/credentials.ts index 281b30b..9965a7b 100644 --- a/apps/desktop/src/main/credentials.ts +++ b/apps/desktop/src/main/credentials.ts @@ -22,7 +22,7 @@ */ import { app, safeStorage } from 'electron'; import { join } from 'node:path'; -import { readFileSync, writeFileSync, existsSync } from 'node:fs'; +import { readFileSync, writeFileSync, existsSync, chmodSync } from 'node:fs'; import { INTEGRATIONS, getIntegration, @@ -67,12 +67,35 @@ function load(): CredentialFile { function persist(): void { try { - writeFileSync(filePath(), JSON.stringify(cache ?? {}, null, 2), 'utf8'); + const p = filePath(); + // 0600: on Linux with no OS keyring the values fall back to base64 + // (`pt:` prefix), so the file must not be world-readable. Mirrors the + // MCP bridge discovery-file pattern (mcp-bridge.ts). + writeFileSync(p, JSON.stringify(cache ?? {}, null, 2), { encoding: 'utf8', mode: 0o600 }); + if (process.platform !== 'win32') { + try { + chmodSync(p, 0o600); + } catch { + /* best-effort — a pre-existing file keeps its mode otherwise */ + } + } } catch (err) { logger.log('error', 'main', `credentials store write failed: ${(err as Error).message}`); } } +/** Encrypt an arbitrary string for at-rest storage outside this module + * (e.g. crawl-config secrets in preferences.json / project meta). Reuses + * the same safeStorage-with-base64-fallback scheme as credential values. */ +export function encryptString(value: string): string { + return encrypt(value); +} + +/** Reverse of {@link encryptString}. Returns '' on any failure. */ +export function decryptString(stored: string): string { + return decrypt(stored); +} + /** Encrypt a value for storage. Falls back to base64 when the OS has no keyring. */ function encrypt(value: string): string { try { diff --git a/apps/desktop/src/main/db-reader-pool.ts b/apps/desktop/src/main/db-reader-pool.ts index 0f7ef4a..e61ebd5 100644 --- a/apps/desktop/src/main/db-reader-pool.ts +++ b/apps/desktop/src/main/db-reader-pool.ts @@ -89,6 +89,20 @@ interface WorkerSlot { worker: Worker | null; pending: number; restartTimes: number[]; + /** Slot burned through MAX_RESTARTS and will not respawn on its own. + * Once every slot is in this state the pool can never recover, so + * `isInitialized()` reports false and heavy reads stop waiting for a + * worker that is never coming back. */ + gaveUp: boolean; +} + +function freshSlots(): WorkerSlot[] { + return Array.from({ length: POOL_SIZE }, () => ({ + worker: null, + pending: 0, + restartTimes: [], + gaveUp: false, + })); } export class DbReaderPool { @@ -107,15 +121,26 @@ export class DbReaderPool { } this.freezeWatchdogSab = freezeWatchdogSab; if (this.dbPath === dbPath && this.slots.some((s) => s.worker !== null)) return; + // Re-pointing at a different file replaces the slot array. Terminate + // whatever it still holds first — dropping the array on the floor + // would leak a live worker per slot, each keeping a SQLite handle and + // a 250 ms interval alive for the rest of the process. + this.disposeSlots(); this.dbPath = dbPath; - this.slots = Array.from({ length: POOL_SIZE }, () => ({ - worker: null, - pending: 0, - restartTimes: [], - })); + this.slots = freshSlots(); for (let i = 0; i < POOL_SIZE; i++) this.spawn(i); } + /** Terminate every live worker without touching `dbPath`/`terminated`. */ + private disposeSlots(): void { + for (const slot of this.slots) { + const w = slot.worker; + if (!w) continue; + slot.worker = null; + void w.terminate().catch(() => undefined); + } + } + /** Switch the pool to a different .seoproject file. */ swap(newPath: string, freezeWatchdogSab: SharedArrayBuffer | null = null): void { if (freezeWatchdogSab !== null) this.freezeWatchdogSab = freezeWatchdogSab; @@ -123,11 +148,7 @@ export class DbReaderPool { this.failPendingWith('reader-swapped'); this.dbPath = newPath; if (this.slots.length === 0) { - this.slots = Array.from({ length: POOL_SIZE }, () => ({ - worker: null, - pending: 0, - restartTimes: [], - })); + this.slots = freshSlots(); } for (let i = 0; i < POOL_SIZE; i++) this.spawn(i); } @@ -150,6 +171,23 @@ export class DbReaderPool { return !this.terminated && this.slots.some((s) => s.worker !== null); } + /** + * True while workers are still a viable execution path for this + * session — i.e. the pool was pointed at a project file AND at least + * one slot can still come back. Lets `callReaderOrFallback` tell a + * *recoverable* outage (workers crashed but will respawn → protect the + * main thread, retry next tick) from one with no recovery at all: + * - RAM-only mode, where the pools are never spawned, + * - a failed `init()`, + * - every slot having burned through MAX_RESTARTS. + * In those cases the main thread is the only place a read can run, so + * heavy methods must fall back rather than reject forever. + */ + isInitialized(): boolean { + if (this.dbPath === null) return false; + return this.slots.some((s) => !s.gaveUp); + } + /** * Dispatch a method call to the least-busy ready worker. Resolves * with the method's return value, rejects with an Error on worker @@ -237,9 +275,13 @@ export class DbReaderPool { w.on('error', (err) => { logger.log('error', 'db-reader', `worker[${slotIdx}] error: ${err.message}`); }); - w.on('exit', (code) => this.handleExit(slotIdx, code)); + // Pass the worker itself: the exit of the worker this one REPLACED + // arrives after we have already installed `w`, and must not be + // allowed to null out the live slot (see `handleExit`). + w.on('exit', (code) => this.handleExit(slotIdx, code, w)); slot.worker = w; slot.pending = 0; + slot.gaveUp = false; logger.log('info', 'db-reader', `worker[${slotIdx}] spawned for ${this.dbPath}`); } catch (err) { logger.log( @@ -268,9 +310,16 @@ export class DbReaderPool { } } - private handleExit(slotIdx: number, code: number): void { + private handleExit(slotIdx: number, code: number, worker?: Worker): void { const slot = this.slots[slotIdx]; if (!slot) return; + // A replaced worker (Open Project → `swap`, or a respawn) exits well + // after its successor is installed. Without this identity check that + // late event nulls the LIVE worker — orphaning a thread that keeps a + // SQLite handle and a 250 ms interval running for the rest of the + // process — and counts the deliberate replacement as a crash, so a + // handful of Open Projects exhausts MAX_RESTARTS and kills the pool. + if (worker && slot.worker !== worker) return; slot.worker = null; if (this.terminated) return; // Reject only this slot's in-flight requests. @@ -286,6 +335,7 @@ export class DbReaderPool { const now = Date.now(); slot.restartTimes = slot.restartTimes.filter((t) => now - t < RESTART_WINDOW_MS); if (slot.restartTimes.length >= MAX_RESTARTS) { + slot.gaveUp = true; logger.log( 'error', 'db-reader', @@ -349,7 +399,16 @@ export async function callReaderOrFallback( const pool = resolveReaderPool(); const heavy = HEAVY_METHODS.has(method); if (!pool.isReady()) { - if (heavy) { + // Heavy methods skip the main thread ONLY when a real worker pool + // exists but is momentarily down (all workers crashed/restarting): + // re-running a multi-second aggregate on the main thread would stall + // every IPC, and the next poll retries against the respawned worker. + // When the pool was never initialised at all — RAM-only storage mode, + // or a failed init — there is no worker to protect and no next-tick + // recovery; the main thread is the only place these reads can run, so + // fall back even for heavy methods (the RAM-only path already runs + // every other query this way). + if (heavy && pool.isInitialized()) { throw new Error(`reader-unavailable: '${method}' skips main-thread fallback`); } return fallback(); diff --git a/apps/desktop/src/main/db-reader-worker.ts b/apps/desktop/src/main/db-reader-worker.ts index 2cf1d1a..67c139a 100644 --- a/apps/desktop/src/main/db-reader-worker.ts +++ b/apps/desktop/src/main/db-reader-worker.ts @@ -86,6 +86,10 @@ const ALLOWED_METHODS = new Set([ 'getUrlHeaders', 'pageImagesDetailed', 'topAnchorTexts', + // Visualization tab — all three walk `links`/`urls` and used to run on + // the main thread, freezing the UI for the duration on large projects. + 'graphSnapshot', + 'crawlPath', 'topUrlsBy', 'externalDomainHealth', 'analyticsCoverage', diff --git a/apps/desktop/src/main/db-writer-pool.ts b/apps/desktop/src/main/db-writer-pool.ts index 18f0849..b5dc7a6 100644 --- a/apps/desktop/src/main/db-writer-pool.ts +++ b/apps/desktop/src/main/db-writer-pool.ts @@ -53,6 +53,12 @@ const MONITOR_INTERVAL_MS = 15_000; * long WITH a request pending means the thread has executed no JS at * all for 10 minutes — treat as wedged and recycle the worker. */ const WEDGE_TIMEOUT_MS = 10 * 60_000; +/** Fallback wedge threshold when there is no freeze-watchdog SAB to read a + * heartbeat from: recycle a worker whose oldest pending call has been + * outstanding this long. Set generously (2×) above the heartbeat threshold + * because without the heartbeat we can't tell a wedged thread from a + * legitimately very slow query, so we err toward patience. */ +const NO_SAB_WEDGE_TIMEOUT_MS = 20 * 60_000; /** * Rejection type for "the worker cannot run this job at all" — @@ -231,7 +237,22 @@ export class DbWriterPool { ); } } - if (!this.worker || !this.watchdogState) return; + if (!this.worker) return; + if (!this.watchdogState) { + // No SAB heartbeat available — fall back to a plain outstanding-time + // check so a genuinely wedged worker still gets recycled instead of + // hanging every pending write forever. Coarser (no per-op detail) and + // more patient to avoid killing a slow-but-live query. + if (now - oldestStartedAt >= NO_SAB_WEDGE_TIMEOUT_MS) { + logger.log( + 'error', + 'main', + `db-writer worker appears wedged: oldest of ${this.pending.size} pending call(s) outstanding for ${Math.round((now - oldestStartedAt) / 1000)}s (no heartbeat available) — terminating and respawning`, + ); + void this.worker.terminate().catch(() => undefined); + } + return; + } const heartbeatAge = now - this.watchdogState.readWriterHeartbeatMs(); if (heartbeatAge >= WEDGE_TIMEOUT_MS && now - oldestStartedAt >= WEDGE_TIMEOUT_MS) { const op = this.watchdogState.readWriterOp(); @@ -263,8 +284,12 @@ export class DbWriterPool { private handleExit(code: number): void { this.worker = null; if (this.terminated) return; - if (code === 0) return; + // Settle in-flight writes BEFORE the clean-exit early return. The pool + // has no request timeout, so a worker that exits with code 0 while + // holding pending calls would leave those promises unsettled forever — + // and every caller awaiting one hangs with it. this.failPendingWith('writer-crashed'); + if (code === 0) return; const now = Date.now(); this.restartTimes = this.restartTimes.filter((t) => now - t < RESTART_WINDOW_MS); if (this.restartTimes.length >= MAX_RESTARTS) { diff --git a/apps/desktop/src/main/google-oauth.ts b/apps/desktop/src/main/google-oauth.ts index 89f6f23..d2be3b9 100644 --- a/apps/desktop/src/main/google-oauth.ts +++ b/apps/desktop/src/main/google-oauth.ts @@ -27,7 +27,7 @@ import { app, safeStorage, shell } from 'electron'; import { createServer, type Server, type IncomingMessage, type ServerResponse } from 'node:http'; import { createHash, randomBytes } from 'node:crypto'; import { join } from 'node:path'; -import { readFileSync, writeFileSync, existsSync } from 'node:fs'; +import { readFileSync, writeFileSync, existsSync, chmodSync } from 'node:fs'; import type { GoogleAccount, GoogleAuthState } from '@freecrawl/shared-types'; import { apiFetch } from './api-fetch.js'; import { resolveCredentials } from './credentials.js'; @@ -167,7 +167,17 @@ function newAccountId(): string { function persist(): void { try { - writeFileSync(filePath(), JSON.stringify(cache ?? {}, null, 2), 'utf8'); + const p = filePath(); + // 0600 — refresh tokens live here (base64 fallback with no keyring), so + // the file must not be world-readable. Same pattern as credentials.ts. + writeFileSync(p, JSON.stringify(cache ?? {}, null, 2), { encoding: 'utf8', mode: 0o600 }); + if (process.platform !== 'win32') { + try { + chmodSync(p, 0o600); + } catch { + /* best-effort */ + } + } } catch (err) { logger.log('error', 'main', `google token store write failed: ${(err as Error).message}`); } diff --git a/apps/desktop/src/main/index.ts b/apps/desktop/src/main/index.ts index 1892fec..e0843a0 100644 --- a/apps/desktop/src/main/index.ts +++ b/apps/desktop/src/main/index.ts @@ -277,6 +277,8 @@ import { setCredentials as setIntegrationCredentials, clearCredentials as clearIntegrationCredentials, resolveCredentials, + encryptString, + decryptString, } from './credentials.js'; import { runPagespeedBatch, type PagespeedBatchItem } from './pagespeed.js'; import { runCruxBatch, type CruxBatchItem } from './crux.js'; @@ -323,7 +325,7 @@ import { WriterUnavailableError, } from './db-writer-pool.js'; import { freezeWatchdog } from './freeze-watchdog.js'; -import { parserPool } from './parser-pool.js'; +import { parserPool, ParserTimeoutError } from './parser-pool.js'; import { parseHtml as inlineParseHtml } from '@freecrawl/core'; import { startMcpBridge, @@ -538,9 +540,8 @@ class ProjectSession { try { const raw = database.getMeta('lastCrawlConfig'); if (raw) { - this.lastCrawlConfig = mergeCrawlConfig( - JSON.parse(raw) as Partial, - ); + const parsed = parseConfigWithEncryptedSecrets(raw); + if (parsed) this.lastCrawlConfig = mergeCrawlConfig(parsed); } } catch { /* malformed JSON or missing key — recovery just won't fire */ @@ -550,6 +551,20 @@ class ProjectSession { } catch { this.pendingRecoveryCheckpoint = []; } + // Wiping the scratch DB is correct on a cold start, but catastrophic if + // we somehow got here *after* teardown: at that point the session's DB + // was closed and nulled, so any late IPC re-enters this cold-start path + // and would erase results the user can still see on screen. Reopen + // read-as-is instead — a post-shutdown caller never wants a fresh + // database, and any code that reaches here then is racing the quit. + if (shutdownComplete) { + logger.log( + 'warn', + 'main', + 'getDb() re-entered after shutdown — reopening without reset to protect crawl data.', + ); + return database; + } database.reset(); // The scratch database is wiped on every launch, so the PNGs the last // session captured into its sidecar are now unreferenced. Drop them with @@ -562,6 +577,22 @@ class ProjectSession { /* best-effort — a locked file just means it is cleaned next launch */ } if (ramMode) { + // Drop the on-disk scratch database this session is NOT going to + // write. The MCP server resolves reads to `/projects/ + // default.seoproject` and has no way to know the desktop is holding + // its project in memory — so a file left over from an earlier + // disk-mode session made every MCP read return that crawl instead, + // silently reporting a completely different site as if it were the + // live one. Removing it turns a wrong answer into a clear "project + // file does not exist". Nothing durable is lost: disk mode wipes + // this same file on every cold start (`database.reset()` above). + for (const suffix of ['', '-wal', '-shm']) { + try { + rmSync(`${path}${suffix}`, { force: true }); + } catch { + /* locked by another instance — it will be swept next launch */ + } + } logger.log( 'info', 'main', @@ -669,6 +700,18 @@ defaultSession.label = 'Primary'; * default session is also registered here once its window is created. */ const sessionsByWc = new Map(); +/** + * Popup windows (Visualization) → the project session that opened them. + * + * Deliberately separate from `sessionsByWc`: several checks use + * `sessionsByWc.has(wcId)` to mean "is this a project window", and putting + * a popup in there would misclassify it. Without this map a popup's + * `webContents.id` matched nothing and `sessionFor` fell through to + * `defaultSession` — so opening Visualization from a second project window + * graphed the PRIMARY window's project, which is usually empty. + */ +const popupSessionsByWc = new Map(); + /** Headless MCP-agent sessions (Issue #12), keyed by bridge session id. These * have no window and are addressed only over the localhost bridge. Kept * separate from `sessionsByWc` (which is webContents-keyed) so the two @@ -692,7 +735,9 @@ const sessionStore = new AsyncLocalStorage(); /** Session that owns a given renderer, or the default session. */ function sessionFor(wc: WebContents): ProjectSession { - return sessionsByWc.get(wc.id) ?? defaultSession; + return ( + sessionsByWc.get(wc.id) ?? popupSessionsByWc.get(wc.id) ?? defaultSession + ); } /** Resolve the session for the currently-running code path: the calling @@ -1273,6 +1318,56 @@ function prefsFilePath(): string { return join(app.getPath('userData'), 'preferences.json'); } +/** Prefs key holding the last crawl config. Its credential fields are + * encrypted at rest (see prefsForDisk / hydration below) so they never sit + * in plaintext in preferences.json. */ +const CRAWL_CONFIG_PREF_KEY = 'crawl-config'; + +/** Build the on-disk prefs object: identical to the in-memory cache except + * the crawl-config's secrets are split out and encrypted. The cache itself + * stays plaintext so the renderer's sync `prefsGetAllSync` is unaffected. */ +function prefsForDisk(): Record { + const out: Record = { ...prefsCache }; + const cfg = out[CRAWL_CONFIG_PREF_KEY]; + if (cfg && typeof cfg === 'object' && !Array.isArray(cfg)) { + const { clean, secrets } = splitConfigSecrets(cfg as CrawlConfig); + out[CRAWL_CONFIG_PREF_KEY] = + Object.keys(secrets).length > 0 + ? { __fcEnc: 1, config: clean, secretsEnc: encryptString(JSON.stringify(secrets)) } + : clean; + } + return out; +} + +/** Reverse of prefsForDisk for the crawl-config key, mutating the cache so + * the renderer receives a plaintext config. Tolerates the legacy bare + * (unencrypted) shape. */ +function hydrateCrawlConfigInCache(): void { + const cfg = prefsCache[CRAWL_CONFIG_PREF_KEY]; + if ( + cfg && + typeof cfg === 'object' && + !Array.isArray(cfg) && + (cfg as { __fcEnc?: number }).__fcEnc === 1 + ) { + const env = cfg as { config: CrawlConfig; secretsEnc?: string }; + const plain = env.config; + if (env.secretsEnc) { + try { + const secrets = JSON.parse(decryptString(env.secretsEnc)) as Record; + applyConfigSecrets(plain, secrets); + } catch { + /* undecryptable — keep the blanked config */ + } + } + prefsCache[CRAWL_CONFIG_PREF_KEY] = plain; + } +} + +function writePrefsToDisk(): void { + writeFileSync(prefsFilePath(), JSON.stringify(prefsForDisk(), null, 2), 'utf8'); +} + function loadPrefs(): void { if (prefsLoaded) return; const path = prefsFilePath(); @@ -1282,6 +1377,7 @@ function loadPrefs(): void { const parsed = JSON.parse(raw) as unknown; if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { prefsCache = parsed as Record; + hydrateCrawlConfigInCache(); } } } catch { @@ -1296,7 +1392,7 @@ function schedulePrefsWrite(): void { prefsWriteTimer = setTimeout(() => { prefsWriteTimer = null; try { - writeFileSync(prefsFilePath(), JSON.stringify(prefsCache, null, 2), 'utf8'); + writePrefsToDisk(); } catch { // ignore — best-effort persistence } @@ -1314,7 +1410,7 @@ function flushPrefs(): void { // `{}` here would clobber the running first instance's saved prefs. if (!prefsLoaded) return; try { - writeFileSync(prefsFilePath(), JSON.stringify(prefsCache, null, 2), 'utf8'); + writePrefsToDisk(); } catch { // ignore } @@ -1594,6 +1690,22 @@ async function parseHtmlViaPool( try { return await parserPool.parse(html, pageUrl, opts); } catch (err) { + if (err instanceof ParserTimeoutError) { + // Do NOT re-parse inline. This document already pinned a worker + // thread for a full minute; running the same synchronous cheerio + // pass on the main thread would freeze the entire app — UI, IPC, + // crawler scheduling — for at least as long. Reject instead: the + // crawler records the URL as failed with this reason, the rest of + // the crawl proceeds, and the pool has already recycled the worker. + logger.log( + 'warn', + 'main', + `parser-pool timed out on ${pageUrl} — recording it as failed rather than blocking the main thread.`, + ); + throw err; + } + // Pool not ready or the worker crashed — the document itself is not + // implicated, so parsing it inline is safe and keeps the crawl whole. logger.log( 'warn', 'main', @@ -1767,11 +1879,22 @@ function applyCurrentThermalState(crawler: Crawler): void { } } +/** Post-crawl passes whose peak memory scales with the whole link graph + * rather than with a bounded batch. If the writer worker dies partway + * through one of these, the most likely cause is that the pass exhausted + * the worker's heap — and re-running the identical work on the main thread + * then takes the entire app down with `FatalProcessOutOfMemory`, losing the + * crawl the user just paid for. Same reasoning as `HEAVY_METHODS` in + * db-reader-pool.ts: contain the failure instead of escalating it. */ +const MEMORY_HEAVY_PASSES = new Set(['recomputeLinkScore']); + async function runDbPassViaPool( session: ProjectSession, method: string, ): Promise { if (!session.writerPool.isReady()) { + // No worker pool at all (RAM-only mode, or init failed) — the main + // thread is the only place this can run, heavy or not. const db = session.getDb(); const fn = (db as unknown as Record)[method]; if (typeof fn === 'function') (fn as () => void).call(db); @@ -1782,6 +1905,18 @@ async function runDbPassViaPool( } catch (err) { // WriterUnavailableError only — see recomputeIssuesViaPool. if (!(err instanceof WriterUnavailableError)) throw err; + if (MEMORY_HEAVY_PASSES.has(method)) { + // Skip rather than escalate. The affected column keeps its previous + // values, which is recoverable; a dead main process is not. + logger.log( + 'error', + 'main', + `db-writer died during ${method} (${err.message}) — skipping this pass ` + + 'rather than re-running it on the main thread. Results are complete ' + + 'apart from this metric; re-crawl or reopen the project to retry.', + ); + return; + } logger.log( 'warn', 'main', @@ -1881,6 +2016,126 @@ function getDb(): ProjectDb { return currentSession().getDb(); } +// ── Crawl-config secret handling ─────────────────────────────────────────── +// +// `CrawlConfig` carries credentials — HTTP-auth password/token, the browser +// form-login password, and a proxy URL that can embed `user:pass@`. These +// must not sit in plaintext in preferences.json or the .seoproject, nor be +// handed to a colleague in a shared settings-export. `configSecretFields` +// enumerates them; the helpers below either blank them (export) or split +// them out for at-rest encryption (prefs / project meta). + +/** Return the secret values from a config as a flat record, and a deep-ish + * copy of the config with those values blanked. */ +function splitConfigSecrets(config: CrawlConfig): { + clean: CrawlConfig; + secrets: Record; +} { + const clean: CrawlConfig = JSON.parse(JSON.stringify(config)) as CrawlConfig; + const secrets: Record = {}; + if (clean.auth) { + if (clean.auth.password) { + secrets['auth.password'] = clean.auth.password; + clean.auth.password = ''; + } + if (clean.auth.token) { + secrets['auth.token'] = clean.auth.token; + clean.auth.token = ''; + } + } + const pv = clean.formLogin?.browser?.passwordValue; + if (pv) { + secrets['formLogin.browser.passwordValue'] = pv; + clean.formLogin!.browser!.passwordValue = ''; + } + if (clean.proxyUrl) { + secrets['proxyUrl'] = clean.proxyUrl; + clean.proxyUrl = ''; + } + return { clean, secrets }; +} + +/** Re-apply previously split secrets onto a (cleaned) config, in place. */ +function applyConfigSecrets( + config: CrawlConfig, + secrets: Record, +): CrawlConfig { + if (secrets['auth.password'] !== undefined && config.auth) { + config.auth.password = secrets['auth.password']; + } + if (secrets['auth.token'] !== undefined && config.auth) { + config.auth.token = secrets['auth.token']; + } + if ( + secrets['formLogin.browser.passwordValue'] !== undefined && + config.formLogin?.browser + ) { + config.formLogin.browser.passwordValue = secrets['formLogin.browser.passwordValue']; + } + if (secrets['proxyUrl'] !== undefined) { + config.proxyUrl = secrets['proxyUrl']; + } + return config; +} + +/** A copy of the config with every secret blanked — for the shareable + * settings-export, which must never carry credentials. Accepts a loose + * record because the export IPC types its payload as `Record`; the JSON round-trip inside `splitConfigSecrets` tolerates + * partial/extra fields. */ +function redactConfigSecrets(config: Record): Record { + return splitConfigSecrets(config as unknown as CrawlConfig).clean as unknown as Record< + string, + unknown + >; +} + +/** Serialize a config for at-rest storage: secrets are pulled out and the + * whole secret bundle is encrypted into one field, the rest stays plain + * (and human-diffable). Returns the JSON string to persist. */ +function serializeConfigWithEncryptedSecrets(config: CrawlConfig): string { + const { clean, secrets } = splitConfigSecrets(config); + const payload: { config: CrawlConfig; secretsEnc?: string } = { config: clean }; + if (Object.keys(secrets).length > 0) { + payload.secretsEnc = encryptString(JSON.stringify(secrets)); + } + return JSON.stringify(payload); +} + +/** Reverse of {@link serializeConfigWithEncryptedSecrets}. Accepts both the + * new `{config, secretsEnc}` envelope and a legacy bare-config JSON string + * (pre-encryption projects) so old files still load. Returns null on parse + * failure. */ +function parseConfigWithEncryptedSecrets(raw: string): Partial | null { + try { + const parsed = JSON.parse(raw) as + | { config?: Partial; secretsEnc?: string } + | Partial; + if ( + parsed && + typeof parsed === 'object' && + 'config' in parsed && + (parsed as { config?: unknown }).config + ) { + const env = parsed as { config: Partial; secretsEnc?: string }; + const cfg = env.config; + if (env.secretsEnc) { + try { + const secrets = JSON.parse(decryptString(env.secretsEnc)) as Record; + applyConfigSecrets(cfg as CrawlConfig, secrets); + } catch { + /* corrupt/undecryptable secrets — load the rest without them */ + } + } + return cfg; + } + // Legacy: the whole thing is a bare config object. + return parsed as Partial; + } catch { + return null; + } +} + /** Monotonic suffix so each open gets its own working file. */ let workingDbCounter = 0; @@ -1901,7 +2156,32 @@ function newWorkingDbPath(): string { return join(dir, `work-${process.pid}-${workingDbCounter}.seoproject`); } -/** Delete working copies orphaned by a crash (never the live one). */ +/** + * Every working copy a live session in this process is currently using. + * + * Both sweeps must honour ALL of them. The mid-session sweep on Open + * Project used to pass only the path it had just created, so opening a + * project in one window deleted the live database of every OTHER window + * and of every headless agent session. On Windows the unlink fails with a + * sharing violation and is swallowed, but on macOS and Linux it succeeds: + * the other window keeps running against an unlinked inode until one of + * its SQLite connections reopens, then silently starts answering from a + * fresh empty file — and its crawl is gone at restart. + */ +function liveWorkingPaths(extra?: string): ReadonlySet { + const live = new Set(); + if (extra) live.add(extra); + for (const s of [ + defaultSession, + ...sessionsByWc.values(), + ...headlessSessions.values(), + ]) { + if (s.scratchPath) live.add(s.scratchPath); + } + return live; +} + +/** Delete working copies orphaned by a crash (never a live one). */ function sweepStaleWorkingCopies(keep: ReadonlySet): void { try { const dir = join(app.getPath('userData'), 'projects'); @@ -1964,11 +2244,8 @@ function sweepStaleWorkingCopies(keep: ReadonlySet): void { function restoreProjectConfig(db: ProjectDb): CrawlConfig | null { const raw = db.getMeta('lastCrawlConfig'); if (raw) { - try { - return mergeCrawlConfig(JSON.parse(raw) as Partial); - } catch { - /* malformed JSON — fall through to the engine seed */ - } + const parsed = parseConfigWithEncryptedSecrets(raw); + if (parsed) return mergeCrawlConfig(parsed); } const seed = db.getMeta('startUrl'); if (seed && /^https?:\/\//i.test(seed)) { @@ -2066,7 +2343,7 @@ async function openProjectAtPath(documentPath: string): Promise { updateWindowTitle(s); // The database we just replaced is nobody's now — reclaim its disk. if (previousWorking && previousWorking !== workingPath) { - sweepStaleWorkingCopies(new Set([workingPath])); + sweepStaleWorkingCopies(liveWorkingPaths(workingPath)); } // Rehydrate the crawl config from the project we just opened and push it // to the renderer so the UI reflects THIS project's saved settings @@ -2197,7 +2474,19 @@ async function saveProjectDocument( try { // eslint-disable-next-line @typescript-eslint/no-explicit-any const rawDb = (database as any).db as { exec: (sql: string) => void }; - rawDb.exec(`VACUUM INTO '${tmpPath.replace(/'/g, "''")}'`); + // `VACUUM INTO` rewrites the whole database in one synchronous call, so + // the window is unresponsive for its duration — seconds on a large + // crawl. It stays on the main thread deliberately: it must run on the + // connection that holds the data, and in RAM-only mode that is the ONLY + // connection (no writer worker exists). Tell the freeze watchdog what + // is happening so this shows up in debug.txt as a known save stall + // rather than an unattributed hang. + freezeWatchdog.setMainOp('save:vacuum'); + try { + rawDb.exec(`VACUUM INTO '${tmpPath.replace(/'/g, "''")}'`); + } finally { + freezeWatchdog.setMainOp('idle'); + } const result = await packProject(tmpPath, documentPath); s.currentProjectPath = documentPath; s.dirty = false; @@ -2345,7 +2634,13 @@ function attachUnsavedGuard(win: BrowserWindow): void { cancelId: 2, noLink: true, }); - if (res.response === 2) return; // Cancel — stay open. + if (res.response === 2) { + // Cancel — stay open, and disarm any quit that was waiting on this + // prompt so the app doesn't tear itself down the moment the last + // other window closes. + cancelPendingQuit(); + return; + } if (res.response === 0) { const target = s.currentProjectPath; const saved = target @@ -2358,7 +2653,10 @@ function attachUnsavedGuard(win: BrowserWindow): void { ) : (await runSaveProjectAs()) !== null; // A failed or cancelled save must not throw the data away. - if (!saved) return; + if (!saved) { + cancelPendingQuit(); + return; + } } allowClose = true; s.dirty = false; @@ -2755,7 +3053,10 @@ async function downloadAndRevealInstaller(asset: GitHubReleaseAsset): Promise((resolve) => { const handler = ( @@ -3144,6 +3445,13 @@ function createWindow(): void { title: `FreeCrawl SEO Tool v${app.getVersion()}`, webPreferences: { preload: join(__dirname, '../preload/index.js'), + // sandbox stays false: enabling it was tried and blanks the renderer — + // under a sandboxed preload `window.freecrawl` is never exposed (the + // bundled preload isn't sandbox-compatible as built), so the React app + // dies on its first IPC call while the native menu still renders. The + // OS-sandbox loss is mitigated by contextIsolation:true plus the + // app-wide window-open / will-navigate policy (installWebContentsPolicy), + // which stops untrusted crawled content from ever loading in a frame. sandbox: false, contextIsolation: true, nodeIntegration: false, @@ -3443,6 +3751,36 @@ function validateProjectPath( return { ok: true, normalized }; } +/** + * Validate an MCP-supplied export file path. The agent driving the bridge + * reads crawled page content, so a prompt-injected `filePath` must not be + * able to write into the app install dir or the app's own data dir + * (preferences.json, credential stores, project working copies, the + * mcp-bridge discovery file). Requires an absolute path outside both. + * Returns the resolved path or throws with a clear message. + */ +function assertMcpExportPath(raw: unknown): string { + if (typeof raw !== 'string' || raw.trim() === '') { + throw new Error('filePath is required (MCP cannot open dialogs).'); + } + const trimmed = raw.trim(); + if (!isAbsolute(trimmed)) { + throw new Error(`filePath must be an absolute path (got '${trimmed}').`); + } + const normalized = resolvePath(trimmed); + const lower = normalized.toLowerCase(); + const denied = [resolvePath(app.getAppPath()), resolvePath(app.getPath('userData'))]; + for (const dir of denied) { + const d = dir.toLowerCase(); + if (lower === d || lower.startsWith(d + pathSep.toLowerCase())) { + throw new Error( + 'filePath must be outside the application and application-data directories.', + ); + } + } + return normalized; +} + /** * Whether a `ProjectSession` is worth advertising to MCP `session_list`. * Build the wire shape from a live session. @@ -3495,6 +3833,56 @@ function isSafeExternalUrl(raw: string): boolean { } } +/** + * Apply the window-opening and navigation policy to EVERY renderer, current + * and future. + * + * This used to be set only on `mainWindow`, and there was no `will-navigate` + * guard anywhere. The renderer opens crawled URLs in ten places — the + * broken-link context menu, several detail-panel tables, and the standalone + * Visualization window's node menus — all of them `window.open(url, + * '_blank')` on a URL that came from the crawled site. In any window other + * than the main one (File → New Project Window, or the Visualization + * window) Electron's default action is `allow`, so that opened a real + * BrowserWindow on the attacker's page. `window.open(…, '_blank')` does not + * imply `noopener`, so the child kept a live `window.opener` and could + * navigate this app's window off `file://`. The preload re-runs on the new + * document, which would hand an attacker origin the entire `window.freecrawl` + * IPC surface — arbitrary file read via `logAnalyze`, arbitrary write via the + * export handlers, credential writes via `integrationsSet`. + * + * Registering on `web-contents-created` means no future window can forget. + */ +function installWebContentsPolicy(): void { + app.on('web-contents-created', (_event, contents) => { + contents.setWindowOpenHandler(({ url }) => { + // Hand real web/mail links to the OS browser; never let Electron + // open a window of its own, which would carry our preload. + if (isSafeExternalUrl(url)) void shell.openExternal(url); + return { action: 'deny' }; + }); + + contents.on('will-navigate', (event, url) => { + // The renderer only ever legitimately navigates within its own + // bundle (file:// in production, the Vite dev server in development). + // Anything else is either a hijack attempt or an accident; send real + // web links to the OS browser instead. + const devUrl = process.env['ELECTRON_RENDERER_URL']; + const allowed = + url.startsWith('file://') || (devUrl ? url.startsWith(devUrl) : false); + if (allowed) return; + event.preventDefault(); + if (isSafeExternalUrl(url)) void shell.openExternal(url); + }); + + // Nothing here needs camera, microphone, geolocation or notifications. + // Deny by default so a page that somehow loads can't prompt for them. + contents.session.setPermissionRequestHandler((_wc, _permission, cb) => { + cb(false); + }); + }); +} + /** * Open (or focus) the Logs popup for a specific project window. Loads the * same renderer bundle with `?logs=1&owner=` so the LogsView filters to @@ -3619,8 +4007,16 @@ function openLogsWindow(ownerWcId: number): void { * Keeps the visualization off the main tab strip and gives it its own * OS window the user can move to a second monitor. */ -function openVisualizationWindow(): void { +function openVisualizationWindow(ownerWcId?: number): void { + // Bind the popup to the project window that asked for it, the way + // `openLogsWindow` does. Without this its IPC resolved to the default + // session and it graphed the wrong project entirely. + const ownerSession = + (ownerWcId !== undefined ? sessionsByWc.get(ownerWcId) : undefined) ?? + defaultSession; if (visualizationWindow && !visualizationWindow.isDestroyed()) { + // Re-point the singleton at whoever is asking now. + popupSessionsByWc.set(visualizationWindow.webContents.id, ownerSession); visualizationWindow.show(); visualizationWindow.focus(); return; @@ -3645,7 +4041,9 @@ function openVisualizationWindow(): void { win.setMenu(null); win.on('ready-to-show', () => win.show()); win.on('page-title-updated', (e) => e.preventDefault()); + popupSessionsByWc.set(win.webContents.id, ownerSession); win.on('closed', () => { + popupSessionsByWc.delete(win.webContents.id); if (visualizationWindow === win) visualizationWindow = null; }); // Mirror the main window's dev-tools lockdown. @@ -3743,6 +4141,20 @@ function openLogAnalyzerWindow(): void { */ function ingestLogResult(filePath: string, result: LogAnalysisResult): LogImportSummary { const fileName = basename(filePath); + // Snapshot the URL aggregate ONCE. `urlStats` and `urlBots` below both + // walked `result.urlStats.values()` independently, so two full-size + // arrays existed alongside the still-live `result` — at the analyzer's + // 500K-path cap that is a lot of main-thread heap for no reason. + const urlAggregates = Array.from(result.urlStats.values()); + // The bot breakdown fans out to one row per (path, bot) pair, so it is + // the term that actually scales. Fail with a clear message instead of + // an OOM abort — the log export path 20 lines down already does this. + let botRowEstimate = 0; + for (const u of urlAggregates) botRowEstimate += u.botCounts.size; + ensureHeapHeadroom( + 'log analysis ingest', + urlAggregates.length * 256 + botRowEstimate * 128, + ); const payload: LogIngestInput = { file: { filePath, @@ -3754,7 +4166,7 @@ function ingestLogResult(filePath: string, result: LogAnalysisResult): LogImport minTs: result.minTs, maxTs: result.maxTs, }, - urlStats: Array.from(result.urlStats.values()).map((u) => ({ + urlStats: urlAggregates.map((u) => ({ path: u.path, totalHits: u.totalHits, botHits: u.botHits, @@ -3784,7 +4196,7 @@ function ingestLogResult(filePath: string, result: LogAnalysisResult): LogImport verifiedIps: agg.verifiedIps, verifiable: agg.verifiable, })), - urlBots: Array.from(result.urlStats.values()).flatMap((u) => + urlBots: urlAggregates.flatMap((u) => Array.from(u.botCounts.entries()).map(([bot, hits]) => ({ path: u.path, bot, hits })), ), }; @@ -4030,7 +4442,9 @@ function registerIpc(): void { // Renderer button in a project window — own the Logs popup by that window. openLogsWindow(currentSession().logTag ?? mainWindow?.webContents.id ?? 0), ); - registerHandle(IPC.visualizationOpenWindow, () => openVisualizationWindow()); + registerHandle(IPC.visualizationOpenWindow, (e) => + openVisualizationWindow(e.sender.id), + ); // ── V2 Faz 2 — Log File Analyzer IPC surface ─────────────────────── registerHandle(IPC.logAnalyzerOpenWindow, () => openLogAnalyzerWindow()); @@ -4690,8 +5104,9 @@ function registerIpc(): void { // Top Words — pull the title+meta+H1 corpus from the active DB then // aggregate via core's `aggregateTopWords` (kept in core because the // db package isn't allowed to depend on core per the monorepo - // dependency graph). Aggregation is sync + cheap so we don't need - // the reader pool's worker offload here. + // dependency graph). The aggregation itself is sync + cheap; the corpus + // is now streamed (`seoTextCorpus` yields), so neither half holds the + // whole table in memory and the main thread stays usable. registerHandle( IPC.reportsTopWords, (_e, input: TopWordsInput): TopWordsRow[] => { @@ -4724,7 +5139,9 @@ function registerIpc(): void { format: 'freecrawl/settings', version: 1, exportedAt: new Date().toISOString(), - config: input.config, + // Strip credentials — this file is meant to be shared with a + // colleague. Import tolerates the blanked fields. + config: redactConfigSecrets(input.config), }; const json = JSON.stringify(payload, null, 2); writeFileSync(filePath, json, 'utf8'); @@ -6468,7 +6885,7 @@ function registerIpc(): void { // without the user re-entering anything. Stored as JSON in // `project_meta` since the table is keyed by string. try { - database.setMeta('lastCrawlConfig', JSON.stringify(config)); + database.setMeta('lastCrawlConfig', serializeConfigWithEncryptedSecrets(config)); } catch { /* never fatal */ } @@ -6911,11 +7328,8 @@ function registerIpc(): void { if (!config) { const stored = db.getMeta('lastCrawlConfig'); if (stored) { - try { - config = JSON.parse(stored) as CrawlConfig; - } catch { - /* corrupt meta — fall through to defaults */ - } + const parsed = parseConfigWithEncryptedSecrets(stored); + if (parsed) config = parsed as CrawlConfig; } } const seed = db.getMeta('startUrl') ?? urls[0]!; @@ -7053,37 +7467,39 @@ function registerIpc(): void { 'export-csv': async (input) => { const i = input as ExportCsvInput; - if (!i.filePath) throw new Error('filePath is required (MCP cannot open dialogs).'); - const { rowsWritten } = await exportUrlsToCsv(getDb(), i.filePath, { + const filePath = assertMcpExportPath(i.filePath); + const { rowsWritten } = await exportUrlsToCsv(getDb(), filePath, { selectedIds: i.selectedIds, + category: i.category, }); - return { filePath: i.filePath, rowsWritten }; + return { filePath, rowsWritten }; }, 'export-json': async (input) => { const i = input as ExportJsonInput; - if (!i.filePath) throw new Error('filePath is required.'); - const { rowsWritten } = await exportUrlsToJson(getDb(), i.filePath, { + const filePath = assertMcpExportPath(i.filePath); + const { rowsWritten } = await exportUrlsToJson(getDb(), filePath, { selectedIds: i.selectedIds, pretty: i.pretty, + category: i.category, }); - return { filePath: i.filePath, rowsWritten }; + return { filePath, rowsWritten }; }, 'export-xml': async (input) => { const i = input as ExportXmlInput; - if (!i.filePath) throw new Error('filePath is required.'); - const { rowsWritten } = await exportUrlsToXml(getDb(), i.filePath, { + const filePath = assertMcpExportPath(i.filePath); + const { rowsWritten } = await exportUrlsToXml(getDb(), filePath, { selectedIds: i.selectedIds, category: i.category, }); - return { filePath: i.filePath, rowsWritten }; + return { filePath, rowsWritten }; }, 'export-html-report': async (input) => { const i = input as ExportHtmlReportInput; - if (!i.filePath) throw new Error('filePath is required.'); - const result = await exportHtmlReport(getDb(), i.filePath, { + const filePath = assertMcpExportPath(i.filePath); + const result = await exportHtmlReport(getDb(), filePath, { startUrl: getDb().getMeta('lastStartUrl') ?? '', }); return { filePath: result.filePath, bytesWritten: result.bytesWritten }; @@ -7091,11 +7507,12 @@ function registerIpc(): void { 'sitemap-generate': async (input) => { const i = input as SitemapGenerateInput; - if (!i.filePath) throw new Error('filePath is required.'); - return exportSitemap(getDb(), i.filePath, { + const filePath = assertMcpExportPath(i.filePath); + return exportSitemap(getDb(), filePath, { variant: i.variant, gzip: i.gzip, splitAtUrlCount: i.splitAtUrlCount, + baseUrl: getDb().getMeta('lastStartUrl') ?? undefined, }); }, @@ -7156,7 +7573,9 @@ function registerIpc(): void { 'compare-load': async (input) => { const i = input as CompareLoadInput; if (!i.filePath) throw new Error('filePath is required (MCP cannot open dialogs).'); - const otherDb = new ProjectDb(i.filePath); + // read-only: never create or mutate the compared file (see the IPC + // compare-load handler for the full rationale). + const otherDb = new ProjectDb(i.filePath, { readOnly: true }); try { const summary = compareCrawls(getDb(), otherDb); return { @@ -7228,12 +7647,17 @@ function registerIpc(): void { }, 'export-broken-links': async (input) => { - const i = input as { filePath?: string; internal?: 'all' | 'internal' | 'external' }; - if (!i.filePath) throw new Error('filePath is required.'); - const { rowsWritten } = await exportBrokenLinksToCsv(getDb(), i.filePath, { + const i = input as { + filePath?: string; + internal?: 'all' | 'internal' | 'external'; + search?: string; + }; + const filePath = assertMcpExportPath(i.filePath); + const { rowsWritten } = await exportBrokenLinksToCsv(getDb(), filePath, { internal: i.internal ?? 'all', + search: i.search, }); - return { filePath: i.filePath, rowsWritten }; + return { filePath, rowsWritten }; }, 'export-images': async (input) => { @@ -7244,24 +7668,26 @@ function registerIpc(): void { duplicateAltOnly?: boolean; search?: string; }; - if (!i.filePath) throw new Error('filePath is required.'); - const { rowsWritten } = await exportImagesToCsv(getDb(), i.filePath, { + const filePath = assertMcpExportPath(i.filePath); + const { rowsWritten } = await exportImagesToCsv(getDb(), filePath, { missingAltOnly: i.missingAltOnly, emptyAltOnly: i.emptyAltOnly, duplicateAltOnly: i.duplicateAltOnly, search: i.search, }); - return { filePath: i.filePath, rowsWritten }; + return { filePath, rowsWritten }; }, 'export-tabular': async (input) => { const i = input as ExportTabularInput; - if (!i.filePath) throw new Error('filePath is required.'); - return exportTabular(getDb(), i.filePath, { + const filePath = assertMcpExportPath(i.filePath); + return exportTabular(getDb(), filePath, { format: i.format, sections: i.sections, columns: i.columns, selectedIds: i.selectedIds, + search: i.search, + filter: i.filter, }); }, @@ -7286,7 +7712,10 @@ function registerIpc(): void { } currentSession().lastCrawlConfig = { ...DEFAULT_CRAWL_CONFIG, ...cfg }; try { - getDb().setMeta('lastCrawlConfig', JSON.stringify(currentSession().lastCrawlConfig)); + getDb().setMeta( + 'lastCrawlConfig', + serializeConfigWithEncryptedSecrets(currentSession().lastCrawlConfig!), + ); } catch { /* non-fatal — config still set in memory */ } @@ -8098,7 +8527,7 @@ function registerIpc(): void { registerHandle(IPC.projectConfigSet, (_e, config: CrawlConfig): void => { if (!currentSession().currentProjectPath) return; try { - getDb().setMeta('lastCrawlConfig', JSON.stringify(config)); + getDb().setMeta('lastCrawlConfig', serializeConfigWithEncryptedSecrets(config)); currentSession().lastCrawlConfig = config; } catch (err) { logger.log( @@ -8718,6 +9147,7 @@ function registerIpc(): void { } const { rowsWritten } = await exportUrlsToCsv(getDb(), filePath, { selectedIds: input.selectedIds, + category: input.category, }); return { filePath, rowsWritten }; }, @@ -8741,6 +9171,7 @@ function registerIpc(): void { const { rowsWritten } = await exportUrlsToJson(getDb(), filePath, { selectedIds: input.selectedIds, pretty: input.pretty, + category: input.category, }); return { filePath, rowsWritten }; }, @@ -8788,6 +9219,7 @@ function registerIpc(): void { } const { rowsWritten } = await exportBrokenLinksToCsv(getDb(), filePath, { internal: input.internal, + search: input.search, }); return { filePath, rowsWritten }; }, @@ -8864,6 +9296,7 @@ function registerIpc(): void { IPC.exportTabular, async (_e, input: ExportTabularInput): Promise => { const { format, sections, columns, selectedIds, csvBom } = input; + const { search, filter } = input; let outputPath = input.filePath ?? ''; const isSelection = (selectedIds?.length ?? 0) > 0; const multiSection = sections.length > 1; @@ -8907,6 +9340,8 @@ function registerIpc(): void { sections, columns, selectedIds, + search, + filter, csvBom, }); return result; @@ -9138,6 +9573,7 @@ function registerIpc(): void { variant, gzip, splitAtUrlCount: input.splitAtUrlCount, + baseUrl: getDb().getMeta('lastStartUrl') ?? undefined, }); if (mainWindow) { const detail = result.sharded @@ -9202,10 +9638,13 @@ function registerIpc(): void { } filePath = res.filePaths[0]!; } - // Open the *other* project read-only — never mutate. The - // ProjectDb constructor opens the file in default mode; that's - // fine because we never call write methods on it during the diff. - const otherDb = new ProjectDb(filePath); + // Open the *other* project read-only. Passing no options opened it in + // WRITER mode: node:sqlite CREATES a missing file (an arbitrary-file + // -create primitive reachable from the renderer/MCP) and, on an + // existing file, runs migrations + takes a write lock — silently + // rewriting a project the user only meant to diff against. readOnly + // requires the file to exist and never mutates it. + const otherDb = new ProjectDb(filePath, { readOnly: true }); try { const summary = compareCrawls(getDb(), otherDb); return { @@ -9223,22 +9662,31 @@ function registerIpc(): void { registerHandle( IPC.crawlPath, - (_e, input: CrawlPathInput): CrawlPathResult => { - return getDb().crawlPath(input.urlId); - }, + async (_e, input: CrawlPathInput): Promise => + callReaderOrFallback('crawlPath', [input.urlId], () => + getDb().crawlPath(input.urlId), + ), ); registerHandle( IPC.graphSnapshot, - (_e, input: GraphSnapshotInput): GraphSnapshotResult => { - return getDb().graphSnapshot(input.nodeLimit ?? 1000); + async (_e, input: GraphSnapshotInput): Promise => { + const nodeLimit = input.nodeLimit ?? 1000; + return callReaderOrFallback( + 'graphSnapshot', + [nodeLimit], + () => getDb().graphSnapshot(nodeLimit), + ); }, ); registerHandle( IPC.topAnchorTexts, - (_e, limit: number | undefined): AnchorTextRow[] => { - return getDb().topAnchorTexts(limit ?? 200); + async (_e, limit: number | undefined): Promise => { + const n = limit ?? 200; + return callReaderOrFallback('topAnchorTexts', [n], () => + getDb().topAnchorTexts(n), + ); }, ); } @@ -9249,6 +9697,10 @@ function registerIpc(): void { logger.installGlobalHooks(); logger.log('info', 'main', `App bootstrap — Node ${process.version} on ${process.platform}`); +// Before any window exists, so every renderer this process ever creates is +// covered — including the popups and any window added later. +installWebContentsPolicy(); + // Single-instance guard. Without this, double-clicking the launcher // shortcut while the app is already open spawns a second Electron // process that races for the same userData/Cache/GPUCache directories @@ -9640,11 +10092,7 @@ void app.whenReady().then(async () => { // live session is using is passed in as `keep`; on a cold start the // only live one is whatever the default session already resolved. try { - const live = new Set(); - for (const s of [defaultSession, ...sessionsByWc.values()]) { - if (s.scratchPath) live.add(s.scratchPath); - } - sweepStaleWorkingCopies(live); + sweepStaleWorkingCopies(liveWorkingPaths()); } catch (err) { logger.log('warn', 'main', `startup working-copy sweep failed: ${(err as Error).message}`); } @@ -9759,7 +10207,45 @@ function performShutdown(): void { logger.flushFileLogging(); } -app.on('before-quit', () => { +/** Every windowed session that still holds unsaved crawl results. Headless + * agent sessions are excluded: they own no document and are disposed by + * `performShutdown` itself. */ +function anyUnsavedWindowSession(): boolean { + const windowed = new Set([defaultSession, ...sessionsByWc.values()]); + for (const s of windowed) if (s.dirty) return true; + return false; +} + +/** A quit was requested and is waiting on the unsaved-changes prompts. */ +let quitRequested = false; + +/** Called by `attachUnsavedGuard` when the user declines to close, so a + * cancelled Cmd/Ctrl+Q doesn't leave a quit armed behind it. */ +function cancelPendingQuit(): void { + quitRequested = false; +} + +app.on('before-quit', (event) => { + // The unsaved-changes prompt lives on each window's `close` handler, and + // Electron runs those AFTER `before-quit`. Tearing the backend down here + // first meant a user who answered "Cancel" was left with a running app + // whose DB was closed and whose worker pools were permanently terminated: + // the next DB touch re-entered `getDb()`'s cold-start path and `reset()` + // wiped the crawl — and answering "Save" instead vacuumed that freshly + // emptied database over the user's `.seoproject`. + // + // So when anything is unsaved, cancel this quit without touching a thing + // and let the windows run their own guard. If the user accepts, the guard + // clears `dirty` and closes the window; `window-all-closed` then re-quits + // and this handler falls through to the real teardown. + if (!shutdownComplete && anyUnsavedWindowSession()) { + event.preventDefault(); + quitRequested = true; + for (const win of BrowserWindow.getAllWindows()) { + if (!win.isDestroyed()) win.close(); + } + return; + } performShutdown(); }); @@ -9768,5 +10254,8 @@ app.on('window-all-closed', () => { // worker pools alive so macOS reopen-from-dock works. The full // teardown happens in `before-quit`. currentSession().activeCrawler?.stop(); - if (process.platform !== 'darwin') app.quit(); + // `quitRequested` covers macOS, where closing the last window normally + // leaves the app resident — but here the user did ask to quit, and the + // windows only closed because they cleared the unsaved-changes prompt. + if (process.platform !== 'darwin' || quitRequested) app.quit(); }); diff --git a/apps/desktop/src/main/logger.ts b/apps/desktop/src/main/logger.ts index 547c6b8..a2f9734 100644 Binary files a/apps/desktop/src/main/logger.ts and b/apps/desktop/src/main/logger.ts differ diff --git a/apps/desktop/src/main/mcp-bridge.ts b/apps/desktop/src/main/mcp-bridge.ts index 970c3da..f2ebadb 100644 --- a/apps/desktop/src/main/mcp-bridge.ts +++ b/apps/desktop/src/main/mcp-bridge.ts @@ -394,6 +394,23 @@ async function handleRequest( token: string, deps: McpBridgeDeps, ): Promise { + // 0) Host allow-list — DNS-rebinding defence. The server binds to + // 127.0.0.1, but a malicious page could point a hostname that resolves + // to 127.0.0.1 at this port; the browser would then send that hostname + // in the Host header. Reject anything whose host is not loopback so a + // rebinding attempt never reaches auth or routing. + const hostHdr = (req.headers['host'] ?? '').toLowerCase(); + const hostName = hostHdr.replace(/:\d+$/, ''); + if ( + hostName !== '127.0.0.1' && + hostName !== 'localhost' && + hostName !== '[::1]' && + hostName !== '::1' + ) { + send(res, 403, { error: 'forbidden-host' }); + return; + } + // 1) Method + path gate — only the small allow-listed routes. const url = req.url ?? ''; const method = req.method ?? 'GET'; diff --git a/apps/desktop/src/main/parser-pool.ts b/apps/desktop/src/main/parser-pool.ts index b7a756a..e7906ed 100644 --- a/apps/desktop/src/main/parser-pool.ts +++ b/apps/desktop/src/main/parser-pool.ts @@ -18,21 +18,42 @@ import * as logger from './logger.js'; * while leaving the main thread headroom for IPC and queue * scheduling. * - * Round-robin dispatch is sufficient: parse durations are roughly - * proportional to body size, and the crawler's URL mix makes it - * unlikely that long-tail pages all land on the same worker. A - * least-loaded scheduler would add complexity for marginal gain. + * Dispatch is least-busy with a round-robin tie-break. Blind round-robin + * was not sufficient in practice: it hands out work without looking at + * what a worker is already carrying, so with concurrency 20 against as + * few as 2 workers (a 4-core machine) several slow pages stack on the + * same thread and push each other past the parse timeout — and a worker + * that has already blown the timeout kept receiving more work. + * + * A timed-out worker is recycled rather than reused: the parse is a + * synchronous cheerio call inside the worker that cannot be cancelled, + * so the only way to stop it burning a core is to terminate the thread + * and spawn a replacement. */ const __dirname = path.dirname(fileURLToPath(import.meta.url)); const WORKER_PATH = path.join(__dirname, 'parser-worker.js'); const REQUEST_TIMEOUT_MS = 60_000; +/** Thrown when a parse exceeds REQUEST_TIMEOUT_MS. Distinguished from a + * crash/not-ready failure because the caller must NOT retry it inline on + * the main thread — a document heavy enough to wedge a worker will freeze + * the whole app there. */ +export class ParserTimeoutError extends Error { + constructor(ms: number) { + super(`parser-pool: timeout > ${ms}ms`); + this.name = 'ParserTimeoutError'; + } +} + type ParseHtmlArgs = Parameters; type ParseOpts = ParseHtmlArgs[2]; type ParseResult = ReturnType; interface PendingRequest { + /** Which worker is carrying this request — needed to release its load + * counter and to fail everything queued on a wedged thread. */ + worker: Worker; resolve: (v: ParseResult) => void; reject: (e: Error) => void; timer: ReturnType; @@ -48,6 +69,8 @@ interface ResponseMessage { class ParserPool { private workers: Worker[] = []; private pending = new Map(); + /** In-flight parse count per worker, for least-busy dispatch. */ + private inFlight = new Map(); private nextRequestId = 1; private nextWorkerIdx = 0; private terminated = false; @@ -114,14 +137,19 @@ class ParserPool { return Promise.reject(new Error('parser-pool: not ready')); } const requestId = this.nextRequestId++; - const worker = this.workers[this.nextWorkerIdx % this.workers.length]!; - this.nextWorkerIdx++; + const worker = this.pickWorker(); + this.inFlight.set(worker, (this.inFlight.get(worker) ?? 0) + 1); return new Promise((resolve, reject) => { const timer = setTimeout(() => { this.pending.delete(requestId); - reject(new Error(`parser-pool: timeout > ${REQUEST_TIMEOUT_MS}ms`)); + this.releaseWorker(worker); + reject(new ParserTimeoutError(REQUEST_TIMEOUT_MS)); + // Recycle AFTER rejecting this caller: the worker is stuck inside a + // synchronous parse that can't be interrupted, so leaving it in the + // pool means it keeps a core pinned and keeps being handed work. + this.recycleWedged(worker); }, REQUEST_TIMEOUT_MS); - this.pending.set(requestId, { resolve, reject, timer }); + this.pending.set(requestId, { worker, resolve, reject, timer }); worker.postMessage({ requestId, html, pageUrl, opts }); }); } @@ -133,6 +161,7 @@ class ParserPool { p.reject(new Error('parser-pool: terminated')); } this.pending.clear(); + this.inFlight.clear(); const workers = this.workers; this.workers = []; await Promise.allSettled(workers.map((w) => w.terminate())); @@ -140,6 +169,56 @@ class ParserPool { // ── private ────────────────────────────────────────────────────── + /** Least in-flight requests wins; ties rotate so equally-idle workers + * don't all receive the next batch on the same thread. */ + private pickWorker(): Worker { + const n = this.workers.length; + let best = this.workers[this.nextWorkerIdx % n]!; + let bestLoad = Number.POSITIVE_INFINITY; + for (let i = 0; i < n; i++) { + const w = this.workers[(this.nextWorkerIdx + i) % n]!; + const load = this.inFlight.get(w) ?? 0; + if (load < bestLoad) { + best = w; + bestLoad = load; + } + } + this.nextWorkerIdx = (this.nextWorkerIdx + 1) % n; + return best; + } + + private releaseWorker(w: Worker): void { + const cur = this.inFlight.get(w) ?? 0; + if (cur <= 1) this.inFlight.delete(w); + else this.inFlight.set(w, cur - 1); + } + + /** Drop a worker that blew the parse timeout and put a fresh one in its + * place. Anything else queued behind it is failed rather than left to + * hang, since the thread is about to be terminated mid-parse. */ + private recycleWedged(w: Worker): void { + const slot = this.workers.indexOf(w); + if (slot < 0) return; // already recycled or removed + // Remove it first so its own 'exit' handler treats it as replaced and + // doesn't respawn a second time. + this.workers.splice(slot, 1); + this.inFlight.delete(w); + for (const [id, p] of this.pending) { + if (p.worker !== w) continue; + clearTimeout(p.timer); + this.pending.delete(id); + p.reject(new ParserTimeoutError(REQUEST_TIMEOUT_MS)); + } + logger.log( + 'warn', + 'main', + `parser-worker exceeded ${REQUEST_TIMEOUT_MS}ms on a single document — terminating and respawning it.`, + ); + void w.terminate().catch(() => undefined); + const fresh = this.tryRespawn(); + if (fresh) this.workers.push(fresh); + } + private spawnWorker(): Worker { const w = new Worker(WORKER_PATH); w.on('message', (msg: ResponseMessage) => this.handleResponse(msg)); @@ -154,6 +233,15 @@ class ParserPool { // would point at the wrong slot (or past the end). const slot = this.workers.indexOf(w); if (slot < 0) return; // already replaced / removed + this.inFlight.delete(w); + // Fail this worker's in-flight parses now instead of letting each one + // sit out the full 60 s timeout for an answer that can never arrive. + for (const [id, p] of this.pending) { + if (p.worker !== w) continue; + clearTimeout(p.timer); + this.pending.delete(id); + p.reject(new Error(`parser-worker exited (code=${code})`)); + } if (code === 0) { // Clean exit — drop the slot so `parse()` never dispatches to it. this.workers.splice(slot, 1); @@ -196,6 +284,7 @@ class ParserPool { if (!pending) return; this.pending.delete(msg.requestId); clearTimeout(pending.timer); + this.releaseWorker(pending.worker); if (msg.ok && msg.result !== undefined) { pending.resolve(msg.result); } else { diff --git a/apps/desktop/src/renderer/src/LogAnalyzerView.tsx b/apps/desktop/src/renderer/src/LogAnalyzerView.tsx index 7b7b0f9..b1494fa 100644 --- a/apps/desktop/src/renderer/src/LogAnalyzerView.tsx +++ b/apps/desktop/src/renderer/src/LogAnalyzerView.tsx @@ -127,20 +127,44 @@ export function LogAnalyzerView() { if (!window.confirm(t('logAnalyzer.clearConfirm', { defaultValue: 'Clear all ingested log data from this project?' }))) { return; } - await window.freecrawl.logClear(); + try { + await window.freecrawl.logClear(); + } catch (e) { + // Destructive and previously silent: on failure the refresh below + // still ran, so the view could look cleared while the project kept + // a partially-deleted log set. + showToast( + t('logAnalyzer.clearFailed', { + defaultValue: 'Could not clear log data: {{detail}}', + detail: e instanceof Error ? e.message : String(e), + }), + ); + return; + } refreshOverview(); setVersion((v) => v + 1); - }, [refreshOverview, t]); + }, [refreshOverview, showToast, t]); const onExport = useCallback( async (format: 'csv' | 'xlsx') => { - const r = await window.freecrawl.logExport({ format }); - if (r.filePath) { + try { + const r = await window.freecrawl.logExport({ format }); + if (r.filePath) { + showToast( + t('logAnalyzer.exported', { + defaultValue: 'Exported {{size}} KB → {{path}}', + size: Math.max(1, Math.round(r.bytesWritten / 1024)), + path: r.filePath, + }), + ); + } + } catch (e) { + // Without this the export just produced no toast and no file — + // indistinguishable from never having clicked the button. showToast( - t('logAnalyzer.exported', { - defaultValue: 'Exported {{size}} KB → {{path}}', - size: Math.max(1, Math.round(r.bytesWritten / 1024)), - path: r.filePath, + t('logAnalyzer.exportFailed', { + defaultValue: 'Export failed: {{detail}}', + detail: e instanceof Error ? e.message : String(e), }), ); } diff --git a/apps/desktop/src/renderer/src/components/BottomDetailPanel.tsx b/apps/desktop/src/renderer/src/components/BottomDetailPanel.tsx index c37dcb1..1db9450 100644 --- a/apps/desktop/src/renderer/src/components/BottomDetailPanel.tsx +++ b/apps/desktop/src/renderer/src/components/BottomDetailPanel.tsx @@ -599,16 +599,27 @@ function SpellingView({ const { t } = useTranslation(); const [data, setData] = useState(null); const [loaded, setLoaded] = useState(false); + const [loadError, setLoadError] = useState(null); const dataVersion = useAppStore((s) => s.dataVersion); useEffect(() => { let cancelled = false; setLoaded(false); - void window.freecrawl.spellingMatches(pageUrl).then((res) => { - if (cancelled) return; - setData(res); - setLoaded(true); - }); + setLoadError(null); + void window.freecrawl + .spellingMatches(pageUrl) + .then((res) => { + if (cancelled) return; + setData(res); + setLoaded(true); + }) + .catch((e: unknown) => { + if (cancelled) return; + // `setLoaded(true)` lived only in `then`, so a rejection left this + // panel on "Loading…" for as long as the URL stayed selected. + setLoadError(e instanceof Error ? e.message : String(e)); + setLoaded(true); + }); return () => { cancelled = true; }; @@ -621,6 +632,18 @@ function SpellingView({ ); } + if (loadError) { + // Distinct from "not checked yet" below, which is a statement about + // the page rather than about the lookup having failed. + return ( +
+
+ {t('common.loadFailedTitle', { defaultValue: "Couldn't load this view" })} +
+
{loadError}
+
+ ); + } if (!data || data.status === null) { return (
@@ -813,16 +836,27 @@ function AnalyticsView({ const { t } = useTranslation(); const [data, setData] = useState(null); const [loaded, setLoaded] = useState(false); + const [loadError, setLoadError] = useState(null); const dataVersion = useAppStore((s) => s.dataVersion); useEffect(() => { let cancelled = false; setLoaded(false); - void window.freecrawl.urlAnalyticsGet(pageUrl).then((res) => { - if (cancelled) return; - setData(res); - setLoaded(true); - }); + setLoadError(null); + void window.freecrawl + .urlAnalyticsGet(pageUrl) + .then((res) => { + if (cancelled) return; + setData(res); + setLoaded(true); + }) + .catch((e: unknown) => { + if (cancelled) return; + // This handler also resolves a Google account in the main process, + // which can reject. Without a catch the panel spun forever. + setLoadError(e instanceof Error ? e.message : String(e)); + setLoaded(true); + }); return () => { cancelled = true; }; @@ -835,6 +869,16 @@ function AnalyticsView({
); } + if (loadError) { + return ( +
+
+ {t('common.loadFailedTitle', { defaultValue: "Couldn't load this view" })} +
+
{loadError}
+
+ ); + } if (!data || (!data.gsc && !data.ga4 && !data.inspection)) { return ( @@ -3410,6 +3454,27 @@ interface JsonLdBlock { * the page declares — supplements the per-URL `schema_types` summary with * the underlying JSON the parser saw. */ +/** Collect the `@type` values a single parsed JSON-LD block declares, + * walking `@graph` and arrays. Used for the per-block "Types" export column, + * which previously repeated the page-level type list on every row. */ +function blockSchemaTypes(parsed: unknown): string[] { + const out = new Set(); + const visit = (node: unknown): void => { + if (Array.isArray(node)) { + for (const n of node) visit(n); + return; + } + if (!node || typeof node !== 'object') return; + const obj = node as Record; + const t = obj['@type']; + if (typeof t === 'string') out.add(t); + else if (Array.isArray(t)) for (const v of t) if (typeof v === 'string') out.add(v); + if (Array.isArray(obj['@graph'])) visit(obj['@graph']); + }; + visit(parsed); + return [...out]; +} + function extractJsonLdBlocks(html: string): JsonLdBlock[] { const blocks: JsonLdBlock[] = []; const re = @@ -3512,7 +3577,9 @@ function StructuredDataView({ rows: blocks.map((b) => [ b.index + 1, b.ok, - types.join(', '), + // This block's own @type(s), not the page-level list — the + // old code repeated the whole page's types on every row. + blockSchemaTypes(b.parsed).join(', '), b.raw, ]), })} diff --git a/apps/desktop/src/renderer/src/components/ExportDialog.tsx b/apps/desktop/src/renderer/src/components/ExportDialog.tsx index b130076..8a349ba 100644 --- a/apps/desktop/src/renderer/src/components/ExportDialog.tsx +++ b/apps/desktop/src/renderer/src/components/ExportDialog.tsx @@ -2,9 +2,25 @@ import { useEffect, useMemo, useState } from 'react'; import { X, Loader2, ChevronRight, ChevronDown } from 'lucide-react'; import clsx from 'clsx'; import { useTranslation } from 'react-i18next'; -import type { CrawlUrlRow, ExportTabularSection, UrlCategory } from '@freecrawl/shared-types'; +import type { + AdvancedFilter, + CrawlUrlRow, + ExportTabularSection, + UrlCategory, +} from '@freecrawl/shared-types'; import { TAB_ORDER, type TabKey } from '../store.js'; -import { COLUMN_SPECS, columnId } from '../tabs/columns.js'; +import { COLUMN_SPECS, columnId, type ColumnSpec } from '../tabs/columns.js'; + +/** The CrawlUrlRow field an export column writes. Most columns use + * `spec.key`, but the two indexability columns share key `'indexability'` + * while displaying different things — "Indexability" is the raw eligibility, + * "Indexability Status" is the specific reason (`indexabilityReason`). Keying + * the export by `spec.key` alone collapsed them into one duplicate column and + * dropped the status entirely; give the status column its own accessor. */ +function exportKeyFor(spec: ColumnSpec): string { + if (columnId(spec) === 'indexability-status') return 'indexabilityReason'; + return spec.key as string; +} import { translateLabel } from '../i18n/labels.js'; type Format = 'csv' | 'xlsx' | 'json' | 'xml'; @@ -158,6 +174,10 @@ interface ExportDialogProps { defaultTab: TabKey; /** Row ids selected in the current table; falsy → export the whole filter set. */ selectedIds?: number[]; + /** Active table search term — forwarded so the export matches the grid. */ + search?: string; + /** Active advanced filter — forwarded so the export matches the grid. */ + filter?: AdvancedFilter; } export function ExportDialog({ @@ -165,6 +185,8 @@ export function ExportDialog({ onClose, defaultTab, selectedIds, + search, + filter, }: ExportDialogProps) { const { t, i18n } = useTranslation(); const lang = i18n.language; @@ -200,10 +222,12 @@ export function ExportDialog({ return new Set(node && node.children.length > 0 ? [node.key] : []); }); + // Keyed by column ID (columnId), not spec.key — two columns can share a + // key (indexability / indexability-status) and must toggle independently. const [pickedColumns, setPickedColumns] = useState>(() => { const node = tree.find((n) => n.tabKey === initialTabKey); const specs = node ? COLUMN_SPECS[node.tabKey] : []; - return new Set(specs.map((s) => s.key as string)); + return new Set(specs.map((s) => columnId(s))); }); const [useSelection, setUseSelection] = useState(selectionCount > 0); const [busy, setBusy] = useState(false); @@ -242,7 +266,7 @@ export function ExportDialog({ const offeredColumns = useMemo(() => { const seen = new Set(); - const out: { key: string; header: string; tab: TabKey }[] = []; + const out: { id: string; key: string; header: string; tab: TabKey }[] = []; for (const node of tree) { if (!pickedTabKeys.has(node.tabKey)) continue; const specs = COLUMN_SPECS[node.tabKey]; @@ -250,14 +274,14 @@ export function ExportDialog({ const id = columnId(spec); if (seen.has(id)) continue; seen.add(id); - out.push({ key: spec.key as string, header: spec.header, tab: node.tabKey }); + out.push({ id, key: exportKeyFor(spec), header: spec.header, tab: node.tabKey }); } } return out; }, [pickedTabKeys, tree]); const orderedColumnList = useMemo( - () => offeredColumns.filter((c) => pickedColumns.has(c.key)).map((c) => c.key), + () => offeredColumns.filter((c) => pickedColumns.has(c.id)).map((c) => c.key), [offeredColumns, pickedColumns], ); @@ -302,10 +326,10 @@ export function ExportDialog({ if (state !== 'all') { setPickedColumns((prev) => { const specs = COLUMN_SPECS[node.tabKey] ?? []; - const hasAny = specs.some((spec) => prev.has(spec.key as string)); + const hasAny = specs.some((spec) => prev.has(columnId(spec))); if (hasAny) return prev; const next = new Set(prev); - for (const spec of specs) next.add(spec.key as string); + for (const spec of specs) next.add(columnId(spec)); return next; }); } @@ -322,10 +346,10 @@ export function ExportDialog({ // Bug #12 — same conditional auto-add as toggleParent. setPickedColumns((prev) => { const specs = COLUMN_SPECS[node.tabKey] ?? []; - const hasAny = specs.some((spec) => prev.has(spec.key as string)); + const hasAny = specs.some((spec) => prev.has(columnId(spec))); if (hasAny) return prev; const next = new Set(prev); - for (const spec of specs) next.add(spec.key as string); + for (const spec of specs) next.add(columnId(spec)); return next; }); }; @@ -367,7 +391,7 @@ export function ExportDialog({ }; const checkAllColumns = () => { - setPickedColumns(new Set(offeredColumns.map((c) => c.key))); + setPickedColumns(new Set(offeredColumns.map((c) => c.id))); }; const uncheckAllColumns = () => { @@ -443,15 +467,17 @@ export function ExportDialog({ } } } + const usingSelection = useSelection && !!selectedIds && selectedIds.length > 0; const result = await window.freecrawl.exportTabular({ format, sections, columns: orderedColumnList, csvBom: format === 'csv' ? csvBom : undefined, - selectedIds: - useSelection && selectedIds && selectedIds.length > 0 - ? selectedIds - : undefined, + selectedIds: usingSelection ? selectedIds : undefined, + // Mirror the on-screen grid — but only when NOT exporting an explicit + // selection (a selection already pins exact rows). + search: usingSelection ? undefined : search || undefined, + filter: usingSelection ? undefined : filter, }); if (!result.filePath) { setBusy(false); @@ -648,10 +674,10 @@ export function ExportDialog({ ) : ( offeredColumns.map((c) => { - const checked = pickedColumns.has(c.key); + const checked = pickedColumns.has(c.id); return (
+ {/* Rows are on screen, so the list itself isn't misleading — but + the counts are stale, and saying so beats silence. */} + {loadError && rows.length > 0 && ( + + {t('common.loadFailedStale', { + defaultValue: + 'Showing the last successful result — the latest refresh failed.', + })} + + )} {t('serpTab.summary', { defaultValue: '{{shown}} of {{total}} indexable pages', shown: rows.length.toLocaleString(), @@ -121,7 +144,23 @@ export function SerpTab() { ref={scrollRef} className="flex-1 overflow-y-auto bg-surface-950 px-6 py-4" > - {rows.length === 0 && ( + {/* A failed query must not be reported as "this crawl has no + indexable pages" — that is a claim about the site. */} + {loadError && rows.length === 0 && ( +
+
+ {t('common.loadFailedTitle', { defaultValue: "Couldn't load this view" })} +
+
+ {t('common.loadFailedBody', { + defaultValue: + 'The query failed, so results are missing rather than empty. Retrying automatically.', + })} +
+
{loadError}
+
+ )} + {!loadError && rows.length === 0 && (
{total === 0 ? t('serpTab.emptyNoData', { defaultValue: 'No indexable HTML pages with a title yet — run a crawl to populate SERP previews.' }) diff --git a/apps/desktop/src/renderer/src/tabs/UrlsTab.tsx b/apps/desktop/src/renderer/src/tabs/UrlsTab.tsx index b44ce2d..3267186 100644 --- a/apps/desktop/src/renderer/src/tabs/UrlsTab.tsx +++ b/apps/desktop/src/renderer/src/tabs/UrlsTab.tsx @@ -1685,6 +1685,8 @@ export function UrlsTab() { onClose={() => setExportDialogOpen(false)} defaultTab={activeTab} selectedIds={selectedIds.size > 0 ? [...selectedIds] : undefined} + search={search || undefined} + filter={filter ?? undefined} /> )} diff --git a/apps/desktop/src/renderer/src/utils/clearCrawl.ts b/apps/desktop/src/renderer/src/utils/clearCrawl.ts index 468c4c6..e3eabce 100644 --- a/apps/desktop/src/renderer/src/utils/clearCrawl.ts +++ b/apps/desktop/src/renderer/src/utils/clearCrawl.ts @@ -1,3 +1,6 @@ +import i18n from '../i18n/index.js'; +import { useAppStore } from '../store.js'; + const SKIP_KEY = 'clear:skip-confirm'; /** @@ -22,6 +25,25 @@ export async function clearCrawlWithConfirm(): Promise { } catch { /* ignore */ } - await window.freecrawl.crawlClear(); + try { + await window.freecrawl.crawlClear(); + } catch (e) { + // Handled here rather than at each call site so every entry point — + // the toolbar button, File → Clear, New Project, New from Template — + // is covered. A destructive operation that fails silently is the + // worst case: `reset()` would still run and the UI would look wiped + // while the database kept a half-deleted crawl, indistinguishable + // from success. Returning false leaves the UI untouched. + const detail = e instanceof Error ? e.message : String(e); + useAppStore + .getState() + .setError( + i18n.t('app.clearFailed', { + defaultValue: 'Could not clear the crawl: {{detail}}', + detail, + }), + ); + return false; + } return true; } diff --git a/apps/mcp-server/src/index.ts b/apps/mcp-server/src/index.ts index 495198f..bbd4237 100644 --- a/apps/mcp-server/src/index.ts +++ b/apps/mcp-server/src/index.ts @@ -51,7 +51,14 @@ let session: Session | null = null; function openProject(projectPath: string): Session { if (!fs.existsSync(projectPath)) { - throw new Error(`Project file does not exist: ${projectPath}`); + throw new Error( + `Project file does not exist: ${projectPath}\n` + + 'If the desktop app is open, it may be running in RAM-only storage ' + + 'mode, which keeps the project in memory and never writes this file. ' + + 'Switch to Disk mode in Settings → Storage, use Save As and point at ' + + 'the saved file with set_project, or create an isolated headless ' + + 'session with session_create.', + ); } const db = new ProjectDb(projectPath, { readOnly: true }); return { projectPath, db }; diff --git a/packages/core/src/browser-login.ts b/packages/core/src/browser-login.ts index 31720c5..de8743f 100644 --- a/packages/core/src/browser-login.ts +++ b/packages/core/src/browser-login.ts @@ -30,6 +30,8 @@ export interface BrowserLoginInput { submitSelector: string; successSelector?: string; waitMs?: number; + /** Accept invalid TLS certs on the login page — opt-in, default false. */ + allowInsecureTls?: boolean; } export interface BrowserLoginOptions { @@ -108,7 +110,9 @@ export async function runBrowserLogin( extraHTTPHeaders: opts.acceptLanguage ? { 'Accept-Language': opts.acceptLanguage } : undefined, - ignoreHTTPSErrors: true, + // Verify TLS by default — the user types real credentials here, so an + // unverifiable cert must not be silently accepted. Opt-in per crawl. + ignoreHTTPSErrors: cfg.allowInsecureTls === true, }); const page = await context.newPage(); page.setDefaultTimeout(timeout); diff --git a/packages/core/src/browser-pool.ts b/packages/core/src/browser-pool.ts index 78224d6..bee5302 100644 --- a/packages/core/src/browser-pool.ts +++ b/packages/core/src/browser-pool.ts @@ -149,9 +149,18 @@ export interface BrowserPoolOptions { prerenderJs?: string; } +/** Cap on automatic Chromium relaunches after a `disconnected` event. + * Enough to ride out a one-off crash; low enough that a permanently + * broken install doesn't pay a launch attempt on every URL. */ +const MAX_RELAUNCHES = 3; + interface Slot { page: Page; busy: boolean; + /** Slot lost its page and could not get a replacement. It will never + * serve another render, so it must not be counted as capacity that a + * waiter can eventually be handed. */ + dead?: boolean; } interface Waiter { @@ -180,6 +189,8 @@ export class BrowserPool { private opts: BrowserPoolOptions; private starting: Promise | null = null; private closed = false; + /** How many times Chromium has been relaunched after dying on us. */ + private relaunches = 0; constructor(opts: BrowserPoolOptions) { this.opts = opts; @@ -228,6 +239,31 @@ export class BrowserPool { const chromium = await loadChromium(); this.browser = await chromium.launch(launchOpts); + // Chromium can die on its own — OOM-killed, crashed, or killed by the + // OS. Nothing polls for that, so without this the pool keeps handing + // out pages that belong to a process that is gone: every render fails, + // every replacement `newPage()` fails, and callers park in `acquire()` + // forever. Fail the slots once, loudly, and let the crawler record the + // renders as failed instead of stalling on them. + this.browser.on('disconnected', () => { + if (this.closed) return; + // Fail the parked callers FIRST, while the slots still exist — they + // are holding pages from a dead process and can never be served. + for (const slot of this.slots) { + slot.dead = true; + slot.busy = true; + } + this.failIfExhausted(new Error('browser disconnected')); + // Then clear the pool so the next `acquire()` relaunches instead of + // handing out corpses. Bounded, because a Chromium that dies on + // every launch would otherwise cost a relaunch attempt per URL for + // the rest of the crawl. + if (this.relaunches >= MAX_RELAUNCHES) return; + this.relaunches++; + this.browser = null; + this.context = null; + this.slots = []; + }); this.context = await this.browser.newContext({ viewport: this.opts.viewport, userAgent: this.opts.userAgent, @@ -283,12 +319,21 @@ export class BrowserPool { private takeSlot(): Promise { return new Promise((resolve, reject) => { - const free = this.slots.find((s) => !s.busy); + const free = this.slots.find((s) => !s.busy && !s.dead); if (free) { free.busy = true; resolve(free); return; } + // Nothing free — but if nothing is even *alive*, waiting is futile. + if (!this.slots.some((s) => !s.dead)) { + reject( + new Error( + 'BrowserPool: no usable pages left (the browser could not create a new page)', + ), + ); + return; + } this.waiters.push({ resolve: (s) => { s.busy = true; @@ -299,6 +344,22 @@ export class BrowserPool { }); } + /** Reject everyone parked in `acquire()` once no slot can ever serve + * them again. Without this a dead pool is indistinguishable from a + * busy one and the crawl hangs instead of failing its renders. */ + private failIfExhausted(cause: unknown): void { + if (this.slots.some((s) => !s.dead)) return; + const detail = cause instanceof Error ? cause.message : String(cause); + const waiters = this.waiters.splice(0); + for (const w of waiters) { + try { + w.reject(new Error(`BrowserPool: all pages lost (${detail})`)); + } catch { + /* swallow — waiter cleanup */ + } + } + } + private releaseSlot(slot: Slot, replace: boolean): void { if (replace && this.context && !this.closed) { // Drop the (possibly torn) page and create a fresh one in its @@ -314,14 +375,23 @@ export class BrowserPool { try { const fresh = await this.context.newPage(); slot.page = fresh; - } catch { - // Couldn't make a fresh page — mark the slot stale by - // leaving it busy. The pool degrades by one slot until - // close(). + } catch (err) { + // Couldn't make a fresh page — this slot is gone for good. + // Leaving it `busy` and returning here used to strand every + // queued waiter: nothing else ever resolves them, `takeSlot` + // has no timeout, so `acquire()` never settled, the crawler's + // task never finished and `queue.onIdle()` never resolved — + // the crawl sat at "Running" with 0 URL/s until the user hit + // Stop. When the whole pool dies this way (a crashed or + // disconnected Chromium fails every slot identically) the + // waiters must be failed, not forgotten. slot.busy = true; + slot.dead = true; + this.failIfExhausted(err); return; } slot.busy = false; + slot.dead = false; const next = this.waiters.shift(); if (next) next.resolve(slot); }); diff --git a/packages/core/src/bulk-export.ts b/packages/core/src/bulk-export.ts index c173029..8e01d0a 100644 --- a/packages/core/src/bulk-export.ts +++ b/packages/core/src/bulk-export.ts @@ -14,6 +14,7 @@ import type { ProjectDb } from '@freecrawl/db'; import type { BulkExportFile, BulkExportResult, + CrawlUrlRow, UrlCategory, } from '@freecrawl/shared-types'; import { exportUrlsToCsv } from './csv-export.js'; @@ -22,8 +23,25 @@ export interface BulkExportTask { label: string; file: string; category: UrlCategory; + /** Column override for topic files whose whole point is a field the + * generic column set omits (extraction results, hreflang, security + * headers). When set these are appended to the base columns. */ + columns?: (keyof CrawlUrlRow)[]; } +/** Base columns every bulk file carries (mirrors csv-export's CSV_COLUMNS). */ +const BULK_BASE_COLUMNS: (keyof CrawlUrlRow)[] = [ + 'url', + 'statusCode', + 'contentKind', + 'indexability', + 'indexabilityReason', + 'title', + 'inlinks', + 'outlinks', + 'depth', +]; + /** Every category/issue family the bulk dump emits, in display order. */ export const BULK_EXPORT_TASKS: BulkExportTask[] = [ { label: 'All URLs', file: 'all-urls.csv', category: 'all' }, @@ -62,7 +80,12 @@ export const BULK_EXPORT_TASKS: BulkExportTask[] = [ { label: 'All Pagination', file: 'all-pagination.csv', category: 'tab:pagination' }, { label: 'Pagination — Sequence Break', file: 'pagination-sequence-break.csv', category: 'issues:pagination-sequence-break' }, { label: 'Pagination — Canonical Conflict', file: 'pagination-canonical-conflict.csv', category: 'issues:pagination-canonical-conflict' }, - { label: 'Hreflang — All', file: 'hreflang-all.csv', category: 'tab:hreflang' }, + { + label: 'Hreflang — All', + file: 'hreflang-all.csv', + category: 'tab:hreflang', + columns: [...BULK_BASE_COLUMNS, 'hreflangs'], + }, { label: 'Hreflang — x-default Missing', file: 'hreflang-x-default-missing.csv', category: 'issues:hreflang-x-default-missing' }, { label: 'Hreflang — Invalid Language Code', file: 'hreflang-invalid-code.csv', category: 'issues:hreflang-invalid-code' }, { label: 'Hreflang — Self-Ref Missing', file: 'hreflang-self-ref-missing.csv', category: 'issues:hreflang-self-ref-missing' }, @@ -70,8 +93,18 @@ export const BULK_EXPORT_TASKS: BulkExportTask[] = [ { label: 'Hreflang — Inconsistent Language', file: 'hreflang-inconsistent-lang.csv', category: 'issues:hreflang-inconsistent-lang' }, { label: 'Duplicate Pages', file: 'duplicate-pages.csv', category: 'tab:duplicates' }, { label: 'Duplicate Content — Exact', file: 'duplicate-content-exact.csv', category: 'issues:duplicate-content-exact' }, - { label: 'Custom Extraction Results', file: 'custom-extraction.csv', category: 'tab:custom-extraction' }, - { label: 'Custom Search Hits', file: 'custom-search.csv', category: 'tab:custom-search' }, + { + label: 'Custom Extraction Results', + file: 'custom-extraction.csv', + category: 'tab:custom-extraction', + columns: [...BULK_BASE_COLUMNS, 'extractionResults'], + }, + { + label: 'Custom Search Hits', + file: 'custom-search.csv', + category: 'tab:custom-search', + columns: [...BULK_BASE_COLUMNS, 'customSearchHits'], + }, { label: 'Structured Data — Missing', file: 'structured-data-missing.csv', category: 'issues:structured-data-missing' }, { label: 'Structured Data — Invalid', file: 'structured-data-invalid.csv', category: 'issues:structured-data-invalid' }, { label: 'Structured Data — Duplicate @id', file: 'structured-data-duplicate-id.csv', category: 'issues:schema-duplicate-id' }, @@ -81,7 +114,18 @@ export const BULK_EXPORT_TASKS: BulkExportTask[] = [ { label: 'Image — Empty Alt', file: 'image-empty-alt.csv', category: 'issues:image-empty-alt' }, { label: 'Image — Too Large', file: 'image-too-large.csv', category: 'issues:image-too-large' }, { label: 'Image — Broken Src', file: 'image-broken-src.csv', category: 'issues:image-broken-src' }, - { label: 'Security Audit', file: 'security-audit.csv', category: 'tab:security' }, + { + label: 'Security Audit', + file: 'security-audit.csv', + category: 'tab:security', + columns: [ + ...BULK_BASE_COLUMNS, + 'hsts', + 'csp', + 'xFrameOptions', + 'xContentTypeOptions', + ], + }, { label: 'Mixed Content — Active', file: 'mixed-content-active.csv', category: 'issues:mixed-content-active' }, { label: 'Mixed Content — Passive', file: 'mixed-content-passive.csv', category: 'issues:mixed-content-passive' }, { label: 'HSTS Missing', file: 'security-hsts-missing.csv', category: 'issues:hsts-missing' }, @@ -107,6 +151,7 @@ export async function runBulkExport( try { const { rowsWritten } = await exportUrlsToCsv(db, filePath, { category: task.category, + columns: task.columns, }); if (rowsWritten === 0) { await unlink(filePath).catch(() => undefined); diff --git a/packages/core/src/compare.ts b/packages/core/src/compare.ts index caa48a8..550ea86 100644 --- a/packages/core/src/compare.ts +++ b/packages/core/src/compare.ts @@ -69,15 +69,20 @@ interface MinimalRow { response_time_ms: number | null; } -function loadAll(db: ProjectDb): MinimalRow[] { +function loadAll(db: ProjectDb): Iterable { // Pull only the fields the diff needs — keeps memory bounded at 1M // URLs (~120 MB for the seven columns vs ~600 MB for the full table). // Reaches into the underlying DatabaseSync via ProjectDb.exec for the // raw SELECT — no public iterator covers exactly this projection and // the diff is a one-shot operation, so we keep the helper here. + // + // Streams rather than returning an array: the caller only ever funnels + // these into a Map, and `.all()` meant the full row array and the Map + // built from it were both live at peak — two copies per side, four in + // total, all on the main thread. // eslint-disable-next-line @typescript-eslint/no-explicit-any const rawDb = (db as any).db as { - prepare: (sql: string) => { all: () => unknown }; + prepare: (sql: string) => { iterate: () => Iterable }; }; return rawDb .prepare( @@ -86,7 +91,7 @@ function loadAll(db: ProjectDb): MinimalRow[] { FROM urls WHERE is_external = 0 AND content_kind = 'html'`, ) - .all() as MinimalRow[]; + .iterate() as Iterable; } const EMPTY_COUNTS: Record = { @@ -109,13 +114,11 @@ export function compareCrawls( const limit = options.perCategoryLimit ?? 5000; const rtThreshold = options.responseTimeThresholdMs ?? 500; - const rowsA = loadAll(a); - const rowsB = loadAll(b); - + // Consume each side straight into its Map — no intermediate array. const mapA = new Map(); - for (const r of rowsA) mapA.set(r.url, r); + for (const r of loadAll(a)) mapA.set(r.url, r); const mapB = new Map(); - for (const r of rowsB) mapB.set(r.url, r); + for (const r of loadAll(b)) mapB.set(r.url, r); const counts: Record = { ...EMPTY_COUNTS }; const samples: CompareDiffRow[] = []; @@ -225,8 +228,10 @@ export function compareCrawls( } return { - totalA: rowsA.length, - totalB: rowsB.length, + // `urls.url` is `NOT NULL UNIQUE`, so one Map entry per row — these + // are the same totals the intermediate arrays used to report. + totalA: mapA.size, + totalB: mapB.size, counts, samples, }; diff --git a/packages/core/src/crawler.ts b/packages/core/src/crawler.ts index 1b180d9..c5f13e7 100644 --- a/packages/core/src/crawler.ts +++ b/packages/core/src/crawler.ts @@ -18,6 +18,7 @@ import { isSameHost, extractExtension, isInScope, + isPrivateHost, isUrlMalformed, detectUrlTrap, resolveStartUrl, @@ -27,7 +28,7 @@ import { } from './url-utils.js'; import { parseHtml, estimatePixelWidth, type ParsedPage } from './html-parser.js'; import { analyseCookies, extractSetCookies } from './cookies.js'; -import { loadRobots, type RobotsChecker } from './robots.js'; +import { loadRobots, robotsUserAgentToken, type RobotsChecker } from './robots.js'; import { BROWSER_FALLBACK_UA, buildDigestAuthHeader, @@ -40,6 +41,7 @@ import { initHttpClient, MOBILE_USER_AGENT, parseDigestChallenge, + redactUrlSecrets, } from './http-client.js'; import { setActiveDnsHook } from './dns-resolver.js'; import { discoverSitemapUrls, fetchSitemaps, type SitemapEntry } from './sitemap.js'; @@ -97,6 +99,19 @@ interface QueueItem { } const EXT_TO_KIND: Record = { + // HTML-ish page extensions. Reached only when the response carried no + // usable Content-Type (WAF/CDN block pages, some misconfigured origins); + // without these entries a `/urun.php` or `/page.html` with a missing + // Content-Type fell to `'other'` and vanished from every + // content_kind='html' issue filter — the page silently disappeared from + // the audit. + html: 'html', + htm: 'html', + xhtml: 'html', + php: 'html', + asp: 'html', + aspx: 'html', + jsp: 'html', css: 'css', js: 'js', mjs: 'js', @@ -119,10 +134,90 @@ const EXT_TO_KIND: Record = { // HEAD 403 / GET 200). Retry these — plus any 5xx or transport failure — // with GET before recording, so the status matches what a visitor sees. // Module-scope so the external-probe hot path doesn't rebuild it per URL. +/** Memory-watchdog samples (2 s apart) to wait for RSS to fall back below + * the resume threshold before concluding it never will — 5 minutes. */ +const MEMORY_STUCK_SAMPLES = 150; + const HEAD_UNRELIABLE_STATUSES = new Set([ 400, 401, 403, 405, 406, 409, 429, 501, ]); +// Known X-Robots-Tag directive keywords. Anything else appearing before a +// colon is a bot name (`googlebot: noindex`), not a directive — but a real +// directive can ALSO carry a colon (`unavailable_after: `, +// `max-snippet: -1`), so "has a colon → bot-scoped" is wrong. Membership +// here is how we tell the two apart. +const X_ROBOTS_DIRECTIVES = new Set([ + 'all', + 'noindex', + 'index', + 'nofollow', + 'follow', + 'none', + 'noarchive', + 'nosnippet', + 'noimageindex', + 'notranslate', + 'unavailable_after', + 'max-snippet', + 'max-image-preview', + 'max-video-preview', + 'indexifembedded', +]); + +/** + * Which of `noindex` / `nofollow` an `X-Robots-Tag` header applies to US. + * + * The header may scope directives to a specific bot + * (`X-Robots-Tag: bingbot: noindex`), which only that bot honours. The old + * check was `header.includes('noindex')`, so a `bingbot: noindex` marked a + * page non-indexable that Google indexes fine. Honour a directive only when + * it is un-scoped (applies to every bot) or scoped to a bot we model: the + * crawl's own UA token, or Googlebot (the indexer the Indexability column + * fundamentally represents — consistent with the meta-robots handling). + * + * `none` is the shorthand for `noindex, nofollow`. + */ +export function xRobotsTagDirectives( + header: string, + uaToken: string, +): { noindex: boolean; nofollow: boolean } { + const ua = uaToken.toLowerCase(); + let noindex = false; + let nofollow = false; + const apply = (directives: string): void => { + if (/\bnone\b/.test(directives)) { + noindex = true; + nofollow = true; + return; + } + if (/\bnoindex\b/.test(directives)) noindex = true; + if (/\bnofollow\b/.test(directives)) nofollow = true; + }; + for (const rawSegment of header.split(',')) { + const segment = rawSegment.trim().toLowerCase(); + if (!segment) continue; + const colon = segment.indexOf(':'); + if (colon > 0) { + const head = segment.slice(0, colon).trim(); + // `head` is a bot name only if it isn't itself a directive keyword + // (which would mean this is a valued directive like `max-snippet: 5`). + if (!X_ROBOTS_DIRECTIVES.has(head)) { + if (head !== 'googlebot' && head !== ua) continue; // other bot + apply(segment.slice(colon + 1)); + continue; + } + } + apply(segment); // un-scoped + } + return { noindex, nofollow }; +} + +/** Back-compat convenience wrapper. */ +export function xRobotsTagNoindex(header: string, uaToken: string): boolean { + return xRobotsTagDirectives(header, uaToken).noindex; +} + const CERT_VERIFY_ERROR_RE = /UNABLE_TO_VERIFY_LEAF_SIGNATURE|UNABLE_TO_GET_ISSUER_CERT_LOCALLY|SELF_SIGNED_CERT_IN_CHAIN|DEPTH_ZERO_SELF_SIGNED_CERT|CERT_UNTRUSTED/; function isCertVerifyError(err: unknown): boolean { @@ -174,6 +269,17 @@ export class Crawler extends EventEmitter { * sequence, replayed on every request. Null when form login is off. */ private sessionJar: SessionCookieJar | null = null; private robots: RobotsChecker | null = null; + /** + * robots.txt checker per ORIGIN. A single start-origin checker was wrong + * for cross-protocol (http vs https of the same host) and subdomain / + * all-subdomains crawls: robots-parser returns `undefined` for a URL whose + * origin doesn't match the file it parsed, which `?? true` turned into + * "allowed", so those URLs bypassed robots entirely. Each origin now gets + * its own checker, loaded lazily and cached. + */ + private robotsByOrigin = new Map(); + /** Origins whose robots.txt fetch is in flight (load-once guard). */ + private robotsLoadingOrigins = new Set(); /** robots.txt `Crawl-delay` for the start origin, in ms (0 = none). */ private robotsCrawlDelayMs = 0; /** Monotonic dispatch clock for the maxRps gate (see acquireRateSlot). */ @@ -840,9 +946,14 @@ export class Crawler extends EventEmitter { // loaded. Both promises are awaited at end-of-crawl so post-crawl // recompute and sitemap-derived issue counts use the full data set. const robotsPromise = this.config.respectRobotsTxt - ? loadRobots(origin, this.config.userAgent).then((r) => { + ? loadRobots(origin, this.config.userAgent, (msg) => { + if (!this.stopped) this.emit('warn', msg); + }).then((r) => { if (!this.stopped) { this.robots = r; + // Seed the per-origin cache so the start origin uses the same + // checker (and its Crawl-delay) as every other origin. + this.robotsByOrigin.set(origin, r); // robots.txt `Crawl-delay` (seconds). Cached once — the start // origin's directive applies for the crawl, matching the single // robots checker used for allow/deny. @@ -898,6 +1009,15 @@ export class Crawler extends EventEmitter { // behaviour. if (this.config.memoryLimitMb > 0) { let autoPaused = false; + // Samples spent waiting for RSS to fall back under `resumeAt`. + // Pausing stops new fetches but releases nothing already retained — + // the SQLite page cache, the worker isolates and `seen` dominate RSS + // and do not shrink. So the resume condition can simply never be met, + // and because a paused PQueue keeps `size > 0`, `queue.onIdle()` in + // `start()` never resolves: the crawl sat at "Running" forever with + // no way out but Stop. Give the spike a fair window, then finish the + // crawl with what we have instead of hanging on it. + let stuckSamples = 0; const limit = this.config.memoryLimitMb; const resumeAt = Math.floor(limit * 0.8); this.memoryWatchdogTimer = setInterval(() => { @@ -910,6 +1030,7 @@ export class Crawler extends EventEmitter { `Will resume automatically once RSS drops below ${resumeAt} MB.`, ); autoPaused = true; + stuckSamples = 0; this.pause(); } else if (autoPaused && this.paused && rssMb < resumeAt) { this.emit( @@ -917,7 +1038,26 @@ export class Crawler extends EventEmitter { `Memory dropped to ${rssMb} MB (< ${resumeAt} MB) — resuming crawler.`, ); autoPaused = false; + stuckSamples = 0; this.resume(); + } else if (autoPaused && this.paused) { + stuckSamples++; + if (stuckSamples >= MEMORY_STUCK_SAMPLES) { + this.emit( + 'error', + `Memory stayed at ${rssMb} MB (≥ ${resumeAt} MB) for ` + + `${Math.round((MEMORY_STUCK_SAMPLES * 2000) / 60000)} minutes after the ` + + 'memory-limit pause. Most of it is memory the crawler cannot release ' + + 'while running, so waiting longer will not help — finishing the crawl ' + + 'with the URLs collected so far. Raise or clear the memory limit to ' + + 'crawl further.', + ); + // Resume so the queue can drain to the terminal state, then + // stop: leaving it paused would keep `onIdle()` unresolved. + autoPaused = false; + this.resume(); + this.stop(); + } } }, 2000); } @@ -1202,7 +1342,14 @@ export class Crawler extends EventEmitter { void import('./webhook.js').then(({ postCrawlCompleteWebhook }) => postCrawlCompleteWebhook(url, payload).then((res) => { if (res.ok) { - this.emit('info', `Webhook posted to ${url} (${res.status} in ${res.durationMs} ms)`); + // Origin only. A Slack / Discord / Zapier incoming-webhook URL is + // bearer-equivalent — anyone holding it can post as the user — and + // this line lands in the on-disk log, the very file users are told + // to send to support. + this.emit( + 'info', + `Webhook posted to ${redactUrlSecrets(url)} (${res.status} in ${res.durationMs} ms)`, + ); } else { this.emit( 'info', @@ -1273,7 +1420,6 @@ export class Crawler extends EventEmitter { let probed = 0; let large = 0; const threshold = Math.max(1, this.config.largeImageBytes); - const userAgent = this.config.userAgent; const worker = async (): Promise => { while (!this.stopped) { @@ -1281,6 +1427,15 @@ export class Crawler extends EventEmitter { if (idx >= unprobed.length) return; const entry = unprobed[idx]; if (!entry) return; + // SSRF guard — same as probeExternal: an image `src` can point at an + // internal address just as a link can. + if (isPrivateHost(entry.src) && !isPrivateHost(this.config.startUrl)) { + this.db.setImageSize(entry.id, null, 0); + continue; + } + // Only send the crawl's auth/custom headers to in-scope image hosts; + // third-party / CDN image URLs must not receive the start site's creds. + const imgInScope = isInScope(this.config.startUrl, entry.src, this.config.scope); const controller = new AbortController(); const timeout = setTimeout( () => controller.abort(), @@ -1292,8 +1447,8 @@ export class Crawler extends EventEmitter { headers: defaultRequestHeaders( this.resolveUserAgent(entry.src), this.config.acceptLanguage, - this.config.customHeaders, - this.config.auth, + imgInScope ? this.config.customHeaders : {}, + imgInScope ? this.config.auth : undefined, ), redirect: 'follow', signal: controller.signal, @@ -1765,6 +1920,7 @@ export class Crawler extends EventEmitter { timeoutMs: this.config.requestTimeoutMs, maxUrls: sitemapMaxUrls, maxDepth: 3, + rewrites: this.urlRewrites, }); entries = result.entries; this.emit( @@ -1970,6 +2126,7 @@ export class Crawler extends EventEmitter { timeoutMs: this.config.requestTimeoutMs, maxUrls: sitemapMaxUrls, maxDepth: 3, + rewrites: this.urlRewrites, }; // (1) Auto-discovered sitemaps — recorded only. @@ -2210,6 +2367,32 @@ export class Crawler extends EventEmitter { private async probeExternal(url: string): Promise { if (this.stopped) return; + + // SSRF guard: never fetch an internal/loopback/link-local address on + // behalf of a crawled page (e.g. `http://169.254.169.254/…` cloud + // metadata, `http://192.168.x/admin`). Skip unless the START host is + // itself private — an intranet crawl legitimately targets private space. + if (isPrivateHost(url) && !isPrivateHost(this.config.startUrl)) { + await this.dbCall('updateExternalProbe', [ + url, + { + statusCode: null, + statusText: 'Skipped: private/internal address (SSRF guard)', + responseTimeMs: 0, + }, + ]); + return; + } + + // Only attach the crawl's Authorization / custom headers when the probe + // host is within the crawl's own scope. Sending the start site's Basic + // auth or a bypass header (`X-Vercel-Protection-Bypass`, …) to an + // arbitrary third-party host is a credential leak — the fetch spec only + // strips auth on cross-origin REDIRECTS, not on a direct fetch. + const inScope = isInScope(this.config.startUrl, url, this.config.scope); + const probeAuth = inScope ? this.config.auth : undefined; + const probeHeaders = inScope ? this.config.customHeaders : {}; + const t0 = Date.now(); const cfgUa = this.resolveUserAgent(url); @@ -2230,10 +2413,17 @@ export class Crawler extends EventEmitter { headers: defaultRequestHeaders( ua, this.config.acceptLanguage, - this.config.customHeaders, - this.config.auth, + probeHeaders, + probeAuth, ), - redirect: this.config.followRedirects ? 'follow' : 'manual', + // Always manual for external outlinks: an external link IS its + // own response, so we report the link's immediate status. Under + // 'follow' the recorded status was the FINAL hop's — so a working + // `/old` (301 → 404 `/new`) was reported as 404 against `/old`, + // and the user "fixed" the wrong link. Manual makes external + // links consistent with how internal redirects are recorded + // (30x + redirect target). + redirect: 'manual', signal: controller.signal, ...(dispatcher ? { dispatcher } : {}), }); @@ -2291,6 +2481,13 @@ export class Crawler extends EventEmitter { } catch { /* ignore */ } + // On a 3xx, resolve the Location so the row shows "→ target" the way + // internal redirects do, instead of a bare status. + let redirectTarget: string | null = null; + if (res.status >= 300 && res.status < 400) { + const loc = res.headers.get('location'); + if (loc) redirectTarget = normalizeUrl(loc, url) ?? loc; + } await this.dbCall('updateExternalProbe', [ url, { @@ -2299,6 +2496,7 @@ export class Crawler extends EventEmitter { contentType: res.headers.get('content-type'), contentLength: parseIntSafe(res.headers.get('content-length')), responseTimeMs, + redirectTarget, }, ]); } else { @@ -2669,6 +2867,35 @@ export class Crawler extends EventEmitter { */ private resourceAdmitted = 0; + /** + * Is `url` allowed by the robots.txt of ITS OWN origin? Caller guards on + * `respectRobotsTxt`. Loads and caches a per-origin checker on first sight; + * while a fetch is in flight the URL is allowed optimistically — the same + * window the start origin already has before its own robots.txt finishes + * loading (see `start`), so behaviour is consistent, just now correct for + * additional origins instead of skipping them entirely. + */ + private robotsAllows(url: string): boolean { + let origin: string; + try { + origin = new URL(url).origin; + } catch { + return true; + } + const checker = this.robotsByOrigin.get(origin); + if (checker) return checker.isAllowed(url); + if (!this.robotsLoadingOrigins.has(origin)) { + this.robotsLoadingOrigins.add(origin); + void loadRobots(origin, this.config.userAgent) + .then((r) => { + if (!this.stopped) this.robotsByOrigin.set(origin, r); + }) + .catch(() => undefined) + .finally(() => this.robotsLoadingOrigins.delete(origin)); + } + return true; + } + private enqueue(item: QueueItem): void { if (this.stopped) return; if (this.seen.has(item.url)) return; @@ -2691,7 +2918,33 @@ export class Crawler extends EventEmitter { this.loopTrapsDropped++; return; } - if (this.robots && !this.robots.isAllowed(item.url)) return; + if (this.config.respectRobotsTxt && !this.robotsAllows(item.url)) { + // Record it instead of dropping it silently. This is the whole point + // of the "Blocked by robots.txt" counter — which was permanently 0 + // because the disallowed URL was never written anywhere. Mark it seen + // so it is recorded exactly once, then persist a lightweight row. + // Fire-and-forget to keep enqueue synchronous, matching the other + // write paths. + if (!this.seen.has(item.url) && this.seen.size < this.config.maxUrls) { + this.seen.add(item.url); + void this.dbCall('upsertUrl', [ + { + url: item.url, + urlMalformed: isUrlMalformed(item.url) ? 1 : 0, + urlTrap: this.trapKind(item.url), + contentKind: 'html', + statusCode: null, + statusText: null, + indexability: 'non-indexable:robots-blocked', + indexabilityReason: 'Blocked by robots.txt', + depth: item.depth, + }, + ]).catch(() => { + /* best-effort — a failed record must not break the crawl */ + }); + } + return; + } if (!this.passesUrlFilter(item.url)) return; if (!this.passesExtensionFilter(item.url)) return; // Hard cap on the in-memory pending queue. Beyond this we drop new @@ -3297,7 +3550,9 @@ export class Crawler extends EventEmitter { this.failed++; this.emit( 'warn', - `Skipped ${item.url}: streamed body exceeded maxFileSizeBytes ${this.config.maxFileSizeBytes}`, + `Skipped ${item.url}: streamed body exceeded the ${effectiveBodyCap( + this.config.maxFileSizeBytes, + )} byte body cap`, ); await this.dbCall('upsertUrl', [ { @@ -3308,7 +3563,9 @@ export class Crawler extends EventEmitter { statusCode, statusText: 'size-cap-exceeded', indexability: 'non-indexable:client-error', - indexabilityReason: `Body skipped — streamed size exceeds maxFileSizeBytes ${this.config.maxFileSizeBytes}`, + indexabilityReason: `Body skipped — streamed size exceeds the ${effectiveBodyCap( + this.config.maxFileSizeBytes, + )} byte body cap`, responseTimeMs, depth: item.depth, }, @@ -3416,13 +3673,12 @@ export class Crawler extends EventEmitter { if (m && m[1]) charset = m[1]; } - const xRobotsLower = xRobotsTag?.toLowerCase() ?? ''; - // Same `none` shorthand as the meta tag, plus the header's optional - // `: ` form (`X-Robots-Tag: googlebot: none`) — the - // colon is a token separator here, not part of a directive. - const headerNoindex = - xRobotsLower.includes('noindex') || - xRobotsLower.split(/[\s,;:]+/).includes('none'); + // Honour bot-scoped directives correctly: `X-Robots-Tag: bingbot: + // noindex` must NOT mark a page Google indexes as non-indexable. + const headerDirectives = xRobotsTag + ? xRobotsTagDirectives(xRobotsTag, robotsUserAgentToken(this.config.userAgent)) + : { noindex: false, nofollow: false }; + const headerNoindex = headerDirectives.noindex; let indexability: Indexability = 'indexable'; let reason: string | null = null; @@ -3761,7 +4017,14 @@ export class Crawler extends EventEmitter { this.enqueueResources(parsed, item.depth); } - if (parsed.hasNofollow || indexability === 'non-indexable:noindex') { + // Follow the page's links unless the page itself says nofollow — + // from meta robots OR the X-Robots-Tag header. `noindex` alone does + // NOT stop link following: `noindex, follow` is the standard + // pattern for paginated archives ("don't index this list page, but + // do crawl through to the items"), and gating on noindex here meant + // every product/article reachable only from page 2+ was never + // fetched and then reported as an orphan. + if (parsed.hasNofollow || headerDirectives.nofollow) { return; } @@ -4196,6 +4459,7 @@ export class Crawler extends EventEmitter { submitSelector: b.submitSelector, successSelector: b.successSelector, waitMs: b.waitMs, + allowInsecureTls: b.allowInsecureTls, }, { headless: jr?.headless ?? true, @@ -4577,6 +4841,28 @@ function parseIntSafe(v: string | null | undefined): number | null { return Number.isFinite(n) ? n : null; } +/** + * Absolute ceiling on a single response body, applied even when the user + * has set no limit of their own. + * + * `maxFileSizeBytes` defaults to 0, and 0 meant "unbounded" — which + * disabled the very protection `readBodyCapped` was written to provide. + * An extension-less URL with no Content-Type is typed as `html` + * (`contentKindFor`), so something like `/download/backup` was buffered + * whole, and at concurrency 20 that is gigabytes in flight against + * Electron's 4 GB heap cap. `Buffer.concat` then doubles the peak. + * + * 64 MB is far beyond any real HTML/CSS/JS document while still bounding + * the worst case. A user-configured limit always wins — this only fills + * in when there isn't one. + */ +const ABSOLUTE_BODY_CAP_BYTES = 64 * 1024 * 1024; + +/** The byte cap actually enforced: the user's, or the safety ceiling. */ +function effectiveBodyCap(configured: number): number { + return configured > 0 ? configured : ABSOLUTE_BODY_CAP_BYTES; +} + /** * Read a fetch Response body as raw bytes with a hard byte cap. undici streams * the DECODED body (content-encoding already stripped), so this bounds the @@ -4591,9 +4877,13 @@ async function readBodyCapped( res: Response, capBytes: number, ): Promise<{ overCap: false; bytes: Buffer } | { overCap: true; bytes: null }> { - if (capBytes <= 0 || !res.body) { + const cap = effectiveBodyCap(capBytes); + if (!res.body) { + // No stream to meter. `arrayBuffer()` is still bounded in practice by + // the Content-Length pre-check upstream. return { overCap: false, bytes: Buffer.from(await res.arrayBuffer()) }; } + capBytes = cap; const reader = (res.body as ReadableStream).getReader(); const chunks: Uint8Array[] = []; let total = 0; diff --git a/packages/core/src/csv-export.ts b/packages/core/src/csv-export.ts index 87474dd..81ca6aa 100644 --- a/packages/core/src/csv-export.ts +++ b/packages/core/src/csv-export.ts @@ -3,6 +3,7 @@ import { pipeline } from 'node:stream/promises'; import { Readable } from 'node:stream'; import type { ProjectDb } from '@freecrawl/db'; import type { CrawlUrlRow, UrlCategory } from '@freecrawl/shared-types'; +import { escapeCsv } from './spreadsheet.js'; const CSV_COLUMNS: (keyof CrawlUrlRow)[] = [ 'url', @@ -31,22 +32,30 @@ const CSV_COLUMNS: (keyof CrawlUrlRow)[] = [ 'crawledAt', ]; -function escapeCsv(value: unknown): string { - if (value === null || value === undefined) return ''; - const str = String(value); - if (/[",\n\r]/.test(str)) { - return `"${str.replace(/"/g, '""')}"`; - } - return str; -} +// Deliberately the shared implementation, not a local copy. This file had +// its own `escapeCsv` with no formula guard, so a crawled `` or +// anchor text beginning with `=`, `+`, `-` or `@` — both fully controlled +// by the crawled site — was written verbatim and executed by Excel / +// LibreOffice / Sheets on open. Every bulk-export file and every +// context-menu "Export as CSV" went through here, while the Export As… +// dialog used the guarded copy: two writers reachable from one grid, only +// one of them safe. export async function exportUrlsToCsv( db: ProjectDb, filePath: string, - options: { selectedIds?: number[]; category?: UrlCategory } = {}, + options: { + selectedIds?: number[]; + category?: UrlCategory; + /** Column override — appended/replaced per topic so a bulk export's + * custom-extraction / hreflang / security files carry their own fields + * instead of only the generic column set. Defaults to CSV_COLUMNS. */ + columns?: (keyof CrawlUrlRow)[]; + } = {}, ): Promise<{ rowsWritten: number }> { let rowsWritten = 0; - const header = CSV_COLUMNS.join(',') + '\n'; + const cols = options.columns && options.columns.length > 0 ? options.columns : CSV_COLUMNS; + const header = cols.map((c) => escapeCsv(c)).join(',') + '\n'; const source: Iterable<CrawlUrlRow> = options.selectedIds && options.selectedIds.length > 0 @@ -58,7 +67,7 @@ export async function exportUrlsToCsv( const generator = async function* (): AsyncGenerator<string> { yield '' + header; for (const row of source) { - const line = CSV_COLUMNS.map((col) => escapeCsv(row[col])).join(',') + '\n'; + const line = cols.map((col) => escapeCsv(row[col])).join(',') + '\n'; rowsWritten++; yield line; } @@ -145,14 +154,14 @@ const BROKEN_LINK_CSV_HEADER = [ export async function exportBrokenLinksToCsv( db: ProjectDb, filePath: string, - options: { internal?: 'all' | 'internal' | 'external' } = {}, + options: { internal?: 'all' | 'internal' | 'external'; search?: string } = {}, ): Promise<{ rowsWritten: number }> { let rowsWritten = 0; const header = BROKEN_LINK_CSV_HEADER.join(',') + '\n'; const generator = async function* (): AsyncGenerator<string> { yield '' + header; - for (const row of db.iterateBrokenLinks(options.internal ?? 'all')) { + for (const row of db.iterateBrokenLinks(options.internal ?? 'all', options.search)) { const cells = [ row.fromUrl, row.fromStatusCode, diff --git a/packages/core/src/html-parser.ts b/packages/core/src/html-parser.ts index 7f0e3bd..4bf5911 100644 --- a/packages/core/src/html-parser.ts +++ b/packages/core/src/html-parser.ts @@ -506,6 +506,27 @@ export function parseHtml( decodeEntities: false, }); + // Document base URL. Per the HTML spec a `<base href>` sets the base + // against which EVERY relative URL on the page resolves — links, + // canonical, images, hreflang, everything. We were resolving all of + // them against `pageUrl` and ignoring `<base>` entirely, so on a page at + // `/shop/cat/page.html` carrying `<base href="https://site/">`, a link + // `products/widget` became `/shop/cat/products/widget` (a phantom 404 in + // Broken Links) and a relative canonical pointed at a URL that doesn't + // exist (page wrongly flagged canonicalised-away). `<base>` changes only + // relative RESOLUTION, never the page's own identity, so `pageUrl` still + // drives self-reference and host checks below. An absolute or malformed + // base falls back to the page URL, matching browser behaviour. + const baseHref = ($('base[href]').first().attr('href') ?? '').trim(); + let baseUrl = pageUrl; + if (baseHref) { + try { + baseUrl = new URL(baseHref, pageUrl).toString(); + } catch { + baseUrl = pageUrl; + } + } + // We parse with decodeEntities:false for speed, so extracted strings // contain raw entities like `'` or `&`. Decode them before // storing so UI / CSV export / search see human-readable text. @@ -557,7 +578,7 @@ export function parseHtml( // `href="/x/"` on `https://a.com/y` is a self-canonical only after // resolution. Same treatment `amphtml` already gets further down. const canonicalResolved = canonical - ? normalizeUrl(canonical, pageUrl, opts.urlRewrites) + ? normalizeUrl(canonical, baseUrl, opts.urlRewrites) : null; // Every declared canonical, resolved and deduped. `canonicalCount > 1` // only says a page repeated the tag - often harmlessly identical (a CMS @@ -567,7 +588,7 @@ export function parseHtml( canonicalEls.each((_, el) => { const href = ($(el).attr('href') ?? '').trim(); if (!href) return; - const abs = normalizeUrl(href, pageUrl, opts.urlRewrites); + const abs = normalizeUrl(href, baseUrl, opts.urlRewrites); if (abs) canonicalTargets.add(abs); }); const canonicalDistinctCount = canonicalTargets.size; @@ -599,9 +620,21 @@ export function parseHtml( // pages as indexable. The contents are joined (comma-separated, in // document order) into one value: the UI's Meta Robots column then // shows the conflict instead of hiding half of it. + // Bot-specific names count too. Google honours `<meta name="googlebot">` + // exactly like `name="robots"` and it overrides the generic tag, so a page + // carrying only `<meta name="googlebot" content="noindex">` was being + // reported as Indexable — the single most consequential thing this column + // can get wrong. + const ROBOTS_META_NAMES = new Set([ + 'robots', + 'googlebot', + 'googlebot-news', + 'bingbot', + ]); const robotsMetaContents: string[] = []; $('meta').each((_, el) => { - if (($(el).attr('name') ?? '').trim().toLowerCase() !== 'robots') return; + const name = ($(el).attr('name') ?? '').trim().toLowerCase(); + if (!ROBOTS_META_NAMES.has(name)) return; const content = ($(el).attr('content') ?? '').trim().toLowerCase(); if (content) robotsMetaContents.push(content); }); @@ -790,7 +823,7 @@ export function parseHtml( const addResource = (raw: string | undefined, kind: ContentKind): void => { const trimmed = (raw ?? '').trim(); if (!trimmed || trimmed.startsWith('data:')) return; - const normalized = normalizeUrl(trimmed, pageUrl, opts.urlRewrites); + const normalized = normalizeUrl(trimmed, baseUrl, opts.urlRewrites); if (!normalized) return; if (!/^https?:/.test(normalized)) return; if (resourceMap.has(normalized)) return; @@ -838,7 +871,7 @@ export function parseHtml( $('iframe[src]').each((_, el) => { const raw = ($(el).attr('src') ?? '').trim(); if (!raw || raw.startsWith('data:') || raw.startsWith('about:')) return; - const normalized = normalizeUrl(raw, pageUrl, opts.urlRewrites); + const normalized = normalizeUrl(raw, baseUrl, opts.urlRewrites); if (!normalized || !/^https?:/.test(normalized)) return; if (iframeMap.has(normalized)) return; iframeMap.set(normalized, { @@ -858,7 +891,7 @@ export function parseHtml( if (($el.attr('hreflang') ?? '').trim()) return; const media = ($el.attr('media') ?? '').toLowerCase(); if (!/max-(device-)?width/.test(media)) return; - const href = normalizeUrl(($el.attr('href') ?? '').trim(), pageUrl, opts.urlRewrites); + const href = normalizeUrl(($el.attr('href') ?? '').trim(), baseUrl, opts.urlRewrites); if (href && /^https?:/.test(href)) mobileAlternate = href; }); @@ -867,8 +900,8 @@ export function parseHtml( // we crawl and store. const paginationNextRaw = ($('link[rel="next"]').attr('href') ?? '').trim(); const paginationPrevRaw = ($('link[rel="prev"]').attr('href') ?? '').trim(); - const paginationNext = paginationNextRaw ? normalizeUrl(paginationNextRaw, pageUrl, opts.urlRewrites) : null; - const paginationPrev = paginationPrevRaw ? normalizeUrl(paginationPrevRaw, pageUrl, opts.urlRewrites) : null; + const paginationNext = paginationNextRaw ? normalizeUrl(paginationNextRaw, baseUrl, opts.urlRewrites) : null; + const paginationPrev = paginationPrevRaw ? normalizeUrl(paginationPrevRaw, baseUrl, opts.urlRewrites) : null; // Hreflang — `<link rel="alternate" hreflang="…" href="…">`. We dedupe // by lang+href because some sites repeat tags accidentally. @@ -878,7 +911,7 @@ export function parseHtml( const lang = ($(el).attr('hreflang') ?? '').trim(); const rawHref = ($(el).attr('href') ?? '').trim(); if (!lang || !rawHref) return; - const href = normalizeUrl(rawHref, pageUrl, opts.urlRewrites); + const href = normalizeUrl(rawHref, baseUrl, opts.urlRewrites); if (!href) return; const key = `${lang}|${href}`; if (hreflangSet.has(key)) return; @@ -909,21 +942,21 @@ export function parseHtml( let rawSrc = ($v.attr('src') ?? '').trim(); if (!rawSrc) rawSrc = ($v.find('source[src]').first().attr('src') ?? '').trim(); if (!rawSrc) return; - const contentLoc = normalizeUrl(rawSrc, pageUrl, opts.urlRewrites); + const contentLoc = normalizeUrl(rawSrc, baseUrl, opts.urlRewrites); if (!contentLoc) return; const posterRaw = ($v.attr('poster') ?? '').trim(); - const thumbnail = posterRaw ? normalizeUrl(posterRaw, pageUrl, opts.urlRewrites) : null; + const thumbnail = posterRaw ? normalizeUrl(posterRaw, baseUrl, opts.urlRewrites) : null; pushVideo(contentLoc, null, thumbnail); }); $('iframe[src]').each((_, el) => { - const embed = parseVideoEmbed(($(el).attr('src') ?? '').trim(), pageUrl, opts.urlRewrites); + const embed = parseVideoEmbed(($(el).attr('src') ?? '').trim(), baseUrl, opts.urlRewrites); if (embed) pushVideo(null, embed.playerLoc, embed.thumbnail); }); // AMP variant — `<link rel="amphtml">` points to the AMP version of // the current page, when one exists. const amphtmlRaw = ($('link[rel="amphtml"]').attr('href') ?? '').trim(); - const amphtml = amphtmlRaw ? normalizeUrl(amphtmlRaw, pageUrl, opts.urlRewrites) : null; + const amphtml = amphtmlRaw ? normalizeUrl(amphtmlRaw, baseUrl, opts.urlRewrites) : null; // AMP smoke validation — pages that declare themselves AMP via // `<html ⚡>` / `<html amp>` get a hand-rolled subset of Google's @@ -942,7 +975,7 @@ export function parseHtml( const faviconRaw = ($('link[rel="icon"]').first().attr('href') ?? '').trim() || ($('link[rel="shortcut icon"]').first().attr('href') ?? '').trim(); - const favicon = faviconRaw ? normalizeUrl(faviconRaw, pageUrl, opts.urlRewrites) : null; + const favicon = faviconRaw ? normalizeUrl(faviconRaw, baseUrl, opts.urlRewrites) : null; // Apple touch icon — iOS/iPadOS home-screen icon. Multiple sizes can be // declared; we surface the first to keep the column simple. @@ -950,7 +983,7 @@ export function parseHtml( ($('link[rel="apple-touch-icon"]').first().attr('href') ?? '').trim() || ($('link[rel="apple-touch-icon-precomposed"]').first().attr('href') ?? '').trim(); const appleTouchIcon = appleTouchRaw - ? normalizeUrl(appleTouchRaw, pageUrl, opts.urlRewrites) + ? normalizeUrl(appleTouchRaw, baseUrl, opts.urlRewrites) : null; // Android Chrome / PWA icon — `<link rel="icon" sizes="192x192">` (or @@ -979,14 +1012,14 @@ export function parseHtml( if (!qualifies) return; const href = ($(el).attr('href') ?? '').trim(); if (!href) return; - const resolved = normalizeUrl(href, pageUrl, opts.urlRewrites); + const resolved = normalizeUrl(href, baseUrl, opts.urlRewrites); if (resolved) androidIcon = resolved; }); // Web app manifest — PWA support signal. const manifestRaw = ($('link[rel="manifest"]').first().attr('href') ?? '').trim(); const manifestUrl = manifestRaw - ? normalizeUrl(manifestRaw, pageUrl, opts.urlRewrites) + ? normalizeUrl(manifestRaw, baseUrl, opts.urlRewrites) : null; // RSS / Atom feed — `<link rel="alternate" type="application/rss+xml">` or @@ -999,7 +1032,7 @@ export function parseHtml( ( $('link[rel="alternate"][type="application/atom+xml"]').first().attr('href') ?? '' ).trim(); - const feedUrl = feedRaw ? normalizeUrl(feedRaw, pageUrl, opts.urlRewrites) : null; + const feedUrl = feedRaw ? normalizeUrl(feedRaw, baseUrl, opts.urlRewrites) : null; // Meta refresh — `<meta http-equiv="refresh" content="N; url=…">`. // Even when the URL is absent (pure auto-reload) we still capture the @@ -1018,7 +1051,7 @@ export function parseHtml( // optional and we strip them defensively. const urlMatch = metaRefresh.match(/[;,]\s*url\s*=\s*['"]?([^'"\s;,]+)['"]?/i); if (urlMatch && urlMatch[1]) { - metaRefreshUrl = normalizeUrl(urlMatch[1], pageUrl, opts.urlRewrites); + metaRefreshUrl = normalizeUrl(urlMatch[1], baseUrl, opts.urlRewrites); } } @@ -1223,7 +1256,7 @@ export function parseHtml( $('a[href]').each((_, el) => { const href = $(el).attr('href'); if (!href) return; - const normalized = normalizeUrl(href, pageUrl, opts.urlRewrites); + const normalized = normalizeUrl(href, baseUrl, opts.urlRewrites); if (!normalized || !/^https?:/.test(normalized)) { const raw = href.trim(); if ( @@ -1236,10 +1269,31 @@ export function parseHtml( } return; } - if (linkMap.has(normalized)) return; - const $el = $(el); const rel = ($el.attr('rel') ?? '').trim().toLowerCase() || null; + + // Same target linked more than once (a nav/footer link that also + // appears in the body). Keep the MOST permissive occurrence rather + // than the first: a `rel="nofollow"` nav link followed by a plain + // content link to the same page used to record the target as nofollow, + // and with `storeNofollowLinks` off (the default) the crawler then + // dropped it entirely — so the page lost an inlink and, if that was its + // only link, was never crawled and reported as an orphan. If any + // occurrence is followable, the link is followable. + const existing = linkMap.get(normalized); + if (existing) { + if (existing.rel?.includes('nofollow') && !rel?.includes('nofollow')) { + existing.rel = rel; + // A followable content link also carries the more useful anchor + // text than an icon-only nav link, so take it when we had none. + if (!existing.anchor) { + const upgradedAnchor = + $el.text().replace(/\s+/g, ' ').trim().slice(0, 200) || null; + if (upgradedAnchor) existing.anchor = decodeEntities(upgradedAnchor); + } + } + return; + } const target = ($el.attr('target') ?? '').trim() || null; const rawAnchor = $el.text().replace(/\s+/g, ' ').trim().slice(0, 200) || null; const anchor = rawAnchor ? decodeEntities(rawAnchor) : null; @@ -1436,7 +1490,7 @@ export function parseHtml( ): void => { if (!rawUrl) return; if (rawUrl.startsWith('data:')) return; - const normalized = normalizeUrl(rawUrl, pageUrl, opts.urlRewrites); + const normalized = normalizeUrl(rawUrl, baseUrl, opts.urlRewrites); if (!normalized) return; if (!/^https?:/.test(normalized)) return; if (imageMap.has(normalized)) return; @@ -1462,7 +1516,7 @@ export function parseHtml( // Skip inline data URIs — they're not "web resources" in the crawler // sense and would bloat the images table fast on any CMS. if (rawSrc.startsWith('data:')) return; - const normalized = normalizeUrl(rawSrc, pageUrl, opts.urlRewrites); + const normalized = normalizeUrl(rawSrc, baseUrl, opts.urlRewrites); if (!normalized) return; if (!/^https?:/.test(normalized)) return; @@ -1537,7 +1591,7 @@ export function parseHtml( // will fill `byte_size` automatically post-crawl. for (const ogish of [ogImage, twitterImage]) { if (!ogish) continue; - const normalized = normalizeUrl(ogish, pageUrl, opts.urlRewrites); + const normalized = normalizeUrl(ogish, baseUrl, opts.urlRewrites); if (!normalized) continue; if (!/^https?:/.test(normalized)) continue; if (imageMap.has(normalized)) continue; diff --git a/packages/core/src/http-client.ts b/packages/core/src/http-client.ts index 6e2fdce..2377ea0 100644 --- a/packages/core/src/http-client.ts +++ b/packages/core/src/http-client.ts @@ -276,6 +276,28 @@ export function detectHttpProtocol(altSvcHeader: string | null): string | null { * fetch failed -> UNABLE_TO_GET_ISSUER_CERT_LOCALLY (TLS root not trusted — check antivirus / corporate proxy) * fetch failed -> ECONNREFUSED */ +/** + * Strip header values that undici echoes back inside its own error text. + * + * When a header value contains a character the spec forbids, undici throws + * `Headers.append: "<the entire value>" is an invalid header value.` If that + * header is `Authorization` — or a custom one like `X-Api-Key` — the secret + * is now inside an Error message. A trailing newline or tab is exactly what + * you get pasting a token out of a terminal or a .env file, so this is a + * routine mistake, not a contrived one. + * + * That message does not stay in memory: it is emitted as a crawler warning + * (disk log + in-app Logs window) and written to the database as both + * `statusText` and `indexabilityReason` — and `indexabilityReason` is a CSV + * export column, so the token would travel with any exported report. + */ +export function redactHeaderValues(text: string): string { + return text.replace( + /(Headers\.(?:append|set): )"[^"]*"( is an invalid header (?:value|name))/gi, + '$1"[redacted]"$2', + ); +} + export function formatFetchError(err: unknown): string { const parts: string[] = []; const seen = new Set<unknown>(); @@ -293,7 +315,7 @@ export function formatFetchError(err: unknown): string { break; } } - const chain = parts.join(' -> '); + const chain = redactHeaderValues(parts.join(' -> ')); // Friendly hints for the most common packaged-app failure modes. // ORDER MATTERS — more specific patterns must come first. DNS-layer // errors (queryA / queryAAAA / EDESTRUCTION) frequently surface as @@ -387,6 +409,21 @@ export function collectNetworkDiagnostics(opts: { proxyOverride?: string } = {}) * if the user configured them themselves they don't want them appearing in * the in-app log window. */ +/** + * Reduce a URL to its origin for logging, dropping userinfo and the path. + * + * For an incoming-webhook URL the PATH is the secret — a Slack or Discord + * webhook is bearer-equivalent, so `https://hooks.slack.com/services/T00/B00/XXXX` + * must never appear in a log file that users are asked to share. + */ +export function redactUrlSecrets(url: string): string { + try { + return `${new URL(url).origin}/…`; + } catch { + return '[unparseable url]'; + } +} + function redactProxyCreds(url: string): string { try { const u = new URL(url); diff --git a/packages/core/src/json-export.ts b/packages/core/src/json-export.ts index 8122c12..9ecc237 100644 --- a/packages/core/src/json-export.ts +++ b/packages/core/src/json-export.ts @@ -2,13 +2,17 @@ import { createWriteStream } from 'node:fs'; import { pipeline } from 'node:stream/promises'; import { Readable } from 'node:stream'; import type { ProjectDb } from '@freecrawl/db'; -import type { CrawlUrlRow } from '@freecrawl/shared-types'; +import type { CrawlUrlRow, UrlCategory } from '@freecrawl/shared-types'; export interface JsonExportOptions { /** When set, export only these URL ids (used by "Export Selected"). */ selectedIds?: number[]; /** Pretty-print with 2-space indent. Default `false` — newline-delimited compact JSON. */ pretty?: boolean; + /** Restrict to a sidebar category (e.g. `issues:title-missing`). Ignored + * when `selectedIds` is set. Without it the export was the whole crawl + * even when the caller asked for a category. */ + category?: UrlCategory; } /** @@ -35,7 +39,9 @@ export async function exportUrlsToJson( const source: Iterable<CrawlUrlRow> = options.selectedIds && options.selectedIds.length > 0 ? db.iterateUrlsByIds(options.selectedIds) - : db.iterateAllUrls(); + : options.category + ? db.iterateUrlsByCategory(options.category) + : db.iterateAllUrls(); const generator = async function* (): AsyncGenerator<string> { yield '[' + lead; diff --git a/packages/core/src/robots.ts b/packages/core/src/robots.ts index 1492275..47b3720 100644 --- a/packages/core/src/robots.ts +++ b/packages/core/src/robots.ts @@ -12,7 +12,41 @@ const NOOP: RobotsChecker = { getCrawlDelay: () => undefined, }; -export async function loadRobots(origin: string, userAgent: string): Promise<RobotsChecker> { +/** + * Reduce a User-Agent *header* to the product token robots.txt groups are + * written against. + * + * `robots-parser` expects a token, not a header: internally it lowercases + * the value and truncates at the first `/`. Handing it the full preset + * header `Mozilla/5.0 (compatible; Googlebot/2.1; +http://…)` therefore + * matched against `mozilla`, which appears in no robots.txt — so NO + * crawler-specific group ever applied and everything silently fell through + * to `User-agent: *`. A site with `User-agent: Googlebot / Disallow: /` + * was crawled in full and every page reported Indexable. + * + * `compatible; <token>` is the conventional place a bot names itself; when + * it is absent, the leading product token is the right answer + * (`FreeCrawlSEO/0.1 (+…)` → `FreeCrawlSEO`). + */ +export function robotsUserAgentToken(userAgent: string): string { + const compat = /compatible;\s*([A-Za-z0-9._*-]+)/i.exec(userAgent); + if (compat?.[1]) return compat[1]; + const leading = userAgent.trim().split(/[/\s;(]/)[0]; + return leading && leading.length > 0 ? leading : userAgent; +} + +export async function loadRobots( + origin: string, + userAgent: string, + /** Optional notice sink. Called when robots.txt returns a server error + * (5xx): RFC 9309 says a crawler SHOULD then treat the whole site as + * disallowed, but we keep crawling (allow-all) so a transient 503 — very + * common behind Cloudflare — doesn't silently nuke the entire crawl. The + * caller surfaces this so the choice is visible rather than hidden. */ + onNotice?: (message: string) => void, +): Promise<RobotsChecker> { + // The header goes on the wire; only the token goes to the matcher. + const uaToken = robotsUserAgentToken(userAgent); const robotsUrl = new URL('/robots.txt', origin).toString(); const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), 5000); @@ -26,10 +60,15 @@ export async function loadRobots(origin: string, userAgent: string): Promise<Rob const body = await res.text(); const parser = robotsParser(robotsUrl, body); return { - isAllowed: (url: string) => parser.isAllowed(url, userAgent) ?? true, - getCrawlDelay: () => parser.getCrawlDelay(userAgent), + isAllowed: (url: string) => parser.isAllowed(url, uaToken) ?? true, + getCrawlDelay: () => parser.getCrawlDelay(uaToken), }; } + if (res.status >= 500 && onNotice) { + onNotice( + `robots.txt for ${origin} returned HTTP ${res.status}. Per RFC 9309 a 5xx should be treated as "disallow all", but FreeCrawl continues crawling to avoid a transient server error blocking the whole run — verify the site is reachable if results look off.`, + ); + } } catch { // ignore — default allow } finally { @@ -73,6 +112,11 @@ export async function testUrlAgainstRobots( // `gamesatis.com`, `www.example.com/foo`, or `//host/path`. Prepend // `https://` when the scheme is missing so `new URL()` succeeds. const probedUrl = normalizeRobotsTestInput(url); + // Same split as `loadRobots`: the full header identifies us on the wire, + // the product token is what robots.txt groups are matched against. The + // tester has to use the identical rule or it would report "allowed" for + // URLs the crawler actually blocks, and vice versa. + const uaToken = robotsUserAgentToken(userAgent); let origin = ''; let robotsUrl = ''; try { @@ -110,8 +154,8 @@ export async function testUrlAgainstRobots( robotsUrl: '<custom>', status: 0, body, - allowed: parser.isAllowed(probedUrl, userAgent) ?? true, - crawlDelay: parser.getCrawlDelay(userAgent) ?? null, + allowed: parser.isAllowed(probedUrl, uaToken) ?? true, + crawlDelay: parser.getCrawlDelay(uaToken) ?? null, sitemaps, error: null, }; @@ -138,8 +182,12 @@ export async function testUrlAgainstRobots( } catch { /* ignore */ } - // Per the robots.txt RFC: any non-success makes the site "allow all" - // for crawlers. Reflect that explicitly. + // RFC 9309 distinguishes the two: a 4xx ("unavailable") means allow + // all; a 5xx ("unreachable") means a crawler SHOULD treat the site as + // fully disallowed. FreeCrawl still crawls on a 5xx (so a transient + // 503 doesn't nuke the run) but the message must not claim the RFC + // says "allow" — state what actually happens per status. + const is5xx = status !== null && status >= 500; return { url: probedUrl, robotsUrl, @@ -148,7 +196,9 @@ export async function testUrlAgainstRobots( allowed: true, crawlDelay: null, sitemaps: [], - error: `robots.txt returned HTTP ${status} — defaulting to allow.`, + error: is5xx + ? `robots.txt returned HTTP ${status}. Per RFC 9309 a 5xx means "disallow all"; FreeCrawl still crawls to survive transient server errors, but a compliant crawler (Googlebot) would stop.` + : `robots.txt returned HTTP ${status} (4xx = unavailable) — allow all, per RFC 9309.`, }; } } catch (err) { @@ -179,8 +229,8 @@ export async function testUrlAgainstRobots( robotsUrl, status, body, - allowed: parser.isAllowed(probedUrl, userAgent) ?? true, - crawlDelay: parser.getCrawlDelay(userAgent) ?? null, + allowed: parser.isAllowed(probedUrl, uaToken) ?? true, + crawlDelay: parser.getCrawlDelay(uaToken) ?? null, sitemaps, error: null, }; diff --git a/packages/core/src/sitemap-export.ts b/packages/core/src/sitemap-export.ts index 877dafa..4132b28 100644 --- a/packages/core/src/sitemap-export.ts +++ b/packages/core/src/sitemap-export.ts @@ -47,6 +47,14 @@ export interface SitemapOptions { * useful when individual entries are large (image variant). */ splitAtUrlCount?: number; + /** + * Absolute base URL (origin) the sharded sitemap-index `<loc>` entries are + * built from, e.g. `https://example.com`. The protocol requires index + * `<loc>` values to be fully-qualified URLs; without this they fell back + * to bare filenames and Search Console rejected the index. When omitted, + * the origin is derived from the first crawled URL. + */ + baseUrl?: string; } export interface SitemapExportResult { @@ -63,12 +71,21 @@ export interface SitemapExportResult { const HARD_URL_LIMIT = 50_000; function escapeXml(s: string): string { - return s - .replace(/&/g, '&') - .replace(/</g, '<') - .replace(/>/g, '>') - .replace(/"/g, '"') - .replace(/'/g, '''); + return ( + s + .replace(/&/g, '&') + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/'/g, ''') + // XML 1.0 forbids these regardless of escaping. They reach the news + // variant through page titles and the image variant through `alt` + // (which, unlike title, isn't trimmed), and one occurrence makes the + // whole sitemap unreadable — Search Console reports "Sitemap could + // not be read" with no indication of which URL caused it. + // eslint-disable-next-line no-control-regex -- intentionally matching control chars to strip them + .replace(/[\x00-\x08\x0B\x0C\x0E-\x1F]/g, '') + ); } function formatLastmod(crawledAt: string): string { @@ -385,10 +402,25 @@ export async function exportSitemap( } const indexPath = gzip && !filePath.endsWith('.gz') ? `${filePath}.gz` : filePath; - // Index references parts by basename; the user typically uploads the - // whole directory side-by-side with the index. Absolute URLs are the - // user's responsibility (typically prepended via post-processing). - await writeIndex(indexPath, partFiles.map((p) => path.basename(p)), gzip); + // The sitemap protocol requires index <loc> values to be fully-qualified + // URLs — a bare filename makes Search Console reject the index. Resolve + // the part filenames against the crawl origin (explicit baseUrl, else the + // first crawled URL's origin). If no origin can be derived we fall back to + // basenames rather than failing the export. + let origin = ''; + const rawBase = (options.baseUrl ?? entries[0]?.url ?? '').trim(); + if (rawBase) { + try { + origin = new URL(rawBase).origin; + } catch { + origin = ''; + } + } + const locFor = (p: string): string => { + const name = path.basename(p); + return origin ? `${origin}/${name}` : name; + }; + await writeIndex(indexPath, partFiles.map(locFor), gzip); files.unshift(indexPath); return { diff --git a/packages/core/src/sitemap.ts b/packages/core/src/sitemap.ts index 24201ac..a5b9fd9 100644 --- a/packages/core/src/sitemap.ts +++ b/packages/core/src/sitemap.ts @@ -1,7 +1,7 @@ import * as cheerio from 'cheerio'; import { gunzipSync } from 'node:zlib'; import { fetch as undiciFetch } from 'undici'; -import { normalizeUrl } from './url-utils.js'; +import { normalizeUrl, type UrlRewriteOptions } from './url-utils.js'; import { defaultRequestHeaders, formatFetchError } from './http-client.js'; /** @@ -11,6 +11,21 @@ import { defaultRequestHeaders, formatFetchError } from './http-client.js'; */ const SITEMAP_MAX_URLS = 50_000; const SITEMAP_MAX_BYTES = 50 * 1024 * 1024; +/** + * Hard ceiling on what a gzipped sitemap may inflate to. + * + * `SITEMAP_MAX_BYTES` above is the sitemaps.org limit, and breaching it is + * deliberately only a warning — the data is still useful. But that check + * ran *after* `gunzipSync` had already materialised the whole buffer, so + * a decompression bomb (a few KB inflating to gigabytes) allocated all of + * it first and could take the process down. Sitemaps are fetched with no + * user action at all (`discoverSitemaps` defaults on), so the input is not + * something the user chose to trust. + * + * 4× the protocol limit: no real sitemap comes close, and a bomb is orders + * of magnitude past it. + */ +const SITEMAP_MAX_INFLATED_BYTES = SITEMAP_MAX_BYTES * 4; /** * Content types a sitemap may legitimately arrive as. Servers are sloppy @@ -169,6 +184,15 @@ interface FetchOpts { maxUrls: number; /** Max sitemap-index nesting (1 = root only, 2 = root + children, …). */ maxDepth: number; + /** + * The crawl's URL-rewrite policy. Sitemap entries MUST be normalised with + * the same rewrites the crawler applies to page URLs — otherwise the + * stored `sitemap_urls.url` (e.g. `…/about/`) never equals the crawled + * `urls.url` (`…/about` under Trailing Slash = Strip), and every + * sitemap↔crawl join silently reports "crawled but not in sitemap" for + * every page and "sitemap orphan" for every sitemap entry. + */ + rewrites?: UrlRewriteOptions; } /** @@ -245,7 +269,11 @@ export async function fetchSitemaps( let xml: string; if (looksGzipped(raw)) { try { - const inflated = gunzipSync(raw); + // `maxOutputLength` makes zlib abort mid-inflation instead of + // allocating the whole bomb and checking its size afterwards. + const inflated = gunzipSync(raw, { + maxOutputLength: SITEMAP_MAX_INFLATED_BYTES, + }); if (inflated.byteLength > SITEMAP_MAX_BYTES) { result.warnings.push({ sitemap: item.url, @@ -298,7 +326,7 @@ export async function fetchSitemaps( result.truncated = true; break; } - const norm = normalizeUrl(entry.url); + const norm = normalizeUrl(entry.url, undefined, opts.rewrites); if (!norm) continue; result.entries.push({ ...entry, url: norm }); } diff --git a/packages/core/src/spreadsheet.ts b/packages/core/src/spreadsheet.ts index fbb922a..2ef2e9b 100644 --- a/packages/core/src/spreadsheet.ts +++ b/packages/core/src/spreadsheet.ts @@ -50,7 +50,18 @@ export function sanitizeFormulaPrefix(str: string): string { export function escapeCsv(value: unknown): string { if (value === null || value === undefined) return ''; - const str = sanitizeFormulaPrefix(String(value)); + // Numbers and booleans cannot be formulas, and prefixing them turns real + // data into text: Excel then skips the column in AVERAGE() and sorts it + // lexicographically. Negative values are ordinary here — Flesch reading + // ease is clamped to [-50, 120] and Flesch-Kincaid grade to [-2, 30] — + // so `-33.4` was being written as `'-33.4`. + if (typeof value === 'number' || typeof value === 'boolean') { + return String(value); + } + // NUL is legal in a JS string but not in a CSV consumer: pandas raises + // `ValueError: embedded null byte` and Excel truncates the cell. It + // reaches us through numeric HTML entities (`�`) in page titles. + const str = sanitizeFormulaPrefix(String(value).replace(/\0/g, '')); if (/[",\n\r]/.test(str)) { return `"${str.replace(/"/g, '""')}"`; } diff --git a/packages/core/src/tabular-export.ts b/packages/core/src/tabular-export.ts index 983411a..4e27cdf 100644 --- a/packages/core/src/tabular-export.ts +++ b/packages/core/src/tabular-export.ts @@ -4,8 +4,18 @@ import { Readable } from 'node:stream'; import path from 'node:path'; import zlib from 'node:zlib'; import type { ProjectDb } from '@freecrawl/db'; -import type { CrawlUrlRow, UrlCategory } from '@freecrawl/shared-types'; +import type { CrawlUrlRow, UrlCategory, AdvancedFilter } from '@freecrawl/shared-types'; import { ensureHeapHeadroom } from './heap-guard.js'; +import { escapeCsv } from './spreadsheet.js'; + +/** How a section selects its rows. `selectedIds` wins; otherwise category is + * narrowed by the active search/filter so the export matches the grid. */ +interface RowQuery { + category: UrlCategory; + selectedIds?: number[]; + search?: string; + filter?: AdvancedFilter; +} export interface TabularSection { label: string; @@ -26,6 +36,11 @@ export interface TabularExportOptions { columns: string[]; /** When provided AND non-empty, restrict every section to these row ids. */ selectedIds?: number[]; + /** Active URL-table search — applied to every section (ignored when + * `selectedIds` is set) so the export matches the filtered grid. */ + search?: string; + /** Active advanced filter — same rationale as `search`. */ + filter?: AdvancedFilter; /** Prefix CSV files with a UTF-8 BOM so Excel-for-Windows opens them * in the correct charset. Ignored for non-CSV formats. Default true. */ csvBom?: boolean; @@ -48,55 +63,75 @@ export interface TabularExportResult { * canonical CSV-injection mitigation per OWASP. The apostrophe is * visible in Excel only when the user is editing the cell. */ -function sanitizeFormulaPrefix(str: string): string { - if (!str) return str; - const first = str.charCodeAt(0); - // 0x09 TAB, 0x0D CR — both can start a formula in some parsers. - if (first === 0x3d || first === 0x2b || first === 0x2d || first === 0x40 || first === 0x09 || first === 0x0d) { - return `'${str}`; +/** + * Excel's hard limit on characters in one cell. Exceed it and Excel + * declares the workbook corrupt ("We found a problem with some content…") + * and silently drops the record when the user clicks Repair — so the cell + * that mattered most is the one that disappears. Reachable today from the + * Structured Data grid export, which writes whole raw JSON-LD blocks. + */ +const XLSX_MAX_CELL_CHARS = 32_767; + +/** Render one text cell for a worksheet. + * + * No formula guard here, deliberately. In SpreadsheetML a cell is a + * formula only if it carries an `<f>` child; `t="inlineStr"` content is + * never evaluated. Prefixing it with `'` therefore protected nothing and + * corrupted real values — a title like `-50% İndirim Fırsatı` reached the + * user as `'-50% İndirim Fırsatı`. (XLSX being inert this way is exactly + * why it is the recommended mitigation for CSV injection.) + * + * `xml:space="preserve"` keeps leading/trailing spaces, which Excel + * otherwise trims — and padded anchor text is itself a finding this tool + * reports, so silently trimming it destroys the evidence. + */ +function inlineStrCell(cellRef: string, value: unknown): string { + let text = String(value); + if (text.length > XLSX_MAX_CELL_CHARS) { + text = text.slice(0, XLSX_MAX_CELL_CHARS - 1) + '…'; } - return str; + const preserve = /^\s|\s$/.test(text) ? ' xml:space="preserve"' : ''; + return `<c r="${cellRef}" t="inlineStr"><is><t${preserve}>${xmlEscape(text)}</t></is></c>`; } -function escapeCsv(value: unknown): string { - if (value === null || value === undefined) return ''; - const str = sanitizeFormulaPrefix(String(value)); - if (/[",\n\r]/.test(str)) { - return `"${str.replace(/"/g, '""')}"`; - } - return str; -} - -function rowSource( - db: ProjectDb, - category: UrlCategory, - selectedIds: number[] | undefined, -): Iterable<CrawlUrlRow> { - if (selectedIds && selectedIds.length > 0) { +function rowSource(db: ProjectDb, q: RowQuery): Iterable<CrawlUrlRow> { + if (q.selectedIds && q.selectedIds.length > 0) { // Apply selectedIds first, then narrow by category client-side. The // sidebar's category predicates can be expensive; for export the - // selection is always the small side. - const ids = new Set(selectedIds); + // selection is always the small side. (An explicit selection overrides + // the search/filter — the user picked exact rows.) + const ids = new Set(q.selectedIds); return (function* () { - for (const row of db.iterateUrlsByCategory(category)) { + for (const row of db.iterateUrlsByCategory(q.category)) { if (ids.has(row.id)) yield row; } })(); } - return db.iterateUrlsByCategory(category); + // Honour the active search / advanced filter so the export mirrors the + // on-screen grid rather than the whole category. + if (q.search || q.filter) { + return db.iterateUrlsByQuery({ + category: q.category, + search: q.search, + filter: q.filter, + }); + } + return db.iterateUrlsByCategory(q.category); } async function writeCsvFile( db: ProjectDb, filePath: string, columns: string[], - category: UrlCategory, - selectedIds: number[] | undefined, + query: RowQuery, withBom: boolean, ): Promise<number> { let rowsWritten = 0; - const header = columns.join(',') + '\n'; - const source = rowSource(db, category, selectedIds); + // Escape the header row too: a column label containing a comma, quote or + // leading `=`/`+`/`-`/`@` would otherwise break the CSV or inject a + // formula. The data rows below already go through escapeCsv. + const header = columns.map((c) => escapeCsv(c)).join(',') + '\n'; + const source = rowSource(db, query); const generator = async function* (): AsyncGenerator<string> { if (withBom) yield '' + header; else yield header; @@ -118,12 +153,22 @@ async function writeCsvFile( function escapeXml(value: unknown): string { if (value === null || value === undefined) return ''; - return String(value) - .replace(/&/g, '&') - .replace(/</g, '<') - .replace(/>/g, '>') - .replace(/"/g, '"') - .replace(/'/g, '''); + return ( + String(value) + .replace(/&/g, '&') + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/'/g, ''') + // Characters XML 1.0 forbids outright — escaping does NOT make them + // legal. One page whose title carries an interior control char (they + // arrive via numeric HTML entities and survive `.trim()`) made the + // whole document unparseable, so every consumer saw zero rows from a + // single bad page. `xmlEscape` below already strips these; this + // escaper was simply missing it. + // eslint-disable-next-line no-control-regex -- intentionally matching control chars to strip them + .replace(/[\x00-\x08\x0B\x0C\x0E-\x1F]/g, '') + ); } function xmlSafeTag(name: string): string { @@ -138,11 +183,10 @@ async function writeJsonFile( db: ProjectDb, filePath: string, columns: string[], - category: UrlCategory, - selectedIds: number[] | undefined, + query: RowQuery, ): Promise<number> { let rowsWritten = 0; - const source = rowSource(db, category, selectedIds); + const source = rowSource(db, query); const generator = async function* (): AsyncGenerator<string> { yield '['; let first = true; @@ -168,12 +212,11 @@ async function writeXmlFile( db: ProjectDb, filePath: string, columns: string[], - category: UrlCategory, - selectedIds: number[] | undefined, + query: RowQuery, sectionLabel: string, ): Promise<number> { let rowsWritten = 0; - const source = rowSource(db, category, selectedIds); + const source = rowSource(db, query); const rootTag = xmlSafeTag(sectionLabel) || 'export'; const cols = columns.map((c) => ({ key: c, tag: xmlSafeTag(c) })); const generator = async function* (): AsyncGenerator<string> { @@ -197,17 +240,36 @@ async function writeXmlFile( return rowsWritten; } +// Windows reserved device names — writing to `con.csv` etc. targets the +// console/printer device, not a file. Rejected in both helpers. +const WINDOWS_DEVICE_NAMES = /^(con|prn|aux|nul|com[1-9]|lpt[1-9])$/i; + +function sanitizeSegment(seg: string): string { + const cleaned = seg + // eslint-disable-next-line no-control-regex -- deliberately stripping control chars from a filename + .replace(/[\\/?*[\]:<>|"\x00-\x1f]/g, '_') + .replace(/\s+/g, '-') + .toLowerCase(); + return WINDOWS_DEVICE_NAMES.test(cleaned) ? `_${cleaned}` : cleaned; +} + function sanitizeFilename(label: string): string { - return label.replace(/[\\/?*[\]:<>|"]/g, '_').replace(/\s+/g, '-').toLowerCase(); + const cleaned = sanitizeSegment(label); + // A bare `.`/`..` (or one that sanitised down to that) must never become a + // filename — strip leading/trailing dots. + const stripped = cleaned.replace(/^\.+|\.+$/g, ''); + return stripped.length > 0 ? stripped : 'export'; } function sanitizeSubdir(subdir: string): string { // Sanitize each path segment individually so a hierarchical export like // 'crawl-data/internal' stays nested rather than collapsing into one - // filename component. + // filename component. `.`/`..` segments are dropped so a caller-supplied + // `../../etc` cannot escape the chosen output root (path traversal). return subdir .split(/[\\/]+/) - .map((seg) => seg.replace(/[\\/?*[\]:<>|"]/g, '_').replace(/\s+/g, '-').toLowerCase()) + .filter((seg) => seg !== '.' && seg !== '..') + .map(sanitizeSegment) .filter((seg) => seg.length > 0) .join(path.sep); } @@ -296,10 +358,7 @@ function buildSheetXmlFromCells( } else if (typeof v === 'boolean') { parts.push(`<c r="${cellRef}" t="b"><v>${v ? 1 : 0}</v></c>`); } else { - const safe = sanitizeFormulaPrefix(String(v)); - parts.push( - `<c r="${cellRef}" t="inlineStr"><is><t>${xmlEscape(safe)}</t></is></c>`, - ); + parts.push(inlineStrCell(cellRef, v)); } }); parts.push('</row>'); @@ -339,13 +398,7 @@ function buildSheetXml( } else if (typeof v === 'boolean') { parts.push(`<c r="${cellRef}" t="b"><v>${v ? 1 : 0}</v></c>`); } else { - // Bug #9 — neutralise leading formula triggers; Excel auto-runs - // `=cmd|...` if the cell isn't sanitised. Apostrophe prefix is - // the canonical guard (OWASP CSV-injection guidance). - const safe = sanitizeFormulaPrefix(String(v)); - parts.push( - `<c r="${cellRef}" t="inlineStr"><is><t>${xmlEscape(safe)}</t></is></c>`, - ); + parts.push(inlineStrCell(cellRef, v)); } }); parts.push('</row>'); @@ -543,6 +596,13 @@ export async function exportTabular( ): Promise<TabularExportResult> { const { format, sections, columns, selectedIds } = options; const csvBom = options.csvBom !== false; + // Build the per-section query once: an explicit selection overrides the + // search/filter (the user picked exact rows), otherwise the active grid + // search + advanced filter apply so the export mirrors the view. + const queryFor = (category: UrlCategory): RowQuery => + selectedIds && selectedIds.length > 0 + ? { category, selectedIds } + : { category, search: options.search, filter: options.filter }; if (sections.length === 0) { throw new Error('exportTabular: at least one section is required'); } @@ -561,7 +621,7 @@ export async function exportTabular( let total = 0; const sheets = sections.map((section) => { const rows: CrawlUrlRow[] = []; - for (const row of rowSource(db, section.category, selectedIds)) { + for (const row of rowSource(db, queryFor(section.category))) { rows.push(row); if (rows.length % 50_000 === 0) { ensureHeapHeadroom('XLSX export', 128 * 1024 * 1024); @@ -595,13 +655,14 @@ export async function exportTabular( : outputPath; const writeOne = async (filePath: string, section: TabularSection): Promise<number> => { + const query = queryFor(section.category); if (format === 'csv') { - return writeCsvFile(db, filePath, columns, section.category, selectedIds, csvBom); + return writeCsvFile(db, filePath, columns, query, csvBom); } if (format === 'json') { - return writeJsonFile(db, filePath, columns, section.category, selectedIds); + return writeJsonFile(db, filePath, columns, query); } - return writeXmlFile(db, filePath, columns, section.category, selectedIds, section.label); + return writeXmlFile(db, filePath, columns, query, section.label); }; if (singleFlat) { diff --git a/packages/core/src/url-utils.ts b/packages/core/src/url-utils.ts index 4d31344..6266ecd 100644 --- a/packages/core/src/url-utils.ts +++ b/packages/core/src/url-utils.ts @@ -118,6 +118,15 @@ export function normalizeUrl( const decoded = raw.replace(/&(?:amp|#0*38|#x0*26);/gi, '&'); const u = new URL(decoded, base); u.hash = ''; + // Drop HTTP-auth userinfo. `https://user:pass@host` is a common way to + // crawl a Basic-auth staging site, but the credentials must not survive + // into `urls.url` — which is logged, stored in the .seoproject, and + // written to every CSV/JSON/XML export. Stripping here covers the start + // URL and any link that carries userinfo. (The crawler still applies + // `config.auth` on the wire; this only sanitises the stored/displayed + // form.) + u.username = ''; + u.password = ''; const keep = rewrites.keepQueryParams; if (keep && keep.length > 0) { @@ -329,6 +338,43 @@ export function isInScope( } } +/** + * True when a URL's host is a loopback / private / link-local address (or + * the localhost name). Used to stop the external-link and image probes from + * fetching internal addresses on the crawl host's behalf — a crawled page + * containing `http://169.254.169.254/…` or `http://192.168.1.1/admin` would + * otherwise be fetched and its status/response-time recorded (SSRF / internal + * port scan). Callers skip the probe unless the START host is itself private + * (an intranet crawl legitimately targets private space). + */ +export function isPrivateHost(url: string): boolean { + let host: string; + try { + host = new URL(url).hostname.toLowerCase(); + } catch { + return false; + } + if (host === 'localhost' || host.endsWith('.localhost') || host.endsWith('.local')) { + return true; + } + // IPv6 loopback / unique-local (fc00::/7) / link-local (fe80::/10). + if (host === '[::1]' || host === '::1') return true; + const v6 = host.replace(/^\[|\]$/g, ''); + if (/^f[cd][0-9a-f]{2}:/i.test(v6) || /^fe[89ab][0-9a-f]:/i.test(v6)) return true; + // IPv4 ranges. + const m = /^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/.exec(host); + if (!m) return false; + const a = Number(m[1]); + const b = Number(m[2]); + if (a === 10) return true; // 10.0.0.0/8 + if (a === 127) return true; // 127.0.0.0/8 loopback + if (a === 169 && b === 254) return true; // 169.254.0.0/16 link-local (cloud metadata) + if (a === 172 && b >= 16 && b <= 31) return true; // 172.16.0.0/12 + if (a === 192 && b === 168) return true; // 192.168.0.0/16 + if (a === 0) return true; // 0.0.0.0/8 + return false; +} + export function extractExtension(url: string): string { try { const u = new URL(url); diff --git a/packages/core/src/xml-export.ts b/packages/core/src/xml-export.ts index 724deeb..c3627f4 100644 --- a/packages/core/src/xml-export.ts +++ b/packages/core/src/xml-export.ts @@ -166,18 +166,25 @@ const XML_COLUMNS: (keyof CrawlUrlRow)[] = [ 'clusterSize', ]; +/** Drop the C0 control characters XML 1.0 forbids outright — everything + * below 0x20 except tab (0x09), LF (0x0A) and CR (0x0D). Escaping does + * not legalise these, and CDATA does not exempt them, so both text paths + * have to strip. */ +export function stripXmlForbidden(raw: string): string { + // eslint-disable-next-line no-control-regex -- intentionally matching control chars to strip them + return raw.replace(/[\x00-\x08\x0B\x0C\x0E-\x1F]/g, ''); +} + /** Escape characters that are invalid inside XML element text bodies. */ function xmlEscape(raw: string): string { - return raw - .replace(/&/g, '&') - .replace(/</g, '<') - .replace(/>/g, '>') - .replace(/"/g, '"') - .replace(/'/g, ''') - // XML 1.0 forbids most C0 control characters; strip everything below - // 0x20 except tab (0x09), LF (0x0A), and CR (0x0D). - // eslint-disable-next-line no-control-regex -- intentionally matching control chars to strip them - .replace(/[\x00-\x08\x0B\x0C\x0E-\x1F]/g, ''); + return stripXmlForbidden( + raw + .replace(/&/g, '&') + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/'/g, '''), + ); } function renderField(key: keyof CrawlUrlRow, value: unknown): string { @@ -197,7 +204,13 @@ function renderField(key: keyof CrawlUrlRow, value: unknown): string { if (/[<>&]/.test(s) && s.length > 200) { // Long values likely contain HTML — CDATA preserves exact bytes. // We split on `]]>` to be safe; standard SAX/DOM parsers reassemble. - const safe = s.replace(/\]\]>/g, ']]]]><![CDATA[>'); + // + // The control-char strip still has to run: CDATA exempts content from + // markup parsing, NOT from XML 1.0's legal-character set. A single + // forbidden byte inside a CDATA block makes the whole document + // unparseable — and unlike a markup error it isn't visible on + // inspection, so the file looks fine until a parser rejects all of it. + const safe = stripXmlForbidden(s).replace(/\]\]>/g, ']]]]><![CDATA[>'); return `<${key}><![CDATA[${safe}]]></${key}>`; } return `<${key}>${xmlEscape(s)}</${key}>`; diff --git a/packages/db/src/migrations.ts b/packages/db/src/migrations.ts index b3f656e..31669b1 100644 --- a/packages/db/src/migrations.ts +++ b/packages/db/src/migrations.ts @@ -2162,6 +2162,28 @@ const MIGRATIONS: Migration[] = [ ); }, }, + { + version: 86, + name: 'add_links_anchor_index', + // The Visualization tab's anchor-text panel runs + // `GROUP BY anchor` over `links` filtered on `is_internal = 1`. The + // table had indexes on `from_url_id`, `to_url` and `(to_url, + // is_internal)` — nothing on `anchor` — so the grouping fell back to a + // full scan plus a temp B-tree sort every time the tab was opened (and + // again on every `dataVersion` bump). `LIMIT 200` bounds the rows + // returned, not the work done. + // + // Partial index: rows with no anchor text are excluded by the same + // predicate the query uses, which keeps the index small on sites whose + // internal links are mostly images. + up: (db) => { + db.exec( + `CREATE INDEX IF NOT EXISTS idx_links_anchor + ON links(anchor) + WHERE is_internal = 1 AND anchor IS NOT NULL AND anchor != ''`, + ); + }, + }, ]; export function runMigrations(db: DatabaseSync): void { diff --git a/packages/db/src/project-db.ts b/packages/db/src/project-db.ts index 14d3de8..b9f8080 100644 --- a/packages/db/src/project-db.ts +++ b/packages/db/src/project-db.ts @@ -780,6 +780,20 @@ const DUPLICATE_ALT_PREDICATE = `alt IS NOT NULL AND alt != '' AND alt IN ( SELECT alt FROM images WHERE alt IS NOT NULL AND alt != '' GROUP BY alt HAVING COUNT(*) > 1 )`; +// Per-usage duplicate-alt: a usage whose alt text is shared by >1 usage. +// Operates on `image_usages.alt` to match the per-usage Images view. +const USAGE_DUPLICATE_ALT_PREDICATE = `iu.alt IS NOT NULL AND iu.alt != '' AND iu.alt IN ( + SELECT alt FROM image_usages WHERE alt IS NOT NULL AND alt != '' GROUP BY alt HAVING COUNT(*) > 1 +)`; + +// Missing / empty alt are computed from PER-USAGE truth (`image_usages.alt`): +// the Images tab, its sidebar counters and the CSV export are all per-usage +// (one row per image+page), so `images.alt` — a lossy first-non-null-wins +// dedup across the whole site — is never consulted for alt correctness. See +// `queryImages`, `iterateImages`, and the `imageMissingAlt`/`imageEmptyAlt` +// counters. Legacy projects with no `image_usages` rows fall back to the +// distinct-image `images.alt` inline in those sites. + /** * Scope shared by every issue counter: only crawled internal HTML pages * are eligible. Kept module-level because `runOverviewBatch` splits the @@ -817,6 +831,39 @@ const INDEXABLE_HTML_SCOPE = * or another row's canonical. Use bare `canonical` only to test whether the * page declared one at all, or to inspect its literal form. */ +/** + * "The URL contains an uppercase letter" — excluding percent-escapes. + * + * `normalizeUrl` uppercases escape hex digits per RFC 3986 §6.2.2.1, so the + * stored form of a perfectly lowercase Turkish slug is + * `/%C3%BCr%C3%BCnler/kad%C4%B1n-ayakkab%C4%B1`. A plain `GLOB '*[A-Z]*'` + * matched those escapes and flagged every non-ASCII URL on the site — so on + * a Turkish, German or Japanese site the "URL Uppercase" issue fired on + * essentially every page while none of them contain an uppercase letter. + * + * SQLite has no percent-decode and GLOB has no lookbehind, so this splits + * the two cases: + * - no `%` in the URL → the original check is exactly right; + * - otherwise → only G-Z, letters that can never be a hex digit. + * + * That removes the false positives completely. The residual is a false + * NEGATIVE for a URL whose only uppercase letters are A-F *and* which also + * contains an escape (`/ABC/%C3%BC`), which is rare and far less harmful + * than flagging every localized URL on the site. Fixing that last case + * exactly needs a column computed at write time. + */ +const URL_HAS_UPPERCASE = + "(CASE WHEN url GLOB '*[%]*' THEN url GLOB '*[G-Z]*' ELSE url GLOB '*[A-Z]*' END)"; + +// "URL contains a non-ASCII character." `normalizeUrl` percent-encodes the +// path and punycodes the host, so the stored `url` is always pure ASCII — +// the old `LENGTH(CAST(url AS BLOB)) != LENGTH(url)` byte-vs-char test could +// therefore never fire. A percent-escaped non-ASCII byte is `%` followed by +// a hex pair whose first nibble is 8–F (byte >= 0x80: UTF-8 lead/continuation +// bytes), which never occurs for percent-encoded ASCII. That is exactly the +// signal we want. +const URL_HAS_NON_ASCII = "url GLOB '*%[89ABCDEFabcdef][0-9A-Fa-f]*'"; + const CANONICAL_CMP = "COALESCE(NULLIF(canonical_resolved, ''), canonical)"; /** Same value, qualified — for use inside a correlated subquery. */ const CANONICAL_CMP_Q = @@ -1653,6 +1700,7 @@ export class ProjectDb { contentType?: string | null; contentLength?: number | null; responseTimeMs?: number | null; + redirectTarget?: string | null; }, ): void { // Re-classify the external stub's content_kind from the probed @@ -1694,7 +1742,8 @@ export class ProjectDb { content_length = :content_length, response_time_ms = :response_time_ms, indexability = :indexability, - indexability_reason = :indexability_reason + indexability_reason = :indexability_reason, + redirect_target = COALESCE(:redirect_target, redirect_target) WHERE url = :url AND is_external = 1`, ) .run({ @@ -1707,6 +1756,7 @@ export class ProjectDb { response_time_ms: patch.responseTimeMs ?? null, indexability, indexability_reason: indexabilityReason, + redirect_target: patch.redirectTarget ?? null, }); } @@ -2396,6 +2446,22 @@ export class ProjectDb { this.db.exec('BEGIN'); try { + // Clear stale results first. This pass only UPDATEs rows that are + // currently 3xx, so a URL that WAS a redirect (or a loop) but now + // returns 200 on re-crawl kept its old redirect_loop / chain_length / + // final_url — and `issues:redirect-loop` has no status gate, so the + // fixed URL still showed up as a loop forever. Reset every non-3xx + // row to the "no redirect" state before recomputing the live ones. + this.db.exec( + `UPDATE urls + SET redirect_chain_length = 0, + redirect_final_url = NULL, + redirect_loop = 0 + WHERE (status_code < 300 OR status_code >= 400 OR status_code IS NULL) + AND (redirect_chain_length != 0 + OR redirect_final_url IS NOT NULL + OR redirect_loop != 0)`, + ); for (const row of redirects) { const visited = new Set<string>(); let current: string | null = row.url; @@ -2474,11 +2540,20 @@ export class ProjectDb { // in `urls`) and self-loops are dropped. const outTargets: number[][] = Array.from({ length: n }, () => []); let edgeCount = 0; + // Streamed, not `.all()`. This runs automatically at the end of every + // crawl (`analyseLinkScore` defaults on), and materialising the whole + // edge set first meant one `{source, to_url}` object per unique internal + // link — each holding a distinct URL string — before a single edge was + // consumed. On a large site that is multi-GB against Electron's 4 GB + // heap cap, and an OOM here killed the writer worker, which then + // escalated to the main process via the fallback in `runDbPassViaPool`. + // Iterating keeps one row in the JS heap; SQLite does the DISTINCT + // dedup in its own (disk-backed) temp B-tree. const edgeRows = this.db .prepare( 'SELECT DISTINCT from_url_id AS source, to_url FROM links WHERE is_internal = 1', ) - .all() as { source: number; to_url: string }[]; + .iterate() as Iterable<{ source: number; to_url: string }>; for (const e of edgeRows) { const si = indexById.get(e.source); if (si === undefined) continue; @@ -5147,6 +5222,17 @@ export class ProjectDb { }; } + /** True when the project has any per-usage image rows. Current crawls + * always populate `image_usages`; only pre-per-usage legacy projects + * don't, and those fall back to the deduped per-image view. */ + private hasImageUsages(): boolean { + return ( + (this.db.prepare('SELECT EXISTS(SELECT 1 FROM image_usages) AS e').get() as { + e: number; + }).e === 1 + ); + } + queryImages(params: { limit: number; offset: number; @@ -5156,20 +5242,68 @@ export class ProjectDb { duplicateAltOnly?: boolean; internalOnly?: boolean; }): { rows: ImageRow[]; total: number } { + // Per-usage view: one row per (image, page) so alt / missing / empty is + // accurate per page. An image with alt on one page and none on another + // becomes two rows — the alt'd one is NOT in Missing Alt, the bare one + // IS — which is what the sidebar count reflects too. Legacy projects + // with no `image_usages` fall back to the deduped per-image path below. + if (this.hasImageUsages()) { + const where: string[] = []; + const args: (string | number)[] = []; + if (params.internalOnly) where.push('i.is_internal = 1'); + if (params.missingAltOnly) where.push('iu.alt IS NULL'); + if (params.emptyAltOnly) where.push("iu.alt = ''"); + if (params.duplicateAltOnly) where.push(USAGE_DUPLICATE_ALT_PREDICATE); + if (params.search) { + where.push('(i.src LIKE ? OR iu.alt LIKE ?)'); + const like = `%${params.search}%`; + args.push(like, like); + } + const whereSql = where.length ? `WHERE ${where.join(' AND ')}` : ''; + const total = ( + this.db + .prepare( + `SELECT COUNT(*) AS c + FROM image_usages iu JOIN images i ON iu.image_id = i.id ${whereSql}`, + ) + .get(...args) as { c: number } + ).c; + const rows = this.db + .prepare( + `SELECT i.id, i.src, iu.alt AS alt, i.width, i.height, i.is_internal, + i.occurrences, u.url AS from_url + FROM image_usages iu + JOIN images i ON iu.image_id = i.id + LEFT JOIN urls u ON iu.from_url_id = u.id + ${whereSql} + ORDER BY i.occurrences DESC, i.id, iu.from_url_id + LIMIT ? OFFSET ?`, + ) + .all(...args, params.limit, params.offset) as unknown as (ImageRowDb & { + from_url: string | null; + })[]; + return { + total, + rows: rows.map((r) => ({ + id: r.id, + src: r.src, + alt: r.alt, + width: r.width, + height: r.height, + isInternal: r.is_internal === 1, + occurrences: r.occurrences, + fromUrl: r.from_url ?? null, + })), + }; + } + + // Legacy fallback — per distinct image (no usage rows to expand). const where: string[] = []; const args: (string | number)[] = []; - if (params.internalOnly) { - where.push('is_internal = 1'); - } - if (params.missingAltOnly) { - where.push('alt IS NULL'); - } - if (params.emptyAltOnly) { - where.push("alt = ''"); - } - if (params.duplicateAltOnly) { - where.push(DUPLICATE_ALT_PREDICATE); - } + if (params.internalOnly) where.push('is_internal = 1'); + if (params.missingAltOnly) where.push('alt IS NULL'); + if (params.emptyAltOnly) where.push("alt = ''"); + if (params.duplicateAltOnly) where.push(DUPLICATE_ALT_PREDICATE); if (params.search) { where.push('(src LIKE ? OR alt LIKE ?)'); const like = `%${params.search}%`; @@ -5196,6 +5330,7 @@ export class ProjectDb { height: r.height, isInternal: r.is_internal === 1, occurrences: r.occurrences, + fromUrl: null, })), }; } @@ -5565,7 +5700,7 @@ export class ProjectDb { noindexCanonicalConflict: countWhere( `${html} AND canonical IS NOT NULL AND canonical != '' AND ${CANONICAL_CMP} != url - AND (meta_robots LIKE '%noindex%' OR x_robots_tag LIKE '%noindex%')`, + AND (meta_robots LIKE '%noindex%' OR meta_robots LIKE '%none%' OR x_robots_tag LIKE '%noindex%' OR x_robots_tag LIKE '%none%')`, ), redirectToNoindex: countWhere( `${html} AND status_code >= 300 AND status_code < 400 @@ -5597,7 +5732,7 @@ export class ProjectDb { AND t.indexability = 'non-indexable:noindex' )`, ), - contentThin: countWhere(`${html} AND word_count IS NOT NULL AND word_count < 300`), + contentThin: countWhere(`${indexable} AND word_count IS NOT NULL AND word_count < 300`), // Only a completed check yields a finding count — an unsupported or // language-mismatched page stores zero because nothing was collected. // Stating `status = 'ok'` keeps that explicit rather than leaning on @@ -5612,12 +5747,12 @@ export class ProjectDb { responseVerySlow: countWhere(`${html} AND response_time_ms > 3000`), pageLarge: countWhere(`${html} AND content_length > 1048576`), urlTooLong: countWhere(`${html} AND LENGTH(url) > 2048`), - urlUppercase: countWhere(`${html} AND url GLOB '*[A-Z]*'`), + urlUppercase: countWhere(`${html} AND ${URL_HAS_UPPERCASE}`), urlUnderscore: countWhere(`${html} AND INSTR(url, '_') > 0`), urlMultipleSlashes: countWhere( `${html} AND INSTR(SUBSTR(url, INSTR(url, '://') + 3), '//') > 0`, ), - urlNonAscii: countWhere(`${html} AND LENGTH(CAST(url AS BLOB)) != LENGTH(url)`), + urlNonAscii: countWhere(`${html} AND ${URL_HAS_NON_ASCII}`), langMissing: countWhere(`${indexable} AND (lang IS NULL OR lang = '')`), viewportMissing: countWhere(`${indexable} AND (viewport IS NULL OR viewport = '')`), ogMissing: countWhere( @@ -5670,7 +5805,7 @@ export class ProjectDb { mixedContentPassive: countWhere( `${html} AND url LIKE 'https://%' AND mixed_content_passive > 0`, ), - faviconMissing: countWhere(`${html} AND (favicon IS NULL OR favicon = '')`), + faviconMissing: countWhere(`${indexable} AND (favicon IS NULL OR favicon = '')`), redirectLoop: countWhere(`${html} AND redirect_loop = 1`), redirectChainLong: countWhere(`${html} AND redirect_chain_length > 3`), redirectSelf: countWhere( @@ -5692,7 +5827,11 @@ export class ProjectDb { AND EXISTS (SELECT 1 FROM sitemap_urls s WHERE s.url = urls.url)`, ), imageMissingAlt: this.scalarCount( - 'SELECT COUNT(*) AS c FROM images WHERE alt IS NULL', + // Per-usage to match the per-usage Images tab (count = table). Legacy + // projects with no usage rows fall back to the distinct-image count. + `SELECT (CASE WHEN EXISTS(SELECT 1 FROM image_usages) + THEN (SELECT COUNT(*) FROM image_usages WHERE alt IS NULL) + ELSE (SELECT COUNT(*) FROM images WHERE alt IS NULL) END) AS c`, ), // Distinct images sharing a non-empty alt with ≥1 other image — // non-descriptive / templated alt reuse. Counted image-level (the @@ -5768,7 +5907,9 @@ export class ProjectDb { // images with an explicit `alt=""` so it's the same unit as Missing // Alt and matches the Images tab drill-down. imageEmptyAlt: this.scalarCount( - "SELECT COUNT(*) AS c FROM images WHERE alt = ''", + `SELECT (CASE WHEN EXISTS(SELECT 1 FROM image_usages) + THEN (SELECT COUNT(*) FROM image_usages WHERE alt = '') + ELSE (SELECT COUNT(*) FROM images WHERE alt = '') END) AS c`, ), linkEmptyAnchor: countWhere(`${html} AND empty_anchor_count > 0`), appleTouchIconMissing: countWhere( @@ -5814,7 +5955,7 @@ export class ProjectDb { LOWER(title) IN ('untitled', 'untitled document', 'default title', 'new page', 'page', 'home', 'index', 'document', 'welcome', 'untitled-1', 'untitled 1', 'home page') - OR LOWER(title) LIKE 'page %' + OR LOWER(title) GLOB 'page [0-9]*' OR LOWER(title) LIKE 'untitled%' )`, ), @@ -6404,6 +6545,15 @@ export class ProjectDb { if (seen.has(name)) continue; seen.add(name); let value = rawValue ?? ''; + // Never persist the VALUE of secret headers. A logged-in crawl's + // `set-cookie` (session tokens) or an `authorization` echo would + // otherwise land in the .seoproject (a file users email around), the + // URL Details panel, and any connected MCP agent's reach. The header's + // presence is still recorded (name kept); `analyseCookies` derives the + // Secure/HttpOnly/SameSite flags the UI needs from its own pass. + if (name === 'set-cookie' || name === 'authorization' || name === 'proxy-authorization') { + value = '[redacted]'; + } if (value.length > 4096) value = value.slice(0, 4093) + '...'; list.push({ name, value }); } @@ -6723,10 +6873,14 @@ export class ProjectDb { } *iterateAllUrls(): IterableIterator<CrawlUrlRow> { - const rows = this.db.prepare('SELECT * FROM urls ORDER BY id').all() as unknown as UrlRowDb[]; - for (const row of rows) { - yield this.rowFromDb(row); - } + // Delegates to the keyset-paginated walker rather than repeating a + // single `.all()` here. This looked like a streamer but wasn't: it + // materialised every row of a ~190-column table before yielding the + // first one, so a full-table CSV/JSON/XML export on a large crawl + // could exhaust Electron's 4 GB heap cap and take the process with + // it. `categoryWhereClause('all')` is null, so the row set and order + // are identical — only the memory profile changes. + yield* this.iterateUrlsByCategory('all'); } /** @@ -6750,6 +6904,44 @@ export class ProjectDb { } = {}): IterableIterator<ImageRow> { const where: string[] = []; const args: (string | number)[] = []; + // Per-usage export to match the per-usage Images tab (one row per page). + if (this.hasImageUsages()) { + if (options.missingAltOnly) where.push('iu.alt IS NULL'); + if (options.emptyAltOnly) where.push("iu.alt = ''"); + if (options.duplicateAltOnly) where.push(USAGE_DUPLICATE_ALT_PREDICATE); + if (options.search) { + where.push('(i.src LIKE ? OR iu.alt LIKE ?)'); + const like = `%${options.search}%`; + args.push(like, like); + } + const whereSql = where.length ? `WHERE ${where.join(' AND ')}` : ''; + const rows = this.db + .prepare( + `SELECT i.id, i.src, iu.alt AS alt, i.width, i.height, i.is_internal, + i.occurrences, u.url AS from_url + FROM image_usages iu + JOIN images i ON iu.image_id = i.id + LEFT JOIN urls u ON iu.from_url_id = u.id + ${whereSql} + ORDER BY i.occurrences DESC, i.id, iu.from_url_id`, + ) + .all(...args) as unknown as (ImageRowDb & { from_url: string | null })[]; + for (const r of rows) { + yield { + id: r.id, + src: r.src, + alt: r.alt, + width: r.width, + height: r.height, + isInternal: r.is_internal === 1, + occurrences: r.occurrences, + fromUrl: r.from_url ?? null, + }; + } + return; + } + + // Legacy fallback — per distinct image. if (options.missingAltOnly) where.push('alt IS NULL'); if (options.emptyAltOnly) where.push("alt = ''"); if (options.duplicateAltOnly) where.push(DUPLICATE_ALT_PREDICATE); @@ -6760,9 +6952,7 @@ export class ProjectDb { } const whereSql = where.length ? `WHERE ${where.join(' AND ')}` : ''; const rows = this.db - .prepare( - `SELECT * FROM images ${whereSql} ORDER BY occurrences DESC, id`, - ) + .prepare(`SELECT * FROM images ${whereSql} ORDER BY occurrences DESC, id`) .all(...args) as unknown as ImageRowDb[]; for (const r of rows) { yield { @@ -6773,16 +6963,26 @@ export class ProjectDb { height: r.height, isInternal: r.is_internal === 1, occurrences: r.occurrences, + fromUrl: null, }; } } *iterateBrokenLinks( internal: 'all' | 'internal' | 'external' = 'all', + search?: string, ): IterableIterator<BrokenLinkRow> { const where: string[] = ['t.status_code >= 400 AND t.status_code < 600']; if (internal === 'internal') where.push('l.is_internal = 1'); else if (internal === 'external') where.push('l.is_internal = 0'); + // Same search predicate as `queryBrokenLinks` so the export mirrors the + // filtered Broken Links grid rather than the whole crawl. + const args: string[] = []; + if (search) { + where.push('(f.url LIKE ? OR l.to_url LIKE ?)'); + const like = `%${search}%`; + args.push(like, like); + } const rows = this.db .prepare( `SELECT f.url AS from_url, f.status_code AS from_status, @@ -6794,7 +6994,12 @@ export class ProjectDb { WHERE ${where.join(' AND ')} ORDER BY t.status_code DESC, f.id, l.id`, ) - .all() as unknown as { + // Streamed, not `.all()`. This is a generator by signature but used + // to materialise everything first, and the row count here is + // broken_targets × linking_pages — a single dead link in a site-wide + // footer produces one row per page, so a large site turns one broken + // URL into hundreds of thousands of rows held in memory at once. + .iterate(...args) as unknown as Iterable<{ from_url: string; from_status: number | null; to_url: string; @@ -6802,7 +7007,7 @@ export class ProjectDb { anchor: string | null; rel: string | null; is_internal: number; - }[]; + }>; for (const r of rows) { yield { fromUrl: r.from_url, @@ -6866,6 +7071,37 @@ export class ProjectDb { } } + /** + * Yield every URL row matching a full table query — category + search + + * advanced filter — via the SAME `buildUrlsWhere` the on-screen table + * uses, keyset-paginated so it streams. The category-only + * `iterateUrlsByCategory` made the export ignore the active search box and + * advanced filter, so an export silently contained different rows than the + * user was looking at. Use this for any export that must match the grid. + */ + *iterateUrlsByQuery(params: { + category?: UrlCategory; + search?: string; + filter?: AdvancedFilter; + }): IterableIterator<CrawlUrlRow> { + const { whereSql, args: baseArgs } = buildUrlsWhere(params); + const BATCH = 2000; + const sql = whereSql + ? `SELECT * FROM urls ${whereSql} AND id > ? ORDER BY id LIMIT ?` + : 'SELECT * FROM urls WHERE id > ? ORDER BY id LIMIT ?'; + const stmt = this.db.prepare(sql); + let lastId = 0; + for (;;) { + const rows = stmt.all(...baseArgs, lastId, BATCH) as unknown as UrlRowDb[]; + if (rows.length === 0) return; + for (const row of rows) { + lastId = row.id; + yield this.rowFromDb(row); + } + if (rows.length < BATCH) return; + } + } + /** * Yields crawled, indexable, internal HTML URLs in depth-then-id order * — the set that belongs in an XML sitemap. Excludes redirects, @@ -8213,10 +8449,14 @@ export class ProjectDb { * `url_sources.body` aggregate would be hundreds of MB and the SEO * signal is well concentrated in the title-meta-h1 trio anyway. * - * Returns an array because the consumer (main process) feeds it into - * `aggregateTopWords` which expects an iterable. + * Streams rather than returning an array. `aggregateTopWords` only ever + * iterates it, and the previous version ran `.all()` over the whole + * matching table and then built a SECOND full-size `string[]` while the + * rows were still live — two complete copies of the corpus on the main + * thread (this handler doesn't use the reader pool). Yielding keeps one + * row alive at a time and leaves the counting cost unchanged. */ - seoTextCorpus(): string[] { + *seoTextCorpus(): IterableIterator<string> { const rows = this.db .prepare( `SELECT title, meta_description, h1 @@ -8226,16 +8466,18 @@ export class ProjectDb { AND status_code BETWEEN 200 AND 299 AND indexability = 'indexable'`, ) - .all() as { title: string | null; meta_description: string | null; h1: string | null }[]; - const out: string[] = []; + .iterate() as Iterable<{ + title: string | null; + meta_description: string | null; + h1: string | null; + }>; for (const r of rows) { const parts: string[] = []; if (r.title) parts.push(r.title); if (r.meta_description) parts.push(r.meta_description); if (r.h1) parts.push(r.h1); - if (parts.length > 0) out.push(parts.join(' ')); + if (parts.length > 0) yield parts.join(' '); } - return out; } /** @@ -9182,7 +9424,7 @@ function categoryWhereClause(cat: UrlCategory): string | null { return `is_external = 0 AND content_kind = 'html' AND canonical IS NOT NULL AND canonical != '' AND ${CANONICAL_CMP} != url - AND (meta_robots LIKE '%noindex%' OR x_robots_tag LIKE '%noindex%')`; + AND (meta_robots LIKE '%noindex%' OR meta_robots LIKE '%none%' OR x_robots_tag LIKE '%noindex%' OR x_robots_tag LIKE '%none%')`; case 'issues:redirect-to-noindex': // A redirect whose final destination is noindexed — the redirect // spends its link equity on a page that will never be indexed. @@ -9261,7 +9503,9 @@ function categoryWhereClause(cat: UrlCategory): string | null { AND u2.url != urls.url )`; case 'issues:content-thin': - return "is_external = 0 AND content_kind = 'html' AND word_count IS NOT NULL AND word_count < 300"; + // 2xx only — a text/html 404 template is short by design and was + // being counted as thin content. + return `${INDEXABLE_HTML_SCOPE} AND word_count IS NOT NULL AND word_count < 300`; case 'issues:spelling-grammar': return `is_external = 0 AND content_kind = 'html' AND EXISTS ( SELECT 1 FROM spelling_results s @@ -9277,7 +9521,7 @@ function categoryWhereClause(cat: UrlCategory): string | null { return "is_external = 0 AND content_kind = 'html' AND LENGTH(url) > 2048"; case 'issues:url-uppercase': // GLOB with [A-Z] is case-sensitive — unlike LIKE which isn't. - return "is_external = 0 AND content_kind = 'html' AND url GLOB '*[A-Z]*'"; + return `is_external = 0 AND content_kind = 'html' AND ${URL_HAS_UPPERCASE}`; case 'issues:url-underscore': return "is_external = 0 AND content_kind = 'html' AND INSTR(url, '_') > 0"; case 'issues:url-multiple-slashes': @@ -9287,7 +9531,7 @@ function categoryWhereClause(cat: UrlCategory): string | null { case 'issues:url-non-ascii': // Byte-length (BLOB cast) > character length only when the string // contains multi-byte UTF-8, i.e. any non-ASCII code point. - return "is_external = 0 AND content_kind = 'html' AND LENGTH(CAST(url AS BLOB)) != LENGTH(url)"; + return `is_external = 0 AND content_kind = 'html' AND ${URL_HAS_NON_ASCII}`; case 'issues:lang-missing': return `is_external = 0 AND content_kind = 'html' AND status_code >= 200 AND status_code < 300 @@ -9365,7 +9609,10 @@ function categoryWhereClause(cat: UrlCategory): string | null { return `is_external = 0 AND content_kind = 'html' AND url LIKE 'https://%' AND mixed_content_count > 0`; case 'issues:favicon-missing': - return `is_external = 0 AND content_kind = 'html' + // 2xx only — a 3xx redirect or a text/html error page carries no + // favicon by nature, so scoping to INTERNAL_HTML alone reported every + // redirect and error template as "favicon missing". + return `${INDEXABLE_HTML_SCOPE} AND (favicon IS NULL OR favicon = '')`; case 'issues:redirect-loop': return "is_external = 0 AND content_kind = 'html' AND redirect_loop = 1"; @@ -9425,12 +9672,17 @@ function categoryWhereClause(cat: UrlCategory): string | null { AND amp_validation_errors IS NOT NULL AND amp_validation_errors != '' AND amp_validation_errors != '[]'`; - // Broken-link categories drive the BrokenLinksTab view; they never - // filter the URL table itself. + // Broken-link categories drive the BrokenLinksTab view (via + // `queryBrokenLinks`); they describe LINK rows, not URL rows, so they + // can't be expressed as a `urls` predicate. Returning null used to mean + // "no clause" — so when the Links tab quick-filter offered "Broken + // Internal" it dropped the filter entirely and displayed the WHOLE + // crawl under that label. Match nothing instead: an empty table is + // obviously not "every URL is a broken link". case 'issues:broken-links-all': case 'issues:broken-links-internal': case 'issues:broken-links-external': - return null; + return '1 = 0'; case 'issues:near-duplicate': // SimHash cluster size > 1 means at least one other crawled page // landed within the configured Hamming-distance threshold of this @@ -9656,7 +9908,7 @@ function categoryWhereClause(cat: UrlCategory): string | null { LOWER(title) IN ('untitled', 'untitled document', 'default title', 'new page', 'page', 'home', 'index', 'document', 'welcome', 'untitled-1', 'untitled 1', 'home page') - OR LOWER(title) LIKE 'page %' + OR LOWER(title) GLOB 'page [0-9]*' OR LOWER(title) LIKE 'untitled%' )`; case 'issues:analytics-missing': diff --git a/packages/shared-types/src/crawl.ts b/packages/shared-types/src/crawl.ts index b9d1465..72f090d 100644 --- a/packages/shared-types/src/crawl.ts +++ b/packages/shared-types/src/crawl.ts @@ -1441,6 +1441,11 @@ export interface BrowserLoginConfig { successSelector?: string; /** Extra settle time after submit, ms (for slow SPA redirects). */ waitMs?: number; + /** Opt-in: accept invalid/self-signed TLS certificates on the login + * page. OFF by default — typing real credentials into a page whose + * certificate can't be verified is a MITM risk, so the user must + * explicitly allow it (e.g. for an internal staging host). */ + allowInsecureTls?: boolean; } export interface FormLoginConfig { @@ -2056,11 +2061,19 @@ export interface DiscoveredResource { export interface ImageRow { id: number; src: string; + /** The alt for THIS usage (this page). The Images tab is per-usage: the + * same image on two pages is two rows, so alt/missing/empty is accurate + * per page rather than a lossy site-wide dedup. */ alt: string | null; width: number | null; height: number | null; isInternal: boolean; + /** Total pages this image appears on (same for every usage row of it). */ occurrences: number; + /** The page this usage is on. Null only for legacy projects with no + * `image_usages` rows (pre-per-usage crawls), which fall back to one + * row per distinct image. */ + fromUrl: string | null; } /** Columns that the Advanced Filter dialog exposes for querying. */ diff --git a/packages/shared-types/src/ipc.ts b/packages/shared-types/src/ipc.ts index 740427f..e2d4a27 100644 --- a/packages/shared-types/src/ipc.ts +++ b/packages/shared-types/src/ipc.ts @@ -572,6 +572,9 @@ export interface ExportBrokenLinksInput { filePath?: string; /** Scope filter — matches the Broken Links tab's sidebar filter. */ internal?: 'all' | 'internal' | 'external'; + /** Search term — matches the tab's search box so the export mirrors the + * filtered grid rather than the whole crawl. */ + search?: string; } export interface ExportBrokenLinksResult { @@ -632,6 +635,12 @@ export interface ExportTabularInput { filePath?: string; /** When set, restrict every section to these row ids. */ selectedIds?: number[]; + /** Active URL-table search term — applied to every section so the export + * mirrors the filtered grid. Ignored when `selectedIds` is set. */ + search?: string; + /** Active advanced filter — applied to every section, same rationale as + * `search`. Ignored when `selectedIds` is set. */ + filter?: AdvancedFilter; /** When true, CSV files are written with a UTF-8 BOM so Excel for Windows * opens them in the correct charset. Ignored for non-CSV formats. */ csvBom?: boolean; diff --git a/tests/agent-session-registry.test.ts b/tests/agent-session-registry.test.ts deleted file mode 100644 index 7df5dfd..0000000 --- a/tests/agent-session-registry.test.ts +++ /dev/null @@ -1,184 +0,0 @@ -import { describe, it, expect } from 'vitest'; -import { - SessionRegistry, - computeGlobalThrottleScale, -} from '../apps/desktop/src/main/agent-session-registry.js'; - -/** - * Deterministic registry: injected clock + id factory + fake timers so queue - * timeout / promotion ordering is fully controllable. - */ -function makeRegistry() { - let id = 0; - const state = { clock: 1000 }; - const timers: Array<{ fn: () => void; handle: symbol }> = []; - const reg = new SessionRegistry({ - genCrawlId: () => `cr_${++id}`, - now: () => state.clock, - setTimer: (fn) => { - const handle = Symbol(); - timers.push({ fn, handle }); - return handle; - }, - clearTimer: (h) => { - const i = timers.findIndex((t) => t.handle === h); - if (i !== -1) timers.splice(i, 1); - }, - maxQueueDepth: 2, - }); - return { - reg, - advance: (ms: number) => (state.clock += ms), - fireTimers: () => { - const pending = timers.splice(0, timers.length); - for (const t of pending) t.fn(); - }, - }; -} - -describe('SessionRegistry crawl lease', () => { - it('grants an idle slot immediately', async () => { - const { reg } = makeRegistry(); - const g = await reg.acquireCrawl('primary', { clientId: 'A', startUrl: 'x' }); - expect(g.ok).toBe(true); - if (g.ok) { - expect(g.queued).toBe(false); - expect(g.tookOver).toBe(false); - } - expect(reg.peekLease('primary')?.ownerClientId).toBe('A'); - }); - - it('rejects a second start by default (crawl-in-progress)', async () => { - const { reg } = makeRegistry(); - await reg.acquireCrawl('primary', { clientId: 'A', startUrl: 'x' }); - const g = await reg.acquireCrawl('primary', { clientId: 'B', startUrl: 'y' }); - expect(g.ok).toBe(false); - if (!g.ok) expect(g.error).toBe('crawl-in-progress'); - }); - - it('queues and promotes FIFO when the lease releases', async () => { - const { reg, advance } = makeRegistry(); - const a = await reg.acquireCrawl('primary', { clientId: 'A', startUrl: 'x' }); - let bResolved = false; - const bPromise = reg - .acquireCrawl('primary', { clientId: 'B', startUrl: 'y', onBusy: 'queue' }) - .then((v) => { - bResolved = true; - return v; - }); - expect(bResolved).toBe(false); - expect(reg.queueDepth('primary')).toBe(1); - advance(5000); - if (a.ok) reg.releaseCrawl('primary', a.crawlId); - const b = await bPromise; - expect(b.ok).toBe(true); - if (b.ok) { - expect(b.queued).toBe(true); - expect(b.waitedMs).toBe(5000); - } - expect(reg.peekLease('primary')?.ownerClientId).toBe('B'); - }); - - it('rejects with session-limit-reached when the queue is full', async () => { - const { reg } = makeRegistry(); - await reg.acquireCrawl('primary', { clientId: 'A', startUrl: 'x' }); - void reg.acquireCrawl('primary', { clientId: 'B', startUrl: 'y', onBusy: 'queue' }); - void reg.acquireCrawl('primary', { clientId: 'C', startUrl: 'z', onBusy: 'queue' }); - const g = await reg.acquireCrawl('primary', { - clientId: 'D', - startUrl: 'w', - onBusy: 'queue', - }); - expect(g.ok).toBe(false); - if (!g.ok) expect(g.error).toBe('session-limit-reached'); - }); - - it('takeover grants a new lease and the superseded release no-ops', async () => { - const { reg } = makeRegistry(); - const a = await reg.acquireCrawl('primary', { clientId: 'A', startUrl: 'x' }); - const g = await reg.acquireCrawl('primary', { - clientId: 'B', - startUrl: 'y', - onBusy: 'takeover', - }); - expect(g.ok).toBe(true); - if (g.ok && a.ok) { - expect(g.tookOver).toBe(true); - expect(g.crawlId).not.toBe(a.crawlId); - // A's stale release must not free B's lease. - reg.releaseCrawl('primary', a.crawlId); - expect(reg.peekLease('primary')?.ownerClientId).toBe('B'); - } - }); - - it('enforces ownership; a null-owner lease is owned by anyone (legacy)', async () => { - const { reg } = makeRegistry(); - await reg.acquireCrawl('primary', { clientId: 'A', startUrl: 'x' }); - expect(reg.isOwner('primary', 'A')).toBe(true); - expect(reg.isOwner('primary', 'B')).toBe(false); - - const { reg: reg2 } = makeRegistry(); - await reg2.acquireCrawl('primary', { clientId: null, startUrl: 'x' }); - expect(reg2.isOwner('primary', 'anyone')).toBe(true); - }); - - it('times out a queued waiter and removes it', async () => { - const { reg, fireTimers } = makeRegistry(); - const a = await reg.acquireCrawl('primary', { clientId: 'A', startUrl: 'x' }); - const bPromise = reg.acquireCrawl('primary', { - clientId: 'B', - startUrl: 'y', - onBusy: 'queue', - queueTimeoutMs: 1000, - }); - fireTimers(); - const b = await bPromise; - expect(b.ok).toBe(false); - if (!b.ok) expect(b.error).toBe('queue-timeout'); - expect(reg.queueDepth('primary')).toBe(0); - if (a.ok) reg.releaseCrawl('primary', a.crawlId); - expect(reg.peekLease('primary')).toBeNull(); - }); - - it('dropSession expires queued waiters', async () => { - const { reg } = makeRegistry(); - await reg.acquireCrawl('primary', { clientId: 'A', startUrl: 'x' }); - const bPromise = reg.acquireCrawl('primary', { - clientId: 'B', - startUrl: 'y', - onBusy: 'queue', - }); - reg.dropSession('primary'); - const b = await bPromise; - expect(b.ok).toBe(false); - if (!b.ok) expect(b.error).toBe('session-expired'); - expect(reg.peekLease('primary')).toBeNull(); - }); - - it('keeps sessions independent — one lease does not block another', async () => { - const { reg } = makeRegistry(); - const a = await reg.acquireCrawl('sess_1', { clientId: 'A', startUrl: 'x' }); - const b = await reg.acquireCrawl('sess_2', { clientId: 'B', startUrl: 'y' }); - expect(a.ok).toBe(true); - expect(b.ok).toBe(true); - expect(reg.peekLease('sess_1')?.ownerClientId).toBe('A'); - expect(reg.peekLease('sess_2')?.ownerClientId).toBe('B'); - }); -}); - -describe('computeGlobalThrottleScale', () => { - it('returns 1 when the summed concurrency fits the budget', () => { - expect(computeGlobalThrottleScale([10, 10], 32)).toBe(1); - expect(computeGlobalThrottleScale([], 32)).toBe(1); - expect(computeGlobalThrottleScale([32], 32)).toBe(1); - }); - - it('scales down proportionally when over budget', () => { - expect(computeGlobalThrottleScale([40, 40], 40)).toBeCloseTo(0.5, 5); - expect(computeGlobalThrottleScale([30, 30, 30], 45)).toBeCloseTo(0.5, 5); - }); - - it('never drops below the 0.1 floor', () => { - expect(computeGlobalThrottleScale([200, 200], 1)).toBe(0.1); - }); -}); diff --git a/tests/bridge-routing.test.ts b/tests/bridge-routing.test.ts deleted file mode 100644 index 382518b..0000000 --- a/tests/bridge-routing.test.ts +++ /dev/null @@ -1,264 +0,0 @@ -import { describe, it, expect, beforeAll, afterAll } from 'vitest'; -import { mkdtempSync, readFileSync, rmSync } from 'node:fs'; -import { tmpdir } from 'node:os'; -import { join } from 'node:path'; -import { - startMcpBridge, - stopMcpBridge, - type McpBridgeDeps, -} from '../apps/desktop/src/main/mcp-bridge.js'; - -/** - * Exercises the localhost bridge's HTTP layer end-to-end (auth, header-based - * session routing, typed error → status mapping, capabilities, session - * lifecycle dispatch) against fake deps — no Electron, no real crawler. - */ - -let dir: string; -let port: number; -let token: string; -const calls: Array<[string, ...unknown[]]> = []; -const sessions = new Map<string, { label: string; doc: string | null }>(); -let seq = 0; - -const deps: McpBridgeDeps = { - userDataDir: '', // set in beforeAll - getCapabilities: () => ({ - bridgeVersion: 2, - appVersion: '0.10.0', - features: ['crawl-lease', 'crawl-queue', 'sessions'], - limits: { maxHeadlessSessions: 3, maxQueueDepth: 4, sessionIdleTtlMs: 1_800_000 }, - }), - startCrawl: async (input, ctx) => { - calls.push(['start', ctx.sessionId, ctx.clientId, input.onBusy ?? null]); - if (input.onBusy === 'reject') - return { error: 'crawl-in-progress', message: 'busy', details: { sessionId: ctx.sessionId } }; - return { - ok: true, - crawlId: 'cr_x', - sessionId: ctx.sessionId, - config: { startUrl: input.startUrl ?? '' } as never, - queued: false, - waitedMs: 0, - }; - }, - stopCrawl: (ctx, opts) => { - calls.push(['stop', ctx.sessionId, opts.force]); - return { ok: true }; - }, - pauseCrawl: () => ({ ok: true }), - resumeCrawl: () => ({ ok: true }), - clearCrawl: async (ctx, opts) => { - if (ctx.sessionId === 'primary' && !opts.force) - return { error: 'project-locked', message: 'unsaved work', details: {} }; - return { ok: true }; - }, - hasAction: (name) => name === 'export-csv', - runAction: async (name, _input, ctx) => { - calls.push(['action', name, ctx.sessionId]); - return { session: ctx.sessionId }; - }, - getCrawlProgress: (ctx) => ({ - progress: null, - crawlId: null, - sessionId: ctx.sessionId, - ownerClientId: null, - ownedByCaller: false, - }), - getProjectInfo: (ctx) => ({ projectPath: `/p/${ctx.sessionId}`, urlsCrawled: 0 }), - listSessions: () => [ - { - sessionId: 'primary', - label: 'Primary', - kind: 'primary', - documentPath: null, - dbPath: null, - urlsCrawled: 0, - crawl: null, - ownerClientId: null, - createdAt: 0, - lastUsedAt: 0, - }, - ...[...sessions.entries()].map(([id, s]) => ({ - sessionId: id, - label: s.label, - kind: 'headless' as const, - documentPath: s.doc, - dbPath: `/db/${id}`, - urlsCrawled: 0, - crawl: null, - ownerClientId: null, - createdAt: 0, - lastUsedAt: 0, - })), - ], - createSession: async (input) => { - const id = `sess_${++seq}`; - sessions.set(id, { label: input.label ?? `agent-${seq}`, doc: input.projectPath ?? null }); - return { - sessionId: id, - dbPath: `/db/${id}`, - documentPath: input.projectPath ?? null, - created: true, - label: input.label ?? `agent-${seq}`, - }; - }, - closeSession: async (input, ctx) => { - if (!sessions.has(ctx.sessionId)) - return { error: 'session-not-found', message: 'gone', details: {} }; - sessions.delete(ctx.sessionId); - return input.save ? { ok: true, savedTo: input.savePath ?? '/s.seoproject' } : { ok: true }; - }, - saveSession: async (input, ctx) => { - if (!sessions.has(ctx.sessionId)) - return { error: 'session-not-found', message: 'gone', details: {} }; - return { filePath: input.projectPath ?? '/d.seoproject', bytesWritten: 1234 }; - }, - log: () => {}, -}; - -beforeAll(async () => { - dir = mkdtempSync(join(tmpdir(), 'fc-bridge-')); - deps.userDataDir = dir; - const started = await startMcpBridge(deps); - if (!started) throw new Error('bridge failed to start'); - port = started.port; - token = started.token; - const disc = JSON.parse(readFileSync(join(dir, 'mcp-bridge.json'), 'utf8')); - expect(disc.version).toBe(2); -}); - -afterAll(() => { - stopMcpBridge(); - try { - rmSync(dir, { recursive: true, force: true }); - } catch { - /* ignore */ - } -}); - -const base = () => `http://127.0.0.1:${port}`; -async function req( - method: string, - path: string, - body?: unknown, - headers: Record<string, string> = {}, -) { - const r = await fetch(base() + path, { - method, - headers: { - Authorization: `Bearer ${token}`, - 'Content-Type': 'application/json', - ...headers, - }, - body: body !== undefined ? JSON.stringify(body) : undefined, - }); - let json: unknown = null; - try { - json = await r.json(); - } catch { - /* no body */ - } - return { status: r.status, body: json as Record<string, unknown> }; -} - -describe('MCP bridge routing', () => { - it('rejects a bad token with 401', async () => { - const r = await fetch(base() + '/v1/capabilities', { - headers: { Authorization: 'Bearer wrong' }, - }); - expect(r.status).toBe(401); - }); - - it('serves capabilities with the sessions feature', async () => { - const r = await req('GET', '/v1/capabilities'); - expect(r.status).toBe(200); - expect(r.body.bridgeVersion).toBe(2); - expect((r.body.features as string[]).includes('sessions')).toBe(true); - }); - - it('routes a headerless request to the primary session with a null client', async () => { - const r = await req('POST', '/v1/crawl/start', { startUrl: 'x' }); - expect(r.status).toBe(200); - expect(r.body.sessionId).toBe('primary'); - expect(calls.at(-1)).toEqual(['start', 'primary', null, null]); - }); - - it('routes by session + client headers', async () => { - const r = await req( - 'POST', - '/v1/crawl/start', - { startUrl: 'y' }, - { 'x-freecrawl-session': 'sess_9', 'x-freecrawl-client': 'clientA' }, - ); - expect(r.status).toBe(200); - expect(r.body.sessionId).toBe('sess_9'); - expect(calls.at(-1)).toEqual(['start', 'sess_9', 'clientA', null]); - }); - - it('maps crawl-in-progress to 409', async () => { - const r = await req('POST', '/v1/crawl/start', { onBusy: 'reject' }); - expect(r.status).toBe(409); - expect(r.body.error).toBe('crawl-in-progress'); - }); - - it('guards clear on primary with unsaved work (project-locked 409), overridable by force', async () => { - const locked = await req('POST', '/v1/crawl/clear', {}); - expect(locked.status).toBe(409); - expect(locked.body.error).toBe('project-locked'); - const forced = await req('POST', '/v1/crawl/clear', { force: true }); - expect(forced.status).toBe(200); - expect(forced.body.ok).toBe(true); - }); - - it('propagates the force flag on stop', async () => { - await req('POST', '/v1/crawl/stop', { force: true }, { 'x-freecrawl-session': 'primary' }); - expect(calls.at(-1)).toEqual(['stop', 'primary', true]); - }); - - it('routes actions to the request session; unknown actions 400', async () => { - const ok = await req('POST', '/v1/action/export-csv', {}, { 'x-freecrawl-session': 'sess_3' }); - expect(ok.status).toBe(200); - expect(ok.body.session).toBe('sess_3'); - const bad = await req('POST', '/v1/action/nope', {}); - expect(bad.status).toBe(400); - expect(bad.body.error).toBe('bad-request'); - }); - - it('runs the session create → list → save → close lifecycle', async () => { - const created = await req('POST', '/v1/session/create', { mode: 'scratch', label: 't' }); - expect(created.status).toBe(200); - const id = created.body.sessionId as string; - expect(id.startsWith('sess_')).toBe(true); - - const list = await req('GET', '/v1/session/list'); - expect((list.body.sessions as Array<{ sessionId: string }>).some((s) => s.sessionId === id)).toBe( - true, - ); - - const saved = await req( - 'POST', - '/v1/session/save', - { projectPath: '/x.seoproject' }, - { 'x-freecrawl-session': id }, - ); - expect(saved.body.bytesWritten).toBe(1234); - - const closed = await req( - 'POST', - '/v1/session/close', - { save: true, savePath: '/y.seoproject' }, - { 'x-freecrawl-session': id }, - ); - expect(closed.body.savedTo).toBe('/y.seoproject'); - - const closedAgain = await req('POST', '/v1/session/close', {}, { 'x-freecrawl-session': id }); - expect(closedAgain.status).toBe(404); - expect(closedAgain.body.error).toBe('session-not-found'); - }); - - it('returns 400 for an unknown route', async () => { - const r = await req('GET', '/v1/nope'); - expect(r.status).toBe(400); - }); -}); diff --git a/tests/canonical-resolution.test.ts b/tests/canonical-resolution.test.ts deleted file mode 100644 index 5fd7a23..0000000 --- a/tests/canonical-resolution.test.ts +++ /dev/null @@ -1,339 +0,0 @@ -import { describe, it, expect } from 'vitest'; -import { ProjectDb } from '../packages/db/src/project-db.js'; -import { normalizeUrl, detectUrlTrap } from '../packages/core/src/url-utils.js'; -import { parseHtml } from '../packages/core/src/html-parser.js'; - -// FEATURES.md gaps 1–5: canonical comparisons were done against the raw -// authored href, so a relative canonical never matched any absolute URL. -// These lock in the resolved-canonical behaviour end-to-end: parse → store -// → the SQL predicates the sidebar and drill-down both read. - -function makeDb(): ProjectDb { - return new ProjectDb(':memory:'); -} - -/** Upsert a crawled HTML page the way the crawler does. */ -function page( - db: ProjectDb, - url: string, - extra: Record<string, unknown> = {}, -): void { - db.upsertUrl({ - url, - depth: 0, - statusCode: 200, - contentKind: 'html', - indexability: 'indexable', - ...extra, - } as Parameters<ProjectDb['upsertUrl']>[0]); -} - -describe('gap #3 — relative canonical resolves before comparison', () => { - it('parses a relative canonical into an absolute resolved form', () => { - const parsed = parseHtml( - '<html><head><link rel="canonical" href="/page/"></head><body>hi</body></html>', - 'https://ex.com/page/', - ); - expect(parsed.canonical).toBe('/page/'); // raw kept verbatim - expect(parsed.canonicalResolved).toBe('https://ex.com/page/'); - }); - - it('counts a relative self-canonical as self-referencing, not canonicalised away', () => { - const db = makeDb(); - page(db, 'https://ex.com/page/', { - canonical: '/page/', - canonicalResolved: 'https://ex.com/page/', - canonicalCount: 1, - canonicalDistinctCount: 1, - }); - const c = db.getOverviewCounts().issues; - expect(c.canonicalSelfReferencing).toBe(1); - // The bug: this used to be 1 because '/page/' !== the absolute url. - expect(c.canonicalNonSelf).toBe(0); - }); - - it('still reports a genuinely non-self canonical', () => { - const db = makeDb(); - page(db, 'https://ex.com/a', { - canonical: '/b', - canonicalResolved: 'https://ex.com/b', - canonicalCount: 1, - canonicalDistinctCount: 1, - }); - const c = db.getOverviewCounts().issues; - expect(c.canonicalNonSelf).toBe(1); - expect(c.canonicalSelfReferencing).toBe(0); - }); - - it('joins canonical → target on the resolved value', () => { - const db = makeDb(); - // /a canonicalises to /gone (relative href), which 404s. - page(db, 'https://ex.com/a', { - canonical: '/gone', - canonicalResolved: 'https://ex.com/gone', - canonicalCount: 1, - canonicalDistinctCount: 1, - }); - page(db, 'https://ex.com/gone', { statusCode: 404 }); - // Previously 0 — 't.url = urls.canonical' never matched '/gone'. - expect(db.getOverviewCounts().issues.canonicalToNon200).toBe(1); - }); - - it('does not flag a relative canonical as an HTTP/HTML mismatch', () => { - const db = makeDb(); - page(db, 'https://ex.com/a', { - canonical: '/a', - canonicalResolved: 'https://ex.com/a', - canonicalHttp: 'https://ex.com/a', - canonicalCount: 1, - canonicalDistinctCount: 1, - }); - expect(db.getOverviewCounts().issues.canonicalMismatch).toBe(0); - }); - - it('keeps working on legacy rows that predate canonical_resolved', () => { - const db = makeDb(); - // No canonicalResolved written — the COALESCE must fall back to raw. - page(db, 'https://ex.com/a', { - canonical: 'https://ex.com/a', - canonicalCount: 1, - }); - expect(db.getOverviewCounts().issues.canonicalSelfReferencing).toBe(1); - }); -}); - -describe('gap #1 — unusual canonical combinations', () => { - it('separates repeated-but-identical canonicals from disagreeing ones', () => { - const html = (hrefs: string[]): string => - `<html><head>${hrefs - .map((h) => `<link rel="canonical" href="${h}">`) - .join('')}</head><body>x</body></html>`; - - // Same target declared twice — noisy, but not a conflict. - const same = parseHtml(html(['/a', 'https://ex.com/a']), 'https://ex.com/a'); - expect(same.canonicalCount).toBe(2); - expect(same.canonicalDistinctCount).toBe(1); - - // Two different targets — search engines discard both. - const diff = parseHtml(html(['/a', '/b']), 'https://ex.com/a'); - expect(diff.canonicalCount).toBe(2); - expect(diff.canonicalDistinctCount).toBe(2); - }); - - it('counts conflicting canonicals as an issue', () => { - const db = makeDb(); - page(db, 'https://ex.com/a', { - canonical: '/a', - canonicalResolved: 'https://ex.com/a', - canonicalCount: 2, - canonicalDistinctCount: 2, - }); - page(db, 'https://ex.com/b', { - canonical: '/b', - canonicalResolved: 'https://ex.com/b', - canonicalCount: 2, - canonicalDistinctCount: 1, // repeated but identical - }); - const c = db.getOverviewCounts().issues; - expect(c.canonicalConflicting).toBe(1); - expect(c.multipleCanonicals).toBe(2); // both still count as "multiple" - }); - - it('detects a cross-domain canonical at parse time and counts it', () => { - const off = parseHtml( - '<html><head><link rel="canonical" href="https://other.com/x"></head></html>', - 'https://ex.com/a', - ); - expect(off.canonicalCrossDomain).toBe(true); - - // A subdomain of the same registrable domain is NOT cross-domain. - const sub = parseHtml( - '<html><head><link rel="canonical" href="https://www.ex.com/a"></head></html>', - 'https://ex.com/a', - ); - expect(sub.canonicalCrossDomain).toBe(false); - - const db = makeDb(); - page(db, 'https://ex.com/a', { - canonical: 'https://other.com/x', - canonicalResolved: 'https://other.com/x', - canonicalCount: 1, - canonicalDistinctCount: 1, - canonicalCrossDomain: 1, - }); - expect(db.getOverviewCounts().issues.canonicalCrossDomain).toBe(1); - }); -}); - -describe('gap #2 — redirect / canonical / noindex relationships', () => { - it('flags noindex sitting on a page canonicalised elsewhere', () => { - const db = makeDb(); - page(db, 'https://ex.com/a', { - canonical: '/b', - canonicalResolved: 'https://ex.com/b', - canonicalCount: 1, - canonicalDistinctCount: 1, - metaRobots: 'noindex, follow', - indexability: 'non-indexable:noindex', - }); - // A self-canonical + noindex is not a conflict — nothing is being - // consolidated, so there is no contradiction to report. - page(db, 'https://ex.com/c', { - canonical: '/c', - canonicalResolved: 'https://ex.com/c', - canonicalCount: 1, - canonicalDistinctCount: 1, - metaRobots: 'noindex', - indexability: 'non-indexable:noindex', - }); - expect(db.getOverviewCounts().issues.noindexCanonicalConflict).toBe(1); - }); - - it('flags a redirect whose final destination is noindexed', () => { - const db = makeDb(); - db.upsertUrl({ - url: 'https://ex.com/old', - depth: 0, - statusCode: 301, - contentKind: 'html', - indexability: 'non-indexable:redirect', - redirectTarget: 'https://ex.com/new', - } as Parameters<ProjectDb['upsertUrl']>[0]); - page(db, 'https://ex.com/new', { - metaRobots: 'noindex', - indexability: 'non-indexable:noindex', - }); - // `redirect_final_url` is written by the post-crawl chain walk, not by - // upsert — the filter reads the terminus, so multi-hop chains resolve. - db.recomputeRedirectChains(); - expect(db.getOverviewCounts().issues.redirectToNoindex).toBe(1); - }); - - it('does not flag a redirect landing on an indexable page', () => { - const db = makeDb(); - db.upsertUrl({ - url: 'https://ex.com/old', - depth: 0, - statusCode: 301, - contentKind: 'html', - indexability: 'non-indexable:redirect', - redirectTarget: 'https://ex.com/new', - } as Parameters<ProjectDb['upsertUrl']>[0]); - page(db, 'https://ex.com/new'); - db.recomputeRedirectChains(); - expect(db.getOverviewCounts().issues.redirectToNoindex).toBe(0); - }); -}); - -describe('gaps #4 / #5 — URL normalisation completeness', () => { - it('folds percent-escape case (RFC 3986) by default', () => { - expect(normalizeUrl('https://x.com/a%2fb')).toBe( - normalizeUrl('https://x.com/a%2Fb'), - ); - expect(normalizeUrl('https://x.com/p?q=%2fc')).toBe( - normalizeUrl('https://x.com/p?q=%2Fc'), - ); - }); - - it('keeps an encoded slash distinct from a literal one', () => { - expect(normalizeUrl('https://x.com/a%2Fb')).not.toBe( - normalizeUrl('https://x.com/a/b'), - ); - }); - - it('leaves a bare percent sign alone', () => { - expect(normalizeUrl('https://x.com/100%pure')).toBe('https://x.com/100%pure'); - }); - - it('sorts query params only when asked', () => { - const a = 'https://x.com/p?b=2&a=1'; - const b = 'https://x.com/p?a=1&b=2'; - expect(normalizeUrl(a)).not.toBe(normalizeUrl(b)); - expect(normalizeUrl(a, undefined, { sortQueryParams: true })).toBe( - normalizeUrl(b, undefined, { sortQueryParams: true }), - ); - }); - - it('preserves repeated-key order when sorting', () => { - expect( - normalizeUrl('https://x.com/p?tag=z&a=1&tag=b', undefined, { - sortQueryParams: true, - }), - ).toBe('https://x.com/p?a=1&tag=z&tag=b'); - }); - - it('collapses duplicate slashes only when asked', () => { - const dup = 'https://x.com//a//b'; - expect(normalizeUrl(dup)).not.toBe(normalizeUrl('https://x.com/a/b')); - expect(normalizeUrl(dup, undefined, { collapseDuplicateSlashes: true })).toBe( - 'https://x.com/a/b', - ); - }); - - it('collapses before applying the trailing-slash policy', () => { - expect( - normalizeUrl('https://x.com/a//b//', undefined, { - collapseDuplicateSlashes: true, - trailingSlash: 'strip', - }), - ).toBe('https://x.com/a/b'); - }); -}); - -describe('gap #8 — crawl-trap heuristics', () => { - const REPEAT = { maxRepeatedSegments: 3 }; - - it('detects a repeated-segment link loop', () => { - expect(detectUrlTrap('https://x.com/shop/shop/shop/', REPEAT)).toBe( - 'repeated-segment', - ); - expect(detectUrlTrap('https://x.com/a/b/a/b/a/b', REPEAT)).toBe( - 'repeated-segment', - ); - // Case-folded: /Shop/shop/SHOP is the same loop. - expect(detectUrlTrap('https://x.com/Shop/shop/SHOP', REPEAT)).toBe( - 'repeated-segment', - ); - }); - - it('leaves ordinary deep paths alone', () => { - expect( - detectUrlTrap('https://x.com/blog/2026/03/how-to-crawl', REPEAT), - ).toBeNull(); - // Twice is not a loop — only the configured threshold is. - expect(detectUrlTrap('https://x.com/shop/cat/shop', REPEAT)).toBeNull(); - }); - - it('honours the threshold and can be disabled', () => { - const url = 'https://x.com/a/a/a'; - expect(detectUrlTrap(url, { maxRepeatedSegments: 4 })).toBeNull(); - expect(detectUrlTrap(url, { maxRepeatedSegments: 0 })).toBeNull(); - }); - - it('detects session-id parameters', () => { - expect(detectUrlTrap('https://x.com/p?PHPSESSID=abc123')).toBe('session-id'); - expect(detectUrlTrap('https://x.com/p?jsessionid=zz')).toBe('session-id'); - }); - - it('detects a calendar navigator but not a plain archive page', () => { - expect(detectUrlTrap('https://x.com/events?year=2026&month=03')).toBe( - 'calendar', - ); - expect(detectUrlTrap('https://x.com/events?year=2026')).toBeNull(); - }); - - it('detects faceted navigation only when a cap is set', () => { - const faceted = 'https://x.com/p?a=1&b=2&c=3&d=4&e=5'; - expect(detectUrlTrap(faceted)).toBeNull(); - expect(detectUrlTrap(faceted, { maxQueryParams: 4 })).toBe('query-params'); - expect(detectUrlTrap(faceted, { maxQueryParams: 5 })).toBeNull(); - }); - - it('surfaces flagged traps through the issue filter', () => { - const db = makeDb(); - page(db, 'https://x.com/p?PHPSESSID=abc', { urlTrap: 'session-id' }); - page(db, 'https://x.com/events?year=2026&month=3', { urlTrap: 'calendar' }); - page(db, 'https://x.com/normal'); - expect(db.getOverviewCounts().issues.crawlTrap).toBe(2); - }); -}); diff --git a/tests/feedback-fixes.test.ts b/tests/feedback-fixes.test.ts deleted file mode 100644 index 7de7904..0000000 --- a/tests/feedback-fixes.test.ts +++ /dev/null @@ -1,158 +0,0 @@ -import { describe, it, expect } from 'vitest'; -import { ProjectDb } from '../packages/db/src/project-db.js'; -import type { AdvancedFilter, FilterClause } from '@freecrawl/shared-types'; - -// These exercise the DB-layer fixes end-to-end against a real (in-memory) -// migrated ProjectDb — the highest-risk SQL changes from the feedback pass: -// #13 redirect/error pages must NOT count as "missing" on-page elements -// #14 GSC data must attach across a trailing-slash URL variant -// #2 Advanced filter must query custom-extraction values (and stay safe) - -function makeDb(): ProjectDb { - // runMigrations runs inside the constructor for a writable connection, so - // an in-memory file is a fully-formed, empty project. - return new ProjectDb(':memory:'); -} - -const filter = (clauses: FilterClause[]): AdvancedFilter => ({ - groups: [{ clauses }], -}); - -describe('#13 — no fake SEO errors on 3xx/4xx pages', () => { - it('excludes a 301 redirect from the "missing element" counters', () => { - const db = makeDb(); - // A real 200 page with no title/meta/h1/lang/viewport — genuinely missing. - db.upsertUrl({ url: 'https://x.com/ok', depth: 0, statusCode: 200, contentKind: 'html' }); - // A 301 redirect carries none of those by nature — must NOT be flagged. - db.upsertUrl({ - url: 'https://x.com/redir', - depth: 0, - statusCode: 301, - contentKind: 'html', - indexability: 'non-indexable:redirect', - }); - - const issues = db.getOverviewCounts().issues; - expect(issues.titleMissing).toBe(1); - expect(issues.metaMissing).toBe(1); - expect(issues.h1Missing).toBe(1); - expect(issues.langMissing).toBe(1); - expect(issues.viewportMissing).toBe(1); - - // The drill-down table must agree with the counter (only the 200 page). - const drill = db.queryUrls({ limit: 100, offset: 0, category: 'issues:title-missing' }); - expect(drill.total).toBe(1); - expect(drill.rows[0]?.url).toBe('https://x.com/ok'); - db.close(); - }); - - it('still flags genuinely-missing elements on 200 pages', () => { - const db = makeDb(); - db.upsertUrl({ url: 'https://x.com/a', depth: 0, statusCode: 200, contentKind: 'html' }); - db.upsertUrl({ - url: 'https://x.com/b', - depth: 0, - statusCode: 200, - contentKind: 'html', - title: 'Has title', - metaDescription: 'Has meta', - h1: 'Has H1', - lang: 'en', - viewport: 'width=device-width', - }); - const issues = db.getOverviewCounts().issues; - expect(issues.titleMissing).toBe(1); // only /a - expect(issues.viewportMissing).toBe(1); - db.close(); - }); -}); - -describe('#14 — GSC join tolerates trailing-slash URL variants', () => { - it('attaches GSC data to a crawled /page/ from a GSC /page (slash-insensitive default)', () => { - const db = makeDb(); - db.upsertUrl({ url: 'https://x.com/page/', depth: 0, statusCode: 200, contentKind: 'html' }); - db.replaceGscResults( - [{ url: 'https://x.com/page', clicks: 5, impressions: 100, ctr: 0.05, position: 3 }], - '2026-01-01T00:00:00Z', - ); - - const tol = db.queryGsc({ limit: 100, offset: 0, filter: 'all', matchSlash: true }); - const row = tol.rows.find((r) => r.url === 'https://x.com/page/'); - expect(row?.gsc?.impressions).toBe(100); - - // With slash-matching turned off the exact string differs → no attach. - const exact = db.queryGsc({ limit: 100, offset: 0, filter: 'all', matchSlash: false }); - const rowExact = exact.rows.find((r) => r.url === 'https://x.com/page/'); - expect(rowExact?.gsc).toBeNull(); - db.close(); - }); -}); - -describe('#2 — Advanced filter on custom-extraction values', () => { - it('filters rows by an extracted field value', () => { - const db = makeDb(); - db.upsertUrl({ - url: 'https://x.com/a', - depth: 0, - statusCode: 200, - contentKind: 'html', - extractionResults: JSON.stringify({ h2: 'Recommended Products' }), - }); - db.upsertUrl({ - url: 'https://x.com/b', - depth: 0, - statusCode: 200, - contentKind: 'html', - extractionResults: JSON.stringify({ h2: 'Newsletter' }), - }); - db.upsertUrl({ url: 'https://x.com/c', depth: 0, statusCode: 200, contentKind: 'html' }); - - const contains = db.queryUrls({ - limit: 100, - offset: 0, - filter: filter([{ field: 'extraction', extractionKey: 'h2', operator: 'contains', value: 'Recommend' }]), - }); - expect(contains.total).toBe(1); - expect(contains.rows[0]?.url).toBe('https://x.com/a'); - - const none = db.queryUrls({ - limit: 100, - offset: 0, - filter: filter([{ field: 'extraction', extractionKey: 'h2', operator: 'contains', value: 'zzz' }]), - }); - expect(none.total).toBe(0); - - const empty = db.queryUrls({ - limit: 100, - offset: 0, - filter: filter([{ field: 'extraction', extractionKey: 'h2', operator: 'is_empty', value: '' }]), - }); - expect(empty.rows.map((r) => r.url)).toEqual(['https://x.com/c']); - db.close(); - }); - - it('drops (does not execute) an unsafe extraction key instead of crashing', () => { - const db = makeDb(); - db.upsertUrl({ - url: 'https://x.com/a', - depth: 0, - statusCode: 200, - contentKind: 'html', - extractionResults: JSON.stringify({ h2: 'x' }), - }); - db.upsertUrl({ url: 'https://x.com/b', depth: 0, statusCode: 200, contentKind: 'html' }); - - // A key with a double-quote would break the JSON path / could be an - // injection vector — the clause must be rejected, the query must not - // throw, and with no valid clause the filter is a no-op (all rows). - const res = db.queryUrls({ - limit: 100, - offset: 0, - filter: filter([ - { field: 'extraction', extractionKey: 'h2"} OR 1=1 --', operator: 'contains', value: 'x' }, - ]), - }); - expect(res.total).toBe(2); - db.close(); - }); -}); diff --git a/vitest.config.ts b/vitest.config.ts deleted file mode 100644 index ecf7345..0000000 --- a/vitest.config.ts +++ /dev/null @@ -1,41 +0,0 @@ -import { defineConfig } from 'vitest/config'; - -// Unit tests target the Electron-free, pure modules (crawl-lease registry, -// the localhost bridge's HTTP routing). Anything that imports `electron` is -// out of scope here — those paths are exercised by the app itself. -export default defineConfig({ - plugins: [ - { - // `node:sqlite` is a newer Node builtin that vite's SSR resolver - // doesn't yet recognise: it strips the `node:` prefix, looks for a - // package called `sqlite`, and fails to load it. Marking it external - // isn't honoured by vitest's module runner here, so instead serve a - // tiny virtual shim that re-exports the real builtin via createRequire - // at run time (Node loads `node:sqlite` natively). Needed by the - // ProjectDb-backed tests; app/build paths are unaffected. - name: 'node-sqlite-shim', - enforce: 'pre', - resolveId(id) { - if (id === 'node:sqlite' || id === 'sqlite') return '\0node-sqlite-shim'; - return null; - }, - load(id) { - if (id === '\0node-sqlite-shim') { - return [ - "import { createRequire } from 'node:module';", - 'const req = createRequire(import.meta.url);', - "const sqlite = req('node:sqlite');", - 'export const DatabaseSync = sqlite.DatabaseSync;', - 'export const StatementSync = sqlite.StatementSync;', - 'export default sqlite;', - ].join('\n'); - } - return null; - }, - }, - ], - test: { - include: ['tests/**/*.test.ts'], - environment: 'node', - }, -});