-
Notifications
You must be signed in to change notification settings - Fork 43
fix: индекси за листовите сортове + екраниране на JSON-LD (перформанс + hardening) #212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 9 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
043942b
perf(db): ordering indexes for the non-default list sorts
B353N 061e53e
fix(web): escape < in the JSON-LD data island (defense-in-depth)
B353N 088476d
Merge branch 'main' into fix/list-hardening
B353N 7b60ff8
chore(db): renumber list-sort-indexes migration 0002 → 0005
B353N bd6101a
test(db): apply all migrations + cover keyset pages; guard jsonLdScri…
B353N 8fbf478
refactor(web): rename jsonLdScript → serializeJsonForScript + documen…
B353N 0a59f1b
docs(db): state the boundaries of the sort-index guarantee (review)
B353N b41b243
chore: drop internal review-marker traces from code comments
B353N 11ae02a
refactor(web): share one JSON-for-script serializer between the JSON-…
B353N 1985da4
fix(web): set nosniff on the .json route; add planner-independent sen…
B353N 727038e
chore: drop stray review-marker artifacts from this PR's comments
B353N cc57e04
Merge branch 'main' into fix/list-hardening
B353N 24c5054
test(db): cover filtered list sorts and guard the sqlite3 dependency
todorkolev f4907fa
Merge branch 'main' into fix/list-hardening
todorkolev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { serializeJsonForScript } from './json-ld'; | ||
|
|
||
| const LS = String.fromCharCode(0x2028); // U+2028 line separator | ||
| const PS = String.fromCharCode(0x2029); // U+2029 paragraph separator | ||
|
|
||
| describe('serializeJsonForScript', () => { | ||
| it('escapes < so a string value cannot close the <script> element', () => { | ||
| const out = serializeJsonForScript({ url: 'https://x/</script><script>alert(1)</script>' }); | ||
| // The raw breakout sequence must not survive — this is the whole point of the sink. | ||
| expect(out).not.toContain('</script>'); | ||
| expect(out).not.toContain('<script>'); | ||
| expect(out).toContain('\\u003c/script'); | ||
| }); | ||
|
|
||
| it('is JSON-equivalent: parsing the output returns the identical value', () => { | ||
| const value = { | ||
| name: 'A </script> B', | ||
| nested: ['<a>', { k: '</SCRIPT >' }], | ||
| n: 42, | ||
| }; | ||
| expect(JSON.parse(serializeJsonForScript(value))).toEqual(value); | ||
| }); | ||
|
|
||
| it('escapes the U+2028 / U+2029 line/paragraph separators', () => { | ||
| const value = { s: `a${LS}b${PS}c` }; | ||
| const out = serializeJsonForScript(value); | ||
| expect(out).toContain('\\u2028'); | ||
| expect(out).toContain('\\u2029'); | ||
| expect(out).not.toContain(LS); | ||
| expect(out).not.toContain(PS); | ||
| // Still round-trips to the original value. | ||
| expect(JSON.parse(out)).toEqual(value); | ||
| }); | ||
|
|
||
| it('leaves injection-free content byte-identical to JSON.stringify', () => { | ||
| const value = { '@context': 'https://schema.org', name: 'СИГМА', url: 'https://sigma.midt.bg' }; | ||
| expect(serializeJsonForScript(value)).toBe(JSON.stringify(value)); | ||
| }); | ||
|
|
||
| it('does NOT escape > or & (only < can break out of a script raw-text context)', () => { | ||
| const out = serializeJsonForScript({ s: 'a > b && c' }); | ||
| expect(out).toContain('a > b && c'); // left verbatim, byte-minimal | ||
| expect(out).not.toContain('&'); | ||
| expect(out).not.toContain('>'); | ||
| }); | ||
|
|
||
| it('returns valid JSON (not a throw) for values that stringify to undefined', () => { | ||
| // JSON.stringify(undefined | function | symbol) === undefined; the helper must not call .replace | ||
| // on it. Emitting "null" keeps the <script> body parseable. | ||
| expect(serializeJsonForScript(undefined)).toBe('null'); | ||
| expect(serializeJsonForScript(() => 1)).toBe('null'); | ||
| expect(JSON.parse(serializeJsonForScript(undefined))).toBeNull(); | ||
| }); | ||
| }); |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| // Serialize a value for embedding inside an inline <script> (e.g. a JSON-LD data island). Plain | ||
| // JSON.stringify does NOT escape `<`, so a raw `</script>` in any string value would close the | ||
| // script element early and inject markup (stored XSS). Escaping `<` as \u003c is JSON-equivalent | ||
| // — JSON.parse returns the identical value — and closes that hole; the U+2028/U+2029 escapes keep | ||
| // the payload safe if a consumer evaluates it as JS rather than parsing it as JSON. | ||
| // | ||
| // `>` and `&` are deliberately NOT escaped: only `<` can start a markup/comment token in a script | ||
| // raw-text context (`</script`, `<!--`, `<script`), and this is not an HTML-attribute context, so | ||
| // `>`/`&` need no escaping — leaving them keeps the output byte-minimal and still valid JSON. | ||
| // | ||
| // This is the SINGLE shared implementation of the project's review standard (docs/review-security.md | ||
| // "Инжекции и валидация"): both root.tsx's JSON-LD island and routes/contract.json.tsx's response use | ||
| // it, so the two sinks cannot drift. Kept as defense-in-depth — today the only value reaching the | ||
| // JSON-LD is the request origin (which `new URL()` cannot make carry `</script>`), but this keeps the | ||
| // sink safe for any DB/user-derived field added later. | ||
| export function serializeJsonForScript(value: unknown): string { | ||
| // JSON.stringify returns `undefined` (not a string) for `undefined`, a function, or a symbol \u2014 a | ||
| // later `.replace` on it would throw. Emit valid JSON (`null`) instead, so the helper is safe for | ||
|
todorkolev marked this conversation as resolved.
|
||
| // any value even though today's callers always pass an object. | ||
| const json = JSON.stringify(value); | ||
|
todorkolev marked this conversation as resolved.
|
||
| if (json === undefined) return 'null'; | ||
| return json | ||
| .replace(/</g, '\\u003c') | ||
|
todorkolev marked this conversation as resolved.
|
||
| .replace(/\u2028/g, '\\u2028') | ||
|
todorkolev marked this conversation as resolved.
todorkolev marked this conversation as resolved.
|
||
| .replace(/\u2029/g, '\\u2029'); | ||
| } | ||
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| -- Ordering indexes for the non-default list sorts, so a keyset page walks an index and stops at | ||
| -- LIMIT instead of scanning + temp-B-tree-sorting the whole table on every request (D1 bills rows | ||
| -- SCANNED). The default sorts already had matching indexes (idx_contracts_value_desc/asc, | ||
| -- idx_company_totals_won/name, idx_authority_totals_spent/name); these cover the sorts that were | ||
| -- missing one. Each index matches the EXACT ORDER BY expression the query layer emits (the COALESCE | ||
| -- forms in queries/contracts.ts SORTS) plus the keyset id tiebreak in the same direction, so SQLite | ||
| -- neither sorts nor buffers. Additive + idempotent; the rollup tables are DELETE+INSERT-refreshed | ||
| -- (never dropped), so these survive every ETL ship. | ||
|
|
||
|
todorkolev marked this conversation as resolved.
|
||
| -- /contracts ?sort=date-desc | date-asc — ORDER BY COALESCE(signed_at, …), c.id (queries/contracts.ts). | ||
| -- idx_contracts_signed is on the bare signed_at column and does NOT match the COALESCE expression. | ||
| -- SYNC: the sentinels below ('' for desc, '9999-99' for asc) must stay byte-identical to the SORTS | ||
| -- map in queries/contracts.ts (`date-desc`/`date-asc` expr). If a default there changes, the index | ||
| -- expression stops matching and SQLite silently falls back to a full scan + temp-B-tree sort — change | ||
| -- both together. The list-sort-indexes.test.ts EXPLAIN assertions catch a drift. | ||
| CREATE INDEX IF NOT EXISTS idx_contracts_signed_desc | ||
|
todorkolev marked this conversation as resolved.
todorkolev marked this conversation as resolved.
|
||
| ON contracts(COALESCE(signed_at, '') DESC, id DESC); | ||
| CREATE INDEX IF NOT EXISTS idx_contracts_signed_asc | ||
| ON contracts(COALESCE(signed_at, '9999-99') ASC, id ASC); | ||
|
todorkolev marked this conversation as resolved.
|
||
|
|
||
| -- /companies ?sort=count | authorities — ORDER BY <col> DESC, bidder_id DESC (queries/companies.ts). | ||
| CREATE INDEX IF NOT EXISTS idx_company_totals_count | ||
|
todorkolev marked this conversation as resolved.
todorkolev marked this conversation as resolved.
|
||
| ON company_totals(contracts DESC, bidder_id DESC); | ||
| CREATE INDEX IF NOT EXISTS idx_company_totals_authorities | ||
| ON company_totals(authorities DESC, bidder_id DESC); | ||
|
|
||
| -- /authorities ?sort=count | avg — ORDER BY <col> DESC, authority_id DESC (queries/authorities.ts). | ||
| CREATE INDEX IF NOT EXISTS idx_authority_totals_count | ||
| ON authority_totals(contracts DESC, authority_id DESC); | ||
| CREATE INDEX IF NOT EXISTS idx_authority_totals_avg | ||
| ON authority_totals(avg_eur DESC, authority_id DESC); | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,167 @@ | ||
| /// <reference types="node" /> | ||
| import { execFileSync } from 'node:child_process'; | ||
| import { mkdtempSync, readdirSync, rmSync } from 'node:fs'; | ||
| import { tmpdir } from 'node:os'; | ||
| import { dirname, resolve } from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
| import { afterAll, beforeAll, describe, expect, it } from 'vitest'; | ||
|
|
||
| // The list pages paginate with a keyset ORDER BY <sortExpr> <dir>, <id> <dir> LIMIT N. When the sort | ||
| // column/expression has a matching index, SQLite walks it and stops at LIMIT. When it does NOT, the | ||
| // planner falls back to "SCAN <table> … USE TEMP B-TREE FOR ORDER BY": it reads and sorts the WHOLE | ||
| // table before applying LIMIT — a full-corpus scan on every request. D1 bills on rows SCANNED, so a | ||
| // missing ordering index is a real cost/latency defect (docs/review-security.md "D1 разход и индекси"). | ||
| // | ||
| // This proves, on a real sqlite3 with no ANALYZE stats (matching production D1), that BEFORE the | ||
| // list-sort-indexes migration the six non-default list sorts full-scan (temp-B-tree sort), and AFTER | ||
| // it each walks its new index with no ORDER BY sort step — on the first page AND on a keyset page. | ||
| // | ||
| // Known boundaries of this guarantee: | ||
| // - The local sqlite3 CLI's query planner is not version-identical to Cloudflare D1's; the EXPLAIN | ||
| // plans are a strong indication, not a bit-exact production proof. (The sqlite3 binary itself is a | ||
| // pre-existing suite-wide dependency — migrations/refresh-slice/ship-domain tests all exec it — so | ||
| // a missing binary fails the whole suite, not just this file.) | ||
| // - The index-walk claim covers the UNFILTERED sort paths (the default list views). With an active | ||
| // filter the planner may prefer the filter's index and sort the (small) filtered set instead — | ||
| // that is the correct trade, but it is not what this test asserts. | ||
|
|
||
| const root = resolve(dirname(fileURLToPath(import.meta.url)), '../../..'); | ||
| const migrationsDir = resolve(root, 'packages/db/migrations'); | ||
|
|
||
| // Apply EVERY migration on the branch, not a hardcoded subset — so the "BEFORE" base is exactly the | ||
| // real served schema minus this PR's index, and the test survives any renumbering. | ||
| const allMigrations = readdirSync(migrationsDir) | ||
| .filter((f) => f.endsWith('.sql')) | ||
| .sort(); | ||
| const sortIndexMigration = allMigrations.find((f) => f.includes('list_sort_indexes')); | ||
| if (!sortIndexMigration) throw new Error('list_sort_indexes migration not found'); | ||
| const baseMigrations = allMigrations.filter((f) => f !== sortIndexMigration); | ||
|
|
||
| function readScript(dbPath: string, file: string): void { | ||
| execFileSync('sqlite3', ['-bail', dbPath], { | ||
|
todorkolev marked this conversation as resolved.
|
||
| input: `.read ${resolve(migrationsDir, file)}\n`, | ||
| stdio: 'pipe', | ||
| }); | ||
| } | ||
|
|
||
| function plan(dbPath: string, sql: string): string { | ||
| return execFileSync('sqlite3', [dbPath], { | ||
| input: `EXPLAIN QUERY PLAN ${sql}\n`, | ||
| encoding: 'utf8', | ||
| }); | ||
| } | ||
|
|
||
| // Faithful shapes of the keyset list queries (queries/{contracts,companies,authorities}.ts): the same | ||
| // FROM/JOINs and the same ORDER BY expression, on the first page (no cursor) and on a keyset page | ||
| // (the `WHERE (expr <cmp> ? OR (expr = ? AND id <cmp> ?))` seek every page after the first uses). | ||
| const CONTRACTS_FROM = | ||
|
todorkolev marked this conversation as resolved.
|
||
| 'FROM contracts c JOIN tenders t ON t.id = c.tender_id ' + | ||
| 'JOIN authorities a ON a.id = t.authority_id JOIN bidders b ON b.id = c.bidder_id'; | ||
|
|
||
| const SORTS = [ | ||
| { | ||
| name: 'contracts date-desc', | ||
| index: 'idx_contracts_signed_desc', | ||
| firstPage: `SELECT c.id ${CONTRACTS_FROM} ORDER BY COALESCE(c.signed_at, '') DESC, c.id DESC LIMIT 16`, | ||
| keysetPage: `SELECT c.id ${CONTRACTS_FROM} WHERE (COALESCE(c.signed_at, '') < '2023-05-20' OR (COALESCE(c.signed_at, '') = '2023-05-20' AND c.id < 'c:500')) ORDER BY COALESCE(c.signed_at, '') DESC, c.id DESC LIMIT 16`, | ||
| }, | ||
| { | ||
| name: 'contracts date-asc', | ||
| index: 'idx_contracts_signed_asc', | ||
| firstPage: `SELECT c.id ${CONTRACTS_FROM} ORDER BY COALESCE(c.signed_at, '9999-99') ASC, c.id ASC LIMIT 16`, | ||
| keysetPage: `SELECT c.id ${CONTRACTS_FROM} WHERE (COALESCE(c.signed_at, '9999-99') > '2023-05-20' OR (COALESCE(c.signed_at, '9999-99') = '2023-05-20' AND c.id > 'c:500')) ORDER BY COALESCE(c.signed_at, '9999-99') ASC, c.id ASC LIMIT 16`, | ||
| }, | ||
| { | ||
| name: 'companies count-desc', | ||
| index: 'idx_company_totals_count', | ||
| firstPage: `SELECT bidder_id FROM company_totals ORDER BY contracts DESC, bidder_id DESC LIMIT 26`, | ||
| keysetPage: `SELECT bidder_id FROM company_totals WHERE (contracts < 10 OR (contracts = 10 AND bidder_id < 'eik:100')) ORDER BY contracts DESC, bidder_id DESC LIMIT 26`, | ||
| }, | ||
| { | ||
| name: 'companies authorities-desc', | ||
| index: 'idx_company_totals_authorities', | ||
| firstPage: `SELECT bidder_id FROM company_totals ORDER BY authorities DESC, bidder_id DESC LIMIT 26`, | ||
| keysetPage: `SELECT bidder_id FROM company_totals WHERE (authorities < 5 OR (authorities = 5 AND bidder_id < 'eik:100')) ORDER BY authorities DESC, bidder_id DESC LIMIT 26`, | ||
| }, | ||
| { | ||
| name: 'authorities count-desc', | ||
| index: 'idx_authority_totals_count', | ||
| firstPage: `SELECT authority_id FROM authority_totals ORDER BY contracts DESC, authority_id DESC LIMIT 26`, | ||
| keysetPage: `SELECT authority_id FROM authority_totals WHERE (contracts < 10 OR (contracts = 10 AND authority_id < 'auth:50')) ORDER BY contracts DESC, authority_id DESC LIMIT 26`, | ||
| }, | ||
| { | ||
| name: 'authorities avg-desc', | ||
| index: 'idx_authority_totals_avg', | ||
| firstPage: `SELECT authority_id FROM authority_totals ORDER BY avg_eur DESC, authority_id DESC LIMIT 26`, | ||
| keysetPage: `SELECT authority_id FROM authority_totals WHERE (avg_eur < 100 OR (avg_eur = 100 AND authority_id < 'auth:50')) ORDER BY avg_eur DESC, authority_id DESC LIMIT 26`, | ||
| }, | ||
| ] as const; | ||
|
|
||
| // A modest, unanalyzed dataset — enough that the planner weighs a real table, none of ANALYZE's | ||
| // stats (production D1 never runs ANALYZE; verified via grep over scripts/ + migrations). | ||
| function seed(dbPath: string): void { | ||
| const stmts: string[] = ['BEGIN;']; | ||
| for (let i = 0; i < 40; i++) | ||
| stmts.push(`INSERT INTO authorities(id,name) VALUES('auth:${i}','A${i}');`); | ||
| for (let i = 0; i < 60; i++) | ||
| stmts.push(`INSERT INTO bidders(id,name) VALUES('eik:${i}','B${i}');`); | ||
| for (let i = 0; i < 120; i++) | ||
| stmts.push( | ||
| `INSERT INTO tenders(id,source_id,title,authority_id,cpv_code,procedure_type,status) ` + | ||
| `VALUES('t:${i}','U${i}','T${i}','auth:${i % 40}','45000000','открита','awarded');`, | ||
| ); | ||
| for (let i = 0; i < 600; i++) | ||
| stmts.push( | ||
| `INSERT INTO contracts(id,tender_id,bidder_id,amount,amount_eur,currency,value_flag,signed_at,bids_received) ` + | ||
| `VALUES('c:${i}','t:${i % 120}','eik:${i % 60}',${i * 10},${i * 10},'EUR','ok','202${i % 5}-0${(i % 9) + 1}-15',${(i % 4) + 1});`, | ||
| ); | ||
| for (let i = 0; i < 300; i++) | ||
| stmts.push( | ||
| `INSERT INTO company_totals(bidder_id,name,kind,eik_valid,won_eur,contracts,authorities,eu_eur) ` + | ||
| `VALUES('eik:${i}','B${i}','company',1,${i * 100},${i % 50},${i % 20},0);`, | ||
| ); | ||
| for (let i = 0; i < 200; i++) | ||
| stmts.push( | ||
| `INSERT INTO authority_totals(authority_id,name,spent_eur,contracts,suppliers,avg_eur,eu_eur) ` + | ||
| `VALUES('auth:${i}','A${i}',${i * 1000},${i % 90},${i % 30},${i * 3},0);`, | ||
| ); | ||
| stmts.push('COMMIT;'); | ||
| execFileSync('sqlite3', ['-bail', dbPath], { input: stmts.join('\n'), stdio: 'pipe' }); | ||
| } | ||
|
|
||
| describe('list sort ordering indexes', () => { | ||
| let dir: string; | ||
|
todorkolev marked this conversation as resolved.
|
||
| let before: string; // every migration EXCEPT the sort-index one (= real main minus this PR) | ||
| let after: string; // every migration (base + the sort-index one) | ||
|
|
||
| beforeAll(() => { | ||
| dir = mkdtempSync(resolve(tmpdir(), 'sigma-sort-idx-')); | ||
| before = resolve(dir, 'before.sqlite'); | ||
| after = resolve(dir, 'after.sqlite'); | ||
| for (const db of [before, after]) { | ||
| for (const m of baseMigrations) readScript(db, m); | ||
| seed(db); | ||
| } | ||
| readScript(after, sortIndexMigration); | ||
| }); | ||
|
|
||
| afterAll(() => rmSync(dir, { recursive: true, force: true })); | ||
|
|
||
| // The defect: without the ordering index, the sort sorts the whole table — first page AND keyset page. | ||
| it.each(SORTS)('$name full-scans + sorts BEFORE the fix', ({ firstPage, keysetPage }) => { | ||
| expect(plan(before, firstPage)).toContain('USE TEMP B-TREE FOR ORDER BY'); | ||
| expect(plan(before, keysetPage)).toContain('USE TEMP B-TREE FOR ORDER BY'); | ||
| }); | ||
|
|
||
| // The fix: each sort walks its dedicated index and drops the ORDER BY sort step — on both pages. | ||
| it.each(SORTS)( | ||
| '$name walks $index with no sort step AFTER the fix', | ||
| ({ index, firstPage, keysetPage }) => { | ||
| for (const sql of [firstPage, keysetPage]) { | ||
| const p = plan(after, sql); | ||
| expect(p).toContain(index); | ||
| expect(p).not.toContain('USE TEMP B-TREE FOR ORDER BY'); | ||
| } | ||
| }, | ||
| ); | ||
| }); | ||
|
todorkolev marked this conversation as resolved.
|
||
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
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.
Uh oh!
There was an error while loading. Please reload this page.