feat: add private address inboxes and Zig CLI#2
Conversation
|
@EmmanuelKeifala is attempting to deploy a commit to the Abdullah Mustapha's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe PR adds recipient-keyed encrypted drops across the Go API, SQLite store, Zig CLI, and web app. It introduces inbox proofs and listing, secure local identities, bounded API clients, safe output handling, recipient-aware reveal flows, and related build, test, documentation, and deployment configuration. ChangesRecipient-Keyed Drop System
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Sender
participant CLI
participant API
participant Store
participant Recipient
Sender->>CLI: encrypt plaintext for recipient
CLI->>API: create recipient-keyed drop
API->>Store: persist encrypted payload and key metadata
Recipient->>API: request inbox key and submit proof
API->>Store: list recipient-keyed drops
Store-->>API: return inbox metadata
API-->>Recipient: return drop identifiers and view counts
Recipient->>API: request metadata and open payload
API->>Store: consume eligible view
Store-->>API: return encrypted payload
API-->>Recipient: return ciphertext, IV, and ephemeral key
Recipient->>CLI: decrypt locally
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ashdrop/api/main.go`:
- Around line 106-111: Update the recipient-key validation around validPublicKey
so recipientPub and ephemeralPub must either both be present or both absent.
When supplied, validate both keys as valid on-curve P-256 points, not merely
correctly shaped keys, before persisting them. Preserve the existing bad-request
response for invalid key input and ensure Store.Fetch cannot expose an unmatched
ephemeral key.
In `@ashdrop/cli/src/config.zig`:
- Around line 5-9: Restore the production HTTPS endpoint defaults in the
managed_api and managed_web configuration constants, or ensure the release build
injects those deployed values instead of localhost URLs. Update corresponding
tests to assert the managed endpoints while preserving override behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8654c6f2-7fe6-416c-8c73-af1b1bc379e9
📒 Files selected for processing (23)
.gitignoreashdrop/api/main.goashdrop/api/store.goashdrop/cli/README.mdashdrop/cli/build.zigashdrop/cli/build.zig.zonashdrop/cli/src/api.zigashdrop/cli/src/commands.zigashdrop/cli/src/config.zigashdrop/cli/src/crypto.zigashdrop/cli/src/crypto_test.zigashdrop/cli/src/files.zigashdrop/cli/src/identity.zigashdrop/cli/src/links.zigashdrop/cli/src/main.zigashdrop/cli/src/test_server.zigashdrop/cli/test.zigashdrop/cli/testdata/generate-protocol-v1.mjsashdrop/cli/testdata/protocol-v1.jsonashdrop/web/src/lib/api.tsashdrop/web/src/routes/drop-for/[pubkey]/+page.svelteashdrop/web/src/routes/s/[id]/+page.svelterender.yaml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ashdrop/cli/src/config.zig (1)
26-33: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReplace
std.Io.net.HostName.fromUrihere. The pinned Zig snapshot doesn’t expose that API;uri.getHost/HostName.initis the available path, so this won’t compile as written.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ashdrop/cli/src/config.zig` around lines 26 - 33, Replace the unavailable std.Io.net.HostName.fromUri call in the endpoint parsing flow with the supported uri.getHost and std.Io.net.HostName.init path. Preserve the existing successful hostname return and InvalidEndpoint handling for missing or invalid hosts and IP literals.
🧹 Nitpick comments (1)
ashdrop/cli/src/config.zig (1)
38-48: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover embedded endpoint precedence.
Add an assertion that an embedded endpoint wins over the environment value; the current test never exercises that branch.
Proposed test
try std.testing.expectEqualStrings( "https://env.example", try resolveApi(null, "https://env.example", null), ); + try std.testing.expectEqualStrings( + "https://embedded.example", + try resolveApi(null, "https://env.example", "https://embedded.example"), + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ashdrop/cli/src/config.zig` around lines 38 - 48, Add a case to the "ordinary API references prefer explicit flag then environment" test that calls resolveApi with no explicit flag, a populated environment value, and the embedded endpoint, asserting the embedded endpoint is returned. Keep the existing precedence assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ashdrop/cli/src/config.zig`:
- Around line 26-33: Replace the unavailable std.Io.net.HostName.fromUri call in
the endpoint parsing flow with the supported uri.getHost and
std.Io.net.HostName.init path. Preserve the existing successful hostname return
and InvalidEndpoint handling for missing or invalid hosts and IP literals.
---
Nitpick comments:
In `@ashdrop/cli/src/config.zig`:
- Around line 38-48: Add a case to the "ordinary API references prefer explicit
flag then environment" test that calls resolveApi with no explicit flag, a
populated environment value, and the embedded endpoint, asserting the embedded
endpoint is returned. Keep the existing precedence assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 414702ae-6d21-489b-ba3b-6048407b7afa
📒 Files selected for processing (2)
ashdrop/api/main.goashdrop/cli/src/config.zig
🚧 Files skipped from review as they are similar to previous changes (1)
- ashdrop/api/main.go
abdullah4tech
left a comment
abdullah4tech
left a comment
There was a problem hiding this comment.
Thanks for this — really solid work. I pulled the branch and verified everything locally before reviewing.
Local verification
| Check | Result |
|---|---|
go vet / go test ./... |
clean / no test files exist (see below) |
zig build test + zig build -Doptimize=ReleaseSafe |
pass |
pnpm check / pnpm build |
0 errors / pass |
E2E: address create → share → inbox → pull |
works; pulled file byte-identical; identity dir 700, key file & output 600 |
| One-view burn | second pull rejected, inbox empties |
Race: 20 concurrent POST /open on a 1-view drop |
exactly 1 response got the ciphertext |
| Wrong-recipient pull | rejected without consuming the view; owner could still pull afterward |
| Bogus inbox proof / legacy GET on recipient drop | 401 / 404 as designed |
What I liked
This is unusually careful: canonical base64 validation everywhere (blocks alternate encodings of the same key), constant-time proof comparison, key material zeroed after use, symlink-refusing path resolution for identity and output files, atomic writes that refuse late-created destinations, a bounded no-redirect HTTP client that never echoes server error text, and the fixture test pinning the Zig crypto to Node Web Crypto output. The web recipient flow correctly checks metadata before spending a view, and legacy drops remain readable.
Blocking
.gitignoreaddsdocs/*— this looks unrelated to the PR and would silently hide any futuredocs/directory from git. Please drop it.- No Go test coverage. The API package has no test files, so the
go test ./...checkbox is vacuous. All the new server logic (atomicOpen, HMAC proof verification, rate limiter, inbox key persistence) is untested. More importantly, the open-race safety I verified depends entirely on the pre-existingdb.SetMaxOpenConns(1)—Open()'s SELECT-then-UPDATE inside a deferred transaction is not intrinsically race-safe. If the store ever moves to a pooled DB (the comments mention Redis/Postgres later), one-view drops become double-readable. At minimum a regression test for concurrent one-view opens, and ideally a guard likeUPDATE ... SET views = views + 1 WHERE id = ? AND views < max_viewsso correctness doesn't hinge on pool size.
Minor / non-blocking
- Proof replay window: inbox proofs are bound to path+limit+timestamp but carry no nonce, so a captured proof replays for ±5 min. HTTPS-only enforcement keeps this low severity — just noting it.
- Rate limiter fail-closed: once the 4096-window map fills, new IPs are denied, so an attacker with many IPs can briefly lock legitimate new clients out of creating drops. Each request also sweeps the whole map under the mutex — fine at this scale, worth revisiting if traffic grows.
/metadatarevealsrecipientPubto anyone holding the drop ID. The web flow needs it, but any link intermediary learns which receive address a drop targets. Possible future hardening: client submits its pubkey and gets a boolean match back.- Zig nit:
inboxProof's 160-byterequest_bufferhas a worst-case canonical string of ~163 bytes (limit=100plus a 19-digitat), and overflow hitscatch unreachable. Unreachable with real clock values, but sizing the buffer to 192 makes it safe by construction rather than by accident. - Cached old web clients will show "gone" for new recipient-keyed drops (legacy GET now 404s them) until users reload the SPA — transient, acceptable.
One housekeeping note: this PR supersedes #1 (same base features plus the inbox work), so I'll close #1 when this lands.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@abdullah4tech this ".gitignore adds docs/* — this looks unrelated to the PR and would silently hide any future docs/ directory from git. Please drop it." should be non-blocking, as docs is my local store where i propose and keep implementation plans, and that is totally not need in this remote origin. what i will do is remove entirely from the ignore file |
|
addressed the blocking items removed the docs ignore added go coverage for every cli api route and its security and concurrency edges and changed open to claim views atomically with update returning so pooled concurrent requests yield exactly one final payload also increased the zig inbox proof buffer and added full width timestamp coverage |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ashdrop/api/store.go (1)
71-86: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRun every column migration before creating the recipient index, and migrate
opened_at.On an existing database, executing
schemafirst attempts to createidx_secrets_recipient_inboxwhilerecipient_pubandcreated_atare still absent, so startup fails before this migration loop runs. The loop also omits the newly introducedopened_at, which subsequently breaks metadata/open queries.Remove the inbox index creation from
schema, add the missing migration, then create the index at Line 86. Add an upgrade test starting from the legacy schema.Proposed migration fix
for _, migration := range []string{ `ALTER TABLE secrets ADD COLUMN pin_protected INTEGER NOT NULL DEFAULT 0`, `ALTER TABLE secrets ADD COLUMN ephemeral_pub TEXT NOT NULL DEFAULT ''`, `ALTER TABLE secrets ADD COLUMN recipient_pub TEXT NOT NULL DEFAULT ''`, `ALTER TABLE secrets ADD COLUMN created_at INTEGER NOT NULL DEFAULT 0`, + `ALTER TABLE secrets ADD COLUMN opened_at INTEGER`, } {Also remove
CREATE INDEX ... idx_secrets_recipient_inboxfrom the initialschemastring; Line 86 already creates it after migration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ashdrop/api/store.go` around lines 71 - 86, Update the schema initialization and migration flow in the store setup: remove idx_secrets_recipient_inbox creation from the initial schema string, add the opened_at column to the migration loop, and keep the index creation after all migrations complete. Add an upgrade test that initializes from the legacy schema and verifies startup and migrated metadata/open queries succeed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ashdrop/api/store.go`:
- Around line 71-86: Update the schema initialization and migration flow in the
store setup: remove idx_secrets_recipient_inbox creation from the initial schema
string, add the opened_at column to the migration loop, and keep the index
creation after all migrations complete. Add an upgrade test that initializes
from the legacy schema and verifies startup and migrated metadata/open queries
succeed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 42b6af90-3533-4aff-9063-e668b72c8f8e
📒 Files selected for processing (5)
.gitignoreashdrop/api/main_test.goashdrop/api/store.goashdrop/cli/src/crypto.zigashdrop/cli/src/crypto_test.zig
💤 Files with no reviewable changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- ashdrop/cli/src/crypto_test.zig
- ashdrop/cli/src/crypto.zig
|
checked this against the full store history the recipient inbox index is already created only after migrations and opened at has existed in the secrets schema since the first api commit so there is no supported legacy schema missing that column and no migration change is needed |
3 participants
Summary
This adds a native Zig CLI for Ashdrop and extends recipient-keyed drops with private address inboxes. A recipient can create a local receive identity, share encrypted environment files, pull a drop safely, and list pending drops with
ashdrop inboxwithout making inbox activity enumerable from the public receive address alone.How private inboxes work
The API creates and persists a dedicated P-256 inbox keypair in SQLite. The CLI combines its existing receive private key with the API inbox public key through ECDH, derives a separate inbox MAC key with HKDF-SHA-256, and signs a short-lived canonical listing request with HMAC-SHA-256. The API independently derives and verifies the same proof before returning inbox metadata.
The listing exposes only drop IDs, expiry times, and remaining views. It never returns ciphertext, IVs, ephemeral keys, sender identity, notification tokens, recipient private keys, or plaintext. The public receive address by itself is not enough to list pending drops.
Included changes
address,share,pull, andinboxcommands.Compatibility and migration
Existing legacy drops remain readable through the original fetch-and-burn flow. New recipient-keyed drops use metadata plus atomic open. Existing databases are migrated in place with default values for the new recipient and inbox metadata columns.
Verification
go test ./...pnpm run checkpnpm run buildzig build testzig build -Doptimize=ReleaseSafegit diff --checkAutomated feature tests are contained in the Zig CLI test suite; the Go API and web changes are verified through compilation, static checks, production build, and end-to-end CLI/API flows.
Summary by CodeRabbit
New Features
Security & Reliability
Documentation / Chores