preauth/DESIGN_CONSIDERATIONS.md

Design Considerations — Preauth

Summary

Preauth is a well-architected TOTP-based authentication gateway that has evolved from a single-file script into a clean, event-listener-driven Symfony application with 100% test coverage. The codebase demonstrates strong security fundamentals (host-prefixed cookies, nonce-based replay protection, rate limiting, backup code system) and thoughtful operational design (dual-layer cache with change tracking, FrankenPHP worker mode).

This document was originally prepared as a design review. Items that have been addressed are marked with ✅ and include a reference to the commit or change that resolved them. Items still open are marked with ⬜ and remain as recommendations for future work.

---

1. Security

1.1 Missing Security Response Headers [HIGH PRIORITY] ✅ Addressed

Current state: Fixed. A SecurityHeadersListener (response event, priority 0) now sets the following headers on all main-request responses:

X-Content-Type-Options: nosniff
X-Frame-Options: DENY
Content-Security-Policy: default-src 'none'; script-src 'unsafe-inline'; style-src 'unsafe-inline'
Referrer-Policy: strict-origin-when-cross-origin
Strict-Transport-Security: max-age=31536000

The inline <script> and <style> in the templates mean a CSP with 'unsafe-inline' for script-src and style-src is the strictest practical policy today. Moving scripts/styles to external files would allow a stricter CSP in the future.

1.2 Remote-User Header Value is User-Controlled [HIGH PRIORITY] ✅ Addressed

Current state: Fixed. The Remote-User header value is now configurable via the REMOTE_USER environment variable, which supports four modes:

• session (default, backward-compatible): Sends the session id, as before. The value is still sanitized via makeCacheKey().
• static: Sends a fixed string (configurable via REMOTE_USER_STATIC, default authenticated) for all authenticated requests. This eliminates the user-controlled header issue entirely.
• mapped: Looks up the session id in a configured map (REMOTE_USER_MAP, format: id1:user1,id2:user2) and sends the mapped value. Falls back to the session id if not found in the map. This is the path to multi-user support.
• none: Omits the Remote-User header entirely. Caddy's forward_auth still accepts the request based on the 200 status code.

The RemoteUserMode enum (src/Enum/RemoteUserMode.php) encapsulates the modes. StringTrait::authSuccessResponse() resolves the header value based on the configured mode, and ConfigBag handles parsing the map string and validating the mode (invalid values fall back to session). AcceptListener now receives ConfigBag as a constructor dependency to support this.

1.3 No CSRF Protection on POST Form Login [MEDIUM PRIORITY] ✅ Addressed

Current state: Resolved through documentation and analysis. The nonce system provides CSRF protection for the POST form path: nonces are server-generated, single-use, and have a 120-second TTL. An attacker cannot forge a POST request without first loading the login page to obtain a valid nonce, which requires being on the auth subdomain. The LoginListener class docblock and login.html.twig template comment now explicitly document this CSRF protection model. The AJAX (header) path embeds the nonce in the base64url payload.

1.4 TOTP Verification Leeway May Be Too Generous [LOW PRIORITY] ✅ Addressed

Current state: Fixed. The TOTP verification window has been reduced from 10 periods (±5 minutes) to 1 period (±30 seconds). With the default 30-second TOTP period, a code is now valid for at most 90 seconds (the current window plus one window on each side), down from the previous 50 seconds per window with 10-period leeway. The ROADMAP has been updated to reflect this change.

1.5 Backup Code Logging Reveals Code Value [LOW PRIORITY] ✅ Addressed

Current state: Fixed. The debug log in BackupCodeManager::verifyAndConsume() no longer includes the backup key name. It now logs only the hit/miss and valid/invalid status: "checking backup code: HIT & VALID" or "checking backup code: miss & invalid".

1.6 TOTP Object Reconstructed on Every Verification [LOW PRIORITY] ⬜ Open

Current state: GetTotpTrait::getTotp() calls OTHP\Factory::loadFromProvisioningUri() on every invocation. This parses the OTP URI string and constructs a new TOTP object each time a token is verified.

Note: An attempt was made to memoize the TOTP object within the request cycle, but PHP 8.4's readonly class constraint prevents traits from defining mutable properties in readonly classes (LoginManager and BackupCodeManager are both final readonly). Resolving this would require either removing readonly from these classes, using a separate memoization service, or refactoring GetTotpTrait into a dedicated injectable service.

Why: This is a minor performance concern — URI parsing and TOTP object construction happen on every login attempt. In a FrankenPHP worker process that handles many requests, this adds unnecessary overhead. It's not a security issue, but it's an easy optimization if the readonly constraint is relaxed.

1.7 CSS Injection in Style Template [MEDIUM PRIORITY] ✅ Addressed

Current state: Fixed. Environment-configured color values (bg_color, fg_color, error_color) in _style.html.twig are now escaped with Twig's |e('css') filter to prevent CSS injection from malicious environment variable values.

1.8 $payload->json Access on Possibly-Null Payload [HIGH PRIORITY] ✅ Addressed

Current state: Fixed. In LoginListener::onKernelRequest(), the $payload->json access on a possibly-null $payload has been replaced with $payload?->json ?? true, and $payload->id with $payload?->id ?? ''. This prevents a crash when a login attempt is detected (e.g., via the X-Preauth header) but the payload is invalid (malformed base64, non-object JSON, etc.).

1.9 validReturn() Doesn't Check false from parse_url [MEDIUM PRIORITY] ✅ Addressed

Current state: Fixed. DomainManager::validReturn() now checks for false and empty string in addition to null when examining the return value of parse_url($url, PHP_URL_HOST). This prevents a TypeError on malformed URLs that filter_var(FILTER_VALIDATE_URL) accepts but parse_url cannot parse.

1.10 Incomplete TLD List in DomainManager [HIGH PRIORITY] ✅ Addressed

Current state: Fixed. The TLD lookup table in DomainManager has been significantly expanded with many previously missing multi-part TLDs, including .com.au, .co.jp, .com.br, .co.kr, .com.tw, .co.za, and dozens more. Without these entries, domains like evil.com.au would incorrectly match auth.example.com.au (both would resolve to base com.au), creating an open redirect vulnerability. The host is also now lowercased before TLD lookup to fix a case-sensitivity issue.

---

2. Architecture & Code Quality

2.1 Trait-Based Dependency Injection Pattern [MEDIUM PRIORITY] ⬜ Open

Current state: Several traits (HasLoggerTrait, GetTotpTrait, MakeNonceTrait) use #[Required] attribute for setter injection into readonly classes. For example, LoginManager receives $config, $logger, and $nonceCache via traits rather than through its constructor. The constructor only accepts three parameters; the rest are wired via setter methods called by the service container after construction.

Recommendation: Move these dependencies into the constructors of the classes that use them. If multiple classes share the same dependencies, that's fine — PHP constructors can accept many parameters, and it makes the dependency graph explicit. Alternatively, create a shared Dependencies value object that bundles logger, config, and nonce cache.

Why: The trait-based setter injection pattern makes it non-obvious what dependencies a class has — you have to look at both the constructor and all the traits it uses. It also creates a temporal coupling issue: the object exists in a partially-constructed state between construction and setter calls. With readonly classes, this works only because the trait properties are declared in the trait, not the class, which is a subtle language detail that could confuse future maintainers. Standard constructor injection is more explicit, testable, and conventional in Symfony.

2.2 Duplicated Cookie Logic Between LoginManager and InterceptListener [MEDIUM PRIORITY] ✅ Addressed

Current state: Resolved. The duplicated cookie name and domain selection logic has been extracted into two shared methods on CookieNameTrait:

• sessionCookieName(DomainInterface $domainManager): string — Returns the appropriate cookie name (__Host-Http-Preauth or __Http-Domain-Preauth) based on whether central auth is active.
• sessionCookieDomain(DomainInterface $domainManager, string $host): ?string — Returns the cookie domain for central auth mode, or null for single-domain mode.

