Skip to content

Commit def44f1

Browse files
AchoArnoldCopilot
andcommitted
fix(api): stop embedding phone ID in adapter notification JWT claims
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
1 parent 4e29d1f commit def44f1

5 files changed

Lines changed: 14 additions & 13 deletions

File tree

api/pkg/services/http_notification_sender.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,11 @@ func (sender *HTTPNotificationSender) Send(
103103
}
104104

105105
// getAuthToken generates a JWT bearer token for the HTTPS adapter, signed with the phone ID
106-
// the same way webhook requests are signed with the webhook signing key.
106+
// the same way webhook requests are signed with the webhook signing key. The phone ID is only
107+
// used as the HMAC secret and is intentionally not embedded in any claim: the adapter already
108+
// knows which phone ID to verify against from its own gateway registration, and putting the
109+
// phone ID in a readable claim would let anyone who intercepts one token read the signing
110+
// secret and forge further tokens.
107111
func (sender *HTTPNotificationSender) getAuthToken(endpoint *url.URL, phoneID uuid.UUID) (string, error) {
108112
audience := *endpoint
109113
audience.User = nil
@@ -115,7 +119,6 @@ func (sender *HTTPNotificationSender) getAuthToken(endpoint *url.URL, phoneID uu
115119
IssuedAt: jwt.NewNumericDate(now),
116120
Issuer: notificationJWTIssuer,
117121
NotBefore: jwt.NewNumericDate(now.Add(-notificationJWTValidity)),
118-
Subject: phoneID.String(),
119122
})
120123
return token.SignedString([]byte(phoneID.String()))
121124
}

api/pkg/services/http_notification_sender_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,8 @@ func TestHTTPNotificationSenderSendsFCMCompatiblePayload(t *testing.T) {
6969
assert.True(t, token.Valid)
7070
claims, ok := token.Claims.(jwt.MapClaims)
7171
require.True(t, ok)
72-
assert.Equal(t, testNotificationPhoneID.String(), claims["sub"])
72+
assert.Empty(t, claims["sub"], "phone ID must not be embedded in a claim since it is also the signing secret")
73+
assert.Equal(t, "api.httpsms.com", claims["iss"])
7374

7475
return response(http.StatusNoContent, http.NoBody), nil
7576
}))

tests/adapter-emulator/emulator_test.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -487,7 +487,6 @@ func validNotificationToken(t *testing.T, phoneID string) string {
487487
IssuedAt: jwt.NewNumericDate(now),
488488
Issuer: notificationJWTIssuer,
489489
NotBefore: jwt.NewNumericDate(now.Add(-10 * time.Minute)),
490-
Subject: phoneID,
491490
})
492491
signed, err := token.SignedString([]byte(phoneID))
493492
if err != nil {

tests/adapter-emulator/notification_handler.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,9 @@ func notificationKind(data map[string]string) (kind string, messageID string, er
111111

112112
// verifyNotificationAuth validates the JWT the httpSMS API signs notification requests with,
113113
// using the gateway's phone ID as the HMAC-SHA256 secret (see
114-
// api/pkg/services/http_notification_sender.go getAuthToken).
114+
// api/pkg/services/http_notification_sender.go getAuthToken). The phone ID is never carried in
115+
// a token claim, only used as the secret, so verification relies on the gateway's own
116+
// registration to know which phone ID to check against rather than trusting a claim.
115117
func verifyNotificationAuth(authorization string, phoneID string) error {
116118
tokenString, ok := strings.CutPrefix(authorization, "Bearer ")
117119
if !ok || strings.TrimSpace(tokenString) == "" {
@@ -131,9 +133,6 @@ func verifyNotificationAuth(authorization string, phoneID string) error {
131133
if !token.Valid {
132134
return fmt.Errorf("token is not valid")
133135
}
134-
if claims.Subject != phoneID {
135-
return fmt.Errorf("subject mismatch")
136-
}
137136
if claims.Issuer != notificationJWTIssuer {
138137
return fmt.Errorf("issuer mismatch")
139138
}

tests/helpers_test.go

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -587,10 +587,9 @@ func assertWebhookJWT(t *testing.T, request wmJournal.Request, signingKey string
587587

588588
// assertAdapterNotificationJWT validates the JWT the API signs adapter notification requests
589589
// with, using the receiving phone's ID as the HMAC-SHA256 secret (see
590-
// api/pkg/services/http_notification_sender.go getAuthToken). The adapter emulator itself
591-
// rejects notifications with an invalid token (401), so a processed record with this header
592-
// recorded is already proof the signature validated; this assertion additionally checks the
593-
// claim shape from the test side.
590+
// api/pkg/services/http_notification_sender.go getAuthToken). The phone ID is only used as the
591+
// secret and is never embedded in a claim, so this only checks the signature and issuer, not a
592+
// subject; the adapter emulator itself rejects notifications with an invalid signature (401).
594593
func assertAdapterNotificationJWT(t *testing.T, record notificationRecord, phoneID string) {
595594
t.Helper()
596595

@@ -608,7 +607,7 @@ func assertAdapterNotificationJWT(t *testing.T, record notificationRecord, phone
608607
claims, ok := token.Claims.(jwt.MapClaims)
609608
require.True(t, ok, "cannot parse claims")
610609
require.Equal(t, "api.httpsms.com", claims["iss"], "issuer mismatch")
611-
require.Equal(t, phoneID, claims["sub"], "subject must be the receiving phone's ID")
610+
require.Empty(t, claims["sub"], "phone ID must not be embedded in a claim since it is also the signing secret")
612611

613612
exp, err := claims.GetExpirationTime()
614613
require.NoError(t, err)

0 commit comments

Comments
 (0)