Skip to content

[rust][contacts] add shared contacts client and bridge#9957

Merged
ua741 merged 20 commits intoente-io:mainfrom
neeraj-pilot:contacts_2_rust
Apr 7, 2026
Merged

[rust][contacts] add shared contacts client and bridge#9957
ua741 merged 20 commits intoente-io:mainfrom
neeraj-pilot:contacts_2_rust

Conversation

@neeraj-pilot
Copy link
Copy Markdown
Contributor

Summary

This PR adds the shared Rust contacts client and the supporting Rust core HTTP changes needed to use it from downstream mobile and web integrations.

What changed

  • add the new rust/contacts crate for contact sync, root-key handling, create/update/delete flows, and profile-picture attachment operations
  • add generic attachment operations in the Rust contacts client while keeping profile-picture convenience wrappers
  • add a bare object-store HTTP client in rust/core for presigned upload/download flows without Ente auth headers
  • tighten and clean up Rust core HTTP construction/error handling used by the new contacts client
  • fix blob JSON error mapping in rust/core so the Rust/WASM stack compiles cleanly

Why

The contacts backend from PR1 needs a reusable Rust client layer before mobile and web integrations can build on top of it. This PR creates that shared domain/client surface and the supporting HTTP plumbing.

Validation

  • cargo test --manifest-path rust/contacts/Cargo.toml
  • cargo clippy --manifest-path rust/contacts/Cargo.toml --all-targets -- -D warnings
  • cargo check --manifest-path rust/core/Cargo.toml

@ua741 ua741 marked this pull request as ready for review April 7, 2026 06:21
@ua741
Copy link
Copy Markdown
Member

ua741 commented Apr 7, 2026

@codex Review these changes

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 58c52c5088

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@neeraj-pilot
Copy link
Copy Markdown
Contributor Author

Follow-up on the Policy::none() change in rust/core/src/http.rs:

I did a repo scan of Rust call sites after the review comment. Today, the redirect-sensitive paths are the raw-byte helpers (HttpClient::get_bytes / put_bytes and ObjectStoreHttpClient), and those paths already implement a manual redirect loop because they need to change header policy on cross-origin hops.

The problem with the previous code was that reqwest's default auto-redirects could bypass that manual policy entirely, so a cross-origin hop could happen before our code had a chance to drop Ente auth headers.

I also checked current Rust usage of ente_core::http::HttpClient: the normal JSON/empty API calls (get_json, post_json, post_empty, etc.) are used against Ente API endpoints and should not rely on redirect-following in normal operation. So the practical blast radius of Policy::none() is limited to cases where an API endpoint unexpectedly returns a 30x, which we would now surface instead of silently following.

I considered narrowing this to a second redirectless client only for the byte-transfer paths. That would work, but it adds extra client plumbing/state for little practical benefit given current usage. I chose the global Policy::none() change because it is smaller, it directly matches the existing intent of the manual redirect code, and it avoids having two subtly different reqwest client behaviors inside the same wrapper.

I also added a regression test proving that a cross-origin redirect on get_bytes drops X-Auth-Token while preserving the public headers.

@ua741
Copy link
Copy Markdown
Member

ua741 commented Apr 7, 2026

@codex Review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b2aa4df3f2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@neeraj-pilot
Copy link
Copy Markdown
Contributor Author

Quick follow-up on the latest two comments:

  1. I narrowed the redirect change.

Instead of disabling redirects on the shared authenticated client, Policy::none() is now only used for the raw byte-transfer client paths (get_bytes / put_bytes and the object-store client), which are the paths that already implement a manual redirect loop to control per-hop headers. Normal API JSON calls keep reqwest's default redirect behavior.

I also added a regression test that get_json still follows redirects, alongside the existing cross-origin get_bytes auth-header test.

  1. I did not add ensure_root_key_confirmed() to every read path.

The goal here is still to allow local decrypt/use of a cached root key without turning server-backed reads into an extra confirmation round-trip every time. The important correctness fix was the create-race path: if we lose the /user-entity/key creation race, we now fetch and apply the authoritative server key instead of keeping the stale local one.

So after this update:

  • write paths still confirm before write
  • cached-key reads stay on the fast path
  • the race that could poison the cached key has been fixed at the source

If we later want an extra self-heal path for already-bad persisted state, I think the right shape is a one-time retry-on-decrypt-failure in the read path, not unconditional confirmation on every read.

@ua741
Copy link
Copy Markdown
Member

ua741 commented Apr 7, 2026

@codex Review again

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2429dac0ed

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +494 to +498
Err(HttpError::Http { .. }) => {
if let Some(remote_root_key) = fetch_root_key(http).await? {
Ok(Some(remote_root_key))
} else {
Err(HttpError::Http {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Propagate non-conflict create-root errors

The create_root_key helper treats every HTTP failure from POST /user-entity/key as a race and falls back to a fetch, then fabricates a 500 if that fetch returns nothing. That masks real failures (for example, a 400 validation error on the POST) and returns the wrong status/message to callers, which makes retry and diagnostics logic unreliable. Restrict the fallback path to the expected conflict status and otherwise return the original HTTP error.

Useful? React with 👍 / 👎.

Comment on lines +346 to +349
self.object_store_http
.get_bytes(&download.url)
.await
.map_err(Into::into)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Make generic attachment get/set behavior symmetric

set_attachment encrypts attachment bytes before upload, but get_attachment returns raw downloaded bytes without decrypting. Since AttachmentType currently only exposes ProfilePicture, this makes the generic API non-roundtrippable for its only supported type and returns ciphertext that callers cannot decrypt through this public surface. Either decrypt in get_attachment (with required contact context) or clearly split encrypted-vs-decrypted APIs.

Useful? React with 👍 / 👎.

Comment on lines +191 to +193
serde_json::from_slice(&plaintext).map_err(|e| {
CryptoError::InvalidKeyDerivationParams(format!("JSON deserialization failed: {}", e))
})
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 Badge Classify combined JSON decode failures as JSON errors

decrypt_json_combined maps serde deserialization failures to InvalidKeyDerivationParams, unlike the existing JSON helpers that return CryptoError::Json. This mislabels payload/schema issues as KDF problems, so error handling and telemetry get the wrong category and message. Use the JSON error variant here for consistency with the rest of the blob JSON API.

Useful? React with 👍 / 👎.

@socket-security
Copy link
Copy Markdown

socket-security bot commented Apr 7, 2026

No dependency changes detected. Learn more about Socket for GitHub.

👍 No dependency changes detected in pull request

@ua741
Copy link
Copy Markdown
Member

ua741 commented Apr 7, 2026

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@ua741 ua741 enabled auto-merge April 7, 2026 10:12
auto-merge was automatically disabled April 7, 2026 10:26

Head branch was pushed to by a user without write access

mnvr
mnvr previously approved these changes Apr 7, 2026
@ua741 ua741 merged commit 6187ba9 into ente-io:main Apr 7, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants