feat(cve/researcher): vault-resolvable secrets for NVD API key and webhook URL #192

Merged

stack72 merged 3 commits from worktree-witty-sniffing-heron into main 2026-08-13 02:16:40 +00:00

Conversation 6 Commits 3 Files changed 4 +80 -15

stack72 commented 2026-08-13 00:59:20 +00:00

Owner

Copy link

Summary

• Add nvdApiKey and webhookUrl as optional, sensitive: true global args on the CVE researcher and Discord webhook extensions
• Both prefer vault-backed values via ${{ vault.get() }} expressions, falling back to NVD_API_KEY / DISCORD_WEBHOOK_URL env vars for backwards compatibility
• Bump model version to 2026.08.13.1 with upgrade stanza, bump manifest version to match
• Update README with secrets documentation, vault setup instructions, and clarify that vault/key names are user-chosen

Test plan

• deno check and deno lint pass (verified locally)
• Existing tests still pass (deno test extensions/models/)
• Verify swamp extension source add accepts the new manifest
• Test with vault-wired global args on a remote worker
• Test env var fallback still works when no vault is configured

🤖 Generated with Claude Code

## Summary - Add `nvdApiKey` and `webhookUrl` as optional, `sensitive: true` global args on the CVE researcher and Discord webhook extensions - Both prefer vault-backed values via `${{ vault.get() }}` expressions, falling back to `NVD_API_KEY` / `DISCORD_WEBHOOK_URL` env vars for backwards compatibility - Bump model version to `2026.08.13.1` with upgrade stanza, bump manifest version to match - Update README with secrets documentation, vault setup instructions, and clarify that vault/key names are user-chosen ## Test plan - [ ] `deno check` and `deno lint` pass (verified locally) - [ ] Existing tests still pass (`deno test extensions/models/`) - [ ] Verify `swamp extension source add` accepts the new manifest - [ ] Test with vault-wired global args on a remote worker - [ ] Test env var fallback still works when no vault is configured 🤖 Generated with [Claude Code](https://claude.com/claude-code)

stack72 added 1 commit 2026-08-13 00:59:22 +00:00

feat(cve/researcher): vault-resolvable secrets for NVD API key and webhook URL ... 93f6a75be3

Add nvdApiKey and webhookUrl as sensitive global args that can be wired
via vault.get() expressions, with env var fallback for backwards compat.
Supports remote worker execution where env vars alone aren't sufficient.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-08-13 01:17:24 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. discord_webhook_env.ts: WebhookGlobalArgs schema defined but not exported — sensitive: true may have no runtime effect.

   The PR adds a WebhookGlobalArgs Zod schema (line 12–21) with .meta({ sensitive: true }) on webhookUrl, but this schema is only used locally for TypeScript type inference (z.infer<typeof WebhookGlobalArgs>). The extension export on line 23 has no globalArguments property, so the framework never sees the sensitive annotation.

   Compare with cve_research.ts where GlobalArgsSchema is properly wired into model.globalArguments (line 717) — the framework can see nvdApiKey's sensitive flag there.

   Breaking scenario: If the framework relies on the extension's declared schema (not just the vault expression syntax) to decide what to redact from logs and reports, the resolved webhookUrl value could appear in plain text in framework-level logs even though the author intended it to be redacted.

   Suggested fix: Either export the schema as globalArguments: WebhookGlobalArgs on the extension object, or confirm that vault-expression-based redaction makes this moot and drop the misleading .meta({ sensitive: true }) to avoid a false sense of coverage.

Low

1. discord_webhook_env.ts:41: || → ?? changes empty-string fallback behavior.

   The original || would treat an empty string webhookUrl as falsy and fall through to DISCORD_WEBHOOK_URL env var. The new ?? only falls through on null/undefined, so an empty string from a misconfigured vault would skip the env var fallback and hit the if (!webhookUrl) error instead of silently recovering.

   This is arguably better behavior (surfacing misconfiguration instead of hiding it), but it is a semantic change worth being aware of. The subsequent if (!webhookUrl) guard on line 43 ensures no empty-string URL reaches the fetch call, so no security or data-integrity risk.

