Skip to content

add platform api changes for event gateway hmac secret generation#2237

Open
tharindu1st wants to merge 1 commit into
wso2:mainfrom
tharindu1st:platform-api-hmac-secret-generation
Open

add platform api changes for event gateway hmac secret generation#2237
tharindu1st wants to merge 1 commit into
wso2:mainfrom
tharindu1st:platform-api-hmac-secret-generation

Conversation

@tharindu1st

Copy link
Copy Markdown
Contributor

add platform api changes for event gateway hmac secret generation

@coderabbitai

coderabbitai Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Overview

This pull request adds support for HMAC secret generation and management for WebSub APIs. The changes implement a complete platform for creating, storing, listing, and managing HMAC secrets that are synchronized with the event gateway controller.

Platform API Changes

New HMAC Secret Management Endpoints

  • POST /websub-apis/{apiId}/secrets - Create a new HMAC secret with optional external secret value or auto-generated secret
  • GET /websub-apis/{apiId}/secrets - List HMAC secrets for a WebSub API (metadata only, plaintext excluded)
  • DELETE /websub-apis/{apiId}/secrets/{secretName} - Delete a specific HMAC secret
  • POST /websub-apis/{apiId}/secrets/{secretName}/regenerate - Regenerate the value for an existing secret

Internal Gateway Endpoint

  • GET /api/internal/v1/websub-apis/:apiId/secrets - Retrieve plaintext HMAC secrets for gateway consumption (authentication required)

Data Model

New database table websub_api_hmac_secrets stores secrets with:

  • Artifact UUID linkage with cascade deletion
  • Unique constraint on (artifact_uuid, name)
  • Encrypted secret storage with status tracking
  • Timestamps for audit purposes

Service Layer

WebSubAPIHmacSecretService provides:

  • Secret generation with configurable encryption
  • Persistence and retrieval operations
  • Event broadcasting to gateways on create/update/delete
  • Support for both auto-generated and externally-provided secrets

Gateway Controller Changes

Webhook Secret Synchronization

The gateway controller now:

  • Receives HMAC secret lifecycle events (websub.hmacsecret.created/updated/deleted)
  • Loads platform-managed secrets into an in-memory webhook secret store on WebSub API deployment
  • Updates the webhook secret xDS snapshot for Envoy when secrets change
  • Cleans up in-memory secrets on API deletion

Dependencies

Added webhook secret store and xDS snapshot manager dependencies to the control plane client for managing secret synchronization.

Configuration

  • Added support for HMAC secret encryption key configuration via environment variable
  • Implements key fallback chain using subscription token encryption key or JWT secret as alternatives

Code Quality

  • OpenAPI specification updated with new WebSub HMAC secret schemas and operations
  • Generated Go types reflect API changes (security scope updates, new response models, deprecated parameter structs)
  • Type safety maintained with new DTO models for request/response handling
  • Centralized error handling for HMAC secret operations with specific error types

Walkthrough

This PR adds end-to-end management of WebSub API HMAC secrets for inbound webhook verification. On the platform side, it introduces a websub_api_hmac_secrets database table (across PostgreSQL and SQLite schemas), a repository, a WebSubAPIHmacSecretService with AES encryption, random secret generation, slugified naming, and gateway event broadcasting, and new HTTP handlers for create/list/delete/regenerate operations. A gateway-internal endpoint decrypts and returns plaintext secrets for gateway consumption. The generated.go file is regenerated to include new HMAC secret and DevPortal types, with manually preserved types moved to manual_types.go. On the gateway-controller side, the control plane WebSocket client is extended to handle websub.hmacsecret.* events, fetch plaintext secrets from the platform internal API, and sync them into an in-memory store for xDS distribution.

Sequence Diagram

