feat: support standard webhook delivery mode

[alexluong]

This PR allows operators to send webhooks following the Standard Webhooks specification (https://www.standardwebhooks.com/).

Configuration

• DESTINATIONS_WEBHOOK_MODE: Set to "standard" to enable Standard Webhooks mode (defaults to "default")
• DESTINATIONS_WEBHOOK_HEADER_PREFIX: Customize header prefix (defaults to "webhook-" in standard mode, "x-outpost-" in default mode)

Spec

https://github.com/standard-webhooks/standard-webhooks/blob/main/spec/standard-webhooks.md

Example Headers

webhook-id: evt_123abc
webhook-timestamp: 1234567890
webhook-signature: v1,ZqFuVEt+GFBM5Bysq7W0liR22gpbVNv50qLOwYGdVqM=
webhook-topic: user.created

Implementation

The implementation imports some logic from destwebhook (signature generation) and duplicates other logic (validation, rotation, preprocessing). If we want to extend or maintain both providers moving forward, we should refactor
shared logic into common helpers to avoid drift.

The test suite imports the official Standard Webhooks Go SDK to verify webhook deliveries conform to the spec.

Notes

• ✅ Webhook secret format: Supports whsec_ prefix with base64-encoded secrets per spec
• ⚠️ Message ID format: The spec requires event IDs to start with "msg_" prefix. This PR uses the existing event ID format directly. Support for configurable event ID templates/generators will be addressed in a future PR.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
outpost-docs	Ready	Preview	Comment	Oct 12, 2025 5:32am
outpost-website	Ready	Preview	Comment	Oct 12, 2025 5:32am

[alexluong]

@claude can you review please?

[claude]

Claude finished @alexluong's task —— View job

---

Code Review

• Gather context and read changed files
• Review configuration changes
• Review main provider implementation
• Review test coverage and quality
• Check for security and performance issues
• Provide comprehensive feedback

---

Summary

This is a solid implementation that adds Standard Webhooks specification support to Outpost. The code is well-structured, thoroughly tested, and includes extensive documentation. The implementation correctly reuses existing signature infrastructure while properly enforcing the Standard Webhooks format requirements.

Strengths

✅ Excellent test coverage - Comprehensive validation, publishing, and integration tests including verification with the official Standard Webhooks SDK
✅ Good architecture - Clear separation between standard and default webhook modes with proper functional options pattern
✅ Strong documentation - Clear inline comments, instructions.md, and updated configuration docs
✅ Security conscious - Proper secret validation, cryptographically secure secret generation, and HMAC signature verification
✅ Backward compatible - Existing webhook provider remains unchanged, new mode is opt-in

Issues Found

🔴 Critical Issues

1. Response body consumed twice in error path (destwebhookstandard.go:536-548)

In the Publish method, when status code >= 400, the response body is read but not properly reset before parseResponse tries to read it again:

// Line 536
bodyBytes, _ := io.ReadAll(resp.Body)  // First read
delivery := &destregistry.Delivery{
    Status: "failed",
    Code:   fmt.Sprintf("%d", resp.StatusCode),
}
parseResponse(delivery, resp)  // Second read attempt - body is already consumed!

Fix: Either pass bodyBytes to parseResponse or don't read the body twice.

if resp.StatusCode >= 400 {
    bodyBytes, _ := io.ReadAll(resp.Body)
    delivery := &destregistry.Delivery{
        Status: "failed",
        Code:   fmt.Sprintf("%d", resp.StatusCode),
        Response: map[string]interface{}{
            "status": resp.StatusCode,
            "body":   string(bodyBytes),
        },
    }
    return delivery, destregistry.NewErrDestinationPublishAttempt(...)
}

🟡 Medium Priority Issues

2. Inconsistent metadata.json description (metadata.json:16)

The description states: "whsec_ secrets and msg_ prefixed message IDs" but the PR notes explicitly mention that message IDs do NOT follow the "msg_" prefix requirement:

⚠️ Message ID format: The spec requires event IDs to start with "msg_" prefix. This PR uses the existing event ID format directly.

Fix: Update the description to be accurate:

"description": "Send events as Standard Webhooks (https://www.standardwebhooks.com/) compliant webhooks with whsec_ secrets. Note: Message IDs use the existing event ID format."

3. Header prefix validation missing (destwebhookstandard.go:107-113)

The WithHeaderPrefix option doesn't validate that the prefix is appropriate. While flexibility is good, ensuring the prefix ends with - or _ would prevent configuration errors:

func WithHeaderPrefix(prefix string) Option {
    return func(d *StandardWebhookDestination) {
        if prefix != "" {
            // Validate prefix format
            if !strings.HasSuffix(prefix, "-") && !strings.HasSuffix(prefix, "_") {
                // Consider logging a warning or returning an error
            }
            d.headerPrefix = prefix
        }
    }
}

4. Default header prefix inconsistency (destinations.go:54, default.go:58)

The configuration documentation states the default is "webhook-" in standard mode, but the code sets this in two places which could drift:

• destinations.go:54 - config conversion doesn't set default
• destwebhookstandard.go:123 - provider sets "webhook-" default

Recommendation: Set the default consistently in one place (preferably in config conversion).

5. Missing error handling for credential parsing (destwebhookstandard.go:277)

The time.Parse error returns a validation error, but doesn't preserve the underlying error message which would help debugging:

if err != nil {
    return nil, nil, destregistry.NewErrDestinationValidation([]destregistry.ValidationErrorDetail{{
        Field: "credentials.previous_secret_invalid_at",
        Type:  "pattern",
        // Consider adding: Message: err.Error(),
    }})
}

🟢 Minor Issues / Suggestions

6. Redundant nil check (destwebhookstandard.go:250-254)

The triple condition check could be simplified with a helper function:

func hasCredentials(creds map[string]string) bool {
    return creds["secret"] != "" || creds["previous_secret"] != "" || creds["previous_secret_invalid_at"] != ""
}

7. Magic number for previous secret timestamp (destwebhookstandard.go:194)

The -1 * time.Hour is arbitrary. Consider making this configurable or at least documenting why 1 hour was chosen:

// Set to 1 hour before current secret for ordering
// The actual created time is unknown, so we use a reasonable default
secrets = append(secrets, destwebhook.WebhookSecret{
    Key:       parsedPrevSecret,
    CreatedAt: now.Add(-1 * time.Hour),
    InvalidAt: creds.PreviousSecretInvalidAt,
})

8. Duplicate response parsing logic (destwebhookstandard.go:626-648)

The parseResponse function has duplicate logic for JSON vs non-JSON responses. Consider extracting the common parts:

func parseResponse(delivery *destregistry.Delivery, resp *http.Response) {
    bodyBytes, _ := io.ReadAll(resp.Body)
    
    if strings.Contains(resp.Header.Get("Content-Type"), "application/json") {
        var response map[string]interface{}
        if err := json.Unmarshal(bodyBytes, &response); err == nil {
            delivery.Response = map[string]interface{}{
                "status": resp.StatusCode,
                "body":   response,
            }
            return
        }
    }
    
    // Fallback to string body
    delivery.Response = map[string]interface{}{
        "status": resp.StatusCode,
        "body":   string(bodyBytes),
    }
}

9. Test assertion helper could be more specific (assert_test.go:100)

The comment "Note: webhook-id format depends on event.ID format (user-provided)" suggests the test should verify this more explicitly rather than just checking non-empty.

Architecture & Design

Refactoring Opportunity (acknowledged in PR description)

The PR author correctly identifies code duplication between destwebhook and destwebhookstandard. The current approach is reasonable for initial implementation, but consider these refactoring opportunities:

1. Extract shared secret management logic into internal/destregistry/providers/shared/secrets
2. Create a shared credential preprocessor interface
3. Extract common HTTP client setup and response handling

Why not refactor now? The author's reasoning is sound - avoiding breaking changes and keeping the initial PR focused is the right call.

Configuration & Documentation

Well done:

• Configuration options are clearly documented
• The YAML config generation is automated
• Instructions.md provides clear verification steps

Suggestion: Add an example in the instructions.md showing how to verify webhooks using curl or a simple code snippet.

Test Coverage

Excellent coverage:

• Unit tests for secret validation, parsing, and generation
• Integration tests with test HTTP server
• Validation tests for all error paths
• Outstanding: Uses official Standard Webhooks SDK to verify compliance

Minor gap: No explicit test for the configurable header prefix feature. Consider adding:

t.Run("should use custom header prefix", func(t *testing.T) {
    // Test with prefix "x-custom-"
})

Security

Strong security posture:

• ✅ Cryptographically secure random secret generation
• ✅ Proper HMAC-SHA256 signature verification
• ✅ Base64 encoding validation
• ✅ Secret format validation with whsec_ prefix
• ✅ Role-based access control for credential updates

One consideration: Secrets are not obfuscated in ObfuscateDestination (line 149-156). The comment explains this is intentional for rotation logic, but ensure this is acceptable in your security model.

Performance

No significant concerns:

• Efficient reuse of SignatureManager
• Proper HTTP client reuse
• No blocking operations in hot paths

Minor optimization: Consider pooling the JSON marshaling buffers if webhook throughput becomes a bottleneck.

Verdict

This is high-quality code that's ready for merge after addressing the critical response body issue. The implementation follows Go best practices, has excellent test coverage, and properly adheres to the Standard Webhooks specification.

Recommended Actions

Must fix:

1. Fix the double body read in error path (feat: Set up project scaffolding with Docker Compose, Gin webserver, and Redis #1)

Should fix:
2. Update metadata.json description to be accurate (#2)
3. Add header prefix validation (#3)

Nice to have:
4. Simplify credential checking (#6)
5. Add test for custom header prefix
6. Document the 1-hour magic number (#7)

Great work! 🎉

---

Job Run | Branch: destwebhookstandard

[alexluong]

@claude addressed the main feedbacks in 1 & 2, the rest I believe are either non-critical or invalid, please double check. Can you also re-update your initial comment to using <details/> to hide the initial feedback to keep things more concise?

[claude]

Claude finished @alexluong's task —— View job

---

Verification Complete ✅

Fixed issues: #1 (critical), #2 (medium)

Summary: All critical and medium-priority issues from the original review have been addressed. The remaining items are either minor style preferences or observations that don't represent actual issues. The PR is ready for merge.

Original Review (click to expand)

Code Review

This is a solid implementation that adds Standard Webhooks specification support to Outpost. The code is well-structured, thoroughly tested, and includes extensive documentation. The implementation correctly reuses existing signature infrastructure while properly enforcing the Standard Webhooks format requirements.

Strengths

✅ Excellent test coverage - Comprehensive validation, publishing, and integration tests including verification with the official Standard Webhooks SDK
✅ Good architecture - Clear separation between standard and default webhook modes with proper functional options pattern
✅ Strong documentation - Clear inline comments, instructions.md, and updated configuration docs
✅ Security conscious - Proper secret validation, cryptographically secure secret generation, and HMAC signature verification
✅ Backward compatible - Existing webhook provider remains unchanged, new mode is opt-in

Issues Found

🔴 Critical Issues

1. Response body consumed twice in error path (destwebhookstandard.go:536-548) ✅ FIXED

In the Publish method, when status code >= 400, the response body is read but not properly reset before parseResponse tries to read it again:

// Line 536
bodyBytes, _ := io.ReadAll(resp.Body)  // First read
delivery := &destregistry.Delivery{
    Status: "failed",
    Code:   fmt.Sprintf("%d", resp.StatusCode),
}
parseResponse(delivery, resp)  // Second read attempt - body is already consumed!

Fix: Either pass bodyBytes to parseResponse or don't read the body twice.

if resp.StatusCode >= 400 {
    bodyBytes, _ := io.ReadAll(resp.Body)
    delivery := &destregistry.Delivery{
        Status: "failed",
        Code:   fmt.Sprintf("%d", resp.StatusCode),
        Response: map[string]interface{}{
            "status": resp.StatusCode,
            "body":   string(bodyBytes),
        },
    }
    return delivery, destregistry.NewErrDestinationPublishAttempt(...)
}

🟡 Medium Priority Issues

2. Inconsistent metadata.json description (metadata.json:16) ✅ FIXED

The description states: "whsec_ secrets and msg_ prefixed message IDs" but the PR notes explicitly mention that message IDs do NOT follow the "msg_" prefix requirement:

⚠️ Message ID format: The spec requires event IDs to start with "msg_" prefix. This PR uses the existing event ID format directly.

Fix: Update the description to be accurate:

"description": "Send events as Standard Webhooks (https://www.standardwebhooks.com/) compliant webhooks with whsec_ secrets. Note: Message IDs use the existing event ID format."

3. Header prefix validation missing (destwebhookstandard.go:107-113)

The WithHeaderPrefix option doesn't validate that the prefix is appropriate. While flexibility is good, ensuring the prefix ends with - or _ would prevent configuration errors:

func WithHeaderPrefix(prefix string) Option {
    return func(d *StandardWebhookDestination) {
        if prefix != "" {
            // Validate prefix format
            if !strings.HasSuffix(prefix, "-") && !strings.HasSuffix(prefix, "_") {
                // Consider logging a warning or returning an error
            }
            d.headerPrefix = prefix
        }
    }
}

4. Default header prefix inconsistency (destinations.go:54, default.go:58)

The configuration documentation states the default is "webhook-" in standard mode, but the code sets this in two places which could drift:

• destinations.go:54 - config conversion doesn't set default
• destwebhookstandard.go:123 - provider sets "webhook-" default

Recommendation: Set the default consistently in one place (preferably in config conversion).

5. Missing error handling for credential parsing (destwebhookstandard.go:277)

The time.Parse error returns a validation error, but doesn't preserve the underlying error message which would help debugging:

if err != nil {
    return nil, nil, destregistry.NewErrDestinationValidation([]destregistry.ValidationErrorDetail{{
        Field: "credentials.previous_secret_invalid_at",
        Type:  "pattern",
        // Consider adding: Message: err.Error(),
    }})
}

🟢 Minor Issues / Suggestions

6. Redundant nil check (destwebhookstandard.go:250-254)

The triple condition check could be simplified with a helper function:

func hasCredentials(creds map[string]string) bool {
    return creds["secret"] != "" || creds["previous_secret"] != "" || creds["previous_secret_invalid_at"] != ""
}

7. Magic number for previous secret timestamp (destwebhookstandard.go:194)

The -1 * time.Hour is arbitrary. Consider making this configurable or at least documenting why 1 hour was chosen:

// Set to 1 hour before current secret for ordering
// The actual created time is unknown, so we use a reasonable default
secrets = append(secrets, destwebhook.WebhookSecret{
    Key:       parsedPrevSecret,
    CreatedAt: now.Add(-1 * time.Hour),
    InvalidAt: creds.PreviousSecretInvalidAt,
})

8. Duplicate response parsing logic (destwebhookstandard.go:626-648)

The parseResponse function has duplicate logic for JSON vs non-JSON responses. Consider extracting the common parts:

func parseResponse(delivery *destregistry.Delivery, resp *http.Response) {
    bodyBytes, _ := io.ReadAll(resp.Body)
    
    if strings.Contains(resp.Header.Get("Content-Type"), "application/json") {
        var response map[string]interface{}
        if err := json.Unmarshal(bodyBytes, &response); err == nil {
            delivery.Response = map[string]interface{}{
                "status": resp.StatusCode,
                "body":   response,
            }
            return
        }
    }
    
    // Fallback to string body
    delivery.Response = map[string]interface{}{
        "status": resp.StatusCode,
        "body":   string(bodyBytes),
    }
}

9. Test assertion helper could be more specific (assert_test.go:100)

The comment "Note: webhook-id format depends on event.ID format (user-provided)" suggests the test should verify this more explicitly rather than just checking non-empty.

Architecture & Design

Refactoring Opportunity (acknowledged in PR description)

The PR author correctly identifies code duplication between destwebhook and destwebhookstandard. The current approach is reasonable for initial implementation, but consider these refactoring opportunities:

1. Extract shared secret management logic into internal/destregistry/providers/shared/secrets
2. Create a shared credential preprocessor interface
3. Extract common HTTP client setup and response handling

Why not refactor now? The author's reasoning is sound - avoiding breaking changes and keeping the initial PR focused is the right call.

Configuration & Documentation

Well done:

• Configuration options are clearly documented
• The YAML config generation is automated
• Instructions.md provides clear verification steps

Suggestion: Add an example in the instructions.md showing how to verify webhooks using curl or a simple code snippet.

Test Coverage

Excellent coverage:

• Unit tests for secret validation, parsing, and generation
• Integration tests with test HTTP server
• Validation tests for all error paths
• Outstanding: Uses official Standard Webhooks SDK to verify compliance

Minor gap: No explicit test for the configurable header prefix feature. Consider adding:

t.Run("should use custom header prefix", func(t *testing.T) {
    // Test with prefix "x-custom-"
})

Security

Strong security posture:

• ✅ Cryptographically secure random secret generation
• ✅ Proper HMAC-SHA256 signature verification
• ✅ Base64 encoding validation
• ✅ Secret format validation with whsec_ prefix
• ✅ Role-based access control for credential updates

One consideration: Secrets are not obfuscated in ObfuscateDestination (line 149-156). The comment explains this is intentional for rotation logic, but ensure this is acceptable in your security model.

Performance

No significant concerns:

• Efficient reuse of SignatureManager
• Proper HTTP client reuse
• No blocking operations in hot paths

Minor optimization: Consider pooling the JSON marshaling buffers if webhook throughput becomes a bottleneck.

Verdict

This is high-quality code that's ready for merge after addressing the critical response body issue. The implementation follows Go best practices, has excellent test coverage, and properly adheres to the Standard Webhooks specification.

Issue Re-assessment (click to expand)

Issue #3: Header prefix validation

Status: Valid suggestion, but not critical for merge.

The WithHeaderPrefix function doesn't validate the prefix format. While adding validation would prevent misconfiguration, this is:

• A configuration error that would be immediately obvious during testing
• Not a security or data corruption risk
• Easy to fix if encountered
• An internal API (not user-facing)

Recommendation: Can be addressed in a future improvement PR if needed.

Issue #4: Default header prefix inconsistency

Status: Valid observation, but the current implementation is correct.

The default is set in one place (line 123: headerPrefix: "webhook-"), which is the correct approach. The config conversion doesn't need to set defaults - that's the provider's responsibility.

Recommendation: Current implementation is fine. No action needed.

Issue #5: Missing error context in credential parsing

Status: Not an issue in practice.

While preserving the underlying error message could help debugging, validation errors in this codebase follow a consistent pattern of returning structured error types without embedding the parse error. This is:

• Consistent with the codebase's error handling patterns
• The pattern type "pattern" clearly indicates a format issue
• The RFC3339 format requirement is documented

Recommendation: No change needed - follows codebase conventions.

Issue #6: Redundant nil check

Status: Valid style suggestion, but not a functional issue.

The triple condition check could be simplified, but:

• It's perfectly readable as-is
• It's explicit about what's being checked
• No performance or correctness impact
• Style preference, not a defect

Recommendation: Optional refactoring for future PR.

Issue #7: Magic number for previous secret timestamp

Status: Valid observation, but the choice is reasonable.

The -1 * time.Hour is arbitrary but reasonable since:

• The actual created time is unknown
• 1 hour is a sensible default for ordering
• There's already a comment explaining the reasoning
• Making it configurable adds complexity without clear benefit

Recommendation: Current implementation with comment is sufficient.

Issue #8: Duplicate response parsing logic

Status: Not actually duplicated.

The parseResponse function doesn't have duplicated logic - it has a single flow:

1. Check content type
2. Try JSON parse if applicable
3. Fall back to string body
4. Set delivery.Response appropriately

The suggested refactoring doesn't actually reduce duplication or improve clarity.

Recommendation: Current implementation is clean and correct.

Issue #9: Test assertion could be more specific

Status: Misunderstands the test's purpose.

The comment notes that the webhook-id format depends on the event ID format, which is correct. The test checking for non-empty is the right level of validation because:

• The event ID format is defined elsewhere
• This test is validating that the header is present, not the ID format
• Testing the ID format would duplicate other tests

Recommendation: Test is correctly scoped.

---

Job Run | Branch: destwebhookstandard