Verdict

PASS — The core change (vault-resolvable nvdApiKey with env var fallback, version bump, upgrade entry) is clean and correctly threaded through queryNvd. The sensitive annotation gap on the discord webhook extension is worth addressing but does not block merge since the webhook URL was never marked sensitive before this PR either, making this a net improvement regardless.

## Adversarial Review ### Medium 1. **`discord_webhook_env.ts`: `WebhookGlobalArgs` schema defined but not exported — `sensitive: true` may have no runtime effect.** The PR adds a `WebhookGlobalArgs` Zod schema (line 12–21) with `.meta({ sensitive: true })` on `webhookUrl`, but this schema is only used locally for TypeScript type inference (`z.infer<typeof WebhookGlobalArgs>`). The `extension` export on line 23 has no `globalArguments` property, so the framework never sees the sensitive annotation. Compare with `cve_research.ts` where `GlobalArgsSchema` is properly wired into `model.globalArguments` (line 717) — the framework *can* see `nvdApiKey`'s sensitive flag there. **Breaking scenario:** If the framework relies on the extension's declared schema (not just the vault expression syntax) to decide what to redact from logs and reports, the resolved `webhookUrl` value could appear in plain text in framework-level logs even though the author intended it to be redacted. **Suggested fix:** Either export the schema as `globalArguments: WebhookGlobalArgs` on the `extension` object, or confirm that vault-expression-based redaction makes this moot and drop the misleading `.meta({ sensitive: true })` to avoid a false sense of coverage. ### Low 1. **`discord_webhook_env.ts:41`: `||` → `??` changes empty-string fallback behavior.** The original `||` would treat an empty string `webhookUrl` as falsy and fall through to `DISCORD_WEBHOOK_URL` env var. The new `??` only falls through on `null`/`undefined`, so an empty string from a misconfigured vault would skip the env var fallback and hit the `if (!webhookUrl)` error instead of silently recovering. This is arguably *better* behavior (surfacing misconfiguration instead of hiding it), but it is a semantic change worth being aware of. The subsequent `if (!webhookUrl)` guard on line 43 ensures no empty-string URL reaches the `fetch` call, so no security or data-integrity risk. ### Verdict **PASS** — The core change (vault-resolvable `nvdApiKey` with env var fallback, version bump, upgrade entry) is clean and correctly threaded through `queryNvd`. The `sensitive` annotation gap on the discord webhook extension is worth addressing but does not block merge since the webhook URL was never marked sensitive before this PR either, making this a net improvement regardless.

stack72 commented 2026-08-13 01:18:15 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

1. discord_webhook_env.ts: extension object is missing globalArguments field (line 23)

   WebhookGlobalArgs schema is defined and used to type ExtensionContext.globalArgs, but never attached to the exported extension object. Compare: cve_research.ts has globalArguments: GlobalArgsSchema in the model export -- that is how the swamp runtime discovers vault-resolvable global args.

   Without globalArguments on the extension object, the runtime cannot populate context.globalArgs.webhookUrl from a vault.get() expression. The field will always be undefined, silently falling through to Deno.env.get("DISCORD_WEBHOOK_URL") -- exactly the old behaviour. The vault path documented in the README will appear to work but the vault-supplied value is never used.

   Fix: add globalArguments: WebhookGlobalArgs to the exported extension object.

Suggestions

1. No tests for vault-resolution path -- no test covers nvdApiKey being read from globalArgs vs the env var fallback. Root CLAUDE.md test rules focus on vault/ and datastore/ so not a hard block, but a mock-HTTP-server test verifying the header is sent correctly would prevent regressions.

