CP-310425: Include user names in the http response message #6766

stephenchengCloud · 2025-11-25T08:58:00Z

When a VNC console connection is rejected due to the session limit,
users want to know which user(s) are currently connected. However,
displaying usernames in HTTP error responses may raise privacy concerns
in some deployments, so also add a configure to enable/disable the
display of the usernames.

The main changes are:
1. To contain the active users in the response message,
changed the active_connections to record the existing users
and use the unique session id to identify them in case
multiple connections have the same user name.

2. Added escape_char to escape html special characters

3. Added `include_console_username_in_error` to enable/disable
   the display of user names

Tested:

display-user-name-in-XC = true by default

# xe pool-param-set uuid=576102a6-c567-c753-5a0e-9a0614674d09 limit-console-sessions=true

Included the currently connected user root in the http response message:

Nov 25 08:35:32 genuk-21-06d xapi: [debug||1441 HTTPS 10.70.0.166->:::80|Connection to VM console R:c7ad391b568e|console] limit_console_sessions is true. Console connection is rejected for VM OpaqueRef:a071805a-370a-c36b-1608-90e42ec28130, active connections: 1
Nov 25 08:35:32 genuk-21-06d xapi: [debug||1441 HTTPS 10.70.0.166->:::80|Connection to VM console R:c7ad391b568e|console] Sending console limit exceeded response: <html><body><h1>Connection Limit Exceeded</h1><p>User 'root' is currently connected to this console (VM OpaqueRef:a071805a-370a-c36b-1608-90e42ec28130). No more connections are allowed when limit_console_sessions is enabled.</p></body></html>

Set display-user-name-in-XC = false
No user name in the http response message:

Nov 25 08:47:11 genuk-21-06d xapi: [debug||282 HTTPS 10.70.0.167->:::80|Connection to VM console R:2ff9049ff22a|console] limit_console_sessions is true. Console connection is rejected for VM OpaqueRef:a071805a-370a-c36b-1608-90e42ec28130, active connections: 1
Nov 25 08:47:11 genuk-21-06d xapi: [debug||282 HTTPS 10.70.0.167->:::80|Connection to VM console R:2ff9049ff22a|console] Sending console limit exceeded response: <html><body><h1>Connection Limit Exceeded</h1><p>There're users currently connected to this console (VM OpaqueRef:a071805a-370a-c36b-1608-90e42ec28130). No more connections are allowed when limit_console_sessions is enabled.</p></body></html>

minglumlu · 2025-11-25T09:29:06Z

ocaml/xapi/xapi_globs.ml


 let ssh_auto_mode_default = ref true

+let display_user_name_in_XC = ref true


In a general way, it could be include_console_username_in_error.

minglumlu · 2025-11-25T09:31:19Z

ocaml/xapi/console.ml

+      in
+      Printf.sprintf
+        "<html><body><h1>Connection Limit Exceeded</h1><p>%s currently \
+         connected to this console (VM %s). No more connections are allowed \


connected -> connecting

connected is correct.
The message would be:
User 'root' is currently connected to ...

lindig · 2025-11-25T09:30:00Z

ocaml/xapi/console.ml


-  let active_connections : int VMMap.t ref = ref VMMap.empty
+  (* VMMap maps VM IDs to a list of (user_name, session_id) tuples representing active connections *)
+  let active_connections : (string * string) list VMMap.t ref = ref VMMap.empty


I'd prefer a record type if possible to avoid that these strings are confused.

Yes. record is much better.

lindig · 2025-11-25T09:32:08Z

ocaml/xapi/console.ml

-        | Some _ | None ->
-            active_connections := VMMap.remove vm_id !active_connections
+        | Some connections ->
+            let updated_connections =


could do:

match List.filter (fun (_, sid) -> sid <> session_id) connections with | [] -> .. | upd -> ..

lindig · 2025-11-25T09:33:05Z

ocaml/xapi/console.ml

+          VMMap.find_opt vm_id !active_connections |> Option.value ~default:[]
        in
