Skip to content

feat(coinbase): validate and protect CDP configuration - #202

Open
georgyia wants to merge 2 commits into
feat/coinbase-foundationfrom
feat/coinbase-config
Open

georgyia wants to merge 2 commits into
feat/coinbase-foundationfrom
feat/coinbase-config

Conversation

@georgyia

Copy link
Copy Markdown
Collaborator

Summary

Closes #189.

This PR adds the fail-closed configuration boundary required before AgentWallet can initialize Coinbase CDP or submit x402 payments:

  • validates all CDP credentials, Base Sepolia selection, executor naming, and x402 safety limits with typed Zod schemas
  • keeps Stripe-only startup independent of Coinbase credentials while rejecting placeholder credentials when crypto is enabled
  • redacts Coinbase credential-shaped fields from structured logs
  • documents least-privilege portal setup, customer prerequisites, rotation, and the explicit mainnet gate

Stack

This PR is stacked on #201 and intentionally targets feat/coinbase-foundation so the review contains only issue #189. After #201 merges, this PR should be retargeted to main.

Security properties

  • backend credentials never use a VITE_ prefix
  • only VITE_CDP_PROJECT_ID is declared as browser-public
  • CDP_NETWORK=base and every non-base-sepolia value fail startup
  • malformed, missing, placeholder, and out-of-bounds values fail without echoing secrets
  • crypto remains disabled by default and no wallet or transaction runtime is introduced here
  • private-key Export scope is explicitly prohibited in the setup guide

Verification

  • npm ci
  • npm run build
  • npm run lint -- --quiet
  • npm run format:check
  • npm run test:unit -- --runInBand (40 suites, 441 tests)
  • focused config/logger tests (2 suites, 30 tests)
  • npm run test:integration (4 suites passed; 36 passed, 96 intentionally skipped)
  • Prisma migration deploy against the local test database (no pending migrations)
  • crypto-disabled API startup and GET /health smoke test (HTTP 200)
  • secret scan confirmed only checked-in placeholders and documentation references

This repository currently has no frontend build target. The server schema deliberately omits VITE_CDP_PROJECT_ID, and no backend CDP credential has a public prefix.

Reviewer focus

  1. Conditional environment typing and disabled-mode behavior
  2. Mainnet fail-closed enforcement and x402 bounds
  3. Logger redaction coverage
  4. Credential rotation and least-privilege guidance

@georgyia georgyia added this to the v.0.1 (first launch) milestone Aug 19, 2026
@georgyia
georgyia requested a review from JonasBaeumer August 19, 2026 05:25
@georgyia georgyia added coinbase Coinbase CDP, AgentKit, wallets, Spend Permissions, and x402 integration security developer-experience labels Aug 19, 2026
@coderabbitai

coderabbitai Bot commented Aug 19, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a16bdf21-aa45-459b-add6-e48748b8c08a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

georgyia added a commit that referenced this pull request Aug 22, 2026
Removing the row in 4d75dd2 was wrong. #202 declares VITE_CDP_PROJECT_ID in
.env.example and docs/coinbase-setup.md as the single CDP value permitted in a
client build, and pairs it with the rule that no backend credential may carry a
VITE_ prefix. The table row is what makes that distinction visible in the
credential inventory, so it belongs here.

Keep the clarification that no frontend exists in the repository yet, which is
the one piece of context the row was missing.
@georgyia

Copy link
Copy Markdown
Collaborator Author

P1 — blank values in .env now crash startup where they previously fell back to a default.

z.string().default(...) only fires on undefined, but dotenv sets a declared-but-empty key to "". Verified against loadEnv on this branch:

PORT absent               -> PORT=3000  STRIPE="sk_test_placeholder"
PORT="" (blank in .env)   -> THROWS: PORT: PORT must be an integer between 1 and 65535
PORT=" 3000 "             -> THROWS: PORT: PORT must be an integer between 1 and 65535
STRIPE_SECRET_KEY=""      -> PORT=3000  STRIPE=""

Old behaviour was parseInt(process.env.PORT || '3000', 10) and process.env.STRIPE_SECRET_KEY || 'sk_test_placeholder' — both treated "" as absent. So PORT= on its own line, which is the shape .env.example already uses for TELEGRAM_TEST_CHAT_ID= and TELEGRAM_TEST_CHANNEL_ID=, is now a hard startup failure, and a blank STRIPE_SECRET_KEY silently reaches the Stripe client as "" instead of the placeholder, so the failure moves to the first API call and gets less legible.

