-
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 3 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
Some comments aren't visible on the classic Files Changed page.
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,40 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { jsonLdScript } from './json-ld'; | ||
|
|
||
| const LS = String.fromCharCode(0x2028); // U+2028 line separator | ||
| const PS = String.fromCharCode(0x2029); // U+2029 paragraph separator | ||
|
|
||
| describe('jsonLdScript', () => { | ||
| it('escapes < so a string value cannot close the <script> element', () => { | ||
| const out = jsonLdScript({ 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(jsonLdScript(value))).toEqual(value); | ||
| }); | ||
|
|
||
| it('escapes the U+2028 / U+2029 line/paragraph separators', () => { | ||
| const value = { s: `a${LS}b${PS}c` }; | ||
| const out = jsonLdScript(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(jsonLdScript(value)).toBe(JSON.stringify(value)); | ||
| }); | ||
| }); |
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,16 @@ | ||
| // 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. | ||
| // | ||
| // Mirrors the project's own review standard (docs/review-security.md "Инжекции и валидация") and the | ||
| // safeJson helper in routes/contract.json.tsx. Kept as defense-in-depth: today the only value that | ||
| // reaches root.tsx's JSON-LD is the request origin (which `new URL()` cannot make carry `</script>`), | ||
| // but this makes the sink safe for any DB/user-derived field added to the graph later. | ||
| export function jsonLdScript(value: unknown): string { | ||
|
todorkolev marked this conversation as resolved.
Outdated
|
||
| return JSON.stringify(value) | ||
|
todorkolev marked this conversation as resolved.
Outdated
|
||
| .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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| -- 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. | ||
|
|
||
| -- /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. | ||
| CREATE INDEX IF NOT EXISTS idx_contracts_signed_desc | ||
| 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); | ||
|
|
||
| -- /companies ?sort=count | authorities — ORDER BY <col> DESC, bidder_id DESC (queries/companies.ts). | ||
| CREATE INDEX IF NOT EXISTS idx_company_totals_count | ||
| 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,136 @@ | ||
| /// <reference types="node" /> | ||
| import { execFileSync } from 'node:child_process'; | ||
| import { mkdtempSync, 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: | ||
| // (a) BEFORE migration 0002 the six non-default list sorts full-scan (temp-B-tree sort), and | ||
| // (b) AFTER 0002 each walks its new index with no ORDER BY sort step. | ||
|
|
||
| const root = resolve(dirname(fileURLToPath(import.meta.url)), '../../..'); | ||
| const migration0 = resolve(root, 'packages/db/migrations/0000_init.sql'); | ||
| const migration1 = resolve(root, 'packages/db/migrations/0001_flow_pairs_bidder_index.sql'); | ||
| const migration2 = resolve(root, 'packages/db/migrations/0002_list_sort_indexes.sql'); | ||
|
|
||
| function readScript(dbPath: string, path: string): void { | ||
| execFileSync('sqlite3', ['-bail', dbPath], { input: `.read ${path}\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). Only the | ||
| // ORDER BY + FROM/JOINs drive the plan, so the SELECT list is trimmed to the id. | ||
| 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', | ||
| sql: `SELECT c.id ${CONTRACTS_FROM} ORDER BY COALESCE(c.signed_at, '') DESC, c.id DESC LIMIT 16`, | ||
| }, | ||
| { | ||
| name: 'contracts date-asc', | ||
| index: 'idx_contracts_signed_asc', | ||
| sql: `SELECT c.id ${CONTRACTS_FROM} ORDER BY COALESCE(c.signed_at, '9999-99') ASC, c.id ASC LIMIT 16`, | ||
| }, | ||
| { | ||
| name: 'companies count-desc', | ||
| index: 'idx_company_totals_count', | ||
| sql: `SELECT bidder_id FROM company_totals ORDER BY contracts DESC, bidder_id DESC LIMIT 26`, | ||
| }, | ||
| { | ||
| name: 'companies authorities-desc', | ||
| index: 'idx_company_totals_authorities', | ||
| sql: `SELECT bidder_id FROM company_totals ORDER BY authorities DESC, bidder_id DESC LIMIT 26`, | ||
| }, | ||
| { | ||
| name: 'authorities count-desc', | ||
| index: 'idx_authority_totals_count', | ||
| sql: `SELECT authority_id FROM authority_totals ORDER BY contracts DESC, authority_id DESC LIMIT 26`, | ||
| }, | ||
| { | ||
| name: 'authorities avg-desc', | ||
| index: 'idx_authority_totals_avg', | ||
| sql: `SELECT authority_id FROM authority_totals 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; // schema WITHOUT 0002 (current main) | ||
| let after: string; // schema WITH 0002 (the fix) | ||
|
|
||
| beforeAll(() => { | ||
| dir = mkdtempSync(resolve(tmpdir(), 'sigma-sort-idx-')); | ||
| before = resolve(dir, 'before.sqlite'); | ||
| after = resolve(dir, 'after.sqlite'); | ||
| for (const db of [before, after]) { | ||
| readScript(db, migration0); | ||
| readScript(db, migration1); | ||
| seed(db); | ||
| } | ||
| readScript(after, migration2); | ||
| }); | ||
|
|
||
| afterAll(() => rmSync(dir, { recursive: true, force: true })); | ||
|
|
||
| // The defect: without the ordering index, every one of these sorts sorts the whole table. | ||
| it.each(SORTS)('$name full-scans + sorts BEFORE the fix', ({ sql }) => { | ||
| expect(plan(before, sql)).toContain('USE TEMP B-TREE FOR ORDER BY'); | ||
| }); | ||
|
|
||
| // The fix: each sort walks its dedicated index and drops the ORDER BY sort step entirely. | ||
| it.each(SORTS)('$name walks $index with no sort step AFTER the fix', ({ index, sql }) => { | ||
| 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.
|
||
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.