I built a guard that refused to read the user's tab. Then my own cleanup code closed it.

작성자

카테고리:

← 피드로
DEV Community · אחיה כהן · 2026-07-27 개발(SW)

Three days ago my browser automation tool closed one of my own tabs. Not a tab it had opened — a dashboard I had open in another window, with a page I hadn’t finished reading.

What makes it worth writing up isn’t the bug. It’s that the guard designed to prevent exactly this had already fired, correctly, ninety seconds earlier.

The guard worked

Safari MCP lets an AI agent drive your real, logged-in Safari. That premise means the single worst thing it can do is act on a tab you’re using. So there’s an identity system: every tab the tool opens gets a marker stamped into window.name, which survives navigation, redirects, and cross-origin loads. Before running anything in a tab, the tool checks the marker.

I was filling in a form. The URL was a forms.gle shortlink, which 302s to docs.google.com — a cross-origin redirect that, it turns out, drops window.name. My next read came back refused:

Tab tracking lost — refusing to target the user’s current tab.

Correct. Exactly the intended behaviour. The tool no longer knew which tab was its own, so it declined to guess.

So I did the tidy thing and cleaned up my orphaned tab:

safari_close_tab

Enter fullscreen mode Exit fullscreen mode

It closed a different tab. One of mine. The tool went from “I can’t prove which tab is mine, so I won’t read” to “let me close a tab” in one step, and nobody stopped it.

The shape of the hole

Here is the close path as it existed:

if (_st().activeTabIndex) {
  await osascript(`... close tab ${_st().activeTabIndex} of ${window}`);
} else {
  await osascript(`... close current tab of ${window}`);  // ← the user's tab
}

Enter fullscreen mode Exit fullscreen mode

current tab of window is whatever the user is looking at. So the fallback for “I don’t know which tab is mine” was “close theirs.”

That branch is only reachable when the index is unknown — which is precisely the state the guard had just announced. The two pieces of code were describing the same condition and disagreeing about what it meant.

Three layers, one mistake

When I went looking, the same fail-open was in all three layers of the stack, and it had the same shape every time: no ownership recorded was read as permission, not as refusal.

The first one is the most embarrassing, because it’s a list:

const _noOwnershipCheck = new Set([
  // Tab management
  "new_tab", "list_tabs", "close_tab", "switch_tab",
  ...

Enter fullscreen mode Exit fullscreen mode

A set of operations exempt from the ownership check, grouped under the comment “read-only or tab management”. Three of those four are harmless: new_tab creates a tab, list_tabs reads, and switch_tab carries its own ownership check at the tool level. close_tab destroys a user tab and had no check anywhere.

It wasn’t exempted by argument. It was exempted by category — it looked like tab management, it sat next to tab management, so it inherited tab management’s safety assumptions. Nobody ever wrote down “closing a tab is safe.” The list said it, silently, by adjacency.

The third layer, the browser extension, had a variant worth naming on its own. Its guard refuses an operation when the session owns tabs and this isn’t one of them — but when the session owns nothing, it allows the operation, with this comment:

// If no tabs owned yet, allow operation (backward compatibility for sessions that don't use new_tab)

Enter fullscreen mode Exit fullscreen mode

That leniency is sensible for a write: worst case it edits the page you’re already on. For a close it’s a disaster. And “owns nothing” is exactly what a session reports after its transport drops and the client re-initialises — mid-task, tab still open. The lenient branch is reachable only in the state where it’s most wrong.

The rule I actually got wrong

My first instinct for the fix was to reuse the rule the read path already uses: refuse a tab carrying another session’s marker, allow an unmarked one. It keeps “read the page I’m looking at” working for a session that genuinely never opened a tab.

That rule would not have prevented this. The tab I destroyed was a genuine user tab. It had no marker at all. Under “unmarked is fine”, it sails straight through to tabs.remove().

Which forced the actual distinction: a wrong read costs information; a wrong close costs the user their work. They don’t get the same rule. The read paths keep the lenient fallback on purpose. The destructive path gets no fallback whatsoever — it closes a tab it can positively prove it owns, or it throws:

const idx = explicitIndex || (await _provenOwnTabIndex());
if (!idx) {
  throw new Error(
    `Tab tracking lost — refusing to close a tab this session cannot prove it opened ...`
  );
}

Enter fullscreen mode Exit fullscreen mode

No else. “Owns nothing” now means “closes nothing”, which is the sentence that should have been in that first list all along.

A few adjacent things fell out of it. Blanking a window’s last tab — the workaround for “closing this would quit Safari” — used the same fallback, and throwing away someone’s loaded page is destructive too; it’s pinned to the proven index now. And in the extension, the guard validated one tab (tabId) while the indexed branch removed a different one (_winTabs[index - 1]), so the check and the action were pointed at different tabs. That one had never fired in production; it was just waiting.

What I’d take from it

The guard wasn’t wrong and the fallback wasn’t wrong. What was wrong is that they were two different answers to one question — do we know which tab is ours? — living in two files, and only one of them had thought about what the answer implied.

I’ve since stopped trusting the word “safe” in a category name. close_tab was in a set called tab management, and the set was right: it is tab management. Categories describe what an operation is. Guards have to be about what it costs when it’s wrong. Sorting by the first and inheriting the second is how a destructive call ends up on the exempt list with no one having decided that.

The test I wrote isn’t behavioural, for the same reason as last time: the defect is a missing refusal on a path that only runs after state is already lost. There’s nothing to exercise. It reads the source and asserts that no AppleScript verb in the close path targets the front document — and I checked that it fails when I put the old branch back, because a regression test you haven’t seen fail is a guess.

Fixed in v2.15.8. The repo is safari-mcp — the full analysis is in issue #68.

A question for you: how do you catch the operations that got their safety assumptions by adjacency — the ones sitting in a list they only half belong to? Every review I’ve done reads the code in the branch, not the membership of the set above it.

원문에서 계속 ↗

코멘트

답글 남기기

이메일 주소는 공개되지 않습니다. 필수 필드는 *로 표시됩니다