Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: luvs01/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| }; | ||
| mac.update( | ||
| format!( | ||
| "opencodex-local-management-v1\n{challenge}\n{}\n{}", | ||
| identity.pid, identity.port |
There was a problem hiding this comment.
🔴 Authenticated desktop requests always fail against the real proxy
The verifier decodes the Base64URL secret, but the server signs with the secret’s UTF-8 bytes. Every genuine proof fails, disabling authenticated tray, widget, and stop requests.
Learn more
The server passes the Base64URL text directly to Node’s createHmac, so that text’s UTF-8 bytes are the HMAC key. Rust currently decodes the text into the underlying 32 random bytes, producing a different key and digest. The added unit vector matches Rust’s decoded-key behavior, not the server protocol in createLocalAttestationProof.
Example: For secret BwcH...Bwc, challenge AAAA...AAA, PID 4242, and port 10100, the server produces Yr9EKHjeAFfsFMsF8Xsd7J6LxBYnObweKZlLyTMk0Lo. This verifier and its test expect T2FWKlQv-CS_ygbwmxZ5QRJtpqmM7J8i4IQ_LEaW1vg, so desktop authentication rejects the running proxy.
Recommended fix: Initialize Hmac<Sha256> with identity.attestation_secret.as_bytes() without Base64URL-decoding the secret. Update the test vector to one generated by the TypeScript server primitive, while continuing to Base64URL-decode the returned proof before constant-time verification.
Was this helpful? React with 👍 or 👎 to provide feedback.
| self.authenticate_target().await?; | ||
| let token = self.auth.token().ok_or(ProxyError::Unauthorized)?; | ||
| let response = self.send(&method, path, Some(token)).await?; |
There was a problem hiding this comment.
🔍 Explicit security review is required before merge
This PR changes admin-token handling and an authentication boundary. Repository policy requires explicit security review and maintainer sponsorship before merge.
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f91d022243
ℹ️ 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".
| let Ok(secret) = URL_SAFE_NO_PAD.decode(&identity.attestation_secret) else { | ||
| return false; | ||
| }; | ||
| let Ok(mut mac) = Hmac::<Sha256>::new_from_slice(&secret) else { |
There was a problem hiding this comment.
For every authenticated desktop request, this decodes the 43-character Base64URL secret before constructing the HMAC, but the server's createLocalAttestationProof calls createHmac("sha256", secret) and therefore uses the encoded string's UTF-8 bytes directly. The resulting proofs never match, so /api/* retries fail with Unauthorized and the tray, widget, and Stop Proxy action lose management access; the unit test currently hard-codes a proof from the incompatible decoded-key algorithm. Construct the HMAC from identity.attestation_secret.as_bytes() (or change both protocol implementations together).
AGENTS.md reference: AGENTS.md:L420-L426
Useful? React with 👍 / 👎.
1 participant
Motivation
Description
Auth::runtime_identity()that loads the protectedruntime-port.json(pid/port/attestation secret) and validate its shape before use in the desktop client (desktop/src-tauri/src/auth.rs).401with the admin token, callauthenticate_target()to issue anx-opencodex-attestation-challengeto/healthzand verify the returned HMAC proof,service,pid,port, and that the runtime record remains unchanged (desktop/src-tauri/src/proxy.rs).hmac+sha2and Base64URL decoding to match the server attestation primitive and map request errors cleanly (desktop/src-tauri/src/proxy.rs).desktop/src-tauri/Cargo.toml(base64,hmac,sha2).Testing
bun run typecheck, which passed.cargo fmt --manifest-path desktop/src-tauri/Cargo.toml -- --checkandgit diff --check, which passed, and committed the changes.bun run privacy:scan, which passed.cargo check --manifest-path desktop/src-tauri/Cargo.toml, which was blocked by the environment missingglib-2.0.pcrequired by Tauri on this CI container (external system dependency), so desktopcargo checkcould not complete here.bun run testuntil environment-driven server-test expectations diverged (existing unrelated failures where some fixtures returned 403 vs expected 401); many test cases executed and passed earlier in the run but the full suite was not completed in this environment.Codex Task