Review Agent PRs That Spread Request Bodies Into Writes

The merge window opened at four on Friday.
A profile-update ticket sat on the sprint board.
An agent opened a compact TypeScript pull request.

The handler looked careful, typed, and complete.
Two unit tests asserted a successful 200 response.
A reviewer approved the patch within minutes.

Support saw the fallout after the weekend deploy.
One user profile now stored an admin role.
The public form never exposed that field.

This article reviews that class of agent patch.
It shows what to trust, revert, and test.
The working artifact is a Node review harness.

The pattern hiding in a clean diff

Agents often simplify update handlers under time pressure.
They spread the request body onto the stored record.
The TypeScript type still appears locally very strict.

Parsed JSON keeps extra enumerable keys at runtime.
Those extra keys reach the write path unchanged.
The editor type never sees them during review.

// Proposal: agent-generated handler. Do not copy into production.
export async function updateProfile(req, res, db) {
  const userId = req.session.userId;
  const current = await db.users.findById(userId);
  const next = { ...current, ...req.body };
  await db.users.save(next);
  return res.status(200).json({ ok: true, user: next });
}

The tests usually send only one displayName field.
They never send role, plan, or emailVerified.
Green CI does not prove a safe write.

What to trust in the pull request

Reviewers should trust small, named field copies only.
Trust a parser that strips unknown keys at the boundary.
Trust tests that assert rejected extra fields on disk.

Numbered signals that can remain in the patch:

1. An allowlist maps body keys to concrete columns.
2. A schema library strips unknown keys before writes.
3. The write uses explicit column parameters only.
4. Tests include at least one forbidden-field case.

Those four signals still need runtime proof in CI.
Do not trust comments that promise later hardening.
Do not trust a type that exists only in the editor.

What to revert on sight

Revert any spread of req.body into persistence.
Revert Object.assign of the payload onto stored records.
Revert queries built from a full client object.

Numbered revert list for the incoming diff:

1. Spread of req.body onto a loaded row.
2. Object.assign of req.body before the save.
3. ORM update calls that pass data: req.body.
4. Unfiltered loops over Object.keys during those writes.
5. Hidden fields such as role, tenantId, and plan.

Agents add these patterns while chasing fewer lines.
Fewer source lines are not a security property.
The human reviewer still owns the write surface.

What to test before merge

Tests must treat extra keys as hostile input.
Status 200 is not the assertion that matters.
The stored row is the assertion that matters.

Numbered test plan for the PR branch:

1. Send a valid displayName and one extra role.
2. Assert HTTP 200 or 400 per the public contract.
3. Reload the row from the database fixture.
4. Assert role remains the original stored value.
5. Repeat for tenantId, plan, and emailVerified.
6. Repeat with a nested object under a known key.
7. Repeat with a proto key on the payload.

The nested case catches shallow allowlists very quickly.
The prototype case catches unsafe object merges quickly.
Both show up in agent patches during refactors.

Review workflow in six steps

Follow this order on every agent update PR.

1. Search the diff for spread and Object.assign calls.
2. List every key the handler persists after merge.
3. Compare that list with the public API document.
4. Add a forbidden-field test before reading more code.
5. Run the test against the PR branch.
6. Keep the patch only if the row stays unchanged.

Command examples for a local checkout follow next.

git fetch origin pull/4821/head:pr-4821
git checkout pr-4821
rg -n "\.\.\.req\.body|Object\.assign\(|data: req\.body" src
grep -R -n -E "\.\.\.req\.body|Object\.assign\(|data: req\.body" src
node --test test/mass-assign.test.js

The ripgrep step is cheap and fully mechanical.
The grep fallback covers machines without ripgrep installed.
Neither command replaces the later database assertion.

Artifact: a reproducible harness

The harness below is a proposal for Node 20.
It does not require a networked database instance.
It uses an in-memory map as the store.

// lib/update-profile.js
const FORBIDDEN = new Set([
  "id",
  "role",
  "tenantId",
  "plan",
  "emailVerified",
  "priceCents",
]);

const ALLOWED = new Set(["displayName", "bio"]);

export function mergeTrusted(current, body) {
  const next = { ...current };
  for (const key of ALLOWED) {
    if (!Object.hasOwn(body, key)) continue;
    if (typeof body[key] !== "string") continue;
    next[key] = body[key];
  }
  return next;
}

