Skip to content

CP-54274: Add user-specific error message for vnc connection limits #6710

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: feature/limit-vnc-console-sessions
Choose a base branch
from
Open

CP-54274: Add user-specific error message for vnc connection limits #6710

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
365f5d0
CP-54274: Add user-specific error message for vnc connection limits
stephenchengCloud Aug 4, 2025
bd6e392
fix
Oct 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ocaml/libs/http-lib/http_svr.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,10 @@ let response_forbidden ?req s =
in
response_error_html ?version s "403" "Forbidden" [] body

let response_forbidden_with_body ?req s body =
let version = Option.map get_return_version req in
response_error_html ?version s "403" "Forbidden" [] body
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want 403, not 503 as before?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a client issue, not a server issue, so a 4xx response is warranted, maybe a 429 (too many requests) would be a better fit here?

https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/429

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we expect more specific error responses - does it make sense to factor this into a function that takes the error code and message as arguments?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is more like a service side issue. It's server that is applying the restriction.
How about 503 + a specific error message?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both 429 and 503 are reasonable to me.
Yeah, it's more of a serser side issue, because the server actively reject the connection.
I'll change this to 503.

Copy link
Collaborator Author

@stephenchengCloud stephenchengCloud Oct 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we expect more specific error responses - does it make sense to factor this into a function that takes the error code and message as arguments?

Yeah, the specific error responses are contained in the body. See https://github.com/xapi-project/xen-api/pull/6710/files#diff-b467704b510207bfdab55411ab1ff91aae3f19c3e0ab0409f142fd1646595a3cR236
But yes, we can make this fucion more generic taking error code and messages as arguments. Will modify it.


let response_badrequest ?req s =
let version = Option.map get_return_version req in
let body =
Expand Down
3 changes: 3 additions & 0 deletions ocaml/libs/http-lib/http_svr.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@ val response_unauthorised :

val response_forbidden : ?req:Http.Request.t -> Unix.file_descr -> unit

val response_forbidden_with_body :
?req:Http.Request.t -> Unix.file_descr -> string -> unit

val response_badrequest : ?req:Http.Request.t -> Unix.file_descr -> unit

val response_internal_error :
Expand Down
16 changes: 14 additions & 2 deletions ocaml/xapi/console.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,19 @@ let real_proxy' ~__context ~vm vnc_port s =
debug "Proxy exited"
with exn -> debug "error: %s" (ExnHelper.string_of_exn exn)

let real_proxy __context vm _ _ vnc_port s =
let respond_console_limit_exceeded ~__context req s =
let session_id = Xapi_http.get_session_id req in
let user = Db.Session.get_auth_user_name ~__context ~self:session_id in
let body =
Printf.sprintf
"<html><body><h1>Connection Limit Exceeded</h1><p>User '%s': Only 1 \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this useful? The caller would know itself right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could create invalid HTML if user has characters with special meaning in HTML that would require escaping.

!https://miro.medium.com/v2/resize:fit:720/format:webp/1*P4nj9fJjSeJ9-c0rwSZqlg.png

Copy link
Collaborator Author

@stephenchengCloud stephenchengCloud Oct 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this useful? The caller would know itself right?

Indeed it's meaningless. Instead, we can send in the response the user name who is using the connection.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that may be a security issue...

connection is allowed at a time. Please try again \
later.</p></body></html>"
user
in
Http_svr.response_forbidden_with_body ~req s body

let real_proxy __context vm req _ vnc_port s =
let vm_id = Ref.string_of vm in
let pool = Helpers.get_pool ~__context in
let is_limit_enabled =
Expand All @@ -241,7 +253,7 @@ let real_proxy __context vm _ _ vnc_port s =
(fun () -> real_proxy' ~__context ~vm vnc_port s)
(fun () -> Connection_limit.drop vm_id)
else
Http_svr.headers s (Http.http_503_service_unavailable ())
respond_console_limit_exceeded ~__context req s

let go_if_no_limit __context s f =
let pool = Helpers.get_pool ~__context in
Expand Down
Loading