feat(search): FTS5 lexical search + security hardening for /api/search - #1058
Open
mmcintosh wants to merge 9 commits into
Open
feat(search): FTS5 lexical search + security hardening for /api/search#1058mmcintosh wants to merge 9 commits into
mmcintosh wants to merge 9 commits into
Conversation
Port infowall FTS5 lexical search into sonicjs-org on the document model. - 0003_documents_fts.sql: documents_fts virtual table (10-col layout, bm25 parity) - DocumentProjection: FTS upsert/delete folded into the document write batch (all write paths incl. updateInPlace), reindexType for backfill/admin reindex - ai-search-plugin: keyword mode now backed by FTS5 engine (legacy content LIKE scan removed), KV settings + search cache, degraded flag - searchable-text harvester, fts5-sanitize (verbatim port), d1-retry - schema: kind:'fulltext' + weight on queryable fields - tests: 6 new sqlite/unit suites + e2e 82-fts5-search Not yet pushed/PR'd to upstream (testing on Infowall infra first).
…r unpublish/delete E2E spec 82 caught a correctness/security leak the unit tests asserted as expected behavior: the KV result cache (cacheTtlSeconds=60 by default) was only invalidated on settings change, never on document writes, so an unpublished/deleted doc stayed in public search results for the TTL. - DEFAULT_FTS_SETTINGS.cacheTtlSeconds = 0 (cache opt-in until write-path invalidation exists) - fts-search-cache.ts doc: proper re-enable = per-tenant search-index version folded into resultCacheKey (needs CACHE_KV threaded into core) - cache unit test updated to opt-in explicitly - plan v2: findings recorded; migrations-bundle timestamp regen
blog_post's seeded document type now declares { name: 'content', kind: 'fulltext' },
so the projection renders the post body (lexical editor stores HTML) into
documents_fts.body — body text becomes keyword-searchable and snippet() returns
highlighted prose instead of coming back empty.
- ai-search manifest 1.1.0: description now reflects the two-engine reality
(always-on FTS5 keyword + optional AI); registry regenerated from manifests
- real-SQLite test: seeded blog_post indexes HTML content, tags stripped
- E2E 82: body-only marker found with <mark> snippet; describe tagged @api
so CI's tag-grep actually selects the spec
Note: bootstrap is skipped when the version-keyed KV marker
(_sonicjs_bootstrap_v<version>) exists, so an already-booted environment must
clear that KV key (or bump the version) for the seed UPDATE to apply.
Existing posts index on next write; reindexType() backfills in bulk.
…ng docs main has since taken 0003 (session_org) and 0004 (forms) for unrelated features — this branch's 0003_documents_fts.sql collided. Renumbered to the next free slot (0005), synced both migrations/ copies, updated the two test harnesses that reference the filename by string, and neutralized one internal-project cross-reference in the migration's header comment. Also dropped the SEARCH-FTS5-PLUGIN-*.md planning/handoff docs — internal working notes from building this, not meant to ship upstream.
… allowlist, XSS-safe highlighting
Three real gaps in the FTS5 keyword-search path, found in review:
- filters.status from the request body could relax the published-only gate
(e.g. filters:{status:['draft']}) on the unauthenticated /api/search
endpoint. publishedOnly is now hardcoded true, server-authoritative, never
derived from client input.
- No restriction on which document types were searchable — internal-only
types (api_key, security_event, analytics_event, user_profile, rbac_*,
site_settings, …) were reachable by an anonymous caller who knew/guessed
the type id. Added publicReadableTypeIds(), intersected against any
requested type scope; empty scope returns no results rather than falling
back to "all types" (the engine's default for an empty/omitted filter).
- FTS5's highlight()/snippet() wrap matches in literal <mark> tags around
RAW, unescaped title/body text — a malicious <img onerror=...> in a
document title would render live in the highlighted result. Fixed with
private-use sentinel characters as the match delimiters: escapeHtml() the
full result first, then swap sentinels for real <mark> tags — so only the
highlight markup we control survives.
Also: a documents_fts self-heal (an existing install that upgrades core past
this migration but hasn't run `wrangler d1 migrations apply` yet would
otherwise brick every document write, not just search — every DocumentsService
write folds an unconditional FTS delete into its batch), a DoS cap on the
public endpoint's result limit, and two deindex fixes for admin-content.ts's
delete paths that bypass DocumentProjection's normal FTS cleanup (a raw
deleted_at UPDATE otherwise leaves a deleted doc searchable indefinitely).
Known, intentional tradeoff: the admin search UI's Draft/Archived status
filter will now silently return nothing (server-authoritative published-only
applies there too) — authenticated non-published search is a separate future
endpoint, not this one. Flagged as follow-up debt.
The previous commit fixed admin-content.ts's two delete paths (raw deleted_at UPDATE, bypassing DocumentProjection's normal FTS cleanup) but missed two more instances of the exact same pattern: DELETE /api/:collection/:id (routes/api.ts) and DELETE /api/content/:id (routes/api-content-crud.ts). Both are real, generic, authenticated content-delete endpoints — after the previous commit landed, deleting through either of these two still left the document fully discoverable (title + body snippet) via the public, unauthenticated /api/search indefinitely. Same fix as the two already-patched sites: an explicit `DELETE FROM documents_fts` follow-up alongside the raw UPDATE. DocumentsService.softDelete() already handles this correctly in one batch, but it isn't a drop-in replacement here — it takes a single document `id`, while both of these routes soft-delete an entire root (WHERE root_id = ?) — so this stays consistent with the pattern the sibling fix already established rather than changing these routes' semantics. Real-SQLite regression tests added to both routes' existing integration suites (assert a documents_fts row exists post-publish, then is gone post-delete). The two matching E2E cases for these routes are in the previous commit's spec 105, alongside the rest of the search-hardening E2E coverage.
Two gaps found in a fresh review of the two prior commits, both real: - MediaDocumentService.softDeleteRoot() has the same raw deleted_at UPDATE pattern the last two commits fixed on the content-delete routes, missed because it lives in a service file, not a route file. media_asset carries baseGrants.public:['read'], so it's in the new publicReadableTypeIds() allowlist — a deleted file's filename stayed permanently, publicly searchable via unauthenticated /api/search. Reachable from three live authenticated routes: DELETE /api/media/:id, bulk media delete, and the admin media library delete action. Same fix as the other four sites: an explicit FTS deindex alongside the raw UPDATE. - Fts5Engine escaped title/snippet before this pass but never slug, despite it going through the exact same JSON response. Not currently exploitable (the only known consumer never renders it via innerHTML, and slug has no charset restriction at the schema layer to rule out attacker content) — but inconsistent with the escaping this same commit series added everywhere else, so closed the gap rather than leave it as a landmine. Regression tests added for both (media-documents.test.ts asserts a documents_fts row exists post-upload and is gone post-delete; ai-search-keyword-fts.sqlite.test.ts asserts a malicious slug is escaped in search results).
…eploy The public search API is core-promoted (mounted in app.ts before the /api/:collection catch-all), not plugin-gated, so a disableAll:true bare-core deploy still serves it. Two guards: - ai-search-disableall.integration.test.ts (runs in CI): with NO plugins mounted, an anonymous POST /api/search returns published results; a self-check reverses the mount order and asserts the historical 401 shadow returns, so the test is provably sensitive to the regression it guards. Uses the mock-middleware route harness (no createSonicJSApp / better-auth) to stay inside the CI-runnable set. - mount-integration.test.ts: route-table assertion that /api/search is registered as a core route under disableAll (quarantined suite; runs when the better-auth CI gap lifts).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Wires FTS5 (BM25-ranked lexical search, snippet/highlight, porter-stemmed tokenization) in as the
ai-search-plugin'skeywordmode backend — replacing today'scontent/collections-basedLIKE '%term%'scan, which has no ranking and currently returns nothing on any install matching this repo's actual migrations (those legacy tables were dropped during the document-model migration; the plugin was never updated to match). Indexed at write-time via the existingDocumentProjectionseam.Also closes three real gaps in
POST /api/search(unauthenticated, no CSRF gate) found while building this, plus five instances of the same underlying bug (a rawdeleted_atUPDATE that bypassesDocumentProjection's FTS cleanup, leaving deleted content/media searchable forever) across four route files and one service.Changes
documents_ftsFTS5 virtual table (migration0005) +Fts5Engine(BM25 ranking,snippet()/highlight()), wired askeywordmode's backend./api/searchwas shadowed and plugin-gated: the plugin mountedPOST /api/searchafter the genericapp.route('/api', apiRoutes)catch-all, so requests were silently captured by/api/:collection(POST /api/search→401) and/api/:collection/:id(GET /api/search/analytics→404) — the plugin's own handlers never ran. Confirmed on a livewrangler dev(not just a routing probe):POST /api/searchreturned{"error":"Authentication required"}, byte-identical toPOST /api/nonexistentcollection. This PR promotes the search API out of the plugin and into coreapp.ts, mounted before the catch-all. Consequences: (a) search actually works; (b) the endpoint can no longer be loaded un-hardened, because the hardened handler is the mount; (c) a plugins-off (disableAll: true, "bare core") deploy still serves a working, hardened/api/searchrather than the endpoint disappearing. This supersedes fix(search): close SQLi, draft leak, and DoS in public /api/search #1064, whose standalone plugin-level hardening protected code that had no reachable HTTP path.kind:'fulltext'queryable field.filters.statusbypass: the request body'sfilters.statuscould relax the published-only gate on the unauthenticated search endpoint (e.g.filters:{status:['draft']}).publishedOnlyis now hardcodedtrue, server-authoritative, never derived from client input.api_key,security_event,user_profile,rbac_*, …) were reachable by an anonymous caller who knew/guessed the type id. Added apublicReadableTypeIds()allowlist (settings.baseGrants.publicincludes'read'); an empty resulting scope returns no results rather than falling back to "all types."highlight()/snippet()wrap matches in literal tags around raw, unescaped title/body/slug text. Fixed with private-use sentinel characters as match delimiters — escape the full result first, then swap sentinels for real<mark>tags, so only markup we control survives.admin-content.ts(bulk-action +DELETE /:id),routes/api.ts(DELETE /:collection/:id),routes/api-content-crud.ts(DELETE /:id), andMediaDocumentService.softDeleteRoot()all do a rawdeleted_atUPDATE that bypassesDocumentProjection's normal FTS deindex — deleted content/media stayed searchable indefinitely. All five now explicitly deindex.documents_ftsself-heal inMigrationService— an install that's past this migration in code but hasn't runwrangler d1 migrations applyyet would otherwise brick every document write (everyDocumentsServicewrite folds an FTS delete into its batch), not just search.MAX_LIMIT=100cap on the public endpoint's result limit (DoS guard).cache defaultActivemanifest fix, both independent, pre-existing bugs found while building this.Known, intentional tradeoff: the admin search UI's Draft/Archived status filter will now silently return nothing when routed through keyword mode (server-authoritative published-only applies there too) — authenticated non-published search is a separate future endpoint, not this one.
Out of scope, flagged not fixed: the sibling
mode:'ai'search path (Vectorize/Workers AI) has its own pre-existing published-only gap and an unimplementedremoveContentFromIndex()— both predate this PR (from the original AI-search plugin) and don't fire without Vectorize bindings configured (not in the default install). Worth a follow-up issue.Testing
Reviewed for security/reality/blast-radius across three independent passes before opening.
Unit Tests
ai-search-disableall.integration.test.ts— with every plugin disabled (disableAll) an anonymousPOST /api/searchstill returns published results; a self-check reverses the core-mount order and asserts the historical401shadow returns, proving the test is sensitive to the exact regression it guards. (mount-integration.test.tsalso asserts, via app-factory route-table introspection, that/api/searchis registered as a core route underdisableAll— that suite is in the better-auth CI-quarantine list, so it runs locally / when the quarantine lifts.)tsc --noEmitclean.E2E Tests
105-fts5-search.spec.ts, renumbered from 82 to avoid colliding with an unrelated existing spec onmain): full create→publish→search flow, draft exclusion, unpublish/delete deindex across all three content-delete routes, and the XSS-escaping regression.Screenshots/Videos
No admin UI changes, but here's the live
POST /api/searchbehavior on a local run of this branch:BM25-ranked keyword search,
<mark>-highlighted title:filters.statusbypass attempt from an unauthenticated caller — ignored server-side, no draft content leaked:Checklist