feat(auth): encrypt Copilot token cache at rest#52
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SecretStore-backed encrypted token storage and secure read/write helpers to the Copilot provider, replaces plaintext token disk I/O with encrypted paths and Zeroizing-wrapped tokens, adds Windows ACL hardening stubs, updates Copilot headers and related tests, removes dashboard PostCSS config and bumps several web deps, plus assorted small formatting/refactors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CopilotProvider
participant SecretStore
participant FileSystem
Note over Client,FileSystem: Secure Token Read Flow
Client->>CopilotProvider: request token
CopilotProvider->>SecretStore: read_token_file_secure(path)
SecretStore->>FileSystem: read encrypted file
FileSystem-->>SecretStore: encrypted bytes
SecretStore->>SecretStore: decrypt(bytes)
SecretStore-->>CopilotProvider: plaintext token
CopilotProvider-->>Client: return token
Note over Client,FileSystem: Secure Token Write Flow
Client->>CopilotProvider: store token
CopilotProvider->>SecretStore: write_token_file_secure(path, token)
SecretStore->>SecretStore: encrypt(token)
SecretStore->>FileSystem: write encrypted bytes
FileSystem-->>SecretStore: write complete
SecretStore-->>CopilotProvider: success
CopilotProvider-->>Client: persisted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
✅ Contributor ReportUser: @yacosta738
Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-02-20 to 2026-02-20 |
Deploying corvus with
|
| Latest commit: |
71f166b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://79691a86.corvus-42x.pages.dev |
| Branch Preview URL: | https://feat-secure-copilot-tokensto.corvus-42x.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
clients/agent-runtime/src/providers/copilot.rs (3)
251-252: Unnecessary extraPathBufclone.
pathandwrite_pathhold identical values;write_pathonly exists to satisfy the move into thespawn_blockingclosure. Passpath.clone()directly to the closure and usepathfor warnings, saving one allocation.♻️ Proposed simplification
- let path = path.to_path_buf(); - let write_path = path.clone(); - let content = content.to_string(); - let secret_store = self.secret_store.clone(); + let path = path.to_path_buf(); + let content = content.to_string(); + let secret_store = self.secret_store.clone(); let result = tokio::task::spawn_blocking(move || -> anyhow::Result<()> { let encrypted = secret_store.encrypt(&content)?; - write_file_secure_blocking(&write_path, &encrypted)?; + write_file_secure_blocking(&path, &encrypted)?; Ok(()) }) .await; match result { Ok(Ok(())) => {} - Ok(Err(err)) => warn!("Failed to write secure Copilot token file {:?}: {err}", path), - Err(err) => warn!("Failed to spawn token write task for {:?}: {err}", path), + Ok(Err(err)) => warn!( ... ), // path moved into closure; capture in Err arms before move if needed + ... }Note: if the warning arms need
pathafter the move, capture it before moving — e.g., storepath.display().to_string()for the error messages.As per coding guidelines: "Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/providers/copilot.rs` around lines 251 - 252, The extra PathBuf clone is unnecessary: remove the write_path variable and pass path.clone() directly into the spawn_blocking closure (use the closure's parameter or move) while continuing to use the original path variable for warnings; if the warning arms need path after it's moved into spawn_blocking, capture path.display().to_string() (or another small precomputed string) before the move and use that in the error/warning messages instead of cloning the whole PathBuf. Ensure references to spawn_blocking, path, and any warning/error branches are updated accordingly.
733-742: Test asserts key presence only — value regressions go undetected.The assertions check that
Editor-VersionandEditor-Plugin-Versionkeys exist, but don't verify their values. If the version strings ("vscode/1.85.1","copilot/1.155.0") are accidentally cleared or changed, the test passes. These are interoperability-critical headers sent to the Copilot API.✅ Tighter assertions
- assert!(headers - .iter() - .any(|(header, _)| *header == "Editor-Version")); - assert!(headers - .iter() - .any(|(header, _)| *header == "Editor-Plugin-Version")); - assert!(headers.iter().any(|(header, _)| *header == "User-Agent")); + assert!(headers + .iter() + .any(|(h, v)| *h == "Editor-Version" && !v.is_empty())); + assert!(headers + .iter() + .any(|(h, v)| *h == "Editor-Plugin-Version" && !v.is_empty())); + assert!(headers + .iter() + .any(|(h, v)| *h == "User-Agent" && !v.is_empty())); + assert!(headers + .iter() + .any(|(h, _)| *h == "Accept"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/providers/copilot.rs` around lines 733 - 742, The test copilot_headers_include_required_fields only checks header keys; update it to also assert the expected values for "Editor-Version", "Editor-Plugin-Version", and "User-Agent" from CopilotProvider::COPILOT_HEADERS — locate each tuple (header, value) by key (e.g., using .iter().find(|(h, _)| *h == "Editor-Version")) and assert_eq! on the value against the canonical strings (e.g., "vscode/1.85.1", "copilot/1.155.0", and the expected User-Agent) so value regressions fail the test.
179-225: Hardencorvus_dirpermissions as defense-in-depth.
SecretStore::new(&corvus_dir, true)passesenabled=trueto control whether encryption is applied—this does not rotate the key or invalidate cached tokens. The second parameter is well-defined.However,
corvus_diris created viacreate_dir_all(&token_dir)without explicit hardening. AlthoughSecretStore::load_or_create_key()protects the.secret_keyfile with0o600permissions on Unix, the parent directory itself remains at umask-governed permissions (typically0o755). Applying0o700tocorvus_diraligns with the principle of least privilege and ensures the entire directory tree is restricted.Suggested hardening
if let Err(err) = std::fs::create_dir_all(&token_dir) { warn!( ... ); } else { #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; + // Harden the key-store parent directory. + if let Err(err) = + std::fs::set_permissions(&corvus_dir, std::fs::Permissions::from_mode(0o700)) + { + warn!("Failed to set corvus config dir permissions on {:?}: {err}", corvus_dir); + } if let Err(err) = std::fs::set_permissions(&token_dir, std::fs::Permissions::from_mode(0o700)) { ... } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/providers/copilot.rs` around lines 179 - 225, The corvus_dir is created indirectly via create_dir_all(&token_dir) but its permissions remain umask-governed; after creation, harden corvus_dir on Unix by calling std::fs::set_permissions(&corvus_dir, std::fs::Permissions::from_mode(0o700)) (similar to the existing token_dir handling) and log a warn on any error; place this in the same cfg(unix) section (or adjacent) alongside the token_dir permission block so SecretStore::new(&corvus_dir, true) and subsequent token operations run with a restricted parent directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/src/providers/copilot.rs`:
- Around line 611-633: In write_file_secure_blocking: remove the redundant
std::fs::set_permissions(...) call in the #[cfg(unix)] branch because
OpenOptions::mode(0o600) already creates the file with the intended bits; and
replace the #[cfg(not(unix))] branch's std::fs::write(path, content) with an
explicit OpenOptions create/write/truncate flow that then applies
platform-appropriate restrictive permissions/ACLs (e.g., use
std::os::windows::fs::OpenOptionsExt + Windows ACL APIs or other OS-specific
APIs) so the ciphertext file is not world-readable; keep the function name
write_file_secure_blocking and the same write-all logic but perform secure
creation and permission setting in a platform-specific way instead of the
current unguarded std::fs::write.
---
Nitpick comments:
In `@clients/agent-runtime/src/providers/copilot.rs`:
- Around line 251-252: The extra PathBuf clone is unnecessary: remove the
write_path variable and pass path.clone() directly into the spawn_blocking
closure (use the closure's parameter or move) while continuing to use the
original path variable for warnings; if the warning arms need path after it's
moved into spawn_blocking, capture path.display().to_string() (or another small
precomputed string) before the move and use that in the error/warning messages
instead of cloning the whole PathBuf. Ensure references to spawn_blocking, path,
and any warning/error branches are updated accordingly.
- Around line 733-742: The test copilot_headers_include_required_fields only
checks header keys; update it to also assert the expected values for
"Editor-Version", "Editor-Plugin-Version", and "User-Agent" from
CopilotProvider::COPILOT_HEADERS — locate each tuple (header, value) by key
(e.g., using .iter().find(|(h, _)| *h == "Editor-Version")) and assert_eq! on
the value against the canonical strings (e.g., "vscode/1.85.1",
"copilot/1.155.0", and the expected User-Agent) so value regressions fail the
test.
- Around line 179-225: The corvus_dir is created indirectly via
create_dir_all(&token_dir) but its permissions remain umask-governed; after
creation, harden corvus_dir on Unix by calling
std::fs::set_permissions(&corvus_dir, std::fs::Permissions::from_mode(0o700))
(similar to the existing token_dir handling) and log a warn on any error; place
this in the same cfg(unix) section (or adjacent) alongside the token_dir
permission block so SecretStore::new(&corvus_dir, true) and subsequent token
operations run with a restricted parent directory.
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 (2)
clients/agent-runtime/src/channels/irc.rs (2)
265-289:⚠️ Potential issue | 🟠 Major
connect()has no application-level timeout —health_checkcan block for 75 s+.Both
TcpStream::connectand the TLS handshake (connector.connect) rely solely on the OS TCP timeout (typically 75–130 s on Linux, much longer on some platforms).health_checkcallsconnect()with no wrapping timeout, so on a firewalled or unresponsive host the call blocks for an indeterminate duration, stalling whatever monitoring loop drives it.
READ_TIMEOUTalready codifies the intent for bounded I/O; the same discipline should apply to the connection phase.🛠️ Proposed fix — wrap `connect()` with a bounded timeout
- async fn connect( + /// Connection timeout for TLS setup; guards health_check and initial connect. + const CONNECT_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(15); + + async fn connect( &self, ) -> anyhow::Result<tokio_rustls::client::TlsStream<tokio::net::TcpStream>> { let addr = format!("{}:{}", self.server, self.port); - let tcp = tokio::net::TcpStream::connect(&addr).await?; + let tcp = tokio::time::timeout(Self::CONNECT_TIMEOUT, tokio::net::TcpStream::connect(&addr)) + .await + .map_err(|_| anyhow::anyhow!("IRC TCP connect timed out after {CONNECT_TIMEOUT:?}"))??; // ... existing TLS config build ... - let tls = connector.connect(domain, tcp).await?; + let tls = tokio::time::timeout(Self::CONNECT_TIMEOUT, connector.connect(domain, tcp)) + .await + .map_err(|_| anyhow::anyhow!("IRC TLS handshake timed out after {CONNECT_TIMEOUT:?}"))??; Ok(tls) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/channels/irc.rs` around lines 265 - 289, The connect() implementation lacks an application-level timeout; wrap both the TCP connect and the TLS handshake in tokio::time::timeout using the existing READ_TIMEOUT constant so connect() returns an error if either phase exceeds the bound. Specifically, replace direct calls to tokio::net::TcpStream::connect(&addr).await and connector.connect(domain, tcp).await with tokio::time::timeout(READ_TIMEOUT, ...).await, map a timeout (Elapsed) into an anyhow::Error with a clear message, and propagate other errors unchanged; update function connect to return early on timeout for the TcpStream or the TLS handshake respectively.
116-149:⚠️ Potential issue | 🟠 MajorUse the
base64crate and split oversized SASL PLAIN payloads.Two unresolved concerns in
encode_sasl_plain:
Custom base64 encoder is unnecessary —
base640.22 is already a direct dependency. The custom implementation adds maintenance burden without benefit.Missing RFC/IRCv3 SASL compliance — IRC's AUTHENTICATE command is limited to 400 bytes per line (IRCv3 SASL spec). For plaintext credentials larger than ~290 bytes, the base64-encoded
\0nick\0passwordpayload will exceed 400 bytes. The current code at line 458 sends a single oversized AUTHENTICATE line, causing server rejection or silent truncation of authentication.The fix requires:
- Replace custom encoder with
STANDARD.encode()from thebase64crate.- Split the encoded payload into 400-byte chunks in the AUTHENTICATE handler, sending each chunk as a separate AUTHENTICATE line, with
AUTHENTICATE +as a continuation terminator between chunks per the IRCv3 SASL spec.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/channels/irc.rs` around lines 116 - 149, The current encode_sasl_plain function implements a custom base64 encoder and the code that sends AUTHENTICATE uses a single oversized line; replace the custom encoder by calling the base64 crate (use the STANDARD encoder / base64::encode) inside encode_sasl_plain, and update the code path that sends the AUTHENTICATE payload (the caller of encode_sasl_plain / the AUTHENTICATE sender) to split the encoded payload into 400-byte chunks and send each chunk as its own AUTHENTICATE line, inserting the continuation marker per IRCv3 SASL (send AUTHENTICATE <chunk> for each chunk and use AUTHENTICATE + as the continuation/terminator protocol where required) so no single AUTHENTICATE line exceeds 400 bytes.
🧹 Nitpick comments (1)
clients/agent-runtime/src/channels/irc.rs (1)
429-448: CAP ACK/NAK detection uses substring matching — prefer exact equality.
p.contains("ACK")will match any param that includes the string"ACK"(e.g.,"NACK"or a capability named"backlog"). The parsed params for a typicalCAP * ACK :saslresponse are["*", "ACK", "sasl"], so exact equality is both safe and more precise.♻️ Proposed fix
- if msg.params.iter().any(|p| p.contains("ACK")) { + if msg.params.iter().any(|p| p == "ACK") { // CAP * ACK :sasl — server accepted, start SASL auth ... - } else if msg.params.iter().any(|p| p.contains("NAK")) { + } else if msg.params.iter().any(|p| p == "NAK") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/channels/irc.rs` around lines 429 - 448, The CAP ACK/NAK checks use substring matching (msg.params.iter().any(|p| p.contains("ACK"/"NAK"))) and should use exact equality to avoid false positives; update the checks in the CAP handling block that references sasl_pending, msg.params, and Self::send_raw so they use p == "ACK" and p == "NAK" (or otherwise compare the exact token after trimming any leading ':' if your parser leaves that), leaving the rest of the logic (sending "AUTHENTICATE PLAIN" and fallback "CAP END" via the writer lock) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@clients/agent-runtime/src/channels/irc.rs`:
- Around line 265-289: The connect() implementation lacks an application-level
timeout; wrap both the TCP connect and the TLS handshake in tokio::time::timeout
using the existing READ_TIMEOUT constant so connect() returns an error if either
phase exceeds the bound. Specifically, replace direct calls to
tokio::net::TcpStream::connect(&addr).await and connector.connect(domain,
tcp).await with tokio::time::timeout(READ_TIMEOUT, ...).await, map a timeout
(Elapsed) into an anyhow::Error with a clear message, and propagate other errors
unchanged; update function connect to return early on timeout for the TcpStream
or the TLS handshake respectively.
- Around line 116-149: The current encode_sasl_plain function implements a
custom base64 encoder and the code that sends AUTHENTICATE uses a single
oversized line; replace the custom encoder by calling the base64 crate (use the
STANDARD encoder / base64::encode) inside encode_sasl_plain, and update the code
path that sends the AUTHENTICATE payload (the caller of encode_sasl_plain / the
AUTHENTICATE sender) to split the encoded payload into 400-byte chunks and send
each chunk as its own AUTHENTICATE line, inserting the continuation marker per
IRCv3 SASL (send AUTHENTICATE <chunk> for each chunk and use AUTHENTICATE + as
the continuation/terminator protocol where required) so no single AUTHENTICATE
line exceeds 400 bytes.
---
Nitpick comments:
In `@clients/agent-runtime/src/channels/irc.rs`:
- Around line 429-448: The CAP ACK/NAK checks use substring matching
(msg.params.iter().any(|p| p.contains("ACK"/"NAK"))) and should use exact
equality to avoid false positives; update the checks in the CAP handling block
that references sasl_pending, msg.params, and Self::send_raw so they use p ==
"ACK" and p == "NAK" (or otherwise compare the exact token after trimming any
leading ':' if your parser leaves that), leaving the rest of the logic (sending
"AUTHENTICATE PLAIN" and fallback "CAP END" via the writer lock) unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clients/agent-runtime/src/channels/irc.rs (1)
263-291:⚠️ Potential issue | 🟡 Minor
READ_TIMEOUT(300 s) is semantically wrong for TCP connect and TLS handshake timeouts.
READ_TIMEOUTwas designed for read-inactivity detection (IRC servers PING every 60–120 s). Reusing it for connection establishment means a failed TCP connect or TLS handshake blocks the task for up to 5 minutes before the error surfaces.Introduce a shorter, dedicated constant for connection setup (10–30 s is typical):
🔧 Proposed fix
+/// Timeout for establishing the TCP connection and completing the TLS handshake. +const CONNECT_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(30); // … inside connect(): - let tcp = tokio::time::timeout(READ_TIMEOUT, tokio::net::TcpStream::connect(&addr)) + let tcp = tokio::time::timeout(CONNECT_TIMEOUT, tokio::net::TcpStream::connect(&addr)) .await .map_err(|_| { - anyhow::anyhow!("IRC TCP connect timed out after {READ_TIMEOUT:?} to {addr}") + anyhow::anyhow!("IRC TCP connect timed out after {CONNECT_TIMEOUT:?} to {addr}") })??; - let tls = tokio::time::timeout(READ_TIMEOUT, connector.connect(domain, tcp)) + let tls = tokio::time::timeout(CONNECT_TIMEOUT, connector.connect(domain, tcp)) .await .map_err(|_| { anyhow::anyhow!( - "IRC TLS handshake timed out after {READ_TIMEOUT:?} with {}", + "IRC TLS handshake timed out after {CONNECT_TIMEOUT:?} with {}", self.server ) })??;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/channels/irc.rs` around lines 263 - 291, The code is using READ_TIMEOUT for connection setup (TcpStream::connect and connector.connect) which is meant for read inactivity; define a new, shorter constant (e.g., CONNECT_TIMEOUT or SETUP_TIMEOUT = 10–30s) and replace usages of READ_TIMEOUT in the TcpStream::connect timeout and the TLS connector.connect timeout so the TCP connect and TLS handshake use the new timeout while leaving READ_TIMEOUT for read inactivity detection; update any relevant error messages to reference the new constant name (symbols: READ_TIMEOUT, TcpStream::connect, tokio_rustls::TlsConnector::connect, connector.connect).
🧹 Nitpick comments (3)
clients/agent-runtime/src/channels/irc.rs (1)
728-745: Add edge-case tests for empty payload and exactly-one-chunk (400-byte) payloads.The two new tests cover the 2×400 exact-multiple case and the non-multiple case. Two boundary cases remain untested:
- Empty payload — the
encoded.is_empty()branch should produce["+"].- Exactly 400 bytes — the smallest exact-multiple should produce
["a"×400, "+"].✅ Suggested additional tests
+ #[test] + fn sasl_payload_chunks_empty_payload_sends_terminator() { + let chunks = split_sasl_authenticate_payload(""); + assert_eq!(chunks, vec!["+"]); + } + + #[test] + fn sasl_payload_chunks_exactly_one_chunk_includes_terminator() { + let payload = "a".repeat(400); + let chunks = split_sasl_authenticate_payload(&payload); + assert_eq!(chunks.len(), 2); + assert_eq!(chunks[0].len(), 400); + assert_eq!(chunks[1], "+"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/channels/irc.rs` around lines 728 - 745, Add two unit tests for split_sasl_authenticate_payload to cover the empty-payload and exact-one-chunk (400-byte) boundary cases: assert that an empty encoded payload returns ["+"] and that a payload of length 400 returns ["a"×400, "+"]. Locate tests near sasl_payload_chunks_include_terminator_for_exact_boundary and sasl_payload_chunks_without_terminator_for_partial_tail in the same module (tests referencing split_sasl_authenticate_payload) and add the two cases so the encoded.is_empty() path and the single-chunk exact-multiple behavior are validated.clients/agent-runtime/src/providers/copilot.rs (2)
231-231: Document or name thetrueargument toSecretStore::new.The boolean literal is opaque at the call site. A brief comment or named constant clarifies intent without requiring readers to look up the
SecretStoreAPI.- secret_store: SecretStore::new(&corvus_dir, true), + // `true` = generate and persist the encryption key if it doesn't yet exist. + secret_store: SecretStore::new(&corvus_dir, true),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/providers/copilot.rs` at line 231, The call SecretStore::new(&corvus_dir, true) uses an opaque boolean literal; replace it with a descriptive named constant (e.g., USE_IN_MEMORY_SECRETS or LOAD_FROM_DISK) or add an inline comment to document the meaning of the true flag so callers reading SecretStore::new(&corvus_dir, ...) understand the behavior without inspecting the SecretStore API.
191-218: Windows lacks directory-level ACL hardening equivalent to the0o700Unix block.The
#[cfg(unix)]block (lines 197–218) correctly restrictscorvus_dirandtoken_dirto owner-only access on Unix. On Windows,create_dir_allinherits the parent's ACLs (AppData\Roamingis already user-scoped, so the practical risk is low), but no equivalenticacls /inheritance:rcall is made on the directories themselves. Consider adding a#[cfg(windows)]block here mirroring theicaclsapproach used inwrite_file_secure_blockingto harden both directories on Windows as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/providers/copilot.rs` around lines 191 - 218, The Windows-specific hardening for the created directories is missing: after create_dir_all(&token_dir) add a #[cfg(windows)] branch (similar to the Windows logic in write_file_secure_blocking) that invokes icacls to remove inherited ACLs and grant the current user full control for both corvus_dir and token_dir, capturing and warning on any errors; target the same symbols corvus_dir, token_dir and keep the existing #[cfg(unix)] block intact so Unix uses set_permissions while Windows runs the icacls commands and logs failures via warn!.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/src/providers/copilot.rs`:
- Around line 625-685: The Windows branch of write_file_secure_blocking has a
TOCTOU: the file is created with inherited ACLs then later hardened with icacls,
and it also calls "icacls" via PATH; fix by creating the file with secure ACLs
atomically and calling icacls via its full system path. Concretely: replace the
OpenOptions::new() creation + subsequent icacls call in
write_file_secure_blocking with a Windows API creation that supplies a
SECURITY_ATTRIBUTES / explicit DACL allowing only the current user (use
CreateFileW with a SECURITY_DESCRIPTOR or equivalent wrapper) so the file is
never briefly world-readable, then if you still call icacls keep the executable
path explicit (e.g., use %SystemRoot%\System32\icacls.exe resolved via
GetEnvironmentVariable or std::env::var("SystemRoot")) instead of relying on
PATH and continue to check output.status.success() as before.
- Around line 259-285: The plaintext token in write_token_file_secure is cloned
into a regular String and moved into a blocking closure, leaving it on the heap
unzeroed; add zeroize = "1.x" to clients/agent-runtime/Cargo.toml and wrap the
plaintext with Zeroizing::new(content.to_string()) (use zeroize::Zeroizing)
before moving it into the spawn_blocking closure so the buffer is zeroed on
drop; adjust any references inside the closure to deref or borrow the
Zeroizing<String> when calling secret_store.encrypt and
write_file_secure_blocking so the encrypted data is produced while the original
is guaranteed to be zeroized afterwards.
---
Outside diff comments:
In `@clients/agent-runtime/src/channels/irc.rs`:
- Around line 263-291: The code is using READ_TIMEOUT for connection setup
(TcpStream::connect and connector.connect) which is meant for read inactivity;
define a new, shorter constant (e.g., CONNECT_TIMEOUT or SETUP_TIMEOUT = 10–30s)
and replace usages of READ_TIMEOUT in the TcpStream::connect timeout and the TLS
connector.connect timeout so the TCP connect and TLS handshake use the new
timeout while leaving READ_TIMEOUT for read inactivity detection; update any
relevant error messages to reference the new constant name (symbols:
READ_TIMEOUT, TcpStream::connect, tokio_rustls::TlsConnector::connect,
connector.connect).
---
Nitpick comments:
In `@clients/agent-runtime/src/channels/irc.rs`:
- Around line 728-745: Add two unit tests for split_sasl_authenticate_payload to
cover the empty-payload and exact-one-chunk (400-byte) boundary cases: assert
that an empty encoded payload returns ["+"] and that a payload of length 400
returns ["a"×400, "+"]. Locate tests near
sasl_payload_chunks_include_terminator_for_exact_boundary and
sasl_payload_chunks_without_terminator_for_partial_tail in the same module
(tests referencing split_sasl_authenticate_payload) and add the two cases so the
encoded.is_empty() path and the single-chunk exact-multiple behavior are
validated.
In `@clients/agent-runtime/src/providers/copilot.rs`:
- Line 231: The call SecretStore::new(&corvus_dir, true) uses an opaque boolean
literal; replace it with a descriptive named constant (e.g.,
USE_IN_MEMORY_SECRETS or LOAD_FROM_DISK) or add an inline comment to document
the meaning of the true flag so callers reading SecretStore::new(&corvus_dir,
...) understand the behavior without inspecting the SecretStore API.
- Around line 191-218: The Windows-specific hardening for the created
directories is missing: after create_dir_all(&token_dir) add a #[cfg(windows)]
branch (similar to the Windows logic in write_file_secure_blocking) that invokes
icacls to remove inherited ACLs and grant the current user full control for both
corvus_dir and token_dir, capturing and warning on any errors; target the same
symbols corvus_dir, token_dir and keep the existing #[cfg(unix)] block intact so
Unix uses set_permissions while Windows runs the icacls commands and logs
failures via warn!.
| async fn write_token_file_secure(&self, path: &Path, content: &str) { | ||
| let path = path.to_path_buf(); | ||
| let path_display = path.display().to_string(); | ||
| let content = content.to_string(); | ||
| let secret_store = self.secret_store.clone(); | ||
|
|
||
| let result = tokio::task::spawn_blocking(move || -> anyhow::Result<()> { | ||
| let encrypted = secret_store.encrypt(&content)?; | ||
| write_file_secure_blocking(&path, &encrypted)?; | ||
| Ok(()) | ||
| }) | ||
| .await; | ||
|
|
||
| match result { | ||
| Ok(Ok(())) => {} | ||
| Ok(Err(err)) => { | ||
| warn!( | ||
| "Failed to write secure Copilot token file {}: {err}", | ||
| path_display | ||
| ) | ||
| } | ||
| Err(err) => warn!( | ||
| "Failed to spawn token write task for {}: {err}", | ||
| path_display | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if zeroize is already a dependency in Cargo files
find clients/agent-runtime -name "Cargo.toml" -type f | head -20Repository: dallay/corvus
Length of output: 308
🏁 Script executed:
# Search for zeroize in dependencies
rg "zeroize" --type tomlRepository: dallay/corvus
Length of output: 421
🏁 Script executed:
# Read the actual file to verify the code snippet
cat -n clients/agent-runtime/src/providers/copilot.rs | sed -n '250,290p'Repository: dallay/corvus
Length of output: 1675
🏁 Script executed:
# Search for other token/credential handling patterns
rg "to_string\(\)" clients/agent-runtime/src/providers/copilot.rs -B 2 -A 2Repository: dallay/corvus
Length of output: 3885
🏁 Script executed:
# Check for write_file_secure_blocking implementation
rg -A 10 "fn write_file_secure_blocking" clients/agent-runtime/srcRepository: dallay/corvus
Length of output: 910
🏁 Script executed:
# Check if zeroize is explicitly listed in the main Cargo.toml as a dependency
cat clients/agent-runtime/Cargo.toml | grep -A 50 "\[dependencies\]"Repository: dallay/corvus
Length of output: 1681
🏁 Script executed:
# Also check what currently uses/imports zeroize in the codebase
rg "use zeroize" clients/agent-runtime/src --type rustRepository: dallay/corvus
Length of output: 39
🏁 Script executed:
# Search for imports related to secret or secure handling
rg "use.*secret|use.*crypto|use.*secure" clients/agent-runtime/src/providers/copilot.rsRepository: dallay/corvus
Length of output: 39
🏁 Script executed:
# Check the secret_store.encrypt signature to understand what it expects
rg -B 5 -A 10 "fn encrypt" clients/agent-runtime/src --type rust | head -40Repository: dallay/corvus
Length of output: 3345
Add zeroize dependency and use Zeroizing<String> to prevent plaintext token from lingering in heap memory.
The plaintext token copied to line 262 is dropped without zeroing after encryption, leaving it exposed in heap memory until the allocator reclaims it. Although zeroize is pulled transitively, it is not explicitly listed as a dependency and is not used anywhere in the codebase.
To fix:
- Add
zeroize = "1.x"to[dependencies]inclients/agent-runtime/Cargo.toml - Wrap the plaintext String with
Zeroizing::new()before moving it into the blocking closure
The zeroize crate uses core::ptr::write_volatile to guarantee the buffer is zeroed, preventing compiler optimizations from eliding the operation.
Proposed fix
+use zeroize::Zeroizing;
async fn write_token_file_secure(&self, path: &Path, content: &str) {
let path = path.to_path_buf();
let path_display = path.display().to_string();
- let content = content.to_string();
+ let content = Zeroizing::new(content.to_string());
let secret_store = self.secret_store.clone();
let result = tokio::task::spawn_blocking(move || -> anyhow::Result<()> {
let encrypted = secret_store.encrypt(&content)?;
write_file_secure_blocking(&path, &encrypted)?;
Ok(())
})
.await;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/agent-runtime/src/providers/copilot.rs` around lines 259 - 285, The
plaintext token in write_token_file_secure is cloned into a regular String and
moved into a blocking closure, leaving it on the heap unzeroed; add zeroize =
"1.x" to clients/agent-runtime/Cargo.toml and wrap the plaintext with
Zeroizing::new(content.to_string()) (use zeroize::Zeroizing) before moving it
into the spawn_blocking closure so the buffer is zeroed on drop; adjust any
references inside the closure to deref or borrow the Zeroizing<String> when
calling secret_store.encrypt and write_file_secure_blocking so the encrypted
data is produced while the original is guaranteed to be zeroized afterwards.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
clients/agent-runtime/src/providers/copilot.rs (3)
752-754: Doc comment is slightly misleading — the function doesn't usespawn_blocking, it is called fromspawn_blocking.📝 Suggested doc fix
-/// Write a file with restrictive owner-only permissions. -/// Uses `spawn_blocking` to avoid blocking the async runtime. +/// Write a file with restrictive owner-only permissions (synchronous). +/// Intended to be called from `spawn_blocking` to avoid blocking the async runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/providers/copilot.rs` around lines 752 - 754, The doc comment for write_file_secure_blocking is misleading about spawn_blocking; update the comment to state that this function is synchronous and should be called from a spawn_blocking context (or other blocking executor) rather than claiming it itself uses spawn_blocking, e.g., mention it writes a file with restrictive owner-only permissions and is intended to be invoked inside spawn_blocking to avoid blocking the async runtime.
254-275: Decrypted plaintext returned as bareString— consider wrapping inZeroizingfor consistency with the write path.
read_token_file_securereturns a plainStringholding the decrypted token. The write path (line 280) usesZeroizing::new(...)to ensure the plaintext is zeroed on drop, but the read path leaves the decrypted value unprotected in heap memory. The callers (get_github_access_token,load_api_key_from_disk) also store it as bareStrings.To get meaningful coverage you'd need
Zeroizing<String>propagated through the return type and intoCachedApiKey.token, which is a larger change. Worth considering as a follow-up for end-to-end zeroization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/providers/copilot.rs` around lines 254 - 275, read_token_file_secure currently returns a plain String containing the decrypted token, which leaves sensitive plaintext unzeroed in heap memory; change the function to return Zeroizing<String> (or Option<Zeroizing<String>>) instead of String and propagate that type through callers such as get_github_access_token and load_api_key_from_disk and into CachedApiKey.token so the decrypted bytes are wrapped with Zeroizing::new(...) (matching the write path behavior referenced at the write path near line ~280) and ensure error branches still return None; update signatures and any places that construct or store the token to accept Zeroizing<String> so plaintext is zeroed on drop.
633-638: Intermediatejsonstring containing the API key token is not zeroized.
serde_json::to_string_pretty(info)produces a plainStringwith the raw token inside. It's passed by reference towrite_token_file_secure, which clones it into aZeroizinginternally (line 280), but the originaljsonis dropped without zeroing after the.await.Same pattern exists in
get_github_access_token(line 519–522) wheretokenis a plainString.This is a defense-in-depth gap — not exploitable without memory access, but it weakens the guarantee
Zeroizingis meant to provide.♻️ Minimal fix for save_api_key_to_disk
async fn save_api_key_to_disk(&self, info: &ApiKeyInfo) { let path = self.token_dir.join("api-key.json"); if let Ok(json) = serde_json::to_string_pretty(info) { - self.write_token_file_secure(&path, &json).await; + let json = Zeroizing::new(json); + self.write_token_file_secure(&path, &json).await; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/providers/copilot.rs` around lines 633 - 638, The temporary plain-String holding sensitive data must be wrapped in a zeroizing container before awaiting; change save_api_key_to_disk to wrap the serde_json::to_string_pretty result in a Zeroizing<String> (e.g., let json = Zeroizing::new(serde_json::to_string_pretty(info)?)); pass json.as_ref() to write_token_file_secure and await, so the only heap copy is inside Zeroizing and will be zeroed on drop; do the same in get_github_access_token for the token variable (wrap the token String in Zeroizing<String> before any await or use outside the function), and add the Zeroizing import (zeroize::Zeroizing). Ensure you don’t leave any plain String alive across awaits and avoid extra clones.clients/agent-runtime/src/channels/irc.rs (4)
130-146: SASL chunking logic and tests are correct; note the implicit ASCII invariant on byte-level slicing.
encoded[start..end]byte-slices a&str. This is safe here because base64 output (A-Z,a-z,0-9,+,/,=) is entirely ASCII and every byte is a valid UTF-8 char boundary. However, the invariant is implicit. A quick comment or adebug_assert!would make it explicit.♻️ Suggested hardening (optional)
fn split_sasl_authenticate_payload(encoded: &str) -> Vec<String> { const SASL_CHUNK_MAX: usize = 400; + // SAFETY: base64 output is ASCII-only, so every byte offset is a valid char boundary. + debug_assert!(encoded.is_ascii(), "SASL payload must be ASCII base64");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/channels/irc.rs` around lines 130 - 146, The byte-level slicing in split_sasl_authenticate_payload assumes encoded is base64 ASCII, which is safe but implicit; make this invariant explicit by adding a debug_assert!(encoded.is_ascii()) (or a short comment) near the top of split_sasl_authenticate_payload and document that SASL_CHUNK_MAX chunking uses ASCII-safe slicing, referencing the function name and the SASL_CHUNK_MAX constant so future readers know why encoded[start..end] is safe.
3-3: Consider anonymous trait import to avoid namespace pollution.The idiomatic base64 pattern imports the trait anonymously —
use base64::{Engine as _, engine::general_purpose};— keeping the trait out of the module namespace while still enabling.encode()onSTANDARD. The current named import works correctly but exportsEngineinto this module's scope unnecessarily.♻️ Proposed refactor
-use base64::Engine; +use base64::{Engine as _, engine::general_purpose};Then at line 123:
- base64::engine::general_purpose::STANDARD.encode(input) + general_purpose::STANDARD.encode(input)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/channels/irc.rs` at line 3, Replace the named import of the base64 trait to avoid polluting the module namespace: instead of `use base64::Engine;` import the trait anonymously and bring in the general purpose engine (e.g. `use base64::{Engine as _, engine::general_purpose};`) so calls like `general_purpose::STANDARD.encode(...)` still work but `Engine` is not exported into the module scope; update any uses that reference `STANDARD` to use `general_purpose::STANDARD` if not already.
731-761: New chunking tests are correct; add a unit test for the CAP NAK path.The four
split_sasl_authenticate_payloadtests are accurate and cover all edge cases well. However, the newly added CAP NAK branch (Lines 449–463) — which ends SASL negotiation and sendsCAP ENDon server rejection — has no corresponding test. Per the coding guideline, auth-path behavior insrc/channels/**should be covered with tests.The NAK path can be tested at the unit level by driving
IrcMessage::parseand asserting the parsed params, combined with a test that directly exercises the logic via a mock writer (or at minimum a doc-test comment explaining expected behavior). A small example:#[test] fn parse_cap_nak() { let msg = IrcMessage::parse(":server CAP * NAK :sasl").unwrap(); assert_eq!(msg.command, "CAP"); assert_eq!(msg.params, vec!["*", "NAK", "sasl"]); // Verify NAK detection: no trim_start_matches needed assert!(msg.params.iter().any(|p| p == "NAK")); assert!(msg.params.iter().any(|p| p.contains("sasl"))); }Based on learnings: "Implement
Channeltrait insrc/channels/with consistentsend,listen, andhealth_checksemantics and cover auth/allowlist/health behavior with tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/channels/irc.rs` around lines 731 - 761, Add a unit test that covers the CAP NAK branch which ends SASL negotiation and sends "CAP END": create a test that uses IrcMessage::parse to parse a NAK message (e.g. ":server CAP * NAK :sasl") and assert msg.command == "CAP" and msg.params contains "NAK" and "sasl", then invoke the code-path that handles CAP messages (the function/method where the CAP NAK branch lives in irc.rs) using a mock writer or test double to verify it triggers the SASL end behavior and sends "CAP END" (or transitions state accordingly); reference split_sasl_authenticate_payload tests as an example for test structure and ensure the new test lives alongside the existing tests in channels/irc.rs so auth NAK behavior is covered.
439-463:trim_start_matches(':')is redundant —IrcMessage::parsealready strips the colon.
IrcMessage::parseseparates trailing parameters (the" :"suffix) and strips the colon before pushing them intoparams. For a typicalCAP * ACK :saslmessage the params arrive as["*", "ACK", "sasl"], so"ACK".trim_start_matches(':')is always a no-op. Removing it would make the comparison intent clearer.♻️ Proposed cleanup
- if msg - .params - .iter() - .any(|p| p.trim_start_matches(':') == "ACK") + if msg.params.iter().any(|p| p == "ACK") { // CAP * ACK :sasl — server accepted, start SASL auth ... - } else if msg - .params - .iter() - .any(|p| p.trim_start_matches(':') == "NAK") + } else if msg.params.iter().any(|p| p == "NAK") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/channels/irc.rs` around lines 439 - 463, The param comparisons are redundantly calling trim_start_matches(':') because IrcMessage::parse already strips the leading colon; update the two checks that look at msg.params.iter().any(|p| p.trim_start_matches(':') == "ACK") and the NAK variant to compare directly to "ACK" and "NAK" (e.g., |p| p == "ACK" and |p| p == "NAK") so behavior is unchanged while making intent clearer; ensure you modify both the ACK branch (before calling Self::send_raw("AUTHENTICATE PLAIN")) and the NAK branch (where sasl_pending is set false and Self::send_raw("CAP END") is sent) so all redundant trimming is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@clients/agent-runtime/src/providers/copilot.rs`:
- Around line 641-680: The duplicate review comment tag should be removed so the
PR/message is not repeated; locate the implementations of windows_icacls_path
and harden_windows_acl in copilot.rs and delete the extraneous
"[duplicate_comment]" or duplicate comment block, leaving only the single
intended comment that documents the SystemRoot fallback for icacls resolution.
- Around line 277-303: Duplicate review comment — no code change needed; remove
the duplicate reviewer note. Confirm that write_token_file_secure already wraps
the plaintext in Zeroizing and moves it into the spawn_blocking closure
(references: write_token_file_secure, Zeroizing::new, spawn_blocking,
secret_store.encrypt), then delete the redundant review comment so only the
addressed comment remains.
---
Nitpick comments:
In `@clients/agent-runtime/src/channels/irc.rs`:
- Around line 130-146: The byte-level slicing in split_sasl_authenticate_payload
assumes encoded is base64 ASCII, which is safe but implicit; make this invariant
explicit by adding a debug_assert!(encoded.is_ascii()) (or a short comment) near
the top of split_sasl_authenticate_payload and document that SASL_CHUNK_MAX
chunking uses ASCII-safe slicing, referencing the function name and the
SASL_CHUNK_MAX constant so future readers know why encoded[start..end] is safe.
- Line 3: Replace the named import of the base64 trait to avoid polluting the
module namespace: instead of `use base64::Engine;` import the trait anonymously
and bring in the general purpose engine (e.g. `use base64::{Engine as _,
engine::general_purpose};`) so calls like
`general_purpose::STANDARD.encode(...)` still work but `Engine` is not exported
into the module scope; update any uses that reference `STANDARD` to use
`general_purpose::STANDARD` if not already.
- Around line 731-761: Add a unit test that covers the CAP NAK branch which ends
SASL negotiation and sends "CAP END": create a test that uses IrcMessage::parse
to parse a NAK message (e.g. ":server CAP * NAK :sasl") and assert msg.command
== "CAP" and msg.params contains "NAK" and "sasl", then invoke the code-path
that handles CAP messages (the function/method where the CAP NAK branch lives in
irc.rs) using a mock writer or test double to verify it triggers the SASL end
behavior and sends "CAP END" (or transitions state accordingly); reference
split_sasl_authenticate_payload tests as an example for test structure and
ensure the new test lives alongside the existing tests in channels/irc.rs so
auth NAK behavior is covered.
- Around line 439-463: The param comparisons are redundantly calling
trim_start_matches(':') because IrcMessage::parse already strips the leading
colon; update the two checks that look at msg.params.iter().any(|p|
p.trim_start_matches(':') == "ACK") and the NAK variant to compare directly to
"ACK" and "NAK" (e.g., |p| p == "ACK" and |p| p == "NAK") so behavior is
unchanged while making intent clearer; ensure you modify both the ACK branch
(before calling Self::send_raw("AUTHENTICATE PLAIN")) and the NAK branch (where
sasl_pending is set false and Self::send_raw("CAP END") is sent) so all
redundant trimming is removed.
In `@clients/agent-runtime/src/providers/copilot.rs`:
- Around line 752-754: The doc comment for write_file_secure_blocking is
misleading about spawn_blocking; update the comment to state that this function
is synchronous and should be called from a spawn_blocking context (or other
blocking executor) rather than claiming it itself uses spawn_blocking, e.g.,
mention it writes a file with restrictive owner-only permissions and is intended
to be invoked inside spawn_blocking to avoid blocking the async runtime.
- Around line 254-275: read_token_file_secure currently returns a plain String
containing the decrypted token, which leaves sensitive plaintext unzeroed in
heap memory; change the function to return Zeroizing<String> (or
Option<Zeroizing<String>>) instead of String and propagate that type through
callers such as get_github_access_token and load_api_key_from_disk and into
CachedApiKey.token so the decrypted bytes are wrapped with Zeroizing::new(...)
(matching the write path behavior referenced at the write path near line ~280)
and ensure error branches still return None; update signatures and any places
that construct or store the token to accept Zeroizing<String> so plaintext is
zeroed on drop.
- Around line 633-638: The temporary plain-String holding sensitive data must be
wrapped in a zeroizing container before awaiting; change save_api_key_to_disk to
wrap the serde_json::to_string_pretty result in a Zeroizing<String> (e.g., let
json = Zeroizing::new(serde_json::to_string_pretty(info)?)); pass json.as_ref()
to write_token_file_secure and await, so the only heap copy is inside Zeroizing
and will be zeroed on drop; do the same in get_github_access_token for the token
variable (wrap the token String in Zeroizing<String> before any await or use
outside the function), and add the Zeroizing import (zeroize::Zeroizing). Ensure
you don’t leave any plain String alive across awaits and avoid extra clones.
…PI key management
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
clients/agent-runtime/src/channels/irc.rs (1)
122-125: SASL PLAIN credential buffer left unzeroized in memory.
inputholds the raw\0nick\0passwordcredential string as a plainString. It is passed by value intogeneral_purpose::STANDARD.encode, after which it is dropped without being zeroed. This leaves the actual plaintext credentials in heap memory until the allocator reclaims the slot — the same class of concern addressed in the Copilot provider withZeroizing.♻️ Proposed fix
+use zeroize::Zeroizing; fn encode_sasl_plain(nick: &str, password: &str) -> String { - let input = format!("\0{nick}\0{password}"); - general_purpose::STANDARD.encode(input) + let input = Zeroizing::new(format!("\0{nick}\0{password}")); + general_purpose::STANDARD.encode(input.as_bytes()) }Based on learnings from
AGENTS.md: "Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements" — and by extension, raw credentials should also be handled withZeroizingto limit their lifetime in memory, consistent with the approach taken incopilot.rs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/channels/irc.rs` around lines 122 - 125, The encode_sasl_plain function currently builds a plain String with credentials that is dropped without zeroing; wrap the credential buffer in a zeroizing container (use zeroize::Zeroizing) instead of a raw String: create a Zeroizing<String> or Zeroizing<Vec<u8>> from format!("\0{nick}\0{password}"), pass its byte slice to general_purpose::STANDARD.encode (e.g., general_purpose::STANDARD.encode(&*zeroized)) so only the base64 result is returned, and let the Zeroizing go out of scope to securely zero the plaintext; add the appropriate use of zeroize::Zeroizing and replace references to the plain `input` variable in encode_sasl_plain with the zeroized wrapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/src/providers/copilot.rs`:
- Around line 222-236: The current flow creates corvus_dir and token_dir with
std::fs::create_dir_all and then calls harden_windows_acl, leaving a TOCTOU
where the directories inherit parent ACLs; change creation to use the Windows
API like CreateDirectoryW with a SECURITY_ATTRIBUTES that contains a restrictive
DACL (same pattern as create_secure_file_windows) so the directory is created
atomically with the intended ACL, or if you accept the residual risk add a clear
comment near the calls to harden_windows_acl referencing corvus_dir and
token_dir explaining why the brief window is tolerable; update code paths that
currently call create_dir_all before harden_windows_acl to instead call the new
secure-directory-creation logic and retain the existing harden_windows_acl calls
as a fallback.
- Around line 254-276: read_token_file_secure currently calls
SecretStore::decrypt synchronously on an async task (blocking the executor);
move the decrypt call inside tokio::task::spawn_blocking (like
write_token_file_secure does for encrypt) so load_or_create_key runs off the
async runtime, then await the JoinHandle and use .await.ok().flatten() to turn
the Result<Option<String>> into Option<String> safely; update the function flow
around read_token_file_secure, SecretStore::decrypt, spawn_blocking and
Zeroizing to create and return Some(Zeroizing::new(token.to_string())) only when
the spawned decrypt returns a non-empty string and log failures as before.
---
Duplicate comments:
In `@clients/agent-runtime/src/channels/irc.rs`:
- Around line 16-18: The CONNECT_TIMEOUT constant's comment was updated to
correctly state that the 20s budget is per-phase (TCP connect and TLS
handshake); no code change is required—keep the const CONNECT_TIMEOUT:
std::time::Duration and its accompanying comment as-is in irc.rs (ensure future
edits preserve that clarified comment next to CONNECT_TIMEOUT).
In `@clients/agent-runtime/src/providers/copilot.rs`:
- Around line 278-304: No change needed: in write_token_file_secure the
plaintext is now wrapped with Zeroizing::new(content.to_string()) before being
moved into the spawn_blocking closure and passed to secret_store.encrypt via
content.as_str(), so keep this usage (Zeroizing::new and the function
write_token_file_secure) as-is to ensure the plaintext buffer is zeroed on drop
after encryption.
---
Nitpick comments:
In `@clients/agent-runtime/src/channels/irc.rs`:
- Around line 122-125: The encode_sasl_plain function currently builds a plain
String with credentials that is dropped without zeroing; wrap the credential
buffer in a zeroizing container (use zeroize::Zeroizing) instead of a raw
String: create a Zeroizing<String> or Zeroizing<Vec<u8>> from
format!("\0{nick}\0{password}"), pass its byte slice to
general_purpose::STANDARD.encode (e.g.,
general_purpose::STANDARD.encode(&*zeroized)) so only the base64 result is
returned, and let the Zeroizing go out of scope to securely zero the plaintext;
add the appropriate use of zeroize::Zeroizing and replace references to the
plain `input` variable in encode_sasl_plain with the zeroized wrapper.
| #[cfg(windows)] | ||
| { | ||
| if let Err(err) = harden_windows_acl(&corvus_dir, true) { | ||
| warn!( | ||
| "Failed to harden Corvus config directory ACL on {:?}: {err}", | ||
| corvus_dir | ||
| ); | ||
| } | ||
| if let Err(err) = harden_windows_acl(&token_dir, true) { | ||
| warn!( | ||
| "Failed to harden Copilot token directory ACL on {:?}: {err}", | ||
| token_dir | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Windows directory hardening still has a brief TOCTOU window.
For files, the past concern was resolved by create_secure_file_windows which atomically creates the file with a restricted DACL. However, the directories corvus_dir and token_dir are created first by std::fs::create_dir_all (inheriting the parent's ACL from AppData\Roaming) and hardened only afterwards by harden_windows_acl. On a standard Windows installation, AppData\Roaming already limits access to the current user, SYSTEM, and Administrators, so the practical risk is low — but the window exists.
The most robust fix is to create the directory using CreateDirectoryW with a pre-built SECURITY_ATTRIBUTES (same approach as create_secure_file_windows), eliminating the TOCTOU entirely. Alternatively, document the acceptable residual risk in a comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/agent-runtime/src/providers/copilot.rs` around lines 222 - 236, The
current flow creates corvus_dir and token_dir with std::fs::create_dir_all and
then calls harden_windows_acl, leaving a TOCTOU where the directories inherit
parent ACLs; change creation to use the Windows API like CreateDirectoryW with a
SECURITY_ATTRIBUTES that contains a restrictive DACL (same pattern as
create_secure_file_windows) so the directory is created atomically with the
intended ACL, or if you accept the residual risk add a clear comment near the
calls to harden_windows_acl referencing corvus_dir and token_dir explaining why
the brief window is tolerable; update code paths that currently call
create_dir_all before harden_windows_acl to instead call the new
secure-directory-creation logic and retain the existing harden_windows_acl calls
as a fallback.
| async fn read_token_file_secure(&self, path: &Path) -> Option<Zeroizing<String>> { | ||
| let data = tokio::fs::read_to_string(path).await.ok()?; | ||
| let value = data.trim(); | ||
| if value.is_empty() { | ||
| return None; | ||
| } | ||
|
|
||
| match self.secret_store.decrypt(value) { | ||
| Ok(decrypted) => { | ||
| let decrypted = Zeroizing::new(decrypted); | ||
| let token = decrypted.trim(); | ||
| if token.is_empty() { | ||
| None | ||
| } else { | ||
| Some(Zeroizing::new(token.to_string())) | ||
| } | ||
| } | ||
| Err(err) => { | ||
| warn!("Failed to decrypt Copilot token file {:?}: {err}", path); | ||
| None | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify SecretStore::decrypt complexity and Clone implementation
rg -n "fn decrypt\|fn encrypt\|struct SecretStore\|impl Clone for SecretStore" \
clients/agent-runtime/src --type rust -A 12Repository: dallay/corvus
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Search for SecretStore in rust files
rg "SecretStore" clients/agent-runtime/src --type rust -n | head -20Repository: dallay/corvus
Length of output: 2121
🏁 Script executed:
#!/bin/bash
# Read the SecretStore definition and implementations
cat -n clients/agent-runtime/src/security/secrets.rs | head -150Repository: dallay/corvus
Length of output: 7298
🏁 Script executed:
#!/bin/bash
# Read load_or_create_key and rest of SecretStore implementation
cat -n clients/agent-runtime/src/security/secrets.rs | sed -n '150,250p'Repository: dallay/corvus
Length of output: 4763
🏁 Script executed:
#!/bin/bash
# Check the write_token_file_secure method in copilot.rs to see spawn_blocking usage
rg -n "write_token_file_secure" clients/agent-runtime/src/providers/copilot.rs -A 25Repository: dallay/corvus
Length of output: 3050
🏁 Script executed:
#!/bin/bash
# Verify read_token_file_secure context around line 254
cat -n clients/agent-runtime/src/providers/copilot.rs | sed -n '254,276p'Repository: dallay/corvus
Length of output: 990
🏁 Script executed:
#!/bin/bash
# Check if there are any other calls to decrypt or if key loading is expensive
rg -n "load_or_create_key\|decrypt(" clients/agent-runtime/src/security/secrets.rs -B2 -A2 | head -40Repository: dallay/corvus
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Check load_or_create_key calls and decrypt calls
rg -n "load_or_create_key" clients/agent-runtime/src/security/secrets.rs -B1 -A1Repository: dallay/corvus
Length of output: 1680
Move the decrypt call inside spawn_blocking to match the encrypt pattern.
read_token_file_secure and write_token_file_secure are inconsistent. Both call SecretStore::decrypt and encrypt, which invoke load_or_create_key()—a synchronous file I/O operation (reads the encryption key from disk). write_token_file_secure (line 284) correctly wraps encrypt in spawn_blocking, but read_token_file_secure (line 261) calls decrypt directly on the async task, blocking the executor.
Move decrypt into spawn_blocking as well for consistency and to prevent blocking the async runtime.
Proposed refactor
Note: The proposed diff has a subtle error—.unwrap_or(None) will panic if spawn_blocking returns Ok(None). Use .await.ok().flatten() instead:
async fn read_token_file_secure(&self, path: &Path) -> Option<Zeroizing<String>> {
let ciphertext = tokio::fs::read_to_string(path).await.ok()?;
let ciphertext = ciphertext.trim().to_string();
if ciphertext.is_empty() {
return None;
}
let secret_store = self.secret_store.clone();
let path_display = path.display().to_string();
- tokio::task::spawn_blocking(move || {
+ tokio::task::spawn_blocking(move || -> Option<Zeroizing<String>> {
match secret_store.decrypt(&ciphertext) {
Ok(decrypted) => {
let decrypted = Zeroizing::new(decrypted);
let token = decrypted.trim().to_string();
if token.is_empty() { None } else { Some(Zeroizing::new(token)) }
}
Err(err) => {
warn!("Failed to decrypt Copilot token file {}: {err}", path_display);
None
}
}
})
.await
- .unwrap_or(None)
+ .ok()
+ .flatten()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/agent-runtime/src/providers/copilot.rs` around lines 254 - 276,
read_token_file_secure currently calls SecretStore::decrypt synchronously on an
async task (blocking the executor); move the decrypt call inside
tokio::task::spawn_blocking (like write_token_file_secure does for encrypt) so
load_or_create_key runs off the async runtime, then await the JoinHandle and use
.await.ok().flatten() to turn the Result<Option<String>> into Option<String>
safely; update the function flow around read_token_file_secure,
SecretStore::decrypt, spawn_blocking and Zeroizing to create and return
Some(Zeroizing::new(token.to_string())) only when the spawned decrypt returns a
non-empty string and log failures as before.
… decryption handling
1 participant
This pull request introduces significant improvements to the security of token storage for the Copilot provider in the
clients/agent-runtimecrate. The main change is the integration of theSecretStorefor encrypting and decrypting sensitive tokens and API keys at rest, replacing the previous plain-text storage. Additionally, the file-writing logic has been refactored for better separation of concerns and reliability. There is also a minor update to test assertions and a removal of an unused PostCSS config file.Security improvements for token storage:
SecretStoreintoCopilotProviderto encrypt and decrypt Copilot access tokens and API keys stored on disk, enhancing security for sensitive credentials. (clients/agent-runtime/src/providers/copilot.rs) [1] [2]read_token_file_secureandwrite_token_file_secureto handle encrypted reading and writing of token files usingSecretStore. (clients/agent-runtime/src/providers/copilot.rs)clients/agent-runtime/src/providers/copilot.rs) [1] [2]Refactoring and reliability:
write_file_secure_blockingfor synchronous secure file writing, and removed the old asyncwrite_file_securefunction. (clients/agent-runtime/src/providers/copilot.rs)Testing and cleanup:
copilot_headers_include_required_fieldstest for clarity. (clients/agent-runtime/src/providers/copilot.rs)postcss.config.jsfile from the dashboard web app. (clients/web/apps/dashboard/postcss.config.js)Summary by CodeRabbit