From 6428037208800c995a9785945f22252c3aec81d9 Mon Sep 17 00:00:00 2001 From: Henry Stern Date: Sun, 21 Jun 2026 13:08:29 -0300 Subject: [PATCH 1/2] Make Parse return value-canonical Subject Identifier form Registry constructors return pointers so the codec can populate them via UnmarshalJSON, but Parse previously leaked that pointer form. A caller constructs identifiers as value literals (IssSubID{}) yet read them back as *IssSubID, so a direct type assertion silently took the !ok branch on whichever form it did not expect. Normalize Parse to the value form: a reflection-based deref dereferences the populated pointer before returning, applied to the built-ins, the UnknownFormat carrier, and every nested aliases element. deref falls back to the original pointer for an extension type registered with pointer-receiver methods rather than panicking. Parse and the SubjectIdentifier interface godoc now document the canonical value form and the extension-type value-receiver constraint. Also make AliasesID.Validate detect nesting by the format discriminator (Format() == "aliases") instead of a value type assertion. The old check missed a nested aliases identifier arriving from the wire, which Parse populates in pointer form, so ErrNestedAliases silently failed to fire on real input; a hand-built value-form nested alias was still caught. Switch the OpaqueID and UnknownFormat compile-time assertions to value form to match the other built-ins. Co-Authored-By: Claude Opus 4.8 (1M context) --- aliases.go | 6 +++- canonical_form_test.go | 64 ++++++++++++++++++++++++++++++++++++++++++ conformance_test.go | 2 +- example_test.go | 4 +-- extension_test.go | 4 +-- forward_compat_test.go | 16 +++++------ opaque.go | 5 ++-- registry.go | 8 ++++++ subjectid.go | 8 ++++++ unknown.go | 5 ++-- unmarshal.go | 41 +++++++++++++++++++++++++-- unmarshal_test.go | 56 ++++++++++++++++++------------------ 12 files changed, 169 insertions(+), 50 deletions(-) create mode 100644 canonical_form_test.go diff --git a/aliases.go b/aliases.go index ab68d28..600e897 100644 --- a/aliases.go +++ b/aliases.go @@ -49,7 +49,11 @@ func (a AliasesID) Validate() error { } errs := make([]error, 0, len(a.Identifiers)) for _, id := range a.Identifiers { - if _, ok := id.(AliasesID); ok { + // Detect nesting by the format discriminator rather than a + // concrete-type assertion: an inner aliases identifier parsed + // from the wire and one built as a Go literal must both be + // caught regardless of their dynamic pointer/value form. + if id.Format() == "aliases" { errs = append(errs, ErrNestedAliases) continue } diff --git a/canonical_form_test.go b/canonical_form_test.go new file mode 100644 index 0000000..f4c1e5c --- /dev/null +++ b/canonical_form_test.go @@ -0,0 +1,64 @@ +// Copyright 2026 The go-subjectid Authors +// SPDX-License-Identifier: Apache-2.0 + +package subjectid_test + +import ( + "errors" + "testing" + + "github.com/hstern/go-subjectid" +) + +// TestParseReturnsValueForm pins the canonical-form decision: +// Parse returns the value form of an identifier, so the +// dynamic type a caller reads back from Parse is identical to the +// value literal they would write by hand (subjectid.IssSubID, not +// *subjectid.IssSubID). A direct value assertion must succeed and the +// pointer assertion must fail. +func TestParseReturnsValueForm(t *testing.T) { + id, err := subjectid.Parse([]byte( + `{"format":"iss_sub","iss":"https://idp.example.com/","sub":"alice"}`)) + if err != nil { + t.Fatalf("Parse: %v", err) + } + if _, ok := id.(subjectid.IssSubID); !ok { + t.Fatalf("Parse returned %T, want value form subjectid.IssSubID", id) + } + if _, ok := id.(*subjectid.IssSubID); ok { + t.Fatalf("Parse returned pointer form %T, want value-canonical", id) + } +} + +// TestParseUnknownReturnsValueForm checks that the forward-compat +// UnknownFormat carrier also escapes Parse in value form, consistent +// with the canonical-form rule. +func TestParseUnknownReturnsValueForm(t *testing.T) { + id, err := subjectid.Parse([]byte( + `{"format":"org.example.unknown","opaque_id":"xyz"}`)) + if err != nil { + t.Fatalf("Parse: %v", err) + } + if _, ok := id.(subjectid.UnknownFormat); !ok { + t.Fatalf("Parse returned %T, want value form subjectid.UnknownFormat", id) + } +} + +// TestParseNestedAliasesFromWireRejected guards the latent bug the +// pointer/value split caused: a nested aliases identifier arriving +// from the wire (which Parse populates recursively) must be rejected +// by Validate with ErrNestedAliases, exactly as a hand-built nested +// alias is. Before the canonical-form fix the value-typed nested +// check missed the pointer-form wire element and this slipped through. +func TestParseNestedAliasesFromWireRejected(t *testing.T) { + raw := []byte(`{"format":"aliases","identifiers":[` + + `{"format":"aliases","identifiers":[` + + `{"format":"email","email":"user@example.com"}]}]}`) + id, err := subjectid.Parse(raw) + if err != nil { + t.Fatalf("Parse: %v", err) + } + if err := id.Validate(); !errors.Is(err, subjectid.ErrNestedAliases) { + t.Fatalf("Validate() of wire-parsed nested aliases = %v, want ErrNestedAliases", err) + } +} diff --git a/conformance_test.go b/conformance_test.go index e7f0754..f70e4e0 100644 --- a/conformance_test.go +++ b/conformance_test.go @@ -61,7 +61,7 @@ func TestSpecFixturesRoundTripByteStable(t *testing.T) { if got, want := id.Format(), fx.Format; got != want { t.Errorf("parsed Format() = %q, want %q", got, want) } - if _, isUnknown := id.(*subjectid.UnknownFormat); isUnknown { + if _, isUnknown := id.(subjectid.UnknownFormat); isUnknown { t.Errorf("parsed into UnknownFormat; built-in dispatch did not fire") } if verr := id.Validate(); verr != nil { diff --git a/example_test.go b/example_test.go index 4fa91c7..d01d32a 100644 --- a/example_test.go +++ b/example_test.go @@ -23,7 +23,7 @@ func ExampleParse() { panic(err) } - email := id.(*subjectid.EmailID) + email := id.(subjectid.EmailID) fmt.Println(id.Format(), email.Email) // Output: email user@example.com } @@ -40,7 +40,7 @@ func ExampleParse_unknownFormat() { panic(err) } - unk := id.(*subjectid.UnknownFormat) + unk := id.(subjectid.UnknownFormat) out, _ := json.Marshal(unk) fmt.Println(unk.FormatName) fmt.Println(string(out)) diff --git a/extension_test.go b/extension_test.go index 1a3de3c..9a7bc5e 100644 --- a/extension_test.go +++ b/extension_test.go @@ -94,9 +94,9 @@ func TestRegisterFormatExtensionSmoke(t *testing.T) { if err != nil { t.Fatalf("Parse: %v", err) } - tenant, ok := id.(*orgTenantID) + tenant, ok := id.(orgTenantID) if !ok { - t.Fatalf("Parse returned %T, want *orgTenantID", id) + t.Fatalf("Parse returned %T, want orgTenantID", id) } if got, want := tenant.Tenant, "acme-prod"; got != want { t.Errorf("Tenant = %q, want %q", got, want) diff --git a/forward_compat_test.go b/forward_compat_test.go index d97f2c4..70111f1 100644 --- a/forward_compat_test.go +++ b/forward_compat_test.go @@ -35,9 +35,9 @@ func TestForwardCompatUnknownFormatRoundTrip(t *testing.T) { t.Fatalf("Parse(unknown format): %v", err) } - unk, ok := id.(*subjectid.UnknownFormat) + unk, ok := id.(subjectid.UnknownFormat) if !ok { - t.Fatalf("Parse returned %T, want *UnknownFormat", id) + t.Fatalf("Parse returned %T, want subjectid.UnknownFormat", id) } if got, want := unk.Format(), "org.example.future"; got != want { t.Errorf("Format() = %q, want %q", got, want) @@ -78,19 +78,17 @@ func TestForwardCompatUnknownFormatInsideAliases(t *testing.T) { if err != nil { t.Fatalf("Parse(aliases with unknown inner): %v", err) } - aliases, ok := id.(*subjectid.AliasesID) + aliases, ok := id.(subjectid.AliasesID) if !ok { - t.Fatalf("Parse returned %T, want *AliasesID", id) + t.Fatalf("Parse returned %T, want subjectid.AliasesID", id) } if got, want := len(aliases.Identifiers), 2; got != want { t.Fatalf("len(Identifiers) = %d, want %d", got, want) } if _, ok := aliases.Identifiers[0].(subjectid.EmailID); !ok { - if _, ok := aliases.Identifiers[0].(*subjectid.EmailID); !ok { - t.Errorf("Identifiers[0] = %T, want EmailID (value or pointer)", aliases.Identifiers[0]) - } + t.Errorf("Identifiers[0] = %T, want subjectid.EmailID", aliases.Identifiers[0]) } - if _, ok := aliases.Identifiers[1].(*subjectid.UnknownFormat); !ok { - t.Errorf("Identifiers[1] = %T, want *UnknownFormat", aliases.Identifiers[1]) + if _, ok := aliases.Identifiers[1].(subjectid.UnknownFormat); !ok { + t.Errorf("Identifiers[1] = %T, want subjectid.UnknownFormat", aliases.Identifiers[1]) } } diff --git a/opaque.go b/opaque.go index 2fffecb..d4dc0e1 100644 --- a/opaque.go +++ b/opaque.go @@ -36,5 +36,6 @@ func (o OpaqueID) Validate() error { func (OpaqueID) sealed() {} -// Compile-time assertion that OpaqueID satisfies SubjectIdentifier. -var _ SubjectIdentifier = (*OpaqueID)(nil) +// Compile-time assertion that OpaqueID satisfies SubjectIdentifier +// in value form — the canonical dynamic form Parse returns. +var _ SubjectIdentifier = OpaqueID{} diff --git a/registry.go b/registry.go index f28e6c4..b7f1e91 100644 --- a/registry.go +++ b/registry.go @@ -15,6 +15,14 @@ import ( // // Constructors registered for non-built-in formats must return a // type that embeds [Seal] so it satisfies the sealed interface. +// +// The returned value is conventionally a pointer so the codec can +// populate it via UnmarshalJSON, but [Parse] normalizes the result +// to its canonical value form before returning it. The extension +// type must therefore satisfy [SubjectIdentifier] with value +// receivers, so the dereferenced value still implements the +// interface; a type whose methods use pointer receivers would not +// survive that normalization. type Constructor func() SubjectIdentifier // formatRegistry is the package-global dispatch table. It is diff --git a/subjectid.go b/subjectid.go index 638730c..44d9ba2 100644 --- a/subjectid.go +++ b/subjectid.go @@ -36,6 +36,14 @@ const SpecVersion = "RFC 9493" // later commit) rather than implementing the interface directly: // registration feeds the codec dispatch table, whereas a direct // implementation would be invisible to it. +// +// Canonical dynamic form: every value this package produces is the +// concrete value type, never a pointer to it. [Parse] returns +// [IssSubID], not *IssSubID, so the dynamic type read back from +// Parse is identical to the struct literal a caller writes by hand. +// Consumers branching with a type switch or assertion should match +// the value forms (case IssSubID, case AliasesID, …); the pointer +// forms never occur on a value obtained from this package's API. type SubjectIdentifier interface { // Format returns the IANA "format" discriminator for this // identifier — "email", "iss_sub", "aliases", and so on. The diff --git a/unknown.go b/unknown.go index 5f98d62..49c2b79 100644 --- a/unknown.go +++ b/unknown.go @@ -45,5 +45,6 @@ func (UnknownFormat) Validate() error { return nil } func (UnknownFormat) sealed() {} -// Compile-time assertion that UnknownFormat satisfies SubjectIdentifier. -var _ SubjectIdentifier = (*UnknownFormat)(nil) +// Compile-time assertion that UnknownFormat satisfies SubjectIdentifier +// in value form — the canonical dynamic form Parse returns. +var _ SubjectIdentifier = UnknownFormat{} diff --git a/unmarshal.go b/unmarshal.go index 736f78c..3ee7de6 100644 --- a/unmarshal.go +++ b/unmarshal.go @@ -6,6 +6,7 @@ package subjectid import ( "encoding/json" "fmt" + "reflect" ) // envelope is the wire shape Parse peeks at first to discover @@ -16,9 +17,43 @@ type envelope struct { Format string `json:"format"` } +// deref normalizes a freshly-parsed identifier to its canonical +// value form. Registry constructors allocate a pointer so the codec +// can populate it via UnmarshalJSON; Parse returns the dereferenced +// value so the dynamic type a caller reads back from Parse matches +// the value literal they would write by hand (subjectid.IssSubID, +// not *subjectid.IssSubID). See [SubjectIdentifier] for the +// canonical-form contract. +// +// Reflection keeps this uniform across the built-in formats and any +// extension type registered via [RegisterFormat]; the constraint it +// imposes — extension types must satisfy [SubjectIdentifier] with +// value receivers, so the dereferenced value still implements the +// interface — is documented on RegisterFormat. A non-pointer is +// returned unchanged. +// +// An extension type registered with pointer-receiver methods would +// not satisfy the interface in dereferenced form; rather than panic +// or drop it, deref returns such a value unchanged. Normalization is +// thus best-effort, and value-canonical is guaranteed only for the +// built-ins and conforming extensions. +func deref(id SubjectIdentifier) SubjectIdentifier { + rv := reflect.ValueOf(id) + if rv.Kind() != reflect.Pointer { + return id + } + if v, ok := rv.Elem().Interface().(SubjectIdentifier); ok { + return v + } + return id +} + // Parse decodes a Subject Identifier from raw JSON, dispatching // on the value of the "format" member to the appropriate -// concrete type: +// concrete type. The returned identifier is always in value form +// (e.g. [IssSubID], never *IssSubID) — the canonical dynamic form +// for every value this package produces, matching what a caller +// constructs as a struct literal. See [SubjectIdentifier]. // // - For a built-in format (account, email, iss_sub, opaque, // phone_number, did, uri, aliases), the matching per-format @@ -57,13 +92,13 @@ func Parse(raw json.RawMessage) (SubjectIdentifier, error) { if err := json.Unmarshal(raw, target); err != nil { return nil, err } - return target, nil + return deref(target), nil } // Unknown format — copy the bytes so the caller cannot mutate // our internal state by holding a reference to raw. bytes := make(json.RawMessage, len(raw)) copy(bytes, raw) - return &UnknownFormat{ + return UnknownFormat{ FormatName: env.Format, Raw: bytes, }, nil diff --git a/unmarshal_test.go b/unmarshal_test.go index d2e0a07..068d072 100644 --- a/unmarshal_test.go +++ b/unmarshal_test.go @@ -26,9 +26,9 @@ func TestParseEachBuiltinFormatDispatchesToCorrectType(t *testing.T) { name: "account", raw: `{"format":"account","uri":"acct:example.user@service.example.com"}`, check: func(t *testing.T, got subjectid.SubjectIdentifier) { - a, ok := got.(*subjectid.AccountID) + a, ok := got.(subjectid.AccountID) if !ok { - t.Fatalf("got %T, want *AccountID", got) + t.Fatalf("got %T, want AccountID", got) } if a.URI != "acct:example.user@service.example.com" { t.Errorf("URI = %q", a.URI) @@ -39,9 +39,9 @@ func TestParseEachBuiltinFormatDispatchesToCorrectType(t *testing.T) { name: "email", raw: `{"format":"email","email":"user@example.com"}`, check: func(t *testing.T, got subjectid.SubjectIdentifier) { - e, ok := got.(*subjectid.EmailID) + e, ok := got.(subjectid.EmailID) if !ok { - t.Fatalf("got %T, want *EmailID", got) + t.Fatalf("got %T, want EmailID", got) } if e.Email != "user@example.com" { t.Errorf("Email = %q", e.Email) @@ -52,9 +52,9 @@ func TestParseEachBuiltinFormatDispatchesToCorrectType(t *testing.T) { name: "iss_sub", raw: `{"format":"iss_sub","iss":"https://issuer.example.com/","sub":"145234573"}`, check: func(t *testing.T, got subjectid.SubjectIdentifier) { - i, ok := got.(*subjectid.IssSubID) + i, ok := got.(subjectid.IssSubID) if !ok { - t.Fatalf("got %T, want *IssSubID", got) + t.Fatalf("got %T, want IssSubID", got) } if i.Iss != "https://issuer.example.com/" || i.Sub != "145234573" { t.Errorf("Iss=%q Sub=%q", i.Iss, i.Sub) @@ -65,9 +65,9 @@ func TestParseEachBuiltinFormatDispatchesToCorrectType(t *testing.T) { name: "opaque", raw: `{"format":"opaque","id":"11112222333344445555"}`, check: func(t *testing.T, got subjectid.SubjectIdentifier) { - o, ok := got.(*subjectid.OpaqueID) + o, ok := got.(subjectid.OpaqueID) if !ok { - t.Fatalf("got %T, want *OpaqueID", got) + t.Fatalf("got %T, want OpaqueID", got) } if o.ID != "11112222333344445555" { t.Errorf("ID = %q", o.ID) @@ -78,9 +78,9 @@ func TestParseEachBuiltinFormatDispatchesToCorrectType(t *testing.T) { name: "phone_number", raw: `{"format":"phone_number","phone_number":"+12065550100"}`, check: func(t *testing.T, got subjectid.SubjectIdentifier) { - p, ok := got.(*subjectid.PhoneNumberID) + p, ok := got.(subjectid.PhoneNumberID) if !ok { - t.Fatalf("got %T, want *PhoneNumberID", got) + t.Fatalf("got %T, want PhoneNumberID", got) } if p.PhoneNumber != "+12065550100" { t.Errorf("PhoneNumber = %q", p.PhoneNumber) @@ -91,9 +91,9 @@ func TestParseEachBuiltinFormatDispatchesToCorrectType(t *testing.T) { name: "did", raw: `{"format":"did","url":"did:example:123456"}`, check: func(t *testing.T, got subjectid.SubjectIdentifier) { - d, ok := got.(*subjectid.DIDID) + d, ok := got.(subjectid.DIDID) if !ok { - t.Fatalf("got %T, want *DIDID", got) + t.Fatalf("got %T, want DIDID", got) } if d.URL != "did:example:123456" { t.Errorf("URL = %q", d.URL) @@ -104,9 +104,9 @@ func TestParseEachBuiltinFormatDispatchesToCorrectType(t *testing.T) { name: "uri", raw: `{"format":"uri","uri":"urn:oasis:names:tc:saml:2.0:nameid-format:transient"}`, check: func(t *testing.T, got subjectid.SubjectIdentifier) { - u, ok := got.(*subjectid.URIID) + u, ok := got.(subjectid.URIID) if !ok { - t.Fatalf("got %T, want *URIID", got) + t.Fatalf("got %T, want URIID", got) } if u.URI != "urn:oasis:names:tc:saml:2.0:nameid-format:transient" { t.Errorf("URI = %q", u.URI) @@ -144,18 +144,18 @@ func TestParseAliasesIsRecursiveAndHeterogeneous(t *testing.T) { if err != nil { t.Fatalf("Parse aliases: err = %v", err) } - a, ok := got.(*subjectid.AliasesID) + a, ok := got.(subjectid.AliasesID) if !ok { - t.Fatalf("got %T, want *AliasesID", got) + t.Fatalf("got %T, want AliasesID", got) } if len(a.Identifiers) != 2 { t.Fatalf("len(Identifiers) = %d, want 2", len(a.Identifiers)) } - if _, ok := a.Identifiers[0].(*subjectid.EmailID); !ok { - t.Errorf("Identifiers[0] = %T, want *EmailID", a.Identifiers[0]) + if _, ok := a.Identifiers[0].(subjectid.EmailID); !ok { + t.Errorf("Identifiers[0] = %T, want EmailID", a.Identifiers[0]) } - if _, ok := a.Identifiers[1].(*subjectid.AccountID); !ok { - t.Errorf("Identifiers[1] = %T, want *AccountID", a.Identifiers[1]) + if _, ok := a.Identifiers[1].(subjectid.AccountID); !ok { + t.Errorf("Identifiers[1] = %T, want AccountID", a.Identifiers[1]) } } @@ -176,10 +176,10 @@ func TestParseAliasesInnerUnknownFormatBecomesUnknownFormat(t *testing.T) { if err != nil { t.Fatalf("Parse aliases: err = %v", err) } - a := got.(*subjectid.AliasesID) - u, ok := a.Identifiers[1].(*subjectid.UnknownFormat) + a := got.(subjectid.AliasesID) + u, ok := a.Identifiers[1].(subjectid.UnknownFormat) if !ok { - t.Fatalf("Identifiers[1] = %T, want *UnknownFormat", a.Identifiers[1]) + t.Fatalf("Identifiers[1] = %T, want UnknownFormat", a.Identifiers[1]) } if u.FormatName != "org.example.future" { t.Errorf("FormatName = %q", u.FormatName) @@ -195,9 +195,9 @@ func TestParseUnknownTopLevelFormatBecomesUnknownFormat(t *testing.T) { if err != nil { t.Fatalf("Parse: err = %v", err) } - u, ok := got.(*subjectid.UnknownFormat) + u, ok := got.(subjectid.UnknownFormat) if !ok { - t.Fatalf("got %T, want *UnknownFormat", got) + t.Fatalf("got %T, want UnknownFormat", got) } if u.FormatName != "org.example.future" { t.Errorf("FormatName = %q", u.FormatName) @@ -213,7 +213,7 @@ func TestParseUnknownFormatRawIsACopy(t *testing.T) { if err != nil { t.Fatalf("Parse: err = %v", err) } - u := got.(*subjectid.UnknownFormat) + u := got.(subjectid.UnknownFormat) // Mutate the source; the carrier should keep the original // bytes because Parse copied them. src[0] = '!' @@ -285,9 +285,9 @@ func TestParseDispatchesExtensionFormatRegisteredViaRegisterFormat(t *testing.T) if err != nil { t.Fatalf("Parse: err = %v", err) } - e, ok := got.(*extensionID) + e, ok := got.(extensionID) if !ok { - t.Fatalf("got %T, want *extensionID", got) + t.Fatalf("got %T, want extensionID", got) } if e.Tag != "hello" { t.Errorf("Tag = %q, want %q", e.Tag, "hello") From 637a60133702c972ad94764138132652e0fdb89c Mon Sep 17 00:00:00 2001 From: Henry Stern Date: Sun, 21 Jun 2026 13:10:24 -0300 Subject: [PATCH 2/2] Add CHANGELOG entries for 0.1.0 and 0.2.0 Record the initial 0.1.0 API surface and the 0.2.0 value-canonical Parse change, replacing the stale pre-release "no public API yet" note. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3abb92d..d9187c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,5 +10,30 @@ without bumping the spec-version constant. ## [Unreleased] -- Repository bootstrap: `go.mod`, license, contributor docs, CI - scaffolding. No public API yet. +## [0.2.0] + +### Changed + +- **Breaking:** `Parse` now returns Subject Identifier values in their + value form (e.g. `IssSubID`) rather than pointer form (`*IssSubID`), + so a value built as a struct literal and a value read back from + `Parse` share the same dynamic type. Code that type-asserts or + type-switches on a `Parse` result must match the value forms. The + `SubjectIdentifier` and `Parse` documentation state this canonical + form, and extension types registered via `RegisterFormat` must + satisfy the interface with value receivers. + +### Fixed + +- `AliasesID.Validate` now rejects a nested `aliases` identifier + regardless of how it was produced. A nested aliases parsed from JSON + previously escaped the `ErrNestedAliases` check; a hand-built one was + already caught. + +## [0.1.0] + +- Initial release: the eight RFC 9493 Subject Identifier formats, a + discriminator-driven JSON codec (`Parse` plus spec-order, byte-stable + `Marshal`), opt-in per-format `Validate` with sentinel errors, a + forward-compatible `UnknownFormat` carrier that preserves unknown + wire bytes verbatim, and the `RegisterFormat` extension hook.