Six Security Findings from One Day of Code Review

I spent one day reading my own codebase the way an auditor would, and came out the other side with six findings—one of them exploitable by literally anyone with a Cloudflare account. None of them came from a scanner. Not because scanners are useless, but because none of these were the kind of thing scanners find. All six came from reading code while holding a single question in my head: what does this claim to protect, and does it? I want to walk through each one, because the individual bugs are instructive but the pattern underneath them is the actual lesson.

This is part 9 of 10 in the RORBT field notes—dispatches from building a governance platform on Cloudflare. The whole series lives on the RORBT case study.

A CORS rule trusting a domain we don’t own

Severity: high. Exploitable by anyone. And I wrote it.

if (hostname === 'rorbt.pages.dev' || hostname.endsWith('.rorbt.pages.dev')) {
  return origin;   // reflected as Access-Control-Allow-Origin, with credentials
}

Looks reasonable. Reads like every CORS allowlist you’ve ever approved in review. The problem is that the Pages project is named rorbt-site, so real deployments live on rorbt-site.pages.dev. Two consequences follow, and the second is the bad one:

1. Every preview deployment failed CORS—annoying, self-correcting, the kind of bug that files its own report.
2. rorbt.pages.dev was unclaimed. Anyone can register a Pages project under an unused name and receive that hostname. Do that, and this API reflects your origin with Access-Control-Allow-Credentials: true—a logged-in user visiting the attacker’s page hands over authenticated cross-origin access. I had written an allowlist whose most privileged entry was available for free at the signup page.

The lesson generalizes well past my typo: wildcard-suffix allowlists on platform-shared domains are a trap. *.pages.dev, *.vercel.app, *.herokuapp.com, *.s3.amazonaws.com—the namespace is shared with everyone on earth, so “ends with our platform domain” is not an ownership check. Only the exact project you control counts.

On a shared platform domain, “ends with our suffix” is not an ownership check. It’s an invitation.

Fixed, with an extra condition:

const PAGES_HOST = 'rorbt-site.pages.dev';
const { hostname, protocol } = new URL(origin);
// Preview deployments are always https; refusing http here stops an
// attacker-controlled plaintext origin from matching the suffix test.
if (protocol === 'https:' && (hostname === PAGES_HOST || hostname.endsWith(`.${PAGES_HOST}`)))
  return origin;

And the tests include the suffix-smuggling case that a careless fix would still allow, because I no longer trust myself to eyeball string suffixes:

expect(corsOrigin(from('https://rorbt-site.pages.dev.evil.com'))).toBe('https://rorbt.com');
expect(corsOrigin(from('https://notrorbt-site.pages.dev'))).toBe('https://rorbt.com');

Hashing low-entropy data is not anonymization

Found reviewing a proposed data model, before it was built—which is the cheapest possible time to find anything.

User: handle, public_key, hashed_location_tags, hashed_vocation_tags, …

This looks like privacy engineering, which is exactly why it survives review. It has the aesthetic of care. Hashed columns read as someone having thought about it, and reviewers wave through things that read as someone having thought about it.

But hashing protects high-entropy secrets, and these are anything but: a few thousand plausible cities, a few hundred plausible trades. An attacker computes the dictionary once—seconds of work—and reverses every user in the table, permanently. No key to steal, no exploit needed, no way to rotate out of it. For a platform whose entire promise is that the server can’t tell who you are or where you work, this was the most dangerous line in the schema, precisely because it reads as careful and nobody ever revisits the careful-looking lines.

The honest options are:

• Bucket and store cleartext—region not city, sector not employer—with a k-anonymity gate so a tag only becomes matchable once k users share it.
• Blinded OPRF / private set intersection if the server must match without learning values.

A plain hash over low-entropy data gives you the costs of both approaches and the protection of neither.

A scalar reputation score is a hierarchy

Same review, two lines down:

User: …, trust_score, capacity_budget

Not a memory-safety bug—an architectural one, and worth naming because it is everywhere. A single number attached to a person is globally comparable, which makes it a ranking, which makes it a caste. It accretes. It never rotates. Unlike every other form of standing in this system it can’t be contested or recalled. And it’s a permanent behavioural summary of a human being sitting on a server—precisely the artifact the threat model exists to avoid. We would have spent months building elaborate cryptographic privacy around a column that quietly reintroduced the thing all the cryptography was for.

Trust belongs on the edge, not the node:

Vouch: vouch_id, voucher_id, applicant_id, orbit_id, status, expiry, revoked_at

Scoped to an orbit, expiring, revocable. Any gate becomes a predicate evaluated at admission—“vouched by ≥2 current Full members”—rather than a stored rank. Show provenance, never a number that can be sorted.

GDPR erasure vs. an append-only hash chain

The privacy policy says: “When you ask us to delete your data, we delete all of it—no tombstones.” There was no mechanism implementing that sentence. Worse, the capability was half-enabled, which is the most dangerous amount: better-auth’s deleteUser was on, nothing in the UI called it, and if anything ever had, it would have deleted the account row and left everything else standing.

onDelete: 'cascade' existed only on session and account. Every other reference was a bare text() column with no FK: proposals.proposer_id, proposal_votes.member_id, transparency_log.actor_id, messages.sender_id, notifications.user_id, push_tokens.user_id, plus R2—the encrypted member envelope, orbit roster entries, assembly delegate seats.

