Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a project-level config Changes
Sequence Diagram(s)sequenceDiagram
participant SDK as Client/SDK
participant Backend as Backend Auth Handler
participant Config as Project Config Service
participant Validator as Key Validator
participant OAuth as OAuth Provider
SDK->>Backend: Request with client_secret (or sentinel/absent)
Backend->>Config: getRenderedProjectConfig(projectId)
Config-->>Backend: project.requirePublishableClientKey (true/false)
alt requirePublishableClientKey == true
alt client_secret present
Backend->>Validator: checkApiKeySet(projectId, {publishableClientKey: secret})
Validator-->>Backend: Result ok / error
alt ok
Backend->>OAuth: proceed with OAuth flow
OAuth-->>SDK: success
else error
Backend-->>SDK: 401 InvalidPublishableClientKey
end
else
Backend-->>SDK: 401 PublishableClientKeyRequiredForProject
end
else requirePublishableClientKey == false
alt client_secret present
Backend->>Validator: checkApiKeySet(projectId, {publishableClientKey: secret})
Validator-->>Backend: Result ok / error
alt ok
Backend->>OAuth: proceed
OAuth-->>SDK: success
else error
Backend-->>SDK: 401 InvalidPublishableClientKey
end
else sentinel or absent
Backend->>Backend: accept sentinel / proceed without publishable key
Backend->>OAuth: proceed
OAuth-->>SDK: success
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Greptile OverviewGreptile SummaryThis PR implements a configurable Key Changes
TestingComprehensive test coverage added for:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Backend
participant OAuthProvider
participant Database
Note over Client,Database: Standard API Request Flow
Client->>Backend: Send API request
Backend->>Database: Query project configuration
Database-->>Backend: requirePublishableClientKey setting
alt Key not required
Backend-->>Client: Process request
else Key required
Backend-->>Client: 401 Unauthorized
end
Note over Client,Database: OAuth Authentication Flow
Client->>Backend: Initiate OAuth authorize
Backend->>Database: Query project configuration
alt Keyless allowed
Backend->>Database: Store flow information
Backend->>OAuthProvider: Redirect user
OAuthProvider-->>Backend: Callback with code
Backend->>Backend: Exchange code
Backend->>Database: Fetch flow information
Backend->>Database: Persist session
Backend-->>Client: Authentication complete
else Key mandatory
Backend-->>Client: 401 Unauthorized
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This pull request adds a configurable "Require publishable client key" toggle that allows projects to optionally require publishable client keys for client access. The feature defaults to false for new projects but is set to true for existing projects via a database migration to maintain backwards compatibility.
Changes:
- Adds
project.requirePublishableClientKeyconfig field at the project level that controls whether client requests must include a valid publishable client key - Introduces a
publicOAuthClientSecretSentinelconstant ("stack_public_client") to represent keyless OAuth clients - Updates OAuth flows (authorize, callback, token) to support optional publishable client keys
- Adds comprehensive E2E test coverage for the new authentication scenarios
Reviewed changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/stack-shared/src/config/schema.ts | Adds project.requirePublishableClientKey boolean field to project config schema with default value false |
| packages/stack-shared/src/known-errors.tsx | Adds new PublishableClientKeyRequiredForProject error for when keys are required but missing |
| packages/stack-shared/src/interface/client-interface.ts | Updates to support optional publishable client keys in OAuth flows |
| packages/stack-shared/src/interface/admin-interface.ts | Adds "project" level support to config override endpoints |
| packages/stack-shared/src/utils/oauth.tsx | Exports publicOAuthClientSecretSentinel constant |
| packages/template/src/lib/stack-app/projects/index.ts | Adds requirePublishableClientKey field to AdminProjectUpdateOptions |
| packages/template/src/lib/stack-app/apps/implementations/*.ts | Updates app constructors to handle optional publishable client keys |
| apps/backend/src/route-handlers/smart-request.tsx | Adds validation logic to check requirePublishableClientKey config before allowing keyless client access |
| apps/backend/src/app/api/latest/auth/oauth/*.tsx | Updates OAuth endpoints to support keyless clients when not required |
| apps/backend/src/oauth/model.tsx | Updates OAuth model to accept public client secret sentinel |
| apps/backend/src/lib/tokens.tsx | Makes publishableClientKey optional in oauthCookieSchema |
| apps/backend/src/app/api/latest/internal/config/override/[level]/route.tsx | Adds project-level config override support |
| apps/backend/prisma/migrations/*.sql | Migration that sets requirePublishableClientKey to true for existing projects |
| apps/dashboard/src/components/data-table/api-key-table.tsx | Updates API key table to conditionally show publishable client key column and handle keys without client keys |
| apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/*.tsx | Updates dashboard pages to support the new toggle and conditionally show publishable keys |
| apps/e2e/tests/backend/*.ts | Adds comprehensive E2E tests for all scenarios with optional/required publishable keys |
| examples/demo/.env.development | Removes hardcoded publishable client key to demonstrate optional behavior |
| claude/CLAUDE-KNOWLEDGE.md | Documents the new feature and related implementation details |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stack-shared/src/interface/admin-interface.ts (1)
549-575:⚠️ Potential issue | 🟠 MajorUse discriminated union typing for level-specific config overrides.
The methods accept
anybut should be strongly typed toProjectConfigOverride,BranchConfigOverride, orEnvironmentConfigOverridebased on thelevelparameter. Since schema.ts already defines these types and callers pass typed values, use overloads or a discriminated union to enforce type safety at the call site.- async setConfigOverride(level: "project" | "branch" | "environment", configOverride: any, source?: BranchConfigSourceApi): Promise<void> { + async setConfigOverride(level: "project", configOverride: ProjectConfigOverride, source?: BranchConfigSourceApi): Promise<void>; + async setConfigOverride(level: "branch", configOverride: BranchConfigOverride, source?: BranchConfigSourceApi): Promise<void>; + async setConfigOverride(level: "environment", configOverride: EnvironmentConfigOverride, source?: BranchConfigSourceApi): Promise<void>; + async setConfigOverride(level: "project" | "branch" | "environment", configOverride: ProjectConfigOverride | BranchConfigOverride | EnvironmentConfigOverride, source?: BranchConfigSourceApi): Promise<void> {Apply the same pattern to
updateConfigOverrideusing the corresponding*ConfigOverrideOverridetypes.
🤖 Fix all issues with AI agents
In `@apps/dashboard/src/components/data-table/api-key-table.tsx`:
- Around line 96-102: serverKeyColumn currently renders "*******" even when
secretServerKey is null; update serverKeyColumn so accessorFn returns the
lastFour or null (e.g., row.secretServerKey?.lastFour ?? null) and change the
cell renderer (serverKeyColumn.cell) to conditionally render the masked value
plus lastFour when row.original.secretServerKey exists, otherwise render a clear
placeholder like "No key" or "—" inside the same TextCell component; keep
enableSorting false and header as-is.
In `@apps/e2e/tests/backend/backend-helpers.ts`:
- Around line 657-671: The includeClientSecret flag in OAuth.getAuthorizeQuery
is misleading because when includeClientSecret is false the code still sets
client_secret to publicOAuthClientSecretSentinel; change the logic so that when
options.includeClientSecret is false you set client_secret to undefined (instead
of publicOAuthClientSecretSentinel) so the parameter is truly omitted in the
generated query; update the same pattern in the other occurrence of
OAuth.getAuthorizeQuery (the later block around the similar client_secret
construction) and keep references to includeClientSecret and
publicOAuthClientSecretSentinel to locate and modify the assignments.
In `@packages/stack-shared/src/known-errors.tsx`:
- Around line 422-433: Narrow the `json` parameter in the
`constructorArgsFromJson` callback for PublishableClientKeyRequiredForProject
instead of using `any`: update the callback signature in the call to
createKnownErrorConstructor so `json` is typed (e.g. `{ project_id?: string |
null }`) and cast/return `[json.project_id ?? undefined] as const`; also add the
short explanatory comment used elsewhere in this file describing why we narrow
the shape locally for type safety. Reference:
PublishableClientKeyRequiredForProject and the constructorArgsFromJson callback
passed to createKnownErrorConstructor.
🧹 Nitpick comments (5)
apps/backend/src/app/api/latest/auth/oauth/callback/[provider_id]/route.tsx (1)
18-19: Centralize the public OAuth sentinel constant.To avoid drift across routes/models, import the shared sentinel from stack-shared instead of redefining it locally.
♻️ Suggested change
-import { KnownError, KnownErrors } from "@stackframe/stack-shared"; +import { KnownError, KnownErrors } from "@stackframe/stack-shared"; +import { publicOAuthClientSecretSentinel } from "@stackframe/stack-shared/dist/utils/oauth"; @@ -const publicOAuthClientSecretSentinel = "__stack_public_client__";apps/backend/src/oauth/model.tsx (1)
45-56: Reuse the shared public-client sentinel constant.This avoids string duplication across OAuth flows and keeps the sentinel consistent everywhere.
♻️ Suggested change
-import { KnownErrors } from "@stackframe/stack-shared"; +import { KnownErrors } from "@stackframe/stack-shared"; +import { publicOAuthClientSecretSentinel } from "@stackframe/stack-shared/dist/utils/oauth"; @@ - const publicOAuthClientSecretSentinel = "__stack_public_client__";apps/backend/src/app/api/latest/auth/oauth/token/route.tsx (1)
30-30: Import the sentinel constant instead of redefining it.The
publicOAuthClientSecretSentinelis already defined inpackages/stack-shared/src/utils/oauth.tsx. Importing it ensures consistency and avoids potential drift if the value changes.♻️ Proposed fix
import { getSoleTenancyFromProjectBranch } from "@/lib/tenancies"; import { getProjectBranchFromClientId, oauthServer } from "@/oauth"; +import { publicOAuthClientSecretSentinel } from "@stackframe/stack-shared/dist/utils/oauth"; import { createSmartRouteHandler } from "@/route-handlers/smart-route-handler"; ... async handler(req, fullReq) { - const publicOAuthClientSecretSentinel = "__stack_public_client__"; const clientId = req.body.client_id;apps/backend/src/app/api/latest/auth/oauth/authorize/[provider_id]/route.tsx (1)
17-17: Import the sentinel constant from shared utils.Same as the token route - this constant is already defined in
packages/stack-shared/src/utils/oauth.tsx. Import it for consistency.♻️ Proposed fix
+import { publicOAuthClientSecretSentinel } from "@stackframe/stack-shared/dist/utils/oauth"; ... -const publicOAuthClientSecretSentinel = "__stack_public_client__";apps/e2e/tests/backend/backend-helpers.ts (1)
1238-1246: Please justify the newanyparameter (or tighten it).Consider adding a short comment explaining why
anyis needed, or replace it with a concrete type/unknown.📝 Minimal guideline-compliant tweak
-export async function updateProjectConfig(config: any) { +export async function updateProjectConfig( + // Config shape varies per test; allow free-form overrides here. + config: any, +) {As per coding guidelines, Try to avoid the
anytype; whenever you need to useany, leave a comment explaining why you're using it.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sdks/spec/src/apps/client-app.spec.md (1)
770-781:⚠️ Potential issue | 🟡 MinorFix list indentation for the client_secret bullet.
Line 780 is flagged by MD005 (inconsistent list indentation). Align it with the other Body list items.🛠️ Suggested fix
- - client_secret=<publishableClientKey | publishableClientKeyNotNecessarySentinel> + - client_secret=<publishableClientKey | publishableClientKeyNotNecessarySentinel>packages/stack-shared/src/interface/client-interface.ts (1)
158-170:⚠️ Potential issue | 🟡 MinorNormalize empty publishableClientKey to avoid empty client_secret.
Line 158 uses??, so an empty string becomesclient_secret="", while Line 328 treats empty as “missing”. This can cause OAuth failures when the key is optional. Consider normalizing to a truthy check (and reusing it in getOAuthUrl/callOAuthCallback).🛠️ Suggested fix (apply similarly in other occurrences)
- const clientSecret = this.options.publishableClientKey ?? publishableClientKeyNotNecessarySentinel; + const publishableClientKey = + "publishableClientKey" in this.options ? this.options.publishableClientKey : undefined; + const clientSecret = + publishableClientKey && publishableClientKey.length > 0 + ? publishableClientKey + : publishableClientKeyNotNecessarySentinel;Also applies to: 995-999, 1036-1048
🤖 Fix all issues with AI agents
In `@apps/e2e/tests/backend/backend-helpers.ts`:
- Around line 1237-1245: Change the updateProjectConfig parameter from any to
the concrete ProjectConfigOverride type: update the function signature of
updateProjectConfig to accept config: ProjectConfigOverride, import
ProjectConfigOverride from packages/stack-shared/src/config/schema (where it’s
defined), and keep the existing JSON.stringify usage and call to
niceBackendFetch unchanged; ensure the import is a type import if your TS
settings require it.
In `@sdks/spec/src/_utilities.spec.md`:
- Around line 34-37: Update the "Required Headers" doc to mark the
x-stack-publishable-client-key header as conditional based on the
requirePublishableClientKey flag: note that when requirePublishableClientKey is
false, callers may use the sentinel publishableClientKeyNotNecessarySentinel
("__stack_public_client__") as the OAuth client_secret and the
x-stack-publishable-client-key header is optional; keep the header described as
required only when requirePublishableClientKey is true and include a short
example or parenthetical indicating the sentinel usage.
🧹 Nitpick comments (1)
sdks/implementations/swift/Sources/StackAuth/APIClient.swift (1)
314-323: Consider reusinggetOAuthClientSecret()for consistency.Line 318 duplicates the sentinel fallback logic that already exists in
getOAuthClientSecret(). While functionally correct, reusing the helper would improve maintainability.♻️ Suggested refactor
if let publishableClientKey = publishableClientKey { request.setValue(publishableClientKey, forHTTPHeaderField: "x-stack-publishable-client-key") } - let oauthClientSecret = publishableClientKey ?? publishableClientKeyNotNecessarySentinel + let oauthClientSecret = getOAuthClientSecret() let body = [ "grant_type=refresh_token", "refresh_token=\(formURLEncode(refreshToken))", "client_id=\(formURLEncode(projectId))", "client_secret=\(formURLEncode(oauthClientSecret))" ].joined(separator: "&")
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/backend/src/app/api/latest/auth/oauth/callback/`[provider_id]/route.tsx:
- Around line 178-182: The publishable-client-key validation currently happens
after providerObj.getCallback(), which consumes the external OAuth auth code;
move the checkApiKeySet call (and the throwPublishableKeyError path) to run
earlier—immediately after tenancy is resolved (i.e., after you have tenancy and
outerInfo.publishableClientKey) and before invoking providerObj.getCallback()—so
we fail fast if the publishable key is invalid and avoid consuming the provider
callback unnecessarily.
In `@sdks/spec/src/_utilities.spec.md`:
- Line 22: Fix the typo ("must functions" -> "most functions") and rewrite the
note sentence to clearly state when the header is required versus optional and
what to do if omitted: replace the existing line in _utilities.spec.md with a
concise sentence that says when requirePublishableClientKey is true the header
is required and functions will throw if it's missing, when the flag is false the
header is optional, and if omitted you should use
publishableClientKeyNotNecessarySentinel for the OAuth client_secret.
🧹 Nitpick comments (7)
apps/backend/src/route-handlers/smart-request.tsx (2)
262-265: Accessing.statuson potentially undefined query results relies on implicit control-flow guarantees.
isClientKeyValid,isServerKeyValid, andisAdminKeyValidareundefinedwhen their respective bundled-query conditions (lines 252–254) are false. The laterswitchcases guard this correctly at runtime (e.g.,publishableClientKeyis truthy before.statusis accessed on line 285), but TypeScript can't prove the relationship between the bundled-query condition and the switch branch. Per the coding guidelines, prefer defensive coding with explicit assertions.Proposed defensive access
case "client": { if (!publishableClientKey) { if (requiresPublishableClientKey) { throw new KnownErrors.PublishableClientKeyRequiredForProject(projectId); } break; } - if (isClientKeyValid.status === "error") { + if ((isClientKeyValid ?? throwErr(new StackAssertionError("Expected isClientKeyValid to be defined when publishableClientKey is present"))).status === "error") {Apply the same pattern for
isServerKeyValid(line 295) andisAdminKeyValid(line 300).Based on coding guidelines: "Code defensively; prefer
?? throwErr(...)over non-null assertions with good error messages that explicitly state the assumption."
265-265: Fallback?? trueis the safe default but would benefit from a brief comment.When
tenancyisnull(branch doesn't exist — caught later on line 309),requiresPublishableClientKeydefaults totrue, which is the stricter/safer path. A short inline comment explaining this intentional choice would prevent future readers from mistaking it for an accidental fallback.apps/e2e/tests/backend/backend-helpers.ts (2)
840-843: Duplicated sentinel fallback logic — consider extracting a helper.The
clientSecretcomputation (lines 840–843) is identical to lines 663–666 ingetAuthorizeQuery. Consider extracting this into a small shared helper within theOAuthnamespace to keep it DRY.♻️ Suggested refactor
export namespace OAuth { + function resolveClientSecret(includeClientSecret: boolean): string { + const projectKeys = backendContext.value.projectKeys; + if (projectKeys === "no-project") throw new Error("No project keys found in the backend context"); + return includeClientSecret + ? (projectKeys.publishableClientKey ?? publishableClientKeyNotNecessarySentinel) + : publishableClientKeyNotNecessarySentinel; + }
848-855:filterUndefinedon the token body is currently a no-op — verify intent.The body is wrapped in
filterUndefinedbutclientSecretis neverundefined(it's always the real key or the sentinel). If this wrapper was added to support truly omittingclient_secretwhenincludeClientSecretisfalse, then theclientSecretvariable should be set toundefinedin that branch (aligning with the earlier comment about the misleading flag). Otherwise, thefilterUndefinedcall is unnecessary here.apps/backend/src/lib/internal-api-keys.tsx (2)
59-63: Simplified return — the intermediate variable is unnecessary.Minor nit:
resultis assigned and immediately returned with no transformation.♻️ Optional simplification
export async function checkApiKeySet(projectId: string, key: KeyType): Promise<Result<void, CheckApiKeySetError>> { - const result = await rawQuery(globalPrismaClient, checkApiKeySetQuery(projectId, key)); - - return result; + return await rawQuery(globalPrismaClient, checkApiKeySetQuery(projectId, key)); }
54-55: Removeasyncor the explicit return type annotation for clarity.Per the RawQuery type definition (prisma-client.tsx, line 514), using an
asyncpostProcess is the documented pattern. However, the explicit: Promise<Result<void, CheckApiKeySetError>>annotation is redundant—theasynckeyword already wraps the return value in a Promise. Either removeasyncand directly return the Promise, or omit the type annotation and let TypeScript infer it from theasyncfunction. The code works correctly due to theAwaited<>utility inrawQuery's return type, but the annotation creates unnecessary redundancy.apps/backend/src/app/api/latest/auth/oauth/callback/[provider_id]/route.tsx (1)
19-24:throwPublishableKeyErroris duplicated across authorize and callback routes.This helper is identical in
authorize/[provider_id]/route.tsx(line 16-21) and here. Extract it into a shared module (e.g.,../oauth-helpers) to avoid drift.#!/bin/bash # Verify all instances of throwPublishableKeyError across the codebase rg -n "throwPublishableKeyError\|throwClientSecretError" --type=ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@sdks/spec/src/apps/client-app.spec.md`:
- Line 781: Fix the inconsistent list indentation by aligning the list item
"client_secret=<publishableClientKey |
publishableClientKeyNotNecessarySentinel>" to use the same 5-space indent as its
sibling items; update the markdown so this line's leading spaces match the other
list entries to satisfy MD005.
🧹 Nitpick comments (3)
apps/backend/src/lib/internal-api-keys.tsx (2)
79-79:anyparameter without explanatory comment.Per coding guidelines, uses of
anyshould include a comment explaining why. This function acts as a runtime type-narrowing validator, so theanyis reasonable — but please add a brief comment.📝 Suggested fix
-function validateKeyType(obj: any): KeyType { +// `any` because this validates untrusted input and narrows to KeyType at runtime +function validateKeyType(obj: any): KeyType {As per coding guidelines: "Try to avoid the
anytype; whenever you need to useany, leave a comment explaining why you're using it."
47-49:Prisma.raw(JSON.stringify(keyType))for column names is fragile but currently safe.
keyTypeis validated to one of three known literals byvalidateKeyTypeat Line 30, so there's no injection risk today. However,Prisma.rawbypasses parameterization — ifvalidateKeyTypeis ever loosened, this becomes a SQL injection vector. A comment documenting the safety invariant would help future maintainers.apps/backend/src/app/api/latest/auth/oauth/token/route.tsx (1)
96-98: Pre-existing(e as any).innercast — consider addressing in a follow-up.This
as anybypasses the type system to access.inneronServerError. Not introduced by this PR, but if you're touching this file, a typed approach (e.g., checking'inner' in e) would be safer. Non-blocking.As per coding guidelines: "Do not use
as/any/type casts or anything else to bypass the type system unless you specifically asked the user about it."
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/stack-shared/src/interface/admin-interface.ts (1)
610-639:⚠️ Potential issue | 🟠 MajorAvoid
anyfor config overrides.Use a typed shape instead of
anyto keep the interface safe.As per coding guidelines, “Try to avoid the `any` type. Whenever using `any`, leave a comment explaining why you're using it” and “Do NOT use `as`/`any`/type casts or anything else to bypass the type system unless explicitly discussed with the user. Avoid wherever possible.”♻️ Suggested typing
- async setConfigOverride(level: "project" | "branch" | "environment", configOverride: any, source?: BranchConfigSourceApi): Promise<void> { + async setConfigOverride(level: "project" | "branch" | "environment", configOverride: Record<string, unknown>, source?: BranchConfigSourceApi): Promise<void> { await this.sendAdminRequest( `/internal/config/override/${level}`, { method: "PUT", @@ - async updateConfigOverride(level: "project" | "branch" | "environment", configOverrideOverride: any): Promise<void> { + async updateConfigOverride(level: "project" | "branch" | "environment", configOverrideOverride: Record<string, unknown>): Promise<void> { await this.sendAdminRequest( `/internal/config/override/${level}`, { method: "PATCH",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack-shared/src/interface/admin-interface.ts` around lines 610 - 639, Replace the use of `any` for config override parameters with a proper typed shape (e.g., declare or import a ConfigOverride interface or type like ConfigOverride = Record<string, unknown> or a concrete shape) and update the method signatures of setConfigOverride(level: "project" | "branch" | "environment", configOverride: ConfigOverride, source?: BranchConfigSourceApi) and updateConfigOverride(level: "project" | "branch" | "environment", configOverrideOverride: ConfigOverride) accordingly; ensure the existing JSON.stringify calls still work (no casts) and if you truly cannot type a field, use `unknown` and validate/transform it before stringifying, or add a brief comment explaining why `any` was unavoidable.apps/backend/src/app/api/latest/internal/config/override/[level]/route.tsx (1)
237-241:⚠️ Potential issue | 🟠 MajorDownstream of missing
validateguard — both PUT and PATCH handlers affectedBoth the PUT (line 237) and PATCH (line 284) handlers call
warnOnValidationFailure(levelConfig, ...)without checking whether the resolvedlevelConfigsupports validation. The false-positivecaptureErrorwill fire for every project-level write until the fix inwarnOnValidationFailureitself is applied (see comment above).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/app/api/latest/internal/config/override/`[level]/route.tsx around lines 237 - 241, Both the PUT and PATCH handlers call warnOnValidationFailure(levelConfig, ...) unconditionally; add a guard to ensure the resolved levelConfig actually supports validation (e.g., check for a validate method or a supportsValidation flag on levelConfig) before invoking warnOnValidationFailure. Update both the PUT handler (where levelConfig is used before line 237) and the PATCH handler (around line 284) to only call warnOnValidationFailure when the guard passes, referencing the same levelConfig variable and keeping the existing arguments.packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (1)
416-426:⚠️ Potential issue | 🟡 MinorUse explicit null check instead of boolean truthiness for
publishableClientKey
publishableClientKeyis typed asstring | undefined. The truthiness guardpublishableClientKey ?silently excludes an empty string"", while!= nullcorrectly passes it through to let downstream validation reject it explicitly.As per coding guidelines: "Unless very clearly equivalent from types, prefer explicit null/undefinedness checks over boolean checks (e.g.,
foo == nullinstead of!foo)".🔧 Proposed fix
- ...(publishableClientKey ? { publishableClientKey } : {}), + ...(publishableClientKey != null ? { publishableClientKey } : {}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts` around lines 416 - 426, The spread into the StackClientInterface currently uses a truthiness check on publishableClientKey which will skip empty strings; change the guard to an explicit null/undefined check so empty string values are passed through for downstream validation: update the conditional that spreads { publishableClientKey } into the new StackClientInterface (the publishableClientKey variable derived from resolvedOptions.publishableClientKey) to use publishableClientKey != null instead of the current boolean/truthiness check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/app/api/latest/internal/config/override/`[level]/route.tsx:
- Around line 40-57: The project config entry currently lacks a validate
function which causes warnOnValidationFailure to call levelConfig.validate(...)
and throw; update warnOnValidationFailure to first check that
levelConfig.validate exists and is a function (e.g. if (typeof
levelConfig.validate !== 'function') return/skip) before invoking it so
validation becomes a no-op for levels without validate; alternatively you can
add a no-op validate to the project entry, but prefer the guard in
warnOnValidationFailure to avoid calling undefined functions.
- Around line 162-164: warnOnValidationFailure currently assumes levelConfig has
a validate() method but project-level configs lack it; modify the two call sites
that pass levelConfig (the places that call warnOnValidationFailure with
req.params.level) to only invoke warnOnValidationFailure when req.params.level
!== "project", or alternatively update warnOnValidationFailure to accept the
broader union type and early-return when levelConfig.validate is undefined
(i.e., check for typeof levelConfig.validate === "function" before calling
validate()). Ensure references include the existing function name
warnOnValidationFailure and the parseAndValidateConfig/levelConfig callers so
reviewers can locate the changes.
In `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts`:
- Around line 2866-2873: In toClientJson replace the truthy guard
"publishableClientKey ? { publishableClientKey } : {}" with a
null/undefined-safe check using "publishableClientKey != null" (same change as
made in the constructor) so undefined or empty-string corner cases are handled
correctly; locate the logic inside the toClientJson method of the
client-app-impl implementation and update the conditional expression to use !=
null to only omit the field when the value is null or undefined.
In `@packages/template/src/lib/stack-app/apps/implementations/server-app-impl.ts`:
- Around line 386-396: The current ternary uses publishableClientKey ? {
publishableClientKey } : {} which treats an empty string as falsy and silently
omits it; instead, read the raw resolvedOptions.publishableClientKey and
validate it for non-empty string before constructing the StackServerInterface
options: if resolvedOptions.publishableClientKey === undefined then omit the
field, but if it's provided and is an empty string (or otherwise invalid) throw
or return a clear error so callers fail loudly; update the logic around
publishableClientKey, StackServerInterface instantiation, and
getDefaultPublishableClientKey() usage to only fall back to the default when
explicitly undefined and to validate non-empty values otherwise.
---
Outside diff comments:
In `@apps/backend/src/app/api/latest/internal/config/override/`[level]/route.tsx:
- Around line 237-241: Both the PUT and PATCH handlers call
warnOnValidationFailure(levelConfig, ...) unconditionally; add a guard to ensure
the resolved levelConfig actually supports validation (e.g., check for a
validate method or a supportsValidation flag on levelConfig) before invoking
warnOnValidationFailure. Update both the PUT handler (where levelConfig is used
before line 237) and the PATCH handler (around line 284) to only call
warnOnValidationFailure when the guard passes, referencing the same levelConfig
variable and keeping the existing arguments.
In `@packages/stack-shared/src/interface/admin-interface.ts`:
- Around line 610-639: Replace the use of `any` for config override parameters
with a proper typed shape (e.g., declare or import a ConfigOverride interface or
type like ConfigOverride = Record<string, unknown> or a concrete shape) and
update the method signatures of setConfigOverride(level: "project" | "branch" |
"environment", configOverride: ConfigOverride, source?: BranchConfigSourceApi)
and updateConfigOverride(level: "project" | "branch" | "environment",
configOverrideOverride: ConfigOverride) accordingly; ensure the existing
JSON.stringify calls still work (no casts) and if you truly cannot type a field,
use `unknown` and validate/transform it before stringifying, or add a brief
comment explaining why `any` was unavoidable.
In `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts`:
- Around line 416-426: The spread into the StackClientInterface currently uses a
truthiness check on publishableClientKey which will skip empty strings; change
the guard to an explicit null/undefined check so empty string values are passed
through for downstream validation: update the conditional that spreads {
publishableClientKey } into the new StackClientInterface (the
publishableClientKey variable derived from resolvedOptions.publishableClientKey)
to use publishableClientKey != null instead of the current boolean/truthiness
check.
---
Duplicate comments:
In `@apps/e2e/tests/backend/backend-helpers.ts`:
- Around line 708-722: The OAuth.getAuthorizeQuery helper currently assigns
publishableClientKeyNotNecessarySentinel to client_secret even when
includeClientSecret is false, so the parameter is still sent; change the logic
in OAuth.getAuthorizeQuery (and the similar block around lines 886-906) to set
client_secret to undefined when options.includeClientSecret is false (i.e.,
clientSecret = includeClientSecret ? (projectKeys.publishableClientKey ??
publishableClientKeyNotNecessarySentinel) : undefined) so filterUndefined will
remove the key and truly simulate a missing parameter; reference
OAuth.getAuthorizeQuery, includeClientSecret, projectKeys.publishableClientKey,
publishableClientKeyNotNecessarySentinel, and filterUndefined to locate and
update the code.
In `@packages/template/src/lib/stack-app/apps/implementations/admin-app-impl.ts`:
- Around line 129-143: The code in the super(...) options block uses a truthy
check for publishableClientKey which will drop valid empty-string values; change
the conditional that spreads publishableClientKey into the StackAdminInterface
options to test explicitly for undefined (e.g., publishableClientKey !==
undefined) so an empty string is preserved, and update the spread that currently
reads ...(publishableClientKey ? { publishableClientKey } : {}) to use the
explicit undefined check; locate this change in the constructor flow around
resolveConstructorOptions, publishableClientKey, and the new StackAdminInterface
instantiation.
In `@sdks/spec/src/apps/client-app.spec.md`:
- Line 781: The list item "- client_secret=<publishableClientKey |
publishableClientKeyNotNecessarySentinel>" has incorrect indentation compared to
its sibling list items (MD005); adjust the leading spaces so this dash aligns
with the other list entries in the same list block (use the same number of
spaces/tabs and the same list marker style as the sibling items) to restore
consistent list indentation.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Note
High Risk
Touches authentication and OAuth token/authorize flows and changes how client requests are validated, so regressions could cause widespread login/client-access failures. Also includes a data migration that alters effective security posture for existing projects.
Overview
Adds a project-level toggle (
project.requirePublishableClientKey) to control whether client requests/OAuth flows must include a publishable client key, including a DB migration that backfills existing projects to require it.Backend auth now treats the publishable client key as optional when allowed, introducing a public sentinel (
__stack_public_client__) and returning a new specific error (PUBLISHABLE_CLIENT_KEY_REQUIRED_FOR_PROJECT) across smart request auth + OAuthauthorize/callback/tokenendpoints.Dashboard and SDKs update key generation/display and request construction to handle missing publishable keys, expose an advanced toggle on the Project Keys page, and extend internal config overrides to support a new
projectlevel; E2E/tests and schema fuzzing are expanded accordingly, and CI adds a forward-compat migration check job when back-compat fails.Written by Cursor Bugbot for commit 5d06c08. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Documentation