feat(webapp): refuse to share a file that looks like it holds credentials (BEA-111) - #137
Open
ssowonny wants to merge 1 commit into
Open
Conversation
…ials (BEA-111) Minting a share link ran zero content checks: a file holding an AWS-shaped key became a public URL on one click, and the CLI printed nothing but the link. handleShareCreate now reads the first 1 MiB and runs six anchored rules between the synced-path check and Shares.Create, answering 409 with rule ids and line numbers unless the request carries confirm: true. The matched text never leaves scanSecrets — not into the body, not into a log line. TestShareSecretNeverEchoed greps both for the planted string, because a 409 body is the easiest place in this codebase to leak it. Both callers carry the override, since the gate alone would turn any false positive into a hard block with no way out: `bdrive share --force`, and the browser's Share-anyway dialog on modalConfirm (no new component). A path that already has a live link skips the scan — its content is public already, so withholding the URL protects nobody — but alreadyPublic drops links whose creator left the org, since those 404 at /s/ and would otherwise wave a secrets file straight through. A failed blob read is 503, not a silent pass: the repo's "degrade rather than fail" posture is for sync cycles, and a check that skips itself on a storage hiccup is the false confidence this exists to remove. Every user-facing string says the file was checked at the moment you shared it. A link serves the file's LATEST content forever, so a key written into an already-shared file is never caught — that open loop stays open, and the copy is the only thing stopping v1 from claiming otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
ifinhandleShareCreate: first 1 MiB, six anchored rules,409with rule + line number — never the matched string, in the body or the log.bdrive share --force, and Share anyway in the browser.Closes BEA-111.
deploy.md(holds anAKIA…key)Share anyway lands in exactly the dialog you'd have got anyway, and a clean file never sees any of this:
And the CLI:
The one rule that cannot bend
The matched bytes never leave
scanSecrets— not into the 409 body, not into alog line, not into a metric label. It returns rule ids and line numbers and
nothing else, the same rule
reads.goalready keeps for actor identity, and a409 body is the easiest place in this codebase to leak a secret by accident.
TestShareSecretNeverEchoedredirectslog.SetOutputto a buffer and grepsboth the response body and that buffer for the planted string, so the
tempting
log.Printf("found %s")fails the suite rather than shipping.Where the gate sits
Between the synced-path check and
Shares.Create, using theSource.Openthevolume already has. One handler, one choke point, no sync invariant touched —
the route's
proj(PermWrite, …)authority is unchanged, and whoever couldalready mint a public link can confirm one.
It is skipped in exactly two cases:
confirm: truein the request body — what--forceand Share anyway send.alreadyPublic(project, path)— a live, non-expired share for this path whose creator is still in the org. That last clause matters:Listreturns links whose creator has left, those 404 at/s/, and treating a dead link as "already public" would wave a secrets file straight through.alreadyPublicdeliberately does not key offShareDB.Create's reuse branch,which is narrower — it also requires no expiry on either side.
Fails closed
An unreadable blob answers 503 and mints nothing. The repo's "degrade rather
than fail" posture is for sync cycles; minting is a rare interactive action, and
a check that silently skips itself on a storage hiccup is exactly the false
confidence this change exists to remove.
Noted as a deliberate inconsistency, not an accident: the surrounding handler
answers 502 for a failed
snapshot, andhandleSharedanswers 502 for a failedOpen. A failed check is "try again"; a failed read is upstream.The rules
aws_access_key_idAKIA[0-9A-Z]{16}openai_api_keysk-[A-Za-z0-9_-]{20,}github_patghp_[A-Za-z0-9]{36}slack_tokenxox[baprs]-[A-Za-z0-9-]{10,}private_key-----BEGIN [A-Z ]*PRIVATE KEY-----gitlab_patglpat-[A-Za-z0-9_-]{20,}Byte-oriented, not
bufio.Scanner: a 1 MiB minified file with no newline blowsthe scanner's 64 KiB token limit and returns nothing at all — a check that
silently passes everything. Findings are deduped on
(rule, line)and sorted,so a line holding three keys is one finding and the response is stable.
sk-is the false-positive risk; the{20,}body is what keeps it off prose.If it still fires on real docs, tighten the body rather than dropping the rule —
--forceand Share anyway are the escape hatch, which is exactly why theyship in the same PR as the gate.
Copy is part of the feature
Every user-facing string says the file was checked at the moment you share
it, never that the file is clean. A link serves the file's LATEST content
forever, so any present-tense claim is a false promise the next sync can break.
That wording is in the dialog, the CLI refusal,
README.md,reference/cli.md,guides/scoping.mdandguides/agent-artifacts.md— andREADME.md's old"don't share folders that hold secrets" line, the objection BearDrive wrote
about itself, is now the check plus its two limits.
Deviations from the reviewed plan
One, and it's a word: the plan's dialog copy read "BearDrive checked it at the
moment you shared it", but the dialog appears before anything is shared, so
the past tense was wrong on screen. It now reads "The check covers the file at
the moment you share it — a link always serves the file's latest content, so
later changes are never checked." Same constraint, right tense. Everything else
follows the plan as written.
Architecture changes
architecture/webapp-server.md: two new types and one new dependency.secretScan(
secrets.go—scanSecrets,secretScanLimit, the six rules) and itssecretFindingreturn type are new;ServergainsalreadyPublicand depends onsecretScanfromhandleShareCreate;server.gogainswriteJSONStatusbesidewriteJSON(nothing inwebappwrote JSON with a non-200 status before this).ShareDB,Share, and every relationship around them are unchanged.flowchart LR Server["Server<br/>(shares.go)<br/>✅ +alreadyPublic(project, path) bool<br/>✅ +writeJSONStatus(w, code, v) — server.go, beside writeJSON"] Scan["✅ secretScan <<secrets.go>><br/>secretScanLimit = 1 MiB<br/>secretRules six anchored regexes<br/>+scanSecrets(buf) []secretFinding"] Finding["✅ secretFinding<br/>+Rule string<br/>+Line int"] ShareDB["ShareDB<br/>+Create +Get +Revoke +SetExpiry +List"] Share["Share<br/>+Token +Project +Path +Creator +Expires"] Server -.->|"✅ handleShareCreate scans the first 1 MiB<br/>unless confirm:true or alreadyPublic"| Scan Scan -.->|"✅ rule ids and line numbers only"| Finding Server --o ShareDB Server -.->|"✅ alreadyPublic reads List + shareCreatorStillBelongs"| ShareDB ShareDB -.-> Share classDef added stroke:#2da44e,stroke-width:2px class Scan,Finding addedWhat was run
go test ./...go vet ./...npm run e2esec13fe.spec.ts(single-volumeVolumeApp), and both fail identically onorigin/mainwith this branch stashed. Pre-existing, unrelated to this change.check-dist.shinternal/webapp/static is freshNew tests:
secrets_test.go(table-driven, one positive per rule with its linenumber, a multi-key line yielding one finding, a 300 KB no-newline buffer, a
clean file),
TestShareSecretScan(409 +Shares.Listunchanged,confirm: true→ a serving link, already-shared skips, clean file unaffected, a key past1 MiB mints silently),
TestShareSecretNeverEchoed,TestShareSecretScanFailsClosed,TestCLIShareSecretGate(the real binary against a real hub), and abrowse.spec.tscase covering Cancel → zero shares, Share anyway → a workingURL, and a second Share that doesn't ask again.
The loop this leaves open
Nobody has decided what happens when the next sync writes a key into a file
that is already shared.
alreadyPublicmeans a re-share of that file neverscans either. Re-scan-on-write is a separate, larger feature; the "at the moment
you share it" copy is the only thing keeping v1 honest about it in the meantime.
Build session
(only works on this machine)
🤖 Generated with Claude Code