feat(vps): port auth middleware to HttpApiMiddleware (Step 3b)#150
Open
guidefari wants to merge 2 commits into
Open
feat(vps): port auth middleware to HttpApiMiddleware (Step 3b)#150guidefari wants to merge 2 commits into
guidefari wants to merge 2 commits into
Conversation
Step 3a of the Hono -> Effect HttpApi migration
(docs/migration-effect-http-api.md). First real HttpApi group taking
over live traffic from the Hono fallback -- health was chosen first
because it's small and low-risk to get wrong.
- packages/api/src/api.ts: composed Api = HttpApi.make('gbfm').add(HealthGroup).
- apps/vps/src/http/health.handlers.ts: HttpApiBuilder.group implementation.
Readiness is cached for 5s via Effect.cachedWithTTL, injected as a
dependency so tests can force the failure path without a real DB outage.
- apps/vps/src/http/routes.ts: HealthLive mounted alongside the existing
fallback/auth routes; AppLoggerLive provided so handler errors reach the
app's real Pino+Sentry logger, not Effect's bare default logger.
- apps/vps/src/app.ts: deleted the old Hono health routes and their
module-level cache/error-class scaffolding.
- Deleted apps/vps/src/health.blackbox.test.ts (tested the Hono app
directly; those routes no longer exist there). Equivalent coverage
now lives in routes.blackbox.test.ts against the new handler.
Two real bugs were found and fixed via adversarial review before this
was pushed anywhere (subagent review, not self-review):
1. The first implementation used a module-level readinessCache variable
with a check-then-write race -- concurrent requests on a cold cache
could each independently hit the DB, and writes could land out of
order under a flapping DB. Replaced with Effect.cachedWithTTL, which
memoizes the in-flight fiber so concurrent callers share one
computation. Verified empirically (not just by reading the source):
50-way concurrent stress through the real handler stays at exactly
1 DB check; failures are cached the same way successes are; TTL
expiry still triggers a fresh check afterward.
2. The DB check's real error (its cause) was being discarded when
converting to ReadinessCheckFailedError, with no logging --
equivalent to the old HealthCheckError's cause field vanishing.
Fixed by logging the cause via Effect.tapError before mapping to
the wire-safe tagged error (the cause itself must never reach the
response body -- a public /health endpoint leaking internal DB
error detail would be an information-disclosure issue). A follow-up
review caught that this log wasn't reaching the app's actual
Pino+Sentry pipeline (AppLoggerLive wasn't provided in the router's
layer chain) -- fixed and verified end-to-end against a genuinely
unreachable DB.
health.handlers.failure.test.ts is a separate file from
routes.blackbox.test.ts specifically to test the failure/concurrency
paths without interference from the happy-path tests -- not because
the cache is shared module state anymore (it isn't; each
createWebHandler call gets its own cache), but because it's a natural
home for the failure-path assertions.
Full apps/vps suite: 141/141 passing, 15 files.
Step 3b of the Hono -> Effect HttpApi migration
(docs/migration-effect-http-api.md). Cookie-based session auth,
validated against a scratch endpoint with no production traffic
(GET /api/internal/whoami) before any real authed route depends on
it in step 4+.
- packages/api/src/middleware/auth.ts: AuthSession (Context.Service
carrying user/session) and AuthMiddleware (HttpApiMiddleware.Service
providing AuthSession, failing HttpApiError.Unauthorized).
- apps/vps/src/middleware/auth.impl.ts: calls auth.api.getSession
exactly like the old Hono betterAuthMiddleware. Logs unauthorized
attempts and genuine getSession errors (distinct from "no session")
through the app's real logger, matching the old middleware's audit
trail and the step 3a precedent for not swallowing error causes.
- packages/api/src/internal.ts + apps/vps/src/http/internal.handlers.ts:
the scratch whoami endpoint, gated by .middleware(AuthMiddleware).
- apps/vps/src/http/routes.ts: wires AuthMiddlewareLive and the
internal group's handlers into the same router already serving
/health* and the Hono fallback.
A genuinely sharp beta-API bug, found and fixed before pushing:
HttpApiMiddleware.Service<AuthMiddleware, { provides: typeof AuthSession }>
compiles fine but silently breaks the type-level machinery that's
supposed to exclude AuthSession from a middleware-gated handler's
inferred requirements. The metadata Provides<A> reads only exists on
a Context.Service class's INSTANCE type, not its static/typeof type --
so `typeof AuthSession` resolves to `never`, nothing gets excluded,
and AuthSession leaks all the way out to toWebHandler's returned
handler function, silently changing its signature to require an
extra context argument. This surfaced as confusing "Expected 2
arguments, but got 1" errors at unrelated call sites, not at the
real fault. Fixed by using the bare class reference (`provides:
AuthSession`), confirmed against a real type-test in the vendored
effect source (.repos/effect .../typetest/unstable/httpapi/HttpApiBuilder.tst.ts)
and verified empirically by revealing the actual inferred Layer
requirement type before and after the fix. Doc updated with the
corrected pattern and an explanation of why the wrong form compiles
silently.
Adversarial subagent review (two rounds) found two more real issues,
both fixed here:
1. getSession failures were silently converted to 401 with no
logging -- same bug class as step 3a's checkDatabase, this time
in auth. Fixed with the same log-the-cause pattern. Verified
empirically against an unreachable DB; also discovered (and noted
in the code comment) that better-auth swallows DB errors during
session lookup internally and returns null rather than throwing,
so this specific scenario looks identical to "no session" in both
the old Hono path and here -- not a regression, a shared blind
spot neither implementation can see past.
2. The old Hono middleware's audit logging (unauthorized attempts
with path/method/ip) wasn't carried over. Added.
Full apps/vps suite: 143/143 passing, 16 files, including two new
tests proving the 401 path against a real HttpApiEndpoint (no
cookie, and an invalid cookie). The 200/authenticated path isn't
covered -- no existing test infra in this repo creates a real
better-auth session, and building that harness is bigger than this
step's scope.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this exists: auth is the hardest, highest-stakes piece of this whole migration -- it needs to work correctly before anything real depends on it. This validates cookie-based session auth on a throwaway endpoint nothing in production calls, so a mistake here is cheap to find and fix now instead of expensive to find later on a real authed route.
Stacked on #149 (merge #142 -> ... -> #149 first).
Process note
Same as #149: ran adversarial subagent review before pushing. Found and fixed a genuinely sharp beta-API type bug (a form that compiles fine but silently breaks middleware-provided-service exclusion -- verified against a real type-test in the vendored effect source, not just reasoned about), plus two logging/audit gaps matching the pattern from #149's health review. Also did a good-faith attempt to reproduce the "DB down during session lookup" scenario and learned something real in the process: better-auth swallows that internally and returns null rather than throwing, so it's indistinguishable from "no session" in both the old code and the new -- not a regression, just an existing blind spot. Noted in the code rather than glossed over.
Verification
Scope note
The 200/authenticated path isn't tested here -- this repo has no existing harness for creating a real better-auth session in tests, and building one is bigger than this step's job (validating rejection, not building session-creation tooling).