Repository navigation
Conversation
Local testing needed real provider credentials and a hand-rolled database. `vp run dev:setup` now starts PostgreSQL with Docker Compose, migrates it, and seeds four personas plus 60 fake people. In `vp dev` with DEV_LOGIN=1, the login page offers one-click sign-in as a persona, and GET /api/dev/login?as=<persona>&redirect=<path> does the same for scripts and agents. It creates a real Better Auth session, so RBAC and the API guards run unchanged, and it resumes OpenID Connect sign-ins. The dev code is imported only behind import.meta.env.DEV and the build fails if it ever reaches a production bundle. Startup refuses DEV_LOGIN, and the public example secret, unless BETTER_AUTH_URL is localhost. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 14 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (18)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lorenzocorallo
left a comment
lorenzocorallo
left a comment
There was a problem hiding this comment.
@toto04 I reviewed and tested commit 07002f5. Please fix the database safety boundary and OIDC continuation before merging. I also reproduced an account/evidence mismatch when refreshing fixtures after configuring an Entra tenant; the inline comments explain each change.
Validation against disposable databases:
vp check,vp run build, and the documented fresh Dockervp run dev:setupflow passed.- All four personas produced the expected identity claims and API access decisions.
- The production bundle excluded the dev implementation, and
/api/dev/loginreturned 404 in the running production build. - With all integration suites enabled, 220 tests passed and 2 failed. Both failures also reproduce on base commit
b6e07a7, so I am not attributing them to this PR. They aredenies escalation through managed and custom permission implicationsanddoes not leak role members through a write-only membership response.
The normal OIDC flow reaches consent, but reauthentication requests loop. The production build boundary works in the tested build; the standalone seed script needs its own safeguards.
|
|
||
| const FAKE_PEOPLE = 60; | ||
|
|
||
| const local = ["localhost", "127.0.0.1", "[::1]"]; |
There was a problem hiding this comment.
[P1] Validate the database target before migrations or seeding.
This guard checks the web origin rather than the database destination. A localhost BETTER_AUTH_URL permits any DB_HOST. It also permits NODE_ENV=production when DEV_LOGIN is unset and the secret is not the public example.
I reproduced this against an isolated PostgreSQL container: DB_HOST=172.18.0.2, NODE_ENV=production, DEV_LOGIN unset, a real secret, and BETTER_AUTH_URL=http://localhost:3000. vp run dev:seed completed and wrote the fixtures. The same path can write fake users, trusted student evidence, and staff role grants into a shared or production database if those credentials are configured. Compiling the endpoint out does not protect this standalone tsx script.
Please reject production execution and require an explicitly designated disposable development database. Apply that validation before drizzle-kit migrate in dev:setup as well, since that command currently migrates the configured database before this guard runs. Add coverage for unsafe database targets and production execution, including attempts with DEV_LOGIN unset.
| {import.meta.env.DEV && ( | ||
| <DevLoginPanel | ||
| redirect={ | ||
| oauthFlow ? `/api/auth/oauth2/authorize?${searchStr.replace(/^\?/, "")}` : callbackURL |
There was a problem hiding this comment.
[P2] Resume OIDC through the authenticated continuation.
This redirect replays the original authorization query, including prompt=login or max_age=0. The dev endpoint creates a session outside Better Auth's normal OAuth continuation hook, which normally clears the satisfied reauthentication requirement.
I reproduced both over HTTP with a registered PKCE client: an anonymous authorize request redirects to login; selecting the student persona creates a valid session; the replayed authorize request redirects back to login instead of /consent. A request without either parameter reaches consent.
Please resume through the provider's authenticated continuation and preserve validation of the signed request. Add integration coverage for prompt=login and max_age=0 so a successful persona sign-in reaches consent.
| userId: entry.id, | ||
| updatedAt: now, | ||
| }) | ||
| .onConflictDoNothing(); |
There was a problem hiding this comment.
Refresh fixture accounts together with their evidence.
The account has a fixed primary key, so onConflictDoNothing() leaves its issuer unchanged while the evidence below is refreshed using the current configured issuer.
I reproduced seeding without an Entra tenant and then refreshing after configuring one. The pn-entra account retained the zero-UUID tenant issuer; the new evidence used the configured tenant issuer. The account/evidence join kept reading the old email and evidence, contradicting the documented update-in-place behavior.
Please update existing fixture account issuer/subject fields consistently with their evidence, after checking that the account belongs to the expected fixture user. Cover changing from the placeholder tenant to a configured tenant in a reseeding test.
2 participants
Testing locally used to mean real provider credentials and a database everyone set up their own way. Now a fresh clone needs only Docker:
What's in it
Four personas. Each gets a real Better Auth session, so RBAC, the session hook and the API guards all run unchanged.
IDP_ADMIN_USER_IDS=dev-admindev-staffrole with onlyidp:users:readandidp:roles:readSocio and Direttivo can't be personas:
identity-subject.tsalways checks them live against Entra and never trusts the database, and this PR doesn't change that.GET /api/dev/login?as=<persona>&redirect=<path>signs in and redirects./api/dev/loginalone lists the personas as JSON, andcurl -c jar 'localhost:3000/api/dev/login?as=admin'works for scripts and agents. Redirects are limited to local paths.OpenID Connect sign-ins resume. When an application sent you to the login page, picking a persona continues to
/consent.vp run dev:seedadds 60 deterministic fake people with mixed accounts and statuses, so the user directory has something to show. Running it again updates them in place.Why it can't reach production
src/dev/is imported only behindimport.meta.env.DEV, which is false invp build. The route file stays and answers 404.scripts/check-server-bundle.mjsnow fails the build if the dev code's marker shows up anywhere in.output/serveror.output/public. I confirmed it: with the guard removed,vp run buildfails and names the chunk. With the guard in place,handleDevLogin,saveDevUserand the persona names are all absent from the output.DEV_LOGIN=1, andscripts/security-config.mjsrefuses to start withDEV_LOGINset unlessBETTER_AUTH_URLis localhost andNODE_ENVisn'tproduction..env.local.examplepublishes aBETTER_AUTH_SECRETso a fresh clone runs as is, and startup refuses that value anywhere but localhost.Verified
vp checkpasses andvp testpasses (159 tests). New tests cover the startup rules, the leak check, and the redirect guard.master-admin.studentstate and its Telegram ID, and a 403 from the user directory.redirect=//evil.examplefalls back to/./userssigned in./consentafter the dev sign-in.vp run dev:setupitself, since my own.env.localpoints elsewhere. I ran the same three steps (compose, migrate, seed) by hand with the example env.🤖 Generated with Claude Code