diff --git a/deploy.sh b/deploy.sh index 6feda02..dc8d2ab 100755 --- a/deploy.sh +++ b/deploy.sh @@ -48,6 +48,37 @@ # # Override the location for a staging box with: WEBROOT=/var/www/ss-staging ./deploy.sh # +# --- the database account ------------------------------------------------ +# +# The API connects as a least-privilege account with DML rights only. This is +# deliberate: the server fetches untrusted URLs for link previews, and a +# service able to reshape or drop its own tables is a far larger blast radius +# than that feature warrants. +# +# CREATE USER 'app'@'localhost' IDENTIFIED BY ''; +# GRANT SELECT, INSERT, UPDATE, DELETE ON application.* TO 'app'@'localhost'; +# FLUSH PRIVILEGES; +# +# -- deliberately NOT granted: CREATE, ALTER, DROP, INDEX +# +# Then set DB_USER=app and DB_PASS= in ~/.env. Note the asymmetry: +# the admin scripts here (backupDatabase.sh, applyLastBackup.sh, +# applyScrapeSchema.sh) still connect as root with $PROD_DB_PASS. +# +# DELETE is genuinely required -- the scraper drops shell attachment rows for +# dead links and purges expired cache entries. +# +# --- schema changes ------------------------------------------------------ +# +# Because the app holds no DDL, a deploy that introduces schema changes needs +# one manual step, run once per environment as a database admin: +# +# ./applyScrapeSchema.sh +# +# It is idempotent, so re-running is safe. The API verifies the schema and the +# grants at boot; if either is missing it logs the remedy and serves normally +# with link previews disabled. +# set -euo pipefail diff --git a/server/helpers/Schema.js b/server/helpers/Schema.js index ee75744..47cf649 100644 --- a/server/helpers/Schema.js +++ b/server/helpers/Schema.js @@ -1,58 +1,57 @@ // -// Idempotent schema convergence for the scraper tables. +// Verification of the scraper's schema and privileges. Issues NO DDL. // -// There is no migration framework in this project and no schema file in git, -// so this runs at boot, checks information_schema for each object, and applies -// only what is missing. Dev and prod self-heal, and deploy.sh needs no change. +// This module used to converge the schema at boot with CREATE TABLE / ALTER +// TABLE. That worked only because dev connects as root (shell.nix exports +// DB_USER=root); on production, which runs as a least-privilege `app` user, it +// failed with: // -// Portability matters here: dev is MariaDB 11.4 and prod is MySQL 8. -// ADD COLUMN IF NOT EXISTS is MariaDB-only and is deliberately not used - -// every ALTER is guarded by an explicit existence check instead. +// CREATE command denied to user 'app'@'localhost' for table scrape_cache +// +// The restricted account is correct - the server ingests untrusted URLs and +// should not be able to reshape its own tables. So DDL moved out of the +// application entirely: an admin runs ./applyScrapeSchema.sh once, and this +// module only checks that the result is present and usable. +// +// Note on why the DDL is REMOVED rather than guarded by an existence check: +// MariaDB evaluates the CREATE privilege before IF NOT EXISTS, so +// `CREATE TABLE IF NOT EXISTS` would keep erroring for a DML-only user on every +// boot even once the table exists. // const db = require('@config/database') let Schema = module.exports = {} -const SCRAPE_CACHE_TABLE = ` - CREATE TABLE IF NOT EXISTS scrape_cache ( - id int(10) unsigned NOT NULL AUTO_INCREMENT, - url_hash char(64) NOT NULL, - url varchar(1000) NOT NULL DEFAULT '', - canonical_url varchar(1000) DEFAULT NULL, - status smallint(6) NOT NULL DEFAULT 0, - title varchar(512) DEFAULT NULL, - site_name varchar(255) DEFAULT NULL, - description mediumtext DEFAULT NULL, - keywords varchar(512) DEFAULT NULL, - search_text mediumtext DEFAULT NULL, - image_url varchar(1000) DEFAULT NULL, - file_location varchar(500) DEFAULT NULL, - etag varchar(255) DEFAULT NULL, - last_modified varchar(128) DEFAULT NULL, - content_type varchar(128) DEFAULT NULL, - fetch_state varchar(20) NOT NULL DEFAULT 'pending', - failure_count int(11) NOT NULL DEFAULT 0, - last_error varchar(255) DEFAULT NULL, - fetched_at int(11) DEFAULT NULL, - expires_at int(11) DEFAULT NULL, - PRIMARY KEY (id), - UNIQUE KEY scrape_cache_url_hash (url_hash), - KEY scrape_cache_expires_at (expires_at), - KEY scrape_cache_fetch_state (fetch_state) - ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 -` +const REQUIRED_TABLE = 'scrape_cache' +const REQUIRED_COLUMNS = ['scrape_state', 'scrape_attempts', 'enqueued_at'] +const REQUIRED_INDEX = 'attachment_scrape_state' -// DEFAULT 2 ("done") keeps existing rows out of the boot recovery sweep. -const ATTACHMENT_COLUMNS = [ - { name: 'scrape_state', ddl: 'ALTER TABLE attachment ADD COLUMN scrape_state tinyint(4) NOT NULL DEFAULT 2' }, - { name: 'scrape_attempts', ddl: 'ALTER TABLE attachment ADD COLUMN scrape_attempts tinyint(4) NOT NULL DEFAULT 0' }, - { name: 'enqueued_at', ddl: 'ALTER TABLE attachment ADD COLUMN enqueued_at int(11) DEFAULT NULL' }, -] +// The scraper writes and deletes, not just reads. DELETE is genuinely needed: +// terminal failures drop the shell attachment row (ScrapeQueue), and +// ScrapeCache.purgeExpired removes expired rows. +const REQUIRED_PRIVILEGES = ['SELECT', 'INSERT', 'UPDATE', 'DELETE'] -const ATTACHMENT_INDEXES = [ - { name: 'attachment_scrape_state', ddl: 'ALTER TABLE attachment ADD KEY attachment_scrape_state (scrape_state, enqueued_at)' }, -] +// Set by verifyScrapeSchema so the per-request paths can check a flag rather +// than querying information_schema on every note save. +let ready = false +let lastResult = null + +Schema.isScrapeSchemaReady = () => ready +Schema.lastVerification = () => lastResult + +// Test seam. +Schema.__setReady = (value) => { ready = !!value } +Schema.__reset = () => { ready = false; lastResult = null } + +const tableExists = async (table) => { + const [rows] = await db.promise().query( + `SELECT 1 FROM information_schema.TABLES + WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = ? LIMIT 1`, + [table] + ) + return rows.length > 0 +} const columnExists = async (table, column) => { const [rows] = await db.promise().query( @@ -71,34 +70,141 @@ const indexExists = async (table, index) => { ) return rows.length > 0 } + +Schema.__tableExists = tableExists Schema.__columnExists = columnExists Schema.__indexExists = indexExists // -// Safe to call repeatedly. Returns the list of statements it actually applied, -// so boot logging says something useful on the one run that matters. +// Parse SHOW GRANTS output into the set of privileges that apply to this +// database. Any user may run SHOW GRANTS FOR CURRENT_USER(), so this needs no +// privileges of its own. // -Schema.ensureScrapeSchema = async () => { - const applied = [] +// Grant lines look like: +// GRANT ALL PRIVILEGES ON *.* TO `root`@`localhost` WITH GRANT OPTION +// GRANT SELECT, INSERT, UPDATE, DELETE ON `application`.* TO `app`@`localhost` +// GRANT SELECT ON `application`.`attachment` TO `app`@`localhost` +// GRANT USAGE ON *.* TO `app`@`localhost` +// +const PRIVILEGE_PATTERN = /^GRANT\s+(.+?)\s+ON\s+(\S+)\s+TO\s/i - await db.promise().query(SCRAPE_CACHE_TABLE) +Schema.parseGrants = (lines, database, tables) => { + const granted = new Set() - for (const column of ATTACHMENT_COLUMNS) { - if (await columnExists('attachment', column.name)) { continue } - await db.promise().query(column.ddl) - applied.push(`attachment.${column.name}`) + // Scope strings that cover our tables. Backticks are optional in output. + const strip = (value) => value.replace(/`/g, '') + const relevantScopes = new Set([ + '*.*', + `${database}.*`, + ...tables.map(table => `${database}.${table}`), + ]) + + for (const line of lines) { + const match = String(line).match(PRIVILEGE_PATTERN) + if (!match) { continue } + + const scope = strip(match[2]) + if (!relevantScopes.has(scope)) { continue } + + // Drop any column-level qualifiers, e.g. "SELECT (id, url)". + const privileges = match[1].replace(/\s*\([^)]*\)/g, '') + + for (const privilege of privileges.split(',')) { + const name = privilege.trim().toUpperCase() + if (!name || name === 'USAGE') { continue } + + if (name === 'ALL PRIVILEGES' || name === 'ALL') { + REQUIRED_PRIVILEGES.forEach(required => granted.add(required)) + continue + } + + granted.add(name) + } } - // Index last: it references the columns above. - for (const index of ATTACHMENT_INDEXES) { - if (await indexExists('attachment', index.name)) { continue } - await db.promise().query(index.ddl) - applied.push(`attachment KEY ${index.name}`) + return granted +} + +const checkPrivileges = async () => { + let lines = [] + + try { + const [rows] = await db.promise().query('SHOW GRANTS FOR CURRENT_USER()') + // Each row is an object with a single, server-named column. + lines = rows.map(row => Object.values(row)[0]) + } catch (error) { + // If we cannot read our own grants, do not block on a guess - the + // object checks above are the real gate. + console.log('[schema] could not read grants, skipping privilege check:', error.message) + return { missing: [], database: null, currentUser: null } + } + + const [dbRows] = await db.promise().query('SELECT DATABASE() AS name, CURRENT_USER() AS who') + const database = dbRows[0] && dbRows[0].name + const currentUser = dbRows[0] && dbRows[0].who + + const granted = Schema.parseGrants(lines, database, ['attachment', REQUIRED_TABLE]) + + return { + missing: REQUIRED_PRIVILEGES.filter(privilege => !granted.has(privilege)), + database, + currentUser, + } +} + +// 'app@localhost' -> "'app'@'localhost'", so the remedy is copy-pasteable. +const quoteUser = (currentUser) => { + if (!currentUser) { return '' } + + const [name, host] = String(currentUser).split('@') + return `'${name}'@'${host || 'localhost'}'` +} + +// +// Check that everything the scraper needs is present. Never mutates anything. +// +// Returns { ok, missingObjects, missingPrivileges, remedy }. +// +Schema.verifyScrapeSchema = async () => { + const missingObjects = [] + + if (!await tableExists(REQUIRED_TABLE)) { + missingObjects.push(`table ${REQUIRED_TABLE}`) + } + + for (const column of REQUIRED_COLUMNS) { + if (!await columnExists('attachment', column)) { + missingObjects.push(`attachment.${column}`) + } + } + + if (!await indexExists('attachment', REQUIRED_INDEX)) { + missingObjects.push(`attachment index ${REQUIRED_INDEX}`) + } + + const privileges = await checkPrivileges() + const missingPrivileges = privileges.missing || [] + + const remedy = [] + if (missingObjects.length) { + remedy.push('run ./applyScrapeSchema.sh as a database admin') + } + if (missingPrivileges.length) { + // Name the real database and user so this can be pasted straight into + // a mysql prompt rather than decoded first. + const target = `${privileges.database || ''}.*` + remedy.push( + `GRANT ${missingPrivileges.join(', ')} ON ${target} TO ${quoteUser(privileges.currentUser)}` + ) } - if (applied.length) { - console.log('[schema] applied scrape schema changes:', applied.join(', ')) + lastResult = { + ok: missingObjects.length === 0 && missingPrivileges.length === 0, + missingObjects, + missingPrivileges, + remedy: remedy.join('; '), } + ready = lastResult.ok - return applied + return lastResult } diff --git a/server/index.js b/server/index.js index fcfc797..7ae398c 100644 --- a/server/index.js +++ b/server/index.js @@ -348,16 +348,34 @@ app.use('/api/metric-tracking', metricTracking) // // Scraper startup. // -// ensureScrapeSchema() converges the scrape_cache table and the attachment -// lifecycle columns. It is idempotent and runs before the listener binds, so -// there is no window where requests arrive against a schema that is mid-change. -// There is no migration framework in this project - see server/helpers/Schema.js. +// The application deliberately holds no DDL privileges, so it does not create +// its own schema - verifyScrapeSchema() only checks that an admin has already +// run ./applyScrapeSchema.sh and that the required DML grants are present. +// +// When anything is missing the scraper stays off rather than failing an INSERT +// on every note save. The listener binds either way: the API must not depend on +// link previews being available. // const Schema = require('@helpers/Schema') const ScrapeQueue = require('@workers/ScrapeQueue') -Schema.ensureScrapeSchema() - .then(() => { +Schema.verifyScrapeSchema() + .then(result => { + if (!result.ok) { + const missing = [ + ...result.missingObjects, + ...result.missingPrivileges.map(privilege => `privilege ${privilege}`), + ].join(', ') + + console.log(`Scraper: DISABLED - missing ${missing}. To fix: ${result.remedy}`) + return null + } + + // Say so positively. Previously a healthy scraper logged nothing, so + // "started" and "silently disabled" looked identical in pm2 logs -- + // which is how a dead scraper went unnoticed on production. + console.log('Scraper: ready') + ScrapeQueue.start() // The queue lives in memory, so a pm2 reload loses whatever was in it. @@ -372,7 +390,7 @@ Schema.ensureScrapeSchema() }) .catch(error => { // A scraper that cannot start must not stop the API from serving. - console.log('Scraper: startup failed, link previews disabled this boot:', error.message) + console.log('Scraper: startup check failed, link previews disabled this boot:', error.message) }) .finally(() => { //Output running status diff --git a/server/migrations/0001_scrape_cache_and_state.sql b/server/migrations/0001_scrape_cache_and_state.sql index aea23ec..3153c7d 100644 --- a/server/migrations/0001_scrape_cache_and_state.sql +++ b/server/migrations/0001_scrape_cache_and_state.sql @@ -1,18 +1,32 @@ -- -- Scrape cache + per-attachment scrape lifecycle. -- --- This file is the human-readable record. It is NOT applied by hand: the same --- statements live in server/helpers/Schema.js and are applied idempotently at --- boot after an information_schema existence check, so dev and prod converge --- without a migration runner. +-- HOW THIS IS APPLIED -- --- Portability: dev is MariaDB 11.4, prod is MySQL 8. Nothing here may use --- ADD COLUMN IF NOT EXISTS (MariaDB-only). +-- ./applyScrapeSchema.sh (from the repo root, as an admin) +-- +-- Run it once per environment. The application does NOT apply this itself: +-- production connects as a least-privilege `app` user with DML rights only, and +-- deliberately holds no CREATE/ALTER. server/helpers/Schema.js only *verifies* +-- that these objects exist and that the required privileges are held; if they +-- are missing it disables link previews and logs the remedy. +-- +-- Safe to re-run: every statement is conditional. +-- +-- Portability: dev and production are both MariaDB 11.4, so ADD COLUMN IF NOT +-- EXISTS and CREATE INDEX IF NOT EXISTS are available. If this project ever +-- moves to MySQL 8 these become invalid and need information_schema guards +-- instead. -- --- One row per URL, shared across users and notes. url_hash carries the UNIQUE --- index because a UNIQUE on varchar(1000) utf8mb4 is 4000 bytes and exceeds --- InnoDB's 3072-byte key limit. +-- One row per URL, shared across users and notes. +-- +-- url_hash carries the UNIQUE index because a UNIQUE on varchar(1000) utf8mb4 +-- is 4000 bytes and exceeds InnoDB's 3072-byte key limit. `url` is kept +-- human-readable for debugging only. +-- +-- COLLATE is stated explicitly to match the existing tables (utf8mb4_general_ci) +-- rather than inheriting whatever the server default happens to be. CREATE TABLE IF NOT EXISTS `scrape_cache` ( `id` int(10) unsigned NOT NULL AUTO_INCREMENT, `url_hash` char(64) NOT NULL, @@ -38,14 +52,16 @@ CREATE TABLE IF NOT EXISTS `scrape_cache` ( UNIQUE KEY `scrape_cache_url_hash` (`url_hash`), KEY `scrape_cache_expires_at` (`expires_at`), KEY `scrape_cache_fetch_state` (`fetch_state`) -) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci; --- Per-attachment lifecycle, so an in-memory queue survives a restart. +-- Per-attachment lifecycle, so the in-memory scrape queue survives a restart. -- -- scrape_state: 0 = pending, 1 = fetching, 2 = done, 3 = failed. --- DEFAULT 2 is load-bearing: it keeps the ~2200 pre-existing rows out of the --- recovery sweep on first boot. -ALTER TABLE `attachment` ADD COLUMN `scrape_state` tinyint(4) NOT NULL DEFAULT 2; -ALTER TABLE `attachment` ADD COLUMN `scrape_attempts` tinyint(4) NOT NULL DEFAULT 0; -ALTER TABLE `attachment` ADD COLUMN `enqueued_at` int(11) DEFAULT NULL; -ALTER TABLE `attachment` ADD KEY `attachment_scrape_state` (`scrape_state`, `enqueued_at`); +-- +-- DEFAULT 2 is load-bearing: it keeps every pre-existing attachment row +-- (~2200 in production) out of the boot recovery sweep. +ALTER TABLE `attachment` ADD COLUMN IF NOT EXISTS `scrape_state` tinyint(4) NOT NULL DEFAULT 2; +ALTER TABLE `attachment` ADD COLUMN IF NOT EXISTS `scrape_attempts` tinyint(4) NOT NULL DEFAULT 0; +ALTER TABLE `attachment` ADD COLUMN IF NOT EXISTS `enqueued_at` int(11) DEFAULT NULL; + +CREATE INDEX IF NOT EXISTS `attachment_scrape_state` ON `attachment` (`scrape_state`, `enqueued_at`); diff --git a/server/models/Attachment.js b/server/models/Attachment.js index af43575..418296f 100644 --- a/server/models/Attachment.js +++ b/server/models/Attachment.js @@ -288,11 +288,18 @@ Attachment.generateThumbnail = (fileName) => { Attachment.scanTextForWebsites = (userId, noteId, noteText) => { const ScrapeQueue = require('@workers/ScrapeQueue') + const Schema = require('@helpers/Schema') // Fixed: the old version called resolve() without returning, so an empty // note fell through and ran the DB lookup anyway. if (!noteText || noteText.length == 0) { return Promise.resolve('') } + // When the scrape schema is absent the new columns do not exist, so + // creating a pending row would fail on every save. The diff and soft-delete + // below still run - they touch no new columns - so removing a link from a + // note behaves exactly as before. + const scrapeAvailable = Schema.isScrapeSchemaReady() + let knownText = '' return Attachment.urlForNote(userId, noteId) @@ -320,6 +327,10 @@ Attachment.scanTextForWebsites = (userId, noteId, noteText) => { if (!remaining.length) { return knownText } + // No schema, no previews. Return what is already cached rather than + // attempting inserts that cannot succeed. + if (!scrapeAvailable) { return knownText } + // Create a shell row per new URL, then hand off to the worker. return Promise.all(remaining.map(url => Attachment.createPendingUrlAttachment(userId, noteId, url) @@ -378,8 +389,10 @@ Attachment.createPendingUrlAttachment = (userId, noteId, url) => { Attachment.scrapeUrlsCreateAttachments = (userId, noteId, foundUrls) => { const ScrapeQueue = require('@workers/ScrapeQueue') + const Schema = require('@helpers/Schema') if (!foundUrls || !foundUrls.length) { return Promise.resolve('') } + if (!Schema.isScrapeSchemaReady()) { return Promise.resolve('') } return Promise.all(foundUrls.map(url => Attachment.createPendingUrlAttachment(userId, noteId, url) @@ -562,6 +575,11 @@ Attachment.getPushkeyBookmarklet = (userId) => { Attachment.pushUrl = (pushkey, url) => { const ScrapeQueue = require('@workers/ScrapeQueue') + const Schema = require('@helpers/Schema') + + // Preserve the bookmarklet contract - resolve truthy, leaking nothing - + // rather than failing an insert against columns that do not exist. + if (!Schema.isScrapeSchemaReady()) { return Promise.resolve(true) } let userId = null pushkey = String(pushkey || '').replace(/ /g, '+') diff --git a/server/tests/models/Attachment.test.js b/server/tests/models/Attachment.test.js index ddcc5e4..b26c6d7 100644 --- a/server/tests/models/Attachment.test.js +++ b/server/tests/models/Attachment.test.js @@ -1,6 +1,7 @@ const Attachment = require('../../models/Attachment') const User = require('../../models/User') const ScrapeQueue = require('@workers/ScrapeQueue') +const Schema = require('@helpers/Schema') const Robots = require('@helpers/Robots') const FetchMock = require('../support/fetchMock') const dns = require('dns').promises @@ -82,6 +83,8 @@ describe('Push URL', () => { beforeEach(() => { FetchMock.install() + // The server marks this at boot; there is no boot phase in tests. + Schema.__setReady(true) Robots.__resetCache() dnsSpy = jest.spyOn(dns, 'lookup').mockResolvedValue([{ address: '93.184.216.34', family: 4 }]) // Real code path, no politeness waiting and no robots lookups. @@ -92,6 +95,7 @@ describe('Push URL', () => { FetchMock.restore() dnsSpy.mockRestore() ScrapeQueue.__reset() + Schema.__reset() }) test('scrapes a pushed url into a searchable attachment', () => { diff --git a/server/tests/models/ScrapeQueue.test.js b/server/tests/models/ScrapeQueue.test.js index 7ebc7c2..6f26896 100644 --- a/server/tests/models/ScrapeQueue.test.js +++ b/server/tests/models/ScrapeQueue.test.js @@ -17,6 +17,7 @@ const testUserName = 'jestTestUserScrapeQueue' const password = 'Beans19934!!!' let userId = null +let noteId = null let dnsSpy = null const page = (title, description) => FetchMock.html(` @@ -35,7 +36,11 @@ const attachmentRow = (id) => db.promise() .then(([rows]) => rows[0] || null) beforeAll(async () => { - await Schema.ensureScrapeSchema() + // The server normally does this at boot; tests have no boot phase. + const verification = await Schema.verifyScrapeSchema() + if (!verification.ok) { + throw new Error(`dev scrape schema not applied - run ./applyScrapeSchema.sh (${verification.remedy})`) + } try { const existing = await User.getByUserName(testUserName) @@ -44,10 +49,19 @@ beforeAll(async () => { const registered = await User.register(testUserName, password) userId = registered.userId + + // A real note, so the soft-delete path below sees an existing note and + // hides the attachment rather than deleting it. + const [note] = await db.promise().query( + 'INSERT INTO note (user_id, created, opened) VALUES (?, ?, ?)', + [userId, Math.round(Date.now() / 1000), Math.round(Date.now() / 1000)] + ) + noteId = note.insertId }) beforeEach(() => { FetchMock.install() + Schema.__setReady(true) Robots.__resetCache() dnsSpy = jest.spyOn(dns, 'lookup').mockResolvedValue([{ address: '93.184.216.34', family: 4 }]) HttpFetch.__setSleep(() => Promise.resolve()) @@ -65,6 +79,7 @@ afterEach(() => { afterAll(async () => { await db.promise().query('DELETE FROM attachment WHERE user_id = ?', [userId]) + await db.promise().query('DELETE FROM note WHERE user_id = ?', [userId]) db.end() }) @@ -305,6 +320,92 @@ describe('robots.txt', () => { }) }) +// +// Production ran for a while with the scrape columns missing, and every note +// save attempted an INSERT that could not succeed. These pin the degraded +// behaviour: do nothing, quietly, and leave everything else working. +// +describe('degrades cleanly when the scrape schema is absent', () => { + + beforeEach(() => { + Schema.__setReady(false) + }) + + test('scanTextForWebsites creates no rows and makes no requests', async () => { + FetchMock.route(/.*/, () => page('Should Not Be Fetched')) + + const [before] = await db.promise().query( + 'SELECT COUNT(*) AS n FROM attachment WHERE user_id = ?', [userId]) + + const text = await Attachment.scanTextForWebsites(userId, null, 'see https://example.com/unavailable') + + const [after] = await db.promise().query( + 'SELECT COUNT(*) AS n FROM attachment WHERE user_id = ?', [userId]) + + expect(after[0].n).toBe(before[0].n) + expect(FetchMock.callCount()).toBe(0) + expect(typeof text).toBe('string') + }) + + // + // Removing a link from a note must still clean up its attachment. That path + // touches none of the new columns, so it is unaffected by the outage. + // + // Note the existing semantics: scanTextForWebsites calls + // Attachment.delete(..., urlDelete = true), which HARD deletes. The + // visible = 0 soft-delete applies only when a user removes an attachment + // directly while its note still exists (urlDelete = false). + // + test('still removes an attachment whose url left the note', async () => { + Schema.__setReady(true) + const url = 'https://example.com/will-be-removed' + await clearCache(url) + FetchMock.route(url, () => page('Removed Later')) + + const attachmentId = await Attachment.createPendingUrlAttachment(userId, noteId, url) + await ScrapeQueue.enqueueUrl({ userId, noteId, attachmentId, url }) + expect(await attachmentRow(attachmentId)).not.toBeNull() + + // Now the schema "disappears" and the note is saved without that link. + Schema.__setReady(false) + await Attachment.scanTextForWebsites(userId, noteId, 'no links here any more') + + // Attachment.delete is deliberately fire-and-forget in scanTextForWebsites, + // so give it a tick to land. + await new Promise(resolve => setTimeout(resolve, 150)) + + expect(await attachmentRow(attachmentId)).toBeNull() + }) + + test('scrapeUrlsCreateAttachments resolves empty without touching the DB', async () => { + FetchMock.route(/.*/, () => page('nope')) + + await expect(Attachment.scrapeUrlsCreateAttachments(userId, null, ['https://example.com/x'])) + .resolves.toBe('') + expect(FetchMock.callCount()).toBe(0) + }) + + test('pushUrl keeps its contract and resolves truthy', async () => { + FetchMock.route(/.*/, () => page('nope')) + + await expect(Attachment.pushUrl('any-key', 'https://example.com/x')).resolves.toBe(true) + expect(FetchMock.callCount()).toBe(0) + }) + + test('recoverPending returns zero without querying', async () => { + await expect(ScrapeQueue.recoverPending()).resolves.toEqual({ requeued: 0, retired: 0 }) + }) + + test('enqueueUrl is a no-op backstop', async () => { + FetchMock.route(/.*/, () => page('nope')) + + await expect(ScrapeQueue.enqueueUrl({ + userId, noteId: null, attachmentId: null, url: 'https://example.com/x', + })).resolves.toBe('') + expect(FetchMock.callCount()).toBe(0) + }) +}) + describe('crash recovery', () => { test('re-enqueues rows left pending by a restart', async () => { diff --git a/server/workers/ScrapeQueue.js b/server/workers/ScrapeQueue.js index 3b13adc..ba8d8f6 100644 --- a/server/workers/ScrapeQueue.js +++ b/server/workers/ScrapeQueue.js @@ -21,6 +21,7 @@ const HttpFetch = require('@helpers/HttpFetch') const HostQueue = require('@helpers/HostQueue') const Robots = require('@helpers/Robots') const Realtime = require('@helpers/Realtime') +const Schema = require('@helpers/Schema') const SiteScrape = require('@helpers/SiteScrape') const ScrapeCache = require('@models/ScrapeCache') @@ -354,6 +355,10 @@ ScrapeQueue.enqueueUrl = ({ userId, noteId, attachmentId, url }) => { const runJob = async ({ userId, noteId, attachmentId, url }) => { if (!settings.enabled) { return '' } + // Backstop, so no future caller can reach the DB layer unguarded when the + // scrape columns are absent. + if (!Schema.isScrapeSchemaReady()) { return '' } + const pending = perUser.get(userId) || 0 if (pending >= settings.maxPendingPerUser) { // Left at scrape_state = 0, so recoverPending() picks it up later. @@ -426,6 +431,10 @@ ScrapeQueue.awaitUrl = async (promise, capMs) => { ScrapeQueue.recoverPending = async () => { if (!settings.enabled) { return { requeued: 0, retired: 0 } } + // Every query below references scrape_state, which only exists once an + // admin has run ./applyScrapeSchema.sh. + if (!Schema.isScrapeSchemaReady()) { return { requeued: 0, retired: 0 } } + // Anything left at FETCHING died with the previous process. await db.promise().query( 'UPDATE attachment SET scrape_state = ? WHERE scrape_state = ?',