[devinder-dev]

Stacked on #31. The base is feat/api-auth, so the diff shows only this step. Once #31 is
merged I move the base to main and rebase (same changes, a new push), so it needs your
approval once more after that.

What

Steps 3.2 and 3.3 of Phase 3: the SSRF check, and the first customer resource behind the auth
hook from #31, POST /v1/endpoints and GET /v1/endpoints.

POST /v1/endpoints
  Zod body ─► checkEndpointUrl (DNS, no lock or connection held)
           ─► sql.begin: lock the owner row ─► count live endpoints ─► insert
           ─► 201 EndpointCreated (the only time signing_secret is sent)

Why it works this way

• The URL is checked where it leads, not by how it looks. ssrf.ts runs four steps:
  shape (https, port 443, no user:pw, no spaces or control characters, because the URL
  parser would silently rewrite them), local names (localhost, single labels, .local,
  .internal, .svc, .cluster.local, refused before any DNS), then every address the
  name resolves to (3 s limit), and every one must be public. Literal IPs skip DNS.
• "Public" has two independent parts. ipaddr.process() first, so ::ffff:7f00:1 is judged
  as the 127.0.0.1 it connects to. IPv4 must be unicast and not our VPS (81.88.23.71).
  IPv6 must be unicast and inside 2000::/3: ipaddr.js also calls ::7f00:1 and
  4000::1 "unicast", so the label alone isn't enough. An empty list or a string that isn't an
  address is refused (fails closed, a 422 and never a 500).
• One message for every address-based refusal. "Doesn't resolve" and "resolves to a
  private address" answer the same, so the API can't be used to map internal names.
• DNS runs before the transaction. A lookup can take seconds and a pod has 5 database
  connections; holding one, plus a lock, during a network wait would let a few slow lookups
  stall the pod.
• The 20-endpoint limit can't be raced. SELECT … FROM owners WHERE id = $1 FOR NO KEY UPDATE makes two creates of the same owner take turns. The count is its own statement after
  the lock, so under READ COMMITTED it sees the other create's committed row. Without the lock
  (or with the count before it) two creates at 19 both insert: the race test turns red in both
  cases. The lock is per owner, so other owners never wait.
• NO KEY UPDATE, not UPDATE. Every insert into events, endpoints or deliveries
  checks its owner with a FOR KEY SHARE lock (the foreign key). FOR UPDATE blocks that, so
  while one endpoint is being created, that owner's incoming events would wait. FOR NO KEY UPDATE still makes endpoint creates take turns but lets events through (we never change an
  owner's id). A test holds the real lock and inserts an event with a 1 s lock timeout; with
  FOR UPDATE it fails.
• The signing secret is shown once. whsec_ + base64 of 32 random bytes, only in the 201.
  GET builds every field by hand, so the secret can't slip into a list. Stored in plain text,
  unlike API keys, because the worker needs it to compute the HMAC.
• Status costs one indexed row per endpoint. failing = the newest delivery is dead, or
  pending after a failed try (attempts > 0); otherwise active, also with no deliveries.
  The newest delivery is ORDER BY d.id DESC LIMIT 1: ids are UUIDv7, so that is newest first
  and matches deliveries_endpoint_id. With 50,000 deliveries on one endpoint, sorting by
  created_at read all 50,000 rows (6.3 ms per endpoint); id DESC reads 1 row (0.011 ms).
• An operator allow-list, empty by default. SSRF_ALLOW_ORIGINS takes exact origins
  (for the Phase 4 demo receiver inside the cluster). Anything with a path, query, fragment,
  credentials or another scheme stops startup. Customers can't set it.

Changes

• API: ssrf.ts, schemas/endpoints.ts, services/endpoints.ts,
  repositories/endpoints.ts, routes/endpoints.ts; routes/v1.ts registers them; app.ts
  takes an optional lookup (tests use fake DNS); config.ts (SSRF_ALLOW_ORIGINS);
  errors.ts (invalid_endpoint_url 422, endpoint_limit_reached 409).
• Dependency: ipaddr.js pinned to exactly 2.5.0.
• Tests: unit tests for every refused shape and name, literal addresses (including
  [::ffff:127.0.0.1], [::127.0.0.1], [4000::1], Teredo, 6to4, NAT64, our VPS in both
  forms), both ends of every reserved IPv4 block with the public neighbours just outside,
  DNS errors, empty answers and timeouts, the allow-list, and allAddressesPublic on its own.
  Schema tests check which field failed. Integration tests against a real Postgres: create and
  list (no secret in the list), owner separation, 401, the limit with a soft-deleted endpoint
  freeing a slot, two simultaneous creates at 19 (exactly one 201 and one 409, held at the lock
  by LOCK TABLE … IN SHARE MODE and pg_stat_activity, no timing guesses), another owner
  not waiting, status from the newest delivery, and a failure after DNS that saves nothing and
  logs no secret.
