From a267fbca962de7da2493dd5a59c557b5a31c0b35 Mon Sep 17 00:00:00 2001 From: Mark Esler Date: Tue, 28 Apr 2026 15:43:38 -0700 Subject: [PATCH 1/2] fix: prevent Resource.response() from mutating handler-owned attributes Resource.response() previously assigned `response := r.Attributes`, a reference copy of the handler's stored map. Subsequent writes of framework-injected keys ("id", "schemas", "meta") therefore propagated back into the handler's own storage. A second GET on the same resource would return those keys as if they were user-supplied attributes. Replace the reference assignment with an explicit copy so the framework operates only on a local map. Adds a regression test that performs a GET request and then inspects the handler's internal store to assert that none of the framework keys were written into it. --- resource_handler.go | 7 ++-- resource_response_mutation_test.go | 67 ++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 3 deletions(-) create mode 100644 resource_response_mutation_test.go diff --git a/resource_handler.go b/resource_handler.go index 8c63987..f328c87 100644 --- a/resource_handler.go +++ b/resource_handler.go @@ -46,9 +46,10 @@ type Resource struct { } func (r Resource) response(resourceType ResourceType, location string) ResourceAttributes { - response := r.Attributes - if response == nil { - response = ResourceAttributes{} + // Copy attributes to avoid mutating the map returned by the handler. + response := make(ResourceAttributes, len(r.Attributes)) + for k, v := range r.Attributes { + response[k] = v } response[schema.CommonAttributeID] = r.ID diff --git a/resource_response_mutation_test.go b/resource_response_mutation_test.go new file mode 100644 index 0000000..0d459c4 --- /dev/null +++ b/resource_response_mutation_test.go @@ -0,0 +1,67 @@ +package scim_test + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/elimity-com/scim" + "github.com/elimity-com/scim/schema" +) + +// TestResponseDoesNotMutateHandlerAttributes verifies that serving a GET request +// does not modify the attributes map stored inside the resource handler. +// +// Resource.response() previously did: +// +// response := r.Attributes // reference copy, not a value copy +// response["id"] = r.ID // writes back into the handler's stored map +// +// The result: a second GET on the same resource would find framework-injected +// keys ("id", "schemas", "meta") already present in the attributes, leading to +// duplicate or stale data in the response. +func TestResponseDoesNotMutateHandlerAttributes(t *testing.T) { + handler := &testResourceHandler{ + data: map[string]testData{ + "0001": { + attributes: scim.ResourceAttributes{ + "userName": "alice", + }, + }, + }, + schema: schema.CoreUserSchema(), + } + + s, err := scim.NewServer(&scim.ServerArgs{ + ServiceProviderConfig: &scim.ServiceProviderConfig{}, + ResourceTypes: []scim.ResourceType{ + { + Name: "User", + Endpoint: "/Users", + Schema: schema.CoreUserSchema(), + Handler: handler, + }, + }, + }) + if err != nil { + t.Fatal(err) + } + + req := httptest.NewRequest(http.MethodGet, "/Users/0001", nil) + w := httptest.NewRecorder() + s.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + + // After the GET, the handler's stored map must not contain any of the + // framework-injected keys. If response() mutated the map, these will be + // present and a subsequent GET would return them as user-supplied data. + stored := handler.data["0001"].attributes + for _, key := range []string{"id", "schemas", "meta"} { + if _, ok := stored[key]; ok { + t.Errorf("Resource.response() mutated handler attributes: key %q was injected into the stored map", key) + } + } +} From 8a6613a04fbf1c0ee1c663bf74d8cc3024dd1ee5 Mon Sep 17 00:00:00 2001 From: Mark Esler Date: Thu, 30 Apr 2026 15:54:58 -0700 Subject: [PATCH 2/2] fix: prevent Page.rawResources() from mutating handler-owned attributes The same aliasing bug found in Resource.response() also exists in rawResources(): the outer attrs map and any handler-provided nested meta map were both reference-copied, so injecting id/externalId/meta fields corrupted the handler's stored state on subsequent calls. Fix both layers with explicit copies. Adds regression tests for both the existing Resource.response() fix and the new rawResources() fix. --- list_response.go | 13 +++++++--- list_response_test.go | 60 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) create mode 100644 list_response_test.go diff --git a/list_response.go b/list_response.go index 821552e..1e9be8d 100644 --- a/list_response.go +++ b/list_response.go @@ -44,9 +44,10 @@ func (p Page) rawResources() []interface{} { var resources []interface{} for _, v := range p.Resources { - attrs := v.Attributes - if attrs == nil { - attrs = ResourceAttributes{} + // Copy outer map to avoid mutating handler-owned attributes. + attrs := make(ResourceAttributes, len(v.Attributes)+3) + for k, val := range v.Attributes { + attrs[k] = val } attrs[schema.CommonAttributeID] = v.ID @@ -55,10 +56,14 @@ func (p Page) rawResources() []interface{} { } // Merge Meta fields into the existing "meta" map if present, or create a new one. + // Copy the nested meta map too, to avoid mutating a handler-owned nested map. var metaMap map[string]interface{} if existing, ok := attrs[schema.CommonAttributeMeta]; ok { if m, ok := existing.(map[string]interface{}); ok { - metaMap = m + metaMap = make(map[string]interface{}, len(m)+3) + for k, val := range m { + metaMap[k] = val + } } } hasMeta := false diff --git a/list_response_test.go b/list_response_test.go new file mode 100644 index 0000000..80f0ae9 --- /dev/null +++ b/list_response_test.go @@ -0,0 +1,60 @@ +package scim + +import ( + "testing" + "time" + + "github.com/elimity-com/scim/schema" +) + +func TestRawResourcesDoesNotMutateHandlerAttributes(t *testing.T) { + t.Parallel() + + t.Run("outer map not mutated", func(t *testing.T) { + t.Parallel() + attrs := ResourceAttributes{"displayName": "Test User"} + original := make(ResourceAttributes, len(attrs)) + for k, v := range attrs { + original[k] = v + } + p := Page{Resources: []Resource{{ID: "42", Attributes: attrs}}} + + _ = p.rawResources() + + for k := range attrs { + if _, ok := original[k]; !ok { + t.Errorf("rawResources() injected %q into handler-owned attributes map", k) + } + } + }) + + t.Run("nested meta map not mutated", func(t *testing.T) { + t.Parallel() + // Handler provides a meta map (e.g. with resourceType pre-filled). + existingMeta := map[string]interface{}{"resourceType": "User"} + originalMeta := make(map[string]interface{}, len(existingMeta)) + for k, v := range existingMeta { + originalMeta[k] = v + } + attrs := ResourceAttributes{ + "displayName": "Test User", + schema.CommonAttributeMeta: existingMeta, + } + now := time.Now() + p := Page{ + Resources: []Resource{{ + ID: "42", + Attributes: attrs, + Meta: Meta{Created: &now, LastModified: &now, Version: "W/\"1\""}, + }}, + } + + _ = p.rawResources() + + for k := range existingMeta { + if _, ok := originalMeta[k]; !ok { + t.Errorf("rawResources() injected %q into handler-owned meta map", k) + } + } + }) +}