sequenceDiagram
  rect rgba(70, 130, 180, 0.5)
    Note over APIClient, WebSubAPIHmacSecretHandler: Platform API - Secret CRUD
    APIClient->>WebSubAPIHmacSecretHandler: POST /api/v1/websub-apis/:apiId/secrets
    WebSubAPIHmacSecretHandler->>WebSubAPIHmacSecretService: Generate(orgUUID, apiHandle, displayName, externalSecret)
    WebSubAPIHmacSecretService->>WebSubAPIHmacSecretRepo: Create(encrypted secret)
    WebSubAPIHmacSecretService->>GatewayEventsService: BroadcastWebSubAPIHmacSecretEvent(CREATED)
    WebSubAPIHmacSecretHandler-->>APIClient: 201 with plaintext secret (shown once)
  end

  rect rgba(60, 179, 113, 0.5)
    Note over GatewayEventsService, SnapshotManager: Gateway sync via WebSocket + xDS
    GatewayEventsService->>ControlplaneClient: WS event websub.hmacsecret.created
    ControlplaneClient->>APIUtilsService: FetchWebSubAPIHmacSecrets(artifactUUID)
    APIUtilsService->>GatewayInternalAPIHandler: GET /api/internal/v1/websub-apis/:apiId/secrets
    GatewayInternalAPIHandler->>WebSubAPIHmacSecretService: ListByArtifactUUID + DecryptSecret
    GatewayInternalAPIHandler-->>APIUtilsService: plaintext secrets
    APIUtilsService-->>ControlplaneClient: []HmacSecretInfo
    ControlplaneClient->>WebhookSecretStore: clear-and-replace secrets
    ControlplaneClient->>SnapshotManager: RefreshSnapshot (xDS)
  end
Loading

Suggested Reviewers

  • pubudu538
  • malinthaprasan
  • Krishanx92
  • RakhithaRR
  • Tharsanan1
  • VirajSalaka
  • dushaniw
  • AnuGayan
  • renuka-fernando
  • chamilaadhi
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request description is minimal and does not follow the provided template structure, lacking required sections such as Purpose, Goals, Approach, User stories, Documentation, Automation tests, and Security checks. Complete the pull request description using the provided template. Include Purpose (why), Goals (what), Approach (how), User stories, Documentation links, test coverage details, and security verification steps.
Docstring Coverage ⚠️ Warning Docstring coverage is 52.94% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title accurately describes the main change: adding platform API support for HMAC secret generation in the event gateway context.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies"


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Nitpick comments (2)
platform-api/src/internal/service/gateway_events.go (1)

192-203: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Reject unsupported actions instead of silently mapping to UPDATED.

Line 201 silently coerces invalid actions, which can produce incorrect event semantics. Prefer returning an error for unknown values.