## Code Review ### Blocking Issues 1. discord_webhook_env.ts: extension object is missing globalArguments field (line 23) WebhookGlobalArgs schema is defined and used to type ExtensionContext.globalArgs, but never attached to the exported extension object. Compare: cve_research.ts has globalArguments: GlobalArgsSchema in the model export -- that is how the swamp runtime discovers vault-resolvable global args. Without globalArguments on the extension object, the runtime cannot populate context.globalArgs.webhookUrl from a vault.get() expression. The field will always be undefined, silently falling through to Deno.env.get("DISCORD_WEBHOOK_URL") -- exactly the old behaviour. The vault path documented in the README will appear to work but the vault-supplied value is never used. Fix: add globalArguments: WebhookGlobalArgs to the exported extension object. ### Suggestions 1. No tests for vault-resolution path -- no test covers nvdApiKey being read from globalArgs vs the env var fallback. Root CLAUDE.md test rules focus on vault/ and datastore/ so not a hard block, but a mock-HTTP-server test verifying the header is sent correctly would prevent regressions.

stack72 added 1 commit 2026-08-13 01:23:39 +00:00

fix(cve/researcher): wire WebhookGlobalArgs into extension export ... 5ac07f0977

Without globalArguments on the extension object, the runtime can't
populate context.globalArgs.webhookUrl from vault expressions and the
sensitive metadata has no effect on redaction.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-08-13 01:30:32 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. discord_webhook_env.ts: WebhookGlobalArgs is unexported. The schema is used via z.infer<typeof WebhookGlobalArgs> inside the same file, which works, but exporting it would let test files or other in-package modules reference the type directly. Minor, but consistent with how GlobalArgsSchema is exposed in cve_research.ts.

2. No test coverage for the nvdApiKey vault path. The new parameter flows through queryNvd and into the apiKey header, but there are no test file changes in this PR. The stated testing rule specifically calls out vault/ and datastore/ extensions; cve/ is not in scope of that rule, so this doesn't block. Worth adding a test that mocks the NVD endpoint and asserts the apiKey header is set when globalArgs.nvdApiKey is supplied.

3. discord_webhook_env.ts: Discord error body is not truncated. errorText from response.text() is thrown verbatim. A malformed or unexpectedly large Discord error response could produce a very large error string. Consider errorText.slice(0, 500) for safety.

Summary

The vault-backed secrets wiring is implemented correctly. Both nvdApiKey and webhookUrl are declared with z.string().meta({ sensitive: true }).optional(), pass through to the right call sites, and fall back to env vars via ?? (correctly preferring ?? over || to avoid treating an empty-string env var as absent). The version bump is consistent between cve_research.ts and manifest.yaml, and the upgrade entry's pass-through upgradeAttributes is appropriate since the new field is optional. No any types, no default exports, no hardcoded secrets.

## Code Review ### Blocking Issues None. ### Suggestions 1. **`discord_webhook_env.ts`: `WebhookGlobalArgs` is unexported.** The schema is used via `z.infer<typeof WebhookGlobalArgs>` inside the same file, which works, but exporting it would let test files or other in-package modules reference the type directly. Minor, but consistent with how `GlobalArgsSchema` is exposed in `cve_research.ts`. 2. **No test coverage for the `nvdApiKey` vault path.** The new parameter flows through `queryNvd` and into the `apiKey` header, but there are no test file changes in this PR. The stated testing rule specifically calls out `vault/` and `datastore/` extensions; `cve/` is not in scope of that rule, so this doesn't block. Worth adding a test that mocks the NVD endpoint and asserts the `apiKey` header is set when `globalArgs.nvdApiKey` is supplied. 3. **`discord_webhook_env.ts`: Discord error body is not truncated.** `errorText` from `response.text()` is thrown verbatim. A malformed or unexpectedly large Discord error response could produce a very large error string. Consider `errorText.slice(0, 500)` for safety. ### Summary The vault-backed secrets wiring is implemented correctly. Both `nvdApiKey` and `webhookUrl` are declared with `z.string().meta({ sensitive: true }).optional()`, pass through to the right call sites, and fall back to env vars via `??` (correctly preferring `??` over `||` to avoid treating an empty-string env var as absent). The version bump is consistent between `cve_research.ts` and `manifest.yaml`, and the upgrade entry's pass-through `upgradeAttributes` is appropriate since the new field is optional. No `any` types, no default exports, no hardcoded secrets.

stack72 commented 2026-08-13 01:30:39 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. discord_webhook_env.ts:42 — || → ?? changes empty-string fallback behavior

   The change from || to ?? alters how an empty-string webhookUrl is handled. With the old ||, an empty string from a vault value would be treated as falsy and fall through to Deno.env.get("DISCORD_WEBHOOK_URL"). With ??, an empty string is not nullish and would be used directly as the URL, causing fetch to fail with an invalid-URL error instead of falling through to the env var.

   Breaking example: A user's vault has a webhookUrl key set to "" (misconfigured or placeholder). Previously this silently fell through to the env var. Now it produces a runtime fetch error.

   Suggested fix: This is arguably the correct behavior (empty string = explicit bad config should error, not silently fall through). If the intent is to preserve the forgiving behavior, add .pipe(z.string().min(1)) to the Zod schema or keep ||. As-is, the Zod schema accepts any string including empty, so the inconsistency exists. Not blocking because an empty-string vault value is an unlikely misconfiguration, and the error would be clear.

Low

1. discord_webhook_env.ts:59 — No timeout on Discord webhook fetch

   The Discord webhook fetch call has no AbortSignal.timeout(), unlike the fetchJson helper in cve_research.ts which uses AbortSignal.timeout(30_000). If the Discord API hangs, this call blocks indefinitely. Pre-existing issue — not introduced by this PR — but worth noting since the author is actively working in this file.

2. cve_research.ts:281 — Same ?? vs || pattern for nvdApiKey

   const apiKey = nvdApiKey ?? Deno.env.get("NVD_API_KEY") — same empty-string behavior as the webhook case above. An empty-string vault value for nvdApiKey would be sent as the apiKey header, which NVD would reject. Lower risk than the webhook case since NVD rejects bad keys with a clear HTTP error, and the retry logic in fetchJson handles it.

Verdict

PASS — This is a clean, well-scoped change. It adds vault-resolvable secrets with proper sensitive metadata, optional fields with env-var fallbacks, a correct no-op upgrade path, consistent version bumps across manifest and model export, and updated documentation. The || → ?? semantic shift is the only behavioral change and is low-risk in practice.

## Adversarial Review ### Medium 1. **`discord_webhook_env.ts:42` — `||` → `??` changes empty-string fallback behavior** The change from `||` to `??` alters how an empty-string `webhookUrl` is handled. With the old `||`, an empty string from a vault value would be treated as falsy and fall through to `Deno.env.get("DISCORD_WEBHOOK_URL")`. With `??`, an empty string is not nullish and would be used directly as the URL, causing `fetch` to fail with an invalid-URL error instead of falling through to the env var. **Breaking example:** A user's vault has a `webhookUrl` key set to `""` (misconfigured or placeholder). Previously this silently fell through to the env var. Now it produces a runtime fetch error. **Suggested fix:** This is arguably the correct behavior (empty string = explicit bad config should error, not silently fall through). If the intent is to preserve the forgiving behavior, add `.pipe(z.string().min(1))` to the Zod schema or keep `||`. As-is, the Zod schema accepts any string including empty, so the inconsistency exists. Not blocking because an empty-string vault value is an unlikely misconfiguration, and the error would be clear. ### Low 1. **`discord_webhook_env.ts:59` — No timeout on Discord webhook `fetch`** The Discord webhook `fetch` call has no `AbortSignal.timeout()`, unlike the `fetchJson` helper in `cve_research.ts` which uses `AbortSignal.timeout(30_000)`. If the Discord API hangs, this call blocks indefinitely. Pre-existing issue — not introduced by this PR — but worth noting since the author is actively working in this file. 2. **`cve_research.ts:281` — Same `??` vs `||` pattern for `nvdApiKey`** `const apiKey = nvdApiKey ?? Deno.env.get("NVD_API_KEY")` — same empty-string behavior as the webhook case above. An empty-string vault value for `nvdApiKey` would be sent as the `apiKey` header, which NVD would reject. Lower risk than the webhook case since NVD rejects bad keys with a clear HTTP error, and the retry logic in `fetchJson` handles it. ### Verdict **PASS** — This is a clean, well-scoped change. It adds vault-resolvable secrets with proper `sensitive` metadata, optional fields with env-var fallbacks, a correct no-op upgrade path, consistent version bumps across manifest and model export, and updated documentation. The `||` → `??` semantic shift is the only behavioral change and is low-risk in practice.

