security: close the development authentication bypass on a deployment - #65
Merged
Conversation
yufoxda
force-pushed
the
security/close-dev-auth-bypass
branch
2 times, most recently
from
July 27, 2026 07:41
357fc37 to
0e479e8
Compare
The port was generic in name only. Its methods returned DiscordGuildMembership, DiscordMessage and DiscordReactionUser, and every identifier was validated against the Discord snowflake format, so the identifier regex reached callers that have no reason to know what a snowflake is. Replacing the provider would have meant editing the interface and both api_v0 services rather than swapping an adapter. The port now speaks CommunityRole, CommunityMembership, CommunityMessage, CommunityReactionUser and CommunityAccountProfile, treats identifiers as opaque strings, and drops the Discord message length limit. Discord's snowflake format, its 2000-character limit, and the global_name field it returns are refinements applied in discord/schema.ts, which is the only layer that issues those values. The provider-specific field name is mapped to displayName at the adapter boundary, matching the provider_display_name column it is stored in. Behaviour is unchanged: the adapter still rejects a malformed provider response and a guild member response for another user, both of which depend on the snowflake assertion that moved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The reaction summary returned the complete member record — student ID, student email, emergency contact, insurance and allergy details — inside every reaction's user list as well as in `members`. A member who reacted with three emoji had those fields serialised four times, so the private data on the wire grew with the number of reactions rather than the number of members. The badges only ever rendered names: the client maps that list through getDisplayName and reads nothing else from it. They now carry a ReactionParticipant with the identity and name fields, while `members` keeps the full record the admin table and its CSV export need, including the emergency contact and allergy details an organiser relies on. Adds a regression test asserting no private field appears in the badge payload. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Better Auth's tables move to app_auth, and every reference that crossed between them and the domain is removed, so the authentication store can be lifted into its own database without touching a domain table. Moving the tables alone would not have achieved that: a foreign key cannot span databases, and three of them crossed this boundary, one of which only surfaced when the new assertion ran. The domain now keeps its own account record in public.app_accounts, holding the membership link and the application role. role has to live on this side because the membership trigger authorizes against it, and a trigger cannot read another database. user_id is stored as a value rather than a foreign key; the reviewer and community identity references become snapshots too, matching member_status_history, which already recorded its actor that way. Losing those foreign keys costs the guarantee that a reviewed user cannot be deleted. That is the price of the move, and it is now stated in the schema comments and asserted in the pgTAP suite rather than left implicit. The auth middleware is the only place that reads both sides. It resolves the subject from the authentication store and the account from the domain in two separate queries, so a future split turns the first into a remote call and leaves the second untouched. The admin views that display an account email do the same thing in batch instead of joining. A replay test asserts the tables sit in app_auth, that "user" carries no domain column, and that no foreign key crosses the boundary in either direction. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The RLS suite still required a reviewed account to be undeletable, which was the guarantee the dropped foreign key provided. That key crossed into the authentication store, so the assertion now states what replaced it: the delete succeeds and the review record keeps the reviewer it was written with. Caught by the pgTAP job, which the membership work added and which is the only thing that executes these suites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign-in now requires a public.app_accounts row, which the Better Auth create hook provisions. If that hook ever fails, or a subject is seeded outside the flow, the account exists in the authentication store with nothing on the domain side, and the JWT endpoint and the auth middleware both reject it. The user is then locked out permanently with no path back except editing the database. The sign-in path now inserts the row when it is absent, which costs one statement per sign-in and makes the missing row self-correcting. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
yufoxda
force-pushed
the
refactor/split-auth-schema
branch
from
July 27, 2026 07:46
049739a to
2e77823
Compare
Both Workers skip token verification entirely when NODE_ENV is 'development', serving every request as DEV_USER_ID. Production was safe only because the variable happened to be unset, so a single misconfigured variable would have opened the whole API, including the admin endpoints if DEV_USER_ID named an admin. The bypass now also requires the request to have arrived on a local hostname. NODE_ENV is an ordinary variable a deployment can carry by mistake; the host a request actually reached is not, so the two must agree. Both wrangler configs additionally state NODE_ENV=production, which .dev.vars overrides locally. Regression tests assert that a deployed hostname carrying NODE_ENV=development still returns 401 without touching the database, and that local development keeps working. The frontend middleware keys on NODE_ENV alone, which is correct there because Next inlines it at build time; the reasoning is recorded next to it so it is not mistaken for the same gap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
yufoxda
force-pushed
the
security/close-dev-auth-bypass
branch
from
July 27, 2026 07:46
0e479e8 to
23ab54b
Compare
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.
レビューで指摘した
DEV_USER_IDの fail-open を塞ぎます。何が問題だったか
両ワーカーは
NODE_ENV === 'development'のときトークン検証を完全にスキップし、全リクエストをDEV_USER_IDのユーザーとして処理します。本番が安全だったのは「たまたま
NODE_ENVが未設定だったから」に過ぎません。Cloudflare のダッシュボードやCIで誰かがNODE_ENV=developmentを設定した瞬間、APIが無認証で誰でも叩ける状態になります。DEV_USER_IDが管理者を指していれば管理APIも通ります。変数ひとつの設定ミスで全認証が消える構造でした。対処
バイパスの条件に「リクエストが実際にローカルのホスト名に到達したこと」を追加しました。
NODE_ENVは設定ミスで紛れ込みうる変数ですが、リクエストが到達したホスト名は環境変数で偽装できません。両者が一致しない限りバイパスは発火しません。あわせて両
wrangler.jsoncにNODE_ENV: "production"を明示しました。デプロイ済みワーカーは本番であることを既定にし、ローカルは.dev.varsが上書きします。退行防止テスト
NODE_ENV=developmentを抱えたまま本番ホスト名に来たリクエストが 401 を返し、DBに一切触れないこと(DB取得時に例外を投げるモックで担保)フロントエンドについて
frontend/src/middleware.tsもNODE_ENVだけで分岐していますが、こちらは問題ありません。Next.js はprocess.env.NODE_ENVをビルド時に定数へ置換するため、本番ビルドを実行時の変数で開発モードに戻すことはできません。同じ穴だと誤解して不要な変更が入らないよう、根拠をコード内に明記しました。
検証
tsc --noEmitクリーン🤖 Generated with Claude Code