-
Notifications
You must be signed in to change notification settings - Fork 512
fix(email-config): show actual env vars in dashboard when isShared=true #1386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| import { afterEach, describe, expect, it, vi } from "vitest"; | ||
| import { getConfigForInternalConfigResponse } from "./route"; | ||
|
|
||
| describe("getConfigForInternalConfigResponse", () => { | ||
| afterEach(() => { | ||
| vi.unstubAllEnvs(); | ||
| }); | ||
|
|
||
| it("injects the configured sender email for shared email server configs", () => { | ||
| vi.stubEnv("STACK_EMAIL_SENDER", "noreply@lolcalho.st"); | ||
| const config = { | ||
| emails: { | ||
| server: { | ||
| isShared: true, | ||
| senderEmail: undefined, | ||
| }, | ||
| }, | ||
| untouched: "value", | ||
| }; | ||
|
|
||
| const result = getConfigForInternalConfigResponse(config); | ||
|
|
||
| expect(result).toEqual({ | ||
| emails: { | ||
| server: { | ||
| isShared: true, | ||
| senderEmail: "noreply@lolcalho.st", | ||
| }, | ||
| }, | ||
| untouched: "value", | ||
| }); | ||
| expect(config.emails.server.senderEmail).toBeUndefined(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,23 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { createSmartRouteHandler } from "@/route-handlers/smart-route-handler"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { adaptSchema, adminAuthTypeSchema, yupNumber, yupObject, yupString } from "@stackframe/stack-shared/dist/schema-fields"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { getEnvVariable } from "@stackframe/stack-shared/dist/utils/env"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function getConfigForInternalConfigResponse<T extends { emails: { server: { isShared?: boolean, senderEmail?: string | undefined } } }>(config: T): T { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!config.emails.server.isShared) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return config; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ...config, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| emails: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ...config.emails, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| server: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ...config.emails.server, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| senderEmail: getEnvVariable("STACK_EMAIL_SENDER"), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/backend/src/app/api/latest/internal/config/route.tsx
Line: 16
Comment:
**`getEnvVariable` throws when `STACK_EMAIL_SENDER` is unset**
`getEnvVariable` (no default value) calls `throwErr('Missing environment variable: STACK_EMAIL_SENDER')` if the env var is absent. Any self-hosted instance that has `isShared=true` in its config but hasn't explicitly set `STACK_EMAIL_SENDER` will receive a 500 error from the `/internal/config` endpoint every time the dashboard loads email settings — effectively breaking the UI for them. Pass the hardcoded fallback as the default to avoid throwing:
```suggestion
senderEmail: getEnvVariable("STACK_EMAIL_SENDER", "noreply@stackframe.co"),
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+5
to
+20
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Locate and inspect getEnvVariable's implementation + signature
fd -e ts -e tsx 'env\.(ts|tsx)$' packages/stack-shared/src | head -20
rg -nP -C3 'export\s+function\s+getEnvVariable' packages/stack-shared/src
# Check how STACK_EMAIL_SENDER is consumed elsewhere to see conventions
rg -nP -C2 'STACK_EMAIL_SENDER'Repository: stack-auth/stack-auth Length of output: 2907 🏁 Script executed: #!/bin/bash
# Check the full test file for the route
cat -n apps/backend/src/app/api/latest/internal/config/route.test.tsx
# Also check if there are tests that verify error handling
rg -A 10 -B 5 'getEnvVariable.*STACK_EMAIL_SENDER' apps/backend/src/app/api/latest/internal/config/route.test.tsxRepository: stack-auth/stack-auth Length of output: 1173 🏁 Script executed: #!/bin/bash
# Check .env templates to see if STACK_EMAIL_SENDER is marked as required
fd -name '.env*' | xargs grep -l 'STACK_EMAIL_SENDER' 2>/dev/null
# Check the contents of .env files related to email sender
rg -B 2 -A 2 'STACK_EMAIL_SENDER' '\.env' --type-list | head -30
# Also look for any comments or docs on shared email server setup
rg -i 'shared.*email.*server|email.*sender' apps/backend/src/app/api/latest/internal/config/ -B 2 -A 2Repository: stack-auth/stack-auth Length of output: 2698 🏁 Script executed: #!/bin/bash
# Find .env files that mention STACK_EMAIL_SENDER
find . -name '.env*' -type f | xargs grep -l 'STACK_EMAIL_SENDER' 2>/dev/null
# Check what those files say about the variable
find . -name '.env*' -type f | xargs grep 'STACK_EMAIL_SENDER' 2>/dev/null
# Look for documentation on email server configuration
rg 'shared.*email|email.*sender' -i --type mdRepository: stack-auth/stack-auth Length of output: 328 🏁 Script executed: #!/bin/bash
# Check git history for this file to see what changed
git log --oneline -n 20 -- apps/backend/src/app/api/latest/internal/config/route.tsx
# Get recent diff to see if this is a new file or recent change
git show HEAD:apps/backend/src/app/api/latest/internal/config/route.tsx 2>/dev/null | head -30
# Check if there's a prior version with fallback
git log -p -- apps/backend/src/app/api/latest/internal/config/route.tsx 2>/dev/null | head -100Repository: stack-auth/stack-auth Length of output: 1089 🏁 Script executed: #!/bin/bash
# Verify that docker/server setup is used in production or tests
grep -r 'docker/server' --include='*.md' --include='*.yml' --include='*.yaml' | head -10
# Check if there are integration tests that would catch this
rg 'docker.*server|STACK_EMAIL_SENDER.*empty|getConfigForInternalConfigResponse' --type ts --type tsx | grep -i testRepository: stack-auth/stack-auth Length of output: 527 Add defensive fallback for The code unconditionally calls The current test suite only covers the case where the env var is set; there's no coverage for the empty/unset scenario. Use a defensive pattern with a default value and fallback to preserve backward-compatibility: 🛡️ Defensive fallback suggestion-export function getConfigForInternalConfigResponse<T extends { emails: { server: { isShared?: boolean, senderEmail?: string | undefined } } }>(config: T): T {
- if (!config.emails.server.isShared) {
- return config;
- }
-
- return {
- ...config,
- emails: {
- ...config.emails,
- server: {
- ...config.emails.server,
- senderEmail: getEnvVariable("STACK_EMAIL_SENDER"),
- },
- },
- };
-}
+export function getConfigForInternalConfigResponse<T extends { emails: { server: { isShared?: boolean, senderEmail?: string | undefined } } }>(config: T): T {
+ if (!config.emails.server.isShared) {
+ return config;
+ }
+
+ // Use the env-configured sender for self-hosted shared servers; fall back to the
+ // existing senderEmail if STACK_EMAIL_SENDER isn't set so we don't break the endpoint.
+ const envSender = getEnvVariable("STACK_EMAIL_SENDER", "");
+ const senderEmail = envSender || config.emails.server.senderEmail;
+
+ return {
+ ...config,
+ emails: {
+ ...config.emails,
+ server: {
+ ...config.emails.server,
+ senderEmail,
+ },
+ },
+ };
+}Also add a test case for the unset/empty scenario to prevent regressions. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export const GET = createSmartRouteHandler({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| metadata: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -22,11 +40,13 @@ export const GET = createSmartRouteHandler({ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }).defined(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| handler: async (req) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const config = getConfigForInternalConfigResponse(req.auth.tenancy.config); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| statusCode: 200, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bodyType: "json", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| body: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config_string: JSON.stringify(req.auth.tenancy.config), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config_string: JSON.stringify(config), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,20 @@ type ServerType = "shared" | "managed" | "resend" | "standard"; | |
|
|
||
| type ManagedDomainStatus = "pending_dns" | "pending_verification" | "verified" | "applied" | "failed"; | ||
|
|
||
| const DEFAULT_SHARED_SENDER_EMAIL = "noreply@stackframe.co"; | ||
|
|
||
| type ServerFieldConfig = { | ||
| label: string, | ||
| key: string, | ||
| type: "text" | "email" | "number" | "password", | ||
| }; | ||
|
|
||
| const SERVER_TYPE_LABELS: Record<Exclude<ServerType, "shared">, string> = { | ||
| managed: "Managed (via managed domain setup)", | ||
| resend: "Resend", | ||
| standard: "Custom SMTP", | ||
| }; | ||
|
|
||
| type ManagedDomain = { | ||
| domainId: string, | ||
| subdomain: string, | ||
|
|
@@ -57,6 +71,10 @@ type SetupState = { | |
| status: ManagedDomainStatus, | ||
| }; | ||
|
|
||
| function getSharedServerTypeLabel(senderEmail: string | undefined): string { | ||
| return `Shared (${senderEmail || DEFAULT_SHARED_SENDER_EMAIL})`; | ||
| } | ||
|
Comment on lines
+74
to
+76
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No callsite in this file, and it isn't exported. If the intent was to compose the "Stack Shared (email@…)" label shown in the provider card, note that the 🤖 Prompt for AI Agents |
||
|
|
||
| const MANAGED_DOMAIN_STATUS_LABELS: Record<ManagedDomainStatus, string> = { | ||
| pending_dns: "Waiting for DNS", | ||
| pending_verification: "Verifying…", | ||
|
|
@@ -82,7 +100,7 @@ function getServerTypeFromConfig(config: CompleteConfig["emails"]["server"]): Se | |
|
|
||
| function getFormValuesFromConfig(config: CompleteConfig["emails"]["server"], projectName: string): Record<string, string> { | ||
| if (config.isShared) { | ||
| return { senderEmail: "noreply@stackframe.co", senderName: projectName }; | ||
| return { senderEmail: config.senderEmail ?? "", senderName: projectName }; | ||
| } | ||
| if (config.provider === "managed") { | ||
| const senderEmail = config.managedSubdomain && config.managedSenderLocalPart | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,8 @@ import { PageLayout } from "../page-layout"; | |
| import { useAdminApp } from "../use-admin-app"; | ||
| import { DesignAnalyticsCard } from "@/components/design-components"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/emails/page-client.tsx
Line: 22
Comment:
**Duplicated `DEFAULT_SHARED_SENDER_EMAIL` constant**
`DEFAULT_SHARED_SENDER_EMAIL = "noreply@stackframe.co"` is defined identically in both `page-client.tsx` and `domain-settings.tsx`. If the address ever changes it must be updated in two places. Consider extracting it into a shared constants file (e.g. `email-constants.ts`) that both modules can import.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| const DEFAULT_SHARED_SENDER_EMAIL = "noreply@stackframe.co"; | ||
|
|
||
| // Section header with icon following design guide | ||
| function SectionHeader({ icon: Icon, title }: { icon: ElementType, title: string }) { | ||
| return ( | ||
|
|
@@ -139,7 +141,7 @@ function EmailServerCard({ emailConfig }: { emailConfig: CompleteConfig['emails' | |
| : (emailConfig.provider === 'resend' ? 'Resend' : 'Custom SMTP'); | ||
|
|
||
| const senderEmail = emailConfig.isShared | ||
| ? 'noreply@stackframe.co' | ||
| ? (emailConfig.senderEmail ?? DEFAULT_SHARED_SENDER_EMAIL) | ||
| : emailConfig.provider === 'managed' && emailConfig.managedSubdomain && emailConfig.managedSenderLocalPart | ||
| ? `${emailConfig.managedSenderLocalPart}@${emailConfig.managedSubdomain}` | ||
| : emailConfig.senderEmail; | ||
|
|
@@ -602,7 +604,7 @@ const getDefaultValues = (emailConfig: CompleteConfig['emails']['server'] | unde | |
| if (!emailConfig) { | ||
| return { type: 'shared', senderName: project.displayName } as const; | ||
| } else if (emailConfig.isShared) { | ||
| return { type: 'shared' } as const; | ||
| return { type: 'shared', senderEmail: emailConfig.senderEmail } as const; | ||
| } else if (emailConfig.provider === 'resend') { | ||
| return { | ||
| type: 'resend', | ||
|
|
@@ -820,7 +822,7 @@ function EditEmailServerDialog(props: { | |
| name="type" | ||
| control={form.control} | ||
| options={[ | ||
| { label: "Shared (noreply@stackframe.co)", value: 'shared' }, | ||
| { label: `Shared (${defaultValues.type === "shared" ? (defaultValues.senderEmail ?? DEFAULT_SHARED_SENDER_EMAIL) : DEFAULT_SHARED_SENDER_EMAIL})`, value: 'shared' }, | ||
| { label: "Managed (via managed domain setup)", value: 'managed' }, | ||
| { label: "Resend (your own email address)", value: 'resend' }, | ||
| { label: "Custom SMTP server (your own email address)", value: 'standard' }, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STACK_EMAIL_SENDERThe only test covers the happy path where the env var is set. There's no test for when
STACK_EMAIL_SENDERis absent — which is exactly the scenario that currently causes a 500 (see the P1 comment onroute.tsx). Adding a case withvi.unstubAllEnvs()(or simply not stubbing the env var) and asserting either a thrown error or the expected default value would make the intended behaviour explicit and prevent regressions.Prompt To Fix With AI