Add specs for custom webhook headers - #8403
Conversation
WalkthroughThis pull request adds specification documentation for custom HTTP headers functionality in webhooks. The addition includes a requirements checklist validating specification completeness and a detailed feature specification document. The specification outlines four user stories, thirteen functional requirements, and three key-value pair node types (Static, Password, Environment Variable). It documents the extension of the Webhook entity with a headers relationship, behavior for header merging with system defaults, sensitive value masking, and environment variable resolution at send time. No implementation code changes are included. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dev/specs/001-webhook-headers/spec.md (1)
81-81: Consider specifying the header count threshold.The edge case mentions "a very large number of custom headers" requiring an upper bound. For clarity, consider either defining a specific threshold in the requirements section (e.g., "System SHOULD enforce a limit of 50 custom headers per webhook") or noting that the threshold will be determined during implementation based on performance testing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/specs/001-webhook-headers/spec.md` at line 81, Update the spec 001-webhook-headers to explicitly state a header-count limit: add a requirement under the webhook custom headers section that the system SHALL enforce a maximum of 50 custom headers per webhook (or alternatively add a note that the threshold will be determined by performance testing and risk assessment if you prefer not to lock a number now); reference the spec ID "001-webhook-headers" and the phrase "custom headers" so implementers know where this limit applies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dev/specs/001-webhook-headers/spec.md`:
- Line 81: Update the spec 001-webhook-headers to explicitly state a
header-count limit: add a requirement under the webhook custom headers section
that the system SHALL enforce a maximum of 50 custom headers per webhook (or
alternatively add a note that the threshold will be determined by performance
testing and risk assessment if you prefer not to lock a number now); reference
the spec ID "001-webhook-headers" and the phrase "custom headers" so
implementers know where this limit applies.
4c731d4 to
a7fb10a
Compare
| ### Measurable Outcomes | ||
|
|
||
| - **SC-001**: Users can successfully send webhook requests to external systems requiring custom authentication headers (e.g., Ansible Automation Platform) without any workarounds or intermediary proxies. | ||
| - **SC-002**: Users can configure a webhook with custom headers and trigger a successful authenticated request to an external endpoint within 5 minutes of starting the configuration process. |
There was a problem hiding this comment.
Where does this 5 minute come from? Also I think we should clarify this a bit, what do we mean with starting the configuration process here? Shouldn't this be "within x time after a event triggers a webhook"?
There was a problem hiding this comment.
I've rephrased this a bit.
There was a problem hiding this comment.
In multiple places we are stating that sensitive values should be stored securely. Given how this is done in Infrahub today, wouldn't it be better to remove that from the spec? Similarly in multiple places it is mentioned that sensitive values would be masked, including the API response. Assuming we mean the GraphQL API here, which doesn't mask attribute values of kind Password, we should probably remove that requirement for the API?
There was a problem hiding this comment.
Yes good point. I've updated this part.
a7fb10a to
613756a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dev/specs/infp-445-webhook-headers/checklists/requirements.md`:
- Line 9: Update the checklist item whose label reads "No implementation details
(languages, frameworks, APIs)" to a less strict wording—either change it to "No
unnecessary implementation details (languages, frameworks, APIs)" or split into
two items like "Required platform/context only" and "No unnecessary
implementation details" so the spec can include necessary platform/API context;
apply the same wording change to the corresponding checklist entries referenced
at lines 35-36 so all related checkbox labels are consistent, and ensure the
brief guidance text (the learnings bullet) remains accurate with the new
phrasing.
In `@dev/specs/infp-445-webhook-headers/spec.md`:
- Line 77: Replace the ambiguous “last-associated value” behavior with a
deterministic precedence rule: add an explicit integer priority field on header
key-value records (higher priority wins), fall back to stable sort by updated_at
(newer wins) if priorities are equal or absent, and finally tie-break by
immutable ID (lower ID wins) to guarantee a total order; update all mentions
(including the header behavior description and any examples at the duplicated
locations) to state this precedence order: priority → updated_at → id.
- Line 76: Update the spec language so security-critical reserved headers (e.g.,
"webhook-signature", any HMAC/signature headers, and other system-reserved names
like "Content-Type") cannot be overridden by user-supplied custom headers;
replace the sentence that currently states "The user's custom header value takes
precedence, overriding the system default" with a rule that the system-reserved
header values always take precedence and that attempts to register custom
headers with those reserved names are rejected or ignored. Also update any
duplicate occurrences of that sentence/behavior (the blocks referencing
"Content-Type" and "webhook-signature"/HMAC) and any example lines to reflect
that custom headers must use non-reserved names and will not replace security
headers.
- Line 81: The spec currently contains conflicting statements — one requiring a
"header-count guardrail" and another saying "no limit initially" — so pick a
single explicit behavior (e.g., enforce a max custom header count such as
"maximum 100 custom headers per webhook") and update the document to remove the
contradictory sentence and replace it with the chosen requirement; ensure the
requirement text (the header-count guardrail) includes the exact numeric limit,
the error response/behavior when exceeded, and a test expectation so tests and
implementation (handlers that parse headers and the webhook validation logic)
can target the same behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dev/specs/infp-445-webhook-headers/checklists/requirements.mddev/specs/infp-445-webhook-headers/spec.md
|
|
||
| ## Content Quality | ||
|
|
||
| - [x] No implementation details (languages, frameworks, APIs) |
There was a problem hiding this comment.
Checklist claim is too strict for current spec content.
Line 9 says there are no implementation details, but the linked spec and notes include platform/API-specific context. Reword this checkbox to “No unnecessary implementation details” (or split into “required platform context only”) to keep readiness reporting accurate.
Based on learnings: “Validate documentation quality before submission: ensure content is accurate and reflects the latest version of Infrahub, instructions are clear with step-by-step guidance, Markdown formatting is correct and compliant with Infrahub's style, and spelling/grammar are checked”.
Also applies to: 35-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/specs/infp-445-webhook-headers/checklists/requirements.md` at line 9,
Update the checklist item whose label reads "No implementation details
(languages, frameworks, APIs)" to a less strict wording—either change it to "No
unnecessary implementation details (languages, frameworks, APIs)" or split into
two items like "Required platform/context only" and "No unnecessary
implementation details" so the spec can include necessary platform/API context;
apply the same wording change to the corresponding checklist entries referenced
at lines 35-36 so all related checkbox labels are consistent, and ensure the
brief guidance text (the learnings bullet) remains accurate with the new
phrasing.
|
|
||
| ### Edge Cases | ||
|
|
||
| - What happens when a custom header uses the same name as a system-reserved header (e.g., `Content-Type`, `webhook-signature`)? The user's custom header value takes precedence, overriding the system default. |
There was a problem hiding this comment.
Prevent overrides of security-critical reserved headers.
Line 76 and Line 92 currently allow overriding signature/security headers. Please reserve security-critical headers (e.g., webhook signature/HMAC headers) so custom headers cannot replace them.
Suggested spec adjustment
-- **FR-006**: System MUST merge custom headers with default system headers (Content-Type, Accept, HMAC signature headers) when sending webhook requests. In case of name conflicts, the user's custom header value MUST take precedence over the system default.
+- **FR-006**: System MUST merge custom headers with default system headers (Content-Type, Accept, HMAC signature headers) when sending webhook requests.
+- **FR-006a**: Security-critical reserved headers (including signature/HMAC headers) MUST NOT be overridden by user-defined headers.
+- **FR-006b**: For non-reserved headers, in case of name conflicts, the user-defined custom header value MUST take precedence over the system default.Also applies to: 92-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/specs/infp-445-webhook-headers/spec.md` at line 76, Update the spec
language so security-critical reserved headers (e.g., "webhook-signature", any
HMAC/signature headers, and other system-reserved names like "Content-Type")
cannot be overridden by user-supplied custom headers; replace the sentence that
currently states "The user's custom header value takes precedence, overriding
the system default" with a rule that the system-reserved header values always
take precedence and that attempts to register custom headers with those reserved
names are rejected or ignored. Also update any duplicate occurrences of that
sentence/behavior (the blocks referencing "Content-Type" and
"webhook-signature"/HMAC) and any example lines to reflect that custom headers
must use non-reserved names and will not replace security headers.
| ### Edge Cases | ||
|
|
||
| - What happens when a custom header uses the same name as a system-reserved header (e.g., `Content-Type`, `webhook-signature`)? The user's custom header value takes precedence, overriding the system default. | ||
| - What happens when multiple key-value pairs linked to the same webhook define the same header name? The system warns the administrator but allows it, using the last-associated value when sending the request. |
There was a problem hiding this comment.
Define deterministic precedence for duplicate header keys.
“Last-associated value” is ambiguous without an explicit ordering model. Please define deterministic precedence (e.g., explicit priority field or stable sort by updated_at) to avoid inconsistent runtime behavior.
Also applies to: 121-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/specs/infp-445-webhook-headers/spec.md` at line 77, Replace the ambiguous
“last-associated value” behavior with a deterministic precedence rule: add an
explicit integer priority field on header key-value records (higher priority
wins), fall back to stable sort by updated_at (newer wins) if priorities are
equal or absent, and finally tie-break by immutable ID (lower ID wins) to
guarantee a total order; update all mentions (including the header behavior
description and any examples at the duplicated locations) to state this
precedence order: priority → updated_at → id.
| - What happens when a key-value pair is deleted but still referenced by webhooks? The relationship should be cleanly removed and the webhook should continue to function without that header. | ||
| - What happens when the webhook cache contains stale header data after a header value is updated? Cache must be invalidated when header nodes or their webhook relationships change. | ||
| - What happens when an environment variable header references a variable name that contains special characters? The system should validate environment variable names follow standard conventions. | ||
| - How does the system behave when a webhook has a very large number of custom headers? A reasonable upper bound should be enforced to prevent abuse. |
There was a problem hiding this comment.
Reconcile conflicting header-limit statements.
Line 81 says a header-count guardrail should be enforced, while Line 115 says no limit initially. Please align these into one explicit requirement so tests and implementation target a single behavior.
Also applies to: 115-115
🧰 Tools
🪛 LanguageTool
[style] ~81-~81: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...es the system behave when a webhook has a very large number of custom headers? A reasonable ...
(EN_WEAK_ADJECTIVE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/specs/infp-445-webhook-headers/spec.md` at line 81, The spec currently
contains conflicting statements — one requiring a "header-count guardrail" and
another saying "no limit initially" — so pick a single explicit behavior (e.g.,
enforce a max custom header count such as "maximum 100 custom headers per
webhook") and update the document to remove the contradictory sentence and
replace it with the chosen requirement; ensure the requirement text (the
header-count guardrail) includes the exact numeric limit, the error
response/behavior when exceeded, and a test expectation so tests and
implementation (handlers that parse headers and the webhook validation logic)
can target the same behavior.
3 participants
Why
Adds specifications for Spec Kit for custom webhook headers INFP-INFP-445
What changed
How to review
Summary by CodeRabbit