parseInt returned NaN, and splice(NaN, 1) deleted the first search

Notifio is an Electron app. The renderer is a web page, and it talks to an Express server running inside the app's own main process, on loopback:

// Bind to loopback only. This API exposes the license email/token and full
// monitor control with no auth, so it must never be reachable from the LAN.
const server = app.listen(Number(PORT), '127.0.0.1', () => {
  console.log(`Notifio running at http://localhost:${PORT}`);
});

So: no auth, no CSRF tokens, no rate limiting, one caller, and that caller is code I wrote and ship in the same binary. I have already argued that a local app cannot keep a secret from its owner and should stop pretending otherwise, and that applies here. There is no threat model in which somebody else is posting to this API and the licence token is not already theirs.

Every route validates its input anyway. Not for security. Here are the four reasons, each of which is a bug I actually had or nearly had.

1. The adversary is parseInt

app.delete('/api/sites/:index', (req: Request, res: Response) => {
  const idx = parseInt(req.params['index'] as string, 10);
  const cfg = loadConfig();
  // Number.isInteger guards against NaN (e.g. a non-numeric :index). Without
  // it, NaN slips past the range check and splice(NaN,1) deletes the first site.
  if (!Number.isInteger(idx) || idx < 0 || idx >= cfg.sites.length) {
    return res.status(404).json({ error: 'Not found' });
  }
  const [removed] = cfg.sites.splice(idx, 1);

This is the bug worth the title. Work through it with the guard removed:

parseInt('undefined', 10) is NaN. NaN < 0 is false. NaN >= cfg.sites.length is false. Every comparison against NaN is false, so a range check built out of comparisons waves it straight through. Then:

['a', 'b', 'c'].splice(NaN, 1)  // removes 'a'

because splice coerces its start index and NaN becomes 0. So a request that identifies no search at all deletes the first one, which for this app means the user silently stops being told about listings on a search they are still looking at in the window.

Two properties of JavaScript conspire here: comparison operators return false for NaN rather than throwing, and array methods coerce rather than rejecting. Neither is going to change, so the guard is Number.isInteger, and a range check that is only comparisons should always make you look for the value that makes all of them false.

2. The adversary is time

Both of those routes take an array index as the identity of a search. That is fragile in a way that has nothing to do with the parsing:

const removeSearch = useCallback(
  async (url: string) => {
    const idx = indexOf(url);
    if (idx < 0) return;
    await deleteSite(idx);
    await refreshSites();
  },
  [indexOf, refreshSites]
);

The renderer resolves the URL the user clicked into an index using its own copy of the list. If that copy is stale, the index refers to a different search than the one whose remove button was pressed, and both the client and the server think the request is perfectly valid. Index as identity means every write is implicitly asserting that the client's list matches the server's.

In this app the window is narrow, because the list only changes from the same window. It is still the reason the 404 exists rather than a clamp: refusing an index the server does not recognise is the only chance to notice the mismatch. If I were adding a second client to this API, this is the design I would change first, to a stable id in the URL.

3. The adversary is a paste

new URL(value) is not a validator for "is this a web page". It parses plenty of things happily:

let parsedUrl: URL;
try {
  parsedUrl = new URL(url);
} catch {
  return res.status(400).json({ error: 'Invalid URL' });
}
// Only http(s) pages are monitorable, so reject file:, javascript:, etc.
if (parsedUrl.protocol !== 'http:' && parsedUrl.protocol !== 'https:') {
  return res.status(400).json({ error: 'URL must start with http:// or https://' });
}

The scraper is a real browser, so a file: URL is not nonsense to it. It is a local file it will cheerfully open, diff against a previous read, and email you about. An allowlist of two protocols is the whole fix, and the failure mode without it is not an exploit, it is an app doing something absurd with total confidence.

Two more refusals live in the same handler, and they are product rules rather than parsing:

if (cfg.sites.some((s) => s.url === url)) {
  return res.status(400).json({ error: 'This URL is already being monitored' });
}
// Searches are scraped sequentially, so an unbounded list quietly degrades
// how fast every other search is checked. Cap it here rather than letting a
// user turn the app into something too slow to be useful.
if (cfg.sites.length >= MAX_SEARCHES) {
  return res.status(400).json({
    error: `You can monitor up to ${MAX_SEARCHES} searches. Remove one to add another.`,
  });
}

MAX_SEARCHES is 15, it lives in a module with no imports so the renderer and the server share the constant rather than each keeping a copy, and why the number is a promise about latency rather than a pricing tier is its own post. The relevant part here is that the server enforces it even though the Add button is already disabled at the limit. A disabled button is a hint, not a rule.

4. The adversary is a change that invalidates data we hold

The most interesting validation in the file is the one on editing a search URL, because what it protects is not the config file:

if (typeof url === 'string' && url.trim()) {
  // Validate that the new URL keeps the same hostname (only path/query edits allowed)
  try {
    const oldUrl = cfg.sites[idx].url;
    const oldHost = new URL(oldUrl).hostname;
    const newUrl = url.trim();
    const newHost = new URL(newUrl).hostname;
    if (oldHost === newHost && newUrl !== oldUrl) {
      cfg.sites[idx].url = newUrl;
      // The snapshot is keyed by URL, so the old baseline is now orphaned,
      // so drop it. The new URL re-baselines silently on its next poll.
      monitor.deleteSnapshot(oldUrl);
    }
  } catch { /* invalid URL, ignore */ }
}

A search carries more state than its row suggests: a diff baseline of what its page held last time, a login session for its host, and a recorded reply flow for that host. Editing the filters on a Kamernet search invalidates exactly one of those, the baseline, which is why the handler deletes it in the same breath as writing the new URL. Editing the hostname would invalidate all three, and silently, since nothing about the row would look different afterwards.

So the server refuses the hostname change and the UI never offers it. The edit control only ever reconstructs the URL from the original origin:

nextUrl = new URL(site.url).origin + editPath;

Which means there are two independent reasons a hostname edit cannot happen, and I want both. The client side one is so the user gets a sensible editing experience. The server side one is so the invariant survives the next time somebody changes that component.

The user is also told which part of the state was thrown away:

showToast(
  ok
    ? "Search URL updated, monitoring restarts from a fresh baseline"
    : "Could not update that URL"
);

"Restarts from a fresh baseline" is there because the first check after that edit will not alert, by design, and a user who is not told that will read the silence as a broken app.

The rule I took from this

Input validation on a trusted API is not about attackers. It is about the fact that the inputs are produced by a program, and programs are wrong in ways people are not. A person does not type undefined into a route parameter. A stale React state does, exactly once, on a Tuesday, and splice will do something for it rather than complaining.

If you have an internal API you have been skipping validation on, the cheap audit is not a security review. Take every route parameter you coerce, and ask what your code does when the coercion fails rather than throws.

The app that all of this sits inside is a free download for Mac and Windows. What a search actually is, per rental site, is spelled out under alerts, for example Kamernet and HousingAnywhere, and the help page covers the editing rules from the user's side.