psycodepath/hrp
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages 1 Wiki Activity Actions

chore/medium-low-hardening #29

Merged
psycodepath merged 10 commits from chore/medium-low-hardening into main 2026-07-08 19:26:38 +00:00
Conversation 0 Commits 10 Files changed 21 +537 -105
psycodepath commented 2026-07-08 19:25:27 +00:00
Owner
Copy link

Addresses the Medium findings from the codebase review. Stacked on
security/tenant-isolation, so review/merge that PR first (or retarget this
one to main after it merges).

  • X2: production 500s no longer leak the exception message/stack trace.
  • Kernel M4: settings locale change now persists (tenant session).
  • Kernel M2: Twig debug/cache environment-driven.
  • Kernel M5: dropped the migration .done marker that permanently skipped
    newly deployed migrations (relies on the idempotent migrations table).
  • Kernel M6: AI rate limiter is atomic (BEGIN IMMEDIATE) so caps can't be
    exceeded under concurrency.
  • Kernel M7: API keys support expiry + revocation (migration 021,
    --expires-in-days, api-key:revoke).
  • Grocery M1: indirect category cycles rejected on move.
  • Grocery M2: web add-item validates amount > 0 (was a 500).
  • Grocery M3: taxonomy worker requeues on Gemini failure instead of saving
    an unclassified product.
  • Meals M5/M7: bounded search input; importer prompt wraps untrusted HTML
    as data.

Verification: 171 tests pass; phpstan level 8 clean.

Addresses the Medium findings from the codebase review. Stacked on security/tenant-isolation, so review/merge that PR first (or retarget this one to main after it merges). - X2: production 500s no longer leak the exception message/stack trace. - Kernel M4: settings locale change now persists (tenant session). - Kernel M2: Twig debug/cache environment-driven. - Kernel M5: dropped the migration .done marker that permanently skipped newly deployed migrations (relies on the idempotent migrations table). - Kernel M6: AI rate limiter is atomic (BEGIN IMMEDIATE) so caps can't be exceeded under concurrency. - Kernel M7: API keys support expiry + revocation (migration 021, --expires-in-days, api-key:revoke). - Grocery M1: indirect category cycles rejected on move. - Grocery M2: web add-item validates amount > 0 (was a 500). - Grocery M3: taxonomy worker requeues on Gemini failure instead of saving an unclassified product. - Meals M5/M7: bounded search input; importer prompt wraps untrusted HTML as data. Verification: 171 tests pass; phpstan level 8 clean.
The front controller's catch-all rendered the exception message and full
stack trace into the 500 response, exposing file paths, SQL fragments,
tenant DB paths and possibly secrets in production.

Log the full detail server-side (via the PSR-3 logger, falling back to
error_log) and only render the message/trace to the client when
APP_ENV=dev; production gets a generic error page.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SettingsAction wrote $_SESSION['auth_user']['locale'] at the top level,
but authentication lives under $_SESSION['tenants'][<tenant>]['auth_user']
(TenantAwareSessionManager). The locale update therefore silently
no-op'd and bypassed the session abstraction.

Inject TenantAwareSessionManager and update the locale via
get/setTenantSessionValue('auth_user') so the change actually persists.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Twig was always built with debug=true and cache=false regardless of
environment, exposing internals via the dump/debug facilities and
recompiling every template on each request in production.

In dev keep debug on and caching off; in all other environments disable
debug and compile templates to var/cache/twig.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MoveCategoryHandler only blocked self-parenting (A under A); an indirect
cycle (A->B, then B->A) could be persisted by POSTing parent_id directly,
violating the tree invariant.

Walk the target parent's ancestor chain and reject the move if the
category being moved appears in it (with a safety valve against a
pre-existing corrupt cycle). Adds tests for indirect-cycle, self-parent,
and valid-move cases.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The web AddGroceryItemAction did not validate the amount, so a
non-positive value threw InvalidArgumentException in the Quantity value
object and surfaced as a 500. Mirror the API action and return a 400
instead, keeping web and API validation consistent.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TenantAwareMigrationRunner short-circuited on a touched
.migrations_<tenant>.done marker before checking for new migration files,
so once the marker existed a tenant never received newly deployed
migrations until it was manually deleted.

Remove the marker entirely and rely on MigrationRunner::up(), which is
idempotent via the `migrations` table (applies only unrun versions). The
flock still serialises concurrent runs and the in-memory cache still
avoids re-running within a single request. Tests now assert applied
versions in each tenant DB instead of marker files.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- M7: SearchRecipesAction now caps the query and category to 100 chars
  and floors page at 1, so a hostile request can't drive a huge LIKE
  pattern or pagination offset.
- M5: GeminiRecipeImporter wraps the untrusted page HTML in SOURCE_HTML
  markers and instructs the model to treat everything inside strictly as
  data and never follow embedded instructions, reducing prompt-injection
  influence over extracted fields (defense in depth alongside the fixed
  JSON-LD XSS sink).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
RateLimiter::checkAndRecord counted requests and then inserted in
separate statements, so two concurrent requests could both pass the
RPM/RPD check before either inserted, exceeding the caps on a paid API.

Wrap the check-then-record in a BEGIN IMMEDIATE transaction so callers
serialise on the write lock and each sees the others' inserts; a rejected
request rolls back and records nothing. Also quote the reserved `key`
identifier. Adds tests covering the limit, rejection-not-recorded, and
daily cap.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
API keys were valid forever with no way to revoke a leaked key.

- Migration 021 adds expires_at and revoked_at to api_keys.
- ApiKeyAuthenticationService rejects revoked and expired keys and now
  selects only the columns it needs.
- api-key:create gains an --expires-in-days option.
- New api-key:revoke command sets revoked_at by --id or --name within a
  tenant.

Tests cover expired, revoked, and unexpired keys.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fix(grocery): fail taxonomy classification instead of saving junk on Gemini error
All checks were successful
CI/CD Pipeline / test (pull_request) Successful in 48s
Details
CI/CD Pipeline / build-and-push (pull_request) Has been skipped
Details
feb66f289c 
When Gemini was enabled but the API call failed, TaxonomySuggestor
swallowed the error and saved an unclassified fallback product (no
category/shelf life), which was then permanently stored and never
re-classified — and the worker treated it as success and deleted the
pending row.

resolveOrSuggest now rethrows on a Gemini/rate-limit failure without
persisting anything, so the worker requeues the item and retries on a
later run. (The Gemini-disabled path still intentionally creates a plain
product.) Adds a test asserting a failure throws and saves nothing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
psycodepath/hrp!29
No description provided.