diff --git a/pkg/common/tokens.go b/pkg/common/tokens.go index 141abfbf..2974ca66 100644 --- a/pkg/common/tokens.go +++ b/pkg/common/tokens.go @@ -112,6 +112,8 @@ func IdempotencyGUID(token string) string { // prior non-idempotent ID, e.g. a random GUID or a timestamp). Centralizing the // choice keeps each executor's PurchaseCommitment a single statement and avoids // repeating the same empty-token guard across every Azure service. +// +// Prefer OrderIDFromToken, which rejects a malformed non-empty token; this function is removed once providers/azure switches (#104). func ReservationOrderID(token, fallback string) string { if guid := IdempotencyGUID(token); guid != "" { return guid @@ -119,6 +121,30 @@ func ReservationOrderID(token, fallback string) string { return fallback } +// ErrInvalidIdempotencyToken is returned by OrderIDFromToken when a non-empty +// token cannot be turned into a reservation order GUID. +var ErrInvalidIdempotencyToken = errors.New("common: invalid idempotency token") + +// OrderIDFromToken returns the Azure reservationOrderID to PUT for a purchase: +// the deterministic GUID derived from token when one is valid (issue #641), or +// fallback when token is empty. +// +// A non-empty token that IdempotencyGUID cannot format (shorter than 32 chars, +// or non-hex in its first 32) returns an error wrapping +// ErrInvalidIdempotencyToken instead of the fallback: silently falling back +// would make a re-drive create a second reservation. The error masks the token +// with MaskToken. +func OrderIDFromToken(token, fallback string) (string, error) { + if token == "" { + return fallback, nil + } + guid := IdempotencyGUID(token) + if guid == "" { + return "", fmt.Errorf("%w: %s", ErrInvalidIdempotencyToken, MaskToken(token)) + } + return guid, nil +} + // MuteNotifScope is the set of valid notification scopes for per-recipient // muting (issue #297). Each scope corresponds to one category of outbound email; // a mute row suppresses only that category for its holder. diff --git a/pkg/common/tokens_test.go b/pkg/common/tokens_test.go index c17d609e..acc471a7 100644 --- a/pkg/common/tokens_test.go +++ b/pkg/common/tokens_test.go @@ -2,6 +2,7 @@ package common //nolint:revive // pkg/common is the module's core domain package import ( "encoding/hex" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -126,6 +127,48 @@ func TestReservationOrderID(t *testing.T) { assert.Equal(t, "fallback-id", ReservationOrderID("abc", "fallback-id")) } +func TestOrderIDFromToken(t *testing.T) { + hex32 := "0123456789abcdef0123456789abcdef" + derived := DeriveIdempotencyToken("exec-1", 0) + + accepted := []struct { + name, token, want string + }{ + {"empty returns fallback", "", "fallback-id"}, + {"exactly 32 lowercase hex", hex32, "01234567-89ab-cdef-0123-456789abcdef"}, + {"32 uppercase hex is lowercased", strings.ToUpper(hex32), "01234567-89ab-cdef-0123-456789abcdef"}, + {"64 hex uses first 32", hex32 + hex32, "01234567-89ab-cdef-0123-456789abcdef"}, + {"derived token", derived, IdempotencyGUID(derived)}, + } + for _, tc := range accepted { + t.Run("accepted/"+tc.name, func(t *testing.T) { + got, err := OrderIDFromToken(tc.token, "fallback-id") + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } + + rejected := []struct { + name, token string + }{ + {"short", "abc"}, + {"31 hex", hex32[:31]}, + {"32 non-hex", strings.Repeat("g", 32)}, + {"non-hex inside first 32 of 64", hex32[:10] + "z" + hex32[11:] + hex32}, + {"dashed uuid", "01234567-89ab-cdef-0123-456789abcdef"}, + } + for _, tc := range rejected { + t.Run("rejected/"+tc.name, func(t *testing.T) { + got, err := OrderIDFromToken(tc.token, "fallback-id") + require.Error(t, err) + assert.ErrorIs(t, err, ErrInvalidIdempotencyToken) + assert.Empty(t, got) + assert.NotContains(t, err.Error(), tc.token, "error must not echo the raw token") + assert.Contains(t, err.Error(), MaskToken(tc.token), "error must carry exactly the masked token") + }) + } +} + func TestDeriveMuteToken_EmptyKeyReturnsEmpty(t *testing.T) { // An empty key is a configuration error: the function must not silently sign // with a well-known fallback, so it yields the empty string.