public/preauth
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: v1.0 release — security hardening, code quality, and documentation #4

Merged
andrew merged 6 commits from fix/v1.0-must-fix into main 2026-08-12 22:32:00 -04:00
Conversation 1 Commits 6 Files Changed 49
+1499 -187
lyra commented 2026-08-12 08:43:56 -04:00
Member
Copy Link
Copy Source

Summary

This PR addresses all must-fix, should-fix, and nice-to-have items from the v1.0 design review. It encompasses 5 commits covering security hardening, code quality improvements, CI/CD fixes, and comprehensive documentation.

Tests: 241 passing (498 assertions) · Linting: PHP CS Fixer clean (57/57 files) · Working tree: clean

Changes

Security (must-fix)

  • Configurable Remote-User header (design consideration 1.2) — four modes via REMOTE_USER env var: session (default/backward-compatible), static, mapped, none. New RemoteUserMode enum, ConfigBag parsing/validation, AcceptListener integration.
  • Security response headers (1.1) — new SecurityHeadersListener sets X-Content-Type-Options, X-Frame-Options, Content-Security-Policy, Referrer-Policy, Strict-Transport-Security on all responses.
  • CSS injection prevention (1.7) — |e('css') filter on environment-configured color values in _style.html.twig.
  • document.write() removal (6.1) — replaced with document.documentElement.innerHTML for CSP compliance.
  • TOTP leeway reduced (1.4) — from 10 periods (±5 min) to 1 period (±30 sec).
  • Backup code logging (1.5) — no longer includes code values in debug output.
  • Expanded TLD list (1.10) — many missing multi-part TLDs added (.com.au, .co.jp, .com.br, etc.) to prevent open redirect vulnerabilities. Host lowercased before TLD lookup.
  • Null-safe payload access (1.8) — $payload?->json and $payload?->id in LoginListener to prevent crash on invalid payload.
  • validReturn() fix (1.9) — checks false from parse_url() to prevent TypeError on malformed URLs.
  • CSRF documentation (1.3) — documented that the nonce system provides CSRF protection for POST form logins.
  • Removed hardcoded APP_SECRET from bin/franken.sh.
  • Added .env to .gitignore.

Code Quality (should-fix / nice-to-have)

  • Shared cookie logic (2.2) — extracted CookieNameTrait::sessionCookieName() and sessionCookieDomain(); removed fragile duplication comment.
  • Shared auth success response (2.8) — StringTrait::authSuccessResponse() replaces duplicated new Response("hi $id", ...) pattern.
  • Shared constants (2.9) — AppConstants::FAR_FUTURE_DATE and AppConstants::MAX_INPUT_LENGTH.
  • Cache error handling (5.1) — try/catch in AcceptListener and AllowListener (fail-closed on cache errors).
  • Kernel::terminate() (5.3) — try/finally ensures parent::terminate() always runs.
  • Symfony sessions disabled (2.5) — session: false in framework.yaml with explanatory comment.
  • Env var validation (7.1) — int:/bool: env processors for type casting.
  • declare(strict_types=1) on all interfaces.
  • #[AsCommand] attribute on GenerateBackupCodesCommand.
  • Input validation on GenerateBackupCodesCommand (rejects count < 1).
  • Response::HTTP_INTERNAL_SERVER_ERROR constant instead of literal 500.

CI/CD

  • Standardized git tag format to v*.*.* prefix.
  • Removed stale develop branch from CI triggers.
  • Fixed publish.yaml remote re-add failure on re-runs.
  • Explicitly install curl in Docker final image.

Documentation

  • CHANGELOG.md — full version history in Keep a Changelog format.
  • CONTRIBUTING.md — development setup, code style, testing, PR process.
  • DESIGN_CONSIDERATIONS.md — complete design review with 22/29 items addressed, 7 intentionally left open as future work.
  • README.md — comprehensive Security Model section, updated setup/config docs.
  • ROADMAP.md — updated branch status, TOTP leeway description, completed items.

New Tests

  • ConfigBagRemoteUserTest — Remote-User mode parsing and map validation.
  • SecurityHeadersListener coverage in AcceptListenerTest.
  • StringTraitTest — authSuccessResponse() and makeCacheKey() edge cases.
  • Additional DomainManagerTest cases for expanded TLD list.

Open Items (Future Work, Not v1.0 Blockers)

7 items are intentionally left open — all are low/medium priority architectural improvements:

  • 1.6: TOTP object memoization (blocked by PHP 8.4 readonly class constraint)
  • 2.1: Trait-based DI → constructor injection refactor
  • 2.3: MonitorCacheKeys as a decorated service
  • 2.7: Payload DTO immutability
  • 3.1: Concurrent access / race condition tests
  • 3.2: Dedicated security test suite
  • 5.2: Global exception listener for auth flow

See DESIGN_CONSIDERATIONS.md for details on each.

## Summary This PR addresses all must-fix, should-fix, and nice-to-have items from the v1.0 design review. It encompasses 5 commits covering security hardening, code quality improvements, CI/CD fixes, and comprehensive documentation. **Tests:** 241 passing (498 assertions) · **Linting:** PHP CS Fixer clean (57/57 files) · **Working tree:** clean --- ## Changes ### Security (must-fix) - **Configurable `Remote-User` header** (design consideration 1.2) — four modes via `REMOTE_USER` env var: `session` (default/backward-compatible), `static`, `mapped`, `none`. New `RemoteUserMode` enum, `ConfigBag` parsing/validation, `AcceptListener` integration. - **Security response headers** (1.1) — new `SecurityHeadersListener` sets `X-Content-Type-Options`, `X-Frame-Options`, `Content-Security-Policy`, `Referrer-Policy`, `Strict-Transport-Security` on all responses. - **CSS injection prevention** (1.7) — `|e('css')` filter on environment-configured color values in `_style.html.twig`. - **`document.write()` removal** (6.1) — replaced with `document.documentElement.innerHTML` for CSP compliance. - **TOTP leeway reduced** (1.4) — from 10 periods (±5 min) to 1 period (±30 sec). - **Backup code logging** (1.5) — no longer includes code values in debug output. - **Expanded TLD list** (1.10) — many missing multi-part TLDs added (`.com.au`, `.co.jp`, `.com.br`, etc.) to prevent open redirect vulnerabilities. Host lowercased before TLD lookup. - **Null-safe payload access** (1.8) — `$payload?->json` and `$payload?->id` in `LoginListener` to prevent crash on invalid payload. - **`validReturn()` fix** (1.9) — checks `false` from `parse_url()` to prevent `TypeError` on malformed URLs. - **CSRF documentation** (1.3) — documented that the nonce system provides CSRF protection for POST form logins. - Removed hardcoded `APP_SECRET` from `bin/franken.sh`. - Added `.env` to `.gitignore`. ### Code Quality (should-fix / nice-to-have) - **Shared cookie logic** (2.2) — extracted `CookieNameTrait::sessionCookieName()` and `sessionCookieDomain()`; removed fragile duplication comment. - **Shared auth success response** (2.8) — `StringTrait::authSuccessResponse()` replaces duplicated `new Response("hi $id", ...)` pattern. - **Shared constants** (2.9) — `AppConstants::FAR_FUTURE_DATE` and `AppConstants::MAX_INPUT_LENGTH`. - **Cache error handling** (5.1) — try/catch in `AcceptListener` and `AllowListener` (fail-closed on cache errors). - **Kernel::terminate()** (5.3) — `try/finally` ensures `parent::terminate()` always runs. - **Symfony sessions disabled** (2.5) — `session: false` in `framework.yaml` with explanatory comment. - **Env var validation** (7.1) — `int:`/`bool:` env processors for type casting. - `declare(strict_types=1)` on all interfaces. - `#[AsCommand]` attribute on `GenerateBackupCodesCommand`. - Input validation on `GenerateBackupCodesCommand` (rejects count < 1). - `Response::HTTP_INTERNAL_SERVER_ERROR` constant instead of literal `500`. ### CI/CD - Standardized git tag format to `v*.*.*` prefix. - Removed stale `develop` branch from CI triggers. - Fixed `publish.yaml` remote re-add failure on re-runs. - Explicitly install `curl` in Docker final image. ### Documentation - **CHANGELOG.md** — full version history in Keep a Changelog format. - **CONTRIBUTING.md** — development setup, code style, testing, PR process. - **DESIGN_CONSIDERATIONS.md** — complete design review with 22/29 items addressed, 7 intentionally left open as future work. - **README.md** — comprehensive Security Model section, updated setup/config docs. - **ROADMAP.md** — updated branch status, TOTP leeway description, completed items. ### New Tests - `ConfigBagRemoteUserTest` — Remote-User mode parsing and map validation. - `SecurityHeadersListener` coverage in `AcceptListenerTest`. - `StringTraitTest` — `authSuccessResponse()` and `makeCacheKey()` edge cases. - Additional `DomainManagerTest` cases for expanded TLD list. --- ## Open Items (Future Work, Not v1.0 Blockers) 7 items are intentionally left open — all are low/medium priority architectural improvements: - 1.6: TOTP object memoization (blocked by PHP 8.4 readonly class constraint) - 2.1: Trait-based DI → constructor injection refactor - 2.3: MonitorCacheKeys as a decorated service - 2.7: Payload DTO immutability - 3.1: Concurrent access / race condition tests - 3.2: Dedicated security test suite - 5.2: Global exception listener for auth flow See `DESIGN_CONSIDERATIONS.md` for details on each.
lyra added 5 commits 2026-08-12 08:43:56 -04:00
fix: must-fix items for v1.0 release d2eb914637 
Security:
- Add SecurityHeadersListener (X-Content-Type-Options, X-Frame-Options,
  CSP, Referrer-Policy, HSTS)