LoginManager::setCookie(), AcceptListener::onKernelRequest(), and InterceptListener::pruneInvalidCookie() all now use these shared methods. The fragile "changes here must be reflected in InterceptListener::pruneInvalidCookie()" comment has been removed.

2.3 MonitorCacheKeys Instantiated Multiple Times for Same Pool [MEDIUM PRIORITY] ⬜ Open

Current state: MonitorCacheKeys is a decorator that tracks cache key changes. It's instantiated independently in PersistCache, LoginManager, and BackupCodeManager, each wrapping the same underlying CacheItemPoolInterface. The key list (__key_list) and change list (__chg_list) are stored in the cache itself, so the instances share state — but each instance calls initialize() in its constructor if the lists don't exist yet, and each save()/saveDeferred() call triggers additional metadata writes.

Recommendation: Register MonitorCacheKeys as a decorated service in the DI container (using Symfony's decorates feature) so there's a single instance per cache pool. Or, make MonitorCacheKeys a stateless service that's injected once, rather than having each consumer create its own wrapper.

Why: Multiple instances wrapping the same pool is wasteful — each save() call triggers a cascade of metadata operations (update key list, log change, commit). With three instances, a single cache write could trigger nine additional cache operations. A single decorator service would be more efficient and would make the lifecycle clearer.

2.4 Payload Base64url Decoding Has Broken Padding [MEDIUM PRIORITY] ✅ Already Correct

Current state: Not an issue. The code correctly uses:

$base64 = strtr($base64url, '-_', '+/');
$base64 .= str_repeat('=', (4 - strlen($base64) % 4) % 4);

This was fixed in a prior commit ("Fix docs, add .dockerignore, fix base64url padding, fix typo"). The original review incorrectly reported the use of str_pad; the implementation now correctly uses str_repeat to add the proper number of = padding characters.

2.5 Symfony Sessions Enabled But Unused [LOW PRIORITY] ✅ Addressed

Current state: Fixed. config/packages/framework.yaml now has session: false with a comment explaining that preauth implements its own cookie/cache-based session management and does not use Symfony's session subsystem.

2.6 config/reference.php Committed to Repository [LOW PRIORITY] ✅ Already Handled

Current state: Not an issue. config/reference.php is already listed in .gitignore under the project-specific section and is not tracked in version control.

2.7 Public Properties on Payload DTO [LOW PRIORITY] ⬜ Open

Current state: Payload uses public properties ($id, $token, $nonce, $json, $scope) with no encapsulation. The object is mutable after construction.

Recommendation: Consider making Payload a readonly class (PHP 8.4+ supports readonly classes natively) with a constructor that takes all fields, or use Symfony's Stringable/value object patterns. Since LoginManager mutates $payload->scope (downgrading IP to Cookie), the current design requires mutability — but this could be handled by returning a new instance instead.

Why: Immutable DTOs are safer to pass around, especially in an event-driven system where the same object might be referenced by multiple listeners. The current mutation in LoginManager::checkToken() (changing $payload->scope) is a side effect that's not obvious from the method signature.

2.8 Duplicated "hi $id" Response Construction [LOW PRIORITY] ✅ Addressed

Current state: Fixed. The duplicated new Response("hi $id", headers: ['Content-Type' => 'text/plain', 'Remote-User' => $id]) pattern in AcceptListener, AllowListener, and LoginManager has been extracted into StringTrait::authSuccessResponse(string $id): Response, which all three classes now use.

2.9 Duplicated Constants [LOW PRIORITY] ✅ Addressed

Current state: Fixed. The duplicated '2999-12-31' far-future date string (previously in Utilities::makeTotp() and BackupCodeManager::verifyAndConsume()/saveCodes()) and the 128 max input length (previously in StringTrait::makeCacheKey() and Payload::create()) have been extracted into AppConstants::FAR_FUTURE_DATE and AppConstants::MAX_INPUT_LENGTH respectively.

---

3. Testing

3.1 No Tests for Concurrent Access / Race Conditions [LOW PRIORITY] ⬜ Open

Current state: The test suite is excellent — 222 tests, 100% coverage, good edge case coverage. However, there are no tests for concurrent access scenarios, such as two requests using the same nonce simultaneously, or cache initialization race conditions in MonitorCacheKeys.

Recommendation: Add a few integration tests that simulate concurrent access (e.g., using process forks or mock caches with delays). At minimum, document that concurrent access is expected to be handled by APCu's atomic operations.

Why: MonitorCacheKeys::initialize() checks if key lists exist and creates them if not — under concurrent startup, two instances could both see missing lists and both call initialize(). This is likely fine because APCu operations are atomic, but it's worth having a test or at least a documented assumption. The race condition between hasItem() and getItem() in AcceptListener and AllowListener is now handled with an isHit() check, but is not tested.

3.2 No Security-Focused Test Suite [LOW PRIORITY] ⬜ Open

Current state: Security behaviors (nonce replay, backup code reuse, rate limiting) are tested as part of the functional and unit tests, but there's no dedicated security test suite that systematically probes for common vulnerabilities.

Recommendation: Consider adding a tests/Security/ directory with tests for: XSS attempts in the username field, header injection via the return parameter, cookie attribute verification (Secure, HttpOnly, SameSite), and response header presence (now that security headers are added).

Why: For an authentication gateway, security testing deserves its own focused suite that's easy to find and extend. This also makes it easier for security reviewers to understand what's been tested.

---

4. Docker & Deployment

4.1 Healthcheck Depends on curl Which May Not Be Installed [MEDIUM PRIORITY] ✅ Addressed

Current state: Fixed. The Dockerfile now explicitly installs curl in the final image with apt-get install -y --no-install-recommends curl and cleans up the apt lists to keep the image small.

4.2 Typo in bin/franken.sh [LOW PRIORITY] ✅ Already Fixed / Addressed

Current state: Fixed. The typo (digtialadapt → digitaladapt) was corrected in a prior commit. The script has since been further improved: the hardcoded APP_SECRET has been removed (now uses the APP_SECRET environment variable or generates a random secret), and the docker container rm command now suppresses errors when the container doesn't exist.

4.3 No .dockerignore File [LOW PRIORITY] ✅ Already Handled

Current state: Not an issue. A .dockerignore file exists and excludes .git/, .gitignore, var/, vendor/, tests/, .phpunit.cache/, docs/, *.md, .env, .env.test, .env.local, and composer.phar from the Docker build context. This was added in a prior commit.

4.4 Dockerfile Uses PHP 8.5 Which Is Bleeding Edge [LOW PRIORITY] ⬜ Open (Deliberate)

Current state: The Dockerfile uses php:8.5-trixie for the build stage and dunglas/frankenphp:php8.5-trixie for the final image. composer.json requires php >= 8.4. The CI workflow in tests.yaml also uses PHP 8.5.

Recommendation: This is a deliberate choice and likely fine for a personal project. If broader compatibility is desired, consider testing against both PHP 8.4 and 8.5 in CI. The composer.json already allows 8.4+.

---

5. Error Handling

5.1 Cache Exceptions Propagate as 500 Errors [MEDIUM PRIORITY] ✅ Addressed

Current state: Fixed. Cache operations in AcceptListener::onKernelRequest() and AllowListener::onKernelRequest() are now wrapped in try/catch blocks that catch Psr\Cache\InvalidArgumentException. On a cache error, the listener logs the error at error level and returns without setting a response — causing the request to fall through to the next listener, which will eventually present the login page. This is a "fail closed" approach: if the cache is unavailable, the user is not authenticated.

Note: LoginManager::checkToken() and BackupCodeManager::verifyAndConsume() still declare @throws InvalidArgumentException. These are called from LoginListener, which does not catch the exception. A cache failure during login verification would still result in a 500 error. This is a lower-priority concern since login failures already result in a 401 response path.

5.2 No Global Exception Handling for Auth Flow [LOW PRIORITY] ⬜ Open

Current state: There is no ExceptionListener or ErrorController configured. Symfony's default error handling will produce a generic error page for uncaught exceptions. In dev mode (APP_DEBUG=1), this shows a full stack trace.

Recommendation: Add a simple exception listener that catches exceptions from the auth flow and returns a clean 401 or 503 response with the login page or error template. Alternatively, configure framework.error_controller to use a custom controller that renders the error template.

Why: For an auth gateway, every response should be intentional. A raw Symfony error page (even in production mode) doesn't match the styled login/error pages and could leak information about the internal architecture.

5.3 Kernel::terminate() Not Using try/finally [MEDIUM PRIORITY] ✅ Addressed

Current state: Fixed. Kernel::terminate() now wraps $this->persistCache->persist() in a try block with a finally block that calls parent::terminate(). This ensures that the Symfony kernel termination always runs, even if the cache persistence throws an exception.

---

6. Frontend

6.1 document.write() in Login Script [MEDIUM PRIORITY] ✅ Addressed

Current state: Fixed. The document.open(); document.write(html); document.close(); pattern in _script.html.twig has been replaced with document.documentElement.innerHTML = html;. This avoids the deprecated document.write() call and is compatible with the Content-Security-Policy now set by SecurityHeadersListener.

6.2 No Input Sanitization in Username Echo [LOW PRIORITY] ⬜ Open

Current state: In login.html.twig, the username is echoed back into the input value: value="{{ username }}". The username comes from the sanitized makeCacheKey() output, which restricts to [A-Za-z0-9_.], so HTML injection is not possible with the current sanitization. Twig's auto-escaping is also on by default.

Recommendation: Add Twig's escape filter explicitly for defense-in-depth: value="{{ username|e('html_attr') }}". Also consider whether the message variable in <p id="preauth-message">{{ message|default }}</p> could ever contain user input.

Why: While the current sanitization prevents XSS, relying on makeCacheKey() for HTML safety is an implicit coupling between cache key logic and output safety. If makeCacheKey() were ever relaxed to allow more characters, the template would become vulnerable. Twig auto-escaping handles HTML body context, but html_attr escaping is more appropriate for attribute contexts.

---

7. Configuration

7.1 No Validation of Environment Variables [LOW PRIORITY] ✅ Addressed

Current state: Fixed. Environment variables in config/services.yaml now use Symfony's env var processors for type casting:

• app.cookie_ttl: '%env(int:COOKIE_TTL)%'
• app.subdomain_redirect: '%env(bool:SUBDOMAIN_REDIRECT)%'
• app.ip_ttl: '%env(int:IP_TTL)%'
• app.teapot: '%env(bool:TEAPOT)%'

This ensures invalid values fail fast at container compilation rather than at runtime with a confusing type error. The rate_limiter.yaml already used %env(int:...)% — this pattern is now applied consistently.

7.2 APP_SECRET Not Used Meaningfully [LOW PRIORITY] ✅ Addressed (Documented)

Current state: APP_SECRET is configured in framework.yaml and is required by Symfony. Preauth doesn't use Symfony sessions (now explicitly disabled), CSRF tokens, or signed cookies — the main uses of APP_SECRET. The README now documents that APP_SECRET is a Symfony requirement and that session cookies are random ULIDs looked up in cache, not signed tokens. The hardcoded APP_SECRET in bin/franken.sh has also been removed.

---

8. Documentation

8.1 Missing Security Model Documentation [MEDIUM PRIORITY] ✅ Addressed

Current state: Addressed. The README now includes a comprehensive "Security Model" section under "Architecture" that covers:

• Cookie security attributes (__Host- prefix, SameSite=Strict, Secure, HttpOnly)
• Nonce system (15-byte random, single-use, 120s TTL)
• TOTP verification window (±1 period / ±30 seconds)
• Backup codes (case-insensitive, single-use, alphanumeric)
• Rate limiting (per-IP, compound sliding window, cannot be disabled)
• Security headers (CSP, X-Frame-Options, X-Content-Type-Options, Referrer-Policy, HSTS)

A dedicated docs/SECURITY.md with the full threat model and Remote-User guidance (see item 1.2) could still be valuable as a standalone document.

8.2 Missing CHANGELOG.md [MEDIUM PRIORITY] ✅ Addressed

Current state: Fixed. A CHANGELOG.md has been created following the Keep a Changelog format, with full version history from v0.0.1 through the unreleased v1.0 changes. The version history was previously inline in the README.

8.3 Missing CONTRIBUTING.md [LOW PRIORITY] ✅ Addressed

Current state: Fixed. A CONTRIBUTING.md has been created with development setup instructions, code style guidelines, testing requirements, PR process, commit message conventions, and architecture overview.

8.4 Stale Branch References in ROADMAP [LOW PRIORITY] ✅ Addressed

Current state: Fixed. The ROADMAP's branch status table has been updated to reflect that all feature branches have been pruned and development uses a feature-branch + PR workflow into main. Completed security review items are now checked off, and the TOTP leeway description has been updated from "10-second leeway" to "±1 period leeway (±30 seconds)".

---

9. CI & Workflows

9.1 Inconsistent Tag Format [MEDIUM PRIORITY] ✅ Addressed

Current state: Fixed. Git tags are now standardized on the v prefix (e.g., v1.0.0 instead of 1.0.0). The Docker workflow (docker.yaml) now triggers on v*.*.* tag patterns and includes a step to extract the version number without the v prefix for the Docker image tag. The existing un-prefixed tags (0.7.0 through 0.10.0) remain in the repository but all future releases will use the v prefix.

9.2 Stale develop Branch in CI Triggers [LOW PRIORITY] ✅ Addressed

Current state: Fixed. The tests.yaml and develop.yaml workflows no longer reference the develop branch, which has been pruned. CI now triggers on main only (for push) and main only (for pull requests).

9.3 publish.yaml Fails on Re-run [LOW PRIORITY] ✅ Addressed

Current state: Fixed. The GitHub sync workflow (publish.yaml) now uses git remote add ... 2>/dev/null || git remote set-url ... instead of bare git remote add, which would fail if the remote already existed from a previous run.

---

What's Done Well

• Listener-based architecture is a good fit for this use case — each listener has a single responsibility, and the priority chain creates a clear request processing pipeline.
• Cookie security is excellent: __Host- prefix, Secure, HttpOnly, SameSite=Strict, and a separate non-prefixed cookie for domain-scoped central auth. Cookie name and domain selection logic is now shared via CookieNameTrait::sessionCookieName() and sessionCookieDomain().
• Nonce-based replay protection with single-use, TTL-limited nonces and collision retry is well-designed. The nonce also serves as CSRF protection for the POST form path.
• Rate limiting with compound sliding windows (burst + sustained) and the humorous teapot option is practical and well-implemented.
• Test suite is exemplary: 100% coverage, good use of test helpers, functional tests that exercise the full kernel, and edge cases like ULID collisions and nonce reuse.
• Dual-layer cache (APCu + filesystem with change tracking) is a clever solution for persistence without a database.
• Backup code system with single-use enforcement, case-insensitivity, and audit trail (keeping consumed codes with false value) is well thought out. Backup code values are no longer logged.
• Interfaces (LoginInterface, DomainInterface, BackupCodeInterface) enable clean mocking in tests. All now have declare(strict_types=1).
• FrankenPHP worker mode via the Caddyfile and Dockerfile is a modern, performant serving strategy.
• Security headers are now set on all responses via SecurityHeadersListener.
• Error handling in cache-dependent listeners now fails closed (denies access on cache errors) rather than propagating 500 errors.

---

Originally prepared as a design review. Updated to reflect the state of the fix/v1.0-must-fix branch.