+        let count = List.length connections in
        if is_limit_enabled && count > 0 then (


It would suffice to check against the empty list and to avoid the length call.

lindig · 2025-11-25T09:35:21Z

ocaml/xapi/console.ml

+      | c ->
+          String.make 1 c
+    in
+    String.fold_left (fun acc c -> acc ^ escape_char c) "" s


This is an expensive way to create a string. We should not use this if this is happen frequently or the string is long. The string by default is constructed one character at a time, creating a lot of garbage along the way.

We probably have code for this; a function like html_escape should be put into an existing library to make it more accessible and to avoid that we re-implement it.

Thank you for telling me. I'll look for existing library or make it a common function.

If nothing exists, I suggest:

have a quick scan over the string. If no character needs escaping, use the string as is

if there is something to do, use Buffer to create the resulting string

Found an exsiting function:
Xapi_stdext_std.Xstringext.String.escaped

This existing function is slow, too - but we are going to improve it.

psafont

The commit message is lacking information on why is this change needed.

I understand is a refinement on the previous design when there was no option to avoid leaking user names to clients, but this needs to be spelled out at least in the commit message.

ocaml/xapi/xapi_globs.ml

minglumlu · 2025-11-25T10:07:51Z

ocaml/xapi/console.ml

+        match connected_users with
+        | [user] ->
+            Printf.sprintf "User '%s' is" (html_escape user)
+        | users ->


This can be empty.

You're right, though it's unlikely to be empty. We have to consider the case.

minglumlu · 2025-11-25T10:49:00Z

ocaml/xapi/console.ml

    with_lock mutex (fun () ->
        match VMMap.find_opt vm_id !active_connections with
-        | Some n when n > 1 ->
-            active_connections := VMMap.add vm_id (n - 1) !active_connections
-        | Some _ | None ->
-            active_connections := VMMap.remove vm_id !active_connections
+        | Some connections ->
+            let updated_connections =
+              List.filter (fun (_, sid) -> sid <> session_id) connections
+            in
+            if updated_connections = [] then
+              active_connections := VMMap.remove vm_id !active_connections
+            else
+              active_connections :=
+                VMMap.add vm_id updated_connections !active_connections
+        | None ->
+            (* Unlikely *)
+            ()


How about:

with_lock mutex (fun () -> let f conns = Option.bind conns (fun conns -> match List.filter (fun (_, sid) -> sid <> session_id) conns with | [] -> None | l -> Some l ) in active_connections := VMMap.update vm_id f !active_connections )

minglumlu · 2025-11-25T10:54:03Z

ocaml/xapi/console.ml

-        )
+    )
+
+  let get_connected_users vm_id =


It's better to get the users from try_add.

stephenchengCloud · 2025-11-25T11:37:59Z

The commit message is lacking information on why is this change needed.

I understand is a refinement on the previous design when there was no option to avoid leaking user names to clients, but this needs to be spelled out at least in the commit message.

Updated the commit message.

stephenchengCloud · 2025-11-25T11:39:06Z

All comments resolved.
Tested:

Nov 25 11:18:36 genuk-21-06d xapi: [debug||215 HTTPS 10.70.0.166->:::80|Connection to VM console R:7fad226cc58f|console] limit_console_sessions is true. Console connection is rejected for VM OpaqueRef:a071805a-370a-c36b-1608-90e42ec28130, active connections: 1
Nov 25 11:18:36 genuk-21-06d xapi: [debug||215 HTTPS 10.70.0.166->:::80|Connection to VM console R:7fad226cc58f|console] Sending console limit exceeded response: <html><body><h1>Connection Limit Exceeded</h1><p>User 'root' is currently connected to this console (VM OpaqueRef:a071805a-370a-c36b-1608-90e42ec28130). No more connections are allowed when limit_console_sessions is enabled.</p></body></html>

ocaml/xapi/console.ml

minglumlu · 2025-11-26T02:21:50Z

ocaml/xapi/console.ml

+    if !Xapi_globs.include_console_username_in_error then
+      let users_text =
+        match connected_users with
+        | [] ->


Since now the connected_users comes from try_add, it must not be empty. So it would be an internal error or bug in this case that I think raise Failure after an error logging can be used.

minglumlu · 2025-11-26T02:25:28Z

ocaml/xapi/console.ml

+      Printf.sprintf
+        "<html><body><h1>Connection Limit Exceeded</h1><p>There're users \
+         currently connected to this console (VM %s). No more connections are \
+         allowed when limit_console_sessions is enabled.</p></body></html>"
+        vm_id


This duplication could be resolved like:

let body = let users_text = match (!Xapi_globs.include_console_username_in_error, connected_users) with | true, [] -> error "The connected user list should not be empty." ; raise Failure | true, [user] -> Printf.sprintf "User '%s' is" (html_escape user) | true, users -> let escaped_users = List.map html_escape users in Printf.sprintf "Users '%s' are" (String.concat ", " escaped_users) | false, _ -> "There're users" in Printf.sprintf "<html><body><h1>Connection Limit Exceeded</h1><p>%s currently \ connected to this console (VM %s). No more connections are allowed \ when limit_console_sessions is enabled.</p></body></html>" users_text vm_id in

I think I need to get more used to using match instead of if in OCaml. :)

When a VNC console connection is rejected due to the session limit, users want to know which user(s) are currently connected. However, displaying usernames in HTTP error responses may raise privacy concerns in some deployments, so also add a configure to enable/disable the display of the usernames. The main changes are: 1. To contain the active users in the response message, changed the active_connections to record the existing users and use the unique session id to identify them in case multiple connections have the same user name. 2. Use Http_svr.escape to escape html special characters 3. Added `include_console_username_in_error` to enable/disable the display of user names Signed-off-by: Stephen Cheng <stephen.cheng@citrix.com>

robhoes · 2025-11-26T13:59:56Z

ocaml/xapi/console.ml

+        let f conns =
+          Option.bind conns (fun conns ->
+              match
+                List.filter (fun conn -> conn.session_id <> session_id) conns


This is dropping all entries for a given session_id when a connection finishes. However, it is possible to use the same session_id on multiple connections.

If that is the case, we need to define an unique id for each connection added.

stephenchengCloud requested review from lindig, minglumlu and robhoes November 25, 2025 09:24

minglumlu reviewed Nov 25, 2025

View reviewed changes

lindig reviewed Nov 25, 2025

View reviewed changes

psafont reviewed Nov 25, 2025

View reviewed changes

ocaml/xapi/xapi_globs.ml Outdated Show resolved Hide resolved

minglumlu reviewed Nov 25, 2025

View reviewed changes

stephenchengCloud force-pushed the private/stephenche/CP-310425 branch 2 times, most recently from 7d30cf5 to 22c1839 Compare November 25, 2025 11:37

lindig approved these changes Nov 25, 2025

View reviewed changes

psafont reviewed Nov 25, 2025

View reviewed changes

ocaml/xapi/console.ml Outdated Show resolved Hide resolved

stephenchengCloud force-pushed the private/stephenche/CP-310425 branch from 22c1839 to 846927e Compare November 26, 2025 02:14

minglumlu reviewed Nov 26, 2025

View reviewed changes

minglumlu approved these changes Nov 26, 2025

View reviewed changes

stephenchengCloud force-pushed the private/stephenche/CP-310425 branch from 846927e to f55ac2c Compare November 26, 2025 04:44

robhoes reviewed Nov 26, 2025

View reviewed changes


		let ssh_auto_mode_default = ref true

		let display_user_name_in_XC = ref true

CP-310425: Include user names in the http response message #6766

Are you sure you want to change the base?

CP-310425: Include user names in the http response message #6766

Conversation

stephenchengCloud commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minglumlu Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenchengCloud Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lindig Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psafont left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minglumlu Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenchengCloud commented Nov 25, 2025

Uh oh!

stephenchengCloud commented Nov 25, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

stephenchengCloud commented Nov 25, 2025 •

edited

Loading

minglumlu Nov 25, 2025 •

edited

Loading

stephenchengCloud Nov 25, 2025 •

edited

Loading

lindig Nov 25, 2025 •

edited

Loading

minglumlu Nov 25, 2025 •

edited

Loading