DEV Community

Cover image for My safety guard was checked in 6 places. Nothing ever set it.
אחיה כהן
אחיה כהן

Posted on

My safety guard was checked in 6 places. Nothing ever set it.

A user of my Safari MCP server — the user was me, from a scheduled run — hit a failure I could not talk my way out of.

Right after the background daemon restarted, the server opened a browser tab and handed back a capability token for it. The token is how every later command proves it is allowed to touch that tab, because the whole point of the ownership system is that the server must never act on a tab you opened. Thirty-three seconds later, the very next command was refused:

Tab safety: receipt is forged, stale, ambiguous, or not valid for this origin
Enter fullscreen mode Exit fullscreen mode

Forged. By me. For a tab I had just opened, four lines earlier.

Worse, it was unrecoverable from the client side. The token could not be rotated. Switching to the tab by index was refused — "not opened by this MCP session." Closing it was refused too, which is correct, since closing is the one action you cannot undo and ownership could no longer be proven. So the tab sat there, stranded and unusable, and the agent had to reopen its work somewhere else.

Ruling things out

I had two obvious hypotheses and both died in the same session.

"A newer tab invalidates the older token." Minted a token, opened a second tab, used the first token afterwards. Worked fine.

"Switching by index corrupts the next command." Switched by index, ran a command with no token at all. Worked fine.

The one thing every failure had in common was timing. Everything opened once the daemon had been up for a couple of minutes behaved perfectly. Only the tabs created inside the restart window went bad. The first tab creation in that window took 22 seconds — the extension's background worker was still cold-starting — and that number turned out to be the entire story.

The actual mechanism

Tokens are stamped with a browser-session epoch: a random identifier that ties a token to one continuous run of the browser. If the browser restarts, the epoch rotates, and every old token is correctly rejected instead of being allowed to point at a recycled tab id. Good design. The epoch lives in session storage, which is the authority.

Here is the function that reads it:

async function _ensureBrowserSessionEpoch() {
  if (!_browserSessionStorageAvailable) {
    throw new Error("Browser session storage is unavailable; refusing durable tab authority");
  }
  if (/^[a-f0-9]{36}$/.test(_browserSessionEpoch)) return _browserSessionEpoch;
  // ... otherwise read storage, or mint and persist a new epoch
}
Enter fullscreen mode Exit fullscreen mode

That last if is the problem. Once the worker has resolved an epoch into memory, it never consults storage again for the rest of its life. The cached copy is treated as the truth.

So when the epoch rotates in storage while the worker is still alive — which is exactly what a daemon restart plus an extension re-handshake does — the worker keeps stamping brand-new tokens with the old epoch, and writes them to durable storage that way. Nothing complains, because nothing is looking.

The complaint arrives later, from the one path that does read storage. When the worker eventually cold-starts, it rehydrates its tables and filters the stored tokens:

const receiptsFresh = receiptEnvelope &&
  receiptEnvelope.version === _TAB_RECEIPTS_VERSION &&
  receiptEnvelope.browserEpoch === browserEpoch && ...
Enter fullscreen mode Exit fullscreen mode

Every stored record carries the stale epoch. receiptsFresh is false. The entire record array is discarded, the in-memory tables are cleared and repopulated from that empty set, and every token minted during the stale window dies at the same instant. Which is precisely what the report showed: two tabs, created five seconds apart, rejected together.

The part that actually bothers me

There was a guard for this. It just could not fire.

The code tracks a counter whose whole job is to notice that the browser-session identity changed underneath an in-flight operation. It is compared at six call sites, across the three functions that own the epoch:

2885, 2896, 2920   _ensureBrowserSessionEpoch
3043               _hydrateOwnedTabs
3075, 3095         _persistOwnedTabs
Enter fullscreen mode Exit fullscreen mode

Each comparison guards a real invariant, and each one has a hand-written error message explaining what went wrong: "Browser-session identity changed during initialization." "Browser session changed during tab ownership recovery."

The variable is assigned exactly once, on line 2819:

let _browserEpochGeneration = 0;
Enter fullscreen mode Exit fullscreen mode

That is the declaration. Nothing increments it. Ever.

$ grep -nE '_browserEpochGeneration\s*(\+\+|--|=[^=])' extension/background.js
2819:let _browserEpochGeneration = 0;
Enter fullscreen mode Exit fullscreen mode

One hit, and it is the let. Six comparisons against a constant, plus three sites that dutifully capture it into a local first.

Why it survived review, including mine

I have been staring at this file for months, and I want to be precise about why I did not see it, because "you should have looked harder" is not a lesson anyone can apply.

The guards read as protection. Every site looks like careful concurrency code. There is a captured local, an await, and a re-check afterwards — the exact shape you are trained to pattern-match as correct. Reading for shape rather than for reachability, it passes.

Three of the six had a live half. They are compound:

if (generation !== _browserEpochGeneration || browserEpoch !== _browserSessionEpoch) {
Enter fullscreen mode Exit fullscreen mode

The second clause works. So those three guards do fire, sometimes, on the epoch comparison alone — which is exactly why this bug shows up at rehydration time rather than never at all. Half of the protection was real, and the working half masked the dead half. If all six had been dead, the failure would have been louder and I would have found it sooner.

The other three, the fully dead ones, were the ones that mattered most. They sit inside _ensureBrowserSessionEpoch and have no second clause, because that function is the thing that sets the value they would compare against. Those three were supposed to catch the rotation at mint time, before a doomed token is ever handed to a caller. Instead the rotation is caught at rehydration, long after the bad tokens are in the client's hands and in durable storage.

A dead guard is worse than a missing one. A missing guard is an obvious gap. A dead guard is a gap plus a comment asserting the gap is covered, and it makes the next person to read the file — me, repeatedly — stop looking.

What would have caught it

Not a unit test. Every behavioural test over these paths passes, and would still pass with all six comparisons deleted, because the code under test never rotates the epoch. The defect is an omission, and you cannot observe an omission by exercising what is there.

This project already had one test for exactly that category, written after a different omission bug, and its own comment says so:

This is a source-level check on purpose: the failure is omission at a call site, which no behavioural test over the existing sites can see.

That test reads the source as text and asserts a property about the call sites. The same trick works here, and the rule is embarrassingly simple:

A variable that is compared must be assigned somewhere other than its declaration.

That is a grep. It is cheap enough to run over every mutable module-level variable in a file, and it would have failed loudly the day this landed. Not a linter rule I had to invent either — "assigned once, read many, never reassigned" is the shape, and the moment you write it down you realise how many state machines and generation counters and dirty-flags could rot this way without a single test turning red.

What I have not done

I have not shipped the fix.

The change itself is small: stop trusting the cached epoch, re-verify it against storage, and increment the counter when the two disagree — which lights up all six existing guards at once. I know what to type.

But this is the tab-ownership guard, and the reason it exists in the first place is that early versions of this server genuinely clicked into and closed people's tabs. The failure mode of getting the fix wrong is inverted from the current bug: today a good token is rejected rarely, in a narrow restart window. An over-eager rotation check would reject good tokens routinely. That is a strictly worse trade, and I am not making it against a theory. I want a staged reproduction that captures the epoch from both storage and the persisted record at mint time, and shows them differing, before I touch the file.

So for now the bug is open, documented, and worse than the fix — on purpose. The guard is dead either way; at least now it is dead and labelled.


If you want to look: the server is safari-mcp, and the full analysis lives on issue #105.

The question I am genuinely stuck on: how do you catch a dead guard in your own code? Grepping for compared-but-never-assigned finds this exact shape, but it is one instance of a bigger family — the if that can never be true, the catch that can never be reached, the flag nobody flips. Has anyone found a check for that family that is cheap enough to run on every commit and quiet enough that people do not start ignoring it?

Top comments (0)