feat(api): sign HTTP adapter notifications with phone-ID JWT - #992
feat(api): sign HTTP adapter notifications with phone-ID JWT#992AchoArnold wants to merge 5 commits into
Conversation
Sign HTTPS adapter notification requests with a JWT the same way webhook requests are signed, using the phone ID (a UUID) as the HMAC secret instead of a per-webhook signing key. - FCMClient.Send now takes the sending phone's ID so HTTP-transport clients can generate a bearer token; Firebase/emulator clients ignore it. - HTTPNotificationSender signs a JWT (10 min validity, audience is the endpoint URL with any userinfo stripped) and sends it via the X-Httpsms-Signature header rather than Authorization, so adapters can still use HTTP basic auth embedded in the endpoint URL. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Send the phone-signed notification JWT via the standard Authorization header, matching webhook requests exactly, instead of a dedicated X-Httpsms-Signature header. Adapter endpoint URLs are no longer expected to carry HTTP basic auth credentials, since Authorization is now always used for the bearer JWT; update the corresponding test to assert basic auth from the URL is ignored. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This PR is not safe to merge until HTTP notification JWTs use a secret that cannot be recovered from the token itself. Findings
|
| NotBefore: jwt.NewNumericDate(now.Add(-notificationJWTValidity)), | ||
| Subject: phoneID.String(), | ||
| }) | ||
| return token.SignedString([]byte(phoneID.String())) |
There was a problem hiding this comment.
The token uses phoneID.String() as both its readable sub claim and its HS256 signing key. Anyone who obtains a notification token can read the phone ID without verifying the token, then use it to create tokens with arbitrary claims or expiration times. An adapter therefore cannot reliably distinguish genuine httpsms notifications from forged requests. Use a separate, non-public signing secret, as the webhook signer does.
How this was verified: The phone ID is embedded in the readable subject at line 118 and the identical value is used as the HMAC key at line 120.
There was a problem hiding this comment.
Fixed in def44f1 / 7896ab5: removed the \sub\ claim entirely instead of just picking a different secret. The adapter already knows which phone ID to verify against from its own gateway registration (not from a token claim), so there's no need to embed the phone ID anywhere in the token — it's now used only as the HMAC signing secret and never appears in a readable claim.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 1 critical |
🟢 Metrics 22 complexity · 30 duplication
Metric Results Complexity 22 Duplication 30
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
The adapter emulator now requires and validates the phone-ID-signed JWT (Authorization: Bearer) that the API sends with every FCM-compatible notification, mirroring the webhook JWT validation already used in integration tests. - adapter-emulator: gateway registration now requires phone_id; notification_handler verifies the JWT (HS256, sub==phone_id, iss==api.httpsms.com) before recording/processing, rejecting invalid/missing tokens with 401. - emulator_test.go: updated existing tests to register phone_id and send valid tokens; added negative tests for missing auth and wrong signing secret. - helpers_test.go: setupAdapterPhone now upserts the phone before registering the gateway (so phone_id is known), and adds an assertAdapterNotificationJWT helper mirroring assertWebhookJWT. - adapter_integration_test.go: asserts the JWT on recorded message and heartbeat notifications. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 45ed9de9-a3ad-41cf-ad32-ebec28d9771c
|
Updated the /tests\ integration suite to validate the new adapter-notification JWT auth:
Note: \go build/\go vet/unit tests pass in both the \ ests\ and \ ests/adapter-emulator\ modules, but the full docker-compose E2E integration suite wasn't run in this environment — CI should rebuild the adapter-emulator image (it now depends on \golang-jwt/jwt/v5) and run the full suite to confirm. |
Address PR review: the token used phoneID as both the readable sub claim and the HS256 signing secret, so anyone who saw one token could read the secret and forge further ones. The sub claim is unnecessary since the adapter already knows which phone ID to verify against from its own gateway registration, so it is removed; the phone ID remains the signing secret only. - http_notification_sender.go: getAuthToken no longer sets Subject. - adapter-emulator/notification_handler.go: verifyNotificationAuth no longer checks claims.Subject. - Updated tests in api/pkg/services and tests/ to assert sub is empty instead of equal to the phone ID. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 45ed9de9-a3ad-41cf-ad32-ebec28d9771c
There was a problem hiding this comment.
🔵 Needs a closer look
It changes security-sensitive request signing/authentication behavior for adapter notifications and should be validated by a human reviewer for correctness and compatibility.
Pull request overview
This PR updates the API’s HTTP adapter notification transport to sign every adapter notification request with a phone-ID–keyed JWT (mirroring the webhook Authorization: Bearer <token> approach), and updates the adapter emulator + integration tests to validate/consume that JWT.
Changes:
- Extend
FCMClient.Sendto accept aphoneIDso HTTP adapter transports can sign outgoing notifications. - Add JWT creation for HTTP adapter notification requests (issuer/subject/audience/time-window) and ensure userinfo in the endpoint URL is excluded from the JWT audience.
- Update the adapter emulator and integration tests to require and assert an
Authorizationbearer token on notifications.
File summaries
| File | Description |
|---|---|
| tests/helpers_test.go | Adds recording/assertion support for adapter notification Authorization header JWTs; reorders adapter phone setup so phone ID is available pre-registration. |
| tests/adapter-emulator/notification_handler.go | Enforces JWT verification on incoming notification callbacks and records the Authorization header. |
| tests/adapter-emulator/go.mod | Adds github.com/golang-jwt/jwt/v5 dependency for JWT verification in the emulator. |
| tests/adapter-emulator/go.sum | Locks checksum entries for the new JWT dependency. |
| tests/adapter-emulator/emulator.go | Persists PhoneID for registered gateways and stores Authorization header in notification records. |
| tests/adapter-emulator/emulator_test.go | Updates tests for new gateway PhoneID and adds auth rejection test cases + token helper. |
| tests/adapter-emulator/control_handler.go | Extends gateway registration payload validation to require phone_id. |
| tests/adapter_integration_test.go | Asserts adapter notifications include a valid phone-ID–signed JWT. |
| api/pkg/services/phone_notification_service.go | Passes phone.ID into the transport client when sending notifications. |
| api/pkg/services/phone_notification_service_test.go | Verifies the mapped notification client receives the correct phoneID. |
| api/pkg/services/http_notification_sender.go | Generates and attaches JWT bearer token to adapter notification HTTP requests (audience strips URL userinfo). |
| api/pkg/services/http_notification_sender_test.go | Adds assertions around JWT presence/claims and verifies userinfo isn’t relied upon for auth. |
| api/pkg/services/fcm_client.go | Updates FCMClient interface signature and adjusts Firebase client implementation accordingly. |
| api/pkg/services/emulator_fcm_client.go | Updates emulator FCM client to accept (and ignore) the new phoneID parameter. |
Review details
- Files reviewed: 13/14 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| claims := jwt.RegisteredClaims{} | ||
| token, err := jwt.ParseWithClaims(tokenString, &claims, func(token *jwt.Token) (interface{}, error) { | ||
| if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok { | ||
| return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"]) | ||
| } | ||
| return []byte(phoneID), nil | ||
| }) |
There was a problem hiding this comment.
Fixed in 7896ab5: verifyNotificationAuth now checks token.Method != jwt.SigningMethodHS256 instead of just asserting *jwt.SigningMethodHMAC, so HS384/HS512 (and any non-HMAC alg) are rejected. Added TestNotificationHandlerRejectsNonHS256SigningMethod to cover it.
| // Send sends a message and returns the transport's delivery identifier on success. | ||
| Send(ctx context.Context, message *messaging.Message) (string, error) | ||
| // phoneID identifies the sending phone and is used by HTTP adapter transports to sign the request. | ||
| Send(ctx context.Context, message *messaging.Message, phoneID uuid.UUID) (string, error) |
There was a problem hiding this comment.
Fixed in 7896ab5: updated the doc comment - phoneID identifies the receiving/target phone (the one being notified), not the sending phone.
- fcm_client.go: fix Send doc comment - phoneID identifies the receiving/target phone, not the sending phone. - adapter-emulator/notification_handler.go: verifyNotificationAuth now requires the HS256 signing method specifically instead of accepting any HMAC variant, matching what the API actually signs with. - emulator_test.go: added TestNotificationHandlerRejectsNonHS256SigningMethod covering the HS256-only check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 45ed9de9-a3ad-41cf-ad32-ebec28d9771c
Summary
Follow-up to #989 (already merged): sign every HTTP adapter notification request with a JWT,
the same claim shape and
Authorization: Bearer <token>header used for webhook requests, butsigned with the receiving phone's ID (a UUID) instead of a per-webhook signing key.
Details
FCMClient.Sendnow takes the sending phone's ID; the Firebase/emulator clients ignore it,HTTPNotificationSenderuses it to sign the request.sub= phone ID,aud= the adapter endpoint URL (userinfo stripped),iss=api.httpsms.com, 10 minute validity window, secret = phone ID string (HS256).Authorization: Bearer <token>, identical to webhook requests.(
https://user:pass@host/path) sinceAuthorizationis always used for the JWT now.HMAC-SHA256 secret.
Validation
cd api && go test ./... -count=1go vet ./...