diff --git a/.changeset/login-no-longer-enumerable.md b/.changeset/login-no-longer-enumerable.md new file mode 100644 index 0000000..27eefd3 --- /dev/null +++ b/.changeset/login-no-longer-enumerable.md @@ -0,0 +1,41 @@ +--- +'seamless-cli': patch +--- + +Stop reading a `401` from `/login` as "no such account", and stop claiming an account +exists when the instance will not say. + +`POST /login` no longer answers `401`. An identifier with no usable account, which used +to cover unknown, unverified, and no-permitted-method, now gets `200` and a decoy pre-auth +token so the response cannot be used to test whether an account exists. Two branches in +`completeLogin` read that `401` and are now unreachable: the "not verified yet" message +and "No account was found for X". Both are removed, because there is no longer an answer +for them to read. + +The messages that surrounded them were making a claim the CLI can no longer support. "A +code was sent to X" is now "If an account exists for X, a code is on its way", and "This +account cannot use email otp login" is now phrased as what the instance offered, since +that method list comes back for an unknown identifier too. + +An unknown identifier therefore runs the ordinary flow and fails at the code step. That is +the intended behaviour and not something the CLI can shortcut, so the final error now says +so: it names the identifier and points at registering, instead of "Could not verify a +code" with no explanation of the likeliest reason. + +`423` is now reported on its own terms, with how long to wait when the instance says. +Previously it fell through to "Login request failed (423)". It is also the one answer left +that does imply an account exists, which is a deliberate and documented tradeoff on the +API side. + +Adds `verify/harness/api/loginEnumeration.spec.ts` to the conformance matrix, pinning the +guarantee against a running instance: an unknown identifier gets the same status and the +same fields as a registered one, the same identifier keeps the same subject and the same +method list across attempts, the OTP send reports success and sends nothing, the verify +fails the way a wrong code fails, and a credential id that cannot exist is refused +identically for both. + +The `loginMethods` assertion is the one worth reading. It is deliberately not equality +between one account and one decoy: that list is filtered by what an account can do, and a +decoy's capabilities are derived per identifier, so any two can legitimately differ. What +must hold is that a real account's list is one a decoy can also produce, which is what +makes a narrow list stop being proof of existence. The spec asserts that over a sample. diff --git a/src/commands/login.test.ts b/src/commands/login.test.ts index 1c67a4c..d06098c 100644 --- a/src/commands/login.test.ts +++ b/src/commands/login.test.ts @@ -292,7 +292,11 @@ describe("runLogin: success", () => { const out = logs(); expect(out.some((l) => l.includes("2 attempts left"))).toBe(true); expect(out.some((l) => l.includes("1 attempt left"))).toBe(true); - expect(out.some((l) => l.includes("A code was sent to dev@example.com."))).toBe(true); + expect( + out.some((l) => + l.includes("If an account exists for dev@example.com, a code is on its way."), + ), + ).toBe(true); }); it("validates the code prompt input as letters for an email login", async () => { diff --git a/src/core/interactiveLogin.ts b/src/core/interactiveLogin.ts index a5a39f3..da946f2 100644 --- a/src/core/interactiveLogin.ts +++ b/src/core/interactiveLogin.ts @@ -78,7 +78,12 @@ export async function promptLogin( notify: (event) => { switch (event.type) { case "code_sent": - console.log(kleur.dim(`A code was sent to ${resolved}.`)); + // The instance answers the same way whether or not the identifier has an + // account, so claiming a code was sent would be stating something this cannot + // know. + console.log( + kleur.dim(`If an account exists for ${resolved}, a code is on its way.`), + ); break; case "code_resent": console.log( diff --git a/src/core/loginFlow.test.ts b/src/core/loginFlow.test.ts index 29cd5ee..ca52db2 100644 --- a/src/core/loginFlow.test.ts +++ b/src/core/loginFlow.test.ts @@ -214,9 +214,11 @@ describe("completeLogin", () => { expect(authHeader(verify)).toBe("Bearer e2"); }); - it("rejects an unverified account with a clear message", async () => { + it("reports a locked account with how long to wait", async () => { mockRouter({ - "/login": [() => json({ error: "Login failed. Need to verify." }, 401)], + "/login": [ + () => json({ error: "account_locked", retryAfterSeconds: 900 }, 423), + ], }); await expect( @@ -225,7 +227,21 @@ describe("completeLogin", () => { identifier: "dev@example.com", getCode: async () => "123456", }), - ).rejects.toThrow(/not verified/i); + ).rejects.toThrow(/Too many failed attempts.*15 minute/s); + }); + + it("reports a locked account without a retry hint when none is given", async () => { + mockRouter({ + "/login": [() => json({ error: "account_locked" }, 423)], + }); + + await expect( + completeLogin({ + instanceUrl: INSTANCE, + identifier: "dev@example.com", + getCode: async () => "123456", + }), + ).rejects.toThrow(/Too many failed attempts for dev@example.com\.$/); }); it("rejects when email OTP is not an available login method", async () => { @@ -241,7 +257,9 @@ describe("completeLogin", () => { identifier: "dev@example.com", getCode: async () => "123456", }), - ).rejects.toThrow(/cannot use email otp/i); + // Phrased as what is on offer rather than what "this account" can do: the method + // list comes back for an unknown identifier too. + ).rejects.toThrow(/email otp login is not available for/i); }); it("returns null when the user cancels the code prompt", async () => { @@ -330,9 +348,50 @@ describe("completeLogin", () => { ).rejects.toThrow(/not a valid email or phone number/); }); - it("rejects an unknown account with a 401 that isn't a verify message", async () => { + // An unknown identifier is answered exactly like a real one: 200, a decoy pre-auth + // token, a method list, and an OTP send that reports success without sending. The CLI + // cannot tell the difference and must not pretend to, so the only place this can fail + // is the code step. + it("runs an unknown identifier through the ordinary flow and fails at the code", async () => { + const attempts: number[] = []; + + mockRouter({ + "/login": [ + () => + json({ + message: "Success", + sub: "4f7158fa-ca90-4c22-a1d1-eba3f0c1a2b3", + token: "decoy", + identifierType: "email", + loginMethods: ["email_otp"], + ttl: 900, + }), + ], + "/otp/generate-login-email-otp": [() => json({ message: "success", token: "decoy" })], + "/otp/verify-login-email-otp": [ + () => json({ error: "Not allowed" }, 401), + () => json({ error: "Not allowed" }, 401), + () => json({ error: "Not allowed" }, 401), + ], + }); + + await expect( + completeLogin({ + instanceUrl: INSTANCE, + identifier: "nobody@example.com", + getCode: async ({ attempt }) => { + attempts.push(attempt); + return "ABCDEF"; + }, + }), + ).rejects.toThrow(/If that address or number has no account yet, register it first/); + + expect(attempts).toEqual([1, 2, 3]); + }); + + it("rejects a forbidden login without claiming the account is missing", async () => { mockRouter({ - "/login": [() => json({ error: "No such account" }, 401)], + "/login": [() => json({ error: "Not allowed" }, 403)], }); await expect( @@ -341,7 +400,7 @@ describe("completeLogin", () => { identifier: "dev@example.com", getCode: async () => "123456", }), - ).rejects.toThrow(/No account was found/); + ).rejects.toThrow(/Login is not permitted for dev@example.com/); }); it("maps other login failures to a generic status error", async () => { diff --git a/src/core/loginFlow.ts b/src/core/loginFlow.ts index 6caa6dc..ebd15fe 100644 --- a/src/core/loginFlow.ts +++ b/src/core/loginFlow.ts @@ -107,22 +107,31 @@ async function startLogin( ); } + // `/login` no longer answers 401. An identifier with no usable account, which used to + // mean unknown, unverified, or with no permitted method, now gets a 200 and a decoy + // pre-auth token so the response cannot be used to test whether an account exists. The + // branches that read 401 as "no such user" and "not verified yet" were removed with it: + // there is no longer an answer for them to read. Such a login fails at the code step + // instead, which is what `completeLogin` reports. if (!res.ok) { - const message = apiMessage(res.data) ?? ""; - if (res.status === 401 && /verify/i.test(message)) { - throw new LoginError( - `The account for ${identifier} is not verified yet. Finish registration, then log in.`, - ); - } if (res.status === 400) { throw new LoginError( `"${identifier}" is not a valid email or phone number.`, ); } - if (res.status === 401 || res.status === 403) { - throw new LoginError( - `No account was found for ${identifier}, or login is not permitted.`, - ); + if (res.status === 423) { + // The one remaining answer that does imply an account, and the one worth naming: + // it needs prior failed attempts against this identifier, and the developer can + // act on it by waiting. + const retryAfter = res.data?.retryAfterSeconds; + const wait = + typeof retryAfter === "number" && retryAfter > 0 + ? ` Try again in about ${Math.ceil(retryAfter / 60)} minute(s).` + : ""; + throw new LoginError(`Too many failed attempts for ${identifier}.${wait}`); + } + if (res.status === 403) { + throw new LoginError(`Login is not permitted for ${identifier}.`); } throw new LoginError(`Login request failed (${res.status}).`); } @@ -233,8 +242,10 @@ export async function completeLogin( const channel = started.channel; const required = channel === "email" ? "email_otp" : "phone_otp"; if (started.loginMethods.length > 0 && !started.loginMethods.includes(required)) { + // Deliberately not "this account cannot": the method list comes back for an unknown + // identifier too, so saying so would report an account that may not exist. throw new LoginError( - `This account cannot use ${required.replace("_", " ")} login. Available methods: ${started.loginMethods.join(", ")}.`, + `${required.replace("_", " ")} login is not available for ${opts.identifier}. Offered: ${started.loginMethods.join(", ")}.`, ); } @@ -294,5 +305,9 @@ export async function completeLogin( notify({ type: "incorrect", attemptsLeft: maxAttempts - attempt }); } - throw new LoginError("Could not verify a code. Run seamless login to try again."); + // The instance does not say whether the identifier has an account, so neither can this. + // A wrong code and an identifier nobody has registered both land here. + throw new LoginError( + `Could not verify a code for ${opts.identifier}. If that address or number has no account yet, register it first. Otherwise run seamless login to try again.`, + ); } diff --git a/verify/harness/api/loginEnumeration.spec.ts b/verify/harness/api/loginEnumeration.spec.ts new file mode 100644 index 0000000..0759691 --- /dev/null +++ b/verify/harness/api/loginEnumeration.spec.ts @@ -0,0 +1,139 @@ +import { expect, test } from '../lib/fixtures'; +import { registerAndVerifyEmail } from '../lib/flows'; + +// `POST /login` answers the same way for an identifier with an account and one without, +// and the endpoints that accept the resulting pre-auth token do too. The API has its own +// unit coverage for this; what only this harness can check is that the guarantee survives +// a real instance, with real signing keys and the real login policy in place. +// +// The specific failures worth catching are the ones where a decoy responder reproduces +// the success path and forgets a refusal, since that is the shape every regression here +// has taken so far. + +const UNKNOWN = () => `nobody-${Date.now()}-${Math.random().toString(36).slice(2)}@example.com`; + +async function startLogin(ctx: Parameters[0], identifier: string) { + const res = await ctx.post('/login', { data: { identifier } }); + return { status: res.status(), body: await res.json() }; +} + +test.describe('login enumeration (api)', () => { + test('an unknown identifier is answered in the same shape as a real one', async ({ + actor, + }) => { + await registerAndVerifyEmail(actor.ctx, actor.email); + + const real = await startLogin(actor.ctx, actor.email); + const unknown = await startLogin(actor.ctx, UNKNOWN()); + + expect(unknown.status).toBe(200); + expect(unknown.status).toBe(real.status); + // `sub` and `token` differ between them exactly as they differ between two real + // accounts, so the comparison is over everything else. + expect(Object.keys(unknown.body).sort()).toEqual(Object.keys(real.body).sort()); + expect(unknown.body.identifierType).toBe(real.body.identifierType); + expect(unknown.body.ttl).toBe(real.body.ttl); + expect(typeof unknown.body.token).toBe('string'); + // Deliberately not `loginMethods`. That list is filtered by what an account can do, + // and a decoy's capabilities are derived per identifier, so any one decoy and any + // one account can legitimately differ. What has to hold is the next test. + }); + + test('a real account\'s method list is one a decoy can also produce', async ({ actor }) => { + // The guarantee is not that two given answers match, it is that a given answer does + // not identify an account. A decoy that always claimed everything would make any + // narrower list proof of existence, so the decoy's passkey and phone are derived per + // identifier and a narrow list has to be reachable without an account behind it. + await registerAndVerifyEmail(actor.ctx, actor.email); + const real = await startLogin(actor.ctx, actor.email); + const target = JSON.stringify(real.body.loginMethods); + + const seen = new Set(); + for (let i = 0; i < 40; i += 1) { + const { body } = await startLogin(actor.ctx, UNKNOWN()); + seen.add(JSON.stringify(body.loginMethods)); + } + + // Each decoy draws two independent bits, so this account's exact list comes up about + // a quarter of the time; over 40 identifiers, missing it entirely is a 1-in-100,000 + // event rather than a flake worth retrying. + expect(seen.has(target), `no decoy offered ${target}; saw ${[...seen].join(' | ')}`).toBe( + true, + ); + expect(seen.size).toBeGreaterThan(1); + }); + + test('the same unknown identifier keeps the same subject', async ({ actor }) => { + const identifier = UNKNOWN(); + + const first = await startLogin(actor.ctx, identifier); + const second = await startLogin(actor.ctx, identifier); + const other = await startLogin(actor.ctx, UNKNOWN()); + + // A real identifier resolves to the same row every time and to a different one from + // anyone else's. A subject that rerolled, or that collided, would be the oracle again + // one request later. + expect(second.body.sub).toBe(first.body.sub); + expect(other.body.sub).not.toBe(first.body.sub); + // The offered methods have to be stable for the same reason: a list that changed + // between attempts would separate a decoy from an account on its own. + expect(second.body.loginMethods).toEqual(first.body.loginMethods); + }); + + test('an unknown identifier gets an OTP send that reports success and sends nothing', async ({ + actor, + }) => { + const { body } = await startLogin(actor.ctx, UNKNOWN()); + + const res = await actor.ctx.get('/otp/generate-login-email-otp', { + headers: { Authorization: `Bearer ${body.token}` }, + }); + + expect(res.status()).toBe(200); + expect((await res.json()).message).toBe('success'); + }); + + test('a decoy OTP verify fails the way a wrong code fails', async ({ actor }) => { + const { body } = await startLogin(actor.ctx, UNKNOWN()); + + const res = await actor.ctx.post('/otp/verify-login-email-otp', { + headers: { Authorization: `Bearer ${body.token}` }, + data: { verificationToken: 'ZZZZZZ' }, + }); + + expect(res.status()).toBe(401); + }); + + test('a credential id that cannot exist is refused for real and unknown alike', async ({ + actor, + }) => { + // The sharpest oracle this surface had. `/webauthn/login/start` filters the account's + // credentials by the requested id and refuses when none survive, so an id no + // credential can hold is refused by every real account. A decoy that answered with a + // challenge anyway was identifiable in two requests, whatever the policy. + await registerAndVerifyEmail(actor.ctx, actor.email); + + const real = await startLogin(actor.ctx, actor.email); + const unknown = await startLogin(actor.ctx, UNKNOWN()); + + const ask = (token: string) => + actor.ctx.post('/webauthn/login/start', { + headers: { Authorization: `Bearer ${token}` }, + data: { credentialId: 'not-a-real-credential-id' }, + }); + + const realRes = await ask(real.body.token); + const unknownRes = await ask(unknown.body.token); + + expect(unknownRes.status()).toBe(realRes.status()); + expect(await unknownRes.text()).toBe(await realRes.text()); + }); + + test('a malformed identifier is still rejected', async ({ actor }) => { + // Not an enumeration signal: it does not depend on whether an account exists, and + // answering 200 here would leave a caller with no way to learn it typed nonsense. + const { status } = await startLogin(actor.ctx, 'not-an-identifier'); + + expect(status).toBe(400); + }); +});