-
Notifications
You must be signed in to change notification settings - Fork 68
refactor: make kv redis optional #326
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: main
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 |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import { kv } from '@vercel/kv' | ||
| import { KV_KEYS } from '@/configs/keys' | ||
| import { getKvValue, setKvValue } from '@/core/shared/clients/kv' | ||
| import { l, serializeErrorForLog } from '@/core/shared/clients/logger/logger' | ||
|
|
||
| /** | ||
|
|
@@ -102,16 +102,54 @@ export const shouldWarnAboutAlternateEmail = async ( | |
| validationResult: EmailValidationResponse | ||
| ): Promise<boolean> => { | ||
| if (validationResult.sub_status === 'alternate') { | ||
| const warnedAlternateEmail = await kv.get( | ||
| const warnedAlternateEmail = await getKvValue<boolean>( | ||
| KV_KEYS.WARNED_ALTERNATE_EMAIL(validationResult.address) | ||
| ) | ||
|
|
||
| if (!warnedAlternateEmail) { | ||
| await kv.set( | ||
| if (!warnedAlternateEmail.ok) { | ||
| l.warn( | ||
| { | ||
| key: 'validate_email:alternate_email_kv_unavailable', | ||
| error: | ||
| warnedAlternateEmail.reason === 'error' | ||
| ? serializeErrorForLog(warnedAlternateEmail.error) | ||
| : undefined, | ||
| context: { | ||
| email: validationResult.address, | ||
| reason: warnedAlternateEmail.reason, | ||
| }, | ||
|
Comment on lines
+117
to
+120
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. 🔒 Agentic Security Review The new KV-unavailable warning path logs raw user emails via Impact: Email addresses can be exposed through centralized log pipelines and wider operator access beyond the primary auth data path. |
||
| }, | ||
| 'Skipping alternate email warning because KV is unavailable' | ||
| ) | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| if (!warnedAlternateEmail.value) { | ||
| const setResult = await setKvValue( | ||
| KV_KEYS.WARNED_ALTERNATE_EMAIL(validationResult.address), | ||
| true | ||
| ) | ||
|
|
||
| if (!setResult.ok) { | ||
| l.warn( | ||
| { | ||
| key: 'validate_email:alternate_email_kv_set_unavailable', | ||
| error: | ||
| setResult.reason === 'error' | ||
| ? serializeErrorForLog(setResult.error) | ||
| : undefined, | ||
| context: { | ||
| email: validationResult.address, | ||
| reason: setResult.reason, | ||
| }, | ||
| }, | ||
| 'Skipping alternate email warning because KV could not persist state' | ||
| ) | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| return true | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,83 @@ | ||
| export { kv } from '@vercel/kv' | ||
| import { kv } from '@vercel/kv' | ||
|
|
||
| export type OptionalKvResult<T> = | ||
| | { ok: true; configured: true; value: T } | ||
| | { | ||
| ok: false | ||
| configured: false | ||
| reason: 'not_configured' | 'misconfigured' | ||
| } | ||
| | { ok: false; configured: true; reason: 'error'; error: unknown } | ||
|
|
||
| export type KvCapabilityStatus = | ||
| | { configured: false; available: false; status: 'not_configured' } | ||
| | { configured: false; available: false; status: 'misconfigured' } | ||
| | { configured: true; available: true; status: 'ok' } | ||
| | { configured: true; available: false; status: 'error'; error: unknown } | ||
|
|
||
| function getKvConfigStatus() { | ||
| const hasUrl = Boolean(process.env.KV_REST_API_URL) | ||
| const hasToken = Boolean(process.env.KV_REST_API_TOKEN) | ||
|
|
||
| if (!(hasUrl || hasToken)) { | ||
| return 'not_configured' | ||
| } | ||
|
|
||
| if (hasUrl && hasToken) { | ||
| return 'configured' | ||
| } | ||
|
|
||
| return 'misconfigured' | ||
| } | ||
|
|
||
| export function isKvConfigured() { | ||
| return getKvConfigStatus() === 'configured' | ||
| } | ||
|
|
||
| export async function pingKv(): Promise<KvCapabilityStatus> { | ||
| const configStatus = getKvConfigStatus() | ||
|
|
||
| if (configStatus !== 'configured') { | ||
| return { configured: false, available: false, status: configStatus } | ||
| } | ||
|
|
||
| try { | ||
| await kv.ping() | ||
| return { configured: true, available: true, status: 'ok' } | ||
| } catch (error) { | ||
| return { configured: true, available: false, status: 'error', error } | ||
| } | ||
| } | ||
|
|
||
| export async function getKvValue<T>( | ||
| key: string | ||
| ): Promise<OptionalKvResult<T | null>> { | ||
| const configStatus = getKvConfigStatus() | ||
|
|
||
| if (configStatus !== 'configured') { | ||
| return { ok: false, configured: false, reason: configStatus } | ||
| } | ||
|
|
||
| try { | ||
| return { ok: true, configured: true, value: await kv.get<T>(key) } | ||
| } catch (error) { | ||
| return { ok: false, configured: true, reason: 'error', error } | ||
| } | ||
| } | ||
|
|
||
| export async function setKvValue( | ||
| key: string, | ||
| value: unknown | ||
| ): Promise<OptionalKvResult<unknown>> { | ||
| const configStatus = getKvConfigStatus() | ||
|
|
||
| if (configStatus !== 'configured') { | ||
| return { ok: false, configured: false, reason: configStatus } | ||
| } | ||
|
|
||
| try { | ||
| return { ok: true, configured: true, value: await kv.set(key, value) } | ||
| } catch (error) { | ||
| return { ok: false, configured: true, reason: 'error', error } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,8 @@ import { z } from 'zod' | |
|
|
||
| export const serverSchema = z.object({ | ||
| SUPABASE_SERVICE_ROLE_KEY: z.string().min(1), | ||
| KV_REST_API_TOKEN: z.string().min(1), | ||
| KV_REST_API_URL: z.url(), | ||
| KV_REST_API_TOKEN: z.string().min(1).optional(), | ||
|
Comment on lines
4
to
+5
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.
When a deployment or local Useful? React with 👍 / 👎. |
||
| KV_REST_API_URL: z.url().optional(), | ||
|
|
||
| ENABLE_USER_BOOTSTRAP: z.string().optional(), | ||
| DASHBOARD_API_ADMIN_TOKEN: z.string().min(1).optional(), | ||
|
|
||
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.
🟡 The new KV
.refineblock setspath: ['KV_REST_API_URL']only, but validates a two-variable invariant. When an operator setsKV_REST_API_URLbut forgetsKV_REST_API_TOKEN(or vice versa), Zod'sprettifyErrorwill attribute the failure toKV_REST_API_URL— potentially the var that was actually set correctly. The two other refines in this same file (billing, captcha) list both fields in the path; fix ispath: ['KV_REST_API_URL', 'KV_REST_API_TOKEN'].Extended reasoning...
What the bug is
In
scripts/check-app-env.ts:9-15, the new refine validates the together-or-neither invariant for KV configuration:The check inspects two environment variables, but the issue
pathreferences only one. When the predicate fails, the path Zod surfaces points atKV_REST_API_URLregardless of which side of the pair was actually omitted.Why this is inconsistent
The two other paired-variable refines added to this same file follow the opposite convention — they list both fields in
path:path: ['BILLING_API_URL', 'NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY']path: ['NEXT_PUBLIC_TURNSTILE_SITE_KEY', 'TURNSTILE_SECRET_KEY']So the new KV refine deviates from a convention already established in the same file.
Concrete proof / step-by-step
KV_REST_API_URL=https://kv.example.comin.env.localbut forgetsKV_REST_API_TOKEN.bun run check:env(or whatever invokes this script).Boolean(data.KV_REST_API_URL) === Boolean(data.KV_REST_API_TOKEN)→true === false→false. Refine emits an issue withpath: ['KV_REST_API_URL'].validateEnvcallsz.prettifyError(parsed.error)(seesrc/lib/env.ts:73), which formats issues using theirpathas the field label.KV_REST_API_URL— the variable they did set correctly — even though the missing one isKV_REST_API_TOKEN.The full message text still names both vars, so a careful reader can recover. But the path label is misleading in CI/build logs, and exactly inverts what's useful when half the pair is set.
Addressing the refutation
One verifier objected that listing two strings in
pathis itself an imprecise use of Zod's API (sincepathsemantically represents a nested object path, not a list of offending fields), so the new code isn't strictly worse — just different. That's a fair critique of the broader pattern, but the practicalprettifyErroroutput for a top-level schema renderspathjoined as a label, so listing both fields does produce a more useful pointer to the operator than listing just one. More importantly, the value here is consistency within the file: the rest of the file uses the two-element form, so a future reader scanning the refines will expect the same shape. The fix is one line and aligns with the established pattern.Severity & fix
Nit. The script still fails the build with a message naming both vars, so no operator is truly stuck. Fix: