🔧 PUT-1524: Sanity check file sharing api

[jfcastro92]

Sanity check of the sharing API now that #3553 and #3595 are on main, plus a fix for an externally reported authorization issue that landed while this was in review.

Reported vulnerability: plus-addressing decided who a share reached

Reported against b795b219a (CWE-863). + is a convention the receiving domain defines, not a rule of SMTP, but the canonical form stripped it on every domain — and share recipient resolution read identity off that form. On a domain where finance@ and finance+board@ are two different mailboxes, sharing to the second granted the first one's account access to the file.

The invite path was worse, and this is not in the report: no account had to exist. An invite to payroll+contractors@ was keyed on payroll@, so whoever later confirmed payroll@ claimed a file addressed to someone else.

Both reproduce, and both are now regression tests.

Fix, in two layers. Sub-addressing is a capability a provider opts into, and the ones that document it are listed — gmail, icloud, outlook, proton, fastmail, zoho. Yahoo stays listed with no rules, so the finding that it makes + significant isn't rediscovered. Unlisted domains are lowercased and otherwise left alone.

Separately, share resolution no longer trusts that: a match that rewrote the local part is honored only on a domain whose semantics we have asserted. Deliberately redundant, so a future edit to the provider list cannot quietly reintroduce a misroute.

Splitting the two folds broke blocking, which is the part worth keeping. An alias we refuse to grant on can still land in the blocker's inbox, so abuseKey keeps stripping + everywhere and answers that question alone — anti-abuse stays aggressive, identity stays exact. That separation is the reporter's own closing recommendation, and it turned out to be load-bearing rather than optional.

Bugs found by the audit

A recipient's listShared() kept publishing withdrawn shares. #liveGrants paired the flat permission read with the permissions it asked for by array index. That read drops misses and dedupes its keys, so the two are not positional — one entry's live grant vouched for another that had none. The withdrawn item's name, size and a signed thumbnail URL stayed visible to someone who could no longer open it. It needs two shares in one page to reproduce, which is why single-share tests never caught it.

getShares() had the same gap from the owner's side, with no liveness check at all. A grant withdrawn through /auth/revoke-user-user or an ACL mode change left the index row behind, and the owner was told someone could reach a file they could not. Checked against the grants now, one batched read per distinct holder. Pending invites are exempt — they have no grant yet, which is the point of them.

An undelivered broadcast event was dropped. #flushOutboundEvents cleared its queue before sending, so a peer that timed out took its events with it. Most were survivable: a lost cache invalidation heals when the entry's TTL lapses. A revoke's flat-perm invalidation is not — grant-path entries carry no expiry, so a peer went on serving a withdrawn grant indefinitely. Failed sends now go back on the queue for the existing flush timer to retry, bounded at 10,000 with the oldest dropped first, and each retry is signed at send time so it is not rejected against the replay window.

Security

Permission-related, per AGENTS.md.

The reported issue is a genuine authorization defect: a share reached an account that was never addressed. The other two are narrower — the ACL was correct throughout, and what failed was a listing naming access that was not there, plus a revoke that could fail to land in another region. The listing one discloses metadata (name, size, signed thumbnail URL) to someone whose grant was withdrawn; the broadcast one is the serious of the pair, because nothing bounds it.

No wire change: no signature, response field or error code is renamed, added or removed. Client-visible behavior changes are exactly the three fixes — withdrawn shares stop being listed, and a plus-addressed share on an unlisted domain now creates an invite for that exact address instead of granting to a different account.

Coverage added where behavior was already correct

• The access-token actor reaches the same reach bound as an app but through a different arm of ACLService.check. Four tests; no behavior change.
• Cross-region revoke, end to end. A revoke arrives as a replicated SQL delete plus three invalidation events. The test asserts the read still succeeds after the SQL delete alone, so it records why replication is not sufficient by itself.
• Masked paths at route level. A leaked uuid must buy no access: reads one shared file through its masked path, then tries the sibling four ways.
• total vs. what paging yields, pinning the approximation the docs change describes.

