feat(auth): add SMTP test-send and instance-wide session limits (RUK-290) - #4
Merged
Conversation
RUK-290, first half. The backend already accepts `remember_me` on the two
built-in sign-in endpoints and ignores it ("session modes are a separate
change" — that change is this ticket). The frontend has been sending nothing,
so Go read the zero value and every session got the same lifetime. This starts
sending the flag truthfully; the backend still decides what it grants.
Chain from `authorize` to the wire, and why it is spelled out so carefully:
five of its sites are object literals rather than types, and widening a
signature never forces a literal to pass the new field. A missed site produces
no type error and no runtime error — the checkbox simply stops working for
everyone, indistinguishable from a user who never ticked it. Three sites land
here (the provider's `credentials` declaration, both `authorize` returns, both
exchange call literals); the UI half follows.
Three decisions worth keeping:
- `authorize` parses fail-closed: only the exact string `"true"` is a yes.
NextAuth carries credentials as strings and `Boolean("false") === true`, so
the obvious coercion would hand long sessions to people who deliberately
unticked the box.
- Normalisation happens once, in `runBuiltInSignIn`. `interface User` must keep
the field optional (Google and dev-bypass share that type and return no such
field), but everything below takes a plain boolean. A `?? false` at the
exchange instead would make the contract test pass whether or not the value
ever arrived.
- The body key is written unconditionally, never `...(x ? {k:v} : {})`. That
idiom lives a few functions away in this same file, and copying it would drop
the key on `false` — the one case worth proving.
The contract test imports the path constants rather than retyping them: a
hardcoded path asserts only itself and stays green against a wrong one. Each
new test was mutation-checked — removing the `credentials` declaration,
swapping the strict comparison for `Boolean()`, and dropping the field from a
call literal each make it fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RUK-290, second half — the UI end of the chain the previous commit wired up. The box appears on the password form and on the OTP code step, unchecked by default, and its value reaches the backend as `remember_me`. Two of the five silent-failure sites land here: the `signIn(...)` object literal in `credentialsSignInAction`, and the prop chain from the forms through `LoginPageProps` to the two `"use server"` wrappers. Widening the input union without touching the literal compiles and sends nothing, so a test that follows the flag to the wire is the only thing that catches it — that is what the new cases in `built-in-sign-in-actions.test.ts` do, asserted off the `signIn` mock. Decisions worth keeping: - The third argument is REQUIRED, not optional. Optional would have let the three existing test files keep compiling while feeding `undefined` into a chain typed `boolean` — a mismatch with no compiler error anywhere. Those three files are updated here instead; the compiler failing on them was the point. - The box lives on the OTP *code* step, not the address step: the code submission is the request that mints a session. Returning to step one resets it explicitly in `backToEmail()` and in the `otp_session_mismatch` branch, because both reset the step in place rather than unmounting — nothing would have cleared it for us. - The label names no duration. The token pair carries no refresh TTL and no echo of the flag, so "stay signed in for N days" would be invented. - When the backend advertises both methods their forms render side by side, each with its own box. The one that counts is the one in the form actually submitted; hoisting shared state across two independent sign-in flows to make a single box serve both would couple them for cosmetics. Also fixes an unrelated jsdom gap this surfaced: Radix's Checkbox measures itself through `useSize`, so every auth test file rendering the login page died on `ResizeObserver is not defined` before reaching an assertion. Stubbed inline per file, matching the existing settings tests — `src/features/**` may not import `@/shared/testing/**`, and that boundary is worth more than deduplicating six lines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review found that the fail-closed check was not a check. It asserted on the
SOURCE TEXT of `authorize`:
expect(authConfigSource).toContain('credentials?.rememberMe === "true"')
so this passes it:
const rememberMe = !(credentials?.rememberMe === "true");
The substring survives, the meaning inverts, and every user who unticked the box
is granted a long session. Confirmed before fixing: the inverted version passed
all 1453 unit tests and 219 contract tests. The `Boolean()` mutation was caught
only because it happened to break the substring, not because anything understood
the semantics — the protection was accidental.
`parseRememberMe` therefore moves into its own module. Not for tidiness: it is
the only way to call it. Importing `auth-config` outside the Next runtime fails
on `next/server` via NextAuth's env module, which is why the tests around that
file read source text at all. The new module imports nothing, so the decision
can be exercised directly, and its ten cases are chosen to kill each plausible
wrong implementation rather than to enumerate inputs.
Also covers three OTP behaviours that no test touched — removing either reset
left 110 tests green:
- the choice is forgotten when the user changes address (`backToEmail`),
- and when a lost binding drops the flow back to step one,
- but survives a wrong code, so five attempts do not mean re-ticking it five
times.
All three assert within a SINGLE render. The first draft of two of them called
`cleanup()` and re-rendered, which destroys the state under test and passes no
matter what the component does — the same defect being fixed here, reintroduced
one directory away. Each is mutation-checked: deleting either reset, or adding
an over-eager one to the wrong-code branch, now fails.
The `credentials` declaration assertion stays source-text (nothing else can see
it) but its regex no longer breaks when prettier reformats the object.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…dy existed `reachCodeStep` was there but only accepted a `requestCode` override, so any test needing its own `submitCode` fell back to `setup()` plus a hand-copied four-line walk to step two — duplicated across eight tests. Being setup rather than assertion, those copies drifted. The helper now takes the same overrides object as `setup`, plus the address as a parameter (the change-email tests walk the path twice with two addresses). `enterAddress` is the step-one half on its own, for a second pass through an existing render; `submitCodeValue` types and submits. Deliberately not converted: the two §6.9 tests that assert the flow STAYS on step one. `reachCodeStep` waits for step two, so they cannot be expressed through it. Also drops a dead first render in "refuses a short code locally", where a `reachCodeStep()` was built and then thrown away by `cleanup()` before the real `setup()`. Behaviour is unchanged and checked as such rather than assumed: the two remember-me guards still fail when their production line is removed — deleting the reset in `backToEmail()` fails the change-address test, and deleting the one in the `otp_session_mismatch` branch fails the lost-binding test. Both still assert within a single render, which is what makes them able to see the state at all. Test file only; no production code touched. 28 tests before, 28 after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…action
The ship review mutated both `"use server"` wrappers in `login/page.tsx` to pass
a literal `rememberMe: false` and the whole gate stayed green — `tsc`, 219
contract tests and 1466 unit tests all silent — while the checkbox stopped doing
anything on both sign-in methods at once. Reproduced before fixing: it is the
widest silent failure in this change and nothing covered it.
SPEC §3.2 classified this site as "not silent" because widening a signature
breaks its callers. That is true for *adding* the parameter and useless against
*bypassing* it afterwards, which is the shape the regression actually takes:
`false` satisfies `boolean`, so the compiler has nothing to say. The other eight
sites were each proven by mutation; this one was reasoned about and got away
with it.
Nothing else reaches the wrappers. The chain is covered from the checkbox down
to `credentialsSignInAction` and from `credentialsSignInAction` out to the wire,
but the four `LoginPage` tests hand in inert stubs (`async () => ({})`) because
they are testing the component, not the page that composes it — so the joint
between the two halves had no test at all.
The page is an async server component, so it is invoked as a function and the
props it returns are inspected; the wrappers are then called the way a
submitting form calls them, and the assertion reads the mock rather than the
input. Mutation-checked both ways: hardcoding the flag in both wrappers fails
two cases, and the easy half-miss — fixing password and forgetting OTP — fails
one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ruko1202
changed the title
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.
Closes stage 1 of built-in sign-in with two independent backend changes: an
admin can now verify SMTP settings before committing them, and session length
becomes operator policy rather than something each sign-in negotiates.
settings in the request body, reads and writes nothing, and answers 502
carrying the far end's own error so an admin can act on it
session_max_lifetime 720h): the row records when the session started rather
than a precomputed deadline, so lowering a limit takes effect on sessions
that already exist
in place an actively refreshed session no longer lives indefinitely, and
there is nothing left for a client to ask for
so save and probe agree: without TLS, CRAM-MD5 hands the server an HMAC-MD5
of the password that grinds offline cheaply
config vocabulary and must not import a gateway
which would otherwise sign everyone out with no error naming the cause