DEV Community

Daniel Pertu
Daniel Pertu

Posted on

Our alert endpoint rate-limits on a string the caller typed, and only then asks who they are

Notifio is a desktop app that watches rental search pages and emails you when something new appears. The desktop app does not talk to an email provider. It posts to /api/notify on our server with a licence token, and the server sends the mail.

That endpoint has five things it does before it sends anything, and the order they happen in is the entire design. Here is the order:

  1. parse the JSON body
  2. check the rate limit
  3. check that email and token are present
  4. look the licence up in Postgres
  5. branch on the alert type and send

Step 2 before steps 3 and 4 is the part that looks wrong, because it means the rate limit is keyed on a value we have not validated, supplied by a caller we have not identified.

The key comes out of the body

// Rate-limit per license (falling back to IP). Keying by token means users
// behind a shared/NAT IP each get their own budget instead of competing, and
// it no longer shares a bucket with /api/validate.
const ip =
  req.headers.get("x-forwarded-for")?.split(",")[0].trim() ?? "unknown";
const { success: withinLimit } = await ratelimit.limit(`notify:${token || ip}`);
if (!withinLimit) {
  return NextResponse.json({ error: "Too many requests." }, { status: 429 });
}

if (!email || !token) {
  return NextResponse.json(
    { error: "email and token are required." },
    { status: 400 }
  );
}

const [license] = await db
  .select()
  .from(licenses)
  .where(eq(licenses.token, token))
  .limit(1);

if (!license || license.email !== email || !license.active) {
  return NextResponse.json(
    { error: "Invalid or inactive license." },
    { status: 403 }
  );
}
Enter fullscreen mode Exit fullscreen mode

token at the point of the ratelimit.limit() call is whatever came in the body, uppercased and trimmed. Not checked against the database, not checked for length, not checked for being a string at all beyond (payload.token ?? "").trim().toUpperCase().

So anyone can pick a key in our rate limiter's namespace and spend it.

Why it is still the right order

The thing a rate limiter is for, on an endpoint like this, is keeping an unauthenticated flood off the database. Move the limit below the licence lookup and a loop of garbage requests is a loop of Postgres round trips, each one an index scan on licenses.token, each one through a connection pooler. The limiter would then protect only the mail provider, which is the second most expensive thing in the handler, and leave the first completely open.

That is not a close call. The limiter has to come before the lookup, and anything before the lookup is by definition unauthenticated. The only real question is what you key it on.

Two endpoints, two keys, two different victims

The sibling endpoint, /api/validate, which the app calls to check a licence is still good, keys on the IP:

const ip =
  req.headers.get("x-forwarded-for")?.split(",")[0].trim() ?? "unknown";
const { success } = await ratelimit.limit(ip);
Enter fullscreen mode Exit fullscreen mode

Both use the same limiter instance:

// 20 requests per 10 seconds, shared by validate and notify.
export const ratelimit = new Ratelimit({
  redis,
  limiter: Ratelimit.slidingWindow(20, "10 s"),
  analytics: false,
  prefix: "notifio:rl",
});
Enter fullscreen mode Exit fullscreen mode

Same window, same Redis prefix. What separates them is only the key string: notifio:rl:<ip> for one, notifio:rl:notify:<token> for the other. The comment in the notify route says it "no longer shares a bucket with /api/validate", and that is true, but it is the notify: in the key doing the separating, not the prefix option, which is identical for both. Worth knowing if you ever go looking in Redis.

The two choices have different victims, and that is the useful way to compare them.

Keyed on IP, the victim is a user sharing an address with other users. That is not hypothetical for this product: student housing is a building full of people hunting for rooms through one router, and a limit of 20 per 10 seconds spread across eight flatmates running the same app is a limit each of them can trip without doing anything wrong. That is why notify moved off IP.

Keyed on a caller-supplied token, the victim is a user whose token somebody else knows and decides to burn. Twenty requests in ten seconds against notify:THEIRTOKEN and their alerts start getting 429s.

The second victim is strictly better, for one reason: by the time you know somebody's token you can already do far worse than deny them. You can post alerts to their address yourself. You can run the app as them. A denial of service that requires the credential is not an escalation, it is a downgrade of an attack you already have. Whereas the IP collision needs no attacker at all; it just needs flatmates.

Tokens are 32 characters from a 32-symbol alphabet with no O, 0, I or 1, so the space is not somewhere you wander into by accident.

What the fallback actually protects

`notify:${token || ip}`
Enter fullscreen mode Exit fullscreen mode

The || ip branch is for a request with no token. Those are going to be rejected eight lines later with a 400, so it exists purely so that a flood of empty bodies gets a bucket of its own rather than all sharing one key. Without it, every tokenless request in the world would key on notify: and the first scanner to find the endpoint would 429 the second one. With it they are separated by source address, which is the correct resolution for traffic we know nothing else about.

Small thing. It is the difference between a limiter and a global kill switch.

The status codes do not agree with each other

One more thing fell out of reading these two side by side. Both check the same identity. They answer differently when it fails.

// /api/notify
if (!license || license.email !== email || !license.active) {
  return NextResponse.json({ error: "Invalid or inactive license." }, { status: 403 });
}

// /api/validate
if (!license) {
  return NextResponse.json({ valid: false, autoReply: false });
}
Enter fullscreen mode Exit fullscreen mode

A 403 on one, a 200 carrying valid: false on the other.

The validate shape is deliberate and I would defend it: the desktop app has to tell "we asked and the answer was no" apart from "we could not get an answer", and if a rejection and a network failure both arrive as non-2xx then the app has to inspect the status code to tell a dead licence from a dead wifi connection. Making the rejection a successful response with a payload puts that distinction in the body where it belongs. The app treats a transport failure as "unknown, carry on", and valid: false as "stop".

The notify route has no such requirement. It is fire-and-report, the caller logs the failure and moves on, and 403 is exactly what a 403 is for. So the two are inconsistent and both are right, which is an unsatisfying conclusion that I think is nonetheless the correct one. The rule is not "one status code per condition across the API". It is "the response shape is decided by what the caller has to do next", and these two callers have to do different things.

One subject line, up to fifteen names

Last observation, from the bottom of the same handler:

const totalCount = alerts.reduce((n, a) => n + a.newListings.length, 0);
const siteNames = alerts.map((a) => a.siteName).join(", ");

const { success, error } = await sendEmail({
  from: EmailAddresses.NOREPLY,
  to: email,
  subject: `Notifio: ${totalCount} new ${totalCount === 1 ? "listing" : "listings"} on ${siteNames}`,
  ...
});
Enter fullscreen mode Exit fullscreen mode

One poll cycle checks every enabled search and collects one AlertEntry per search that found something, then posts them all at once. So siteNames is every search that fired this cycle, joined, with nothing capping the length.

The cap is further upstream and it is a product decision rather than an email one: a user can have at most fifteen searches, because every search is scraped in sequence inside one cycle and fifteen is what keeps a full cycle under a minute. That number is written down on the pricing page and enforced in a constant the main process and the renderer both import.

Fifteen names a user typed themselves, joined with commas, in a subject line. RFC 5322 suggests header lines stay under 78 characters and permits up to 998, so nothing breaks, but Gmail is showing about 70 and a phone far fewer. The fix is not a character limit, it is to say "on Kamernet and 4 others" past some count, and to put the full list in the body where there is room. That is a nicer email and I have not written it yet.

If you want to see the per-site end of this, every site the app watches has its own page, for example the Kamernet one.

Top comments (0)