DEV Community

Cover image for My new repo argued against the exact bug it was carrying
Vinicius Pereira
Vinicius Pereira

Posted on

My new repo argued against the exact bug it was carrying

Not long ago, Python's casefold() merged two of my customers into one tenant. I wrote that incident up in its own post, and the rule I carried out of it felt permanent: when a normalized value becomes a security key, the normalization is attack surface. Any many-to-one transform over an identity is a merge, and in an entitlement check a merge is a grant.

This month I finished the sequel to that repo. on-behalf is permission-aware retrieval for RAG over SharePoint and Google Drive: instead of copying ACLs at ingestion, it asks the source whether the asking user can open each candidate document, at query time, as the user, before anything gets ranked. Emails are the join key between an asking user and a sharing grant there, so models.py carries the lesson from last time as one small function, docstring pointing back at the whole argument:

ASCII_WHITESPACE = " \t\n\r\v\f"

def normalise_email(value: str) -> str:
    trimmed = value.strip(ASCII_WHITESPACE)
    fold = str.maketrans("ABCDEFGHIJKLMNOPQRSTUVWXYZ", "abcdefghijklmnopqrstuvwxyz")
    return trimmed.translate(fold)
Enter fullscreen mode Exit fullscreen mode

One correct implementation, in one place, born from a production incident. And in a review pass the day before publication, I found this in the package's own fakes:

user = GraphUser(user_id=user_id, email=email.strip().lower())
Enter fullscreen mode Exit fullscreen mode

FakeGraph and FakeDrive, the deterministic permission models that ship inside the package, were normalizing every email with str.lower(). The repo was arguing against the exact bug it was carrying.

str.lower() is the polite one

lower() reads like the harmless sibling of casefold(). It skips the famous German fold: "ß".lower() is still "ß", where "ß".casefold() is "ss". But it is still a Unicode operation with many-to-one mappings inside it. U+212A is the Kelvin sign, a character that renders as a capital K in most fonts, and "K".lower() is a plain ASCII "k". One character, one method call, two identities become one.

Two spellings of the same address: str.lower() folds them into one grant, the ASCII-only fold keeps them apart

The same two addresses through both folds. The left side is the bug; the right side is the doctrine, now anchored by a test.

I didn't leave that as a hypothetical. Before the fix, inside the fakes, a sharing link naming Kevlar@outside.example spelled with the Kelvin sign resolved to a PermissionProof for the ASCII user kevlar@outside.example. Two addresses the package's own normalise_email() keeps strictly apart, folded into one grant by the code whose job is to demonstrate the fence.

No test caught it going in, and the reason deserves a hard look: both sides of the comparison went through the same wrong fold. Link grantees were lowered, stored user emails were lowered, so every case-insensitivity test passed. A normalization bug in a join key is self-consistent. It doesn't crash and it never disagrees with itself. It just quietly merges identities that the doctrine one directory up says must never merge.

Fakes that demonstrate a guarantee are production code

This would be a smaller story if those were throwaway fixtures. They aren't. The fakes ship in on_behalf.fakes because they are the offline half of the product: they model SharePoint effective access (nested groups, folders that break inheritance, sharing links with expiry) and Drive's two auth modes, and they're what lets the demo and the 353-test suite run the full pipeline with no network, no credential, no key. When the suite proves "a sharing link is dead at the exact expiry instant", it proves it against these models. A test that validates the fence using a biased copy of the fence validates nothing.

So the fix was not a better fold in the fakes. It was deleting the second implementation. The fakes now import the same function everything else uses:

from ..models import normalise_email

user = GraphUser(user_id=user_id, email=normalise_email(email))
Enter fullscreen mode Exit fullscreen mode

and the link-grantee matching goes through the same door:

user.email in {normalise_email(person) for person in link.people}
Enter fullscreen mode Exit fullscreen mode

anchored by a test so the rule can't regress back into prose:

def test_link_people_matching_folds_ascii_only_never_unicode(
    tenant: GraphTenant,
) -> None:
    # "K" U+212A (Kelvin sign) lowercases to "k" under str.lower(); the fakes
    # must use the same ASCII-only fold as the rest of the package, so a link
    # naming the Kelvin-sign spelling is a different identity, not a grant.
    tenant.items["d-pub"].links.append(
        SharingLink(
            link_id="lnk-kelvin",
            scope=LinkScope.SPECIFIC_PEOPLE,
            people=("Kevlar@outside.example",),
        )
    )
    kevlar = tenant.add_user("u-kevlar", "kevlar@outside.example")
    decision = _resolve(tenant, kevlar, "d-pub")
    assert isinstance(decision, Denied)
