never let the client tell you who owns it

i ended the owner-scoped rbac post with a promise: that CreateDojo was next, because it never checked anything. i went back to fix it expecting to spend an afternoon threading a session through a service method.

it took four seconds. the proof was in the signature.

func (svc *DojoService) CreateDojo(
	dojo *entities.Dojo,
) (*entities.Dojo, error) {
	return svc.dojoRepo.Create(dojo)
}

every other verb on that service takes a session *aggregates.Session and does something with it. this one doesn’t take a session at all. it cannot check who’s calling, because it was never handed anything to check. you don’t need to read the body of a function to know it doesn’t do authorization when the argument list has nowhere to put the answer.

quick framing, same as ever: mojodojo here is the 2024 learning project — a rest api for martial-arts gyms i built to see whether hexagonal architecture would survive contact with fourteen entities. no users, nothing shipped, which is exactly why i get to publish its security holes rather than quietly patching them. (it shares a name with the saas i actually run now. different codebase, and the bug below is the learning project’s, not the product’s.) everything described here is fixed as of writing — the diffs at the bottom are what landed, not what i’m planning.

the field came off the wire

here’s the create handler, gin, trimmed of logging:

internal/ports/http/dojo_handler.go

func (instance *DojoHandler) Create(ctx *gin.Context) {
	var payload Dojo
	if err := ctx.ShouldBindJSON(&payload); err != nil {
		ctx.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
		return
	}

	entity, err := entities.NewDojo(payload.Name, payload.OwnerID)
	if err != nil {
		ctx.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
		return
	}

	createdEntity, err := instance.dojoService.CreateDojo(&entity)
	// ...
}

and the payload type it binds into:

internal/ports/http/dojo_model.go

type Dojo struct {
	ID      string `json:"id"`
	Name    string `json:"name"`
	OwnerID string `json:"ownerId"`
}

payload.OwnerID. straight off the json, into the entity constructor, into the database. the handler never calls sessionFromContext — it doesn’t establish who is making the request at any point. the route is behind auth middleware, so you have to be somebody. nothing anywhere establishes that you’re the somebody in the ownerId field.

the route sits behind PermissionsMiddleware(DojoCreate), and the migration seeds DOJO_CREATE to ADMIN and OPERATOR only — so this isn’t reachable by every logged-in user, and i’d be overselling it if i said so. it’s reachable by any operator:

curl -X POST https://api.example/dojos \
  -H "Authorization: Bearer $MY_TOKEN" \
  -d '{"name":"not mine","ownerId":"<any real user id>"}'

owner_id carries a foreign key to users(id), so an invented uuid gets rejected by the database — the id has to belong to a real account, which is the version of the attack you’d actually run anyway. i create a row. somebody else owns it. and because the whole permission model from the last post keys on dojo.OwnerID == session.User.ID, that stranger now sees a dojo in their list that i wrote and they didn’t. i can put anything in the name field. i’ve written into another user’s account without ever touching their account.

the field was validated. that turned out to mean nothing.

this is the part i want to sit with, because at first glance the code looks careful:

internal/domain/entities/dojo.go

func NewDojo(
	name,
	ownerID string,
) (Dojo, error) {
	if err := validator.Validate(name, validator.StringNotEmpty); err != nil {
		return Dojo{}, fmt.Errorf("%v: %s %v", ErrDojoValidation, "Name", err)
	}

	if err := validator.Validate(ownerID, validator.ValidUUID); err != nil {
		return Dojo{}, fmt.Errorf("%v: %s %v", ErrDojoValidation, "OwnerID", err)
	}

	return Dojo{
		ID:      uuid.NewString(),
		Name:    name,
		OwnerID: ownerID,
	}, nil
}

there is a check on ownerID. it’s in the domain layer, where i’d argue checks belong. it rejects empty strings, it rejects garbage, it rejects "; DROP TABLE. it is a real validator doing its job correctly.

and it is completely irrelevant to this bug, because validation answers “is this value well-formed?” and authorization answers “may you write it?” those questions have nothing to do with each other, and a well-formed uuid belonging to a stranger sails through a validator built to check shape.

i think this is why the bug survived a review by me. the field wasn’t ignored. it had a check next to it, the check was correct, and my eye read “validated” and moved on. a validated field feels handled. this one was handled the way a bouncer checking that your ID is a plausible rectangle is handling the door.

note the line directly above it, too: ID: uuid.NewString(). the entity’s own id is generated server-side, deliberately, because obviously you don’t let a client pick a primary key. the correct instinct was in the same struct literal, two fields up. it just never generalised to the field that decides who owns the thing.