Suggested change
 func (s *GatewayEventsService) BroadcastWebSubAPIHmacSecretEvent(gatewayID, action string, event *model.WebSubAPIHmacSecretEvent) error {
 	var eventType string
 	switch action {
 	case "CREATED":
 		eventType = EventTypeWebSubAPIHmacSecretCreated
 	case "UPDATED":
 		eventType = EventTypeWebSubAPIHmacSecretUpdated
 	case "DELETED":
 		eventType = EventTypeWebSubAPIHmacSecretDeleted
 	default:
-		eventType = EventTypeWebSubAPIHmacSecretUpdated
+		return fmt.Errorf("unsupported hmac secret action: %s", action)
 	}
 	return s.broadcastEvent(gatewayID, eventType, event)
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@platform-api/src/internal/service/gateway_events.go` around lines 192 - 203,
The default case in the switch statement within the
BroadcastWebSubAPIHmacSecretEvent method currently silently maps unknown actions
to EventTypeWebSubAPIHmacSecretUpdated, which masks unsupported input. Replace
the default case to return an error instead of coercing invalid actions,
ensuring that only the supported actions (CREATED, UPDATED, DELETED) are
processed and unknown actions are explicitly rejected with a descriptive error
message.
platform-api/src/api/generated.go (1)

473-478: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Inconsistent naming for WebSub visibility constants.

The generated constants Private and Public lack the type prefix, unlike the WebBroker equivalents (WebBrokerAPIDevPortalResponseVisibilityPrivate/Public). This may cause naming conflicts. If this is a code-generation artifact, consider updating the OpenAPI spec to ensure consistent naming.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@platform-api/src/api/generated.go` around lines 473 - 478, The constants
Private and Public for WebSubAPIDevPortalResponseVisibility lack the type prefix
that exists in the equivalent WebBrokerAPIDevPortalResponseVisibility constants,
creating potential naming conflicts. Rename the Private constant to
WebSubAPIDevPortalResponseVisibilityPrivate and the Public constant to
WebSubAPIDevPortalResponseVisibilityPublic to maintain consistency with the
WebBroker equivalents. If this is generated code from an OpenAPI spec, ensure
the spec is updated to produce consistent naming patterns across all visibility
constant definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@gateway/gateway-controller/pkg/controlplane/client.go`:
- Around line 2499-2512: The RemoveAllByAPI error handler currently logs a
warning but continues to the Store loop, which can leave old and new secrets
mixed together and break the intended clear-and-replace behavior. Modify the
error handling for the RemoveAllByAPI call on the webhookSecretStore to abort
the entire operation by returning early or skipping the subsequent for loop that
stores new secrets, ensuring that the Store operations only execute if the
RemoveAllByAPI call succeeds.

In `@platform-api/src/config/config.go`:
- Around line 179-183: The HMACSecretEncryptionKey field is missing the koanf
tag and corresponding mapping in the envToKoanfKey function. Add a koanf tag to
the HMACSecretEncryptionKey struct field following the naming pattern of other
fields, and add a case statement in the envToKoanfKey function to map the
environment variable DATABASE_HMAC_SECRET_ENCRYPTION_KEY to the appropriate
koanf key path (following the same pattern as SubscriptionTokenEncryptionKey).
This ensures the environment variable will be properly loaded by the koanf
config mechanism.

In `@platform-api/src/internal/handler/websub_api_hmac_secret.go`:
- Around line 239-241: In the case statement handling errors.Is(err,
constants.ErrHmacSecretEncryptionKeyMissing), change the HTTP status code from
http.StatusInternalServerError (500) to http.StatusServiceUnavailable (503) to
align with the OpenAPI spec which defines 503 as the response for when HMAC
secret management is not configured. Keep the error message the same.

In `@platform-api/src/internal/service/websub_api_hmac_secret.go`:
- Around line 91-97: The current logic in the slugifyHmacSecret function
truncates the name to 63 characters, but when the name becomes empty and a
fallback value is assigned (name = "secret-" + apiHandle), there is no length
enforcement on this fallback. After the fallback name assignment, reapply the
max-length truncation logic to ensure the final name does not exceed 63
characters, matching the enforcement applied before the fallback assignment.

In `@platform-api/src/resources/gateway-internal-api.yaml`:
- Around line 819-825: The OpenAPI specification for this endpoint is missing
the 503 Service Unavailable response definition even though the handler returns
this status when HMAC secret management is not configured. Add a 503 response
entry after the existing 500 response in the responses section, with appropriate
description documenting the service-not-configured state, and include the same
ErrorResponse schema reference to align the spec with the actual runtime
behavior.

In `@platform-api/src/resources/openapi.yaml`:
- Around line 5804-5809: The regenerate endpoint's requestBody references the
WebSubAPIHmacSecretRequest schema which requires a displayName field, but this
conflicts with the regenerate contract where only a new secret value should be
needed (since the secret is already identified by secretName in the path).
Create a new schema specifically for the regenerate request (for example,
WebSubAPIHmacSecretRegenerateRequest) that only requires the secret field
without displayName, then update the requestBody reference in the regenerate
endpoint to use this new schema instead of WebSubAPIHmacSecretRequest.

---

Nitpick comments:
In `@platform-api/src/api/generated.go`:
- Around line 473-478: The constants Private and Public for
WebSubAPIDevPortalResponseVisibility lack the type prefix that exists in the
equivalent WebBrokerAPIDevPortalResponseVisibility constants, creating potential
naming conflicts. Rename the Private constant to
WebSubAPIDevPortalResponseVisibilityPrivate and the Public constant to
WebSubAPIDevPortalResponseVisibilityPublic to maintain consistency with the
WebBroker equivalents. If this is generated code from an OpenAPI spec, ensure
the spec is updated to produce consistent naming patterns across all visibility
constant definitions.

In `@platform-api/src/internal/service/gateway_events.go`:
- Around line 192-203: The default case in the switch statement within the
BroadcastWebSubAPIHmacSecretEvent method currently silently maps unknown actions
to EventTypeWebSubAPIHmacSecretUpdated, which masks unsupported input. Replace
the default case to return an error instead of coercing invalid actions,
ensuring that only the supported actions (CREATED, UPDATED, DELETED) are
processed and unknown actions are explicitly rejected with a descriptive error
message.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 98e4f5a1-baa3-470d-922b-20c9c4999616

📥 Commits

Reviewing files that changed from the base of the PR and between 01d2cb2 and e6a92a7.

📒 Files selected for processing (24)
  • gateway/gateway-controller/cmd/controller/main.go
  • gateway/gateway-controller/pkg/controlplane/client.go
  • gateway/gateway-controller/pkg/controlplane/client_integration_test.go
  • gateway/gateway-controller/pkg/controlplane/controlplane_test.go
  • gateway/gateway-controller/pkg/utils/api_utils.go
  • platform-api/src/api/generated.go
  • platform-api/src/api/manual_types.go
  • platform-api/src/api/websub_hmac_secret.go
  • platform-api/src/config/config.go
  • platform-api/src/internal/constants/error.go
  • platform-api/src/internal/database/schema.postgres.sql
  • platform-api/src/internal/database/schema.sql
  • platform-api/src/internal/database/schema.sqlite.sql
  • platform-api/src/internal/dto/gateway_internal.go
  • platform-api/src/internal/handler/gateway_internal.go
  • platform-api/src/internal/handler/websub_api_hmac_secret.go
  • platform-api/src/internal/model/websub_api_hmac_secret.go
  • platform-api/src/internal/repository/interfaces.go
  • platform-api/src/internal/repository/websub_api_hmac_secret.go
  • platform-api/src/internal/server/server.go
  • platform-api/src/internal/service/gateway_events.go
  • platform-api/src/internal/service/websub_api_hmac_secret.go
  • platform-api/src/resources/gateway-internal-api.yaml
  • platform-api/src/resources/openapi.yaml

Comment on lines +2499 to +2512
if err := c.webhookSecretStore.RemoveAllByAPI(artifactID); err != nil {
c.logger.Warn("Failed to clear existing HMAC secrets for WebSub API",
slog.String("artifact_id", artifactID),
slog.Any("error", err))
}

for _, s := range secrets {
if err := c.webhookSecretStore.Store(artifactID, s.Name, s.Plaintext); err != nil {
c.logger.Warn("Failed to store platform HMAC secret in memory",
slog.String("artifact_id", artifactID),
slog.String("secret_name", s.Name),
slog.Any("error", err))
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Abort sync when clearing existing secrets fails.

If RemoveAllByAPI fails, continuing with Store(...) can leave stale and new entries mixed, which breaks the intended clear-and-replace behavior.

Suggested fix
 	if err := c.webhookSecretStore.RemoveAllByAPI(artifactID); err != nil {
 		c.logger.Warn("Failed to clear existing HMAC secrets for WebSub API",
 			slog.String("artifact_id", artifactID),
 			slog.Any("error", err))
+		return
 	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if err := c.webhookSecretStore.RemoveAllByAPI(artifactID); err != nil {
c.logger.Warn("Failed to clear existing HMAC secrets for WebSub API",
slog.String("artifact_id", artifactID),
slog.Any("error", err))
}
for _, s := range secrets {
if err := c.webhookSecretStore.Store(artifactID, s.Name, s.Plaintext); err != nil {
c.logger.Warn("Failed to store platform HMAC secret in memory",
slog.String("artifact_id", artifactID),
slog.String("secret_name", s.Name),
slog.Any("error", err))
}
}
if err := c.webhookSecretStore.RemoveAllByAPI(artifactID); err != nil {
c.logger.Warn("Failed to clear existing HMAC secrets for WebSub API",
slog.String("artifact_id", artifactID),
slog.Any("error", err))
return
}
for _, s := range secrets {
if err := c.webhookSecretStore.Store(artifactID, s.Name, s.Plaintext); err != nil {
c.logger.Warn("Failed to store platform HMAC secret in memory",
slog.String("artifact_id", artifactID),
slog.String("secret_name", s.Name),
slog.Any("error", err))
}
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@gateway/gateway-controller/pkg/controlplane/client.go` around lines 2499 -
2512, The RemoveAllByAPI error handler currently logs a warning but continues to
the Store loop, which can leave old and new secrets mixed together and break the
intended clear-and-replace behavior. Modify the error handling for the
RemoveAllByAPI call on the webhookSecretStore to abort the entire operation by
returning early or skipping the subsequent for loop that stores new secrets,
ensuring that the Store operations only execute if the RemoveAllByAPI call
succeeds.

Comment on lines +179 to +183

// HMACSecretEncryptionKey is the 32-byte key for AES-256-GCM encryption of WebSub API HMAC secrets.
// Provide as 64 hex chars or 44 base64 chars.
// Env: DATABASE_HMAC_SECRET_ENCRYPTION_KEY. If empty, falls back to SubscriptionTokenEncryptionKey then JWT_SECRET_KEY.
HMACSecretEncryptionKey string `envconfig:"HMAC_SECRET_ENCRYPTION_KEY" default:""`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check how other database encryption keys are loaded and if koanf tags are required
rg -n "SubscriptionTokenEncryptionKey|HMACSecretEncryptionKey" --type=go -C2

Repository: wso2/api-platform

Length of output: 3451


🏁 Script executed:

# Check how the config is loaded and if koanf tags are required
rg -n "envToKoanfKey|koanf\.|viper\.|envconfig" platform-api/src/config/ --type=go -A2 -B2

Repository: wso2/api-platform

Length of output: 2228


🏁 Script executed:

# Check the Database struct definition to see all fields and their tags
sed -n '/^type Database struct/,/^}/p' platform-api/src/config/config.go

Repository: wso2/api-platform

Length of output: 1106


🏁 Script executed:

# Search for the config loading function to understand the mechanism
rg -n "func GetConfig|func load|koanf.New|envconfig" platform-api/src/config/ --type=go -A5

Repository: wso2/api-platform

Length of output: 1727


🏁 Script executed:

# Get the full envToKoanfKey function to see all cases
sed -n '/^func envToKoanfKey/,/^}/p' platform-api/src/config/config.go

Repository: wso2/api-platform

Length of output: 6419


🏁 Script executed:

# Check if there's any mention of hmac_secret_encryption_key in the file
rg -n "hmac_secret_encryption_key" platform-api/src/config/config.go

Repository: wso2/api-platform

Length of output: 43


Add missing koanf tag and envToKoanfKey mapping for HMACSecretEncryptionKey.

The field uses only an envconfig tag, but the config loading mechanism uses koanf exclusively (not envconfig). The environment variable DATABASE_HMAC_SECRET_ENCRYPTION_KEY will not be loaded without both a koanf:"hmac_secret_encryption_key" tag and a corresponding case in the envToKoanfKey function:

case "database_hmac_secret_encryption_key":
    return "database.hmac_secret_encryption_key"

This follows the established pattern of SubscriptionTokenEncryptionKey and ensures the configuration is actually populated from the environment.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@platform-api/src/config/config.go` around lines 179 - 183, The
HMACSecretEncryptionKey field is missing the koanf tag and corresponding mapping
in the envToKoanfKey function. Add a koanf tag to the HMACSecretEncryptionKey
struct field following the naming pattern of other fields, and add a case
statement in the envToKoanfKey function to map the environment variable
DATABASE_HMAC_SECRET_ENCRYPTION_KEY to the appropriate koanf key path (following
the same pattern as SubscriptionTokenEncryptionKey). This ensures the
environment variable will be properly loaded by the koanf config mechanism.

Comment on lines +239 to +241
case errors.Is(err, constants.ErrHmacSecretEncryptionKeyMissing):
h.slogger.Error("HMAC secret encryption key is not configured")
c.JSON(http.StatusInternalServerError, utils.NewErrorResponse(500, "Internal Server Error", "HMAC secret management is not configured"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Consider returning 503 for ErrHmacSecretEncryptionKeyMissing.

The OpenAPI spec defines a 503 response for when HMAC secret management is not configured. Currently, ErrHmacSecretEncryptionKeyMissing returns 500. If this error can surface at runtime (not just startup), align with the spec:

 case errors.Is(err, constants.ErrHmacSecretEncryptionKeyMissing):
     h.slogger.Error("HMAC secret encryption key is not configured")
-    c.JSON(http.StatusInternalServerError, utils.NewErrorResponse(500, "Internal Server Error", "HMAC secret management is not configured"))
+    c.JSON(http.StatusServiceUnavailable, utils.NewErrorResponse(503, "Service Unavailable", "HMAC secret management is not configured on this server"))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
case errors.Is(err, constants.ErrHmacSecretEncryptionKeyMissing):
h.slogger.Error("HMAC secret encryption key is not configured")
c.JSON(http.StatusInternalServerError, utils.NewErrorResponse(500, "Internal Server Error", "HMAC secret management is not configured"))
case errors.Is(err, constants.ErrHmacSecretEncryptionKeyMissing):
h.slogger.Error("HMAC secret encryption key is not configured")
c.JSON(http.StatusServiceUnavailable, utils.NewErrorResponse(503, "Service Unavailable", "HMAC secret management is not configured on this server"))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@platform-api/src/internal/handler/websub_api_hmac_secret.go` around lines 239
- 241, In the case statement handling errors.Is(err,
constants.ErrHmacSecretEncryptionKeyMissing), change the HTTP status code from
http.StatusInternalServerError (500) to http.StatusServiceUnavailable (503) to
align with the OpenAPI spec which defines 503 as the response for when HMAC
secret management is not configured. Keep the error message the same.

Comment on lines +91 to +97
name := slugifyHmacSecret(displayName)
if len(name) > 63 {
name = name[:63]
}
if name == "" {
name = "secret-" + apiHandle
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Reapply max-length enforcement after fallback name assignment.

Line 96 sets a fallback after truncation logic, so the final name can exceed the 63-character limit in edge cases.

Suggested fix
 	name := slugifyHmacSecret(displayName)
-	if len(name) > 63 {
-		name = name[:63]
-	}
 	if name == "" {
-		name = "secret-" + apiHandle
+		name = slugifyHmacSecret("secret-" + apiHandle)
+	}
+	if len(name) > 63 {
+		name = name[:63]
 	}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@platform-api/src/internal/service/websub_api_hmac_secret.go` around lines 91
- 97, The current logic in the slugifyHmacSecret function truncates the name to
63 characters, but when the name becomes empty and a fallback value is assigned
(name = "secret-" + apiHandle), there is no length enforcement on this fallback.
After the fallback name assignment, reapply the max-length truncation logic to
ensure the final name does not exceed 63 characters, matching the enforcement
applied before the fallback assignment.

Comment on lines +819 to +825
'500':
description: Internal server error
content:
application/json:
schema:
$ref: '#/components/schemas/ErrorResponse'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Document the 503 response for service-not-configured state

The endpoint contract omits the 503 response even though the handler returns Service Unavailable when HMAC secret management is not configured. Please add 503 to keep the internal API spec aligned with runtime behavior.

Proposed spec update
       responses:
@@
         '500':
           description: Internal server error
           content:
             application/json:
               schema:
                 $ref: '`#/components/schemas/ErrorResponse`'
+        '503':
+          description: Service unavailable - HMAC secret management not configured
+          content:
+            application/json:
+              schema:
+                $ref: '`#/components/schemas/ErrorResponse`'
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@platform-api/src/resources/gateway-internal-api.yaml` around lines 819 - 825,
The OpenAPI specification for this endpoint is missing the 503 Service
Unavailable response definition even though the handler returns this status when
HMAC secret management is not configured. Add a 503 response entry after the
existing 500 response in the responses section, with appropriate description
documenting the service-not-configured state, and include the same ErrorResponse
schema reference to align the spec with the actual runtime behavior.

Comment on lines +5804 to +5809
requestBody:
required: false
content:
application/json:
schema:
$ref: '#/components/schemas/WebSubAPIHmacSecretRequest'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Decouple regenerate request from displayName requirement

Line 5805 makes the regenerate body optional, but if provided it references a schema that requires displayName (Line 11045). This conflicts with the regenerate contract, where {secretName} already identifies the secret and clients may only need to send a new secret value.

Proposed OpenAPI fix
   /websub-apis/{apiId}/secrets/{secretName}/regenerate:
     post:
@@
       requestBody:
         required: false
         content:
           application/json:
             schema:
-              $ref: '`#/components/schemas/WebSubAPIHmacSecretRequest`'
+              $ref: '`#/components/schemas/WebSubAPIHmacSecretRegenerateRequest`'
@@
     WebSubAPIHmacSecretRequest:
       type: object
       required:
         - displayName
@@
           minLength: 32
           example: my-external-secret-value-that-is-at-least-32-chars
+
+    WebSubAPIHmacSecretRegenerateRequest:
+      type: object
+      properties:
+        secret:
+          type: string
+          description: Optional externally supplied replacement secret.
+          minLength: 32
+          example: my-external-secret-value-that-is-at-least-32-chars

Also applies to: 11042-11058

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@platform-api/src/resources/openapi.yaml` around lines 5804 - 5809, The
regenerate endpoint's requestBody references the WebSubAPIHmacSecretRequest
schema which requires a displayName field, but this conflicts with the
regenerate contract where only a new secret value should be needed (since the
secret is already identified by secretName in the path). Create a new schema
specifically for the regenerate request (for example,
WebSubAPIHmacSecretRegenerateRequest) that only requires the secret field
without displayName, then update the requestBody reference in the regenerate
endpoint to use this new schema instead of WebSubAPIHmacSecretRequest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant