From 629dcec4190a07789c1fdb31fc99f2ed994874a6 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 15 Jul 2026 19:34:27 +0000 Subject: [PATCH 01/10] feat(webhooks): add timestamped HMAC verification --- docs/spec/configuration.md | 26 +++- evals/README.md | 2 +- evals/fixtures/config/netclaw.json | 3 + evals/run-evals.sh | 9 ++ .../.system/files/netclaw-operations/SKILL.md | 2 +- .../netclaw-operations/references/webhooks.md | 20 ++- .../.openspec.yaml | 2 + .../add-timestamped-webhook-hmac/design.md | 54 +++++++ .../add-timestamped-webhook-hmac/proposal.md | 31 ++++ .../specs/inbound-webhooks/spec.md | 111 ++++++++++++++ .../add-timestamped-webhook-hmac/tasks.md | 29 ++++ openspec/specs/inbound-webhooks/spec.md | 92 ++++++++++- .../Tools/SetWebhookToolProvenanceTests.cs | 46 ++++++ src/Netclaw.Actors/Tools/SetWebhookTool.cs | 29 +++- .../Webhooks/WebhooksCommandTests.cs | 107 +++++++++++++ src/Netclaw.Cli/Webhooks/WebhooksCommand.cs | 123 ++++++++++++--- .../WebhookRouteStoreTests.cs | 88 +++++++++++ .../Schemas/webhook-route.v1.schema.json | 22 ++- .../WebhookRouteValidator.cs | 39 +++++ src/Netclaw.Configuration/WebhooksConfig.cs | 17 ++- ...hookEndpointRouteBuilderExtensionsTests.cs | 72 +++++++++ .../Webhooks/WebhookRequestVerifierTests.cs | 143 +++++++++++++++++- .../Webhooks/WebhookRouteCatalogTests.cs | 20 +++ .../Webhooks/RegisteredWebhookRoute.cs | 19 +++ .../Webhooks/WebhookRequestVerifier.cs | 126 ++++++++++++++- 25 files changed, 1196 insertions(+), 36 deletions(-) create mode 100644 openspec/changes/add-timestamped-webhook-hmac/.openspec.yaml create mode 100644 openspec/changes/add-timestamped-webhook-hmac/design.md create mode 100644 openspec/changes/add-timestamped-webhook-hmac/proposal.md create mode 100644 openspec/changes/add-timestamped-webhook-hmac/specs/inbound-webhooks/spec.md create mode 100644 openspec/changes/add-timestamped-webhook-hmac/tasks.md diff --git a/docs/spec/configuration.md b/docs/spec/configuration.md index 388e974af..6eab591a5 100644 --- a/docs/spec/configuration.md +++ b/docs/spec/configuration.md @@ -377,6 +377,26 @@ Example route file `~/.netclaw/config/webhooks/github-issues.json`: } ``` +Stripe-style providers use an explicit timestamped verifier. It signs the exact +timestamp text, a separator, and the raw request body, and rejects deliveries +outside the replay-tolerance window: + +```json +{ + "Verification": { + "Kind": "HmacTimestamped", + "Secret": "whsec_...", + "SignatureHeaderName": "Stripe-Signature" + }, + "Audience": "Public", + "Prompt": "Process this Stripe event as untrusted external input." +} +``` + +`Hmac`, `HmacTimestamped`, and `HeaderSecret` are distinct sender protocols. +Netclaw does not infer or fall back between them. Existing routes remain on +their configured verifier after upgrade; `Hmac` remains the default. + Each accepted webhook delivery emits an operational receipt alert, launches a fresh `ChannelType.Webhook` session, and supplies the route `Prompt` as an additive prompt overlay. `NotifyInstructions` and `DeliveryRequired` work the same @@ -397,7 +417,7 @@ Route-file fields: | Field | Type | Default | Description | |-------|------|---------|-------------| | `Enabled` | bool | `true` | Enables or disables this specific route. | -| `Verification.Kind` | string | `Hmac` | Verification mode. Current values: `Hmac`, `HeaderSecret`. | +| `Verification.Kind` | string | `Hmac` | Verification mode: `Hmac`, `HmacTimestamped`, or `HeaderSecret`. | | `Verification.HmacAlgorithm` | string | `Sha256` | HMAC hash algorithm. MVP supports `Sha256` only. | | `Verification.Secret` | string? | `null` | Shared secret used for signature/header validation. Route files are secret-bearing config. | | `Verification.SignatureHeaderName` | string? | `null` | Header name containing the HMAC signature. Defaults to `X-Webhook-Signature`. | @@ -405,6 +425,10 @@ Route-file fields: | `Verification.SecretHeaderName` | string? | `null` | Header name for `HeaderSecret` mode. Defaults to `X-Webhook-Secret`. | | `Verification.EventHeaderName` | string? | `null` | Event-name header. Defaults to `X-Webhook-Event`. | | `Verification.DeliveryIdHeaderName` | string? | `null` | Delivery ID header. Defaults to `X-Webhook-Delivery`. | +| `Verification.ToleranceSeconds` | int? | `300` | Maximum past or future clock difference for `HmacTimestamped`, from 1 through 3600 seconds. | +| `Verification.TimestampField` | string? | `t` | Structured-header timestamp field for `HmacTimestamped`. | +| `Verification.SignatureField` | string? | `v1` | Structured-header signature field for `HmacTimestamped`; multiple instances support sender secret rotation. | +| `Verification.SignedPayloadSeparator` | string? | `.` | Separator between the exact timestamp text and raw body for `HmacTimestamped`. | | `Events` | string[] | `[]` | Optional allow-list of event types. Empty means all verified events are accepted. | | `Audience` | string | `Public` | Source audience for the autonomous webhook session (`Public`, `Team`, `Personal`). | | `Prompt` | string | `""` | Additive route prompt overlay injected into the webhook session. | diff --git a/evals/README.md b/evals/README.md index 85bd194ac..5cd5fce02 100644 --- a/evals/README.md +++ b/evals/README.md @@ -71,7 +71,7 @@ log patterns** (skill loading, memory recall, checkpoint formation). | Identity & Self-Awareness | 5 | Bot knows its name, version, repo, session ID, and routes all identity-file concerns without a skill dependency | | Skill Auto-Loading | 4 | Keyword matching triggers correct skills | | Memory Pipeline | 4 | Memory recall is active, identity-vs-memory routing is correct, explicit saves use memory tools, and automatic checkpointing still fires | -| Tool Discovery & Use | 4 | Progressive tool discovery and invocation | +| Tool Discovery & Use | 9 | Progressive tool discovery and invocation, including timestamped webhook configuration | | Grounding & Alignment | 3 | Uses tools to verify facts, admits uncertainty | | Autonomy & Execution | 2 | Executes tasks rather than describing them | | Deployment Mission | 1 | Applies the disk mission playbook, loads its required skill, and returns reviewed sales email | diff --git a/evals/fixtures/config/netclaw.json b/evals/fixtures/config/netclaw.json index 2cc51d363..eb7c2197c 100644 --- a/evals/fixtures/config/netclaw.json +++ b/evals/fixtures/config/netclaw.json @@ -1,5 +1,8 @@ { "configVersion": 1, + "Webhooks": { + "Enabled": true + }, "Tools": { "AudienceProfiles": { "Personal": { diff --git a/evals/run-evals.sh b/evals/run-evals.sh index 8d1f1abd7..e288fb0f7 100755 --- a/evals/run-evals.sh +++ b/evals/run-evals.sh @@ -1162,6 +1162,12 @@ assert_tool_file_list() { stdout_contains '\[tool:call\] file_list' } +assert_tool_timestamped_webhook() { + stdout_tool_called 'set_webhook' \ + && stdout_contains 'HmacTimestamped' \ + && stdout_contains 'Stripe-Signature' +} + assert_tool_timeout_arg_recovery() { # Spelling-tolerant meta keys: a near-miss timeout key (TimeoutSeconds, # timeout_seconds, Timeout) now resolves onto _timeout_seconds and is @@ -1765,6 +1771,9 @@ run_all() { run_case tool_file_list "file_list called" \ "What files are in my session directory?" + run_case tool_timestamped_webhook "set_webhook called with Stripe timestamp verification" \ + "Create a public inbound webhook route named stripe-events for Stripe. Use secret eval-whsec-123 and have it summarize each payment event." + run_case tool_timeout_arg_recovery "long-timeout shell call lands on _timeout_seconds" \ "Run 'echo netclaw-timeout-eval-ok' in the shell with a 5 minute timeout." \ "Use the shell to run: echo netclaw-timeout-eval-ok — give it a 300 second timeout since it might be slow." diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index 71630e930..e2dbdf318 100644 --- a/feeds/skills/.system/files/netclaw-operations/SKILL.md +++ b/feeds/skills/.system/files/netclaw-operations/SKILL.md @@ -3,7 +3,7 @@ name: netclaw-operations description: "REQUIRED when the user asks about scheduling, reminders, cron jobs, timers, background jobs, diagnostics, troubleshooting, MCP tools, daemon health, identity updates, or Netclaw capabilities and self-maintenance." metadata: author: netclaw - version: "2.28.0" + version: "2.29.0" --- # Netclaw Operations diff --git a/feeds/skills/.system/files/netclaw-operations/references/webhooks.md b/feeds/skills/.system/files/netclaw-operations/references/webhooks.md index ad9642d46..038ffb7bf 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/webhooks.md +++ b/feeds/skills/.system/files/netclaw-operations/references/webhooks.md @@ -35,8 +35,24 @@ broad file reads/writes there unless the user explicitly wants raw config work. Verification kinds are generic: -- `Hmac` -- `HeaderSecret` +- `Hmac` — HMAC-SHA256 over the raw body; use for GitHub-style senders. This + remains the default. +- `HmacTimestamped` — HMAC-SHA256 over `{timestamp}.{rawBody}` from a structured + `t=...,v1=...` header; use for Stripe, TextForge, and compatible senders. +- `HeaderSecret` — a static shared secret in one header. + +These are different sender protocols, not old and new security levels. Never +switch an existing route or fall back between modes unless the sender's protocol +also changes. + +For Stripe, call `set_webhook` with `verification_kind: HmacTimestamped`, +`signature_header_name: Stripe-Signature`, and the Stripe endpoint secret. For +TextForge, use `signature_header_name: X-TextForge-Signature`. The timestamped +defaults are `timestamp_field: t`, `signature_field: v1`, +`signed_payload_separator: .`, and `tolerance_seconds: 300`; only override them +when the sender documents a different wire format. Multiple `v1` values are +accepted for sender-side secret rotation. Missing, malformed, stale, or +future-dated signatures fail closed. Route files hot-reload without restarting the daemon. If a route file becomes invalid, Netclaw removes that route immediately and emits an operational alert. diff --git a/openspec/changes/add-timestamped-webhook-hmac/.openspec.yaml b/openspec/changes/add-timestamped-webhook-hmac/.openspec.yaml new file mode 100644 index 000000000..4f63482c1 --- /dev/null +++ b/openspec/changes/add-timestamped-webhook-hmac/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-07-15 diff --git a/openspec/changes/add-timestamped-webhook-hmac/design.md b/openspec/changes/add-timestamped-webhook-hmac/design.md new file mode 100644 index 000000000..f9df11997 --- /dev/null +++ b/openspec/changes/add-timestamped-webhook-hmac/design.md @@ -0,0 +1,54 @@ +## Context + +Webhook routes are secret-bearing JSON files loaded independently at request time. `WebhookRequestVerifier` currently verifies either HMAC-SHA256 over the raw body or a static secret header. The endpoint already retains the raw bytes, and the daemon already registers `TimeProvider`, so timestamped verification can reuse both seams without changing actor messages, persistence, dispatch, filtering, deduplication, or rate limiting. + +Compatibility includes existing files, CLI/tool callers, and downgrade behavior. A new daemon must not reinterpret an old route; an old daemon encountering a route that explicitly selects the new kind must fail that route closed without affecting other routes. + +## Goals / Non-Goals + +**Goals:** + +- Verify Stripe-style `t=...,v1=...` signatures over the exact `timestamp.separator.rawBody` bytes. +- Enforce a bounded replay window using injectable time. +- Support multiple signature values for sender-side secret rotation. +- Keep existing verification behavior, defaults, and route files unchanged. +- Give operators generic CLI/tool controls plus copyable Stripe and TextForge configurations. + +**Non-Goals:** + +- Provider-specific verifier types, key derivation, or automatic provider detection. +- Deprecating body-only HMAC or static header secrets. +- Persisting replay state beyond the existing delivery-ID deduplication behavior. +- Changing webhook sessions, endpoint responses, or ingress ordering. + +## Decisions + +1. **Add an explicit discriminator rather than auto-detection.** `HmacTimestamped` is appended to the enum. `Hmac` remains the default because senders use incompatible protocols. Auto-detection or fallback could accept a request under a weaker mode after the intended mode failed. + +2. **Keep raw configuration optional and resolve effective defaults at runtime.** Nullable `ToleranceSeconds`, `TimestampField`, `SignatureField`, and `SignedPayloadSeparator` fields preserve legacy JSON. Their effective values for the new kind are `300`, `t`, `v1`, and `.`. Null fields are omitted when writing routes. The v1 route schema gains only optional properties and the additive enum value. + +3. **Parse strictly and sign exact received bytes.** The parser accepts comma-separated `key=value` components, requires exactly one timestamp and at least one signature, and rejects malformed or ambiguous input. The numeric timestamp is used for tolerance checks, while its original text is used in the signed payload. Signature comparison decodes 32-byte SHA-256 hex values and uses fixed-time equality. + +4. **Use the existing `TimeProvider` registration.** `WebhookRequestVerifier` requires `TimeProvider` through DI. Tests use `FakeTimeProvider`; production uses `TimeProvider.System` already registered by the daemon. + +5. **Keep configuration generic at the user surface.** The CLI adds `hmac-timestamped` plus advanced optional flags. Providers still specify `SignatureHeaderName` because Stripe and TextForge use different names. Provider presets are deferred until repeated configuration demonstrates a need. + +6. **Preserve inactive fields.** Validation applies timestamp constraints only when `HmacTimestamped` is selected. Switching kinds does not erase dormant settings, and old kinds do not acquire new behavior. + +No actor or persistence boundary changes. Verification still returns the existing in-memory result consumed by the endpoint before any session actor is created. + +## Risks / Trade-offs + +- **Clock skew rejects legitimate events** → use a documented 300-second default, configurable from 1 through 3600 seconds, and expose a distinct internal rejection reason. +- **Structured header ambiguity** → reject duplicate timestamps, missing values, malformed pairs, invalid Unix timestamps, and invalid signature hex. +- **CLI output consumers break on additive fields** → emit timestamp-specific fields only for the new kind and preserve old-kind output shape. +- **Downgrade encounters the new enum** → older daemons fail that route during parsing; the route catalog removes it and emits its existing invalid-route alert. +- **Configuration files accumulate irrelevant fields** → omit nullable timestamp fields from serialization and ignore dormant values for other verifier kinds. + +## Migration Plan + +No migration runs. Existing routes continue to deserialize with null timestamp settings and retain their existing discriminator. Operators opt in by changing or creating a route with `HmacTimestamped`. Rollback requires changing such routes back to a verifier supported by the older daemon before downgrading; otherwise only those routes remain unavailable. + +## Open Questions + +None. diff --git a/openspec/changes/add-timestamped-webhook-hmac/proposal.md b/openspec/changes/add-timestamped-webhook-hmac/proposal.md new file mode 100644 index 000000000..f01531ea5 --- /dev/null +++ b/openspec/changes/add-timestamped-webhook-hmac/proposal.md @@ -0,0 +1,31 @@ +## Why + +PRD-009 permits external services to launch webhook sessions, but the current verifier only accepts body-only HMAC signatures or static secret headers. Providers such as Stripe and TextForge sign `timestamp.rawBody` and require a timestamp tolerance, so Netclaw cannot receive their events without weakening verification outside the daemon. + +## What Changes + +- Add an opt-in, generic timestamped-HMAC verification kind for structured `t=...,v1=...` signature headers. +- Verify the exact timestamp text and raw request bytes with HMAC-SHA256, accept multiple signatures for secret rotation, and reject deliveries outside a configurable replay window. +- Expose the new mode through route JSON, `netclaw webhooks`, and `set_webhook`, with Stripe-style defaults for field names, separator, and tolerance. +- Preserve body-only `Hmac` as the default and retain `HeaderSecret`; existing route files and callers require no migration. +- Update route schema, operator documentation, runtime skill guidance, and behavioral evals. + +In scope for PRD-009 Phase 2 is generic timestamped verification and its configuration surfaces. Provider presets, provider-specific key derivation, a route-authoring TUI, and changes to webhook dispatch, filtering, deduplication, or rate limiting are out of scope. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `inbound-webhooks`: Add explicit timestamped-HMAC verification while preserving existing verification modes and route compatibility. + +## Impact + +- Configuration: additive enum value and optional fields in the v1 webhook-route schema; no route migration. +- Runtime: one new fail-closed verifier branch using the existing raw request body and injected `TimeProvider`. +- Interfaces: additive CLI flags and optional `set_webhook` arguments; existing invocations remain valid. +- Security: timestamped routes require a valid signature and bounded timestamp; no verifier auto-detection or fallback is introduced. +- Operations: malformed or stale timestamped signatures remain ordinary `401` verification failures and use existing structured logs and counters. diff --git a/openspec/changes/add-timestamped-webhook-hmac/specs/inbound-webhooks/spec.md b/openspec/changes/add-timestamped-webhook-hmac/specs/inbound-webhooks/spec.md new file mode 100644 index 000000000..850712391 --- /dev/null +++ b/openspec/changes/add-timestamped-webhook-hmac/specs/inbound-webhooks/spec.md @@ -0,0 +1,111 @@ +## MODIFIED Requirements + +### Requirement: Verification kinds are generic and minimal + +Route verification SHALL be modeled as generic verification kinds rather than +one first-class verifier type per provider. The system SHALL support generic +body-only HMAC verification, timestamped HMAC verification, and shared-header +secret verification. Existing body-only HMAC and shared-header secret routes +SHALL retain their current behavior and defaults when timestamped-HMAC support +is introduced. + +#### Scenario: Generic HMAC verification is configured + +- **GIVEN** a route file configures HMAC verification with header metadata and a + shared secret +- **WHEN** a request arrives with a valid matching signature +- **THEN** the route verification succeeds without requiring a provider-specific + verifier type + +#### Scenario: Timestamped HMAC verification is configured + +- **GIVEN** a route explicitly configures timestamped HMAC verification +- **WHEN** a request arrives with a valid structured signature over the timestamp + and raw request body +- **THEN** the route verification succeeds without requiring a provider-specific + verifier type + +#### Scenario: Shared-header secret verification is configured + +- **GIVEN** a route file configures shared-header secret verification +- **WHEN** a request arrives with the expected secret header value +- **THEN** the route verification succeeds without requiring a provider-specific + verifier type + +#### Scenario: Existing route omits timestamped settings + +- **GIVEN** a route file created before timestamped-HMAC support selects `Hmac` + or `HeaderSecret` +- **WHEN** the upgraded daemon loads and verifies that route +- **THEN** the route uses the same verifier, defaults, and signed bytes as before +- **AND** no migration or automatic verifier selection occurs + +## ADDED Requirements + +### Requirement: Timestamped HMAC verification is replay bounded + +Timestamped HMAC verification SHALL parse one timestamp and one or more +signatures from the configured structured header, compute HMAC-SHA256 over the +exact received timestamp text, configured separator, and raw request body, and +accept the request only when at least one signature matches using constant-time +comparison. The timestamp SHALL be within the configured tolerance of the +daemon's current time; the default tolerance SHALL be 300 seconds. + +#### Scenario: Valid timestamped signature is accepted + +- **GIVEN** a timestamped-HMAC route using the default `t`, `v1`, `.`, and + 300-second settings +- **WHEN** a request contains a matching signature and a timestamp within the + tolerance window +- **THEN** verification succeeds + +#### Scenario: Any rotation signature may match + +- **GIVEN** a structured signature header contains multiple `v1` signatures +- **WHEN** any one signature matches the configured secret and signed payload +- **THEN** verification succeeds + +#### Scenario: Stale or future timestamp is rejected + +- **GIVEN** a request has a cryptographically valid timestamped signature +- **WHEN** its timestamp is more than the configured tolerance before or after + the daemon's current time +- **THEN** verification fails with a timestamp-out-of-tolerance reason +- **AND** no webhook session is dispatched + +#### Scenario: Malformed structured signature is rejected + +- **WHEN** the configured signature header is missing, malformed, contains an + ambiguous timestamp, an invalid Unix timestamp, or no valid signature values +- **THEN** verification fails cleanly +- **AND** the endpoint returns its normal unauthorized response without crashing + +### Requirement: Timestamped verification configuration is additive + +The route schema, CLI, and `set_webhook` tool SHALL expose timestamp field, +signature field, signed-payload separator, and tolerance settings as optional +configuration for timestamped HMAC routes. Body-only HMAC SHALL remain the +default verification kind, and the system SHALL NOT infer or fall back between +verification kinds. + +#### Scenario: Existing route is updated without timestamp options + +- **GIVEN** an existing body-only HMAC or shared-header secret route +- **WHEN** an operator updates an unrelated route property using the CLI +- **THEN** the stored verifier kind and settings remain unchanged +- **AND** timestamped-HMAC properties are not introduced into the route file + +#### Scenario: New timestamped route uses effective defaults + +- **GIVEN** an operator selects timestamped HMAC and supplies a signature header + and secret without advanced timestamp options +- **WHEN** the route is saved and loaded by the daemon +- **THEN** it uses timestamp field `t`, signature field `v1`, separator `.`, and + tolerance 300 seconds + +#### Scenario: Older daemon encounters a timestamped route + +- **GIVEN** a route file explicitly selects the new timestamped-HMAC enum name +- **WHEN** an older daemon that does not recognize that name loads the route +- **THEN** that route fails closed as invalid +- **AND** other webhook routes and the daemon remain available diff --git a/openspec/changes/add-timestamped-webhook-hmac/tasks.md b/openspec/changes/add-timestamped-webhook-hmac/tasks.md new file mode 100644 index 000000000..956151448 --- /dev/null +++ b/openspec/changes/add-timestamped-webhook-hmac/tasks.md @@ -0,0 +1,29 @@ +## 1. Configuration Contract + +- [x] 1.1 Add the timestamped-HMAC enum and optional settings with effective defaults that preserve legacy routes +- [x] 1.2 Extend the v1 webhook-route schema and shared validation for the new kind +- [x] 1.3 Add legacy load, round-trip, schema, and invalid-config coverage + +## 2. Runtime Verification + +- [x] 2.1 Implement strict structured-header parsing, raw-payload HMAC verification, rotation signatures, and replay tolerance using `TimeProvider` +- [x] 2.2 Add verifier and endpoint tests for valid, boundary, malformed, stale, future, and multiple-signature deliveries +- [x] 2.3 Prove existing HMAC and header-secret runtime behavior remains unchanged + +## 3. Operator Surfaces + +- [x] 3.1 Add mode parsing, timestamp flags, mode-specific display, validation, and help to `netclaw webhooks` +- [x] 3.2 Add trailing optional timestamp arguments and validation to `set_webhook` +- [x] 3.3 Add CLI and tool tests for new and legacy invocations + +## 4. Documentation and Guidance + +- [x] 4.1 Update engineering configuration docs and the inbound-webhooks OpenSpec main capability after verification +- [x] 4.2 Update and version the `netclaw-operations` webhook guidance with mode selection and examples +- [x] 4.3 Update behavioral eval cases for the changed tool schema and skill guidance + +## 5. Verification and External Documentation + +- [ ] 5.1 Run targeted tests, full test suite, evals, Slopwatch, header verification, and diff checks +- [x] 5.2 Verify implementation against OpenSpec artifacts and sync the delta spec +- [x] 5.3 File scoped configuration and CLI documentation issues in `netclaw-dev/netclaw-website` diff --git a/openspec/specs/inbound-webhooks/spec.md b/openspec/specs/inbound-webhooks/spec.md index 670eaf284..0c0d71913 100644 --- a/openspec/specs/inbound-webhooks/spec.md +++ b/openspec/specs/inbound-webhooks/spec.md @@ -163,8 +163,11 @@ config. ### Requirement: Verification kinds are generic and minimal Route verification SHALL be modeled as generic verification kinds rather than -one first-class verifier type per provider. MVP SHALL support a minimal set that -includes generic HMAC verification and shared-header secret verification. +one first-class verifier type per provider. The system SHALL support generic +body-only HMAC verification, timestamped HMAC verification, and shared-header +secret verification. Existing body-only HMAC and shared-header secret routes +SHALL retain their current behavior and defaults when timestamped-HMAC support +is introduced. #### Scenario: Generic HMAC verification is configured @@ -174,6 +177,14 @@ includes generic HMAC verification and shared-header secret verification. - **THEN** the route verification succeeds without requiring a provider-specific verifier type +#### Scenario: Timestamped HMAC verification is configured + +- **GIVEN** a route explicitly configures timestamped HMAC verification +- **WHEN** a request arrives with a valid structured signature over the timestamp + and raw request body +- **THEN** the route verification succeeds without requiring a provider-specific + verifier type + #### Scenario: Shared-header secret verification is configured - **GIVEN** a route file configures shared-header secret verification @@ -181,6 +192,82 @@ includes generic HMAC verification and shared-header secret verification. - **THEN** the route verification succeeds without requiring a provider-specific verifier type +#### Scenario: Existing route omits timestamped settings + +- **GIVEN** a route file created before timestamped-HMAC support selects `Hmac` + or `HeaderSecret` +- **WHEN** the upgraded daemon loads and verifies that route +- **THEN** the route uses the same verifier, defaults, and signed bytes as before +- **AND** no migration or automatic verifier selection occurs + +### Requirement: Timestamped HMAC verification is replay bounded + +Timestamped HMAC verification SHALL parse one timestamp and one or more +signatures from the configured structured header, compute HMAC-SHA256 over the +exact received timestamp text, configured separator, and raw request body, and +accept the request only when at least one signature matches using constant-time +comparison. The timestamp SHALL be within the configured tolerance of the +daemon's current time; the default tolerance SHALL be 300 seconds. + +#### Scenario: Valid timestamped signature is accepted + +- **GIVEN** a timestamped-HMAC route using the default `t`, `v1`, `.`, and + 300-second settings +- **WHEN** a request contains a matching signature and a timestamp within the + tolerance window +- **THEN** verification succeeds + +#### Scenario: Any rotation signature may match + +- **GIVEN** a structured signature header contains multiple `v1` signatures +- **WHEN** any one signature matches the configured secret and signed payload +- **THEN** verification succeeds + +#### Scenario: Stale or future timestamp is rejected + +- **GIVEN** a request has a cryptographically valid timestamped signature +- **WHEN** its timestamp is more than the configured tolerance before or after + the daemon's current time +- **THEN** verification fails with a timestamp-out-of-tolerance reason +- **AND** no webhook session is dispatched + +#### Scenario: Malformed structured signature is rejected + +- **WHEN** the configured signature header is missing, malformed, contains an + ambiguous timestamp, an invalid Unix timestamp, or no valid signature values +- **THEN** verification fails cleanly +- **AND** the endpoint returns its normal unauthorized response without crashing + +### Requirement: Timestamped verification configuration is additive + +The route schema, CLI, and `set_webhook` tool SHALL expose timestamp field, +signature field, signed-payload separator, and tolerance settings as optional +configuration for timestamped HMAC routes. Body-only HMAC SHALL remain the +default verification kind, and the system SHALL NOT infer or fall back between +verification kinds. + +#### Scenario: Existing route is updated without timestamp options + +- **GIVEN** an existing body-only HMAC or shared-header secret route +- **WHEN** an operator updates an unrelated route property using the CLI +- **THEN** the stored verifier kind and settings remain unchanged +- **AND** timestamped-HMAC properties are not introduced into the route file + +#### Scenario: New timestamped route uses effective defaults + +- **GIVEN** an operator selects timestamped HMAC and supplies a signature header + and secret without advanced timestamp options +- **WHEN** the route is saved and loaded by the daemon +- **THEN** it uses timestamp field `t`, signature field `v1`, separator `.`, and + tolerance 300 seconds + +#### Scenario: Older daemon encounters a timestamped route + +- **GIVEN** a route file explicitly selects the new timestamped-HMAC enum name +- **WHEN** an older daemon that does not recognize that name loads the route +- **THEN** that route fails closed as invalid +- **AND** other webhook routes and the daemon remain available + ### Requirement: Route files are secret-bearing config Route files MAY store inline verification secrets. The system SHALL treat @@ -369,4 +456,3 @@ order, not an error condition. - **THEN** `Webhooks.Enabled = true` is written to `netclaw.json` - **AND** the UI displays a success-tone status message - **AND** no advisory is shown - diff --git a/src/Netclaw.Actors.Tests/Tools/SetWebhookToolProvenanceTests.cs b/src/Netclaw.Actors.Tests/Tools/SetWebhookToolProvenanceTests.cs index 4d20631e9..dd18fb81b 100644 --- a/src/Netclaw.Actors.Tests/Tools/SetWebhookToolProvenanceTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/SetWebhookToolProvenanceTests.cs @@ -99,4 +99,50 @@ public async Task Notify_instructions_require_notification_target() Assert.Equal("Error: NotificationTarget is required when NotifyInstructions are provided.", result); Assert.False(_store.TryGet("notify-without-target", out _)); } + + [Fact] + public async Task Timestamped_hmac_settings_are_persisted() + { + var tool = new SetWebhookTool(_store); + + var result = await tool.ExecuteAsync(new Dictionary + { + ["RouteName"] = "stripe-events", + ["Prompt"] = "Handle Stripe delivery.", + ["VerificationKind"] = "HmacTimestamped", + ["Secret"] = "whsec_test", + ["SignatureHeaderName"] = "Stripe-Signature", + ["TimestampField"] = "timestamp", + ["SignatureField"] = "signature", + ["SignedPayloadSeparator"] = "::", + ["ToleranceSeconds"] = 120 + }, Context(TrustAudience.Public), TestContext.Current.CancellationToken); + + Assert.DoesNotContain("Error", result); + Assert.True(_store.TryGet("stripe-events", out var saved)); + var verification = saved.Definition!.Verification; + Assert.Equal(WebhookVerifierKind.HmacTimestamped, verification.Kind); + Assert.Equal("timestamp", verification.TimestampField); + Assert.Equal("signature", verification.SignatureField); + Assert.Equal("::", verification.SignedPayloadSeparator); + Assert.Equal(120, verification.ToleranceSeconds); + } + + [Fact] + public async Task Timestamp_settings_are_rejected_for_body_hmac() + { + var tool = new SetWebhookTool(_store); + + var result = await tool.ExecuteAsync(new Dictionary + { + ["RouteName"] = "invalid-route", + ["Prompt"] = "Handle delivery.", + ["VerificationKind"] = "Hmac", + ["Secret"] = "test-secret", + ["TimestampField"] = "t" + }, Context(TrustAudience.Public), TestContext.Current.CancellationToken); + + Assert.Contains("require 'verificationKind' to be 'HmacTimestamped'", result); + Assert.False(_store.TryGet("invalid-route", out _)); + } } diff --git a/src/Netclaw.Actors/Tools/SetWebhookTool.cs b/src/Netclaw.Actors/Tools/SetWebhookTool.cs index e8ae5c4f3..9916956b5 100644 --- a/src/Netclaw.Actors/Tools/SetWebhookTool.cs +++ b/src/Netclaw.Actors/Tools/SetWebhookTool.cs @@ -21,7 +21,7 @@ public record Params( string RouteName, [property: Description("Prompt overlay instructions for this route.")] string Prompt, - [property: Description("Verification kind: 'Hmac' or 'HeaderSecret'.")] + [property: Description("Verification kind: 'Hmac', 'HmacTimestamped', or 'HeaderSecret'.")] string VerificationKind, [property: Description("Shared secret used to verify incoming requests.")] string Secret, @@ -50,7 +50,15 @@ public record Params( [property: Description("Per-route accepted requests per minute.")] int? RateLimitPerMinute = null, [property: Description("Whether this route is enabled. Defaults to true.")] - bool? Enabled = null); + bool? Enabled = null, + [property: Description("Timestamp field name for HmacTimestamped routes. Defaults to 't'.")] + string? TimestampField = null, + [property: Description("Signature field name for HmacTimestamped routes. Defaults to 'v1'.")] + string? SignatureField = null, + [property: Description("Separator between timestamp and raw body for HmacTimestamped routes. Defaults to '.'.")] + string? SignedPayloadSeparator = null, + [property: Description("Accepted timestamp tolerance in seconds for HmacTimestamped routes, from 1 to 3600. Defaults to 300.")] + int? ToleranceSeconds = null); public SetWebhookTool(WebhookRouteStore store) { @@ -67,8 +75,17 @@ protected override Task ExecuteAsync(Params args, ToolInvocationContext if (string.IsNullOrWhiteSpace(args.Secret)) return Task.FromResult("Error: 'secret' is required."); - if (!Enum.TryParse(args.VerificationKind, ignoreCase: true, out var verificationKind)) - return Task.FromResult("Error: 'verificationKind' must be 'Hmac' or 'HeaderSecret'."); + if (!WebhookRouteValidator.TryParseVerifierKind(args.VerificationKind, out var verificationKind)) + return Task.FromResult("Error: 'verificationKind' must be 'Hmac', 'HmacTimestamped', or 'HeaderSecret'."); + + if ((args.TimestampField is not null + || args.SignatureField is not null + || args.SignedPayloadSeparator is not null + || args.ToleranceSeconds is not null) + && verificationKind != WebhookVerifierKind.HmacTimestamped) + { + return Task.FromResult("Error: Timestamp signature settings require 'verificationKind' to be 'HmacTimestamped'."); + } if (!TryResolveAudience(args.Audience, context.Audience, out var audience, out var audienceError)) return Task.FromResult(audienceError!); @@ -92,6 +109,10 @@ protected override Task ExecuteAsync(Params args, ToolInvocationContext SecretHeaderName = string.IsNullOrWhiteSpace(args.SecretHeaderName) ? null : args.SecretHeaderName.Trim(), EventHeaderName = string.IsNullOrWhiteSpace(args.EventHeaderName) ? null : args.EventHeaderName.Trim(), DeliveryIdHeaderName = string.IsNullOrWhiteSpace(args.DeliveryIdHeaderName) ? null : args.DeliveryIdHeaderName.Trim(), + TimestampField = args.TimestampField?.Trim(), + SignatureField = args.SignatureField?.Trim(), + SignedPayloadSeparator = args.SignedPayloadSeparator, + ToleranceSeconds = args.ToleranceSeconds } }; diff --git a/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs b/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs index 8fba901bf..c5eaef8b6 100644 --- a/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs @@ -425,6 +425,113 @@ public async Task Set_MissingSecretEnvVariable_ReturnsOne() Assert.Equal(1, result); } + [Fact] + public async Task Set_TimestampedHmac_Persists_advanced_settings() + { + var result = await WebhooksCommand.RunAsync([ + "webhooks", "set", "stripe-events", + "--prompt", "Process Stripe event", + "--secret", "whsec_test", + "--verification-kind", "hmac-timestamped", + "--signature-header", "Stripe-Signature", + "--timestamp-field", "timestamp", + "--signature-field", "signature", + "--signed-payload-separator", "::", + "--signature-tolerance-seconds", "120" + ], _paths); + + Assert.Equal(0, result); + var route = ReadRoute("stripe-events"); + Assert.Equal(WebhookVerifierKind.HmacTimestamped, route.Verification.Kind); + Assert.Equal("Stripe-Signature", route.Verification.SignatureHeaderName); + Assert.Equal("timestamp", route.Verification.TimestampField); + Assert.Equal("signature", route.Verification.SignatureField); + Assert.Equal("::", route.Verification.SignedPayloadSeparator); + Assert.Equal(120, route.Verification.ToleranceSeconds); + } + + [Fact] + public async Task Set_HeaderSecret_Accepts_documented_hyphenated_spelling() + { + var result = await WebhooksCommand.RunAsync([ + "webhooks", "set", "internal-events", + "--prompt", "Process internal event", + "--secret", "shared-secret", + "--verification-kind", "header-secret", + "--secret-header", "X-Internal-Secret" + ], _paths); + + Assert.Equal(0, result); + Assert.Equal(WebhookVerifierKind.HeaderSecret, ReadRoute("internal-events").Verification.Kind); + } + + [Fact] + public async Task Set_Timestamp_options_with_body_hmac_fails_without_persisting() + { + var result = await WebhooksCommand.RunAsync([ + "webhooks", "set", "invalid-route", + "--prompt", "Process event", + "--secret", "secret", + "--timestamp-field", "t" + ], _paths); + + Assert.Equal(1, result); + Assert.False(File.Exists(Path.Combine(_paths.WebhooksDirectory, "invalid-route.json"))); + } + + [Fact] + public async Task Set_Unrelated_update_preserves_legacy_verifier_without_timestamp_fields() + { + CreateValidRoute("legacy-route"); + + var result = await WebhooksCommand.RunAsync([ + "webhooks", "set", "legacy-route", + "--rate-limit", "12" + ], _paths); + + Assert.Equal(0, result); + var route = ReadRoute("legacy-route"); + Assert.Equal(WebhookVerifierKind.Hmac, route.Verification.Kind); + Assert.Equal(12, route.RateLimitPerMinute); + var json = File.ReadAllText(Path.Combine(_paths.WebhooksDirectory, "legacy-route.json")); + Assert.DoesNotContain("ToleranceSeconds", json, StringComparison.Ordinal); + Assert.DoesNotContain("TimestampField", json, StringComparison.Ordinal); + } + + [Fact] + public async Task Show_Json_adds_timestamp_fields_only_for_timestamped_kind() + { + CreateValidRoute("legacy-route"); + var timestamped = new WebhookRouteConfig + { + Prompt = "Process Stripe event", + Verification = new WebhookVerificationConfig + { + Kind = WebhookVerifierKind.HmacTimestamped, + Secret = new SensitiveString("whsec_test"), + SignatureHeaderName = "Stripe-Signature" + } + }; + new WebhookRouteStore(_paths).Save("stripe-events", timestamped); + + using var legacyOutput = new StringWriter(); + using var timestampedOutput = new StringWriter(); + Assert.Equal(0, await WebhooksCommand.RunAsync( + ["webhooks", "show", "legacy-route", "--json"], _paths, legacyOutput)); + Assert.Equal(0, await WebhooksCommand.RunAsync( + ["webhooks", "show", "stripe-events", "--json"], _paths, timestampedOutput)); + + using var legacy = JsonDocument.Parse(legacyOutput.ToString()); + using var stripe = JsonDocument.Parse(timestampedOutput.ToString()); + var legacyVerification = legacy.RootElement.GetProperty("verification"); + var stripeVerification = stripe.RootElement.GetProperty("verification"); + Assert.False(legacyVerification.TryGetProperty("toleranceSeconds", out _)); + Assert.Equal(300, stripeVerification.GetProperty("toleranceSeconds").GetInt32()); + Assert.Equal("t", stripeVerification.GetProperty("timestampField").GetString()); + Assert.Equal("v1", stripeVerification.GetProperty("signatureField").GetString()); + Assert.Equal(".", stripeVerification.GetProperty("signedPayloadSeparator").GetString()); + } + [Fact] public async Task Delete_ExistingRoute_ReturnsZero() { diff --git a/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs b/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs index 6098d2d5d..a00b37c2f 100644 --- a/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs +++ b/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs @@ -84,7 +84,7 @@ private static int RunList(string[] args, WebhookRouteStore store, NetclawPaths name = r.RouteName, status = r.Status, audience = r.IsValid ? r.Definition!.Audience.ToString().ToLowerInvariant() : "unknown", - verification = r.IsValid ? r.Definition!.Verification.Kind.ToString().ToLowerInvariant() : "unknown", + verification = r.IsValid ? ToCliVerifierKind(r.Definition!.Verification.Kind) : "unknown", deliveryRequired = r.IsValid && r.Definition!.DeliveryRequired }) .ToList(); @@ -95,7 +95,7 @@ private static int RunList(string[] args, WebhookRouteStore store, NetclawPaths const int colName = 24; const int colStatus = 10; const int colAudience = 10; - const int colVerification = 14; + const int colVerification = 18; output.WriteLine( $"{"NAME",-colName} {"STATUS",-colStatus} {"AUDIENCE",-colAudience} {"VERIFICATION",-colVerification} DELIVERY"); @@ -109,7 +109,7 @@ private static int RunList(string[] args, WebhookRouteStore store, NetclawPaths continue; var audience = route.IsValid ? route.Definition!.Audience.ToString().ToLowerInvariant() : "-"; - var verification = route.IsValid ? route.Definition!.Verification.Kind.ToString().ToLowerInvariant() : "-"; + var verification = route.IsValid ? ToCliVerifierKind(route.Definition!.Verification.Kind) : "-"; var delivery = route.IsValid && route.Definition!.DeliveryRequired ? "required" : "optional"; output.WriteLine( @@ -169,17 +169,7 @@ private static int RunShow(string[] args, WebhookRouteStore store, NetclawPaths file = match.FilePath, endpoint = $"/api/webhooks/{routeName}", enabled = route.Enabled, - verification = new - { - kind = route.Verification.Kind.ToString().ToLowerInvariant(), - secret = showSecret ? route.Verification.Secret?.Value : "********", - hmacAlgorithm = route.Verification.HmacAlgorithm.ToString().ToLowerInvariant(), - signatureHeader = route.Verification.SignatureHeaderName, - signaturePrefix = route.Verification.SignaturePrefix, - secretHeader = route.Verification.SecretHeaderName, - eventHeader = route.Verification.EventHeaderName, - deliveryIdHeader = route.Verification.DeliveryIdHeaderName - }, + verification = BuildVerificationOutput(route.Verification, showSecret), audience = route.Audience.ToString().ToLowerInvariant(), events = route.Events, prompt = route.Prompt, @@ -203,13 +193,23 @@ private static int RunShow(string[] args, WebhookRouteStore store, NetclawPaths output.WriteLine($"Endpoint: /api/webhooks/{routeName}"); output.WriteLine(); output.WriteLine("Verification:"); - output.WriteLine($" Kind: {route.Verification.Kind.ToString().ToLowerInvariant()}"); + output.WriteLine($" Kind: {ToCliVerifierKind(route.Verification.Kind)}"); output.WriteLine($" Secret: {(showSecret ? route.Verification.Secret?.Value ?? "(not set)" : "********** (use --show-secret to reveal)")}"); - if (route.Verification.Kind == WebhookVerifierKind.Hmac) + if (route.Verification.Kind is WebhookVerifierKind.Hmac or WebhookVerifierKind.HmacTimestamped) { output.WriteLine($" Algorithm: {route.Verification.HmacAlgorithm.ToString().ToLowerInvariant()}"); output.WriteLine($" Signature Header: {route.Verification.SignatureHeaderName ?? "(default)"}"); - output.WriteLine($" Signature Prefix: {route.Verification.SignaturePrefix ?? "(none)"}"); + if (route.Verification.Kind == WebhookVerifierKind.Hmac) + { + output.WriteLine($" Signature Prefix: {route.Verification.SignaturePrefix ?? "(none)"}"); + } + else + { + output.WriteLine($" Timestamp Field: {route.Verification.TimestampField ?? "t (default)"}"); + output.WriteLine($" Signature Field: {route.Verification.SignatureField ?? "v1 (default)"}"); + output.WriteLine($" Payload Separator: {route.Verification.SignedPayloadSeparator ?? ". (default)"}"); + output.WriteLine($" Tolerance: {route.Verification.ToleranceSeconds?.ToString() ?? "300 (default)"} seconds"); + } } else { @@ -325,9 +325,9 @@ private static int RunSet(string[] args, WebhookRouteStore store, NetclawPaths p if (hasVerificationKind) { - if (!Enum.TryParse(verificationKind, ignoreCase: true, out var kind)) + if (!WebhookRouteValidator.TryParseVerifierKind(verificationKind, out var kind)) { - Console.Error.WriteLine($"[FAIL] Invalid verification kind: '{verificationKind}'. Use 'hmac' or 'header-secret'."); + Console.Error.WriteLine($"[FAIL] Invalid verification kind: '{verificationKind}'. Use 'hmac', 'hmac-timestamped', or 'header-secret'."); return 1; } route.Verification.Kind = kind; @@ -364,6 +364,45 @@ private static int RunSet(string[] args, WebhookRouteStore store, NetclawPaths p if (hasDeliveryHeader) route.Verification.DeliveryIdHeaderName = deliveryHeader; + if (!TryGetFlagValue(args, "--timestamp-field", out var timestampField, out var hasTimestampField)) + return 1; + + if (hasTimestampField) + route.Verification.TimestampField = timestampField; + + if (!TryGetFlagValue(args, "--signature-field", out var signatureField, out var hasSignatureField)) + return 1; + + if (hasSignatureField) + route.Verification.SignatureField = signatureField; + + if (!TryGetFlagValue(args, "--signed-payload-separator", out var payloadSeparator, out var hasPayloadSeparator)) + return 1; + + if (hasPayloadSeparator) + route.Verification.SignedPayloadSeparator = payloadSeparator; + + if (!TryGetFlagValue(args, "--signature-tolerance-seconds", out var tolerance, out var hasTolerance)) + return 1; + + if (hasTolerance) + { + if (!int.TryParse(tolerance, out var toleranceSeconds)) + { + Console.Error.WriteLine($"[FAIL] Invalid signature tolerance: '{tolerance}'. Must be a whole number from 1 to 3600."); + return 1; + } + + route.Verification.ToleranceSeconds = toleranceSeconds; + } + + if ((hasTimestampField || hasSignatureField || hasPayloadSeparator || hasTolerance) + && route.Verification.Kind != WebhookVerifierKind.HmacTimestamped) + { + Console.Error.WriteLine("[FAIL] Timestamp signature options require '--verification-kind hmac-timestamped'."); + return 1; + } + // Parse events if (!TryGetFlagValue(args, "--events", out var events, out var hasEvents)) return 1; @@ -618,6 +657,38 @@ private static bool TryGetFlagValue(string[] args, string flag, out string value return true; } + private static string ToCliVerifierKind(WebhookVerifierKind kind) + => kind == WebhookVerifierKind.HmacTimestamped + ? "hmac-timestamped" + : kind.ToString().ToLowerInvariant(); + + private static Dictionary BuildVerificationOutput( + WebhookVerificationConfig verification, + bool showSecret) + { + var output = new Dictionary + { + ["kind"] = ToCliVerifierKind(verification.Kind), + ["secret"] = showSecret ? verification.Secret?.Value : "********", + ["hmacAlgorithm"] = verification.HmacAlgorithm.ToString().ToLowerInvariant(), + ["signatureHeader"] = verification.SignatureHeaderName, + ["signaturePrefix"] = verification.SignaturePrefix, + ["secretHeader"] = verification.SecretHeaderName, + ["eventHeader"] = verification.EventHeaderName, + ["deliveryIdHeader"] = verification.DeliveryIdHeaderName + }; + + if (verification.Kind == WebhookVerifierKind.HmacTimestamped) + { + output["timestampField"] = verification.TimestampField ?? "t"; + output["signatureField"] = verification.SignatureField ?? "v1"; + output["signedPayloadSeparator"] = verification.SignedPayloadSeparator ?? "."; + output["toleranceSeconds"] = verification.ToleranceSeconds ?? 300; + } + + return output; + } + private static bool TryResolveTextInput( string[] args, string inlineFlag, @@ -785,12 +856,18 @@ private static void WriteSetHelp(TextWriter output) output.WriteLine(" --secret-env Read secret from environment variable"); output.WriteLine(); output.WriteLine("Verification:"); - output.WriteLine(" --verification-kind 'hmac' (default) or 'header-secret'"); + output.WriteLine(" --verification-kind 'hmac' (default), 'hmac-timestamped', or 'header-secret'"); output.WriteLine(" --signature-header HMAC signature header (e.g., X-Hub-Signature-256)"); output.WriteLine(" --signature-prefix HMAC signature prefix (e.g., sha256=)"); output.WriteLine(" --secret-header Header-secret header name"); output.WriteLine(" --event-header Event type header"); output.WriteLine(" --delivery-header Delivery ID header"); + output.WriteLine(" --timestamp-field Timestamped HMAC field (default: t)"); + output.WriteLine(" --signature-field Timestamped HMAC signature field (default: v1)"); + output.WriteLine(" --signed-payload-separator "); + output.WriteLine(" Timestamp/body separator (default: .)"); + output.WriteLine(" --signature-tolerance-seconds "); + output.WriteLine(" Replay tolerance, 1-3600 (default: 300)"); output.WriteLine(); output.WriteLine("Behavior:"); output.WriteLine(" --events Comma-separated event allowlist"); @@ -818,5 +895,11 @@ private static void WriteSetHelp(TextWriter output) output.WriteLine(" --signature-header X-Hub-Signature-256 \\"); output.WriteLine(" --signature-prefix \"sha256=\" \\"); output.WriteLine(" --events issues.opened,issues.closed"); + output.WriteLine(); + output.WriteLine(" netclaw webhooks set stripe-events \\"); + output.WriteLine(" --prompt \"Process this Stripe event\" \\"); + output.WriteLine(" --secret-env STRIPE_WEBHOOK_SECRET \\"); + output.WriteLine(" --verification-kind hmac-timestamped \\"); + output.WriteLine(" --signature-header Stripe-Signature"); } } diff --git a/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs b/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs index 62f115127..0bc1f39fb 100644 --- a/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs +++ b/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs @@ -95,6 +95,94 @@ public void Save_NormalizesTrimAndCase() Assert.True(File.Exists(Path.Combine(_paths.WebhooksDirectory, "github-issues.json"))); } + [Theory] + [InlineData("Hmac")] + [InlineData("HeaderSecret")] + public void Legacy_route_loads_and_round_trips_without_timestamped_properties(string verifierKind) + { + var path = Path.Combine(_paths.WebhooksDirectory, "legacy.json"); + File.WriteAllText(path, $$""" +{ + "Prompt": "process legacy delivery", + "Verification": { + "Kind": "{{verifierKind}}", + "Secret": "legacy-secret", + "SignatureHeaderName": "X-Legacy-Signature", + "SecretHeaderName": "X-Legacy-Secret" + } +} +"""); + var store = new WebhookRouteStore(_paths); + + Assert.True(store.TryGet("legacy", out var loaded)); + var route = Assert.IsType(loaded.Definition); + Assert.Equal(verifierKind, route.Verification.Kind.ToString()); + Assert.Null(route.Verification.ToleranceSeconds); + Assert.Null(route.Verification.TimestampField); + + route.RateLimitPerMinute = 12; + store.Save("legacy", route); + var saved = File.ReadAllText(path); + + Assert.DoesNotContain("ToleranceSeconds", saved, StringComparison.Ordinal); + Assert.DoesNotContain("TimestampField", saved, StringComparison.Ordinal); + Assert.DoesNotContain("SignatureField", saved, StringComparison.Ordinal); + Assert.DoesNotContain("SignedPayloadSeparator", saved, StringComparison.Ordinal); + } + + [Fact] + public void Timestamped_route_without_advanced_fields_uses_effective_defaults() + { + var path = Path.Combine(_paths.WebhooksDirectory, "stripe.json"); + File.WriteAllText(path, """ +{ + "Prompt": "process Stripe event", + "Verification": { + "Kind": "HmacTimestamped", + "Secret": "whsec_test", + "SignatureHeaderName": "Stripe-Signature" + } +} +"""); + var store = new WebhookRouteStore(_paths); + + Assert.True(store.TryGet("stripe", out var loaded)); + var route = Assert.IsType(loaded.Definition); + Assert.Empty(WebhookRouteValidator.Validate("stripe", route)); + Assert.Null(route.Verification.ToleranceSeconds); + Assert.Null(route.Verification.TimestampField); + Assert.Null(route.Verification.SignatureField); + Assert.Null(route.Verification.SignedPayloadSeparator); + } + + [Theory] + [InlineData(0)] + [InlineData(3601)] + public void Timestamped_route_rejects_unsafe_tolerance(int toleranceSeconds) + { + var route = CreateValidRoute(); + route.Verification.Kind = WebhookVerifierKind.HmacTimestamped; + route.Verification.ToleranceSeconds = toleranceSeconds; + + var errors = WebhookRouteValidator.Validate("stripe", route); + + Assert.Contains(errors, error => error.Contains("ToleranceSeconds", StringComparison.Ordinal)); + } + + [Theory] + [InlineData("hmac", WebhookVerifierKind.Hmac)] + [InlineData("header-secret", WebhookVerifierKind.HeaderSecret)] + [InlineData("HeaderSecret", WebhookVerifierKind.HeaderSecret)] + [InlineData("hmac-timestamped", WebhookVerifierKind.HmacTimestamped)] + [InlineData("HmacTimestamped", WebhookVerifierKind.HmacTimestamped)] + public void TryParseVerifierKind_accepts_documented_and_config_spellings( + string value, + WebhookVerifierKind expected) + { + Assert.True(WebhookRouteValidator.TryParseVerifierKind(value, out var actual)); + Assert.Equal(expected, actual); + } + private static WebhookRouteConfig CreateValidRoute() => new() { diff --git a/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json b/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json index 1c0aa9234..981a4e411 100644 --- a/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json +++ b/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json @@ -13,7 +13,7 @@ "properties": { "Kind": { "type": "string", - "enum": ["Hmac", "HeaderSecret"], + "enum": ["Hmac", "HeaderSecret", "HmacTimestamped"], "default": "Hmac" }, "HmacAlgorithm": { @@ -38,6 +38,26 @@ }, "DeliveryIdHeaderName": { "type": ["string", "null"] + }, + "ToleranceSeconds": { + "type": ["integer", "null"], + "minimum": 1, + "maximum": 3600, + "default": 300 + }, + "TimestampField": { + "type": ["string", "null"], + "minLength": 1, + "default": "t" + }, + "SignatureField": { + "type": ["string", "null"], + "minLength": 1, + "default": "v1" + }, + "SignedPayloadSeparator": { + "type": ["string", "null"], + "default": "." } }, "additionalProperties": false, diff --git a/src/Netclaw.Configuration/WebhookRouteValidator.cs b/src/Netclaw.Configuration/WebhookRouteValidator.cs index c0736ed33..d0e539d5f 100644 --- a/src/Netclaw.Configuration/WebhookRouteValidator.cs +++ b/src/Netclaw.Configuration/WebhookRouteValidator.cs @@ -40,6 +40,24 @@ public static IReadOnlyList Validate(string routeName, WebhookRouteConfi if (route.Verification.Secret.IsNullOrEmpty()) errors.Add("Verification secret is required."); + if (route.Verification.Kind == WebhookVerifierKind.HmacTimestamped) + { + if (route.Verification.ToleranceSeconds is < 1 or > 3600) + errors.Add("Verification.ToleranceSeconds must be between 1 and 3600."); + + if (route.Verification.TimestampField is { } timestampField + && string.IsNullOrWhiteSpace(timestampField)) + { + errors.Add("Verification.TimestampField cannot be blank."); + } + + if (route.Verification.SignatureField is { } signatureField + && string.IsNullOrWhiteSpace(signatureField)) + { + errors.Add("Verification.SignatureField cannot be blank."); + } + } + if (route.MaxBodyBytes < 1) errors.Add("MaxBodyBytes must be >= 1."); @@ -82,4 +100,25 @@ public static void ValidateOrThrow(string routeName, WebhookRouteConfig route) => WebhookRouteStore.TryNormalizeRouteName(routeName, out _, out var error) ? null : error; + + public static bool TryParseVerifierKind(string value, out WebhookVerifierKind kind) + { + switch (value.Trim().ToLowerInvariant()) + { + case "hmac": + kind = WebhookVerifierKind.Hmac; + return true; + case "header-secret": + case "headersecret": + kind = WebhookVerifierKind.HeaderSecret; + return true; + case "hmac-timestamped": + case "hmactimestamped": + kind = WebhookVerifierKind.HmacTimestamped; + return true; + default: + kind = default; + return false; + } + } } diff --git a/src/Netclaw.Configuration/WebhooksConfig.cs b/src/Netclaw.Configuration/WebhooksConfig.cs index b6034c365..65d8f0b75 100644 --- a/src/Netclaw.Configuration/WebhooksConfig.cs +++ b/src/Netclaw.Configuration/WebhooksConfig.cs @@ -3,6 +3,8 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using System.Text.Json.Serialization; + namespace Netclaw.Configuration; /// @@ -68,12 +70,25 @@ public sealed class WebhookVerificationConfig public string? EventHeaderName { get; set; } public string? DeliveryIdHeaderName { get; set; } + + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public int? ToleranceSeconds { get; set; } + + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public string? TimestampField { get; set; } + + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public string? SignatureField { get; set; } + + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public string? SignedPayloadSeparator { get; set; } } public enum WebhookVerifierKind { Hmac = 0, - HeaderSecret = 1 + HeaderSecret = 1, + HmacTimestamped = 2 } public enum WebhookHmacAlgorithm diff --git a/src/Netclaw.Daemon.Tests/Webhooks/WebhookEndpointRouteBuilderExtensionsTests.cs b/src/Netclaw.Daemon.Tests/Webhooks/WebhookEndpointRouteBuilderExtensionsTests.cs index 7b9e94e58..c58222831 100644 --- a/src/Netclaw.Daemon.Tests/Webhooks/WebhookEndpointRouteBuilderExtensionsTests.cs +++ b/src/Netclaw.Daemon.Tests/Webhooks/WebhookEndpointRouteBuilderExtensionsTests.cs @@ -91,6 +91,45 @@ public async Task Invalid_signature_returns_401() Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); } + [Fact] + public async Task Timestamped_signature_within_tolerance_dispatches() + { + _store.Save("stripe-events", CreateTimestampedRoute()); + BumpWriteTime("stripe-events"); + await using var app = await CreateHostAsync(); + var body = "{\"type\":\"payment_intent.succeeded\"}"; + var timestamp = DateTimeOffset.Parse("2026-04-02T18:30:00Z").ToUnixTimeSeconds(); + + using var request = BuildTimestampedRequest( + "/api/webhooks/stripe-events", + body, + timestamp, + "secret"); + var response = await app.GetTestClient().SendAsync(request, TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.Accepted, response.StatusCode); + Assert.Single(app.Services.GetRequiredService().Invocations); + } + + [Fact] + public async Task Stale_timestamped_signature_returns_401_without_dispatch() + { + _store.Save("stripe-events", CreateTimestampedRoute()); + BumpWriteTime("stripe-events"); + await using var app = await CreateHostAsync(); + var timestamp = DateTimeOffset.Parse("2026-04-02T18:24:59Z").ToUnixTimeSeconds(); + + using var request = BuildTimestampedRequest( + "/api/webhooks/stripe-events", + "{\"type\":\"payment_intent.succeeded\"}", + timestamp, + "secret"); + var response = await app.GetTestClient().SendAsync(request, TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + Assert.Empty(app.Services.GetRequiredService().Invocations); + } + [Fact] public async Task Oversized_body_returns_413() { @@ -330,6 +369,27 @@ private static HttpRequestMessage BuildGitHubRequest(string path, string body, s return request; } + private static HttpRequestMessage BuildTimestampedRequest( + string path, + string body, + long timestamp, + string secret) + { + var bodyBytes = Encoding.UTF8.GetBytes(body); + var signedPayload = Encoding.UTF8.GetBytes($"{timestamp}.{body}"); + using var hmac = new HMACSHA256(Encoding.UTF8.GetBytes(secret)); + var signature = Convert.ToHexString(hmac.ComputeHash(signedPayload)).ToLowerInvariant(); + + var request = new HttpRequestMessage(HttpMethod.Post, path) + { + Content = new ByteArrayContent(bodyBytes) + }; + request.Content.Headers.ContentType = new("application/json") { CharSet = "utf-8" }; + request.Headers.Add("Stripe-Signature", $"t={timestamp},v1={signature}"); + request.Headers.Add("X-Webhook-Delivery", "evt-123"); + return request; + } + private static WebhookRouteConfig CreateRoute(int maxBodyBytes = 1024 * 1024, int rateLimitPerMinute = 30) => new() { Prompt = "triage this event", @@ -352,6 +412,18 @@ private static HttpRequestMessage BuildGitHubRequest(string path, string body, s RateLimitPerMinute = rateLimitPerMinute }; + private static WebhookRouteConfig CreateTimestampedRoute() => new() + { + Prompt = "process this Stripe event", + Events = [], + Verification = new WebhookVerificationConfig + { + Kind = WebhookVerifierKind.HmacTimestamped, + Secret = new SensitiveString("secret"), + SignatureHeaderName = "Stripe-Signature" + } + }; + private sealed class FakeWebhookExecutionService : IWebhookExecutionService { public List Invocations { get; } = []; diff --git a/src/Netclaw.Daemon.Tests/Webhooks/WebhookRequestVerifierTests.cs b/src/Netclaw.Daemon.Tests/Webhooks/WebhookRequestVerifierTests.cs index 33150dcf0..4fc0212e0 100644 --- a/src/Netclaw.Daemon.Tests/Webhooks/WebhookRequestVerifierTests.cs +++ b/src/Netclaw.Daemon.Tests/Webhooks/WebhookRequestVerifierTests.cs @@ -6,6 +6,7 @@ using System.Security.Cryptography; using System.Text; using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Time.Testing; using Netclaw.Configuration; using Netclaw.Daemon.Webhooks; using Xunit; @@ -14,7 +15,8 @@ namespace Netclaw.Daemon.Tests.Webhooks; public sealed class WebhookRequestVerifierTests { - private readonly WebhookRequestVerifier _sut = new(); + private static readonly DateTimeOffset Now = DateTimeOffset.Parse("2026-04-02T18:30:00Z"); + private readonly WebhookRequestVerifier _sut = new(new FakeTimeProvider(Now)); [Fact] public void Hmac_accepts_valid_signature_and_reads_headers() @@ -120,6 +122,119 @@ public void HeaderSecret_rejects_missing_secret_header() Assert.Equal("missing_secret_header", result.RejectionReason); } + [Fact] + public void TimestampedHmac_accepts_exact_raw_body_and_default_fields() + { + var body = Encoding.UTF8.GetBytes("{\n \"amount\": 1200, \"currency\": \"usd\"\n}"); + var timestamp = Now.ToUnixTimeSeconds().ToString(); + var route = CreateTimestampedRoute(); + var signature = CreateTimestampedSignature("super-secret", timestamp, ".", body); + + var result = _sut.Verify(route, new HeaderDictionary + { + ["Stripe-Signature"] = $"t={timestamp},v1={signature}", + ["X-Webhook-Event"] = "payment.succeeded", + ["X-Webhook-Delivery"] = "evt-123" + }, body); + + Assert.True(result.IsAccepted); + Assert.Equal("payment.succeeded", result.EventType); + Assert.Equal("evt-123", result.DeliveryId); + } + + [Fact] + public void TimestampedHmac_accepts_any_matching_rotation_signature() + { + var body = Encoding.UTF8.GetBytes("{\"event\":\"rotated\"}"); + var timestamp = Now.ToUnixTimeSeconds().ToString(); + var route = CreateTimestampedRoute(); + var signature = CreateTimestampedSignature("super-secret", timestamp, ".", body); + + var result = _sut.Verify(route, new HeaderDictionary + { + ["Stripe-Signature"] = $"t={timestamp},v1={new string('0', 64)},v1={signature.ToUpperInvariant()}" + }, body); + + Assert.True(result.IsAccepted); + } + + [Fact] + public void TimestampedHmac_uses_custom_fields_separator_and_tolerance_boundary() + { + var body = Encoding.UTF8.GetBytes("{\"event\":\"custom\"}"); + var timestamp = Now.AddSeconds(-30).ToUnixTimeSeconds().ToString(); + var route = CreateTimestampedRoute(new WebhookVerificationConfig + { + Kind = WebhookVerifierKind.HmacTimestamped, + Secret = new SensitiveString("super-secret"), + SignatureHeaderName = "X-Custom-Signature", + TimestampField = "time", + SignatureField = "sig", + SignedPayloadSeparator = "::", + ToleranceSeconds = 30 + }); + var signature = CreateTimestampedSignature("super-secret", timestamp, "::", body); + + var result = _sut.Verify(route, new HeaderDictionary + { + ["X-Custom-Signature"] = $"ignored=value,time={timestamp},sig={signature}" + }, body); + + Assert.True(result.IsAccepted); + } + + [Theory] + [InlineData(-301)] + [InlineData(301)] + public void TimestampedHmac_rejects_timestamp_outside_tolerance(int offsetSeconds) + { + var body = Encoding.UTF8.GetBytes("{}"); + var timestamp = Now.AddSeconds(offsetSeconds).ToUnixTimeSeconds().ToString(); + var route = CreateTimestampedRoute(); + var signature = CreateTimestampedSignature("super-secret", timestamp, ".", body); + + var result = _sut.Verify(route, new HeaderDictionary + { + ["Stripe-Signature"] = $"t={timestamp},v1={signature}" + }, body); + + Assert.False(result.IsAccepted); + Assert.Equal("timestamp_out_of_tolerance", result.RejectionReason); + } + + [Theory] + [InlineData("")] + [InlineData("t=123")] + [InlineData("v1=abcd")] + [InlineData("t=not-a-number,v1=abcd")] + [InlineData("t=123,t=123,v1=abcd")] + [InlineData("t=123,v1=")] + [InlineData("t=123,broken,v1=abcd")] + public void TimestampedHmac_rejects_missing_or_malformed_header(string header) + { + var result = _sut.Verify(CreateTimestampedRoute(), new HeaderDictionary + { + ["Stripe-Signature"] = header + }, Encoding.UTF8.GetBytes("{}")); + + Assert.False(result.IsAccepted); + Assert.Contains(result.RejectionReason, new[] { "missing_signature", "invalid_signature_header" }); + } + + [Fact] + public void TimestampedHmac_rejects_invalid_hex_signature_without_throwing() + { + var timestamp = Now.ToUnixTimeSeconds(); + + var result = _sut.Verify(CreateTimestampedRoute(), new HeaderDictionary + { + ["Stripe-Signature"] = $"t={timestamp},v1=not-hex" + }, Encoding.UTF8.GetBytes("{}")); + + Assert.False(result.IsAccepted); + Assert.Equal("invalid_signature", result.RejectionReason); + } + private static RegisteredWebhookRoute CreateRoute(WebhookRouteConfig config) => new( "github-issues", @@ -127,9 +242,35 @@ private static RegisteredWebhookRoute CreateRoute(WebhookRouteConfig config) DateTimeOffset.Parse("2026-04-02T18:30:00Z"), config); + private static RegisteredWebhookRoute CreateTimestampedRoute(WebhookVerificationConfig? verification = null) + => CreateRoute(new WebhookRouteConfig + { + Prompt = "process event", + Verification = verification ?? new WebhookVerificationConfig + { + Kind = WebhookVerifierKind.HmacTimestamped, + Secret = new SensitiveString("super-secret"), + SignatureHeaderName = "Stripe-Signature" + } + }); + private static string CreateGitHubSignature(string secret, byte[] body) { using var hmac = new HMACSHA256(Encoding.UTF8.GetBytes(secret)); return $"sha256={Convert.ToHexString(hmac.ComputeHash(body)).ToLowerInvariant()}"; } + + private static string CreateTimestampedSignature( + string secret, + string timestamp, + string separator, + byte[] body) + { + var prefix = Encoding.UTF8.GetBytes(timestamp + separator); + var payload = new byte[prefix.Length + body.Length]; + Buffer.BlockCopy(prefix, 0, payload, 0, prefix.Length); + Buffer.BlockCopy(body, 0, payload, prefix.Length, body.Length); + using var hmac = new HMACSHA256(Encoding.UTF8.GetBytes(secret)); + return Convert.ToHexString(hmac.ComputeHash(payload)).ToLowerInvariant(); + } } diff --git a/src/Netclaw.Daemon.Tests/Webhooks/WebhookRouteCatalogTests.cs b/src/Netclaw.Daemon.Tests/Webhooks/WebhookRouteCatalogTests.cs index 46e62e3b8..679d3d66f 100644 --- a/src/Netclaw.Daemon.Tests/Webhooks/WebhookRouteCatalogTests.cs +++ b/src/Netclaw.Daemon.Tests/Webhooks/WebhookRouteCatalogTests.cs @@ -61,6 +61,26 @@ public void Invalid_route_file_emits_alert_and_route_is_unavailable() Assert.Equal(AlertType.WebhookRouteInvalid, Assert.Single(_sink.Alerts).Category); } + [Fact] + public void Unknown_future_verifier_invalidates_only_that_route() + { + WriteRouteFile("valid-route", CreateRoute()); + WriteRouteText("future-route", """ +{ + "Prompt": "process future event", + "Verification": { + "Kind": "FutureVerifier", + "Secret": "secret" + } +} +"""); + var sut = CreateCatalog(); + + Assert.False(sut.TryGetRoute("future-route", out _)); + Assert.True(sut.TryGetRoute("valid-route", out _)); + Assert.Contains(_sink.Alerts, alert => alert.Category == AlertType.WebhookRouteInvalid); + } + [Fact] public void Invalid_edit_removes_previously_loaded_route() { diff --git a/src/Netclaw.Daemon/Webhooks/RegisteredWebhookRoute.cs b/src/Netclaw.Daemon/Webhooks/RegisteredWebhookRoute.cs index d16fdd418..86226f43e 100644 --- a/src/Netclaw.Daemon/Webhooks/RegisteredWebhookRoute.cs +++ b/src/Netclaw.Daemon/Webhooks/RegisteredWebhookRoute.cs @@ -21,6 +21,9 @@ public sealed record RegisteredWebhookRoute(string Name, string FilePath, DateTi WebhookVerifierKind.Hmac => string.IsNullOrWhiteSpace(Config.Verification.SignatureHeaderName) ? "X-Webhook-Signature" : Config.Verification.SignatureHeaderName!, + WebhookVerifierKind.HmacTimestamped => string.IsNullOrWhiteSpace(Config.Verification.SignatureHeaderName) + ? "X-Webhook-Signature" + : Config.Verification.SignatureHeaderName!, WebhookVerifierKind.HeaderSecret => string.Empty, _ => throw new ArgumentOutOfRangeException(nameof(Config.Verification.Kind), Config.Verification.Kind, null) }; @@ -30,6 +33,7 @@ public sealed record RegisteredWebhookRoute(string Name, string FilePath, DateTi WebhookVerifierKind.Hmac => string.IsNullOrWhiteSpace(Config.Verification.SignaturePrefix) ? string.Empty : Config.Verification.SignaturePrefix!, + WebhookVerifierKind.HmacTimestamped => string.Empty, WebhookVerifierKind.HeaderSecret => string.Empty, _ => throw new ArgumentOutOfRangeException(nameof(Config.Verification.Kind), Config.Verification.Kind, null) }; @@ -37,6 +41,7 @@ public sealed record RegisteredWebhookRoute(string Name, string FilePath, DateTi public string SecretHeaderName => Config.Verification.Kind switch { WebhookVerifierKind.Hmac => string.Empty, + WebhookVerifierKind.HmacTimestamped => string.Empty, WebhookVerifierKind.HeaderSecret => string.IsNullOrWhiteSpace(Config.Verification.SecretHeaderName) ? "X-Webhook-Secret" : Config.Verification.SecretHeaderName!, @@ -48,6 +53,9 @@ public sealed record RegisteredWebhookRoute(string Name, string FilePath, DateTi WebhookVerifierKind.Hmac => string.IsNullOrWhiteSpace(Config.Verification.EventHeaderName) ? "X-Webhook-Event" : Config.Verification.EventHeaderName!, + WebhookVerifierKind.HmacTimestamped => string.IsNullOrWhiteSpace(Config.Verification.EventHeaderName) + ? "X-Webhook-Event" + : Config.Verification.EventHeaderName!, WebhookVerifierKind.HeaderSecret => string.IsNullOrWhiteSpace(Config.Verification.EventHeaderName) ? "X-Webhook-Event" : Config.Verification.EventHeaderName!, @@ -59,12 +67,23 @@ public sealed record RegisteredWebhookRoute(string Name, string FilePath, DateTi WebhookVerifierKind.Hmac => string.IsNullOrWhiteSpace(Config.Verification.DeliveryIdHeaderName) ? "X-Webhook-Delivery" : Config.Verification.DeliveryIdHeaderName!, + WebhookVerifierKind.HmacTimestamped => string.IsNullOrWhiteSpace(Config.Verification.DeliveryIdHeaderName) + ? "X-Webhook-Delivery" + : Config.Verification.DeliveryIdHeaderName!, WebhookVerifierKind.HeaderSecret => string.IsNullOrWhiteSpace(Config.Verification.DeliveryIdHeaderName) ? "X-Webhook-Delivery" : Config.Verification.DeliveryIdHeaderName!, _ => throw new ArgumentOutOfRangeException(nameof(Config.Verification.Kind), Config.Verification.Kind, null) }; + public int TimestampToleranceSeconds => Config.Verification.ToleranceSeconds ?? 300; + + public string TimestampField => Config.Verification.TimestampField ?? "t"; + + public string TimestampSignatureField => Config.Verification.SignatureField ?? "v1"; + + public string SignedPayloadSeparator => Config.Verification.SignedPayloadSeparator ?? "."; + public bool IsEventAllowed(string? eventType) { if (Config.Events.Count == 0) diff --git a/src/Netclaw.Daemon/Webhooks/WebhookRequestVerifier.cs b/src/Netclaw.Daemon/Webhooks/WebhookRequestVerifier.cs index 7d4fe3d17..91e9cb4fd 100644 --- a/src/Netclaw.Daemon/Webhooks/WebhookRequestVerifier.cs +++ b/src/Netclaw.Daemon/Webhooks/WebhookRequestVerifier.cs @@ -3,6 +3,7 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using System.Globalization; using System.Security.Cryptography; using System.Text; using Microsoft.AspNetCore.Http; @@ -10,8 +11,10 @@ namespace Netclaw.Daemon.Webhooks; -public sealed class WebhookRequestVerifier +public sealed class WebhookRequestVerifier(TimeProvider timeProvider) { + private readonly TimeProvider _timeProvider = timeProvider; + public WebhookVerificationResult Verify( RegisteredWebhookRoute route, IHeaderDictionary headers, @@ -20,11 +23,64 @@ public WebhookVerificationResult Verify( return route.Config.Verification.Kind switch { WebhookVerifierKind.Hmac => VerifyHmac(route, headers, bodyBytes), + WebhookVerifierKind.HmacTimestamped => VerifyTimestampedHmac(route, headers, bodyBytes), WebhookVerifierKind.HeaderSecret => VerifyHeaderSecret(route, headers), _ => throw new ArgumentOutOfRangeException(nameof(route.Config.Verification.Kind), route.Config.Verification.Kind, null) }; } + private WebhookVerificationResult VerifyTimestampedHmac( + RegisteredWebhookRoute route, + IHeaderDictionary headers, + byte[] bodyBytes) + { + var signatureHeader = RegisteredWebhookRoute.GetHeaderValue(headers, route.SignatureHeaderName); + if (string.IsNullOrWhiteSpace(signatureHeader)) + return WebhookVerificationResult.Reject("missing_signature"); + + if (!TryParseTimestampedHeader( + signatureHeader, + route.TimestampField, + route.TimestampSignatureField, + out var timestampText, + out var signatures)) + { + return WebhookVerificationResult.Reject("invalid_signature_header"); + } + + if (!long.TryParse(timestampText, NumberStyles.Integer, CultureInfo.InvariantCulture, out var timestamp)) + return WebhookVerificationResult.Reject("invalid_signature_header"); + + DateTimeOffset signedAt; + try + { + signedAt = DateTimeOffset.FromUnixTimeSeconds(timestamp); + } + catch (ArgumentOutOfRangeException) + { + return WebhookVerificationResult.Reject("invalid_signature_header"); + } + + if ((_timeProvider.GetUtcNow() - signedAt).Duration() + > TimeSpan.FromSeconds(route.TimestampToleranceSeconds)) + { + return WebhookVerificationResult.Reject("timestamp_out_of_tolerance"); + } + + var secret = route.Config.Verification.Secret!.Value; + var expected = ComputeTimestampedSha256( + secret, + timestampText, + route.SignedPayloadSeparator, + bodyBytes); + if (!signatures.Any(signature => IsMatchingHexSignature(expected, signature))) + return WebhookVerificationResult.Reject("invalid_signature"); + + return WebhookVerificationResult.Accept( + RegisteredWebhookRoute.GetHeaderValue(headers, route.EventHeaderName), + RegisteredWebhookRoute.GetHeaderValue(headers, route.DeliveryIdHeaderName)); + } + private static WebhookVerificationResult VerifyHmac( RegisteredWebhookRoute route, IHeaderDictionary headers, @@ -78,6 +134,74 @@ private static string ComputeExpectedSha256(string secret, byte[] bodyBytes, str var hash = Convert.ToHexString(hmac.ComputeHash(bodyBytes)).ToLowerInvariant(); return string.Concat(prefix, hash); } + + private static byte[] ComputeTimestampedSha256( + string secret, + string timestamp, + string separator, + byte[] bodyBytes) + { + var prefixBytes = Encoding.UTF8.GetBytes(timestamp + separator); + var signedPayload = new byte[prefixBytes.Length + bodyBytes.Length]; + Buffer.BlockCopy(prefixBytes, 0, signedPayload, 0, prefixBytes.Length); + Buffer.BlockCopy(bodyBytes, 0, signedPayload, prefixBytes.Length, bodyBytes.Length); + + using var hmac = new HMACSHA256(Encoding.UTF8.GetBytes(secret)); + return hmac.ComputeHash(signedPayload); + } + + private static bool IsMatchingHexSignature(byte[] expected, string providedHex) + { + byte[] provided; + try + { + provided = Convert.FromHexString(providedHex); + } + catch (FormatException) + { + return false; + } + + return provided.Length == expected.Length + && CryptographicOperations.FixedTimeEquals(expected, provided); + } + + private static bool TryParseTimestampedHeader( + string header, + string timestampField, + string signatureField, + out string timestamp, + out List signatures) + { + timestamp = string.Empty; + signatures = []; + + foreach (var component in header.Split(',', StringSplitOptions.TrimEntries)) + { + var separatorIndex = component.IndexOf('=', StringComparison.Ordinal); + if (separatorIndex <= 0 || separatorIndex == component.Length - 1) + return false; + + var key = component[..separatorIndex].Trim(); + var value = component[(separatorIndex + 1)..].Trim(); + if (key.Length == 0 || value.Length == 0) + return false; + + if (string.Equals(key, timestampField, StringComparison.Ordinal)) + { + if (timestamp.Length > 0) + return false; + + timestamp = value; + } + else if (string.Equals(key, signatureField, StringComparison.Ordinal)) + { + signatures.Add(value); + } + } + + return timestamp.Length > 0 && signatures.Count > 0; + } } public sealed record WebhookVerificationResult(bool IsAccepted, string? RejectionReason, string? EventType, string? DeliveryId) From 606d23322b4bf6125905c3f1de9f5a7addbb9d19 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 15 Jul 2026 19:46:36 +0000 Subject: [PATCH 02/10] fix(webhooks): preserve verified route configuration --- docs/spec/configuration.md | 4 +- .../.system/files/netclaw-operations/SKILL.md | 2 +- .../netclaw-operations/references/webhooks.md | 5 + .../add-timestamped-webhook-hmac/design.md | 4 +- .../specs/inbound-webhooks/spec.md | 17 ++- openspec/specs/inbound-webhooks/spec.md | 17 ++- .../Tools/SetWebhookToolProvenanceTests.cs | 101 ++++++++++++++++++ src/Netclaw.Actors/Tools/SetWebhookTool.cs | 88 ++++++++++++--- .../Webhooks/WebhooksCommandTests.cs | 21 ++++ .../WebhookRouteStoreTests.cs | 64 +++++++++++ .../Schemas/netclaw-config.v1.schema.json | 24 ++++- .../Schemas/webhook-route.v1.schema.json | 2 + .../WebhookRouteValidator.cs | 37 +++++-- 13 files changed, 347 insertions(+), 39 deletions(-) diff --git a/docs/spec/configuration.md b/docs/spec/configuration.md index 6eab591a5..af6af810f 100644 --- a/docs/spec/configuration.md +++ b/docs/spec/configuration.md @@ -426,8 +426,8 @@ Route-file fields: | `Verification.EventHeaderName` | string? | `null` | Event-name header. Defaults to `X-Webhook-Event`. | | `Verification.DeliveryIdHeaderName` | string? | `null` | Delivery ID header. Defaults to `X-Webhook-Delivery`. | | `Verification.ToleranceSeconds` | int? | `300` | Maximum past or future clock difference for `HmacTimestamped`, from 1 through 3600 seconds. | -| `Verification.TimestampField` | string? | `t` | Structured-header timestamp field for `HmacTimestamped`. | -| `Verification.SignatureField` | string? | `v1` | Structured-header signature field for `HmacTimestamped`; multiple instances support sender secret rotation. | +| `Verification.TimestampField` | string? | `t` | Structured-header timestamp field for `HmacTimestamped`; must differ from the signature field and cannot have surrounding whitespace or contain `,` or `=`. | +| `Verification.SignatureField` | string? | `v1` | Structured-header signature field for `HmacTimestamped`; follows the same name constraints, and multiple instances support sender secret rotation. | | `Verification.SignedPayloadSeparator` | string? | `.` | Separator between the exact timestamp text and raw body for `HmacTimestamped`. | | `Events` | string[] | `[]` | Optional allow-list of event types. Empty means all verified events are accepted. | | `Audience` | string | `Public` | Source audience for the autonomous webhook session (`Public`, `Team`, `Personal`). | diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index e2dbdf318..204b376ed 100644 --- a/feeds/skills/.system/files/netclaw-operations/SKILL.md +++ b/feeds/skills/.system/files/netclaw-operations/SKILL.md @@ -3,7 +3,7 @@ name: netclaw-operations description: "REQUIRED when the user asks about scheduling, reminders, cron jobs, timers, background jobs, diagnostics, troubleshooting, MCP tools, daemon health, identity updates, or Netclaw capabilities and self-maintenance." metadata: author: netclaw - version: "2.29.0" + version: "2.30.0" --- # Netclaw Operations diff --git a/feeds/skills/.system/files/netclaw-operations/references/webhooks.md b/feeds/skills/.system/files/netclaw-operations/references/webhooks.md index 038ffb7bf..06b806c67 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/webhooks.md +++ b/feeds/skills/.system/files/netclaw-operations/references/webhooks.md @@ -54,6 +54,11 @@ when the sender documents a different wire format. Multiple `v1` values are accepted for sender-side secret rotation. Missing, malformed, stale, or future-dated signatures fail closed. +Timestamp and signature field names must be distinct, have no surrounding +whitespace, and contain neither `,` nor `=`. When updating a route through +`set_webhook`, omitted optional settings retain their existing values; provide +an argument only when changing that setting. + Route files hot-reload without restarting the daemon. If a route file becomes invalid, Netclaw removes that route immediately and emits an operational alert. diff --git a/openspec/changes/add-timestamped-webhook-hmac/design.md b/openspec/changes/add-timestamped-webhook-hmac/design.md index f9df11997..491045922 100644 --- a/openspec/changes/add-timestamped-webhook-hmac/design.md +++ b/openspec/changes/add-timestamped-webhook-hmac/design.md @@ -33,7 +33,9 @@ Compatibility includes existing files, CLI/tool callers, and downgrade behavior. 5. **Keep configuration generic at the user surface.** The CLI adds `hmac-timestamped` plus advanced optional flags. Providers still specify `SignatureHeaderName` because Stripe and TextForge use different names. Provider presets are deferred until repeated configuration demonstrates a need. -6. **Preserve inactive fields.** Validation applies timestamp constraints only when `HmacTimestamped` is selected. Switching kinds does not erase dormant settings, and old kinds do not acquire new behavior. +6. **Preserve inactive and omitted fields.** Validation applies timestamp constraints only when `HmacTimestamped` is selected. Switching kinds does not erase dormant settings, and old kinds do not acquire new behavior. CLI and `set_webhook` updates retain optional route and verification values that the caller omits; updating an existing route also requires authority for its current audience. + +7. **Reject unrepresentable structured-header field names before persistence.** Effective timestamp and signature field names must be distinct, have no leading or trailing whitespace, and contain neither `,` nor `=`. These constraints match the verifier's comma-separated `key=value` grammar and prevent routes that could never authenticate. No actor or persistence boundary changes. Verification still returns the existing in-memory result consumed by the endpoint before any session actor is created. diff --git a/openspec/changes/add-timestamped-webhook-hmac/specs/inbound-webhooks/spec.md b/openspec/changes/add-timestamped-webhook-hmac/specs/inbound-webhooks/spec.md index 850712391..9abb789a0 100644 --- a/openspec/changes/add-timestamped-webhook-hmac/specs/inbound-webhooks/spec.md +++ b/openspec/changes/add-timestamped-webhook-hmac/specs/inbound-webhooks/spec.md @@ -86,15 +86,26 @@ The route schema, CLI, and `set_webhook` tool SHALL expose timestamp field, signature field, signed-payload separator, and tolerance settings as optional configuration for timestamped HMAC routes. Body-only HMAC SHALL remain the default verification kind, and the system SHALL NOT infer or fall back between -verification kinds. +verification kinds. Effective timestamp and signature field names SHALL be +distinct, SHALL NOT have leading or trailing whitespace, and SHALL NOT contain +the structured-header delimiters `,` or `=`. #### Scenario: Existing route is updated without timestamp options -- **GIVEN** an existing body-only HMAC or shared-header secret route -- **WHEN** an operator updates an unrelated route property using the CLI +- **GIVEN** an existing body-only HMAC, timestamped-HMAC, or shared-header secret + route +- **WHEN** an operator updates an unrelated route property using the CLI or + `set_webhook` without supplying optional verification settings - **THEN** the stored verifier kind and settings remain unchanged - **AND** timestamped-HMAC properties are not introduced into the route file +#### Scenario: Unrepresentable structured-header fields are rejected + +- **GIVEN** a timestamped-HMAC route has equal timestamp and signature fields, + surrounding field-name whitespace, or a field name containing `,` or `=` +- **WHEN** the operator attempts to persist the route +- **THEN** validation rejects the configuration before persistence + #### Scenario: New timestamped route uses effective defaults - **GIVEN** an operator selects timestamped HMAC and supplies a signature header diff --git a/openspec/specs/inbound-webhooks/spec.md b/openspec/specs/inbound-webhooks/spec.md index 0c0d71913..ee89b0251 100644 --- a/openspec/specs/inbound-webhooks/spec.md +++ b/openspec/specs/inbound-webhooks/spec.md @@ -244,15 +244,26 @@ The route schema, CLI, and `set_webhook` tool SHALL expose timestamp field, signature field, signed-payload separator, and tolerance settings as optional configuration for timestamped HMAC routes. Body-only HMAC SHALL remain the default verification kind, and the system SHALL NOT infer or fall back between -verification kinds. +verification kinds. Effective timestamp and signature field names SHALL be +distinct, SHALL NOT have leading or trailing whitespace, and SHALL NOT contain +the structured-header delimiters `,` or `=`. #### Scenario: Existing route is updated without timestamp options -- **GIVEN** an existing body-only HMAC or shared-header secret route -- **WHEN** an operator updates an unrelated route property using the CLI +- **GIVEN** an existing body-only HMAC, timestamped-HMAC, or shared-header secret + route +- **WHEN** an operator updates an unrelated route property using the CLI or + `set_webhook` without supplying optional verification settings - **THEN** the stored verifier kind and settings remain unchanged - **AND** timestamped-HMAC properties are not introduced into the route file +#### Scenario: Unrepresentable structured-header fields are rejected + +- **GIVEN** a timestamped-HMAC route has equal timestamp and signature fields, + surrounding field-name whitespace, or a field name containing `,` or `=` +- **WHEN** the operator attempts to persist the route +- **THEN** validation rejects the configuration before persistence + #### Scenario: New timestamped route uses effective defaults - **GIVEN** an operator selects timestamped HMAC and supplies a signature header diff --git a/src/Netclaw.Actors.Tests/Tools/SetWebhookToolProvenanceTests.cs b/src/Netclaw.Actors.Tests/Tools/SetWebhookToolProvenanceTests.cs index dd18fb81b..2496a5143 100644 --- a/src/Netclaw.Actors.Tests/Tools/SetWebhookToolProvenanceTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/SetWebhookToolProvenanceTests.cs @@ -145,4 +145,105 @@ public async Task Timestamp_settings_are_rejected_for_body_hmac() Assert.Contains("require 'verificationKind' to be 'HmacTimestamped'", result); Assert.False(_store.TryGet("invalid-route", out _)); } + + [Fact] + public async Task Update_preserves_omitted_route_and_verification_settings() + { + var tool = new SetWebhookTool(_store); + var createResult = await tool.ExecuteAsync(new Dictionary + { + ["RouteName"] = "stripe-events", + ["Prompt"] = "Handle Stripe delivery.", + ["VerificationKind"] = "HmacTimestamped", + ["Secret"] = "old-secret", + ["SignatureHeaderName"] = "Stripe-Signature", + ["EventHeaderName"] = "Stripe-Event", + ["DeliveryIdHeaderName"] = "Stripe-Delivery", + ["TimestampField"] = "timestamp", + ["SignatureField"] = "signature", + ["SignedPayloadSeparator"] = "::", + ["ToleranceSeconds"] = 120, + ["Events"] = "payment.created,payment.failed", + ["Audience"] = "team", + ["NotifyInstructions"] = "Notify the payments channel.", + ["DeliveryRequired"] = false, + ["NotificationChannelId"] = "C-PAYMENTS", + ["MaxBodyBytes"] = 4096, + ["RateLimitPerMinute"] = 12, + ["Enabled"] = false + }, Context(TrustAudience.Personal), TestContext.Current.CancellationToken); + Assert.DoesNotContain("Error", createResult); + + var updateResult = await tool.ExecuteAsync(new Dictionary + { + ["RouteName"] = "stripe-events", + ["Prompt"] = "Handle and summarize Stripe delivery.", + ["VerificationKind"] = "HmacTimestamped", + ["Secret"] = "new-secret", + ["RateLimitPerMinute"] = 24 + }, Context(TrustAudience.Team), TestContext.Current.CancellationToken); + + Assert.DoesNotContain("Error", updateResult); + Assert.True(_store.TryGet("stripe-events", out var saved)); + var route = saved.Definition!; + Assert.Equal("Handle and summarize Stripe delivery.", route.Prompt); + Assert.Equal(new SensitiveString("new-secret"), route.Verification.Secret); + Assert.Equal(24, route.RateLimitPerMinute); + Assert.False(route.Enabled); + Assert.Equal(4096, route.MaxBodyBytes); + Assert.Equal(TrustAudience.Team, route.Audience); + Assert.Equal(["payment.created", "payment.failed"], route.Events); + Assert.Equal("Notify the payments channel.", route.NotifyInstructions); + Assert.False(route.DeliveryRequired); + Assert.Equal("C-PAYMENTS", route.NotificationTarget?.ChannelId); + Assert.Equal("Stripe-Signature", route.Verification.SignatureHeaderName); + Assert.Equal("Stripe-Event", route.Verification.EventHeaderName); + Assert.Equal("Stripe-Delivery", route.Verification.DeliveryIdHeaderName); + Assert.Equal("timestamp", route.Verification.TimestampField); + Assert.Equal("signature", route.Verification.SignatureField); + Assert.Equal("::", route.Verification.SignedPayloadSeparator); + Assert.Equal(120, route.Verification.ToleranceSeconds); + } + + [Fact] + public async Task Lower_audience_cannot_update_higher_audience_route() + { + var createResult = await CreateRouteAsync("team-route", TrustAudience.Team, requestedAudience: null); + Assert.DoesNotContain("Error", createResult); + var tool = new SetWebhookTool(_store); + + var updateResult = await tool.ExecuteAsync(new Dictionary + { + ["RouteName"] = "team-route", + ["Prompt"] = "Replace team instructions.", + ["VerificationKind"] = "Hmac", + ["Secret"] = "replacement-secret" + }, Context(TrustAudience.Public), TestContext.Current.CancellationToken); + + Assert.Contains("exceeds creator authority", updateResult); + Assert.True(_store.TryGet("team-route", out var saved)); + Assert.Equal("Handle inbound delivery.", saved.Definition!.Prompt); + Assert.Equal(new SensitiveString("test-secret"), saved.Definition.Verification.Secret); + } + + [Theory] + [InlineData("v1")] + [InlineData(" timestamp")] + [InlineData("time=stamp")] + public async Task Unusable_timestamp_field_names_are_rejected_before_save(string timestampField) + { + var tool = new SetWebhookTool(_store); + + var result = await tool.ExecuteAsync(new Dictionary + { + ["RouteName"] = "invalid-route", + ["Prompt"] = "Handle delivery.", + ["VerificationKind"] = "HmacTimestamped", + ["Secret"] = "test-secret", + ["TimestampField"] = timestampField + }, Context(TrustAudience.Public), TestContext.Current.CancellationToken); + + Assert.Contains("Verification.TimestampField", result); + Assert.False(_store.TryGet("invalid-route", out _)); + } } diff --git a/src/Netclaw.Actors/Tools/SetWebhookTool.cs b/src/Netclaw.Actors/Tools/SetWebhookTool.cs index 9916956b5..26ff5e6d3 100644 --- a/src/Netclaw.Actors/Tools/SetWebhookTool.cs +++ b/src/Netclaw.Actors/Tools/SetWebhookTool.cs @@ -70,6 +70,20 @@ protected override Task ExecuteAsync(Params args, ToolInvocationContext if (!WebhookRouteStore.TryNormalizeRouteName(args.RouteName, out var routeName, out var routeError)) return Task.FromResult($"Error: {routeError}"); + WebhookRouteConfig? existing = null; + if (_store.TryGet(routeName, out var stored)) + { + if (stored.Definition is null) + return Task.FromResult($"Error: Existing webhook route '{routeName}' could not be parsed."); + + existing = stored.Definition; + if (existing.Audience > context.Audience) + { + return Task.FromResult( + $"Error: Existing route audience '{existing.Audience.ToWireValue()}' exceeds creator authority ({context.Audience.ToWireValue()})."); + } + } + if (string.IsNullOrWhiteSpace(args.Prompt)) return Task.FromResult("Error: 'prompt' is required."); if (string.IsNullOrWhiteSpace(args.Secret)) @@ -87,36 +101,68 @@ protected override Task ExecuteAsync(Params args, ToolInvocationContext return Task.FromResult("Error: Timestamp signature settings require 'verificationKind' to be 'HmacTimestamped'."); } - if (!TryResolveAudience(args.Audience, context.Audience, out var audience, out var audienceError)) + TrustAudience audience; + if (string.IsNullOrWhiteSpace(args.Audience) && existing is not null) + { + audience = existing.Audience; + } + else if (!TryResolveAudience(args.Audience, context.Audience, out audience, out var audienceError)) + { return Task.FromResult(audienceError!); + } + + var existingVerification = existing?.Verification; var definition = new WebhookRouteConfig { - Enabled = args.Enabled ?? true, + Enabled = args.Enabled ?? existing?.Enabled ?? true, Prompt = args.Prompt.Trim(), - Events = ParseEvents(args.Events), + Events = args.Events is null ? [.. existing?.Events ?? []] : ParseEvents(args.Events), Audience = audience, - NotifyInstructions = args.NotifyInstructions?.Trim() ?? string.Empty, - DeliveryRequired = args.DeliveryRequired ?? true, - MaxBodyBytes = args.MaxBodyBytes ?? 1024 * 1024, - RateLimitPerMinute = args.RateLimitPerMinute ?? 30, + NotifyInstructions = args.NotifyInstructions?.Trim() ?? existing?.NotifyInstructions ?? string.Empty, + DeliveryRequired = args.DeliveryRequired ?? existing?.DeliveryRequired ?? true, + MaxBodyBytes = args.MaxBodyBytes ?? existing?.MaxBodyBytes ?? 1024 * 1024, + RateLimitPerMinute = args.RateLimitPerMinute ?? existing?.RateLimitPerMinute ?? 30, Verification = new WebhookVerificationConfig { Kind = verificationKind, + HmacAlgorithm = existingVerification?.HmacAlgorithm ?? WebhookHmacAlgorithm.Sha256, Secret = new SensitiveString(args.Secret), - SignatureHeaderName = string.IsNullOrWhiteSpace(args.SignatureHeaderName) ? null : args.SignatureHeaderName.Trim(), - SignaturePrefix = string.IsNullOrWhiteSpace(args.SignaturePrefix) ? null : args.SignaturePrefix, - SecretHeaderName = string.IsNullOrWhiteSpace(args.SecretHeaderName) ? null : args.SecretHeaderName.Trim(), - EventHeaderName = string.IsNullOrWhiteSpace(args.EventHeaderName) ? null : args.EventHeaderName.Trim(), - DeliveryIdHeaderName = string.IsNullOrWhiteSpace(args.DeliveryIdHeaderName) ? null : args.DeliveryIdHeaderName.Trim(), - TimestampField = args.TimestampField?.Trim(), - SignatureField = args.SignatureField?.Trim(), - SignedPayloadSeparator = args.SignedPayloadSeparator, - ToleranceSeconds = args.ToleranceSeconds + SignatureHeaderName = args.SignatureHeaderName is null + ? existingVerification?.SignatureHeaderName + : NormalizeOptional(args.SignatureHeaderName), + SignaturePrefix = args.SignaturePrefix is null + ? existingVerification?.SignaturePrefix + : NormalizeOptional(args.SignaturePrefix, trim: false), + SecretHeaderName = args.SecretHeaderName is null + ? existingVerification?.SecretHeaderName + : NormalizeOptional(args.SecretHeaderName), + EventHeaderName = args.EventHeaderName is null + ? existingVerification?.EventHeaderName + : NormalizeOptional(args.EventHeaderName), + DeliveryIdHeaderName = args.DeliveryIdHeaderName is null + ? existingVerification?.DeliveryIdHeaderName + : NormalizeOptional(args.DeliveryIdHeaderName), + TimestampField = args.TimestampField is null + ? existingVerification?.TimestampField + : args.TimestampField, + SignatureField = args.SignatureField is null + ? existingVerification?.SignatureField + : args.SignatureField, + SignedPayloadSeparator = args.SignedPayloadSeparator ?? existingVerification?.SignedPayloadSeparator, + ToleranceSeconds = args.ToleranceSeconds ?? existingVerification?.ToleranceSeconds } }; - if (!string.IsNullOrWhiteSpace(args.NotificationChannelId)) + if (args.NotificationChannelId is null && existing?.NotificationTarget is { } existingTarget) + { + definition.NotificationTarget = new NotificationTargetConfig + { + Kind = existingTarget.Kind, + ChannelId = existingTarget.ChannelId + }; + } + else if (!string.IsNullOrWhiteSpace(args.NotificationChannelId)) { definition.NotificationTarget = new NotificationTargetConfig { @@ -179,4 +225,12 @@ private static List ParseEvents(string? value) return [.. value.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries).Where(x => !string.IsNullOrWhiteSpace(x))]; } + + private static string? NormalizeOptional(string value, bool trim = true) + { + if (string.IsNullOrWhiteSpace(value)) + return null; + + return trim ? value.Trim() : value; + } } diff --git a/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs b/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs index c5eaef8b6..77907952d 100644 --- a/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs @@ -479,6 +479,27 @@ public async Task Set_Timestamp_options_with_body_hmac_fails_without_persisting( Assert.False(File.Exists(Path.Combine(_paths.WebhooksDirectory, "invalid-route.json"))); } + [Theory] + [InlineData("v1", "v1")] + [InlineData(" timestamp", "v1")] + [InlineData("time=stamp", "v1")] + public async Task Set_Unusable_timestamp_fields_fail_without_persisting( + string timestampField, + string signatureField) + { + var result = await WebhooksCommand.RunAsync([ + "webhooks", "set", "invalid-route", + "--prompt", "Process event", + "--secret", "secret", + "--verification-kind", "hmac-timestamped", + "--timestamp-field", timestampField, + "--signature-field", signatureField + ], _paths); + + Assert.Equal(1, result); + Assert.False(File.Exists(Path.Combine(_paths.WebhooksDirectory, "invalid-route.json"))); + } + [Fact] public async Task Set_Unrelated_update_preserves_legacy_verifier_without_timestamp_fields() { diff --git a/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs b/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs index 0bc1f39fb..3bdec2ed6 100644 --- a/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs +++ b/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs @@ -3,6 +3,7 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using System.Text.Json; using Netclaw.Configuration; using Netclaw.Tests.Utilities; using Xunit; @@ -169,6 +170,59 @@ public void Timestamped_route_rejects_unsafe_tolerance(int toleranceSeconds) Assert.Contains(errors, error => error.Contains("ToleranceSeconds", StringComparison.Ordinal)); } + [Theory] + [InlineData("t", "t")] + [InlineData(null, "t")] + [InlineData("v1", null)] + [InlineData(" timestamp", "v1")] + [InlineData("timestamp ", "v1")] + [InlineData("time,stamp", "v1")] + [InlineData("time=stamp", "v1")] + public void Timestamped_route_rejects_unusable_structured_header_fields( + string? timestampField, + string? signatureField) + { + var route = CreateValidRoute(); + route.Verification.Kind = WebhookVerifierKind.HmacTimestamped; + route.Verification.TimestampField = timestampField; + route.Verification.SignatureField = signatureField; + + var errors = WebhookRouteValidator.Validate("stripe", route); + + Assert.NotEmpty(errors); + } + + [Fact] + public void Embedded_config_and_route_schemas_share_timestamped_verification_contract() + { + using var configSchema = LoadEmbeddedSchema("netclaw-config.v1.schema.json"); + using var routeSchema = LoadEmbeddedSchema("webhook-route.v1.schema.json"); + var configVerification = configSchema.RootElement + .GetProperty("$defs") + .GetProperty("WebhookVerification") + .GetProperty("properties"); + var routeVerification = routeSchema.RootElement + .GetProperty("properties") + .GetProperty("Verification") + .GetProperty("properties"); + + foreach (var propertyName in new[] + { + "Kind", + "ToleranceSeconds", + "TimestampField", + "SignatureField", + "SignedPayloadSeparator" + }) + { + var configProperty = configVerification.GetProperty(propertyName); + var routeProperty = routeVerification.GetProperty(propertyName); + Assert.Equal( + JsonSerializer.Serialize(routeProperty), + JsonSerializer.Serialize(configProperty)); + } + } + [Theory] [InlineData("hmac", WebhookVerifierKind.Hmac)] [InlineData("header-secret", WebhookVerifierKind.HeaderSecret)] @@ -193,4 +247,14 @@ private static WebhookRouteConfig CreateValidRoute() Secret = new SensitiveString("secret") } }; + + private static JsonDocument LoadEmbeddedSchema(string fileName) + { + var assembly = typeof(EmbeddedSchemaLoader).Assembly; + var resourceName = Assert.Single( + assembly.GetManifestResourceNames(), + name => name.EndsWith(fileName, StringComparison.Ordinal)); + using var stream = Assert.IsAssignableFrom(assembly.GetManifestResourceStream(resourceName)); + return JsonDocument.Parse(stream); + } } diff --git a/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json b/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json index 83c0064dd..8516fb96b 100644 --- a/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json +++ b/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json @@ -905,7 +905,7 @@ "properties": { "Kind": { "type": "string", - "enum": ["Hmac", "HeaderSecret"], + "enum": ["Hmac", "HeaderSecret", "HmacTimestamped"], "default": "Hmac" }, "HmacAlgorithm": { @@ -930,6 +930,28 @@ }, "DeliveryIdHeaderName": { "type": ["string", "null"] + }, + "ToleranceSeconds": { + "type": ["integer", "null"], + "minimum": 1, + "maximum": 3600, + "default": 300 + }, + "TimestampField": { + "type": ["string", "null"], + "minLength": 1, + "pattern": "^[^,=\\s](?:[^,=]*[^,=\\s])?$", + "default": "t" + }, + "SignatureField": { + "type": ["string", "null"], + "minLength": 1, + "pattern": "^[^,=\\s](?:[^,=]*[^,=\\s])?$", + "default": "v1" + }, + "SignedPayloadSeparator": { + "type": ["string", "null"], + "default": "." } }, "additionalProperties": false diff --git a/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json b/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json index 981a4e411..a9b0caf1d 100644 --- a/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json +++ b/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json @@ -48,11 +48,13 @@ "TimestampField": { "type": ["string", "null"], "minLength": 1, + "pattern": "^[^,=\\s](?:[^,=]*[^,=\\s])?$", "default": "t" }, "SignatureField": { "type": ["string", "null"], "minLength": 1, + "pattern": "^[^,=\\s](?:[^,=]*[^,=\\s])?$", "default": "v1" }, "SignedPayloadSeparator": { diff --git a/src/Netclaw.Configuration/WebhookRouteValidator.cs b/src/Netclaw.Configuration/WebhookRouteValidator.cs index d0e539d5f..aacf3f48f 100644 --- a/src/Netclaw.Configuration/WebhookRouteValidator.cs +++ b/src/Netclaw.Configuration/WebhookRouteValidator.cs @@ -45,17 +45,13 @@ public static IReadOnlyList Validate(string routeName, WebhookRouteConfi if (route.Verification.ToleranceSeconds is < 1 or > 3600) errors.Add("Verification.ToleranceSeconds must be between 1 and 3600."); - if (route.Verification.TimestampField is { } timestampField - && string.IsNullOrWhiteSpace(timestampField)) - { - errors.Add("Verification.TimestampField cannot be blank."); - } - - if (route.Verification.SignatureField is { } signatureField - && string.IsNullOrWhiteSpace(signatureField)) - { - errors.Add("Verification.SignatureField cannot be blank."); - } + var timestampField = route.Verification.TimestampField ?? "t"; + var signatureField = route.Verification.SignatureField ?? "v1"; + ValidateStructuredHeaderField(errors, "TimestampField", timestampField); + ValidateStructuredHeaderField(errors, "SignatureField", signatureField); + + if (string.Equals(timestampField, signatureField, StringComparison.Ordinal)) + errors.Add("Verification.TimestampField and Verification.SignatureField must be different."); } if (route.MaxBodyBytes < 1) @@ -121,4 +117,23 @@ public static bool TryParseVerifierKind(string value, out WebhookVerifierKind ki return false; } } + + private static void ValidateStructuredHeaderField( + List errors, + string propertyName, + string value) + { + if (string.IsNullOrWhiteSpace(value)) + { + errors.Add($"Verification.{propertyName} cannot be blank."); + return; + } + + if (!string.Equals(value, value.Trim(), StringComparison.Ordinal)) + errors.Add($"Verification.{propertyName} cannot have leading or trailing whitespace."); + + if (value.Contains(',', StringComparison.Ordinal) + || value.Contains('=', StringComparison.Ordinal)) + errors.Add($"Verification.{propertyName} cannot contain ',' or '='."); + } } From 0a2a46b218e9be5aadd65b0b80d517497bdd9770 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 15 Jul 2026 20:25:46 +0000 Subject: [PATCH 03/10] fix(webhooks): harden concurrent route updates --- docs/spec/configuration.md | 4 +- .../.system/files/netclaw-operations/SKILL.md | 2 +- .../netclaw-operations/references/webhooks.md | 7 +- .../add-timestamped-webhook-hmac/design.md | 6 +- .../specs/inbound-webhooks/spec.md | 24 +- openspec/specs/inbound-webhooks/spec.md | 24 +- .../Tools/SetWebhookToolProvenanceTests.cs | 2 + src/Netclaw.Actors/Tools/SetWebhookTool.cs | 52 ++- .../Webhooks/WebhooksCommandTests.cs | 19 +- src/Netclaw.Cli/Webhooks/WebhooksCommand.cs | 363 +++++++++--------- .../WebhookRouteStoreTests.cs | 83 +++- .../Schemas/netclaw-config.v1.schema.json | 4 +- .../Schemas/webhook-route.v1.schema.json | 4 +- .../WebhookRouteStore.cs | 102 ++++- .../WebhookRouteValidator.cs | 23 +- .../Webhooks/WebhookRouteCatalogTests.cs | 22 +- 16 files changed, 508 insertions(+), 233 deletions(-) diff --git a/docs/spec/configuration.md b/docs/spec/configuration.md index af6af810f..bfa91cef5 100644 --- a/docs/spec/configuration.md +++ b/docs/spec/configuration.md @@ -426,8 +426,8 @@ Route-file fields: | `Verification.EventHeaderName` | string? | `null` | Event-name header. Defaults to `X-Webhook-Event`. | | `Verification.DeliveryIdHeaderName` | string? | `null` | Delivery ID header. Defaults to `X-Webhook-Delivery`. | | `Verification.ToleranceSeconds` | int? | `300` | Maximum past or future clock difference for `HmacTimestamped`, from 1 through 3600 seconds. | -| `Verification.TimestampField` | string? | `t` | Structured-header timestamp field for `HmacTimestamped`; must differ from the signature field and cannot have surrounding whitespace or contain `,` or `=`. | -| `Verification.SignatureField` | string? | `v1` | Structured-header signature field for `HmacTimestamped`; follows the same name constraints, and multiple instances support sender secret rotation. | +| `Verification.TimestampField` | string? | `t` | Structured-header timestamp field for `HmacTimestamped`; must be an ASCII HTTP token and differ from the signature field. | +| `Verification.SignatureField` | string? | `v1` | Structured-header signature field for `HmacTimestamped`; follows the same HTTP-token constraint, and multiple instances support sender secret rotation. | | `Verification.SignedPayloadSeparator` | string? | `.` | Separator between the exact timestamp text and raw body for `HmacTimestamped`. | | `Events` | string[] | `[]` | Optional allow-list of event types. Empty means all verified events are accepted. | | `Audience` | string | `Public` | Source audience for the autonomous webhook session (`Public`, `Team`, `Personal`). | diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index 204b376ed..6ff82b499 100644 --- a/feeds/skills/.system/files/netclaw-operations/SKILL.md +++ b/feeds/skills/.system/files/netclaw-operations/SKILL.md @@ -3,7 +3,7 @@ name: netclaw-operations description: "REQUIRED when the user asks about scheduling, reminders, cron jobs, timers, background jobs, diagnostics, troubleshooting, MCP tools, daemon health, identity updates, or Netclaw capabilities and self-maintenance." metadata: author: netclaw - version: "2.30.0" + version: "2.31.0" --- # Netclaw Operations diff --git a/feeds/skills/.system/files/netclaw-operations/references/webhooks.md b/feeds/skills/.system/files/netclaw-operations/references/webhooks.md index 06b806c67..eb98eb5dd 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/webhooks.md +++ b/feeds/skills/.system/files/netclaw-operations/references/webhooks.md @@ -54,10 +54,9 @@ when the sender documents a different wire format. Multiple `v1` values are accepted for sender-side secret rotation. Missing, malformed, stale, or future-dated signatures fail closed. -Timestamp and signature field names must be distinct, have no surrounding -whitespace, and contain neither `,` nor `=`. When updating a route through -`set_webhook`, omitted optional settings retain their existing values; provide -an argument only when changing that setting. +Timestamp and signature field names must be distinct ASCII HTTP tokens. When +updating a route through `set_webhook`, omitted optional settings retain their +existing values; provide an argument only when changing that setting. Route files hot-reload without restarting the daemon. If a route file becomes invalid, Netclaw removes that route immediately and emits an operational alert. diff --git a/openspec/changes/add-timestamped-webhook-hmac/design.md b/openspec/changes/add-timestamped-webhook-hmac/design.md index 491045922..0f3a5877f 100644 --- a/openspec/changes/add-timestamped-webhook-hmac/design.md +++ b/openspec/changes/add-timestamped-webhook-hmac/design.md @@ -33,9 +33,11 @@ Compatibility includes existing files, CLI/tool callers, and downgrade behavior. 5. **Keep configuration generic at the user surface.** The CLI adds `hmac-timestamped` plus advanced optional flags. Providers still specify `SignatureHeaderName` because Stripe and TextForge use different names. Provider presets are deferred until repeated configuration demonstrates a need. -6. **Preserve inactive and omitted fields.** Validation applies timestamp constraints only when `HmacTimestamped` is selected. Switching kinds does not erase dormant settings, and old kinds do not acquire new behavior. CLI and `set_webhook` updates retain optional route and verification values that the caller omits; updating an existing route also requires authority for its current audience. +6. **Preserve inactive and omitted fields.** Validation applies timestamp constraints only when `HmacTimestamped` is selected. Switching kinds does not erase dormant settings, and old kinds do not acquire new behavior. CLI and `set_webhook` updates retain optional route and verification values that the caller omits; `set_webhook` performs its read, audience authorization, patch, validation, and write under one store lock. -7. **Reject unrepresentable structured-header field names before persistence.** Effective timestamp and signature field names must be distinct, have no leading or trailing whitespace, and contain neither `,` nor `=`. These constraints match the verifier's comma-separated `key=value` grammar and prevent routes that could never authenticate. +7. **Reject unrepresentable structured-header field names before persistence.** Effective timestamp and signature field names must be distinct HTTP tokens. This excludes whitespace, delimiters, non-ASCII characters, and controls that cannot form a valid structured-header key. + +8. **Reject undefined numeric enum values during shared validation.** Route deserialization retains its prior ability to read numeric enum values for compatibility, but values outside the defined verifier-kind and HMAC-algorithm sets fail route validation before request handling. No actor or persistence boundary changes. Verification still returns the existing in-memory result consumed by the endpoint before any session actor is created. diff --git a/openspec/changes/add-timestamped-webhook-hmac/specs/inbound-webhooks/spec.md b/openspec/changes/add-timestamped-webhook-hmac/specs/inbound-webhooks/spec.md index 9abb789a0..de0f54040 100644 --- a/openspec/changes/add-timestamped-webhook-hmac/specs/inbound-webhooks/spec.md +++ b/openspec/changes/add-timestamped-webhook-hmac/specs/inbound-webhooks/spec.md @@ -87,8 +87,8 @@ signature field, signed-payload separator, and tolerance settings as optional configuration for timestamped HMAC routes. Body-only HMAC SHALL remain the default verification kind, and the system SHALL NOT infer or fall back between verification kinds. Effective timestamp and signature field names SHALL be -distinct, SHALL NOT have leading or trailing whitespace, and SHALL NOT contain -the structured-header delimiters `,` or `=`. +distinct HTTP tokens. Undefined numeric verifier-kind or HMAC-algorithm values +SHALL be rejected during route validation before request handling. #### Scenario: Existing route is updated without timestamp options @@ -99,13 +99,29 @@ the structured-header delimiters `,` or `=`. - **THEN** the stored verifier kind and settings remain unchanged - **AND** timestamped-HMAC properties are not introduced into the route file +#### Scenario: Concurrent tool updates are serialized + +- **GIVEN** two authorized `set_webhook` invocations concurrently update the + same existing route +- **WHEN** each invocation reads, patches, validates, and saves the definition +- **THEN** those operations execute atomically under the route store lock +- **AND** neither invocation overwrites fields retained from the other's update + #### Scenario: Unrepresentable structured-header fields are rejected -- **GIVEN** a timestamped-HMAC route has equal timestamp and signature fields, - surrounding field-name whitespace, or a field name containing `,` or `=` +- **GIVEN** a timestamped-HMAC route has equal timestamp and signature fields or + a field name containing characters outside the HTTP token grammar - **WHEN** the operator attempts to persist the route - **THEN** validation rejects the configuration before persistence +#### Scenario: Undefined numeric verification enum is rejected + +- **GIVEN** a route contains a numeric verifier-kind or HMAC-algorithm value not + defined by the running daemon +- **WHEN** the route catalog validates that definition +- **THEN** the route is invalidated before request handling +- **AND** other valid routes remain available + #### Scenario: New timestamped route uses effective defaults - **GIVEN** an operator selects timestamped HMAC and supplies a signature header diff --git a/openspec/specs/inbound-webhooks/spec.md b/openspec/specs/inbound-webhooks/spec.md index ee89b0251..ab0988549 100644 --- a/openspec/specs/inbound-webhooks/spec.md +++ b/openspec/specs/inbound-webhooks/spec.md @@ -245,8 +245,8 @@ signature field, signed-payload separator, and tolerance settings as optional configuration for timestamped HMAC routes. Body-only HMAC SHALL remain the default verification kind, and the system SHALL NOT infer or fall back between verification kinds. Effective timestamp and signature field names SHALL be -distinct, SHALL NOT have leading or trailing whitespace, and SHALL NOT contain -the structured-header delimiters `,` or `=`. +distinct HTTP tokens. Undefined numeric verifier-kind or HMAC-algorithm values +SHALL be rejected during route validation before request handling. #### Scenario: Existing route is updated without timestamp options @@ -257,13 +257,29 @@ the structured-header delimiters `,` or `=`. - **THEN** the stored verifier kind and settings remain unchanged - **AND** timestamped-HMAC properties are not introduced into the route file +#### Scenario: Concurrent tool updates are serialized + +- **GIVEN** two authorized `set_webhook` invocations concurrently update the + same existing route +- **WHEN** each invocation reads, patches, validates, and saves the definition +- **THEN** those operations execute atomically under the route store lock +- **AND** neither invocation overwrites fields retained from the other's update + #### Scenario: Unrepresentable structured-header fields are rejected -- **GIVEN** a timestamped-HMAC route has equal timestamp and signature fields, - surrounding field-name whitespace, or a field name containing `,` or `=` +- **GIVEN** a timestamped-HMAC route has equal timestamp and signature fields or + a field name containing characters outside the HTTP token grammar - **WHEN** the operator attempts to persist the route - **THEN** validation rejects the configuration before persistence +#### Scenario: Undefined numeric verification enum is rejected + +- **GIVEN** a route contains a numeric verifier-kind or HMAC-algorithm value not + defined by the running daemon +- **WHEN** the route catalog validates that definition +- **THEN** the route is invalidated before request handling +- **AND** other valid routes remain available + #### Scenario: New timestamped route uses effective defaults - **GIVEN** an operator selects timestamped HMAC and supplies a signature header diff --git a/src/Netclaw.Actors.Tests/Tools/SetWebhookToolProvenanceTests.cs b/src/Netclaw.Actors.Tests/Tools/SetWebhookToolProvenanceTests.cs index 2496a5143..dfd2faca0 100644 --- a/src/Netclaw.Actors.Tests/Tools/SetWebhookToolProvenanceTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/SetWebhookToolProvenanceTests.cs @@ -230,6 +230,8 @@ public async Task Lower_audience_cannot_update_higher_audience_route() [InlineData("v1")] [InlineData(" timestamp")] [InlineData("time=stamp")] + [InlineData("time\nstamp")] + [InlineData("téstamp")] public async Task Unusable_timestamp_field_names_are_rejected_before_save(string timestampField) { var tool = new SetWebhookTool(_store); diff --git a/src/Netclaw.Actors/Tools/SetWebhookTool.cs b/src/Netclaw.Actors/Tools/SetWebhookTool.cs index 26ff5e6d3..9cc9b8fb7 100644 --- a/src/Netclaw.Actors/Tools/SetWebhookTool.cs +++ b/src/Netclaw.Actors/Tools/SetWebhookTool.cs @@ -1,4 +1,4 @@ -// ----------------------------------------------------------------------- +// ----------------------------------------------------------------------- // // Copyright (C) 2026 - 2026 Petabridge, LLC // @@ -70,20 +70,6 @@ protected override Task ExecuteAsync(Params args, ToolInvocationContext if (!WebhookRouteStore.TryNormalizeRouteName(args.RouteName, out var routeName, out var routeError)) return Task.FromResult($"Error: {routeError}"); - WebhookRouteConfig? existing = null; - if (_store.TryGet(routeName, out var stored)) - { - if (stored.Definition is null) - return Task.FromResult($"Error: Existing webhook route '{routeName}' could not be parsed."); - - existing = stored.Definition; - if (existing.Audience > context.Audience) - { - return Task.FromResult( - $"Error: Existing route audience '{existing.Audience.ToWireValue()}' exceeds creator authority ({context.Audience.ToWireValue()})."); - } - } - if (string.IsNullOrWhiteSpace(args.Prompt)) return Task.FromResult("Error: 'prompt' is required."); if (string.IsNullOrWhiteSpace(args.Secret)) @@ -101,14 +87,40 @@ protected override Task ExecuteAsync(Params args, ToolInvocationContext return Task.FromResult("Error: Timestamp signature settings require 'verificationKind' to be 'HmacTimestamped'."); } + try + { + var result = _store.Update( + routeName, + existing => BuildUpdate(routeName, args, context.Audience, verificationKind, existing)); + return Task.FromResult(result); + } + catch (InvalidDataException ex) + { + return Task.FromResult($"Error: {ex.Message}"); + } + } + + private static (WebhookRouteConfig? Definition, string Result) BuildUpdate( + string routeName, + Params args, + TrustAudience creatorAudience, + WebhookVerifierKind verificationKind, + WebhookRouteConfig? existing) + { + if (existing is not null && existing.Audience > creatorAudience) + { + return (null, + $"Error: Existing route audience '{existing.Audience.ToWireValue()}' exceeds creator authority ({creatorAudience.ToWireValue()})."); + } + TrustAudience audience; if (string.IsNullOrWhiteSpace(args.Audience) && existing is not null) { audience = existing.Audience; } - else if (!TryResolveAudience(args.Audience, context.Audience, out audience, out var audienceError)) + else if (!TryResolveAudience(args.Audience, creatorAudience, out audience, out var audienceError)) { - return Task.FromResult(audienceError!); + return (null, audienceError!); } var existingVerification = existing?.Verification; @@ -173,10 +185,10 @@ protected override Task ExecuteAsync(Params args, ToolInvocationContext var validationErrors = WebhookRouteValidator.Validate(routeName, definition); if (validationErrors.Count > 0) - return Task.FromResult($"Error: {validationErrors[0]}"); + return (null, $"Error: {validationErrors[0]}"); - _store.Save(routeName, definition); - return Task.FromResult($"Webhook route '{routeName}' saved at /api/webhooks/{routeName}. Secret stored in the route file; keep it aligned with the sender configuration."); + return (definition, + $"Webhook route '{routeName}' saved at /api/webhooks/{routeName}. Secret stored in the route file; keep it aligned with the sender configuration."); } /// diff --git a/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs b/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs index 77907952d..244779f29 100644 --- a/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs @@ -1,4 +1,4 @@ -// ----------------------------------------------------------------------- +// ----------------------------------------------------------------------- // // Copyright (C) 2026 - 2026 Petabridge, LLC // @@ -151,6 +151,21 @@ public async Task Set_NewRoute_CreatesFile() Assert.True(File.Exists(Path.Combine(_paths.WebhooksDirectory, "new-route.json"))); } + [Fact] + public async Task Set_WriteFailure_DoesNotReportSuccess() + { + Directory.CreateDirectory(Path.Combine(_paths.WebhooksDirectory, "blocked-route.json")); + using var output = new StringWriter(); + + await Assert.ThrowsAnyAsync(() => WebhooksCommand.RunAsync([ + "webhooks", "set", "blocked-route", + "--prompt", "Test prompt", + "--secret", "test-secret" + ], _paths, output)); + + Assert.DoesNotContain("[OK]", output.ToString(), StringComparison.Ordinal); + } + [Fact] public async Task Set_WithUppercaseRoute_NormalizesToLowercase() { @@ -483,6 +498,8 @@ public async Task Set_Timestamp_options_with_body_hmac_fails_without_persisting( [InlineData("v1", "v1")] [InlineData(" timestamp", "v1")] [InlineData("time=stamp", "v1")] + [InlineData("time stamp", "v1")] + [InlineData("téstamp", "v1")] public async Task Set_Unusable_timestamp_fields_fail_without_persisting( string timestampField, string signatureField) diff --git a/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs b/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs index a00b37c2f..6226c4e3c 100644 --- a/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs +++ b/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs @@ -1,4 +1,4 @@ -// ----------------------------------------------------------------------- +// ----------------------------------------------------------------------- // // Copyright (C) 2026 - 2026 Petabridge, LLC // @@ -286,244 +286,253 @@ private static int RunSet(string[] args, WebhookRouteStore store, NetclawPaths p return 1; } - var exists = store.TryGet(routeName, out var existing); - - if (createOnly && exists) + var routeSaved = false; + var updatedExistingRoute = false; + var result = store.Update(routeName, existing => { - Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' already exists (--create-only specified)."); - return 1; - } + var exists = existing is not null; - if (updateOnly && !exists) - { - Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' does not exist (--update-only specified)."); - return 1; - } + if (createOnly && exists) + { + Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' already exists (--create-only specified)."); + return (null, 1); + } - // Start with existing config or defaults - var route = existing.Definition ?? new WebhookRouteConfig(); - route.Verification ??= new WebhookVerificationConfig(); - route.Events ??= []; + if (updateOnly && !exists) + { + Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' does not exist (--update-only specified)."); + return (null, 1); + } - // Parse prompt - if (!TryResolveTextInput(args, "--prompt", "--prompt-file", out var prompt, out var hasPrompt)) - return 1; + // Start with existing config or defaults + var route = existing ?? new WebhookRouteConfig(); + route.Verification ??= new WebhookVerificationConfig(); + route.Events ??= []; - if (hasPrompt) - route.Prompt = prompt; + // Parse prompt + if (!TryResolveTextInput(args, "--prompt", "--prompt-file", out var prompt, out var hasPrompt)) + return (null, 1); - // Parse secret - if (!TryResolveSecret(args, out var secret, out var hasSecret)) - return 1; + if (hasPrompt) + route.Prompt = prompt; - if (hasSecret) - route.Verification.Secret = new SensitiveString(secret); + // Parse secret + if (!TryResolveSecret(args, out var secret, out var hasSecret)) + return (null, 1); - // Parse verification kind - if (!TryGetFlagValue(args, "--verification-kind", out var verificationKind, out var hasVerificationKind)) - return 1; + if (hasSecret) + route.Verification.Secret = new SensitiveString(secret); - if (hasVerificationKind) - { - if (!WebhookRouteValidator.TryParseVerifierKind(verificationKind, out var kind)) + // Parse verification kind + if (!TryGetFlagValue(args, "--verification-kind", out var verificationKind, out var hasVerificationKind)) + return (null, 1); + + if (hasVerificationKind) { - Console.Error.WriteLine($"[FAIL] Invalid verification kind: '{verificationKind}'. Use 'hmac', 'hmac-timestamped', or 'header-secret'."); - return 1; + if (!WebhookRouteValidator.TryParseVerifierKind(verificationKind, out var kind)) + { + Console.Error.WriteLine($"[FAIL] Invalid verification kind: '{verificationKind}'. Use 'hmac', 'hmac-timestamped', or 'header-secret'."); + return (null, 1); + } + route.Verification.Kind = kind; } - route.Verification.Kind = kind; - } - // Parse verification headers - if (!TryGetFlagValue(args, "--signature-header", out var signatureHeader, out var hasSignatureHeader)) - return 1; + // Parse verification headers + if (!TryGetFlagValue(args, "--signature-header", out var signatureHeader, out var hasSignatureHeader)) + return (null, 1); - if (hasSignatureHeader) - route.Verification.SignatureHeaderName = signatureHeader; + if (hasSignatureHeader) + route.Verification.SignatureHeaderName = signatureHeader; - if (!TryGetFlagValue(args, "--signature-prefix", out var signaturePrefix, out var hasSignaturePrefix)) - return 1; + if (!TryGetFlagValue(args, "--signature-prefix", out var signaturePrefix, out var hasSignaturePrefix)) + return (null, 1); - if (hasSignaturePrefix) - route.Verification.SignaturePrefix = signaturePrefix; + if (hasSignaturePrefix) + route.Verification.SignaturePrefix = signaturePrefix; - if (!TryGetFlagValue(args, "--secret-header", out var secretHeader, out var hasSecretHeader)) - return 1; + if (!TryGetFlagValue(args, "--secret-header", out var secretHeader, out var hasSecretHeader)) + return (null, 1); - if (hasSecretHeader) - route.Verification.SecretHeaderName = secretHeader; + if (hasSecretHeader) + route.Verification.SecretHeaderName = secretHeader; - if (!TryGetFlagValue(args, "--event-header", out var eventHeader, out var hasEventHeader)) - return 1; + if (!TryGetFlagValue(args, "--event-header", out var eventHeader, out var hasEventHeader)) + return (null, 1); - if (hasEventHeader) - route.Verification.EventHeaderName = eventHeader; + if (hasEventHeader) + route.Verification.EventHeaderName = eventHeader; - if (!TryGetFlagValue(args, "--delivery-header", out var deliveryHeader, out var hasDeliveryHeader)) - return 1; + if (!TryGetFlagValue(args, "--delivery-header", out var deliveryHeader, out var hasDeliveryHeader)) + return (null, 1); - if (hasDeliveryHeader) - route.Verification.DeliveryIdHeaderName = deliveryHeader; + if (hasDeliveryHeader) + route.Verification.DeliveryIdHeaderName = deliveryHeader; - if (!TryGetFlagValue(args, "--timestamp-field", out var timestampField, out var hasTimestampField)) - return 1; + if (!TryGetFlagValue(args, "--timestamp-field", out var timestampField, out var hasTimestampField)) + return (null, 1); - if (hasTimestampField) - route.Verification.TimestampField = timestampField; + if (hasTimestampField) + route.Verification.TimestampField = timestampField; - if (!TryGetFlagValue(args, "--signature-field", out var signatureField, out var hasSignatureField)) - return 1; + if (!TryGetFlagValue(args, "--signature-field", out var signatureField, out var hasSignatureField)) + return (null, 1); - if (hasSignatureField) - route.Verification.SignatureField = signatureField; + if (hasSignatureField) + route.Verification.SignatureField = signatureField; - if (!TryGetFlagValue(args, "--signed-payload-separator", out var payloadSeparator, out var hasPayloadSeparator)) - return 1; + if (!TryGetFlagValue(args, "--signed-payload-separator", out var payloadSeparator, out var hasPayloadSeparator)) + return (null, 1); - if (hasPayloadSeparator) - route.Verification.SignedPayloadSeparator = payloadSeparator; + if (hasPayloadSeparator) + route.Verification.SignedPayloadSeparator = payloadSeparator; - if (!TryGetFlagValue(args, "--signature-tolerance-seconds", out var tolerance, out var hasTolerance)) - return 1; + if (!TryGetFlagValue(args, "--signature-tolerance-seconds", out var tolerance, out var hasTolerance)) + return (null, 1); - if (hasTolerance) - { - if (!int.TryParse(tolerance, out var toleranceSeconds)) + if (hasTolerance) { - Console.Error.WriteLine($"[FAIL] Invalid signature tolerance: '{tolerance}'. Must be a whole number from 1 to 3600."); - return 1; - } + if (!int.TryParse(tolerance, out var toleranceSeconds)) + { + Console.Error.WriteLine($"[FAIL] Invalid signature tolerance: '{tolerance}'. Must be a whole number from 1 to 3600."); + return (null, 1); + } - route.Verification.ToleranceSeconds = toleranceSeconds; - } + route.Verification.ToleranceSeconds = toleranceSeconds; + } - if ((hasTimestampField || hasSignatureField || hasPayloadSeparator || hasTolerance) - && route.Verification.Kind != WebhookVerifierKind.HmacTimestamped) - { - Console.Error.WriteLine("[FAIL] Timestamp signature options require '--verification-kind hmac-timestamped'."); - return 1; - } + if ((hasTimestampField || hasSignatureField || hasPayloadSeparator || hasTolerance) + && route.Verification.Kind != WebhookVerifierKind.HmacTimestamped) + { + Console.Error.WriteLine("[FAIL] Timestamp signature options require '--verification-kind hmac-timestamped'."); + return (null, 1); + } - // Parse events - if (!TryGetFlagValue(args, "--events", out var events, out var hasEvents)) - return 1; + // Parse events + if (!TryGetFlagValue(args, "--events", out var events, out var hasEvents)) + return (null, 1); - if (hasEvents) - { - route.Events = [.. events.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)]; - } + if (hasEvents) + { + route.Events = [.. events.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)]; + } - // Parse audience - if (!TryGetFlagValue(args, "--audience", out var audience, out var hasAudience)) - return 1; + // Parse audience + if (!TryGetFlagValue(args, "--audience", out var audience, out var hasAudience)) + return (null, 1); - if (hasAudience) - { - if (!Enum.TryParse(audience, ignoreCase: true, out var aud)) + if (hasAudience) { - Console.Error.WriteLine($"[FAIL] Invalid audience: '{audience}'. Use 'public', 'team', or 'personal'."); - return 1; + if (!Enum.TryParse(audience, ignoreCase: true, out var aud)) + { + Console.Error.WriteLine($"[FAIL] Invalid audience: '{audience}'. Use 'public', 'team', or 'personal'."); + return (null, 1); + } + route.Audience = aud; } - route.Audience = aud; - } - // Parse notification settings - if (!TryResolveTextInput(args, "--notify-instructions", "--notify-instructions-file", out var notifyInstructions, out var hasNotifyInstructions)) - return 1; + // Parse notification settings + if (!TryResolveTextInput(args, "--notify-instructions", "--notify-instructions-file", out var notifyInstructions, out var hasNotifyInstructions)) + return (null, 1); - if (hasNotifyInstructions) - route.NotifyInstructions = notifyInstructions; + if (hasNotifyInstructions) + route.NotifyInstructions = notifyInstructions; - var deliveryRequired = HasFlag(args, "--delivery-required"); - var noDeliveryRequired = HasFlag(args, "--no-delivery-required"); - if (deliveryRequired && noDeliveryRequired) - { - Console.Error.WriteLine("[FAIL] --delivery-required and --no-delivery-required cannot be used together."); - return 1; - } + var deliveryRequired = HasFlag(args, "--delivery-required"); + var noDeliveryRequired = HasFlag(args, "--no-delivery-required"); + if (deliveryRequired && noDeliveryRequired) + { + Console.Error.WriteLine("[FAIL] --delivery-required and --no-delivery-required cannot be used together."); + return (null, 1); + } - if (deliveryRequired) - route.DeliveryRequired = true; - if (noDeliveryRequired) - route.DeliveryRequired = false; + if (deliveryRequired) + route.DeliveryRequired = true; + if (noDeliveryRequired) + route.DeliveryRequired = false; - if (!TryGetFlagValue(args, "--notification-channel", out var notificationChannel, out var hasNotificationChannel)) - return 1; + if (!TryGetFlagValue(args, "--notification-channel", out var notificationChannel, out var hasNotificationChannel)) + return (null, 1); - if (hasNotificationChannel) - { - route.NotificationTarget ??= new NotificationTargetConfig(); - route.NotificationTarget.ChannelId = notificationChannel; - } + if (hasNotificationChannel) + { + route.NotificationTarget ??= new NotificationTargetConfig(); + route.NotificationTarget.ChannelId = notificationChannel; + } - // Parse limits - if (!TryGetFlagValue(args, "--max-body", out var maxBody, out var hasMaxBody)) - return 1; + // Parse limits + if (!TryGetFlagValue(args, "--max-body", out var maxBody, out var hasMaxBody)) + return (null, 1); - if (hasMaxBody) - { - if (!int.TryParse(maxBody, out var bytes) || bytes < 1) + if (hasMaxBody) { - Console.Error.WriteLine($"[FAIL] Invalid max body size: '{maxBody}'. Must be a positive integer."); - return 1; + if (!int.TryParse(maxBody, out var bytes) || bytes < 1) + { + Console.Error.WriteLine($"[FAIL] Invalid max body size: '{maxBody}'. Must be a positive integer."); + return (null, 1); + } + route.MaxBodyBytes = bytes; } - route.MaxBodyBytes = bytes; - } - if (!TryGetFlagValue(args, "--rate-limit", out var rateLimit, out var hasRateLimit)) - return 1; + if (!TryGetFlagValue(args, "--rate-limit", out var rateLimit, out var hasRateLimit)) + return (null, 1); - if (hasRateLimit) - { - if (!int.TryParse(rateLimit, out var limit) || limit < 1) + if (hasRateLimit) { - Console.Error.WriteLine($"[FAIL] Invalid rate limit: '{rateLimit}'. Must be a positive integer."); - return 1; + if (!int.TryParse(rateLimit, out var limit) || limit < 1) + { + Console.Error.WriteLine($"[FAIL] Invalid rate limit: '{rateLimit}'. Must be a positive integer."); + return (null, 1); + } + route.RateLimitPerMinute = limit; } - route.RateLimitPerMinute = limit; - } - // Parse enabled/disabled - var enabled = HasFlag(args, "--enabled"); - var disabled = HasFlag(args, "--disabled"); - if (enabled && disabled) - { - Console.Error.WriteLine("[FAIL] --enabled and --disabled cannot be used together."); - return 1; - } + // Parse enabled/disabled + var enabled = HasFlag(args, "--enabled"); + var disabled = HasFlag(args, "--disabled"); + if (enabled && disabled) + { + Console.Error.WriteLine("[FAIL] --enabled and --disabled cannot be used together."); + return (null, 1); + } - if (enabled) - route.Enabled = true; - if (disabled) - route.Enabled = false; + if (enabled) + route.Enabled = true; + if (disabled) + route.Enabled = false; - // Validate - var errors = WebhookRouteValidator.Validate(routeName, route); - if (errors.Count > 0) - { - Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' has validation errors:"); - foreach (var error in errors) + // Validate + var errors = WebhookRouteValidator.Validate(routeName, route); + if (errors.Count > 0) { - Console.Error.WriteLine($" - {error}"); + Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' has validation errors:"); + foreach (var error in errors) + { + Console.Error.WriteLine($" - {error}"); + } + return (null, 1); } - return 1; - } - if (dryRun) + if (dryRun) + { + output.WriteLine($"[OK] Webhook route '{routeName}' is valid (dry run, not saved)."); + output.WriteLine($" Endpoint: /api/webhooks/{routeName}"); + return (null, 0); + } + + routeSaved = true; + updatedExistingRoute = exists; + return (route, 0); + }); + + if (routeSaved) { - output.WriteLine($"[OK] Webhook route '{routeName}' is valid (dry run, not saved)."); + var action = updatedExistingRoute ? "Updated" : "Created"; + output.WriteLine($"[OK] {action} webhook route '{routeName}'."); + output.WriteLine($" File: {Path.Combine(paths.WebhooksDirectory, $"{routeName}.json")}"); output.WriteLine($" Endpoint: /api/webhooks/{routeName}"); - return 0; } - // Save - store.Save(routeName, route); - - var action = exists ? "Updated" : "Created"; - output.WriteLine($"[OK] {action} webhook route '{routeName}'."); - output.WriteLine($" File: {Path.Combine(paths.WebhooksDirectory, $"{routeName}.json")}"); - output.WriteLine($" Endpoint: /api/webhooks/{routeName}"); - - return 0; + return result; } // ── delete ── diff --git a/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs b/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs index 3bdec2ed6..a9a92f94c 100644 --- a/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs +++ b/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs @@ -1,4 +1,4 @@ -// ----------------------------------------------------------------------- +// ----------------------------------------------------------------------- // // Copyright (C) 2026 - 2026 Petabridge, LLC // @@ -192,6 +192,78 @@ public void Timestamped_route_rejects_unusable_structured_header_fields( Assert.NotEmpty(errors); } + [Theory] + [InlineData("time stamp")] + [InlineData("time\nstamp")] + [InlineData("t\0stamp")] + [InlineData("téstamp")] + public void Timestamped_route_rejects_non_token_structured_header_fields(string timestampField) + { + var route = CreateValidRoute(); + route.Verification.Kind = WebhookVerifierKind.HmacTimestamped; + route.Verification.TimestampField = timestampField; + + var errors = WebhookRouteValidator.Validate("stripe", route); + + Assert.Contains(errors, error => error.Contains("HTTP token", StringComparison.Ordinal)); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void Route_rejects_undefined_numeric_verification_enums(bool invalidKind) + { + var route = CreateValidRoute(); + if (invalidKind) + route.Verification.Kind = (WebhookVerifierKind)99; + else + route.Verification.HmacAlgorithm = (WebhookHmacAlgorithm)99; + + var errors = WebhookRouteValidator.Validate("invalid-enum", route); + + Assert.Contains(errors, error => error.Contains("not supported", StringComparison.Ordinal)); + } + + [Fact] + public async Task Update_serializes_read_modify_write_operations_across_store_instances() + { + var firstStore = new WebhookRouteStore(_paths); + var secondStore = new WebhookRouteStore(_paths); + firstStore.Save("concurrent-route", CreateValidRoute()); + using var firstEntered = new ManualResetEventSlim(); + using var secondStarted = new ManualResetEventSlim(); + using var releaseFirst = new ManualResetEventSlim(); + var cancellationToken = TestContext.Current.CancellationToken; + + var first = Task.Run(() => firstStore.Update("concurrent-route", existing => + { + firstEntered.Set(); + Assert.True(secondStarted.Wait(TimeSpan.FromSeconds(10), cancellationToken)); + Assert.True(releaseFirst.Wait(TimeSpan.FromSeconds(10), cancellationToken)); + existing!.RateLimitPerMinute = 12; + return (existing, true); + }), cancellationToken); + Assert.True(firstEntered.Wait(TimeSpan.FromSeconds(10), cancellationToken)); + + var second = Task.Run(() => + { + secondStarted.Set(); + return secondStore.Update("concurrent-route", existing => + { + existing!.MaxBodyBytes = 2048; + return (existing, true); + }); + }, cancellationToken); + + Assert.True(secondStarted.Wait(TimeSpan.FromSeconds(10), cancellationToken)); + releaseFirst.Set(); + await Task.WhenAll(first, second); + + Assert.True(firstStore.TryGet("concurrent-route", out var saved)); + Assert.Equal(12, saved.Definition!.RateLimitPerMinute); + Assert.Equal(2048, saved.Definition.MaxBodyBytes); + } + [Fact] public void Embedded_config_and_route_schemas_share_timestamped_verification_contract() { @@ -237,6 +309,15 @@ public void TryParseVerifierKind_accepts_documented_and_config_spellings( Assert.Equal(expected, actual); } + [Theory] + [InlineData("0")] + [InlineData("1")] + [InlineData("2")] + public void TryParseVerifierKind_rejects_numeric_aliases(string value) + { + Assert.False(WebhookRouteValidator.TryParseVerifierKind(value, out _)); + } + private static WebhookRouteConfig CreateValidRoute() => new() { diff --git a/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json b/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json index 8516fb96b..63a94b234 100644 --- a/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json +++ b/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json @@ -940,13 +940,13 @@ "TimestampField": { "type": ["string", "null"], "minLength": 1, - "pattern": "^[^,=\\s](?:[^,=]*[^,=\\s])?$", + "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$", "default": "t" }, "SignatureField": { "type": ["string", "null"], "minLength": 1, - "pattern": "^[^,=\\s](?:[^,=]*[^,=\\s])?$", + "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$", "default": "v1" }, "SignedPayloadSeparator": { diff --git a/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json b/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json index a9b0caf1d..b9f76e34d 100644 --- a/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json +++ b/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json @@ -48,13 +48,13 @@ "TimestampField": { "type": ["string", "null"], "minLength": 1, - "pattern": "^[^,=\\s](?:[^,=]*[^,=\\s])?$", + "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$", "default": "t" }, "SignatureField": { "type": ["string", "null"], "minLength": 1, - "pattern": "^[^,=\\s](?:[^,=]*[^,=\\s])?$", + "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$", "default": "v1" }, "SignedPayloadSeparator": { diff --git a/src/Netclaw.Configuration/WebhookRouteStore.cs b/src/Netclaw.Configuration/WebhookRouteStore.cs index 1e3d8b0d4..afd7dcfcf 100644 --- a/src/Netclaw.Configuration/WebhookRouteStore.cs +++ b/src/Netclaw.Configuration/WebhookRouteStore.cs @@ -1,8 +1,10 @@ -// ----------------------------------------------------------------------- +// ----------------------------------------------------------------------- // // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using System.Security.Cryptography; +using System.Text; using System.Text.Json; using System.Text.Json.Serialization; using System.Text.RegularExpressions; @@ -99,12 +101,39 @@ public void Save(string routeName, WebhookRouteConfig definition) { lock (_sync) { - Directory.CreateDirectory(_paths.WebhooksDirectory); + var filePath = GetPath(routeName); + using var routeLock = AcquireRouteLock(filePath); + Write(filePath, definition); + } + } + /// + /// Reads and conditionally replaces one route while holding a route-scoped interprocess lock. + /// Returning a null definition leaves the file unchanged. + /// + public TResult Update( + string routeName, + Func update) + { + ArgumentNullException.ThrowIfNull(update); + + lock (_sync) + { var filePath = GetPath(routeName); - var tempPath = $"{filePath}.tmp"; - File.WriteAllText(tempPath, JsonSerializer.Serialize(definition, JsonOptions)); - File.Move(tempPath, filePath, overwrite: true); + using var routeLock = AcquireRouteLock(filePath); + WebhookRouteConfig? existing = null; + if (File.Exists(filePath)) + { + existing = TryRead(filePath); + if (existing is null) + throw new InvalidDataException($"Existing webhook route '{routeName}' could not be parsed."); + } + + var outcome = update(existing); + if (outcome.Definition is not null) + Write(filePath, outcome.Definition); + + return outcome.Result; } } @@ -113,6 +142,7 @@ public bool Delete(string routeName) lock (_sync) { var filePath = GetPath(routeName); + using var routeLock = AcquireRouteLock(filePath); if (!File.Exists(filePath)) return false; @@ -133,6 +163,68 @@ public bool Delete(string routeName) } } + private void Write(string filePath, WebhookRouteConfig definition) + { + Directory.CreateDirectory(_paths.WebhooksDirectory); + var tempPath = $"{filePath}.{Environment.ProcessId}.{Guid.NewGuid():N}.tmp"; + try + { + File.WriteAllText(tempPath, JsonSerializer.Serialize(definition, JsonOptions)); + File.Move(tempPath, filePath, overwrite: true); + } + finally + { + if (File.Exists(tempPath)) + File.Delete(tempPath); + } + } + + private static IDisposable AcquireRouteLock(string filePath) + { + var canonicalPath = Path.GetFullPath(filePath); + var lockId = Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(canonicalPath))); + var mutex = new Mutex(initiallyOwned: false, $"netclaw-webhook-route-{lockId}"); + + try + { + try + { + mutex.WaitOne(); + } + catch (AbandonedMutexException) + { + // The abandoning process exited while holding the route lock. This process now owns it, + // and the route's atomic file replacement guarantees the existing file is still complete. + DeleteAbandonedTempFiles(canonicalPath); + } + + return new RouteLock(mutex); + } + catch + { + mutex.Dispose(); + throw; + } + } + + private static void DeleteAbandonedTempFiles(string filePath) + { + var directory = Path.GetDirectoryName(filePath) + ?? throw new InvalidOperationException("Webhook route path has no parent directory."); + var fileName = Path.GetFileName(filePath); + foreach (var tempPath in Directory.EnumerateFiles(directory, $"{fileName}.*.tmp")) + File.Delete(tempPath); + } + + private sealed class RouteLock(Mutex mutex) : IDisposable + { + public void Dispose() + { + mutex.ReleaseMutex(); + mutex.Dispose(); + } + } + private string GetPath(string routeName) { var normalizedRouteName = NormalizeRouteName(routeName); diff --git a/src/Netclaw.Configuration/WebhookRouteValidator.cs b/src/Netclaw.Configuration/WebhookRouteValidator.cs index aacf3f48f..ac49cf12c 100644 --- a/src/Netclaw.Configuration/WebhookRouteValidator.cs +++ b/src/Netclaw.Configuration/WebhookRouteValidator.cs @@ -1,4 +1,4 @@ -// ----------------------------------------------------------------------- +// ----------------------------------------------------------------------- // // Copyright (C) 2026 - 2026 Petabridge, LLC // @@ -40,6 +40,12 @@ public static IReadOnlyList Validate(string routeName, WebhookRouteConfi if (route.Verification.Secret.IsNullOrEmpty()) errors.Add("Verification secret is required."); + if (!Enum.IsDefined(route.Verification.Kind)) + errors.Add($"Verification.Kind value '{(int)route.Verification.Kind}' is not supported."); + + if (!Enum.IsDefined(route.Verification.HmacAlgorithm)) + errors.Add($"Verification.HmacAlgorithm value '{(int)route.Verification.HmacAlgorithm}' is not supported."); + if (route.Verification.Kind == WebhookVerifierKind.HmacTimestamped) { if (route.Verification.ToleranceSeconds is < 1 or > 3600) @@ -129,11 +135,14 @@ private static void ValidateStructuredHeaderField( return; } - if (!string.Equals(value, value.Trim(), StringComparison.Ordinal)) - errors.Add($"Verification.{propertyName} cannot have leading or trailing whitespace."); - - if (value.Contains(',', StringComparison.Ordinal) - || value.Contains('=', StringComparison.Ordinal)) - errors.Add($"Verification.{propertyName} cannot contain ',' or '='."); + if (!value.All(IsHttpTokenCharacter)) + errors.Add($"Verification.{propertyName} must contain only HTTP token characters."); } + + private static bool IsHttpTokenCharacter(char value) + => value is >= '0' and <= '9' + or >= 'A' and <= 'Z' + or >= 'a' and <= 'z' + or '!' or '#' or '$' or '%' or '&' or '\'' or '*' or '+' or '-' + or '.' or '^' or '_' or '`' or '|' or '~'; } diff --git a/src/Netclaw.Daemon.Tests/Webhooks/WebhookRouteCatalogTests.cs b/src/Netclaw.Daemon.Tests/Webhooks/WebhookRouteCatalogTests.cs index 679d3d66f..695ead035 100644 --- a/src/Netclaw.Daemon.Tests/Webhooks/WebhookRouteCatalogTests.cs +++ b/src/Netclaw.Daemon.Tests/Webhooks/WebhookRouteCatalogTests.cs @@ -1,4 +1,4 @@ -// ----------------------------------------------------------------------- +// ----------------------------------------------------------------------- // // Copyright (C) 2026 - 2026 Petabridge, LLC // @@ -81,6 +81,26 @@ public void Unknown_future_verifier_invalidates_only_that_route() Assert.Contains(_sink.Alerts, alert => alert.Category == AlertType.WebhookRouteInvalid); } + [Fact] + public void Undefined_numeric_verifier_invalidates_only_that_route() + { + WriteRouteFile("valid-route", CreateRoute()); + WriteRouteText("numeric-route", """ +{ + "Prompt": "process invalid numeric verifier", + "Verification": { + "Kind": 99, + "Secret": "secret" + } +} +"""); + var sut = CreateCatalog(); + + Assert.False(sut.TryGetRoute("numeric-route", out _)); + Assert.True(sut.TryGetRoute("valid-route", out _)); + Assert.Contains(_sink.Alerts, alert => alert.Category == AlertType.WebhookRouteInvalid); + } + [Fact] public void Invalid_edit_removes_previously_loaded_route() { From f90af77741a2c04442368e32b6958a5112ea567a Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 15 Jul 2026 20:25:51 +0000 Subject: [PATCH 04/10] fix(daemon): make restart drain tests deterministic --- .../Services/DaemonRestartCoordinatorTests.cs | 222 +++++++++++++----- src/Netclaw.Daemon/Program.cs | 8 +- .../Services/DaemonRestartCoordinator.cs | 14 +- .../Services/SessionDrainHelper.cs | 16 +- 4 files changed, 182 insertions(+), 78 deletions(-) diff --git a/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs b/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs index 8e88ee701..3e653de4c 100644 --- a/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs +++ b/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs @@ -1,12 +1,14 @@ -// ----------------------------------------------------------------------- +// ----------------------------------------------------------------------- // // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using System.Collections.Concurrent; using Akka.Actor; using Akka.Hosting; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Time.Testing; using Netclaw.Actors.Channels; using Netclaw.Actors.Hosting; using Netclaw.Actors.Protocol; @@ -18,8 +20,10 @@ namespace Netclaw.Daemon.Tests.Services; -public sealed class DaemonRestartCoordinatorTests : IDisposable +public sealed class DaemonRestartCoordinatorTests : IAsyncDisposable { + private static readonly TimeSpan DrainTimeout = TimeSpan.FromSeconds(20); + private readonly DisposableTempDir _dir = new(); private readonly ActorSystem _system; private readonly NetclawPaths _paths; @@ -38,9 +42,16 @@ public DaemonRestartCoordinatorTests() [Fact] public async Task RequestConfigRestartAsync_drains_active_sessions_and_requests_stop() { - var coordinator = CreateCoordinator(["slack/C123.1", "slack/C123.2"], restartDrainTimeout: TimeSpan.FromMilliseconds(250)); + var time = new FakeTimeProvider(); + var (coordinator, drain) = CreateCoordinator( + ["slack/C123.1", "slack/C123.2"], + timeProvider: time); - await coordinator.RequestConfigRestartAsync(CancellationToken.None); + var restart = coordinator.RequestConfigRestartAsync(CancellationToken.None); + await drain.AllRequestsObserved; + drain.AcknowledgeAll(); + await drain.AllAcknowledgementsSent; + await restart; Assert.True(_restartSignal.RestartRequested); Assert.True(_appLifetime.StopRequested); @@ -59,12 +70,18 @@ public async Task RequestConfigRestartAsync_drains_active_sessions_and_requests_ [Fact] public async Task RequestConfigRestartAsync_records_timed_out_sessions() { - var coordinator = CreateCoordinator( + var time = new FakeTimeProvider(); + var (coordinator, drain) = CreateCoordinator( ["slack/C123.1", "slack/C123.2"], timedOutSessionIds: ["slack/C123.2"], - restartDrainTimeout: TimeSpan.FromSeconds(3)); + timeProvider: time); - await coordinator.RequestConfigRestartAsync(CancellationToken.None); + var restart = coordinator.RequestConfigRestartAsync(CancellationToken.None); + await drain.AllRequestsObserved; + drain.AcknowledgeAll(); + await drain.AllAcknowledgementsSent; + time.Advance(DrainTimeout); + await restart; Assert.True(_restartSignal.RestartRequested); Assert.True(_appLifetime.StopRequested); @@ -78,62 +95,123 @@ public async Task RequestConfigRestartAsync_records_timed_out_sessions() Assert.Equal("1", alert.Context["timedOutSessions"]); } + [Fact] + public async Task RequestConfigRestartAsync_propagates_caller_cancellation_and_reopens_ingress() + { + var time = new FakeTimeProvider(); + var (coordinator, drain) = CreateCoordinator( + ["slack/C123.1"], + timedOutSessionIds: ["slack/C123.1"], + timeProvider: time); + using var callerCts = new CancellationTokenSource(); + + var restart = coordinator.RequestConfigRestartAsync(callerCts.Token); + await drain.AllRequestsObserved; + callerCts.Cancel(); + + await Assert.ThrowsAnyAsync(() => restart); + Assert.False(_restartSignal.RestartRequested); + Assert.False(_appLifetime.StopRequested); + Assert.Null(_ingressGate.ClosedReason); + } + [Fact] public async Task RequestConfigRestartAsync_reopens_ingress_when_coordination_fails() { - // Drain timeout is intentionally wide. The test asserts that the - // stub's InvalidOperationException propagates and reopens the - // ingress gate; a tight timeout (100ms) races with the Ask on - // slow CI runners and yields TaskCanceledException instead, - // making this test flake. The timeout is irrelevant to the - // behavior under test. - var coordinator = CreateCoordinator([], throwOnEnumeration: true, restartDrainTimeout: TimeSpan.FromSeconds(30)); + var time = new FakeTimeProvider(); + var (coordinator, _) = CreateCoordinator( + [], + throwOnEnumeration: true, + timeProvider: time); - await Assert.ThrowsAsync(() => coordinator.RequestConfigRestartAsync(CancellationToken.None)); + await Assert.ThrowsAsync( + () => coordinator.RequestConfigRestartAsync(CancellationToken.None)); Assert.False(_restartSignal.RestartRequested); Assert.False(_appLifetime.StopRequested); Assert.Null(_ingressGate.ClosedReason); } - public void Dispose() + [Fact] + public async Task SessionDrainHelper_queries_manager_drains_sessions_and_reports_timeouts() + { + var time = new FakeTimeProvider(); + var activeIds = new[] { "slack/drain-ok", "slack/drain-timeout" }; + var timedOut = new[] { "slack/drain-timeout" }; + var drain = new DrainControl(activeIds, timedOut); + var sessionManager = _system.ActorOf(Props.Create(() => new StubSessionManagerActor( + activeIds, + drain, + throwOnEnumeration: false))); + using var deadlineCts = new CancellationTokenSource(DrainTimeout, time); + + var operation = SessionDrainHelper.DrainAsync( + sessionManager, + "integration-test", + NullLogger.Instance, + deadlineCts.Token, + CancellationToken.None); + await drain.AllRequestsObserved; + drain.AcknowledgeAll(); + await drain.AllAcknowledgementsSent; + time.Advance(DrainTimeout); + + var result = await operation; + + Assert.Equal(2, result.AllSessionIds.Count); + Assert.Single(result.DrainedSessionIds); + Assert.Equal("slack/drain-ok", result.DrainedSessionIds[0].Value); + Assert.Single(result.TimedOutSessionIds); + Assert.Equal("slack/drain-timeout", result.TimedOutSessionIds[0].Value); + + var context = result.ToNotificationContext(); + Assert.Equal("timeout", context["drainOutcome"]); + Assert.Equal("2", context["activeSessions"]); + Assert.Equal("1", context["drainedSessions"]); + Assert.Equal("1", context["timedOutSessions"]); + } + + public async ValueTask DisposeAsync() { - _system.Terminate().GetAwaiter().GetResult(); + await _system.Terminate(); _dir.Dispose(); } - private DaemonRestartCoordinator CreateCoordinator( + private (DaemonRestartCoordinator Coordinator, DrainControl Drain) CreateCoordinator( IReadOnlyList activeSessionIds, IReadOnlyList? timedOutSessionIds = null, bool throwOnEnumeration = false, - TimeSpan? restartDrainTimeout = null) + FakeTimeProvider? timeProvider = null) { + var timedOut = timedOutSessionIds ?? Array.Empty(); + var drain = new DrainControl(activeSessionIds, timedOut); var sessionManager = _system.ActorOf(Props.Create(() => new StubSessionManagerActor( activeSessionIds, - timedOutSessionIds ?? Array.Empty(), + drain, throwOnEnumeration))); - var notifier = new DaemonLifecycleNotifier(_sink, TimeProvider.System, NullLogger.Instance); + var time = timeProvider ?? new FakeTimeProvider(); + var notifier = new DaemonLifecycleNotifier( + _sink, + time, + NullLogger.Instance); - return new DaemonRestartCoordinator( + var coordinator = new DaemonRestartCoordinator( _ingressGate, new RestartManifestStore(_paths), new StubRequiredActor(sessionManager), _restartSignal, _appLifetime, notifier, - TimeProvider.System, + time, NullLogger.Instance, - restartDrainTimeout); + DrainTimeout); + + return (coordinator, drain); } - private sealed class StubRequiredActor : IRequiredActor + private sealed class StubRequiredActor(IActorRef actorRef) : IRequiredActor { - public StubRequiredActor(IActorRef actorRef) - { - ActorRef = actorRef; - } - - public IActorRef ActorRef { get; } + public IActorRef ActorRef { get; } = actorRef; public Task GetAsync(CancellationToken cancellationToken = default) => Task.FromResult(ActorRef); @@ -143,7 +221,7 @@ private sealed class StubSessionManagerActor : ReceiveActor { public StubSessionManagerActor( IReadOnlyList activeSessionIds, - IReadOnlyList timedOutSessionIds, + DrainControl drain, bool throwOnEnumeration) { Receive(_ => @@ -157,48 +235,62 @@ public StubSessionManagerActor( Sender.Tell(new ActiveEntityIds(activeSessionIds)); }); - Receive(msg => - { - if (timedOutSessionIds.Contains(msg.SessionId.Value, StringComparer.Ordinal)) - return; - - Sender.Tell(CommandAck.For(msg.SessionId)); - }); + Receive(msg => drain.Observe(msg, Sender)); } } - [Fact] - public async Task SessionDrainHelper_queries_manager_drains_sessions_and_reports_timeouts() + private sealed class DrainControl { - // One session acks normally, the other times out - var activeIds = new[] { "slack/drain-ok", "slack/drain-timeout" }; - var timedOut = new[] { "slack/drain-timeout" }; - var sessionManager = _system.ActorOf(Props.Create(() => new StubSessionManagerActor( - activeIds, timedOut, false))); + private readonly HashSet _timedOutSessionIds; + private readonly ConcurrentDictionary _pendingRequests = new(StringComparer.Ordinal); + private readonly TaskCompletionSource _allRequestsObserved = + new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TaskCompletionSource _allAcknowledgementsSent = + new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly int _expectedRequestCount; + private readonly int _expectedAcknowledgementCount; + private int _requestCount; + private int _acknowledgementCount; + + public DrainControl( + IReadOnlyList activeSessionIds, + IReadOnlyList timedOutSessionIds) + { + _timedOutSessionIds = timedOutSessionIds.ToHashSet(StringComparer.Ordinal); + _expectedRequestCount = activeSessionIds.Count; + _expectedAcknowledgementCount = activeSessionIds.Count - _timedOutSessionIds.Count; + + if (_expectedRequestCount == 0) + _allRequestsObserved.SetResult(); + if (_expectedAcknowledgementCount == 0) + _allAcknowledgementsSent.SetResult(); + } - using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(3)); + public Task AllRequestsObserved => _allRequestsObserved.Task; - var result = await SessionDrainHelper.DrainAsync( - sessionManager, - "integration-test", - NullLogger.Instance, - cts.Token); + public Task AllAcknowledgementsSent => _allAcknowledgementsSent.Task; - // All sessions were discovered - Assert.Equal(2, result.AllSessionIds.Count); + public void Observe(PrepareForDaemonRestart request, IActorRef replyTo) + { + if (!_pendingRequests.TryAdd(request.SessionId.Value, replyTo)) + throw new InvalidOperationException($"Duplicate drain request for {request.SessionId.Value}."); - // One drained, one timed out - Assert.Single(result.DrainedSessionIds); - Assert.Equal("slack/drain-ok", result.DrainedSessionIds[0].Value); - Assert.Single(result.TimedOutSessionIds); - Assert.Equal("slack/drain-timeout", result.TimedOutSessionIds[0].Value); + if (Interlocked.Increment(ref _requestCount) == _expectedRequestCount) + _allRequestsObserved.TrySetResult(); + } - // Notification context reflects the outcome - var ctx = result.ToNotificationContext(); - Assert.Equal("timeout", ctx["drainOutcome"]); - Assert.Equal("2", ctx["activeSessions"]); - Assert.Equal("1", ctx["drainedSessions"]); - Assert.Equal("1", ctx["timedOutSessions"]); + public void AcknowledgeAll() + { + foreach (var (sessionId, replyTo) in _pendingRequests) + { + if (_timedOutSessionIds.Contains(sessionId)) + continue; + + replyTo.Tell(CommandAck.For(new SessionId(sessionId))); + if (Interlocked.Increment(ref _acknowledgementCount) == _expectedAcknowledgementCount) + _allAcknowledgementsSent.TrySetResult(); + } + } } private sealed class FakeApplicationLifetime : IHostApplicationLifetime diff --git a/src/Netclaw.Daemon/Program.cs b/src/Netclaw.Daemon/Program.cs index 500418b71..a81e6eb59 100644 --- a/src/Netclaw.Daemon/Program.cs +++ b/src/Netclaw.Daemon/Program.cs @@ -1,4 +1,4 @@ -// ----------------------------------------------------------------------- +// ----------------------------------------------------------------------- // // Copyright (C) 2026 - 2026 Petabridge, LLC // @@ -1074,7 +1074,11 @@ static void ConfigureDaemonServices( try { var drainResult = await SessionDrainHelper.DrainAsync( - sessionManager, "daemon-stop", drainLogger, CancellationToken.None); + sessionManager, + "daemon-stop", + drainLogger, + CancellationToken.None, + CancellationToken.None); lifecycleNotifier.NotifyShutdown("daemon-stop", drainResult.ToNotificationContext()); } diff --git a/src/Netclaw.Daemon/Services/DaemonRestartCoordinator.cs b/src/Netclaw.Daemon/Services/DaemonRestartCoordinator.cs index 92a7e4b43..ecf60bf36 100644 --- a/src/Netclaw.Daemon/Services/DaemonRestartCoordinator.cs +++ b/src/Netclaw.Daemon/Services/DaemonRestartCoordinator.cs @@ -1,4 +1,4 @@ -// ----------------------------------------------------------------------- +// ----------------------------------------------------------------------- // // Copyright (C) 2026 - 2026 Petabridge, LLC // @@ -70,11 +70,17 @@ public async Task RequestConfigRestartAsync(CancellationToken cancellationToken) { var sessionManager = await _sessionManagerProvider.GetAsync(cancellationToken); - using var drainCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - drainCts.CancelAfter(_restartDrainTimeout); + using var deadlineCts = new CancellationTokenSource(_restartDrainTimeout, _timeProvider); + using var drainCts = CancellationTokenSource.CreateLinkedTokenSource( + cancellationToken, + deadlineCts.Token); var drainResult = await SessionDrainHelper.DrainAsync( - sessionManager, "config-reload", _logger, drainCts.Token); + sessionManager, + "config-reload", + _logger, + drainCts.Token, + cancellationToken); var manifest = new RestartManifest { diff --git a/src/Netclaw.Daemon/Services/SessionDrainHelper.cs b/src/Netclaw.Daemon/Services/SessionDrainHelper.cs index eade99adb..55270b7a4 100644 --- a/src/Netclaw.Daemon/Services/SessionDrainHelper.cs +++ b/src/Netclaw.Daemon/Services/SessionDrainHelper.cs @@ -1,4 +1,4 @@ -// ----------------------------------------------------------------------- +// ----------------------------------------------------------------------- // // Copyright (C) 2026 - 2026 Petabridge, LLC // @@ -23,19 +23,21 @@ internal static class SessionDrainHelper /// to each in parallel, and waits for acknowledgement or cancellation. /// /// - /// Callers control the timeout by providing a from a - /// with the desired deadline. + /// Callers control the timeout through . + /// distinguishes an expired drain deadline, + /// which records an undrained session, from an explicit caller cancellation, which propagates. /// public static async Task DrainAsync( IActorRef sessionManager, string reason, ILogger logger, - CancellationToken cancellationToken) + CancellationToken operationCancellationToken, + CancellationToken callerCancellationToken) { var activeIdsResponse = await sessionManager.Ask( GetActiveEntityIds.Instance, timeout: Timeout.InfiniteTimeSpan, - cancellationToken: cancellationToken); + cancellationToken: operationCancellationToken); var sessionIds = activeIdsResponse.EntityIds .Select(id => new SessionId(id)) @@ -55,11 +57,11 @@ public static async Task DrainAsync( var ack = await sessionManager.Ask( new PrepareForDaemonRestart(sessionId, reason), timeout: Timeout.InfiniteTimeSpan, - cancellationToken: cancellationToken); + cancellationToken: operationCancellationToken); return new DrainOutcome(sessionId, ack.SessionId == sessionId); } - catch (OperationCanceledException) + catch (OperationCanceledException) when (!callerCancellationToken.IsCancellationRequested) { return new DrainOutcome(sessionId, false); } From ea2fab2a65e7f60d0f55bc81d38b038e05e09511 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 15 Jul 2026 20:48:13 +0000 Subject: [PATCH 05/10] fix: address webhook and restart review findings --- src/Netclaw.Actors/Tools/SetWebhookTool.cs | 5 + .../Webhooks/WebhooksCommandTests.cs | 72 ++++ src/Netclaw.Cli/Webhooks/WebhooksCommand.cs | 356 +++++++++--------- .../WebhookRouteStoreTests.cs | 115 ++++-- .../Schemas/netclaw-config.v1.schema.json | 35 +- .../Schemas/webhook-route.v1.schema.json | 35 +- .../WebhookRouteStore.cs | 74 +++- .../Services/DaemonRestartCoordinatorTests.cs | 42 ++- .../Services/SessionDrainHelper.cs | 4 + 9 files changed, 504 insertions(+), 234 deletions(-) diff --git a/src/Netclaw.Actors/Tools/SetWebhookTool.cs b/src/Netclaw.Actors/Tools/SetWebhookTool.cs index 9cc9b8fb7..fadf30e5e 100644 --- a/src/Netclaw.Actors/Tools/SetWebhookTool.cs +++ b/src/Netclaw.Actors/Tools/SetWebhookTool.cs @@ -91,6 +91,7 @@ protected override Task ExecuteAsync(Params args, ToolInvocationContext { var result = _store.Update( routeName, + ct, existing => BuildUpdate(routeName, args, context.Audience, verificationKind, existing)); return Task.FromResult(result); } @@ -98,6 +99,10 @@ protected override Task ExecuteAsync(Params args, ToolInvocationContext { return Task.FromResult($"Error: {ex.Message}"); } + catch (TimeoutException ex) + { + return Task.FromResult($"Error: {ex.Message}"); + } } private static (WebhookRouteConfig? Definition, string Result) BuildUpdate( diff --git a/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs b/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs index 244779f29..4ec8344e3 100644 --- a/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs @@ -4,6 +4,8 @@ // // ----------------------------------------------------------------------- using System.Text.Json; +using System.Text.Json.Nodes; +using Json.Schema; using Netclaw.Cli.Json; using Netclaw.Cli.Webhooks; using Netclaw.Configuration; @@ -536,6 +538,21 @@ public async Task Set_Unrelated_update_preserves_legacy_verifier_without_timesta Assert.DoesNotContain("TimestampField", json, StringComparison.Ordinal); } + [Fact] + public async Task Set_MalformedExistingRoute_ReturnsOneWithoutOverwriting() + { + WriteRouteText("malformed-route", "{"); + + var result = await WebhooksCommand.RunAsync([ + "webhooks", "set", "malformed-route", + "--prompt", "updated prompt", + "--secret", "updated-secret" + ], _paths); + + Assert.Equal(1, result); + Assert.Equal("{", File.ReadAllText(Path.Combine(_paths.WebhooksDirectory, "malformed-route.json"))); + } + [Fact] public async Task Show_Json_adds_timestamp_fields_only_for_timestamped_kind() { @@ -611,6 +628,52 @@ public async Task Validate_ValidRoute_ReturnsZero() Assert.Equal(0, result); } + [Fact] + public async Task Validate_TimestampedRoute_UsesDocumentedKindSpelling() + { + CreateValidRoute("timestamped-route"); + var route = ReadRoute("timestamped-route"); + route.Verification.Kind = WebhookVerifierKind.HmacTimestamped; + new WebhookRouteStore(_paths).Save("timestamped-route", route); + using var output = new StringWriter(); + + var result = await WebhooksCommand.RunAsync( + ["webhooks", "validate", "timestamped-route"], + _paths, + output); + + Assert.Equal(0, result); + Assert.Contains("Verification: hmac-timestamped", output.ToString(), StringComparison.Ordinal); + } + + [Theory] + [InlineData("Hmac", 0, "not valid", true)] + [InlineData("HmacTimestamped", 0, "t", false)] + [InlineData("HmacTimestamped", 300, "not valid", false)] + [InlineData("HmacTimestamped", 300, "t", true)] + public void RouteSchema_applies_timestamp_constraints_only_to_timestamped_kind( + string kind, + int toleranceSeconds, + string timestampField, + bool expectedValid) + { + var schema = JsonSchema.FromText(LoadRouteSchema()); + var route = new JsonObject + { + ["Prompt"] = "process delivery", + ["Verification"] = new JsonObject + { + ["Kind"] = kind, + ["ToleranceSeconds"] = toleranceSeconds, + ["TimestampField"] = timestampField + } + }; + + var evaluation = schema.Evaluate(route); + + Assert.Equal(expectedValid, evaluation.IsValid); + } + [Fact] public async Task Validate_NonexistentRoute_ReturnsOne() { @@ -698,6 +761,15 @@ private void WriteRouteText(string routeName, string text) File.WriteAllText(Path.Combine(_paths.WebhooksDirectory, $"{routeName}.json"), text); } + private static string LoadRouteSchema() + { + using var stream = typeof(EmbeddedSchemaLoader).Assembly.GetManifestResourceStream( + "Netclaw.Configuration.Schemas.webhook-route.v1.schema.json"); + Assert.NotNull(stream); + using var reader = new StreamReader(stream!); + return reader.ReadToEnd(); + } + private sealed class RouteListItem { public string Name { get; set; } = string.Empty; diff --git a/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs b/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs index 6226c4e3c..53a0f0e69 100644 --- a/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs +++ b/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs @@ -286,243 +286,249 @@ private static int RunSet(string[] args, WebhookRouteStore store, NetclawPaths p return 1; } + if (!TryResolveTextInput(args, "--prompt", "--prompt-file", out var prompt, out var hasPrompt)) + return 1; + + if (!TryResolveSecret(args, out var secret, out var hasSecret)) + return 1; + + if (!TryResolveTextInput(args, "--notify-instructions", "--notify-instructions-file", out var notifyInstructions, out var hasNotifyInstructions)) + return 1; + var routeSaved = false; var updatedExistingRoute = false; - var result = store.Update(routeName, existing => + int result; + try { - var exists = existing is not null; - - if (createOnly && exists) - { - Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' already exists (--create-only specified)."); - return (null, 1); - } - - if (updateOnly && !exists) + result = store.Update(routeName, CancellationToken.None, existing => { - Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' does not exist (--update-only specified)."); - return (null, 1); - } + var exists = existing is not null; - // Start with existing config or defaults - var route = existing ?? new WebhookRouteConfig(); - route.Verification ??= new WebhookVerificationConfig(); - route.Events ??= []; + if (createOnly && exists) + { + Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' already exists (--create-only specified)."); + return (null, 1); + } - // Parse prompt - if (!TryResolveTextInput(args, "--prompt", "--prompt-file", out var prompt, out var hasPrompt)) - return (null, 1); + if (updateOnly && !exists) + { + Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' does not exist (--update-only specified)."); + return (null, 1); + } - if (hasPrompt) - route.Prompt = prompt; + // Start with existing config or defaults + var route = existing ?? new WebhookRouteConfig(); + route.Verification ??= new WebhookVerificationConfig(); + route.Events ??= []; - // Parse secret - if (!TryResolveSecret(args, out var secret, out var hasSecret)) - return (null, 1); + if (hasPrompt) + route.Prompt = prompt; - if (hasSecret) - route.Verification.Secret = new SensitiveString(secret); + if (hasSecret) + route.Verification.Secret = new SensitiveString(secret); - // Parse verification kind - if (!TryGetFlagValue(args, "--verification-kind", out var verificationKind, out var hasVerificationKind)) - return (null, 1); + // Parse verification kind + if (!TryGetFlagValue(args, "--verification-kind", out var verificationKind, out var hasVerificationKind)) + return (null, 1); - if (hasVerificationKind) - { - if (!WebhookRouteValidator.TryParseVerifierKind(verificationKind, out var kind)) + if (hasVerificationKind) { - Console.Error.WriteLine($"[FAIL] Invalid verification kind: '{verificationKind}'. Use 'hmac', 'hmac-timestamped', or 'header-secret'."); - return (null, 1); + if (!WebhookRouteValidator.TryParseVerifierKind(verificationKind, out var kind)) + { + Console.Error.WriteLine($"[FAIL] Invalid verification kind: '{verificationKind}'. Use 'hmac', 'hmac-timestamped', or 'header-secret'."); + return (null, 1); + } + route.Verification.Kind = kind; } - route.Verification.Kind = kind; - } - // Parse verification headers - if (!TryGetFlagValue(args, "--signature-header", out var signatureHeader, out var hasSignatureHeader)) - return (null, 1); + // Parse verification headers + if (!TryGetFlagValue(args, "--signature-header", out var signatureHeader, out var hasSignatureHeader)) + return (null, 1); + + if (hasSignatureHeader) + route.Verification.SignatureHeaderName = signatureHeader; + + if (!TryGetFlagValue(args, "--signature-prefix", out var signaturePrefix, out var hasSignaturePrefix)) + return (null, 1); - if (hasSignatureHeader) - route.Verification.SignatureHeaderName = signatureHeader; + if (hasSignaturePrefix) + route.Verification.SignaturePrefix = signaturePrefix; - if (!TryGetFlagValue(args, "--signature-prefix", out var signaturePrefix, out var hasSignaturePrefix)) - return (null, 1); + if (!TryGetFlagValue(args, "--secret-header", out var secretHeader, out var hasSecretHeader)) + return (null, 1); - if (hasSignaturePrefix) - route.Verification.SignaturePrefix = signaturePrefix; + if (hasSecretHeader) + route.Verification.SecretHeaderName = secretHeader; - if (!TryGetFlagValue(args, "--secret-header", out var secretHeader, out var hasSecretHeader)) - return (null, 1); + if (!TryGetFlagValue(args, "--event-header", out var eventHeader, out var hasEventHeader)) + return (null, 1); - if (hasSecretHeader) - route.Verification.SecretHeaderName = secretHeader; + if (hasEventHeader) + route.Verification.EventHeaderName = eventHeader; - if (!TryGetFlagValue(args, "--event-header", out var eventHeader, out var hasEventHeader)) - return (null, 1); + if (!TryGetFlagValue(args, "--delivery-header", out var deliveryHeader, out var hasDeliveryHeader)) + return (null, 1); - if (hasEventHeader) - route.Verification.EventHeaderName = eventHeader; + if (hasDeliveryHeader) + route.Verification.DeliveryIdHeaderName = deliveryHeader; - if (!TryGetFlagValue(args, "--delivery-header", out var deliveryHeader, out var hasDeliveryHeader)) - return (null, 1); + if (!TryGetFlagValue(args, "--timestamp-field", out var timestampField, out var hasTimestampField)) + return (null, 1); - if (hasDeliveryHeader) - route.Verification.DeliveryIdHeaderName = deliveryHeader; + if (hasTimestampField) + route.Verification.TimestampField = timestampField; - if (!TryGetFlagValue(args, "--timestamp-field", out var timestampField, out var hasTimestampField)) - return (null, 1); + if (!TryGetFlagValue(args, "--signature-field", out var signatureField, out var hasSignatureField)) + return (null, 1); - if (hasTimestampField) - route.Verification.TimestampField = timestampField; + if (hasSignatureField) + route.Verification.SignatureField = signatureField; - if (!TryGetFlagValue(args, "--signature-field", out var signatureField, out var hasSignatureField)) - return (null, 1); + if (!TryGetFlagValue(args, "--signed-payload-separator", out var payloadSeparator, out var hasPayloadSeparator)) + return (null, 1); - if (hasSignatureField) - route.Verification.SignatureField = signatureField; + if (hasPayloadSeparator) + route.Verification.SignedPayloadSeparator = payloadSeparator; - if (!TryGetFlagValue(args, "--signed-payload-separator", out var payloadSeparator, out var hasPayloadSeparator)) - return (null, 1); + if (!TryGetFlagValue(args, "--signature-tolerance-seconds", out var tolerance, out var hasTolerance)) + return (null, 1); - if (hasPayloadSeparator) - route.Verification.SignedPayloadSeparator = payloadSeparator; + if (hasTolerance) + { + if (!int.TryParse(tolerance, out var toleranceSeconds)) + { + Console.Error.WriteLine($"[FAIL] Invalid signature tolerance: '{tolerance}'. Must be a whole number from 1 to 3600."); + return (null, 1); + } - if (!TryGetFlagValue(args, "--signature-tolerance-seconds", out var tolerance, out var hasTolerance)) - return (null, 1); + route.Verification.ToleranceSeconds = toleranceSeconds; + } - if (hasTolerance) - { - if (!int.TryParse(tolerance, out var toleranceSeconds)) + if ((hasTimestampField || hasSignatureField || hasPayloadSeparator || hasTolerance) + && route.Verification.Kind != WebhookVerifierKind.HmacTimestamped) { - Console.Error.WriteLine($"[FAIL] Invalid signature tolerance: '{tolerance}'. Must be a whole number from 1 to 3600."); + Console.Error.WriteLine("[FAIL] Timestamp signature options require '--verification-kind hmac-timestamped'."); return (null, 1); } - route.Verification.ToleranceSeconds = toleranceSeconds; - } + // Parse events + if (!TryGetFlagValue(args, "--events", out var events, out var hasEvents)) + return (null, 1); - if ((hasTimestampField || hasSignatureField || hasPayloadSeparator || hasTolerance) - && route.Verification.Kind != WebhookVerifierKind.HmacTimestamped) - { - Console.Error.WriteLine("[FAIL] Timestamp signature options require '--verification-kind hmac-timestamped'."); - return (null, 1); - } + if (hasEvents) + { + route.Events = [.. events.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)]; + } - // Parse events - if (!TryGetFlagValue(args, "--events", out var events, out var hasEvents)) - return (null, 1); + // Parse audience + if (!TryGetFlagValue(args, "--audience", out var audience, out var hasAudience)) + return (null, 1); - if (hasEvents) - { - route.Events = [.. events.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)]; - } + if (hasAudience) + { + if (!Enum.TryParse(audience, ignoreCase: true, out var aud)) + { + Console.Error.WriteLine($"[FAIL] Invalid audience: '{audience}'. Use 'public', 'team', or 'personal'."); + return (null, 1); + } + route.Audience = aud; + } - // Parse audience - if (!TryGetFlagValue(args, "--audience", out var audience, out var hasAudience)) - return (null, 1); + if (hasNotifyInstructions) + route.NotifyInstructions = notifyInstructions; - if (hasAudience) - { - if (!Enum.TryParse(audience, ignoreCase: true, out var aud)) + var deliveryRequired = HasFlag(args, "--delivery-required"); + var noDeliveryRequired = HasFlag(args, "--no-delivery-required"); + if (deliveryRequired && noDeliveryRequired) { - Console.Error.WriteLine($"[FAIL] Invalid audience: '{audience}'. Use 'public', 'team', or 'personal'."); + Console.Error.WriteLine("[FAIL] --delivery-required and --no-delivery-required cannot be used together."); return (null, 1); } - route.Audience = aud; - } - // Parse notification settings - if (!TryResolveTextInput(args, "--notify-instructions", "--notify-instructions-file", out var notifyInstructions, out var hasNotifyInstructions)) - return (null, 1); + if (deliveryRequired) + route.DeliveryRequired = true; + if (noDeliveryRequired) + route.DeliveryRequired = false; - if (hasNotifyInstructions) - route.NotifyInstructions = notifyInstructions; + if (!TryGetFlagValue(args, "--notification-channel", out var notificationChannel, out var hasNotificationChannel)) + return (null, 1); - var deliveryRequired = HasFlag(args, "--delivery-required"); - var noDeliveryRequired = HasFlag(args, "--no-delivery-required"); - if (deliveryRequired && noDeliveryRequired) - { - Console.Error.WriteLine("[FAIL] --delivery-required and --no-delivery-required cannot be used together."); - return (null, 1); - } + if (hasNotificationChannel) + { + route.NotificationTarget ??= new NotificationTargetConfig(); + route.NotificationTarget.ChannelId = notificationChannel; + } - if (deliveryRequired) - route.DeliveryRequired = true; - if (noDeliveryRequired) - route.DeliveryRequired = false; + // Parse limits + if (!TryGetFlagValue(args, "--max-body", out var maxBody, out var hasMaxBody)) + return (null, 1); - if (!TryGetFlagValue(args, "--notification-channel", out var notificationChannel, out var hasNotificationChannel)) - return (null, 1); + if (hasMaxBody) + { + if (!int.TryParse(maxBody, out var bytes) || bytes < 1) + { + Console.Error.WriteLine($"[FAIL] Invalid max body size: '{maxBody}'. Must be a positive integer."); + return (null, 1); + } + route.MaxBodyBytes = bytes; + } - if (hasNotificationChannel) - { - route.NotificationTarget ??= new NotificationTargetConfig(); - route.NotificationTarget.ChannelId = notificationChannel; - } + if (!TryGetFlagValue(args, "--rate-limit", out var rateLimit, out var hasRateLimit)) + return (null, 1); - // Parse limits - if (!TryGetFlagValue(args, "--max-body", out var maxBody, out var hasMaxBody)) - return (null, 1); + if (hasRateLimit) + { + if (!int.TryParse(rateLimit, out var limit) || limit < 1) + { + Console.Error.WriteLine($"[FAIL] Invalid rate limit: '{rateLimit}'. Must be a positive integer."); + return (null, 1); + } + route.RateLimitPerMinute = limit; + } - if (hasMaxBody) - { - if (!int.TryParse(maxBody, out var bytes) || bytes < 1) + // Parse enabled/disabled + var enabled = HasFlag(args, "--enabled"); + var disabled = HasFlag(args, "--disabled"); + if (enabled && disabled) { - Console.Error.WriteLine($"[FAIL] Invalid max body size: '{maxBody}'. Must be a positive integer."); + Console.Error.WriteLine("[FAIL] --enabled and --disabled cannot be used together."); return (null, 1); } - route.MaxBodyBytes = bytes; - } - if (!TryGetFlagValue(args, "--rate-limit", out var rateLimit, out var hasRateLimit)) - return (null, 1); + if (enabled) + route.Enabled = true; + if (disabled) + route.Enabled = false; - if (hasRateLimit) - { - if (!int.TryParse(rateLimit, out var limit) || limit < 1) + // Validate + var errors = WebhookRouteValidator.Validate(routeName, route); + if (errors.Count > 0) { - Console.Error.WriteLine($"[FAIL] Invalid rate limit: '{rateLimit}'. Must be a positive integer."); + Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' has validation errors:"); + foreach (var error in errors) + { + Console.Error.WriteLine($" - {error}"); + } return (null, 1); } - route.RateLimitPerMinute = limit; - } - - // Parse enabled/disabled - var enabled = HasFlag(args, "--enabled"); - var disabled = HasFlag(args, "--disabled"); - if (enabled && disabled) - { - Console.Error.WriteLine("[FAIL] --enabled and --disabled cannot be used together."); - return (null, 1); - } - - if (enabled) - route.Enabled = true; - if (disabled) - route.Enabled = false; - // Validate - var errors = WebhookRouteValidator.Validate(routeName, route); - if (errors.Count > 0) - { - Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' has validation errors:"); - foreach (var error in errors) + if (dryRun) { - Console.Error.WriteLine($" - {error}"); + output.WriteLine($"[OK] Webhook route '{routeName}' is valid (dry run, not saved)."); + output.WriteLine($" Endpoint: /api/webhooks/{routeName}"); + return (null, 0); } - return (null, 1); - } - - if (dryRun) - { - output.WriteLine($"[OK] Webhook route '{routeName}' is valid (dry run, not saved)."); - output.WriteLine($" Endpoint: /api/webhooks/{routeName}"); - return (null, 0); - } - routeSaved = true; - updatedExistingRoute = exists; - return (route, 0); - }); + routeSaved = true; + updatedExistingRoute = exists; + return (route, 0); + }); + } + catch (Exception ex) when (ex is InvalidDataException or TimeoutException) + { + Console.Error.WriteLine($"[FAIL] {ex.Message}"); + return 1; + } if (routeSaved) { @@ -619,7 +625,7 @@ private static int RunValidate(string[] args, NetclawPaths paths, TextWriter out output.WriteLine($"[OK] Webhook route '{routeName}' is valid."); output.WriteLine($" Endpoint: /api/webhooks/{routeName}"); - output.WriteLine($" Verification: {route.Verification.Kind.ToString().ToLowerInvariant()}"); + output.WriteLine($" Verification: {ToCliVerifierKind(route.Verification.Kind)}"); output.WriteLine($" Audience: {route.Audience.ToString().ToLowerInvariant()}"); return 0; } diff --git a/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs b/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs index a9a92f94c..8230c663f 100644 --- a/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs +++ b/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs @@ -225,43 +225,100 @@ public void Route_rejects_undefined_numeric_verification_enums(bool invalidKind) } [Fact] - public async Task Update_serializes_read_modify_write_operations_across_store_instances() + public async Task Update_serializes_read_modify_write_operations_across_store_instances_and_path_aliases() { var firstStore = new WebhookRouteStore(_paths); - var secondStore = new WebhookRouteStore(_paths); + string? aliasPath = null; + NetclawPaths secondPaths = _paths; + if (!OperatingSystem.IsWindows()) + { + aliasPath = $"{_dir.Path}-alias"; + Directory.CreateSymbolicLink(aliasPath, _dir.Path); + secondPaths = new NetclawPaths(aliasPath); + } + + var secondStore = new WebhookRouteStore(secondPaths); firstStore.Save("concurrent-route", CreateValidRoute()); using var firstEntered = new ManualResetEventSlim(); using var secondStarted = new ManualResetEventSlim(); using var releaseFirst = new ManualResetEventSlim(); var cancellationToken = TestContext.Current.CancellationToken; - var first = Task.Run(() => firstStore.Update("concurrent-route", existing => + try { - firstEntered.Set(); + var first = Task.Run(() => firstStore.Update("concurrent-route", cancellationToken, existing => + { + firstEntered.Set(); + Assert.True(secondStarted.Wait(TimeSpan.FromSeconds(10), cancellationToken)); + Assert.True(releaseFirst.Wait(TimeSpan.FromSeconds(10), cancellationToken)); + existing!.RateLimitPerMinute = 12; + return (existing, true); + }), cancellationToken); + Assert.True(firstEntered.Wait(TimeSpan.FromSeconds(10), cancellationToken)); + + var second = Task.Run(() => + { + secondStarted.Set(); + return secondStore.Update("concurrent-route", cancellationToken, existing => + { + existing!.MaxBodyBytes = 2048; + return (existing, true); + }); + }, cancellationToken); + Assert.True(secondStarted.Wait(TimeSpan.FromSeconds(10), cancellationToken)); - Assert.True(releaseFirst.Wait(TimeSpan.FromSeconds(10), cancellationToken)); - existing!.RateLimitPerMinute = 12; - return (existing, true); - }), cancellationToken); - Assert.True(firstEntered.Wait(TimeSpan.FromSeconds(10), cancellationToken)); + releaseFirst.Set(); + await Task.WhenAll(first, second); - var second = Task.Run(() => + Assert.True(firstStore.TryGet("concurrent-route", out var saved)); + Assert.Equal(12, saved.Definition!.RateLimitPerMinute); + Assert.Equal(2048, saved.Definition.MaxBodyBytes); + } + finally { - secondStarted.Set(); - return secondStore.Update("concurrent-route", existing => + releaseFirst.Set(); + if (aliasPath is not null) + Directory.Delete(aliasPath); + } + } + + [Fact] + public async Task Update_lock_wait_honors_cancellation() + { + var firstStore = new WebhookRouteStore(_paths); + var secondStore = new WebhookRouteStore(_paths); + firstStore.Save("cancelled-update", CreateValidRoute()); + using var firstEntered = new ManualResetEventSlim(); + using var releaseFirst = new ManualResetEventSlim(); + using var cancellation = new CancellationTokenSource(); + var testCancellation = TestContext.Current.CancellationToken; + + var first = Task.Run(() => firstStore.Update( + "cancelled-update", + testCancellation, + existing => { - existing!.MaxBodyBytes = 2048; + firstEntered.Set(); + Assert.True(releaseFirst.Wait(TimeSpan.FromSeconds(10), testCancellation)); return (existing, true); - }); - }, cancellationToken); + }), testCancellation); + Assert.True(firstEntered.Wait(TimeSpan.FromSeconds(10), testCancellation)); - Assert.True(secondStarted.Wait(TimeSpan.FromSeconds(10), cancellationToken)); - releaseFirst.Set(); - await Task.WhenAll(first, second); + var second = Task.Run(() => secondStore.Update( + "cancelled-update", + cancellation.Token, + existing => (existing, true)), testCancellation); + cancellation.Cancel(); - Assert.True(firstStore.TryGet("concurrent-route", out var saved)); - Assert.Equal(12, saved.Definition!.RateLimitPerMinute); - Assert.Equal(2048, saved.Definition.MaxBodyBytes); + try + { + await Assert.ThrowsAnyAsync(() => second); + } + finally + { + releaseFirst.Set(); + await first; + } } [Fact] @@ -269,14 +326,14 @@ public void Embedded_config_and_route_schemas_share_timestamped_verification_con { using var configSchema = LoadEmbeddedSchema("netclaw-config.v1.schema.json"); using var routeSchema = LoadEmbeddedSchema("webhook-route.v1.schema.json"); - var configVerification = configSchema.RootElement + var configVerificationSchema = configSchema.RootElement .GetProperty("$defs") - .GetProperty("WebhookVerification") - .GetProperty("properties"); - var routeVerification = routeSchema.RootElement + .GetProperty("WebhookVerification"); + var routeVerificationSchema = routeSchema.RootElement .GetProperty("properties") - .GetProperty("Verification") - .GetProperty("properties"); + .GetProperty("Verification"); + var configVerification = configVerificationSchema.GetProperty("properties"); + var routeVerification = routeVerificationSchema.GetProperty("properties"); foreach (var propertyName in new[] { @@ -293,6 +350,10 @@ public void Embedded_config_and_route_schemas_share_timestamped_verification_con JsonSerializer.Serialize(routeProperty), JsonSerializer.Serialize(configProperty)); } + + Assert.Equal( + JsonSerializer.Serialize(routeVerificationSchema.GetProperty("allOf")), + JsonSerializer.Serialize(configVerificationSchema.GetProperty("allOf"))); } [Theory] diff --git a/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json b/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json index 63a94b234..3ffc4a463 100644 --- a/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json +++ b/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json @@ -933,20 +933,14 @@ }, "ToleranceSeconds": { "type": ["integer", "null"], - "minimum": 1, - "maximum": 3600, "default": 300 }, "TimestampField": { "type": ["string", "null"], - "minLength": 1, - "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$", "default": "t" }, "SignatureField": { "type": ["string", "null"], - "minLength": 1, - "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$", "default": "v1" }, "SignedPayloadSeparator": { @@ -954,6 +948,35 @@ "default": "." } }, + "allOf": [ + { + "if": { + "properties": { + "Kind": { "const": "HmacTimestamped" } + }, + "required": ["Kind"] + }, + "then": { + "properties": { + "ToleranceSeconds": { + "type": ["integer", "null"], + "minimum": 1, + "maximum": 3600 + }, + "TimestampField": { + "type": ["string", "null"], + "minLength": 1, + "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$" + }, + "SignatureField": { + "type": ["string", "null"], + "minLength": 1, + "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$" + } + } + } + } + ], "additionalProperties": false }, "WebhookRoute": { diff --git a/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json b/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json index b9f76e34d..11e235ed5 100644 --- a/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json +++ b/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json @@ -41,20 +41,14 @@ }, "ToleranceSeconds": { "type": ["integer", "null"], - "minimum": 1, - "maximum": 3600, "default": 300 }, "TimestampField": { "type": ["string", "null"], - "minLength": 1, - "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$", "default": "t" }, "SignatureField": { "type": ["string", "null"], - "minLength": 1, - "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$", "default": "v1" }, "SignedPayloadSeparator": { @@ -62,6 +56,35 @@ "default": "." } }, + "allOf": [ + { + "if": { + "properties": { + "Kind": { "const": "HmacTimestamped" } + }, + "required": ["Kind"] + }, + "then": { + "properties": { + "ToleranceSeconds": { + "type": ["integer", "null"], + "minimum": 1, + "maximum": 3600 + }, + "TimestampField": { + "type": ["string", "null"], + "minLength": 1, + "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$" + }, + "SignatureField": { + "type": ["string", "null"], + "minLength": 1, + "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$" + } + } + } + } + ], "additionalProperties": false, "required": ["Kind"] }, diff --git a/src/Netclaw.Configuration/WebhookRouteStore.cs b/src/Netclaw.Configuration/WebhookRouteStore.cs index afd7dcfcf..9272a3c94 100644 --- a/src/Netclaw.Configuration/WebhookRouteStore.cs +++ b/src/Netclaw.Configuration/WebhookRouteStore.cs @@ -3,6 +3,7 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using System.Diagnostics; using System.Security.Cryptography; using System.Text; using System.Text.Json; @@ -13,6 +14,9 @@ namespace Netclaw.Configuration; public sealed class WebhookRouteStore { + private static readonly TimeSpan RouteLockTimeout = TimeSpan.FromSeconds(30); + private static readonly TimeSpan RouteLockPollInterval = TimeSpan.FromMilliseconds(50); + private static readonly Regex RouteNamePattern = new( "^[a-z0-9]+(?:-[a-z0-9]+)*$", RegexOptions.Compiled | RegexOptions.CultureInvariant); @@ -102,7 +106,7 @@ public void Save(string routeName, WebhookRouteConfig definition) lock (_sync) { var filePath = GetPath(routeName); - using var routeLock = AcquireRouteLock(filePath); + using var routeLock = AcquireRouteLock(filePath, CancellationToken.None); Write(filePath, definition); } } @@ -113,6 +117,7 @@ public void Save(string routeName, WebhookRouteConfig definition) /// public TResult Update( string routeName, + CancellationToken cancellationToken, Func update) { ArgumentNullException.ThrowIfNull(update); @@ -120,7 +125,7 @@ public TResult Update( lock (_sync) { var filePath = GetPath(routeName); - using var routeLock = AcquireRouteLock(filePath); + using var routeLock = AcquireRouteLock(filePath, cancellationToken); WebhookRouteConfig? existing = null; if (File.Exists(filePath)) { @@ -142,7 +147,7 @@ public bool Delete(string routeName) lock (_sync) { var filePath = GetPath(routeName); - using var routeLock = AcquireRouteLock(filePath); + using var routeLock = AcquireRouteLock(filePath, CancellationToken.None); if (!File.Exists(filePath)) return false; @@ -179,34 +184,87 @@ private void Write(string filePath, WebhookRouteConfig definition) } } - private static IDisposable AcquireRouteLock(string filePath) + private static IDisposable AcquireRouteLock(string filePath, CancellationToken cancellationToken) { - var canonicalPath = Path.GetFullPath(filePath); - var lockId = Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(canonicalPath))); - var mutex = new Mutex(initiallyOwned: false, $"netclaw-webhook-route-{lockId}"); + var canonicalPath = GetCanonicalLockPath(filePath); + var lockIdentity = OperatingSystem.IsWindows() + ? canonicalPath.ToUpperInvariant() + : canonicalPath; + var lockId = Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(lockIdentity))); + var lockScope = OperatingSystem.IsWindows() ? @"Global\" : string.Empty; + var mutex = new Mutex(initiallyOwned: false, $"{lockScope}netclaw-webhook-route-{lockId}"); + var ownsMutex = false; try { try { - mutex.WaitOne(); + ownsMutex = WaitForMutex(mutex, cancellationToken); } catch (AbandonedMutexException) { + ownsMutex = true; // The abandoning process exited while holding the route lock. This process now owns it, // and the route's atomic file replacement guarantees the existing file is still complete. DeleteAbandonedTempFiles(canonicalPath); } + if (!ownsMutex) + throw new TimeoutException($"Timed out waiting to update webhook route '{Path.GetFileNameWithoutExtension(filePath)}'."); + return new RouteLock(mutex); } catch { + if (ownsMutex) + mutex.ReleaseMutex(); mutex.Dispose(); throw; } } + private static bool WaitForMutex(Mutex mutex, CancellationToken cancellationToken) + { + if (!cancellationToken.CanBeCanceled) + return mutex.WaitOne(RouteLockTimeout); + + var startedAt = Stopwatch.GetTimestamp(); + while (true) + { + cancellationToken.ThrowIfCancellationRequested(); + if (mutex.WaitOne(TimeSpan.Zero)) + return true; + + var remaining = RouteLockTimeout - Stopwatch.GetElapsedTime(startedAt); + if (remaining <= TimeSpan.Zero) + return false; + + var wait = remaining < RouteLockPollInterval ? remaining : RouteLockPollInterval; + if (cancellationToken.WaitHandle.WaitOne(wait)) + cancellationToken.ThrowIfCancellationRequested(); + } + } + + private static string GetCanonicalLockPath(string filePath) + { + var fullPath = Path.GetFullPath(filePath); + var directoryPath = Path.GetDirectoryName(fullPath) + ?? throw new InvalidOperationException("Webhook route path has no parent directory."); + var root = Path.GetPathRoot(directoryPath) + ?? throw new InvalidOperationException("Webhook route path has no root directory."); + var current = root; + var relativeDirectory = Path.GetRelativePath(root, directoryPath); + foreach (var segment in relativeDirectory.Split( + [Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar], + StringSplitOptions.RemoveEmptyEntries)) + { + var directory = new DirectoryInfo(Path.Combine(current, segment)); + current = (directory.ResolveLinkTarget(returnFinalTarget: true) ?? directory).FullName; + } + + return Path.Combine(current, Path.GetFileName(fullPath)); + } + private static void DeleteAbandonedTempFiles(string filePath) { var directory = Path.GetDirectoryName(filePath) diff --git a/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs b/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs index 3e653de4c..7720f881d 100644 --- a/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs +++ b/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs @@ -72,14 +72,12 @@ public async Task RequestConfigRestartAsync_records_timed_out_sessions() { var time = new FakeTimeProvider(); var (coordinator, drain) = CreateCoordinator( - ["slack/C123.1", "slack/C123.2"], + ["slack/C123.2"], timedOutSessionIds: ["slack/C123.2"], timeProvider: time); var restart = coordinator.RequestConfigRestartAsync(CancellationToken.None); await drain.AllRequestsObserved; - drain.AcknowledgeAll(); - await drain.AllAcknowledgementsSent; time.Advance(DrainTimeout); await restart; @@ -133,10 +131,10 @@ await Assert.ThrowsAsync( } [Fact] - public async Task SessionDrainHelper_queries_manager_drains_sessions_and_reports_timeouts() + public async Task SessionDrainHelper_reports_deadline_timeouts() { var time = new FakeTimeProvider(); - var activeIds = new[] { "slack/drain-ok", "slack/drain-timeout" }; + var activeIds = new[] { "slack/drain-timeout" }; var timedOut = new[] { "slack/drain-timeout" }; var drain = new DrainControl(activeIds, timedOut); var sessionManager = _system.ActorOf(Props.Create(() => new StubSessionManagerActor( @@ -152,25 +150,45 @@ public async Task SessionDrainHelper_queries_manager_drains_sessions_and_reports deadlineCts.Token, CancellationToken.None); await drain.AllRequestsObserved; - drain.AcknowledgeAll(); - await drain.AllAcknowledgementsSent; time.Advance(DrainTimeout); var result = await operation; - Assert.Equal(2, result.AllSessionIds.Count); - Assert.Single(result.DrainedSessionIds); - Assert.Equal("slack/drain-ok", result.DrainedSessionIds[0].Value); + Assert.Single(result.AllSessionIds); + Assert.Empty(result.DrainedSessionIds); Assert.Single(result.TimedOutSessionIds); Assert.Equal("slack/drain-timeout", result.TimedOutSessionIds[0].Value); var context = result.ToNotificationContext(); Assert.Equal("timeout", context["drainOutcome"]); - Assert.Equal("2", context["activeSessions"]); - Assert.Equal("1", context["drainedSessions"]); + Assert.Equal("1", context["activeSessions"]); + Assert.Equal("0", context["drainedSessions"]); Assert.Equal("1", context["timedOutSessions"]); } + [Fact] + public async Task SessionDrainHelper_propagates_caller_cancellation() + { + var activeIds = new[] { "slack/drain-cancelled" }; + var drain = new DrainControl(activeIds, activeIds); + var sessionManager = _system.ActorOf(Props.Create(() => new StubSessionManagerActor( + activeIds, + drain, + throwOnEnumeration: false))); + using var callerCts = new CancellationTokenSource(); + + var operation = SessionDrainHelper.DrainAsync( + sessionManager, + "integration-test", + NullLogger.Instance, + callerCts.Token, + callerCts.Token); + await drain.AllRequestsObserved; + callerCts.Cancel(); + + await Assert.ThrowsAnyAsync(() => operation); + } + public async ValueTask DisposeAsync() { await _system.Terminate(); diff --git a/src/Netclaw.Daemon/Services/SessionDrainHelper.cs b/src/Netclaw.Daemon/Services/SessionDrainHelper.cs index 55270b7a4..e71ee3e9b 100644 --- a/src/Netclaw.Daemon/Services/SessionDrainHelper.cs +++ b/src/Netclaw.Daemon/Services/SessionDrainHelper.cs @@ -65,6 +65,10 @@ public static async Task DrainAsync( { return new DrainOutcome(sessionId, false); } + catch (OperationCanceledException) + { + throw; + } catch (Exception ex) { logger.LogWarning(ex, "Failed to drain session {SessionId} before shutdown.", sessionId.Value); From 5289a445bf73d3f8fcfbbdcc2d1a4a5773c31ffd Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 15 Jul 2026 21:03:29 +0000 Subject: [PATCH 06/10] fix: close final concurrency review gaps --- src/Netclaw.Actors/Tools/DeleteWebhookTool.cs | 15 +++- .../Webhooks/WebhooksCommandTests.cs | 35 ++++++-- src/Netclaw.Cli/Webhooks/WebhooksCommand.cs | 13 ++- .../WebhookRouteStoreTests.cs | 43 ++++++---- .../Schemas/webhook-route.v1.schema.json | 62 +++++++------- .../WebhookRouteStore.cs | 82 ++++++++----------- .../Services/DaemonRestartCoordinatorTests.cs | 38 ++++++++- .../Services/SessionDrainHelper.cs | 15 +++- 8 files changed, 192 insertions(+), 111 deletions(-) diff --git a/src/Netclaw.Actors/Tools/DeleteWebhookTool.cs b/src/Netclaw.Actors/Tools/DeleteWebhookTool.cs index f9e2d94e5..db7d96a49 100644 --- a/src/Netclaw.Actors/Tools/DeleteWebhookTool.cs +++ b/src/Netclaw.Actors/Tools/DeleteWebhookTool.cs @@ -1,4 +1,4 @@ -// ----------------------------------------------------------------------- +// ----------------------------------------------------------------------- // // Copyright (C) 2026 - 2026 Petabridge, LLC // @@ -30,8 +30,15 @@ protected override Task ExecuteAsync(Params args, ToolInvocationContext if (!WebhookRouteStore.TryNormalizeRouteName(args.RouteName, out var routeName, out var routeError)) return Task.FromResult($"Error: {routeError}"); - return Task.FromResult(_store.Delete(routeName) - ? $"Webhook route '{routeName}' deleted." - : $"Webhook route '{routeName}' not found."); + try + { + return Task.FromResult(_store.Delete(routeName, ct) + ? $"Webhook route '{routeName}' deleted." + : $"Webhook route '{routeName}' not found."); + } + catch (TimeoutException ex) + { + return Task.FromResult($"Error: {ex.Message}"); + } } } diff --git a/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs b/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs index 4ec8344e3..b0cffa881 100644 --- a/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs @@ -660,12 +660,12 @@ public void RouteSchema_applies_timestamp_constraints_only_to_timestamped_kind( var schema = JsonSchema.FromText(LoadRouteSchema()); var route = new JsonObject { - ["Prompt"] = "process delivery", - ["Verification"] = new JsonObject + ["prompt"] = "process delivery", + ["verification"] = new JsonObject { - ["Kind"] = kind, - ["ToleranceSeconds"] = toleranceSeconds, - ["TimestampField"] = timestampField + ["kind"] = kind, + ["toleranceSeconds"] = toleranceSeconds, + ["timestampField"] = timestampField } }; @@ -674,6 +674,31 @@ public void RouteSchema_applies_timestamp_constraints_only_to_timestamped_kind( Assert.Equal(expectedValid, evaluation.IsValid); } + [Fact] + public void RouteSchema_accepts_exact_store_serialization() + { + var route = new WebhookRouteConfig + { + Prompt = "process delivery", + Verification = new WebhookVerificationConfig + { + Kind = WebhookVerifierKind.HmacTimestamped, + Secret = new SensitiveString("test-secret"), + ToleranceSeconds = 300, + TimestampField = "t", + SignatureField = "v1" + } + }; + new WebhookRouteStore(_paths).Save("schema-route", route); + var serializedRoute = JsonNode.Parse( + File.ReadAllText(Path.Combine(_paths.WebhooksDirectory, "schema-route.json"))); + var schema = JsonSchema.FromText(LoadRouteSchema()); + + var evaluation = schema.Evaluate(serializedRoute); + + Assert.True(evaluation.IsValid); + } + [Fact] public async Task Validate_NonexistentRoute_ReturnsOne() { diff --git a/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs b/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs index 53a0f0e69..a56d27b84 100644 --- a/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs +++ b/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs @@ -567,7 +567,18 @@ private static int RunDelete(string[] args, WebhookRouteStore store, TextWriter } } - if (!store.Delete(routeName)) + bool deleted; + try + { + deleted = store.Delete(routeName, CancellationToken.None); + } + catch (TimeoutException ex) + { + Console.Error.WriteLine($"[FAIL] {ex.Message}"); + return 1; + } + + if (!deleted) { Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' not found."); return 1; diff --git a/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs b/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs index 8230c663f..b6cca36bb 100644 --- a/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs +++ b/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs @@ -74,7 +74,7 @@ public void Delete_RejectsTraversalRouteName() { var store = new WebhookRouteStore(_paths); - Assert.Throws(() => store.Delete("../secrets")); + Assert.Throws(() => store.Delete("../secrets", CancellationToken.None)); } [Fact] @@ -286,7 +286,6 @@ public async Task Update_serializes_read_modify_write_operations_across_store_in public async Task Update_lock_wait_honors_cancellation() { var firstStore = new WebhookRouteStore(_paths); - var secondStore = new WebhookRouteStore(_paths); firstStore.Save("cancelled-update", CreateValidRoute()); using var firstEntered = new ManualResetEventSlim(); using var releaseFirst = new ManualResetEventSlim(); @@ -304,7 +303,7 @@ public async Task Update_lock_wait_honors_cancellation() }), testCancellation); Assert.True(firstEntered.Wait(TimeSpan.FromSeconds(10), testCancellation)); - var second = Task.Run(() => secondStore.Update( + var second = Task.Run(() => firstStore.Update( "cancelled-update", cancellation.Token, existing => (existing, true)), testCancellation); @@ -331,29 +330,43 @@ public void Embedded_config_and_route_schemas_share_timestamped_verification_con .GetProperty("WebhookVerification"); var routeVerificationSchema = routeSchema.RootElement .GetProperty("properties") - .GetProperty("Verification"); + .GetProperty("verification"); var configVerification = configVerificationSchema.GetProperty("properties"); var routeVerification = routeVerificationSchema.GetProperty("properties"); - foreach (var propertyName in new[] + foreach (var (configName, routeName) in new[] { - "Kind", - "ToleranceSeconds", - "TimestampField", - "SignatureField", - "SignedPayloadSeparator" + ("Kind", "kind"), + ("ToleranceSeconds", "toleranceSeconds"), + ("TimestampField", "timestampField"), + ("SignatureField", "signatureField"), + ("SignedPayloadSeparator", "signedPayloadSeparator") }) { - var configProperty = configVerification.GetProperty(propertyName); - var routeProperty = routeVerification.GetProperty(propertyName); + var configProperty = configVerification.GetProperty(configName); + var routeProperty = routeVerification.GetProperty(routeName); Assert.Equal( JsonSerializer.Serialize(routeProperty), JsonSerializer.Serialize(configProperty)); } - Assert.Equal( - JsonSerializer.Serialize(routeVerificationSchema.GetProperty("allOf")), - JsonSerializer.Serialize(configVerificationSchema.GetProperty("allOf"))); + var configConditionalProperties = configVerificationSchema.GetProperty("allOf")[0] + .GetProperty("then") + .GetProperty("properties"); + var routeConditionalProperties = routeVerificationSchema.GetProperty("allOf")[0] + .GetProperty("then") + .GetProperty("properties"); + foreach (var (configName, routeName) in new[] + { + ("ToleranceSeconds", "toleranceSeconds"), + ("TimestampField", "timestampField"), + ("SignatureField", "signatureField") + }) + { + Assert.Equal( + JsonSerializer.Serialize(routeConditionalProperties.GetProperty(routeName)), + JsonSerializer.Serialize(configConditionalProperties.GetProperty(configName))); + } } [Theory] diff --git a/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json b/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json index 11e235ed5..8cc440372 100644 --- a/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json +++ b/src/Netclaw.Configuration/Schemas/webhook-route.v1.schema.json @@ -4,54 +4,54 @@ "title": "Netclaw Webhook Route v1", "type": "object", "properties": { - "Enabled": { + "enabled": { "type": "boolean", "default": true }, - "Verification": { + "verification": { "type": "object", "properties": { - "Kind": { + "kind": { "type": "string", "enum": ["Hmac", "HeaderSecret", "HmacTimestamped"], "default": "Hmac" }, - "HmacAlgorithm": { + "hmacAlgorithm": { "type": "string", "enum": ["Sha256"], "default": "Sha256" }, - "Secret": { + "secret": { "type": ["string", "null"] }, - "SignatureHeaderName": { + "signatureHeaderName": { "type": ["string", "null"] }, - "SignaturePrefix": { + "signaturePrefix": { "type": ["string", "null"] }, - "SecretHeaderName": { + "secretHeaderName": { "type": ["string", "null"] }, - "EventHeaderName": { + "eventHeaderName": { "type": ["string", "null"] }, - "DeliveryIdHeaderName": { + "deliveryIdHeaderName": { "type": ["string", "null"] }, - "ToleranceSeconds": { + "toleranceSeconds": { "type": ["integer", "null"], "default": 300 }, - "TimestampField": { + "timestampField": { "type": ["string", "null"], "default": "t" }, - "SignatureField": { + "signatureField": { "type": ["string", "null"], "default": "v1" }, - "SignedPayloadSeparator": { + "signedPayloadSeparator": { "type": ["string", "null"], "default": "." } @@ -60,23 +60,23 @@ { "if": { "properties": { - "Kind": { "const": "HmacTimestamped" } + "kind": { "const": "HmacTimestamped" } }, - "required": ["Kind"] + "required": ["kind"] }, "then": { "properties": { - "ToleranceSeconds": { + "toleranceSeconds": { "type": ["integer", "null"], "minimum": 1, "maximum": 3600 }, - "TimestampField": { + "timestampField": { "type": ["string", "null"], "minLength": 1, "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$" }, - "SignatureField": { + "signatureField": { "type": ["string", "null"], "minLength": 1, "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$" @@ -86,56 +86,56 @@ } ], "additionalProperties": false, - "required": ["Kind"] + "required": ["kind"] }, - "Events": { + "events": { "type": "array", "items": { "type": "string" }, "default": [] }, - "Audience": { + "audience": { "type": "string", "enum": ["Public", "Team", "Personal"], "default": "Public" }, - "Prompt": { + "prompt": { "type": "string", "default": "" }, - "NotifyInstructions": { + "notifyInstructions": { "type": "string", "default": "" }, - "DeliveryRequired": { + "deliveryRequired": { "type": "boolean", "default": true }, - "NotificationTarget": { + "notificationTarget": { "type": ["object", "null"], "properties": { - "Kind": { + "kind": { "type": "string", "enum": ["Slack"], "default": "Slack" }, - "ChannelId": { + "channelId": { "type": ["string", "null"] } }, "additionalProperties": false, "default": null }, - "MaxBodyBytes": { + "maxBodyBytes": { "type": "integer", "minimum": 1, "default": 1048576 }, - "RateLimitPerMinute": { + "rateLimitPerMinute": { "type": "integer", "minimum": 1, "default": 30 } }, - "required": ["Prompt", "Verification"], + "required": ["prompt", "verification"], "additionalProperties": false } diff --git a/src/Netclaw.Configuration/WebhookRouteStore.cs b/src/Netclaw.Configuration/WebhookRouteStore.cs index 9272a3c94..52a5a0126 100644 --- a/src/Netclaw.Configuration/WebhookRouteStore.cs +++ b/src/Netclaw.Configuration/WebhookRouteStore.cs @@ -29,7 +29,6 @@ public sealed class WebhookRouteStore }; private readonly NetclawPaths _paths; - private readonly object _sync = new(); public WebhookRouteStore(NetclawPaths paths) { @@ -77,38 +76,27 @@ public static bool TryNormalizeRouteName(string value, out string normalized, ou /// public bool TryGet(string routeName, out (string FilePath, WebhookRouteConfig? Definition) result) { - lock (_sync) + var filePath = GetPath(routeName); + if (!File.Exists(filePath)) { - var filePath = GetPath(routeName); - if (!File.Exists(filePath)) - { - result = default; - return false; - } - result = (filePath, TryRead(filePath)); - return true; + result = default; + return false; } + result = (filePath, TryRead(filePath)); + return true; } public IReadOnlyList<(string RouteName, string FilePath, WebhookRouteConfig? Definition)> ListRouteFiles() - { - lock (_sync) - { - return Directory.EnumerateFiles(_paths.WebhooksDirectory, "*.json", SearchOption.TopDirectoryOnly) - .Select(file => (Path.GetFileNameWithoutExtension(file), file, TryRead(file))) - .OrderBy(x => x.Item1, StringComparer.OrdinalIgnoreCase) - .ToList(); - } - } + => Directory.EnumerateFiles(_paths.WebhooksDirectory, "*.json", SearchOption.TopDirectoryOnly) + .Select(file => (Path.GetFileNameWithoutExtension(file), file, TryRead(file))) + .OrderBy(x => x.Item1, StringComparer.OrdinalIgnoreCase) + .ToList(); public void Save(string routeName, WebhookRouteConfig definition) { - lock (_sync) - { - var filePath = GetPath(routeName); - using var routeLock = AcquireRouteLock(filePath, CancellationToken.None); - Write(filePath, definition); - } + var filePath = GetPath(routeName); + using var routeLock = AcquireRouteLock(filePath, CancellationToken.None); + Write(filePath, definition); } /// @@ -122,38 +110,32 @@ public TResult Update( { ArgumentNullException.ThrowIfNull(update); - lock (_sync) + var filePath = GetPath(routeName); + using var routeLock = AcquireRouteLock(filePath, cancellationToken); + WebhookRouteConfig? existing = null; + if (File.Exists(filePath)) { - var filePath = GetPath(routeName); - using var routeLock = AcquireRouteLock(filePath, cancellationToken); - WebhookRouteConfig? existing = null; - if (File.Exists(filePath)) - { - existing = TryRead(filePath); - if (existing is null) - throw new InvalidDataException($"Existing webhook route '{routeName}' could not be parsed."); - } + existing = TryRead(filePath); + if (existing is null) + throw new InvalidDataException($"Existing webhook route '{routeName}' could not be parsed."); + } - var outcome = update(existing); - if (outcome.Definition is not null) - Write(filePath, outcome.Definition); + var outcome = update(existing); + if (outcome.Definition is not null) + Write(filePath, outcome.Definition); - return outcome.Result; - } + return outcome.Result; } - public bool Delete(string routeName) + public bool Delete(string routeName, CancellationToken cancellationToken) { - lock (_sync) - { - var filePath = GetPath(routeName); - using var routeLock = AcquireRouteLock(filePath, CancellationToken.None); - if (!File.Exists(filePath)) - return false; + var filePath = GetPath(routeName); + using var routeLock = AcquireRouteLock(filePath, cancellationToken); + if (!File.Exists(filePath)) + return false; - File.Delete(filePath); - return true; - } + File.Delete(filePath); + return true; } private WebhookRouteConfig? TryRead(string filePath) diff --git a/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs b/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs index 7720f881d..e2171c0b0 100644 --- a/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs +++ b/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs @@ -7,6 +7,7 @@ using Akka.Actor; using Akka.Hosting; using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Time.Testing; using Netclaw.Actors.Channels; @@ -71,13 +72,17 @@ public async Task RequestConfigRestartAsync_drains_active_sessions_and_requests_ public async Task RequestConfigRestartAsync_records_timed_out_sessions() { var time = new FakeTimeProvider(); + var logger = new DrainAcknowledgementLogger(); var (coordinator, drain) = CreateCoordinator( - ["slack/C123.2"], + ["slack/C123.1", "slack/C123.2"], timedOutSessionIds: ["slack/C123.2"], - timeProvider: time); + timeProvider: time, + logger: logger); var restart = coordinator.RequestConfigRestartAsync(CancellationToken.None); await drain.AllRequestsObserved; + drain.AcknowledgeAll(); + await logger.Acknowledged; time.Advance(DrainTimeout); await restart; @@ -199,7 +204,8 @@ public async ValueTask DisposeAsync() IReadOnlyList activeSessionIds, IReadOnlyList? timedOutSessionIds = null, bool throwOnEnumeration = false, - FakeTimeProvider? timeProvider = null) + FakeTimeProvider? timeProvider = null, + ILogger? logger = null) { var timedOut = timedOutSessionIds ?? Array.Empty(); var drain = new DrainControl(activeSessionIds, timedOut); @@ -221,7 +227,7 @@ public async ValueTask DisposeAsync() _appLifetime, notifier, time, - NullLogger.Instance, + logger ?? NullLogger.Instance, DrainTimeout); return (coordinator, drain); @@ -311,6 +317,30 @@ public void AcknowledgeAll() } } + private sealed class DrainAcknowledgementLogger : ILogger + { + private readonly TaskCompletionSource _acknowledged = + new(TaskCreationOptions.RunContinuationsAsynchronously); + + public Task Acknowledged => _acknowledged.Task; + + public IDisposable? BeginScope(TState state) where TState : notnull + => NullLogger.Instance.BeginScope(state); + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + if (eventId == SessionDrainHelper.SessionDrainAcknowledgedEvent) + _acknowledged.TrySetResult(); + } + } + private sealed class FakeApplicationLifetime : IHostApplicationLifetime { public bool StopRequested { get; private set; } diff --git a/src/Netclaw.Daemon/Services/SessionDrainHelper.cs b/src/Netclaw.Daemon/Services/SessionDrainHelper.cs index e71ee3e9b..14481c893 100644 --- a/src/Netclaw.Daemon/Services/SessionDrainHelper.cs +++ b/src/Netclaw.Daemon/Services/SessionDrainHelper.cs @@ -18,6 +18,9 @@ namespace Netclaw.Daemon.Services; /// internal static class SessionDrainHelper { + internal static readonly EventId SessionDrainAcknowledgedEvent = + new(1, nameof(SessionDrainAcknowledgedEvent)); + /// /// Queries the session manager for active sessions, sends /// to each in parallel, and waits for acknowledgement or cancellation. @@ -59,7 +62,17 @@ public static async Task DrainAsync( timeout: Timeout.InfiniteTimeSpan, cancellationToken: operationCancellationToken); - return new DrainOutcome(sessionId, ack.SessionId == sessionId); + var drained = ack.SessionId == sessionId; + if (drained) + { + logger.LogDebug( + SessionDrainAcknowledgedEvent, + "Session {SessionId} acknowledged drain for {Reason}.", + sessionId.Value, + reason); + } + + return new DrainOutcome(sessionId, drained); } catch (OperationCanceledException) when (!callerCancellationToken.IsCancellationRequested) { From bf65389981937e31337291a8dcf0f3821660a3a3 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 15 Jul 2026 21:21:50 +0000 Subject: [PATCH 07/10] fix(webhooks): canonicalize nested route path aliases --- .../WebhookRouteStoreTests.cs | 11 ++++++++++- src/Netclaw.Configuration/WebhookRouteStore.cs | 17 +++++++++++++---- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs b/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs index b6cca36bb..078364759 100644 --- a/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs +++ b/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs @@ -229,11 +229,18 @@ public async Task Update_serializes_read_modify_write_operations_across_store_in { var firstStore = new WebhookRouteStore(_paths); string? aliasPath = null; + string? parentAliasPath = null; NetclawPaths secondPaths = _paths; if (!OperatingSystem.IsWindows()) { + var parentPath = Path.GetDirectoryName(_dir.Path) + ?? throw new InvalidOperationException("Test directory has no parent directory."); + parentAliasPath = $"{_dir.Path}-parent-alias"; + Directory.CreateSymbolicLink(parentAliasPath, parentPath); + + var targetThroughParentAlias = Path.Combine(parentAliasPath, Path.GetFileName(_dir.Path)); aliasPath = $"{_dir.Path}-alias"; - Directory.CreateSymbolicLink(aliasPath, _dir.Path); + Directory.CreateSymbolicLink(aliasPath, targetThroughParentAlias); secondPaths = new NetclawPaths(aliasPath); } @@ -279,6 +286,8 @@ public async Task Update_serializes_read_modify_write_operations_across_store_in releaseFirst.Set(); if (aliasPath is not null) Directory.Delete(aliasPath); + if (parentAliasPath is not null) + Directory.Delete(parentAliasPath); } } diff --git a/src/Netclaw.Configuration/WebhookRouteStore.cs b/src/Netclaw.Configuration/WebhookRouteStore.cs index 52a5a0126..46478aa0a 100644 --- a/src/Netclaw.Configuration/WebhookRouteStore.cs +++ b/src/Netclaw.Configuration/WebhookRouteStore.cs @@ -232,19 +232,28 @@ private static string GetCanonicalLockPath(string filePath) var fullPath = Path.GetFullPath(filePath); var directoryPath = Path.GetDirectoryName(fullPath) ?? throw new InvalidOperationException("Webhook route path has no parent directory."); - var root = Path.GetPathRoot(directoryPath) + return Path.Combine(GetCanonicalDirectoryPath(directoryPath), Path.GetFileName(fullPath)); + } + + private static string GetCanonicalDirectoryPath(string directoryPath) + { + var fullPath = Path.GetFullPath(directoryPath); + var root = Path.GetPathRoot(fullPath) ?? throw new InvalidOperationException("Webhook route path has no root directory."); var current = root; - var relativeDirectory = Path.GetRelativePath(root, directoryPath); + var relativeDirectory = Path.GetRelativePath(root, fullPath); foreach (var segment in relativeDirectory.Split( [Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar], StringSplitOptions.RemoveEmptyEntries)) { var directory = new DirectoryInfo(Path.Combine(current, segment)); - current = (directory.ResolveLinkTarget(returnFinalTarget: true) ?? directory).FullName; + var target = directory.ResolveLinkTarget(returnFinalTarget: true); + current = target is null + ? directory.FullName + : GetCanonicalDirectoryPath(target.FullName); } - return Path.Combine(current, Path.GetFileName(fullPath)); + return current; } private static void DeleteAbandonedTempFiles(string filePath) From 7a6d23598d83fab56449d1001fdec3724146885a Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 15 Jul 2026 21:31:16 +0000 Subject: [PATCH 08/10] test(webhooks): accept platform-specific write errors --- src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs b/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs index b0cffa881..529740b9c 100644 --- a/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs @@ -159,12 +159,14 @@ public async Task Set_WriteFailure_DoesNotReportSuccess() Directory.CreateDirectory(Path.Combine(_paths.WebhooksDirectory, "blocked-route.json")); using var output = new StringWriter(); - await Assert.ThrowsAnyAsync(() => WebhooksCommand.RunAsync([ + var exception = await Assert.ThrowsAnyAsync(() => WebhooksCommand.RunAsync([ "webhooks", "set", "blocked-route", "--prompt", "Test prompt", "--secret", "test-secret" ], _paths, output)); + Assert.True(exception is IOException or UnauthorizedAccessException, + $"Expected a persistence IO exception, got {exception.GetType().Name}: {exception.Message}"); Assert.DoesNotContain("[OK]", output.ToString(), StringComparison.Ordinal); } From 984b64934cb9ec4b3faae2a41eb31bd013189eb6 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 15 Jul 2026 21:47:11 +0000 Subject: [PATCH 09/10] docs(openspec): complete webhook verification checklist --- openspec/changes/add-timestamped-webhook-hmac/tasks.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openspec/changes/add-timestamped-webhook-hmac/tasks.md b/openspec/changes/add-timestamped-webhook-hmac/tasks.md index 956151448..e1d000e29 100644 --- a/openspec/changes/add-timestamped-webhook-hmac/tasks.md +++ b/openspec/changes/add-timestamped-webhook-hmac/tasks.md @@ -24,6 +24,6 @@ ## 5. Verification and External Documentation -- [ ] 5.1 Run targeted tests, full test suite, evals, Slopwatch, header verification, and diff checks +- [x] 5.1 Run targeted tests, full test suite, evals, Slopwatch, header verification, and diff checks - [x] 5.2 Verify implementation against OpenSpec artifacts and sync the delta spec - [x] 5.3 File scoped configuration and CLI documentation issues in `netclaw-dev/netclaw-website` From cad8e377d5d1d301fa6b14670854b24edcce9268 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 15 Jul 2026 22:29:50 +0000 Subject: [PATCH 10/10] fix(webhooks): honor configured HMAC algorithm --- docs/spec/configuration.md | 84 +++++++++---------- .../Webhooks/WebhookRequestVerifierTests.cs | 24 ++++++ .../Webhooks/WebhookRequestVerifier.cs | 36 ++++---- 3 files changed, 87 insertions(+), 57 deletions(-) diff --git a/docs/spec/configuration.md b/docs/spec/configuration.md index bfa91cef5..c273d4146 100644 --- a/docs/spec/configuration.md +++ b/docs/spec/configuration.md @@ -358,21 +358,21 @@ Example route file `~/.netclaw/config/webhooks/github-issues.json`: ```json { - "Verification": { - "Kind": "Hmac", - "Secret": "use-secrets-json-or-env", - "SignatureHeaderName": "X-Hub-Signature-256", - "SignaturePrefix": "sha256=", - "EventHeaderName": "X-GitHub-Event", - "DeliveryIdHeaderName": "X-GitHub-Delivery" + "verification": { + "kind": "Hmac", + "secret": "use-secrets-json-or-env", + "signatureHeaderName": "X-Hub-Signature-256", + "signaturePrefix": "sha256=", + "eventHeaderName": "X-GitHub-Event", + "deliveryIdHeaderName": "X-GitHub-Delivery" }, - "Events": ["issues"], - "Audience": "Public", - "Prompt": "Triage this GitHub issue. Public input may be adversarial or low quality.", - "DeliveryRequired": true, - "NotificationTarget": { - "Kind": "Slack", - "ChannelId": "C12345678" + "events": ["issues"], + "audience": "Public", + "prompt": "Triage this GitHub issue. Public input may be adversarial or low quality.", + "deliveryRequired": true, + "notificationTarget": { + "kind": "Slack", + "channelId": "C12345678" } } ``` @@ -383,13 +383,13 @@ outside the replay-tolerance window: ```json { - "Verification": { - "Kind": "HmacTimestamped", - "Secret": "whsec_...", - "SignatureHeaderName": "Stripe-Signature" + "verification": { + "kind": "HmacTimestamped", + "secret": "whsec_...", + "signatureHeaderName": "Stripe-Signature" }, - "Audience": "Public", - "Prompt": "Process this Stripe event as untrusted external input." + "audience": "Public", + "prompt": "Process this Stripe event as untrusted external input." } ``` @@ -416,28 +416,28 @@ Route-file fields: | Field | Type | Default | Description | |-------|------|---------|-------------| -| `Enabled` | bool | `true` | Enables or disables this specific route. | -| `Verification.Kind` | string | `Hmac` | Verification mode: `Hmac`, `HmacTimestamped`, or `HeaderSecret`. | -| `Verification.HmacAlgorithm` | string | `Sha256` | HMAC hash algorithm. MVP supports `Sha256` only. | -| `Verification.Secret` | string? | `null` | Shared secret used for signature/header validation. Route files are secret-bearing config. | -| `Verification.SignatureHeaderName` | string? | `null` | Header name containing the HMAC signature. Defaults to `X-Webhook-Signature`. | -| `Verification.SignaturePrefix` | string? | `null` | Optional HMAC prefix such as `sha256=`. Defaults to empty string. | -| `Verification.SecretHeaderName` | string? | `null` | Header name for `HeaderSecret` mode. Defaults to `X-Webhook-Secret`. | -| `Verification.EventHeaderName` | string? | `null` | Event-name header. Defaults to `X-Webhook-Event`. | -| `Verification.DeliveryIdHeaderName` | string? | `null` | Delivery ID header. Defaults to `X-Webhook-Delivery`. | -| `Verification.ToleranceSeconds` | int? | `300` | Maximum past or future clock difference for `HmacTimestamped`, from 1 through 3600 seconds. | -| `Verification.TimestampField` | string? | `t` | Structured-header timestamp field for `HmacTimestamped`; must be an ASCII HTTP token and differ from the signature field. | -| `Verification.SignatureField` | string? | `v1` | Structured-header signature field for `HmacTimestamped`; follows the same HTTP-token constraint, and multiple instances support sender secret rotation. | -| `Verification.SignedPayloadSeparator` | string? | `.` | Separator between the exact timestamp text and raw body for `HmacTimestamped`. | -| `Events` | string[] | `[]` | Optional allow-list of event types. Empty means all verified events are accepted. | -| `Audience` | string | `Public` | Source audience for the autonomous webhook session (`Public`, `Team`, `Personal`). | -| `Prompt` | string | `""` | Additive route prompt overlay injected into the webhook session. | -| `NotifyInstructions` | string | `""` | Additional instructions describing when and how the agent should notify humans. | -| `DeliveryRequired` | bool | `true` | Reminder-style delivery policy: when `true`, routes with notification instructions/targets fail if no notification is produced. | -| `NotificationTarget.Kind` | string | `Slack` | Human-facing notification channel type. Slack is the only implementation today. | -| `NotificationTarget.ChannelId` | string? | `null` | Slack channel ID used when the agent decides to notify. | -| `MaxBodyBytes` | int | `1048576` | Maximum accepted request-body size in bytes. Requests larger than this are rejected before dispatch. | -| `RateLimitPerMinute` | int | `30` | Maximum accepted deliveries per minute for this route. | +| `enabled` | bool | `true` | Enables or disables this specific route. | +| `verification.kind` | string | `Hmac` | Verification mode: `Hmac`, `HmacTimestamped`, or `HeaderSecret`. | +| `verification.hmacAlgorithm` | string | `Sha256` | HMAC hash algorithm. MVP supports `Sha256` only. | +| `verification.secret` | string? | `null` | Shared secret used for signature/header validation. Route files are secret-bearing config. | +| `verification.signatureHeaderName` | string? | `null` | Header name containing the HMAC signature. Defaults to `X-Webhook-Signature`. | +| `verification.signaturePrefix` | string? | `null` | Optional HMAC prefix such as `sha256=`. Defaults to empty string. | +| `verification.secretHeaderName` | string? | `null` | Header name for `HeaderSecret` mode. Defaults to `X-Webhook-Secret`. | +| `verification.eventHeaderName` | string? | `null` | Event-name header. Defaults to `X-Webhook-Event`. | +| `verification.deliveryIdHeaderName` | string? | `null` | Delivery ID header. Defaults to `X-Webhook-Delivery`. | +| `verification.toleranceSeconds` | int? | `300` | Maximum past or future clock difference for `HmacTimestamped`, from 1 through 3600 seconds. | +| `verification.timestampField` | string? | `t` | Structured-header timestamp field for `HmacTimestamped`; must be an ASCII HTTP token and differ from the signature field. | +| `verification.signatureField` | string? | `v1` | Structured-header signature field for `HmacTimestamped`; follows the same HTTP-token constraint, and multiple instances support sender secret rotation. | +| `verification.signedPayloadSeparator` | string? | `.` | Separator between the exact timestamp text and raw body for `HmacTimestamped`. | +| `events` | string[] | `[]` | Optional allow-list of event types. Empty means all verified events are accepted. | +| `audience` | string | `Public` | Source audience for the autonomous webhook session (`Public`, `Team`, `Personal`). | +| `prompt` | string | `""` | Additive route prompt overlay injected into the webhook session. | +| `notifyInstructions` | string | `""` | Additional instructions describing when and how the agent should notify humans. | +| `deliveryRequired` | bool | `true` | Reminder-style delivery policy: when `true`, routes with notification instructions/targets fail if no notification is produced. | +| `notificationTarget.kind` | string | `Slack` | Human-facing notification channel type. Slack is the only implementation today. | +| `notificationTarget.channelId` | string? | `null` | Slack channel ID used when the agent decides to notify. | +| `maxBodyBytes` | int | `1048576` | Maximum accepted request-body size in bytes. Requests larger than this are rejected before dispatch. | +| `rateLimitPerMinute` | int | `30` | Maximum accepted deliveries per minute for this route. | Route files are hot-reloaded on request. If a route file becomes missing, malformed, or invalid, Netclaw removes that route immediately and returns `404` diff --git a/src/Netclaw.Daemon.Tests/Webhooks/WebhookRequestVerifierTests.cs b/src/Netclaw.Daemon.Tests/Webhooks/WebhookRequestVerifierTests.cs index 4fc0212e0..2de9bb44c 100644 --- a/src/Netclaw.Daemon.Tests/Webhooks/WebhookRequestVerifierTests.cs +++ b/src/Netclaw.Daemon.Tests/Webhooks/WebhookRequestVerifierTests.cs @@ -235,6 +235,30 @@ public void TimestampedHmac_rejects_invalid_hex_signature_without_throwing() Assert.Equal("invalid_signature", result.RejectionReason); } + [Fact] + public void TimestampedHmac_rejects_unsupported_hmac_algorithm() + { + var timestamp = Now.ToUnixTimeSeconds().ToString(); + var body = Encoding.UTF8.GetBytes("{}"); + var route = CreateTimestampedRoute(new WebhookVerificationConfig + { + Kind = WebhookVerifierKind.HmacTimestamped, + HmacAlgorithm = (WebhookHmacAlgorithm)99, + Secret = new SensitiveString("super-secret"), + SignatureHeaderName = "Stripe-Signature" + }); + + var exception = Assert.Throws(() => _sut.Verify( + route, + new HeaderDictionary + { + ["Stripe-Signature"] = $"t={timestamp},v1={new string('0', 64)}" + }, + body)); + + Assert.Equal("algorithm", exception.ParamName); + } + private static RegisteredWebhookRoute CreateRoute(WebhookRouteConfig config) => new( "github-issues", diff --git a/src/Netclaw.Daemon/Webhooks/WebhookRequestVerifier.cs b/src/Netclaw.Daemon/Webhooks/WebhookRequestVerifier.cs index 91e9cb4fd..b1c995774 100644 --- a/src/Netclaw.Daemon/Webhooks/WebhookRequestVerifier.cs +++ b/src/Netclaw.Daemon/Webhooks/WebhookRequestVerifier.cs @@ -68,11 +68,14 @@ private WebhookVerificationResult VerifyTimestampedHmac( } var secret = route.Config.Verification.Secret!.Value; - var expected = ComputeTimestampedSha256( - secret, + var signedPayload = CreateTimestampedPayload( timestampText, route.SignedPayloadSeparator, bodyBytes); + var expected = ComputeHmac( + route.Config.Verification.HmacAlgorithm, + secret, + signedPayload); if (!signatures.Any(signature => IsMatchingHexSignature(expected, signature))) return WebhookVerificationResult.Reject("invalid_signature"); @@ -91,11 +94,10 @@ private static WebhookVerificationResult VerifyHmac( return WebhookVerificationResult.Reject("missing_signature"); var secret = route.Config.Verification.Secret!.Value; - var expected = route.Config.Verification.HmacAlgorithm switch - { - WebhookHmacAlgorithm.Sha256 => ComputeExpectedSha256(secret, bodyBytes, route.SignaturePrefix), - _ => throw new ArgumentOutOfRangeException(nameof(route.Config.Verification.HmacAlgorithm), route.Config.Verification.HmacAlgorithm, null) - }; + var hash = ComputeHmac(route.Config.Verification.HmacAlgorithm, secret, bodyBytes); + var expected = string.Concat( + route.SignaturePrefix, + Convert.ToHexString(hash).ToLowerInvariant()); if (!FixedTimeEquals(signature, expected)) return WebhookVerificationResult.Reject("invalid_signature"); @@ -128,15 +130,20 @@ private static bool FixedTimeEquals(string left, string right) return CryptographicOperations.FixedTimeEquals(leftBytes, rightBytes); } - private static string ComputeExpectedSha256(string secret, byte[] bodyBytes, string prefix) + private static byte[] ComputeHmac( + WebhookHmacAlgorithm algorithm, + string secret, + byte[] payload) { - using var hmac = new HMACSHA256(Encoding.UTF8.GetBytes(secret)); - var hash = Convert.ToHexString(hmac.ComputeHash(bodyBytes)).ToLowerInvariant(); - return string.Concat(prefix, hash); + using HMAC hmac = algorithm switch + { + WebhookHmacAlgorithm.Sha256 => new HMACSHA256(Encoding.UTF8.GetBytes(secret)), + _ => throw new ArgumentOutOfRangeException(nameof(algorithm), algorithm, null) + }; + return hmac.ComputeHash(payload); } - private static byte[] ComputeTimestampedSha256( - string secret, + private static byte[] CreateTimestampedPayload( string timestamp, string separator, byte[] bodyBytes) @@ -146,8 +153,7 @@ private static byte[] ComputeTimestampedSha256( Buffer.BlockCopy(prefixBytes, 0, signedPayload, 0, prefixBytes.Length); Buffer.BlockCopy(bodyBytes, 0, signedPayload, prefixBytes.Length, bodyBytes.Length); - using var hmac = new HMACSHA256(Encoding.UTF8.GetBytes(secret)); - return hmac.ComputeHash(signedPayload); + return signedPayload; } private static bool IsMatchingHexSignature(byte[] expected, string providedHex)