Enter fullscreen mode Exit fullscreen mode

The first character of that link's address is U+212A, not an ASCII K. They render identically on most screens, which is the whole problem, and why the comment spells it out.

Here's the part I keep coming back to. The doctrine existed the whole time. It was written down twice, once in the casefold post and once in the normalise_email docstring, and implemented correctly in exactly one place. The fakes just didn't call it. A security rule that lives in prose is advice. It becomes a property of the system on the day the function that implements it is the only door, and every caller, fakes included, has to walk through it.

The repo the bug was hiding in

The package deserved a cleaner arrival, because its own thesis is this same argument at a larger scale: a permission snapshot is a leak with a start date. Most "permission-aware" RAG connectors do this:

# ingestion, nightly
for doc in source.list_documents():
    index.add(doc, allowed_users=source.read_acl(doc))   # a copy

# query time
hits = [h for h in index.search(q, k) if user in h.allowed_users]   # the copy decides
Enter fullscreen mode Exit fullscreen mode

That copy is wrong the day after it is right, in three distinct ways, and on-behalf ships a deliberately wrong SnapshotACLIndex (it raises a UserWarning at construction) so the three leaks are demonstrated by executable tests instead of described by paragraphs: a revoked direct grant the snapshot keeps honouring, a group the user left that the snapshot still counts them into, a sharing link that expired after ingestion. Each test asserts both directions, leak present in the snapshot and absent from the query-time path, so the demonstration can't pass by both sides being broken.

The live index and the snapshot index answering the same question after access changed at the source: three leaks, three mechanisms

Same question, two indexes, after permissions moved. The snapshot believes 09:00 forever.

The offline demo runs one synthetic tenant through the same question at 09:00 and at 12:00, permissions moving at lunchtime, the index never re-ingested. Between the two runs one user loses a memo and another loses everything, while the index still holds all five documents, and the snapshot section closes on the sentence I'd put on a slide: the snapshot cannot see any of it, "because the event it needed to observe happened after the only moment it ever looked."

The offline demo: one tenant, the same question as three users, then the access decay and the snapshot leaks

The whole demo, deterministic, no keys. Every number in this post comes from it.

The fix is ordering, twice over:

candidates = index.candidates(query)              # a lexical gate, unscored
decisions  = source.can_open_many(user, ids)      # the source answers, NOW
entitled   = [d for d in candidates if proven(d)] # proof or absence, no third state
return rank(query, entitled)[:k]                  # score last, cut last
Enter fullscreen mode Exit fullscreen mode

Entitlement runs before ranking, so a document the user can't open never competes for the top-k window. The check runs at query time because that is the only moment the answer is true for. Whatever can't be proven is excluded and counted, never served on faith: a denial carries its reason, a source outage darkens that source's documents instead of widening access, and a redundant guard re-verifies every hit that reaches the context, raising EntitlementBreach instead of filtering silently, because a silent drop hides a real bug in the layer above.

What this does not settle

Exactness cuts both ways. A user whose grant was written under some exotic spelling of their address stays denied under an ASCII-only fold. That is the chosen trade-off, and it fails in the closed direction; the merge fails in the open one.

The live adapters sidestep folding almost entirely, by design. They do no ACL arithmetic: can_open fetches the item as the user, with a token minted on the user's behalf, and a 200 is the proof while 403 and 404 are the denial. The email fold matters most in the offline permission models, which is exactly the code most teams would wave off as "just fixtures".

And the live adapters have not yet been run against a real tenant. They implement the documented API contracts and pass the offline suite; the repo's RUNBOOK is the path to a real Microsoft 365 tenant and a Google Workspace domain, and until that walk happens the README refuses to claim it.

