Catch a credential when it syncs, not only when you share it - #162
Open
ssowonny wants to merge 4 commits into
Open
Catch a credential when it syncs, not only when you share it#162ssowonny wants to merge 4 commits into
ssowonny wants to merge 4 commits into
Conversation
…/secrets The rules only ever ran on the rarest path a file takes. Moving them out of internal/webapp is what lets internal/syncer run the same six rules on the path every file takes, without inverting the dependency. Pure move plus one addition: Label(), the six human strings that until now lived only in the frontend's SECRET_LABELS — so 'bdrive share' stops printing a bare rule id where the web dialog says 'an AWS access key'. Rule ids and the rule/line JSON tags are unchanged: Browser.tsx keys off them, so they are a wire contract.
The six share-time rules now run on the path every file takes. A file with an AWS key in it used to ride a normal sync to the hub, to every teammate's disk and into every future agent's context with no badge and no warning — while the Share dialog one click later blocked that exact file. Warn, never block: the op is journaled and pushed exactly as before. A hold arm would mean a false positive silently parks someone's changes, and it would break the cycle's degrade-to-offline posture. - scan() reads the blob PutBlobFile just wrote (the bytes that were actually journaled), only on the branches that wrote one — an unchanged file is still never re-read. - Findings persist per path in secrets-<mount>.json, merged rather than replaced: nearly every cycle scans zero files, and a whole-set rewrite would erase the warning seconds after it appeared. Fixing the file clears it. - bdrive status grows a secrets block; the agent hook appends one advisory sentence. Rule ids and line numbers only, never the matched bytes. - SaveSecrets failing logs and continues: advisory telemetry never gets a veto over convergence.
README, the CLI reference and project-files get the new bdrive status block and the warn-never-block posture, with the three limits stated (checked when it changes, first 1 MiB, writing device only). Diagrams: internal/secrets is a package of its own in the overview, secretLog joins the sync engine, and the share-gate class notes that it no longer owns the rules.
The check must ride the branch that already reads the file. Clearing the record by hand and cycling proves it: a scan that re-read unchanged files would put the finding back, and the daemon's 3-second tick would pay for it on every file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
bdrive statusand as one sentence in the agent's next turn. Fixing the file clears it — no command, no flag.bdrive share— both below.Closes BEA-145.
The whole change in one picture
The rules didn't change. Where they run did.
flowchart TB rules["<b>internal/secrets</b><br/>six anchored rules, stdlib only<br/>(was internal/webapp/secrets.go)"] share["<b>bdrive share</b> — the rare path<br/>reads the first 1 MiB at mint time<br/>names rule + line<br/><b>REFUSES to mint</b> (--force overrides)<br/><i>unchanged</i>"] sync["<b>every sync</b> — the path every file takes<br/>reads the blob it just wrote<br/>names rule + line<br/><b>WARNS, journals and pushes anyway</b><br/><i>new</i>"] rules --> share rules --> sync classDef added fill:#22c55e22,stroke:#22c55e,stroke-width:2px class sync addedWhy warn and never hold
This is the spec's condition, not a phase-1 compromise. A hold arm means not
journaling the op, which parks that file until the user notices a surface they
may never read — and it breaks the cycle's errors degrade to offline rather*
*than failing posture. Warn keeps the invariant; hold breaks it. A false
positive costs one line of text instead of somebody's changes.
So: nothing is blocked, held, paused, or un-pushed anywhere in this PR. The
existing share-time gate is untouched and stays OSS.
What it looks like
And at the start of an agent's next turn, appended to the hook's
additionalContextalongside the peer-changes sentence:Rule ids and line numbers only. The matched bytes never reach the record, the
terminal, the agent's context, or a log line — asserted in three tests.
How it's built
1 —
internal/webapp/secrets.go→internal/secrets(first commit, a puremove worth reviewing on its own). Stdlib-only, so
internal/syncercan importit without inverting the dependency.
secretFinding→secrets.Findingwiththe
rule/lineJSON tags unchanged —Browser.tsx'sSECRET_LABELSkeysoff the six rule ids, so they are a wire contract. One addition:
Label(rule),the six human strings that until now lived only in the frontend.
2 — the scan reads the blob, not the file again. On the branches that just
called
PutBlobFile, so the cheap size+mtime path never re-reads anything. Theblob is the exact bytes that were hashed and journaled, so a line number can
never describe content no op captured — and it costs the same page-cache read
the plan budgeted for.
3 — findings persist per path, in
secrets-<mount-id>.jsonin the volumedir (0600, atomic). Not a
Resultfield: the daemon has usually scanned theagent's write seconds before
bdrive sync --hookruns its own cycle, so aResultwould report nothing — the same lessoninternal/store/inbound.godocuments. Written in
finishonly when the cycle changed it, so a quiet daemontick still writes nothing, and a write error is logged, never returned:
advisory telemetry gets no veto over convergence.
The one deliberate deviation from the plan
The spec says whole-file replace per cycle. That erases itself. Nearly every
cycle scans zero changed files, so the set goes empty ~3s after the warning
appears — while the same spec's acceptance criteria require the finding to stand
until the credential is removed. The plan caught this and this PR builds the
per-path merge instead;
TestSecretClearsWhenFixedcycles twice specifically tocatch a regression back to whole-replace.
One thing the plan didn't call:
bdrive sharenow prints both the label and****the rule id (
an AWS access key (aws_access_key_id)) rather than swapping onefor the other. The web dialog has always said the words; the id is what someone
greps for, and keeping it means the existing e2e assertion still holds.
What was run
go build ./...,go vet ./...go test ./...npm run e2e(Playwright, seeded hub)frontend/check-dist.shinternal/webapp/staticis freshNew tests: three multi-device syncer tests (converges + is flagged; the warning
survives a quiet cycle and dies with the key; an unchanged file is never
re-read), a store test for foreign/torn keys and the 0600 mode, a CLI e2e test
driving the real binary end to end, three hook tests including the empty case*
*is byte-identical to today — that one is paid on every turn of every session.
Known gaps, all deliberate
materializeupdates the cache, so a pulled file isn't scanned either. Backfill is explicitly out of scope. Worth knowing before someone files it as a bug.shares_test.goasserts that boundary.Architecture changes
Three diagrams changed.
internal/secretsis a new package, sooverview.mdmoves too.
overview.md— new nodeinternal/secrets, with edges from bothinternal/syncer(scan: warn) andinternal/webapp(share mint: refuse).cli-sync.md— new classsecretLog(the cycle's view of the findings:scanned in
scan, persisted infinish), andStoregainsLoadSecrets/SaveSecrets.webapp-server.md—secretScanis no longer a webapp file: stereotype<<secrets.go>>→<<internal/secrets>>,secretScanLimit→ScanLimit,scanSecrets→Scan, plusLabel. Mint-time behavior is untouched.architecture/overview.md✅ added · ❌ removed (strikethrough) · unmarked = unchanged
flowchart LR eng["internal/syncer Session.Cycle<br/>internal/journal ops + replay"] srv["internal/webapp Server<br/>auth, orgs, projects, shares,<br/>history, read heat, store proxy"] sec["<div style='text-align:left'><b>internal/secrets</b><br/>six credential rules, stdlib only<br/>shared by the share gate and the sync scan</div>"] eng -- "<span style='background:#22c55e55;padding:0 5px;border-radius:3px'>✅ scan: warn, never hold</span>" --> sec srv -- "<span style='background:#22c55e55;padding:0 5px;border-radius:3px'>✅ share mint: refuse</span>" --> sec eng <-->|"https:// backend, device token"| srv classDef added fill:#22c55e22,stroke:#22c55e,stroke-width:2px class sec added linkStyle 0 stroke:#22c55e,stroke-width:2px linkStyle 1 stroke:#22c55e,stroke-width:2pxarchitecture/cli-sync.md✅ added · ❌ removed (strikethrough) · unmarked = unchanged
flowchart TB Session["<div style='text-align:left'><b>Session</b><br/>+Folder, +MountID, +Store<br/>+Device, +Account, +Backend<br/>+Cycle(ctx) Result</div>"] Store["<div style='text-align:left'><b>Store</b><br/>+PutBlob / OpenBlob / HasBlob<br/>+AppendOps / DeviceOps / AllOps<br/>+LoadCache / SaveCache mountID<br/>+LoadSync / SaveSync<br/>+LogInbound / DrainInbound<br/><span style='background:#22c55e55;padding:0 4px;border-radius:3px'>✅ +LoadSecrets / SaveSecrets mountID</span><br/>+Lock() flock</div>"] secretLog["<div style='text-align:left'><b>secretLog</b> <i>syncer/secrets.go</i><br/>found map path to Findings<br/>dirty bool<br/>+scanBlob(store, rel, sum)<br/>+set / drop per path</div>"] Note["Warn only — the op is journaled and pushed as before.<br/>Merged PER PATH into secrets-mount-id.json,<br/>written in finish only when dirty.<br/>A save error is logged, never returned."] Session --> Store Session -- "<span style='background:#22c55e55;padding:0 5px;border-radius:3px'>✅ scan flags, finish persists</span>" --> secretLog secretLog -- "<span style='background:#22c55e55;padding:0 5px;border-radius:3px'>✅ reads the blob, writes the record</span>" --> Store secretLog -.- Note classDef added fill:#22c55e22,stroke:#22c55e,stroke-width:2px classDef noteBox fill:#88888822,stroke:#888888,stroke-dasharray:2 2 class secretLog added class Note noteBox linkStyle 1 stroke:#22c55e,stroke-width:2px linkStyle 2 stroke:#22c55e,stroke-width:2pxarchitecture/webapp-server.md✅ added · ❌ removed (strikethrough) · unmarked = unchanged
flowchart TB secretScan["<div style='text-align:left'><b>secretScan</b><br/><span style='background:#ef444455;padding:0 4px;border-radius:3px'>❌ <s><<secrets.go>></s></span><br/><span style='background:#22c55e55;padding:0 4px;border-radius:3px'>✅ <<internal/secrets>></span><br/><span style='background:#ef444455;padding:0 4px;border-radius:3px'>❌ <s>secretScanLimit = 1 MiB</s></span><br/><span style='background:#22c55e55;padding:0 4px;border-radius:3px'>✅ +ScanLimit = 1 MiB</span><br/>secretRules six anchored regexes<br/><span style='background:#ef444455;padding:0 4px;border-radius:3px'>❌ <s>+scanSecrets(buf) []secretFinding</s></span><br/><span style='background:#22c55e55;padding:0 4px;border-radius:3px'>✅ +Scan(buf) []Finding</span><br/><span style='background:#22c55e55;padding:0 4px;border-radius:3px'>✅ +Label(rule) human words</span></div>"] secretFinding["<div style='text-align:left'><b>secretFinding</b><br/><span style='background:#22c55e55;padding:0 4px;border-radius:3px'>✅ <<secrets.Finding>></span><br/>+Rule string<br/>+Line int</div>"] Note["Rule ids and the rule/line JSON tags are unchanged:<br/>Browser.tsx's SECRET_LABELS keys off them,<br/>so they are a wire contract.<br/>Mint-time behavior is untouched — still refuses,<br/>still fails closed, still never echoes the match."] secretScan --> secretFinding secretScan -.- Note classDef noteBox fill:#88888822,stroke:#888888,stroke-dasharray:2 2 class Note noteBoxBuild session
(only works on the machine this ran on)