feat: implement authenication#80
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:
📝 WalkthroughWalkthroughThis PR implements end-to-end password-based authentication with session management and at-rest encryption. It adds a backend auth module with Argon2id hashing and AES-256-GCM encryption, refactors the shared state structure, creates authentication endpoints, middleware, and frontend guards, and updates all API handlers to work with the new state shape. ChangesAuthentication System Implementation
Sequence DiagramsequenceDiagram
participant Client as Client / Browser
participant Frontend as Frontend Router
participant Auth as /auth Endpoints
participant Middleware as Auth Middleware
participant Handlers as API Handlers
participant Store as State (Sessions/Auth)
Note over Client,Store: First-run: Setup Phase
Client->>Frontend: GET / (first time)
Frontend->>Auth: GET /api/auth/status
Auth->>Store: Check auth initialized
Auth-->>Frontend: {initialized: false}
Frontend->>Frontend: Redirect to /setup
Client->>Frontend: POST /setup (new password)
Frontend->>Auth: POST /api/auth/setup {password}
Auth->>Store: Create AuthConfig, save to disk
Auth->>Store: Create session token
Auth-->>Frontend: 200 + session cookie
Frontend->>Frontend: Redirect to /
Note over Client,Store: Authenticated Request Flow
Client->>Frontend: GET / (with session cookie)
Frontend->>Auth: GET /api/auth/status (cookie sent)
Auth-->>Frontend: {initialized: true, authenticated: true}
Frontend->>Frontend: Render protected routes
Client->>Handlers: GET /api/makers (with cookie)
Handlers->>Middleware: Check session token
Middleware->>Store: Validate token in SessionStore
Middleware-->>Handlers: Token valid, proceed
Handlers-->>Client: 200 + maker list
Note over Client,Store: Login After Logout
Client->>Frontend: GET /login
Client->>Frontend: POST /login {password}
Frontend->>Auth: POST /api/auth/login {password}
Auth->>Store: Verify password vs AuthConfig
Auth->>Store: Create session token
Auth-->>Frontend: 200 + session cookie
Frontend->>Frontend: Redirect to /
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/maker_manager/persistence.rs (1)
215-264: 🧹 Nitpick | 🔵 TrivialLoad/migration logic looks correct; minor nit on error discrimination.
The envelope-vs-legacy disambiguation via
get("v")+get("data")/get("makers")is sound, and the in-place migration (lines 256–259) will auto-upgrade existing deployments on the first run after they configure a password. A legacy file is written once to encrypted form and then future loads take the encrypted branch.One minor note: an envelope with
{"v": 2, ...}written by a future version will be interpreted as "encrypted" here and fail onaes_decryptwith a generic error. Consider explicitly checkingv == 1and returning a clearer "unsupported envelope version" message to make forward-compat failures actionable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/maker_manager/persistence.rs` around lines 215 - 264, In load(), the branch that treats a JSON envelope as encrypted currently accepts any presence of "v" and "data" and then calls aes_decrypt; update the logic in the load method to explicitly read and validate that value["v"] == 1 before attempting base64 decode / aes_decrypt, and if v is present but not 1 return a clear error like "unsupported makers.json envelope version: <v>"; reference the load function, the checks on value.get("v")/value.get("data"), the enc_key usage, and the aes_decrypt call to locate where to add the explicit version check and error.frontend/app/api.ts (1)
294-303:⚠️ Potential issue | 🟡 MinorSSE stream does not handle 401 / session expiry.
streamLogsbypassesrequest(), so if the session expires while a stream is open (or if the cookie is absent on reconnect), the server will reply 401 andEventSourcewill silently error-loop without redirecting to/login. Consider surfacing an auth-specific failure by closing the stream on 401 and redirecting, e.g. by triggeringauth.status()from insideonErrorand callingwindow.location.href = "/login"when it resolves to{ authenticated: false }.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api.ts` around lines 294 - 303, streamLogs creates an EventSource directly and doesn't handle 401/session expiry, causing silent reconnect loops; modify streamLogs to detect auth failures by wiring es.onerror to inspect the EventSource.readyState or response status (via the error event), call auth.status() (or the app's auth check) inside the onerror handler and if it resolves to { authenticated: false } close the EventSource and redirect window.location.href = "/login"; keep the existing onLine callback behavior and still call the provided onError if present after handling auth-specific logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker/docker-compose.yml`:
- Around line 79-81: The compose file currently supplies a weak default for
DASHBOARD_PASSWORD using `${DASHBOARD_PASSWORD:-coinswap}` which undermines the
"required" comment and conflicts with docker/.env.example; change the
DASHBOARD_PASSWORD entry in docker-compose.yml to require the variable (no
default) so Compose will fail if not provided (use the mandatory variable
syntax) and update or ensure docker/.env.example matches (e.g., document the
required variable) and keep in mind DASHBOARD_ALLOW_REMOTE remains true so the
password must be required before startup.
In `@frontend/app/main.tsx`:
- Line 37: The current auth check returns null (if (checking) return null;)
which causes a blank-screen flicker; replace that early return with rendering a
lightweight centered loading indicator matching the login aesthetic (e.g., a
small Spinner/Loading component or a simple centered div with animated ellipsis)
so the UI shows a spinner while auth.status() is in flight; keep the same
redirect/logic flow (only change what renders when checking is true) and update
references in main.tsx around the checking variable and the auth.status() call
to use the spinner component.
- Around line 20-40: AuthLayout's useEffect must guard against component unmount
and React StrictMode double-invocation: wrap the auth.status() call so it
records whether the request was already started (use a startedRef) and use a
cancelled/isMounted flag that the effect cleanup sets to true; after the
auth.status() promise resolves or rejects, only call setAuthenticated and
setChecking if not cancelled. Update the useEffect in AuthLayout to declare e.g.
const startedRef = useRef(false) and let cancelled = false (or use a ref), skip
starting a second request if startedRef.current is true, set startedRef.current
= true when initiating auth.status(), and return a cleanup that sets
cancelled/flag to true so the .then/.catch handlers check the flag before
calling setAuthenticated/setChecking. Ensure all references to auth.status,
setAuthenticated, and setChecking are guarded.
In `@frontend/app/routes/login.tsx`:
- Line 17: The login route currently calls navigate("/") unconditionally;
instead read the pre-login destination passed via React Router state (e.g.,
location.state?.from) and navigate there after successful login. Update the
login handler to accept/use the location object (or use useLocation()) and call
navigate(location?.state?.from ?? "/", { replace: true }) so users are returned
to the originally requested route that AuthLayout may have stored.
- Around line 63-67: The error banner rendered in the Login route when "error"
is truthy is not announced to assistive tech; update the div that wraps {error}
in frontend/app/routes/login.tsx (the conditional block rendering the error
message) to include an accessibility live region—e.g., add role="alert" (or
aria-live="polite") so screen readers announce the message when it appears,
keeping the existing styling and markup otherwise.
In `@packaging/umbrel/docker-compose.yml`:
- Line 33: The compose entry currently injects DASHBOARD_PASSWORD without a
guard; update the docker-compose variable from plain ${DASHBOARD_PASSWORD} to a
strict substitution (e.g., use the parameter expansion that fails if empty, like
${DASHBOARD_PASSWORD:?DASHBOARD_PASSWORD must be set}) so composition fails fast
when DASHBOARD_PASSWORD is not provided, or alternatively switch to using
DASHBOARD_PASSWORD_FILE (backed by an Umbrel-managed secret) to supply the
password; target the DASHBOARD_PASSWORD / DASHBOARD_PASSWORD_FILE variable in
the compose service and ensure src/main.rs’s existing check (if !p.is_empty())
remains compatible.
In `@SECURITY.md`:
- Around line 40-42: The documentation points out that re-encrypting makers.json
after a password change is manual and error-prone; create a tracked follow-up to
implement an atomic in-dashboard password-rotation flow that re-encrypts
makers.json and updates auth.json. In the follow-up issue describe the problem,
propose an endpoint (e.g., POST /api/auth/rotate-password) that accepts
oldPassword/newPassword, validates the old password, decrypts makers.json,
re-encrypts it with the new password, atomically writes the new makers.json and
updated auth.json, handles failures with rollback, and include tests and
migration notes for fidelity-bond associations and UX messaging in the settings
UI.
In `@src/api/auth.rs`:
- Around line 28-56: The login handler (async fn login) lacks auditing and rate
limiting; add per-client rate limiting and failed-attempt logging by: obtain
client IP/peer info (e.g., via axum's ConnectInfo or request headers) at the
start of login, plug a shared RateLimiter (new struct or extend
SessionStore/SessionRateLimiter behind State<Arc<Mutex<...>>>) and
check/increment the counter before password verification, return a short
cooldown response when threshold exceeded, and on Ok(false) call tracing::warn!
with the client IP, timestamp, and increment the failure counter; on Ok(true)
reset that client's counter and proceed to call sessions.lock().await.create()
as before, and on Err(e) emit an audit error log (tracing::error!) with
e.to_string() so failed verifications are recorded.
- Around line 33-55: Replace the hand-rolled json! envelopes in the login,
logout, and status handlers with the typed ApiResponse<T> from super::dto
(import ApiResponse) and use ApiResponse::<()>::ok(()) on success and
ApiResponse::<()>::err(msg) for errors; update the Json responses to wrap
ApiResponse (e.g., Json(ApiResponse::ok(...)) or Json(ApiResponse::err(...))).
Also add the Secure attribute to cookie strings created where
sessions.lock().await.create() is used (the cookie format that currently sets
HttpOnly; SameSite=Strict; Path=/; Max-Age=86400 should include Secure), and
ensure header::SET_COOKIE is still used for setting the cookie header.
- Around line 36-37: The session cookie and its clearing counterpart are missing
the Secure attribute; update the string construction for the session cookie (the
assignment to the cookie variable created with format!("session={token}; ..."))
and the clear-cookie string used when logging out to include "; Secure" so the
cookie is only sent over TLS. Locate the session cookie construction and the
logout/clear-cookie construction in src/api/auth.rs (the format! calls that
build "session=...; HttpOnly; SameSite=Strict; Path=/; Max-Age=...") and append
" Secure" as an attribute to both cookie strings.
In `@src/api/mod.rs`:
- Line 131: The OpenAPI spec is missing the auth endpoints merged via
auth::routes(); add documentation for the handlers (login, logout, status) by
annotating each handler in auth.rs with #[utoipa::path(...)] (or adding
#[utoipa::path] variants) including request/response schemas, then include those
functions in the existing #[openapi(paths(...))] attribute used by the router
(e.g., add auth::login, auth::logout, auth::status to the paths list) so the
Swagger UI and generated OpenAPI document include the auth endpoints and their
schemas.
In `@src/auth.rs`:
- Around line 127-133: In validate, after calling self.sessions.retain(|_,
expiry| *expiry > now), replace the final get(...).map_or(false, |expiry|
*expiry > now) with a simple self.sessions.contains_key(token) since retain
already removed expired entries; this simplifies the logic in the validate
method and removes the redundant expiry check while keeping the retain call and
Instant::now() as-is.
- Around line 62-74: The save function currently writes auth.json with default
permissions and then tightens them, and it writes non-atomically; fix save by
creating/writing a temporary sibling file with OpenOptions set to mode(0o600)
(use std::fs::OpenOptions::mode on Unix) so the file is created with owner-only
permissions up front, write the serialized JSON to that temp file, flush and
sync it, then atomically rename/replace the target auth.json with
std::fs::rename (or std::fs::replace on platforms that support it); ensure
errors propagate from write/sync/rename and remove any leftover temp on failure.
- Around line 68-71: Gate the Unix-specific permission-hardening block that uses
std::os::unix::fs::PermissionsExt with #[cfg(unix)] and add a brief doc comment
that the owner-only permission change is Unix-only; alternatively implement a
Windows branch (e.g., using Windows ACL APIs) behind #[cfg(windows)] if you want
parity. Specifically wrap the lines that call std::fs::metadata,
PermissionsExt::set_mode, and std::fs::set_permissions in the #[cfg(unix)]
conditional (or move them into a helper like set_owner_only_permissions() and
provide platform-specific impls) so the crate builds on Windows CI.
In `@src/main.rs`:
- Around line 37-38: The code currently reads DASHBOARD_PASSWORD directly into
the password variable; change the logic in main to prefer
DASHBOARD_PASSWORD_FILE (read file contents and trim) and only fall back to
DASHBOARD_PASSWORD for dev/testing, and when that fallback is used emit a
tracing::warn! including the env var name and a short risk note; update any
password-loading helper (refer to the password variable and the env lookup for
"DASHBOARD_PASSWORD") to first check "DASHBOARD_PASSWORD_FILE", read/trim the
file, then fallback to "DASHBOARD_PASSWORD" while warning, and add a README note
documenting the recommended _FILE variant for secrets.
- Around line 37-60: The code block that reads DASHBOARD_PASSWORD or
DASHBOARD_PASSWORD_FILE needs two fixes: run cargo fmt on src/main.rs to satisfy
formatting checks, and add a guard when reading DASHBOARD_PASSWORD_FILE to treat
an empty path as an unset/invalid value before calling std::fs::read_to_string;
update the logic around std::env::var("DASHBOARD_PASSWORD_FILE") /
read_to_string(&path) (the password binding and error branches) to check if
path.trim().is_empty() and emit the same "No password set" / clear error message
(and exit) rather than calling read_to_string on an empty path, ensuring
tracing::error messages remain informative.
In `@src/maker_manager/persistence.rs`:
- Around line 190-212: The save path in persistence.rs is non-atomic and applies
perms after writing; modify the method that writes state_file() (the block using
fs::write and the enc_key branch) to write to a temporary file on the same
directory (e.g. path.with_extension(".tmp") or similar), create the temp with
OpenOptions::new().write(true).create_new(true).mode(0o600) (or
set_permissions(0o600) on the temp before rename) so the file is created with
0o600, write the JSON or encrypted envelope into that temp, flush and sync
(where reasonable), then atomically rename (fs::rename) the temp to path; retain
the existing context() error messages around encryption/serialization/write and
keep the final metadata/perms step only if needed for non-Unix platforms or as a
fallback.
In `@src/server.rs`:
- Around line 68-89: Add a defensive check in Server::new() to reject an empty
password in the provided ServerConfig before proceeding to load/create
AuthConfig: inspect config.password (e.g., trim and test is_empty) at the top of
Server::new() and return an error (use anyhow::bail! or Err with a clear message
like "empty dashboard password not allowed") if empty; do this prior to calling
AuthConfig::load / AuthConfig::new and before derive_key so the function fails
fast when given an invalid ServerConfig.
In `@tests/integration_test.rs`:
- Line 143: The test fails because ApiClient creates a ureq::Agent without
cookie support and never authenticates, so protected /api/* endpoints return
401; update the ApiClient (the constructor or new() used in
tests/integration_test.rs and any helper methods used by test_maker_integration)
to build a ureq::Agent with cookie store enabled and perform a POST to
/api/auth/login with the test credentials (password "integration-test-password")
to establish a session before calling /api/health, /api/makers,
/api/makers/{id}/balance, etc.; ensure subsequent requests reuse that agent so
the session cookie is sent.
---
Outside diff comments:
In `@frontend/app/api.ts`:
- Around line 294-303: streamLogs creates an EventSource directly and doesn't
handle 401/session expiry, causing silent reconnect loops; modify streamLogs to
detect auth failures by wiring es.onerror to inspect the EventSource.readyState
or response status (via the error event), call auth.status() (or the app's auth
check) inside the onerror handler and if it resolves to { authenticated: false }
close the EventSource and redirect window.location.href = "/login"; keep the
existing onLine callback behavior and still call the provided onError if present
after handling auth-specific logic.
In `@src/maker_manager/persistence.rs`:
- Around line 215-264: In load(), the branch that treats a JSON envelope as
encrypted currently accepts any presence of "v" and "data" and then calls
aes_decrypt; update the logic in the load method to explicitly read and validate
that value["v"] == 1 before attempting base64 decode / aes_decrypt, and if v is
present but not 1 return a clear error like "unsupported makers.json envelope
version: <v>"; reference the load function, the checks on
value.get("v")/value.get("data"), the enc_key usage, and the aes_decrypt call to
locate where to add the explicit version check and error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4a6482b5-d560-41c1-9395-8c44200af6b7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
.gitignoreCargo.tomlREADME.mdSECURITY.mddocker/.env.exampledocker/docker-compose.ymlfrontend/app/api.tsfrontend/app/main.tsxfrontend/app/routes/login.tsxpackaging/umbrel/docker-compose.ymlsrc/api/auth.rssrc/api/bitcoind.rssrc/api/fidelity.rssrc/api/makers.rssrc/api/mod.rssrc/api/monitoring.rssrc/api/wallet.rssrc/auth.rssrc/lib.rssrc/main.rssrc/maker_manager/mod.rssrc/maker_manager/persistence.rssrc/middlewares/mod.rssrc/server.rstests/api/mod.rstests/integration_test.rs
| {error && ( | ||
| <div className="px-3 py-2 bg-red-900/40 border border-red-700 rounded-lg text-sm text-red-300 animate-fade-in"> | ||
| {error} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Error banner is not announced to assistive tech.
The error container has no role="alert" / aria-live, so screen-reader users submitting the form won't be notified when the password is rejected. Add role="alert" (or aria-live="polite") so the message is announced when it appears.
♿ Proposed fix
- <div className="px-3 py-2 bg-red-900/40 border border-red-700 rounded-lg text-sm text-red-300 animate-fade-in">
+ <div
+ role="alert"
+ className="px-3 py-2 bg-red-900/40 border border-red-700 rounded-lg text-sm text-red-300 animate-fade-in"
+ >
{error}
</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {error && ( | |
| <div className="px-3 py-2 bg-red-900/40 border border-red-700 rounded-lg text-sm text-red-300 animate-fade-in"> | |
| {error} | |
| </div> | |
| )} | |
| {error && ( | |
| <div | |
| role="alert" | |
| className="px-3 py-2 bg-red-900/40 border border-red-700 rounded-lg text-sm text-red-300 animate-fade-in" | |
| > | |
| {error} | |
| </div> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/routes/login.tsx` around lines 63 - 67, The error banner
rendered in the Login route when "error" is truthy is not announced to assistive
tech; update the div that wraps {error} in frontend/app/routes/login.tsx (the
conditional block rendering the error message) to include an accessibility live
region—e.g., add role="alert" (or aria-live="polite") so screen readers announce
the message when it appears, keeping the existing styling and markup otherwise.
hulxv
force-pushed
the
feat/authenication
branch
from
0302896 to
9c475d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/api/monitoring.rs (1)
57-78:⚠️ Potential issue | 🔴 CriticalCritical:
MutexGuardheld across.await— systemic pattern across multiple API handlers. This serializes all concurrent requests.Every
state.lock().await.<async_method>(...).awaitchain holds the managerMutexGuardfor the full statement, blocking the inter-process round-trip while the global lock is held:In
src/api/monitoring.rs:
- L67
state.lock().await.ping(&id).await.is_ok()(get_status)- L105
state.lock().await.get_swap_utxos(&id).await(get_swaps)- L120
state.lock().await.get_swept_swap_utxos(&id).await(get_swaps)- L561
state.lock().await.get_tor_address(&id).await(get_tor_address)- L600
state.lock().await.get_data_dir(&id).await(get_data_dir)This pattern also appears in:
src/api/wallet.rs(L50, L96, L189, L226, L266, L313, L360)src/api/mod.rs(L160)src/api/fidelity.rs(L40)All underlying methods (
ping,get_swap_utxos,get_swept_swap_utxos,get_tor_address,get_data_dir) are async and delegate to the shared maker pool via IPC. A single hung or slow maker will stall every other/api/*handler that needs the manager.Compare with correct patterns in
get_logs,get_logs_download,get_logs_stream,get_rpc_status, andget_swap_reports, which scope the guard:let manager = state.lock().await; ...; drop(manager); /* then await */.Additionally,
get_status(lines 61/67/68) takes the lock three separate times, leaving a TOCTOU window where the maker can be removed betweenhas_makerandping/is_server_running.The structural fix requires
MakerManager/MakerPoolto expose per-maker handles (cloneable channels) so IPC occurs without holding the manager mutex. As an immediate mitigation forget_status:♻️ Mitigation sketch
async fn get_status( State(state): State<Arc<Mutex<MakerManager>>>, Path(id): Path<String>, ) -> (StatusCode, Json<ApiResponse<MakerStatus>>) { - if !state.lock().await.has_maker(&id) { - return ( - StatusCode::NOT_FOUND, - Json(ApiResponse::err(format!("Maker '{id}' not found"))), - ); - } - let alive = state.lock().await.ping(&id).await.is_ok(); - let is_server_running = state.lock().await.is_server_running(&id); + let is_server_running = { + let mut mgr = state.lock().await; + if !mgr.has_maker(&id) { + return ( + StatusCode::NOT_FOUND, + Json(ApiResponse::err(format!("Maker '{id}' not found"))), + ); + } + mgr.is_server_running(&id) + }; + // TODO: ping should not require holding the manager lock; expose a + // per-maker handle from MakerPool so the IPC await happens lock-free. + let alive = state.lock().await.ping(&id).await.is_ok();Apply equivalent scoping to all call sites across all API handler files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/monitoring.rs` around lines 57 - 78, get_status currently holds the MakerManager MutexGuard across async calls (state.lock().await.ping(...).await) and takes the lock multiple times creating a TOCTOU window; change it to acquire the lock once, check has_maker, obtain a cloneable per-maker handle or the minimal data needed (e.g., call a new/get_handle method or clone a channel/reference for maker identified by id), then drop the guard before performing any async await (call ping() on the cloned handle and await it, call is_server_running on either the cloned handle or by copying a boolean under the lock), and finally build the ApiResponse; update MakerManager to expose a cloneable per-maker handle (or get_handle/cloneable channel) so ping/get_swap_utxos/get_swept_swap_utxos/get_tor_address/get_data_dir calls never await while holding the manager lock, and apply this scoping pattern to all other handlers referencing state.lock().await.<async_method>.src/api/fidelity.rs (1)
30-54:⚠️ Potential issue | 🔴 CriticalCritical:
MutexGuardheld across.awaitserializes all manager-backed requests.
state.lock().await.list_fidelity(&id).awaitkeeps thetokio::sync::MutexGuardalive for the full statement, so the inter-process round-trip to the maker happens while the globalMakerManagermutex is held. Every other/api/*handler that locks the same manager (status, logs, ping, balances, utxos, address generation, etc.) will queue behind this single in-flight request. A slow or hung maker can stall the entire dashboard.This pattern is pervasive across
wallet.rs,fidelity.rs, and parts ofmonitoring.rs. Acquire what you need (e.g., a clone of the per-maker channel/handle from the pool, or a precondition check), drop the guard before the inner.await, and only then perform the async IPC. Some handlers inmonitoring.rsalready use the correct pattern—bind the guard to a variable and drop it before any async call.♻️ Sketch
- if !state.lock().await.has_maker(&id) { - return ( - StatusCode::NOT_FOUND, - Json(ApiResponse::err(format!("Maker '{id}' not found"))), - ); - } - match state.lock().await.list_fidelity(&id).await { + { + let mgr = state.lock().await; + if !mgr.has_maker(&id) { + return ( + StatusCode::NOT_FOUND, + Json(ApiResponse::err(format!("Maker '{id}' not found"))), + ); + } + } + match state.lock().await.list_fidelity(&id).await {The above only narrows the existence check; the real fix requires letting the IPC await happen without holding the manager lock (e.g., by exposing a per-maker handle from
MakerPoolor refactoringlist_fidelityto accept a pre-fetched handle).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/fidelity.rs` around lines 30 - 54, In list_fidelity, avoid holding the Arc<Mutex<MakerManager>> guard across the async IPC call: first lock the manager to run has_maker and retrieve or clone a per-maker handle (or the specific channel/handle used to talk to the maker) from MakerManager, then drop the MutexGuard before calling the async maker IPC (i.e., do not call state.lock().await.list_fidelity(&id).await while the guard is live). Concretely, use State(state): State<Arc<Mutex<MakerManager>>> to acquire a short-lived guard, call has_maker(&id) and get a clone of the maker handle (or a reference you can move out) from MakerManager, release the guard, then call the manager’s list_fidelity via that cloned handle (or a refactored list_fidelity that accepts the handle) and await the result; update error handling to match the existing match arms.src/api/bitcoind.rs (1)
100-119:⚠️ Potential issue | 🟡 Minor
start_bitcoindis invoked while holding the async mutex.Line 105 awaits
state.lock()and then callsstart_bitcoind(body.network)with the guard still alive across the call. Ifstart_bitcoindperforms synchronous child-spawn / fs / network work (it spawnsbitcoind), every other/api/*handler that touchesMakerManager(makers, monitoring, wallet, fidelity, bitcoind/status, bitcoind/stop) will queue behind it. Worse, a panic insidestart_bitcoindwould poison the lock semantics for the rest of the request lifecycle.Prefer the same pattern used in
stop: pull the minimum data needed out of the manager under the lock, drop the guard, then run the blocking startup insidetokio::task::spawn_blocking(or make the manager method itself take&self+ interior synchronization for the child handle).♻️ Sketch
- let network = body.network.clone(); - match state.lock().await.start_bitcoind(body.network) { + let network = body.network.clone(); + let state_cloned = state.clone(); + let result = tokio::task::spawn_blocking(move || { + // Lock is acquired+released inside the blocking thread; consider a std::sync::Mutex + // on the child handle inside MakerManager so this doesn't need the async mutex at all. + futures::executor::block_on(state_cloned.lock()).start_bitcoind(body.network) + }) + .await + .unwrap_or_else(|e| Err(anyhow::anyhow!("spawn_blocking panicked: {e}"))); + match result {(The cleaner refactor is to make
MakerManager::start_bitcoindnot require the async mutex by storing the child behind its ownstd::sync::Mutex, mirroring howtake_bitcoindis already used.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/bitcoind.rs` around lines 100 - 119, The start handler currently calls state.lock().await.start_bitcoind(...) while holding the async Mutex guard (in start), which can block other handlers and poison the lock; change it to take only the minimal data under the lock (e.g., clone body.network or extract any needed flags) and drop the guard before invoking the blocking startup; then run the blocking work using tokio::task::spawn_blocking (or call a new Manager method that uses interior std::sync::Mutex for the child handle) so that MakerManager::start_bitcoind is executed off the async mutex and does not hold the guard across child spawn/FS/network operations.
♻️ Duplicate comments (12)
src/auth.rs (1)
146-152: 🧹 Nitpick | 🔵 TrivialRedundant expiry check after
retain.
self.sessions.retain(|_, expiry| *expiry > now)already drops every expired entry, somap_or(false, |expiry| *expiry > now)is equivalent tocontains_key(token).♻️ Proposed simplification
pub fn validate(&mut self, token: &str) -> bool { let now = Instant::now(); self.sessions.retain(|_, expiry| *expiry > now); - self.sessions - .get(token) - .map_or(false, |expiry| *expiry > now) + self.sessions.contains_key(token) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auth.rs` around lines 146 - 152, In validate, after calling self.sessions.retain(|_, expiry| *expiry > now) the subsequent map_or check is redundant; replace the map_or(...) expression with a simple self.sessions.contains_key(token) so validate returns whether the token is present (keep the function signature validate(&mut self, token: &str) and the retain call on self.sessions).frontend/app/main.tsx (2)
24-35:⚠️ Potential issue | 🟡 MinorEffect still lacks an unmount/StrictMode cancellation guard.
Same concern raised in a past review still applies: in React 19.2 StrictMode (enabled at line 43), the effect runs twice on mount in dev, firing two
auth.status()requests; and ifAuthLayoutunmounts before the request resolves (e.g. fast navigation),setChecking/setAuthenticatedwill run on an unmounted component. Add a cancellation flag in the cleanup.♻️ Proposed fix
useEffect(() => { + let cancelled = false; auth .status() - .then(({ authenticated }) => { + .then(({ authenticated }) => { + if (cancelled) return; setAuthenticated(authenticated); setChecking(false); }) - .catch(() => { + .catch(() => { + if (cancelled) return; setAuthenticated(false); setChecking(false); }); + return () => { + cancelled = true; + }; }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/main.tsx` around lines 24 - 35, The effect in AuthLayout calling auth.status() can update state after unmount (and runs twice under StrictMode); add a cancellation guard in the useEffect cleanup: create a local boolean (e.g. let cancelled = false) or an AbortController before calling auth.status(), and in the promise resolution/rejection check the guard (e.g. if (cancelled) return) before calling setAuthenticated and setChecking; mark cancelled = true in the cleanup returned by useEffect so setAuthenticated/setChecking are never called on an unmounted AuthLayout.
37-37: 🧹 Nitpick | 🔵 TrivialBlank-screen flicker during the initial auth check is still unresolved.
return nullwhileauth.status()is in flight produces a momentary blank page on every full page load, which is jarring next to the styled/loginroute. Render a lightweight centered spinner (or even the same skeleton the rest of the app uses) instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/main.tsx` at line 37, The current early return "if (checking) return null;" in main.tsx causes a blank-screen flicker during auth.status() checks; replace that return with rendering the app's lightweight centered loading UI (e.g., the shared Spinner or LoadingSkeleton component) so users see a styled centered spinner/skeleton while "checking" is true; update the main component where "checking" and auth.status() are used to import and render the existing Spinner/LoadingSkeleton (or create a minimal centered spinner markup) instead of returning null so the initial auth check matches the styled /login experience.packaging/umbrel/docker-compose.yml (1)
33-33:⚠️ Potential issue | 🟡 MinorFail fast when
DASHBOARD_PASSWORDis not provided.
${DASHBOARD_PASSWORD}silently expands to an empty string if the variable is missing, which then has to be detected at runtime bysrc/main.rs. Use the strict-substitution form so Compose refuses to start a misconfigured Umbrel install up front.🛡️ Proposed fix
- DASHBOARD_PASSWORD: ${DASHBOARD_PASSWORD} + DASHBOARD_PASSWORD: ${DASHBOARD_PASSWORD:?DASHBOARD_PASSWORD must be set for Umbrel installation}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packaging/umbrel/docker-compose.yml` at line 33, Replace the permissive env substitution for DASHBOARD_PASSWORD with Docker Compose's strict/substitution form so the container startup fails if the variable is missing; specifically update the environment entry that currently uses DASHBOARD_PASSWORD: ${DASHBOARD_PASSWORD} to use the strict form (e.g., DASHBOARD_PASSWORD: ${DASHBOARD_PASSWORD:?DASHBOARD_PASSWORD must be set}) so Compose refuses to start on misconfiguration.docker/docker-compose.yml (1)
76-81:⚠️ Potential issue | 🟠 MajorRemove the hardcoded
coinswapfallback — remote-accessible service is shipping with a public default password.
DASHBOARD_ALLOW_REMOTE=trueis set on line 76 and the password defaults to a value (coinswap) that is searchable in this public repository. An operator who runsdocker compose upwithout populating.envends up with a remotely reachable dashboard protected by a known password. The inline comment marks the variable as "Required" but:-silently provides a default instead of failing — anddocker/.env.exampleuseschangeme, so the documented setup also diverges from the in-compose fallback.🛡️ Proposed fix
- DASHBOARD_NO_COLOR=true # Required: set in a .env file or export in your shell before running. # Example .env entry: DASHBOARD_PASSWORD=yourpassword - - DASHBOARD_PASSWORD=${DASHBOARD_PASSWORD:-coinswap} + - DASHBOARD_PASSWORD=${DASHBOARD_PASSWORD:?DASHBOARD_PASSWORD must be set (see .env.example)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/docker-compose.yml` around lines 76 - 81, Remove the hardcoded fallback for DASHBOARD_PASSWORD in the docker-compose.yml so the service cannot start with a public default; specifically, replace the usage of DASHBOARD_PASSWORD=${DASHBOARD_PASSWORD:-coinswap} with a required/empty reference (e.g., ${DASHBOARD_PASSWORD} or a shell-style required parameter) so compose will not silently use "coinswap", and ensure this aligns with the documented .env example (DASHBOARD_PASSWORD=changeme) and the remote enabling flag DASHBOARD_ALLOW_REMOTE remains deliberate.src/server.rs (1)
68-89: 🧹 Nitpick | 🔵 TrivialOptional: defensive non-empty password check.
ServerConfig::passworddefaults to""andServer::newwill happily callAuthConfig::new(&"")on first run ifmain.rsvalidation is bypassed (e.g., a future caller, integration test, or refactor that constructsServerConfigdirectly). A two-line guard at the top ofServer::newwould prevent silently provisioning anauth.jsonwhose hash matches the empty string.pub fn new(config: ServerConfig) -> anyhow::Result<Self> { use crate::auth::{AuthConfig, SessionStore}; + + if config.password.is_empty() { + anyhow::bail!("Dashboard password is required (set DASHBOARD_PASSWORD or DASHBOARD_PASSWORD_FILE)"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.rs` around lines 68 - 89, Add a defensive check in Server::new that validates ServerConfig::password is non-empty before attempting to load/create auth config: if the password is empty, return an error (anyhow::bail or appropriate Result::Err) rather than calling AuthConfig::new or AuthConfig::load; this prevents creating an auth.json whose hash matches the empty string and ensures derive_key is only called with a real password.src/api/mod.rs (1)
56-131: 🧹 Nitpick | 🔵 TrivialAuth routes still absent from the OpenAPI spec.
auth::routes()is merged at line 131, butpaths(...)(lines 56-90) does not includeauth::login,auth::logout, orauth::status, andcomponents(schemas(...))(lines 91-110) lacks anyLoginRequest/AuthStatusschemas. As a result, the Swagger UI shipped at/swagger-ui/and the publishedopenapi.jsonwill not document the new authentication boundary — a notable gap given that this is the only way clients discover the contract.onboarding::run_startup_check, + auth::login, + auth::logout, + auth::status, health_check, ),(plus corresponding
#[utoipa::path(...)]annotations and request/response schema registrations insrc/api/auth.rs.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/mod.rs` around lines 56 - 131, The OpenAPI spec is missing the auth endpoints and schemas: add auth::login, auth::logout and auth::status to the paths(...) list and register the corresponding DTO schemas (e.g., LoginRequest, LoginResponse or AuthStatus) in components(schemas(...)) so the Swagger UI and openapi.json include the auth contract; also ensure the handlers in src/api/auth.rs have matching #[utoipa::path(...)] annotations for each route (login/logout/status) and the DTO structs used there are annotated with #[derive(utoipa::ToSchema)] or equivalent to expose their request/response shapes.src/api/auth.rs (3)
33-55:⚠️ Potential issue | 🟠 MajorCoding-guideline violation: hand-rolled JSON envelopes in
login.
Json(json!({"success": ..., ...}))constructs the response envelope inline rather than using the typedApiResponse<T>defined insrc/api/dto.rs, while every other handler insrc/api/*usesApiResponse::ok(...)/ApiResponse::err(...). This drifts from the contract that the frontend (request<T>infrontend/app/api.ts) parses asApiResponse<T>, and a future schema change to the envelope (e.g., a newcodefield) would silently miss these three endpoints.-use serde_json::json; +// remove serde_json::json import ... - match auth.verify(&body.password) { + use super::dto::ApiResponse; + match auth.verify(&body.password) { Ok(true) => { let token = sessions.lock().await.create(); let cookie = - format!("session={token}; HttpOnly; SameSite=Strict; Path=/; Max-Age=86400"); + format!("session={token}; HttpOnly; Secure; SameSite=Strict; Path=/; Max-Age=86400"); ( StatusCode::OK, [(header::SET_COOKIE, cookie)], - Json(json!({"success": true, "data": null})), + Json(ApiResponse::<()>::ok(())), ) .into_response() } Ok(false) => ( StatusCode::UNAUTHORIZED, - Json(json!({"success": false, "error": "Invalid password"})), + Json(ApiResponse::<()>::err("Invalid password".to_string())), ) .into_response(), Err(e) => ( StatusCode::INTERNAL_SERVER_ERROR, - Json(json!({"success": false, "error": e.to_string()})), + Json(ApiResponse::<()>::err(e.to_string())), ) .into_response(), }Apply the analogous change to
logout(line 69) andstatus(line 82). As per coding guidelines: "All HTTP API endpoints must returnApiResponse<T>shape withsuccessboolean and eitherdataorerrorfield (defined insrc/api/dto.rs)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/auth.rs` around lines 33 - 55, The login handler is constructing JSON envelopes by hand; replace those inline Json(json!(...)) responses with the typed ApiResponse wrappers used elsewhere: return ApiResponse::ok(...) on success and ApiResponse::err(...) on failures (use the appropriate type parameter for empty data, e.g., unit/None), while preserving the Set-Cookie header and status codes (StatusCode::OK, UNAUTHORIZED, INTERNAL_SERVER_ERROR); apply the same change to the logout and status handlers so all three endpoints use ApiResponse::ok / ApiResponse::err instead of hand-rolled Json objects.
36-37:⚠️ Potential issue | 🟠 MajorSession cookie still missing
Secure.Both the login
Set-Cookie(line 36-37) and the logout clear-cookie (line 65) omitSecure, so the cookie is sent in cleartext if the dashboard is reached via plain HTTP — exactly the failure mode described inSECURITY.md/README.mdwhen a--allow-remotedeployment sits behind a misconfigured proxy. As of 2026, all current major browsers acceptSecurecookies onhttp://localhost, so there is no localhost-development cost to setting it unconditionally.- let cookie = - format!("session={token}; HttpOnly; SameSite=Strict; Path=/; Max-Age=86400"); + let cookie = + format!("session={token}; HttpOnly; Secure; SameSite=Strict; Path=/; Max-Age=86400"); ... - let clear_cookie = "session=; HttpOnly; SameSite=Strict; Path=/; Max-Age=0"; + let clear_cookie = "session=; HttpOnly; Secure; SameSite=Strict; Path=/; Max-Age=0";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/auth.rs` around lines 36 - 37, The session Set-Cookie string (variable "cookie" in auth.rs) and the logout clear-cookie should include the Secure attribute so the browser only sends the cookie over HTTPS; update the format strings that build the cookie (the login "cookie" format and the logout clear-cookie string) to append "; Secure" (e.g., "...; Path=/; Max-Age=86400; Secure") and similarly include "; Secure" on the clear-cookie response so both issuance and clearing are always Secure.
28-56:⚠️ Potential issue | 🟡 MinorNo rate limiting, lockout, or audit log on failed login.
/auth/loginis the sole authentication boundary and is reachable from every connected client (including remote ones once--allow-remoteis set). Argon2id verification throttles online brute force somewhat, but there is currently no per-IP counter, exponential backoff, or even atracing::warn!onOk(false). For a dashboard guarding hot-wallet RPC credentials, at minimum log failed attempts (with peer IP viaConnectInfo) so operators can spot brute force; ideally add a short per-IP cooldown after N failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/auth.rs` around lines 28 - 56, The login handler currently neither logs failed attempts nor enforces per-IP cooldowns; update the async fn login to accept axum::extract::ConnectInfo<SocketAddr> (to get peer IP), call AuthConfig::verify as before but on Ok(false) do tracing::warn! with the peer IP and username/context, and increment a per-IP failure counter stored in shared state (e.g., add a new Arc<Mutex<FailureTracker>> or extend SessionStore with failure tracking methods). Enforce cooldown/backoff by returning 429 Too Many Requests when the FailureTracker reports the IP is over the threshold (implement exponential backoff timing), and reset the counter on successful login (when SessionStore::create is called); keep error handling for Err(e) unchanged but consider logging audit info there too.SECURITY.md (1)
40-42: 🧹 Nitpick | 🔵 TrivialManual re-encryption guidance remains user-hostile.
Re-iterating the prior concern: this workflow expects users to decrypt with the old password and re-encrypt with the new one by hand; in practice users will delete
makers.json, losing all maker registrations and fidelity-bond associations. Tracking a follow-up that adds an in-dashboard rotation endpoint (validate old, re-encryptmakers.json, rewriteauth.jsonatomically) would prevent silent data loss after a password change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SECURITY.md` around lines 40 - 42, The SECURITY.md note describes a manual, destructive password-change workflow for makers.json; implement an in-dashboard password rotation endpoint that accepts the old and new password, validates the old password against current auth data, decrypts makers.json with the old password and re-encrypts it with the new one, and then atomically rewrites auth.json and the re-encrypted makers.json to avoid data loss; ensure the endpoint performs validation failures without mutating files, reports errors, and uses transactional/atomic file replace semantics (e.g., write temp files then rename) so makers.json and auth.json remain consistent.src/maker_manager/persistence.rs (1)
187-213:⚠️ Potential issue | 🟠 MajorSave is still non-atomic and applies perms after write.
The previously-raised major concern remains unaddressed in this revision:
fs::writetruncates and streams the destination; a crash, OOM kill, or power loss between truncate and close leaves a half-writtenmakers.json, which on nextload()will failserde_json::from_sliceand prevent recovery of all registered makers.fs::writecreates the file with the process umask (typically0644); the subsequentset_permissions(0o600)closes the window, but during it any local user/process can read the file. On theenc_key = Nonebranch this exposes cleartextrpc_user,rpc_password, walletpassword, andtor_auth.This becomes more impactful now that
load()may invokesave()mid-migration (line 261): a crash during the legacy → encrypted re-save loses the plaintext source as well.🛡️ Proposed fix (atomic + mode-on-create)
let path = self.state_file(); - - if let Some(key) = &self.enc_key { - let raw = crate::auth::aes_encrypt(key, json.as_bytes()) - .context("Failed to encrypt maker configs")?; - let b64 = B64.encode(&raw); - let envelope = serde_json::json!({ "v": 1, "data": b64 }); - let envelope_json = serde_json::to_string(&envelope) - .context("Failed to serialize encrypted envelope")?; - fs::write(&path, envelope_json) - .with_context(|| format!("Failed to write state file: {}", path.display()))?; - } else { - fs::write(&path, json) - .with_context(|| format!("Failed to write state file: {}", path.display()))?; - } - - // Restrict to owner read/write only (Unix only). - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let mut perms = std::fs::metadata(&path)?.permissions(); - perms.set_mode(0o600); - std::fs::set_permissions(&path, perms)?; - } + let payload: Vec<u8> = if let Some(key) = &self.enc_key { + let raw = crate::auth::aes_encrypt(key, json.as_bytes()) + .context("Failed to encrypt maker configs")?; + let envelope = serde_json::json!({ "v": 1, "data": B64.encode(&raw) }); + serde_json::to_vec(&envelope).context("Failed to serialize encrypted envelope")? + } else { + json.into_bytes() + }; + + let tmp = path.with_extension("json.tmp"); + { + use std::io::Write; + let mut opts = std::fs::OpenOptions::new(); + opts.write(true).create(true).truncate(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + opts.mode(0o600); + } + let mut f = opts + .open(&tmp) + .with_context(|| format!("Failed to open temp file: {}", tmp.display()))?; + f.write_all(&payload) + .with_context(|| format!("Failed to write temp file: {}", tmp.display()))?; + f.sync_all().ok(); + } + fs::rename(&tmp, &path) + .with_context(|| format!("Failed to rename temp file into {}", path.display()))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/maker_manager/persistence.rs` around lines 187 - 213, The save path is not written atomically and permissions are applied after write; update the save implementation (the block that calls self.state_file() and checks self.enc_key) to: create a temporary file in the same directory (e.g., state_file().with_extension(".tmp") or similar), open it with explicit mode on Unix using OpenOptionsExt::mode(0o600) so the file is created with owner-only perms, write the serialized/encrypted bytes with write_all, call file.sync_all() (and optionally parent dir sync) and then atomically rename the temp file to the final path (fs::rename); keep the same envelope logic around crate::auth::aes_encrypt and serde_json serialization but write them to the temp file instead of using fs::write; this ensures atomic replace and correct initial permissions for load() and migration flows that call save().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 29-33: The rand dependency declaration currently includes an
explicit features setting: rand = { version = "0.8", features = ["getrandom"] },
which is redundant because thread_rng() pulls in getrandom transitively; remove
the features = ["getrandom"] part and simplify the entry to rand = "0.8" (keep
the other new crypto deps unchanged) so the Cargo.toml dependency list is
cleaner.
In `@frontend/app/main.tsx`:
- Around line 46-52: The /login route currently renders Login outside AuthLayout
so authenticated users can still see the form; update the Login route/component
to check authentication (reuse the same logic as AuthLayout or call
auth.status()) and return a redirect when authenticated (e.g., render <Navigate
to="/" replace />) instead of the login form; locate the Login component or the
Route for "/login" and add the auth check with Navigate to ensure
already-authenticated users are forwarded to Home.
In `@SECURITY.md`:
- Around line 69-70: The sentence fragment "Deleting a maker from the dashboard
does not reclaim those funds. it only removes the dashboard registration." in
SECURITY.md has incorrect capitalization; change the lowercase "it" to "It" so
the second sentence starts with a capital letter, preserving the rest of the
sentence text and punctuation.
In `@src/api/auth.rs`:
- Around line 85-91: extract_session_token currently finds "session=" by
splitting on ';' and trimming which can mis-identify tokens if a prior cookie
value ends with "session="; update this to robustly parse cookies by either
using the cookie crate (e.g., Cookie::parse) or by splitting each segment into
name/value with splitn(2,'=') and matching name == "session" (after trimming)
before returning the value; ensure you update the function extract_session_token
to use header::COOKIE as input and return the decoded cookie value safely.
In `@src/auth.rs`:
- Around line 63-93: In save(), ensure the temporary file (tmp /
"auth.json.tmp") is removed on all failure paths: wrap the temp-file lifecycle
so any early Err from file.write_all, file.sync_all, or std::fs::rename triggers
std::fs::remove_file(&tmp) before returning the error (or use
scopeguard::defer/guard to remove the temp unless rename succeeded); keep the
existing platform-specific OpenOptions and drop(file) semantics, but arrange the
cleanup so successful rename cancels removal and any error returns propagate the
original error after removing tmp.
- Around line 70-85: The non-unix branch using std::fs::OpenOptions
(cfg(not(unix))) creates the credentials file without setting owner-only
permissions (unlike the unix branch that uses .mode(0o600)), so on Windows the
file will inherit the parent directory ACL; update the docstring at the comment
starting around line 62 to document this limitation (mention that
OpenOptions::open(&tmp) on non-unix platforms does not set POSIX mode, Windows
will inherit directory ACLs) and add guidance for callers (e.g., create the file
in a secure directory or implement Windows-specific ACL setting via WinAPI/ACL
libraries later), or alternatively implement Windows ACL setting in the
cfg(not(unix)) branch using a platform-specific API—refer to the OpenOptions
usage and the tmp variable to locate the code to change.
In `@src/maker_manager/persistence.rs`:
- Around line 251-267: The migration from legacy plaintext to encrypted format
can leave makers.json corrupted because persistence::save is non-atomic; modify
the migration branch that detects StoredState (StoredState, MakerConfig,
enc_key) to perform an atomic write when re-saving (use or implement the same
temp-file + fsync + rename routine used elsewhere for safe writes) and log the
intended "Migrated makers.json to encrypted format" message before attempting
the save so operators see the action; ensure the migration uses the same
save_atomic helper (or extend save to expose an atomic option) so failures leave
the original file intact and the migration is safely retryable.
- Around line 231-250: The branch currently treats any present "v" as v1; change
the conditional to explicitly validate that the envelope version equals 1 (e.g.,
check value.get("v").and_then(|v| v.as_u64()) == Some(1) or similar) before
proceeding with base64 decode, decryption via crate::auth::aes_decrypt and
deserializing to StoredState, and otherwise return a clear error indicating an
unsupported makers.json envelope version; update the branch that uses
self.enc_key, b64_str, raw, plaintext, StoredState and MakerConfig so only v==1
uses the v1 decryption path.
In `@src/middlewares/mod.rs`:
- Around line 43-49: The current middleware unconditionally allows "/swagger-ui"
and "/api-docs" through (the path.starts_with checks) which exposes OpenAPI UI
without authentication; change the logic so these routes either require the same
session check as other "/api/*" endpoints or are allowed only when a runtime
config flag (e.g., enable_swagger()) is true—update the conditional around the
path.starts_with("/swagger-ui") and path.starts_with("/api-docs") checks to
consult the flag (or remove them from the unconditional bypass) so that requests
to those paths use the existing session validation before calling
next.run(request).await.
In `@tests/integration_test.rs`:
- Around line 203-218: Replace the nonexistent .cookies(true) call with ureq's
cookie store API: enable the "cookies" feature for the ureq dev-dependency in
Cargo.toml, add an import for CookieStore (use ureq::cookie::CookieStore),
create a CookieStore instance and pass it into AgentBuilder via
.cookie_store(store) when building the agent (refer to
AgentBuilder::cookie_store and CookieStore in tests/integration_test.rs), then
keep the subsequent .post(...).send_json(...) login call as-is to persist the
session cookie.
---
Outside diff comments:
In `@src/api/bitcoind.rs`:
- Around line 100-119: The start handler currently calls
state.lock().await.start_bitcoind(...) while holding the async Mutex guard (in
start), which can block other handlers and poison the lock; change it to take
only the minimal data under the lock (e.g., clone body.network or extract any
needed flags) and drop the guard before invoking the blocking startup; then run
the blocking work using tokio::task::spawn_blocking (or call a new Manager
method that uses interior std::sync::Mutex for the child handle) so that
MakerManager::start_bitcoind is executed off the async mutex and does not hold
the guard across child spawn/FS/network operations.
In `@src/api/fidelity.rs`:
- Around line 30-54: In list_fidelity, avoid holding the
Arc<Mutex<MakerManager>> guard across the async IPC call: first lock the manager
to run has_maker and retrieve or clone a per-maker handle (or the specific
channel/handle used to talk to the maker) from MakerManager, then drop the
MutexGuard before calling the async maker IPC (i.e., do not call
state.lock().await.list_fidelity(&id).await while the guard is live).
Concretely, use State(state): State<Arc<Mutex<MakerManager>>> to acquire a
short-lived guard, call has_maker(&id) and get a clone of the maker handle (or a
reference you can move out) from MakerManager, release the guard, then call the
manager’s list_fidelity via that cloned handle (or a refactored list_fidelity
that accepts the handle) and await the result; update error handling to match
the existing match arms.
In `@src/api/monitoring.rs`:
- Around line 57-78: get_status currently holds the MakerManager MutexGuard
across async calls (state.lock().await.ping(...).await) and takes the lock
multiple times creating a TOCTOU window; change it to acquire the lock once,
check has_maker, obtain a cloneable per-maker handle or the minimal data needed
(e.g., call a new/get_handle method or clone a channel/reference for maker
identified by id), then drop the guard before performing any async await (call
ping() on the cloned handle and await it, call is_server_running on either the
cloned handle or by copying a boolean under the lock), and finally build the
ApiResponse; update MakerManager to expose a cloneable per-maker handle (or
get_handle/cloneable channel) so
ping/get_swap_utxos/get_swept_swap_utxos/get_tor_address/get_data_dir calls
never await while holding the manager lock, and apply this scoping pattern to
all other handlers referencing state.lock().await.<async_method>.
---
Duplicate comments:
In `@docker/docker-compose.yml`:
- Around line 76-81: Remove the hardcoded fallback for DASHBOARD_PASSWORD in the
docker-compose.yml so the service cannot start with a public default;
specifically, replace the usage of
DASHBOARD_PASSWORD=${DASHBOARD_PASSWORD:-coinswap} with a required/empty
reference (e.g., ${DASHBOARD_PASSWORD} or a shell-style required parameter) so
compose will not silently use "coinswap", and ensure this aligns with the
documented .env example (DASHBOARD_PASSWORD=changeme) and the remote enabling
flag DASHBOARD_ALLOW_REMOTE remains deliberate.
In `@frontend/app/main.tsx`:
- Around line 24-35: The effect in AuthLayout calling auth.status() can update
state after unmount (and runs twice under StrictMode); add a cancellation guard
in the useEffect cleanup: create a local boolean (e.g. let cancelled = false) or
an AbortController before calling auth.status(), and in the promise
resolution/rejection check the guard (e.g. if (cancelled) return) before calling
setAuthenticated and setChecking; mark cancelled = true in the cleanup returned
by useEffect so setAuthenticated/setChecking are never called on an unmounted
AuthLayout.
- Line 37: The current early return "if (checking) return null;" in main.tsx
causes a blank-screen flicker during auth.status() checks; replace that return
with rendering the app's lightweight centered loading UI (e.g., the shared
Spinner or LoadingSkeleton component) so users see a styled centered
spinner/skeleton while "checking" is true; update the main component where
"checking" and auth.status() are used to import and render the existing
Spinner/LoadingSkeleton (or create a minimal centered spinner markup) instead of
returning null so the initial auth check matches the styled /login experience.
In `@packaging/umbrel/docker-compose.yml`:
- Line 33: Replace the permissive env substitution for DASHBOARD_PASSWORD with
Docker Compose's strict/substitution form so the container startup fails if the
variable is missing; specifically update the environment entry that currently
uses DASHBOARD_PASSWORD: ${DASHBOARD_PASSWORD} to use the strict form (e.g.,
DASHBOARD_PASSWORD: ${DASHBOARD_PASSWORD:?DASHBOARD_PASSWORD must be set}) so
Compose refuses to start on misconfiguration.
In `@SECURITY.md`:
- Around line 40-42: The SECURITY.md note describes a manual, destructive
password-change workflow for makers.json; implement an in-dashboard password
rotation endpoint that accepts the old and new password, validates the old
password against current auth data, decrypts makers.json with the old password
and re-encrypts it with the new one, and then atomically rewrites auth.json and
the re-encrypted makers.json to avoid data loss; ensure the endpoint performs
validation failures without mutating files, reports errors, and uses
transactional/atomic file replace semantics (e.g., write temp files then rename)
so makers.json and auth.json remain consistent.
In `@src/api/auth.rs`:
- Around line 33-55: The login handler is constructing JSON envelopes by hand;
replace those inline Json(json!(...)) responses with the typed ApiResponse
wrappers used elsewhere: return ApiResponse::ok(...) on success and
ApiResponse::err(...) on failures (use the appropriate type parameter for empty
data, e.g., unit/None), while preserving the Set-Cookie header and status codes
(StatusCode::OK, UNAUTHORIZED, INTERNAL_SERVER_ERROR); apply the same change to
the logout and status handlers so all three endpoints use ApiResponse::ok /
ApiResponse::err instead of hand-rolled Json objects.
- Around line 36-37: The session Set-Cookie string (variable "cookie" in
auth.rs) and the logout clear-cookie should include the Secure attribute so the
browser only sends the cookie over HTTPS; update the format strings that build
the cookie (the login "cookie" format and the logout clear-cookie string) to
append "; Secure" (e.g., "...; Path=/; Max-Age=86400; Secure") and similarly
include "; Secure" on the clear-cookie response so both issuance and clearing
are always Secure.
- Around line 28-56: The login handler currently neither logs failed attempts
nor enforces per-IP cooldowns; update the async fn login to accept
axum::extract::ConnectInfo<SocketAddr> (to get peer IP), call AuthConfig::verify
as before but on Ok(false) do tracing::warn! with the peer IP and
username/context, and increment a per-IP failure counter stored in shared state
(e.g., add a new Arc<Mutex<FailureTracker>> or extend SessionStore with failure
tracking methods). Enforce cooldown/backoff by returning 429 Too Many Requests
when the FailureTracker reports the IP is over the threshold (implement
exponential backoff timing), and reset the counter on successful login (when
SessionStore::create is called); keep error handling for Err(e) unchanged but
consider logging audit info there too.
In `@src/api/mod.rs`:
- Around line 56-131: The OpenAPI spec is missing the auth endpoints and
schemas: add auth::login, auth::logout and auth::status to the paths(...) list
and register the corresponding DTO schemas (e.g., LoginRequest, LoginResponse or
AuthStatus) in components(schemas(...)) so the Swagger UI and openapi.json
include the auth contract; also ensure the handlers in src/api/auth.rs have
matching #[utoipa::path(...)] annotations for each route (login/logout/status)
and the DTO structs used there are annotated with #[derive(utoipa::ToSchema)] or
equivalent to expose their request/response shapes.
In `@src/auth.rs`:
- Around line 146-152: In validate, after calling self.sessions.retain(|_,
expiry| *expiry > now) the subsequent map_or check is redundant; replace the
map_or(...) expression with a simple self.sessions.contains_key(token) so
validate returns whether the token is present (keep the function signature
validate(&mut self, token: &str) and the retain call on self.sessions).
In `@src/maker_manager/persistence.rs`:
- Around line 187-213: The save path is not written atomically and permissions
are applied after write; update the save implementation (the block that calls
self.state_file() and checks self.enc_key) to: create a temporary file in the
same directory (e.g., state_file().with_extension(".tmp") or similar), open it
with explicit mode on Unix using OpenOptionsExt::mode(0o600) so the file is
created with owner-only perms, write the serialized/encrypted bytes with
write_all, call file.sync_all() (and optionally parent dir sync) and then
atomically rename the temp file to the final path (fs::rename); keep the same
envelope logic around crate::auth::aes_encrypt and serde_json serialization but
write them to the temp file instead of using fs::write; this ensures atomic
replace and correct initial permissions for load() and migration flows that call
save().
In `@src/server.rs`:
- Around line 68-89: Add a defensive check in Server::new that validates
ServerConfig::password is non-empty before attempting to load/create auth
config: if the password is empty, return an error (anyhow::bail or appropriate
Result::Err) rather than calling AuthConfig::new or AuthConfig::load; this
prevents creating an auth.json whose hash matches the empty string and ensures
derive_key is only called with a real password.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: be8cdd65-14ac-4d23-a8a6-c30b5bc7e280
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
.gitignoreCargo.tomlREADME.mdSECURITY.mddocker/.env.exampledocker/docker-compose.ymlfrontend/app/api.tsfrontend/app/main.tsxfrontend/app/routes/login.tsxpackaging/umbrel/docker-compose.ymlsrc/api/auth.rssrc/api/bitcoind.rssrc/api/fidelity.rssrc/api/makers.rssrc/api/mod.rssrc/api/monitoring.rssrc/api/wallet.rssrc/auth.rssrc/lib.rssrc/main.rssrc/maker_manager/mod.rssrc/maker_manager/persistence.rssrc/middlewares/mod.rssrc/server.rstests/api/mod.rstests/integration_test.rs
hulxv
force-pushed
the
feat/authenication
branch
3 times, most recently
from
46c7d37 to
5f30f0b
Compare
hulxv
force-pushed
the
feat/authenication
branch
from
e42b11d to
61a7dd1
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/api/fidelity.rs (1)
30-53:⚠️ Potential issue | 🟡 MinorLock held across
.awaitand potential TOCTOU race condition.Two concerns in this handler:
TOCTOU between checks: Lines 34 and 40 execute separate
lock().awaitcalls. Between thehas_makercheck and thelist_fidelitycall, another task can remove the maker, causing the second lock to succeed but the request to fail internally rather than returning a consistent 404.Lock held across
.await: Line 40 holds theMutexGuardacross the innerawaitoflist_fidelity(&id).await. This blocks all other API handlers contending forMakerManagerfor the duration of an upstream RPC round-trip. While this pattern appears throughout the API handlers, it's an anti-pattern that can cause unnecessary contention.Suggestion: Perform the existence check and request within a single critical section, then drop the guard before awaiting:
let id = id.clone(); let pool = state.lock().await.get_pool(&id)?; drop(state); // or restructure to avoid holding guard match pool.list_fidelity().await { ... }Alternatively, if
list_fidelityonly forwards to the pool, expose a method that acquires the pool reference without holding the manager lock across the await.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/fidelity.rs` around lines 30 - 53, The handler list_fidelity currently calls state.lock().await twice (has_maker and list_fidelity) and holds the MutexGuard across an await, causing TOCTOU and blocking issues; fix it by acquiring the guard once in list_fidelity (call MakerManager::has_maker and then obtain the pool/client/handle for that maker via a non-async accessor like get_pool or by cloning a reference), return the 404 immediately if the maker is absent, then drop the guard before performing any async RPC (i.e., call pool.list_fidelity().await after dropping the MutexGuard) so the manager mutex is not held across awaits; alternatively add a MakerManager method that returns a clonable pool handle without awaiting under the lock and use that from the list_fidelity handler.src/api/monitoring.rs (1)
387-413:⚠️ Potential issue | 🟠 MajorSSE log stream re-reads the entire log file every 500 ms.
tokio::fs::read(&path).awaiton line 390 reads the whole file into memory each tick, then slices&contents[last_pos as usize..]to find new bytes. With the 100 MB cap enforced inget_logs_download(line 309), a maker can have logs large enough that this loop allocates and discards up to 100 MB every 500 ms per active stream subscriber — sustained ~200 MB/s I/O and allocation churn for one client doing nothing but watching logs.Use a positioned read against the appended region instead: open the file, seek to
last_pos, read only the new bytes, and update the cursor. This bounds per-tick work to the size of newly-written data.♻️ Sketch using
tokio::fs::File+ seek- let stream = stream::unfold((initial_len, log_path), |(last_pos, path)| async move { - tokio::time::sleep(Duration::from_millis(500)).await; - - let contents = tokio::fs::read(&path).await.unwrap_or_default(); - let current_len = contents.len() as u64; - - let (events, new_pos): (Vec<Result<Event, Infallible>>, u64) = if current_len < last_pos { - // File was truncated or rotated — restart from the beginning. - let events = String::from_utf8_lossy(&contents) - .lines() - .map(|line| Ok(Event::default().data(line))) - .collect(); - (events, current_len) - } else if current_len > last_pos { - let new_data = &contents[last_pos as usize..]; - let events = String::from_utf8_lossy(new_data) - .lines() - .map(|line| Ok(Event::default().data(line))) - .collect(); - (events, current_len) - } else { - (Vec::new(), last_pos) - }; + let stream = stream::unfold((initial_len, log_path), |(last_pos, path)| async move { + use tokio::io::{AsyncReadExt, AsyncSeekExt, SeekFrom}; + tokio::time::sleep(Duration::from_millis(500)).await; + + let current_len = tokio::fs::metadata(&path).await.map(|m| m.len()).unwrap_or(0); + let (start, new_pos) = if current_len < last_pos { + (0, current_len) // truncation/rotation + } else if current_len > last_pos { + (last_pos, current_len) + } else { + return Some((stream::iter(Vec::<Result<Event, Infallible>>::new()), (last_pos, path))); + }; + + let mut buf = Vec::with_capacity((current_len - start) as usize); + if let Ok(mut f) = tokio::fs::File::open(&path).await { + let _ = f.seek(SeekFrom::Start(start)).await; + let _ = f.read_to_end(&mut buf).await; + } + let events: Vec<Result<Event, Infallible>> = String::from_utf8_lossy(&buf) + .lines() + .map(|line| Ok(Event::default().data(line))) + .collect();Note that the same lock-across-await concern flagged on
src/api/wallet.rs(get_balance) applies throughout this file as well —get_statusalone takes theMakerManagermutex three separate times (lines 61, 67, 68) per request, so a fix at the root cause should also clean those up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/monitoring.rs` around lines 387 - 413, The SSE stream currently calls tokio::fs::read(&path).await inside stream::unfold (using initial_len, log_path, last_pos, new_pos) which re-reads the entire file every tick; replace that logic to open the file with tokio::fs::File, seek to last_pos, and perform a positioned read of only the appended bytes (updating new_pos to last_pos + bytes_read) and handle truncation/rotation by seeking to 0 when file size is smaller than last_pos; keep producing Vec<Result<Event, Infallible>> (Event::default().data(...)) from the newly-read data and preserve the stream::unfold structure and sleep timing so per-tick work is bounded to new bytes rather than rereading whole file.src/api/wallet.rs (1)
40-73:⚠️ Potential issue | 🟠 MajorHolding the
MakerManagermutex across maker RPC awaits serializes every wallet/monitoring request.The pattern
state.lock().await.get_balances(&id).await(line 50, and repeated inget_utxos,send_to_address,get_new_address,sync_wallet,get_swap_utxos,get_contract_utxos,get_fidelity_utxos, plus all the monitoring handlers insrc/api/monitoring.rslikeget_status,get_swaps,get_tor_address,get_data_dir) takes a tokioMutexand holds it for the entire duration of a per-maker network call. Two concrete problems:
- Global serialization: while one maker is syncing or doing an RPC roundtrip, no other request — even one targeting a different maker — can acquire the lock. With multiple makers and the polling done by the frontend (
/status,/balance,/utxos,/logs/stream, etc.), this will produce visible request stalls, especially under the 3-minutesyncdeadline used in the integration test.- TOCTOU between
has_makerand the action: every handler doesstate.lock().await.has_maker(...)then drops and re-acquires the lock for the actual call. Between those two acquisitions another request can delete the maker, leading to a different (and noisier) error path than the intended 404. It also doubles the lock/unlock cost on the hot path.Recommended fix: have
MakerManagerexpose a way to obtain a per-maker handle (e.g.Arc<Maker>or a clonable client) under the lock, then drop the outer lock before doing async I/O. At minimum, do thehas_makercheck and capture whatever you need (e.g. the maker handle / config) in a single critical section and release the mutex before.await-ing on RPC.async fn get_balance( State(state): State<Arc<Mutex<MakerManager>>>, Path(id): Path<String>, ) -> (StatusCode, Json<ApiResponse<BalanceInfo>>) { - if !state.lock().await.has_maker(&id) { - return ( - StatusCode::NOT_FOUND, - Json(ApiResponse::err(format!("Maker '{id}' not found"))), - ); - } - match state.lock().await.get_balances(&id).await { + // Acquire a maker handle under the lock, then drop the lock before awaiting RPC. + let maker = { + let mgr = state.lock().await; + match mgr.get_maker_handle(&id) { + Some(m) => m, + None => return ( + StatusCode::NOT_FOUND, + Json(ApiResponse::err(format!("Maker '{id}' not found"))), + ), + } + }; + match maker.get_balances().await {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/wallet.rs` around lines 40 - 73, The handlers like get_balance (and similarly get_utxos, send_to_address, get_new_address, sync_wallet, get_swap_utxos, get_contract_utxos, get_fidelity_utxos and the monitoring handlers in src/api/monitoring.rs such as get_status/get_swaps/get_tor_address/get_data_dir) currently call state.lock().await and then .await on RPCs while holding the global MakerManager mutex; change this by adding a MakerManager method (e.g. get_maker_handle or into_maker_client) that, under the lock, validates the maker exists and returns a clonable per-maker handle (Arc/cloneable client or a small struct with needed config) and then drop the mutex before performing any async RPCs; update each handler (e.g. get_balance) to call that single critical-section method instead of has_maker + later get_balances, eliminating TOCTOU and preventing global serialization of network calls.tests/integration_test.rs (1)
143-216: 🧹 Nitpick | 🔵 TrivialExtract the test password into a constant.
The integration password literal
"integration-test-password"appears both inServerConfig(line 143) and the login call (line 216). If they ever drift, every authenticated request will silently 401 in CI and the failure mode (transport-level success but no cookie set, or a panic from.expect("dashboard login failed")) is non-obvious. Promote it to aconstalongside the other test constants near lines 38–46.+const TEST_DASHBOARD_PASSWORD: &str = "integration-test-password"; @@ - password: "integration-test-password".to_string(), + password: TEST_DASHBOARD_PASSWORD.to_string(), @@ - .send_json(serde_json::json!({ "password": "integration-test-password" })) + .send_json(serde_json::json!({ "password": TEST_DASHBOARD_PASSWORD }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration_test.rs` around lines 143 - 216, Extract the literal "integration-test-password" into a single constant (e.g., const TEST_PASSWORD: &str = "integration-test-password") alongside the existing test constants near the top of the file, then replace the two literal occurrences with that constant: use TEST_PASSWORD when building ServerConfig (the password field passed into Server::new) and use TEST_PASSWORD in ApiClient::new when sending the login JSON for the agent POST; ensure no other behavior changes.
♻️ Duplicate comments (2)
frontend/app/main.tsx (1)
20-64: 🛠️ Refactor suggestion | 🟠 MajorExtract the duplicated auth-status guard into a shared hook/component, and apply the same StrictMode/unmount guard to
GuestLayout.
AuthLayoutandGuestLayoutare byte-for-byte identical except for the redirect target — both run the sameauth.status()effect, hold the samechecking/authenticatedstate pair, and have the same blank-screen-while-checking branch. Pull the effect into auseAuthStatus()hook (or a singleRequireAuth/RequireGuestwrapper) so future fixes only need to land in one place.Note that the past comment on
AuthLayout(lines 20–40) about StrictMode double-invocation and unmount-race protection applies equally toGuestLayout(lines 44–64). Whatever cancellation guard lands inAuthLayoutmust also be applied here — extracting a shared hook makes that automatic.♻️ Sketch of the shared hook
+function useAuthStatus() { + const [checking, setChecking] = useState(true); + const [authenticated, setAuthenticated] = useState(false); + useEffect(() => { + let cancelled = false; + auth + .status() + .then(({ authenticated }) => { + if (cancelled) return; + setAuthenticated(authenticated); + setChecking(false); + }) + .catch(() => { + if (cancelled) return; + setAuthenticated(false); + setChecking(false); + }); + return () => { + cancelled = true; + }; + }, []); + return { checking, authenticated }; +} + -function AuthLayout() { - const [checking, setChecking] = useState(true); - const [authenticated, setAuthenticated] = useState(false); - useEffect(() => { /* ...duplicated... */ }, []); +function AuthLayout() { + const { checking, authenticated } = useAuthStatus(); if (checking) return null; if (!authenticated) return <Navigate to="/login" replace />; return <Outlet />; } -function GuestLayout() { - const [checking, setChecking] = useState(true); - const [authenticated, setAuthenticated] = useState(false); - useEffect(() => { /* ...duplicated... */ }, []); +function GuestLayout() { + const { checking, authenticated } = useAuthStatus(); if (checking) return null; if (authenticated) return <Navigate to="/" replace />; return <Outlet />; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/main.tsx` around lines 20 - 64, Extract the duplicated auth-status logic from AuthLayout and GuestLayout into a shared hook (e.g. useAuthStatus) or a single guard component (e.g. RequireAuth / RequireGuest) that returns {checking, authenticated}, and replace both components to use that hook/guard; ensure the hook implements the StrictMode/unmount cancellation guard used previously (cancel or ignore stale promises in auth.status() to avoid setState after unmount/double-invoke) so both AuthLayout and GuestLayout inherit the fix and only the redirect target differs (Navigate to "/login" vs "/" respectively).src/middlewares/mod.rs (1)
41-51:⚠️ Potential issue | 🟠 MajorSwagger UI and OpenAPI spec still bypass authentication.
The previous explicit allow-list for
/swagger-uiand/api-docswas removed, but the gating logic at lines 48–51 only enforces auth for paths that literally start with/api/(note the trailing slash). The OpenAPI spec is mounted at/api-docs/openapi.json(persrc/server.rs), and/api-docs/...does not match the prefix/api/—"/api-docs/...".starts_with("/api/") == false. Likewise/swagger-uiis unaffected. So both endpoints continue to fall through tonext.run(...)unauthenticated, which is the same exposure flagged previously.If the intent is to gate docs behind login, also check
/swagger-uiand/api-docs(or move the swagger merge to apply the auth layer). If the intent is to keep docs public, please make that an explicit, documented decision rather than an implicit consequence of the prefix check.- // Only apply auth check to /api/* routes - if !path.starts_with("/api/") { - return next.run(request).await; - } + // Auth applies to all API + docs surface; only non-API/non-docs paths bypass. + let is_protected = path.starts_with("/api/") + || path.starts_with("/api-docs") + || path.starts_with("/swagger-ui"); + if !is_protected { + return next.run(request).await; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middlewares/mod.rs` around lines 41 - 51, The auth gating wrongly uses path.starts_with("/api/") so /api-docs and /swagger-ui slip through; update the check around request.uri().path() (the path variable used with starts_with and the branch returning next.run(request).await) to explicitly include docs or broaden the API prefix: either change the condition to use path.starts_with("/api") (so /api-docs is treated as API) and also include an explicit path.starts_with("/swagger-ui") test, or replace the if with if !(path.starts_with("/api") || path.starts_with("/api-docs") || path.starts_with("/swagger-ui")) { return next.run(request).await; } so the docs are handled by the auth branch (or conversely make an explicit allow-list for docs if you intend them public).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker/.env.example`:
- Around line 1-13: The .env.example currently sets a weak default
DASHBOARD_PASSWORD=changeme which, combined with DASHBOARD_ALLOW_REMOTE=true in
docker-compose.yml, can expose the dashboard; update the template so the
password is not a usable default by either leaving DASHBOARD_PASSWORD empty or
using a nondiscoverable placeholder that forces manual set (e.g.,
DASHBOARD_PASSWORD=) and add a clear one-line warning comment referencing
DASHBOARD_ALLOW_REMOTE and DASHBOARD_PASSWORD to require replacement before
enabling remote access; ensure you only modify the docker/.env.example file and
mention the variable names DASHBOARD_PASSWORD and DASHBOARD_ALLOW_REMOTE in the
comment so operators notice the dependency.
In `@docker/docker-compose.yml`:
- Line 81: The strict-substitution message for the DASHBOARD_PASSWORD
environment variable contains a typo ("DASHBOARD_PiASSWORD") which will be shown
to operators if the variable is unset; update the substitution on the
DASHBOARD_PASSWORD line (the ${DASHBOARD_PASSWORD:?...} expression in
docker-compose.yml) to use the correct message text (e.g., "Set
DASHBOARD_PASSWORD in a .env file or your shell environment") so the error shown
when the variable is missing is accurate.
In `@frontend/app/api.ts`:
- Around line 227-230: The current 401 handling in the response branch (the
res.status === 401 && !path.startsWith("/auth/") check) does a hard redirect to
"/login" and drops the original URL; change it to preserve the originating URL
by building a next query param (e.g. const next =
encodeURIComponent(window.location.pathname + window.location.search +
window.location.hash) and set window.location.href = `/login?next=${next}`)
instead of plain "/login", and add a guard so if the current location is already
on "/login" (or path.startsWith("/auth/") covers this) you do not perform
another redirect; keep returning Promise.reject(new ApiError(401,
"Unauthorized")) as before so callers still handle the error. Ensure the login
route reads the next param and navigates there after successful auth.
In `@frontend/app/routes/login.tsx`:
- Around line 42-60: The login form currently allows browser password saving;
update the <form onSubmit={handleSubmit}> to include autoComplete="off" and add
autoComplete="new-password" to the password <input id="password" ...> (the
controlled field using value={password} and onChange={(e) =>
setPassword(e.target.value)}) so browsers don't persist the credential—keep the
rest of attributes and behavior (disabled={loading}, required, etc.) unchanged.
In `@SECURITY.md`:
- Around line 27-28: Update the cookie attribute list in the SECURITY.md
parenthetical to match the actual implementation in src/api/auth.rs by including
`Secure` so it reads something like `HttpOnly; Secure; SameSite=Strict; 24 h
expiry`; ensure the documented attributes exactly match the session cookie setup
used by the auth code (the session cookie creation in auth handlers).
In `@src/api/auth.rs`:
- Line 71: The clear-cookie string used when logging out (the clear_cookie
variable) omits the Secure attribute and should mirror the cookie attributes
used in the login path (e.g., the cookie set in login). Update the clear_cookie
value to include "Secure" (e.g., "session=; HttpOnly; Secure; SameSite=Strict;
Path=/; Max-Age=0") so the Set-Cookie header emitted by the logout handler
matches issuance, and ensure the logout code that sends clear_cookie uses this
updated constant.
In `@src/auth.rs`:
- Around line 122-156: Remove the persistent cookie flags from the login
response by omitting Max-Age/Expires (and any Expires header) when setting the
auth cookie in the login handler so it becomes a browser-session cookie; keep
SameSite/Secure/HttpOnly as-is. Replace Instant-based expiry in SessionStore
with wall-clock times (use std::time::SystemTime or a UNIX timestamp) by
changing the sessions map type and the create and validate methods: in create()
set expiry = SystemTime::now() + Duration::from_secs(86400) (or store expiry as
now().duration_since(UNIX_EPOCH).as_secs() + 86400), in validate() prune entries
using SystemTime::now() comparisons and check token existence against those
wall-clock expiries; update remove() unchanged. Ensure you reference the
SessionStore struct and its create()/validate() methods when making these edits
so suspend/clock changes won’t incorrectly extend sessions.
In `@src/maker_manager/mod.rs`:
- Line 111: The constructor signature and stored field use a plain Option<[u8;
32]> which duplicates key bytes and never zeroizes; change the API to accept and
store a secrecy::Secret (or heap-backed secrecy::SecretBox) instead — i.e.,
update the new(...) parameter from enc_key: Option<[u8; 32]> to enc_key:
Option<secrecy::Secret<[u8; 32]>> (or Option<secrecy::SecretBox<[u8; 32]>>) and
update PersistenceManager to hold the same Secret type so keys are zeroized on
drop; also add the secrecy dependency with the alloc feature in Cargo.toml and
adjust any callers of maker_manager::new and places constructing
PersistenceManager to wrap raw key bytes into the chosen secrecy type before
passing.
In `@src/utils/log_writer.rs`:
- Around line 59-60: Remove the unnecessary Clippy suppression on
MakerLogWriter::new by deleting the #[allow(clippy::new_without_default)]
attribute from the new() constructor; since MakerLogWriter already implements
Default, the lint will not trigger and the suppression is redundant—leave the
Default impl and the pub fn new() as-is minus the attribute.
---
Outside diff comments:
In `@src/api/fidelity.rs`:
- Around line 30-53: The handler list_fidelity currently calls
state.lock().await twice (has_maker and list_fidelity) and holds the MutexGuard
across an await, causing TOCTOU and blocking issues; fix it by acquiring the
guard once in list_fidelity (call MakerManager::has_maker and then obtain the
pool/client/handle for that maker via a non-async accessor like get_pool or by
cloning a reference), return the 404 immediately if the maker is absent, then
drop the guard before performing any async RPC (i.e., call
pool.list_fidelity().await after dropping the MutexGuard) so the manager mutex
is not held across awaits; alternatively add a MakerManager method that returns
a clonable pool handle without awaiting under the lock and use that from the
list_fidelity handler.
In `@src/api/monitoring.rs`:
- Around line 387-413: The SSE stream currently calls
tokio::fs::read(&path).await inside stream::unfold (using initial_len, log_path,
last_pos, new_pos) which re-reads the entire file every tick; replace that logic
to open the file with tokio::fs::File, seek to last_pos, and perform a
positioned read of only the appended bytes (updating new_pos to last_pos +
bytes_read) and handle truncation/rotation by seeking to 0 when file size is
smaller than last_pos; keep producing Vec<Result<Event, Infallible>>
(Event::default().data(...)) from the newly-read data and preserve the
stream::unfold structure and sleep timing so per-tick work is bounded to new
bytes rather than rereading whole file.
In `@src/api/wallet.rs`:
- Around line 40-73: The handlers like get_balance (and similarly get_utxos,
send_to_address, get_new_address, sync_wallet, get_swap_utxos,
get_contract_utxos, get_fidelity_utxos and the monitoring handlers in
src/api/monitoring.rs such as get_status/get_swaps/get_tor_address/get_data_dir)
currently call state.lock().await and then .await on RPCs while holding the
global MakerManager mutex; change this by adding a MakerManager method (e.g.
get_maker_handle or into_maker_client) that, under the lock, validates the maker
exists and returns a clonable per-maker handle (Arc/cloneable client or a small
struct with needed config) and then drop the mutex before performing any async
RPCs; update each handler (e.g. get_balance) to call that single
critical-section method instead of has_maker + later get_balances, eliminating
TOCTOU and preventing global serialization of network calls.
In `@tests/integration_test.rs`:
- Around line 143-216: Extract the literal "integration-test-password" into a
single constant (e.g., const TEST_PASSWORD: &str = "integration-test-password")
alongside the existing test constants near the top of the file, then replace the
two literal occurrences with that constant: use TEST_PASSWORD when building
ServerConfig (the password field passed into Server::new) and use TEST_PASSWORD
in ApiClient::new when sending the login JSON for the agent POST; ensure no
other behavior changes.
---
Duplicate comments:
In `@frontend/app/main.tsx`:
- Around line 20-64: Extract the duplicated auth-status logic from AuthLayout
and GuestLayout into a shared hook (e.g. useAuthStatus) or a single guard
component (e.g. RequireAuth / RequireGuest) that returns {checking,
authenticated}, and replace both components to use that hook/guard; ensure the
hook implements the StrictMode/unmount cancellation guard used previously
(cancel or ignore stale promises in auth.status() to avoid setState after
unmount/double-invoke) so both AuthLayout and GuestLayout inherit the fix and
only the redirect target differs (Navigate to "/login" vs "/" respectively).
In `@src/middlewares/mod.rs`:
- Around line 41-51: The auth gating wrongly uses path.starts_with("/api/") so
/api-docs and /swagger-ui slip through; update the check around
request.uri().path() (the path variable used with starts_with and the branch
returning next.run(request).await) to explicitly include docs or broaden the API
prefix: either change the condition to use path.starts_with("/api") (so
/api-docs is treated as API) and also include an explicit
path.starts_with("/swagger-ui") test, or replace the if with if
!(path.starts_with("/api") || path.starts_with("/api-docs") ||
path.starts_with("/swagger-ui")) { return next.run(request).await; } so the docs
are handled by the auth branch (or conversely make an explicit allow-list for
docs if you intend them public).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc6e6902-a8f5-4a56-b043-d1e5ceef9c68
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
.github/workflows/docker-build.yml.gitignoreCargo.tomlREADME.mdSECURITY.mddocker/.env.exampledocker/docker-compose.ymlfrontend/app/api.tsfrontend/app/main.tsxfrontend/app/routes/login.tsxfrontend/tsconfig.tsbuildinfopackaging/umbrel/docker-compose.ymlsrc/api/auth.rssrc/api/bitcoind.rssrc/api/fidelity.rssrc/api/makers.rssrc/api/mod.rssrc/api/monitoring.rssrc/api/wallet.rssrc/auth.rssrc/lib.rssrc/main.rssrc/maker_manager/mod.rssrc/maker_manager/persistence.rssrc/middlewares/mod.rssrc/server.rssrc/utils/log_writer.rstests/api/mod.rstests/integration_test.rs
💤 Files with no reviewable changes (1)
- frontend/tsconfig.tsbuildinfo
| - DASHBOARD_NO_COLOR=true | ||
| # Required: set in a .env file or export in your shell before running. | ||
| # Example .env entry: DASHBOARD_PASSWORD=yourpassword | ||
| - DASHBOARD_PASSWORD=${DASHBOARD_PASSWORD:?Set DASHBOARD_PiASSWORD in a .env file or your shell environment} |
There was a problem hiding this comment.
Typo in the strict-substitution error message.
DASHBOARD_PiASSWORD (extra i) will be shown to operators when the variable is unset, undermining the error message's usefulness.
✏️ Proposed fix
- - DASHBOARD_PASSWORD=${DASHBOARD_PASSWORD:?Set DASHBOARD_PiASSWORD in a .env file or your shell environment}
+ - DASHBOARD_PASSWORD=${DASHBOARD_PASSWORD:?Set DASHBOARD_PASSWORD in a .env file or your shell environment}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - DASHBOARD_PASSWORD=${DASHBOARD_PASSWORD:?Set DASHBOARD_PiASSWORD in a .env file or your shell environment} | |
| - DASHBOARD_PASSWORD=${DASHBOARD_PASSWORD:?Set DASHBOARD_PASSWORD in a .env file or your shell environment} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker/docker-compose.yml` at line 81, The strict-substitution message for
the DASHBOARD_PASSWORD environment variable contains a typo
("DASHBOARD_PiASSWORD") which will be shown to operators if the variable is
unset; update the substitution on the DASHBOARD_PASSWORD line (the
${DASHBOARD_PASSWORD:?...} expression in docker-compose.yml) to use the correct
message text (e.g., "Set DASHBOARD_PASSWORD in a .env file or your shell
environment") so the error shown when the variable is missing is accurate.
| if (res.status === 401 && !path.startsWith("/auth/")) { | ||
| window.location.href = "/login"; | ||
| return Promise.reject(new ApiError(401, "Unauthorized")); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Preserve the originating URL across the login redirect.
The hard redirect always lands the user on /login and discards where they were trying to go. After authenticating they will be dropped at the SPA root rather than the deep link they originally requested, which is a noticeable UX regression for bookmarked/deep-linked dashboard pages. Also consider a guard so a 401 fired while already on /login does not trigger a redundant full navigation.
♻️ Proposed refactor
- if (res.status === 401 && !path.startsWith("/auth/")) {
- window.location.href = "/login";
- return Promise.reject(new ApiError(401, "Unauthorized"));
- }
+ if (res.status === 401 && !path.startsWith("/auth/")) {
+ if (window.location.pathname !== "/login") {
+ const next = encodeURIComponent(
+ window.location.pathname + window.location.search,
+ );
+ window.location.href = `/login?next=${next}`;
+ }
+ return Promise.reject(new ApiError(401, "Unauthorized"));
+ }The login route can then read ?next= and navigate() there after auth.login() succeeds.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (res.status === 401 && !path.startsWith("/auth/")) { | |
| window.location.href = "/login"; | |
| return Promise.reject(new ApiError(401, "Unauthorized")); | |
| } | |
| if (res.status === 401 && !path.startsWith("/auth/")) { | |
| if (window.location.pathname !== "/login") { | |
| const next = encodeURIComponent( | |
| window.location.pathname + window.location.search, | |
| ); | |
| window.location.href = `/login?next=${next}`; | |
| } | |
| return Promise.reject(new ApiError(401, "Unauthorized")); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/api.ts` around lines 227 - 230, The current 401 handling in the
response branch (the res.status === 401 && !path.startsWith("/auth/") check)
does a hard redirect to "/login" and drops the original URL; change it to
preserve the originating URL by building a next query param (e.g. const next =
encodeURIComponent(window.location.pathname + window.location.search +
window.location.hash) and set window.location.href = `/login?next=${next}`)
instead of plain "/login", and add a guard so if the current location is already
on "/login" (or path.startsWith("/auth/") covers this) you do not perform
another redirect; keep returning Promise.reject(new ApiError(401,
"Unauthorized")) as before so callers still handle the error. Ensure the login
route reads the next param and navigates there after successful auth.
| pub struct SessionStore { | ||
| sessions: HashMap<String, Instant>, | ||
| } | ||
|
|
||
| impl SessionStore { | ||
| pub fn new() -> Self { | ||
| Self { | ||
| sessions: HashMap::new(), | ||
| } | ||
| } | ||
|
|
||
| /// Creates a new session with a 24 h TTL. | ||
| /// Returns the session token (32 random bytes, hex-encoded). | ||
| pub fn create(&mut self) -> String { | ||
| let mut bytes = [0u8; 32]; | ||
| thread_rng().fill_bytes(&mut bytes); | ||
| let token = hex::encode(bytes); | ||
| let expiry = Instant::now() + Duration::from_secs(86400); | ||
| self.sessions.insert(token.clone(), expiry); | ||
| token | ||
| } | ||
|
|
||
| /// Returns `true` if `token` exists and hasn't expired. | ||
| /// Prunes all expired sessions on every call. | ||
| pub fn validate(&mut self, token: &str) -> bool { | ||
| let now = Instant::now(); | ||
| self.sessions.retain(|_, expiry| *expiry > now); | ||
| self.sessions.get(token).is_some_and(|expiry| *expiry > now) | ||
| } | ||
|
|
||
| /// Removes `token` from the store. | ||
| pub fn remove(&mut self, token: &str) { | ||
| self.sessions.remove(token); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the login cookie is a browser-session cookie (no Max-Age / Expires).
fd -t f 'auth.rs' src/api --exec cat {} \;Repository: citadel-tech/maker-dashboard
Length of output: 2810
🏁 Script executed:
# Verify the login endpoint cookie configuration and SessionStore implementation
cat -n src/auth.rs | sed -n '122,156p'Repository: citadel-tech/maker-dashboard
Length of output: 1379
Remove Max-Age=86400 from login cookie to satisfy session-only requirement.
The code sets a persistent cookie with Max-Age=86400 in src/api/auth.rs (login endpoint), which contradicts the stated requirement "Login must not be cached in the browser / no persisted session across browser restarts." This allows the session to survive browser restarts for up to 24 hours. Omit Max-Age and Expires to make it a true session cookie cleared on browser close.
Additionally, SessionStore uses Instant for session expiry (line 139 in src/auth.rs), which does not advance during system suspend on macOS pre-Sonoma and some Linux configurations. If a server process survives suspend, sessions will incorrectly remain valid beyond their intended 24h window. Consider using SystemTime or wall-clock time at issue + absolute deadline instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/auth.rs` around lines 122 - 156, Remove the persistent cookie flags from
the login response by omitting Max-Age/Expires (and any Expires header) when
setting the auth cookie in the login handler so it becomes a browser-session
cookie; keep SameSite/Secure/HttpOnly as-is. Replace Instant-based expiry in
SessionStore with wall-clock times (use std::time::SystemTime or a UNIX
timestamp) by changing the sessions map type and the create and validate
methods: in create() set expiry = SystemTime::now() + Duration::from_secs(86400)
(or store expiry as now().duration_since(UNIX_EPOCH).as_secs() + 86400), in
validate() prune entries using SystemTime::now() comparisons and check token
existence against those wall-clock expiries; update remove() unchanged. Ensure
you reference the SessionStore struct and its create()/validate() methods when
making these edits so suspend/clock changes won’t incorrectly extend sessions.
| /// Loads any previously saved maker configs and re-initializes them (but does NOT start servers). | ||
| pub fn new(config_dir: PathBuf) -> Result<Self> { | ||
| let persistence = PersistenceManager::new(config_dir.clone())?; | ||
| pub fn new(config_dir: PathBuf, enc_key: Option<[u8; 32]>) -> Result<Self> { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
What is the current recommended way to handle AES-256 key material in Rust to ensure zeroization on drop?
💡 Result:
The current recommended way to handle AES-256 key material in Rust for zeroization on drop is to use the aes crate from RustCrypto (with the zeroize feature enabled) for the AES primitive, combined with the aes-gcm crate for AEAD usage, and wrap the raw key bytes in secrecy::Secret<[u8; 32]> (or secrecy::SecretVec for derived keys) from the secrecy crate. This ensures the key is securely zeroized on drop via zeroize::ZeroizeOnDrop, prevents accidental logging via redacted Debug impls, and requires explicit exposure via ExposeSecret. Key workflow: 1. Generate or load key into secrecy::Secret<GenericArray<u8, U32>> or equivalent. 2. Initialize Aes256Gcm::new(key.expose_secret) - the cipher itself zeroizes internal state if zeroize enabled. 3. Use for encryption/decryption; key wrapper handles zeroization. Example: use aes_gcm::{Aes256Gcm, Key, KeyInit, Nonce}; use secrecy::{ExposeSecret, Secret}; let key_bytes: [u8; 32] = /* generate/load */; let secret_key = Secret::new(Key::::from_slice(&key_bytes)); let cipher = Aes256Gcm::new(secret_key.expose_secret); // Use cipher... // On drop, secret_key zeroizes the key bytes. Dependencies in Cargo.toml: [dependencies] aes-gcm = { version = "0.10", features = ["zeroize"] } # or latest aes = { version = "0.9", features = ["zeroize"] } secrecy = { version = "0.8", features = ["alloc"] } zeroize = { version = "1", features = ["zeroize_derive"] } Rationale: RustCrypto crates (aes, aes-gcm) implement Zeroize when feature enabled, zeroizing expanded key schedules and GHASH keys. secrecy provides higher-level secret management with zeroization and exposure control, standard in Rust crypto ecosystem. Avoid stack-allocated raw arrays due to Rust's move/copy semantics leaving copies (use heap via SecretBox or GenericArray).
Citations:
- 1: https://docs.rs/aes-gcm/latest/aes_gcm/
- 2: https://github.com/RustCrypto/AEADs/blob/master/aes-gcm/src/lib.rs
- 3: https://docs.rs/secrecy/latest/secrecy/
- 4: https://docs.rs/aes/latest/aes/
- 5: https://github.com/RustCrypto/block-ciphers/blob/master/aes/src/lib.rs
🏁 Script executed:
# First, check the Cargo.toml for current dependencies
find . -name "Cargo.toml" -type f | head -5Repository: citadel-tech/maker-dashboard
Length of output: 85
🏁 Script executed:
# Look at the structure of the repository
ls -laRepository: citadel-tech/maker-dashboard
Length of output: 1734
🏁 Script executed:
# Examine the file under review
head -150 src/maker_manager/mod.rsRepository: citadel-tech/maker-dashboard
Length of output: 5070
🏁 Script executed:
# Read Cargo.toml to see what dependencies are available
cat Cargo.tomlRepository: citadel-tech/maker-dashboard
Length of output: 1353
🏁 Script executed:
# Find the PersistenceManager implementation
find . -name "persistence.rs" -type fRepository: citadel-tech/maker-dashboard
Length of output: 107
🏁 Script executed:
# Search for existing zeroize or secrecy usage in the codebase
rg "zeroize|secrecy" --type rsRepository: citadel-tech/maker-dashboard
Length of output: 99
🏁 Script executed:
# Search without type specification
rg "zeroize|secrecy"Repository: citadel-tech/maker-dashboard
Length of output: 192
🏁 Script executed:
# Read the persistence.rs file to see how enc_key is used
wc -l src/maker_manager/persistence.rsRepository: citadel-tech/maker-dashboard
Length of output: 109
🏁 Script executed:
# Read the persistence.rs file
cat src/maker_manager/persistence.rsRepository: citadel-tech/maker-dashboard
Length of output: 10122
🏁 Script executed:
# Find the auth module
find . -name "auth.rs" -o -name "auth" -type dRepository: citadel-tech/maker-dashboard
Length of output: 104
🏁 Script executed:
# Check how aes_encrypt and aes_decrypt are implemented
rg "fn aes_encrypt|fn aes_decrypt" -A 20Repository: citadel-tech/maker-dashboard
Length of output: 1803
Wrap enc_key in a secure secret type to ensure automatic zeroization on drop.
enc_key: Option<[u8; 32]> is Copy, so key bytes are duplicated on the caller's stack and into PersistenceManager, where they persist for the process lifetime with no explicit zeroization. Use secrecy::Secret<[u8; 32]> (or heap-based secrecy::SecretBox<[u8; 32]>) at all layers to ensure automatic zeroization on drop and prevent accidental logging via redacted Debug impls. This significantly reduces memory-disclosure risk from core dumps, swap, or debug logs. As a prerequisite, add secrecy as a dependency with the alloc feature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/maker_manager/mod.rs` at line 111, The constructor signature and stored
field use a plain Option<[u8; 32]> which duplicates key bytes and never
zeroizes; change the API to accept and store a secrecy::Secret (or heap-backed
secrecy::SecretBox) instead — i.e., update the new(...) parameter from enc_key:
Option<[u8; 32]> to enc_key: Option<secrecy::Secret<[u8; 32]>> (or
Option<secrecy::SecretBox<[u8; 32]>>) and update PersistenceManager to hold the
same Secret type so keys are zeroized on drop; also add the secrecy dependency
with the alloc feature in Cargo.toml and adjust any callers of
maker_manager::new and places constructing PersistenceManager to wrap raw key
bytes into the chosen secrecy type before passing.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
docker/.env.example (1)
3-4:⚠️ Potential issue | 🟠 MajorDo not ship a real default credential.
changemeis a valid password, so copying this template can start the dashboard with a universally known secret. LeaveDASHBOARD_PASSWORDblank or use a non-usable placeholder so startup fails until the operator sets their own value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/.env.example` around lines 3 - 4, The .env example currently ships a real default credential (DASHBOARD_PASSWORD=changeme); remove the usable default by leaving DASHBOARD_PASSWORD empty or replacing it with an explicit non-usable placeholder so startup fails until an operator sets it (e.g., DASHBOARD_PASSWORD= or DASHBOARD_PASSWORD=__REQUIRED__). Update any README or startup docs that reference DASHBOARD_PASSWORD to instruct users to set a secure password before starting the dashboard.frontend/app/routes/login.tsx (2)
17-23:⚠️ Potential issue | 🟡 MinorReturn users to their original route after login.
navigate("/")still drops the location that triggered the auth redirect, so deep links land at the dashboard root instead of the requested page. Read the saved destination andreplaceback to it after successful authentication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/routes/login.tsx` around lines 17 - 23, In handleSubmit, instead of always calling navigate("/"), read the saved destination used by the auth redirect (e.g. the stored location in navigation state like location.state?.from or the session/localStorage key your auth redirect sets) and call navigate(savedDestination || "/", { replace: true }) so successful logins return the user to the original route; update the handleSubmit function to obtain that savedDestination and use navigate(..., { replace: true }) after await auth.login(password).
62-81:⚠️ Potential issue | 🟠 MajorThis still allows the browser to persist the dashboard password.
The form/input have no
autoCompletehints, so browsers can still offer to save or autofill this credential, which conflicts with issue#77. Add form-levelautoComplete="off"and an explicit password token on the field to suppress persistence as much as browsers allow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/routes/login.tsx` around lines 62 - 81, Add form-level autoComplete="off" to the <form> that uses handleSubmit, and set an explicit password autocomplete token on the password <input> (e.g., autoComplete="new-password") alongside the existing id="password", type handling (showPassword), value={password} and onChange={setPassword} so browsers are instructed not to persist or autofill this dashboard password.frontend/app/api.ts (1)
227-230:⚠️ Potential issue | 🟡 MinorPreserve the current URL when forcing a login redirect.
This sends every 401 to plain
/login, dropping the attempted route and hash, and it can re-navigate even if the app is already on/login. Encode the current location into the redirect target so the login page can send the user back after authentication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api.ts` around lines 227 - 230, The 401 handler currently forces a plain redirect to "/login" losing the attempted route and causing redundant redirects; update the logic in the block that checks res.status === 401 (the code that sets window.location.href and returns new ApiError) to build a redirect URL that preserves the current location (combine location.pathname + location.search + location.hash), URL-encode it (use encodeURIComponent) and append it as a query param such as "?redirect=<encodedLocation>" to "/login", and only perform the redirect if the current location is not already a login page (e.g., path does not startWith "/login" or "/auth/") so you don't re-navigate while on login; keep the ApiError return behavior unchanged.src/maker_manager/mod.rs (1)
111-112:⚠️ Potential issue | 🟠 MajorDo not keep the encryption key in
Copybyte arrays.
Option<[u8; 32]>duplicates key material and never zeroizes it on drop. Carry the derived key throughMakerManagerandPersistenceManagerin a secret wrapper such assecrecy::Secret/SecretBoxinstead of raw arrays.Read-only verification
#!/bin/bash rg -n 'enc_key:\s*Option<\[u8; 32\]>' src rg -n '\bsecrecy\b' Cargo.toml src🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/maker_manager/mod.rs` around lines 111 - 112, The constructor currently accepts enc_key: Option<[u8; 32]> which copies key material; change the type to use a secrecy wrapper (e.g. secrecy::Secret<[u8; 32]> or SecretVec<u8>) throughout and pass/move the secret rather than copying it: update the signature of MakerManager::new (enc_key) and the call to PersistenceManager::new to accept Option<Secret<...>> (or SecretVec) and update PersistenceManager::new' s parameter and fields accordingly so the key is stored in a non-Copy secret type that will zeroize on drop; ensure you stop using array Copy semantics (no cloning into raw [u8;32]) and use secrecy::ExposeSecret only where necessary.src/api/mod.rs (1)
61-97:⚠️ Potential issue | 🟡 MinorAdd the auth handlers to
ApiDoc.
auth::routes()is now live, butlogin,logout,status, androtate_passwordare still missing from#[openapi(paths(...))], so the generated spec/Swagger UI stays incomplete for the new auth surface.Also applies to: 138-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/mod.rs` around lines 61 - 97, The OpenAPI paths list for ApiDoc is missing the auth handlers, so add the four auth route functions to the paths(...) in the #[openapi(...)] macro: include auth::login, auth::logout, auth::status, and auth::rotate_password (and mirror the same additions in the other ApiDoc/openapi declaration mentioned around the duplicate region). This will ensure the auth::routes() endpoints are represented in the generated spec/Swagger UI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@SECURITY.md`:
- Around line 27-29: The SECURITY.md text that describes the session cookie
("session cookie (`HttpOnly`, `Secure`, `SameSite=Strict`, 24 h expiry)") is
incorrect given issue `#77`; replace the explicit "24 h expiry" with wording that
the cookie must be a non-persistent session cookie (i.e., omit Expires/Max-Age)
so a new browser session requires re-login, and keep the existing HttpOnly,
Secure, SameSite=Strict attributes intact.
In `@src/api/auth.rs`:
- Around line 143-166: The current sequence calls
makers.lock().await.rotate_enc_key(Some(new_key)) and then
new_auth.save(&config_dir), which can leave makers.json re-encrypted with the
new key while auth.json still points to the old key if the process crashes;
implement a two-phase/journaled rotation: create a small journal state file
(e.g., "auth.rotate.journal") and change the flow in the password-rotation path
so you (1) write new_auth to a temporary file (use new_auth.save_to_tmp or
similar), (2) write a journal entry recording the pending new key/step, (3)
re-encrypt makers.json to a temporary file via
makers.lock().await.rotate_enc_key(Some(new_key)) but target a temp path, (4)
atomically rename the temp makers.json into place, (5) atomically rename the
temp auth.json into place (or save new_auth.finalize), and (6) remove the
journal; also add startup recovery logic that checks the journal and either
completes the pending rename sequence or rolls back (calling
rotate_enc_key(old_key) if necessary using
auth.read().unwrap().derive_key(&body.old_password) to recover the old key) so
partial failure is recoverable. Ensure all temp writes and renames use atomic
filesystem renames and update the code paths around
makers.lock().await.rotate_enc_key, new_auth.save, and startup auth
initialization to handle the journal.
- Around line 48-50: The cookie construction currently includes "Max-Age=86400"
which makes the cookie persistent across browser restarts; update the cookie
string built around the token (created by sessions.lock().await.create()) to
omit the "Max-Age=86400" attribute so it becomes a session cookie (keep
HttpOnly, Secure, SameSite=Strict, Path=/), and ensure any 24-hour lifetime
checks remain enforced server-side in your session store/validation logic rather
than in the cookie.
In `@src/maker_manager/mod.rs`:
- Around line 289-293: rotate_enc_key currently calls
persistence.update_enc_key(new_key) before persistence.save(&self.configs),
leaving the in-memory key changed if save fails; change this so the new
encryption key is only committed after a successful save (or revert the
in-memory key on error). Specifically, in rotate_enc_key, first attempt to
persist the re-encrypted configs using persistence.save (or save to a temp
buffer/file via the persistence API), and only call
persistence.update_enc_key(new_key) after save returns Ok; if save returns Err,
ensure the original key remains set (roll back any temporary state) so the
manager does not remain in a mixed-key state. Ensure references to
rotate_enc_key, persistence.update_enc_key, persistence.save, and self.configs
are used so reviewers can locate the change.
In `@tests/api/mod.rs`:
- Around line 42-50: The test_app() setup currently constructs AppState and
returns api_router() directly, skipping the real auth middleware used in
src/server.rs so protected endpoints aren't exercised; update test_app() to wrap
the returned router with the same authentication layer or use the same
router-construction path as in src/server.rs (reuse the auth middleware function
or builder that composes api_router() with the auth/session middleware), or
alternatively add an authenticated test helper that injects a valid session into
AppState and applies the auth middleware before returning the test router;
locate test_app(), AppState, and api_router() in tests/api/mod.rs and mimic the
middleware composition from the server bootstrap (the auth middleware
initializer or function used in src/server.rs) so HTTP tests go through the
actual auth boundary.
---
Duplicate comments:
In `@docker/.env.example`:
- Around line 3-4: The .env example currently ships a real default credential
(DASHBOARD_PASSWORD=changeme); remove the usable default by leaving
DASHBOARD_PASSWORD empty or replacing it with an explicit non-usable placeholder
so startup fails until an operator sets it (e.g., DASHBOARD_PASSWORD= or
DASHBOARD_PASSWORD=__REQUIRED__). Update any README or startup docs that
reference DASHBOARD_PASSWORD to instruct users to set a secure password before
starting the dashboard.
In `@frontend/app/api.ts`:
- Around line 227-230: The 401 handler currently forces a plain redirect to
"/login" losing the attempted route and causing redundant redirects; update the
logic in the block that checks res.status === 401 (the code that sets
window.location.href and returns new ApiError) to build a redirect URL that
preserves the current location (combine location.pathname + location.search +
location.hash), URL-encode it (use encodeURIComponent) and append it as a query
param such as "?redirect=<encodedLocation>" to "/login", and only perform the
redirect if the current location is not already a login page (e.g., path does
not startWith "/login" or "/auth/") so you don't re-navigate while on login;
keep the ApiError return behavior unchanged.
In `@frontend/app/routes/login.tsx`:
- Around line 17-23: In handleSubmit, instead of always calling navigate("/"),
read the saved destination used by the auth redirect (e.g. the stored location
in navigation state like location.state?.from or the session/localStorage key
your auth redirect sets) and call navigate(savedDestination || "/", { replace:
true }) so successful logins return the user to the original route; update the
handleSubmit function to obtain that savedDestination and use navigate(..., {
replace: true }) after await auth.login(password).
- Around line 62-81: Add form-level autoComplete="off" to the <form> that uses
handleSubmit, and set an explicit password autocomplete token on the password
<input> (e.g., autoComplete="new-password") alongside the existing
id="password", type handling (showPassword), value={password} and
onChange={setPassword} so browsers are instructed not to persist or autofill
this dashboard password.
In `@src/api/mod.rs`:
- Around line 61-97: The OpenAPI paths list for ApiDoc is missing the auth
handlers, so add the four auth route functions to the paths(...) in the
#[openapi(...)] macro: include auth::login, auth::logout, auth::status, and
auth::rotate_password (and mirror the same additions in the other ApiDoc/openapi
declaration mentioned around the duplicate region). This will ensure the
auth::routes() endpoints are represented in the generated spec/Swagger UI.
In `@src/maker_manager/mod.rs`:
- Around line 111-112: The constructor currently accepts enc_key: Option<[u8;
32]> which copies key material; change the type to use a secrecy wrapper (e.g.
secrecy::Secret<[u8; 32]> or SecretVec<u8>) throughout and pass/move the secret
rather than copying it: update the signature of MakerManager::new (enc_key) and
the call to PersistenceManager::new to accept Option<Secret<...>> (or SecretVec)
and update PersistenceManager::new' s parameter and fields accordingly so the
key is stored in a non-Copy secret type that will zeroize on drop; ensure you
stop using array Copy semantics (no cloning into raw [u8;32]) and use
secrecy::ExposeSecret only where necessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8155ab3d-ea32-4ff8-bbc7-8ec9abb46594
📒 Files selected for processing (11)
SECURITY.mddocker/.env.examplefrontend/app/api.tsfrontend/app/components/Nav.tsxfrontend/app/routes/login.tsxsrc/api/auth.rssrc/api/mod.rssrc/maker_manager/mod.rssrc/maker_manager/persistence.rssrc/server.rstests/api/mod.rs
| // Re-encrypt makers.json with the new key | ||
| if let Err(e) = makers.lock().await.rotate_enc_key(Some(new_key)) { | ||
| return ( | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| Json(ApiResponse::<()>::err(format!( | ||
| "Failed to re-encrypt makers config: {e}" | ||
| ))), | ||
| ) | ||
| .into_response(); | ||
| } | ||
|
|
||
| // Atomically write the new auth.json; on failure roll back makers.json | ||
| if let Err(e) = new_auth.save(&config_dir) { | ||
| // Best-effort rollback: restore the old enc key so makers.json stays consistent | ||
| let old_key = auth.read().unwrap().derive_key(&body.old_password).ok(); | ||
| let _ = makers.lock().await.rotate_enc_key(old_key); | ||
| return ( | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| Json(ApiResponse::<()>::err(format!( | ||
| "Failed to save auth config: {e}" | ||
| ))), | ||
| ) | ||
| .into_response(); | ||
| } |
There was a problem hiding this comment.
Password rotation is not crash-safe across makers.json and auth.json.
Line 144 persists makers.json with the new key before Line 155 commits auth.json. A crash or kill between those two writes leaves old auth metadata pointing at new ciphertext, and the next startup can no longer decrypt maker state. This needs a recoverable two-phase rotation/journal, not two independent atomic writes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/auth.rs` around lines 143 - 166, The current sequence calls
makers.lock().await.rotate_enc_key(Some(new_key)) and then
new_auth.save(&config_dir), which can leave makers.json re-encrypted with the
new key while auth.json still points to the old key if the process crashes;
implement a two-phase/journaled rotation: create a small journal state file
(e.g., "auth.rotate.journal") and change the flow in the password-rotation path
so you (1) write new_auth to a temporary file (use new_auth.save_to_tmp or
similar), (2) write a journal entry recording the pending new key/step, (3)
re-encrypt makers.json to a temporary file via
makers.lock().await.rotate_enc_key(Some(new_key)) but target a temp path, (4)
atomically rename the temp makers.json into place, (5) atomically rename the
temp auth.json into place (or save new_auth.finalize), and (6) remove the
journal; also add startup recovery logic that checks the journal and either
completes the pending rename sequence or rolls back (calling
rotate_enc_key(old_key) if necessary using
auth.read().unwrap().derive_key(&body.old_password) to recover the old key) so
partial failure is recoverable. Ensure all temp writes and renames use atomic
filesystem renames and update the code paths around
makers.lock().await.rotate_enc_key, new_auth.save, and startup auth
initialization to handle the journal.
| /// Re-encrypts all maker configs with `new_key` and saves to disk. | ||
| /// Call after rotating the dashboard password so makers.json uses the new key. | ||
| pub fn rotate_enc_key(&mut self, new_key: Option<[u8; 32]>) -> anyhow::Result<()> { | ||
| self.persistence.update_enc_key(new_key); | ||
| self.persistence.save(&self.configs) |
There was a problem hiding this comment.
rotate_enc_key() leaves the process in a mixed-key state on write failure.
update_enc_key(new_key) happens before save(). If that write fails, the manager keeps the new key in memory while auth.json and the existing makers.json can still correspond to the old password. Any later persist can then silently re-encrypt with the new key and make the next restart fail. Only commit the new key after a successful rewrite, or roll it back on error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/maker_manager/mod.rs` around lines 289 - 293, rotate_enc_key currently
calls persistence.update_enc_key(new_key) before
persistence.save(&self.configs), leaving the in-memory key changed if save
fails; change this so the new encryption key is only committed after a
successful save (or revert the in-memory key on error). Specifically, in
rotate_enc_key, first attempt to persist the re-encrypted configs using
persistence.save (or save to a temp buffer/file via the persistence API), and
only call persistence.update_enc_key(new_key) after save returns Ok; if save
returns Err, ensure the original key remains set (roll back any temporary state)
so the manager does not remain in a mixed-key state. Ensure references to
rotate_enc_key, persistence.update_enc_key, persistence.save, and self.configs
are used so reviewers can locate the change.
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/api/mod.rs (1)
40-45: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueTemp dirs from
test_app()are never cleaned up after the test.The
config_diris removed only if it pre-exists from a previous run (lines 42–44) and recreated, but nothing in the helper deletes it once the test finishes — so each test run leaves a freshmaker-api-test-<pid>-<n>/directory under the system temp. Consider usingtempfile::TempDir(already a transitive dep in most workspaces) so the directory is dropped with the value, or returning the path so the test canfs::remove_dir_allon teardown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/api/mod.rs` around lines 40 - 45, The temp directory created by test_app() using the local variable config_dir is never removed after tests, leaking maker-api-test-<pid>-<n> folders; replace the manual create/remove logic with a tempfile::TempDir (or return config_dir so callers can remove it) so the directory is automatically deleted on drop — update the code that constructs config_dir to use tempfile::TempDir::new() (or return the TempDir from test_app()) and adjust any consumers to use TempDir.path() instead of the raw PathBuf.
♻️ Duplicate comments (17)
SECURITY.md (2)
73-77:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCapitalization:
it only removes→It only removes.Sentence-start lowercase typo persists.
✏️ Proposed fix
-not reclaim those funds. it only removes the dashboard registration. The locked output +not reclaim those funds. It only removes the dashboard registration. The locked output🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SECURITY.md` around lines 73 - 77, In SECURITY.md fix the sentence-start capitalization: change "it only removes the dashboard registration." to "It only removes the dashboard registration." so the sentence following "Deleting a maker from the dashboard does not reclaim those funds." starts with a capital letter.
10-12:⚠️ Potential issue | 🟠 Major | ⚡ Quick win24-hour cookie expiry still conflicts with issue
#77.Issue
#77requires "Login must not be cached in the browser. New sessions must require a new login (no persisted session across browser restarts)." A cookie advertised with a 24-hour expiry is a persistent cookie that survives browser restart, contradicting the requirement. Update the doc to describe the cookie as a non-persistent session cookie (noExpires/Max-Age) and note that the 24-hour TTL is enforced server-side bySessionStore. The implementation insrc/api/auth.rsline 119 also needs the matchingMax-Age=86400removed (covered separately).✏️ Proposed fix
-Once logged in, the browser holds a session cookie (`HttpOnly`, `Secure`, -`SameSite=Strict`, 24 h expiry). All `/api/*` routes reject requests without a -valid session with HTTP 401. +Once logged in, the browser holds a non-persistent session cookie (`HttpOnly`, +`Secure`, `SameSite=Strict`, no `Max-Age`/`Expires`). The cookie is dropped on +browser restart, and the server-side session has a 24 h TTL. All `/api/*` +routes reject requests without a valid session with HTTP 401.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SECURITY.md` around lines 10 - 12, Update SECURITY.md to describe the session cookie as a non-persistent session cookie (no Expires/Max-Age attributes so it does not survive browser restart) and state that the 24-hour TTL is enforced server-side by SessionStore; then remove the client-side Max-Age=86400 being set when issuing the cookie in the auth login handler (the function that sets the session cookie in auth.rs) so the browser only keeps a session cookie while the browser process is open and session expiration is enforced by SessionStore.src/api/auth.rs (3)
455-455:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLogout
clear_cookieis still missingSecure(inconsistent with login).The login cookie at line 119 sets
HttpOnly; Secure; SameSite=Strict, but the logout clear-cookie at line 455 omitsSecure. While browsers don't requireSecureon the matching deletion cookie, emitting an attribute set that would be acceptable over plaintext HTTP contradicts the security posture documented inSECURITY.mdand is an avoidable inconsistency. Mirror the issuance attributes:🔒 Proposed fix
- let clear_cookie = "session=; HttpOnly; SameSite=Strict; Path=/; Max-Age=0"; + let clear_cookie = "session=; HttpOnly; Secure; SameSite=Strict; Path=/; Max-Age=0";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/auth.rs` at line 455, The logout clear-cookie string assigned to clear_cookie omits the Secure attribute and should mirror the login cookie attributes; update the clear_cookie value (in src/api/auth.rs where clear_cookie is defined) to include Secure along with HttpOnly, SameSite=Strict, Path=/ and Max-Age=0 so the deletion cookie matches the issuance attributes used at login.
117-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Max-Age=86400violates issue#77— cookie persists across browser restarts.Issue
#77requires that "Login must not be cached in the browser" and "New sessions must require a new login (no persisted session across browser restarts)". WithMax-Age=86400, browsers treat the cookie as persistent and will re-send it after a restart for up to 24 hours. DropMax-Ageso it becomes a session cookie; keep the 24h TTL enforced bySessionStore::validateserver-side (seesrc/auth.rslines 138-145). The same fix is needed at line 220 forsetup.🔒 Proposed fix
- let cookie = - format!("session={token}; HttpOnly; Secure; SameSite=Strict; Path=/; Max-Age=86400"); + let cookie = + format!("session={token}; HttpOnly; Secure; SameSite=Strict; Path=/");(Apply the same change to the
setuphandler's cookie at line 220.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/auth.rs` around lines 117 - 119, The cookie is being set with a persistent Max-Age which violates session non-persistence; remove the "Max-Age=86400" attribute from the cookie string where the token is created (the code that calls sessions.lock().await.create() and builds the cookie with format!("session={token}; HttpOnly; Secure; SameSite=Strict; Path=/; Max-Age=86400")), and make the identical change in the setup handler where its cookie is built (the cookie at the other location around line 220); keep server-side TTL enforcement via SessionStore::validate (see SessionStore::validate in src/auth.rs) so sessions still expire after 24h but the browser cookie becomes a session cookie (no Max-Age).
49-126:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNo failed-login telemetry or rate limiting still leaves brute force unobservable.
/auth/loginis the sole authentication boundary protecting hot-wallet RPC credentials, and the handler still has notracing::warn!onverified == falseand no per-IP throttle. Argon2id raises the per-attempt cost but does not bound attempt rate, and once--allow-remoteis enabled the endpoint is reachable from arbitrary clients. At minimum, add a structured log on failed attempts so operators can detect ongoing brute force (ideally withaxum::extract::ConnectInfo<SocketAddr>for client IP). A small per-IP cooldown keyed offSessionStore(or a new failure counter) is the next step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/auth.rs` around lines 49 - 126, The login handler lacks telemetry and rate-limiting on failed attempts; update the async fn login signature to accept axum::extract::ConnectInfo<SocketAddr> to capture client IP, emit a structured tracing::warn! (or tracing::info!) when verified == false including fields like ip and username, and implement a simple per-IP cooldown using the existing SessionStore (or add a small failure counter map inside SessionStore) that checks before verification and increments on failure—return StatusCode::TOO_MANY_REQUESTS when the IP is throttled; use the symbols login, SessionStore (and its lock()/create()/existing API) and makers to locate where to add the IP param, logging, pre-check, increment-on-fail and expiration logic.src/maker_manager/mod.rs (1)
376-382:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
rotate_enc_key()still leaves a mixed-key state on save failure.Same concern as the prior review:
self.persistence.update_enc_key(new_key)runs beforeself.persistence.save(&self.configs), so a save error leaves the in-memory key pointing atnew_keywhilemakers.jsonon disk is still encrypted with the old key. Any later persist (e.g.create_maker,remove_maker,update_config, all of which callself.persist()) will silently re-encrypt with the new key, and the next restart will fail to decrypt.The API-layer rollback in
src/api/auth.rs(lines 321-336) papers over this by callingrotate_enc_key(old_key)onauth.savefailure, but it does not cover the case whererotate_enc_keyitself fails — at that pointself.persistence.enc_keyis already updated and you've returnedErr. Snapshot the previous key, swap-and-save, and restore on error so this primitive is safe to call directly:🛡️ Proposed fix
pub fn rotate_enc_key(&mut self, new_key: Option<[u8; 32]>) -> anyhow::Result<()> { if !self.unlocked { anyhow::bail!("MakerManager is locked; cannot rotate encryption key before login"); } - self.persistence.update_enc_key(new_key); - self.persistence.save(&self.configs) + let previous = self.persistence.current_enc_key(); // add a getter + self.persistence.update_enc_key(new_key); + if let Err(e) = self.persistence.save(&self.configs) { + self.persistence.update_enc_key(previous); + return Err(e); + } + Ok(()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/maker_manager/mod.rs` around lines 376 - 382, rotate_enc_key currently updates the in-memory encryption key via self.persistence.update_enc_key(new_key) before saving, which can leave the manager in a mixed-key state if save fails; change rotate_enc_key to snapshot the current key (e.g. let old = self.persistence.enc_key.clone()), perform the update, attempt self.persistence.save(&self.configs), and on any save error restore the previous key (call update_enc_key(old)) before returning Err so the in-memory state matches disk; ensure this uses the existing methods (rotate_enc_key, self.persistence.update_enc_key, self.persistence.save) and restores the exact previous key value on failure.src/api/mod.rs (1)
75-150:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
/auth/*endpoints are still missing from the OpenAPI surface.The
paths(...)block on theApiDocderive does not list any ofauth::login,auth::logout,auth::status,auth::setup, orauth::rotate_password, even thoughauth::routes()is merged intoapi_router()at line 150. Swagger UI and the publishedopenapi.jsonwill continue to omit the entire authentication surface, which is the most important set of endpoints for a security audit. Add#[utoipa::path(...)]attributes to each handler insrc/api/auth.rs(with request/response schemas forLoginRequest,SetupRequest,RotatePasswordRequest,AuthStatus) and reference them here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/mod.rs` around lines 75 - 150, The OpenAPI surface omits authentication endpoints because auth handlers lack #[utoipa::path(...)] and their schemas aren't referenced in ApiDoc; add #[utoipa::path] attributes to each handler in src/api/auth.rs (for functions auth::login, auth::logout, auth::status, auth::setup, auth::rotate_password) specifying request/response types (LoginRequest, SetupRequest, RotatePasswordRequest, AuthStatus, etc.), then include those handler paths in the ApiDoc paths(...) list and add the corresponding DTO types to the components(schemas(...)) section so the generated openapi.json/Swagger UI include the auth endpoints.frontend/app/api.ts (1)
227-230: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win401 still drops the originating URL on the redirect to
/login.After authenticating, the user is dropped at the SPA root rather than the deep link they originally requested (e.g.,
/makers/maker01/swaps). This is a noticeable UX regression for bookmarked dashboard pages. Pass the original location via?next=and have the login route navigate there post-auth; also guard against re-redirecting when already on/login.♻️ Proposed refactor
- if (res.status === 401 && !path.startsWith("/auth/")) { - window.location.href = "/login"; - return Promise.reject(new ApiError(401, "Unauthorized")); - } + if (res.status === 401 && !path.startsWith("/auth/")) { + if (window.location.pathname !== "/login") { + const next = encodeURIComponent( + window.location.pathname + window.location.search + window.location.hash, + ); + window.location.href = `/login?next=${next}`; + } + return Promise.reject(new ApiError(401, "Unauthorized")); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/api.ts` around lines 227 - 230, The 401 handler in frontend/app/api.ts (the block checking res.status === 401 and path.startsWith("/auth/")) should preserve the original location by redirecting to /login?next=... instead of dropping the deep link; modify that block to first guard against already being on /login (avoid redirect loop), compute a safe next value from the current location (e.g., pathname + search + hash and encodeURIComponent it), then set window.location.href = `/login?next=${encodedNext}` and still return Promise.reject(new ApiError(401, "Unauthorized")); keep the existing check for auth routes and reference the ApiError symbol when returning the rejection.frontend/app/components/Nav.tsx (1)
121-125:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError banner is not announced to assistive tech.
Same issue flagged on
login.tsx: this<div>has norole="alert"/aria-live, so screen-reader users won't be notified when password rotation fails. Addrole="alert".♿ Proposed fix
- <div className="px-3 py-2 bg-red-900/40 border border-red-700 rounded-lg text-sm text-red-300 animate-fade-in"> + <div + role="alert" + className="px-3 py-2 bg-red-900/40 border border-red-700 rounded-lg text-sm text-red-300 animate-fade-in" + > {error} </div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/components/Nav.tsx` around lines 121 - 125, The error banner in Nav.tsx (the JSX fragment rendering {error}) isn't announced to assistive tech; update the element that renders the error message to include an accessibility role/attribute such as role="alert" (or aria-live="assertive") so screen readers are notified when the {error} state appears; locate the JSX block that outputs {error} and add the role/aria-live attribute to that <div> (or replace it with an appropriate semantic element) and ensure the error text remains unchanged.frontend/app/routes/login.tsx (3)
62-81:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
autoCompleteattributes still absent on the login form.Previously flagged. Add
autoComplete="off"on the<form>andautoComplete="current-password"on the password input (or"new-password"if the goal is to suppress autosave entirely). Browsers ignoreautoComplete="off"on password fields, so the field-level value is required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/routes/login.tsx` around lines 62 - 81, The form currently lacks autocomplete attributes; update the <form onSubmit={handleSubmit}> element to include autoComplete="off" and add autoComplete="current-password" on the password <input id="password" ...> (or "new-password" if you want to suppress autosave entirely); ensure you update the JSX where showPassword, password, setPassword and loading are used so the attributes sit on the same elements that render the password field.
22-23: 🧹 Nitpick | 🔵 Trivial | 💤 Low valuePreserve the originally requested route on successful login.
Already raised in a previous review: redirecting unconditionally to
/discards any pre-login destination forwarded viaNavigate statefromAuthLayout. Usinglocation.state?.fromhere would restore the intended target.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/routes/login.tsx` around lines 22 - 23, The login flow unconditionally navigates to "/" after auth.login which discards any pre-login target; update the Login component to read the original target from the React Router location state (e.g., via useLocation and location.state?.from) and, after await auth.login(password), call navigate(location.state?.from ?? "/", { replace: true }) instead of navigate("/"), so successful login returns the user to their originally requested route; keep references to auth.login and navigate in the updated logic.
98-102:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError banner still missing
role="alert".Previously flagged — keeping a pointer because the autoComplete fix and accessibility fix have not been applied in the current diff.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/routes/login.tsx` around lines 98 - 102, The error banner rendered in frontend/app/routes/login.tsx inside the "{error && ( ... )}" JSX block is missing an accessibility role; add role="alert" to that <div> so assistive technologies announce the message (optionally also add aria-live="assertive" for redundancy). While you're in that component, re-apply the prior accessibility/autocomplete fixes to the login inputs (e.g., ensure the username/email and password inputs have proper autoComplete attributes) so the page is both announced and inputs behave correctly.frontend/app/main.tsx (2)
32-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAsync effect still has no cancellation/StrictMode guard.
Already raised in a previous review and not yet fixed: in
StrictMode(line 86 / 103) the effect mounts twice in dev soauth.status()runs twice; more importantly, if the layout unmounts before the promise resolves (fast navigation, login completes, etc.),setState/setCheckingare called on an unmounted component. Add acancelledflag in the effect cleanup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/main.tsx` around lines 32 - 43, The effect calling auth.status() in main.tsx needs a cancellation/StrictMode guard to avoid calling setState or setChecking after unmount; inside the useEffect that calls auth.status(), introduce a local `cancelled` boolean (false), and in the cleanup set `cancelled = true`; after the promise resolves (both in then and catch) check `if (!cancelled)` before calling setState({ initialized, authenticated }) or setState({ initialized: true, authenticated: false }) and before calling setChecking(false); this prevents state updates from the auth.status() promise if the component unmounts or StrictMode causes double mounting.
55-55: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueBlank page during auth check — duplicate of prior nit.
Still rendering
nullwhile checking; previously suggested replacing with a centered spinner to avoid the blank-frame flicker on every navigation/reload. Same applies to lines 68 and 80.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/main.tsx` at line 55, The code returns null when the auth state is being checked (the `checking` conditional in frontend/app/main.tsx), causing a blank frame; replace these `return null` instances (the one at `if (checking) return null;` and the similar returns around lines 68 and 80) with a centered loading indicator component instead (e.g., render an existing Spinner or a small wrapper like a div with centered styles that contains the spinner) so the app shows a consistent centered spinner during auth checks rather than a blank page.frontend/app/routes/setup.tsx (2)
24-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
autoComplete="new-password"to setup password inputs.Same concern as raised on
login.tsx: the form has noautoCompletedirectives, so browsers may save the chosen password. For the first-run setup screen both the password and confirm fields should be markedautoComplete="new-password"(and considerautoComplete="off"on the form). Expose anautoCompleteprop onPasswordInputand pass"new-password"from both call sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/routes/setup.tsx` around lines 24 - 34, The password <input> lacks an autoComplete attribute; add support for an autoComplete prop to the PasswordInput component and set it on the input element (e.g., <input ... autoComplete={autoComplete}>), then update both call sites (routes/setup.tsx and the other place using PasswordInput, e.g., routes/login.tsx) to pass autoComplete="new-password" for the password and confirm fields; optionally set the parent form's autoComplete="off" where appropriate.
135-139:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError banner missing
role="alert".Same accessibility gap flagged on
login.tsxandNav.tsx. Addrole="alert"so screen readers announce setup errors when they appear.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/routes/setup.tsx` around lines 135 - 139, The error banner div that renders {error} in routes/setup.tsx should include role="alert" so screen readers announce it; update the JSX where the conditional renders the error div (the element containing {error}) to add role="alert". Apply the same change to the equivalent error banner usages in login.tsx and Nav.tsx to ensure consistent accessibility.tests/api/mod.rs (1)
38-61:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftTests still bypass the production auth middleware — duplicate of prior comment.
Same concern as previously raised:
test_app()wiresAppState(includingsessionsandauth) but returnsapi_router().with_state(state)directly, without the auth/session middleware layer thatsrc/server.rscomposes. Protected endpoints will keep passing here regardless of whether the production auth boundary is correct, so middleware regressions cannot be caught by these HTTP tests. Either apply the same middleware here, or add a separate authenticated test helper that exercises the full stack.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/api/mod.rs` around lines 38 - 61, test_app currently returns api_router().with_state(state) and thus bypasses the production auth/session middleware (even though AppState includes sessions and auth), so update test_app to either apply the same middleware composition used in src/server.rs to the router it returns or replace it with a new authenticated test helper that builds the full production stack; specifically, modify test_app to call the same router factory or middleware-applier used in server.rs (the function that composes auth/session middleware around api_router) or add a helper (e.g., build_authenticated_test_app) that takes the AppState (makers, sessions, auth, setup_lock, config_dir) and returns the router with production auth middleware applied so protected endpoints are exercised by tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/app/api.ts`:
- Around line 413-414: The rotatePassword API should use camelCase parameter
names for TypeScript callers: rename the function signature parameters to
oldPassword and newPassword in rotatePassword, and when calling
post("/auth/rotate-password", ...) map them to the wire-format keys by sending {
old_password: oldPassword, new_password: newPassword }; keep the return type
Promise<void> and leave the backend request shape unchanged.
In `@frontend/app/components/Nav.tsx`:
- Around line 7-44: Extract the PasswordInput component into a shared file and
replace duplicate inline implementations with imports: create
frontend/app/components/PasswordInput.tsx exporting the existing PasswordInput
functional component (preserving props value, onChange, placeholder, autoFocus,
disabled, internal show state, toggle button, and classes) and update callers
(Nav's PasswordInput, frontend/app/routes/setup.tsx, and
frontend/app/routes/login.tsx) to import { PasswordInput } from that module;
ensure prop types/signature remain identical and move any specific attributes
like autoComplete or accessibility tweaks into the shared component so all three
consumers use the same implementation.
- Around line 63-64: After calling auth.rotatePassword(...) the code navigates
to "/login" without router state, so the login route's check of
location.state.passwordChanged won't see the flag; update the navigate call that
follows auth.rotatePassword(oldPassword, newPassword) to forward the flag (e.g.,
pass state: { passwordChanged: true }) so the login route (which reads
location.state.passwordChanged) can render the required banner; ensure the
navigate invocation near auth.rotatePassword is the only change and keep typings
if your router types require a specific shape for location.state.
- Around line 78-83: The overlay currently calls onClose on any backdrop click
even while auth.rotatePassword/loading is in flight; update the backdrop click
handler in Nav.tsx to only call onClose when !loading, add an Escape key
listener (useEffect) that triggers onClose only when !loading, and implement a
simple focus-trap/restore: on mount save document.activeElement, move focus into
the modal (first focusable element or a sentry) and on unmount restore the saved
focus; ensure you reference the existing overlay div's onClick handler, the
loading/auth.rotatePassword state, and the component unmount cleanup so any
in-flight redirect from rotatePassword doesn't run from an unmounted component.
- Around line 61-74: The success path never clears the loading state—move the
setLoading(false) into a finally block so it always runs; specifically, in the
password change flow that calls auth.rotatePassword(oldPassword, newPassword)
and navigate("/login"), remove the setLoading(false) call from the catch and
wrap the await auth.rotatePassword(...) / navigate(...) sequence in try { ... }
finally { setLoading(false) } while keeping the existing ApiError-based error
handling in the catch for setError messages.
In `@frontend/app/main.tsx`:
- Around line 25-46: Multiple components call useAuthStatus so each layout
independently re-fetches /api/auth/status; lift this into a top-level
AuthContext provider: create an AuthProvider component that holds checking and
state (initialize like useAuthStatus), perform the single auth.status() fetch in
AuthProvider's useEffect, expose { checking, state, refreshAuth } (or methods
like login/logout/rotate that call refreshAuth) via context, replace direct
calls to useAuthStatus in SetupLayout, GuestLayout, and AuthLayout with
useContext(AuthContext), and ensure refreshAuth is called on login/logout/rotate
to invalidate and re-fetch; keep initial initialized/authenticated defaults and
error handling consistent with the current useAuthStatus behavior.
- Around line 52-83: The three layout components (AuthLayout, GuestLayout,
SetupLayout) and the hook useAuthStatus are currently defined in the same file
as the app bootstrap (createRoot) which breaks Fast Refresh; extract AuthLayout,
GuestLayout, SetupLayout and useAuthStatus into their own module(s) under
frontend/app/auth/ (one file per component or a single auth module), export them
from those new files, then import and use them in main.tsx so main.tsx only
imports component modules; ensure the extracted files export the same named
functions (AuthLayout, GuestLayout, SetupLayout, useAuthStatus) and update all
imports in main.tsx accordingly to resolve the ESLint/React Refresh errors.
In `@frontend/app/routes/login.tsx`:
- Around line 1-8: The Login component calls useLocation() but useLocation is
not imported from react-router-dom; update the import statement that currently
imports useNavigate to also import useLocation (i.e., add useLocation to the
named imports from "react-router-dom") so the Login function can call
useLocation without runtime/compile errors.
In `@frontend/app/routes/onboarding.tsx`:
- Line 4: Remove the unused ArrowBigRight import from the import list in
onboarding.tsx to fix the ESLint failure: locate the import that includes the
symbol ArrowBigRight and delete only that identifier (or remove the entire
import line if it only contained ArrowBigRight), then run lint to confirm no
remaining unused-import errors.
In `@frontend/app/routes/setup.tsx`:
- Around line 55-88: The setup form currently only checks password non-empty and
confirmation in handleSubmit; add client-side password-strength/length
enforcement before calling auth.setup(password) (e.g., require min length 12 and
optionally check character variety) and show a small inline hint near the
password input when password is too weak; update handleSubmit to validate
(password and confirm) against the new policy, call setError with a descriptive
message (e.g., "Password must be at least 12 characters") and return early if
validation fails, and also add a realtime indicator (near the password input
component state tied to password) to render strength feedback to the user.
- Around line 64-87: Move the setLoading(false) call out of the catch block into
a finally so loading is always cleared after auth.setup completes (success or
failure); keep the 409 branch behavior by changing its setTimeout to call both
navigate("/login") and setLoading(false) inside the timer callback (instead of
leaving loading true while the timer runs or immediately clearing it), and
remove the setLoading(false) lines from the catch; update code around
setLoading, auth.setup, navigate, ApiError/err.status and setError accordingly.
In `@README.md`:
- Line 143: Fix the punctuation/capitalization in the remote-access note by
correcting "front. the built-in password auth is the only protection on the
wire." to either join the clauses or capitalize the start of the second
sentence; e.g., change "front. the built-in..." to "front. The built-in..." in
the README line containing "By default the dashboard only accepts connections
from the local machine. If you enable `--allow-remote`, place a TLS-terminating
reverse proxy in front. the built-in password auth is the only protection on the
wire."
In `@src/api/auth.rs`:
- Around line 138-166: The code holds setup_lock across the setup critical
section and later acquires makers.lock(), which can lead to an AB/BA deadlock if
other code ever acquires makers before setup_lock (e.g., create_maker); add a
concise module-level comment documenting and enforcing the lock ordering
invariant (for example: "Lock ordering invariant: setup_lock → makers → sessions
→ auth") near the top of src/api/auth.rs (or the file-level docblock) so future
contributors see and follow the required acquire order and avoid inverting it;
mention the specific symbols setup_lock, makers.lock(), create_maker in the
comment so it's discoverable.
- Around line 229-343: There are two definitions of rotate_password causing a
duplicate-symbol compile error; remove the entire second/stale definition (the
one that expects State(auth): State<Arc<RwLock<AuthConfig>>>) so only the
correct handler remains; ensure the remaining rotate_password continues to work
with Arc<RwLock<Option<AuthConfig>>> (as used by AppState::auth) and contains
the rollback path that re-derives old_key and calls
makers.lock().await.rotate_enc_key(old_key) before returning on save failure.
- Around line 138-227: The setup endpoint currently only checks
body.password.is_empty(); change validation in the setup handler to enforce a
minimum length (e.g. >= 8) and reject whitespace-only passwords by trimming and
checking length, returning StatusCode::BAD_REQUEST with a clear error message
when invalid (replace the existing empty check around body.password). Apply the
same validation logic to the rotate_password handler's new_password parameter
(the rotate_password function) so it rejects short or whitespace-only new
passwords the same way.
In `@src/api/mod.rs`:
- Around line 60-64: The FromRef impl for Arc<Mutex<()>> is fragile because any
other Arc<Mutex<()>> in AppState will cause ambiguous extraction; introduce a
newtype (e.g., struct SetupLock(pub Arc<Mutex<()>>)) and implement
FromRef<AppState> for SetupLock to return AppState.setup_lock wrapped in the
newtype, then update the handler signature in src/api/auth.rs (function setup)
to accept State<SetupLock> and replace direct locking calls with
lock.0.lock().await to access the inner mutex.
In `@src/maker_manager/mod.rs`:
- Around line 122-152: Refactor the envelope-detection logic out of
MakerManager::new by adding a PersistenceManager::is_encrypted_state() (or
similarly named) method and replacing calls to
Self::file_is_encrypted_envelope(&mgr.persistence) with
mgr.persistence.is_encrypted_state(); ensure the new PersistenceManager method
uses the same detection/parse logic as PersistenceManager::load so we don't
duplicate file reads/parsing, update MakerManager::new to call that method when
computing can_load, and remove or deprecate the private
MakerManager::file_is_encrypted_envelope helper to keep detection centralized.
In `@tests/api/mod.rs`:
- Around line 47-51: Tests call AuthConfig::new and AuthConfig::derive_key (slow
Argon2id ops) on every test_app() invocation; change this by creating a shared
static cached pair using OnceLock or Lazy (e.g., static SHARED_AUTH:
OnceLock<(AuthConfig, [u8;32])>) and replace the per-test calls to
AuthConfig::new("test-password") and .derive_key("test-password") with a call to
a helper shared_auth() that returns the cached (AuthConfig, key) tuple, then use
the cached key when creating MakerManager::new(...) and calling
manager.unlock(...).
---
Outside diff comments:
In `@tests/api/mod.rs`:
- Around line 40-45: The temp directory created by test_app() using the local
variable config_dir is never removed after tests, leaking
maker-api-test-<pid>-<n> folders; replace the manual create/remove logic with a
tempfile::TempDir (or return config_dir so callers can remove it) so the
directory is automatically deleted on drop — update the code that constructs
config_dir to use tempfile::TempDir::new() (or return the TempDir from
test_app()) and adjust any consumers to use TempDir.path() instead of the raw
PathBuf.
---
Duplicate comments:
In `@frontend/app/api.ts`:
- Around line 227-230: The 401 handler in frontend/app/api.ts (the block
checking res.status === 401 and path.startsWith("/auth/")) should preserve the
original location by redirecting to /login?next=... instead of dropping the deep
link; modify that block to first guard against already being on /login (avoid
redirect loop), compute a safe next value from the current location (e.g.,
pathname + search + hash and encodeURIComponent it), then set
window.location.href = `/login?next=${encodedNext}` and still return
Promise.reject(new ApiError(401, "Unauthorized")); keep the existing check for
auth routes and reference the ApiError symbol when returning the rejection.
In `@frontend/app/components/Nav.tsx`:
- Around line 121-125: The error banner in Nav.tsx (the JSX fragment rendering
{error}) isn't announced to assistive tech; update the element that renders the
error message to include an accessibility role/attribute such as role="alert"
(or aria-live="assertive") so screen readers are notified when the {error} state
appears; locate the JSX block that outputs {error} and add the role/aria-live
attribute to that <div> (or replace it with an appropriate semantic element) and
ensure the error text remains unchanged.
In `@frontend/app/main.tsx`:
- Around line 32-43: The effect calling auth.status() in main.tsx needs a
cancellation/StrictMode guard to avoid calling setState or setChecking after
unmount; inside the useEffect that calls auth.status(), introduce a local
`cancelled` boolean (false), and in the cleanup set `cancelled = true`; after
the promise resolves (both in then and catch) check `if (!cancelled)` before
calling setState({ initialized, authenticated }) or setState({ initialized:
true, authenticated: false }) and before calling setChecking(false); this
prevents state updates from the auth.status() promise if the component unmounts
or StrictMode causes double mounting.
- Line 55: The code returns null when the auth state is being checked (the
`checking` conditional in frontend/app/main.tsx), causing a blank frame; replace
these `return null` instances (the one at `if (checking) return null;` and the
similar returns around lines 68 and 80) with a centered loading indicator
component instead (e.g., render an existing Spinner or a small wrapper like a
div with centered styles that contains the spinner) so the app shows a
consistent centered spinner during auth checks rather than a blank page.
In `@frontend/app/routes/login.tsx`:
- Around line 62-81: The form currently lacks autocomplete attributes; update
the <form onSubmit={handleSubmit}> element to include autoComplete="off" and add
autoComplete="current-password" on the password <input id="password" ...> (or
"new-password" if you want to suppress autosave entirely); ensure you update the
JSX where showPassword, password, setPassword and loading are used so the
attributes sit on the same elements that render the password field.
- Around line 22-23: The login flow unconditionally navigates to "/" after
auth.login which discards any pre-login target; update the Login component to
read the original target from the React Router location state (e.g., via
useLocation and location.state?.from) and, after await auth.login(password),
call navigate(location.state?.from ?? "/", { replace: true }) instead of
navigate("/"), so successful login returns the user to their originally
requested route; keep references to auth.login and navigate in the updated
logic.
- Around line 98-102: The error banner rendered in frontend/app/routes/login.tsx
inside the "{error && ( ... )}" JSX block is missing an accessibility role; add
role="alert" to that <div> so assistive technologies announce the message
(optionally also add aria-live="assertive" for redundancy). While you're in that
component, re-apply the prior accessibility/autocomplete fixes to the login
inputs (e.g., ensure the username/email and password inputs have proper
autoComplete attributes) so the page is both announced and inputs behave
correctly.
In `@frontend/app/routes/setup.tsx`:
- Around line 24-34: The password <input> lacks an autoComplete attribute; add
support for an autoComplete prop to the PasswordInput component and set it on
the input element (e.g., <input ... autoComplete={autoComplete}>), then update
both call sites (routes/setup.tsx and the other place using PasswordInput, e.g.,
routes/login.tsx) to pass autoComplete="new-password" for the password and
confirm fields; optionally set the parent form's autoComplete="off" where
appropriate.
- Around line 135-139: The error banner div that renders {error} in
routes/setup.tsx should include role="alert" so screen readers announce it;
update the JSX where the conditional renders the error div (the element
containing {error}) to add role="alert". Apply the same change to the equivalent
error banner usages in login.tsx and Nav.tsx to ensure consistent accessibility.
In `@SECURITY.md`:
- Around line 73-77: In SECURITY.md fix the sentence-start capitalization:
change "it only removes the dashboard registration." to "It only removes the
dashboard registration." so the sentence following "Deleting a maker from the
dashboard does not reclaim those funds." starts with a capital letter.
- Around line 10-12: Update SECURITY.md to describe the session cookie as a
non-persistent session cookie (no Expires/Max-Age attributes so it does not
survive browser restart) and state that the 24-hour TTL is enforced server-side
by SessionStore; then remove the client-side Max-Age=86400 being set when
issuing the cookie in the auth login handler (the function that sets the session
cookie in auth.rs) so the browser only keeps a session cookie while the browser
process is open and session expiration is enforced by SessionStore.
In `@src/api/auth.rs`:
- Line 455: The logout clear-cookie string assigned to clear_cookie omits the
Secure attribute and should mirror the login cookie attributes; update the
clear_cookie value (in src/api/auth.rs where clear_cookie is defined) to include
Secure along with HttpOnly, SameSite=Strict, Path=/ and Max-Age=0 so the
deletion cookie matches the issuance attributes used at login.
- Around line 117-119: The cookie is being set with a persistent Max-Age which
violates session non-persistence; remove the "Max-Age=86400" attribute from the
cookie string where the token is created (the code that calls
sessions.lock().await.create() and builds the cookie with
format!("session={token}; HttpOnly; Secure; SameSite=Strict; Path=/;
Max-Age=86400")), and make the identical change in the setup handler where its
cookie is built (the cookie at the other location around line 220); keep
server-side TTL enforcement via SessionStore::validate (see
SessionStore::validate in src/auth.rs) so sessions still expire after 24h but
the browser cookie becomes a session cookie (no Max-Age).
- Around line 49-126: The login handler lacks telemetry and rate-limiting on
failed attempts; update the async fn login signature to accept
axum::extract::ConnectInfo<SocketAddr> to capture client IP, emit a structured
tracing::warn! (or tracing::info!) when verified == false including fields like
ip and username, and implement a simple per-IP cooldown using the existing
SessionStore (or add a small failure counter map inside SessionStore) that
checks before verification and increments on failure—return
StatusCode::TOO_MANY_REQUESTS when the IP is throttled; use the symbols login,
SessionStore (and its lock()/create()/existing API) and makers to locate where
to add the IP param, logging, pre-check, increment-on-fail and expiration logic.
In `@src/api/mod.rs`:
- Around line 75-150: The OpenAPI surface omits authentication endpoints because
auth handlers lack #[utoipa::path(...)] and their schemas aren't referenced in
ApiDoc; add #[utoipa::path] attributes to each handler in src/api/auth.rs (for
functions auth::login, auth::logout, auth::status, auth::setup,
auth::rotate_password) specifying request/response types (LoginRequest,
SetupRequest, RotatePasswordRequest, AuthStatus, etc.), then include those
handler paths in the ApiDoc paths(...) list and add the corresponding DTO types
to the components(schemas(...)) section so the generated openapi.json/Swagger UI
include the auth endpoints.
In `@src/maker_manager/mod.rs`:
- Around line 376-382: rotate_enc_key currently updates the in-memory encryption
key via self.persistence.update_enc_key(new_key) before saving, which can leave
the manager in a mixed-key state if save fails; change rotate_enc_key to
snapshot the current key (e.g. let old = self.persistence.enc_key.clone()),
perform the update, attempt self.persistence.save(&self.configs), and on any
save error restore the previous key (call update_enc_key(old)) before returning
Err so the in-memory state matches disk; ensure this uses the existing methods
(rotate_enc_key, self.persistence.update_enc_key, self.persistence.save) and
restores the exact previous key value on failure.
In `@tests/api/mod.rs`:
- Around line 38-61: test_app currently returns api_router().with_state(state)
and thus bypasses the production auth/session middleware (even though AppState
includes sessions and auth), so update test_app to either apply the same
middleware composition used in src/server.rs to the router it returns or replace
it with a new authenticated test helper that builds the full production stack;
specifically, modify test_app to call the same router factory or
middleware-applier used in server.rs (the function that composes auth/session
middleware around api_router) or add a helper (e.g.,
build_authenticated_test_app) that takes the AppState (makers, sessions, auth,
setup_lock, config_dir) and returns the router with production auth middleware
applied so protected endpoints are exercised by tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 02d1c371-3410-494f-a57c-b7a7e24b8653
📒 Files selected for processing (21)
.github/workflows/docker-build.ymlREADME.mdSECURITY.mddocker/.env.exampledocker/docker-compose.ymlfrontend/app/api.tsfrontend/app/components/Nav.tsxfrontend/app/main.tsxfrontend/app/routes/login.tsxfrontend/app/routes/onboarding.tsxfrontend/app/routes/setup.tsxpackaging/umbrel/dev.mdsrc/api/auth.rssrc/api/mod.rssrc/cli.rssrc/main.rssrc/maker_manager/mod.rssrc/maker_manager/persistence.rssrc/server.rssrc/utils/log_writer.rstests/api/mod.rs
keraliss
left a comment
keraliss
left a comment
There was a problem hiding this comment.
please change these two things
| import { useNavigate } from "react-router-dom"; | ||
| import { |
There was a problem hiding this comment.
add useLocation, else this is throwing an error.
There was a problem hiding this comment.
but we don't need it. can you show me the error?
2 participants
Close #77
Summary by CodeRabbit
New Features
Documentation
Chores