• Docs: docs/api.md (no control characters in name, the URL rules, unknown fields →
  422, the one refusal message, the allow-list).

No migration: the tables and indexes are from migration 001.

Tested

• bun run typecheck, bun run test:db: 239 pass, 0 fail.
• Negative controls: removing the IPv6 2000::/3 rule, the VPS rule, the empty-list check,
  the catch, or process() each turns at least one test red; so does removing the owner
  lock or counting before it.
• Snyk Code: 0 issues. bun audit: no vulnerabilities (62 packages).

After merge, checked from outside

• https://[::ffff:127.0.0.1]/, https://[::127.0.0.1]/,
  https://postgres.relay.svc.cluster.local/, https://81.88.23.71/,
  https://api.myrelay.tech/ → all 422 invalid_endpoint_url, the same message.
• A real public https URL we control with "event_types": ["live.check"] → 201 (the
  positive control).

Still open, on purpose (Phase 4)

The worker checks the URL again before every attempt, connects only to the checked address,
gets a NetworkPolicy, and never follows redirects. A second check alone only narrows DNS
rebinding; those together close it.

[AbdikhaniAbdirashid]

Read all 20 files. Approving — the SSRF work is the most thorough thing in the repo. Two findings,
neither a blocker for today.

What convinced me. ssrf-edges.test.ts checks both ends of every reserved IPv4 block plus the
ordinary neighbours just outside them, which is the only way to show the boundary is where you think
it is. ipaddr.process rather than parse means ::ffff:10.0.0.1 is judged as the IPv4 it would
actually connect to. allAddressesPublic requires every resolved address to be public, so a name
answering with one public and one private address is refused. The check also runs before
sql.begin(), so no transaction or connection is held during a 3 s DNS wait — and the operator
exception is validated in config.ts and empty by default, so customers can never reach it.

1. DNS rebinding is the remaining gap, and I'd rather we write it down than be asked about it.
The check resolves a name; it doesn't pin the address:

creation:  attacker.com -> 93.184.215.14   accepted, stored
delivery:  attacker.com -> 10.42.0.5       now inside the cluster

Re-checking before every attempt (Phase 4) narrows this a lot, but between resolve and connect there
is still a window. The complete fix is to resolve once and connect to that exact IP with the Host and
SNI preserved. I'm not asking for it in this PR — Phase 4 isn't written. I'd like one line in
docs/api.md or an ADR saying the gap is known and where it closes, because the layered defence that
saves us today (cluster addresses are private, so allAddressesPublic refuses them even when DNS
lies) is worth stating rather than relying on by accident.

2. DELETE /v1/endpoints/:id is in the contract but not implemented. docs/api.md specifies it,
and the 20-endpoint limit counts deleted_at IS NULL, so the slot-freeing the contract describes
can't happen yet. Which step does it land in? I ask because of the dashboard — see my note on #33.

One question, not a finding: generateSigningSecret uses plain base64, so the secret can contain
+, / and =. Fine for a header value and it matches Standard Webhooks, but is there a reason it
isn't base64url like the API key?

FOR NO KEY UPDATE instead of FOR UPDATE, because the foreign-key check on an event insert takes
FOR KEY SHARE and would otherwise queue behind it — that's the kind of reasoning I want in the
comments, and it's there. Thanks.

[AbdikhaniAbdirashid]

Approved. Two questions above - the DELETE route and where the DNS-pinning gap gets documented.

The base branch was changed.

[devinder-dev]

Rebased onto main after #31 merged, so this needs your approval once more (moving the base to main dismissed the earlier one). Nothing changed in the code:

$ git range-diff 59bd09a..a23b652 origin/main..HEAD
1:  af28301 = 1:  9387ac1 feat: endpoints with an SSRF check and a 20-endpoint limit (steps 3.2 + 3.3)
2:  a23b652 = 2:  f8abf60 fix: lock the owner row FOR NO KEY UPDATE so endpoint creates never block events

= means the same patch. bun run test:db on the rebased branch: 239 pass, 0 fail.

[AbdikhaniAbdirashid]

Re-approving after the rebase: range-diff shows both commits as =, so nothing changed in the code. My three points from the earlier review are still open — the rebinding note especially, since that is the one an examiner will ask about.