fix: prevent Resource.response() from mutating handler-owned attributes

[eslerm]

Problem

Resource.response() opened with:

    response := r.Attributes

This is a reference copy, not a value copy. Every subsequent write to response (injecting "id", "schemas", "meta") therefore propagated back into the map owned by the resource handler.

The consequence is silent corruption of handler state: a second GET on the same resource would find those framework-injected keys already present in the attributes map and return them as if they were user-supplied data.

Fix

Replace the reference assignment with an explicit copy:

    response := make(ResourceAttributes, len(r.Attributes)+5)
    for k, v := range r.Attributes {
        response[k] = v
    }

Test

TestResponseDoesNotMutateHandlerAttributes performs a GET request, then inspects the handler's internal store directly and asserts that none of "id", "schemas", or "meta" were written into it. Without the fix the test fails on all three keys.

[q-uint]

Thanks for tracking this down! The diagnosis and the test are great, and the fact that a second GET would surface the injected keys as user data is a nasty failure mode.

While reviewing I noticed the same aliasing pattern lives in one other place: Page.rawResources(), the parallel implementation used by GET / and POST /.search root queries (call sites at handlers.go:480 and handlers.go:535). It has two layers of the same bug:

Outer map, same shape as the one you fixed:

attrs := v.Attributes // reference copy
if attrs == nil {
    attrs = ResourceAttributes{}
}
attrs[schema.CommonAttributeID] = v.ID
// ...

Nested meta map, sneakier; if the RootQueryHandler returns a resource whose Attributes["meta"] is a map[string]interface{} (which the doc comment explicitly suggests, e.g. for resourceType), rawResources aliases it and merges created/lastModified/version straight in:

if existing, ok := attrs[schema.CommonAttributeMeta]; ok {
    if m, ok := existing.(map[string]interface{}); ok {
        metaMap = m // alias of caller's nested map
    }
}
// ...
metaMap["created"] = ... // mutates it

Same fix shape as your PR works: copy the outer map up front, and copy the nested meta map before merging:

attrs := make(ResourceAttributes, len(v.Attributes)+3)
for k, val := range v.Attributes {
    attrs[k] = val
}
// ...
if existing, ok := attrs[schema.CommonAttributeMeta]; ok {
    if m, ok := existing.(map[string]interface{}); ok {
        metaMap = make(map[string]interface{}, len(m)+3)
        for k, val := range m {
            metaMap[k] = val
        }
    }
}

Would you be willing to include a fix for this in the PR too @eslerm? If not, happy to merge this and do it later.

NOTE: I have not tested the code above yet, so please validated it 🙏🏼

[q-uint]

One thought on scope: the most obvious reproducer needs a long-lived in-memory map (like the repo's own testResourceHandler), and many production handlers rebuild a fresh map per call from a DB row, so they'd never observe this. That said, in-memory caches, LRU layers, and ORM-hydrated entities do probably alias. The framework can't tell from the outside which kind it got, so safe-by-default still seems like the right contract.

Out of curiosity, how did you find this? Real incident, code read, or tooling? Useful to know whether there's a class of similar bugs worth sweeping in one pass.

[eslerm]

Cheers, added.

I upgraded to Go 1.26 and ran go run golang.org/x/tools/go/analysis/passes/modernize/cmd/modernize@latest, which prompted adding t.Parallel() to the test suite. Running tests in parallel exposed the mutation as a race.

This analysis was loose, but you may want to investigate b34c00f