the update verb is worse, because the check passes

create has no check at all, which is at least legible. update is the one that kept me up.

internal/ports/http/dojo_handler.go

entity, err := entities.NewDojo(payload.Name, payload.OwnerID)
// ...
session, err := sessionFromContext(ctx)
// ...
entity.ID = entityID

updatedEntity, err := instance.dojoService.UpdateDojo(entityID, &entity, session)

this handler does everything create didn’t. it pulls the session. it passes it down. and UpdateDojo opens by calling GetDojo(dojoID, session), which is the ownership check from the last post, working exactly as designed — if you don’t own this dojo and you don’t hold the global permission, you get an error and nothing happens.

so the check runs. the check passes. and then:

return svc.dojoRepo.Update(dojoID, dojo)

it writes the entity. the entity whose OwnerID came from payload.OwnerID. i own the dojo, i’m allowed to update the dojo, and the update i’m allowed to make includes handing it to somebody else. one PUT with a different ownerId and the row leaves my account. i can’t get it back, because now i don’t own it and the check that just let me through won’t let me through again — someone holding the global DOJO_UPDATE permission can, but by ordinary-user standards it’s gone. and this one is broadly reachable: PUT /dojos/{id} accepts DOJO_UPDATE or DOJO_UPDATE_OWNED, and every dojo owner holds the latter.

the authorization check guarded the row. it never guarded the field that decides who the row belongs to. that gap has a name — owasp calls it api3:2023, broken object property level authorization ↗ , which merged the older “mass assignment” and “excessive data exposure” categories precisely because they’re the same mistake viewed from two ends. an endpoint is vulnerable when it “allows a user to change, add/or delete the value of a sensitive object’s property which the user should not be able to access.” and the recommendation is a single sentence i’d have saved a weekend by reading first: “if possible, avoid using functions that automatically bind a client’s input into code variables, internal objects, or object properties.”

ctx.ShouldBindJSON(&payload) is exactly that function.

look at entity.ID = entityID in that handler, though. that line takes the id from the url path and stamps it onto the entity — the trusted source wins. (NewDojo never even receives the body’s id; it generates a fresh uuid, which this line then overwrites. so the body’s id is discarded twice over, by an argument list and then by an assignment.) the trusted-source-wins instinct is right there, in this file, one line above the call that ships the untrusted OwnerID into the database. i knew the move. i applied it to the field that couldn’t hurt me.

i went looking for this shape everywhere else, and it wasn’t there

my next assumption was that a codebase which got this wrong once got it wrong fourteen times. every child entity names its parent by id — a class names a dojo, a fee names a dojo, a practice names a class — and every one of those is a place the tree could be grafted onto by whoever’s asking. i went to write that section.

it isn’t true. here’s CreateClass:

internal/application/services/class.go

func (svc *ClassService) CreateClass(
	class *aggregates.Class,
	session *aggregates.Session,
) (*aggregates.Class, error) {
	_, err := svc.dojoService.GetDojo(class.Dojo.ID, session)
	if err != nil {
		return nil, repositories.ErrClassCreate
	}

	return svc.classRepo.Create(class)
}

it takes the session. it resolves the parent dojo through the ownership check before touching the repository. try to create a class in a dojo you don’t own and GetDojo refuses, so CreateClass refuses. and it isn’t just classes — CreateFee and CreateFighter both open with GetDojo(…, session), CreatePractice goes through GetClass(…, session), CreateGraduation through GetFighter(…, session). every create that hangs off a dojo checks. the three that take no session — belt, technique, martial art — aren’t dojo children at all; they’re catalog entities that hang off a martial art.

thirteen out of fourteen. i had the pattern right, consistently, everywhere, and i want to be honest that i went looking for a bigger fire and didn’t find one.

but the exception is not random, and that’s the actual finding. look at what the correct thirteen all do: they take the id of a parent, and they hand it to that parent’s ownership check. the pattern is “resolve upward, and let the parent say no.”

a dojo has no parent. it’s the root. so CreateDojo had nothing to resolve upward to, the pattern had no answer for it, and what got written instead was nothing at all — not a weaker check, not a wrong check, an absent one and a signature with nowhere to put a session.

that’s a much more interesting bug than “he forgot one.” a security pattern that derives authority from a parent object silently has no story for the root, and the root is the one object where ownership is established rather than inherited. every entity below it was protected by asking a question upward. the top of the tree is where the question has to be answered instead.