- Replace document.write() with document.documentElement.innerHTML
  in login JS to avoid CSP violations
- Add CSS escaping (|e('css')) to env color values in _style.html.twig
- Document CSRF protection model: nonce serves as CSRF token for POST
  form path (single-use, server-generated, 120s TTL)
- Reduce TOTP verification window from 10 periods (±5 min) to 1 (±30s)
- Remove hardcoded APP_SECRET from bin/franken.sh (now uses env or
  generates random)
- Remove backup code values from debug log output
- Add .env to .gitignore

Bug fixes:
- Fix ->json access on possibly-null  in LoginListener
  (uses null-safe operator ?->)
- Fix validReturn() not checking false from parse_url (could cause
  TypeError on malformed URLs)
- Add isHit() race condition check in AcceptListener and AllowListener
- Add try/finally in Kernel::terminate() so parent::terminate() always
  runs even if persist() throws
- Add input validation to GenerateBackupCodesCommand (reject count < 1)
- Use Response::HTTP_INTERNAL_SERVER_ERROR constant in GetTotpTrait
  instead of literal 500

Docker/CI:
- Explicitly install curl in Docker final image (needed for healthcheck)
- Update workflow tag pattern to v*.*.* (standardize on v-prefix)
- Extract version without v-prefix for Docker image tag
- Remove stale develop branch from CI triggers
- Fix publish.yaml git remote add to use set-url on re-runs

Code quality:
- Add declare(strict_types=1) to all interface files
- Add #[AsCommand] attribute to GenerateBackupCodesCommand
- Fix BackupCodeInterface default count to match implementation (10)
- Lowercase host before TLD lookup in DomainManager
- Expand TLD list with many missing multi-part TLDs (.com.au, .co.jp,
  .com.br, .co.kr, .com.tw, .co.za, etc.) to prevent open redirect
  vulnerabilities
- Disable unused Symfony sessions in framework.yaml

Tests:
- Update DomainManagerTest for corrected TLD parsing (.com.au, .co.jp,
  .com.br now correctly recognized as multi-part)
- Update GetTotpTraitTest for corrected error message
- Update GenerateBackupCodesCommandTest: zero count now throws exception
fix: should-fix items for v1.0 release b89070e985 
Documentation:
- Create CHANGELOG.md with full version history (v0.0.1 through unreleased)
- Rewrite README with comprehensive setup guide, configuration reference,
  architecture overview, security model, and feature list
- Update ROADMAP.md: fix branch status table, mark completed security
  review items, update TOTP leeway description
- Fix 'centeral' typo in docs/Caddyfile
- Remove TODO comment from docs/compose.yaml
- Add DESIGN_CONSIDERATIONS.md (design review document)