Now, the genuine tension, because there is one: the transparency log is a hash chain. Deleting an entry forks it and destroys the auditability every member is promised. Messages and votes are other people’s context—removing them rewrites decisions an orbit already made. For about an hour I thought the privacy policy and the architecture were simply at war.

They aren’t. The resolution is that none of those records store PII. They store an opaque user id. The user row is what binds that id to a human—email, phone, handle. Destroy it and the record stays intact and unlinkable.

graph LR
  subgraph Before["Before erasure"]
    U[user row<br/>email, phone, handle] -.->|binds| L[log: actor_id=abc123]
    U -.->|binds| M[messages: sender_id=abc123]
    U -.->|binds| V[votes: member_id=abc123]
  end
  subgraph After["After erasure"]
    L2[log: actor_id=abc123]
    M2[messages: sender_id=abc123]
    V2[votes: member_id=abc123]
    X[user row destroyed]
    style X fill:#7f1d1d,color:#fff
  end

That’s real erasure: the key that re-identifies is gone, and what remains is pseudonymous history naming nobody.

Ordering is deliberate—everything that re-identifies dies before the account row, so a crash mid-way leaves an account whose data is already gone rather than orphaned PII with no owner to ask about it. And the D1 deletes are one batch(), because partial erasure is the worst available outcome.

Designing an unsubscribe link that can’t be abused

Nobody should be able to unsubscribe a stranger. That’s the entire spec, and it turns out to hide a surprising number of decisions. The token:

<keyVersion>.<subscriberId>.<base64url(HMAC-SHA256)>

Each choice here is load-bearing:

HKDF domain separation. The signing key is derived from the same rotatable keystore that encrypts subscriber emails, under a distinct salt/info pair—so the signing key can never decrypt a record and the record key can never mint a link.

const UNSUBSCRIBE_DOMAIN = { salt: 'rorbt-unsubscribe-salt', info: 'unsubscribe-link-signing' };

No PII in the URL. The id is an opaque snowflake, so the link is safe in mail logs, referrer headers, and screenshots.

The key version is inside the signed message, not just alongside it—so a token can’t be replayed under a different declared version:

const message = `unsubscribe:v1|${keyVersion}|${id}`;

Constant-time comparison, and the id is regex-pinned to digits before it ever builds an R2 key.

Deliberately no expiry. CAN-SPAM requires the link to work for at least 30 days after send; permanent is safer and stateless.

GET validates, POST mutates. This is the one people get wrong, and I’ve watched teams get it wrong at every company I’ve worked at. Mail clients, security scanners, and link pre-fetchers issue GETs on everything in an inbox. If GET unsubscribes, a corporate mail scanner silently unsubscribes your users, and you will spend a quarter wondering why churn spiked at companies with strict IT departments.

GET  /api/unsubscribe?t=<token>   validate, report status. Read-only.
POST /api/unsubscribe {token}     perform it.

Every failure collapses to one identical 400—missing token, bad signature, deleted record—so the endpoint can’t be used to probe which subscriber ids exist.

14 tests: round-trip, wrong key, tampered id, tampered signature, unknown version, garbage, non-numeric id, extra fields, URL-safety, and rotation in both directions. For an unsubscribe link. I stand by every one of them.

PII in logs, and log-spam as a DoS

Converting 58 console.error calls to structured logging sounds like janitorial work, which is why it surfaced two real findings—janitorial work is when you actually read everything.

A provider error body echoing user data. The email-send path logged the provider’s raw error response, which could plausibly contain the recipient address:

log.error('confirmation send rejected by the email provider', {
  scope: 'newsletter',
  status: res.status,
  providerBody: body.slice(0, 200),   // truncated on principle
});

Attacker-triggerable error volume. Several calls logged expected rejections at error level—failed iOS/Android attestation, chief among them. An unattested client failing is routine. Logging it as an error both drowns real failures and hands anyone a way to fill your error log on demand. Seven such calls were downgraded to warn, with the reasoning left in the code so future me doesn’t helpfully revert it:

// Rejections that are *expected* under normal operation get warn, not error:
// an unattested client failing attestation is routine, and logging it at error
// both drowns real failures and lets anyone spam the error log on demand.

The pattern across all six

Line the findings up and four of the six are the same finding wearing different clothes—claims that outran their implementation:

• CORS claimed to allow only our deployments.
• Hashed tags claimed anonymity.
• The privacy policy claimed complete deletion.
• Error-level logs claimed something had gone wrong.

The other two are structural choices with security consequences that no linter will ever flag, because linters check code and these were decisions: a scalar trust score, and a GET that mutates.

The review question worth institutionalizing is not “is this code correct” but “what does this claim, and does the implementation deliver the claim?”

Every finding here came from asking that question against one day’s worth of reading. None would have come from a dependency scanner, because a dependency scanner audits your suppliers and this class of bug lives in your promises. Your codebase is full of claims—in comments, in policy documents, in the shape of an allowlist, in the severity of a log line. Once a day, pick one and check whether it’s true.

---

Previous: Astro After Next.js · Next: Observability on the Edge