mike-jumper commented on a change in pull request #271:
URL: https://github.com/apache/guacamole-server/pull/271#discussion_r575726813
##########
File path: src/guacd/connection.c
##########
@@ -278,10 +278,18 @@ static int guacd_route_connection(guacd_proc_map* map,
guac_socket* socket) {
proc = guacd_proc_map_retrieve(map, identifier);
new_process = 0;
- /* Warn if requested connection does not exist */
- if (proc == NULL)
- guacd_log(GUAC_LOG_INFO, "Connection \"%s\" does not exist.",
- identifier);
+ /* Warn and ward off client if requested connection does not exist */
+ if (proc == NULL) {
+ char message[2048];
+
+ snprintf(message, sizeof(message),
+ "Connection \"%s\" does not exist", identifier);
+
+ guacd_log(GUAC_LOG_INFO, message);
Review comment:
`guacd_log()` will interpret `message` here as a printf-style format
string:
https://github.com/apache/guacamole-server/blob/ca1fbd5e980392dc91b52ddff473698035d8f357/src/guacd/log.h#L45-L49
To do this safely, the call would need to be:
```c
guacd_log(GUAC_LOG_INFO, "%s", message);
```
However, given that the connection is failing, I think `GUAC_LOG_ERROR`
would be more appropriate.
##########
File path: src/guacd/connection.c
##########
@@ -278,10 +278,18 @@ static int guacd_route_connection(guacd_proc_map* map,
guac_socket* socket) {
proc = guacd_proc_map_retrieve(map, identifier);
new_process = 0;
- /* Warn if requested connection does not exist */
- if (proc == NULL)
- guacd_log(GUAC_LOG_INFO, "Connection \"%s\" does not exist.",
- identifier);
+ /* Warn and ward off client if requested connection does not exist */
+ if (proc == NULL) {
+ char message[2048];
+
+ snprintf(message, sizeof(message),
+ "Connection \"%s\" does not exist", identifier);
+
+ guacd_log(GUAC_LOG_INFO, message);
+ guac_protocol_send_error(socket, message,
+ GUAC_PROTOCOL_STATUS_CLIENT_BAD_REQUEST);
Review comment:
I think `GUAC_PROTOCOL_STATUS_RESOURCE_NOT_FOUND` would be a better
choice.
##########
File path: src/guacd/connection.c
##########
@@ -278,10 +278,18 @@ static int guacd_route_connection(guacd_proc_map* map,
guac_socket* socket) {
proc = guacd_proc_map_retrieve(map, identifier);
new_process = 0;
- /* Warn if requested connection does not exist */
- if (proc == NULL)
- guacd_log(GUAC_LOG_INFO, "Connection \"%s\" does not exist.",
- identifier);
+ /* Warn and ward off client if requested connection does not exist */
Review comment:
What about the case where a particular protocol is requested (for a new
connection) and that protocol is not defined?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