Code quality:
- Extract duplicated cookie name/domain logic into CookieNameTrait
  methods: sessionCookieName() and sessionCookieDomain()
- Update AcceptListener, AllowListener, InterceptListener, and
  LoginManager to use the shared methods
- Remove fragile cross-file coupling comment between LoginManager
  and InterceptListener

Error handling:
- Wrap cache operations in AcceptListener and AllowListener with
  try/catch to fail closed (don't authenticate on cache errors)
- Log cache errors at error level instead of propagating as 500s
- Early return pattern in AcceptListener and AllowListener for
  cleaner control flow
chore: nice-to-have improvements for v1.0
Sync GitHub / sync (push) Successful in 8s
Details
408d75dda1 
Code quality:
- Create AppConstants class with shared constants:
  - FAR_FUTURE_DATE (replaces duplicated '2999-12-31' strings)
  - MAX_INPUT_LENGTH (replaces duplicated 128 in Payload and StringTrait)
- Extract duplicated 'hi $id' response body into StringTrait::authSuccessResponse()
  method, used by AcceptListener, AllowListener, and LoginManager
- Add missing @throws InvalidArgumentException annotations to
  MonitorCacheKeys (getItem, hasItem, deleteItem, deleteItems, commit)

Configuration:
- Add proper env var type casting in services.yaml:
  - COOKIE_TTL → env(int:)
  - SUBDOMAIN_REDIRECT → env(bool:)
  - IP_TTL → env(int:)
  - TEAPOT → env(bool:)

Documentation:
- Create CONTRIBUTING.md with development setup, code style,
  testing guidelines, and PR process
docs: update DESIGN_CONSIDERATIONS.md to reflect addressed items
Sync GitHub / sync (push) Successful in 8s
Details
2258839bd6 
- Mark all resolved items with ✅ and describe the fix applied
- Mark remaining open items with ⬜ and keep recommendations
- Add new sections for items discovered during the fix work:
  - 1.7 CSS injection in style template
  - 1.8 ->json null safety
  - 1.9 validReturn() parse_url false check
  - 1.10 Incomplete TLD list
  - 2.8 Duplicated response construction
  - 2.9 Duplicated constants
  - 5.3 Kernel::terminate() try/finally
  - 9. CI & Workflows (tag format, stale branches, publish.yaml)
- Update 'What's Done Well' to reflect new improvements
- Add summary noting this is a living document tracking the
  fix/v1.0-must-fix branch state
feat: configurable Remote-User header (design consideration 1.2)
Sync GitHub / sync (push) Successful in 7s
Details
Tests / test (pull_request) Successful in 58s
Details
9ad54f8e2a 
Add REMOTE_USER env var with four modes:
- session (default): sends session id, backward-compatible
- static: sends a fixed string (REMOTE_USER_STATIC)
- mapped: looks up session id in REMOTE_USER_MAP
- none: omits the header entirely

New RemoteUserMode enum, ConfigBag parsing/validation, and
StringTrait::authSuccessResponse resolves the header value based
on the configured mode. AcceptListener now receives ConfigBag as
a constructor dependency.

Addresses design consideration 1.2 (Remote-User header value is
user-controlled) from DESIGN_CONSIDERATIONS.md.

241 tests pass, 0 cs-fixer violations.
lyra added 1 commit 2026-08-12 11:21:42 -04:00
fix: restore develop branch trigger support for CI workflows
Tests / test (pull_request) Successful in 1m1s
Details
Push Develop / docker (push) Successful in 6m29s
Details
Sync GitHub / sync (push) Successful in 10s
Details
Tests / test (push) Successful in 1m0s
Details
44e4c60f80 
Re-adds 'develop' to push triggers in develop.yaml and tests.yaml
so the :develop Docker image can be built from the develop branch,
enabling dev testing without requiring a merge to main.
andrew approved these changes 2026-08-12 22:31:44 -04:00
andrew merged commit 72c41fec77 into main 2026-08-12 22:32:00 -04:00
Sign in to join this conversation.
Reviewers
No Reviewers
andrew
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
andrew (Andrew) lyra
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: public/preauth#4