stack72 added 1 commit 2026-08-13 01:49:24 +00:00

fix(cve/researcher): add min(1) to sensitive schema fields, timeout on Discord fetch ... 3fdccc4255

Add .min(1) to nvdApiKey and webhookUrl schemas so empty strings are
rejected at validation time rather than silently producing bad requests.
Add 30s AbortSignal timeout to the Discord webhook fetch to match the
fetchJson helper used elsewhere.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-08-13 02:14:05 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. discord_webhook_env.ts — error body may echo sensitive data (very low risk). When the Discord webhook returns a non-2xx response, the full response body is included in the thrown error:

   const errorText = await response.text();
   throw new Error(`Discord webhook error (${response.status}): ${errorText}`);
   

   Discord error payloads don't normally contain the webhook URL, but if this error propagates to a log, any unexpected content from Discord could appear there. Consider truncating errorText to a safe length (e.g., .slice(0, 200)) to bound log output and prevent any future surprise from Discord changing its error format.

2. No test coverage for the new nvdApiKey forwarding path. The testing rules specifically call out vault/ and datastore/ extensions, so this isn't a hard requirement for cve/. That said, the key forwarding in queryNvd (the nvdApiKey ?? Deno.env.get("NVD_API_KEY") line) has no corresponding test that verifies the argument-supplied key is actually set in the apiKey header. A small unit test with a local HTTP server capturing request headers would lock in this behaviour cheaply.

3. discord_webhook_env.ts methods is an array of objects rather than a plain object. methods: [{ sendFromEnv: { ... } }] differs from the model convention used in cve_research.ts (methods: { research: { ... } }). This is pre-existing and not introduced by this PR, so out of scope to fix here — but worth tracking if the extension schema has a canonical shape.

Notes

• The ?? replacement for || in the webhook URL resolution is correct: .min(1) on the schema ensures an empty string can't arrive via globalArgs.webhookUrl, and ?? is semantically tighter (only falls back on null/undefined).
• AbortSignal.timeout(30_000) added to the Discord fetch matches the timeout already present in cve_research.ts — good consistency.
• Version bump, upgrade entry, and migration callback (upgradeAttributes: (old) => old) are all correct for an additive optional field.
• No any types, all exports are named, and the npm:zod@4.3.6 import is pinned to an exact version.

## Code Review ### Blocking Issues None. ### Suggestions 1. **`discord_webhook_env.ts` — error body may echo sensitive data (very low risk).** When the Discord webhook returns a non-2xx response, the full response body is included in the thrown error: ```typescript const errorText = await response.text(); throw new Error(`Discord webhook error (${response.status}): ${errorText}`); ``` Discord error payloads don't normally contain the webhook URL, but if this error propagates to a log, any unexpected content from Discord could appear there. Consider truncating `errorText` to a safe length (e.g., `.slice(0, 200)`) to bound log output and prevent any future surprise from Discord changing its error format. 2. **No test coverage for the new `nvdApiKey` forwarding path.** The testing rules specifically call out `vault/` and `datastore/` extensions, so this isn't a hard requirement for `cve/`. That said, the key forwarding in `queryNvd` (the `nvdApiKey ?? Deno.env.get("NVD_API_KEY")` line) has no corresponding test that verifies the argument-supplied key is actually set in the `apiKey` header. A small unit test with a local HTTP server capturing request headers would lock in this behaviour cheaply. 3. **`discord_webhook_env.ts` `methods` is an array of objects rather than a plain object.** `methods: [{ sendFromEnv: { ... } }]` differs from the model convention used in `cve_research.ts` (`methods: { research: { ... } }`). This is pre-existing and not introduced by this PR, so out of scope to fix here — but worth tracking if the extension schema has a canonical shape. ### Notes - The `??` replacement for `||` in the webhook URL resolution is correct: `.min(1)` on the schema ensures an empty string can't arrive via `globalArgs.webhookUrl`, and `??` is semantically tighter (only falls back on `null`/`undefined`). - `AbortSignal.timeout(30_000)` added to the Discord fetch matches the timeout already present in `cve_research.ts` — good consistency. - Version bump, upgrade entry, and migration callback (`upgradeAttributes: (old) => old`) are all correct for an additive optional field. - No `any` types, all exports are named, and the `npm:zod@4.3.6` import is pinned to an exact version.

