Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion api/pkg/services/emulator_fcm_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"firebase.google.com/go/messaging"
"github.com/NdoleStudio/httpsms/pkg/telemetry"
"github.com/NdoleStudio/stacktrace"
"github.com/google/uuid"
)

// EmulatorFCMClient sends FCM messages to the phone emulator via HTTP.
Expand Down Expand Up @@ -50,7 +51,7 @@ type emulatorFCMResponse struct {
}

// Send sends a message to the emulator's FCM endpoint.
func (c *EmulatorFCMClient) Send(ctx context.Context, message *messaging.Message) (string, error) {
func (c *EmulatorFCMClient) Send(ctx context.Context, message *messaging.Message, _ uuid.UUID) (string, error) {
payload := &emulatorFCMRequest{
Message: &emulatorFCMMessage{
Token: message.Token,
Expand Down
7 changes: 5 additions & 2 deletions api/pkg/services/fcm_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,15 @@ import (
"context"

"firebase.google.com/go/messaging"
"github.com/google/uuid"
)

// FCMClient sends Firebase-compatible messages through a phone notification transport.
type FCMClient interface {
// 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 receiving phone (the notification's target) and is used by HTTP
// adapter transports to sign the request.
Send(ctx context.Context, message *messaging.Message, phoneID uuid.UUID) (string, error)
Comment on lines 12 to +15

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 7896ab5: updated the doc comment - phoneID identifies the receiving/target phone (the one being notified), not the sending phone.

}

// FirebaseFCMClient wraps the real Firebase messaging.Client.
Expand All @@ -23,6 +26,6 @@ func NewFirebaseFCMClient(client *messaging.Client) *FirebaseFCMClient {
}

// Send sends a message via the real Firebase SDK.
func (c *FirebaseFCMClient) Send(ctx context.Context, message *messaging.Message) (string, error) {
func (c *FirebaseFCMClient) Send(ctx context.Context, message *messaging.Message, _ uuid.UUID) (string, error) {
return c.client.Send(ctx, message)
}
39 changes: 37 additions & 2 deletions api/pkg/services/http_notification_sender.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"context"
"encoding/json"
"errors"
"fmt"
"io"
"net/http"
"net/url"
Expand All @@ -14,13 +15,17 @@ import (
"github.com/NdoleStudio/httpsms/pkg/telemetry"
"github.com/NdoleStudio/stacktrace"
"github.com/avast/retry-go/v5"
"github.com/golang-jwt/jwt/v5"
"github.com/google/uuid"
)

const (
maxNotificationResponseDiscardBytes = 4 * 1024
notificationHTTPAttempts = 3
notificationHTTPTimeout = 5 * time.Second
notificationHTTPRetryDelay = 250 * time.Millisecond
notificationJWTIssuer = "api.httpsms.com"
notificationJWTValidity = 10 * time.Minute
)

// HTTPNotificationSender sends FCM-compatible gateway notifications to HTTPS adapters.
Expand Down Expand Up @@ -62,6 +67,7 @@ func newHTTPNotificationSenderWithRetrier(
func (sender *HTTPNotificationSender) Send(
ctx context.Context,
message *messaging.Message,
phoneID uuid.UUID,
) (string, error) {
if message == nil {
return "", sender.notificationError("", "notification message is nil")
Expand All @@ -78,8 +84,13 @@ func (sender *HTTPNotificationSender) Send(
return "", sender.notificationError(hostname, "cannot encode notification")
}

authToken, err := sender.getAuthToken(endpoint, phoneID)
if err != nil {
return "", sender.notificationError(hostname, "cannot generate notification auth token")
}

err = sender.retrier.Do(func() error {
return sender.deliver(ctx, endpoint, body)
return sender.deliver(ctx, endpoint, body, authToken)
})
if err == nil {
return "http/success", nil
Expand All @@ -91,6 +102,27 @@ func (sender *HTTPNotificationSender) Send(
return "", sender.notificationError(hostname, "notification request failed")
}

// getAuthToken generates a JWT bearer token for the HTTPS adapter, signed with the phone ID
// the same way webhook requests are signed with the webhook signing key. The phone ID is only
// used as the HMAC secret and is intentionally not embedded in any claim: the adapter already
// knows which phone ID to verify against from its own gateway registration, and putting the
// phone ID in a readable claim would let anyone who intercepts one token read the signing
// secret and forge further tokens.
func (sender *HTTPNotificationSender) getAuthToken(endpoint *url.URL, phoneID uuid.UUID) (string, error) {
audience := *endpoint
audience.User = nil

now := time.Now().UTC()
token := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.RegisteredClaims{
Audience: []string{audience.String()},
ExpiresAt: jwt.NewNumericDate(now.Add(notificationJWTValidity)),
IssuedAt: jwt.NewNumericDate(now),
Issuer: notificationJWTIssuer,
NotBefore: jwt.NewNumericDate(now.Add(-notificationJWTValidity)),
})
return token.SignedString([]byte(phoneID.String()))

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.

P1 security JWT signing key is exposed

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

}

func encodeHTTPNotificationPayload(message *messaging.Message) ([]byte, error) {
return json.Marshal(map[string]any{
"message": message,
Expand All @@ -101,6 +133,7 @@ func (sender *HTTPNotificationSender) deliver(
ctx context.Context,
endpoint *url.URL,
body []byte,
authToken string,
) error {
if err := ctx.Err(); err != nil {
return terminalNotificationRequestError{cause: err}
Expand All @@ -109,7 +142,7 @@ func (sender *HTTPNotificationSender) deliver(
attemptCtx, cancel := context.WithTimeout(ctx, sender.timeout)
defer cancel()

request, err := createHTTPNotificationRequest(attemptCtx, endpoint, body)
request, err := createHTTPNotificationRequest(attemptCtx, endpoint, body, authToken)
if err != nil {
return terminalNotificationRequestError{cause: err}
}
Expand All @@ -125,6 +158,7 @@ func createHTTPNotificationRequest(
ctx context.Context,
endpoint *url.URL,
body []byte,
authToken string,
) (*http.Request, error) {
request, err := http.NewRequestWithContext(
ctx,
Expand All @@ -136,6 +170,7 @@ func createHTTPNotificationRequest(
return nil, err
}
request.Header.Set("Content-Type", "application/json")
request.Header.Set("Authorization", fmt.Sprintf("Bearer %s", authToken))
return request, nil
}

Expand Down
54 changes: 47 additions & 7 deletions api/pkg/services/http_notification_sender_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,21 @@ import (
"net/http"
"net/url"
"reflect"
"strings"
"testing"
"time"

"firebase.google.com/go/messaging"
"github.com/NdoleStudio/httpsms/pkg/telemetry"
"github.com/golang-jwt/jwt/v5"
"github.com/google/uuid"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.opentelemetry.io/otel/trace"
)

var testNotificationPhoneID = uuid.New()

type roundTripFunc func(*http.Request) (*http.Response, error)

func (roundTrip roundTripFunc) RoundTrip(request *http.Request) (*http.Response, error) {
Expand Down Expand Up @@ -57,10 +62,20 @@ func TestHTTPNotificationSenderSendsFCMCompatiblePayload(t *testing.T) {
assert.Equal(t, "high", payload.Message.Android.Priority)
assert.Equal(t, "600s", payload.Message.Android.TTL)

token, err := jwt.Parse(strings.TrimPrefix(request.Header.Get("Authorization"), "Bearer "), func(*jwt.Token) (interface{}, error) {
return []byte(testNotificationPhoneID.String()), nil
})
require.NoError(t, err)
assert.True(t, token.Valid)
claims, ok := token.Claims.(jwt.MapClaims)
require.True(t, ok)
assert.Empty(t, claims["sub"], "phone ID must not be embedded in a claim since it is also the signing secret")
assert.Equal(t, "api.httpsms.com", claims["iss"])

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

result, err := sender.Send(context.Background(), message)
result, err := sender.Send(context.Background(), message, testNotificationPhoneID)

require.NoError(t, err)
assert.Equal(t, "http/success", result)
Expand Down Expand Up @@ -150,6 +165,7 @@ func TestHTTPNotificationSenderRetriesOnlyTransientFailures(t *testing.T) {
result, err := sender.Send(
context.Background(),
&messaging.Message{Token: "https://adapter.example.com/notify"},
testNotificationPhoneID,
)

if test.wantErr {
Expand Down Expand Up @@ -179,6 +195,7 @@ func TestHTTPNotificationSenderReusesRetrierAcrossSends(t *testing.T) {
_, err := sender.Send(
context.Background(),
&messaging.Message{Token: "https://adapter.example.com/notify"},
testNotificationPhoneID,
)
require.NoError(t, err)
}
Expand Down Expand Up @@ -206,6 +223,7 @@ func TestHTTPNotificationSenderCreatesFreshRequestAndBodyForEveryAttempt(t *test
Token: "https://adapter.example.com/notify",
Data: map[string]string{"KEY_MESSAGE_ID": "message-1"},
},
testNotificationPhoneID,
)

require.NoError(t, err)
Expand All @@ -227,6 +245,7 @@ func TestHTTPNotificationSenderBoundsResponseBodyDiscard(t *testing.T) {
_, err := sender.Send(
context.Background(),
&messaging.Message{Token: "https://adapter.example.com/notify"},
testNotificationPhoneID,
)

require.NoError(t, err)
Expand All @@ -253,6 +272,7 @@ func TestHTTPNotificationSenderOmitsTTLForHeartbeat(t *testing.T) {
Priority: "high",
},
},
testNotificationPhoneID,
)

require.NoError(t, err)
Expand All @@ -274,12 +294,29 @@ func TestHTTPNotificationSenderUsesInjectedHTTPClientUnchanged(t *testing.T) {
assert.Equal(t, time.Minute, sender.client.Timeout)
}

func TestHTTPNotificationSenderAllowsEndpointUserInformation(t *testing.T) {
func TestHTTPNotificationSenderIgnoresEndpointUserInformation(t *testing.T) {
// Adapter endpoints must not rely on HTTP basic auth embedded in the URL; the Authorization
// header always carries the phone-signed JWT instead.
sender := newHTTPNotificationSender(t, roundTripFunc(func(request *http.Request) (*http.Response, error) {
username, password, ok := request.BasicAuth()
assert.True(t, ok)
assert.Equal(t, "adapter-user", username)
assert.Equal(t, "adapter-password", password)
_, _, ok := request.BasicAuth()
assert.False(t, ok)

authorization := request.Header.Get("Authorization")
assert.True(t, strings.HasPrefix(authorization, "Bearer "))

token, err := jwt.Parse(strings.TrimPrefix(authorization, "Bearer "), func(*jwt.Token) (interface{}, error) {
return []byte(testNotificationPhoneID.String()), nil
})
require.NoError(t, err)
assert.True(t, token.Valid)
claims, ok := token.Claims.(jwt.MapClaims)
require.True(t, ok)
audience, err := claims.GetAudience()
require.NoError(t, err)
require.Len(t, audience, 1)
assert.NotContains(t, audience[0], "adapter-user")
assert.NotContains(t, audience[0], "adapter-password")

return response(http.StatusNoContent, http.NoBody), nil
}))
endpoint := &url.URL{
Expand All @@ -292,6 +329,7 @@ func TestHTTPNotificationSenderAllowsEndpointUserInformation(t *testing.T) {
_, err := sender.Send(
context.Background(),
&messaging.Message{Token: endpoint.String()},
testNotificationPhoneID,
)

require.NoError(t, err)
Expand All @@ -309,6 +347,7 @@ func TestHTTPNotificationSenderBoundsEveryAttemptByTimeout(t *testing.T) {
_, err := sender.Send(
context.Background(),
&messaging.Message{Token: "https://adapter.example.com/notify"},
testNotificationPhoneID,
)

require.Error(t, err)
Expand All @@ -328,6 +367,7 @@ func TestHTTPNotificationSenderStopsRetriesWhenParentContextIsCancelled(t *testi
_, err := sender.Send(
ctx,
&messaging.Message{Token: "https://adapter.example.com/notify"},
testNotificationPhoneID,
)

require.Error(t, err)
Expand All @@ -339,7 +379,7 @@ func TestHTTPNotificationSenderRejectsNilMessage(t *testing.T) {
return response(http.StatusNoContent, http.NoBody), nil
}))

_, err := sender.Send(context.Background(), nil)
_, err := sender.Send(context.Background(), nil, testNotificationPhoneID)

require.Error(t, err)
assert.Contains(t, err.Error(), "notification message is nil")
Expand Down
2 changes: 1 addition & 1 deletion api/pkg/services/phone_notification_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ func (service *PhoneNotificationService) sendPhoneNotification(
}

message.Token = strings.TrimSpace(*phone.FcmToken)
result, err := client.Send(ctx, message)
result, err := client.Send(ctx, message, phone.ID)
if err != nil {
return "", transport, stacktrace.Propagatef(
err,
Expand Down
4 changes: 4 additions & 0 deletions api/pkg/services/phone_notification_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ func (logger *phoneNotificationLogger) Printf(string, ...interface{}) {}

type recordingPhoneNotificationClient struct {
message *messaging.Message
phoneID uuid.UUID
result string
err error
calls int
Expand All @@ -98,9 +99,11 @@ type recordingPhoneNotificationClient struct {
func (client *recordingPhoneNotificationClient) Send(
_ context.Context,
message *messaging.Message,
phoneID uuid.UUID,
) (string, error) {
client.calls++
client.message = message
client.phoneID = phoneID
return client.result, client.err
}

Expand All @@ -122,6 +125,7 @@ func TestPhoneNotificationServiceSendPhoneNotificationUsesMappedClient(t *testin
assert.Equal(t, entities.NotificationTransportHTTP, transport)
assert.Equal(t, "https://adapter.example.com/notify", message.Token)
assert.Same(t, message, httpClient.message)
assert.Equal(t, phone.ID, httpClient.phoneID)
assert.Equal(t, 1, httpClient.calls)
}

Expand Down
6 changes: 4 additions & 2 deletions tests/adapter-emulator/control_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const maxControlBodyBytes = 1024 * 1024
type gatewayRegistration struct {
PhoneNumber string `json:"phone_number"`
PhoneAPIKey string `json:"phone_api_key"`
PhoneID string `json:"phone_id"`
}

type incomingMessageRequest struct {
Expand All @@ -39,8 +40,9 @@ func (instance *emulator) handleGatewayRegistration(writer http.ResponseWriter,
}
registration.PhoneNumber = strings.TrimSpace(registration.PhoneNumber)
registration.PhoneAPIKey = strings.TrimSpace(registration.PhoneAPIKey)
if registration.PhoneNumber == "" || registration.PhoneAPIKey == "" {
writeControlError(writer, http.StatusBadRequest, errors.New("phone_number and phone_api_key are required"))
registration.PhoneID = strings.TrimSpace(registration.PhoneID)
if registration.PhoneNumber == "" || registration.PhoneAPIKey == "" || registration.PhoneID == "" {
writeControlError(writer, http.StatusBadRequest, errors.New("phone_number, phone_api_key and phone_id are required"))
return
}

Expand Down
Loading
Loading