Feat/db config onboarding - #63
Merged
Merged
Conversation
…CM with HMAC_SECRET
|
Warning Review limit reached
More reviews will be available in 57 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughEncrypted Dodo credentials are added to metadata, with AES-256-GCM helpers, schema columns, selective upsert/decrypt, async metadata-backed Dodo client initialization with caching/clear, onboarding persistence updates, a GET /api/v1/internals/config endpoint, and small wiring/await fixes in checkout/webhook flows. ChangesDodo Configuration Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/gRPC/payment/paymentProvider.ts (1)
72-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid sending an empty
return_urlto Dodo whenredirect_urlis unset.
getPaymentProviderConfig()setsreturnUrlto""whenmetadata?.redirect_urlis missing, andcreateProviderCheckout()always passesreturn_url: config.returnUrl. Dodo’s docs definereturn_urlas a redirect destination (nullablestring | null) and do not document""as a supported value—empty strings can trigger validation/unexpected checkout behavior. Prefer omittingreturn_urlfrom the payload (or sendingnull) when no redirect URL is configured.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/gRPC/payment/paymentProvider.ts` around lines 72 - 83, getPaymentProviderConfig currently sets returnUrl to an empty string which then gets sent to Dodo; change getPaymentProviderConfig to return returnUrl as null (or undefined) instead of "" when metadata.redirect_url is unset, update the PaymentProviderConfig type if needed to allow returnUrl?: string | null, and modify createProviderCheckout to omit return_url from the outgoing Dodo payload (or explicitly send null) when config.returnUrl is null/undefined so an empty string is never sent.
🧹 Nitpick comments (3)
src/storage/db/postgres/helpers/metadata.ts (1)
26-30: ⚡ Quick winDon't silently swallow decryption failures.
The empty
catchtreats every failure the same way. Migration-era plaintext is the intended case, but a genuinely encrypted value that fails to decrypt (secret rotation, corruption, tampering) is also silently passed through — the raw ciphertext then flows downstream as if it were the credential, with no signal. At minimum, log the failure so these cases are observable.Suggested change
try { (result as Record<string, unknown>)[field] = decrypt(value); - } catch { - // leave as-is (e.g. plaintext from migration) + } catch (e) { + // Tolerate plaintext (e.g. from migration) but surface genuine failures. + logger.lifecycleWarning(/* decryption failed for field */); }As per coding guidelines: "Use the
WideEventLoggerfromerrors/logger".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/storage/db/postgres/helpers/metadata.ts` around lines 26 - 30, The catch block swallowing decryption errors should instead log the failure using the WideEventLogger from errors/logger so genuine decryption problems are observable; in the catch for the decrypt(value) call (referencing decrypt, result, field, value) import and use WideEventLogger to emit an error/wide-event that includes the field name and the caught error details (and any safe contextual identifiers), then continue to leave the value as-is for migration/plaintext cases as currently done.src/utils/encryptMetadata.ts (1)
11-17: ⚡ Quick winDerive the AES key via HKDF with domain separation from
HMAC_SECRET
src/utils/encryptMetadata.tsderives the AES-256-GCM key asSHA-256(HMAC_SECRET), butHMAC_SECRETis also used for HMAC hashing insrc/utils/hashAPIKey.ts. Use HKDF with a distinctinfolabel (e.g., for metadata encryption) to derive independent keys for each cryptographic purpose.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/encryptMetadata.ts` around lines 11 - 17, The current assertSecret function derives the AES key by hashing HMAC_SECRET; instead derive a distinct 32-byte AES-256-GCM key from process.env.HMAC_SECRET using HKDF with a clear domain-separating info label (e.g., "metadata-encryption") so it is independent from the HMAC key in hashAPIKey.ts; update the assertSecret function to validate HMAC_SECRET, then call Node's crypto.hkdfSync (or hkdf) with a zero or appropriate salt, the secret as IKM, the info string for metadata encryption, and output length 32, and return that derived key Buffer (replace the createHash usage and keep the same function name assertSecret).src/zod/internals.ts (1)
59-59: ⚡ Quick winZod 4: use top-level
z.url()forredirectUrl
In Zod 4, the chained validatorz.string().url()is deprecated in favor of the standalonez.url()schema.♻️ Proposed change
- redirectUrl: z.string().url("Redirect URL must be a valid URL").optional(), + redirectUrl: z.url("Redirect URL must be a valid URL").optional(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/zod/internals.ts` at line 59, The redirectUrl schema uses the deprecated chained form z.string().url(...); replace that with the Zod 4 top-level z.url() keeping the .optional() and the same validation message (i.e., change the expression from redirectUrl: z.string().url("Redirect URL must be a valid URL").optional() to using z.url(...).optional()), so update the redirectUrl definition to use z.url() while preserving the existing error text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/routes/http/api/onboarding.ts`:
- Around line 86-90: The masking function maskApiKey currently reveals 8
characters for any key longer than 8, which exposes too much for short secrets;
update maskApiKey so that it returns a fully masked value (e.g., "****") for
keys of length <= 16 to avoid revealing most of short secrets, and keep the
existing slice behavior only for keys longer than 16 (i.e., if key.length > 16
return key.slice(0,4) + "****" + key.slice(-4) else return "****").
- Around line 92-137: handleGetConfig is exposing internals without auth;
protect it by invoking the existing HTTP API key guard and wiring the route the
same way other internals endpoints do: call authenticateHttpApiKey at the start
of handleGetConfig (or add it as a preHandler hook when registering the route)
so requests are validated before returning config, and ensure errors from failed
authentication short-circuit with the same response pattern used by other
routes; reference authenticateHttpApiKey and handleGetConfig to locate where to
add the check and mirror the auth pattern used by other internals routes in your
route registration logic.
In `@src/storage/db/postgres/helpers/metadata.ts`:
- Around line 105-119: upsertMetadata is vulnerable to race conditions because
it does a select-then-insert on metadataTable (existingMetadata / insertValues)
without a uniqueness constraint or locking; make the singleton write atomic by
using a deterministic singleton key and a database upsert or an advisory lock:
either (A) enforce a single-row invariant (add a fixed/deterministic id for the
singleton row and a unique constraint) and replace the select-then-insert with a
single INSERT ... ON CONFLICT(id) DO UPDATE using metadataTable and insertValues
inside txn, or (B) acquire a Postgres advisory lock (pg_advisory_xact_lock) at
the start of upsertMetadata before reading existingMetadata and inserting to
ensure only one concurrent creator proceeds; update the upsertMetadata
implementation (and any schema migration to add the unique/partial index if you
choose option A) and keep getMetadata calls unchanged.
In `@src/storage/db/postgres/schema.ts`:
- Line 258: The redirect_url column currently defaults to
"http://localhost:3000", which allows production redirects to land on localhost;
remove that unsafe hardcoded default and instead supply a safe default from
configuration or require the caller to provide it: update the schema's
redirect_url definition to not default to localhost (either make it nullable or
default to a value read from an environment/config var), and modify
upsertMetadata (the function that checks input.redirect_url !== undefined) to
set input.redirect_url to a configured DEFAULT_REDIRECT_URL when undefined; also
ensure the onboarding flow that passes validated.redirectUrl either always
validates/provides a value or falls back to the same configured default before
calling upsertMetadata.
In `@src/utils/encryptMetadata.ts`:
- Line 14: Replace plain Error throws in assertSecret and decrypt with a
project-style custom domain error (e.g., MetadataEncryptionError) that includes
an error type, message, and optional originalError; implement static factory
methods like MetadataEncryptionError.missingSecret(message) and
.decryptionFailed(message, originalError) and use those in assertSecret and
decrypt so the thrown error carries type, descriptive message and the original
underlying exception where applicable.
---
Outside diff comments:
In `@src/routes/gRPC/payment/paymentProvider.ts`:
- Around line 72-83: getPaymentProviderConfig currently sets returnUrl to an
empty string which then gets sent to Dodo; change getPaymentProviderConfig to
return returnUrl as null (or undefined) instead of "" when metadata.redirect_url
is unset, update the PaymentProviderConfig type if needed to allow returnUrl?:
string | null, and modify createProviderCheckout to omit return_url from the
outgoing Dodo payload (or explicitly send null) when config.returnUrl is
null/undefined so an empty string is never sent.
---
Nitpick comments:
In `@src/storage/db/postgres/helpers/metadata.ts`:
- Around line 26-30: The catch block swallowing decryption errors should instead
log the failure using the WideEventLogger from errors/logger so genuine
decryption problems are observable; in the catch for the decrypt(value) call
(referencing decrypt, result, field, value) import and use WideEventLogger to
emit an error/wide-event that includes the field name and the caught error
details (and any safe contextual identifiers), then continue to leave the value
as-is for migration/plaintext cases as currently done.
In `@src/utils/encryptMetadata.ts`:
- Around line 11-17: The current assertSecret function derives the AES key by
hashing HMAC_SECRET; instead derive a distinct 32-byte AES-256-GCM key from
process.env.HMAC_SECRET using HKDF with a clear domain-separating info label
(e.g., "metadata-encryption") so it is independent from the HMAC key in
hashAPIKey.ts; update the assertSecret function to validate HMAC_SECRET, then
call Node's crypto.hkdfSync (or hkdf) with a zero or appropriate salt, the
secret as IKM, the info string for metadata encryption, and output length 32,
and return that derived key Buffer (replace the createHash usage and keep the
same function name assertSecret).
In `@src/zod/internals.ts`:
- Line 59: The redirectUrl schema uses the deprecated chained form
z.string().url(...); replace that with the Zod 4 top-level z.url() keeping the
.optional() and the same validation message (i.e., change the expression from
redirectUrl: z.string().url("Redirect URL must be a valid URL").optional() to
using z.url(...).optional()), so update the redirectUrl definition to use
z.url() while preserving the existing error text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ded03d0-c6b5-49da-8572-a3e4e8a7159d
📒 Files selected for processing (10)
.env.examplesrc/routes/gRPC/payment/createCheckoutLink.tssrc/routes/gRPC/payment/paymentProvider.tssrc/routes/http/api/onboarding.tssrc/routes/http/api/registerApiRoutes.tssrc/routes/http/createdCheckout.tssrc/storage/db/postgres/helpers/metadata.tssrc/storage/db/postgres/schema.tssrc/utils/encryptMetadata.tssrc/zod/internals.ts
…vation, custom MetadataEncryptionError, optional return_url in Dodo call, z.url()
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor