GUACAMOLE-1174: Merge correct handling of truncated parameters when appending to URLs.

configure.ac

@@ -180,6 +180,10 @@ AC_SUBST([PULSE_INCLUDE], '-I$(top_srcdir)/src/pulse')
 AC_SUBST([COMMON_SSH_LTLIB],   '$(top_builddir)/src/common-ssh/libguac_common_ssh.la')
 AC_SUBST([COMMON_SSH_INCLUDE], '-I$(top_srcdir)/src/common-ssh')
 
+# Kubernetes support
+AC_SUBST([LIBGUAC_CLIENT_KUBERNETES_LTLIB],   '$(top_builddir)/src/protocols/kubernetes/libguac-client-kubernetes.la')
+AC_SUBST([LIBGUAC_CLIENT_KUBERNETES_INCLUDE], '-I$(top_srcdir)/src/protocols/kubernetes')
+
 # RDP support
 AC_SUBST([LIBGUAC_CLIENT_RDP_LTLIB],   '$(top_builddir)/src/protocols/rdp/libguac-client-rdp.la')
 AC_SUBST([LIBGUAC_CLIENT_RDP_INCLUDE], '-I$(top_srcdir)/src/protocols/rdp')
@@ -1191,6 +1195,7 @@ AC_CONFIG_FILES([Makefile
                  src/guaclog/man/guaclog.1
                  src/pulse/Makefile
                  src/protocols/kubernetes/Makefile
+                 src/protocols/kubernetes/tests/Makefile
                  src/protocols/rdp/Makefile
                  src/protocols/rdp/tests/Makefile
                  src/protocols/ssh/Makefile

src/protocols/kubernetes/.gitignore

@@ -0,0 +1,5 @@
+
+# Auto-generated test runner and binary
+_generated_runner.c
+test_kubernetes
+

src/protocols/kubernetes/Makefile.am

@@ -27,6 +27,7 @@ AUTOMAKE_OPTIONS = foreign
 ACLOCAL_AMFLAGS = -I m4
 
 lib_LTLIBRARIES = libguac-client-kubernetes.la
+SUBDIRS = . tests
 
 libguac_client_kubernetes_la_SOURCES = \
     argv.c                             \

src/protocols/kubernetes/tests/Makefile.am

@@ -0,0 +1,66 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+# NOTE: Parts of this file (Makefile.am) are automatically transcluded verbatim
+# into Makefile.in. Though the build system (GNU Autotools) automatically adds
+# its own license boilerplate to the generated Makefile.in, that boilerplate
+# does not apply to the transcluded portions of Makefile.am which are licensed
+# to you by the ASF under the Apache License, Version 2.0, as described above.
+#
+
+AUTOMAKE_OPTIONS = foreign
+ACLOCAL_AMFLAGS = -I m4
+
+#
+# Unit tests for Kubernetes support
+#
+
+check_PROGRAMS = test_kubernetes
+TESTS = $(check_PROGRAMS)
+
+test_kubernetes_SOURCES = \
+    url/append.c          \
+    url/escape.c
+
+test_kubernetes_CFLAGS =                \
+    -Werror -Wall -pedantic             \
+    @LIBGUAC_CLIENT_KUBERNETES_INCLUDE@ \
+    @LIBGUAC_INCLUDE@
+
+test_kubernetes_LDADD =               \
+    @CUNIT_LIBS@                      \
+    @LIBGUAC_CLIENT_KUBERNETES_LTLIB@
+
+#
+# Autogenerate test runner
+#
+
+GEN_RUNNER = $(top_srcdir)/util/generate-test-runner.pl
+CLEANFILES = _generated_runner.c
+
+_generated_runner.c: $(test_kubernetes_SOURCES)
+	$(AM_V_GEN) $(GEN_RUNNER) $(test_kubernetes_SOURCES) > $@
+
+nodist_test_kubernetes_SOURCES = \
+    _generated_runner.c
+
+# Use automake's TAP test driver for running any tests
+LOG_DRIVER =                \
+    env AM_TAP_AWK='$(AWK)' \
+    $(SHELL) $(top_srcdir)/build-aux/tap-driver.sh
+

src/protocols/kubernetes/tests/url/append.c

@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "url.h"
+
+#include <CUnit/CUnit.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+/**
+ * Verifies that guac_kubernetes_append_endpoint_param() correctly appends
+ * parameters to URLs that do not already have a query string.
+ */
+void test_url__append_no_query() {
+
+    char url[256] = "http://example.net";
+
+    CU_ASSERT(!guac_kubernetes_append_endpoint_param(url, sizeof(url), "foo", "100% test value"));
+    CU_ASSERT_STRING_EQUAL(url, "http://example.net?foo=100%25%20test%20value");
+
+}
+
+/**
+ * Verifies that guac_kubernetes_append_endpoint_param() correctly appends
+ * parameters to URLs that already have a query string.
+ */
+void test_url__append_existing_query() {
+
+    char url[256] = "http://example.net?foo=test%20value";
+
+    CU_ASSERT(!guac_kubernetes_append_endpoint_param(url, sizeof(url), "foo2", "yet&another/test\\value"));
+    CU_ASSERT_STRING_EQUAL(url, "http://example.net?foo=test%20value&foo2=yet%26another%2Ftest%5Cvalue");
+
+}
+
+/**
+ * Verifies that guac_kubernetes_append_endpoint_param() refuses to overflow
+ * the bounds of the provided buffer.
+ */
+void test_url__append_bounds() {
+
+    char url[256];
+
+    /* Appending "?a=1" to the 18-character string "http://example.net" should
+     * fail for all buffer sizes with 22 bytes or less, with a 22-byte buffer
+     * lacking space for the null terminator */
+    for (int length = 18; length <= 22; length++) {
+        strcpy(url, "http://example.net");
+        printf("Testing buffer with length %i ...\n", length);
+        CU_ASSERT(guac_kubernetes_append_endpoint_param(url, length, "a", "1"));
+    }
+
+    /* A 23-byte buffer should be sufficient */
+    strcpy(url, "http://example.net");
+    CU_ASSERT(!guac_kubernetes_append_endpoint_param(url, 23, "a", "1"));
+
+}
+

src/protocols/kubernetes/tests/url/escape.c

@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "url.h"
+
+#include <CUnit/CUnit.h>
+#include <stdlib.h>
+
+/**
+ * Verifies that guac_kubernetes_escape_url_component() correctly escapes
+ * characters that would otherwise have special meaning within URLs.
+ */
+void test_url__escape_special() {
+
+    char value[256];
+
+    CU_ASSERT(!guac_kubernetes_escape_url_component(value, sizeof(value), "?foo%20bar\\1/2&3=4"));
+    CU_ASSERT_STRING_EQUAL(value, "%3Ffoo%2520bar%5C1%2F2%263%3D4");
+
+}
+
+/**
+ * Verifies that guac_kubernetes_escape_url_component() leaves strings
+ * untouched if they contain no characters requiring escaping.
+ */
+void test_url__escape_nospecial() {
+
+    char value[256];
+
+    CU_ASSERT(!guac_kubernetes_escape_url_component(value, sizeof(value), "potato"));
+    CU_ASSERT_STRING_EQUAL(value, "potato");
+
+}
+
+/**
+ * Verifies that guac_kubernetes_escape_url_component() refuses to overflow the
+ * bounds of the provided buffer.
+ */
+void test_url__escape_bounds() {
+
+    char value[256];
+
+    /* Escaping "?potato" (or "potato?") should fail for all buffer sizes with
+     * 9 bytes or less, with a 9-byte buffer lacking space for the null
+     * terminator */
+    for (int length = 0; length <= 9; length++) {
+        printf("Testing buffer with length %i ...\n", length);
+        CU_ASSERT(guac_kubernetes_escape_url_component(value, length, "?potato"));
+        CU_ASSERT(guac_kubernetes_escape_url_component(value, length, "potato?"));
+    }
+
+    /* A 10-byte buffer should be sufficient */
+    CU_ASSERT(!guac_kubernetes_escape_url_component(value, 10, "?potato"));
+
+}
+

src/protocols/kubernetes/url.c

@@ -123,10 +123,14 @@ int guac_kubernetes_append_endpoint_param(char* buffer, int length,
     char delimiter = '?';
     if (qmark) delimiter = '&';
 
-    /* Write the parameter to the buffer */
-    int written;
-    written = snprintf(buffer + str_len, length - str_len,
-            "%c%s=%s", delimiter, param_name, escaped_param_value);
+    /* Advance to end of buffer, where the new parameter and delimiter need to
+     * be appended */
+    buffer += str_len;
+    length -= str_len;
+
+    /* Write the parameter and delimiter to the buffer */
+    int written = snprintf(buffer, length, "%c%s=%s", delimiter,
+            param_name, escaped_param_value);
 
     /* The parameter was successfully added if it was written to the given
      * buffer without truncation */

src/protocols/kubernetes/url.h

@@ -50,8 +50,11 @@ int guac_kubernetes_escape_url_component(char* output, int length,
         const char* str);
 
 /**
- * Append the parameter to the endpoint path.
- * Value within the path will be URL-escaped as necessary.
+ * Appends the given query parameter and value to the given buffer. If the
+ * buffer does not already contain the '?' character denoting the start of the
+ * query string, it will be added. If the buffer already contains a query
+ * string, a '&' character will be added before the new parameter. The
+ * parameter value will automatically be URL-escaped as necessary.
  *
  * @param buffer
  *     The buffer which should receive the parameter. It could contain the endpoint path.
@@ -61,7 +64,8 @@ int guac_kubernetes_escape_url_component(char* output, int length,
  *     The number of bytes available in the given buffer.
  *
  * @param param_name
- *     The name of the parameter.
+ *     The name of the parameter. If the parameter name contains characters
+ *     with special meaning to URLs, it must already be URL-escaped.
  *
  * @param param_value
  *     The value of the parameter.