docs(phase-12): add security threat verification
This commit is contained in:
parent
80897c62df
commit
ff55ff75c2
1 changed files with 81 additions and 0 deletions
|
|
@ -0,0 +1,81 @@
|
|||
---
|
||||
phase: 12
|
||||
slug: orders-invoices-company-matching
|
||||
status: verified
|
||||
threats_open: 0
|
||||
asvs_level: 1
|
||||
created: 2026-07-11
|
||||
---
|
||||
|
||||
# Phase 12 — Orders/Invoices/Company Matching — Security Verification
|
||||
|
||||
**Audit date:** 2026-07-11
|
||||
**ASVS Level:** 1
|
||||
**block_on:** none
|
||||
**Scope:** Plans 01–05 (`12-01` through `12-05`), plus post-acceptance code-review fixes
|
||||
CR-01, CR-02, WR-01, WR-02, WR-03 (commits `591fc5c`, `bddf612`, `cb8ae85`, `76a652d`,
|
||||
`1bb2b8b`/`be6f07b`).
|
||||
|
||||
Verification method: every `mitigate` threat was checked against the actual
|
||||
implementation file/line (not the plan prose, not the SUMMARY's self-report).
|
||||
Every `accept` threat was checked against the real, live-verified data scale
|
||||
that justified the acceptance. No threat was closed on code-structure
|
||||
resemblance alone.
|
||||
|
||||
## Threat Verification
|
||||
|
||||
| Threat ID | Category | Disposition | Verdict | Evidence |
|
||||
|-----------|----------|-------------|---------|----------|
|
||||
| T-12-01 | Tampering | mitigate | **CLOSED** | `migrations/093_pax8_orders_company_matching.sql` — every statement is `ADD COLUMN IF NOT EXISTS` / `CREATE INDEX IF NOT EXISTS` / `CREATE EXTENSION IF NOT EXISTS`, no `DROP`/destructive `ALTER`. `lib/services/pax8-company-matcher.ts:71-80` (`findCandidates`) binds the PAX8 name as `$1` in `similarity($1, company_name)`; `CANDIDATE_FLOOR` is a hardcoded module constant interpolated into the SQL, not external input. `lib/services/pax8-sync-service.ts:465-545` (`syncOrders` upsert) binds every PAX8 field value as `$1`…`$17`; `grep -c "company_name +"` and equivalent concatenation patterns return 0. |
|
||||
| T-12-02 | Tampering / Repudiation | mitigate | **CLOSED** | `AUTO_LINK_THRESHOLD = 0.9`, `TIE_MARGIN = 0.05` exported constants in `lib/services/pax8-company-matcher.ts:31,36`; `decide()` (lines 91-99) implements the tie-margin/threshold/top-3 logic exactly as specified. `lib/services/pax8-company-matcher.test.ts` — 6/6 tests passing, covering auto-link, below-threshold review, near-tie review, empty-candidate review, idempotency, dryRun (verified by direct test run, not SUMMARY claim). Human-verify checkpoint (12-05 Task 2) reviewed a 10-row auto-match sample and returned `"approved"` — recorded in `12-05-SUMMARY.md`. |
|
||||
| T-12-03 | Information Disclosure | mitigate | **CLOSED** | `scripts/verify-pax8-invoice-items.ts` and `scripts/verify-pax8-orders-matching.ts` read in full — neither references `access_token`, `client_secret`, `accessToken`, or any credential field; both print only counts, company names, item types, and scores. |
|
||||
| T-12-04 | Tampering | mitigate | **CLOSED** | `applyLink()` (`pax8-company-matcher.ts:110-137`) UPDATE contains both `match_method IS DISTINCT FROM 'manual'` and a `NOT EXISTS (... resolved_at IS NOT NULL ...)` guard, verified by direct file read (not grep-only). The `matchPax8Companies()` eligibility SELECT (lines 216-231) excludes `match_method = 'manual'` and rows with a human-resolved review via `NOT EXISTS (... r2.resolved_at IS NOT NULL ...)`. 12-05's SC#4 live double-run captured 80 auto-matched rows before/after a second `fullSync()` and confirmed an identical set (`12-05-SUMMARY.md`). |
|
||||
| T-12-05 | Elevation of Privilege | mitigate | **CLOSED** | Full read of `lib/services/pax8-client.ts` — the only `method:` override anywhere in the file is `'POST'` on the OAuth2 token exchange (`getToken()`, line 38 — authentication, not a PAX8 data-mutation call); every data-fetching method (`listAllCompanies`, `listAllSubscriptions`, `listAllProducts`, `listAllInvoices`, `listAllInvoiceItems`) goes through `fetchJson`/`paginateAll`, which issue plain `fetch()` calls with no method override (defaults to GET). `syncOrders()` in `pax8-sync-service.ts` calls only `listAllInvoices`/`listAllInvoiceItems`. |
|
||||
| T-12-06 | Denial of Service | accept | **CLOSED** (accepted risk, logged below) | Original accept rationale (limit=1000, static `ORDER BY name`) was superseded by WR-02 (`76a652d`): default `limit` raised to 10000, ordering changed to `ORDER BY COALESCE(r.detected_at, c.matched_at, 'epoch'::timestamptz) ASC` to prevent alphabetical starvation of never-yet-scored rows — confirmed present in `pax8-company-matcher.ts:194,216-229`. Live-verified scale (12-05): 118 pax8_companies (80 auto-matched + 16 no-candidate + 22 ambiguous) — 3 orders of magnitude under the new limit. |
|
||||
| T-12-07 | Denial of Service | accept | **CLOSED** (accepted risk, logged below) | Live-verified scale (12-05-SUMMARY.md): 94 invoices, 29,696 order items total — matches the ~94×~300 estimate in the accept rationale. Full-sync-only, no unbounded growth path (PAX8 has no streaming/webhook ingestion for this entity). |
|
||||
| T-12-08 | Repudiation | mitigate | **CLOSED** — required a post-acceptance fix (CR-02), now verified present | `lib/types/pax8.ts:116-129` — `Pax8EntitySyncResult` carries a dedicated `flaggedForReview?: number` field, with an inline comment explaining it is "kept separate from `tombstoned` ... so it is never folded into sync_history.records_deleted". `lib/services/pax8-sync-service.ts:618-651` (`syncCompanyMatches()`) sets `tombstoned: 0` unconditionally and reports `flaggedForReview: result.flaggedAmbiguous + result.flaggedNoCandidate` separately, with an explicit `// CR-02` comment. Confirmed via direct read, not the SUMMARY's self-report. Commit `bddf612` present in `git log`. |
|
||||
| T-12-SC | Tampering | accept | **CLOSED** (accepted risk, logged below) | `git log --oneline -- package.json` shows no commits since `1b7c453` (pre-Phase-12, unrelated); `git status --short package.json package-lock.json` is clean. pg_trgm confirmed a Postgres 16 contrib extension (`CREATE EXTENSION IF NOT EXISTS pg_trgm`), not a registry package. |
|
||||
|
||||
### Post-acceptance code-review fixes (not in the original register — verified independently)
|
||||
|
||||
| Fix ID | Commit | What it hardens | Verified present? |
|
||||
|--------|--------|------------------|--------------------|
|
||||
| CR-01 | `591fc5c` | `pax8_order_items.partner_cost`/`partner_cost_total` were `NUMERIC(12,2)` (migration 093), silently rounding real PAX8 values (e.g. `22.176`→`22.18`) — a data-integrity bug, not an adversarial threat, but a silent-corruption class of issue worth registering. | **YES** — `migrations/095_pax8_order_items_partner_cost_numeric.sql` widens both columns to `NUMERIC(14,4)`; confirmed via direct file read. |
|
||||
| CR-02 | `bddf612` | See T-12-08 above. | **YES** — see T-12-08 evidence. |
|
||||
| WR-01 | `cb8ae85` | A single malformed/failing PAX8 record (bad row) previously aborted the *entire* remaining sync batch for that entity, silently truncating tombstone reconciliation and data coverage — an availability/integrity issue an adversarial or merely malformed upstream record could trigger. | **YES** — confirmed per-row `try/catch` in `syncCompanies`, `syncSubscriptions`, `syncProducts`, and a nested per-invoice/per-item `try/catch` in `syncOrders`, all in `pax8-sync-service.ts`; "seen" arrays are populated before the write attempt so tombstoning isn't corrupted by a row failure. |
|
||||
| WR-02 | `76a652d` | Reinforces T-12-06's accept disposition — see above. | **YES** |
|
||||
| WR-03 | `1bb2b8b`/`be6f07b` | `Pax8Client.getToken()` previously trusted `res.ok` alone; a 200 response with an unexpected body shape (compromised/misconfigured proxy, CDN error page reshaped as JSON, a PAX8 API version change) would silently set `accessToken` to `undefined` and mask the real failure behind a confusing downstream 401. This is new attack surface not covered by any T-12-XX entry — recommend formally registering as **T-12-09 (Spoofing — malformed/adversarial token-endpoint response)** with disposition `mitigate` in any future phase touching `pax8-client.ts`. | **YES** — `lib/services/pax8-client.ts:54-60` throws `PAX8 token response missing access_token/expires_in` if `!data.access_token \|\| typeof data.expires_in !== 'number'`, before ever assigning to `this.accessToken`. |
|
||||
|
||||
## Unregistered Flags
|
||||
|
||||
- **T-12-09 (proposed, not yet formally registered)** — `Pax8Client.getToken()` token-response-shape validation (WR-03). Attack surface: an unexpected/malformed 200 response from the PAX8 token endpoint. Currently mitigated in code (see above) but was never a STRIDE entry in any of the five plans' `<threat_model>` blocks — it surfaced only during the post-acceptance code review. Logged here per the audit's "surface every unregistered attack surface" mandate. Not a blocker (`block_on: none`; already mitigated in code) — recommend adding it to the threat register of the next phase that touches `pax8-client.ts`'s auth flow.
|
||||
- **CR-01** (partner_cost/partner_cost_total precision loss) is a data-integrity/correctness bug, not an adversarial STRIDE threat (no attacker model applies to Postgres silently rounding a NUMERIC column) — noted for completeness but not logged as a security finding.
|
||||
- No other new network endpoints, auth paths, or trust-boundary changes were found outside the five plans' declared scope. `lib/services/pax8-sync-service.ts`'s `fullSync()` orchestration, `pax8-company-matcher.ts`, and `pax8-client.ts`'s new methods are the only diffs in this phase; all are covered by the register above.
|
||||
|
||||
## Accepted Risks Log
|
||||
|
||||
The following threats carry disposition `accept` per the phase's threat model. Each is
|
||||
re-validated here against real, live-verified data (not merely the plan's original estimate)
|
||||
before being logged as an accepted risk:
|
||||
|
||||
1. **T-12-06 — `matchPax8Companies()` loop DoS.** Accepted because the real dataset (118
|
||||
PAX8 companies) is three orders of magnitude below the enforced `LIMIT` (10000, raised from
|
||||
1000 by WR-02) and each candidate query is floor-filtered + `LIMIT 5`. Re-scoring runs once
|
||||
per full sync, not per-request. Risk owner: whoever owns `pax8-company-matcher.ts`.
|
||||
2. **T-12-07 — nested invoice→item fetch volume.** Accepted because the real dataset (94
|
||||
invoices, 29,696 order items, live-verified in 12-05) is well within normal Postgres/client
|
||||
sizing, full-sync-only (no incremental/streaming path), and the sync already isolates
|
||||
per-row failures (WR-01) so one bad item cannot cascade. Risk owner: whoever owns
|
||||
`pax8-sync-service.ts`.
|
||||
3. **T-12-SC — no new npm packages.** Accepted; confirmed no `package.json`/`package-lock.json`
|
||||
changes attributable to Phase 12. `pg_trgm` is a Postgres 16 contrib extension enabled via
|
||||
`CREATE EXTENSION IF NOT EXISTS`, not a supply-chain dependency.
|
||||
|
||||
## Test Verification (supporting evidence, not a substitute for the above)
|
||||
|
||||
```
|
||||
npx vitest run lib/services/pax8-company-matcher.test.ts lib/services/pax8-sync-service.test.ts lib/services/pax8-client.test.ts
|
||||
→ 3 files passed, 23 tests passed
|
||||
```
|
||||
|
||||
SECURITY.md: `.planning/phases/12-orders-invoices-company-matching/SECURITY.md`
|
||||
Loading…
Add table
Add a link
Reference in a new issue