core/php-commerce

Fork 0

[audit] Security, code quality, missing tests, error handling #18

New issue

Open

opened 2026-03-22 16:41:17 +00:00 by Virgil · 6 comments

Virgil commented

2026-03-22 16:41:17 +00:00

Member

Full audit:

Security: SQL injection, XSS, CSRF bypass, mass assignment, path traversal, insecure deserialization
Code quality: missing strict_types, missing type hints, missing return types
Missing tests: modules/actions without Pest test coverage
Error handling: swallowed exceptions, bare try/catch, missing validation
UK English: American spellings (color→colour, organization→organisation, center→centre)
Coding standards: PSR-12 compliance, Action pattern usage, BelongsToWorkspace scoping
Missing SPDX licence headers (EUPL-1.2)

Report all findings with severity and file:line. Do NOT fix.

Full audit: 1. Security: SQL injection, XSS, CSRF bypass, mass assignment, path traversal, insecure deserialization 2. Code quality: missing strict_types, missing type hints, missing return types 3. Missing tests: modules/actions without Pest test coverage 4. Error handling: swallowed exceptions, bare try/catch, missing validation 5. UK English: American spellings (color→colour, organization→organisation, center→centre) 6. Coding standards: PSR-12 compliance, Action pattern usage, BelongsToWorkspace scoping 7. Missing SPDX licence headers (EUPL-1.2) Report all findings with severity and file:line. Do NOT fix.

Virgil commented

2026-03-22 19:37:37 +00:00

Author

Member

Codex Audit Findings

CRITICAL (2)

Webhook deduplication uses object IDs instead of gateway event IDs — later legitimate lifecycle events skipped as duplicates (StripeGateway.php:557, BTCPayGateway.php:390)
Catalogue/warehouse/inventory models don't match shipped schema — subsystem not deployable from clean database (create_commerce_tables.php:101, Product.php:99, Warehouse.php:54, Inventory.php:32, InventoryMovement.php:33, ContentOverride.php:45)

HIGH (1)

Stripe checkout dereferences missing Order::workspace relation, missing OrderItem::name field, treats onetime items as recurring subscriptions (StripeGateway.php)

## Codex Audit Findings ### CRITICAL (2) 1. Webhook deduplication uses object IDs instead of gateway event IDs — later legitimate lifecycle events skipped as duplicates (StripeGateway.php:557, BTCPayGateway.php:390) 2. Catalogue/warehouse/inventory models don't match shipped schema — subsystem not deployable from clean database (create_commerce_tables.php:101, Product.php:99, Warehouse.php:54, Inventory.php:32, InventoryMovement.php:33, ContentOverride.php:45) ### HIGH (1) 3. Stripe checkout dereferences missing Order::workspace relation, missing OrderItem::name field, treats onetime items as recurring subscriptions (StripeGateway.php)

Virgil commented

2026-03-23 06:49:16 +00:00

Author

Member

Fix Applied

Commit 1086fdc: fix(commerce): address issue 18 critical regressions

Webhook dedup now uses gateway event IDs (Stripe + BTCPay)
Commerce migration schema fixed — catalogue/warehouse/inventory tables aligned
Stripe checkout: Order::workspace relation + OrderItem::name resolved
142-line Issue18RegressionTest added
585 additions across 13 files

## Fix Applied Commit 1086fdc: fix(commerce): address issue 18 critical regressions - Webhook dedup now uses gateway event IDs (Stripe + BTCPay) - Commerce migration schema fixed — catalogue/warehouse/inventory tables aligned - Stripe checkout: Order::workspace relation + OrderItem::name resolved - 142-line Issue18RegressionTest added - 585 additions across 13 files

Virgil commented

2026-03-23 08:56:27 +00:00

Author

Member

Verification: FAIL

HIGH: Schema repair rewrites shipped bootstrap migration instead of adding forward migration — breaks existing installs
MEDIUM: Field naming misleading — gateway_session_id persists into gateway_order_id, name persists into description
Tests couldn't run — composer autoload fatals on missing helpers.php

Needs: forward migration for schema changes, field name alignment.

## Verification: FAIL 1. HIGH: Schema repair rewrites shipped bootstrap migration instead of adding forward migration — breaks existing installs 2. MEDIUM: Field naming misleading — gateway_session_id persists into gateway_order_id, name persists into description 3. Tests couldn't run — composer autoload fatals on missing helpers.php Needs: forward migration for schema changes, field name alignment.

Virgil commented

2026-03-23 09:36:07 +00:00

Author

Member

Fix Round 2

Commit 2fe80a4: fix(commerce): add forward schema repair migration

Reverted bootstrap migration changes
Added 310-line forward migration (repair_catalogue_schema_forward.php)
Field names aligned (gateway_session_id → gateway_order_id etc)
441 additions across 15 files
Dispatching verification.

## Fix Round 2 Commit 2fe80a4: fix(commerce): add forward schema repair migration - Reverted bootstrap migration changes - Added 310-line forward migration (repair_catalogue_schema_forward.php) - Field names aligned (gateway_session_id → gateway_order_id etc) - 441 additions across 15 files Dispatching verification.

Virgil commented

2026-03-23 09:45:19 +00:00

Author

Member

Verification Round 2: FAIL (medium — API contract)

plan_name in billing API now returns item description (UI copy like 'Pro Plan - Monthly') instead of stable plan name. API contract changed from stable identifier to presentation text.

CommerceController.php:464 reads items->first()->description, CommerceService.php:156 writes UI copy into it.

Round 2, medium finding. Forward migration is correct. Escalating to needs-human for API contract decision.

## Verification Round 2: FAIL (medium — API contract) plan_name in billing API now returns item description (UI copy like 'Pro Plan - Monthly') instead of stable plan name. API contract changed from stable identifier to presentation text. CommerceController.php:464 reads items->first()->description, CommerceService.php:156 writes UI copy into it. Round 2, medium finding. Forward migration is correct. Escalating to needs-human for API contract decision.

Virgil commented

2026-03-23 12:19:08 +00:00

Author

Member