Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions apps/web/app/lib/json-ld.test.ts
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('&amp;');
expect(out).not.toContain('&gt;');
});

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();
});
});
26 changes: 26 additions & 0 deletions apps/web/app/lib/json-ld.ts
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.
Comment thread
todorkolev marked this conversation as resolved.
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
Comment thread
todorkolev marked this conversation as resolved.
// any value even though today's callers always pass an object.
const json = JSON.stringify(value);
Comment thread
todorkolev marked this conversation as resolved.
if (json === undefined) return 'null';
return json
.replace(/</g, '\\u003c')
Comment thread
todorkolev marked this conversation as resolved.
.replace(/\u2028/g, '\\u2028')
Comment thread
todorkolev marked this conversation as resolved.
Comment thread
todorkolev marked this conversation as resolved.
.replace(/\u2029/g, '\\u2029');
}
3 changes: 2 additions & 1 deletion apps/web/app/root.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { AccessibilityWidget } from './components/AccessibilityWidget';
import { ScrollToTop } from './components/ScrollToTop';
import { PageHeader } from './components/PageHeader';
import { getCoverageMeta } from './lib/coverage';
import { serializeJsonForScript } from './lib/json-ld';
import { withDbRetry } from './lib/retry';
import stylesheet from './app.css?url';

Expand Down Expand Up @@ -70,7 +71,7 @@ export function Layout({ children }: { children: React.ReactNode }) {
const origin = rootData?.origin;
const imageUrl = origin ? `${origin}/og.png` : undefined;
const schemaOrg = origin
? JSON.stringify({
? serializeJsonForScript({
'@context': 'https://schema.org',
'@graph': [
{
Expand Down
16 changes: 8 additions & 8 deletions apps/web/app/routes/contract.json.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,23 @@ import { contractIdFromSlug, getContract, getDb } from '@sigma/db';
import type { Route } from './+types/contract.json';
import { publicCache } from '../lib/cache';
import { withDataSource } from '../lib/dataSource';

function safeJson(value: unknown): string {
return JSON.stringify(value)
.replace(/\u2028/g, '\\u2028')
.replace(/\u2029/g, '\\u2029')
.replace(/<\//g, '<\\/');
}
import { serializeJsonForScript } from '../lib/json-ld';

// Resource route: the assembled contract record as machine-readable JSON (/contracts/:id.json).
export async function loader({ params, context }: Route.LoaderArgs) {
const id = (params.id ?? '').replace(/\.json$/, '');
const record = await getContract(getDb(context.cloudflare.env), contractIdFromSlug(id));
if (!record) return withDataSource(Response.json({ error: 'not_found' }, { status: 404 }));
// Shared serializer (lib/json-ld.ts) instead of a second local copy \u2014 same `<`/U+2028/U+2029
// escaping, JSON-equivalent, so the two can't drift. Escaping the content is defense in
// depth; the actual MIME-sniffing guard is `X-Content-Type-Options: nosniff` \u2014 the worker sets it
// globally (baseSecurityHeaders), and it is set explicitly here too so this resource route is safe
// on its own, not only via the global layer.
return withDataSource(
Comment thread
todorkolev marked this conversation as resolved.
new Response(safeJson(record), {
new Response(serializeJsonForScript(record), {
headers: {
'Content-Type': 'application/json; charset=utf-8',
Comment thread
todorkolev marked this conversation as resolved.
'X-Content-Type-Options': 'nosniff',
'Cache-Control': publicCache(3600),
},
}),
Expand Down
31 changes: 31 additions & 0 deletions packages/db/migrations/0005_list_sort_indexes.sql
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.

Comment thread
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
Comment thread
todorkolev marked this conversation as resolved.
Comment thread
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);
Comment thread
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
Comment thread
todorkolev marked this conversation as resolved.
Comment thread
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);
207 changes: 207 additions & 0 deletions packages/db/src/list-sort-indexes.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,207 @@
/// <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 is asserted for the UNFILTERED sort paths (the default list views) and, for
// contracts, with an active list filter as well (FILTERED_SORTS below): the planner keeps walking the
// ordering index and still drops the sort step, so a filtered page is not a full-corpus sort either.

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], {
Comment thread
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 =
Comment thread
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;

// The same defect/fix, but with an active list filter on top of the sort — the case a reader of the
// unfiltered assertions above cannot infer. Filtering does NOT make the ordering index redundant: the
// planner keeps walking it and drops the sort step, so a filtered page stops being a whole-table sort
// too. Contracts only: it is the one list whose filters (sector via tenders.cpv_code, EU funding)
// join out to another table, so it is the case where the planner could most plausibly switch away.
const FILTERED_SORTS = [
{
name: 'contracts date-desc + sector filter',
index: 'idx_contracts_signed_desc',
sql: `SELECT c.id ${CONTRACTS_FROM} WHERE t.cpv_code LIKE '45%' ORDER BY COALESCE(c.signed_at, '') DESC, c.id DESC LIMIT 16`,
},
{
name: 'contracts date-desc + eu-funded filter',
index: 'idx_contracts_signed_desc',
sql: `SELECT c.id ${CONTRACTS_FROM} WHERE c.eu_funded = 1 ORDER BY COALESCE(c.signed_at, '') DESC, c.id DESC LIMIT 16`,
},
] 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;
Comment thread
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(() => {
// Every step below shells out to `sqlite3`. Without this probe a missing binary surfaces as an
// opaque ENOENT from the first execFileSync, which reads like a broken test rather than a missing
// tool. Fail loudly with the fix instead — deliberately NOT a skip: this file is a perf/cost gate,
// and silently passing it on a runner image that dropped sqlite3 would retire the gate unnoticed.
try {
execFileSync('sqlite3', ['-version'], { stdio: 'pipe' });
} catch {
throw new Error(
'The `sqlite3` CLI is required by this suite (the migration, refresh-slice and ship-domain ' +
'tests exec it too) but is not on PATH. Install it (e.g. `apt-get install -y sqlite3`) and re-run.',
);
}
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');
}
},
);

it.each(FILTERED_SORTS)('$name full-scans + sorts BEFORE the fix', ({ sql }) => {
expect(plan(before, sql)).toContain('USE TEMP B-TREE FOR ORDER BY');
});

it.each(FILTERED_SORTS)('$name still walks $index 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');
});
});
Comment thread
todorkolev marked this conversation as resolved.
Loading