DEV Community

Mahiro Hirakawa
Mahiro Hirakawa

Posted on

My guard asserted the output path was a scratch directory. The row landed in the permanent log anyway.

There is a rule here that tests never write to the real record store, and unlike most of my rules it was already a device rather than a sentence. Before its first write, any check asserts that its data root is set and points outside the real tree, and refuses otherwise.

The rule exists because of an earlier incident, where an end-to-end run appended three entries into the permanent log and nobody noticed until the numbering looked wrong. The guard was the repair.

Months later a reviewer tool minted a row into that same permanent log. The guard was green on the run that did it.

What the guard compared

Reduced to its shape:

const root = process.env.RECORD_ROOT;
if (!root) throw new Error('RECORD_ROOT not set');
if (root.startsWith(REAL_TREE)) throw new Error('refusing to write the real tree');
// ... proceed to write under root
Enter fullscreen mode Exit fullscreen mode

The scratch root was a fresh directory under the temp area. The string did not start with the real tree's path. The assertion was true.

Inside that scratch tree was a junction, the Windows form of a directory symlink, created to give the tool the same layout it sees in the real checkout without copying a large tree. When the tool opened a file under that junction, the runtime followed it. The write went where the link pointed.

what the guard looked at what the write used
the string in an environment variable the path the filesystem resolved
the intent, before anything opened the destination, after every link was followed

Both are called "the path". They are not the same object, and a string comparison cannot tell them apart.

The fix is two lines, and one of them is the interesting one

import { realpathSync } from 'node:fs';

const root = realpathSync(process.env.RECORD_ROOT);        // resolve, then compare
if (root.startsWith(realpathSync(REAL_TREE))) throw new Error('...');
Enter fullscreen mode Exit fullscreen mode

Resolving both sides is the obvious half. The half worth keeping is the second change, which is to move the assertion from the intent to the handle. Instead of checking a path before writing, check what you actually opened:

const fh = await open(target, 'w');
const { ino, dev } = await fh.stat();
if (isUnderRealTree(dev, ino)) throw new Error('...');
Enter fullscreen mode Exit fullscreen mode

That version cannot be defeated by anything the resolver does, because it is asking the thing that was actually opened rather than the request that produced it.

The general shape

A guard that validates an intent is not guarding the action. Between the value you check and the effect that happens, there is a resolver, and the resolver is a component you did not audit.

This is not specific to symlinks. The same gap is everywhere:

what you check                 what resolves it            what actually happens
a path string          ->      symlinks, junctions, ..  -> a different inode
a hostname             ->      DNS, /etc/hosts, proxy   -> a different server
a database name        ->      connection pooling, DSN  -> a different database
a branch name          ->      remote refs, worktrees   -> a different ref
a container mount      ->      bind mounts              -> the host filesystem
Enter fullscreen mode Exit fullscreen mode

Every row is the same failure waiting. The check passes on the name, the effect lands on the resolution, and the distance between them is invisible in the log line that says the check was green.

Plant the resolver in your own fixtures

The lasting change was not the two lines. It was that this class became something the test fixtures deliberately produce.

A related checker in the same project had been silently skipping files it could not read, which nobody knew because no fixture had ever contained one. Its selftest now plants two: a dangling junction, and a file with permissions denied. The checker is required to report both rather than pass over them.

Once you know a resolver sits between your check and your effect, the fixture has to contain something that resolves surprisingly. Otherwise the only place that ever happens is production.

Two things I keep

Compare resolved values, or better, do not compare paths at all. Ask the opened handle where it is. A string is a request, and the answer to a request is a different thing than the request.

A rule that is already a device can still be wrong about what it measures. I had promoted this one from a sentence to a check and then stopped thinking about it, which is precisely the moment it is least examined. Being enforced is not the same as being correct.

Top comments (0)