diff --git a/.ilana/architecture.md b/.ilana/architecture.md index fbb0dbc..f14ea8a 100644 --- a/.ilana/architecture.md +++ b/.ilana/architecture.md @@ -492,7 +492,8 @@ Redis queue polling (200ms..2s), not blocking primitives (multi-condition wake-u - New env var: `MAILX_JWT_SECRET` (same convention as `MAILX_API_KEY_PEPPER`; unset falls back to an ephemeral per-process secret with a startup warning — sessions do not survive a restart in that mode). - Password reset (DEC-213): migration 000029 adds `password_reset_tokens` (human_id FK CASCADE, `token_hash` unique-indexed, `expires_at`, `used_at` nullable, partial index on unresolved tokens per human). `Service.ForgotPassword`/`ResetPassword`: 5-minute single-use tokens (`PasswordResetTokenTTL`), same anti-enumeration response shape as `Login`, one collapsed `ErrPasswordResetTokenInvalid` for any unknown/expired/used/race-lost token. `database.ResetPassword` is one transaction: consume-token (RowsAffected-checked), update `password_hash`, revoke every refresh token the account has. Email delivery reuses `api.SubmissionAcceptor` (the v0.42 accept-a-message primitive) via a new `humanauth.Mailer` interface, configured by `MAILX_SYSTEM_TENANT_ID`/`MAILX_SYSTEM_FROM_ADDRESS`/`MAILX_DASHBOARD_BASE_URL`; unset means no mailer (token still created, logged not sent) rather than a startup failure. New routes `POST /v1/auth/forgot-password`/`reset-password`, public, behind their own `passwordResetIPLimitMiddleware` bucket (`Policy.PasswordResetIPRate`/`PasswordResetIPBurst`, default 1/60 rps burst 3 — much tighter than login's, since forgot-password sends a real email per call). - Organization invitations (DEC-216): migration 000030 adds `org_invitations` (tenant_id FK CASCADE, invited_by FK CASCADE, `normalized_email`/`raw_email`, `token_hash` unique-indexed, `expires_at`, `accepted_at` nullable, partial index on `(tenant_id, normalized_email) WHERE accepted_at IS NULL`) plus plain nullable URL columns `tenants.logo_url` and `humans.avatar_url` (no upload pipeline — operator decision). `Service.InviteToOrganization(ctx, inviterHumanID, tenantID, email)`: owner-only (`database.IsTenantOwner` re-checked server-side, `ErrNotOrgOwner` otherwise — no broader RBAC), 5-hour single-use tokens (`OrgInvitationTTL`), same `humanauth.Mailer`/`WithDashboardBaseURL` delivery wiring as password reset, degrades the same way when no mailer is configured. `Service.AcceptOrgInvitation(ctx, rawToken, existingHumanID, signupName, signupPassword)` is a single combined accept path (operator decision, not separate signup-then-join calls): with an existing session, the invitation's email must match that account's own (`ErrOrgInvitationEmailMismatch` otherwise) and `database.AcceptOrgInvitationForExistingHuman` atomically consumes the token and inserts `tenant_members` (`ON CONFLICT DO NOTHING`); with no session, `database.AcceptOrgInvitationWithSignup` atomically consumes the token, creates the human account (ALWAYS using the invitation's own stored email, never client-supplied), and inserts membership, all in one transaction. Both failure paths collapse to `ErrOrgInvitationInvalid`. New routes: `POST /v1/orgs/{id}/invites` (owner-only, behind `humanAuthMiddleware` then a new per-human `orgInviteLimitMiddleware` — `Policy.OrgInviteRate`/`OrgInviteBurst`, default 1/30 rps burst 10, keyed by the inviting human's ID rather than IP since the caller is already authenticated) and `POST /v1/orgs/invites/accept` (deliberately NOT behind `humanAuthMiddleware` — the invitee may have no account yet; reads an optional bearer token directly). -- Known limitation / explicitly deferred (not built): email verification and any RBAC beyond owner/member. The frontend's `{name, slug}` org-create contract is accepted; `slug` is currently a no-op input (tenants has no slug column yet). Avatar/logo are URL-only fields with no upload/hosting pipeline. +- Email verification (DEC-241..244): migration 000034 adds nullable `humans.email_verified_at` (NULL = unverified) and `email_verification_tokens` (same shape as `password_reset_tokens`). `SignUp` issues a 15-minute token (`EmailVerificationTokenTTL`) and emails `{MAILX_DASHBOARD_BASE_URL}/verify-email?token=...` via `humanauth.Mailer`, bounded by a 10s timeout so a slow mail-accept can't stall account creation; send failure (or no mailer / no dashboard URL, DEC-215 guard) is logged and never fails signup. `database.CreateEmailVerificationToken` locks the `humans` row `FOR UPDATE` and inserts a fresh token WITHOUT touching any prior one; only after `sendVerificationEmail` confirms the new email actually sent does `SupersedeOtherEmailVerificationTokens` invalidate every other pending token strictly older (by `(created_at, id)`, `created_at` via `clock_timestamp()`) than the new one — a failed send therefore leaves the previous working link intact instead of orphaning the account (DEC-244, closing RSK-049; same fix shape as DEC-218/226 for org invitations). `POST /v1/auth/verify-email {token}` (public, `authIPLimitMiddleware`): one transaction, RowsAffected-checked consume + set `email_verified_at`; one collapsed `ErrEmailVerificationTokenInvalid` (422 `invalid_verification_token`). `POST /v1/auth/resend-verification {email}` (public): always the same 200 body; own bucket `Policy.EmailVerificationResendIPRate/Burst` (default 1/60 rps, burst 3; env `MAILX_LIMIT_EMAIL_VERIFICATION_RESEND_IP_RPS`/`_BURST`). `GET/PATCH /v1/me` expose `email_verified_at`. Verification gates nothing (DEC-243). +- Known limitation / explicitly deferred (not built): any RBAC beyond owner/member. The frontend's `{name, slug}` org-create contract is accepted; `slug` is currently a no-op input (tenants has no slug column yet). Avatar/logo are URL-only fields with no upload/hosting pipeline. ## OAuth sign-in & TOTP MFA (v0.47 phase 3b; design decisions DEC-228..231) @@ -520,4 +521,5 @@ Redis queue polling (200ms..2s), not blocking primitives (multi-condition wake-u - AuthZ: non-member -> 404 `organization_not_found`; member non-owner on owner-only route -> 403 `not_org_owner` (DEC-228). - Invariant: an org always has >= 1 owner; removals are serialized by the tenant row lock (DEC-229). Owners cannot remove themselves (409 `cannot_remove_self`). - DB: `internal/database/dashboard.go` (`UpdateHumanProfile`, `UpdateOrganization`, `ListTenantMembers`, `RemoveTenantMember`, `ListPendingOrgInvitations`, `RevokeOrgInvitation`). No migration. +- Org API keys (DEC-241..243, `internal/api/apikeys_handler.go`): `POST /v1/orgs/{id}/api-keys` (owner; `{name, scopes}` -> 201 key resource + raw `key` once), `GET .../api-keys` (member; `{data:[{id(=key_id), name, scopes, status active|revoked|expired, created_at, last_used_at, expires_at, revoked_at}]}`), `POST .../api-keys/{keyId}/rotate` (owner; 200 + new raw `key`, old key dead immediately, 409 if not active), `DELETE .../api-keys/{keyId}` (owner; 204, 404 if unknown/other tenant/already revoked). Same `auth.Service` as the CLI; per-human mint limit on create/rotate. This is how a web-signed-up org obtains the API key every `requireScope` route needs. - Limitations: no leave-org / transfer-ownership / role-change flow; no email change; no org slug; `/v1/orgs/*` dashboard routes are not in `TestOpenAPIRoutesMatchRuntime` (its mux has no humanAuth), covered by `dashboard_handler_test.go` instead. diff --git a/.ilana/decisions.md b/.ilana/decisions.md index 254169a..7ac21b2 100644 --- a/.ilana/decisions.md +++ b/.ilana/decisions.md @@ -245,3 +245,12 @@ Process decisions above (DEC-001..DEC-007) belong to the v0.23 FLEET run and sta - DEC-238 [v0.47 phase 3c]: at most one in-flight charge per tenant period. Claim under tenant `FOR UPDATE`, committed as `pending` before the network call; a pending attempt (outcome unknown) blocks all further attempts until `verify` settles it (success -> `ApplyPlanPayment`, idempotent by reference; failed/abandoned/reversed/"reference not found" -> failed). A success whose amount/currency differs from the claim is never applied and stays pending (logged `billing_renewal_amount_mismatch` every pass) for manual resolution. Verified by `TestClaimRenewalAttemptConcurrentExactlyOnce` (16 racers; fails when `FOR UPDATE` is removed) and `TestAutoRenewConcurrentPassesChargeOnce` (12 concurrent passes -> 1 Paystack charge, 1 extension). - DEC-239 [OAuth/MFA + auto-renewal, CodeRabbit review pass]: fixed 2 findings on PR #26, one a genuine account-takeover vulnerability. (1) **OAuth email-linking account takeover** (CWE-287, major): `ResolveOAuthHuman` linked a provider-verified OAuth identity to ANY existing human with the same normalized email, including one created via ordinary password signup. An attacker could pre-register a victim's real email with a password the attacker controls; when the victim later signs in via Google/GitHub with that same (genuinely theirs) verified email, the OAuth login would silently attach to the attacker's account - the attacker keeps password access to whatever the victim then does under that account. Fixed by adding `database.ErrOAuthAccountRequiresPasswordLogin`: `ResolveOAuthHuman` now only auto-links when the existing account's `password_hash` is still `unusablePasswordHash` (i.e. it was itself only ever reached through OAuth, so every existing "owner" already proved control via provider verification, not a password an attacker could have set unilaterally). A password-holding account is never touched; the caller gets a clear, distinct error instead (`oauth_account_requires_password_login`, 409) telling them to log in with the password first. Deliberately did NOT build an authenticated "link accounts" flow in this pass - refusing silently-unsafe linking is the security-correct minimum; an explicit opt-in linking flow (log in with password, then link a provider) is a reasonable follow-up, not required to close this hole. (2) **Paystack verify-call 4xx misclassification** (major, financial correctness): `Paystack.doTx` mapped ANY 4xx response to `ChargeFailed` ("no money moved"), a rule that is only true for `charge_authorization` (a rejected charge request never creates a charge) - NOT for `VerifyTransaction`, which `reconcilePending` only ever calls for an attempt whose outcome is ALREADY ambiguous, meaning the charge may well have already succeeded. A verify call failing with 401/403 (rotated/misconfigured secret key) or any other non-"unknown reference" 4xx says nothing about whether money moved, but was being classified as `ChargeFailed` - which unblocks `ClaimRenewalAttempt` for a fresh attempt under a NEW reference, double-charging the card. Fixed by splitting verify's 4xx handling: only the specific "Paystack has no record of this reference" response (400/404 with `status:false`) is `ChargeFailed`; every other verify 4xx is `ChargeUnknown` (stays pending, blocks further attempts, gets reconciled again later) - `charge_authorization`'s 4xx handling is unchanged. Both fixes covered by new regression tests: `TestOAuthNewAccountLinkAndState` updated (the old test literally asserted the vulnerable auto-link behavior - now asserts it's refused, and separately that a password-less OAuth-only account still links correctly across a changed provider ID), `TestOAuthRespectsMFA` updated to use an OAuth-created account (a password account can no longer reach the MFA-required path via OAuth at all, by design), and a new `TestAutoRenewVerify401NeverDoubleCharges` (ambiguous charge stays pending through a verify-layer auth failure, no second charge fires). Verified: gofmt/go vet/go build clean, full `go test ./...` and `go test -race ./...` clean, Docker rebuild+boot smoke clean. - DEC-240 [RSK-046/RSK-048 risk reduction pass, no live credentials available]: before starting v0.48, spent a pass trying to lower the two open OAuth/billing risks without requiring new setup. Verified Google's and GitHub's OAuth contracts against their CURRENT real documentation (not just "as understood"): confirmed the OIDC userinfo endpoint (`https://openidconnect.googleapis.com/v1/userinfo`) returns `email_verified` as a genuine boolean (the LEGACY `oauth2/v3/userinfo` endpoint is known to sometimes return it as a string - this codebase correctly uses the OIDC endpoint, not the legacy one), confirmed GitHub's token endpoint requires `Accept: application/json` to avoid a form-encoded response (already correctly set in `doJSON` for every request) and confirmed GitHub reports a bad/expired code via an `error` JSON field. Paystack's docs are bot-blocked from automated fetching, so `internal/billing`'s charge_authorization/verify contracts remain unverified against current live documentation - RSK-048 stands as-is; only a live Paystack test-mode credential can actually close it. For RSK-046 (OAuth login-CSRF, no client-side state binding since no frontend consumes this API yet): added a real, zero-frontend-dependency defense-in-depth layer rather than leaving it purely as an accepted risk. `handleOAuthCallback` now rejects a request whose `Referer` header is PRESENT and names neither the expected OAuth provider's own domain (`accounts.google.com` / `github.com`) nor nothing at all. This works because a genuine callback arrival is always a browser redirect FROM the provider's consent page (Referer names the provider), while the practical CSRF delivery vector - a crafted callback URL placed in an email or on an attacker's page - arrives with either no Referer (many clients strip it) or the WRONG one (the attacker's own page), never the provider's. An absent Referer is still allowed through to the normal state-validation path (some browsers/extensions legitimately omit it, and rejecting on absence would break real users for a header that was never guaranteed) - this is explicitly NOT a complete fix (a determined attacker's page could still be made to omit or spoof a referrer via `rel=noreferrer`/meta tag, though spoofing the exact string `accounts.google.com`/`github.com` from an arbitrary page is not possible), but it meaningfully raises the bar for the common real-world delivery vector (email/chat/webpage links) at zero cost to any legitimate flow. RSK-046 stays open in risks.md - this is risk reduction, not closure; the complete fix still needs client-side state binding once a real frontend exists. Verified: gofmt/go vet/go build clean, full `go test ./...` and `go test -race ./...` clean (new `TestOAuthCallbackRejectsWrongReferer` covers wrong/correct/absent referer), Docker rebuild+boot smoke clean. +- DEC-241 [v0.47 email verification]: signup verification token TTL is 15 minutes (`EmailVerificationTokenTTL`, operator's own number; distinct from `PasswordResetTokenTTL`). Token issued and emailed inside `SignUp`; any issue/send failure is logged and signup still mints a session (user can resend). Same hashed single-use token table shape and collapsed-error anti-enumeration posture as DEC-213. +- DEC-242 [v0.47 email verification]: a new verification token supersedes every pending one for the account (at most one valid link). Implemented as lock-`humans`-row `FOR UPDATE` -> mark pending tokens used -> insert, in one transaction; `VerifyEmail` takes the same lock first. Chosen over DEC-226's created_at-ordering scheme because this is a single account resending to itself: the human row is a natural per-account mutex, so concurrent resends fully serialize and the last committed token always survives (proven by `TestConcurrentResendsLeaveExactlyOneValidToken`). Trade-off: supersede happens before the send, so a resend whose email fails leaves no working link until the next resend (RSK-049). `verify-email` uses the shared `authIPLimitMiddleware` (256-bit token entropy makes guessing moot; the limit is load protection); `resend-verification` gets its own password-reset-class bucket because each call can send email. +- DEC-243 [v0.47 email verification]: verification does NOT gate login, dashboard, org actions, or sending in this pass. It is informational (`email_verified_at` on `/v1/me` for a frontend banner). Operator did not ask for blocking; choosing what to block is a separate, larger decision. Existing accounts stay NULL (unverified), no backfill. +- DEC-244 [email verification, caught before merge]: fixed the resend-failure ordering bug the building agent self-flagged as RSK-049, same class as DEC-218/226. `IssueEmailVerificationToken` invalidated every other pending token for the account BEFORE attempting to send the new one's email, so a failed send left the account with neither a working old link nor a delivered new one. Split it into `CreateEmailVerificationToken` (pure insert, still locks the `humans` row to serialize concurrent issues and check "not already verified") and `SupersedeOtherEmailVerificationTokens` (called only after `sendVerificationEmail` confirms the new email was actually sent). Also pre-emptively applied DEC-226's fix for the SAME underlying bug the naive version of this split would have reintroduced: `SupersedeOtherEmailVerificationTokens` compares the full `(created_at, id)` tuple, not `id != keep`, so two concurrent successful resends can never mutually invalidate each other (whichever's UPDATE runs last), and `created_at` is set via `clock_timestamp()` at INSERT time rather than the column's `now()` default, since a transaction that waited on the `humans` row lock would otherwise capture an earlier timestamp than one that acquired the lock and committed first - the exact ordering-skew DEC-226 diagnosed for org invitations. Proven by a new `TestFailedResendDoesNotInvalidateTheWorkingVerificationLink` and by re-running the existing `TestConcurrentResendsLeaveExactlyOneValidToken` 15x (150 total race trials) with `-race`, all clean. Verified: gofmt/go vet/go build clean, full `go test ./...` and `go test -race ./...` clean, migration 000034 round-trip validated on the real dev Postgres then rolled back, Docker rebuild+boot smoke clean, live-curl-verified signup/`/me`/resend/verify-email against the running server. +- DEC-245 [v0.47 dashboard API keys] (renumbered from the agent's own DEC-241 - collided with email verification's DEC-241, built concurrently from the same base commit): org owners manage API keys over HTTP (`/v1/orgs/{id}/api-keys`, human JWT, `internal/api/apikeys_handler.go`) by calling the SAME `auth.Service` Create/Rotate/Revoke/List the `mailx *-api-key` CLI uses (wired by type-asserting newMux's `authSvc` to `apiKeyManager`); no key generation/hashing is reimplemented. AuthZ reuses `dashboardHandler.orgAccess` (DEC-228): non-member 404, non-owner 403 on create/rotate/revoke, list open to any member. A `{keyId}` of another tenant returns 404 `api_key_not_found` (no cross-tenant enumeration). Raw key returned once (create 201 / rotate 200 `key` field); list never returns key or hash. No migration. +- DEC-246 [v0.47 dashboard API keys] (renumbered from DEC-242): an owner may grant any scope in `auth.ValidScopes`; no extra restriction, because owner is the trust boundary and the CLI has none beyond operator access. The handler validates scopes itself before calling Create (unknown/duplicate/empty -> 422 `invalid_scopes` naming the bad value) so a made-up scope can never be stored. Keys created over HTTP never expire (no TTL field yet; CLI `-ttl` remains the only way). +- DEC-247 [v0.47 dashboard API keys] (renumbered from DEC-243): (a) create/rotate pass a per-human `apiKeyMintLimitMiddleware` (`Policy.APIKeyMintRate`/`APIKeyMintBurst`, default 1/30 rps burst 10, env `MAILX_LIMIT_APIKEY_MINT_RPS`/`_BURST`), same shape as `orgInviteLimitMiddleware`; list/revoke are unlimited beyond humanAuth. (b) HTTP rotate uses grace 0 (old key invalid immediately); rotating a revoked/expired key -> 409 `api_key_not_active`. (c) Revoke matches the CLI exactly: an already-revoked key yields 404 `api_key_not_found` and no change (NOT idempotent-204 like invite revoke) - deliberate consistency with `database.RevokeAPIKey`. +- DEC-248 [org API keys, merge review]: independently verified the API-key-management agent's work before pushing (full test suite + `-race`, manual review of `orgAPIKeyHandler`/`orgKey`'s cross-tenant isolation and scope validation, OpenAPI, Docker boot smoke) and additionally live-tested the exact end-to-end claim that matters most: signed up a real account via `POST /v1/auth/signup`, created an org, minted an API key via `POST /v1/orgs/{id}/api-keys`, and used the raw key to successfully call the real `GET /v1/domains` (a `requireScope`-protected, API-key-only route) against the running server - confirming a dashboard-signed-up user can now reach the rest of the tenant-scoped API without any CLI/operator involvement, closing the gap this milestone exists to close. Resolved a DEC-number collision with the concurrently-built email-verification work (both independently claimed DEC-241..243 from the same base commit): renumbered this work's entries to DEC-245..247 in decisions.md and the two source files (`internal/api/apikeys_handler.go`, `internal/ratelimit/policy.go`) that reference them. +- DEC-249 [PR #28, CodeRabbit review pass]: fixed 3 findings. (1) **Rotate error mapping** (functional correctness): `auth.Service.Rotate` returned a bare `fmt.Errorf` string for a revoked/expired key, not the `auth.ErrRevoked`/`ErrExpired` sentinels used elsewhere - so the HTTP rotate handler's `errors.Is` check could never match the race where a key is revoked/expired concurrently between the handler's own unlocked pre-check and the actual `Rotate` call, falling through to a generic 500 instead of the correct 409 `api_key_not_active`. Fixed by wrapping both returns with their sentinel (`%w`) and adding both to the handler's `errors.Is` check; strengthened the two existing tests (`TestServiceRotateRejectsExpiredKey`/`RevokedKey`, previously only checking `err != nil`) to assert the specific sentinel. (2) **Unbounded verification-email send** (stability): `SignUp` called `sendVerificationEmail` synchronously with no timeout on its own request context, so a slow `SubmissionAcceptor.Accept` (synchronous file+DB work) could stall account creation indefinitely; a naive fix of detaching it into a goroutine tied to a context that gets canceled after the response is written would risk losing the durable outbound record mid-write. Fixed by wrapping the call in a 10-second `context.WithTimeout`, keeping it synchronous (so success/failure still means the record was actually durably written or cleanly gave up) while capping how long a slow mailer can hold up signup. (3) **Stale architecture doc**: `architecture.md`'s email-verification section still described the pre-DEC-244 invalidate-before-send design; updated to describe the current create-send-then-supersede flow. Verified: gofmt/go vet/go build clean, full `go test ./...` and `go test -race ./...` clean, Docker rebuild+boot smoke clean. diff --git a/.ilana/ledger.md b/.ilana/ledger.md index c655695..db4f1cf 100644 --- a/.ilana/ledger.md +++ b/.ilana/ledger.md @@ -344,3 +344,18 @@ CodeRabbit reviewed the combined OAuth/MFA + auto-renewal PR and caught 2 real f ## 2026-09-26 | RSK-046/RSK-048 risk reduction before v0.48 | GATE PASS User asked to lower the two open risks flagged after v0.47 phase 3 before moving to the next milestone. Without live Paystack/Google/GitHub credentials, focused on what was actually achievable: verified Google's and GitHub's OAuth API contracts against their current real documentation rather than relying on prior knowledge - confirmed the OIDC userinfo endpoint's email_verified field is genuinely boolean (the codebase correctly avoids the legacy endpoint known for returning it as a string) and confirmed GitHub's Accept: application/json requirement was already correctly handled. Paystack's docs are bot-blocked from automated fetching, so RSK-048's live-verification gap remains open - genuinely needs a live test-mode credential to close, not something checkable from here. For RSK-046 (OAuth login-CSRF), added a real backend-only defense-in-depth layer instead of leaving it purely as an accepted risk: the callback route now rejects a request whose Referer is present and names neither the OAuth provider's own domain nor nothing at all, since a genuine callback always arrives as a redirect FROM the provider (Referer = the provider) while the practical attack (a crafted link in an email or on an attacker's page) arrives with either no Referer or the wrong one. This is explicitly risk reduction, not closure - documented as such in risks.md - since a determined attacker can still omit or spoof a referrer from their own page; the complete fix needs client-side state binding once a real frontend exists. Verified: gofmt/go vet/go build clean, full go test ./... and go test -race ./... clean (new TestOAuthCallbackRejectsWrongReferer), Docker rebuild+boot smoke clean. Ilana updated: decisions.md (DEC-240), risks.md (RSK-046/048 updated in place), state.json (DEC counter). + +## 2026-09-26 | Email verification for human accounts | GATE PASS (pending operator re-verification) +Migration 000034 (`humans.email_verified_at`, `email_verification_tokens`); signup sends a 15-min verify link; `POST /v1/auth/verify-email`, `POST /v1/auth/resend-verification` (own rate-limit bucket); `/v1/me` exposes `email_verified_at`; gates nothing. DEC-241..243, RSK-049. Evidence: gofmt/vet/build clean; `go test -race ./...` against real Postgres+Redis; migration 000034 down/up round-trip on a disposable database. + +## 2026-09-26 | email verification: fixed before merge (RSK-049 closed) | GATE PASS +Merged the email-verification agent's work and reviewed it before pushing. It had self-flagged a real bug (RSK-049): resend invalidated the old link before attempting to send the new one, same failure-ordering bug DEC-218 fixed for org invitations. Fixed it, and in doing so pre-emptively applied the DEC-226 tuple-ordering fix too, since the naive create-send-then-supersede split would have reintroduced THAT bug (two concurrent successful resends mutually invalidating each other) rather than the one being fixed. This is now the third time this exact bug shape (fail-ordering, then the follow-on concurrency-ordering issue) has been recognized and fixed proactively rather than discovered live - the pattern from DEC-217 through DEC-226 is now a checklist applied on sight whenever new create-then-invalidate-a-prior-token logic appears. Verified: gofmt/go vet/go build clean, full go test ./... and go test -race ./... clean, the concurrency invariant test re-run 15x (150 total race trials), migration 000034 round-trip validated on the real dev Postgres (the agent could not run this itself due to a port conflict with the already-running compose stack), Docker rebuild+boot smoke clean, live curl verification of signup/me/resend/verify-email against the running server. Ilana updated: decisions.md (DEC-244), risks.md (RSK-049 closed), state.json (DEC counter). + +## 2026-09-26 | Org API-key management over HTTP (DEC-245..247) | GATE PASS +Dashboard users had no way to obtain an API key (CLI only) - the real unblocker for domains/emails/templates/webhooks/logs, which only ever accept an API key, never a human JWT. Added owner-gated `/v1/orgs/{id}/api-keys` create/list/rotate/revoke on the existing `auth.Service` (no key generation/hashing reimplemented), OpenAPI documented, per-human mint rate limit. No migration. Built concurrently with email verification from the same base commit, so its own DEC-241..243 collided with email verification's; renumbered to DEC-245..247 (decisions.md and the two source files that reference them) on merge. Evidence: new tests pass against real Postgres, including proving a key minted over HTTP actually authenticates a real scope-checked route; gofmt/vet/build clean; full `go test -race ./...` run. + +## 2026-09-26 | org API-key management: merge review + live end-to-end test | GATE PASS +Merged the API-key-management agent's work (built concurrently with email verification, from the same base commit - both independently claimed DEC-241..243, renumbered this work's to DEC-245..247 in decisions.md and the two source files that reference them). Independently reviewed the authorization code by hand (cross-tenant key access correctly 404s, scope validation rejects unknown/duplicate scopes before creating anything, rotate/revoke race-lost cases handled), then went further than reading code: signed up a real test account against the running server, created an org, minted an API key through the new HTTP endpoint, and used the raw key to successfully call the real GET /v1/domains - proving live, not just by test suite, that a web-signed-up user can now reach the tenant-scoped API without any CLI access. This is the actual milestone this work exists to hit, so it got a live check rather than trusting tests alone. Verified: gofmt/go vet/go build clean, full go test ./... and go test -race ./... clean, OpenAPI tests pass, Docker rebuild+boot smoke clean. Ilana updated: decisions.md (DEC-248), state.json (DEC counter). + +## 2026-09-26 | PR #28: CodeRabbit review pass | GATE PASS +Checked CodeRabbit on PR #28 as asked and got 3 real, all-minor findings, fixed all three. The most consequential: auth.Service.Rotate returned a bare error string instead of a matchable sentinel for a revoked/expired key, so the exact race the HTTP handler was written to handle (key revoked concurrently between its own pre-check and the Rotate call) fell through to a generic 500 instead of the intended 409 - the handler's error-mapping code was correct in shape but had nothing real to match against. Fixed by wrapping the returns with auth.ErrRevoked/ErrExpired and strengthening two existing tests that had been asserting only "an error happened" to assert the actual sentinel. Also bounded a previously-unbounded email send in SignUp with a 10s timeout (stalls signup less under a slow mailer, without risking the durable-record-loss a naive goroutine-detach would introduce) and fixed a stale architecture.md paragraph describing the pre-DEC-244 email-verification design. Verified: gofmt/go vet/go build clean, full go test ./... and go test -race ./... clean, Docker rebuild+boot smoke clean. Ilana updated: decisions.md (DEC-249), state.json (DEC counter). diff --git a/.ilana/milestones.md b/.ilana/milestones.md index 7fdb348..59ffa48 100644 --- a/.ilana/milestones.md +++ b/.ilana/milestones.md @@ -123,3 +123,12 @@ explicitly deferred to a later phase — v0.47 as a whole is NOT complete. - Migration 000033 (`tenants.auto_renew`, encrypted `paystack_auth_*`, `plan_reminder_*`; `billing_renewal_attempts`). `PATCH /v1/billing/auto-renew`; subscription response gains `auto_renew`, `card_on_file`. - `billing.Paystack.ChargeAuthorization`/`VerifyTransaction`; `api.Renewer` run by the hourly `plan-lapse` component before lapsing. New env `MAILX_BILLING_MASTER_KEY`. - Tests: `internal/api/billing_renewal_test.go` (toggle owner-only/never charges/no key -> 503; reminder exactly once for on/off; success extends from period end + webhook replay; declines bounded to 3 then lapse with retention pinned; late opt-in notice; unknown outcome reconciled never re-charged; 12 concurrent passes -> 1 charge), `internal/database/renewal_test.go` (16-racer claim, exact billing_payments/attempt counts; preconditions). Full `go test -race ./...` clean vs real Postgres+Redis; 000033 down/up round-trip; Docker boot smoke with/without Paystack, bad key fails startup. + +## v0.47 follow-up — Email verification for human accounts — COMPLETE (scope per DEC-241..244; RSK-049 closed) +- Migration 000034 (`humans.email_verified_at`, `email_verification_tokens`); down/up round-trip validated on a disposable database. +- Signup emails a 15-minute link; `POST /v1/auth/verify-email`, `POST /v1/auth/resend-verification` (own per-IP bucket); `email_verified_at` on `/v1/me`. Gates nothing. +- Tests: `internal/humanauth/email_verification_test.go` (signup sends + verify, reuse/unknown/expired generic error, signup survives mail failure and no mailer, resend no-op for unknown/verified, resend supersedes old token, concurrent-resend exactly-one-valid race, failed-resend-keeps-working-link), `internal/api/humanauth_verify_handler_test.go` (generic resend body, 422 code, 429 at burst). + +## v0.47 dashboard — Org API-key management over HTTP — COMPLETE (DEC-245..247) +- Owner-managed create/list/rotate/revoke under `/v1/orgs/{id}/api-keys` (human JWT), reusing the CLI's `auth.Service`. Unblocker for the rest of the dashboard: domains/emails/templates/webhooks/logs routes stay API-key + `requireScope`, and a web-signed-up org can now mint that key itself. +- Tests: `internal/api/apikeys_handler_test.go` (non-member 404 / non-owner 403 table; HTTP-created key authenticates on real `GET /v1/domains`; scope enforced (403 without domains:read); list leaks no key material; rotate kills old key and new key works; revoke kills key; unknown/duplicate/empty scope 422). Rate limiter itself not exercised by a test (mirrors the tested invite limiter). diff --git a/.ilana/risks.md b/.ilana/risks.md index 8a9e290..b8d9cfd 100644 --- a/.ilana/risks.md +++ b/.ilana/risks.md @@ -57,3 +57,4 @@ - RSK-046 [medium, reduced not closed, v0.47 phase 3b/DEC-240]: OAuth `state` is single-use and server-stored but not bound to the initiating browser (no cookie), because start/callback are JSON API endpoints. A login-CSRF (attacker gets a victim to complete the attacker's own callback URL, logging the victim into the attacker's account) is not fully prevented. DEC-240 added a Referer check on `/callback` (reject if present and not the provider's own domain, allow if absent) - meaningfully raises the bar for the practical delivery vector (a crafted link in an email/chat/webpage) at zero cost to legitimate flows, but is bypassable by an attacker page that spoofs or omits Referer via `rel=noreferrer`. Full mitigation still needs client-side state binding (HttpOnly cookie or the dashboard verifying it initiated the flow) once a real frontend exists. - RSK-047 [medium, v0.47 phase 3b]: MFA brute-force limits are per-IP (5 burst, 1/12s) and per-challenge (5 wrong codes). An attacker who already has the password and many IPs can open new challenges (each login is also authIP-limited) and keep guessing; there is no per-account lockout or alert. Losing `MAILX_MFA_MASTER_KEY` makes TOTP unverifiable (backup codes still work). - RSK-048 [medium, v0.47 phase 3c; DEC-240 fixed one sub-item, rest unchanged]: auto-renewal residuals. (1) "Transaction reference not found" from verify is treated as no charge; if Paystack were eventually consistent past the 1h reconcile delay this could permit a retry (not observed; unverified against live Paystack). (2) Charge-endpoint request/response shapes were implemented from Paystack's documented API and tested only against a fake server; needs a live test-mode run before launch - Paystack's docs are bot-blocked from automated fetching (tried during DEC-240's pass), so this could not be re-verified without live credentials; Google's and GitHub's OAuth contracts WERE re-verified against current docs and confirmed correct. (3) An unknown-outcome attempt still pending at period end does not delay the lapse; a later verified success re-activates with a fresh 30-day period. (4) Amount-mismatch successes need manual operator resolution. (5) Renewal requires the system mailer; without it nothing is reminded or charged (safe but silent beyond a startup WARN). (6) No endpoint to delete a saved card (disable auto_renew stops charging but keeps the encrypted authorization). +- RSK-049 [CLOSED before merge, DEC-244]: `IssueEmailVerificationToken` originally superseded the previous link before the email was sent. Fixed by reordering to create-send-then-supersede (same shape as DEC-218) plus the DEC-226 tuple-ordering fix for the supersede step itself, since the naive split would have reintroduced that exact bug too. diff --git a/.ilana/state.json b/.ilana/state.json index 487cdfc..94760a0 100644 --- a/.ilana/state.json +++ b/.ilana/state.json @@ -17,8 +17,8 @@ "TC": 0, "DEF": 27, "CR": 24, - "RSK": 48, - "DEC": 240, + "RSK": 49, + "DEC": 249, "MET": 72, "ETH": 1 }, @@ -34,10 +34,10 @@ "G8": 1 }, "mailx": { - "last_completed_milestone": "v0.47 phase 3c (plan auto-renewal + reminders)", - "ilana_current_through": "v0.47 phase 3c", + "last_completed_milestone": "v0.47 email verification", + "ilana_current_through": "v0.47 email verification", "current_milestone": "v0.47 Human Accounts & Organizations", - "current_milestone_status": "v0.47 phase 1 (human auth, orgs, password reset, invitations), phase 2 (billing), phase 3a (dashboard backend), phase 3b (OAuth + TOTP MFA, DEC-231..234, independently security-reviewed) and phase 3c (opt-in plan auto-renewal + reminders, DEC-235..238, RSK-044 closed, residuals RSK-048; pending live Paystack test-mode verification) complete; leave-org/transfer-ownership, email change, org slug, email verification deferred", + "current_milestone_status": "v0.47 phase 1 (human auth, orgs, password reset, invitations), phase 2 (billing), phase 3a (dashboard backend), phase 3b (OAuth + TOTP MFA, DEC-231..234, independently security-reviewed) and phase 3c (opt-in plan auto-renewal + reminders, DEC-235..238, RSK-044 closed, residuals RSK-048; pending live Paystack test-mode verification) complete; email verification (DEC-241..244, RSK-049 closed) complete; dashboard org API-key management over HTTP (DEC-245..247) complete - the real unblocker for domains/emails/templates/webhooks/logs; leave-org/transfer-ownership, email change, org slug still deferred", "next_milestone": "v0.48 (TBD); live Paystack test-mode verification of auto-renewal before launch (RSK-048)" } } diff --git a/cmd/mailx/abuseconfig.go b/cmd/mailx/abuseconfig.go index 092451b..11088d4 100644 --- a/cmd/mailx/abuseconfig.go +++ b/cmd/mailx/abuseconfig.go @@ -93,10 +93,14 @@ func loadAbusePolicy(get func(string) string) (policy ratelimit.Policy, enabled n("MAILX_LIMIT_AUTH_IP_BURST", &policy.AuthIPBurst) f("MAILX_LIMIT_PASSWORD_RESET_IP_RPS", &policy.PasswordResetIPRate) n("MAILX_LIMIT_PASSWORD_RESET_IP_BURST", &policy.PasswordResetIPBurst) + f("MAILX_LIMIT_EMAIL_VERIFICATION_RESEND_IP_RPS", &policy.EmailVerificationResendIPRate) + n("MAILX_LIMIT_EMAIL_VERIFICATION_RESEND_IP_BURST", &policy.EmailVerificationResendIPBurst) f("MAILX_LIMIT_MFA_VERIFY_IP_RPS", &policy.MFAVerifyIPRate) n("MAILX_LIMIT_MFA_VERIFY_IP_BURST", &policy.MFAVerifyIPBurst) f("MAILX_LIMIT_ORG_INVITE_RPS", &policy.OrgInviteRate) n("MAILX_LIMIT_ORG_INVITE_BURST", &policy.OrgInviteBurst) + f("MAILX_LIMIT_APIKEY_MINT_RPS", &policy.APIKeyMintRate) + n("MAILX_LIMIT_APIKEY_MINT_BURST", &policy.APIKeyMintBurst) n("MAILX_RETRY_JITTER_PERCENT", &policy.RetryJitterPercent) if raw := strings.TrimSpace(get("MAILX_LIMIT_PERMIT_TTL")); raw != "" { d, e := time.ParseDuration(raw) diff --git a/internal/api/abuse.go b/internal/api/abuse.go index 6003cab..0286ac2 100644 --- a/internal/api/abuse.go +++ b/internal/api/abuse.go @@ -203,6 +203,43 @@ func passwordResetIPLimitMiddleware(a *AbuseControls) func(http.Handler) http.Ha } } +// emailVerificationResendIPLimitMiddleware protects POST +// /v1/auth/resend-verification with its own bucket, same tightness class +// as passwordResetIPLimitMiddleware: every call can send a real email. +func emailVerificationResendIPLimitMiddleware(a *AbuseControls) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + if a == nil || a.Limiter == nil { + return next + } + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ip := authClientIP(r, a.TrustedProxyCIDRs) + p := a.Policy + dec, err := a.Limiter.Allow(r.Context(), + ratelimit.Bucket{Key: "auth:verify-resend:ip:" + ip, Rate: p.EmailVerificationResendIPRate, Burst: p.EmailVerificationResendIPBurst, Cost: 1}, + ) + switch { + case err != nil: + a.Metrics.AbuseDecision("email_verification_resend", "unavailable") + a.log().Error("rate_limiter_unavailable", "route_class", "email_verification_resend") + e := newError(ErrTemporarilyUnavailable, "rate_limiter_unavailable", "request limiting is temporarily unavailable; retry later") + e.RetryAfter = unavailableRetryAfter + writeError(w, r, e) + case dec.Impossible: + a.Metrics.AbuseDecision("email_verification_resend", "impossible") + writeError(w, r, newError(ErrInternal, "internal_error", "request limit is misconfigured")) + case !dec.Allowed: + a.Metrics.AbuseDecision("email_verification_resend", "limited") + e := newError(ErrRateLimited, "email_verification_resend_rate_limited", "too many verification email requests from this address; retry after the interval in Retry-After") + e.RetryAfter = ratelimit.RetryAfterSeconds(dec.RetryAfter) + writeError(w, r, e) + default: + a.Metrics.AbuseDecision("email_verification_resend", "allowed") + next.ServeHTTP(w, r) + } + }) + } +} + // orgInviteLimitMiddleware protects POST /v1/orgs/{id}/invites with its own // tighter bucket, keyed by the inviting human's ID rather than IP — unlike // the auth-surface middlewares above, this route is already authenticated diff --git a/internal/api/apikeys_handler.go b/internal/api/apikeys_handler.go new file mode 100644 index 0000000..8752972 --- /dev/null +++ b/internal/api/apikeys_handler.go @@ -0,0 +1,255 @@ +package api + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "strings" + "time" + + "github.com/Ferousco-dev/mailx/internal/auth" + "github.com/Ferousco-dev/mailx/internal/database" + "github.com/Ferousco-dev/mailx/internal/ratelimit" +) + +// apiKeyManager is the subset of *auth.Service the org API-key routes need. +// These routes call the SAME Create/Rotate/Revoke/List the CLI +// (cmd/mailx/apikeys.go) uses, so key generation, hashing, and storage exist +// in exactly one place (DEC-245). +type apiKeyManager interface { + Create(ctx context.Context, tenantID, name string, scopes []string, ttl *time.Duration) (auth.Generated, database.APIKey, error) + Rotate(ctx context.Context, keyID string, grace time.Duration) (auth.Generated, database.APIKey, error) + Revoke(ctx context.Context, keyID string) error + List(ctx context.Context, tenantID string) ([]database.APIKey, error) +} + +// orgAPIKeyHandler serves /v1/orgs/{id}/api-keys: human-JWT authenticated, +// authorized with dashboardHandler.orgAccess (member for list, owner for +// create/rotate/revoke — DEC-228 posture). The org is the tenant (DEC-205). +type orgAPIKeyHandler struct { + dash *dashboardHandler + keys apiKeyManager +} + +const maxAPIKeyNameLen = 200 + +type apiKeyResource struct { + ID string `json:"id"` + Name string `json:"name"` + Scopes []string `json:"scopes"` + Status string `json:"status"` + CreatedAt string `json:"created_at"` + LastUsedAt *string `json:"last_used_at"` + ExpiresAt *string `json:"expires_at"` + RevokedAt *string `json:"revoked_at"` +} + +type createdAPIKeyResource struct { + apiKeyResource + // Key is the raw credential. It is returned only by create/rotate and + // is never stored or retrievable again. + Key string `json:"key"` +} + +func fmtTimePtr(t *time.Time) *string { + if t == nil { + return nil + } + s := t.UTC().Format(time.RFC3339) + return &s +} + +func toAPIKeyResource(k database.APIKey, now time.Time) apiKeyResource { + status := "active" + switch { + case k.RevokedAt != nil: + status = "revoked" + case k.ExpiresAt != nil && !k.ExpiresAt.After(now): + status = "expired" + } + scopes := k.Scopes + if scopes == nil { + scopes = []string{} + } + return apiKeyResource{ + ID: k.KeyID, Name: k.Name, Scopes: scopes, Status: status, + CreatedAt: k.CreatedAt.UTC().Format(time.RFC3339), + LastUsedAt: fmtTimePtr(k.LastUsedAt), ExpiresAt: fmtTimePtr(k.ExpiresAt), RevokedAt: fmtTimePtr(k.RevokedAt), + } +} + +type createAPIKeyBody struct { + Name string `json:"name"` + Scopes []string `json:"scopes"` +} + +func (h *orgAPIKeyHandler) handleCreate(w http.ResponseWriter, r *http.Request) { + _, tenantID, ok := h.dash.orgAccess(w, r, true) + if !ok { + return + } + if !acceptsJSONContentType(r.Header.Get("Content-Type")) { + writeError(w, r, newError(ErrUnsupportedMediaType, "unsupported_media_type", "Content-Type must be application/json")) + return + } + var b createAPIKeyBody + if err := json.NewDecoder(http.MaxBytesReader(w, r.Body, maxBodyBytes)).Decode(&b); err != nil { + writeError(w, r, newError(ErrInvalidRequest, "invalid_json", "request body is not valid JSON")) + return + } + name := strings.TrimSpace(b.Name) + if name == "" || len(name) > maxAPIKeyNameLen { + writeError(w, r, newError(ErrValidation, "invalid_name", "name must be 1-200 characters")) + return + } + if len(b.Scopes) == 0 { + writeError(w, r, newError(ErrValidation, "invalid_scopes", "at least one scope is required")) + return + } + seen := make(map[string]bool, len(b.Scopes)) + for _, s := range b.Scopes { + if !auth.ValidScope(s) { + writeError(w, r, newError(ErrValidation, "invalid_scopes", "unknown scope: "+s)) + return + } + if seen[s] { + writeError(w, r, newError(ErrValidation, "invalid_scopes", "duplicate scope: "+s)) + return + } + seen[s] = true + } + gen, rec, err := h.keys.Create(r.Context(), tenantID, name, b.Scopes, nil) + if err != nil { + writeError(w, r, newError(ErrInternal, "internal_error", "failed to create api key")) + return + } + writeJSON(w, http.StatusCreated, createdAPIKeyResource{apiKeyResource: toAPIKeyResource(rec, h.dash.now()), Key: gen.Raw}) +} + +func (h *orgAPIKeyHandler) handleList(w http.ResponseWriter, r *http.Request) { + _, tenantID, ok := h.dash.orgAccess(w, r, false) + if !ok { + return + } + keys, err := h.keys.List(r.Context(), tenantID) + if err != nil { + writeError(w, r, newError(ErrInternal, "internal_error", "failed to list api keys")) + return + } + now := h.dash.now() + out := make([]apiKeyResource, 0, len(keys)) + for _, k := range keys { + out = append(out, toAPIKeyResource(k, now)) + } + writeJSON(w, http.StatusOK, map[string]any{"data": out}) +} + +// orgKey loads {keyId} and confirms it belongs to the {id} org. A key of +// another tenant is indistinguishable from a nonexistent one (404). +func (h *orgAPIKeyHandler) orgKey(w http.ResponseWriter, r *http.Request, tenantID string) (database.APIKey, bool) { + k, err := h.dash.db.GetAPIKeyByKeyID(r.Context(), r.PathValue("keyId")) + if errors.Is(err, database.ErrNotFound) || (err == nil && k.TenantID != tenantID) { + writeError(w, r, newError(ErrNotFoundType, "api_key_not_found", "api key not found")) + return database.APIKey{}, false + } + if err != nil { + writeError(w, r, newError(ErrInternal, "internal_error", "failed to load api key")) + return database.APIKey{}, false + } + return k, true +} + +func (h *orgAPIKeyHandler) handleRotate(w http.ResponseWriter, r *http.Request) { + _, tenantID, ok := h.dash.orgAccess(w, r, true) + if !ok { + return + } + k, ok := h.orgKey(w, r, tenantID) + if !ok { + return + } + now := h.dash.now() + if k.RevokedAt != nil || (k.ExpiresAt != nil && !k.ExpiresAt.After(now)) { + writeError(w, r, newError(ErrConflictType, "api_key_not_active", "only an active api key can be rotated; create a new one instead")) + return + } + // Grace 0: the old key stops authenticating immediately (DEC-247). + gen, rec, err := h.keys.Rotate(r.Context(), k.KeyID, 0) + if err != nil { + if errors.Is(err, database.ErrConflict) || errors.Is(err, database.ErrNotFound) || + errors.Is(err, auth.ErrRevoked) || errors.Is(err, auth.ErrExpired) { + // Lost a race where the key was revoked/expired (by a + // concurrent revoke, or simply reaching its expiry) between + // this handler's own unlocked pre-check above and the actual + // Rotate call - auth.Service.Rotate wraps ErrRevoked/ErrExpired + // for exactly this case (CodeRabbit, PR #28); previously this + // fell through to a generic 500 instead of the correct 409. + writeError(w, r, newError(ErrConflictType, "api_key_not_active", "only an active api key can be rotated; create a new one instead")) + return + } + writeError(w, r, newError(ErrInternal, "internal_error", "failed to rotate api key")) + return + } + writeJSON(w, http.StatusOK, createdAPIKeyResource{apiKeyResource: toAPIKeyResource(rec, now), Key: gen.Raw}) +} + +// handleRevoke matches the CLI's revoke-api-key semantics exactly: revoking +// an already-revoked key reports not-found (database.RevokeAPIKey only +// updates rows WHERE revoked_at IS NULL) and changes nothing (DEC-247). +func (h *orgAPIKeyHandler) handleRevoke(w http.ResponseWriter, r *http.Request) { + _, tenantID, ok := h.dash.orgAccess(w, r, true) + if !ok { + return + } + k, ok := h.orgKey(w, r, tenantID) + if !ok { + return + } + err := h.keys.Revoke(r.Context(), k.KeyID) + switch { + case err == nil: + w.WriteHeader(http.StatusNoContent) + case errors.Is(err, database.ErrNotFound): + writeError(w, r, newError(ErrNotFoundType, "api_key_not_found", "api key not found or already revoked")) + default: + writeError(w, r, newError(ErrInternal, "internal_error", "failed to revoke api key")) + } +} + +// apiKeyMintLimitMiddleware bounds POST create/rotate per human, mirroring +// orgInviteLimitMiddleware (DEC-247): an authenticated, owner-gated, +// occasional mutation keyed on the verified human ID rather than IP. +func apiKeyMintLimitMiddleware(a *AbuseControls) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + if a == nil || a.Limiter == nil { + return next + } + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + humanID, _ := humanIDFromContext(r.Context()) + p := a.Policy + dec, err := a.Limiter.Allow(r.Context(), + ratelimit.Bucket{Key: "org:apikey:human:" + humanID, Rate: p.APIKeyMintRate, Burst: p.APIKeyMintBurst, Cost: 1}, + ) + switch { + case err != nil: + a.Metrics.AbuseDecision("api_key_mint", "unavailable") + a.log().Error("rate_limiter_unavailable", "route_class", "api_key_mint") + e := newError(ErrTemporarilyUnavailable, "rate_limiter_unavailable", "request limiting is temporarily unavailable; retry later") + e.RetryAfter = unavailableRetryAfter + writeError(w, r, e) + case dec.Impossible: + a.Metrics.AbuseDecision("api_key_mint", "impossible") + writeError(w, r, newError(ErrInternal, "internal_error", "request limit is misconfigured")) + case !dec.Allowed: + a.Metrics.AbuseDecision("api_key_mint", "limited") + e := newError(ErrRateLimited, "api_key_rate_limited", "too many api keys created or rotated; retry after the interval in Retry-After") + e.RetryAfter = ratelimit.RetryAfterSeconds(dec.RetryAfter) + writeError(w, r, e) + default: + a.Metrics.AbuseDecision("api_key_mint", "allowed") + next.ServeHTTP(w, r) + } + }) + } +} diff --git a/internal/api/apikeys_handler_test.go b/internal/api/apikeys_handler_test.go new file mode 100644 index 0000000..9a5b4a0 --- /dev/null +++ b/internal/api/apikeys_handler_test.go @@ -0,0 +1,164 @@ +package api + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// callDomains hits a real requireScope(domains:read) route with an API key. +func (d dashAPI) callDomains(t *testing.T, rawKey string) int { + t.Helper() + req := httptest.NewRequest("GET", "/v1/domains", nil) + req.Header.Set("Authorization", "Bearer "+rawKey) + rec := httptest.NewRecorder() + d.mux.ServeHTTP(rec, req) + return rec.Code +} + +func createOrgKey(t *testing.T, d dashAPI, f dashFixture, scopes []string) map[string]any { + t.Helper() + rec := d.do(t, "POST", "/v1/orgs/"+f.orgID+"/api-keys", f.owner.AccessToken, map[string]any{"name": "ci", "scopes": scopes}) + if rec.Code != http.StatusCreated { + t.Fatalf("create: %d %s", rec.Code, rec.Body) + } + return decodeJSON(t, rec) +} + +func TestOrgAPIKeysAuthorization(t *testing.T) { + d := newDashAPI(t) + f := newDashFixture(t, d) + key := createOrgKey(t, d, f, []string{"domains:read"}) + kid := key["id"].(string) + base := "/v1/orgs/" + f.orgID + "/api-keys" + body := map[string]any{"name": "x", "scopes": []string{"domains:read"}} + cases := []struct { + name, method, path, token string + body any + want int + }{ + {"outsider list", "GET", base, f.outsider.AccessToken, nil, http.StatusNotFound}, + {"outsider create", "POST", base, f.outsider.AccessToken, body, http.StatusNotFound}, + {"outsider rotate", "POST", base + "/" + kid + "/rotate", f.outsider.AccessToken, nil, http.StatusNotFound}, + {"outsider revoke", "DELETE", base + "/" + kid, f.outsider.AccessToken, nil, http.StatusNotFound}, + {"nonexistent org", "GET", "/v1/orgs/00000000-0000-0000-0000-000000000000/api-keys", f.owner.AccessToken, nil, http.StatusNotFound}, + {"member create", "POST", base, f.member.AccessToken, body, http.StatusForbidden}, + {"member rotate", "POST", base + "/" + kid + "/rotate", f.member.AccessToken, nil, http.StatusForbidden}, + {"member revoke", "DELETE", base + "/" + kid, f.member.AccessToken, nil, http.StatusForbidden}, + {"member list", "GET", base, f.member.AccessToken, nil, http.StatusOK}, + {"no token", "GET", base, "", nil, http.StatusUnauthorized}, + {"unknown key", "DELETE", base + "/mx_nope", f.owner.AccessToken, nil, http.StatusNotFound}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if rec := d.do(t, c.method, c.path, c.token, c.body); rec.Code != c.want { + t.Fatalf("got %d want %d: %s", rec.Code, c.want, rec.Body) + } + }) + } + // The member/outsider attempts must not have revoked or rotated anything. + if code := d.callDomains(t, key["key"].(string)); code != http.StatusOK { + t.Fatalf("key disturbed by unauthorized calls: %d", code) + } +} + +func TestOrgAPIKeyCreateWorksAndListNeverLeaks(t *testing.T) { + d := newDashAPI(t) + f := newDashFixture(t, d) + key := createOrgKey(t, d, f, []string{"domains:read", "emails:read"}) + raw, _ := key["key"].(string) + if raw == "" || key["status"] != "active" || key["id"] == "" { + t.Fatalf("bad create response: %v", key) + } + if _, ok := key["secret_hash"]; ok { + t.Fatal("create leaked hash") + } + // A real, working API key on a real requireScope route. + if code := d.callDomains(t, raw); code != http.StatusOK { + t.Fatalf("HTTP-created key rejected by /v1/domains: %d", code) + } + // Scopes are honored: a key without domains:read is refused. + narrow := createOrgKey(t, d, f, []string{"emails:read"}) + if code := d.callDomains(t, narrow["key"].(string)); code != http.StatusForbidden { + t.Fatalf("scope not enforced: %d", code) + } + + rec := d.do(t, "GET", "/v1/orgs/"+f.orgID+"/api-keys", f.member.AccessToken, nil) + if rec.Code != http.StatusOK { + t.Fatalf("list: %d", rec.Code) + } + body := rec.Body.String() + secret := raw[strings.LastIndex(raw, "_")+1:] + if strings.Contains(body, raw) || strings.Contains(body, secret) || strings.Contains(body, "hash") || strings.Contains(body, `"key"`) { + t.Fatalf("list leaked key material: %s", body) + } + if n := len(decodeJSON(t, rec)["data"].([]any)); n != 2 { + t.Fatalf("want 2 keys, got %d", n) + } +} + +func TestOrgAPIKeyRotateAndRevoke(t *testing.T) { + d := newDashAPI(t) + f := newDashFixture(t, d) + base := "/v1/orgs/" + f.orgID + "/api-keys/" + old := createOrgKey(t, d, f, []string{"domains:read"}) + + rec := d.do(t, "POST", base+old["id"].(string)+"/rotate", f.owner.AccessToken, nil) + if rec.Code != http.StatusOK { + t.Fatalf("rotate: %d %s", rec.Code, rec.Body) + } + nk := decodeJSON(t, rec) + if nk["id"] == old["id"] || nk["key"] == "" { + t.Fatalf("bad rotate response: %v", nk) + } + if code := d.callDomains(t, old["key"].(string)); code != http.StatusUnauthorized { + t.Fatalf("old key still works after rotate: %d", code) + } + if code := d.callDomains(t, nk["key"].(string)); code != http.StatusOK { + t.Fatalf("rotated key rejected: %d", code) + } + // The rotated-out key cannot be rotated again. + if rec := d.do(t, "POST", base+old["id"].(string)+"/rotate", f.owner.AccessToken, nil); rec.Code != http.StatusConflict { + t.Fatalf("rotate dead key: %d", rec.Code) + } + + if rec := d.do(t, "DELETE", base+nk["id"].(string), f.owner.AccessToken, nil); rec.Code != http.StatusNoContent { + t.Fatalf("revoke: %d %s", rec.Code, rec.Body) + } + if code := d.callDomains(t, nk["key"].(string)); code != http.StatusUnauthorized { + t.Fatalf("revoked key still works: %d", code) + } + // CLI-matching semantics: a second revoke is not-found, no state change. + if rec := d.do(t, "DELETE", base+nk["id"].(string), f.owner.AccessToken, nil); rec.Code != http.StatusNotFound { + t.Fatalf("second revoke: %d", rec.Code) + } +} + +func TestOrgAPIKeyCreateValidation(t *testing.T) { + d := newDashAPI(t) + f := newDashFixture(t, d) + path := "/v1/orgs/" + f.orgID + "/api-keys" + for name, b := range map[string]map[string]any{ + "unknown scope": {"name": "x", "scopes": []string{"domains:read", "admin:all"}}, + "duplicate scope": {"name": "x", "scopes": []string{"domains:read", "domains:read"}}, + "no scopes": {"name": "x", "scopes": []string{}}, + "blank name": {"name": " ", "scopes": []string{"domains:read"}}, + } { + t.Run(name, func(t *testing.T) { + rec := d.do(t, "POST", path, f.owner.AccessToken, b) + if rec.Code != http.StatusUnprocessableEntity { + t.Fatalf("got %d: %s", rec.Code, rec.Body) + } + }) + } + rec := d.do(t, "POST", path, f.owner.AccessToken, map[string]any{"name": "x", "scopes": []string{"admin:all"}}) + if !strings.Contains(rec.Body.String(), "admin:all") || !strings.Contains(rec.Body.String(), "invalid_scopes") { + t.Fatalf("error not clear: %s", rec.Body) + } + // Nothing was created by the rejected requests. + list := d.do(t, "GET", path, f.owner.AccessToken, nil) + if n := len(decodeJSON(t, list)["data"].([]any)); n != 0 { + t.Fatalf("rejected requests created %d keys", n) + } +} diff --git a/internal/api/dashboard_handler.go b/internal/api/dashboard_handler.go index 2a77f94..7da29fa 100644 --- a/internal/api/dashboard_handler.go +++ b/internal/api/dashboard_handler.go @@ -107,13 +107,15 @@ func rfc3339Ptr(t *time.Time) *string { } type meResponse struct { - ID string `json:"id"` - Name string `json:"name"` - Email string `json:"email"` - AvatarURL *string `json:"avatar_url"` - LastLoginAt *string `json:"last_login_at"` - CreatedAt string `json:"created_at"` - Organizations []orgResource `json:"organizations"` + ID string `json:"id"` + Name string `json:"name"` + Email string `json:"email"` + AvatarURL *string `json:"avatar_url"` + LastLoginAt *string `json:"last_login_at"` + // EmailVerifiedAt is null until verified; informational only (DEC-243). + EmailVerifiedAt *string `json:"email_verified_at"` + CreatedAt string `json:"created_at"` + Organizations []orgResource `json:"organizations"` } func (h *dashboardHandler) writeMe(w http.ResponseWriter, r *http.Request, humanID string) { @@ -137,7 +139,7 @@ func (h *dashboardHandler) writeMe(w http.ResponseWriter, r *http.Request, human } writeJSON(w, http.StatusOK, meResponse{ ID: hu.ID, Name: hu.Name, Email: hu.Email, AvatarURL: hu.AvatarURL, - LastLoginAt: rfc3339Ptr(hu.LastLoginAt), CreatedAt: hu.CreatedAt.UTC().Format(time.RFC3339), + LastLoginAt: rfc3339Ptr(hu.LastLoginAt), EmailVerifiedAt: rfc3339Ptr(hu.EmailVerifiedAt), CreatedAt: hu.CreatedAt.UTC().Format(time.RFC3339), Organizations: orgs, }) } diff --git a/internal/api/humanauth_handler.go b/internal/api/humanauth_handler.go index 12e15aa..815612e 100644 --- a/internal/api/humanauth_handler.go +++ b/internal/api/humanauth_handler.go @@ -208,6 +208,53 @@ func (h *humanAuthHandler) handleResetPassword(w http.ResponseWriter, r *http.Re writeJSON(w, http.StatusOK, map[string]string{"message": "password updated; all other sessions have been signed out"}) } +type verifyEmailRequest struct { + Token string `json:"token"` +} + +// handleVerifyEmail is public: the link may be opened on a device with no +// session. Every token failure collapses to one generic error. +func (h *humanAuthHandler) handleVerifyEmail(w http.ResponseWriter, r *http.Request) { + if !acceptsJSONContentType(r.Header.Get("Content-Type")) { + writeError(w, r, newError(ErrUnsupportedMediaType, "unsupported_media_type", "Content-Type must be application/json")) + return + } + var req verifyEmailRequest + if err := json.NewDecoder(http.MaxBytesReader(w, r.Body, maxBodyBytes)).Decode(&req); err != nil || req.Token == "" { + writeError(w, r, newError(ErrInvalidRequest, "invalid_json", "token is required")) + return + } + if err := h.svc.VerifyEmail(r.Context(), req.Token); err != nil { + if errors.Is(err, humanauth.ErrEmailVerificationTokenInvalid) { + writeError(w, r, newError(ErrValidation, "invalid_verification_token", "this verification link is invalid or has expired")) + return + } + slog.Default().Error("verify_email_failed", "error", err.Error()) + writeError(w, r, newError(ErrInternal, "internal_error", "could not verify email")) + return + } + writeJSON(w, http.StatusOK, map[string]string{"message": "email verified"}) +} + +// handleResendVerification ALWAYS responds with the same generic message +// (unknown, already verified, sent, or internal failure) - same +// anti-enumeration posture as handleForgotPassword. +func (h *humanAuthHandler) handleResendVerification(w http.ResponseWriter, r *http.Request) { + if !acceptsJSONContentType(r.Header.Get("Content-Type")) { + writeError(w, r, newError(ErrUnsupportedMediaType, "unsupported_media_type", "Content-Type must be application/json")) + return + } + var req forgotPasswordRequest + if err := json.NewDecoder(http.MaxBytesReader(w, r.Body, maxBodyBytes)).Decode(&req); err != nil || req.Email == "" { + writeError(w, r, newError(ErrInvalidRequest, "invalid_json", "email is required")) + return + } + if err := h.svc.ResendVerification(r.Context(), req.Email); err != nil { + slog.Default().Error("resend_verification_failed", "error", err.Error()) + } + writeJSON(w, http.StatusOK, map[string]string{"message": "if an unverified account exists for that email, a verification link has been sent"}) +} + type createOrgRequest struct { Name string `json:"name"` Slug string `json:"slug"` diff --git a/internal/api/humanauth_verify_handler_test.go b/internal/api/humanauth_verify_handler_test.go new file mode 100644 index 0000000..2745cc6 --- /dev/null +++ b/internal/api/humanauth_verify_handler_test.go @@ -0,0 +1,64 @@ +package api + +import ( + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/Ferousco-dev/mailx/internal/auth" + "github.com/Ferousco-dev/mailx/internal/humanauth" + "github.com/Ferousco-dev/mailx/internal/storage" +) + +// Resend-verification: identical 200 body for unknown vs existing +// accounts (anti-enumeration), then 429 past its own per-IP burst. +// Verify-email: unknown token is the generic 422. +func TestEmailVerificationHandlersAndResendRateLimit(t *testing.T) { + rig := newAbuseRig(t, testPolicy()) + svc, err := humanauth.NewService(rig.db, []byte("test-secret-at-least-32-bytes-long!!")) + if err != nil { + t.Fatal(err) + } + if _, err := svc.SignUp(t.Context(), "Ada", "ada@example.com", "hunter22hunter"); err != nil { + t.Fatal(err) + } + store, err := storage.NewFileStore(t.TempDir()) + if err != nil { + t.Fatal(err) + } + p := testPolicy() + ac := &AbuseControls{Limiter: rig.limit, Policy: p} + mux := newMux(newEmailHandler(rig.db, store), auth.NewService(rig.db, nil), func() error { return nil }, routeServices{abuse: ac, humanAuth: svc}) + + // Unique IP per run: the Redis limiter state outlives a single test run. + ip := fmt.Sprintf("198.51.100.%d:1234", time.Now().UnixMicro()%250+1) + do := func(path, body string) (int, string) { + req := httptest.NewRequest("POST", path, strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req.RemoteAddr = ip + w := httptest.NewRecorder() + mux.ServeHTTP(w, req) + return w.Code, w.Body.String() + } + + if c, body := do("/v1/auth/verify-email", `{"token":"not-a-real-token"}`); c != http.StatusUnprocessableEntity || !strings.Contains(body, "invalid_verification_token") { + t.Fatalf("unknown token: got %d %s", c, body) + } + + c1, b1 := do("/v1/auth/resend-verification", `{"email":"ada@example.com"}`) + c2, b2 := do("/v1/auth/resend-verification", `{"email":"nobody@example.com"}`) + if c1 != http.StatusOK || c2 != http.StatusOK || b1 != b2 { + t.Fatalf("resend must be generic: %d %q vs %d %q", c1, b1, c2, b2) + } + for i := 2; i < p.EmailVerificationResendIPBurst; i++ { + if c, _ := do("/v1/auth/resend-verification", `{"email":"nobody@example.com"}`); c != http.StatusOK { + t.Fatalf("attempt %d: got %d want 200", i, c) + } + } + if c, body := do("/v1/auth/resend-verification", `{"email":"nobody@example.com"}`); c != http.StatusTooManyRequests || !strings.Contains(body, "email_verification_resend_rate_limited") { + t.Fatalf("over burst: got %d %s want 429", c, body) + } +} diff --git a/internal/api/openapi.go b/internal/api/openapi.go index efd969b..7ea8d39 100644 --- a/internal/api/openapi.go +++ b/internal/api/openapi.go @@ -197,6 +197,33 @@ const openAPISpec = `{ } } }, + "/auth/verify-email": { + "post": { + "summary": "Verify an account's email using a verification token", + "description": "Public (no auth required; the link may be opened on a device with no session). The token is single-use, expires 15 minutes after issuance, and is superseded by any later resend. On success sets the account's email_verified_at. Unknown, expired, used, and superseded tokens all return the same 422. Verification is informational only; it gates nothing.", + "security": [], + "requestBody": {"required": true, "content": {"application/json": {"schema": {"type": "object", "required": ["token"], "properties": {"token": {"type": "string"}}}}}}, + "responses": { + "200": {"description": "OK", "content": {"application/json": {"schema": {"type": "object", "properties": {"message": {"type": "string"}}}}}}, + "400": {"description": "Missing token or invalid JSON", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, + "422": {"description": "The verification token is invalid/expired/used (code invalid_verification_token)", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, + "429": {"description": "Rate limited (per client IP, auth bucket)", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}} + } + } + }, + "/auth/resend-verification": { + "post": { + "summary": "Resend the account verification email", + "description": "Public (no auth required). Always returns the same generic response whether the email is unknown, already verified, or unverified, to avoid account enumeration. For an unverified account, invalidates any pending verification token and emails a new 15-minute link. Rate-limited per client IP with its own tight bucket (same class as forgot-password).", + "security": [], + "requestBody": {"required": true, "content": {"application/json": {"schema": {"$ref": "#/components/schemas/ForgotPasswordRequest"}}}}, + "responses": { + "200": {"description": "OK (generic; does not reveal whether the account exists or is verified)", "content": {"application/json": {"schema": {"type": "object", "properties": {"message": {"type": "string"}}}}}}, + "400": {"description": "Missing email or invalid JSON", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, + "429": {"description": "Rate limited (code email_verification_resend_rate_limited)", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}} + } + } + }, "/orgs": { "post": { "summary": "Create an organization", @@ -245,14 +272,14 @@ const openAPISpec = `{ "summary": "Get the caller's profile and organizations", "description": "Requires a human access token (HumanAuth).", "security": [{"HumanAuth": []}], - "responses": { "200": {"description": "OK", "content": {"application/json": {"schema": {"type": "object", "properties": {"id": {"type": "string"}, "name": {"type": "string"}, "email": {"type": "string"}, "avatar_url": {"type": "string", "nullable": true}, "last_login_at": {"type": "string", "format": "date-time", "nullable": true}, "created_at": {"type": "string", "format": "date-time"}, "organizations": {"type": "array", "items": {"$ref": "#/components/schemas/Organization"}}}}}}}, "401": {"description": "Missing or invalid access token", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}} } + "responses": { "200": {"description": "OK", "content": {"application/json": {"schema": {"type": "object", "properties": {"id": {"type": "string"}, "name": {"type": "string"}, "email": {"type": "string"}, "avatar_url": {"type": "string", "nullable": true}, "last_login_at": {"type": "string", "format": "date-time", "nullable": true}, "email_verified_at": {"type": "string", "format": "date-time", "nullable": true, "description": "Null until the account's email is verified. Informational only; gates nothing."}, "created_at": {"type": "string", "format": "date-time"}, "organizations": {"type": "array", "items": {"$ref": "#/components/schemas/Organization"}}}}}}}, "401": {"description": "Missing or invalid access token", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}} } }, "patch": { "summary": "Update the caller's name and/or avatar_url", "description": "Requires a human access token (HumanAuth). Omitted fields are unchanged. Email cannot be changed here.", "security": [{"HumanAuth": []}], "requestBody": {"required": true, "content": {"application/json": {"schema": {"type": "object", "properties": {"name": {"type": "string", "minLength": 1, "maxLength": 200}, "avatar_url": {"type": "string", "description": "Absolute http(s) URL, max 2048 chars; empty string clears it."}}}}}}, - "responses": { "200": {"description": "OK", "content": {"application/json": {"schema": {"type": "object", "properties": {"id": {"type": "string"}, "name": {"type": "string"}, "email": {"type": "string"}, "avatar_url": {"type": "string", "nullable": true}, "last_login_at": {"type": "string", "format": "date-time", "nullable": true}, "created_at": {"type": "string", "format": "date-time"}, "organizations": {"type": "array", "items": {"$ref": "#/components/schemas/Organization"}}}}}}}, "401": {"description": "Missing or invalid access token", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, "422": {"description": "Invalid name or URL", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}} } + "responses": { "200": {"description": "OK", "content": {"application/json": {"schema": {"type": "object", "properties": {"id": {"type": "string"}, "name": {"type": "string"}, "email": {"type": "string"}, "avatar_url": {"type": "string", "nullable": true}, "last_login_at": {"type": "string", "format": "date-time", "nullable": true}, "email_verified_at": {"type": "string", "format": "date-time", "nullable": true, "description": "Null until the account's email is verified. Informational only; gates nothing."}, "created_at": {"type": "string", "format": "date-time"}, "organizations": {"type": "array", "items": {"$ref": "#/components/schemas/Organization"}}}}}}}, "401": {"description": "Missing or invalid access token", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, "422": {"description": "Invalid name or URL", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}} } } }, "/orgs/{id}": { @@ -317,6 +344,41 @@ const openAPISpec = `{ "responses": { "200": {"description": "OK", "content": {"application/json": {"schema": {"type": "array", "items": {"$ref": "#/components/schemas/AnalyticsBucket"}}}}}, "401": {"description": "Missing or invalid access token", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, "404": {"description": "Organization not found or caller is not a member", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, "422": {"description": "Invalid range or interval", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}} } } }, + "/orgs/{id}/api-keys": { + "post": { + "summary": "Create an API key for the organization", + "description": "Owner only (403 not_org_owner for other members, 404 for non-members). Uses the same key generation and hashing as the mailx create-api-key CLI. Every scope must be a known MailX scope (422 invalid_scopes otherwise); an owner may grant any known scope. The raw key is returned ONLY in this response and can never be retrieved again. The key does not expire. Rate-limited per human.", + "security": [{"HumanAuth": []}], + "parameters": [{"name": "id", "in": "path", "required": true, "schema": {"type": "string"}, "description": "Organization (tenant) ID."}], + "requestBody": {"required": true, "content": {"application/json": {"schema": {"$ref": "#/components/schemas/CreateOrgAPIKeyRequest"}}}}, + "responses": { "201": {"description": "Created", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/OrgAPIKeyCreated"}}}}, "401": {"description": "Missing or invalid access token", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, "403": {"description": "Caller is a member but not an owner of this organization", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, "404": {"description": "Organization not found or caller is not a member", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, "422": {"description": "Invalid name or unknown/duplicate/missing scopes", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, "429": {"description": "Too many keys created or rotated by this human (api_key_rate_limited)", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}} } + }, + "get": { + "summary": "List the organization's API keys", + "description": "Any member. Returns active and historical keys (revoked, expired, rotated-out) with metadata only; never the key value or its hash.", + "security": [{"HumanAuth": []}], + "parameters": [{"name": "id", "in": "path", "required": true, "schema": {"type": "string"}, "description": "Organization (tenant) ID."}], + "responses": { "200": {"description": "OK", "content": {"application/json": {"schema": {"type": "object", "properties": {"data": {"type": "array", "items": {"$ref": "#/components/schemas/OrgAPIKey"}}}}}}}, "401": {"description": "Missing or invalid access token", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, "404": {"description": "Organization not found or caller is not a member", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}} } + } + }, + "/orgs/{id}/api-keys/{keyId}/rotate": { + "post": { + "summary": "Rotate an API key", + "description": "Owner only. Issues a new key with the same name and scopes and invalidates the old key immediately (no grace period). The new raw key is returned only in this response. Rotating a revoked or expired key is rejected (409 api_key_not_active). Rate-limited per human.", + "security": [{"HumanAuth": []}], + "parameters": [{"name": "id", "in": "path", "required": true, "schema": {"type": "string"}, "description": "Organization (tenant) ID."}, {"name": "keyId", "in": "path", "required": true, "schema": {"type": "string"}, "description": "The key's public id (key_id)."}], + "responses": { "200": {"description": "Rotated", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/OrgAPIKeyCreated"}}}}, "401": {"description": "Missing or invalid access token", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, "403": {"description": "Caller is a member but not an owner of this organization", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, "404": {"description": "Organization or api key not found", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, "409": {"description": "Key is revoked or expired", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, "429": {"description": "Too many keys created or rotated by this human (api_key_rate_limited)", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}} } + } + }, + "/orgs/{id}/api-keys/{keyId}": { + "delete": { + "summary": "Revoke an API key", + "description": "Owner only. The key stops authenticating immediately. Matches the mailx revoke-api-key CLI: revoking an already-revoked key returns 404 api_key_not_found and changes nothing.", + "security": [{"HumanAuth": []}], + "parameters": [{"name": "id", "in": "path", "required": true, "schema": {"type": "string"}, "description": "Organization (tenant) ID."}, {"name": "keyId", "in": "path", "required": true, "schema": {"type": "string"}, "description": "The key's public id (key_id)."}], + "responses": { "204": {"description": "Revoked"}, "401": {"description": "Missing or invalid access token", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, "403": {"description": "Caller is a member but not an owner of this organization", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}}, "404": {"description": "Organization or api key not found (or key already revoked)", "content": {"application/json": {"schema": {"$ref": "#/components/schemas/APIError"}}}} } + } + }, "/orgs/invites/accept": { "post": { "summary": "Accept an organization invitation", @@ -3132,6 +3194,30 @@ const openAPISpec = `{ "data": {"type": "array", "items": {"$ref": "#/components/schemas/Organization"}} } }, + "CreateOrgAPIKeyRequest": { + "type": "object", + "required": ["name", "scopes"], + "properties": { + "name": {"type": "string", "minLength": 1, "maxLength": 200}, + "scopes": {"type": "array", "minItems": 1, "items": {"type": "string", "enum": ["emails:send", "emails:read", "domains:read", "domains:write", "webhooks:read", "webhooks:write", "suppressions:read", "suppressions:write", "templates:read", "templates:write", "contacts:read", "contacts:write", "audiences:read", "audiences:write", "broadcasts:read", "broadcasts:write", "analytics:read"]}} + } + }, + "OrgAPIKey": { + "type": "object", + "properties": { + "id": {"type": "string", "description": "Public key_id (the non-secret prefix part of the key)."}, + "name": {"type": "string"}, + "scopes": {"type": "array", "items": {"type": "string"}}, + "status": {"type": "string", "enum": ["active", "revoked", "expired"]}, + "created_at": {"type": "string", "format": "date-time"}, + "last_used_at": {"type": "string", "format": "date-time", "nullable": true, "description": "Approximate (updated at most every 5 minutes)."}, + "expires_at": {"type": "string", "format": "date-time", "nullable": true}, + "revoked_at": {"type": "string", "format": "date-time", "nullable": true} + } + }, + "OrgAPIKeyCreated": { + "allOf": [{"$ref": "#/components/schemas/OrgAPIKey"}, {"type": "object", "properties": {"key": {"type": "string", "description": "The raw API key. Shown only once; store it now."}}}] + }, "InviteRequest": { "type": "object", "required": ["email"], diff --git a/internal/api/routes.go b/internal/api/routes.go index 0650cee..91ac30b 100644 --- a/internal/api/routes.go +++ b/internal/api/routes.go @@ -185,6 +185,8 @@ func newMux(h *emailHandler, authSvc authService, readiness func() error, extras mux.HandleFunc("POST /v1/auth/logout", ha.handleLogout) mux.Handle("POST /v1/auth/forgot-password", chain(http.HandlerFunc(ha.handleForgotPassword), passwordResetIPLimitMiddleware(abuse))) mux.Handle("POST /v1/auth/reset-password", chain(http.HandlerFunc(ha.handleResetPassword), passwordResetIPLimitMiddleware(abuse))) + mux.Handle("POST /v1/auth/verify-email", chain(http.HandlerFunc(ha.handleVerifyEmail), authIPLimitMiddleware(abuse))) + mux.Handle("POST /v1/auth/resend-verification", chain(http.HandlerFunc(ha.handleResendVerification), emailVerificationResendIPLimitMiddleware(abuse))) orgsAuthenticated := humanAuthMiddleware(extras[0].humanAuth) registerMFAAndOAuthRoutes(mux, ha, orgsAuthenticated, abuse) mux.Handle("POST /v1/orgs", orgsAuthenticated(http.HandlerFunc(ha.handleCreateOrg))) @@ -214,6 +216,15 @@ func newMux(h *emailHandler, authSvc authService, readiness func() error, extras mux.Handle("DELETE /v1/orgs/{id}/invites/{inviteId}", orgsAuthenticated(http.HandlerFunc(dh.handleRevokeInvite))) mux.Handle("GET /v1/orgs/{id}/analytics/overview", orgsAuthenticated(dh.orgAnalytics(analytics.handleOverview))) mux.Handle("GET /v1/orgs/{id}/analytics/timeseries", orgsAuthenticated(dh.orgAnalytics(analytics.handleTimeseries))) + // Org API-key management (DEC-241..243): the same auth.Service the + // CLI uses; create/rotate additionally pass a per-human mint limit. + if km, ok := authSvc.(apiKeyManager); ok { + kh := &orgAPIKeyHandler{dash: dh, keys: km} + mux.Handle("POST /v1/orgs/{id}/api-keys", orgsAuthenticated(chain(http.HandlerFunc(kh.handleCreate), apiKeyMintLimitMiddleware(abuse)))) + mux.Handle("GET /v1/orgs/{id}/api-keys", orgsAuthenticated(http.HandlerFunc(kh.handleList))) + mux.Handle("POST /v1/orgs/{id}/api-keys/{keyId}/rotate", orgsAuthenticated(chain(http.HandlerFunc(kh.handleRotate), apiKeyMintLimitMiddleware(abuse)))) + mux.Handle("DELETE /v1/orgs/{id}/api-keys/{keyId}", orgsAuthenticated(http.HandlerFunc(kh.handleRevoke))) + } } if len(extras) > 0 && extras[0].billing != nil { lg := extras[0].log diff --git a/internal/auth/service.go b/internal/auth/service.go index 911eade..81e72fb 100644 --- a/internal/auth/service.go +++ b/internal/auth/service.go @@ -156,10 +156,14 @@ func (s *Service) Rotate(ctx context.Context, keyID string, grace time.Duration) // the grace window - exactly the opposite of what grace is for. // Create a fresh key instead. if old.RevokedAt != nil { - return Generated{}, database.APIKey{}, fmt.Errorf("auth: cannot rotate a revoked key; create a new one instead") + // Wrapped in ErrRevoked (CodeRabbit, PR #28) so a caller like the + // HTTP rotate handler can distinguish "key is dead" from a generic + // failure and answer 409, not 500, for the race where a key is + // revoked/expired between an unlocked pre-check and this call. + return Generated{}, database.APIKey{}, fmt.Errorf("auth: cannot rotate a revoked key; create a new one instead: %w", ErrRevoked) } if old.ExpiresAt != nil && !old.ExpiresAt.After(now) { - return Generated{}, database.APIKey{}, fmt.Errorf("auth: cannot rotate an already-expired key; create a new one instead") + return Generated{}, database.APIKey{}, fmt.Errorf("auth: cannot rotate an already-expired key; create a new one instead: %w", ErrExpired) } gen, err := Generate(s.pepper) if err != nil { diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index 4c70fcb..e5816e4 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -339,9 +339,16 @@ func TestServiceRotateRejectsExpiredKey(t *testing.T) { } svc.now = func() time.Time { return frozen.Add(2 * time.Hour) } // now past expiry - if _, _, err := svc.Rotate(ctx, record.KeyID, time.Hour); err == nil { + _, _, err = svc.Rotate(ctx, record.KeyID, time.Hour) + if err == nil { t.Fatal("expected rotating an already-expired key to be rejected, not revive it") } + // Wrapped in ErrExpired (CodeRabbit, PR #28) so a caller like the HTTP + // rotate handler can map this to 409 instead of a generic 500 - it + // used to be a bare fmt.Errorf with no matchable sentinel. + if !errors.Is(err, ErrExpired) { + t.Fatalf("expected err to wrap ErrExpired, got %v", err) + } } func TestServiceRotateRejectsRevokedKey(t *testing.T) { @@ -356,7 +363,16 @@ func TestServiceRotateRejectsRevokedKey(t *testing.T) { if err := svc.Revoke(ctx, record.KeyID); err != nil { t.Fatal(err) } - if _, _, err := svc.Rotate(ctx, record.KeyID, time.Hour); err == nil { + _, _, err = svc.Rotate(ctx, record.KeyID, time.Hour) + if err == nil { t.Fatal("expected rotating a revoked key to be rejected") } + // Wrapped in ErrRevoked (CodeRabbit, PR #28) for the same reason as + // ErrExpired above: this is exactly the race where a key is revoked + // concurrently between the HTTP handler's own unlocked pre-check and + // this call - the handler must be able to tell this apart from a real + // internal error. + if !errors.Is(err, ErrRevoked) { + t.Fatalf("expected err to wrap ErrRevoked, got %v", err) + } } diff --git a/internal/database/email_verification.go b/internal/database/email_verification.go new file mode 100644 index 0000000..a94ba74 --- /dev/null +++ b/internal/database/email_verification.go @@ -0,0 +1,151 @@ +package database + +import ( + "context" + "errors" + "fmt" + "time" +) + +// EmailVerificationToken is one issued email-verification token row +// (migration 000034). Same shape as PasswordResetToken. +type EmailVerificationToken struct { + ID string + HumanID string + TokenHash string + ExpiresAt time.Time + UsedAt *time.Time + CreatedAt time.Time +} + +// ErrEmailAlreadyVerified is returned by IssueEmailVerificationToken when +// the human is already verified (checked under the row lock). +var ErrEmailAlreadyVerified = errors.New("database: email already verified") + +// ErrEmailVerificationTokenConsumed means this exact token row was already +// used or superseded by a concurrent call - see VerifyEmail's doc. +var ErrEmailVerificationTokenConsumed = errors.New("database: email verification token already used") + +// CreateEmailVerificationToken locks the humans row (serializing concurrent +// issues for the same account and providing a consistent point to check +// "not already verified"), then inserts a fresh token. It deliberately does +// NOT invalidate any other pending token here - see +// SupersedeOtherEmailVerificationTokens, called only after this token's +// email has actually been sent. An earlier version invalidated the old +// token before attempting delivery, so a failed send left the account with +// NEITHER a working old link nor a delivered new one (the exact bug class +// DEC-218 fixed for org invitations - caught here before merge, not by a +// live incident). +// +// created_at is set via clock_timestamp(), not the column's now() default: +// now()/transaction_timestamp() is fixed at this transaction's BEGIN, which +// happens BEFORE the FOR UPDATE lock wait above - a transaction that +// waited on the lock would otherwise still capture an earlier timestamp +// than one that acquired the lock and committed first, which would break +// SupersedeOtherEmailVerificationTokens' created_at-ordering guarantee +// exactly the way DEC-226 found for org invitations. clock_timestamp() +// reflects the actual moment this INSERT runs, i.e. after the lock is held. +func (db *DB) CreateEmailVerificationToken(ctx context.Context, humanID, tokenHash string, expiresAt time.Time) (EmailVerificationToken, error) { + id, err := newID() + if err != nil { + return EmailVerificationToken{}, err + } + tx, err := db.pool.Begin(ctx) + if err != nil { + return EmailVerificationToken{}, fmt.Errorf("database: begin issue email verification: %w", normalizeErr(err)) + } + defer func() { _ = tx.Rollback(ctx) }() + + var verifiedAt *time.Time + if err := tx.QueryRow(ctx, `SELECT email_verified_at FROM humans WHERE id = $1 FOR UPDATE`, humanID).Scan(&verifiedAt); err != nil { + return EmailVerificationToken{}, normalizeErr(err) + } + if verifiedAt != nil { + return EmailVerificationToken{}, ErrEmailAlreadyVerified + } + var t EmailVerificationToken + err = tx.QueryRow(ctx, ` + INSERT INTO email_verification_tokens (id, human_id, token_hash, expires_at, created_at) + VALUES ($1, $2, $3, $4, clock_timestamp()) + RETURNING id, human_id, token_hash, expires_at, used_at, created_at`, + id, humanID, tokenHash, expiresAt, + ).Scan(&t.ID, &t.HumanID, &t.TokenHash, &t.ExpiresAt, &t.UsedAt, &t.CreatedAt) + if err != nil { + return EmailVerificationToken{}, normalizeErr(err) + } + if err := tx.Commit(ctx); err != nil { + return EmailVerificationToken{}, fmt.Errorf("database: commit issue email verification: %w", normalizeErr(err)) + } + return t, nil +} + +// SupersedeOtherEmailVerificationTokens invalidates every OTHER still-valid +// verification token for humanID that ORDERS STRICTLY BEFORE keepTokenID - +// called only once the keeper token's email has been confirmed sent (see +// CreateEmailVerificationToken's doc). Ordering by (created_at, id), not +// simply "id != keep", matters under concurrency for the exact reason +// DEC-219 found for org invitations: two resends completing their sends at +// nearly the same instant would otherwise each try to supersede the OTHER +// after both had already been sent, and whichever UPDATE ran last would +// win - invalidating the link that had just been delivered, so BOTH +// emailed links could end up dead even though both requests reported +// success. Superseding only strictly-older rows makes this commutative: +// the newest token always survives no matter which request's UPDATE runs +// last. created_at alone is not a total order (two rows can share a +// timestamp), so id breaks the tie, matching DEC-226's fix. +func (db *DB) SupersedeOtherEmailVerificationTokens(ctx context.Context, humanID, keepTokenID string, keepCreatedAt, now time.Time) error { + _, err := db.pool.Exec(ctx, + `UPDATE email_verification_tokens SET used_at = $4 WHERE human_id = $1 AND (created_at, id) < ($3, $2) AND used_at IS NULL`, + humanID, keepTokenID, keepCreatedAt, now, + ) + if err != nil { + return fmt.Errorf("database: supersede email verification tokens: %w", normalizeErr(err)) + } + return nil +} + +// GetEmailVerificationTokenByHash loads a token row by hash. Returns +// ErrNotFound if absent (callers check used/expired themselves). +func (db *DB) GetEmailVerificationTokenByHash(ctx context.Context, tokenHash string) (EmailVerificationToken, error) { + var t EmailVerificationToken + err := db.pool.QueryRow(ctx, ` + SELECT id, human_id, token_hash, expires_at, used_at, created_at + FROM email_verification_tokens WHERE token_hash = $1`, tokenHash, + ).Scan(&t.ID, &t.HumanID, &t.TokenHash, &t.ExpiresAt, &t.UsedAt, &t.CreatedAt) + if err != nil { + return EmailVerificationToken{}, normalizeErr(err) + } + return t, nil +} + +// VerifyEmail atomically marks tokenID used (only if not already used and +// not expired at now - RowsAffected-checked, same pattern as +// ResetPassword) and sets humans.email_verified_at. Both or neither. +// Keeps the first verification time if the human was somehow already +// verified. Locks the humans row first, same order as +// IssueEmailVerificationToken, so the two never deadlock or interleave. +func (db *DB) VerifyEmail(ctx context.Context, tokenID, humanID string, now time.Time) error { + tx, err := db.pool.Begin(ctx) + if err != nil { + return fmt.Errorf("database: begin verify email: %w", normalizeErr(err)) + } + defer func() { _ = tx.Rollback(ctx) }() + + if _, err := tx.Exec(ctx, `SELECT 1 FROM humans WHERE id = $1 FOR UPDATE`, humanID); err != nil { + return fmt.Errorf("database: lock human: %w", normalizeErr(err)) + } + tag, err := tx.Exec(ctx, `UPDATE email_verification_tokens SET used_at = $3 WHERE id = $1 AND human_id = $2 AND used_at IS NULL AND expires_at > $3`, tokenID, humanID, now) + if err != nil { + return fmt.Errorf("database: consume email verification token: %w", normalizeErr(err)) + } + if tag.RowsAffected() == 0 { + return ErrEmailVerificationTokenConsumed + } + if _, err := tx.Exec(ctx, `UPDATE humans SET email_verified_at = COALESCE(email_verified_at, $2), updated_at = $2 WHERE id = $1`, humanID, now); err != nil { + return fmt.Errorf("database: mark email verified: %w", normalizeErr(err)) + } + if err := tx.Commit(ctx); err != nil { + return fmt.Errorf("database: commit verify email: %w", normalizeErr(err)) + } + return nil +} diff --git a/internal/database/humans.go b/internal/database/humans.go index 77874c0..cd7ca53 100644 --- a/internal/database/humans.go +++ b/internal/database/humans.go @@ -24,6 +24,9 @@ type Human struct { AvatarURL *string // MFAEnabled is true once TOTP MFA is confirmed (migration 000032). MFAEnabled bool + // EmailVerifiedAt is nil until the human consumes an email + // verification token (migration 000034). Gates nothing (DEC-243). + EmailVerifiedAt *time.Time } // RefreshToken is one issued refresh token row (see migration 000025). @@ -73,10 +76,10 @@ func (db *DB) CreateHuman(ctx context.Context, name, email, passwordHash string) func (db *DB) GetHumanByEmail(ctx context.Context, email string) (Human, error) { var h Human err := db.pool.QueryRow(ctx, ` - SELECT id, name, email, password_hash, role, created_at, updated_at, last_login_at, avatar_url, mfa_enabled + SELECT id, name, email, password_hash, role, created_at, updated_at, last_login_at, avatar_url, mfa_enabled, email_verified_at FROM humans WHERE normalized_email = $1`, normalizeEmail(email), - ).Scan(&h.ID, &h.Name, &h.Email, &h.PasswordHash, &h.Role, &h.CreatedAt, &h.UpdatedAt, &h.LastLoginAt, &h.AvatarURL, &h.MFAEnabled) + ).Scan(&h.ID, &h.Name, &h.Email, &h.PasswordHash, &h.Role, &h.CreatedAt, &h.UpdatedAt, &h.LastLoginAt, &h.AvatarURL, &h.MFAEnabled, &h.EmailVerifiedAt) if err != nil { return Human{}, normalizeErr(err) } @@ -87,9 +90,9 @@ func (db *DB) GetHumanByEmail(ctx context.Context, email string) (Human, error) func (db *DB) GetHuman(ctx context.Context, id string) (Human, error) { var h Human err := db.pool.QueryRow(ctx, ` - SELECT id, name, email, password_hash, role, created_at, updated_at, last_login_at, avatar_url, mfa_enabled + SELECT id, name, email, password_hash, role, created_at, updated_at, last_login_at, avatar_url, mfa_enabled, email_verified_at FROM humans WHERE id = $1`, id, - ).Scan(&h.ID, &h.Name, &h.Email, &h.PasswordHash, &h.Role, &h.CreatedAt, &h.UpdatedAt, &h.LastLoginAt, &h.AvatarURL, &h.MFAEnabled) + ).Scan(&h.ID, &h.Name, &h.Email, &h.PasswordHash, &h.Role, &h.CreatedAt, &h.UpdatedAt, &h.LastLoginAt, &h.AvatarURL, &h.MFAEnabled, &h.EmailVerifiedAt) if err != nil { return Human{}, normalizeErr(err) } diff --git a/internal/database/migrations/000034_email_verification.down.sql b/internal/database/migrations/000034_email_verification.down.sql new file mode 100644 index 0000000..14fec09 --- /dev/null +++ b/internal/database/migrations/000034_email_verification.down.sql @@ -0,0 +1,2 @@ +DROP TABLE email_verification_tokens; +ALTER TABLE humans DROP COLUMN email_verified_at; diff --git a/internal/database/migrations/000034_email_verification.up.sql b/internal/database/migrations/000034_email_verification.up.sql new file mode 100644 index 0000000..1dd768c --- /dev/null +++ b/internal/database/migrations/000034_email_verification.up.sql @@ -0,0 +1,21 @@ +-- Email verification for human accounts (DEC-241..243). +-- NULL = unverified (same nullable-timestamp convention as last_login_at). +-- Existing accounts stay NULL: verification gates nothing in this pass +-- (DEC-243), so no backfill is needed. +ALTER TABLE humans ADD COLUMN email_verified_at TIMESTAMPTZ; + +-- Same shape as password_reset_tokens (migration 000029): only a hash is +-- stored, single use, short-lived (15-minute TTL enforced by +-- humanauth.EmailVerificationTokenTTL, not a CHECK). +CREATE TABLE email_verification_tokens ( + id TEXT PRIMARY KEY, + human_id TEXT NOT NULL REFERENCES humans(id) ON DELETE CASCADE, + token_hash TEXT NOT NULL, + expires_at TIMESTAMPTZ NOT NULL, + used_at TIMESTAMPTZ, + created_at TIMESTAMPTZ NOT NULL DEFAULT now() +); + +CREATE UNIQUE INDEX idx_email_verification_tokens_hash ON email_verification_tokens (token_hash); +-- Used by the issue path's "supersede my other pending tokens" update. +CREATE INDEX idx_email_verification_tokens_human ON email_verification_tokens (human_id) WHERE used_at IS NULL; diff --git a/internal/humanauth/email_verification.go b/internal/humanauth/email_verification.go new file mode 100644 index 0000000..0f580c9 --- /dev/null +++ b/internal/humanauth/email_verification.go @@ -0,0 +1,108 @@ +package humanauth + +import ( + "context" + "errors" + "fmt" + "html" + + "github.com/Ferousco-dev/mailx/internal/database" +) + +// ErrEmailVerificationTokenInvalid covers every way an email verification +// token can fail - unknown, expired, already used, superseded by a resend, +// or lost a concurrent consume-race - never distinguished to the caller, +// same posture as ErrPasswordResetTokenInvalid. +var ErrEmailVerificationTokenInvalid = errors.New("humanauth: email verification token invalid or expired") + +// sendVerificationEmail issues a fresh 15-minute verification token for h +// (superseding any pending one, DEC-242) and emails it. Returns nil and +// does nothing if h is already verified. Errors are infrastructure +// failures the caller logs; they must never fail SignUp or be surfaced +// by ResendVerification. +func (s *Service) sendVerificationEmail(ctx context.Context, h database.Human) error { + if s.mailer == nil { + return fmt.Errorf("humanauth: no mailer is configured; cannot send a verification email") + } + if s.dashboardBaseURL == "" { + // Same guard as ForgotPassword (DEC-215): never send a relative, + // unusable link. + return fmt.Errorf("humanauth: mailer is configured but dashboard base URL is not; refusing to send an unusable verification link") + } + raw, err := generateRawToken() + if err != nil { + return err + } + now := s.now() + t, err := s.db.CreateEmailVerificationToken(ctx, h.ID, hashRawToken(raw), now.Add(EmailVerificationTokenTTL)) + if err != nil { + if errors.Is(err, database.ErrEmailAlreadyVerified) { + return nil + } + return fmt.Errorf("humanauth: issue email verification token: %w", err) + } + link := s.dashboardBaseURL + "/verify-email?token=" + raw + text := "Hello " + h.Name + ", welcome to MailX!\n\n" + + "To finish setting up your account, verify your email within 15 minutes:\n" + link + "\n\n" + + "If you didn't create a MailX account, you can safely ignore this email." + body := `
Hello ` + html.EscapeString(h.Name) + `, welcome to MailX!
` + + `To finish setting up your account, click the button below within 15 minutes to verify your email.
` + + `` + + `If you didn't create a MailX account, you can safely ignore this email.
` + if err := s.mailer.SendSystemEmail(ctx, h.Email, "Verify your MailX account", text, body); err != nil { + // Deliberately do NOT supersede prior tokens here: this new one was + // never delivered, so invalidating an older, still-working link + // would leave the account holder with nothing usable at all + // (same fix shape as DEC-218, PR #23). + return fmt.Errorf("humanauth: send verification email: %w", err) + } + // Only now that the new link is confirmed delivered is it safe to + // invalidate any other pending token - see + // SupersedeOtherEmailVerificationTokens's doc. + if err := s.db.SupersedeOtherEmailVerificationTokens(ctx, h.ID, t.ID, t.CreatedAt, s.now()); err != nil { + return fmt.Errorf("humanauth: supersede prior verification tokens: %w", err) + } + return nil +} + +// ResendVerification issues a new verification email if an UNVERIFIED +// account exists for email. The caller must ALWAYS present the same +// generic response (anti-enumeration, same as ForgotPassword): unknown +// and already-verified emails return nil; a non-nil error is an +// infrastructure failure to log, never to surface. +func (s *Service) ResendVerification(ctx context.Context, email string) error { + h, err := s.db.GetHumanByEmail(ctx, email) + if err != nil { + if errors.Is(err, database.ErrNotFound) { + return nil + } + return fmt.Errorf("humanauth: get human: %w", err) + } + if h.EmailVerifiedAt != nil { + return nil + } + return s.sendVerificationEmail(ctx, h) +} + +// VerifyEmail consumes a verification token and marks the account +// verified (database.DB.VerifyEmail - atomic, RowsAffected-checked). +func (s *Service) VerifyEmail(ctx context.Context, rawToken string) error { + rec, err := s.db.GetEmailVerificationTokenByHash(ctx, hashRawToken(rawToken)) + if err != nil { + if errors.Is(err, database.ErrNotFound) { + return ErrEmailVerificationTokenInvalid + } + return fmt.Errorf("humanauth: get email verification token: %w", err) + } + now := s.now() + if rec.UsedAt != nil || !rec.ExpiresAt.After(now) { + return ErrEmailVerificationTokenInvalid + } + if err := s.db.VerifyEmail(ctx, rec.ID, rec.HumanID, now); err != nil { + if errors.Is(err, database.ErrEmailVerificationTokenConsumed) { + return ErrEmailVerificationTokenInvalid + } + return fmt.Errorf("humanauth: verify email: %w", err) + } + return nil +} diff --git a/internal/humanauth/email_verification_test.go b/internal/humanauth/email_verification_test.go new file mode 100644 index 0000000..40ba8c2 --- /dev/null +++ b/internal/humanauth/email_verification_test.go @@ -0,0 +1,230 @@ +package humanauth + +import ( + "context" + "errors" + "strings" + "sync" + "testing" + "time" +) + +func tokenFromText(t *testing.T, text string) string { + t.Helper() + idx := strings.Index(text, "token=") + if idx == -1 { + t.Fatalf("no token in email body: %q", text) + } + raw := text[idx+len("token="):] + if end := strings.IndexAny(raw, "\n "); end != -1 { + raw = raw[:end] + } + return raw +} + +func (m *fakeMailer) verifySnapshot() []struct{ to, subject, text, html string } { + m.mu.Lock() + defer m.mu.Unlock() + return append([]struct{ to, subject, text, html string }(nil), m.verifyCalls...) +} + +func TestSignUpSendsVerificationAndVerifyEmail(t *testing.T) { + db := newTestDB(t) + mailer := &fakeMailer{} + svc, err := NewService(db, testSecret(), WithMailer(mailer), WithDashboardBaseURL("https://app.mailx.dev")) + if err != nil { + t.Fatal(err) + } + ctx := context.Background() + sess, err := svc.SignUp(ctx, "Ada", "ada@example.com", "hunter22hunter") + if err != nil { + t.Fatal(err) + } + if sess.Human.EmailVerifiedAt != nil { + t.Fatal("new account must start unverified") + } + calls := mailer.verifySnapshot() + if len(calls) != 1 || calls[0].to != "ada@example.com" { + t.Fatalf("expected one verification email to ada, got %+v", calls) + } + if !strings.Contains(calls[0].text, "Hello Ada, welcome to MailX") || + !strings.Contains(calls[0].text, "https://app.mailx.dev/verify-email?token=") { + t.Fatalf("unexpected verification body: %q", calls[0].text) + } + raw := tokenFromText(t, calls[0].text) + + if err := svc.VerifyEmail(ctx, raw); err != nil { + t.Fatal(err) + } + h, err := db.GetHuman(ctx, sess.Human.ID) + if err != nil { + t.Fatal(err) + } + if h.EmailVerifiedAt == nil { + t.Fatal("expected email_verified_at to be set") + } + if err := svc.VerifyEmail(ctx, raw); !errors.Is(err, ErrEmailVerificationTokenInvalid) { + t.Fatalf("reuse: expected ErrEmailVerificationTokenInvalid, got %v", err) + } + if err := svc.VerifyEmail(ctx, "not-a-real-token"); !errors.Is(err, ErrEmailVerificationTokenInvalid) { + t.Fatalf("unknown: expected ErrEmailVerificationTokenInvalid, got %v", err) + } + // Resend to an already-verified account is a silent no-op. + if err := svc.ResendVerification(ctx, "ada@example.com"); err != nil { + t.Fatal(err) + } + if n := len(mailer.verifySnapshot()); n != 1 { + t.Fatalf("resend to verified account sent mail (%d total)", n) + } +} + +func TestSignUpSucceedsWhenVerificationEmailFails(t *testing.T) { + db := newTestDB(t) + mailer := &fakeMailer{failNext: true} + svc, err := NewService(db, testSecret(), WithMailer(mailer), WithDashboardBaseURL("https://app.mailx.dev")) + if err != nil { + t.Fatal(err) + } + sess, err := svc.SignUp(context.Background(), "Ada", "ada@example.com", "hunter22hunter") + if err != nil || sess.AccessToken == "" { + t.Fatalf("signup must succeed despite mail failure: %v", err) + } + // No mailer at all: same. + svc2, _ := NewService(db, testSecret()) + if _, err := svc2.SignUp(context.Background(), "Bo", "bo@example.com", "hunter22hunter"); err != nil { + t.Fatalf("signup without mailer: %v", err) + } +} + +func TestVerifyEmailExpiredToken(t *testing.T) { + db := newTestDB(t) + mailer := &fakeMailer{} + start := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + current := start + svc, err := NewService(db, testSecret(), WithMailer(mailer), WithDashboardBaseURL("https://app.mailx.dev"), + WithNow(func() time.Time { return current })) + if err != nil { + t.Fatal(err) + } + ctx := context.Background() + if _, err := svc.SignUp(ctx, "Ada", "ada@example.com", "hunter22hunter"); err != nil { + t.Fatal(err) + } + raw := tokenFromText(t, mailer.verifySnapshot()[0].text) + current = start.Add(EmailVerificationTokenTTL + time.Second) + if err := svc.VerifyEmail(ctx, raw); !errors.Is(err, ErrEmailVerificationTokenInvalid) { + t.Fatalf("expected ErrEmailVerificationTokenInvalid for expired token, got %v", err) + } +} + +func TestResendVerificationSupersedesOldToken(t *testing.T) { + db := newTestDB(t) + mailer := &fakeMailer{} + svc, err := NewService(db, testSecret(), WithMailer(mailer), WithDashboardBaseURL("https://app.mailx.dev")) + if err != nil { + t.Fatal(err) + } + ctx := context.Background() + if _, err := svc.SignUp(ctx, "Ada", "ada@example.com", "hunter22hunter"); err != nil { + t.Fatal(err) + } + if err := svc.ResendVerification(ctx, "unknown@example.com"); err != nil { + t.Fatal(err) + } + if n := len(mailer.verifySnapshot()); n != 1 { + t.Fatalf("unknown email must not send, got %d emails", n) + } + if err := svc.ResendVerification(ctx, "ADA@example.com"); err != nil { + t.Fatal(err) + } + calls := mailer.verifySnapshot() + if len(calls) != 2 { + t.Fatalf("expected a second verification email, got %d", len(calls)) + } + oldTok, newTok := tokenFromText(t, calls[0].text), tokenFromText(t, calls[1].text) + if err := svc.VerifyEmail(ctx, oldTok); !errors.Is(err, ErrEmailVerificationTokenInvalid) { + t.Fatalf("old token must be superseded, got %v", err) + } + if err := svc.VerifyEmail(ctx, newTok); err != nil { + t.Fatalf("new token must work: %v", err) + } +} + +// TestConcurrentResendsLeaveExactlyOneValidToken is the DEC-226 lesson +// applied here: concurrent invalidate-then-insert must never leave zero +// (both superseded) or two valid tokens. IssueEmailVerificationToken +// serializes on the humans row (FOR UPDATE), so exactly one - the last +// committed - survives. +func TestConcurrentResendsLeaveExactlyOneValidToken(t *testing.T) { + db := newTestDB(t) + mailer := &fakeMailer{} + svc, err := NewService(db, testSecret(), WithMailer(mailer), WithDashboardBaseURL("https://app.mailx.dev")) + if err != nil { + t.Fatal(err) + } + ctx := context.Background() + if _, err := svc.SignUp(ctx, "Ada", "ada@example.com", "hunter22hunter"); err != nil { + t.Fatal(err) + } + for round := 0; round < 10; round++ { + const n = 4 + start := make(chan struct{}) + var wg sync.WaitGroup + for i := 0; i < n; i++ { + wg.Add(1) + go func() { + defer wg.Done() + <-start + if err := svc.ResendVerification(ctx, "ada@example.com"); err != nil { + t.Errorf("resend: %v", err) + } + }() + } + close(start) + wg.Wait() + var valid int + for _, c := range mailer.verifySnapshot() { + rec, err := db.GetEmailVerificationTokenByHash(ctx, hashRawToken(tokenFromText(t, c.text))) + if err != nil { + t.Fatal(err) + } + if rec.UsedAt == nil { + valid++ + } + } + if valid != 1 { + t.Fatalf("round %d: expected exactly one valid token, got %d", round, valid) + } + } +} + +// TestFailedResendDoesNotInvalidateTheWorkingVerificationLink is a +// regression test for a bug caught before merge (same class as DEC-218): a +// resend whose email delivery fails must NOT invalidate the previous, +// still-working verification link - otherwise a transient mailer failure +// leaves the account with neither a delivered new link nor a working old +// one. +func TestFailedResendDoesNotInvalidateTheWorkingVerificationLink(t *testing.T) { + db := newTestDB(t) + mailer := &fakeMailer{} + svc, err := NewService(db, testSecret(), WithMailer(mailer), WithDashboardBaseURL("https://app.mailx.dev")) + if err != nil { + t.Fatal(err) + } + ctx := context.Background() + if _, err := svc.SignUp(ctx, "Ada", "ada@example.com", "hunter22hunter"); err != nil { + t.Fatal(err) + } + firstToken := tokenFromText(t, mailer.verifySnapshot()[0].text) + + mailer.failNext = true + if err := svc.ResendVerification(ctx, "ada@example.com"); err == nil { + t.Fatal("expected the simulated send failure to surface as an error") + } + + // The original verification link must still work: the failed resend's + // delivery failure must not have invalidated it. + if err := svc.VerifyEmail(ctx, firstToken); err != nil { + t.Fatalf("expected the original verification token to still be valid after a failed resend: %v", err) + } +} diff --git a/internal/humanauth/service.go b/internal/humanauth/service.go index d80f01f..c288cd9 100644 --- a/internal/humanauth/service.go +++ b/internal/humanauth/service.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "html" + "log/slog" "strings" "time" @@ -63,6 +64,13 @@ const ( // whatever channel the inviter chooses, not a same-session self-serve // flow). OrgInvitationTTL = 5 * time.Hour + // EmailVerificationTokenTTL is the operator's own stated number: + // a signup verification link works for 15 minutes (DEC-241). + EmailVerificationTokenTTL = 15 * time.Minute + // verificationEmailSendTimeout bounds SignUp's best-effort verification + // email so a slow mail-accept path can't stall account creation + // indefinitely (CodeRabbit, PR #28). + verificationEmailSendTimeout = 10 * time.Second ) // Mailer sends a system-originated email to a human account holder @@ -205,6 +213,24 @@ func (s *Service) SignUp(ctx context.Context, name, email, password string) (Ses } return Session{}, fmt.Errorf("humanauth: create human: %w", err) } + // Verification email is best-effort: signup still succeeds and mints a + // session if it fails (the user can resend). Logged, never surfaced. + // Bounded with its own timeout (CodeRabbit, PR #28) rather than left to + // signup's own request context indefinitely: SendSystemEmail's + // SubmissionAcceptor.Accept does synchronous file+DB work before + // returning, so an unbounded call here would let a slow acceptance + // path stall the signup response - and simply detaching it into a + // goroutine tied to a context that gets canceled once the response is + // written could lose the durable outbound record mid-write. A bounded + // timeout keeps this synchronous (so success/failure here still means + // the record was actually durably written or cleanly gave up) while + // capping how long a slow mailer can hold up account creation. + verifyCtx, cancel := context.WithTimeout(ctx, verificationEmailSendTimeout) + err = s.sendVerificationEmail(verifyCtx, h) + cancel() + if err != nil { + slog.Default().Error("signup_verification_email_failed", "human_id", h.ID, "error", err.Error()) + } return s.mintSession(ctx, h) } diff --git a/internal/humanauth/service_test.go b/internal/humanauth/service_test.go index 73b4e7b..a95e87b 100644 --- a/internal/humanauth/service_test.go +++ b/internal/humanauth/service_test.go @@ -328,9 +328,10 @@ func TestAccessTokenRoundTripsAndRejectsTamperedOrExpired(t *testing.T) { // (Greptile P1, PR #23: an unsynchronized slice append here raced under // -race and could silently drop a recorded call). type fakeMailer struct { - mu sync.Mutex - calls []struct{ to, subject, text, html string } - failNext bool // when true, the NEXT call fails (and is not recorded) then resets + mu sync.Mutex + calls []struct{ to, subject, text, html string } + verifyCalls []struct{ to, subject, text, html string } + failNext bool // when true, the NEXT call fails (and is not recorded) then resets } func (m *fakeMailer) SendSystemEmail(_ context.Context, to, subject, text, html string) error { @@ -340,7 +341,14 @@ func (m *fakeMailer) SendSystemEmail(_ context.Context, to, subject, text, html m.failNext = false return fmt.Errorf("fakeMailer: simulated send failure") } - m.calls = append(m.calls, struct{ to, subject, text, html string }{to, subject, text, html}) + c := struct{ to, subject, text, html string }{to, subject, text, html} + // SignUp now also sends a verification email; record those separately + // so tests about other mail (reset, invites) keep exact call counts. + if subject == "Verify your MailX account" { + m.verifyCalls = append(m.verifyCalls, c) + return nil + } + m.calls = append(m.calls, c) return nil } diff --git a/internal/ratelimit/policy.go b/internal/ratelimit/policy.go index 26d4bfc..ffc0757 100644 --- a/internal/ratelimit/policy.go +++ b/internal/ratelimit/policy.go @@ -66,6 +66,12 @@ type Policy struct { // dangerous credential-change action. PasswordResetIPRate float64 PasswordResetIPBurst int + // EmailVerificationResendIPRate/Burst bound POST + // /v1/auth/resend-verification per client IP. Same tightness as + // PasswordResetIPRate for the same reason: each call can send a real + // email (email-bombing surface). + EmailVerificationResendIPRate float64 + EmailVerificationResendIPBurst int // OrgInviteRate/OrgInviteBurst bound POST /v1/orgs/{id}/invites per // inviting human (not IP — unlike the auth-surface buckets above, this // caller is already authenticated, so their human ID is a stronger, @@ -75,6 +81,11 @@ type Policy struct { // email-bombing concern PasswordResetIPRate exists for. OrgInviteRate float64 OrgInviteBurst int + // APIKeyMintRate/APIKeyMintBurst bound POST /v1/orgs/{id}/api-keys and + // .../rotate per human (DEC-247): same shape as OrgInviteRate, an + // occasional owner-gated mutation keyed on the verified human ID. + APIKeyMintRate float64 + APIKeyMintBurst int // MFAVerifyIPRate/MFAVerifyIPBurst bound POST /v1/auth/mfa/verify and // /v1/auth/mfa/confirm per client IP: their own, tight bucket because a // 6-digit TOTP code is brute-forceable. A second, per-challenge cap @@ -104,7 +115,9 @@ func DefaultPolicy() Policy { MaxRecipientsPerMessage: 50, AuthIPRate: 1, AuthIPBurst: 10, PasswordResetIPRate: 1.0 / 60, PasswordResetIPBurst: 3, + EmailVerificationResendIPRate: 1.0 / 60, EmailVerificationResendIPBurst: 3, OrgInviteRate: 1.0 / 30, OrgInviteBurst: 10, + APIKeyMintRate: 1.0 / 30, APIKeyMintBurst: 10, MFAVerifyIPRate: 1.0 / 12, MFAVerifyIPBurst: 5, } } @@ -134,8 +147,10 @@ func (p Policy) Validate() error { count("max recipients per message", p.MaxRecipientsPerMessage, 1000), rate("auth IP rate", p.AuthIPRate), count("auth IP burst", p.AuthIPBurst, maxBurst), rate("password reset IP rate", p.PasswordResetIPRate), count("password reset IP burst", p.PasswordResetIPBurst, maxBurst), + rate("email verification resend IP rate", p.EmailVerificationResendIPRate), count("email verification resend IP burst", p.EmailVerificationResendIPBurst, maxBurst), rate("mfa verify IP rate", p.MFAVerifyIPRate), count("mfa verify IP burst", p.MFAVerifyIPBurst, maxBurst), rate("org invite rate", p.OrgInviteRate), count("org invite burst", p.OrgInviteBurst, maxBurst), + rate("api key mint rate", p.APIKeyMintRate), count("api key mint burst", p.APIKeyMintBurst, maxBurst), } { if e != nil { return e