uwe.admin/oatpp-authkit

Fork 0

Code Audit Report — 2026-05-29 #16

New issue

Open

opened 2026-05-29 12:22:33 +02:00 by uwe.admin · 3 comments

uwe.admin commented

2026-05-29 12:22:33 +02:00

Owner

Code Audit Report — 2026-05-29

Automated audit of oatpp-authkit covering 23 audit categories (12 security + 11 code quality), run via the /code-audit skill. Each category was executed by an independent subagent reading the live source under include/oatpp-authkit/**.

Framing: oatpp-authkit is a reusable header-only C++ auth/security library consumed by webapps (e.g. fewo-webapp). It has no frontend, no controllers/DTOs of its own, and stores no PII itself. The dominant risk theme is insecure-by-default contracts: a fail-open default in a security library has fleet-wide blast radius across every consumer that inherits it. Frontend-specific audits (and audit 23/Orval) are N/A.

Executive Summary

Overall posture: MEDIUM, trending HIGH-risk-by-default for naive consumers. The library has genuinely strong defaults (strict CSP/HSTS/nosniff/frame headers, body-size + WS connection caps, leak-free error handler, no secret logging, fail-closed encryption-key gate, fully parameterized SQL). Its highest-impact weaknesses are fail-open trust boundaries and incomplete authorization decorators that silently shift risk onto consumers. No Critical in the default posture; the worst trust-boundary flaw (cert-DN spoofing) becomes effectively critical under the standard loopback-behind-proxy misdeployment.

Unique security findings by severity (deduplicated):

🔴 Critical: 0
🟠 High: 5
🟡 Medium: 11
🔵 Low: 8 (+ informational)

Immediate actions (top 3):

Flip the dangerous defaults to fail-closed — certAuthTrusted() → false, WS empty propertyIds → no access, validate RateLimiter ctor.
Close the ScopeGuard bypasses — wrap/refuse IQueryable::query(), and make save() check the existing row's scope, not just the incoming DTO.
Self-defend SMTP against CRLF/header injection in to/fromAddress.

Security Findings

1. Initial Security Analysis — risk 5.5/10

[Medium, CWE-330/252] Unchecked randomness paths and insecure-by-default trust decisions shift burden to consumers (cert-DN trust, setup mode, SMTP TLS).
[Low, CWE-352] CSRF for cookie-session mutations relies solely on X-Requested-With; cert path has no CSRF gate.
[Low, CWE-248] requireUser std::stoi on bundle id (auth/RequireRole.hpp:35) can throw → 500 instead of 401; AuthPrincipal::id is int while the user store migrated to TEXT/UUID.
[Low, CWE-116] AuditLog::escapeJson (db/AuditLog.hpp:89-98) escapes only \/".
Strong: parameterized SQL, validated loopback-gated XFF, body-size limits, WS caps, leak-free error handler.

[Medium, CWE-565/697] Cookie parser uses substring find("session=") (util/TokenExtract.hpp:23-28) — matches xsession=/notsession=/etc., takes first match → token confusion, defeats __Host- prefix guarantees. Fix: tokenize on ;, exact-name match.
[Medium, CWE-1004/614] Library ships no safe setSessionCookie helper (Secure/HttpOnly/SameSite); consumers hand-roll Set-Cookie and the baseline doc only recommends Secure.
[Medium, CWE-352] CSRF defense is single X-Requested-With check, no Origin/Referer validation, broken if consumer enables permissive CORS.
[Medium, CWE-290/348] X-SSL-Client-DN cert trust defaults to isLoopback() (see synthesis H-1).

3. Authentication Flow — risk 5.5/10

[Medium, CWE-307] Brute-force protection: RateLimiter exists but is NOT wired into AuthInterceptor — entirely consumer responsibility, undocumented.
[Medium, CWE-636] Setup-mode pseudo-admin (AuthInterceptor.hpp:226-231) fails open: any request → admin when setupModeActive() && !hasActiveUsers(); fail-open if hasActiveUsers() swallows a DB error to false; no loopback requirement.
[Low, CWE-208/916] TokenHasher is consumer-supplied with no enforced minimum strength; no constant-time compare helper shipped.
[Low, CWE-613] Session-expiry sweep is GC-only and runs only on request traffic; if a consumer relies on it (instead of resolve-time expiry), expired sessions stay valid.
Strong: IAuthBackend/IAuthPolicy/IRuntimeConfig strategy split is excellent; library never sees plaintext passwords; deny-by-default at the interceptor.

4. Input Validation — risk 5/10

[High, CWE-93/88] SMTP header/CRLF injection: to and fromAddress concatenated into headers + envelope with no CRLF/NUL validation (mail/SmtpTransport.hpp:84,87,132-134). Subject is base64-safe; to/from are not. A security library must self-defend.
[Low, CWE-150] LIKE patterns not escaped in the query DSL (repo/IQueryable.hpp:221-224) — wildcard metachars pass through (no SQLi, values bound; full-scan/match-semantics risk).
[Low, CWE-20] BodySizeLimitInterceptor Content-Length parse accepts +/leading-whitespace forms (theoretical parser-desync).
No request-data-reachable SQLi: all values parameterized; identifiers come from compile-time registration.

5. Authorization — risk 6.5/10 (highest)

[HIGH, CWE-639/863] ScopeGuardRepository IQueryable::query() bypass: the guard decorates only the 4 Repository<T> methods; IQueryable::query() (repo/IQueryable.hpp:314-319) is a separate surface it does NOT wrap → arbitrary filtered SELECTs bypass scope entirely (full cross-tenant read).
[HIGH, CWE-639/863] ScopeGuard save() checks only the incoming DTO, never the existing row: actor scoped to prop-A submits an update for an entity currently in prop-B with propertyId=prop-A in the body → overwrites/reparents the prop-B row. Fix: load existing row, require predicate on BOTH.
[HIGH, CWE-1188/636] Multiple fail-open defaults: (a) WS empty propertyIds = "all access" (ws/Hub.hpp:159-163); (b) cert trust loopback default; (c) BodySizeLimit requireContentLength=false escape hatch.
Interceptor itself is deny-by-default with readonly + CSRF enforcement; requireAdmin is a proper gate.

6. Database Security — risk 3.5/10

[Medium, CWE-662] Temporal close-then-insert is non-atomic (repo/TemporalRepository.hpp:196-207): two separate saves, no transaction → crash/error leaves duplicate-live or lost-version. Cascade soft-deletes likewise.
[Medium, CWE-212/522] RedactedFieldRepository fails open if decorators misordered: redaction fires only when stacked between Temporal and concrete; wrong order silently retains password_hash in history. Field matched by name only — silent if typo'd.
[Low, CWE-89 mitigated] ORDER BY / table / column identifiers concatenated but sourced from compile-time registration (not request-reachable). Document the invariant.
All values parameterized; no request-reachable SQLi.

7. Secrets Management — risk 1.5/10

[Low, CWE-532/312] AuditLog::logUpdate<Dto> can persist password_hash/tls_cert_dn into changed_fields (not in SKIP_FIELDS).
[Low, CWE-319] SMTP opportunistic TLS (see #9).
No hardcoded secrets, git history clean, encryption-key startup gate fails closed, --allow-plaintext is dev-only and logs only the env-var name.

8. Business / Stateful Logic — risk 5.5/10

[High, CWE-362/367] TemporalRepository::save TOCTOU: read-modify-write across 3 inner calls, no lock/transaction → concurrent same-entity saves cause lost update or two SENTINEL (live) rows. (Same root as #6 atomicity.)
[Medium, CWE-369/770] RateLimiter ctor unvalidated: refillRate<=0 evicts every bucket each sweep → limiter silently disabled (brute-force bypass); NaN → DoS. Fix: validate capacity>=1, refillRate>0, finite.
[Low-Med, CWE-488/362] WS thread_local t_pendingAuth handoff: if onAfterCreate runs on another thread / isn't reset on a failure path, stale auth could attach to another socket. Fails closed when empty.
[Low, CWE-338] Default entity-id gen uses non-crypto mt19937_64; predictable if consumers expose entity_id as unguessable handles.

9. API & Infrastructure — risk 4/10

[Medium, CWE-346] WebSocket Origin never validated → CSWSH risk for cookie-authenticated WS; library offers no Origin-allowlist primitive.
[Medium, CWE-770/307] Rate-limit key collapses to a single "unknown" bucket when not loopback-bound / XFF absent → shared-bucket DoS + per-IP brute-force throttle stops working.
[Low, CWE-319] SMTP CURLUSESSL_TRY opportunistic TLS — STARTTLS-strip sends credentials/mail cleartext; verify disabled for localhost. Fix: CURLUSESSL_ALL for non-loopback.
Strong: strict CSP/HSTS/nosniff/SAMEORIGIN defaults, body-size 411/413 + chunked-reject, WS 64KiB/500-conn/idle caps, JsonErrorHandler leaks no stack traces. No CORS shipped (same-origin by design) — but no guardrail against the *-with-credentials mistake.

10. Logging & Monitoring — risk 4/10

[Medium, CWE-117] Log injection: request path/method logged raw via OATPP_LOGW in logEvent (auth/AuthInterceptor.hpp:178-182) — CR/LF/control chars in the request target forge log lines.
[Low, CWE-117] Authenticated username logged unsanitized at LOGD (ws/Hub.hpp:203).
[Low, CWE-116] AuditLog::escapeJson incomplete → invalid/injectable JSON stored in audit_log.changed_fields.
Strong: no secrets/tokens/hashes/PII logged anywhere; responses escaped via ObjectMapper; audit-sink failures isolated from the write path.

11. File Handling — risk 0.5/10

N/A: no uploads, no static serving, no image processing — the library has essentially no file I/O.
[Info, CWE-15] Only filesystem-adjacent code is the systemd NOTIFY_SOCKET path (systemd/Notify.hpp), sourced from the trusted init environment, correctly bounded (no overflow), datagram target (no file primitive). Minor: over-long path silently truncated.

OWASP Top 10: A03 Injection (no request-reachable SQLi — strong; SMTP CRLF + log injection open), A09 Logging (strong) — mostly addressed. A01/A04/A07 — the headline gaps (scope bypasses, fail-open defaults, cert spoofing, unwired brute-force protection). A10 SSRF N/A.
GDPR: Largely N/A — the library stores no PII; erasure/export/consent are consumer responsibilities. Its GDPR-relevant contribution is the credential-redaction primitive, which has a fail-open hazard (misorder/typo silently retains password_hash in history) — treat as the GDPR-adjacent priority. Plus AuditLog can persist credential hashes into changed_fields.
PCI DSS: N/A — no payment/cardholder data path.

Code Quality Findings

13. Architecture & Design — modularity 6/10

[Imp 8] ws/Hub.hpp (438 lines) is a god header riddled with consumer-domain vocabulary (notifyBooking/notifyPerson/booking_id/propertyIds), contradicting the library's stated portability contract. An all-static singleton fusing registry + broadcast + presence + JSON + a static-init detached thread.
[Imp 8] Two incompatible audit_log table designs ship in one library: db/AuditLog.hpp (table_name/operation/changed_fields/actor/created_at) vs repo/AuditLogRepository.hpp (actor_user_id/entity_type/op/timestamp_ms). A consumer wiring both gets conflicting CREATE TABLE audit_log — latent runtime collision.
[Imp 7] AuthPrincipal::id is int but the repo/db layer migrated user IDs to TEXT/UUID — latent contract break (and the std::stoi throw).
[Imp 6] repo/ claims ORM-agnosticism but db/* + Concrete* hard-couple to oatpp-sqlite, with no CMake component split.
Strong: the IAuthBackend/IAuthPolicy/IRuntimeConfig seams and the Repository<T> decorator stack are textbook dependency-inversion + decorator.

14. Code Duplication

[Imp 8] Two parallel, incompatible audit-log designs (same as #13) — a correctness hazard, not just DRY.
[Imp 7] Four near-identical Concrete*Repository adapters (~150 LOC of identical fetch-into-vector boilerplate). Extract a FunctionRepository/CRTP base.
[Imp 6] Temporal live-row predicate valid_from <= datetime('now') AND valid_until > datetime('now') repeated 14×; soft-delete SQL 5×; upsert ON CONFLICT blocks per table.
[Imp 5] SENTINEL timestamp literal 9999-12-31T23:59:59Z re-typed in 5 places (drift risk vs the runtime comparison).
[Imp 5] oatpp PolymorphicDispatcher reflection walk copy-pasted across 3 decorators (cloneDto, redactFields, logUpdate) — fragile coupling to oatpp internals.

15. Error Handling

[Imp 6] Concrete repo writes (save/softDelete) discard the DB result — a failed INSERT ... ON CONFLICT/UPDATE is silently swallowed; combined with the non-transactional TemporalRepository::save, a mid-sequence failure leaves a half-applied version with no rollback, and the audit decorator records "success."
[Imp 6] requireUser std::stoi can throw → generic 500 instead of clean 401.
[Imp 5] SMTP send() leaks CURL*/curl_slist/curl_mime handles on any throw (e.g. std::bad_alloc during header assembly) — no RAII guard.
[Imp 5] SMTP TLS downgrade fail-open (CURLUSESSL_TRY).
[Imp 4] ScopeDeniedException/SchemaContractViolation have no in-library HTTP-status mapping — an uncaught ScopeDeniedException becomes a 500 (authz denial leaks as server error). Ship a translation helper.

