diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl index d0425ce3..2b2409df 100644 --- a/.beads/issues.jsonl +++ b/.beads/issues.jsonl @@ -5,11 +5,11 @@ {"_type":"issue","id":"onetwo3d-ims-3i0","title":"COGS recognition deferred to nightly Xero batch — null cost-layer snapshots until Group B","description":"ShipmentLine.costLayerSnapshot is only populated by the daily Xero Group B batch. Until that runs, interim P\u0026L and margin reports are wrong. A failed batch leaves COGS unrecognized indefinitely. Manual SHIPPED transitions (outside the allocation flow) crash the next batch run.\n\nRefs:\n- app/actions/allocation.ts:1089 (confirmAllocations creates ShipmentLine without snapshot)\n- lib/connectors/xero/daily-sync.ts:124-126 (Group B throws if snapshot is null)\n- lib/connectors/xero/daily-sync.ts:950 (snapshot only populated here, hours later)\n\nFix direction: take the snapshot at the SHIPPED transition, not in the batch. Make the batch idempotent against pre-existing snapshots.","status":"closed","priority":0,"issue_type":"bug","assignee":"OneTwo3D IMS","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:55:51Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:31:49Z","started_at":"2026-04-25T10:31:49Z","closed_at":"2026-04-25T10:31:49Z","close_reason":"Verified: not a bug. costLayerSnapshot IS populated on ShipmentLine at the SHIPPED transition (allocation.ts:1341-1350) — the agent confused snapshot *consumption* by the daily batch with snapshot *creation*. Snapshots are written synchronously during transitionShipmentStatus (allocation.ts:1277), inside the same tx as the FIFO consumption + StockLevel decrement. The daily-sync.ts:124-126 throw is a defensive guard that fires only if a shipment was somehow marked SHIPPED outside the standard path — and there is no such path in the codebase (grep confirms allocation.ts is the only writer of status:SHIPPED on Shipment).","labels":["code-review","cogs","o2c","sync-xero"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-ahu","title":"No realised FX gain/loss on payment, no AR/AP revaluation","description":"Pay a EUR bill weeks later at a different rate — settlement variance is silently dropped. Sales invoices in non-base currency are never revalued. No grep hits for 'realised', 'fx gain', 'fx loss' anywhere in the codebase.\n\nImpact: P\u0026L missing FX gains/losses for every multi-currency settlement; AR/AP balances drift away from underlying foreign value over time; month-end reconciliation to Xero will fail.\n\nRefs:\n- app/actions/sales.ts:2690-2800 (payment matching is foreign-amount-only)\n- app/actions/purchase-orders.ts:2620-2720 (markBillPaid)\n\nFix direction: at payment, compute (foreign × rate-now) - (foreign × rate-on-invoice), book to FX gain/loss account. Add a periodic revaluation job for open AR/AP.","notes":"Verified: claim holds up. Grep for 'realised', 'fx gain', 'fx loss' returns 0 hits in the codebase; payment matching at sales.ts:2690-2800 and markBillPaid at purchase-orders.ts:2620-2720 reconcile by foreign-currency amount only, never compute the GBP settlement variance.\n\nTwo distinct features needed:\n1. Realised FX on payment: when a foreign bill/invoice is settled at a different rate than booked, post the difference to a P\u0026L FX gain/loss account.\n2. AR/AP revaluation: at month-end (or on demand), revalue open foreign-currency AR and AP to current rate, post unrealised FX gain/loss to a B/S contra account, reverse on the next revaluation.\n\nBoth depend on the FX policy decision in onetwo3d-ims-a8u — the booking rate must be stable to compute the variance. Schedule together.\nRe-checked 2026-04-25 during a COGS/Tax/FX workflow analysis. The implementation appears to have landed since this issue was filed:\n\n- lib/accounting-fx.ts:32-105 computes realised FX gain/loss on payment settlement (threshold 0.01 base; AP/AR control vs FX gain/loss leg).\n- lib/accounting-fx-revaluation.ts + /api/cron/accounting-fx-revaluation handle daily unrealised FX revaluation of open AR/AP, with prior-day reversal.\n- lib/connectors/xero/settings.ts exposes xero_realised_fx_gain_loss_account / xero_unrealised_fx_gain_loss_account and the xero_sync_realised_fx_journal / xero_sync_unrealised_fx_journal toggles.\n\nThe original 'grep returns 0 hits' verification is no longer accurate. Recommend either closing this with a link to the implementing commit or narrowing scope to whatever (if anything) is still missing — e.g. payment matching at sales.ts:2690-2800 / purchase-orders.ts:2620-2720 should be re-verified to confirm it now invokes the realised FX path.","status":"open","priority":0,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:55:51Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T20:48:07Z","labels":["accounting","code-review","fx"],"dependencies":[{"issue_id":"onetwo3d-ims-ahu","depends_on_id":"onetwo3d-ims-a8u","type":"blocks","created_at":"2026-04-25T12:38:45Z","created_by":"OneTwo3D IMS","metadata":"{}"}],"dependency_count":1,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-a8u","title":"FIFO cost layer not reconciled with supplier invoice (FX/landed cost)","description":"Cost layers are created with the PO line's base-cost at receipt, but the bill can arrive later with a different FX rate or unit cost. There is no reconciliation, so the FIFO layer (and therefore COGS) is permanently in a different base currency than AP.\n\nRefs:\n- app/actions/purchase-orders.ts:1827-1837 (layer create on receipt uses unitCostBase from PO line)\n- app/actions/purchase-orders.ts:2398-2401 (invoice FX rate looked up at invoice date, not PO date)\n- app/actions/purchase-orders.ts:3360-3370 (recalculateDirectLandedCosts uses primary PO unitCostBase, ignores invoice FX)\n- app/actions/purchase-orders.ts:2157-2158 (returns reverse PO-line cost, not billed cost)\n\nImpact: AP/inventory mismatch in base currency; balance sheet does not tie out for foreign-currency suppliers; landed-cost allocations are skewed when invoice FX != PO FX.\n\nDecision needed: pick a single FX policy (PO-rate, receipt-rate, or invoice-rate for the layer; book the FX delta as gain/loss) and retrofit. See related issues for AR/AP revaluation and realised FX on payment.","notes":"Verified: claim holds up. Cost layer cost basis (purchase-orders.ts:1827-1837 receipt path) uses the PO line's unitCostBase (computed at PO creation time from the PO's fxRateToBase). Invoice creation (purchase-orders.ts:2398-2401) re-resolves FX rate at invoice date. The two are never reconciled, so AP and inventory base-currency totals diverge for foreign-currency suppliers when PO date and invoice date differ.\n\nDecision needed (finance team):\n- Policy A: lock FX at PO issuance (current FIFO behaviour). Invoice rate unused for valuation; FX delta booked at invoice as gain/loss.\n- Policy B: lock FX at receipt (current FIFO behaviour matches if PO rate == receipt-day rate); invoice rate booked separately to AP, FX delta at receipt vs invoice booked as gain/loss.\n- Policy C: lock FX at invoice; FIFO layer gets retroactively corrected when bill arrives (most accurate, hardest to implement — requires re-walking COGS for shipments that consumed the layer between receipt and invoice).\n\nRecommend Policy A for simplicity unless finance team is doing accrual revaluation. Pair with onetwo3d-ims-ahu (realised FX gain/loss) for a complete fix.","status":"open","priority":0,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:55:50Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:31:53Z","labels":["code-review","cogs","fx","p2p"],"dependency_count":0,"dependent_count":1,"comment_count":0} -{"_type":"issue","id":"onetwo3d-ims-ecp","title":"Guest checkouts (externalCustomerId=0) bypass the customer-link check on the shopping invoice PDF route (PR #154)","description":"PR #154's `findShoppingInvoicePdfOrder` (app/api/shopping/[connector]/invoice-pdf/route.ts) verifies the customer link only when externalCustomerId is set AND non-zero:\n\n```ts\nif (request.externalCustomerId \u0026\u0026 request.externalCustomerId !== '0' \u0026\u0026 order.customerId) {\n const customerLink = await db.shoppingCustomerLink.findFirst({\n where: { connector, externalCustomerId, customerId: order.customerId },\n select: { id: true },\n })\n if (!customerLink) return null\n}\nreturn { orderId, invoiceNumber, invoicePdfPath }\n```\n\nThe WP plugin's payload includes `externalCustomerId: (string) $order-\u003eget_customer_id()`. For **guest checkouts**, WooCommerce sets `customer_id = 0`. So the WP plugin sends `externalCustomerId: '0'` in the HMAC payload.\n\nThe IMS route's check `request.externalCustomerId !== '0'` SKIPS the customer-link verification entirely for guest checkout payloads. After the skip, the function returns the order's PDF metadata unconditionally — anyone with a valid HMAC signature for that order ID can fetch the guest-checkout invoice.\n\n**Bypass:**\n\n1. Attacker controls a WordPress site with the OTI helper plugin and a copy of the shared HMAC secret (e.g., a partner store, a compromised plugin install, or a leaked secret from any of the per-tenant deployments using the same secret).\n2. Attacker enumerates / guesses guest order IDs (often sequential, even when 'random' the search space is bounded).\n3. Attacker constructs HMAC-signed payload: `{connector: 'woocommerce', externalOrderId: '\u003cguess\u003e', externalCustomerId: '0', issuedAt, expiresAt, nonce}`.\n4. POST to /api/shopping/woocommerce/invoice-pdf with valid X-OTI-Signature.\n5. IMS verifies signature (✓), parses payload (✓), connector matches (✓), TTL (✓), then looks up order. If the order exists AND was a guest checkout (customer_id = 0), the customer-link skip fires and the PDF is returned.\n\nThe check that GATES this exists for member orders (`customerLink` lookup). Guest orders are silently exempted.\n\n**Why this matters:** Invoice PDFs contain customer name, billing address, line items with prices, payment method — PII. Guest checkout users have NO authenticated channel; the customer-link verification is the only authorization for member orders. Skipping it for guests means ZERO authorization on guest invoices beyond 'know the order ID + have the HMAC secret'.\n\n**Why the check exists at all:** the original intent was to prevent logged-in customer A from accessing customer B's invoice. For guest orders there's no customer to scope against, but the right answer isn't to skip the check — it's to require some other proof of order ownership (email match, order key, etc.).","acceptance_criteria":"- For guest checkouts (externalCustomerId == '0' or missing), require a SECOND proof-of-ownership token from the WP plugin (e.g., WooCommerce order key, billing email hash, recent order session marker)\n- Alternative: refuse guest-checkout invoice PDF requests via this route entirely; route guests to a different flow (e.g., email-only delivery)\n- Add a test fixture for the guest case: assert that externalCustomerId='0' WITHOUT proof of ownership returns 404\n- Audit the WP plugin's `oti_can_download_invoice_pdf`: it already requires wp_verify_nonce for the WP-local request, but the IMS-side check is what stops cross-site abuse","notes":"Filed during PR #154 review. The WP plugin's nonce check prevents same-site cross-user abuse — but a cross-site attacker with the HMAC secret bypasses WP entirely and calls IMS directly. Highest priority of the PR's findings because it's a real, currently-exploitable bypass.","status":"closed","priority":1,"issue_type":"bug","assignee":"Jan","owner":"dev@onetwo3d.com","created_at":"2026-06-10T11:40:17Z","created_by":"Jan","updated_at":"2026-06-10T13:34:51Z","started_at":"2026-06-10T13:25:33Z","closed_at":"2026-06-10T13:34:51Z","close_reason":"Fixed: Woo guest invoice PDF requests now require the stored Woo order key; order imports persist that key; tests cover missing/mismatched proof.","labels":["code-review","pr-154","security","shopping"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-ecp","title":"Guest checkouts (externalCustomerId=0) bypass the customer-link check on the shopping invoice PDF route (PR #154)","description":"PR #154's `findShoppingInvoicePdfOrder` (app/api/shopping/[connector]/invoice-pdf/route.ts) verifies the customer link only when externalCustomerId is set AND non-zero:\n\n```ts\nif (request.externalCustomerId \u0026\u0026 request.externalCustomerId !== '0' \u0026\u0026 order.customerId) {\n const customerLink = await db.shoppingCustomerLink.findFirst({\n where: { connector, externalCustomerId, customerId: order.customerId },\n select: { id: true },\n })\n if (!customerLink) return null\n}\nreturn { orderId, invoiceNumber, invoicePdfPath }\n```\n\nThe WP plugin's payload includes `externalCustomerId: (string) $order-\u003eget_customer_id()`. For **guest checkouts**, WooCommerce sets `customer_id = 0`. So the WP plugin sends `externalCustomerId: '0'` in the HMAC payload.\n\nThe IMS route's check `request.externalCustomerId !== '0'` SKIPS the customer-link verification entirely for guest checkout payloads. After the skip, the function returns the order's PDF metadata unconditionally — anyone with a valid HMAC signature for that order ID can fetch the guest-checkout invoice.\n\n**Bypass:**\n\n1. Attacker controls a WordPress site with the OTI helper plugin and a copy of the shared HMAC secret (e.g., a partner store, a compromised plugin install, or a leaked secret from any of the per-tenant deployments using the same secret).\n2. Attacker enumerates / guesses guest order IDs (often sequential, even when 'random' the search space is bounded).\n3. Attacker constructs HMAC-signed payload: `{connector: 'woocommerce', externalOrderId: '\u003cguess\u003e', externalCustomerId: '0', issuedAt, expiresAt, nonce}`.\n4. POST to /api/shopping/woocommerce/invoice-pdf with valid X-OTI-Signature.\n5. IMS verifies signature (✓), parses payload (✓), connector matches (✓), TTL (✓), then looks up order. If the order exists AND was a guest checkout (customer_id = 0), the customer-link skip fires and the PDF is returned.\n\nThe check that GATES this exists for member orders (`customerLink` lookup). Guest orders are silently exempted.\n\n**Why this matters:** Invoice PDFs contain customer name, billing address, line items with prices, payment method — PII. Guest checkout users have NO authenticated channel; the customer-link verification is the only authorization for member orders. Skipping it for guests means ZERO authorization on guest invoices beyond 'know the order ID + have the HMAC secret'.\n\n**Why the check exists at all:** the original intent was to prevent logged-in customer A from accessing customer B's invoice. For guest orders there's no customer to scope against, but the right answer isn't to skip the check — it's to require some other proof of order ownership (email match, order key, etc.).","acceptance_criteria":"- For guest checkouts (externalCustomerId == '0' or missing), require a SECOND proof-of-ownership token from the WP plugin (e.g., WooCommerce order key, billing email hash, recent order session marker)\n- Alternative: refuse guest-checkout invoice PDF requests via this route entirely; route guests to a different flow (e.g., email-only delivery)\n- Add a test fixture for the guest case: assert that externalCustomerId='0' WITHOUT proof of ownership returns 404\n- Audit the WP plugin's `oti_can_download_invoice_pdf`: it already requires wp_verify_nonce for the WP-local request, but the IMS-side check is what stops cross-site abuse","notes":"Filed during PR #154 review. The WP plugin's nonce check prevents same-site cross-user abuse — but a cross-site attacker with the HMAC secret bypasses WP entirely and calls IMS directly. Highest priority of the PR's findings because it's a real, currently-exploitable bypass.","status":"open","priority":1,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-10T11:40:17Z","created_by":"Jan","updated_at":"2026-06-10T11:40:17Z","labels":["code-review","pr-154","security","shopping"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-v6z","title":"Connection-test fingerprint includes raw secrets — DB-read attacker can brute-force passwords (PR #152)","description":"PR #152's `buildIntegrationConnectionFingerprint` is called with raw secrets baked into the input:\n\n```ts\n// app/actions/company.ts - SMTP\nbuildIntegrationConnectionFingerprint({\n host, port, user,\n pass: storedPassword, // \u003c-- plaintext password\n secure, fromName, fromEmail, replyTo,\n})\n\n// app/actions/wc-sync.ts - WooCommerce \nbuildWcConnectionFingerprint({\n url, key,\n secret: input.secret.trim(), // \u003c-- consumer secret\n})\n\n// app/actions/mintsoft-sync.ts - Mintsoft\nbuildMintsoftConnectionFingerprint({\n baseUrl, username,\n password: input.password.trim(), // \u003c-- plaintext password\n orderLookupConnector,\n})\n```\n\nThe resulting SHA256 hex is then **stored in the `setting` table** under `{id}_connection_test_fingerprint`.\n\n**Why this matters:**\n\nA user / attacker with READ access to the `setting` table (admin auditor, partial DB dump, backup compromise) cannot directly reverse the SHA256. But they CAN brute-force the secret offline:\n\n1. Read `wc_url` (plaintext), `wc_consumer_key` (plaintext), `woocommerce_connection_test_fingerprint` (hash).\n2. The hash is `SHA256(JSON({url, key, secret}))` — all inputs except `secret` are visible.\n3. Iterate candidate secrets (offline, fast — billions per second on commodity GPUs), compute the hash, compare.\n4. For a 32-character random WooCommerce consumer secret, this is computationally infeasible.\n5. For an SMTP password chosen by a human (8-16 chars, common patterns), this is feasible with consumer hardware in hours/days.\n\nSame applies to Mintsoft passwords.\n\nThis is the same attack class as 'stop using fast hashes for password storage' — SHA256 is the wrong primitive for hashing user-chosen secrets.\n\n**Mitigations:**\n\n- **Don't include the secret in the fingerprint at all** — fingerprint changes when other fields change. The fingerprint's purpose is to detect 'settings have changed'; including the secret is one way to detect a rotated secret, but you could ALSO detect rotation by storing a stable secret-version identifier (e.g., a UUID generated on secret-write) and including that in the fingerprint instead of the secret itself.\n- **OR hash the secret separately with HMAC + server-side key** — `hash = SHA256(JSON({url, key, HMAC_serverKey(secret)}))`. DB-read attacker cannot precompute candidates without the server key (which lives in env, not DB).\n- **OR use a slow KDF** (Argon2id, bcrypt, scrypt) for the secret portion — makes offline brute-force impractical.","acceptance_criteria":"- Choose between: (a) drop secret from fingerprint input + use secret-version UUID, (b) HMAC with server key, (c) Argon2id on secret portion\n- Update buildIntegrationConnectionFingerprint to apply the chosen pattern across SMTP, WC, Mintsoft (and any future caller)\n- Add a test asserting the same secret reproduces the same fingerprint, AND a fingerprint comparison cannot be brute-forced (effectively: assert HMAC key is needed, OR slow-KDF is applied)\n- Consider a migration to re-fingerprint existing connection-test records once the new scheme lands (otherwise existing records' status flips to 'fingerprint mismatch' and operators must retest)","notes":"Filed during PR #152 review. Highest priority of the findings — secret leakage from fingerprint, even if difficult, is a tangible security risk. Mintsoft and SMTP passwords are the highest concern because they're often human-chosen.","status":"open","priority":1,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-10T09:48:57Z","created_by":"Jan","updated_at":"2026-06-10T09:48:57Z","labels":["code-review","integrations","pr-152","security"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-8vp","title":"Backup manifest critical-tables list is 4 tables; FIFO/COGS/accounting tables missing (PR #148)","description":"PR #148's `lib/backup-manifest.ts` defines critical tables for backup-integrity validation:\n\n```ts\nexport const CRITICAL_BACKUP_TABLES = [\n 'users',\n 'products',\n 'sales_orders',\n 'purchase_orders',\n] as const\n```\n\nThe IMS schema has 59+ Prisma models. The 4-table critical list catches the most user-visible tables but MISSES the tables whose absence would silently destroy financial state:\n\n- **`cost_layers`** — FIFO costing depends entirely on these rows. A restore with empty cost_layers but populated sales/purchases means COGS becomes uncomputable; refunds posts wrong amounts; analytics show zero margin.\n- **`cogs_entries`** — audit trail for consumed FIFO layers. Required for refund reversal correctness.\n- **`stock_levels`** — denormalized stock-on-hand. Recoverable from movements but expensive.\n- **`stock_movements`** — every inventory event. Without these, FIFO is unreplayable.\n- **`accounting_sync_logs`** — Xero outbox. Restoring without these double-posts every prior journal on next sync.\n- **`activity_logs`** — audit / compliance trail.\n- **`payments`** — AR/AP settlement records.\n- **`invoices`** / **`shipments`** / **`order_allocations`**\n\n**Why this matters:** A restore from a backup that's missing `cost_layers` passes manifest validation today, but produces an inventory system with no FIFO history. The PR's stated purpose ('critical-table backup manifests') is satisfied superficially but the choice of 4 tables doesn't match what 'critical' means for IMS-class data.\n\nThe 4 chosen tables (`users`, `products`, `sales_orders`, `purchase_orders`) are PRESENCE-of-business-data signals. Missing tables that break accounting/inventory are different signals — schema integrity vs business-data presence.","acceptance_criteria":"- Expand CRITICAL_BACKUP_TABLES to include the FIFO/COGS/accounting-critical tables listed above\n- Add an integration test that creates a backup, deletes all rows from cost_layers (or similar), and asserts the manifest validation rejects on restore\n- Document in the manifest's JSDoc what 'critical' means — is it 'should not be empty' or 'must exist in schema'?\n- Consider dynamic critical-table list: compute from Prisma metadata (tables with FK refs, or @@map'd to known financial models)","notes":"Filed during PR #148 review. The 4-table list reads as an MVP starting point; production-readiness requires the financial-state tables to be in scope.","status":"open","priority":1,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-09T15:58:21Z","created_by":"Jan","updated_at":"2026-06-09T15:58:21Z","labels":["accounting","backup","code-review","fifo","pr-148"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-fpr","title":"Backup manifest validation only fires on stored-backup restores, not uploaded restores (PR #148)","description":"PR #148 adds manifest validation in `createBackupRestorePostHandler` (`app/api/backup/restore/route.ts`) but only on the STORED-backup branch:\n\n```ts\n// Stored-backup path (line ~605):\nmanifest = await resolvedDeps.validateBackupManifest(restorePath)\nif (manifest.backupFilename !== path.basename(restorePath)) {\n return NextResponse.json({ error: 'Backup manifest does not match the selected backup.' }, { status: 400 })\n}\n\n// Upload path: no manifest validation. The uploaded file goes straight to runRestore.\n```\n\nAn admin uploading a hand-crafted SQL file via the multipart upload bypasses:\n- The critical-table presence check (`validateCriticalBackupTables`)\n- The schema version check\n- The row-count integrity signal\n\nThis is the same admin trust model as the rest of the system, but the PR's claim is that the manifest improves restore safety — for uploaded backups (the most common 'restore from external source' workflow), it provides zero new protection.\n\n**Why this matters:** An admin who accidentally uploads a partial or corrupt SQL dump (e.g., a dev-env dump missing several tables, or a dump from a different IMS instance with different schema) restores it with no integrity check. The manifest's stated purpose — 'critical-table backup manifests for generated/restored backups' — is half-implemented.\n\nThe upload path also can't easily check the manifest because manifests are sidecar files that live next to local backups. For uploaded restores, the manifest would need to be uploaded alongside (multi-file form) or embedded in the SQL header.","acceptance_criteria":"- Decide policy:\n - **Option A (preferred)**: require manifest upload alongside SQL file in the multipart form for uploaded restores\n - **Option B**: embed manifest as a SQL comment block at the start of the dump (`-- BACKUP MANIFEST: {json}`); read and validate during upload\n - **Option C**: skip manifest for uploads but require operator to explicitly check 'I acknowledge no integrity validation' UI flag\n- Update tests to assert the chosen behavior for uploaded restores\n- Update docs/plan.md P7.7 'Backup manifest' to reflect the chosen scope (uploaded vs stored)","notes":"Filed during PR #148 review. The PR addresses the stored-backup path well but the upload path is where most operator-driven restore-from-remote workflows go. The security claim is stronger than the implementation provides.","status":"open","priority":1,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-09T15:58:01Z","created_by":"Jan","updated_at":"2026-06-09T15:58:01Z","labels":["backup","code-review","pr-148","security"],"dependency_count":0,"dependent_count":0,"comment_count":0} -{"_type":"issue","id":"onetwo3d-ims-2it","title":"Invoice PDF Content-Disposition filename now uses random nonce instead of invoice id (PR #146)","description":"PR #146 changes the invoice PDF download filename from the invoice id to the token's nonce:\n\n```diff\n- 'Content-Disposition': `inline; filename=\"invoice-${safeInvoiceFilenameId(id)}.pdf\"`,\n+ 'Content-Disposition': `inline; filename=\"invoice-${safeInvoiceFilenameId(verification.payload.nonce)}.pdf\"`,\n```\n\n(`app/api/invoices/[id]/route.ts` around line 165 of the diff)\n\nThe nonce is `randomBytes(16).toString('base64url')` — a 22-character random string like `aB3xY7zP9_4kQrmTuVwXyZ`.\n\nUser impact: instead of downloading `invoice-INV-2025-001.pdf`, the user gets `invoice-aB3xY7zP9_4kQrmTuVwXyZ.pdf`. Files in the downloads folder have random-looking names that don't match the invoice they represent.\n\n**Why this matters:**\n1. **UX regression** — users can't identify invoice files at a glance; finance teams downloading dozens of invoices can't filter/sort by invoice number.\n2. **No security benefit** — the URL path `/api/invoices/{id}?token=...` already exposes the invoice id; the user clicked a link knowing what invoice they wanted. Hiding the id in the filename adds zero security.\n3. **Likely accidental** — the change isn't mentioned in the PR description's bullet points. The other PDF changes (token binding, TTL) are explicit; this filename change reads as collateral damage from the same edit.","acceptance_criteria":"- Revert the Content-Disposition filename to use `safeInvoiceFilenameId(id)` (the invoice id), not the nonce\n- Add a test that asserts the Content-Disposition header includes the invoice id, not the nonce\n- If there was a deliberate reason to use the nonce (e.g., preventing concurrent downloads from clashing), document it explicitly in a code comment AND update the PR body","notes":"Filed during PR #146 review. Highest priority of the findings because it's a likely unintentional UX regression that ships out of an otherwise good security PR.","status":"closed","priority":1,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-08T23:07:42Z","created_by":"Jan","updated_at":"2026-06-10T13:35:04Z","closed_at":"2026-06-10T13:35:04Z","close_reason":"Fixed in final security-hardening branch: invoice PDF Content-Disposition now uses the requested invoice/order id again, with sanitization and a test.","labels":["code-review","pr-146","security","ux"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-2it","title":"Invoice PDF Content-Disposition filename now uses random nonce instead of invoice id (PR #146)","description":"PR #146 changes the invoice PDF download filename from the invoice id to the token's nonce:\n\n```diff\n- 'Content-Disposition': `inline; filename=\"invoice-${safeInvoiceFilenameId(id)}.pdf\"`,\n+ 'Content-Disposition': `inline; filename=\"invoice-${safeInvoiceFilenameId(verification.payload.nonce)}.pdf\"`,\n```\n\n(`app/api/invoices/[id]/route.ts` around line 165 of the diff)\n\nThe nonce is `randomBytes(16).toString('base64url')` — a 22-character random string like `aB3xY7zP9_4kQrmTuVwXyZ`.\n\nUser impact: instead of downloading `invoice-INV-2025-001.pdf`, the user gets `invoice-aB3xY7zP9_4kQrmTuVwXyZ.pdf`. Files in the downloads folder have random-looking names that don't match the invoice they represent.\n\n**Why this matters:**\n1. **UX regression** — users can't identify invoice files at a glance; finance teams downloading dozens of invoices can't filter/sort by invoice number.\n2. **No security benefit** — the URL path `/api/invoices/{id}?token=...` already exposes the invoice id; the user clicked a link knowing what invoice they wanted. Hiding the id in the filename adds zero security.\n3. **Likely accidental** — the change isn't mentioned in the PR description's bullet points. The other PDF changes (token binding, TTL) are explicit; this filename change reads as collateral damage from the same edit.","acceptance_criteria":"- Revert the Content-Disposition filename to use `safeInvoiceFilenameId(id)` (the invoice id), not the nonce\n- Add a test that asserts the Content-Disposition header includes the invoice id, not the nonce\n- If there was a deliberate reason to use the nonce (e.g., preventing concurrent downloads from clashing), document it explicitly in a code comment AND update the PR body","notes":"Filed during PR #146 review. Highest priority of the findings because it's a likely unintentional UX regression that ships out of an otherwise good security PR.","status":"open","priority":1,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-08T23:07:42Z","created_by":"Jan","updated_at":"2026-06-08T23:07:42Z","labels":["code-review","pr-146","security","ux"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-xin","title":"WC tax fallback block-policy allows fallback to zero-rate even when destination should be taxed (PR #144)","description":"PR #144's `shouldBlockWcTaxRateFallback()` (lib/connectors/woocommerce/sync/order-import.ts) blocks WooCommerce imports only when the fallback tax rate value is \u003e 0:\n\n```ts\nexport function shouldBlockWcTaxRateFallback(lines: WcTaxRateFallbackLine[]): boolean {\n return lines.some((line) =\u003e line.taxRateValue \u003e 0)\n}\n```\n\nThe original bd issue `onetwo3d-ims-xx4` (now closed) was about tax-rate fallback being silent on WC import. The fix correctly blocks imports when the fallback uses a non-zero default rate. But the inverse case is still silent:\n\n**Scenario:** A WC order ships to Germany (DE), 19% standard VAT. The DE/STANDARD tax rate exists in IMS but the WC tax rate is unmapped. The IMS resolver falls back through (country+category → country+STANDARD → global+category → order default). If the order default is a 0% rate (e.g., for a tenant that has a misconfigured zero-rated default), the resolver returns rate 0 — and `shouldBlockWcTaxRateFallback` does NOT block because `taxRateValue === 0`.\n\nResult: a 19% DE order is imported with 0% tax. Understated VAT, silent.\n\n**Why this matters:** This is the same class of bug as the original `xx4`: silent tax misconfiguration produces wrong VAT. The fix only addresses the over-rated direction.","acceptance_criteria":"- Add a second blocking condition: when the destination country has any active non-zero tax rate configured for the product category and the resolver fell back to a different (zero or otherwise non-matching) rate, block the import\n- Alternatively: require that the resolver's matched rate ID actually matches the destination jurisdiction (countryCode == destCountry) — block on any fallback regardless of rate value\n- Add a focused WC import test for the zero-rated fallback case where the destination should have been non-zero\n- Update the Sales Settings fallback widget to also surface zero-rate fallback events","notes":"Identified during PR #144 review. The PR addressed bd onetwo3d-ims-xx4 in the over-rated direction but the under-rated direction (silent zero-rate import for a non-zero-rated jurisdiction) is the more dangerous failure mode because under-reported VAT is harder to detect than over-reported VAT.","status":"open","priority":1,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-07T23:36:43Z","created_by":"Jan","updated_at":"2026-06-07T23:36:43Z","labels":["code-review","connectors","pr-144","vat"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-xjj","title":"Tax snapshot refresh has a read-then-write race against status transitions (PR #144)","description":"PR #144 adds `refreshMutableDocumentTaxSnapshotsForRate()` (lib/tax/document-tax-snapshot-refresh.ts) which is called transactionally from `updateTaxRate()` when a TaxRate is edited. The function:\n\n1. `findMany` for SalesOrderLines whose `order.status IN [DRAFT, PENDING_PAYMENT]`.\n2. Loops through each line and calls `salesOrderLine.update()` plus header `salesOrder.update()` to apply the new rate.\n\nThe findMany result is materialized at read time. If a SalesOrder transitions from DRAFT → PROCESSING (or PENDING_PAYMENT → PROCESSING) between the findMany and the update, the function silently rewrites tax fields on an order that should now be immutable.\n\nThe transaction surrounding it doesn't take a SELECT FOR UPDATE on the orders, so concurrent writers can sneak in.\n\nSame gap exists in `refreshMutablePurchaseOrderTaxSnapshots` (status set `[DRAFT, RFQ_SENT, QUOTE_RECEIVED]`).\n\n**Why this matters:** Tax rate edits typically happen during operator setup, but a user shipping an order at the same moment as another user editing the tax rate can cause the shipped order's persisted tax to silently change. Hard to detect post-hoc.","acceptance_criteria":"- Either (a) add SELECT FOR UPDATE on the SalesOrder/PurchaseOrder rows before re-checking status and applying updates, or (b) re-check the order status inside the update by using a conditional update (`update where: { id, status: { in: MUTABLE_STATUSES } }`) and counting affected rows\n- Add a concurrency test that exercises the race: start the tax refresh transaction, transition the order to PROCESSING from another connection, assert no update lands on the now-immutable order\n- Document the chosen safety mechanism in lib/tax/document-tax-snapshot-refresh.ts","notes":"Identified during PR #144 review. The risk is bounded by how rarely operators edit a TaxRate concurrently with order processing, but the failure mode is silent — tax fields are denormalized onto orders/lines, so a stale-write here changes accounting numbers without an audit trail beyond the tax_rate_snapshot_refresh activity log entry (which doesn't include which lines were touched).","status":"open","priority":1,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-07T23:36:24Z","created_by":"Jan","updated_at":"2026-06-07T23:36:24Z","labels":["code-review","concurrency","pr-144","tax"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-67x","title":"Add action-level idempotency test: cancelPurchaseOrder twice (P4.4 follow-up)","description":"Plan item P4.4 closed via PR #138, but only the `isPurchaseOrderCancellationNoop` helper has a test. `tests/domain/purchasing/po-cancellation.test.ts:246` asserts the boolean returns true/false for given statuses. The plan acceptance criterion specifies an action-level test:\n\n\u003e Call `cancelPurchaseOrder` twice; assert second call succeeds silently.\n\nNo such test exists. The helper test cannot prove that `cancelPurchaseOrder` actually wires the noop check, that the second call:\n- returns `{ success: true }` (not an error)\n- does NOT write a second 'cancelled' activity log row\n- does NOT enqueue a second accounting sync\n- does NOT re-emit stock-sync events\n\n**Why this matters:** A future refactor that removes the noop check (or moves it after the side-effects) would not be caught by the current helper test. Repeated cancellation requests producing duplicate Xero journals or activity logs is the failure mode P4.4 was meant to prevent.","acceptance_criteria":"- Add test in tests/domain/purchasing/po-cancellation.test.ts (or action-level test file) that:\n - Sets up a CANCELLED PO\n - Calls `cancelPurchaseOrder('po-1')` (or fixtures the action call)\n - Asserts result is `{ success: true }`\n - Asserts no duplicate activity log row added\n - Asserts no duplicate accounting sync queue row added\n - Asserts no duplicate stock-sync enqueue\n- Update docs/plan.md P4.4 Tests pointer to name the new test","notes":"Identified during PR #140 review audit. Same gap pattern as P1.3 (helper test only; action-level integration not proven). My PR #138 review inline finding #6 flagged this explicitly with suggested test shape — recommend implementing that shape.","status":"open","priority":1,"issue_type":"task","owner":"dev@onetwo3d.com","created_at":"2026-06-07T18:08:51Z","created_by":"Jan","updated_at":"2026-06-07T18:08:51Z","labels":["idempotency","plan-followup","purchasing","tests"],"dependency_count":0,"dependent_count":0,"comment_count":0} @@ -24,9 +24,13 @@ {"_type":"issue","id":"onetwo3d-ims-5oh","title":"Allocation snapshots editable between A2 staging and Group B run","description":"resetAllocationAccountingIfStaged() only triggers when shipmentJournalDate is set — i.e. after Group B has stamped the order. Between A2 staging and Group B, allocations can be edited and the next batch will use stale snapshots. Shipment-line snapshots are also not cleared on reset (only allocation snapshots are).\n\nRefs:\n- app/actions/allocation.ts:42, :74-77\n\nFix direction: lock allocations on A2 staging and require an explicit unstage that clears both allocation- and shipment-line snapshots.","status":"closed","priority":1,"issue_type":"bug","assignee":"OneTwo3D IMS","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:57:53Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:17:44Z","started_at":"2026-04-25T10:17:43Z","closed_at":"2026-04-25T10:17:44Z","close_reason":"Verified: not a bug. resetAllocationAccountingIfStaged (allocation.ts:42-79) is called inside every allocation-mutation tx (e.g. deallocateOrder line 937). When inventoryAllocatedDate is set: (a) blocks edits if any shipment has shipmentJournalDate (Group B already ran) — line 55-64; (b) otherwise clears inventoryAllocatedDate AND OrderAllocation.costLayerSnapshot so A2 rebuilds on next batch. ShipmentLine snapshots are intentionally NOT cleared because they reflect the actual SHIPPED state and should be preserved. The 'stale snapshots in next batch' claim is incorrect.","labels":["code-review","cogs","sync-xero"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-jgi","title":"StockLevel.quantity drifts from sum of cost-layer remainingQty","description":"StockLevel.quantity is mutated directly in several places and is not derived from the cost layers. Partial transaction failures or layer adjustments can leave the two out of sync. There is no reconciliation job.\n\nRefs:\n- lib/stock.ts:166-172, :193-197 (direct StockLevel mutations)\n- lib/transfers.ts:558-559 (transfer mutations)\n- lib/cost-layers.ts:181-197 (layer mutations not synced back)\n\nFix direction: either make quantity a derived view (sum of layers) or add a reconciliation job + alert when they diverge.","status":"closed","priority":1,"issue_type":"bug","assignee":"OneTwo3D IMS","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:57:53Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:17:45Z","started_at":"2026-04-25T10:17:44Z","closed_at":"2026-04-25T10:17:45Z","close_reason":"Verified: not a real bug. StockLevel.quantity and cost-layer sums are mutated together inside the same Prisma $transaction at every site (allocation.ts:1303-1310 + 1327-1329, manufacturing.ts:684-705 + 722-738 + 780-798, transfers.ts:405-414 + 556-578, stock.ts:689-756, purchase-orders.ts receipts). Postgres ACID prevents partial commits. Drift is only possible via out-of-band DB writes, which no app fix can prevent. A reconciliation job would be defensive observability, not a bug fix — open a separate feature request if desired.","labels":["code-review","fifo","inventory"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-0r8","title":"FIFO consumption race: candidate read happens before per-row lock","description":"lib/cost-layers.ts:62-95 — findMany reads the candidate cost-layer set, then takes per-row locks. Two concurrent shippers can read the same candidate set before either acquires a lock, then both decrement the same layer.\n\nImpact: physical stock corruption — over-consumption, negative remainingQty, broken FIFO ordering.\n\nVERIFY: confirm whether the lock is actually `SELECT ... FOR UPDATE` on the same row IDs and whether the surrounding $transaction is at Serializable isolation. If not, switch to Serializable or take the row locks before/inside the candidate read (e.g. `SELECT ... FOR UPDATE SKIP LOCKED`).","status":"closed","priority":1,"issue_type":"bug","assignee":"OneTwo3D IMS","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:57:52Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:31:50Z","started_at":"2026-04-25T10:31:49Z","closed_at":"2026-04-25T10:31:50Z","close_reason":"Verified: not a bug. consumeFifoLayers (lib/cost-layers.ts:62-95) implements the textbook lock-then-re-read pattern: (1) findMany candidate IDs, (2) lockCostLayers acquires SELECT...FOR UPDATE on those IDs, (3) re-reads the locked rows via findMany on id IN [...] for fresh remainingQty values, (4) decrements based on the post-lock state. The explicit comment at line 69-70 documents this: 'Re-read after taking row locks so concurrent consumers cannot both act on the same pre-lock remainingQty snapshot and double-decrement a layer.' The agent missed the re-read.","labels":["code-review","concurrency","fifo","verify"],"dependency_count":0,"dependent_count":0,"comment_count":0} -{"_type":"issue","id":"onetwo3d-ims-fa5","title":"WP plugin invoice-pdf proxy follows unvalidated order-meta endpoint URL — SSRF surface (PR #154)","description":"PR #154's WP plugin (`lib/connectors/woocommerce/wp-plugin/onetwoinventory-helper.php`) adds a customer-facing invoice download endpoint that proxies to IMS:\n\n```php\nfunction oti_proxy_invoice_pdf(WP_REST_Request $request) {\n // ...\n $endpoint = oti_get_order_meta($order_id, '_ims_invoice_pdf_endpoint');\n $secret = (string) get_option(OTI_FX_SECRET_OPT, '');\n if ($endpoint === '' || $secret === '') {\n return new WP_Error('oti_invoice_not_configured', ...);\n }\n // ...\n $response = wp_remote_post($endpoint, [\n 'timeout' =\u003e 30,\n 'headers' =\u003e [\n 'Content-Type' =\u003e 'application/json',\n 'X-OTI-Signature' =\u003e hash_hmac('sha256', $body, $secret),\n ],\n 'body' =\u003e $body,\n ]);\n // ...\n}\n```\n\nThe proxy reads `$endpoint` from per-order meta key `_ims_invoice_pdf_endpoint`. This value is set by IMS (presumably) when generating the PDF. But:\n\n1. **Where does meta set?** Order meta is writable from many places: WP admin UI, REST API (with permissions), other plugins, direct DB. If an attacker can write to that meta key (compromised admin, malicious plugin, SQL injection via another vector), they redirect the proxy to an attacker-controlled URL.\n\n2. **No URL validation.** The endpoint is used verbatim — no scheme allowlist (http vs https), no domain allowlist, no DNS-rebinding protection. A malicious endpoint can be:\n - `https://attacker.example.com/` — capture the HMAC-signed payload\n - `http://169.254.169.254/...` — AWS metadata service (SSRF to cloud metadata)\n - `http://localhost:3306/...` — local services on the WP host\n - `gopher://...` — protocol smuggling\n\n3. **HMAC payload exfiltration.** Even if the response body is discarded by the plugin (which it actually uses for the PDF body, so the attacker controls what the customer downloads), the OUTBOUND request to the attacker URL contains the HMAC signature for the payload. The attacker now has a (body, signature) pair signed with the shared secret — confirms the secret hasn't rotated and provides material for further forgery.\n\n4. **PDF content substitution.** The proxy echoes `$pdf = wp_remote_retrieve_body($response);` as the customer-facing PDF. So if attacker controls the endpoint, attacker delivers a malicious PDF to the customer (phishing, malware-laden PDF, etc.) under the trusted WC site origin.\n\n**Why this matters:** The proxy is a server-side fetch with HMAC signing. The endpoint is meant to be a trusted IMS URL, but the code treats per-order meta as the source of truth without validating it's still pointing at IMS.\n\n**Mitigation:**\n\nPin the endpoint base URL in WP site options (set once during plugin configuration, signed/protected by admin-only access), and reject per-order meta endpoints. The order id can still come from meta; only the BASE URL must be pinned.","acceptance_criteria":"- Add a new site option `oti_ims_invoice_pdf_base_url` to the plugin settings page (admin-only)\n- Construct the endpoint server-side: `${base_url}/api/shopping/woocommerce/invoice-pdf` — ignore per-order meta for the URL\n- Drop the `_ims_invoice_pdf_endpoint` meta key entirely (or keep it as advisory only, with hash verification against the pinned base)\n- Add tests / docs explaining the trust boundary: WP plugin trusts WP site options (admin-set); does not trust per-order meta for URL choice\n- Audit any other places in the plugin that take URLs from meta or external data","notes":"Filed during PR #154 review. SSRF surface in a customer-facing plugin endpoint, mitigated by 'attacker needs to write order meta' but the attack surface is real. Mid-priority because the plugin is in OneTwo3D's deployment scope, not a public marketplace plugin — easier to audit and patch.","status":"closed","priority":2,"issue_type":"bug","assignee":"Jan","owner":"dev@onetwo3d.com","created_at":"2026-06-10T11:41:38Z","created_by":"Jan","updated_at":"2026-06-10T13:34:52Z","started_at":"2026-06-10T13:25:34Z","closed_at":"2026-06-10T13:34:52Z","close_reason":"Fixed: Woo plugin now constructs IMS invoice PDF endpoint from an admin-pinned HTTPS base URL and no longer follows per-order endpoint meta.","labels":["code-review","pr-154","security","ssrf","wordpress"],"dependency_count":0,"dependent_count":0,"comment_count":0} -{"_type":"issue","id":"onetwo3d-ims-0qv","title":"Shopping invoice PDF nonce not tracked — replay within 5-minute TTL is unbounded (PR #154)","description":"PR #154's `parseShoppingInvoicePdfRequest` validates that the payload includes a `nonce` (non-empty string) but doesn't track which nonces have been seen.\n\n```ts\n// app/api/shopping/[connector]/invoice-pdf/route.ts\nconst parsed = parseShoppingInvoicePdfRequest(rawBody, connector, { now: dependencies.now })\nif (!parsed.valid) return reject(dependencies, auditBase, parsed.reason)\n// nonce is parsed but never checked against a 'seen' set\nconst order = await dependencies.findOrder(parsed.request)\n```\n\nThe PR's stated security model includes the nonce, but the IMS side accepts any non-empty UUID-shape string. So a valid HMAC-signed payload can be REPLAYED:\n\n- Same payload, same X-OTI-Signature, same timestamps\n- IMS validates HMAC (✓ — body is unchanged)\n- IMS parses payload (✓ — fields are still valid)\n- IMS checks issuedAt+TTL (✓ — still within 5 minutes)\n- IMS returns the PDF\n\nSo within the 5-minute TTL window, the SAME signed payload can be replayed N times. Rate limit caps at 60 req/min per IP, but:\n\n- N = 60/min × 5min = up to 300 replays per IP for the same valid payload\n- Across multiple IPs (proxied attacker, residential proxy networks), much higher\n\n**Why this matters:**\n\nFor most flows this is moot — the PDF doesn't change, the customer downloads it once. But:\n\n1. **PDF generation cost**: `loadInvoicePdf` reads from disk; cheap. If a future change generates the PDF on-demand, 300× replay = 300× generation cost.\n2. **Activity log spam**: each verified-and-accepted attempt is logged. The audit trail bloats with duplicates.\n3. **Combined with onetwo3d-ims-ecp (guest bypass)**: a compromised secret + enumerable guest order IDs means an attacker exfiltrates all guest invoices with bounded HMAC-signature work — and the nonce doesn't slow this down at all because they only need to sign once per order.\n\n**Standard nonce-tracking:**\n\n```ts\n// On verify, check the nonce hasn't been seen in the TTL window\nconst existing = await db.shoppingInvoicePdfNonce.findUnique({ where: { nonce } })\nif (existing) return reject(dependencies, auditBase, 'replay')\nawait db.shoppingInvoicePdfNonce.create({ data: { nonce, expiresAt } })\n// Periodic sweep clears expired nonces\n```\n\nOr use a TTL-bounded cache (Redis SET NX with EX). Simple, cheap, defeats replay.","acceptance_criteria":"- Add nonce tracking to the shopping invoice PDF route:\n - Option A (db): new table 'shopping_invoice_pdf_nonces' with (nonce, expiresAt); index on nonce; periodic sweep\n - Option B (redis/in-memory): if rate-limit backend already uses redis, store nonces there with TTL = TOKEN_TTL_SECONDS + clock skew\n- Tests: assert second request with same payload returns 'replay' rejection (or appropriate failure reason)\n- Update the failure-reason union: add 'replay_detected'\n- Document the nonce contract in lib/shopping-invoice-pdf.ts JSDoc","notes":"Filed during PR #154 review. Mid-priority — replay is bounded by HMAC secret possession AND TTL AND rate limit. But the design intent of 'nonce' is one-shot, and the implementation is currently advisory only.","status":"closed","priority":2,"issue_type":"bug","assignee":"Jan","owner":"dev@onetwo3d.com","created_at":"2026-06-10T11:41:09Z","created_by":"Jan","updated_at":"2026-06-10T13:34:52Z","started_at":"2026-06-10T13:25:34Z","closed_at":"2026-06-10T13:34:52Z","close_reason":"Fixed: shopping invoice PDF request nonces are recorded in one_time_tokens and rejected on replay; tests cover replay rejection.","labels":["code-review","pr-154","replay-protection","security"],"dependency_count":0,"dependent_count":0,"comment_count":0} -{"_type":"issue","id":"onetwo3d-ims-d5l","title":"Shopping invoice PDF HMAC reuses wc_webhook_secret — key reuse across two purposes (PR #154)","description":"PR #154's `lib/shopping-invoice-pdf.ts`:\n\n```ts\nconst SHOPPING_INVOICE_SECRET_SETTING: Record\u003cShoppingConnectorId, string\u003e = {\n woocommerce: 'wc_webhook_secret',\n shopify: 'shopify_webhook_secret',\n}\n```\n\nThe same `wc_webhook_secret` is used for:\n\n1. **Incoming webhook verification**: WooCommerce → IMS posts an event, signs body with this secret, IMS verifies the signature.\n2. **Outgoing PDF request signing**: WP plugin → IMS posts a PDF request, signs body with this secret, IMS verifies the signature.\n\nThis is **key reuse across distinct purposes** — a well-known cryptographic anti-pattern (RFC 4949 §3.4). Concrete risks:\n\n1. **Cross-protocol replay**: a signed message intended for one purpose might be reinterpreted as another. WC webhook payloads and PDF request payloads are different JSON shapes, but:\n - A signed payload that happens to be valid JSON for BOTH schemas could be replayed across endpoints\n - The PDF schema is small (5-6 fields) — there's nontrivial overlap with hypothetical webhook payloads\n - Defense-in-depth principle: don't rely on schema differences to keep purposes separate\n\n2. **Leak amplification**: if the webhook secret leaks (e.g., from a WordPress site backup, a wp-config dump, a compromised admin), the attacker gets BOTH:\n - Ability to forge webhooks (data injection)\n - Ability to forge PDF requests (data exfiltration — coupled with the guest-checkout bypass in onetwo3d-ims-ecp this is total invoice access)\n\n3. **Rotation amplification**: rotating the webhook secret because of a webhook abuse incident also invalidates ALL pending PDF requests, breaking customer downloads. Same with rotating because of a PDF-request abuse incident. Coupling rotation events couples incident response.\n\n**Cryptographic best practice:**\n\nEach distinct cryptographic purpose gets its own key. Or use HKDF to derive purpose-specific subkeys from a single root:\n\n```ts\nhmacHex(`shopping-invoice-pdf:${connector}`, rootSecret) // derive\n// store derived key, use for signing/verifying PDF requests only\n```\n\n**Why this matters:** The HMAC contract here is the only authorization for the new shopping invoice PDF route. Cryptographic hygiene of the secret directly limits the abuse surface.","acceptance_criteria":"- Add a NEW secret `wc_invoice_pdf_secret` (and `shopify_invoice_pdf_secret`) distinct from the webhook secret\n- WP plugin's settings page should expose a separate field for this; OR derive the PDF secret from the webhook secret via HKDF with a purpose label\n- Test that a payload signed with the webhook secret is REJECTED by the PDF route (cross-protocol replay defense)\n- Document the key separation in docs/architecture.md and the helper plugin's README\n- Migration: existing installations need a settings UI to set the new secret OR documentation for the derived-key approach","notes":"Filed during PR #154 review. Pair with onetwo3d-ims-ecp because both undermine the PDF authorization layer, just from different angles.","status":"closed","priority":2,"issue_type":"bug","assignee":"Jan","owner":"dev@onetwo3d.com","created_at":"2026-06-10T11:40:41Z","created_by":"Jan","updated_at":"2026-06-10T13:34:53Z","started_at":"2026-06-10T13:25:33Z","closed_at":"2026-06-10T13:34:53Z","close_reason":"Fixed: shopping invoice PDF requests use dedicated wc_invoice_pdf_secret/shopify_invoice_pdf_secret settings and docs/env/plugin UI separate them from webhook secrets.","labels":["code-review","hmac","pr-154","security","shopping"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-2uu","title":"P8.2 'latest placed PO wins' supplier rule doesn't handle emergency/backup POs or transition timing (PR #155 follow-up)","description":"PR #155 P8.2 proposes:\n\n\u003e Update the product supplier automatically when a purchase order is placed for that product. If a PO contains multiple products, update each included product to that PO's supplier. If a product appears on multiple POs, the latest placed PO wins.\n\nThis rule has several operational gaps:\n\n**1. Emergency / backup supplier overrides the primary.**\n\nScenario: Product P is consistently sourced from Supplier A. A's stock is out for one week; ops places an emergency PO for P with Supplier B (a backup). After 'latest placed PO wins', P's supplier field is now B. The next reorder forecast → draft PO generator scopes P to supplier B, not A. Operator has to manually re-bind P to A, or A is silently demoted.\n\nWorkarounds: a 'supplier_lock' boolean per product to opt out of auto-update; or 'mark as one-off' flag on POs to skip the auto-update.\n\n**2. Update timing: PO_SENT vs receipt status.**\n\nThe plan says 'when a purchase order is placed'. Placement is the `PO_SENT` transition. If the PO is later returned or cancelled (RETURNED, CANCELLED) for fulfillment failure, the supplier auto-update is NOT rolled back. P now points at a supplier that couldn't deliver. Should the update fire on:\n - PO_SENT (decision: 'we're committing to this supplier')\n - PARTIALLY_RECEIVED (decision: 'this supplier has actually delivered')\n - RECEIVED (decision: 'this supplier completed delivery')\n\nDifferent operations teams answer this differently. Plan should pick.\n\n**3. Mass-update side effects.**\n\nA single PO line update transaction now triggers N product updates (one per line). For a 200-line PO, that's 200 `product` row updates in the same transaction as the PO line writes. Worth specifying:\n - Same-transaction or async via outbox?\n - Lock ordering: products by ID asc to avoid deadlock with concurrent stock movements that touch the same product rows\n - Activity-log volume: 200 'product_supplier_updated' activity-log entries per PO\n\n**4. PO multi-supplier impossible scenarios.**\n\nThe plan says 'If a PO contains multiple products, update each included product to that PO's supplier.' A PO has exactly one supplier (PurchaseOrder.supplierId), so 'PO's supplier' is always singular. Fine. But:\n - What about a PO line for a product where SupplierProduct doesn't have a row for this supplier? Implicitly creates one, or refuses?\n - What if the new supplier's currency differs from the product's expected cost basis?\n\n**5. Race with multi-PO concurrent placement.**\n\nTwo POs placed within milliseconds for the same product, different suppliers. Final value of `product.supplier` depends on transaction commit ordering. The plan says 'latest placed PO wins' but doesn't define 'placed' precisely — committed-at? sent-at? created-at?\n\n**Why this matters:**\n\nThe auto-update is the load-bearing mechanism for reorder draft generation in P8.2. Subtle race / timing / opt-out semantics determine whether ops trusts the suggestion or treats it as 'noise that must be manually corrected after every PO'.","acceptance_criteria":"- Update P8.2 to specify:\n - When the update fires (PO_SENT vs received)\n - Whether the update is per-tx or async\n - Opt-out mechanism for one-off / emergency POs (e.g., a 'don't update product supplier' flag on the PO header)\n - Concurrency policy for multi-PO placement (which timestamp wins)\n - Rollback semantics when a PO is cancelled / returned","notes":"Filed during PR #155 second-pass review. The 'latest wins' rule is simple to implement but loses signal about primary vs ad-hoc supplier choices. Worth getting ops/procurement sign-off before implementation.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-10T14:22:13Z","created_by":"Jan","updated_at":"2026-06-10T14:22:13Z","labels":["code-review","p8.2","plan","pr-155","procurement"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-tep","title":"P8.2 product supplier field overlaps with existing SupplierProduct many-to-many relation — spec needs to reconcile (PR #155 follow-up)","description":"PR #155 (commit 65b726da) adds P8.2 to docs/plan.md:\n\n\u003e 1. Add a supplier field to products, backed by a relation to the supplier/vendor model used by purchase orders.\n\nBut the codebase already has `SupplierProduct` (prisma/schema.prisma:943-958) — a many-to-many join model between Product and Supplier with per-supplier metadata:\n\n```prisma\nmodel SupplierProduct {\n id String @id\n supplierId String\n productId String\n supplierSku String?\n lastUnitCost Decimal\n currency String\n leadTimeDays Int?\n updatedAt DateTime @updatedAt\n\n @@unique([supplierId, productId])\n}\n```\n\nThis model is used by:\n- Supplier-portal product editing (referenced in PR #154)\n- Multi-supplier procurement (different lead times, costs, SKUs per supplier)\n- Supplier-specific historical cost tracking\n\nP8.2 proposes a SINGULAR `product.supplier` field updated by 'latest placed PO wins'. The spec doesn't reconcile:\n\n1. **Is the new field a 'preferred/primary supplier' pointer that supplements SupplierProduct?**\n - If yes, semantics are: 'this is the most recent supplier we used; the SupplierProduct relation still lists every supplier we've ever bought from'\n - The reorder draft generator uses the new field; the SupplierProduct relation continues to store per-supplier metadata\n \n2. **Is the new field replacing SupplierProduct?**\n - If yes, where does per-supplier metadata go (supplierSku, lastUnitCost, leadTimeDays)?\n - Multi-supplier procurement breaks: a product with 3 suppliers loses 2 of them\n - Supplier-portal features that depend on SupplierProduct break\n\n3. **Migration story:**\n - Backfill product.supplier from: most recent PO line, OR the first SupplierProduct row, OR null?\n - If a product has SupplierProduct rows but no PO history yet (e.g., newly added to a supplier's catalog), what's the supplier field set to?\n\n4. **Concurrent multi-supplier scenarios:**\n - A product genuinely sourced from 2 suppliers gets its 'supplier' field oscillating per-PO\n - Reorder forecasts generated per-supplier may exclude the product from the wrong supplier's draft\n\n**Why this matters:**\n\nThe spec's brevity hides a significant data-modeling decision. Whether to ADD a field next to an existing relation or REPLACE the relation is the kind of choice that benefits from being explicit upfront — implementing the wrong interpretation costs a redo.\n\nThe cleanest interpretation given the existing schema: keep `SupplierProduct` for historical/per-supplier metadata; add `Product.preferredSupplierId` (or similar) as an explicit 'most-recent-PO supplier' pointer; the auto-update rule populates this pointer only.","acceptance_criteria":"- Update P8.2 in docs/plan.md to:\n - Explicitly state the relationship to SupplierProduct (supplement vs replace)\n - Specify the field name (`preferredSupplierId` or `latestPoSupplierId` may be clearer than `supplier`)\n - Document the backfill policy: derive from most recent PO line, with fallback to most-recently-updated SupplierProduct row\n - Address multi-supplier products: how do reorder drafts handle a product with multiple historical suppliers? Per-supplier slicing?\n- Add transitions to P8.2's acceptance criteria for the SupplierProduct relation (does it stay, does it change?)","notes":"Filed during PR #155 second-pass review (commit 65b726da). P8.2 is plan-only; this is about pinning the spec before implementation. Related to onetwo3d-ims-ljw (P8.1 migration path needed).","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-10T14:21:45Z","created_by":"Jan","updated_at":"2026-06-10T14:21:45Z","labels":["code-review","p8.2","plan","pr-155","product-lifecycle"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-ljw","title":"P8.1 product-lifecycle plan doesn't specify the migration path from existing NOT_FOR_SALE / ACTIVE / ARCHIVED enum (PR #155)","description":"PR #155 adds Group 21 / P8.1 to plan.md with a proposed new product-lifecycle status enum:\n\n\u003e Product statuses: `active`, `draft`, `eol`, `archived`\n\nBut the codebase already has `ProductLifecycleStatus { ACTIVE, NOT_FOR_SALE, ARCHIVED }` at `prisma/schema.prisma:36-40`, used by `Product.lifecycleStatus`. The plan item doesn't address:\n\n1. **Mapping NOT_FOR_SALE → new states.** What happens to existing products in NOT_FOR_SALE? Some are arguably 'draft' (never published), some are arguably 'eol' (was published, removed from sale, still has stock). The plan needs a migration policy.\n\n2. **`draft` overlap with existing publication flag.** The PR's description of draft says 'generated or being prepared, not published for sale yet, but can already be purchased.' This may overlap with:\n - WooCommerce's product status (publish/draft/private)\n - A separate IMS 'published' boolean if one exists\n - The existing NOT_FOR_SALE status\n\n3. **Code paths assuming the current enum.** Search for `ProductLifecycleStatus`, `lifecycleStatus`, `ACTIVE`, `NOT_FOR_SALE`, `ARCHIVED` to find every existing consumer. Each needs updating to handle the new state machine. The plan acceptance criteria don't enumerate this surface.\n\n4. **`isOperationalProductStatus` helper.** Referenced in app/actions/purchase-orders.ts — what's it currently doing, and how should it map to the new states?\n\n5. **PO line creation / sales order creation / allocation** all probably check product status today. The plan doesn't enumerate which actions need updates.\n\n**Why this matters:** P8.1's acceptance criteria say 'each allowed and blocked transition' — but the transition diagram is unstated. Without it, the implementer has to invent the mapping from existing data + existing code paths to the new state machine, which is exactly the kind of decision that benefits from upfront planning.\n\n**Suggested additions to P8.1:**\n\n1. **Migration table**:\n | Old | New |\n |-----|-----|\n | ACTIVE | active |\n | NOT_FOR_SALE | eol (default) — operators reclassify to draft if pre-publication |\n | ARCHIVED | archived |\n\n2. **State transition diagram** explicitly listing which transitions are allowed:\n - draft → active (publish)\n - active → eol (start sell-off)\n - active → archived (immediate withdraw)\n - eol → archived (auto-archive job, or manual)\n - eol → active (un-EOL — undo)\n - archived → ? (allowed/disallowed?)\n\n3. **Code-path inventory**: list every action/query that checks lifecycleStatus today and what it should do for each new state.","acceptance_criteria":"- Update P8.1 in docs/plan.md with the migration table from current → new enum\n- Add the explicit state transition diagram (allowed/disallowed transitions)\n- Enumerate code paths that need updating (purchase-orders.ts, sales.ts, allocation.ts, product-list filters, etc.)\n- Decide explicitly whether 'draft' is a new IMS concept or maps to an existing publication flag","notes":"Filed during PR #155 review. The QG1 part of the PR is shippable; the P8.1 part is a plan-only addition that needs more spec before implementation.","status":"closed","priority":2,"issue_type":"bug","assignee":"Jan","owner":"dev@onetwo3d.com","created_at":"2026-06-10T14:14:04Z","created_by":"Jan","updated_at":"2026-06-10T14:54:05Z","started_at":"2026-06-10T14:41:29Z","closed_at":"2026-06-10T14:54:05Z","close_reason":"Addressed in PR #155: expanded P8.1 migration/transition/incoming-stock plan detail, added script-level preflight tests, added real-data preflight fixture workflow step, and documented CI gate scope.","labels":["code-review","p8.1","plan","pr-155","product-lifecycle"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-ejx","title":"invariant-preflight CI runs against a fresh empty DB — code-level gate only, not data-level (PR #155)","description":"PR #155's CI workflow runs:\n\n```yaml\n- run: npx prisma migrate deploy --schema prisma/schema.prisma\n- run: npm run invariant-check:preflight\n```\n\nThe DB is freshly migrated, has zero rows, and the invariant reporters scan an empty database. Every invariant check passes trivially because there's nothing to violate. The PR body acknowledges this: 'tenant data checks still need cron/operator runs against tenant databases'.\n\nThis is fine as a **code-level gate** (catches syntax errors, broken imports, queries that throw on empty data, new code paths that violate constraints at write time when the test suite exercises them). But it's NOT a data-level gate.\n\nConcrete misnomers:\n\n1. **Plan.md QG1 closure** says: `fail on report failures or critical invariant findings`. The PR body says the gate `blocks on critical findings/report failures`. Neither is wrong, but readers may assume the gate would catch a stock-quantity violation introduced by a refactor.\n\n2. **An empty-DB invariant report will return 0 findings even if the refactor under PR breaks the invariant logic itself** — unless the refactor causes the reporter to throw, the gate stays green.\n\n3. **The QG1 plan item** original intent: `Synthetic data with a known invariant violation; assert CI fails.` PR's tests assert CI fails ON A MOCKED RESULT but the actual CI workflow doesn't seed any violation.\n\n**Why this matters:**\n\nThe gate provides a false sense of coverage for the kind of bug it's named after ('inventory invariant violation'). A reviewer seeing the green CI badge might assume the underlying data has been verified. In practice, the CI checks that the report **code** runs to completion against an empty DB.\n\n**Suggested strengthening:**\n\n- Seed a small fixture with known invariant violations (negative stock, orphan cost layer, refund status mismatch) → assert the preflight FAILS on this fixture\n- Then RESET the fixture so the main preflight runs against an empty DB and passes\n- Document explicitly in docs/development.md that the CI gate covers code correctness, not tenant data correctness\n- Make the QG2 'per-phase regression test' convention the actual data-level safety net (tested with the focused unit tests on each PR)","acceptance_criteria":"- Add a CI step that seeds a known-violating fixture and asserts `invariant-check:preflight` exits non-zero\n- Then reset/clear the fixture and run the real preflight (which should pass)\n- Update docs/development.md to label the gate explicitly: 'code-correctness gate; data-level invariant checks live in scheduled cron jobs against tenant databases'\n- Update plan.md QG1 entry to reflect the gate's scope","notes":"Filed during PR #155 review. The gate has value (catches breaking code changes to invariant reporters) but the PR claims coverage broader than what it actually delivers.","status":"closed","priority":2,"issue_type":"bug","assignee":"Jan","owner":"dev@onetwo3d.com","created_at":"2026-06-10T14:13:34Z","created_by":"Jan","updated_at":"2026-06-10T14:54:06Z","started_at":"2026-06-10T14:41:30Z","closed_at":"2026-06-10T14:54:06Z","close_reason":"Addressed in PR #155: expanded P8.1 migration/transition/incoming-stock plan detail, added script-level preflight tests, added real-data preflight fixture workflow step, and documented CI gate scope.","labels":["ci","code-review","invariants","pr-155"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-fa5","title":"WP plugin invoice-pdf proxy follows unvalidated order-meta endpoint URL — SSRF surface (PR #154)","description":"PR #154's WP plugin (`lib/connectors/woocommerce/wp-plugin/onetwoinventory-helper.php`) adds a customer-facing invoice download endpoint that proxies to IMS:\n\n```php\nfunction oti_proxy_invoice_pdf(WP_REST_Request $request) {\n // ...\n $endpoint = oti_get_order_meta($order_id, '_ims_invoice_pdf_endpoint');\n $secret = (string) get_option(OTI_FX_SECRET_OPT, '');\n if ($endpoint === '' || $secret === '') {\n return new WP_Error('oti_invoice_not_configured', ...);\n }\n // ...\n $response = wp_remote_post($endpoint, [\n 'timeout' =\u003e 30,\n 'headers' =\u003e [\n 'Content-Type' =\u003e 'application/json',\n 'X-OTI-Signature' =\u003e hash_hmac('sha256', $body, $secret),\n ],\n 'body' =\u003e $body,\n ]);\n // ...\n}\n```\n\nThe proxy reads `$endpoint` from per-order meta key `_ims_invoice_pdf_endpoint`. This value is set by IMS (presumably) when generating the PDF. But:\n\n1. **Where does meta set?** Order meta is writable from many places: WP admin UI, REST API (with permissions), other plugins, direct DB. If an attacker can write to that meta key (compromised admin, malicious plugin, SQL injection via another vector), they redirect the proxy to an attacker-controlled URL.\n\n2. **No URL validation.** The endpoint is used verbatim — no scheme allowlist (http vs https), no domain allowlist, no DNS-rebinding protection. A malicious endpoint can be:\n - `https://attacker.example.com/` — capture the HMAC-signed payload\n - `http://169.254.169.254/...` — AWS metadata service (SSRF to cloud metadata)\n - `http://localhost:3306/...` — local services on the WP host\n - `gopher://...` — protocol smuggling\n\n3. **HMAC payload exfiltration.** Even if the response body is discarded by the plugin (which it actually uses for the PDF body, so the attacker controls what the customer downloads), the OUTBOUND request to the attacker URL contains the HMAC signature for the payload. The attacker now has a (body, signature) pair signed with the shared secret — confirms the secret hasn't rotated and provides material for further forgery.\n\n4. **PDF content substitution.** The proxy echoes `$pdf = wp_remote_retrieve_body($response);` as the customer-facing PDF. So if attacker controls the endpoint, attacker delivers a malicious PDF to the customer (phishing, malware-laden PDF, etc.) under the trusted WC site origin.\n\n**Why this matters:** The proxy is a server-side fetch with HMAC signing. The endpoint is meant to be a trusted IMS URL, but the code treats per-order meta as the source of truth without validating it's still pointing at IMS.\n\n**Mitigation:**\n\nPin the endpoint base URL in WP site options (set once during plugin configuration, signed/protected by admin-only access), and reject per-order meta endpoints. The order id can still come from meta; only the BASE URL must be pinned.","acceptance_criteria":"- Add a new site option `oti_ims_invoice_pdf_base_url` to the plugin settings page (admin-only)\n- Construct the endpoint server-side: `${base_url}/api/shopping/woocommerce/invoice-pdf` — ignore per-order meta for the URL\n- Drop the `_ims_invoice_pdf_endpoint` meta key entirely (or keep it as advisory only, with hash verification against the pinned base)\n- Add tests / docs explaining the trust boundary: WP plugin trusts WP site options (admin-set); does not trust per-order meta for URL choice\n- Audit any other places in the plugin that take URLs from meta or external data","notes":"Filed during PR #154 review. SSRF surface in a customer-facing plugin endpoint, mitigated by 'attacker needs to write order meta' but the attack surface is real. Mid-priority because the plugin is in OneTwo3D's deployment scope, not a public marketplace plugin — easier to audit and patch.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-10T11:41:38Z","created_by":"Jan","updated_at":"2026-06-10T11:41:38Z","labels":["code-review","pr-154","security","ssrf","wordpress"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-0qv","title":"Shopping invoice PDF nonce not tracked — replay within 5-minute TTL is unbounded (PR #154)","description":"PR #154's `parseShoppingInvoicePdfRequest` validates that the payload includes a `nonce` (non-empty string) but doesn't track which nonces have been seen.\n\n```ts\n// app/api/shopping/[connector]/invoice-pdf/route.ts\nconst parsed = parseShoppingInvoicePdfRequest(rawBody, connector, { now: dependencies.now })\nif (!parsed.valid) return reject(dependencies, auditBase, parsed.reason)\n// nonce is parsed but never checked against a 'seen' set\nconst order = await dependencies.findOrder(parsed.request)\n```\n\nThe PR's stated security model includes the nonce, but the IMS side accepts any non-empty UUID-shape string. So a valid HMAC-signed payload can be REPLAYED:\n\n- Same payload, same X-OTI-Signature, same timestamps\n- IMS validates HMAC (✓ — body is unchanged)\n- IMS parses payload (✓ — fields are still valid)\n- IMS checks issuedAt+TTL (✓ — still within 5 minutes)\n- IMS returns the PDF\n\nSo within the 5-minute TTL window, the SAME signed payload can be replayed N times. Rate limit caps at 60 req/min per IP, but:\n\n- N = 60/min × 5min = up to 300 replays per IP for the same valid payload\n- Across multiple IPs (proxied attacker, residential proxy networks), much higher\n\n**Why this matters:**\n\nFor most flows this is moot — the PDF doesn't change, the customer downloads it once. But:\n\n1. **PDF generation cost**: `loadInvoicePdf` reads from disk; cheap. If a future change generates the PDF on-demand, 300× replay = 300× generation cost.\n2. **Activity log spam**: each verified-and-accepted attempt is logged. The audit trail bloats with duplicates.\n3. **Combined with onetwo3d-ims-ecp (guest bypass)**: a compromised secret + enumerable guest order IDs means an attacker exfiltrates all guest invoices with bounded HMAC-signature work — and the nonce doesn't slow this down at all because they only need to sign once per order.\n\n**Standard nonce-tracking:**\n\n```ts\n// On verify, check the nonce hasn't been seen in the TTL window\nconst existing = await db.shoppingInvoicePdfNonce.findUnique({ where: { nonce } })\nif (existing) return reject(dependencies, auditBase, 'replay')\nawait db.shoppingInvoicePdfNonce.create({ data: { nonce, expiresAt } })\n// Periodic sweep clears expired nonces\n```\n\nOr use a TTL-bounded cache (Redis SET NX with EX). Simple, cheap, defeats replay.","acceptance_criteria":"- Add nonce tracking to the shopping invoice PDF route:\n - Option A (db): new table 'shopping_invoice_pdf_nonces' with (nonce, expiresAt); index on nonce; periodic sweep\n - Option B (redis/in-memory): if rate-limit backend already uses redis, store nonces there with TTL = TOKEN_TTL_SECONDS + clock skew\n- Tests: assert second request with same payload returns 'replay' rejection (or appropriate failure reason)\n- Update the failure-reason union: add 'replay_detected'\n- Document the nonce contract in lib/shopping-invoice-pdf.ts JSDoc","notes":"Filed during PR #154 review. Mid-priority — replay is bounded by HMAC secret possession AND TTL AND rate limit. But the design intent of 'nonce' is one-shot, and the implementation is currently advisory only.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-10T11:41:09Z","created_by":"Jan","updated_at":"2026-06-10T11:41:09Z","labels":["code-review","pr-154","replay-protection","security"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-d5l","title":"Shopping invoice PDF HMAC reuses wc_webhook_secret — key reuse across two purposes (PR #154)","description":"PR #154's `lib/shopping-invoice-pdf.ts`:\n\n```ts\nconst SHOPPING_INVOICE_SECRET_SETTING: Record\u003cShoppingConnectorId, string\u003e = {\n woocommerce: 'wc_webhook_secret',\n shopify: 'shopify_webhook_secret',\n}\n```\n\nThe same `wc_webhook_secret` is used for:\n\n1. **Incoming webhook verification**: WooCommerce → IMS posts an event, signs body with this secret, IMS verifies the signature.\n2. **Outgoing PDF request signing**: WP plugin → IMS posts a PDF request, signs body with this secret, IMS verifies the signature.\n\nThis is **key reuse across distinct purposes** — a well-known cryptographic anti-pattern (RFC 4949 §3.4). Concrete risks:\n\n1. **Cross-protocol replay**: a signed message intended for one purpose might be reinterpreted as another. WC webhook payloads and PDF request payloads are different JSON shapes, but:\n - A signed payload that happens to be valid JSON for BOTH schemas could be replayed across endpoints\n - The PDF schema is small (5-6 fields) — there's nontrivial overlap with hypothetical webhook payloads\n - Defense-in-depth principle: don't rely on schema differences to keep purposes separate\n\n2. **Leak amplification**: if the webhook secret leaks (e.g., from a WordPress site backup, a wp-config dump, a compromised admin), the attacker gets BOTH:\n - Ability to forge webhooks (data injection)\n - Ability to forge PDF requests (data exfiltration — coupled with the guest-checkout bypass in onetwo3d-ims-ecp this is total invoice access)\n\n3. **Rotation amplification**: rotating the webhook secret because of a webhook abuse incident also invalidates ALL pending PDF requests, breaking customer downloads. Same with rotating because of a PDF-request abuse incident. Coupling rotation events couples incident response.\n\n**Cryptographic best practice:**\n\nEach distinct cryptographic purpose gets its own key. Or use HKDF to derive purpose-specific subkeys from a single root:\n\n```ts\nhmacHex(`shopping-invoice-pdf:${connector}`, rootSecret) // derive\n// store derived key, use for signing/verifying PDF requests only\n```\n\n**Why this matters:** The HMAC contract here is the only authorization for the new shopping invoice PDF route. Cryptographic hygiene of the secret directly limits the abuse surface.","acceptance_criteria":"- Add a NEW secret `wc_invoice_pdf_secret` (and `shopify_invoice_pdf_secret`) distinct from the webhook secret\n- WP plugin's settings page should expose a separate field for this; OR derive the PDF secret from the webhook secret via HKDF with a purpose label\n- Test that a payload signed with the webhook secret is REJECTED by the PDF route (cross-protocol replay defense)\n- Document the key separation in docs/architecture.md and the helper plugin's README\n- Migration: existing installations need a settings UI to set the new secret OR documentation for the derived-key approach","notes":"Filed during PR #154 review. Pair with onetwo3d-ims-ecp because both undermine the PDF authorization layer, just from different angles.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-10T11:40:41Z","created_by":"Jan","updated_at":"2026-06-10T11:40:41Z","labels":["code-review","hmac","pr-154","security","shopping"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-ym4","title":"shouldGateWcEnable only checks first transition — config changes mid-stream bypass the gate (PR #152)","description":"PR #152's WC gate:\n\n```ts\nconst WC_ENABLE_SETTING_KEYS = [\n 'wc_sync_enabled',\n 'wc_sync_product_enabled',\n 'wc_stock_sync_enabled',\n 'wc_fx_push_enabled',\n] as const\n\nasync function shouldGateWcEnable(data: Partial\u003cWcSyncSettings\u003e): Promise\u003cboolean\u003e {\n const enabledKeys = WC_ENABLE_SETTING_KEYS.filter((key) =\u003e data[key] === 'true')\n if (enabledKeys.length === 0) return false\n const current = await getSettingValues([...enabledKeys])\n return enabledKeys.some((key) =\u003e current.get(key) !== 'true')\n}\n```\n\nThe gate fires only when AT LEAST ONE of the four enable-toggles TRANSITIONS from non-'true' to 'true'. Once a toggle is on, subsequent saves with the same toggle still 'true' DON'T fire the gate — even if other settings change.\n\n**Concrete bypass:**\n\n1. Operator tests WC connection with credentials X. Test passes.\n2. Operator saves `wc_sync_enabled = true`. Gate fires (transition off→on), passes (fingerprint matches), settings saved.\n3. Operator changes `wc_webhook_secret`, `wc_sync_interval_minutes`, or any other setting EXCEPT the 4 enable toggles. Calls `saveWcSyncSettings` again.\n4. Gate does NOT fire (no toggle transition). Settings saved.\n\nIf the changed setting is `wc_webhook_secret` (which affects webhook payload validation), the operator has saved a new secret without verifying it works. WC starts sending webhooks; IMS rejects them because the secret doesn't match what WC was configured with.\n\n**Why this matters:**\n\nThe gate's name suggests 'enabling sync requires a test'. But once enabled, any subsequent config change can land without retest. For settings that affect runtime behavior (webhook secret, base URL change, intervals), this is a real path to silent breakage.\n\n**Related observation:** `saveWcCredentials` (separate action) DOES record its own test result. But settings changes via `saveWcSyncSettings` (different action) DON'T retrigger the gate.","acceptance_criteria":"- Decide policy:\n - **Option A (strict)**: gate fires on ANY save when ANY of the enable-toggles is currently true OR being set to true\n - **Option B (settings-specific)**: identify which settings affect connectivity/runtime and gate ONLY when those change AND something is enabled\n - **Option C (status quo)**: keep first-transition gating, but document the limit explicitly\n- For SMTP/Mintsoft/Xero, audit the same pattern — when does the gate fire vs not?\n- Add a regression test for each integration that exercises 'enabled-but-config-changed' → assert gate fires (per chosen policy)","notes":"Filed during PR #152 review. The current shape catches the obvious 'enable for the first time' case but misses ongoing configuration drift.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-10T09:50:09Z","created_by":"Jan","updated_at":"2026-06-10T09:50:09Z","labels":["code-review","integrations","pr-152","woocommerce"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-16r","title":"Xero connection fingerprint doesn't include xero_client_secret — secret rotation bypasses the gate (PR #152)","description":"PR #152's Xero fingerprint:\n\n```ts\nasync function buildXeroConnectionFingerprint(): Promise\u003cstring\u003e {\n const [clientId, expectedTenantId, token] = await Promise.all([\n getSettingValue('xero_client_id'),\n getSettingValue('xero_expected_tenant_id'),\n db.accountingToken.findUnique({\n where: { connector: 'xero' },\n select: { tenantId: true, tenantName: true },\n }),\n ])\n return buildIntegrationConnectionFingerprint({\n clientId, expectedTenantId,\n tenantId: token?.tenantId ?? '',\n tenantName: token?.tenantName ?? '',\n })\n}\n```\n\nThe fingerprint includes `clientId`, `expectedTenantId`, `tenantId`, `tenantName` — but NOT `xero_client_secret`.\n\nIf an operator rotates `xero_client_secret` (e.g., after a suspected compromise, after a Xero app rotation) without re-testing, the fingerprint is UNCHANGED. The gate accepts the previous test's success result. The sync is enabled with an unverified secret.\n\nIf the new secret is correct, the sync just works (no harm). If the new secret is wrong (typo, copy-paste error), the sync FAILS on every API call but the gate said 'all good'. Operator sees confusing errors with no test-verified-baseline to compare against.\n\n**Why this matters:**\n\nThe gate's stated purpose (per the PR description) is 'require successful current-fingerprint connection tests before enabling sync'. The intent is to prevent enabling with invalid settings. Missing `client_secret` from the fingerprint creates a path where settings change AND the gate accepts the old result.\n\nSame audit needed for:\n- **WooCommerce** (`buildWcConnectionFingerprint`): includes url/key/secret — but does the codebase have other WC settings that affect the connection? (e.g., wc_api_version, wc_user_agent, etc.) If yes, those should be in the fingerprint too.\n- **Mintsoft** (`buildMintsoftConnectionFingerprint`): includes baseUrl/username/password/orderLookupConnector — but Mintsoft has additional config (webhook secrets, etc.) — check what's relevant.\n- **SMTP**: includes host/port/user/pass/secure/fromName/fromEmail/replyTo — looks comprehensive.\n\n**Concrete repro for Xero:**\n1. Configure Xero with current valid client_id + client_secret. Test passes; status='success'; fingerprint stored.\n2. Rotate `xero_client_secret` to a typo'd value via settings UI (assume no in-place validation).\n3. Toggle xero_sync_enabled off → on. The gate checks fingerprint: unchanged (still the same client_id etc.). Status is success. Gate PASSES.\n4. Next sync API call fails with 'unauthorized' from Xero.","acceptance_criteria":"- Audit each integration's fingerprint inputs against ALL settings that affect the connection:\n - Xero: add `clientSecret` (or its server-side HMAC per onetwo3d-ims-v6z)\n - WooCommerce: audit for missed settings\n - Mintsoft: audit for missed settings\n- Document in lib/integration-connection-test-gate.ts JSDoc the contract for fingerprint inputs ('include every setting that affects connectivity')\n- Add a focused test: for each integration, change each input field and assert the fingerprint changes\n- Coordinate with onetwo3d-ims-v6z (secret-in-fingerprint) — the fix for that may change how secrets are included","notes":"Filed during PR #152 review. Coupled with v6z because both concern the fingerprint's coverage of secret values.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-10T09:49:46Z","created_by":"Jan","updated_at":"2026-06-10T09:49:46Z","labels":["code-review","integrations","pr-152","security","xero"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-iys","title":"recordIntegrationConnectionTest does 4 separate upserts not in transaction — partial-write corrupts gate state (PR #152)","description":"PR #152's `recordIntegrationConnectionTest` writes 4 settings rows via `Promise.all` of 4 separate `upsert` calls:\n\n```ts\nawait Promise.all(TEST_KEYS.map((suffix) =\u003e\n repository.upsert({\n where: { key: testSettingKey(id, suffix) },\n create: { key: testSettingKey(id, suffix), value: values[suffix] },\n update: { value: values[suffix] },\n }),\n))\n```\n\nNot wrapped in a Prisma `$transaction`. If 1-3 of the 4 upserts fail (e.g., connection blip mid-batch, lock timeout), partial state lands:\n\n- `status: 'success'` written ✓\n- `tested_at` written ✓\n- `message` FAILED — keeps old value (e.g., 'Connection refused')\n- `fingerprint` FAILED — keeps old value (or null on first test)\n\nThe subsequent `getIntegrationConnectionTestState` reads these as 4 independent values, returning a mixed state:\n\n- `status: 'success'` + `fingerprint: \u003cstale\u003e` → gate compares against current expected fingerprint → MISMATCH → 'Retest the connection because settings changed' (false negative, user retests pointlessly)\n- OR worse: `status: 'success'` + `fingerprint: null` if first-ever test → gate says 'Retest because the saved settings changed' — but the test DID pass; operator confused\n\nThe more dangerous case: previous test was `status: 'failed'` with the CURRENT fingerprint. Operator clicks Test, the upserts partially succeed — `status: 'success'` written, but message + fingerprint failed. Gate evaluates: status=success + fingerprint matches (the OLD fingerprint that happens to be the current one) → GATE PASSES even though the test result is half-recorded and incoherent.\n\n**Why this matters:** The gate's job is to be a reliable trust signal. Partial writes that mix old + new fields produce reliability gaps in exactly the scenario the gate is meant to handle (settings flux).\n\n**Concrete repro:**\n1. Force a DB connection pool exhaustion during `recordIntegrationConnectionTest` (e.g., concurrent stress test)\n2. Observe 4 settings rows where some have new value, others have old value\n3. Hit any of the gate-checking save flows; observe the gate's verdict doesn't match what the operator just did","acceptance_criteria":"- Wrap the 4 upserts in `db.$transaction`:\n ```ts\n await db.$transaction(TEST_KEYS.map((suffix) =\u003e\n db.setting.upsert({ ... })\n ))\n ```\n Or use Prisma's interactive transaction if the repository abstraction needs adjustments.\n- Add a test that simulates upsert failure for one of the 4 keys and asserts that NONE land (atomic rollback)\n- Consider: store the 4 fields as JSON in a single setting row instead of 4 rows. Single write, no atomicity concern, but loses the 'individual key' query convenience. Trade-off.","notes":"Filed during PR #152 review. The Promise.all-of-upserts pattern is the same shape that's caused partial-state bugs in many systems. The fix is one transaction wrapper.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-10T09:49:20Z","created_by":"Jan","updated_at":"2026-06-10T09:49:20Z","labels":["code-review","consistency","integrations","pr-152"],"dependency_count":0,"dependent_count":0,"comment_count":0} @@ -43,7 +47,7 @@ {"_type":"issue","id":"onetwo3d-ims-p1s","title":"Remote backup manifest upload is unguarded — orphan SQL on upload failure (PR #148)","description":"PR #148's `app/api/backup/upload-remote/route.ts` chains two upload calls:\n\n```ts\nconst result = await uploadBackupToTarget(filePath, safe, target)\nawait uploadBackupToTarget(\n backupManifestPath(filePath),\n `${safe}.manifest.json`,\n target,\n 'application/json',\n)\n\nawait logActivity({ ... description: `Uploaded backup ... to ${target}` })\n```\n\nIf the FIRST upload (SQL) succeeds but the SECOND upload (manifest) throws:\n- The SQL backup is on the remote\n- The manifest is NOT on the remote\n- The error bubbles up to the operator\n- But the SQL file is orphaned on the remote with no manifest sidecar\n\nSame pattern in `app/api/cron/backup/route.ts` cron path — if SQL upload succeeds and manifest upload fails, cron returns the SQL upload error and the orphan stays.\n\n**Why this matters:** Future restore from the remote (via download → upload to restore endpoint) requires the manifest. An orphan SQL on the remote can't be restored cleanly via the stored-backup path because there's no manifest to validate.\n\nThe orphan also doesn't trigger any cleanup or alert — operator has to manually notice and either re-upload or delete.","acceptance_criteria":"- Wrap the two-upload sequence in a try/catch\n- If the manifest upload fails after SQL upload succeeded, delete the orphan SQL file from the remote (or queue a retry of the manifest upload)\n- Log the failure with sufficient detail to manually recover (`level: 'ERROR'` with metadata: { target, sqlKey, manifestKey })\n- Consider: a single `uploadBackupArtifactsToTarget()` helper that takes both files and either uploads both or neither\n- Add a test that asserts cleanup happens on manifest upload failure","notes":"Filed during PR #148 review. Same anti-pattern as PR #128/#129/#142's silent activity-log catches and PR #137's enqueueStockSync outside transaction — partial-success operations need explicit recovery.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-09T15:59:12Z","created_by":"Jan","updated_at":"2026-06-09T15:59:12Z","labels":["backup","code-review","pr-148","remote-storage"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-ed1","title":"Disk-space preflight uses file.size * 2 multiplier — wrong for SQL restores (PR #148)","description":"PR #148 adds a disk-space preflight in `createBackupRestorePostHandler`:\n\n```ts\nconst availableBytes = await resolvedDeps.getAvailableDiskBytes(resolvedDeps.backupDir)\nif (availableBytes \u003c file.size * 2) {\n return NextResponse.json({ error: 'Not enough disk space for restore upload.' }, { status: 507 })\n}\n```\n\nThe 2x multiplier assumes the file's own size dominates restore disk needs. For SQL restores, that's wrong.\n\nA SQL backup file is compressed text — `INSERT INTO ... VALUES (...)`. The same data in PostgreSQL's row format with indexes is typically **5-20x larger** than the SQL text representation. A 50MB SQL file restoring into Postgres produces a database that occupies hundreds of MB to several GB depending on indexes.\n\nThe 2x multiplier (~100MB available for a 50MB upload) is enough to copy the SQL file to disk — but nowhere near enough to actually run psql -f against it for a non-trivial database.\n\n**Why this matters:** The preflight gives operators false confidence ('disk space check passed') but the actual restore can still fail with `ERROR: could not extend file: No space left on device` mid-restore — leaving the database in a half-restored corrupted state because the SQL ran statements partially.\n\n**Why the 2x doesn't work even for small backups:** even if the database itself fits, pg's WAL during a large insert needs working space too.\n\nConcrete: a tenant with a 200MB SQL backup (50MB after gzip) restores to a 2-3GB database. The preflight passes on a server with 500MB free, but the restore exhausts disk halfway.","acceptance_criteria":"- Replace the file.size * 2 heuristic with an estimate based on backup-time database size (stored in manifest? add `databaseSizeBytes` field?)\n- For initial implementation, use a conservative multiplier: file.size * 10 catches more realistic cases\n- Document the heuristic limit explicitly in the preflight error message (\"requires ~Nx the SQL file size in free disk\")\n- Consider a separate `databaseSizeBytes` field in the manifest that the preflight reads directly","notes":"Filed during PR #148 review. The check is well-intentioned but its multiplier doesn't match how SQL restores consume disk. A failed restore mid-way is worse than a rejected restore upfront.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-09T15:58:56Z","created_by":"Jan","updated_at":"2026-06-09T15:58:56Z","labels":["backup","code-review","operational","pr-148"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-e30","title":"Backup manifest row counts collected separately from pg_dump — snapshot drift between dump and manifest (PR #148)","description":"PR #148's flow in `app/api/backup/create/route.ts` and `app/api/cron/backup/route.ts`:\n\n1. `execFile('pg_dump', args, ...)` — pg_dump runs in its own transaction snapshot.\n2. After pg_dump finishes successfully, `writeBackupManifestForFile(filePath, filename, prisma)` runs.\n3. The manifest function executes `SELECT COUNT(*) FROM \u003ctable\u003e` for every table.\n\nThe pg_dump snapshot and the manifest's COUNT queries are TWO DIFFERENT transaction snapshots. Between pg_dump finishing and the manifest collecting row counts (could be seconds for a large DB), writes can land that mismatch the manifest from the actual dump contents.\n\nConcrete scenarios:\n- A backup runs at 02:00. pg_dump dumps `sales_orders` with N rows. A new order lands at 02:01. The manifest writes `sales_orders.rowCount = N+1`. A restore that checks \"actual rows match manifest rowCount\" would fail or pass spuriously depending on which side is right.\n- Currently no restore-side validation actually compares the manifest's rowCount against post-restore counts — it just checks the manifest exists and critical tables are listed. So the snapshot mismatch is silent today. But the manifest claims to represent the dump.\n\n**Why this matters:** Future enhancements (\"restore validates row count parity\") would build on a foundation that doesn't actually guarantee snapshot consistency. The audit story of the manifest is also weaker — it's a snapshot of the database TAKEN AFTER pg_dump finished, not OF the dump itself.","acceptance_criteria":"- Use pg_dump's `--snapshot=\u003cid\u003e` flag to export with a known snapshot, then run the manifest queries against the same snapshot via `SET TRANSACTION SNAPSHOT '\u003cid\u003e'` in a Prisma transaction\n- Alternatively, run pg_dump and the manifest queries inside a single shared read-only transaction with REPEATABLE READ isolation\n- Add a test that exercises the race: write rows between pg_dump completion and manifest write, assert the manifest matches the dump (or document that it doesn't and the rowCount is advisory)\n- Document the consistency model in BackupManifest type's JSDoc","notes":"Filed during PR #148 review. Operational impact is low today (no restore-side validation uses rowCount). But this is foundational — fixing it later requires rebuilding manifest collection.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-09T15:58:38Z","created_by":"Jan","updated_at":"2026-06-09T15:58:38Z","labels":["backup","code-review","consistency","pr-148"],"dependency_count":0,"dependent_count":0,"comment_count":0} -{"_type":"issue","id":"onetwo3d-ims-eda","title":"Password policy common-password list contains 7 entries — order-of-magnitude too small (PR #146)","description":"PR #146 adds `lib/security/password-policy.ts` with:\n\n```ts\nconst COMMON_PASSWORDS = new Set([\n 'password',\n 'password1',\n 'password12',\n 'password123',\n 'password123!',\n 'adminadmin123!',\n 'onetwoinventory1!',\n])\n```\n\n7 entries. The intent is the right one (block obviously-bad passwords) but the list is way too small to materially improve security:\n\n- The well-known SecLists 'common-passwords-top-10000' has 10k entries — none of which (except the seed entries above) are caught.\n- Real attackers use lists with 100k–1M entries.\n- The RockYou leak alone contains 14M entries.\n\nEffect: `'Letmein123!'`, `'Qwerty123!'`, `'P@ssword123'`, `'Welcome2024!'` — none caught by this list. Each meets all the structural requirements (length, uppercase, number, symbol) and passes validation.\n\n**Why this matters:** The plan item P6.6 specifically calls for stronger password requirements. A 7-entry common-password list satisfies the letter (some check exists) but not the spirit (real common passwords are blocked) of the requirement.","acceptance_criteria":"- Either:\n - **Option A** (preferred): integrate HaveIBeenPwned Pwned Passwords k-anonymity API — submit first 5 chars of SHA1 hash, get list of suffixes, check locally. Catches actual breached passwords without sending the full password anywhere.\n - **Option B**: bundle a static list of top 10k common passwords (e.g., from SecLists). Lower fidelity than HIBP but better than 7.\n - **Option C**: keep the 7-entry list AS A FLOOR plus dictionary checks (no plain English words, max repeated characters, etc.)\n- Add tests asserting the policy rejects common attack-list passwords (e.g., 'Welcome2024!', 'Qwerty123!', 'P@ssword123')\n- Also add (probably worth a separate issue): max password length (e.g., 256) to prevent bcrypt DoS via long inputs","notes":"Filed during PR #146 review. The implementation works as written, but the protection is illusory for real attacker password lists.","status":"closed","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-08T23:08:33Z","created_by":"Jan","updated_at":"2026-06-10T13:35:03Z","closed_at":"2026-06-10T13:35:03Z","close_reason":"Fixed in final security-hardening branch: password policy now has a broader common-password floor, common pattern checks, max length guard, and tests for Welcome/Qwerty/P@ssword variants.","labels":["code-review","passwords","pr-146","security"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-eda","title":"Password policy common-password list contains 7 entries — order-of-magnitude too small (PR #146)","description":"PR #146 adds `lib/security/password-policy.ts` with:\n\n```ts\nconst COMMON_PASSWORDS = new Set([\n 'password',\n 'password1',\n 'password12',\n 'password123',\n 'password123!',\n 'adminadmin123!',\n 'onetwoinventory1!',\n])\n```\n\n7 entries. The intent is the right one (block obviously-bad passwords) but the list is way too small to materially improve security:\n\n- The well-known SecLists 'common-passwords-top-10000' has 10k entries — none of which (except the seed entries above) are caught.\n- Real attackers use lists with 100k–1M entries.\n- The RockYou leak alone contains 14M entries.\n\nEffect: `'Letmein123!'`, `'Qwerty123!'`, `'P@ssword123'`, `'Welcome2024!'` — none caught by this list. Each meets all the structural requirements (length, uppercase, number, symbol) and passes validation.\n\n**Why this matters:** The plan item P6.6 specifically calls for stronger password requirements. A 7-entry common-password list satisfies the letter (some check exists) but not the spirit (real common passwords are blocked) of the requirement.","acceptance_criteria":"- Either:\n - **Option A** (preferred): integrate HaveIBeenPwned Pwned Passwords k-anonymity API — submit first 5 chars of SHA1 hash, get list of suffixes, check locally. Catches actual breached passwords without sending the full password anywhere.\n - **Option B**: bundle a static list of top 10k common passwords (e.g., from SecLists). Lower fidelity than HIBP but better than 7.\n - **Option C**: keep the 7-entry list AS A FLOOR plus dictionary checks (no plain English words, max repeated characters, etc.)\n- Add tests asserting the policy rejects common attack-list passwords (e.g., 'Welcome2024!', 'Qwerty123!', 'P@ssword123')\n- Also add (probably worth a separate issue): max password length (e.g., 256) to prevent bcrypt DoS via long inputs","notes":"Filed during PR #146 review. The implementation works as written, but the protection is illusory for real attacker password lists.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-08T23:08:33Z","created_by":"Jan","updated_at":"2026-06-08T23:08:33Z","labels":["code-review","passwords","pr-146","security"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-quf","title":"Invoice PDF token max TTL hard-capped at 10 minutes with no admin override (PR #146)","description":"PR #146 sets:\n\n```ts\nconst DEFAULT_INVOICE_PDF_TOKEN_TTL_SECONDS = 10 * 60\nconst MAX_INVOICE_PDF_TOKEN_TTL_SECONDS = 10 * 60\n```\n\nBoth default and max are 10 minutes. The old values were 3-day default and 30-day max. The 30-day max was clearly too lax, but 10-min max means:\n\n1. **No admin override possible** — if a tenant needs longer TTL for a specific workflow (e.g., finance team scheduled an invoice review for tomorrow), there's no env or settings configuration that allows it.\n2. **Email-an-invoice-link workflows break** — operator emails themselves an invoice link from desktop, opens it on phone 15 minutes later → expired.\n3. **Batch download flows break** — opening multiple invoices, leaving tabs in background while doing other work, then downloading → first tabs expired.\n\nCombined with the IP-binding fragility (`onetwo3d-ims-qv1`), the link is effectively single-use within ~5 minutes from same device, same network. That's appropriate for the highest-security path but not configurable for tenants with more relaxed threat models.","acceptance_criteria":"- Make max TTL configurable via env var (e.g., `INVOICE_PDF_TOKEN_MAX_TTL_SECONDS`) with a sensible production default\n- Document the env var in `.env.example` and `docs/installation.md`\n- Keep the 10-minute DEFAULT (most operators won't override) but allow tenants to raise the cap when needed\n- Consider: separate TTL for 'fresh download from invoice page' (short, 10min) vs 'recovery from emailed link' (longer, 1h-24h)","notes":"Filed during PR #146 review. The PR's stated rationale ('shortens max/default TTL to 10 minutes') is sensible for the default; the lack of any override path is the operational risk.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-08T23:08:19Z","created_by":"Jan","updated_at":"2026-06-08T23:08:19Z","labels":["code-review","operational","pr-146","security"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-qv1","title":"Invoice PDF token IP-binding is operationally fragile for legitimate users (PR #146)","description":"PR #146 binds invoice PDF tokens to both the session and the client IP. The IP binding breaks token validity for legitimate users in several realistic scenarios:\n\n1. **Mobile users**: a user opens the invoice list on home WiFi, then their phone switches to cellular (different IP), then they tap the invoice link → token rejected with `wrong_ip`.\n2. **Corporate NAT / load balancers**: enterprise networks rotate the apparent client IP on subsequent requests through different egress routes → token rejected mid-session.\n3. **VPN reconnect**: VPN clients drop and reconnect with a new IP regularly → broken links.\n4. **Roaming / hotspot handoff**: mobile networks shift IPs every few minutes on long-running pages.\n5. **Tab restored after device sleep**: IP may have changed.\n\nThe threat model the binding addresses is 'shared link from a stolen URL'. But session-binding alone already prevents this for non-authenticated attackers (they don't have the session cookie). For authenticated attackers who do have the session, IP binding helps marginally but at significant operational cost.\n\n**Why this matters:** The PR description says PDF links 'are no longer generated into WooCommerce customer notes because the hardened token now requires current session/IP binding'. That's an explicit acknowledgment that the binding is too strict for non-interactive use. But the same strictness applies to interactive users on mobile/VPN/corporate networks.\n\n**Refs:**\n- `lib/invoice-pdf.ts` — `bindingHash('clientIp', ...)` in signing and verification\n- `app/api/invoices/[id]/route.ts` — `getInvoicePdfTokenBinding` extracts clientIp from `getClientIp(request.headers)`","acceptance_criteria":"- Decide on the threat model explicitly: is IP binding worth the operational cost given session binding is already enforced?\n- If yes (keep IP binding): document the operational caveats in user-facing help docs ('if you switch networks, refresh the invoice list to get a new link')\n- If no (drop IP binding): keep session binding; remove the `ipHash` field and `wrong_ip` reason\n- Consider middle ground: bind to /24 subnet rather than exact IP — catches most legitimate IP changes (mobile carrier subnet stays similar), still blocks attackers from another network\n- Add a 'PDF link expired or invalid — please request a new one from the invoice page' user-facing error that operators see when the binding fails (currently the route returns 404 / 403 which is opaque)","notes":"Filed during PR #146 review. The same trade-off was implicitly acknowledged by the PR's own removal of WC customer-note links. Worth making the trade-off explicit rather than discovering it via support tickets.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-08T23:08:02Z","created_by":"Jan","updated_at":"2026-06-08T23:08:02Z","labels":["code-review","operational","pr-146","security"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-gyk","title":"No daily cron creates account balance snapshots — tightened freshness window will fail in production (PR #145)","description":"Codex adversarial review of PR #145 (P5.3) identified:\n\nThe PR tightens the snapshot freshness window from 7 days to 1 day (previous-day required). But snapshot creation depends on a manual UI sync button (`app/(dashboard)/sync/xero-client.tsx:352`) and `app/actions/xero-sync.ts` — no scheduled cron handler exists in `app/api/cron/` for account balance snapshots specifically.\n\nConsequence: tenants that don't run a manual sync every day — or any fresh-install tenant — will have no previous-day snapshot. With this PR merged, every GL period-movement query will either return null (current behaviour) or fail loudly (if bd-304 is addressed).\n\nThis is a deployment dependency gap: the code tightens the contract without the operational mechanism to satisfy the contract.\n\n**Why this matters:** A tenant that never manually clicks Sync will see their COGS variance / GL period reports break silently (current) or loudly (after bd-304). Either way, the report is unavailable until someone clicks Sync — which they may not realise is required.\n\n**Reproduction:** Fresh tenant; no manual sync performed; invoke a GL COGS variance report for yesterday's period — no previous-day snapshot exists; report shows the user notice / fails.","acceptance_criteria":"- Add a scheduled cron handler (e.g. `app/api/cron/account-balance-snapshot/route.ts`) that creates the previous-day snapshot daily\n- Wire the cron entry into docs/installation.md / docs/architecture.md cron table\n- Add a fallback in the domain function: if no snapshot exists for asOf, fetch it on-demand from Xero (with explicit error if the fetch fails)\n- Document the cron dependency in CHANGELOG.md so tenants upgrading know to verify the cron is wired up\n- Add a startup / preflight assertion that fails if the cron is unscheduled in production","notes":"Filed during Codex review of PR #145. Codex severity P2; this is the classic 'tighten the contract without the operational backbone' rollout risk.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-08T22:16:10Z","created_by":"Jan","updated_at":"2026-06-08T22:16:10Z","labels":["accounting","code-review","cron","deployment","pr-145"],"dependency_count":0,"dependent_count":0,"comment_count":0} @@ -73,7 +77,10 @@ {"_type":"issue","id":"onetwo3d-ims-ggg","title":"Invoice/bill edits after Xero push are silently lost","description":"Once accountingInvoiceId is set, queueSalesInvoiceForOrder returns immediately. Even if it didn't, the Xero idempotency key is built from the internal sync-log entry ID, not invoice content, so Xero would dedupe content changes.\n\nRefs:\n- app/actions/sales.ts:1310 (early return on accountingInvoiceId)\n- lib/connectors/xero/sync-processor.ts:35 (idempotency key from sync log entry ID)\n- lib/connectors/xero/sync-processor.ts:50-65, :73 (same on bill side)\n\nFix direction: idempotency key = hash(content); on edit, queue an UPDATE op (Xero supports invoice updates pre-AUTHORISED).","notes":"Verified: claim holds up. queueSalesInvoiceForOrder at sales.ts:1310 short-circuits when accountingInvoiceId is set, and lib/connectors/xero/sync-processor.ts:35 builds idempotency keys from the internal sync-log entry ID rather than invoice content. So edits to a SalesOrder after Xero push are silently dropped on the floor — no UPDATE path exists.\n\nFixing this needs design work, not just a code patch:\n1. Schema: add SALES_INVOICE_UPDATE / PURCHASE_INVOICE_UPDATE entries to AccountingSyncType enum.\n2. Edit detection: SalesOrder line/discount/tax mutations need a hook that enqueues an UPDATE sync log when accountingInvoiceId is already set.\n3. Processor: branch in sync-processor.ts to PUT the invoice (Xero supports invoice updates while DRAFT or AUTHORISED, blocked once PAID).\n4. Idempotency: use a content hash (lines+totals+date) so re-edits generate fresh keys.\n5. UX: surface 'invoice in Xero is stale' in the UI when an edit happened post-push but the UPDATE is queued.\n\nRecommend: keep open, schedule as a focused 1-2 day task with finance team alignment on which fields trigger an UPDATE.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:57:55Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:28:38Z","labels":["code-review","sync-xero"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-qnu","title":"Refund of an unshipped order throws with unhelpful error","description":"Refund COGS reversal hard-fails if there are no shipment/allocation snapshots. The error doesn't distinguish 'no shipments exist' from 'snapshots missing' from 'snapshots stale'.\n\nRefs:\n- app/actions/sales.ts:2162-2214\n\nFix direction: detect the no-history case explicitly and either short-circuit (refund without COGS reversal) or surface a clear error to the user.","status":"closed","priority":2,"issue_type":"bug","assignee":"OneTwo3D IMS","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:57:55Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:10:57Z","started_at":"2026-04-25T10:09:34Z","closed_at":"2026-04-25T10:10:57Z","close_reason":"Refund cost-reversal helpers now: (1) short-circuit with [] when the line has no shipment/allocation history (pre-fulfillment refunds), (2) throw clear messages when history exists but is insufficient — distinguishing 'never fulfilled' from 'stale snapshot'","labels":["code-review","o2c","refunds"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-v30","title":"Cancelling a partially shipped order orphans PACKED shipments and stock","description":"When status is set to CANCELLED, all PENDING/PICKING/PACKED shipments are deleted but stock is not returned. SHIPPED shipments remain, so the order cannot be fully refunded — and the deleted PACKED shipment's stock is permanently lost.\n\nRefs:\n- app/actions/sales.ts:1467\n\nFix direction: generate RETURN_INBOUND stock movements for non-SHIPPED shipments before deletion, OR forbid CANCELLED on partially shipped orders.","status":"closed","priority":2,"issue_type":"bug","assignee":"OneTwo3D IMS","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:57:54Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:14:17Z","started_at":"2026-04-25T10:14:17Z","closed_at":"2026-04-25T10:14:17Z","close_reason":"Verified: not a bug. Cancel flow at sales.ts:1456-1483 (a) calls deallocateOrder which releases reservedQty via applyAllocationReservationDelta('release'); (b) only deletes PENDING/PICKING/PACKED shipments — none of which decrement physical stock (FIFO consumption + StockLevel.quantity decrement happens only at SHIPPED transition in allocation.ts:1303-1310, 1327-1329); (c) blocks CANCELLED on fully SHIPPED orders. No stock is lost. Partially shipped orders end up in CANCELLED status with SHIPPED lines preserved for refund processing — that's intentional, not a leak.","labels":["code-review","o2c","state-machine"],"dependency_count":0,"dependent_count":0,"comment_count":0} -{"_type":"issue","id":"onetwo3d-ims-xht","title":"PR #154 reintroduces invoice- filename + 10-min TTL cap + weak password list — already-filed PR #146 findings recur (PR #154)","description":"PR #154 makes substantively the same changes to the IMS admin invoice-pdf flow that PR #146 made. Several findings on PR #146 recur in PR #154 verbatim:\n\n- **Content-Disposition uses `invoice-${verification.payload.nonce}.pdf`** — random nonce in user's downloads folder, no security benefit. Filed as **onetwo3d-ims-2it** [P1].\n- **DEFAULT_INVOICE_PDF_TOKEN_TTL_SECONDS = 10 * 60 = MAX_INVOICE_PDF_TOKEN_TTL_SECONDS** — no admin override path. Filed as **onetwo3d-ims-quf** [P2].\n- **`COMMON_PASSWORDS`** is a 7-entry hardcoded Set. Filed as **onetwo3d-ims-eda** [P2].\n- **IP-binding via getClientIp() ?? 'unknown' fallback** — header-stripped requests collide on 'unknown'. Filed as **onetwo3d-ims-qv1** [P2] (PR #146) and **onetwo3d-ims-61b** [P2] (PR #148).\n- **`wrong_session` / `wrong_ip` audit not surfaced separately** — security signal not aggregated. Filed as **onetwo3d-ims-d1f** [P3].\n\n**Why this matters:** PR #146 was open at the time PR #154 was authored; PR #154 is presumably the merge-target that supersedes PR #146 with the additional shopping handoff route. The pre-existing findings carry forward and need to be addressed once at the merged scope, not separately per PR.\n\n**No new work needed** — this issue exists to:\n1. Flag the duplication so reviewers don't waste time re-finding the same issues\n2. Link the PR #146 → PR #154 trail in the bd tracker\n3. Make it clear which findings cluster needs to be addressed before PR #154 (or its merged successor) is merged","acceptance_criteria":"- When addressing PR #146 findings (or PR #154's equivalents), do so against whichever branch is the final merge target\n- Add a 'duplicates: onetwo3d-ims-2it, quf, eda, qv1/61b, d1f' note to PR #154's bd row\n- Close this issue when the underlying findings are addressed (or supersede with a single tracker if preferred)","notes":"Filed during PR #154 review. Cross-reference for the recurring findings between PR #146 and PR #154.","status":"closed","priority":3,"issue_type":"chore","assignee":"Jan","owner":"dev@onetwo3d.com","created_at":"2026-06-10T11:42:02Z","created_by":"Jan","updated_at":"2026-06-10T13:35:02Z","started_at":"2026-06-10T13:25:35Z","closed_at":"2026-06-10T13:35:02Z","close_reason":"Closed as duplicate tracker for PR #154: this branch fixed the actionable carried-over items in scope (human invoice filename, no unknown IP binding fallback for invoice PDF, stronger password-policy floor). TTL/IP policy and audit aggregation remain tracked by their original PR #146 beads.","labels":["already-filed","code-review","pr-154","security"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-f91","title":"P8.2 'preserve forecast evidence on each line' needs schema spec — draft-PO lines have no metadata field today (PR #155 follow-up)","description":"PR #155 P8.2 acceptance:\n\n\u003e Generated draft PO lines carry enough forecast metadata to explain the suggested quantity.\n\nBut the existing `PurchaseOrderLine` schema doesn't have a structured place for forecast evidence:\n\n- Existing fields: qty, unitCostForeign, unitCostBase, taxRateId, totalForeign, totalBase, taxBase, taxForeign, productId, poId, sortOrder, etc.\n- No `forecastEvidence` or `reorderMetadata` field\n\nThe plan implies a schema addition without enumerating it. The 'forecast metadata' likely includes:\n- Days-of-stock-remaining at forecast time\n- Forecast horizon used (e.g., 30 days)\n- Lead-time days\n- Forecast model used (avg / median / EMA)\n- Inputs (current stock, average daily sales, on-order qty, etc.)\n- Generated-at timestamp\n\n**Why this matters:**\n\nThe acceptance criterion 'lines carry enough forecast metadata to explain the suggested quantity' is unverifiable without knowing what 'enough' means and where it lives. Either:\n\n1. **Add a JSON field to PurchaseOrderLine** for reorder metadata — e.g., `reorderEvidence Json? // null unless line was generated from a reorder forecast`. Schema migration needed.\n\n2. **Add a parallel table** `ReorderDraftLineEvidence` keyed on the PO line — keeps the main schema clean but requires joins.\n\n3. **Activity log only** — write a one-off activity log entry per line generated, with the forecast inputs. Less queryable but no schema change.\n\nThe plan doesn't pick. Without picking, the implementer guesses, and subsequent UI work to 'show the forecast evidence' depends on the guess.\n\n**Also worth specifying:**\n\n- What forecast model is used? Existing reorder report logic at `lib/domain/inventory/replenishment-reports.ts` — does P8.2 reuse this or introduce a new model?\n- How is the suggested quantity computed? Days-of-stock × daily sales rate × safety factor? The plan should pin the formula or reference the existing one.\n- The 'reorder-eligible' check: is it based on the existing reorder report's threshold logic, or a new rule?","acceptance_criteria":"- Update P8.2 to specify:\n - Where forecast evidence is stored (PO line field vs parallel table vs activity log)\n - What evidence is required (specific fields)\n - Whether P8.2 reuses the existing reorder-report logic or introduces new\n - Concrete forecast model / formula reference","notes":"Filed during PR #155 second-pass review. P8.2 is plan-only; this is about pinning the schema decision before implementation begins.","status":"open","priority":3,"issue_type":"chore","owner":"dev@onetwo3d.com","created_at":"2026-06-10T14:22:43Z","created_by":"Jan","updated_at":"2026-06-10T14:22:43Z","labels":["code-review","p8.2","plan","pr-155","reorder-forecast"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-l7d","title":"invariant-check-preflight script: no integration test against the real runScheduledInvariantCheck path (PR #155)","description":"PR #155's tests (`tests/cron/invariant-check-preflight.test.ts`) pass a mocked `runCheck` function to `runInvariantCheckPreflight`:\n\n```ts\nconst result = await runInvariantCheckPreflight(async () =\u003e scheduledResult({ ... }))\n```\n\nFour tests cover:\n- Happy path (no critical findings)\n- Critical findings present\n- Summary/list divergence (defensive check)\n- Partial report failure\n\nWhat's NOT tested:\n\n1. **The default `runCheck` parameter** (which constructs a real `runScheduledInvariantCheck` call with disabled side effects). The default is the codepath actually used by the script. If a future change breaks the parameter defaults (e.g., `writeActivityLog` accidentally re-enabled, causing CI side effects), no test catches it.\n\n2. **The `scripts/invariant-check-preflight.ts` orchestration**: `disconnectDb`, `process.exit(process.exitCode ?? 0)`, console output format, MAX_PRINTED_CRITICAL_FINDINGS truncation. None of these have tests.\n\n3. **End-to-end CI smoke**: the workflow yaml is verified-on-paper (run lint, type-check, validate). No test invokes `npm run invariant-check:preflight` against a known fixture and asserts behavior.\n\n**Why this matters:**\n\nThe CI gate is binary (pass/fail) — its only output is the exit code. A regression that always passes (e.g., `preflight.ok` accidentally hard-coded to true, or the script's `process.exitCode = 1` not firing) would silently turn the gate into a no-op, and no test would catch it.\n\nThis is the same 'test by inspection, not behavior' theme as PRs #150 (`9pk`), #151 (`cgq`), #152 (helper-only), #153 (`40l`).\n\n**Suggested fix:**\n\nAdd a focused script-level test that:\n1. Sets up a fixture DB or a mock-injected dependency\n2. Invokes the script's main function (extract it as exported for testability)\n3. Asserts the exit code matches expectations for: clean run, critical findings, report errors\n4. Asserts console output contains expected strings (truncation, remediation hint)","acceptance_criteria":"- Extract scripts/invariant-check-preflight.ts main() into a testable export\n- Add tests for: exit-code 0 on clean, exit-code 1 on critical findings, exit-code 1 on report errors, MAX_PRINTED truncation\n- Optionally: add an integration test that runs the real runScheduledInvariantCheck against a seeded fixture, asserts a known critical finding triggers exit 1","notes":"Filed during PR #155 review. Recurring 'mocked test only' pattern across the rollout. Lower priority because the script is small and the failure modes are well-bounded.","status":"closed","priority":3,"issue_type":"chore","assignee":"Jan","owner":"dev@onetwo3d.com","created_at":"2026-06-10T14:14:51Z","created_by":"Jan","updated_at":"2026-06-10T14:54:06Z","started_at":"2026-06-10T14:41:31Z","closed_at":"2026-06-10T14:54:06Z","close_reason":"Addressed in PR #155: expanded P8.1 migration/transition/incoming-stock plan detail, added script-level preflight tests, added real-data preflight fixture workflow step, and documented CI gate scope.","labels":["ci","invariants","pr-155","tests"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-dn2","title":"P8.1 'incoming stock' definition for auto-archive job spans many tables — needs concrete query spec (PR #155)","description":"PR #155's P8.1 plan item proposes an automatic archive job:\n\n\u003e when an EOL product has zero stock in every warehouse and no incoming stock remains, transition it to archived\n\u003e Incoming stock should include open purchase-order receipts and any modeled inbound supply such as inbound transfers, manufacturing outputs, or WMS ASN/booked-in evidence.\n\n'Incoming stock' as defined spans at least 5 different table contexts:\n\n1. **Open PO lines**: `purchase_order_lines` with `qty - qtyReceived \u003e 0` on POs in committed statuses (PO_SENT, SHIPPED, PARTIALLY_RECEIVED)\n2. **Inbound stock transfers**: `stock_transfer_lines` with `qty - qtyReceived \u003e 0` on transfers IN_TRANSIT to a warehouse holding the product\n3. **Manufacturing output**: `production_orders` with status in pre-completion states where outputProductId matches\n4. **WMS ASN evidence**: `wms_asn_lines` / `wms_booked_in_events` for the product\n5. **Implicit inbound** — refund returns? Customer returns? Inbound from supplier returns?\n\nA single 'has any incoming stock?' check requires querying ALL of these. The plan acceptance ('archive only after all warehouse stock is zero and incoming supply is zero') glosses over the actual implementation surface.\n\n**Why this matters:**\n\n1. Each table is independently maintained — missing one means false-positive archive (archive a product that has incoming stock the job didn't check)\n2. The job needs to run periodically (cron) — frequent enough to catch the 'stock reached zero' transition, infrequent enough not to thrash. Plan doesn't specify cadence.\n3. Race window: between the check and the status update, a new inbound supply event could land (e.g., operator creates a new PO line). The job needs to be transaction-safe.\n\n**Suggested concrete spec for P8.1:**\n\nDefine a single `getProductIncomingStock(productId)` helper that:\n- Returns a structured breakdown by source (open PO, transfer in, mfg output, WMS ASN, etc.)\n- Each source is its own query function for testability\n- Helper used by the auto-archive job AND by the operator-facing 'why didn't this archive?' diagnostic\n\nDefine cron cadence: probably daily, since the underlying event is 'stock dropped to zero', which itself happens at most a few times per day per product.\n\nDefine the race-safe transition: the archive should happen INSIDE a transaction that locks the product row and re-verifies the incoming-stock-zero condition.","acceptance_criteria":"- Update P8.1 in docs/plan.md with the concrete getProductIncomingStock breakdown (list of source tables and their conditions)\n- Define cron cadence and rationale\n- Define transaction-safety policy for the archive transition\n- Add a test acceptance: 'job archives product P → no transitions happen if a new PO line for P is created between the read and the write'","notes":"Filed during PR #155 review. Lower priority because P8.1 is plan-only; the implementer can fill in this detail. But it's the kind of detail that surfaces inconsistencies when not pinned upfront.","status":"closed","priority":3,"issue_type":"chore","assignee":"Jan","owner":"dev@onetwo3d.com","created_at":"2026-06-10T14:14:26Z","created_by":"Jan","updated_at":"2026-06-10T14:54:06Z","started_at":"2026-06-10T14:41:30Z","closed_at":"2026-06-10T14:54:06Z","close_reason":"Addressed in PR #155: expanded P8.1 migration/transition/incoming-stock plan detail, added script-level preflight tests, added real-data preflight fixture workflow step, and documented CI gate scope.","labels":["code-review","p8.1","plan","pr-155","product-lifecycle"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-xht","title":"PR #154 reintroduces invoice- filename + 10-min TTL cap + weak password list — already-filed PR #146 findings recur (PR #154)","description":"PR #154 makes substantively the same changes to the IMS admin invoice-pdf flow that PR #146 made. Several findings on PR #146 recur in PR #154 verbatim:\n\n- **Content-Disposition uses `invoice-${verification.payload.nonce}.pdf`** — random nonce in user's downloads folder, no security benefit. Filed as **onetwo3d-ims-2it** [P1].\n- **DEFAULT_INVOICE_PDF_TOKEN_TTL_SECONDS = 10 * 60 = MAX_INVOICE_PDF_TOKEN_TTL_SECONDS** — no admin override path. Filed as **onetwo3d-ims-quf** [P2].\n- **`COMMON_PASSWORDS`** is a 7-entry hardcoded Set. Filed as **onetwo3d-ims-eda** [P2].\n- **IP-binding via getClientIp() ?? 'unknown' fallback** — header-stripped requests collide on 'unknown'. Filed as **onetwo3d-ims-qv1** [P2] (PR #146) and **onetwo3d-ims-61b** [P2] (PR #148).\n- **`wrong_session` / `wrong_ip` audit not surfaced separately** — security signal not aggregated. Filed as **onetwo3d-ims-d1f** [P3].\n\n**Why this matters:** PR #146 was open at the time PR #154 was authored; PR #154 is presumably the merge-target that supersedes PR #146 with the additional shopping handoff route. The pre-existing findings carry forward and need to be addressed once at the merged scope, not separately per PR.\n\n**No new work needed** — this issue exists to:\n1. Flag the duplication so reviewers don't waste time re-finding the same issues\n2. Link the PR #146 → PR #154 trail in the bd tracker\n3. Make it clear which findings cluster needs to be addressed before PR #154 (or its merged successor) is merged","acceptance_criteria":"- When addressing PR #146 findings (or PR #154's equivalents), do so against whichever branch is the final merge target\n- Add a 'duplicates: onetwo3d-ims-2it, quf, eda, qv1/61b, d1f' note to PR #154's bd row\n- Close this issue when the underlying findings are addressed (or supersede with a single tracker if preferred)","notes":"Filed during PR #154 review. Cross-reference for the recurring findings between PR #146 and PR #154.","status":"open","priority":3,"issue_type":"chore","owner":"dev@onetwo3d.com","created_at":"2026-06-10T11:42:02Z","created_by":"Jan","updated_at":"2026-06-10T11:42:02Z","labels":["already-filed","code-review","pr-154","security"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-e5e","title":"getSidebarAnalyticsChildren exported from React component file — pulls React into test imports (PR #153)","description":"PR #153 exports the pure function `getSidebarAnalyticsChildren` from `components/layout/sidebar.tsx` — the same file that defines the React component `Sidebar`.\n\n```ts\n// components/layout/sidebar.tsx\nexport function getSidebarAnalyticsChildren(userRole: string): SidebarLink[] {\n // ... pure data + permission logic\n}\n\nexport function Sidebar({ ... }) {\n // ... React component\n}\n```\n\nThe test in `tests/components/sidebar.test.ts` imports the function:\n\n```ts\nimport { getSidebarAnalyticsChildren } from '../../components/layout/sidebar.tsx'\n```\n\nThis pulls React, the JSX runtime, all icon imports, all child component imports into the test's module graph just to call a pure function. Side effects:\n\n1. **Test startup cost** — Node has to parse all the JSX imports even though they're not used\n2. **Cross-environment coupling** — pure logic depends on React being available; can't easily move the function to a non-React context (e.g., a future API endpoint that returns the sidebar structure as JSON)\n3. **Standard pattern violation** — most codebases separate pure rendering logic into `lib/*` or `utils/*` modules; the component imports from there\n\nThe clean shape:\n\n```ts\n// lib/sidebar/analytics-access.ts (new file)\nexport function getSidebarAnalyticsChildren(userRole: string): SidebarLink[] { ... }\nexport const BASE_ANALYTICS_LINKS = [ ... ]\nexport const REPORT_ACCESS_GROUPS = [ ... ]\n\n// components/layout/sidebar.tsx\nimport { getSidebarAnalyticsChildren } from '@/lib/sidebar/analytics-access'\n\nexport function Sidebar({ ... }) {\n const analyticsChildren = getSidebarAnalyticsChildren(userRole)\n // ...\n}\n\n// tests/lib/sidebar/analytics-access.test.ts\nimport { getSidebarAnalyticsChildren } from '@/lib/sidebar/analytics-access'\n// no React in scope\n```\n\n**Why this matters:** The PR's stated goal is consolidation. The current shape consolidates the logic but co-locates it with the React component. A better consolidation puts the logic in its own module and lets the component import it. Net code is the same, but the architecture is cleaner and the test surface is narrower.\n\n**Lower priority because:** the current shape works, the code compiles, the test passes. This is structural cleanliness, not a correctness or performance concern.","acceptance_criteria":"- Extract getSidebarAnalyticsChildren, BASE_ANALYTICS_LINKS, REPORT_ACCESS_GROUPS, uniqueLinks, and SidebarLink type to lib/sidebar/analytics-access.ts (or similar)\n- Update components/layout/sidebar.tsx to import from the new module\n- Move tests/components/sidebar.test.ts to tests/lib/sidebar/analytics-access.test.ts and update import paths\n- Confirm no other consumers of the sidebar file need updating","notes":"Filed during PR #153 review. The refactor's stated goal is consolidation; this is just suggesting a more architectural consolidation pattern.","status":"open","priority":3,"issue_type":"chore","owner":"dev@onetwo3d.com","created_at":"2026-06-10T10:21:41Z","created_by":"Jan","updated_at":"2026-06-10T10:21:41Z","labels":["architecture","code-review","pr-153","sidebar"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-40l","title":"Sidebar tests assert hand-built href lists per role — couples test to data, not policy (PR #153)","description":"PR #153 adds `tests/components/sidebar.test.ts` with EXPECTED_ANALYTICS_HREFS — hand-built lists of 38 hrefs for ADMIN, 35 for MANAGER, 9 for WAREHOUSE, etc.\n\n```ts\nconst EXPECTED_ANALYTICS_HREFS = {\n ADMIN: ['/analytics/sales-stats', '/analytics/purchase-stats', ... 36 more],\n MANAGER: [...],\n WAREHOUSE: ['/analytics/stock-on-hand', '/analytics/inventory-aging', ...],\n ...\n}\n\nfor (const [role, expectedHrefs] of Object.entries(EXPECTED_ANALYTICS_HREFS)) {\n test(`sidebar analytics links are scoped for ${role}`, () =\u003e {\n const hrefs = getSidebarAnalyticsChildren(role).map((link) =\u003e link.href)\n assert.deepEqual(hrefs, expectedHrefs)\n })\n}\n```\n\nThis pattern has a maintenance cost that grows with the number of analytics pages:\n\n1. **Adding a new analytics report** = update the source link list AND every relevant role's expected list. The test author has to think 'which roles should see this?' for every new page. Easy to forget one (test passes for the wrong reason).\n\n2. **Permission rule changes** = update many test expectations. A change like 'FINANCE no longer sees production-variance' requires deleting that href from the FINANCE expected list. The test asserts the current list state, not the rule.\n\n3. **Test doesn't catch the underlying bug**: if a permission check is silently broken (e.g., `canAccessSalesAnalytics` returns true for everyone), the test fails because the expected list doesn't include the extra link. But the actual root cause is the permission check, not the link list — the test points at the wrong thing.\n\nA more durable test shape:\n\n```ts\n// For each (role, link) pair, assert: link appears iff the underlying permission check would allow it.\nfor (const role of ALL_ROLES) {\n for (const group of REPORT_ACCESS_GROUPS) {\n const links = getSidebarAnalyticsChildren(role)\n const includes = (href: string) =\u003e links.some((link) =\u003e link.href === href)\n for (const link of group.links) {\n assert.equal(includes(link.href), group.canAccess(role), `${role}: ${link.href} visibility matches canAccess`)\n }\n }\n}\n```\n\nThis decouples the test from the SPECIFIC list and asserts the RULE the function implements. Adding a new page only requires the page to be in the right group; the test continues to assert the rule.\n\n**Why this matters:** Same theme as previous PRs (PR #150 `9pk`, PR #151 `cgq`, PR #152's helper-only coverage). The test verifies output shape, not the underlying contract. This makes the test brittle to legitimate changes and weak against the bugs you'd actually want to catch.","acceptance_criteria":"- Refactor the test to assert the RULE (link appears iff canAccess returns true)\n- Keep one happy-path snapshot test for ADMIN as a regression sanity check (catches widespread misconfiguration)\n- Add a test asserting that BASE_ANALYTICS_LINKS appear only when role has 'analytics' permission (independently of the report groups)\n- Add a test for the dedup behavior (currently uncovered): two groups with overlapping hrefs → assert the link appears exactly once","notes":"Filed during PR #153 review. The current tests catch the happy path well; they're just expensive to maintain as the analytics surface grows.","status":"open","priority":3,"issue_type":"chore","owner":"dev@onetwo3d.com","created_at":"2026-06-10T10:21:20Z","created_by":"Jan","updated_at":"2026-06-10T10:21:20Z","labels":["pr-153","rbac","sidebar","tests"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-y6q","title":"Sidebar analytics gate behavior change: previously stock-position-only, now any-analytics-links (PR #153)","description":"PR #153 changes the Analytics NavGroup render gate:\n\n```diff\n- {canAccessStockPositionReports(userRole) \u0026\u0026 (\n+ {analyticsChildren.length \u003e 0 \u0026\u0026 (\n \u003cNavGroup label=\"Analytics\" icon={BarChart3} items={analyticsChildren} ... /\u003e\n )}\n```\n\nThe old gate: show the Analytics group only if the role has stock-position access.\nThe new gate: show the Analytics group if ANY analytics link is accessible.\n\n**For today's role definitions:** the test expected lists show every role with analytics links also has stock-on-hand → the behavior is identical in practice. ADMIN, MANAGER, FINANCE, READONLY all see Analytics; WAREHOUSE sees Analytics (because they have stock-position access); SUPPLIER sees nothing.\n\n**For future role tuning:** if a role is granted 'analytics' permission OR access to one report group BUT has stock-position access removed:\n- **Old**: Analytics group HIDDEN (gate failed on stock-position specifically)\n- **New**: Analytics group SHOWN with that role's accessible links\n\nThis is a behavioral broadening with no permission policy change behind it.\n\nThe PR description says 'No auth policy change; this only consolidates the sidebar visibility logic'. The link contents are unchanged, but the gate condition's *intent* shifted from 'must have stock-position to see the group' to 'has any analytics link to see the group'. Worth confirming this aligns with intent.\n\n**Why this matters:**\n\nThis is purely about future-proofing. Today, no role hits the edge case. The next time someone tweaks permissions (e.g., creates a 'SALES_ANALYST' role with only sales analytics access), the new gate decides correctly without intervention; the old gate would have required someone to remember to add stock-position permission. Could be a feature, could be a regression depending on intent.\n\n**Possible improvements:**\n- Document the gate's intent in a comment ('any analytics link → show the group') so it's clear this is deliberate\n- Or, if the original 'requires stock-position' was intentional (e.g., 'admin gives stock-position as a sentinel for analytics visibility'), restore that semantic explicitly","acceptance_criteria":"- Confirm with the team: is the new 'any link visible → show group' gate intentional?\n- If yes, add a code comment on the NavGroup render explaining the semantic\n- Optionally: add a test asserting the gate-vs-content alignment (no role has analyticsChildren.length \u003e 0 but is meant to hide the group)","notes":"Filed during PR #153 review. The refactor is small and the behavior matches today, but the gate's semantic shifted in a way that's invisible until permissions are tuned.","status":"open","priority":3,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-10T10:20:53Z","created_by":"Jan","updated_at":"2026-06-10T10:20:53Z","labels":["code-review","pr-153","rbac","sidebar"],"dependency_count":0,"dependent_count":0,"comment_count":0} @@ -101,5 +108,5 @@ {"_type":"issue","id":"onetwo3d-ims-n39","title":"Xero rate-limit budget is in-memory only — resets on Node restart","description":"Minute/day buckets reset on every Node restart. Deployments mid-sync trigger cascading 429s on the new process.\n\nRefs:\n- lib/connectors/xero/api.ts:83\n\nFix direction: persist bucket counters to Redis or a small DB table keyed on tenant ID.","status":"closed","priority":3,"issue_type":"bug","assignee":"OneTwo3D IMS","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:57:57Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:28:37Z","started_at":"2026-04-25T10:28:37Z","closed_at":"2026-04-25T10:28:37Z","close_reason":"Verified: minor concern, not a real bug in practice. In-memory rate-limit buckets reset on Node restart, BUT processPendingXeroSync caps at MAX_PER_RUN=50 (sync-processor.ts:19) and runs every 5 min — i.e. ~10 req/min average vs Xero's 60/min cap. Even with cold-start bursting, 50 req in \u003c60s is at most 83% of the per-minute cap (one process). The exponential backoff on 429 (api.ts:83-88) handles the rare case where we collide with another process. Persisting buckets to Redis would be a defensive improvement but isn't a bug fix.","labels":["code-review","reliability","sync-xero"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-7mf","title":"Xero invoices stuck in DRAFT after payment lands","description":"Posting mode (DRAFT vs AUTHORISED) is decided once at sync time. There is no follow-up to promote a DRAFT invoice to AUTHORISED when payment is detected.\n\nRefs:\n- lib/connectors/xero/sync-processor.ts:233-235 (comment acknowledges the distinction), :246-250\n\nVERIFY: may not be IMS's responsibility if the Xero workflow handles AUTHORISED transitions itself.","status":"closed","priority":3,"issue_type":"bug","assignee":"OneTwo3D IMS","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:57:56Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:31:51Z","started_at":"2026-04-25T10:31:51Z","closed_at":"2026-04-25T10:31:51Z","close_reason":"Closing as not-actionable. Posting mode is decided per AccountingSyncLog entry; downstream tooling (Xero web UI, Xero approver workflows) is responsible for promoting DRAFT to AUTHORISED. Building an IMS-side promotion job duplicates Xero's invoice approval workflow, which is the auditor-preferred path. If a customer has DRAFT invoices that need to be AUTHORISED en masse on payment, that's a Xero workflow rule, not an IMS code change.","labels":["code-review","sync-xero","verify"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-5mp.3","title":"Document idempotency behaviour when fxRateToBase changes between requeues","description":"Concern surfaced in the Phase 1 Codex review.\n\naccountingPayloadKey() (app/actions/purchase-orders.ts:27) hashes the full payload as the idempotency key. With the new currencyRateToBase field included in the payload, a re-queue of the same document where the FX rate has changed produces a *different* hash, which means:\n\n- queue-level dedupe at lib/accounting.ts:157-169 misses (different key)\n- Xero idempotency at lib/connectors/xero/api.ts:110-112 is entry-id based, so it also won't dedupe\n\nIn theory this could allow a duplicate Xero post if the rate changed between the original queue and a retry. In practice rates don't change between retries (the rate was already stamped on the IMS document at creation time and we read it from there, not re-fetched), but the behaviour is non-obvious.\n\nAction:\n1. Document this in the unified-FX plan (docs/todo/unified-fx-rates-plan.md) under 'Open questions' or a new 'Idempotency' section.\n2. Decide: should accountingPayloadKey() exclude currencyRateToBase from the hash so a rate change doesn't produce a new key? OR should it be included so a deliberate re-stamp does produce a new post?\n3. Either way, add a code comment explaining the choice.\n\nLow priority — this is a 'nice to have clarification', not a known bug.","status":"open","priority":4,"issue_type":"task","owner":"dev@onetwo3d.com","created_at":"2026-04-25T11:48:27Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T11:48:27Z","labels":["connectors","fx","idempotency","woocommerce","xero"],"dependencies":[{"issue_id":"onetwo3d-ims-5mp.3","depends_on_id":"onetwo3d-ims-5mp","type":"parent-child","created_at":"2026-04-25T11:48:27Z","created_by":"OneTwo3D IMS","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0} -{"_type":"memory","key":"pr-review-bd-followup-push-target","value":"When filing bd issues as PR-review follow-ups, push the resulting .beads/issues.jsonl change to the SAME PR's branch (not to whatever branch the local working tree is on). Workflow: check out the PR's branch (gh pr checkout N), file bd issues, commit only .beads/issues.jsonl, push to the PR branch. This keeps review findings tied to the PR's history and avoids creating drive-by commits on unrelated feature branches."} {"_type":"memory","key":"list","value":"list"} +{"_type":"memory","key":"pr-review-bd-followup-push-target","value":"When filing bd issues as PR-review follow-ups, push the resulting .beads/issues.jsonl change to the SAME PR's branch (not to whatever branch the local working tree is on). Workflow: check out the PR's branch (gh pr checkout N), file bd issues, commit only .beads/issues.jsonl, push to the PR branch. This keeps review findings tied to the PR's history and avoids creating drive-by commits on unrelated feature branches."} diff --git a/.github/workflows/production-readiness.yml b/.github/workflows/production-readiness.yml index 07451bce..03807f58 100644 --- a/.github/workflows/production-readiness.yml +++ b/.github/workflows/production-readiness.yml @@ -142,6 +142,39 @@ jobs: FILE_SCAN_MODE: command FILE_SCAN_COMMAND_ARGV: '["true","{file}"]' + invariant-preflight: + needs: classify_changes + if: needs.classify_changes.outputs.run_expensive == 'true' + runs-on: ubuntu-latest + timeout-minutes: 15 + env: + DATABASE_URL: postgresql://postgres:postgres@127.0.0.1:5432/ims_ci + services: + postgres: + image: postgres:14.13 + env: + POSTGRES_USER: postgres + POSTGRES_PASSWORD: postgres + POSTGRES_DB: ims_ci + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U postgres -d ims_ci" + --health-interval 10s + --health-timeout 5s + --health-retries 5 + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: 22 + cache: npm + - run: npm ci + - run: npx prisma generate --schema prisma/schema.prisma + - run: npx prisma migrate deploy --schema prisma/schema.prisma + - run: npm run invariant-check:preflight:fixture + - run: npm run invariant-check:preflight + build: needs: classify_changes if: needs.classify_changes.outputs.run_expensive == 'true' diff --git a/docs/development.md b/docs/development.md index 88455648..a2860740 100644 --- a/docs/development.md +++ b/docs/development.md @@ -56,6 +56,21 @@ Focused tests can also be run directly: npx tsx --test tests/.test.ts ``` +## Invariant Preflight + +Production-readiness CI runs the invariant reporters against a freshly migrated database as a code-correctness gate, not as tenant-data validation. The workflow first runs `npm run invariant-check:preflight:fixture`, which seeds a known reserved-source mismatch, asserts the preflight fails, removes the fixture rows, and asserts the clean database passes. It then runs `npm run invariant-check:preflight` against the clean migrated database. + +The preflight command uses the same inventory, accounting, and sales invariant reporters as the scheduled cron but disables activity-log writes, admin notifications, and stored critical-finding hashes. It fails the build when any report fails or when any critical invariant finding is present. Tenant data still needs scheduled cron/operator runs against the tenant database, because CI's clean database cannot prove production data is healthy. + +Warnings and info findings do not block deploy, but they should be reviewed before merge when they relate to financial, inventory, or sales-order state. + +Remediation path: + +1. Run `npm run invariant-check:preflight` locally against the target database, or trigger `/api/cron/invariant-check` with `CRON_SECRET` in an environment where activity logs and notifications are expected. +2. Use the printed `domain:code` and entity references to inspect the matching invariant reporter details. +3. Repair the underlying data or writer bug. Do not suppress the finding unless the invariant contract itself is wrong and the PR updates the invariant plus its regression tests. +4. Rerun the preflight command before merging. + This repository treats `prisma/schema.prisma` as the canonical application schema. Migrations, deployment scripts, and CI all assume the Prisma schema and the live database describe the same shape unless a difference is intentionally documented as unsupported by Prisma. ## Decimal Boundary Guard diff --git a/docs/plan.md b/docs/plan.md index 3076dc5f..7cec09c1 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -467,14 +467,93 @@ These are silent-corruption risks where the failure mode is "the numbers are wro --- +## Phase 8 — Product lifecycle controls + +### P8.1 — Product lifecycle statuses and end-of-life sell-off +- **Status:** Planned. +- **Files:** `prisma/schema.prisma`, product create/edit actions and pages, reorder forecast/report modules, product list filters, inventory/purchasing incoming-stock helpers, lifecycle cron/job. +- **Problem:** Product lifecycle state is not expressive enough for sell-off workflows. Operators need to mark products as end-of-life (EOL): still sellable while stock remains, but blocked from re-ordering and excluded from replenishment forecasts. Once stock reaches zero across all warehouses and no incoming stock remains, EOL products should automatically become archived. +- **Product statuses:** + - `active`: published/sellable and can be re-ordered. + - `draft`: generated or being prepared, not published for sale yet, but can already be purchased. + - `eol`: can still be sold from existing stock, cannot be re-ordered, and must not appear in reorder forecasts. + - `archived`: withdrawn from sales and cannot be re-ordered. +- **Migration from the current enum:** `Product.lifecycleStatus` already uses `ProductLifecycleStatus { ACTIVE, NOT_FOR_SALE, ARCHIVED }`. Migrate existing rows as: + - `ACTIVE` -> `active`. + - `NOT_FOR_SALE` -> `eol` by default, because it is the safest sell-off interpretation for existing stock. Operators can reclassify individual products to `draft` during the rollout if they were genuinely pre-publication products rather than sell-off products. + - `ARCHIVED` -> `archived`. + - `draft` is a new IMS lifecycle concept for purchasable, not-yet-sellable products. It does not replace channel publication state such as WooCommerce draft/private/published status; shopping connector publication remains a separate channel concern. +- **Allowed lifecycle transitions:** + - `draft` -> `active` when a product is ready to sell. + - `active` -> `eol` to start sell-off without replenishment. + - `active` -> `archived` for immediate withdrawal. + - `eol` -> `active` to undo EOL. + - `eol` -> `archived` manually or through the auto-archive job. + - `archived` -> `active` or `archived` -> `eol` only through an admin restore action with an activity-log reason. `archived` -> `draft` is not allowed; clone/recreate instead. +- **Fix:** + 1. Model product lifecycle state explicitly, either by extending the existing product status enum or by replacing the old status/archived marker with a single status field that represents `active`, `draft`, `eol`, and `archived`. + 2. Add an EOL/status control on the product page and product edit form, with list/report filters for EOL and archived products. + 3. Treat `eol` products as sellable wherever available stock exists, but block purchase/reorder creation and replenishment suggestions for them. + 4. Exclude `eol` and `archived` products from reorder forecasts and reorder recommendation reports. + 5. Add a single lifecycle helper used by product actions, purchase-order actions, sales-order actions, allocation/shipment flows, reorder reports, shopping/WMS sync filters, and imports so `active`, `draft`, `eol`, and `archived` are interpreted consistently. Replace current ad-hoc `ProductLifecycleStatus`, `lifecycleStatus`, `ACTIVE`, `NOT_FOR_SALE`, `ARCHIVED`, `active`, and `isOperationalProductStatus` checks with that helper. + 6. Add a `getProductIncomingStock(productId)` helper that returns a structured breakdown by source. The minimum source contract is: + - Open purchase orders: `purchase_order_lines` where the parent `purchase_orders.status` is `PO_SENT`, `SHIPPED`, or `PARTIALLY_RECEIVED`, and `qty - qtyReceived > 0`. + - Inbound stock transfers: `stock_transfer_lines` where the parent `stock_transfers.status` is `IN_TRANSIT`, `productId` matches, and `qty - qtyReceived > 0`. + - Manufacturing output: `production_orders` where `outputProductId` matches, status is `DRAFT` or `IN_PROGRESS`, and `qtyPlanned - qtyProduced > 0`. + - WMS inbound evidence: `wms_asn_line_maps` whose parent `wms_asn_maps.status` is `CREATE_PENDING`, `CREATE_IN_FLIGHT`, `OPEN`, or `PARTIALLY_BOOKED_IN`, using `expectedQty - qtyAccountedViaSnapshot - qtyAccountedViaReceipt > 0`, plus pending `wms_inbound_receipt_events` for matching ASN/product evidence where available. + - Customer returns do not count as incoming stock unless they create explicit inbound receipt/restock evidence; speculative returns must not block EOL archival. + 7. Add an automatic archival job: when an EOL product has zero stock in every warehouse and `getProductIncomingStock(productId)` returns zero for every source, transition it to `archived` and write an activity-log entry. Run daily after inventory/WMS receipt processing. The job must lock the product row or take a product-scoped advisory lock, re-read stock levels and incoming-stock breakdown inside the transaction, and update only if the product is still `eol`. PO/reorder creation must also re-check lifecycle after locking the product so a concurrent inbound order cannot race the archive transition. +- **Acceptance:** + - Operators can filter product lists/reports by `active`, `draft`, `eol`, and `archived`. + - `active` products can be sold and re-ordered. + - `draft` products can be purchased but are not published/sellable. + - `eol` products remain sellable from existing stock, cannot be re-ordered, and do not appear in reorder forecasts. + - `archived` products cannot be sold or re-ordered. + - The archive job only archives EOL products after all warehouse stock is zero and incoming supply is zero. + - Lifecycle transitions are activity-logged with actor/job context and previous/new status. + - Existing `NOT_FOR_SALE` products are migrated to `eol` by default, with an operator-visible rollout note for reclassifying true drafts. +- **Tests:** + - Product action/unit tests for each allowed and blocked transition. + - Reorder forecast tests asserting EOL and archived products are excluded while active products remain included. + - Purchase/reorder creation tests asserting EOL and archived products are blocked, while draft products can be purchased. + - Sales/allocation tests asserting EOL products can be sold from available stock but archived and draft products cannot be sold. + - Auto-archive job tests for zero-stock/no-incoming, positive-stock, open PO incoming, inbound transfer incoming, production-order incoming, WMS ASN/receipt incoming, and a race case where a new PO line for the product is created between the initial read and the transaction re-check. + +### P8.2 — Product supplier field and supplier-scoped reorder drafts +- **Status:** Planned. +- **Files:** `prisma/schema.prisma`, product create/edit actions and pages, product list filters, purchase-order create/update actions, reorder forecast/report modules, draft purchase-order generation workflow. +- **Problem:** Reorder planning has no product-level supplier signal. Operators need to filter products by supplier and turn reorder forecasts into supplier-specific draft purchase orders. The product supplier should reflect the supplier used for the most recent purchase order placed for that product, so the replenishment workflow follows real buying history instead of stale manual metadata. +- **Fix:** + 1. Add a supplier field to products, backed by a relation to the supplier/vendor model used by purchase orders. + 2. Update the product supplier automatically when a purchase order is placed for that product. If a PO contains multiple products, update each included product to that PO's supplier. If a product appears on multiple POs, the latest placed PO wins. + 3. Surface supplier on product create/edit/detail screens and product exports/reports where SKU/EAN/MPN/product identity fields are shown. + 4. Add supplier filters to product lists and relevant inventory/replenishment reports. + 5. Add a supplier-scoped draft-PO generator from reorder forecasts. It should let operators choose a supplier, include only reorder-eligible products assigned to that supplier, group lines into a draft purchase order for that supplier, and preserve forecast evidence on each line. + 6. Respect lifecycle constraints from P8.1: `eol` and `archived` products must not be included in generated reorder drafts; `draft` and `active` products may be included when otherwise reorder-eligible. +- **Acceptance:** + - Products expose a supplier field in the UI, API/action payloads, and relevant reports. + - Placing a purchase order updates every included product's supplier to the PO supplier. + - Product list/report filters can narrow results by supplier. + - Reorder forecasts can generate a draft PO for one supplier, containing only products assigned to that supplier and only products that are reorder-eligible. + - Generated draft PO lines carry enough forecast metadata to explain the suggested quantity. + - EOL and archived products are excluded from supplier draft-PO generation even if the forecast would otherwise suggest a reorder. +- **Tests:** + - Product action/unit tests for setting and filtering by supplier. + - Purchase-order tests asserting the latest placed PO supplier updates product supplier for all included products. + - Reorder forecast tests asserting supplier filters and lifecycle exclusions. + - Draft-PO generation tests for one supplier, mixed-supplier forecast candidates, EOL/archive exclusions, and forecast-evidence metadata. + +--- + ## Quality gates — tests + invariants These are not a separate implementation phase except for the invariant CI gate. They are rules that apply to every PR in the plan. ### QG1 — Inventory invariant check blocks deploy +- **Status:** Complete. - **File:** `app/api/cron/invariant-check/route.ts`, CI pipeline -- **Fix:** Add `npm run invariant-check:preflight` step to CI. Fail build on critical findings. Document remediation path. -- **Tests:** Synthetic data with a known invariant violation; assert CI fails. +- **Fix:** `npm run invariant-check:preflight` runs the scheduled inventory, accounting, and sales invariant reporters with cron side effects disabled. Production-readiness CI applies migrations to a clean PostgreSQL service, runs `npm run invariant-check:preflight:fixture` to seed a known reserved-source mismatch and prove the gate fails on real data, clears that fixture, and then runs the clean preflight. This is a code-correctness gate; scheduled cron/operator runs remain responsible for tenant-data invariant checks. The remediation path is documented in `docs/development.md`. +- **Tests:** `tests/cron/invariant-check-preflight.test.ts` covers clean reports, critical findings, divergent critical summary/list evidence, and partial report failures. `tests/scripts/invariant-check-preflight-script.test.ts` covers CLI exit codes, report-error output, critical-finding output, and truncation. ### QG2 — Per-phase regression test requirement - For each fix in Phases 1–6, the regression test must exist and pass before the PR can merge. No exceptions. @@ -577,6 +656,10 @@ This reduces the plan from 45+ tiny PRs to roughly 16-20 coherent PRs. Split any 19. **Sidebar cleanup:** P2.3, CR5. - [x] P2.3 / CR5 — Sidebar analytics links are built from a single report-access grouping helper with per-role coverage. 20. **CI invariant gate:** QG1 plus QG2's regression-test convention. + - [x] QG1 — Production-readiness CI runs `npm run invariant-check:preflight` against a migrated database and blocks on critical findings/report failures. +21. **Product lifecycle, supplier, and reorder planning:** P8.1 and P8.2. + - [ ] P8.1 — Add `active`, `draft`, `eol`, and `archived` product statuses; block EOL reordering and forecasts; auto-archive EOL products once stock and incoming supply are exhausted. + - [ ] P8.2 — Add product supplier tracking from latest placed PO, supplier filters, and supplier-scoped draft PO generation from reorder forecasts. After each PR: - Run `npm run validate` and `npm run validate:db`. diff --git a/lib/cron/invariant-check-preflight.ts b/lib/cron/invariant-check-preflight.ts new file mode 100644 index 00000000..81225730 --- /dev/null +++ b/lib/cron/invariant-check-preflight.ts @@ -0,0 +1,34 @@ +import { + runScheduledInvariantCheck, + type ScheduledInvariantCheckResult, +} from '@/lib/cron/invariant-check' + +export type InvariantCheckPreflightFailure = + | 'report_failed' + | 'critical_findings' + +export type InvariantCheckPreflightResult = { + ok: boolean + failure: InvariantCheckPreflightFailure | null + result: ScheduledInvariantCheckResult +} + +export async function runInvariantCheckPreflight( + runCheck: () => Promise = () => runScheduledInvariantCheck({ + createRunId: () => `preflight-${new Date().toISOString()}`, + writeActivityLog: async () => {}, + notifyAdmins: async () => {}, + getPreviousCriticalFindingsHash: async () => null, + setCriticalFindingsHash: async () => {}, + }), +): Promise { + const result = await runCheck() + if (result.status !== 'completed' || result.errors.length > 0) { + return { ok: false, failure: 'report_failed', result } + } + if (result.summary.total.critical > 0 || result.criticalFindings.length > 0) { + return { ok: false, failure: 'critical_findings', result } + } + return { ok: true, failure: null, result } +} + diff --git a/lib/domain/inventory/invariants.ts b/lib/domain/inventory/invariants.ts index ee7a4718..34f26d55 100644 --- a/lib/domain/inventory/invariants.ts +++ b/lib/domain/inventory/invariants.ts @@ -960,6 +960,10 @@ function sqlOptionalStockMovementLookbackFilter(days: number | null | undefined) return Prisma.sql`AND sm."createdAt" >= NOW() - (${normalizedDays}::int * INTERVAL '1 day')` } +function sqlNumeric(value: number): Prisma.Sql { + return Prisma.sql`${value}::numeric` +} + function sqlOptionalAllocationProductFilter(productId: string | undefined): Prisma.Sql { return productId ? Prisma.sql`AND oa."productId" = ${productId}` : Prisma.empty } @@ -1000,6 +1004,11 @@ function buildSqlInventoryInvariantQuery(options: Required ${tolerance} + HAVING SUM(GREATEST(oa.qty - COALESCE(asl.qty, 0), 0)) > ${toleranceSql} UNION ALL @@ -1099,7 +1108,7 @@ function buildSqlInventoryInvariantQuery(options: Required ${tolerance} + HAVING SUM(po."qtyPlanned" * pc.qty) > ${toleranceSql} UNION ALL @@ -1113,7 +1122,7 @@ function buildSqlInventoryInvariantQuery(options: Required ${tolerance} + HAVING SUM(po."qtyPlanned") > ${toleranceSql} ), reservation_totals AS ( SELECT @@ -1139,7 +1148,7 @@ function buildSqlInventoryInvariantQuery(options: Required ${tolerance} + AND sl."reservedQty" - sl.quantity > ${toleranceSql} ${stockProductFilter} ${stockWarehouseFilter} @@ -1212,7 +1221,7 @@ function buildSqlInventoryInvariantQuery(options: Required ${tolerance} + WHERE ABS(COALESCE(sl."reservedQty", 0) - COALESCE(rt."knownReservedQty", 0)) > ${toleranceSql} ${reservationProductFilter} ${reservationWarehouseFilter} @@ -1254,7 +1263,7 @@ function buildSqlInventoryInvariantQuery(options: Required ${tolerance} + WHERE cl."remainingQty" - cl."receivedQty" > ${toleranceSql} ${costLayerProductFilter} ${costLayerWarehouseFilter} @@ -1301,7 +1310,7 @@ function buildSqlInventoryInvariantQuery(options: Required ${tolerance} + WHERE ABS(fsl.quantity - COALESCE(clt."remainingCostLayerQty", 0)) > ${toleranceSql} UNION ALL @@ -1325,7 +1334,7 @@ function buildSqlInventoryInvariantQuery(options: Required ${tolerance} + AND ABS(clt."remainingCostLayerQty") > ${toleranceSql} UNION ALL @@ -1401,8 +1410,8 @@ function buildSqlInventoryInvariantQuery(options: Required 0 @@ -1410,19 +1419,19 @@ function buildSqlInventoryInvariantQuery(options: Required CASE - WHEN ABS(ABS(sm.qty) * sm."unitCostBase") < 1 THEN ${STOCK_MOVEMENT_VALUE_SMALL_TOLERANCE} - ELSE ${STOCK_MOVEMENT_VALUE_TOLERANCE} + WHEN ABS(ABS(sm.qty) * sm."unitCostBase") < 1 THEN ${stockMovementValueSmallToleranceSql} + ELSE ${stockMovementValueToleranceSql} END AND CASE WHEN ABS(ABS(sm.qty) * sm."unitCostBase") = 0 THEN TRUE - ELSE ABS(sm."totalValueBase" - (ABS(sm.qty) * sm."unitCostBase")) / ABS(ABS(sm.qty) * sm."unitCostBase") > ${STOCK_MOVEMENT_VALUE_RELATIVE_TOLERANCE} + ELSE ABS(sm."totalValueBase" - (ABS(sm.qty) * sm."unitCostBase")) / ABS(ABS(sm.qty) * sm."unitCostBase") > ${stockMovementValueRelativeToleranceSql} END ${movementProductFilter} ${movementPrimaryWarehouseFilter} @@ -1477,14 +1486,14 @@ function buildSqlInventoryInvariantQuery(options: Required ${tolerance} + AND sm.qty > ${toleranceSql} AND sm."toWarehouseId" IS NOT NULL AND NOT EXISTS ( SELECT 1 FROM "cost_layers" cl WHERE cl."productId" = sm."productId" AND cl."warehouseId" = sm."toWarehouseId" - AND ABS(cl."receivedQty" - sm.qty) <= ${tolerance} + AND ABS(cl."receivedQty" - sm.qty) <= ${toleranceSql} AND ( (sm.type = 'PRODUCTION_IN' AND sm."referenceType" = 'ProductionOrder' @@ -1535,7 +1544,7 @@ function buildSqlInventoryInvariantQuery(options: Required ${tolerance} + AND sm.qty > ${toleranceSql} AND NOT EXISTS ( SELECT 1 FROM "cogs_entries" ce @@ -1569,7 +1578,7 @@ function buildSqlInventoryInvariantQuery(options: Required 'REFUNDED' AND p.type::text IN (${sqlFifoProductTypes()}) - AND sl.qty > ${tolerance} + AND sl.qty > ${toleranceSql} AND CASE WHEN sl."costLayerSnapshot" IS NULL THEN true WHEN jsonb_typeof(sl."costLayerSnapshot") <> 'array' THEN true diff --git a/package.json b/package.json index c88f94e2..085382b6 100644 --- a/package.json +++ b/package.json @@ -20,6 +20,8 @@ "docs:workflows": "tsx scripts/generate-workflow-docs.ts", "docs:workflows:check": "tsx scripts/generate-workflow-docs.ts --check", "preflight:production": "tsx scripts/preflight-production.ts", + "invariant-check:preflight": "tsx scripts/invariant-check-preflight.ts", + "invariant-check:preflight:fixture": "tsx scripts/invariant-check-preflight-fixture.ts", "inventory:snapshots:backfill": "tsx scripts/backfill-inventory-snapshots.ts", "validate": "bash scripts/validate-local.sh", "validate:db": "bash scripts/validate-db.sh", diff --git a/scripts/invariant-check-preflight-fixture.ts b/scripts/invariant-check-preflight-fixture.ts new file mode 100644 index 00000000..ce0b72cd --- /dev/null +++ b/scripts/invariant-check-preflight-fixture.ts @@ -0,0 +1,146 @@ +#!/usr/bin/env tsx + +import { config as loadDotenv } from 'dotenv' +import { Pool } from 'pg' + +import { runInvariantCheckPreflight } from '@/lib/cron/invariant-check-preflight' +import type { InvariantCheckPreflightResult } from '@/lib/cron/invariant-check-preflight' +import { runInvariantCheckPreflightCli } from './invariant-check-preflight.ts' + +loadDotenv({ path: '.env.local', override: false, quiet: true }) +loadDotenv({ path: '.env', override: false, quiet: true }) + +const DATABASE_URL = process.env.DATABASE_URL + +if (!DATABASE_URL) { + throw new Error('DATABASE_URL is required for the invariant preflight fixture') +} + +const pool = new Pool({ connectionString: DATABASE_URL }) +const fixtureId = `invariant-preflight-fixture-${Date.now()}` +const productId = `${fixtureId}-product` +const warehouseId = `${fixtureId}-warehouse` +const stockLevelId = `${fixtureId}-stock-level` +const warehouseCode = `IPF${Date.now().toString(36).slice(-5).toUpperCase()}` + +async function cleanupFixture(): Promise { + await pool.query('DELETE FROM "stock_levels" WHERE id = $1', [stockLevelId]) + await pool.query('DELETE FROM "warehouses" WHERE id = $1', [warehouseId]) + await pool.query('DELETE FROM "products" WHERE id = $1', [productId]) +} + +async function seedReservedSourceMismatch(): Promise { + await cleanupFixture() + await pool.query('BEGIN') + try { + await pool.query( + ` + INSERT INTO "products" ( + id, + sku, + name, + type, + "lifecycleStatus", + "oversellAllowed", + active, + "createdAt", + "updatedAt" + ) + VALUES ($1, $2, $3, 'SIMPLE', 'ACTIVE', false, true, NOW(), NOW()) + `, + [productId, fixtureId, 'Invariant preflight fixture product'], + ) + await pool.query( + ` + INSERT INTO "warehouses" ( + id, + code, + name, + type, + country, + active, + "createdAt", + "updatedAt" + ) + VALUES ($1, $2, $3, 'STANDARD', 'GB', true, NOW(), NOW()) + `, + [warehouseId, warehouseCode, 'Invariant preflight fixture warehouse'], + ) + await pool.query( + ` + INSERT INTO "stock_levels" ( + id, + "productId", + "warehouseId", + quantity, + "reservedQty", + "updatedAt" + ) + VALUES ($1, $2, $3, 2, 1, NOW()) + `, + [stockLevelId, productId, warehouseId], + ) + await pool.query('COMMIT') + } catch (error) { + await pool.query('ROLLBACK') + throw error + } +} + +async function runCliWithCapturedPreflight(): Promise<{ + exitCode: number + preflight: InvariantCheckPreflightResult +}> { + let capturedPreflight: InvariantCheckPreflightResult | undefined + const exitCode = await runInvariantCheckPreflightCli({ + runPreflight: async () => { + capturedPreflight = await runInvariantCheckPreflight() + return capturedPreflight + }, + }) + if (!capturedPreflight) { + throw new Error('Invariant preflight did not return a result') + } + return { exitCode, preflight: capturedPreflight } +} + +async function main(): Promise { + try { + await seedReservedSourceMismatch() + + const { + exitCode: failingExitCode, + preflight: seededPreflight, + } = await runCliWithCapturedPreflight() + if (failingExitCode !== 1 || seededPreflight?.failure !== 'critical_findings') { + throw new Error('Expected invariant preflight to fail with critical findings from the fixture') + } + const seededFindingCodes = seededPreflight.result.criticalFindings.map((finding) => finding.code) + if (!seededFindingCodes.includes('stock_reserved_source_mismatch')) { + throw new Error( + `Expected stock_reserved_source_mismatch from the fixture; got ${seededFindingCodes.join(', ') || 'none'}`, + ) + } + + await cleanupFixture() + + const { + exitCode: passingExitCode, + preflight: cleanPreflight, + } = await runCliWithCapturedPreflight() + if (passingExitCode !== 0) { + const failure = cleanPreflight.failure ?? 'unknown' + throw new Error(`Expected invariant preflight to pass after removing the fixture data; got ${failure}`) + } + + console.log('Invariant preflight fixture passed: seeded violation failed, cleanup pass succeeded.') + } finally { + await cleanupFixture().catch(() => {}) + await pool.end() + } +} + +void main().catch((error: unknown) => { + console.error(error instanceof Error ? error.message : String(error)) + process.exit(1) +}) diff --git a/scripts/invariant-check-preflight.ts b/scripts/invariant-check-preflight.ts new file mode 100644 index 00000000..462d86e6 --- /dev/null +++ b/scripts/invariant-check-preflight.ts @@ -0,0 +1,114 @@ +import path from 'node:path' +import { pathToFileURL } from 'node:url' + +import { runInvariantCheckPreflight } from '@/lib/cron/invariant-check-preflight' +import type { InvariantCheckPreflightResult } from '@/lib/cron/invariant-check-preflight' +import type { ScheduledInvariantCriticalFinding } from '@/lib/cron/invariant-check' + +const MAX_PRINTED_CRITICAL_FINDINGS = 20 +const DISCONNECT_TIMEOUT_MS = 5_000 + +type Logger = Pick + +export type InvariantCheckPreflightCliOptions = { + runPreflight?: () => Promise + stdout?: Pick + stderr?: Pick + disconnect?: () => Promise +} + +async function disconnectDb(): Promise { + const { db } = await import('@/lib/db') + let timeout: ReturnType | null = null + await Promise.race([ + db.$disconnect(), + new Promise((resolve) => { + timeout = setTimeout(resolve, DISCONNECT_TIMEOUT_MS) + timeout.unref() + }), + ]) + if (timeout) clearTimeout(timeout) +} + +function findingReference(finding: ScheduledInvariantCriticalFinding): string { + return [ + finding.productId ? `product=${finding.productId}` : null, + finding.warehouseId ? `warehouse=${finding.warehouseId}` : null, + finding.orderId ? `order=${finding.orderId}` : null, + finding.shipmentId ? `shipment=${finding.shipmentId}` : null, + finding.refundId ? `refund=${finding.refundId}` : null, + finding.syncLogId ? `syncLog=${finding.syncLogId}` : null, + ].filter(Boolean).join(', ') +} + +export function formatCriticalFinding(finding: ScheduledInvariantCriticalFinding): string { + const reference = findingReference(finding) + return `${finding.domain}:${finding.code}${reference ? ` (${reference})` : ''} - ${finding.message}` +} + +function printPreflightResult( + preflight: InvariantCheckPreflightResult, + stdout: Pick, + stderr: Pick, +): number { + const { result } = preflight + + stdout.log( + `Invariant preflight summary: status=${result.status}, total=${result.summary.total.total}, critical=${result.summary.total.critical}, warning=${result.summary.total.warning}, info=${result.summary.total.info}`, + ) + + if (preflight.ok) { + stdout.log('Invariant preflight passed: no critical findings.') + return 0 + } + + if (preflight.failure === 'report_failed') { + stderr.error('Invariant preflight failed: one or more invariant reports did not complete.') + for (const error of result.errors) { + stderr.error(`- ${error.domain}: ${error.message}`) + } + } else { + stderr.error('Invariant preflight failed: critical invariant findings exist.') + for (const finding of result.criticalFindings.slice(0, MAX_PRINTED_CRITICAL_FINDINGS)) { + stderr.error(`- ${formatCriticalFinding(finding)}`) + } + const omittedCount = result.criticalFindings.length - MAX_PRINTED_CRITICAL_FINDINGS + if (omittedCount > 0) { + stderr.error(`... ${omittedCount} more critical finding(s) omitted`) + } + } + + stderr.error( + 'Remediation: inspect the invariant finding code/details, repair the underlying data or writer bug, and rerun npm run invariant-check:preflight.', + ) + return 1 +} + +export async function runInvariantCheckPreflightCli({ + runPreflight = runInvariantCheckPreflight, + stdout = console, + stderr = console, + disconnect = disconnectDb, +}: InvariantCheckPreflightCliOptions = {}): Promise { + try { + return printPreflightResult(await runPreflight(), stdout, stderr) + } catch (error: unknown) { + const message = error instanceof Error && error.message ? error.message : String(error) + stderr.error(`Invariant preflight failed before report completion: ${message}`) + return 1 + } finally { + await disconnect() + } +} + +if (process.argv[1] && import.meta.url === pathToFileURL(path.resolve(process.argv[1])).href) { + void runInvariantCheckPreflightCli() + .then((exitCode) => { + process.exit(exitCode) + }) + .catch((error: unknown) => { + const message = error instanceof Error && error.message ? error.message : String(error) + console.error(`Invariant preflight failed before report completion: ${message}`) + process.exit(1) + }) +} diff --git a/tests/cron/invariant-check-preflight.test.ts b/tests/cron/invariant-check-preflight.test.ts new file mode 100644 index 00000000..22fbdb61 --- /dev/null +++ b/tests/cron/invariant-check-preflight.test.ts @@ -0,0 +1,113 @@ +import assert from 'node:assert/strict' +import test from 'node:test' + +import { runInvariantCheckPreflight } from '@/lib/cron/invariant-check-preflight' +import type { ScheduledInvariantCheckResult } from '@/lib/cron/invariant-check' + +function emptySummary() { + return { + total: 0, + info: 0, + warning: 0, + critical: 0, + } +} + +function scheduledResult( + overrides: Partial = {}, +): ScheduledInvariantCheckResult { + const summary = { + total: emptySummary(), + inventory: emptySummary(), + accounting: emptySummary(), + sales: emptySummary(), + ...overrides.summary, + } + + return { + runId: 'preflight-test', + checkedAt: '2026-06-10T00:00:00.000Z', + status: 'completed', + summary, + errors: [], + criticalFindings: [], + reports: { + inventory: null, + accounting: null, + sales: null, + }, + ...overrides, + } +} + +test('invariant preflight passes completed reports without critical findings', async () => { + const result = await runInvariantCheckPreflight(async () => scheduledResult({ + summary: { + total: { total: 2, info: 1, warning: 1, critical: 0 }, + inventory: { total: 1, info: 0, warning: 1, critical: 0 }, + accounting: emptySummary(), + sales: { total: 1, info: 1, warning: 0, critical: 0 }, + }, + })) + + assert.equal(result.ok, true) + assert.equal(result.failure, null) + assert.equal(result.result.summary.total.warning, 1) +}) + +test('invariant preflight fails when critical findings are present', async () => { + const result = await runInvariantCheckPreflight(async () => scheduledResult({ + summary: { + total: { total: 1, info: 0, warning: 0, critical: 1 }, + inventory: { total: 1, info: 0, warning: 0, critical: 1 }, + accounting: emptySummary(), + sales: emptySummary(), + }, + criticalFindings: [{ + domain: 'inventory', + code: 'stock_negative_quantity', + message: 'Stock quantity is negative', + details: { quantity: '-1' }, + productId: 'product-1', + warehouseId: 'warehouse-1', + }], + })) + + assert.equal(result.ok, false) + assert.equal(result.failure, 'critical_findings') + assert.equal(result.result.criticalFindings[0]?.code, 'stock_negative_quantity') +}) + +test('invariant preflight also fails when critical summary and finding list diverge', async () => { + const result = await runInvariantCheckPreflight(async () => scheduledResult({ + summary: { + total: { total: 1, info: 0, warning: 1, critical: 0 }, + inventory: { total: 1, info: 0, warning: 1, critical: 0 }, + accounting: emptySummary(), + sales: emptySummary(), + }, + criticalFindings: [{ + domain: 'accounting', + code: 'accounting_sync_failed_without_error', + message: 'Accounting sync failed without a visible error', + details: { syncLogId: 'sync-1' }, + syncLogId: 'sync-1', + }], + })) + + assert.equal(result.ok, false) + assert.equal(result.failure, 'critical_findings') +}) + +test('invariant preflight fails when any report fails', async () => { + const result = await runInvariantCheckPreflight(async () => scheduledResult({ + status: 'partial_failure', + errors: [{ domain: 'sales', message: 'refund reconciliation failed' }], + })) + + assert.equal(result.ok, false) + assert.equal(result.failure, 'report_failed') + assert.deepEqual(result.result.errors, [ + { domain: 'sales', message: 'refund reconciliation failed' }, + ]) +}) diff --git a/tests/domain/inventory/invariants.test.ts b/tests/domain/inventory/invariants.test.ts index e23b304a..3a7c7396 100644 --- a/tests/domain/inventory/invariants.test.ts +++ b/tests/domain/inventory/invariants.test.ts @@ -1279,7 +1279,10 @@ test('inventory SQL collector keeps partially refunded orders eligible for shipp const sql = String((capturedQuery as { sql?: string }).sql ?? '') assert.match(sql, /so\.status <> 'REFUNDED'/) assert.match(sql, /ABS\(sm\.qty\) \* sm\."unitCostBase"/) + assert.match(sql, /sl\."reservedQty" - sl\.quantity > \?::numeric/) + assert.match(sql, /sm\.qty > \?::numeric/) assert.match(sql, /relativeTolerance/) + assert.match(sql, /relativeTolerance', \?::numeric/) assert.doesNotMatch(sql, /PARTIALLY_REFUNDED/) }) diff --git a/tests/scripts/invariant-check-preflight-script.test.ts b/tests/scripts/invariant-check-preflight-script.test.ts new file mode 100644 index 00000000..31755067 --- /dev/null +++ b/tests/scripts/invariant-check-preflight-script.test.ts @@ -0,0 +1,202 @@ +import assert from 'node:assert/strict' +import test from 'node:test' + +import { + formatCriticalFinding, + runInvariantCheckPreflightCli, +} from '@/scripts/invariant-check-preflight' +import type { InvariantCheckPreflightResult } from '@/lib/cron/invariant-check-preflight' +import type { + ScheduledInvariantCheckResult, + ScheduledInvariantCriticalFinding, +} from '@/lib/cron/invariant-check' + +function emptySummary() { + return { + total: 0, + info: 0, + warning: 0, + critical: 0, + } +} + +function scheduledResult( + overrides: Partial = {}, +): ScheduledInvariantCheckResult { + const summary = { + total: emptySummary(), + inventory: emptySummary(), + accounting: emptySummary(), + sales: emptySummary(), + ...overrides.summary, + } + + return { + runId: 'preflight-script-test', + checkedAt: '2026-06-10T00:00:00.000Z', + status: 'completed', + summary, + errors: [], + criticalFindings: [], + reports: { + inventory: null, + accounting: null, + sales: null, + }, + ...overrides, + } +} + +function preflightResult( + overrides: Partial = {}, +): InvariantCheckPreflightResult { + const result = overrides.result ?? scheduledResult() + return { + ok: true, + failure: null, + result, + ...overrides, + } +} + +function captureLogs() { + const stdout: string[] = [] + const stderr: string[] = [] + return { + stdout, + stderr, + logger: { + stdout: { log: (message: string) => stdout.push(message) }, + stderr: { error: (message: string) => stderr.push(message) }, + }, + } +} + +function criticalFinding( + overrides: Partial = {}, +): ScheduledInvariantCriticalFinding { + return { + domain: 'inventory', + code: 'stock_reserved_source_mismatch', + message: 'Reserved quantity does not match known reservation sources', + details: { reservedQty: '1', knownReservedQty: '0' }, + productId: 'product-1', + warehouseId: 'warehouse-1', + ...overrides, + } +} + +test('formatCriticalFinding includes entity references', () => { + assert.equal( + formatCriticalFinding(criticalFinding()), + 'inventory:stock_reserved_source_mismatch (product=product-1, warehouse=warehouse-1) - Reserved quantity does not match known reservation sources', + ) +}) + +test('runInvariantCheckPreflightCli exits 0 and prints pass output on clean preflight', async () => { + const logs = captureLogs() + const exitCode = await runInvariantCheckPreflightCli({ + runPreflight: async () => preflightResult(), + stdout: logs.logger.stdout, + stderr: logs.logger.stderr, + disconnect: async () => {}, + }) + + assert.equal(exitCode, 0) + assert.match(logs.stdout.join('\n'), /Invariant preflight summary: status=completed/) + assert.match(logs.stdout.join('\n'), /Invariant preflight passed/) + assert.deepEqual(logs.stderr, []) +}) + +test('runInvariantCheckPreflightCli exits 1 and prints critical findings', async () => { + const finding = criticalFinding() + const logs = captureLogs() + const exitCode = await runInvariantCheckPreflightCli({ + runPreflight: async () => preflightResult({ + ok: false, + failure: 'critical_findings', + result: scheduledResult({ + summary: { + total: { total: 1, info: 0, warning: 0, critical: 1 }, + inventory: { total: 1, info: 0, warning: 0, critical: 1 }, + accounting: emptySummary(), + sales: emptySummary(), + }, + criticalFindings: [finding], + }), + }), + stdout: logs.logger.stdout, + stderr: logs.logger.stderr, + disconnect: async () => {}, + }) + + assert.equal(exitCode, 1) + assert.match(logs.stderr.join('\n'), /critical invariant findings exist/) + assert.match(logs.stderr.join('\n'), /stock_reserved_source_mismatch/) + assert.match(logs.stderr.join('\n'), /Remediation:/) +}) + +test('runInvariantCheckPreflightCli exits 1 and prints report errors', async () => { + const logs = captureLogs() + const exitCode = await runInvariantCheckPreflightCli({ + runPreflight: async () => preflightResult({ + ok: false, + failure: 'report_failed', + result: scheduledResult({ + status: 'partial_failure', + errors: [{ domain: 'sales', message: 'sales invariant failed' }], + }), + }), + stdout: logs.logger.stdout, + stderr: logs.logger.stderr, + disconnect: async () => {}, + }) + + assert.equal(exitCode, 1) + assert.match(logs.stderr.join('\n'), /one or more invariant reports did not complete/) + assert.match(logs.stderr.join('\n'), /sales: sales invariant failed/) +}) + +test('runInvariantCheckPreflightCli truncates long critical-finding output', async () => { + const findings = Array.from({ length: 22 }, (_, index) => criticalFinding({ + productId: `product-${index + 1}`, + })) + const logs = captureLogs() + const exitCode = await runInvariantCheckPreflightCli({ + runPreflight: async () => preflightResult({ + ok: false, + failure: 'critical_findings', + result: scheduledResult({ + summary: { + total: { total: 22, info: 0, warning: 0, critical: 22 }, + inventory: { total: 22, info: 0, warning: 0, critical: 22 }, + accounting: emptySummary(), + sales: emptySummary(), + }, + criticalFindings: findings, + }), + }), + stdout: logs.logger.stdout, + stderr: logs.logger.stderr, + disconnect: async () => {}, + }) + + assert.equal(exitCode, 1) + assert.match(logs.stderr.join('\n'), /2 more critical finding\(s\) omitted/) + assert.doesNotMatch(logs.stderr.join('\n'), /product-21/) +}) + +test('runInvariantCheckPreflightCli exits 1 when preflight throws before report completion', async () => { + const logs = captureLogs() + const exitCode = await runInvariantCheckPreflightCli({ + runPreflight: async () => { + throw new Error('database unavailable') + }, + stdout: logs.logger.stdout, + stderr: logs.logger.stderr, + disconnect: async () => {}, + }) + + assert.equal(exitCode, 1) + assert.match(logs.stderr.join('\n'), /failed before report completion: database unavailable/) +})