Fix the first security audit's findings - #14
Merged
Merged
Conversation
… (audit Q1) The audit's Email codes FAIL: Better Auth's database limiter stored "<ip>|<path>" in plaintext, and on Node the IP came from x-pgstencil-client-ip, a header the client supplies, so rotating it escaped the limit and pinning a victim's address exhausted theirs. - rateLimit.customStorage keeps Better Auth's limits and windows but stores keyed(secret, 'rate-limit', key) through one atomic upsert; migration 005 deletes the plaintext rows. - protectAuth overwrites x-pgstencil-client-ip on every request from the @hono/node-server socket (env.incoming), or from a header only when the application opts in with ipAddressHeaders. Better Auth and the per-IP email budget both read that one trusted value. - SECURITY.md states the hashed-key and trusted-IP rules and pins them to five new integration tests; PACKAGES.md documents the opt-in. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… Q2) The audit's OAuth FAIL: SECURITY.md said no provider error code reaches a diagnostic record, but diagnosticError deliberately records allowlisted OAuth codes and Microsoft's bounded numeric code, and a unit test pins that. The rule now says exactly that: a provider's error description, or a code outside the errorCodes allowlist, never reaches a redirect, a page or a diagnostic record; an allowlisted code may reach only a diagnostic record. The Diagnostics rule names the allowlist and provider descriptions too. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The audit found controls that exist in code but that no Pinned-by test drives, so a regression would pass CI. New integration tests: - RS256 pinning: ID tokens forged under HS256 (keyed with the public key), none, PS256 and RS512 are rejected for Google, Apple and Microsoft. With the pin removed, Microsoft's PS256 token signs in, so this test fails. - 415 for a non-JSON body on an allowlisted write. - 405 for a non-GET on a non-Apple callback and for Apple POST without form encoding. - authOptions rejects a non-canonical origin, a secret under 32 characters and off-origin success/error paths (//, /\, tab variants). - The fifteen-verifications-per-fifteen-minutes budget, across IPs and against the right code. - X-Content-Type-Options and X-Frame-Options on a page and an API response. SECURITY.md names each test in its Pinned-by line. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…, Q4, Q5, Q8) - Q5: paths.ts refuses a pgstencil.json migrations path that resolves outside the project root; a unit test pins it and PACKAGES.md says so. - Q3: the Build and test isolation rule now says better-auth-testing is a published export, imported by packages:verify, and inert unless esbuild injects it; the rule fails on an injection in a normal build or a global changed by import, pinned by the production-bundle and unchanged-host-clock tests. - Q4, Q8: "What is not defended" names Facebook's unverified email and two applications sharing both a database and AUTH_SECRET. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The per-IP email budget hashed the full address, so a client rotating
within its IPv6 /64 got fresh budgets while Better Auth's limiter held.
It now keys on ipBucket, which reproduces Better Auth's
normalizeIP(ip, { ipv6Subnet: 64 }). Better Auth does not re-export that
helper, and depending on @better-auth/core directly drags in its exactly
pinned peers, which failed packages:verify's fresh install; a unit test
instead compares ipBucket with the normalizeIP Better Auth's limiter runs.
An integration test shows two addresses in one /64 share one budget, and
the trusted-IP rule names the /64 grouping.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes every finding in #13, the first run of the nightly security audit.
<ip>|<path>, written by one atomic upsert throughrateLimit.customStorage. Migration005deletes the old plaintext rows.ipAddressHeaders. The Workers adapter keepscf-connecting-ip.x-pgstencil-client-ipis always overwritten. Both limiters read the same trusted IP and group IPv6 by /64.SECURITY.mdnow says what the code does. A provider error description never reaches a redirect, a page or a diagnostic record. An allowlisted error code, and Microsoft's bounded numeric code, may reach a diagnostic record only.none, PS256, RS512)authOptionsvalidation, including off-origin redirect pathsX-Content-Type-OptionsandX-Frame-Optionspgstencil.jsonmigrations path can no longer leave the project root. The testing module is documented as inert unless injected. The Facebook-email and shared-secret limits are stated under "What is not defended".Residuals, not rules today:
session."ipAddress".betterAuth(authOptions(...))itself, bypassingcreateAuthApp, would trust the header. The JSDoc and PACKAGES.md say to usecreateAuthApp.Verified: format, typecheck,
db:verify,pnpm test(175),test:scripts,packages:verify.🤖 Generated with Claude Code