feat: change client secret to optional for public client#14
Conversation
- Updated `createOAuthClient` function to handle client type (public/confidential). - Introduced new test suite for public client flow in `public-client.spec.ts`. - Added database migration to include `client_type` in `oauth_clients` table. - Modified validation schemas to accommodate public clients without client secrets. - Updated token request handling to allow authorization code and refresh token flows without client secrets for public clients. - Adjusted basic auth parsing to allow undefined client secrets for public clients.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR enables OAuth “public clients” by making client_secret optional in validation and persistence, adding a clientType attribute to clients, and enforcing the correct behavior at the token/revoke endpoints.
Changes:
- Make
client_secretoptional for authorization_code/refresh_token (and revoke) validation to support public clients. - Add
clientType(public/confidential) to OAuth client creation/UI and persist it in the DB (withsecretnullable for public clients). - Add E2E coverage for public-client flows and switch in-memory E2E DB setup to run Drizzle migrations.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/validations/oauth.ts | Loosens request validation to allow missing client_secret for public-client flows. |
| lib/validations/oauth.test.ts | Updates schema tests to reflect optional client_secret and adds coverage for public-client cases. |
| lib/validations/admin.ts | Adds clientType to OAuth client creation validation. |
| lib/oauth/jwt.ts | Adds jti to access tokens. |
| lib/oauth/basic-auth.ts | Adjusts Basic auth parsing to allow empty secrets (now normalized). |
| lib/db/schema/oauth-clients.ts | Adds clientType column and makes secret nullable for public clients. |
| lib/db/migrations/meta/_journal.json | Registers the new migration. |
| lib/db/migrations/meta/0003_snapshot.json | Captures updated schema snapshot including client_type and nullable secret. |
| lib/db/migrations/0003_chilly_husk.sql | Attempts to migrate schema for public clients (currently problematic on SQLite). |
| lib/db/index.ts | Runs migrations automatically for in-memory E2E DB instead of creating tables manually. |
| e2e/oauth/public-client.spec.ts | Adds full E2E public-client authorization/refresh/revoke coverage. |
| e2e/fixtures/test-helpers.ts | Extends helper to create public vs confidential clients and handle missing secrets. |
| components/admin/client-form.tsx | Adds client type selection and hides secret display for public clients. |
| app/api/oauth/token/route.ts | Enforces public-client restrictions and conditionally requires client_secret. |
| app/api/oauth/revoke/route.ts | Allows public clients to revoke without a secret; confidential clients still require it. |
| actions/admin/clients/create.ts | Creates public clients without generating/storing a secret; persists clientType. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| client_id: z.string().min(1, "client_id is required"), | ||
| client_secret: z.string().min(1, "client_secret is required"), | ||
| client_secret: z.string().optional(), | ||
| }), |
There was a problem hiding this comment.
client_secret: z.string().optional() will accept an empty string (""), which is typically not desired for OAuth client authentication fields. If the intent is to allow omission for public clients while still rejecting empty secrets when provided, use a non-empty constraint (e.g., z.string().min(1).optional()), and consider adding a test for the empty-string case.
| refresh_token: z.string().min(1, "refresh_token is required"), | ||
| client_id: z.string().min(1, "client_id is required"), | ||
| client_secret: z.string().min(1, "client_secret is required"), | ||
| client_secret: z.string().optional(), | ||
| scope: z.string().optional(), |
There was a problem hiding this comment.
Same issue as above: client_secret: z.string().optional() allows an empty string. If empty secrets should not be treated as valid input, change this to require non-empty when present (e.g., min(1).optional()).
| token_type_hint: z.enum(["access_token", "refresh_token"]).optional(), | ||
| client_id: z.string().min(1, "client_id is required"), | ||
| client_secret: z.string().min(1, "client_secret is required"), | ||
| client_secret: z.string().optional(), |
There was a problem hiding this comment.
client_secret: z.string().optional() in the revoke schema allows an empty string to pass validation. If you want to allow omission for public clients but reject empty secrets when sent, prefer z.string().min(1).optional() (and keep route behavior of always returning 200 per RFC 7009).
| client_secret: z.string().optional(), | |
| client_secret: z.string().min(1).optional(), |
| DROP INDEX "email_whitelist_email_idx";--> statement-breakpoint | ||
| ALTER TABLE `oauth_clients` ALTER COLUMN "secret" TO "secret" text;--> statement-breakpoint | ||
| CREATE UNIQUE INDEX `users_email_unique` ON `users` (`email`);--> statement-breakpoint |
There was a problem hiding this comment.
This migration uses ALTER TABLE ... ALTER COLUMN, which is not supported by SQLite/libSQL. As written, applying migrations to SQLite (including the in-memory E2E DB) will fail at this statement. To make oauth_clients.secret nullable you’ll need a table-rebuild migration pattern (create new table with nullable secret + new client_type column, copy data, drop old table, rename), or regenerate the migration using a strategy compatible with SQLite.
1 participant
No description provided.