export async function updateProfile(req, db) {
  const userId = req.session.userId;
  const current = await db.users.findById(userId);
  if (!current) return { status: 404, body: { ok: false } };

  for (const key of Object.keys(req.body || {})) {
    if (FORBIDDEN.has(key)) {
      return { status: 400, body: { ok: false, error: "unwritable_field" } };
    }
  }

  const next = mergeTrusted(current, req.body || {});
  await db.users.save(next);
  return { status: 200, body: { ok: true, user: publicUser(next) } };
}

export function publicUser(user) {
  return {
    id: user.id,
    displayName: user.displayName,
    bio: user.bio,
  };
}

// Teaching control only. Do not export from app entrypoints.
export async function updateProfileUnsafe(req, db) {
  const current = await db.users.findById(req.session.userId);
  const next = { ...current, ...req.body };
  await db.users.save(next);
  return { status: 200, body: { ok: true, user: next } };
}

// test/mass-assign.test.js
import test from "node:test";
import assert from "node:assert/strict";
import {
  updateProfile,
  updateProfileUnsafe,
} from "../lib/update-profile.js";

function memoryDb(seed) {
  const rows = new Map([[seed.id, { ...seed }]]);
  return {
    users: {
      async findById(id) {
        const row = rows.get(id);
        return row ? { ...row } : null;
      },
      async save(row) {
        rows.set(row.id, { ...row });
      },
    },
    async read(id) {
      return { ...rows.get(id) };
    },
  };
}

const seed = {
  id: "u1",
  displayName: "Ada",
  bio: "",
  role: "member",
  tenantId: "t-home",
  plan: "free",
  emailVerified: false,
};

test("unsafe spread promotes role from the client", async () => {
  const db = memoryDb(seed);
  const req = {
    session: { userId: "u1" },
    body: { displayName: "Ada Lovelace", role: "admin" },
  };
  await updateProfileUnsafe(req, db);
  const stored = await db.read("u1");
  assert.equal(stored.role, "admin");
});

test("safe handler rejects a role write", async () => {
  const db = memoryDb(seed);
  const req = {
    session: { userId: "u1" },
    body: { displayName: "Ada Lovelace", role: "admin" },
  };
  const result = await updateProfile(req, db);
  assert.equal(result.status, 400);
  const stored = await db.read("u1");
  assert.equal(stored.role, "member");
  assert.equal(stored.displayName, "Ada");
});

test("safe handler copies only allowlisted strings", async () => {
  const db = memoryDb(seed);
  const req = {
    session: { userId: "u1" },
    body: { displayName: "Ada Lovelace", bio: "math" },
  };
  const result = await updateProfile(req, db);
  assert.equal(result.status, 200);
  const stored = await db.read("u1");
  assert.equal(stored.displayName, "Ada Lovelace");
  assert.equal(stored.bio, "math");
  assert.equal(stored.plan, "free");
});

Run the drill with one local command only.

node --test test/mass-assign.test.js

The first test documents the failure mode clearly.
The second test is the actual merge gate.
The third test protects the intended feature path.

Label the unsafe export as a teaching control.
Do not ship updateProfileUnsafe into any production bundle.
Keep that export only inside the review drill.

Decision table for the reviewer

Types lie when the boundary is HTTP JSON.
Comments also lie when agents generate them.
Database rows do not lie after the write.

Optional remote runner

Disclosure: This article was prepared as part of MonkeyCode's product outreach.
MonkeyCode provides free model access and a free server option.
Reviewers may run this harness there after a laptop check.
The steps stay valid if that product is ignored.

Limitations

This drill covers mass assignment on object writes.
It does not cover SQL injection or authz gaps.
It does not cover file uploads or webhook signatures.

The in-memory store hides real column-type coercion.
A real ORM may cast strings into booleans.
A real ORM may drop unknown keys by luck.

Luck is not a control for merge review.
Re-run the same tests against the staging schema.
Watch the actual UPDATE statement in query logs.

The forbidden list will rot without named owners.
New columns appear during unrelated refactors every quarter.
Agents then spread those new columns by default.

A 400 on unknown keys can break tolerant clients.
Some public patch APIs ignore extra fields instead.
That still requires a stored-row assertion after each write.

Shallow copies miss nested preference blobs.
Deep clones without allowlists copy too much.
Review nested objects as a second write surface.

Who should not use this approach

Do not use this checklist as a merge button.
Do not use it on services without a user model.
Do not use it as the only security review.

Skip the 400-on-forbidden policy for public patch APIs.
Some products ignore unknown keys on purpose by design.
That choice still needs the persist assertion in CI.

Skip any shared remote runner for regulated data.
The fixture above uses fake names only on purpose.
Never replay production dumps into a shared server.

The Friday patch failed a simple stored invariant.
Client JSON is not a trusted stored record.
Review the write, then review the stored row.