feat: add recipient-keyed encrypted drops and CLI#1
Open
EmmanuelKeifala wants to merge 7 commits into
Open
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThe change adds recipient-keyed secret support across the Go API, web client, and a new Zig CLI. It includes P-256/ECDH encryption, persistent identities, canonical links, HTTP handling, safe atomic output, command flows, endpoint configuration, tests, fixtures, and documentation. ChangesRecipient-keyed drops
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Sender
participant CLI
participant API
participant Recipient
Sender->>CLI: Read and seal plaintext for recipient
CLI->>API: Create secret with ciphertext, ephemeralPub, and recipientPub
Recipient->>API: Request metadata
API-->>Recipient: Recipient public key and viewsLeft
Recipient->>API: Open recipient-keyed secret
API-->>Recipient: Ciphertext, IV, and ephemeralPub
Recipient->>Recipient: Derive key, decrypt, and write plaintext atomically
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 5
🧹 Nitpick comments (2)
ashdrop/cli/src/test_server.zig (1)
78-85: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRead request bodies up to the API limit
The fixed 8 KiB buffer truncates any body larger than that, while
files.max_api_body_sizeallows up to 96 KiB. Use a bounded allocating or streaming reader capped atfiles.max_api_body_sizeso valid create requests don’t fail JSON parsing.🤖 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/test_server.zig` around lines 78 - 85, Update the request-body reading logic in the test server around request.readerExpectNone and body_buffer so it can read up to files.max_api_body_size (96 KiB) using a bounded allocating or streaming reader instead of the fixed 8192-byte buffer. Preserve the existing TestServerReadFailed handling and JSON validation via std.json.parseFromSlice, ensuring complete valid create-request bodies are parsed.ashdrop/web/src/routes/s/[id]/+page.svelte (1)
54-62: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winLegacy
fetchSecretfallback skips exact recipient-key verification.When
metadata.recipientKeyedis true butmetadata.recipientPubis falsy, control falls through to the pre-metadatafetchSecretpath, which gatesrecipient-sealedvsno-local-keysolely onhasMyKeyPair()— there's norecipientPubonFetchedSecretto check against, so this path can't perform the exact-match verification the metadata path just added. This should be unreachable if the API always populatesrecipientPubfor recipient-keyed drops, but if that invariant is ever violated, it silently bypasses the new recipient-verification guarantee this PR introduces.🤖 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/web/src/routes/s/`[id]/+page.svelte around lines 54 - 62, Update the legacy fetchSecret flow so recipient-keyed secrets are not classified using hasMyKeyPair() alone. Propagate recipientPub through FetchedSecret/fetchSecret and reuse the exact recipient-key verification used by the metadata path; if recipientPub is missing, fail closed instead of entering recipient-sealed.
🤖 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`:
- Line 7: Update validPublicKey and the key-creation/recipient-metadata
verification flows to parse both SEC1 keys as P-256 points before storing or
accepting them. Reject any key that is not a valid on-curve P-256 point,
including ephemeralPub, rather than validating only the canonical 65-byte
uncompressed format.
In `@ashdrop/cli/src/api.zig`:
- Around line 117-129: Add a per-request deadline around the http_client.fetch
call in the request flow, ensuring stalled header or body reads terminate rather
than blocking indefinitely. Use the available Zig snapshot-compatible mechanism,
preserve the existing redirect behavior and error mapping, and ensure the
deadline is applied to every HTTP request.
In `@ashdrop/cli/src/config.zig`:
- Around line 22-36: Update validateEndpoint to reject plain HTTP for
non-loopback hosts while continuing to allow HTTPS and explicit loopback HTTP
addresses (localhost and loopback IPs). Reuse the parsed host/address data to
identify loopback endpoints, and add a validation test covering rejection of
remote http:// endpoints.
In `@ashdrop/cli/src/files.zig`:
- Around line 119-130: Update writeAtomically after the output.atomic_file
link/replace operation to sync the destination’s parent directory before
returning success. Preserve the existing force and OutputExists handling, and
use the atomic file’s parent-directory sync facility.
In `@ashdrop/cli/src/identity.zig`:
- Around line 33-66: Update save to write the encoded identity to a private
temporary file in the target directory, sync and close it, then atomically
install it as filename without replacing an existing identity. Preserve
IdentityAlreadyExists mapping for an existing destination, clean up the
temporary file on failure, and sync dir after installation before create
returns.
---
Nitpick comments:
In `@ashdrop/cli/src/test_server.zig`:
- Around line 78-85: Update the request-body reading logic in the test server
around request.readerExpectNone and body_buffer so it can read up to
files.max_api_body_size (96 KiB) using a bounded allocating or streaming reader
instead of the fixed 8192-byte buffer. Preserve the existing
TestServerReadFailed handling and JSON validation via std.json.parseFromSlice,
ensuring complete valid create-request bodies are parsed.
In `@ashdrop/web/src/routes/s/`[id]/+page.svelte:
- Around line 54-62: Update the legacy fetchSecret flow so recipient-keyed
secrets are not classified using hasMyKeyPair() alone. Propagate recipientPub
through FetchedSecret/fetchSecret and reuse the exact recipient-key verification
used by the metadata path; if recipientPub is missing, fail closed instead of
entering recipient-sealed.
🪄 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: 6e3b4fdd-c482-4a20-94a6-9de022c1d89d
📒 Files selected for processing (22)
.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.svelte
Author
|
Review follow-up fixed in ec96b30:
|
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
1 participant
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.
Summary
Test Plan
go test ./...zig build testpnpm checkprotocol-v1.jsonFollow-up
POST /openrequests.Summary by CodeRabbit