the fix, in the layer that already lost this argument once

the fix, in the layer that already lost this argument once

the last post’s whole conclusion was: pick one layer and make it the authority, and for this codebase that layer is the service — the only place holding the row and the session in the same function. so the fix goes there, not in the handler.

the service method grows the argument it should always have had, and stamps:

internal/application/services/dojo.go

func (svc *DojoService) CreateDojo(
	dojo *entities.Dojo,
	session *aggregates.Session,
) (*entities.Dojo, error) {
	// ownership is not an input. whoever is authenticated owns what they
	// create — the request body does not get a vote.
	dojo.OwnerID = session.User.ID

	return svc.dojoRepo.Create(dojo)
}

three lines, and the important one is an assignment rather than a check. that distinction is the whole post. a check is if payload.OwnerID != session.User.ID { reject } — which is also correct, and which i deliberately didn’t write. a check has to be remembered at all fourteen call sites and can be forgotten at any one of them; that’s precisely how the last post’s bug happened. an assignment cannot be forgotten, because there is no path through this function that leaves OwnerID holding what the client sent. whatever arrives, it’s overwritten.

then the field stops existing on the way in:

internal/ports/http/dojo_model.go

// the inbound shape and the outbound shape are not the same type, on
// purpose. OwnerID is on the response because clients need to read it;
// it is absent from the request because they must not write it.
type CreateDojoRequest struct {
	Name string `json:"name"`
}

this is the belt-and-braces half, and it’s the one i’d fight for in review. with OwnerID gone from the request struct, ShouldBindJSON has nowhere to put an ownerId from the wire — it lands in a field that isn’t there and evaporates. the attack isn’t rejected, it’s unsayable. the compiler now enforces what a code comment used to ask for, and a future me adding a fifteenth entity has to consciously add the field back to reintroduce the hole.

one shared struct for request and response is a tidy-looking habit that quietly makes every readable property a writable one. the read shape and the write shape are different shapes and deserve different types.

update gets the same treatment — CreateDojoRequest’s sibling has no OwnerID either, so the transfer-by-PUT disappears with it. transferring a dojo to another user is a legitimate feature someone might want one day. it will be its own endpoint, with its own permission, and it will not be a side effect of the verb for renaming things.

what i rejected

• checking instead of stamping (if payload.OwnerID != session.User.ID). correct, and fragile for the exact reason the previous post documented at length: it’s a rule that has to be remembered per verb, and i have already proven i don’t remember it per verb.
• doing it in middleware. tempting — one place, applies everywhere. but middleware doesn’t know which entity this route creates or what its ownership field is called, so it’d end up reflecting over structs looking for something named OwnerID. magic that fails silently when someone names a field OwnedBy is worse than no magic.
• leaving OwnerID on the request struct and just ignoring it. the field would still be in the swagger docs, still bind, still read as meaningful to anyone integrating. an ignored input is a loaded gun that happens to be pointed away right now.

lessons learned

• validation is not authorization, and a validated field reads as a handled field. ValidUUID on ownerID was a correct check that proved the value was shaped like an id and said nothing about whether the caller may set it. if a field answers “who is this for”, shape-checking it is not a security control, and having one there makes the missing control harder to see.
• prefer assigning over checking for any field the server owns. a check is a rule you must remember at every call site; an assignment is a fact with no path around it. dojo.OwnerID = session.User.ID cannot be forgotten the way an if can.
• make the bad request unsayable, not just rejected. dropping the field from the inbound struct means the malicious value has nowhere to bind. this is the same instinct as a check constraint over an app-layer guard : push it down to the layer where forgetting isn’t an available move.
• the request shape and the response shape are different types. sharing one struct for both silently promotes every readable field to a writable one — which is owasp’s api3:2023 ↗ in a single design decision.
• ownership propagates down a tree, so audit the children too. OwnerID on create was one hole. dojoId on every child-entity create was the same hole, fourteen times, and i only found it because i went looking for the shape rather than the field name.

this is the third time on this blog i’ve arrived at the same sentence from a different direction — a rate limiter keyed on a header the client writes , a tenant filter applied where the data is read instead of where it’s written , and now an owner id read off a request body. every one of them is a control that keys on a value the caller chose. i’m starting to think it’s not three lessons.

next: back to rust and the logmole series — memory-mapping a multi-gigabyte log and parsing records straight out of the mapping without copying a line, what the borrow checker makes you say out loud to do it, and the benchmark that decides whether it was worth it.