stack72 commented 2026-08-13 02:14:15 +00:00

Author

Owner

Copy link

Adversarial Review

I traced all code paths introduced by this PR: vault-resolvable nvdApiKey global arg in cve_research.ts, WebhookGlobalArgs schema and export in discord_webhook_env.ts, the || → ?? fix, the 30-second timeout on the Discord fetch, the upgrade entry, version bump, and documentation changes.

Critical / High

None found.

Medium

None found.

Low

1. discord_webhook_env.ts:66-78 — Response body not consumed on success path. When response.ok is true, the code proceeds to writeResource without consuming or canceling the response body. In Deno, this is technically a resource leak (the connection isn't returned to the pool until GC or the abort signal fires at 30s). This is pre-existing behavior — the PR only added the timeout, which actually bounds the leak duration. Not blocking, just noting for a future cleanup: add await response.body?.cancel() after the response.ok check on the success path.

Verdict

PASS — Clean, well-scoped change. The nvdApiKey global arg is correctly optional with .min(1) to reject empty strings, sensitive: true for log redaction, and env-var fallback. The || to ?? fix in discord_webhook_env.ts is correct (the .min(1) schema constraint guarantees a provided value is non-empty, so the operators are equivalent, but ?? is semantically precise). The AbortSignal.timeout(30_000) on the Discord fetch is good hardening. The upgrade entry is a no-op passthrough, which is correct for an additive optional field. No security, logic, or data integrity issues found.

## Adversarial Review I traced all code paths introduced by this PR: vault-resolvable `nvdApiKey` global arg in `cve_research.ts`, `WebhookGlobalArgs` schema and export in `discord_webhook_env.ts`, the `||` → `??` fix, the 30-second timeout on the Discord fetch, the upgrade entry, version bump, and documentation changes. ### Critical / High None found. ### Medium None found. ### Low 1. **`discord_webhook_env.ts:66-78` — Response body not consumed on success path.** When `response.ok` is true, the code proceeds to `writeResource` without consuming or canceling the response body. In Deno, this is technically a resource leak (the connection isn't returned to the pool until GC or the abort signal fires at 30s). This is pre-existing behavior — the PR only added the timeout, which actually *bounds* the leak duration. Not blocking, just noting for a future cleanup: add `await response.body?.cancel()` after the `response.ok` check on the success path. ### Verdict **PASS** — Clean, well-scoped change. The `nvdApiKey` global arg is correctly optional with `.min(1)` to reject empty strings, `sensitive: true` for log redaction, and env-var fallback. The `||` to `??` fix in `discord_webhook_env.ts` is correct (the `.min(1)` schema constraint guarantees a provided value is non-empty, so the operators are equivalent, but `??` is semantically precise). The `AbortSignal.timeout(30_000)` on the Discord fetch is good hardening. The upgrade entry is a no-op passthrough, which is correct for an additive optional field. No security, logic, or data integrity issues found.

stack72 merged commit 8cd17e0b59 into main 2026-08-13 02:16:39 +00:00

stack72 referenced this pull request from a commit 2026-08-13 02:16:42 +00:00

feat(cve/researcher): vault-resolvable secrets for NVD API key and webhook URL (#192)

stack72 deleted branch worktree-witty-sniffing-heron 2026-08-13 02:17:00 +00:00

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

swamp-club/swamp-extensions!192

No description provided.