From 4496f05e8a50afcde00ff5cf8651129eef768ef8 Mon Sep 17 00:00:00 2001 From: Jad Seifeddine Date: Tue, 13 Oct 2026 13:58:05 +0700 Subject: [PATCH] api: create a session at the size it will be shown at, and reuse a fresh Pending one Sizing. The entry connect request now carries the screen's size as well as the window's (screen_width/screen_height, sent by every connect site in connections.html and the client's relaunch). initial_session_size() picks the size the session will be shown at: an RDP/VNC entry that opens fullscreen on connect is created at the screen's size, everything else at the window's, and every RDP size is rounded down to the 16px grid that guacd's display channel (patches/007) rounds the first post-connect size send to. Creation and that send therefore agree, guacd coalesces them, and there is no resize, no H.264 decoder rebuild and no flash on connect. The client's post-connect size send stays as it is. Reuse. A repeat Connect on the same entry within 15 s no longer opens a duplicate: a Pending session (guacd connected, owner never attached) this user created for the entry is handed back instead. The match is made on the fully built CreateSessionRequest and includes its credentials: CreateSessionRequest::credential_fingerprint() is a SHA-256 over the target and every credential field (username, password, private key, domain, auth package, KDC, jump hosts and their credentials, container and Proxmox credentials), stored on the Session and compared, never logged or served. A retry with a different password, key or domain, such as a mistyped prompt, gets a new session rather than the old Pending one with the old credentials. Reuse is also scoped to the same user, the same entry key, Pending status with a live guacd stream, and sessions younger than SessionManager::REUSABLE_PENDING_SECS. #238 (native/HiDPI resolution, display_scale) has not merged: both sizes here are CSS pixels, matching today's post-connect send. When it lands the mod-16 rounding must be applied to the device-pixel size, after the devicePixelRatio multiply; the rounding site carries that note. Tests: fullscreen_sizing_applies_to_bitmap_sessions_only and round_down_mod16_floors_at_sixteen (api); credential_fingerprint_tracks_target_and_credentials, fresh_pending_session_reused_for_same_entry_and_credentials, retry_with_different_credentials_gets_a_new_session and reuse_is_scoped_to_user_entry_status_and_age (session). Part 1/2 of #257. --- src/api.rs | 144 +++++++++++++++++++- src/session.rs | 281 ++++++++++++++++++++++++++++++++++++++++ static/client.html | 2 + static/connections.html | 11 +- 4 files changed, 433 insertions(+), 5 deletions(-) diff --git a/src/api.rs b/src/api.rs index cfe236da..bb960b80 100644 --- a/src/api.rs +++ b/src/api.rs @@ -2738,6 +2738,13 @@ pub struct ConnectRequest { pub width: Option, #[serde(default)] pub height: Option, + /// The screen's size in CSS pixels. An entry that opens fullscreen on + /// connect is created at this size instead of the window's, so nothing + /// has to be resized once the fullscreen transition lands. + #[serde(default)] + pub screen_width: Option, + #[serde(default)] + pub screen_height: Option, #[serde(default)] pub dpi: Option, #[serde(default)] @@ -2829,6 +2836,62 @@ pub struct ProbeHostKeyRequest { pub port: Option, } +/// The size a new session is created at. +/// +/// A bitmap (RDP/VNC) entry that opens fullscreen on connect is created at +/// the screen's size, which is the size it will have once fullscreen lands. +/// Until the client's fullscreen gesture fires it scales that picture to fit +/// the window, so the desktop is painted once at its final size and the +/// fullscreen transition brings no resize, no decoder rebuild and no second +/// repaint (#257). Everything else keeps the window's size: a terminal +/// reflows to fullscreen instantly. +/// +/// Both sizes are CSS pixels, which is what the client's post-connect size +/// send uses today, so creation and that send agree. When #238 lands +/// (native/HiDPI resolution, `display_scale`), that send becomes the +/// device-pixel size, and the mod-16 rounding below must then be applied to +/// the final size after the devicePixelRatio multiply, not to the CSS size. +pub fn initial_session_size( + session_type: &str, + fullscreen_on_connect: bool, + window: (Option, Option), + screen: (Option, Option), +) -> (Option, Option) { + let bitmap = matches!(session_type, "rdp" | "vnc"); + let (w, h) = match (bitmap && fullscreen_on_connect, screen) { + (true, (Some(w), Some(h))) if w > 0 && h > 0 => (Some(w), Some(h)), + _ => window, + }; + // RDP's display channel rounds every requested size down to a multiple + // of 16 (patches/007, which prevents the H.264 green band), so the + // client's first post-connect size send always lands on that grid. + // Create the session there already, fullscreen or windowed, so the two + // match: guacd coalesces them and there is no resize, the H.264 decoder + // is not rebuilt and the picture never flashes on connect (#257). VNC + // has no such rounding, so it keeps the exact size. + // + // #238 (not merged at the time of writing): once the session is created + // at the device-pixel size, round that size, i.e. apply round_down_mod16 + // after multiplying by devicePixelRatio, since rounding the CSS size and + // then scaling it would leave the grid again. + if session_type == "rdp" { + (w.map(round_down_mod16), h.map(round_down_mod16)) + } else { + (w, h) + } +} + +/// Round a dimension down to a multiple of 16, with a 16px floor so a tiny +/// screen never rounds to zero. Mirrors guacd's RDP display-channel rounding. +fn round_down_mod16(v: u32) -> u32 { + let r = v - (v % 16); + if r < 16 { + 16 + } else { + r + } +} + /// POST /api/addressbook/folders/:scope/:folder/entries/:entry/connect — Create session from entry. #[allow(clippy::too_many_arguments)] pub async fn ab_connect_entry( @@ -2931,6 +2994,12 @@ pub async fn ab_connect_entry( // Build CreateSessionRequest from the Vault entry + connect request display params. // ConnectRequest credentials override address book values (for prompted credentials). let ab_entry_key = format!("{}/{}/{}", scope, folder, entry); + let (initial_width, initial_height) = initial_session_size( + &ab_entry.session_type, + ab_entry.fullscreen_on_connect.unwrap_or(false), + (req.width, req.height), + (req.screen_width, req.screen_height), + ); let create_req = CreateSessionRequest { session_type, hostname: ab_entry.hostname, @@ -2954,8 +3023,8 @@ pub async fn ab_connect_entry( jump_username: None, jump_password: None, jump_private_key: None, - width: req.width, - height: req.height, + width: initial_width, + height: initial_height, dpi: req.dpi, banner: req.banner.or(ab_entry.banner), enable_drive: ab_entry.enable_drive, @@ -2964,7 +3033,7 @@ pub async fn ab_connect_entry( remote_app_args: ab_entry.remote_app_args, enable_recording: ab_entry.enable_recording, record_typescript: ab_entry.record_typescript, - address_book_entry: Some(ab_entry_key), + address_book_entry: Some(ab_entry_key.clone()), address_book_folder: Some(folder.to_string()), entry_display_name: ab_entry.display_name.clone(), max_recordings: ab_entry.max_recordings, @@ -3025,6 +3094,28 @@ pub async fn ab_connect_entry( "Address book connect requested" ); + // Reuse a Pending session this user already opened for this entry rather + // than spawning a duplicate (e.g. a repeat Connect click). Matched on the + // fully built request above, credentials included: a retry with a + // different password, key or domain (a mistyped prompt) must not get + // the old Pending session, which still holds the old credentials. The + // fingerprint is a hash, compared only, never logged. + if let Some(existing) = manager + .find_reusable_session( + &admin_name, + &ab_entry_key, + &create_req.credential_fingerprint(), + ) + .await + { + tracing::info!( + user = %admin_name, + session_id = %existing.session_id, + "Reusing existing address book session" + ); + return (StatusCode::CREATED, Json(json!(existing))).into_response(); + } + match manager.create_session(create_req, admin_name.clone()).await { Ok(info) => { tracing::info!( @@ -5780,6 +5871,53 @@ mod tests { use super::*; use std::path::Path; + #[test] + fn fullscreen_sizing_applies_to_bitmap_sessions_only() { + let win = (Some(1710), Some(954)); + let scr = (Some(1710), Some(1112)); + // RDP opening fullscreen: created at the screen size rounded down to + // a multiple of 16, matching guacd's display-channel rounding so there + // is no post-connect resize (#257). VNC is not mod-16 rounded. + assert_eq!( + initial_session_size("rdp", true, win, scr), + (Some(1696), Some(1104)) + ); + assert_eq!(initial_session_size("vnc", true, win, scr), scr); + // A screen that is already mod-16 is created at its exact size. + assert_eq!( + initial_session_size("rdp", true, win, (Some(1920), Some(1088))), + (Some(1920), Some(1088)) + ); + // a terminal reflows instantly: window size, fullscreen or not + assert_eq!(initial_session_size("ssh", true, win, scr), win); + assert_eq!(initial_session_size("telnet", true, win, scr), win); + assert_eq!(initial_session_size("web", true, win, scr), win); + // not fullscreen, or no screen size offered: window size — still on + // the mod-16 grid for RDP, since the client's first size send is + // rounded to it by guacd either way (no resize on connect). + let win16 = (Some(1696), Some(944)); + assert_eq!(initial_session_size("rdp", false, win, scr), win16); + assert_eq!(initial_session_size("rdp", true, win, (None, None)), win16); + assert_eq!( + initial_session_size("rdp", true, win, (Some(0), Some(0))), + win16 + ); + assert_eq!(initial_session_size("vnc", false, win, scr), win); + assert_eq!( + initial_session_size("rdp", false, (None, None), scr), + (None, None) + ); + } + + #[test] + fn round_down_mod16_floors_at_sixteen() { + assert_eq!(round_down_mod16(1710), 1696); + assert_eq!(round_down_mod16(1696), 1696); + assert_eq!(round_down_mod16(16), 16); + assert_eq!(round_down_mod16(15), 16); + assert_eq!(round_down_mod16(0), 16); + } + #[test] fn report_bounds_read_a_bare_date_as_a_whole_local_day() { // Sydney in September: UTC+10, so getTimezoneOffset() is -600. diff --git a/src/session.rs b/src/session.rs index ef28dcbb..b1b57592 100644 --- a/src/session.rs +++ b/src/session.rs @@ -205,6 +205,61 @@ pub struct CreateSessionRequest { pub wol_wait_time: Option, } +impl CreateSessionRequest { + /// SHA-256 over the target and every credential this request would + /// connect with, as hex. Two requests with the same fingerprint would + /// open the same connection as the same principal, so a fresh Pending + /// session made from one can stand in for the other + /// (`SessionManager::find_reusable_session`). A retry with a different + /// password, key, domain or jump credential gets a different + /// fingerprint and therefore a new session. The hash is never logged + /// or returned to a client, and a secret cannot be recovered from it. + pub fn credential_fingerprint(&self) -> String { + use sha2::{Digest, Sha256}; + let mut h = Sha256::new(); + // Every field is tagged and length-prefixed, so `("ab", "c")` and + // `("a", "bc")` hash differently and an absent field differs from + // an empty one. + let mut field = |tag: &str, value: Option<&str>| { + h.update(tag.as_bytes()); + match value { + Some(v) => { + h.update((v.len() as u64).to_le_bytes()); + h.update(v.as_bytes()); + } + None => h.update([0xffu8; 8]), + } + }; + let port = self.port.map(|p| p.to_string()); + let jump_hosts = self + .jump_hosts + .as_ref() + .and_then(|j| serde_json::to_string(j).ok()); + let jump_port = self.jump_port.map(|p| p.to_string()); + field("type", Some(&format!("{:?}", self.session_type))); + field("host", self.hostname.as_deref()); + field("port", port.as_deref()); + field("user", self.username.as_deref()); + field("pass", self.password.as_deref()); + field("key", self.private_key.as_deref()); + field("domain", self.domain.as_deref()); + field("auth_pkg", self.auth_pkg.as_deref()); + field("kdc", self.kdc_url.as_deref()); + field("url", self.url.as_deref()); + field("jumps", jump_hosts.as_deref()); + field("jump_host", self.jump_host.as_deref()); + field("jump_port", jump_port.as_deref()); + field("jump_user", self.jump_username.as_deref()); + field("jump_pass", self.jump_password.as_deref()); + field("jump_key", self.jump_private_key.as_deref()); + field("container_user", self.container_username.as_deref()); + field("container_pass", self.container_password.as_deref()); + field("proxmox_token_id", self.proxmox_token_id.as_deref()); + field("proxmox_token_secret", self.proxmox_token_secret.as_deref()); + hex::encode(h.finalize()) + } +} + /// Session status in the lifecycle. #[derive(Debug, Clone, Serialize, PartialEq)] #[serde(rename_all = "lowercase")] @@ -325,6 +380,10 @@ pub struct Session { /// (#154). Surfaced verbatim in `SessionInfo` so client.html can /// trigger fullscreen on first user gesture after CONNECTED. pub fullscreen_on_connect: bool, + /// `CreateSessionRequest::credential_fingerprint()` of the request this + /// session was created from. Compared, never logged or served, when a + /// repeat Connect asks to reuse a Pending session. + pub credential_fingerprint: String, /// Copied from the source entry's `autohide_side_tabs` flag. /// Surfaced in `SessionInfo` so client.html can auto-hide the /// clipboard/files side tabs. @@ -789,6 +848,9 @@ impl SessionManager { req: CreateSessionRequest, created_by: String, ) -> Result { + // Taken before any field of the request is moved out below. + let credential_fingerprint = req.credential_fingerprint(); + // Enforce session limits (only count active/pending sessions) { let sessions = self.sessions.read().await; @@ -918,6 +980,8 @@ impl SessionManager { session_id = %session_id, hostname = %hostname, username = %username, + width, + height, "Creating new SSH session" ); @@ -1945,6 +2009,7 @@ impl SessionManager { shadow_tokens: Vec::new(), share_allowed, fullscreen_on_connect: req.fullscreen_on_connect.unwrap_or(false), + credential_fingerprint, autohide_side_tabs: req.autohide_side_tabs.unwrap_or(false), }; @@ -2009,6 +2074,45 @@ impl SessionManager { result } + /// How long after creation a Pending session is offered to a repeat + /// Connect instead of a fresh one. Short on purpose: an older Pending + /// one, whose guacd may since have gone away, is left to the + /// pending-timeout reaper. + pub const REUSABLE_PENDING_SECS: i64 = 15; + + /// Find a live session this user can be sent back to instead of opening a + /// fresh one for the same address-book entry. Reusing a `Pending` session + /// (guacd connected, owner never attached) stops repeat Connect clicks from + /// piling up orphans; only a Pending session is reusable, since an owner + /// connection consumes the guacd stream and cannot be re-owned. + /// + /// `credential_fingerprint` is `CreateSessionRequest::credential_fingerprint()` + /// of the fully built request, so a session is only reused by a request + /// that would connect to the same target with the same credentials. A + /// retry with a different password, key or domain (a mistyped prompt) + /// never gets the old session, which still holds the old credentials. + pub async fn find_reusable_session( + &self, + created_by: &str, + entry_key: &str, + credential_fingerprint: &str, + ) -> Option { + let sessions = self.sessions.read().await; + for session in sessions.values() { + let s = session.lock().await; + if s.status == SessionStatus::Pending + && s.guacd_stream.is_some() + && s.created_by == created_by + && s.address_book_entry.as_deref() == Some(entry_key) + && s.credential_fingerprint == credential_fingerprint + && (Utc::now() - s.created_at).num_seconds() < Self::REUSABLE_PENDING_SECS + { + return Some(s.info()); + } + } + None + } + /// Get a specific session's info. pub async fn get_session(&self, id: Uuid) -> Option { let sessions = self.sessions.read().await; @@ -2915,6 +3019,7 @@ mod tests { shadow_tokens: Vec::new(), share_allowed: true, fullscreen_on_connect: false, + credential_fingerprint: String::new(), autohide_side_tabs: false, } } @@ -3455,6 +3560,182 @@ mod tests { let cfg = DriveConfig::default(); assert_eq!(drive_cleanup_settings(&Some(cfg)), (true, 0)); } + + // ── Pending-session reuse (repeat Connect on an entry) ── + + fn entry_request(extra: serde_json::Value) -> CreateSessionRequest { + let mut body = serde_json::json!({ + "session_type": "rdp", + "hostname": "rdp.example.test", + "port": 3389, + "username": "alice", + "password": "correct horse", + "domain": "CORP", + "address_book_entry": "shared/ops/rdp-1", + }); + body.as_object_mut() + .unwrap() + .extend(extra.as_object().unwrap().clone()); + serde_json::from_value(body).unwrap() + } + + /// A Pending session as `create_session` leaves it for an entry connect: + /// guacd connected (any stream will do here), owner not yet attached. + fn pending_entry_session(req: &CreateSessionRequest, created_by: &str) -> Session { + let (a, _b) = tokio::io::duplex(64); + let mut s = seed_test_session(&format!("tok-{}", Uuid::new_v4())); + s.status = SessionStatus::Pending; + s.guacd_stream = Some(Box::new(a)); + s.created_by = created_by.into(); + s.address_book_entry = req.address_book_entry.clone(); + s.credential_fingerprint = req.credential_fingerprint(); + s + } + + #[test] + fn credential_fingerprint_tracks_target_and_credentials() { + let base = entry_request(serde_json::json!({})); + let same = entry_request(serde_json::json!({})); + assert_eq!(base.credential_fingerprint(), same.credential_fingerprint()); + assert_eq!(base.credential_fingerprint().len(), 64); + // Does not contain a secret in the clear. + assert!(!base.credential_fingerprint().contains("correct horse")); + + for (key, value) in [ + ("password", serde_json::json!("wrong horse")), + ("username", serde_json::json!("bob")), + ("domain", serde_json::json!("OTHER")), + ("private_key", serde_json::json!("-----BEGIN KEY-----")), + ("hostname", serde_json::json!("rdp2.example.test")), + ("port", serde_json::json!(3390)), + ("jump_password", serde_json::json!("hop")), + ] { + let other = entry_request(serde_json::json!({ key: value })); + assert_ne!( + base.credential_fingerprint(), + other.credential_fingerprint(), + "{key} must change the fingerprint" + ); + } + // Display params are not part of it: a second click from a resized + // window still reuses the session. + let resized = entry_request(serde_json::json!({ "width": 800, "height": 600 })); + assert_eq!( + base.credential_fingerprint(), + resized.credential_fingerprint() + ); + // Absent and empty are different, and so is a shifted boundary. + let empty_pw = entry_request(serde_json::json!({ "password": "" })); + let no_pw = { + let mut r = entry_request(serde_json::json!({})); + r.password = None; + r + }; + assert_ne!( + empty_pw.credential_fingerprint(), + no_pw.credential_fingerprint() + ); + let ab_c = entry_request(serde_json::json!({ "username": "ab", "password": "c" })); + let a_bc = entry_request(serde_json::json!({ "username": "a", "password": "bc" })); + assert_ne!(ab_c.credential_fingerprint(), a_bc.credential_fingerprint()); + } + + #[tokio::test] + async fn fresh_pending_session_reused_for_same_entry_and_credentials() { + let mgr = new_manager_for_tests(); + let req = entry_request(serde_json::json!({})); + let id = insert_session(&mgr, pending_entry_session(&req, "alice@example.com")).await; + + let again = entry_request(serde_json::json!({})); + let found = mgr + .find_reusable_session( + "alice@example.com", + "shared/ops/rdp-1", + &again.credential_fingerprint(), + ) + .await + .expect("same entry, same credentials, within the window: reused"); + assert_eq!(found.session_id, id); + } + + #[tokio::test] + async fn retry_with_different_credentials_gets_a_new_session() { + let mgr = new_manager_for_tests(); + let req = entry_request(serde_json::json!({})); + insert_session(&mgr, pending_entry_session(&req, "alice@example.com")).await; + + // A mistyped password at the prompt, retried within 15 s: the old + // Pending session holds the old password, so it must not be handed + // back. + let retry = entry_request(serde_json::json!({ "password": "wrong horse" })); + assert!(mgr + .find_reusable_session( + "alice@example.com", + "shared/ops/rdp-1", + &retry.credential_fingerprint(), + ) + .await + .is_none()); + let other_user = entry_request(serde_json::json!({ "username": "bob" })); + assert!(mgr + .find_reusable_session( + "alice@example.com", + "shared/ops/rdp-1", + &other_user.credential_fingerprint(), + ) + .await + .is_none()); + } + + #[tokio::test] + async fn reuse_is_scoped_to_user_entry_status_and_age() { + let mgr = new_manager_for_tests(); + let req = entry_request(serde_json::json!({})); + let fp = req.credential_fingerprint(); + + insert_session(&mgr, pending_entry_session(&req, "alice@example.com")).await; + // Another user's identical Pending session is not ours to take. + assert!(mgr + .find_reusable_session("mallory@example.com", "shared/ops/rdp-1", &fp) + .await + .is_none()); + // Nor is the same user's session for a different entry. + assert!(mgr + .find_reusable_session("alice@example.com", "shared/ops/rdp-2", &fp) + .await + .is_none()); + + // An owner already attached: the stream is consumed, not re-ownable. + let mgr = new_manager_for_tests(); + let mut active = pending_entry_session(&req, "alice@example.com"); + active.status = SessionStatus::Active; + insert_session(&mgr, active).await; + assert!(mgr + .find_reusable_session("alice@example.com", "shared/ops/rdp-1", &fp) + .await + .is_none()); + + // Older than the window: left to the pending-timeout reaper. + let mgr = new_manager_for_tests(); + let mut stale = pending_entry_session(&req, "alice@example.com"); + stale.created_at = + Utc::now() - chrono::Duration::seconds(SessionManager::REUSABLE_PENDING_SECS + 1); + insert_session(&mgr, stale).await; + assert!(mgr + .find_reusable_session("alice@example.com", "shared/ops/rdp-1", &fp) + .await + .is_none()); + + // No guacd stream (deferred connect): nothing to hand over yet. + let mgr = new_manager_for_tests(); + let mut deferred = pending_entry_session(&req, "alice@example.com"); + deferred.guacd_stream = None; + insert_session(&mgr, deferred).await; + assert!(mgr + .find_reusable_session("alice@example.com", "shared/ops/rdp-1", &fp) + .await + .is_none()); + } } impl std::fmt::Display for SessionError { diff --git a/static/client.html b/static/client.html index 055d1961..49fa3dea 100644 --- a/static/client.html +++ b/static/client.html @@ -1750,6 +1750,8 @@

Session Ended

var body = { width: window.innerWidth, height: window.innerHeight, + screen_width: screen.width, + screen_height: screen.height, dpi: Math.round((window.devicePixelRatio || 1) * 96) }; diff --git a/static/connections.html b/static/connections.html index 4b0f0b24..1e888bea 100644 --- a/static/connections.html +++ b/static/connections.html @@ -1601,7 +1601,7 @@

Enter Credentials

method: 'POST', headers: Object.assign({'Content-Type': 'application/json'}, apiHeaders()), credentials: 'same-origin', - body: JSON.stringify({ width: window.innerWidth, height: window.innerHeight }) + body: JSON.stringify({ width: window.innerWidth, height: window.innerHeight, screen_width: screen.width, screen_height: screen.height }) }) .then(function(res) { return res.json(); }) .then(function(data) { @@ -1895,7 +1895,7 @@

Enter Credentials

method: 'POST', headers: Object.assign({'Content-Type': 'application/json'}, apiHeaders()), credentials: 'same-origin', - body: JSON.stringify({ width: window.innerWidth, height: window.innerHeight }) + body: JSON.stringify({ width: window.innerWidth, height: window.innerHeight, screen_width: screen.width, screen_height: screen.height }) }).then(function(res) { return res.json(); }).then(function(data) { if (data.session_id) window.open('/client/' + data.session_id, '_blank'); else showError(data.error || 'Failed to reconnect'); @@ -3021,6 +3021,13 @@

Enter Credentials

body.height = bh; if (!body.dpi) body.dpi = Math.round((window.devicePixelRatio || 1) * 96); } + // And the screen's, so an entry that opens fullscreen on connect + // is created at the size it will be shown at (no resize after + // the fullscreen transition). + if (!body.screen_width && window.screen && screen.width > 0 && screen.height > 0) { + body.screen_width = screen.width; + body.screen_height = screen.height; + } fetch('/api/addressbook/folders/' + encodeURIComponent(scope) + '/' + encodeURIComponent(folder) + '/entries/' + encodeURIComponent(name) + '/connect', { method: 'POST', headers: apiHeaders({ 'Content-Type': 'application/json' }),