Skip to content
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.

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

GUACAMOLE-1921: Process argv handler even when the connection is read-only. #490

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

necouchman
Copy link
Contributor

This simple change processes the argv handler for the VNC protocol even when the connection is read-only, which is required to prompt for credentials for read-only connections. As these are the only parameters that are processed by the argv handler, there is no additional impact on the code.

It's worth noting that, if other parameters are added in the future, the argv handler may need to check the read_only parameter and make decisions about which parameters should be allowed to be processed and which should be ignored or blocked.

Copy link
Contributor

@mike-jumper mike-jumper left a comment

Choose a reason for hiding this comment

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

To avoid any future issues, I think we should add some comments here noting:

  • That read-only may need to be considered for handling of future argument values received via argv that are not related to authentication.
  • That changing things such that argv is handled for users other than the owner may have security implications unless the handling of argv is modified to only accept username/password from the owner.

Otherwise, LGTM!

@necouchman
Copy link
Contributor Author

@mike-jumper I have added some comments - let me know if the style of those is okay, as they're a bit of a one-off in terms of the types of comments used throughout the code. In addition to the comment in user.c, I added a note to the argv.h header file where the user callback is documented.

Copy link
Contributor

@mike-jumper mike-jumper left a comment

Choose a reason for hiding this comment

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

Very nice.

@mike-jumper mike-jumper merged commit 9164a79 into apache:staging/1.5.5 Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants