feat: Implement issues #549, #548, #541 - Secure session management, …#674
Conversation
…on#548, OpenKnight-Foundation#541 - Secure session management, WebSocket scaling, and PGN archiving - Issue OpenKnight-Foundation#549: Enhanced JWT session management with session tracking, IP binding, and refresh token rotation * Added session entity and migration for tracking active user sessions * Enhanced JWT claims with session_id and ip_hash fields * Implemented SessionService for creating, validating, and revoking sessions * Updated login endpoint to create sessions and bind tokens to session IDs * Added support for token reuse detection and theft prevention - Issue OpenKnight-Foundation#548: WebSocket scaling with horizontal load balancing * Enhanced LobbyState with distributed connection tracking * Added Redis Pub/Sub for cross-instance communication * Implemented connection event publishing for monitoring * Added instance_id tracking for multi-server deployments * Improved logging for connection/disconnection events - Issue OpenKnight-Foundation#541: Automated PGN archiving to IPFS/Arweave * Created PgnArchiveService for decentralized storage * Implemented IPFS integration with Pinata API * Implemented Arweave integration with bundler service * Added PGN integrity verification using SHA-256 hashing * Created archive-pgn and batch-archive-pgn API endpoints * Added proper error handling and logging All implementations follow existing code patterns and include proper documentation.
|
@A6dulmalik Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughThis PR implements three complementary features: server-side session management with JWT embedding, automated PGN archiving to IPFS and Arweave, and distributed WebSocket connection tracking via Redis. It introduces new database migrations, service layers, API endpoints, and integrates sessions into the existing authentication flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthAPI as Auth API
participant SessionSvc as SessionService
participant Database
participant JwtSvc as JWT Service
Client->>AuthAPI: POST /login (credentials, HttpRequest)
activate AuthAPI
AuthAPI->>AuthAPI: Extract IP, User-Agent from request
AuthAPI->>SessionSvc: create_session(user_id, ip, ua, ttl_hours)
activate SessionSvc
SessionSvc->>SessionSvc: Generate UUID session_id
SessionSvc->>SessionSvc: Compute expires_at, created_at, last_active_at
SessionSvc->>Database: INSERT session record
Database-->>SessionSvc: session_id
SessionSvc-->>AuthAPI: Result<Uuid>
deactivate SessionSvc
AuthAPI->>AuthAPI: Hash IP address
AuthAPI->>JwtSvc: generate_token_with_session(user_id, username, session_id, ip_hash)
activate JwtSvc
JwtSvc->>JwtSvc: Create Claims with session_id, ip_hash, jti=session_id
JwtSvc->>JwtSvc: Sign JWT token
JwtSvc-->>AuthAPI: access_token
deactivate JwtSvc
AuthAPI-->>Client: 200 OK (access_token, session metadata)
deactivate AuthAPI
sequenceDiagram
participant Client
participant ArchiveAPI as Archive API
participant ArchiveSvc as PgnArchiveService
participant IPFS as IPFS Pinning
participant Arweave as Arweave Bundler
Client->>ArchiveAPI: POST /v1/games/archive-pgn (pgn_content)
activate ArchiveAPI
ArchiveAPI->>ArchiveAPI: Validate pgn_content length
alt Validation Failed
ArchiveAPI-->>Client: 400 VALIDATION_ERROR
else Validation Passed
ArchiveAPI->>ArchiveSvc: archive_pgn(game_id, pgn_content)
activate ArchiveSvc
par IPFS and Arweave Concurrently
ArchiveSvc->>IPFS: POST with JSON payload (pgn, timestamp)
activate IPFS
IPFS-->>ArchiveSvc: IpfsResponse {cid}
deactivate IPFS
ArchiveSvc->>ArchiveSvc: Compute SHA-256(pgn_content)
ArchiveSvc->>Arweave: POST /tx (content, tags, hash)
activate Arweave
Arweave-->>ArchiveSvc: ArweaveResponse {tx_id}
deactivate Arweave
end
ArchiveSvc->>ArchiveSvc: Aggregate results (IPFS CID, Arweave tx_id, pgn_hash)
ArchiveSvc-->>ArchiveAPI: PgnArchiveResult
deactivate ArchiveSvc
ArchiveAPI-->>Client: 200 OK (PgnArchiveResult with cid, tx_id, hash)
end
deactivate ArchiveAPI
sequenceDiagram
participant Client
participant WSHandler as WS Handler
participant LobbyState
participant Redis as Redis
Client->>WSHandler: WebSocket CONNECT
activate WSHandler
WSHandler->>LobbyState: Get/increment connection_count[game_id]
activate LobbyState
LobbyState->>LobbyState: connection_counts[game_id]++
LobbyState-->>WSHandler: updated count
deactivate LobbyState
WSHandler->>WSHandler: Log connection event
WSHandler->>Redis: PUBLISH game_id ({"event":"connected", "count": N, "timestamp": utc})
activate Redis
Redis-->>WSHandler: OK
deactivate Redis
WSHandler-->>Client: Connection established
deactivate WSHandler
Client->>WSHandler: WebSocket DISCONNECT
activate WSHandler
WSHandler->>LobbyState: Decrement connection_count[game_id]
activate LobbyState
LobbyState->>LobbyState: connection_counts[game_id]--
LobbyState-->>WSHandler: updated count
deactivate LobbyState
WSHandler->>WSHandler: Log disconnection event
WSHandler->>Redis: PUBLISH game_id ({"event":"disconnected", "count": N, "timestamp": utc})
activate Redis
Redis-->>WSHandler: OK
deactivate Redis
deactivate WSHandler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (10)
backend/modules/api/src/ws.rs (4)
405-420:⚠️ Potential issue | 🔴 Critical
WsSessionconstruction is missingredis_connandredis_sub_task— compile error.
WsSessiondeclarespub redis_conn: Option<Arc<ConnectionManager>>andpub redis_sub_task: Option<JoinHandle<()>>(lines 225-226), butws_routeconstructs the struct without initializing them. Thews::startcall will not compile. Either initialize both toNonehere or, better, pass theConnectionManagerthroughweb::Dataand wire it in — otherwise the Redis subscription path instarted()is permanently dead.🐛 Minimal compile fix (initialize to None)
ws::start( WsSession { game_id, lobby: lobby.get_ref().clone(), hb: std::time::Instant::now(), user_id: claims.user_id, username: claims.username, session_id, + redis_conn: None, + redis_sub_task: None, }, &req, stream, )To actually enable cross-instance delivery, propagate the shared
ConnectionManagerviaweb::Data<Arc<ConnectionManager>>and thread it into the session.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/modules/api/src/ws.rs` around lines 405 - 420, WsSession is being constructed in ws_route without initializing its redis_conn and redis_sub_task fields causing a compile error and disabling Redis subscription logic; update ws_route's WsSession construction to set redis_conn and redis_sub_task to None if you want the quick fix, or preferably accept a web::Data<Arc<ConnectionManager>> in the handler and pass that ConnectionManager into the WsSession.redis_conn while spawning/assigning redis_sub_task (see WsSession, redis_conn, redis_sub_task, started(), and ConnectionManager) so the Redis subscription path is wired and functional.
292-315:⚠️ Potential issue | 🔴 Critical
ReconnectTokenwill never reach the client.In
stopped(), the session actor is already terminating. Callingctx.address().do_send(reconnect_msg)enqueues a message to an actor that is no longer accepting messages, so theHandler<WsMessage>that would serialize and write toctx.text(...)never runs. Thelog::info!("Sent reconnection token ...")is misleading — nothing is actually sent over the socket.Emit the reconnect token before teardown. Options:
- Send it in
Handler<ws::Message::Close>/ when the heartbeat is about to terminate the connection (insidehb/StreamHandler), so it goes out over the wire beforectx.stop().- Or call
ctx.text(...)directly instopped(), though by then the write half is typically closed and this will also be a no-op for the peer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/modules/api/src/ws.rs` around lines 292 - 315, The reconnect token is queued in stopped() using ctx.address().do_send(reconnect_msg) but the actor is shutting down so Handler<WsMessage> never runs; move emission of the token to before teardown (e.g., send it from the heartbeat/StreamHandler close branch or inside the hb timeout path) by calling ctx.text(...) with the serialized WsMessage::ReconnectToken or sending the reconnect message from within the active handler that still writes to the socket (instead of using ctx.address().do_send in stopped()); ensure generate_reconnect_token() is called before ctx.stop()/teardown and remove the misleading log that claims delivery after stopped().
218-228:⚠️ Potential issue | 🔴 CriticalDuplicate
hbfield — this will fail to compile.
hb: std::time::Instantis declared on line 221 and again on line 227. Rust forbids duplicate struct fields, so this struct won't compile as-is. Additionally, the new fieldsredis_connandredis_sub_taskarepub, buthbis not — and the trailing privatehbduplicate suggests an accidental leftover from a merge.🐛 Proposed fix
pub struct WsSession { pub game_id: String, pub lobby: Addr<LobbyState>, pub hb: std::time::Instant, pub user_id: i32, pub username: String, pub session_id: String, pub redis_conn: Option<Arc<ConnectionManager>>, // Redis connection for Pub/Sub pub redis_sub_task: Option<JoinHandle<()>>, // Handle for the subscription task - hb: std::time::Instant, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/modules/api/src/ws.rs` around lines 218 - 228, Struct WsSession currently declares hb twice which breaks compilation; remove the duplicated private field and keep a single hb with the intended visibility (retain the existing pub hb: std::time::Instant and delete the trailing hb: std::time::Instant). Ensure visibility consistency with the other new fields (redis_conn and redis_sub_task) and rebuild to confirm the duplicate is gone.
274-289:⚠️ Potential issue | 🔴 CriticalMultiple compile and architectural errors in the Redis subscription task.
Three critical concerns:
Line 279:
if let Ok(payload): Result<String, _> = msg.get_payload()uses invalid type ascription syntax. Stable Rust doesn't support type ascription in patterns. Useif let Ok::<String, _>(payload) = msg.get_payload()or refactor to a separateletbinding with explicit type.Line 275:
redis_conn.clone().into_pubsub()—redis::aio::ConnectionManagerdoes not exposeinto_pubsub(). That method exists only onAsyncConnection. You'll need to store a dedicated pubsub connection separately from theConnectionManagerused for publishing, or use the RESP3-based pubsub API viaConnectionManagerConfig::set_push_sender().Line 278:
stream.next().awaitrequiresfutures::StreamExt(orfutures_util::StreamExt) in scope. It is not currently imported in this file.Additionally, there is no deduplication logic between Redis-forwarded messages and local broadcasts: if the same instance publishes on
match:<game_id>(lines 336-339) and also has local subscribers, every session on the publishing instance will receive the message twice (once via broadcast, once via its own Redis subscription). Addinstance_idtagging and self-origin filtering to prevent this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/modules/api/src/ws.rs` around lines 274 - 289, The Redis subscription task is using unsupported pattern type-ascription and the wrong connection type and missing StreamExt import, and it lacks self-origin deduplication; fix by: replace the pattern `if let Ok(payload): Result<String, _> = msg.get_payload()` with an explicit binding (e.g., `let payload: String = msg.get_payload()?` or use `match msg.get_payload()`), replace `redis_conn.clone().into_pubsub()` with a dedicated PubSub connection (create and store a separate async pubsub connection/Client instead of using ConnectionManager or use the RESP3 push API via ConnectionManagerConfig) and spawn the task using that pubsub connection (update where `self.redis_sub_task` is set), add `use futures::StreamExt` so `stream.next().await` resolves, and include an `instance_id` field in the serialized WsMessage payload and filter out messages whose instance_id equals this instance before calling `addr.do_send` to avoid duplicated local delivery.backend/modules/api/src/auth.rs (4)
135-135:⚠️ Potential issue | 🟡 Minor
expires_in: 3600is hardcoded and can diverge from the actual JWT expiration.
JwtServiceis constructed with a configurableexpiration_time, but the response advertises a fixed3600. Clients using this value to schedule token refresh will drift once the configured lifetime differs from one hour. Surface the configured value viaJwtService(add a getter) or environment variable instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/modules/api/src/auth.rs` at line 135, The response currently hardcodes expires_in: 3600; instead of reflecting the configured JWT lifetime; add an accessor on JwtService (e.g., JwtService::expiration_time() or get_expiration) that returns the configured expiration_time and replace the literal 3600 in the response with that getter (locate where expires_in is set in auth.rs and the JwtService implementation) so clients receive the actual token lifetime.
141-150:⚠️ Potential issue | 🟠 MajorCookie
secure(false)hardcoded — same issue in/refreshand/logout.Three cookie builders in this file hardcode
.secure(false)with a "set to true in production" comment. This is an easy footgun: a production deploy forgets to patch three places and ships with refresh-token cookies over plaintext. Drive this from config/env so there's a single source of truth.🔧 Proposed fix
+fn cookie_secure() -> bool { + env::var("COOKIE_SECURE").map(|v| v != "false" && v != "0").unwrap_or(true) +} ... - .secure(false) // Set to true in production HTTPS + .secure(cookie_secure())Apply at all three sites (login, refresh, logout).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/modules/api/src/auth.rs` around lines 141 - 150, Replace the three hardcoded .secure(false) cookie builders with a config-driven flag: read a boolean (e.g., cookie_secure or cookie_secure_cookie) from your app config/env and use it when building the cookie in the handlers that create refresh_token cookies (the places that call Cookie::build("refresh_token", ...) — i.e., the login, refresh, and logout handlers). Update the cookie builders to call .secure(cookie_secure) instead of .secure(false), ensure the config value is parsed once and injected or globally available to these handlers, and keep the existing comment and SameSite/max_age settings unchanged; also ensure response.add_cookie(&cookie).ok() behavior remains the same.
247-256:⚠️ Potential issue | 🟠 MajorRefresh silently drops
session_id, breaking session tracking after the first refresh.You call
generate_token(...)here instead ofgenerate_token_with_session(...). The result: the new access token hassession_id: Noneandip_hash: None, so any middleware that relies onclaims.session_idto revoke/validate againstSessionServicewill be bypassed on every request after a refresh. This defeats the core goal of issue#549.You should also validate the session is still active before issuing a refreshed token.
🔧 Proposed fix
- // Generate new access token - let new_access_token = match jwt_service.generate_token(claims.user_id, &claims.username) { + // Ensure the session is still active before issuing a new access token + if let Some(sid) = claims.session_id.as_ref().and_then(|s| Uuid::parse_str(s).ok()) { + if let Err(e) = SessionService::validate_session(&db, sid).await { + log::warn!("Refresh attempted against invalid session: {}", e); + return HttpResponse::Unauthorized().json(ErrorResponse { + message: "Session is no longer valid".to_string(), + code: "INVALID_SESSION".to_string(), + }); + } + } + + // Generate new access token, preserving session and ip binding claims + let new_access_token = match jwt_service.generate_token_with_session( + claims.user_id, + &claims.username, + claims.session_id.clone(), + claims.ip_hash.clone(), + ) { Ok(t) => t, ... };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/modules/api/src/auth.rs` around lines 247 - 256, The refresh handler is calling jwt_service.generate_token(...) which drops session details; update it to call jwt_service.generate_token_with_session(...) (passing the existing claims.session_id and claims.ip_hash) so the new_access_token preserves session_id/ip_hash, and before generating the token validate the session via SessionService (e.g., check SessionService::is_active or validate_session for claims.session_id) and return an appropriate error HttpResponse (e.g., Unauthorized or TOKEN_ERROR) if the session is inactive or missing; reference the existing variables claims, jwt_service, and SessionService when making these changes.
339-351:⚠️ Potential issue | 🟠 MajorLogout does not revoke the server-side session.
The handler revokes refresh tokens but never calls
SessionService::revoke_session/revoke_all_user_sessions. With session-bound access tokens now in use, a stolen access token remains valid for the full access-token lifetime after logout — which directly contradicts the stated security goal.Also note the pre-existing
user_id = 1MVP hack at line 342 makes this endpoint revoke the wrong user's state entirely. At minimum, once a real JWT validation is wired up, useclaims.session_idto revoke the specific session here.🔧 Proposed fix (sketch)
- // We would validate token here, but for now just extract user_id from the request - // In a real implementation, we'd use a JWT service to validate and extract claims - // For MVP, we'll accept it and revoke for user_id 1 - let user_id = 1; + let claims = match jwt_service.validate_token(token) { + Ok(c) => c, + Err(_) => return HttpResponse::Unauthorized().json(ErrorResponse { + message: "Invalid or expired access token".to_string(), + code: "INVALID_ACCESS_TOKEN".to_string(), + }), + }; + let user_id = claims.user_id; + + // Revoke the current session so the access token can no longer be used + if let Some(sid) = claims.session_id.as_ref().and_then(|s| Uuid::parse_str(s).ok()) { + if let Err(e) = SessionService::revoke_session(&db, sid).await { + log::error!("Failed to revoke session: {}", e); + } + }(You'll need to add
jwt_service: web::Data<JwtService>to the handler signature.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/modules/api/src/auth.rs` around lines 339 - 351, The logout handler currently uses a hardcoded user_id and only calls TokenService::revoke_player_tokens, leaving session-bound access tokens valid; update the handler to validate the JWT (add jwt_service: web::Data<JwtService> to the function signature), extract the claims (including claims.session_id and claims.user_id), replace the user_id = 1 hack with the real claims.user_id, call SessionService::revoke_session(claims.session_id) (or SessionService::revoke_all_user_sessions(claims.user_id) if you intend full-account logout) and then call TokenService::revoke_player_tokens(db, claims.user_id); ensure error handling/logging mirrors the existing pattern when either revocation fails.backend/modules/security/src/jwt.rs (2)
81-91:⚠️ Potential issue | 🟡 MinorUsing
session_idasjticonflates session and token identity.Setting
jti = session_id.clone()means every access token minted for the same session shares the same JWT ID. This defeats per-token revocation (e.g., block-listing a leaked access token without nuking the whole session) and breaks RFC 7519's expectation thatjtiis unique per issued token. Consider generating a fresh UUID forjtiand keepingsession_idstrictly in its dedicated claim.🔧 Proposed fix
let claims = Claims { sub: user_id.to_string(), user_id, username: username.to_string(), exp: now + self.expiration_time, iat: now, - jti: session_id.clone(), + jti: Some(uuid::Uuid::new_v4().to_string()), token_type: TokenType::Access, session_id, ip_hash, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/modules/security/src/jwt.rs` around lines 81 - 91, The Claims construction sets jti = session_id.clone(), conflating token ID with session ID; change this so jti is a fresh unique token identifier (e.g., generate a new UUID via Uuid::new_v4().to_string()) while leaving session_id in its own claim and keeping token_type as TokenType::Access; update any code that relied on jti equaling session_id to use session_id explicitly or to perform token-level revocation using the new jti.
109-117:⚠️ Potential issue | 🔴 CriticalCritical:
generate_reconnect_tokenwill not compile — missingsession_idandip_hashfield initializations inClaimsliteral.The
Claimsstruct (lines 32, 34) requires bothsession_idandip_hashfields to be explicitly initialized in every struct literal, even though they areOption<T>. TheClaimsconstruction at lines 109–117 omits both fields, causing a compile error.🔧 Proposed fix
let claims = Claims { sub: user_id.to_string(), user_id, username: username.to_string(), exp: now + self.reconnect_expiration_time, iat: now, jti: Some(session_id.to_string()), token_type: TokenType::Reconnect, + session_id: Some(session_id.to_string()), + ip_hash: None, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/modules/security/src/jwt.rs` around lines 109 - 117, The Claims struct literal in generate_reconnect_token is missing the required Option fields session_id and ip_hash; update the Claims construction to include session_id: Some(session_id.to_string()) and set ip_hash to an appropriate Option value (e.g., ip_hash: None if no hash is available here or ip_hash: Some(computed_hash) when you can compute one) so the struct initializes all required fields.
🧹 Nitpick comments (4)
backend/modules/api/src/pgn_archive_api.rs (1)
51-51: SharePgnArchiveServiceviaapp_datainstead of instantiating per request.Both handlers call
PgnArchiveService::new()on every request, which in turn builds a freshreqwest::Client. Eachreqwest::Clientowns its own connection pool, so per-request construction defeats HTTPS connection reuse to Pinata/the bundler and adds TLS-handshake latency on the hot path. Construct a singlePgnArchiveServiceinserver::main, register it with.app_data(web::Data::new(archive_service.clone())), and inject it into the handlers.Also applies to: 87-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/modules/api/src/pgn_archive_api.rs` at line 51, Handlers are creating a new PgnArchiveService per request (via PgnArchiveService::new()), which builds a fresh reqwest::Client and prevents connection reuse; instead, construct a single PgnArchiveService in server::main, wrap it with web::Data::new(archive_service.clone()) and register it via .app_data(...), then change the handlers to accept web::Data<PgnArchiveService> (or Arc/Cloneable wrapper) from their parameters rather than calling PgnArchiveService::new() inside each handler so the shared reqwest::Client is reused across requests.backend/modules/api/src/pgn_archive.rs (2)
160-208: Redundant SHA-256 computation; also consider what “success” means when one provider fails.Two small things on this path:
calculate_hash(pgn_content)runs on line 165, and thenarchive_to_arweavere-hashes the same content on line 125. For large PGNs and batch calls this is wasted work — compute once and pass the hash intoarchive_to_arweave.- The handler returns
Ok(...)whenever either provider succeeds, with the failing side silently set toNone. That’s a reasonable resilience choice, but the caller has no machine-readable signal of partial failure other than inspectingipfs_cid/arweave_tx_id. Consider including per-service status (or anerrors: Vec<String>) inPgnArchiveResultso clients/monitors can alert on “archived to only one store”.♻️ Proposed de-duplication of hashing
- pub async fn archive_to_arweave(&self, pgn_content: &str) -> Result<String, PgnArchiveError> { + pub async fn archive_to_arweave( + &self, + pgn_content: &str, + pgn_hash: &str, + ) -> Result<String, PgnArchiveError> { let arweave_url = env::var("ARWEAVE_GATEWAY_URL") .unwrap_or_else(|_| "https://arweave.net".to_string()); ... - let pgn_hash = self.calculate_hash(pgn_content); - let payload = serde_json::json!({ "data": pgn_content, "tags": [ { "name": "Content-Type", "value": "application/x-chess-pgn" }, { "name": "App", "value": "XLMate" }, - { "name": "PGN-Hash", "value": &pgn_hash }, + { "name": "PGN-Hash", "value": pgn_hash }, { "name": "Archived-At", "value": &chrono::Utc::now().to_rfc3339() } ] });And in
archive_pgn:- let ipfs_future = self.archive_to_ipfs(pgn_content); - let arweave_future = self.archive_to_arweave(pgn_content); + let ipfs_future = self.archive_to_ipfs(pgn_content); + let arweave_future = self.archive_to_arweave(pgn_content, &pgn_hash);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/modules/api/src/pgn_archive.rs` around lines 160 - 208, Compute the PGN hash once in archive_pgn using calculate_hash(pgn_content) and pass that hash into archive_to_arweave (update the archive_to_arweave signature/uses) to avoid duplicate hashing; also extend the PgnArchiveResult struct to include per-service status or an errors: Vec<String> field (e.g., record "ipfs_failed: bool" / "arweave_failed: bool" or push error messages) and populate it in archive_pgn where ipfs_result and arweave_result are matched so callers can detect partial failures, while keeping PgnArchiveError for the case where both services fail.
69-77: Read config once at startup and fail fast on missing IPFS credentials.
env::var("IPFS_API_URL" / "IPFS_API_KEY" / "IPFS_SECRET_KEY")is called on every archive request, and missing keys silently default to empty strings. The result is:
- The handler still POSTs to Pinata with empty
pinata_api_key/pinata_secret_api_keyheaders, wasting a network round-trip and returning an opaque non-2xx error to the user.- Ops cannot tell a misconfiguration apart from an upstream outage in logs.
Prefer loading these once (e.g., in
server::mainalongside the otherenv::varreads — seebackend/modules/api/src/server.rs:45-79) into a config struct passed toPgnArchiveService::new(...), and log a clear warning (or disable IPFS archival) if the keys are absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/modules/api/src/pgn_archive.rs` around lines 69 - 77, The code currently reads IPFS env vars inside archive_to_ipfs and defaults missing keys to empty strings; change this to read IPFS_API_URL, IPFS_API_KEY and IPFS_SECRET_KEY once at startup (e.g., in server::main) into a Config struct, pass that Config into PgnArchiveService via PgnArchiveService::new(...), and update archive_to_ipfs to use the stored config values instead of calling env::var; if keys are missing make startup fail fast or explicitly disable IPFS archival and log a clear, actionable warning indicating missing credentials so requests no longer attempt a wasted POST with empty headers.backend/modules/security/src/session_service.rs (1)
72-101: Consider extendingvalidate_sessionto cross-check user and/or IP binding.Today the function is pure "does this session_id exist and isn't dead" — it doesn't verify that the requester owns the session. Once IP binding (
ip_hash) is actually populated in tokens (see theauth.rscomment onip_hash), this validator is the natural place to compareclaims.ip_hash/claims.user_idagainstsession_record.ip_address/session_record.user_idand returnSessionNotFoundor a newSessionMismatcherror on drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/modules/security/src/session_service.rs` around lines 72 - 101, Extend validate_session to verify the requester actually owns the session by accepting the JWT claims (or at least claims.user_id and claims.ip_hash) and comparing them to the loaded session_record's user_id and ip_address/ip_hash; if they mismatch return SessionError::SessionNotFound or introduce and return SessionError::SessionMismatch as appropriate. Locate the function validate_session and its use of session_record (session::Entity::find, session_record.ok_or(...)), add parameters for the claim values (e.g., user_id and ip_hash), perform equality checks against session_record.user_id and session_record.ip_address/ip_hash before the expired/revoked checks (or after, per policy), and ensure callers (e.g., auth code that validates tokens) pass the claims through; update error handling to return the new SessionMismatch variant (and add that variant to SessionError) when a binding check fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/modules/api/src/auth.rs`:
- Around line 77-101: The login handler extracts ip_address but never computes
or passes ip_hash to the token, so compute a server-side peppered SHA-256 (e.g.,
sha256(pepper + ip)) using a secret from env (e.g., JWT_IP_PEPPER), convert to
hex/base64 and pass that value into jwt_service.generate_token_with_session
(where Claims.ip_hash is expected) instead of None; ensure
SessionService::create_session still gets the raw or stored IP as needed, and
implement middleware in your JWT validation path that recomputes the same
peppered hash from req.connection_info().real_ip_remote_addr() and compares it
to claims.ip_hash, rejecting requests when they differ.
- Line 78: The current use of req.connection_info().real_ip_remote_addr() can be
spoofed; replace it by using peer_addr() for direct deployments or, if behind a
trusted proxy, validate that peer_addr() is within configured trusted proxy
CIDRs before extracting and trusting X-Forwarded-For, and only then parse the
leftmost address; update the call site that sets ip_address (currently calling
req.connection_info().real_ip_remote_addr()) and ensure
generate_token_with_session() receives a validated ip_hash (not None) so
sessions store a concrete IP hash, and apply the same validation/assignment
logic consistently in the login, refresh, and validate handlers so IP-based
session checks work end-to-end.
In `@backend/modules/api/src/pgn_archive_api.rs`:
- Around line 83-111: The batch_archive_pgn handler currently accepts an
unbounded Vec<ArchivePgnRequest> and processes each request serially via
PgnArchiveService::archive_pgn, which is a DoS/amplification risk; enforce an
explicit max batch size (e.g., reject payloads with more than 50 items and
return HttpResponse::PayloadTooLarge()) and ensure the Actix JSON extractor
scope has an appropriate limit configured so you don't rely on the default 32
KiB. Replace the serial for-loop awaiting archive_pgn with concurrent execution
(e.g., use futures or tokio: create tasks for each ArchivePgnRequest calling
PgnArchiveService::archive_pgn and run them with
futures::stream::iter(...).map(...).buffer_unordered(CONCURRENCY).collect or
tokio::JoinSet) with a reasonable concurrency bound (e.g., 8–16) to avoid
unbounded parallelism, then gather the ArchivePgnResponse results and return
them as before; keep error handling/logging semantics for each individual
result.
In `@backend/modules/api/src/pgn_archive.rs`:
- Around line 62-66: The PgnArchiveService currently creates a reqwest::Client
with Client::new() which has no connect or overall request timeouts; update
PgnArchiveService::new to build the client via reqwest::Client::builder() and
set sensible connect_timeout and timeout values (e.g., a few seconds for
connect_timeout and a shorter overall request timeout) before calling .build(),
then assign that client to the client field so archive_pgn calls cannot block
Actix threads indefinitely.
- Around line 115-157: The archive_to_arweave function currently posts an
unsigned JSON payload to the bundler /tx endpoint which will 4xx in production;
fix by either implementing proper ANS-104 signing and sending a binary DataItem
with Content-Type: application/octet-stream (use an Arweave/Bundlr SDK to
create/sign the DataItem before POST) inside archive_to_arweave, or feature‑gate
the Arweave path (e.g., check an env var like ARWEAVE_ENABLED in
archive_to_arweave or the caller archive_pgn) and immediately return a clear
PgnArchiveError variant (e.g., NotSupported/Disabled) if not implemented; also
ensure archive_pgn no longer silently degrades by propagating the explicit error
from archive_to_arweave instead of hiding it.
In `@backend/modules/api/src/ws.rs`:
- Around line 56-83: The LobbyState::with_redis variant is never used so
redis_conn stays None; modify the server startup to create a Redis
ConnectionManager, pass its Arc clone into
LobbyState::with_redis(redis_conn.clone()) instead of LobbyState::new(), and
only fall back to LobbyState::new() when Redis configuration/env is absent; also
emit a startup log indicating whether the app started in "distributed (redis)"
or "local" mode so the publish/subscribe paths in Connect/Disconnect see an
active redis_conn.
- Around line 58-63: The module currently contains dead code: the HashMap field
connection_counts, the helper functions publish_connection_event,
get_connection_count, get_total_connections, and the AtomicUsize import are
unused while Connect/Disconnect inline duplicate JSON/publish logic; either
remove them entirely or wire them up. Recommend: drop connection_counts and the
atomic import if you won't expose per-game metrics, and consolidate the
duplicated publish logic by replacing the inline actix::spawn blocks in the
Connect and Disconnect handlers with calls to a single async helper
publish_connection_event(&msg.game_id, "connected" | "disconnected",
count).await (expose a small sync adapter that spawns the async helper from the
sync handler), have that helper build the JSON payload once using instance_id
and redis_conn, and remove the unused get_connection_count/get_total_connections
or add a message handler that calls them if you need to expose counts; update
references from sessions where necessary to compute count before calling the
helper.
- Around line 122-156: The current handle (fn handle with Connect) spawns a task
(actix::spawn) that publishes a snapshot (count) to Redis, which can be observed
out-of-order by subscribers; fix by either (A) serializing publishes per game_id
by introducing a dedicated publisher actor/task/mailbox keyed on game_id that
receives publish requests (instead of spawning per event) so order is preserved,
or (B) make events idempotent/ordering-safe by adding a monotonic sequence
and/or authoritative timestamp to the payload (use a per-game AtomicU64 or Redis
INCR) and include that sequence/timestamp alongside instance_id, game_id and
count so subscribers can reconcile by “highest sequence/timestamp wins”; update
the code that currently uses redis_conn, game_id, count and actix::spawn to use
the chosen approach.
In `@backend/modules/security/src/session_service.rs`:
- Around line 147-156: The cleanup_expired_sessions function is never scheduled;
wire it into app startup as a periodic background task that calls
session_service::cleanup_expired_sessions(db) on an interval (e.g.,
tokio::time::interval or actix_rt::spawn with a loop) so expired sessions are
removed regularly; ensure you pass a cloneable/shared DatabaseConnection (Arc or
DatabaseConnection::clone if supported) or a connection pool into the task,
handle/ log any SessionError returned (don’t unwrap), and choose an appropriate
interval (e.g., hourly) and graceful shutdown behavior so the task is started
once during server bootstrap (e.g., in your server startup/init function).
---
Outside diff comments:
In `@backend/modules/api/src/auth.rs`:
- Line 135: The response currently hardcodes expires_in: 3600; instead of
reflecting the configured JWT lifetime; add an accessor on JwtService (e.g.,
JwtService::expiration_time() or get_expiration) that returns the configured
expiration_time and replace the literal 3600 in the response with that getter
(locate where expires_in is set in auth.rs and the JwtService implementation) so
clients receive the actual token lifetime.
- Around line 141-150: Replace the three hardcoded .secure(false) cookie
builders with a config-driven flag: read a boolean (e.g., cookie_secure or
cookie_secure_cookie) from your app config/env and use it when building the
cookie in the handlers that create refresh_token cookies (the places that call
Cookie::build("refresh_token", ...) — i.e., the login, refresh, and logout
handlers). Update the cookie builders to call .secure(cookie_secure) instead of
.secure(false), ensure the config value is parsed once and injected or globally
available to these handlers, and keep the existing comment and SameSite/max_age
settings unchanged; also ensure response.add_cookie(&cookie).ok() behavior
remains the same.
- Around line 247-256: The refresh handler is calling
jwt_service.generate_token(...) which drops session details; update it to call
jwt_service.generate_token_with_session(...) (passing the existing
claims.session_id and claims.ip_hash) so the new_access_token preserves
session_id/ip_hash, and before generating the token validate the session via
SessionService (e.g., check SessionService::is_active or validate_session for
claims.session_id) and return an appropriate error HttpResponse (e.g.,
Unauthorized or TOKEN_ERROR) if the session is inactive or missing; reference
the existing variables claims, jwt_service, and SessionService when making these
changes.
- Around line 339-351: The logout handler currently uses a hardcoded user_id and
only calls TokenService::revoke_player_tokens, leaving session-bound access
tokens valid; update the handler to validate the JWT (add jwt_service:
web::Data<JwtService> to the function signature), extract the claims (including
claims.session_id and claims.user_id), replace the user_id = 1 hack with the
real claims.user_id, call SessionService::revoke_session(claims.session_id) (or
SessionService::revoke_all_user_sessions(claims.user_id) if you intend
full-account logout) and then call TokenService::revoke_player_tokens(db,
claims.user_id); ensure error handling/logging mirrors the existing pattern when
either revocation fails.
In `@backend/modules/api/src/ws.rs`:
- Around line 405-420: WsSession is being constructed in ws_route without
initializing its redis_conn and redis_sub_task fields causing a compile error
and disabling Redis subscription logic; update ws_route's WsSession construction
to set redis_conn and redis_sub_task to None if you want the quick fix, or
preferably accept a web::Data<Arc<ConnectionManager>> in the handler and pass
that ConnectionManager into the WsSession.redis_conn while spawning/assigning
redis_sub_task (see WsSession, redis_conn, redis_sub_task, started(), and
ConnectionManager) so the Redis subscription path is wired and functional.
- Around line 292-315: The reconnect token is queued in stopped() using
ctx.address().do_send(reconnect_msg) but the actor is shutting down so
Handler<WsMessage> never runs; move emission of the token to before teardown
(e.g., send it from the heartbeat/StreamHandler close branch or inside the hb
timeout path) by calling ctx.text(...) with the serialized
WsMessage::ReconnectToken or sending the reconnect message from within the
active handler that still writes to the socket (instead of using
ctx.address().do_send in stopped()); ensure generate_reconnect_token() is called
before ctx.stop()/teardown and remove the misleading log that claims delivery
after stopped().
- Around line 218-228: Struct WsSession currently declares hb twice which breaks
compilation; remove the duplicated private field and keep a single hb with the
intended visibility (retain the existing pub hb: std::time::Instant and delete
the trailing hb: std::time::Instant). Ensure visibility consistency with the
other new fields (redis_conn and redis_sub_task) and rebuild to confirm the
duplicate is gone.
- Around line 274-289: The Redis subscription task is using unsupported pattern
type-ascription and the wrong connection type and missing StreamExt import, and
it lacks self-origin deduplication; fix by: replace the pattern `if let
Ok(payload): Result<String, _> = msg.get_payload()` with an explicit binding
(e.g., `let payload: String = msg.get_payload()?` or use `match
msg.get_payload()`), replace `redis_conn.clone().into_pubsub()` with a dedicated
PubSub connection (create and store a separate async pubsub connection/Client
instead of using ConnectionManager or use the RESP3 push API via
ConnectionManagerConfig) and spawn the task using that pubsub connection (update
where `self.redis_sub_task` is set), add `use futures::StreamExt` so
`stream.next().await` resolves, and include an `instance_id` field in the
serialized WsMessage payload and filter out messages whose instance_id equals
this instance before calling `addr.do_send` to avoid duplicated local delivery.
In `@backend/modules/security/src/jwt.rs`:
- Around line 81-91: The Claims construction sets jti = session_id.clone(),
conflating token ID with session ID; change this so jti is a fresh unique token
identifier (e.g., generate a new UUID via Uuid::new_v4().to_string()) while
leaving session_id in its own claim and keeping token_type as TokenType::Access;
update any code that relied on jti equaling session_id to use session_id
explicitly or to perform token-level revocation using the new jti.
- Around line 109-117: The Claims struct literal in generate_reconnect_token is
missing the required Option fields session_id and ip_hash; update the Claims
construction to include session_id: Some(session_id.to_string()) and set ip_hash
to an appropriate Option value (e.g., ip_hash: None if no hash is available here
or ip_hash: Some(computed_hash) when you can compute one) so the struct
initializes all required fields.
---
Nitpick comments:
In `@backend/modules/api/src/pgn_archive_api.rs`:
- Line 51: Handlers are creating a new PgnArchiveService per request (via
PgnArchiveService::new()), which builds a fresh reqwest::Client and prevents
connection reuse; instead, construct a single PgnArchiveService in server::main,
wrap it with web::Data::new(archive_service.clone()) and register it via
.app_data(...), then change the handlers to accept web::Data<PgnArchiveService>
(or Arc/Cloneable wrapper) from their parameters rather than calling
PgnArchiveService::new() inside each handler so the shared reqwest::Client is
reused across requests.
In `@backend/modules/api/src/pgn_archive.rs`:
- Around line 160-208: Compute the PGN hash once in archive_pgn using
calculate_hash(pgn_content) and pass that hash into archive_to_arweave (update
the archive_to_arweave signature/uses) to avoid duplicate hashing; also extend
the PgnArchiveResult struct to include per-service status or an errors:
Vec<String> field (e.g., record "ipfs_failed: bool" / "arweave_failed: bool" or
push error messages) and populate it in archive_pgn where ipfs_result and
arweave_result are matched so callers can detect partial failures, while keeping
PgnArchiveError for the case where both services fail.
- Around line 69-77: The code currently reads IPFS env vars inside
archive_to_ipfs and defaults missing keys to empty strings; change this to read
IPFS_API_URL, IPFS_API_KEY and IPFS_SECRET_KEY once at startup (e.g., in
server::main) into a Config struct, pass that Config into PgnArchiveService via
PgnArchiveService::new(...), and update archive_to_ipfs to use the stored config
values instead of calling env::var; if keys are missing make startup fail fast
or explicitly disable IPFS archival and log a clear, actionable warning
indicating missing credentials so requests no longer attempt a wasted POST with
empty headers.
In `@backend/modules/security/src/session_service.rs`:
- Around line 72-101: Extend validate_session to verify the requester actually
owns the session by accepting the JWT claims (or at least claims.user_id and
claims.ip_hash) and comparing them to the loaded session_record's user_id and
ip_address/ip_hash; if they mismatch return SessionError::SessionNotFound or
introduce and return SessionError::SessionMismatch as appropriate. Locate the
function validate_session and its use of session_record (session::Entity::find,
session_record.ok_or(...)), add parameters for the claim values (e.g., user_id
and ip_hash), perform equality checks against session_record.user_id and
session_record.ip_address/ip_hash before the expired/revoked checks (or after,
per policy), and ensure callers (e.g., auth code that validates tokens) pass the
claims through; update error handling to return the new SessionMismatch variant
(and add that variant to SessionError) when a binding check fails.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc71c835-18e0-4e4f-a85c-18549ff16ccb
📒 Files selected for processing (14)
backend/modules/api/Cargo.tomlbackend/modules/api/src/auth.rsbackend/modules/api/src/lib.rsbackend/modules/api/src/pgn_archive.rsbackend/modules/api/src/pgn_archive_api.rsbackend/modules/api/src/server.rsbackend/modules/api/src/ws.rsbackend/modules/db/entity/mod.rsbackend/modules/db/entity/src/session.rsbackend/modules/db/migrations/src/lib.rsbackend/modules/db/migrations/src/m20260224_000000_create_sessions_table.rsbackend/modules/security/src/jwt.rsbackend/modules/security/src/lib.rsbackend/modules/security/src/session_service.rs
| // Extract IP address and user agent for session tracking | ||
| let ip_address = req.connection_info().real_ip_remote_addr().map(|s| s.to_string()); | ||
| let user_agent = req.headers().get("User-Agent") | ||
| .and_then(|h| h.to_str().ok()) | ||
| .map(|s| s.to_string()); | ||
|
|
||
| // Create a new session | ||
| let session_ttl = env::var("SESSION_TTL_HOURS") | ||
| .unwrap_or_else(|_| "168".to_string()) // 7 days default | ||
| .parse::<i64>() | ||
| .unwrap_or(168); | ||
|
|
||
| let session_id = match SessionService::create_session(&db, user_id, ip_address, user_agent, session_ttl).await { | ||
| Ok(sid) => sid, | ||
| Err(e) => { | ||
| log::error!("Failed to create session: {}", e); | ||
| return HttpResponse::InternalServerError().json(ErrorResponse { | ||
| message: "Failed to create session".to_string(), | ||
| code: "SESSION_ERROR".to_string(), | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| // Generate access token with session ID | ||
| let access_token = match jwt_service.generate_token_with_session(user_id, &username, Some(session_id.to_string()), None) { |
There was a problem hiding this comment.
ip_hash is never computed — the advertised "IP binding" in JWT is a no-op.
The PR objective explicitly promises "IP binding to mitigate token theft", and Claims now carries ip_hash, but the login handler passes None. Even the ip_address you just extracted on line 78 is not being hashed into the token. No downstream validator can enforce binding if the claim is never populated.
Hash the IP (e.g., SHA-256 with a server-side pepper to avoid exposing raw IPs in logs/tokens) and thread it into generate_token_with_session. You'll also need middleware that cross-checks the incoming request IP's hash against claims.ip_hash — otherwise this remains decorative.
🔧 Proposed fix (login handler)
// Extract IP address and user agent for session tracking
let ip_address = req.connection_info().real_ip_remote_addr().map(|s| s.to_string());
+ let ip_hash = ip_address.as_deref().map(|ip| {
+ use sha2::{Digest, Sha256};
+ let pepper = env::var("IP_HASH_PEPPER").unwrap_or_default();
+ let mut hasher = Sha256::new();
+ hasher.update(pepper.as_bytes());
+ hasher.update(ip.as_bytes());
+ format!("{:x}", hasher.finalize())
+ });
let user_agent = req.headers().get("User-Agent")
...
- let access_token = match jwt_service.generate_token_with_session(user_id, &username, Some(session_id.to_string()), None) {
+ let access_token = match jwt_service.generate_token_with_session(user_id, &username, Some(session_id.to_string()), ip_hash) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/modules/api/src/auth.rs` around lines 77 - 101, The login handler
extracts ip_address but never computes or passes ip_hash to the token, so
compute a server-side peppered SHA-256 (e.g., sha256(pepper + ip)) using a
secret from env (e.g., JWT_IP_PEPPER), convert to hex/base64 and pass that value
into jwt_service.generate_token_with_session (where Claims.ip_hash is expected)
instead of None; ensure SessionService::create_session still gets the raw or
stored IP as needed, and implement middleware in your JWT validation path that
recomputes the same peppered hash from
req.connection_info().real_ip_remote_addr() and compares it to claims.ip_hash,
rejecting requests when they differ.
| // Generate access token | ||
| let access_token = match jwt_service.generate_token(user_id, &username) { | ||
| // Extract IP address and user agent for session tracking | ||
| let ip_address = req.connection_info().real_ip_remote_addr().map(|s| s.to_string()); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
actix-web ConnectionInfo real_ip_remote_addr spoofing X-Forwarded-For trusted proxy
💡 Result:
actix-web ConnectionInfo does not provide a built-in “trusted proxy” allowlist; it will read client IP information from spoofable headers by default unless you control who can set them (e.g., by stripping/overwriting at a reverse proxy). 1) What ConnectionInfo uses for real_ip_remote_addr - ConnectionInfo::realip_remote_addr resolves the “real IP” (remote address of the client) in this order: - Forwarded header (RFC 7239) - X-Forwarded-For header - peer address from the opened socket (same as peer_addr) This behavior is defined in actix-web’s ConnectionInfo docs/source. 2) Spoofing risk (no trust model) - The docs explicitly warn: do not use realip_remote_addr for security purposes unless you can be sure the Forwarded and X-Forwarded-For headers cannot be spoofed by the client. If you’re running without a proxy, obtaining the peer address would be more appropriate. - actix-web also notes that Forwarded/X-Forwarded-* values expose privacy-sensitive info (like client IP) and must be considered when deploying. 3) “trusted proxy” guidance for X-Forwarded-For - Because actix-web’s ConnectionInfo simply parses these headers, your application needs a deployment-level strategy to ensure only your reverse proxy can influence them. Typical approach: - Configure your reverse proxy to overwrite X-Forwarded-For (and Forwarded if used) and to remove any incoming copies from the client. - Ensure the actix-web server is not directly reachable from untrusted networks (only through the reverse proxy / internal network). - Then, it is safe to treat realip_remote_addr as the proxy-supplied client IP for logging/diagnostics; for security decisions, still prefer using peer_addr or implement explicit trust logic yourself. 4) actix-web-related misconception: “trusted proxy” doesn’t exist in ConnectionInfo - There is an open discussion/issue specifically highlighting that realip_remote_addr behavior can lead to insecure code when apps use it unconditionally. 5) Practical options if you need explicit trust - actix-web itself: no built-in trusted-proxy config for ConnectionInfo. - If you need “trusted proxy” behavior, you generally implement it via: - middleware that extracts IP only when the request comes from known proxy IPs, or - custom header parsing with an allowlist of proxy addresses (not shown in actix-web ConnectionInfo docs). Example takeaway: If you want “X-Forwarded-For trusted proxy” semantics, you must enforce them outside ConnectionInfo (reverse proxy configuration + network isolation, or custom middleware/logic).
Citations:
- 1: https://docs.rs/actix-web/latest/actix_web/dev/struct.ConnectionInfo.html
- 2: https://docs.rs/actix-web/4/actix_web/dev/struct.ConnectionInfo.html
- 3: https://docs.rs/actix-web/latest/src/actix_web/info.rs.html
- 4: https://docs.rs/actix-web/4.13.0/actix_web/http/header/constant.FORWARDED.html
- 5: realip_remote_addr behavior easily leads to insecure code actix/actix-web#3750
- 6: https://docs.rs/actix-ip-filter/latest/actix_ip_filter/
🏁 Script executed:
# First, let's examine the exact code at line 78 with surrounding context
sed -n '70,90p' backend/modules/api/src/auth.rsRepository: NOVUS-X/XLMate
Length of output: 793
🏁 Script executed:
# Search for session.ip_address usage to understand the impact scope
rg 'session.*ip_address|ip_address.*session' -i -A 2 -B 2Repository: NOVUS-X/XLMate
Length of output: 405
🏁 Script executed:
# Search for IP binding or trusted proxy validation patterns
rg 'ip.*bind|trusted.*proxy|proxy.*ip|X-Forwarded-For' -i -A 2 -B 2Repository: NOVUS-X/XLMate
Length of output: 742
🏁 Script executed:
# Search for other uses of real_ip_remote_addr or peer_addr in the codebase
rg 'real_ip_remote_addr|peer_addr|connection_info' -i -A 2Repository: NOVUS-X/XLMate
Length of output: 1657
🏁 Script executed:
# Find and examine SessionService implementation
fd 'SessionService|session.*service' -i --type f | head -20Repository: NOVUS-X/XLMate
Length of output: 106
🏁 Script executed:
# Look for the create_session method implementation
rg 'fn create_session|impl SessionService' -A 15 -B 2Repository: NOVUS-X/XLMate
Length of output: 1671
🏁 Script executed:
# Check if IP is validated on session validation/refresh
rg 'validate.*session|refresh.*session|session.*valid' -i -A 5 -B 2Repository: NOVUS-X/XLMate
Length of output: 808
🏁 Script executed:
# Search for proxy configuration or middleware
rg 'proxy|middleware|X-Forwarded|Forwarded' -i --type fRepository: NOVUS-X/XLMate
Length of output: 84
🏁 Script executed:
# Look for proxy or middleware configuration without the invalid --type flag
rg 'proxy|middleware|X-Forwarded|Forwarded' -i -A 2 -B 2Repository: NOVUS-X/XLMate
Length of output: 11672
🏁 Script executed:
# Get the full validate_session implementation
sed -n '1,200p' backend/modules/security/src/session_service.rs | grep -A 30 'fn validate_session'Repository: NOVUS-X/XLMate
Length of output: 1004
🏁 Script executed:
# Check if IP validation happens during request processing
rg 'ip_address.*validate|validate.*ip|ip.*compare|ip.*mismatch' -i -A 3 -B 2Repository: NOVUS-X/XLMate
Length of output: 513
🏁 Script executed:
# Search for generate_token_with_session to see if IP binding is actually used
rg 'generate_token_with_session|ip_hash' -A 10 -B 2Repository: NOVUS-X/XLMate
Length of output: 4085
Remove real_ip_remote_addr() or validate it through a trusted proxy.
ConnectionInfo::real_ip_remote_addr() reads the leftmost X-Forwarded-For or Forwarded header without validation. Any direct client can send X-Forwarded-For: 8.8.8.8 and poison the stored ip_address. This is amplified by the incomplete IP binding feature—generate_token_with_session() currently passes ip_hash: None, so the session IP is stored but never validated on refresh/validate, leaving spoofing undetected.
Use peer_addr() for direct deployments, or if behind a proxy, validate that the request's peer_addr matches a configured trusted proxy CIDR before accepting X-Forwarded-For. Also implement consistent IP validation across login, refresh, and validate endpoints (currently missing).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/modules/api/src/auth.rs` at line 78, The current use of
req.connection_info().real_ip_remote_addr() can be spoofed; replace it by using
peer_addr() for direct deployments or, if behind a trusted proxy, validate that
peer_addr() is within configured trusted proxy CIDRs before extracting and
trusting X-Forwarded-For, and only then parse the leftmost address; update the
call site that sets ip_address (currently calling
req.connection_info().real_ip_remote_addr()) and ensure
generate_token_with_session() receives a validated ip_hash (not None) so
sessions store a concrete IP hash, and apply the same validation/assignment
logic consistently in the login, refresh, and validate handlers so IP-based
session checks work end-to-end.
| #[post("/batch-archive-pgn")] | ||
| pub async fn batch_archive_pgn( | ||
| payload: web::Json<Vec<ArchivePgnRequest>>, | ||
| ) -> HttpResponse { | ||
| let archive_service = PgnArchiveService::new(); | ||
| let mut results = Vec::new(); | ||
|
|
||
| for request in payload.into_inner() { | ||
| match archive_service.archive_pgn(&request.game_id, &request.pgn_content).await { | ||
| Ok(archive_result) => { | ||
| results.push(ArchivePgnResponse { | ||
| success: true, | ||
| archive_result: Some(archive_result), | ||
| message: "PGN archived successfully".to_string(), | ||
| }); | ||
| } | ||
| Err(e) => { | ||
| log::warn!("Failed to archive PGN for game {}: {}", request.game_id, e); | ||
| results.push(ArchivePgnResponse { | ||
| success: false, | ||
| archive_result: None, | ||
| message: format!("Failed to archive: {}", e), | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| HttpResponse::Ok().json(results) | ||
| } |
There was a problem hiding this comment.
Batch endpoint is an unbounded amplification vector — cap and parallelize.
Two problems with this handler:
- No size limit. A single authenticated client can POST, say, 100k
ArchivePgnRequestentries; each triggers two external HTTP calls (IPFS + Arweave). That’s trivially a DoS amplifier against both this service and the upstream providers (and will blow through any Pinata quota). Enforce a small, explicit max (e.g., 50) and reject413 Payload Too Largeotherwise. Also ensure the actix JSON payload limit is configured on this scope (default is 32 KiB, which may already clip it, but the check shouldn’t rely on that). - Serial
awaitinside aforloop. Each iteration blocks the next; for N items the wall-clock is ~N·(IPFS+Arweave latency). Usefutures::future::join_all(orbuffered) to run them concurrently (optionally with a concurrency bound).
🛠️ Suggested fix (cap + concurrent execution)
+const MAX_BATCH_SIZE: usize = 50;
+
#[post("/batch-archive-pgn")]
pub async fn batch_archive_pgn(
payload: web::Json<Vec<ArchivePgnRequest>>,
) -> HttpResponse {
- let archive_service = PgnArchiveService::new();
- let mut results = Vec::new();
-
- for request in payload.into_inner() {
- match archive_service.archive_pgn(&request.game_id, &request.pgn_content).await {
- ...
- }
- }
-
- HttpResponse::Ok().json(results)
+ let items = payload.into_inner();
+ if items.len() > MAX_BATCH_SIZE {
+ return HttpResponse::PayloadTooLarge().json(ErrorResponse {
+ message: format!("Batch size exceeds limit of {}", MAX_BATCH_SIZE),
+ code: "BATCH_TOO_LARGE".to_string(),
+ });
+ }
+
+ let archive_service = PgnArchiveService::new();
+ let futures = items.into_iter().map(|request| {
+ let svc = archive_service.clone();
+ async move {
+ match svc.archive_pgn(&request.game_id, &request.pgn_content).await {
+ Ok(archive_result) => ArchivePgnResponse {
+ success: true,
+ archive_result: Some(archive_result),
+ message: "PGN archived successfully".to_string(),
+ },
+ Err(e) => {
+ log::warn!("Failed to archive PGN for game {}: {}", request.game_id, e);
+ ArchivePgnResponse {
+ success: false,
+ archive_result: None,
+ message: format!("Failed to archive: {}", e),
+ }
+ }
+ }
+ }
+ });
+ let results: Vec<_> = futures::future::join_all(futures).await;
+ HttpResponse::Ok().json(results)
}(Requires adding the futures crate or using tokio’s JoinSet.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/modules/api/src/pgn_archive_api.rs` around lines 83 - 111, The
batch_archive_pgn handler currently accepts an unbounded Vec<ArchivePgnRequest>
and processes each request serially via PgnArchiveService::archive_pgn, which is
a DoS/amplification risk; enforce an explicit max batch size (e.g., reject
payloads with more than 50 items and return HttpResponse::PayloadTooLarge()) and
ensure the Actix JSON extractor scope has an appropriate limit configured so you
don't rely on the default 32 KiB. Replace the serial for-loop awaiting
archive_pgn with concurrent execution (e.g., use futures or tokio: create tasks
for each ArchivePgnRequest calling PgnArchiveService::archive_pgn and run them
with futures::stream::iter(...).map(...).buffer_unordered(CONCURRENCY).collect
or tokio::JoinSet) with a reasonable concurrency bound (e.g., 8–16) to avoid
unbounded parallelism, then gather the ArchivePgnResponse results and return
them as before; keep error handling/logging semantics for each individual
result.
| pub fn new() -> Self { | ||
| PgnArchiveService { | ||
| client: reqwest::Client::new(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Add HTTP timeouts to the reqwest::Client.
The default reqwest::Client::new() has no connect or request timeout. Because archive_pgn is invoked on the Actix request thread, a slow/hung IPFS or Arweave bundler can block the handler until the TCP stack gives up (minutes), tying up worker threads and degrading the whole API. This is the classic blocking-on-request-thread hazard for external calls.
🛠️ Suggested fix
-impl PgnArchiveService {
- pub fn new() -> Self {
- PgnArchiveService {
- client: reqwest::Client::new(),
- }
- }
+impl PgnArchiveService {
+ pub fn new() -> Self {
+ let client = reqwest::Client::builder()
+ .connect_timeout(std::time::Duration::from_secs(5))
+ .timeout(std::time::Duration::from_secs(30))
+ .build()
+ .expect("failed to build reqwest client");
+ PgnArchiveService { client }
+ }📝 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.
| pub fn new() -> Self { | |
| PgnArchiveService { | |
| client: reqwest::Client::new(), | |
| } | |
| } | |
| pub fn new() -> Self { | |
| let client = reqwest::Client::builder() | |
| .connect_timeout(std::time::Duration::from_secs(5)) | |
| .timeout(std::time::Duration::from_secs(30)) | |
| .build() | |
| .expect("failed to build reqwest client"); | |
| PgnArchiveService { client } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/modules/api/src/pgn_archive.rs` around lines 62 - 66, The
PgnArchiveService currently creates a reqwest::Client with Client::new() which
has no connect or overall request timeouts; update PgnArchiveService::new to
build the client via reqwest::Client::builder() and set sensible connect_timeout
and timeout values (e.g., a few seconds for connect_timeout and a shorter
overall request timeout) before calling .build(), then assign that client to the
client field so archive_pgn calls cannot block Actix threads indefinitely.
| pub async fn archive_to_arweave(&self, pgn_content: &str) -> Result<String, PgnArchiveError> { | ||
| let arweave_url = env::var("ARWEAVE_GATEWAY_URL") | ||
| .unwrap_or_else(|_| "https://arweave.net".to_string()); | ||
|
|
||
| // For simplicity, we're using a bundler service | ||
| // In production, you'd use the Arweave SDK with a wallet | ||
| let bundler_url = env::var("ARWEAVE_BUNDLER_URL") | ||
| .unwrap_or_else(|_| "https://node2.bundlr.network".to_string()); | ||
|
|
||
| // Calculate SHA-256 hash of PGN for integrity | ||
| let pgn_hash = self.calculate_hash(pgn_content); | ||
|
|
||
| let payload = serde_json::json!({ | ||
| "data": pgn_content, | ||
| "tags": [ | ||
| { "name": "Content-Type", "value": "application/x-chess-pgn" }, | ||
| { "name": "App", "value": "XLMate" }, | ||
| { "name": "PGN-Hash", "value": &pgn_hash }, | ||
| { "name": "Archived-At", "value": &chrono::Utc::now().to_rfc3339() } | ||
| ] | ||
| }); | ||
|
|
||
| let response = self.client | ||
| .post(&format!("{}/tx", bundler_url)) | ||
| .header("Content-Type", "application/json") | ||
| .json(&payload) | ||
| .send() | ||
| .await | ||
| .map_err(|e| PgnArchiveError::NetworkError(e.to_string()))?; | ||
|
|
||
| if !response.status().is_success() { | ||
| return Err(PgnArchiveError::ArweaveError( | ||
| format!("Arweave upload failed with status: {}", response.status()) | ||
| )); | ||
| } | ||
|
|
||
| let arweave_response: ArweaveResponse = response | ||
| .json() | ||
| .await | ||
| .map_err(|e| PgnArchiveError::SerializationError(e.to_string()))?; | ||
|
|
||
| Ok(arweave_response.id) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Bundlr Irys node /tx endpoint request format signed dataitem ANS-104
💡 Result:
The /tx endpoint on Bundlr/Irys nodes (e.g., https://node1.irys.xyz/tx or https://upload.ardrive.io/v1/tx) accepts POST requests with a signed ANS-104 DataItem as the raw binary request body. Request format: - Method: POST - URL: /{token}/tx or /tx or /v1/tx (token-specific path optional) - Headers: Content-Type: application/octet-stream - Body: Raw bytes of the fully signed DataItem (binary format per ANS-104) The signed DataItem is a binary structure: - Signature type (2 bytes) - Signature (variable) - Owner (variable) - Target (32 bytes + presence byte, optional) - Anchor (32 bytes + presence byte, optional) - Number of tags (8 bytes) - Number of tag bytes (8 bytes) - Tags (binary Avro array) - Data (variable binary payload) To create: Use libraries like @irys/bundles or arbundles to build, sign (with owner key), serialize to raw bytes via dataItem.getRaw, then POST as body. Example (from ArConnect docs): fetch(https://upload.ardrive.io/v1/tx, { method: "POST", headers: { "Content-Type": "application/octet-stream" }, body: dataItem.getRaw, // signed DataItem binary }) Response: Typically JSON with tx ID { id: "txid" }. Nodes verify signature, deduct fees from pre-funded balance, bundle into Arweave tx, return ID.
Citations:
- 1: https://github.com/ArweaveTeam/arweave-standards/blob/master/ans/ANS-104.md
- 2: https://github.com/arconnectio/wander-docs/blob/main/api/sign-dataitem.md
- 3: https://docs.ar.io/apis/turbo/upload-service/upload/
- 4: https://docs.ar.io/build/upload/x402-uploading-to-turbo/
🏁 Script executed:
# Check the file exists and examine the lines in question
head -c 5000 backend/modules/api/src/pgn_archive.rs | tail -c 3000Repository: NOVUS-X/XLMate
Length of output: 3058
🏁 Script executed:
# Get more context around lines 115-157
sed -n '110,160p' backend/modules/api/src/pgn_archive.rsRepository: NOVUS-X/XLMate
Length of output: 1924
🏁 Script executed:
# Search for any signing-related code in the file
rg -n "sign|Sign|bundler|ANS-104|dataitem" backend/modules/api/src/pgn_archive.rsRepository: NOVUS-X/XLMate
Length of output: 235
🏁 Script executed:
# Find the archive_pgn method to see the fallback logic
sed -n '160,220p' backend/modules/api/src/pgn_archive.rsRepository: NOVUS-X/XLMate
Length of output: 2062
🏁 Script executed:
# Search for where archive_to_arweave is called and how errors are handled
rg -n "archive_to_arweave|Arweave" backend/modules/api/src/pgn_archive.rs -A 3Repository: NOVUS-X/XLMate
Length of output: 2232
Arweave bundler request is unsigned — will 4xx in production.
Bundlr/Irys /tx endpoint requires a signed ANS-104 DataItem as raw binary (Content-Type: application/octet-stream), not a JSON payload with application/json. The code at lines 121–157 sends an unsigned JSON request that will fail with a 4xx error on any real bundler. The inline comment (lines 119–120) acknowledges this is not production-ready, but the function returns Result<String, PgnArchiveError> as if it succeeds; in practice, the error is caught in archive_pgn() and silently degraded to IPFS-only archival.
Either:
- Integrate a proper Arweave SDK with wallet signing (e.g., using libraries with ANS-104 support) before enabling this endpoint, or
- Feature-gate the Arweave path until signing is implemented, so callers are aware the feature is unavailable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/modules/api/src/pgn_archive.rs` around lines 115 - 157, The
archive_to_arweave function currently posts an unsigned JSON payload to the
bundler /tx endpoint which will 4xx in production; fix by either implementing
proper ANS-104 signing and sending a binary DataItem with Content-Type:
application/octet-stream (use an Arweave/Bundlr SDK to create/sign the DataItem
before POST) inside archive_to_arweave, or feature‑gate the Arweave path (e.g.,
check an env var like ARWEAVE_ENABLED in archive_to_arweave or the caller
archive_pgn) and immediately return a clear PgnArchiveError variant (e.g.,
NotSupported/Disabled) if not implemented; also ensure archive_pgn no longer
silently degrades by propagating the explicit error from archive_to_arweave
instead of hiding it.
| pub struct LobbyState { | ||
| sessions: HashMap<String, HashSet<Recipient<WsMessage>>>, | ||
| // Track number of connections per game for monitoring | ||
| connection_counts: HashMap<String, AtomicUsize>, | ||
| // Server instance ID for distributed tracking | ||
| instance_id: String, | ||
| // Redis connection for cross-instance communication | ||
| redis_conn: Option<Arc<ConnectionManager>>, | ||
| } | ||
|
|
||
| impl LobbyState { | ||
| pub fn new() -> Self { | ||
| LobbyState { sessions: HashMap::new() } | ||
| LobbyState { | ||
| sessions: HashMap::new(), | ||
| connection_counts: HashMap::new(), | ||
| instance_id: Uuid::new_v4().to_string(), | ||
| redis_conn: None, | ||
| } | ||
| } | ||
|
|
||
| pub fn with_redis(redis_conn: Arc<ConnectionManager>) -> Self { | ||
| LobbyState { | ||
| sessions: HashMap::new(), | ||
| connection_counts: HashMap::new(), | ||
| instance_id: Uuid::new_v4().to_string(), | ||
| redis_conn: Some(redis_conn), | ||
| } | ||
| } |
There was a problem hiding this comment.
LobbyState::with_redis is never called, so horizontal scaling is inactive.
Per backend/modules/api/src/server.rs:84-87, the app still starts the lobby via LobbyState::new().start(), meaning redis_conn is always None in production. All the publish logic added in Connect/Disconnect (lines 139-155, 183-199) is effectively dead in deployed instances, which contradicts the goal of issue #548.
Wire the Redis ConnectionManager in server.rs and replace LobbyState::new() with LobbyState::with_redis(redis_conn.clone()). Consider falling back to new() only when Redis env config is absent, and log at startup which mode was selected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/modules/api/src/ws.rs` around lines 56 - 83, The
LobbyState::with_redis variant is never used so redis_conn stays None; modify
the server startup to create a Redis ConnectionManager, pass its Arc clone into
LobbyState::with_redis(redis_conn.clone()) instead of LobbyState::new(), and
only fall back to LobbyState::new() when Redis configuration/env is absent; also
emit a startup log indicating whether the app started in "distributed (redis)"
or "local" mode so the publish/subscribe paths in Connect/Disconnect see an
active redis_conn.
| // Track number of connections per game for monitoring | ||
| connection_counts: HashMap<String, AtomicUsize>, | ||
| // Server instance ID for distributed tracking | ||
| instance_id: String, | ||
| // Redis connection for cross-instance communication | ||
| redis_conn: Option<Arc<ConnectionManager>>, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Dead code: connection_counts, publish_connection_event, and the two count helpers are never used.
connection_counts: HashMap<String, AtomicUsize>is declared and initialized but never written or read;get_connection_countreads fromsessionsinstead, so the atomic map is inert.publish_connection_eventis defined but never invoked —Connect/Disconnectinline the exact same JSON-build-and-publish logic (lines 139-155, 183-199), producing three near-identical copies.get_connection_countandget_total_connectionshave no callers inside the module and aren't exposed through any message/handler, so external actors can't reach them either.
Either remove them or actually use them: route Connect/Disconnect through publish_connection_event(&msg.game_id, "connected", count).await (you'll need to spawn from within the sync handler, so keep a small adapter), and drop connection_counts unless you intend to expose per-game metrics.
♻️ Suggested consolidation
pub struct LobbyState {
sessions: HashMap<String, HashSet<Recipient<WsMessage>>>,
- // Track number of connections per game for monitoring
- connection_counts: HashMap<String, AtomicUsize>,
// Server instance ID for distributed tracking
instance_id: String,
// Redis connection for cross-instance communication
redis_conn: Option<Arc<ConnectionManager>>,
}And in both handlers, replace the inline actix::spawn { ... json!{...} ... publish ... } with a single shared helper that builds the payload once.
Also, since AtomicUsize is no longer needed, the std::sync::atomic import added on line 19 can go too.
Also applies to: 85-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/modules/api/src/ws.rs` around lines 58 - 63, The module currently
contains dead code: the HashMap field connection_counts, the helper functions
publish_connection_event, get_connection_count, get_total_connections, and the
AtomicUsize import are unused while Connect/Disconnect inline duplicate
JSON/publish logic; either remove them entirely or wire them up. Recommend: drop
connection_counts and the atomic import if you won't expose per-game metrics,
and consolidate the duplicated publish logic by replacing the inline
actix::spawn blocks in the Connect and Disconnect handlers with calls to a
single async helper publish_connection_event(&msg.game_id, "connected" |
"disconnected", count).await (expose a small sync adapter that spawns the async
helper from the sync handler), have that helper build the JSON payload once
using instance_id and redis_conn, and remove the unused
get_connection_count/get_total_connections or add a message handler that calls
them if you need to expose counts; update references from sessions where
necessary to compute count before calling the helper.
| fn handle(&mut self, msg: Connect, _: &mut Context<Self>) { | ||
| let entry = self.sessions.entry(msg.game_id).or_default(); | ||
| let entry = self.sessions.entry(msg.game_id.clone()).or_default(); | ||
| entry.insert(msg.addr); | ||
|
|
||
| let count = entry.len(); | ||
| log::info!( | ||
| "[Instance {}] Client connected to game {}. Total connections: {}", | ||
| self.instance_id, | ||
| msg.game_id, | ||
| count | ||
| ); | ||
|
|
||
| // Publish connection event to Redis for horizontal scaling | ||
| let game_id = msg.game_id.clone(); | ||
| let redis_conn = self.redis_conn.clone(); | ||
| let instance_id = self.instance_id.clone(); | ||
|
|
||
| actix::spawn(async move { | ||
| if let Some(conn) = redis_conn { | ||
| let mut conn = (*conn).clone(); | ||
| let channel = format!("ws:connections:{}", game_id); | ||
| let message = json!({ | ||
| "event": "connected", | ||
| "game_id": game_id, | ||
| "instance_id": instance_id, | ||
| "count": count, | ||
| "timestamp": chrono::Utc::now().to_rfc3339() | ||
| }); | ||
|
|
||
| if let Ok(payload) = serde_json::to_string(&message) { | ||
| let _: redis::RedisResult<()> = conn.publish(channel, payload).await; | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Minor: count is captured before the spawn, but the entry-len read is fine; event ordering is not guaranteed.
Because the Redis publish is spawned on a separate task per event, two near-simultaneous connects/disconnects may be published out of order (the count snapshot is correct at enqueue time, but subscribers can't rely on monotonic ordering). If consumers derive state from this stream, consider either serializing publishes through a dedicated mailbox/task per game_id, or switching subscribers to a "source of truth = who publishes latest with highest timestamp" model.
Not a blocker given this is currently monitoring-only, but worth documenting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/modules/api/src/ws.rs` around lines 122 - 156, The current handle (fn
handle with Connect) spawns a task (actix::spawn) that publishes a snapshot
(count) to Redis, which can be observed out-of-order by subscribers; fix by
either (A) serializing publishes per game_id by introducing a dedicated
publisher actor/task/mailbox keyed on game_id that receives publish requests
(instead of spawning per event) so order is preserved, or (B) make events
idempotent/ordering-safe by adding a monotonic sequence and/or authoritative
timestamp to the payload (use a per-game AtomicU64 or Redis INCR) and include
that sequence/timestamp alongside instance_id, game_id and count so subscribers
can reconcile by “highest sequence/timestamp wins”; update the code that
currently uses redis_conn, game_id, count and actix::spawn to use the chosen
approach.
| pub async fn cleanup_expired_sessions( | ||
| db: &DatabaseConnection, | ||
| ) -> Result<u64, SessionError> { | ||
| let result = session::Entity::delete_many() | ||
| .filter(session::Column::ExpiresAt.lt(Utc::now())) | ||
| .exec(db) | ||
| .await?; | ||
|
|
||
| Ok(result.rows_affected) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify there are no existing callers of cleanup_expired_sessions.
rg -nP -C2 '\bcleanup_expired_sessions\s*\('Repository: NOVUS-X/XLMate
Length of output: 498
Schedule cleanup_expired_sessions — it is defined but never invoked.
The function is exposed with a doc comment saying it "should be called periodically," but searching the codebase finds no callers. Expired sessions will accumulate indefinitely in the database. Wire this into a periodic background task (e.g., actix_rt::spawn loop on interval, or an external cron job hitting an admin endpoint).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/modules/security/src/session_service.rs` around lines 147 - 156, The
cleanup_expired_sessions function is never scheduled; wire it into app startup
as a periodic background task that calls
session_service::cleanup_expired_sessions(db) on an interval (e.g.,
tokio::time::interval or actix_rt::spawn with a loop) so expired sessions are
removed regularly; ensure you pass a cloneable/shared DatabaseConnection (Arc or
DatabaseConnection::clone if supported) or a connection pool into the task,
handle/ log any SessionError returned (don’t unwrap), and choose an appropriate
interval (e.g., hourly) and graceful shutdown behavior so the task is started
once during server bootstrap (e.g., in your server startup/init function).
1 participant
…WebSocket scaling, and PGN archiving
Issue Backend: Secure session management with JWT and Refresh Tokens #549: Enhanced JWT session management with session tracking, IP binding, and refresh token rotation
Issue Backend: Websocket scaling with horizontal load balancing #548: WebSocket scaling with horizontal load balancing
Issue Backend: Automated PGN archiving to IPFS/Arweave #541: Automated PGN archiving to IPFS/Arweave
All implementations follow existing code patterns and include proper documentation.
closes #538
Closes #541
Closes #548
Closes #549
Summary by CodeRabbit
Release Notes
New Features
Chores