16. Exception Flow

[Imp 8] DB failure inside AuthInterceptor::intercept is uncaught — a transient DB blip during auth yields an unlogged, uncategorized error surface (and the hasActiveUsers()-swallowed-to-false path can fail open to setup-mode admin).
[Imp 6] Concrete repos map query failure to "empty result"/"not found" — a failed read during TemporalRepository::save is misinterpreted as "fresh insert" → duplicate live row.
[Imp 5] TemporalRepository::save two-write sequence with no transaction (same as #6/#8).
[Imp 4] AuditLogRepository::emit catch(...) swallows non-std::exception sink failures silently (no log) → undetectable audit gaps.
[Imp 4] ScopeGuard uses exceptions for routine (attacker-triggerable) authorization denials on the hot path.

17. Code Quality Metrics

[Imp 6] ws/Hub.hpp god header (438 lines, 5 responsibilities, static-init thread).
[Imp 6] Two divergent audit impls with conflicting audit_log schema.
[Imp 5] repo/IQueryable.hpp (323 lines) packs the whole SQL-AST DSL in one header.
[Imp 5] TemporalRepository::save mixes abstraction levels; findByEntityId/list/history each re-implement the same O(n) full-list() scan (4 duplicated scans; per-write full-table fetch).
[Imp 5] AuthInterceptor::intercept ~80-line, ~12-complexity decision gauntlet with an embedded function-static session-sweep timer.
[Imp 4] SchemaContract.hpp fan-in=9 → rebuild hot-spot. Otherwise well-sized: only 2 headers strictly >300 lines.

18. Design Patterns

[Imp 7] IQueryable is not forwarded by any decorator — wrapping an IQueryable repo in the stack loses the queryable surface, and a downcast to the inner bypasses ScopeGuardRepository (authz bypass; ties to security H-1).
[Imp 7] Decorator composition trap: RedactedFieldRepository infers "is historical" from valid_until, working only when stacked below TemporalRepository; wrong order silently disables redaction with no error.
[Imp 6] WS observer is the right pattern but the all-static singleton + static-init detached thread + baked-in domain vocabulary blocks reuse/testing.
[Imp 6] Session-sweep timer bolted onto intercept() via a function-local static (stateful at process scope, untestable, on the hot path).
Strong: strategy/policy interface split, interceptor chain-of-responsibility, repository decorator stack, error-handler strategy — all appropriate and not over-engineered.

19. SOLID — rating 7/10

[SRP/DIP, Imp 7] Dual audit-log design (SRP: two reasons to change auditing; DIP: db::AuditLog depends on concrete oatpp::orm::Executor instead of implementing the existing IAuditSink).
[SRP, Imp 6] ws/Hub.hpp god object with fewo-domain notifications in a reusable header.
[LSP, Imp 5] RedactedFieldRepository not substitutable as a bare Repository<T> — redaction only correct when stacked under Temporal.
[ISP, Imp 4] IAuthBackend could give defaults to more methods so an API-key-only consumer needn't stub session/maintenance methods.
[DIP, Imp 6] SchemaContract::verify hardcodes SQLite pragma_table_info probe SQL despite claiming engine portability — inject an ISchemaIntrospector.
[OCP, Imp 4] AuditLog::valueToJson hardcodes a type ladder; unknown types silently serialize as null.
Strong OCP/ISP via the decorator stack + segregated auth interfaces; SQLite coupling correctly confined to the concrete tier.

20. Testing

[Imp 9] Auth interceptor decision flow (intercept()) is entirely untested — public-path bypass, setup-mode grant, cert-DN trust gate, session-vs-apikey resolution, CSRF, readonly-mutation block all unverified. Every authorization boundary in the library is untested.
[Imp 8] clientIpTrusted / cookie + bearer token extraction untested (the xsession= substring trap may reveal a real bug).
[Imp 7] Body-size limiter & security-headers tested as compile-smoke only — zero behavior verified despite rich security branching.
[Imp 7] RateLimiter untested (refill, eviction, concurrency, ctor validation) — needs an injectable clock.
[Imp 6] WS socketHasPropertyAccess authz matrix, SMTP base64Encode + CRLF guard untested.
Strong where present: the decorator/queryable/audit/schema suites are exemplary (injected clocks, behavior-over-implementation, real edge cases). The gap is breadth. Recommend a shared makeReq HTTP fixture as the keystone, and standardizing the abort()-vs-counter harness idiom.

21. Readability & Naming

[Imp 8] Two audit_log table shapes under the same name with divergent column vocabulary (table_name vs entity_type, operation vs op, actor vs actor_user_id).
[Imp 6] repo/IQueryable.hpp is the only file using trailing-underscore member naming (col_, where_, …) vs the library-wide m_ prefix.
[Imp 5] Four include-guard styles coexist; BodySizeLimitInterceptor_hpp / SecurityHeadersInterceptor_hpp / UTIL_RATE_LIMITER_HPP are un-namespaced and collision-prone for a header-only lib.
[Imp 5] dto/InternalDto.hpp uses snake_case C++ identifiers (booking_id) mixing with camelCase in the same struct — also leaks consumer domain.
Otherwise unusually well-documented; consistent English naming (no German leakage); class/file names match.

22. Resilience & Fault Tolerance — rating 6/10

[Imp 7] Auth interceptor fails open on transient backend exceptions via the under-specified IAuthBackend error contract — a swallowed hasActiveUsers() error can grant setup-mode admin. Document fail-closed contract / add a BackendUnavailable→503 path.
[Imp 5] WS thread_local handoff fragility; function-static session-sweep timer runs on the request hot path and only on traffic.
[Imp 5] TemporalRepository::save non-atomic multi-step (consumer-delegated, but the library should document the transaction requirement or wrap it).
[Imp 4] WS housekeeper thread is detached with no shutdown path; broadcast holds s_mx across all sends (head-of-line blocking) + a raw-pointer UAF window vs socket teardown.
Strong: SMTP curl timeouts, WS connection/message/idle caps, rate-limiter memory bound, body-size limits, audit-sink failure isolation, clean systemd notify primitive.

23. Orval Codegen Duplication

N/A — verified: no frontend/, no openapi.json, no Orval config, no React/TypeScript, no generated client. This audit category does not apply to a backend C++ library.

Prioritized Remediation Plan

[HIGH] Default certAuthTrusted() to false; require explicit opt-in + document that the proxy must strip inbound X-SSL-Client-DN — auth/IRuntimeConfig.hpp:47-49, auth/AuthInterceptor.hpp:236-246
[HIGH] Close the ScopeGuard IQueryable::query() bypass — provide a scope-aware queryable wrapper or refuse to expose it — repo/ScopeGuardRepository.hpp, repo/IQueryable.hpp:314-319
[HIGH] Make ScopeGuardRepository::save() load the existing row and require the scope predicate on BOTH existing and incoming DTO — repo/ScopeGuardRepository.hpp:94-99
[HIGH] Make TemporalRepository::save atomic (transaction or per-entity lock) and check write results so a failed read isn't misread as "fresh insert" — repo/TemporalRepository.hpp:180-207, repo/ConcreteUserRepository.hpp:53-59
[HIGH] Reject CR/LF/NUL in SMTP to/fromAddress before building headers/envelope — mail/SmtpTransport.hpp:84,87,132-134
[MEDIUM] Flip WS empty propertyIds from "all access" to "no access" — ws/Hub.hpp:159-163
[MEDIUM] Validate RateLimiter ctor (capacity>=1, refillRate>0, finite) so it cannot silently disable — util/RateLimiter.hpp:31-32
[MEDIUM] Harden setup-mode: require loopback, treat hasActiveUsers() errors as "users exist" (fail closed), synchronize the check — auth/AuthInterceptor.hpp:226-231
[MEDIUM] Replace cookie substring find("session=") with exact-name ;-tokenized parsing — util/TokenExtract.hpp:23-28
[MEDIUM] Validate Origin on the WS handshake (ship an allowlist primitive) — ws/Hub.hpp, ws/Listener.hpp
[MEDIUM] Wrap backend calls in AuthInterceptor::intercept in try/catch → 503; document the IAuthBackend fail-closed contract — auth/AuthInterceptor.hpp:203-283
[MEDIUM] Sanitize request path/method before logging (CWE-117) — auth/AuthInterceptor.hpp:178-182
[QUALITY] Unify the two audit_log designs (one schema; make db::AuditLog an IAuditSink impl) and complete escapeJson control-char escaping + add credential fields to SKIP_FIELDS — db/AuditLog.hpp, repo/AuditLogRepository.hpp
[QUALITY] Add test_auth_interceptor.cpp covering the full decision matrix (the single highest-value test gap), plus clientIpTrusted/extractToken and behavioral body-size/security-header tests
[QUALITY] Decompose ws/Hub.hpp, lift fewo-domain notifyBooking/notifyPerson/booking_id out of the library, and give the housekeeper thread a shutdown path

Findings cite exact file:line. Items marked "Unable to verify" require the consumer app (e.g. concrete IAuthBackend DB-error behavior, runtime proxy/TLS config). The recurring theme is insecure-by-default contracts in a library whose defaults propagate to every consumer — flipping the fail-open defaults to fail-closed is the highest-leverage hardening, protecting the entire downstream fleet rather than one app.

## Code Audit Report — 2026-05-29 Automated audit of **oatpp-authkit** covering 23 audit categories (12 security + 11 code quality), run via the `/code-audit` skill. Each category was executed by an independent subagent reading the live source under `include/oatpp-authkit/**`. **Framing:** oatpp-authkit is a reusable **header-only C++ auth/security library** consumed by webapps (e.g. fewo-webapp). It has no frontend, no controllers/DTOs of its own, and stores no PII itself. The dominant risk theme is **insecure-by-default contracts**: a fail-open default in a security library has fleet-wide blast radius across every consumer that inherits it. Frontend-specific audits (and audit 23/Orval) are N/A. ### Executive Summary **Overall posture: MEDIUM**, trending **HIGH-risk-by-default for naive consumers.** The library has genuinely strong defaults (strict CSP/HSTS/nosniff/frame headers, body-size + WS connection caps, leak-free error handler, no secret logging, fail-closed encryption-key gate, fully parameterized SQL). Its highest-impact weaknesses are fail-open trust boundaries and incomplete authorization decorators that silently shift risk onto consumers. **No Critical** in the default posture; the worst trust-boundary flaw (cert-DN spoofing) becomes effectively critical under the *standard* loopback-behind-proxy misdeployment. **Unique security findings by severity (deduplicated):** - 🔴 Critical: 0 - 🟠 High: 5 - 🟡 Medium: 11 - 🔵 Low: 8 (+ informational) **Immediate actions (top 3):** 1. Flip the dangerous defaults to fail-closed — `certAuthTrusted()` → false, WS empty `propertyIds` → no access, validate `RateLimiter` ctor. 2. Close the ScopeGuard bypasses — wrap/refuse `IQueryable::query()`, and make `save()` check the existing row's scope, not just the incoming DTO. 3. Self-defend SMTP against CRLF/header injection in `to`/`fromAddress`. --- ## Security Findings ### 1. Initial Security Analysis — risk 5.5/10 - **[Medium, CWE-330/252]** Unchecked randomness paths and insecure-by-default trust decisions shift burden to consumers (cert-DN trust, setup mode, SMTP TLS). - **[Low, CWE-352]** CSRF for cookie-session mutations relies solely on `X-Requested-With`; cert path has no CSRF gate. - **[Low, CWE-248]** `requireUser` `std::stoi` on bundle id (`auth/RequireRole.hpp:35`) can throw → 500 instead of 401; `AuthPrincipal::id` is `int` while the user store migrated to TEXT/UUID. - **[Low, CWE-116]** `AuditLog::escapeJson` (`db/AuditLog.hpp:89-98`) escapes only `\`/`"`. - _Strong: parameterized SQL, validated loopback-gated XFF, body-size limits, WS caps, leak-free error handler._ ### 2. Session & Cookie Security — risk 4.5/10 - **[Medium, CWE-565/697]** Cookie parser uses substring `find("session=")` (`util/TokenExtract.hpp:23-28`) — matches `xsession=`/`notsession=`/etc., takes first match → token confusion, defeats `__Host-` prefix guarantees. Fix: tokenize on `;`, exact-name match. - **[Medium, CWE-1004/614]** Library ships **no** safe `setSessionCookie` helper (Secure/HttpOnly/SameSite); consumers hand-roll `Set-Cookie` and the baseline doc only *recommends* `Secure`. - **[Medium, CWE-352]** CSRF defense is single `X-Requested-With` check, no Origin/Referer validation, broken if consumer enables permissive CORS. - **[Medium, CWE-290/348]** `X-SSL-Client-DN` cert trust defaults to `isLoopback()` (see synthesis H-1). ### 3. Authentication Flow — risk 5.5/10 - **[Medium, CWE-307]** Brute-force protection: `RateLimiter` exists but is **NOT wired into** `AuthInterceptor` — entirely consumer responsibility, undocumented. - **[Medium, CWE-636]** Setup-mode pseudo-admin (`AuthInterceptor.hpp:226-231`) fails open: any request → admin when `setupModeActive() && !hasActiveUsers()`; fail-open if `hasActiveUsers()` swallows a DB error to `false`; no loopback requirement. - **[Low, CWE-208/916]** `TokenHasher` is consumer-supplied with no enforced minimum strength; no constant-time compare helper shipped. - **[Low, CWE-613]** Session-expiry sweep is GC-only and runs only on request traffic; if a consumer relies on it (instead of resolve-time expiry), expired sessions stay valid. - _Strong: `IAuthBackend`/`IAuthPolicy`/`IRuntimeConfig` strategy split is excellent; library never sees plaintext passwords; deny-by-default at the interceptor._ ### 4. Input Validation — risk 5/10 - **[High, CWE-93/88]** SMTP header/CRLF injection: `to` and `fromAddress` concatenated into headers + envelope with no CRLF/NUL validation (`mail/SmtpTransport.hpp:84,87,132-134`). Subject is base64-safe; to/from are not. A security library must self-defend. - **[Low, CWE-150]** LIKE patterns not escaped in the query DSL (`repo/IQueryable.hpp:221-224`) — wildcard metachars pass through (no SQLi, values bound; full-scan/match-semantics risk). - **[Low, CWE-20]** `BodySizeLimitInterceptor` `Content-Length` parse accepts `+`/leading-whitespace forms (theoretical parser-desync). - _No request-data-reachable SQLi: all values parameterized; identifiers come from compile-time registration._ ### 5. Authorization — risk 6.5/10 (highest) - **[HIGH, CWE-639/863]** **ScopeGuardRepository `IQueryable::query()` bypass**: the guard decorates only the 4 `Repository<T>` methods; `IQueryable::query()` (`repo/IQueryable.hpp:314-319`) is a separate surface it does NOT wrap → arbitrary filtered SELECTs bypass scope entirely (full cross-tenant read). - **[HIGH, CWE-639/863]** **ScopeGuard `save()` checks only the incoming DTO, never the existing row**: actor scoped to prop-A submits an update for an entity currently in prop-B with `propertyId=prop-A` in the body → overwrites/reparents the prop-B row. Fix: load existing row, require predicate on BOTH. - **[HIGH, CWE-1188/636]** Multiple fail-open defaults: (a) WS empty `propertyIds` = "all access" (`ws/Hub.hpp:159-163`); (b) cert trust loopback default; (c) `BodySizeLimit requireContentLength=false` escape hatch. - _Interceptor itself is deny-by-default with readonly + CSRF enforcement; `requireAdmin` is a proper gate._ ### 6. Database Security — risk 3.5/10 - **[Medium, CWE-662]** Temporal close-then-insert is **non-atomic** (`repo/TemporalRepository.hpp:196-207`): two separate saves, no transaction → crash/error leaves duplicate-live or lost-version. Cascade soft-deletes likewise. - **[Medium, CWE-212/522]** `RedactedFieldRepository` fails open if decorators misordered: redaction fires only when stacked between Temporal and concrete; wrong order silently retains `password_hash` in history. Field matched by name only — silent if typo'd. - **[Low, CWE-89 mitigated]** ORDER BY / table / column identifiers concatenated but sourced from compile-time registration (not request-reachable). Document the invariant. - _All values parameterized; no request-reachable SQLi._ ### 7. Secrets Management — risk 1.5/10 - **[Low, CWE-532/312]** `AuditLog::logUpdate<Dto>` can persist `password_hash`/`tls_cert_dn` into `changed_fields` (not in `SKIP_FIELDS`). - **[Low, CWE-319]** SMTP opportunistic TLS (see #9). - _No hardcoded secrets, git history clean, encryption-key startup gate fails closed, `--allow-plaintext` is dev-only and logs only the env-var name._ ### 8. Business / Stateful Logic — risk 5.5/10 - **[High, CWE-362/367]** `TemporalRepository::save` TOCTOU: read-modify-write across 3 inner calls, no lock/transaction → concurrent same-entity saves cause lost update or **two SENTINEL (live) rows**. (Same root as #6 atomicity.) - **[Medium, CWE-369/770]** `RateLimiter` ctor unvalidated: `refillRate<=0` evicts every bucket each sweep → limiter **silently disabled** (brute-force bypass); `NaN` → DoS. Fix: validate `capacity>=1`, `refillRate>0`, finite. - **[Low-Med, CWE-488/362]** WS `thread_local t_pendingAuth` handoff: if `onAfterCreate` runs on another thread / isn't reset on a failure path, stale auth could attach to another socket. Fails closed when empty. - **[Low, CWE-338]** Default entity-id gen uses non-crypto `mt19937_64`; predictable if consumers expose `entity_id` as unguessable handles. ### 9. API & Infrastructure — risk 4/10 - **[Medium, CWE-346]** WebSocket Origin never validated → CSWSH risk for cookie-authenticated WS; library offers no Origin-allowlist primitive. - **[Medium, CWE-770/307]** Rate-limit key collapses to a single `"unknown"` bucket when not loopback-bound / XFF absent → shared-bucket DoS + per-IP brute-force throttle stops working. - **[Low, CWE-319]** SMTP `CURLUSESSL_TRY` opportunistic TLS — STARTTLS-strip sends credentials/mail cleartext; verify disabled for localhost. Fix: `CURLUSESSL_ALL` for non-loopback. - _Strong: strict CSP/HSTS/nosniff/SAMEORIGIN defaults, body-size 411/413 + chunked-reject, WS 64KiB/500-conn/idle caps, `JsonErrorHandler` leaks no stack traces. No CORS shipped (same-origin by design) — but no guardrail against the `*`-with-credentials mistake._ ### 10. Logging & Monitoring — risk 4/10 - **[Medium, CWE-117]** Log injection: request path/method logged raw via `OATPP_LOGW` in `logEvent` (`auth/AuthInterceptor.hpp:178-182`) — CR/LF/control chars in the request target forge log lines. - **[Low, CWE-117]** Authenticated username logged unsanitized at `LOGD` (`ws/Hub.hpp:203`). - **[Low, CWE-116]** `AuditLog::escapeJson` incomplete → invalid/injectable JSON stored in `audit_log.changed_fields`. - _Strong: no secrets/tokens/hashes/PII logged anywhere; responses escaped via ObjectMapper; audit-sink failures isolated from the write path._ ### 11. File Handling — risk 0.5/10 - **N/A:** no uploads, no static serving, no image processing — the library has essentially no file I/O. - **[Info, CWE-15]** Only filesystem-adjacent code is the systemd `NOTIFY_SOCKET` path (`systemd/Notify.hpp`), sourced from the trusted init environment, correctly bounded (no overflow), datagram target (no file primitive). Minor: over-long path silently truncated. ### 12. Compliance Synthesis (OWASP / GDPR / PCI) - **OWASP Top 10:** A03 Injection (no request-reachable SQLi — strong; SMTP CRLF + log injection open), A09 Logging (strong) — mostly addressed. A01/A04/A07 — **the headline gaps** (scope bypasses, fail-open defaults, cert spoofing, unwired brute-force protection). A10 SSRF N/A. - **GDPR:** Largely **N/A** — the library stores no PII; erasure/export/consent are consumer responsibilities. Its GDPR-relevant contribution is the credential-redaction primitive, which has a **fail-open hazard** (misorder/typo silently retains `password_hash` in history) — treat as the GDPR-adjacent priority. Plus `AuditLog` can persist credential hashes into `changed_fields`. - **PCI DSS:** **N/A** — no payment/cardholder data path. --- ## Code Quality Findings ### 13. Architecture & Design — modularity 6/10 - **[Imp 8]** `ws/Hub.hpp` (438 lines) is a god header riddled with **consumer-domain vocabulary** (`notifyBooking`/`notifyPerson`/`booking_id`/`propertyIds`), contradicting the library's stated portability contract. An all-static singleton fusing registry + broadcast + presence + JSON + a static-init detached thread. - **[Imp 8]** **Two incompatible `audit_log` table designs ship in one library**: `db/AuditLog.hpp` (`table_name/operation/changed_fields/actor/created_at`) vs `repo/AuditLogRepository.hpp` (`actor_user_id/entity_type/op/timestamp_ms`). A consumer wiring both gets conflicting `CREATE TABLE audit_log` — latent runtime collision. - **[Imp 7]** `AuthPrincipal::id` is `int` but the repo/db layer migrated user IDs to TEXT/UUID — latent contract break (and the `std::stoi` throw). - **[Imp 6]** `repo/` claims ORM-agnosticism but `db/*` + `Concrete*` hard-couple to oatpp-sqlite, with no CMake component split. - _Strong: the `IAuthBackend`/`IAuthPolicy`/`IRuntimeConfig` seams and the `Repository<T>` decorator stack are textbook dependency-inversion + decorator._ ### 14. Code Duplication - **[Imp 8]** Two parallel, incompatible audit-log designs (same as #13) — a correctness hazard, not just DRY. - **[Imp 7]** Four near-identical `Concrete*Repository` adapters (~150 LOC of identical fetch-into-vector boilerplate). Extract a `FunctionRepository`/CRTP base. - **[Imp 6]** Temporal live-row predicate `valid_from <= datetime('now') AND valid_until > datetime('now')` repeated 14×; soft-delete SQL 5×; upsert `ON CONFLICT` blocks per table. - **[Imp 5]** `SENTINEL` timestamp literal `9999-12-31T23:59:59Z` re-typed in 5 places (drift risk vs the runtime comparison). - **[Imp 5]** oatpp `PolymorphicDispatcher` reflection walk copy-pasted across 3 decorators (`cloneDto`, `redactFields`, `logUpdate`) — fragile coupling to oatpp internals. ### 15. Error Handling - **[Imp 6]** Concrete repo writes (`save`/`softDelete`) **discard the DB result** — a failed `INSERT ... ON CONFLICT`/`UPDATE` is silently swallowed; combined with the non-transactional `TemporalRepository::save`, a mid-sequence failure leaves a half-applied version with no rollback, and the audit decorator records "success." - **[Imp 6]** `requireUser` `std::stoi` can throw → generic 500 instead of clean 401. - **[Imp 5]** SMTP `send()` leaks `CURL*`/`curl_slist`/`curl_mime` handles on any throw (e.g. `std::bad_alloc` during header assembly) — no RAII guard. - **[Imp 5]** SMTP TLS downgrade fail-open (`CURLUSESSL_TRY`). - **[Imp 4]** `ScopeDeniedException`/`SchemaContractViolation` have no in-library HTTP-status mapping — an uncaught `ScopeDeniedException` becomes a 500 (authz denial leaks as server error). Ship a translation helper. ### 16. Exception Flow - **[Imp 8]** DB failure inside `AuthInterceptor::intercept` is uncaught — a transient DB blip during auth yields an unlogged, uncategorized error surface (and the `hasActiveUsers()`-swallowed-to-false path can fail open to setup-mode admin). - **[Imp 6]** Concrete repos map query **failure** to "empty result"/"not found" — a failed read during `TemporalRepository::save` is misinterpreted as "fresh insert" → duplicate live row. - **[Imp 5]** `TemporalRepository::save` two-write sequence with no transaction (same as #6/#8). - **[Imp 4]** `AuditLogRepository::emit` `catch(...)` swallows non-`std::exception` sink failures **silently** (no log) → undetectable audit gaps. - **[Imp 4]** ScopeGuard uses exceptions for routine (attacker-triggerable) authorization denials on the hot path. ### 17. Code Quality Metrics - **[Imp 6]** `ws/Hub.hpp` god header (438 lines, 5 responsibilities, static-init thread). - **[Imp 6]** Two divergent audit impls with conflicting `audit_log` schema. - **[Imp 5]** `repo/IQueryable.hpp` (323 lines) packs the whole SQL-AST DSL in one header. - **[Imp 5]** `TemporalRepository::save` mixes abstraction levels; `findByEntityId`/`list`/`history` each re-implement the same O(n) full-`list()` scan (4 duplicated scans; per-write full-table fetch). - **[Imp 5]** `AuthInterceptor::intercept` ~80-line, ~12-complexity decision gauntlet with an embedded function-static session-sweep timer. - **[Imp 4]** `SchemaContract.hpp` fan-in=9 → rebuild hot-spot. _Otherwise well-sized: only 2 headers strictly >300 lines._ ### 18. Design Patterns - **[Imp 7]** **`IQueryable` is not forwarded by any decorator** — wrapping an `IQueryable` repo in the stack loses the queryable surface, and a downcast to the inner bypasses `ScopeGuardRepository` (authz bypass; ties to security H-1). - **[Imp 7]** Decorator **composition trap**: `RedactedFieldRepository` infers "is historical" from `valid_until`, working only when stacked below `TemporalRepository`; wrong order silently disables redaction with no error. - **[Imp 6]** WS observer is the right pattern but the all-static singleton + static-init detached thread + baked-in domain vocabulary blocks reuse/testing. - **[Imp 6]** Session-sweep timer bolted onto `intercept()` via a function-local static (stateful at process scope, untestable, on the hot path). - _Strong: strategy/policy interface split, interceptor chain-of-responsibility, repository decorator stack, error-handler strategy — all appropriate and not over-engineered._ ### 19. SOLID — rating 7/10 - **[SRP/DIP, Imp 7]** Dual audit-log design (SRP: two reasons to change auditing; DIP: `db::AuditLog` depends on concrete `oatpp::orm::Executor` instead of implementing the existing `IAuditSink`). - **[SRP, Imp 6]** `ws/Hub.hpp` god object with fewo-domain notifications in a reusable header. - **[LSP, Imp 5]** `RedactedFieldRepository` not substitutable as a bare `Repository<T>` — redaction only correct when stacked under Temporal. - **[ISP, Imp 4]** `IAuthBackend` could give defaults to more methods so an API-key-only consumer needn't stub session/maintenance methods. - **[DIP, Imp 6]** `SchemaContract::verify` hardcodes SQLite `pragma_table_info` probe SQL despite claiming engine portability — inject an `ISchemaIntrospector`. - **[OCP, Imp 4]** `AuditLog::valueToJson` hardcodes a type ladder; unknown types silently serialize as `null`. - _Strong OCP/ISP via the decorator stack + segregated auth interfaces; SQLite coupling correctly confined to the concrete tier._ ### 20. Testing - **[Imp 9]** **Auth interceptor decision flow (`intercept()`) is entirely untested** — public-path bypass, setup-mode grant, cert-DN trust gate, session-vs-apikey resolution, CSRF, readonly-mutation block all unverified. Every authorization boundary in the library is untested. - **[Imp 8]** `clientIpTrusted` / cookie + bearer token extraction untested (the `xsession=` substring trap may reveal a real bug). - **[Imp 7]** Body-size limiter & security-headers tested as **compile-smoke only** — zero behavior verified despite rich security branching. - **[Imp 7]** `RateLimiter` untested (refill, eviction, concurrency, ctor validation) — needs an injectable clock. - **[Imp 6]** WS `socketHasPropertyAccess` authz matrix, SMTP `base64Encode` + CRLF guard untested. - _Strong where present: the decorator/queryable/audit/schema suites are exemplary (injected clocks, behavior-over-implementation, real edge cases). The gap is breadth. Recommend a shared `makeReq` HTTP fixture as the keystone, and standardizing the `abort()`-vs-counter harness idiom._ ### 21. Readability & Naming - **[Imp 8]** Two `audit_log` table shapes under the same name with divergent column vocabulary (`table_name` vs `entity_type`, `operation` vs `op`, `actor` vs `actor_user_id`). - **[Imp 6]** `repo/IQueryable.hpp` is the **only** file using trailing-underscore member naming (`col_`, `where_`, …) vs the library-wide `m_` prefix. - **[Imp 5]** Four include-guard styles coexist; `BodySizeLimitInterceptor_hpp` / `SecurityHeadersInterceptor_hpp` / `UTIL_RATE_LIMITER_HPP` are un-namespaced and collision-prone for a header-only lib. - **[Imp 5]** `dto/InternalDto.hpp` uses snake_case C++ identifiers (`booking_id`) mixing with camelCase in the same struct — also leaks consumer domain. - _Otherwise unusually well-documented; consistent English naming (no German leakage); class/file names match._ ### 22. Resilience & Fault Tolerance — rating 6/10 - **[Imp 7]** Auth interceptor **fails open on transient backend exceptions** via the under-specified `IAuthBackend` error contract — a swallowed `hasActiveUsers()` error can grant setup-mode admin. Document fail-closed contract / add a `BackendUnavailable`→503 path. - **[Imp 5]** WS `thread_local` handoff fragility; function-static session-sweep timer runs on the request hot path and only on traffic. - **[Imp 5]** `TemporalRepository::save` non-atomic multi-step (consumer-delegated, but the library should document the transaction requirement or wrap it). - **[Imp 4]** WS housekeeper thread is detached with no shutdown path; broadcast holds `s_mx` across all sends (head-of-line blocking) + a raw-pointer UAF window vs socket teardown. - _Strong: SMTP curl timeouts, WS connection/message/idle caps, rate-limiter memory bound, body-size limits, audit-sink failure isolation, clean systemd notify primitive._ ### 23. Orval Codegen Duplication - **N/A** — verified: no `frontend/`, no `openapi.json`, no Orval config, no React/TypeScript, no generated client. This audit category does not apply to a backend C++ library. --- ## Prioritized Remediation Plan - [ ] **[HIGH]** Default `certAuthTrusted()` to false; require explicit opt-in + document that the proxy must strip inbound `X-SSL-Client-DN` — `auth/IRuntimeConfig.hpp:47-49`, `auth/AuthInterceptor.hpp:236-246` - [ ] **[HIGH]** Close the ScopeGuard `IQueryable::query()` bypass — provide a scope-aware queryable wrapper or refuse to expose it — `repo/ScopeGuardRepository.hpp`, `repo/IQueryable.hpp:314-319` - [ ] **[HIGH]** Make `ScopeGuardRepository::save()` load the existing row and require the scope predicate on BOTH existing and incoming DTO — `repo/ScopeGuardRepository.hpp:94-99` - [ ] **[HIGH]** Make `TemporalRepository::save` atomic (transaction or per-entity lock) and check write results so a failed read isn't misread as "fresh insert" — `repo/TemporalRepository.hpp:180-207`, `repo/ConcreteUserRepository.hpp:53-59` - [ ] **[HIGH]** Reject CR/LF/NUL in SMTP `to`/`fromAddress` before building headers/envelope — `mail/SmtpTransport.hpp:84,87,132-134` - [ ] **[MEDIUM]** Flip WS empty `propertyIds` from "all access" to "no access" — `ws/Hub.hpp:159-163` - [ ] **[MEDIUM]** Validate `RateLimiter` ctor (`capacity>=1`, `refillRate>0`, finite) so it cannot silently disable — `util/RateLimiter.hpp:31-32` - [ ] **[MEDIUM]** Harden setup-mode: require loopback, treat `hasActiveUsers()` errors as "users exist" (fail closed), synchronize the check — `auth/AuthInterceptor.hpp:226-231` - [ ] **[MEDIUM]** Replace cookie substring `find("session=")` with exact-name `;`-tokenized parsing — `util/TokenExtract.hpp:23-28` - [ ] **[MEDIUM]** Validate Origin on the WS handshake (ship an allowlist primitive) — `ws/Hub.hpp`, `ws/Listener.hpp` - [ ] **[MEDIUM]** Wrap backend calls in `AuthInterceptor::intercept` in try/catch → 503; document the `IAuthBackend` fail-closed contract — `auth/AuthInterceptor.hpp:203-283` - [ ] **[MEDIUM]** Sanitize request path/method before logging (CWE-117) — `auth/AuthInterceptor.hpp:178-182` - [ ] **[QUALITY]** Unify the two `audit_log` designs (one schema; make `db::AuditLog` an `IAuditSink` impl) and complete `escapeJson` control-char escaping + add credential fields to `SKIP_FIELDS` — `db/AuditLog.hpp`, `repo/AuditLogRepository.hpp` - [ ] **[QUALITY]** Add `test_auth_interceptor.cpp` covering the full decision matrix (the single highest-value test gap), plus `clientIpTrusted`/`extractToken` and behavioral body-size/security-header tests - [ ] **[QUALITY]** Decompose `ws/Hub.hpp`, lift fewo-domain `notifyBooking`/`notifyPerson`/`booking_id` out of the library, and give the housekeeper thread a shutdown path --- _Findings cite exact file:line. Items marked "Unable to verify" require the consumer app (e.g. concrete `IAuthBackend` DB-error behavior, runtime proxy/TLS config). The recurring theme is **insecure-by-default contracts** in a library whose defaults propagate to every consumer — flipping the fail-open defaults to fail-closed is the highest-leverage hardening, protecting the entire downstream fleet rather than one app._

uwe.admin referenced this issue from a commit

2026-05-29 12:49:05 +02:00

#16 (audit H-1..H-5): fix the five high-severity findings

uwe.admin commented

2026-05-29 12:49:18 +02:00

Author

Owner

The 5 high-severity findings are fixed in commit 2e11408 on main:

H-1 cert-DN spoofing — IRuntimeConfig::certAuthTrusted() now defaults to false (fail-closed); explicit opt-in required.
H-2 IQueryable bypass — added ScopeGuardQueryable<T> which filters query() results through the scope predicate.
H-3 scope reparenting — ScopeGuardRepository::save() now also checks the existing row's scope (new required entity-id accessor), blocking relabel-to-claim.
H-4 TemporalRepository TOCTOU — per-instance mutex serialises the read-modify-write + optional TxRunner for atomic close-then-insert.
H-5 SMTP header injection — to/fromAddress rejected if they contain CR/LF/NUL.

Tests: expanded test_repository_decorators (reparenting + queryable filtering) and added curl-guarded test_smtp_transport; all 15 ctest targets pass.

Note: the ScopeGuardRepository constructor gained a required entity-id accessor (breaking) — safe because consumers pin authkit by commit SHA and bump deliberately. The Medium/Low items remain open.

The 5 high-severity findings are fixed in commit `2e11408` on `main`: - **H-1 cert-DN spoofing** — `IRuntimeConfig::certAuthTrusted()` now defaults to `false` (fail-closed); explicit opt-in required. - **H-2 IQueryable bypass** — added `ScopeGuardQueryable<T>` which filters `query()` results through the scope predicate. - **H-3 scope reparenting** — `ScopeGuardRepository::save()` now also checks the existing row's scope (new required entity-id accessor), blocking relabel-to-claim. - **H-4 TemporalRepository TOCTOU** — per-instance mutex serialises the read-modify-write + optional `TxRunner` for atomic close-then-insert. - **H-5 SMTP header injection** — `to`/`fromAddress` rejected if they contain CR/LF/NUL. Tests: expanded `test_repository_decorators` (reparenting + queryable filtering) and added curl-guarded `test_smtp_transport`; all 15 ctest targets pass. Note: the `ScopeGuardRepository` constructor gained a required entity-id accessor (breaking) — safe because consumers pin authkit by commit SHA and bump deliberately. The Medium/Low items remain open.

uwe.admin referenced this issue from a commit

2026-05-29 13:53:23 +02:00

#16 (audit M-1..M-12): fix the medium-severity findings

uwe.admin commented

2026-05-29 13:53:40 +02:00

Author

Owner

The medium-severity findings are fixed in commit fafee12 on main:

M-1 TokenExtract exact-name cookie parse (new cookieValue helper) — no more substring shadowing of session=.
M-2 Setup-mode pseudo-admin now gated on a loopback bind + logged; IAuthBackend::hasActiveUsers() documented as must-fail-closed.
M-3 ws/Hub empty propertyIds now = no access for non-admins (was "all").
M-4 new util/OriginCheck.hpp + Hub doc — WSController must validate Origin at handshake (CSWSH).
M-6 RedactedFieldRepository throws on an unknown redaction field name.
M-7 RateLimiter ctor validates capacity/refillRate (throws on 0/negative/NaN/inf).
M-8 documented the clientIpTrusted "unknown"/"invalid" shared-bucket caveat.
M-9 new util/SessionCookie.hpp — safe-by-default Set-Cookie builder.
M-10 AuthInterceptor Origin/Referer-vs-Host check on session mutations; cert path documented as non-browser / not CSRF-gated.
M-11 AuthInterceptor optional injected RateLimiter throttles invalid-token attempts → 429.
M-12 request method/path sanitised before logging (CWE-117).

M-5 (temporal non-atomic save) was already resolved by the H-4 fix.

Tests: 4 new suites (token_extract, rate_limiter, origin_check, session_cookie) + extended redacted-field; all 19 ctest targets pass.

fewo-webapp integration note (for the next pin bump): three are behaviour changes, not just additive — (1) WS non-admin sockets with an empty scope set now receive nothing (M-3); (2) session-cookie mutations with a cross-host Origin/Referer are now 403'd by authkit itself (M-10) — fewo's own CsrfStrictInterceptor already does this, so same-origin traffic is unaffected; (3) setup-mode requires a loopback bind (M-2). The AuthInterceptor constructor only gained an optional trailing param (non-breaking). Remaining: the Low findings.

The medium-severity findings are fixed in commit `fafee12` on `main`: - **M-1** TokenExtract exact-name cookie parse (new `cookieValue` helper) — no more substring shadowing of `session=`. - **M-2** Setup-mode pseudo-admin now gated on a loopback bind + logged; `IAuthBackend::hasActiveUsers()` documented as must-fail-closed. - **M-3** `ws/Hub` empty `propertyIds` now = **no** access for non-admins (was "all"). - **M-4** new `util/OriginCheck.hpp` + Hub doc — WSController must validate Origin at handshake (CSWSH). - **M-6** `RedactedFieldRepository` throws on an unknown redaction field name. - **M-7** `RateLimiter` ctor validates capacity/refillRate (throws on 0/negative/NaN/inf). - **M-8** documented the `clientIpTrusted` "unknown"/"invalid" shared-bucket caveat. - **M-9** new `util/SessionCookie.hpp` — safe-by-default Set-Cookie builder. - **M-10** `AuthInterceptor` Origin/Referer-vs-Host check on session mutations; cert path documented as non-browser / not CSRF-gated. - **M-11** `AuthInterceptor` optional injected RateLimiter throttles invalid-token attempts → 429. - **M-12** request method/path sanitised before logging (CWE-117). **M-5** (temporal non-atomic save) was already resolved by the H-4 fix. Tests: 4 new suites (token_extract, rate_limiter, origin_check, session_cookie) + extended redacted-field; all 19 ctest targets pass. **fewo-webapp integration note** (for the next pin bump): three are behaviour changes, not just additive — (1) WS non-admin sockets with an empty scope set now receive nothing (M-3); (2) session-cookie mutations with a cross-host Origin/Referer are now 403'd by authkit itself (M-10) — fewo's own CsrfStrictInterceptor already does this, so same-origin traffic is unaffected; (3) setup-mode requires a loopback bind (M-2). The `AuthInterceptor` constructor only gained an optional trailing param (non-breaking). Remaining: the Low findings.

uwe.admin referenced this issue from a commit

2026-05-29 14:03:02 +02:00

#16 (audit L-1..L-8): fix the low-severity findings

uwe.admin commented

2026-05-29 14:11:14 +02:00

Author

Owner

The low-severity findings are fixed in commit 9976efe on main, and fewo-webapp's pin is bumped to it (fewo 99dee97).

L-1 requireUser guards std::stoi — malformed bundle id → clean 401, not a 500; AuthPrincipal::id documented as numeric-only.
L-2 SMTP requires TLS (CURLUSESSL_ALL) for non-loopback relays (no silent STARTTLS-strip to cleartext).
L-3 AuditLog::escapeJson escapes all control chars (RFC 8259); SKIP_FIELDS now excludes credential fields from diffs.
L-4 ws/Hub consumes the thread_local auth handoff once up-front and clears it unconditionally (no stale leak to a reused thread).
L-5 default entity-id generator draws 128 bits from the platform CSPRNG per call (was a once-seeded mt19937_64).
L-6 session sweep is a lock-free atomic timer + exception-isolated; resolveBySessionHash documented as the authoritative expiry gate.
L-7 new util/ConstantTime.hpp (constantTimeEquals) + TokenHasher ≥256-bit contract.
L-8 likeEscape + Field::likeContains/likePrefix emit LIKE ? ESCAPE '\'; compile-time identifier-source invariant documented.

Tests: new test_constant_time; LIKE-escaping cases in test_queryable. All 20 authkit ctest targets pass.

All actionable findings (5 High, 11 Medium [M-5 = H-4], 8 Low) are now resolved across H/M/L. Only the informational items remain (e.g. broad img-src CSP, the unscoped legacy notifyBooking overload). Suggest closing this issue once those are triaged.

Operational note for fewo: L-2 now refuses a non-loopback SMTP relay that can't STARTTLS (previously it would fall back to cleartext). If the production relay is non-loopback, confirm it supports STARTTLS or front it via a localhost relay.

The low-severity findings are fixed in commit `9976efe` on `main`, and fewo-webapp's pin is bumped to it (fewo `99dee97`). - **L-1** `requireUser` guards `std::stoi` — malformed bundle id → clean 401, not a 500; `AuthPrincipal::id` documented as numeric-only. - **L-2** SMTP requires TLS (`CURLUSESSL_ALL`) for non-loopback relays (no silent STARTTLS-strip to cleartext). - **L-3** `AuditLog::escapeJson` escapes all control chars (RFC 8259); `SKIP_FIELDS` now excludes credential fields from diffs. - **L-4** `ws/Hub` consumes the `thread_local` auth handoff once up-front and clears it unconditionally (no stale leak to a reused thread). - **L-5** default entity-id generator draws 128 bits from the platform CSPRNG per call (was a once-seeded mt19937_64). - **L-6** session sweep is a lock-free atomic timer + exception-isolated; `resolveBySessionHash` documented as the authoritative expiry gate. - **L-7** new `util/ConstantTime.hpp` (`constantTimeEquals`) + TokenHasher ≥256-bit contract. - **L-8** `likeEscape` + `Field::likeContains/likePrefix` emit `LIKE ? ESCAPE '\'`; compile-time identifier-source invariant documented. Tests: new `test_constant_time`; LIKE-escaping cases in `test_queryable`. All 20 authkit ctest targets pass. **All actionable findings (5 High, 11 Medium [M-5 = H-4], 8 Low) are now resolved across H/M/L.** Only the informational items remain (e.g. broad `img-src` CSP, the unscoped legacy `notifyBooking` overload). Suggest closing this issue once those are triaged. **Operational note for fewo:** L-2 now refuses a non-loopback SMTP relay that can't STARTTLS (previously it would fall back to cleartext). If the production relay is non-loopback, confirm it supports STARTTLS or front it via a localhost relay.