Docs

listShared.md said total was "the total count" and the example printed it as one. It counts recorded shares before the filtering the same page already documents, so it can exceed what paging yields — now described as a headline figure.

Notes for reviewers

BroadcastService is well outside sharing. It is here because that is where the revoke-propagation defect lives, and the sharing audit surfaced it — but it deserves eyes from whoever owns that file. The alternative fix (giving grant-path flat entries a TTL) was deliberately not taken: it changes permission semantics, where this repairs delivery for every outer.* event.

There are two email canonicalizers. EmailClient.clean is a second, independent implementation with its own rule tables, and it still folds + globally. That is not a regression — its only consumers are the blocked-domain check (domain-suffix only) and the invite budget hash, both abuse decisions where the aggressive fold is correct, and neither is identity-bearing. But two near-identically-named functions now have opposite semantics with nothing saying which is which. Worth reconciling in a follow-up; not done here because unifying them changes Yahoo budget behavior, which does not belong in a security fix.

Two of #3595's tests asserted the vulnerable behavior and were rewritten. The alias half of shares to the account behind a case or alias variant is gone — that assertion was the vulnerability. The case half remains.

Testing

npm run test:backend green (237 files, 6,132 tests), npm run typecheck clean.

Regression check on the canonicalization change specifically: login is unaffected (it matches the raw column exactly); existing rows whose clean_email was computed under the old rules still resolve, and fail strict rather than permissive, so no backfill is needed; the blocked-domain list matches on the domain and is untouched. The one intended behavior change is write-path dedup — foo+other@unlisted.com can now register alongside foo@unlisted.com, which is why the provider list was expanded to cover the major providers that were relying on the global default.

Every fix is mutation-checked — the guard is broken and the test must fail. That caught two tests of my own that were passing vacuously (share.issuer_user_id and share.fsentry_id both cascade on delete, so the states they set up never existed); both were discarded rather than committed. Separately: the existing drops an invite whose issuer can no longer share it passes via that same cascade rather than the code path it names.

Also audited with no findings: blocking, invite claim, path masking internals, quota and fan-out. Not audited: the GUI.

[github-actions]

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	93.6% 🟰 ±0%	24245 / 25902
🔵	Statements	91.88% ⬇️ -0.02%	26114 / 28419
🔵	Functions	89.81% 🟰 ±0%	4182 / 4656
🔵	Branches	80.46% ⬇️ -0.01%	17608 / 21884

File Coverage

File	Stmts	Branches	Functions	Lines	Uncovered Lines
Changed Files
src/backend/services/broadcast/BroadcastService.ts	90.12% ⬇️ -0.54%	86.87% ⬇️ -0.63%	91.17% ⬆️ +0.27%	91.89% ⬆️ +0.07%	127, 262, 271-278, 328-329, 338, 384, 390-395, 404, 407, 439-441, 448-453, 463, 482, 510-513, 627, 635
src/backend/services/share/ShareService.ts	92.52% ⬇️ -0.15%	80.6% ⬆️ +0.32%	94.87% ⬆️ +0.33%	95.37% ⬆️ +0.03%	40, 77, 88, 100-104, 109, 116-120, 133, 183, 184, 267-269, 301-306, 355, 360, 370, 459, 482, 518, 588, 789, 839, 847-848, 858-859, 874-875, 884, 897-898, 906-910, 967-968, 1000-1002, 1023-1025, 1054, 1188-1189
src/backend/util/email.ts	91.66% ⬇️ -8.34%	72.22% ⬇️ -27.78%	85.71% ⬇️ -14.29%	90% ⬇️ -10.00%	61-62

Generated in workflow #789 for commit a16028d by the Vitest Coverage Report Action

[Salazareo]

looks good, we should test this on prod tho pointing to latest, ill merge then update deps on prod