fix: merod auth

[frdomovic]

WIP

---

Note

Medium Risk
Touches client-side authentication, token refresh/re-auth retry behavior, and persistence of JWTs to disk; mistakes could cause login loops or accidental auth bypass/lockout. Changes are localized but affect all authenticated requests and CLI login UX.

Overview
Improves client/CLI authentication robustness and token persistence. The client connection code now reloads the auth header on every request attempt and retry, and on 401 it refreshes tokens or falls back to full re-authentication; non-2xx errors now surface a sanitized, structured message via a new extract_error_message helper (with tests).

Adjusts how meroctl discovers and stores credentials. Auth-mode detection now probes a protected endpoint instead of admin-api/health, the browser-login flow has a shorter (2 min) timeout and updated callback HTML, and authenticate_with_session_cache now persists/updates node entries (Local vs Remote) and their JWTs in config to avoid repeated browser prompts.

Hardens credential storage and fixes small UX/CLI behaviors. Config saves set Unix file permissions to 0600, MeroctlAuthenticator cloning preserves output wiring, context creation normalizes protocol to lowercase and defaults init params to {}, and application install-from-URL now fails fast with a truncated body when the registry returns non-success.

^{Written by Cursor Bugbot for commit 94743bd. This will update automatically on new commits. Configure here.}

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 3 agents | Quality score: 83% | Review time: 206.4s

🟡 3 warnings, 💡 2 suggestions, 📝 1 nitpicks. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-5bedf82c}

[meroreviewer]

🟡 404 response incorrectly treated as no-auth-required

Treating 404 as indicating no authentication is needed is incorrect; a protected endpoint returning 404 (e.g., no contexts exist) doesn't mean auth is disabled—it may still require auth for mutations.

Suggested fix:

Only treat 2xx responses without a prior 401 challenge as AuthMode::None; 404 should likely be treated as AuthMode::Required for safety. 

[meroreviewer]

💡 Tokens written to config without explicit permission check

JWT tokens are persisted to the config file; ensure the config file has restrictive permissions (e.g., 0600) to prevent token theft by other local users.

Suggested fix:

Verify that Config::save() sets restrictive file permissions, or add a comment confirming this is handled elsewhere. 

[meroreviewer]

🟡 Hidden side effect in authentication function

authenticate_with_session_cache now modifies Config as a side effect, violating single responsibility; callers expect authentication, not config persistence.

Suggested fix:

Extract node registration into a separate function or return the tokens and let the caller decide whether to persist to config. 

[meroreviewer]

💡 TOCTOU race when persisting node config

The check !config.nodes.contains_key(node_name) followed by insert and save is racy if multiple processes authenticate the same node concurrently, potentially causing one write to be lost.

Suggested fix:

Use file locking or an atomic compare-and-swap pattern when updating the config file. 

[meroreviewer]

📝 Nit: Error message includes response body verbatim

Including the full response body in error messages could leak sensitive information if the registry unexpectedly returns credentials or internal details in error responses.

Suggested fix:

Consider truncating the body or sanitizing it before including in the error message. 

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 3 agents | Quality score: 89% | Review time: 775.9s

🟡 1 warnings, 💡 3 suggestions, 📝 2 nitpicks. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-881c097a}

[meroreviewer]

🟡 Auth mode detection treats 404 as unauthenticated

Treating HTTP 404 as 'no auth required' could cause incorrect auth mode detection if a server returns 404 from an unregistered handler that doesn't check auth, while other endpoints do require it; however, the retry logic in execute_request_with_auth_retry should still handle 401 responses on actual requests.

Suggested fix:

Consider only treating 2xx responses as definitive 'no auth required' signals, or document that 404 is intentionally treated as no-auth due to retry fallback behavior. 

[meroreviewer]

💡 Full response body included in error message

Including the entire response body in error messages could inadvertently expose sensitive server-side information in logs or user-facing output.

Suggested fix:

Consider truncating the body similar to extract_error_message (300 chars) or sanitizing it before including in the error. 

[meroreviewer]

💡 JWT tokens persisted to config file for remote nodes

Storing JWT tokens (including refresh tokens) in the config file increases exposure if the file is compromised; ensure config file has restrictive permissions (0600) to limit access.

Suggested fix:

Verify that Config::save() applies restrictive file permissions, or add a comment acknowledging the security trade-off of persisting tokens. 

[meroreviewer]

💡 Truncated error message doesn't indicate truncation to user

When body exceeds 300 chars it's silently truncated; users may miss important context at the end.

Suggested fix:

Append an ellipsis or '[truncated]' when the body is cut off. 

[meroreviewer]

📝 Nit: Error body included in full without truncation

Unlike extract_error_message in connection.rs which truncates to 300 chars, the registry error body is included untruncated and could be large.

Suggested fix:

Truncate `body` to a reasonable limit (e.g., 500 chars) before including in the error message for consistency. 

[meroreviewer]

📝 Nit: unwrap after guard could use if-let for idiomatic Rust

Using unwrap() after an is_none() check is safe but an if-let binding would be clearer.

Suggested fix:

Use `let Some(node_name) = &self.node_name else { return Ok(None); };` or match expression. 

[cursor]

Dead code: NoOpOutputHandler is now unused

Low Severity

NoOpOutputHandler struct and its MeroctlOutputHandler impl are now dead code. The Clone impl for MeroctlAuthenticator was changed from Box::new(NoOpOutputHandler) to create_cli_authenticator(self.raw_output), which was the whole reason NoOpOutputHandler existed. No other code references it.

 

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 3 agents | Quality score: 96% | Review time: 486.4s

🟡 1 warnings, 💡 5 suggestions, 📝 2 nitpicks. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-91cdfc24}

[meroreviewer]

🟡 Auth bypass risk: 404 response treated as no-auth-required

Treating HTTP 404 the same as 200 in auth mode detection can cause the client to skip authentication when the probed endpoint doesn't exist but other endpoints still require auth.

Suggested fix:

Only treat 200 OK as no-auth-required; 404 should either indicate unknown auth status or trigger a fallback probe to another known endpoint. 

[meroreviewer]

💡 detect_auth_mode behavioral change removes localhost bypass

The removal of the localhost/127.0.0.1 early-return changes behavior: previously localhost was always AuthMode::None, now it probes the endpoint, which could break existing setups expecting local nodes to skip auth.

Suggested fix:

Ensure this is the intended behavior; if local-only nodes still need auth bypass, consider a config flag or document the migration. 

[meroreviewer]

💡 Config update has read-modify-write race window

Loading config, checking if node exists, then inserting and saving is not atomic; concurrent CLI sessions could overwrite each other's entries for the same node_name.

Suggested fix:

For a CLI tool this is low-risk; consider file locking or atomic config writes if concurrent usage becomes common. 

[meroreviewer]

💡 New helper function extract_error_message lacks unit tests

The extract_error_message function has multiple JSON parsing branches and edge cases that would benefit from unit tests to ensure correctness.

Suggested fix:

Add unit tests covering: empty body, valid JSON with nested error.message, JSON with flat error string, JSON with message field, invalid JSON, and body exceeding 300 chars. 

[meroreviewer]

💡 Config modification has implicit side effect without clear ownership

The function silently registers a node in global config if it doesn't exist, which is a side effect that may surprise callers expecting only session-level caching.

Suggested fix:

Consider documenting this behavior clearly in the function's doc comment or extracting the config registration to a separate explicit call. 

[meroreviewer]

💡 Error response body may leak sensitive server information

The extract_error_message function exposes up to 300 chars of server response body in error messages; if the server inadvertently includes internal details, they could be logged or displayed.

Suggested fix:

Consider only extracting known safe fields (error/message) and avoiding raw body fallback, or sanitize the output further. 

[meroreviewer]

📝 Nit: Inconsistent truncation between JSON messages and raw body

Raw body text is truncated to 300 chars, but JSON-extracted messages are returned without length limit, which could produce unexpectedly long error strings.

Suggested fix:

Consider applying the same 300-char truncation to JSON-extracted messages for consistency. 

[meroreviewer]

📝 Nit: Prefer if-let over unwrap after is_none check

Using unwrap() after verifying is_none() is safe but not idiomatic Rust; prefer if let Some(node_name) = &self.node_name { ... } for clarity.

Suggested fix:

Refactor to use `if let Some(node_name) = &self.node_name` pattern to avoid the unwrap. 

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 3 agents | Quality score: 95% | Review time: 376.4s

🟡 2 warnings, 💡 3 suggestions, 📝 2 nitpicks. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-57ddd7a3}

[meroreviewer]

🟡 TOCTOU race in persist_node_in_config can lose concurrent config updates

Between Config::load() and config.save(), another meroctl process could write to the config file, and this process would silently overwrite those changes; the comment acknowledges this but concurrent CLI usage can lose node registrations.

Suggested fix:

Consider using file locking (e.g., flock or advisory locks) when reading and writing the config file to prevent concurrent modification data loss. 

[meroreviewer]

🟡 Inconsistent error body handling may leak server internals

Unlike extract_error_message in connection.rs which carefully extracts only known-safe JSON fields, this code includes raw truncated response body in error messages, potentially exposing server-internal details from the registry.

Suggested fix:

Consider using a similar approach to `extract_error_message` that only extracts known-safe fields like `error` or `message` from JSON responses, falling back to just the HTTP status code. 

[meroreviewer]

💡 Extra storage round-trip after successful token refresh

After storing refreshed tokens, the loop continues and load_auth_header reads them back from storage on the next iteration; the freshly-obtained tokens could be passed directly to avoid the redundant I/O.

Suggested fix:

Return the new auth header string directly after token refresh instead of continuing and reloading from storage. 

[meroreviewer]

📝 Nit: extract_error_message discards status when message found

When a JSON error message is extracted, the HTTP status code is not included, which may reduce debuggability for operators.

Suggested fix:

Consider returning `format!("HTTP {}: {}", status.as_u16(), msg)` instead of just the message. 

[meroreviewer]

💡 DRY: Timeout duration and message are coupled but separate

The 2-minute timeout is hardcoded in both the user-facing message and Duration::from_secs(120), risking desync if one is changed.

Suggested fix:

Define a constant `const AUTH_TIMEOUT_SECS: u64 = 120;` and format the message dynamically, or place the message near the timeout value. 

[meroreviewer]

💡 Default init params change is undocumented at CLI level

Defaulting params to {} is a sensible fix but CLI help text should mention this default so users understand what's passed to WASM.

Suggested fix:

Update the `--params` help string to mention `(defaults to "{}" if omitted)`. 

[meroreviewer]

📝 Nit: Synchronous set_permissions in async function

std::fs::set_permissions is a blocking syscall used inside an async function; while chmod is typically fast, tokio::fs::set_permissions would be more consistent.

Suggested fix:

Replace std::fs::set_permissions with tokio::fs::set_permissions(...).await for consistency with the surrounding async I/O. 

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

^{Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

[cursor]

Persisted local node path is double-joined, breaking future lookups

High Severity

node_path is set to self.args.home.join(node) (e.g. ~/.calimero/mynode) and then passed to persist_node_in_config, which stores it as the path field in NodeConnection::Local. Later, get_connection passes that stored path to load_config(path, node), which internally does path.join(node_name) — producing ~/.calimero/mynode/mynode. This doubled path won't exist, so every subsequent CLI invocation using the auto-registered node will fail. The value passed as local_node_path needs to be self.args.home (the home directory), not self.args.home.join(node).

Additional Locations (1)

• crates/meroctl/src/auth.rs#L435-L440