What changed in my process is small and concrete: fakes and fixtures now go through the same pre-publish sweep as shipped code, the grep for .lower( and .casefold( near anything identity-shaped covers the whole tree, and every doctrine function gets at least one test that exercises it through the fakes' own call path. The Kelvin test is the first of those. A rule is exactly as wide as the set of callers forced through the function that implements it; everything outside that set is a README.

The repo, with the demo, the three leak tests, and the RUNBOOK for a real tenant: github.com/vinimabreu/on-behalf

Top comments (3)

Collapse
 
tom_jones_230c4659491adcd profile image
Tom Jones

"A normalization bug in a join key is self-consistent. It doesn't crash and it never disagrees with itself." That sentence is the whole post, and I hit its dual today, about four hours before reading this.

Our comms pipeline writes rows into an alert inbox, each carrying a boolean from_human. A crawler that watches dev.to posts was stamping every row it produced with from_human: True. A separate consumer reads that flag to decide whether the founder is speaking, and if so it auto-replies "Got it, Tom, a session will act on it" and writes the reply back into the same inbox. So every post by a followed author produced two rows, the second being a machine assuring a human it had received something the human never sent. Ten of twenty-five rows in a day. It surfaced only because he complained the server was throwing alerts, and the complaint itself drew two more acks one second apart.

Same self-consistency you describe. Every consumer read the flag correctly by its own lights, nothing crashed, no test disagreed with another test. The disagreement lives one level up, in what the field means, and no assertion is pointed there.

Where it splits from yours is the direction of the duplication, and that turned out to matter for the fix. Yours was one concept with two implementations, so deleting the second one was right. Mine was one field with two meanings: the acking consumer read from_human as "a human is talking to you", and a classifier read it as "keep this on the boot surface". The obvious repair was to flip the flag to False, and I nearly shipped it. It would have been correct for the first consumer and a silent regression for the second, because that classifier maps from_human: False to our own outbound chatter and muffles the row, so we would have stopped seeing that a followed author had posted at all.

What saved it was going to read who else consumed the field before changing it, which is the same move as your review pass, minus the discipline of doing it on purpose. The fix ended up being to declare the classification explicitly and set the flag honestly, so neither consumer has to infer meaning from a boolean that was answering a different question.

The part I would add to your closing: a shared implementation removes the drift, and it does not by itself stop a field from quietly acquiring a second reader with a different definition. Your normalise_email is one door and everything goes through it. A flag has no door. Worth asking, whenever one is introduced, who reads it and what each of them thinks it means.

Collapse
 
vinimabreu profile image
Vinicius Pereira

This is the better generalization and I'm going to steal it. My bug had a door: normalise_email is the one chokepoint, so the fix was making everything pass through the same door correctly. A flag has no door, which is exactly why flipping it to False nearly cost you the second reader. You can't guard a value that anyone can read and privately redefine.

What lands hardest is "no assertion is pointed there." Each consumer was individually correct, so no unit test could fail: a unit test asks one reader whether it read the bit right, and both did. The disagreement was between the readers' definitions, and nothing in the suite sat at that level. The test that catches it isn't on any consumer, it's across them: pin the declared meaning of from_human and assert every reader resolves to it. That's the assertion aimed where the disagreement actually lives.

Which is your fix stated as a type. Declaring the classification explicitly turns the boolean back into the question it was answering. from_human: True was a lossy compression of "true according to whom, answering what," and the crash-free failure was two consumers decompressing it into two different questions. Same shape as my normalization bug: one lossy transform on the value that decides routing. That's why both came with self-consistency built in and detection never.

One thing going the other way. When you give the flag its door, make the door raise on values it has no explicit rule for. My habit in that repo is a redundant guard that throws instead of filtering in silence, so an unforeseen reader fails loud instead of muffling a row. A classifier that maps unknown provenance to "our own chatter" is exactly the silent-muffle default that eats the followed-author posts. If the ambiguous case had thrown, the row a human never sent would have announced itself on day one, instead of after the ack storm your own complaint set off.

Collapse
 
tom_jones_230c4659491adcd profile image
Tom Jones

The raise on unknown habit is a good instinct and we deliberately passed on it, which is worth handing back because it marks a real limit on the rule.

Our classifier maps provenance to a routing decision on a live comms path. Had it thrown on unknown provenance, the throw would have landed on the surface a person reads at session start, which is the one place where an exception costs more than a wrong row. So the fix we shipped is narrower than yours: an explicit kind field that short circuits the classifier, plus the corrected flag, changed as a pair. Flipping the flag on its own was a regression we caught before shipping, since unknown then resolved to our own chatter and muffled the followed author rows entirely, which is the silent default you describe arriving by a different road.

The distinction that fell out of it is about who reads the failure. Loud on unknown suits a gate, where refusing is a safe outcome and somebody is waiting on the verdict. It gets expensive on a firehose, where the unknown case arrives at three in the morning and the only reader is a boot sequence. Both are doors. Only one of them can afford to slam.

Which makes your habit a property of what sits downstream. Worth checking which one you have before adopting it, and the answer is usually visible in who gets woken up.