Neither change is mentioned in the description and neither is needed for the CDP work. Is the intent that blank means unset (.transform(v => v === '' ? undefined : v) before the default), or that blank is now an error? Whichever you pick, PORT and STRIPE_SECRET_KEY should agree — right now one throws and the other passes an empty string through.

P1 — the redaction wildcard depth is capped at 3 and nothing pins the boundary.

COINBASE_REDACTION_PATHS expands each field to field, *.field, *.*.field, *.*.*.field. fast-redact's * matches exactly one level, so nesting deeper than three keys is not redacted. Probed with this PR's own path list:

depth 0  {CDP_API_KEY_SECRET}              redacted
depth 1  {a:{...}}                         redacted
depth 2  {a:{b:{...}}}                     redacted
depth 3  {a:{b:{c:{...}}}}                 redacted
depth 4  {a:{b:{c:{d:{...}}}}}             LEAKED
array    {list:[{...}]}                    redacted

The two tests cover depths 0, 1 and 2, so the cliff at 4 is invisible — and this is the item the description asks reviewers to focus on ("Logger redaction coverage"). Depth 4 is reachable in practice: a CDP or Axios error logged as logger.error({ err }) nests config/headers several levels down, and a child logger's bindings add another.

fast-redact has no recursive wildcard, so raising the cap to 5 or 6 only moves the cliff. Two options that actually hold: add a serializers entry that walks the object and censors by key name regardless of depth, or keep the cap and make it explicit — a named MAX_REDACTION_DEPTH constant, a comment stating that deeper nesting is not covered, and a test asserting the boundary so the next person changing it sees the tradeoff.

P1 — required_error is Zod 3-only syntax, and #183 is open to move this repo to Zod 4.

Three uses in env.ts (nonEmptyString, integerString, X402_MAX_PAYMENT_ATOMIC_UNITS). Zod 4 removes required_error and invalid_type_error in favour of the unified error parameter, so each of these becomes a silent no-op — the custom messages revert to Zod defaults and the tests asserting on them fail. Repo is on zod@3.25.76 today so this is correct as written; it just adds three more call sites to #183's migration surface. Worth a line in #183 noting src/config/env.ts so it isn't discovered mid-migration.

Minor — CDP_NETWORK reimplements z.literal.

z.string().default('base-sepolia')
  .refine((v) => v === 'base-sepolia', 'CDP_NETWORK must be base-sepolia')
  .transform(() => 'base-sepolia' as const)

z.literal('base-sepolia').default('base-sepolia') gives the same runtime check and the same narrowed type. The transform is doing nothing the refine has not already guaranteed.

Nothing else. The disabled-mode discriminated union is the right shape — CRYPTO_PAYMENTS_ENABLED as a literal discriminant means callers cannot reach a CDP credential without narrowing first, which is stronger than optional fields would have been. Fail-closed on CDP_NETWORK, the BigInt handling of X402_MAX_PAYMENT_ATOMIC_UNITS with a uint256 bound, and the placeholder rejection all check out, and no validation error echoes a value.

@JonasBaeumer

Copy link
Copy Markdown
Owner

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ccf1e07ba0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/config/logger.ts Outdated
Comment thread docs/coinbase-setup.md
Comment thread src/config/env.ts
@georgyia
georgyia force-pushed the feat/coinbase-config branch from ccf1e07 to 7e27f94 Compare August 29, 2026 22:13
@georgyia

Copy link
Copy Markdown
Collaborator Author

Pushed 7e27f94 (rebased onto the updated feat/coinbase-foundation).

P1 — blank values crash startup. Fixed, and it was reachable today: the .env on my machine has three blank-valued keys. normalizeSource now maps empty and whitespace-only values to undefined before parsing, so blank means unset for every key and PORT and STRIPE_SECRET_KEY give the same answer to the same input again. integerString also trims, so PORT=" 3000 " parses as parseInt did. Seven it.each cases pin the defaults, plus one that a blank required value still fails.

P1 — redaction depth (also @chatgpt-codex-connector). fast-redact has no recursive wildcard, so raising the cap only moves the cliff — replaced the path list with a walk that censors by key name at any depth. It handles errors (cloning through property descriptors so message and stack survive for Pino's serializer), arrays, cycles, and root bindings, and returns the original object untouched when nothing matched so the common path allocates nothing. MAX_REDACTION_DEPTH is a named constant with a test asserting the boundary. Cases at depths 0–11 replace the old 0/1/2 coverage.

One honest limitation, documented in the code and pinned by a test asserting both halves: Pino bakes child() bindings into a string at creation and never routes them through a formatter, so those are outside the walk. I tried overriding child to close it and reverted — app.ts hands this instance to Fastify as loggerInstance, Fastify builds its per-request logger through child(), and shadowing it dropped Fastify's own req/res serializers, which silently turned the req.headers redact paths into no-ops and dumped raw request objects into the log (61 unrelated tests failed). redactCredentials is exported as the caller-side escape hatch; every child() call in this repo binds a static { module } string.

P1 — required_error is Zod 3 syntax. Left as-is since we're on zod 3, with a comment at the call sites naming #183. Worth adding src/config/env.ts to that issue's migration surface.

Minor — CDP_NETWORK. Now z.literal('base-sepolia').default(...); the refine/transform pair is gone.

@chatgpt-codex-connector — TELEGRAM_MOCK strictness. Deliberate, now documented and tested. Blank keeps meaning unset (the normalization above), so existing deployments are unaffected. Non-canonical values still fail closed: under the old === 'true' test a typo like TELEGRAM_MOCK=1 read as "not mocked" and sent real Telegram traffic from a run meant to be mocked, which is the worse failure. Four resolution cases and four rejection cases added.

@chatgpt-codex-connector — rotation runbooks. The conflict was real and the ADR was wrong; fixed on #201 and cross-referenced here. The setup doc now leads with the split: the API key supports an overlap window, the Wallet Secret does not.

Unit suite is 475 green (was 442).

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

Three problems in the configuration boundary:

Blank values crashed startup where they previously fell back to a default.
dotenv turns a bare KEY= line into '', and z.string().default() only fires on
undefined, so PORT= became a hard startup failure while a blank
STRIPE_SECRET_KEY silently reached the Stripe client as '' instead of the
placeholder -- two different answers to the same input. Both previously used
process.env.X || fallback. normalizeSource now treats blank as absent for every
key, so the two agree again. The local .env in this repo already has three
blank-valued keys, so this was reachable today.

Credential redaction stopped at three levels. COINBASE_REDACTION_PATHS expanded
each field to field, *.field, *.*.field, *.*.*.field, and fast-redact's * matches
exactly one level, so a secret nested deeper leaked -- reachable through a CDP or
Axios error logged as { err }, whose config and headers nest several levels down.
fast-redact has no recursive wildcard, so raising the cap only moves the cliff.
Redaction is now a walk that censors by key name at any depth, bounded only by a
named MAX_REDACTION_DEPTH, and covers errors (preserving message and stack),
arrays, cycles, and child logger bindings.

TELEGRAM_MOCK now fails closed on non-canonical values. Blank keeps meaning
unset, so existing deployments are unaffected, but a typo such as
TELEGRAM_MOCK=1 no longer reads as 'not mocked' and sends real Telegram traffic.

Also replaces the CDP_NETWORK refine/transform pair with z.literal, records that
required_error is Zod 3 syntax that #183 will have to migrate, and reconciles the
wallet-secret rotation runbook with ADR 001: the API key supports an overlap
window, the wallet secret does not.
georgyia added a commit that referenced this pull request Aug 29, 2026
The Coinbase work lands as a stack: #202, #203, and #204 are all based on
feat/coinbase-foundation rather than on main. The pull_request trigger filtered
on branches: [main], so none of those three ran lint, type check, unit tests,
integration tests, or CodeQL -- the only green checks on them were Dependabot and
CodeRabbit, and the gap is invisible because a PR with no matching workflow looks
the same as one with nothing to run.

Dropping the filter costs nothing: a pull_request event still only fires for an
open PR, and same-repo branches keep access to the Stripe secret the integration
job needs.
@georgyia
georgyia force-pushed the feat/coinbase-config branch from 7e27f94 to e4a1124 Compare August 29, 2026 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

coinbase Coinbase CDP, AgentKit, wallets, Spend Permissions, and x402 integration developer-experience security

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants