Add browser-based device authorization#16
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Companion backend/dashboard PR: https://github.com/Edison-Watch/edison-watch/pull/1077 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 24 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- Treat a user or org change on a reused installation as an account change so the previous per-user secret is cleared and the old credential revoked - Restrict the --backend mismatch error to configs holding issuer-bound auth state so first-run/logged-out configs can switch servers - Disable HTTP redirects on the auth client so the PKCE verifier and tokens are never re-posted to a redirect target - Accept token_type case-insensitively per RFC 6749 - Abort the WS sink task on every run_frame_loop exit path, including reasoned closes and recv errors - Move the current credential file aside instead of deleting it during Windows replacement so a failure never loses the previous contents - Restart a child whose outbound queue overflows instead of leaving it alive while dropping subsequent MCP requests - Share the child handle with the pumps so terminal diagnostics report the real exit code, and assert it in the exit-status regression test - Assert stale scoped tokens are dropped in the invalid-saved-backend test Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
3 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/edison-stdiod/src/proc.rs">
<violation number="1" location="crates/edison-stdiod/src/proc.rs:646">
P1: Child-process descendants can survive a later restart, logout, or shutdown: the new status poll reaps the direct child before `shutdown` tries to kill its process tree, causing `child.id()` to be unavailable and both tree-kill branches to be skipped. Preserving a process-group/job handle or performing tree termination before reaping would keep descendant MCP processes from leaking.</violation>
</file>
<file name="crates/edison-stdiod/src/secure_file.rs">
<violation number="1" location="crates/edison-stdiod/src/secure_file.rs:96">
P2: When the destination path is an existing directory and the initial Windows rename returns one of the handled errors, this fallback moves that directory to the backup name and installs the credential file in its place. That silently relocates arbitrary directory contents; checking and rejecting directory targets before moving `path` would preserve the existing failure behavior.</violation>
<violation number="2" location="crates/edison-stdiod/src/secure_file.rs:96">
P2: A daemon crash or forced termination after this rename but before `tmp -> path` leaves Windows `config.toml`/`server_envs.json` absent, even though the old bytes remain in `.bak`. The next startup therefore loses the stored auth or environment state that this backup is intended to preserve; stale-backup recovery is needed before treating the canonical file as missing.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| async fn child_exit_status(child: Option<&SharedChild>) -> Option<ExitStatus> { | ||
| let child = child?; | ||
| for _ in 0..10 { | ||
| if let Ok(Some(status)) = child.lock().await.try_wait() { |
There was a problem hiding this comment.
P1: Child-process descendants can survive a later restart, logout, or shutdown: the new status poll reaps the direct child before shutdown tries to kill its process tree, causing child.id() to be unavailable and both tree-kill branches to be skipped. Preserving a process-group/job handle or performing tree termination before reaping would keep descendant MCP processes from leaking.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/edison-stdiod/src/proc.rs, line 646:
<comment>Child-process descendants can survive a later restart, logout, or shutdown: the new status poll reaps the direct child before `shutdown` tries to kill its process tree, causing `child.id()` to be unavailable and both tree-kill branches to be skipped. Preserving a process-group/job handle or performing tree termination before reaping would keep descendant MCP processes from leaking.</comment>
<file context>
@@ -619,20 +626,38 @@ impl ChildServer {
+async fn child_exit_status(child: Option<&SharedChild>) -> Option<ExitStatus> {
+ let child = child?;
+ for _ in 0..10 {
+ if let Ok(Some(status)) = child.lock().await.try_wait() {
+ return Some(status);
+ }
</file context>
| // destination is absent remains, but never one where no copy of | ||
| // the data exists. | ||
| let backup = unique_sibling_path(path, "bak"); | ||
| match std::fs::rename(path, &backup) { |
There was a problem hiding this comment.
P2: When the destination path is an existing directory and the initial Windows rename returns one of the handled errors, this fallback moves that directory to the backup name and installs the credential file in its place. That silently relocates arbitrary directory contents; checking and rejecting directory targets before moving path would preserve the existing failure behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/edison-stdiod/src/secure_file.rs, line 96:
<comment>When the destination path is an existing directory and the initial Windows rename returns one of the handled errors, this fallback moves that directory to the backup name and installs the credential file in its place. That silently relocates arbitrary directory contents; checking and rejecting directory targets before moving `path` would preserve the existing failure behavior.</comment>
<file context>
@@ -81,14 +85,32 @@ fn replace(tmp: &Path, path: &Path) -> io::Result<()> {
+ // destination is absent remains, but never one where no copy of
+ // the data exists.
+ let backup = unique_sibling_path(path, "bak");
+ match std::fs::rename(path, &backup) {
Ok(()) => {}
- Err(error) if error.kind() == io::ErrorKind::NotFound => {}
</file context>
| match std::fs::rename(path, &backup) { | |
| if std::fs::metadata(path) | |
| .map(|metadata| metadata.is_dir()) | |
| .unwrap_or(false) | |
| { | |
| return Err(io::Error::new( | |
| io::ErrorKind::IsADirectory, | |
| format!("destination is a directory: {}", path.display()), | |
| )); | |
| } | |
| match std::fs::rename(path, &backup) { |
| // destination is absent remains, but never one where no copy of | ||
| // the data exists. | ||
| let backup = unique_sibling_path(path, "bak"); | ||
| match std::fs::rename(path, &backup) { |
There was a problem hiding this comment.
P2: A daemon crash or forced termination after this rename but before tmp -> path leaves Windows config.toml/server_envs.json absent, even though the old bytes remain in .bak. The next startup therefore loses the stored auth or environment state that this backup is intended to preserve; stale-backup recovery is needed before treating the canonical file as missing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/edison-stdiod/src/secure_file.rs, line 96:
<comment>A daemon crash or forced termination after this rename but before `tmp -> path` leaves Windows `config.toml`/`server_envs.json` absent, even though the old bytes remain in `.bak`. The next startup therefore loses the stored auth or environment state that this backup is intended to preserve; stale-backup recovery is needed before treating the canonical file as missing.</comment>
<file context>
@@ -81,14 +85,32 @@ fn replace(tmp: &Path, path: &Path) -> io::Result<()> {
+ // destination is absent remains, but never one where no copy of
+ // the data exists.
+ let backup = unique_sibling_path(path, "bak");
+ match std::fs::rename(path, &backup) {
Ok(()) => {}
- Err(error) if error.kind() == io::ErrorKind::NotFound => {}
</file context>
1 participant
Summary
needs_reauthandneeds_upgradestatesValidation
cargo test --workspace(95 tests)cargo clippy --workspace --all-targets -- -D warningscargo package -p edison-stdiod --allow-dirtyprekhooksSummary by cubic
Adds OAuth-style device authorization with PKCE for
edison-stdiod, enabling browser login/logout and scoped client credentials. Also hardens daemon/tunnel behavior, storage safety, and server diagnostics.New Features
loginopens the browser;logoutrevokes and clears local credentials./api/v1/client/...surface; legacy API-key flows remain compatible for add/list/remove where valid.needs_reauth/needs_upgradestates, and clean child process tree termination on auth changes or shutdown.Bug Fixes
token_typecase-insensitively.Written for commit e98d81f. Summary will update on new commits.