url: fix encoding for use in http context

9 files changed, 166 insertions, 26 deletions

diff --git a/browse.c b/browse.c
index b233c8b..801b539 100644
--- a/browse.c
+++ b/browse.c
@@ -114,8 +114,8 @@ build_file_list(struct kreq * r, const struct file * request_dir)
 		    || strcmp("..", dir->d_name) == 0)
 			continue;
 		/* build file path */
-		file_path_len = url_build(file_path, PATH_MAX, r->path,
-					  dir->d_name, NULL);
+		file_path_len = path_build(file_path, PATH_MAX, r->path,
+					   dir->d_name, NULL);
 		if (file_path_len == 0 || file_path_len >= PATH_MAX) {
 			kutil_warn(r, NULL,
 				 "browse: unable to build file, skipping %s",
diff --git a/download.c b/download.c
index c18dcc1..008de02 100644
--- a/download.c
+++ b/download.c
@@ -33,6 +33,7 @@
 
 #include <fcntl.h>
 #include <limits.h>
+#include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
@@ -44,7 +45,6 @@
 #include "http.h"
 #include "str.h"
 #include "url.h"
-#include "util.h"
 
 /*
  * download url = r->pname / DOWNLOAD_URL / file->path
diff --git a/file.c b/file.c
index 24fb557..cd76def 100644
--- a/file.c
+++ b/file.c
@@ -31,6 +31,7 @@
 #include <sys/stat.h>
 
 #include <limits.h>
+#include <stdarg.h>
 #include <stdlib.h>
 #include <string.h>
 
@@ -39,7 +40,6 @@
 #include "mime.h"
 #include "str.h"
 #include "url.h"
-#include "util.h"
 
 void
 file_free(struct file * file)
@@ -74,7 +74,7 @@ fill_metadata(struct file * f)
 	f->mime = MIME_BIN;
 
 	/* check if it is a directory */
-	path_len = url_build(path, PATH_MAX, data_dir, f->path, NULL);
+	path_len = path_build(path, PATH_MAX, data_dir, f->path, NULL);
 	if (path_len == 0 || path_len >= PATH_MAX)
 		return false;
 
@@ -156,10 +156,41 @@ file_get_data_path(const struct file * f, char *data_path, size_t data_path_len,
 		return 0;
 
 	/* check if it is a directory */
-	path_len = url_build(data_path, data_path_len, data_dir, f->path,
-			     append, NULL);
+	path_len = path_build(data_path, data_path_len, data_dir, f->path,
+			      append, NULL);
 	if (path_len == 0 || path_len >= PATH_MAX)
 		return 0;
 
 	return path_len;
 }
+
+size_t
+path_build(char *dst, size_t dst_size,...)
+{
+	va_list path_list;
+	const char *path;
+	size_t 	w_size;
+
+	dst[0] = '\0';
+	w_size = 0;
+	va_start(path_list, dst_size);
+	while ((path = va_arg(path_list, char *)) != NULL) {
+		/* no more space in dst buffer */
+		if (w_size >= dst_size)
+			break;
+
+		if (path[0] == '\0')
+			continue;
+
+		if (path[0] == '.' && path[1] == '\0')
+			continue;
+
+		if (path[0] != '/' && w_size > 0 && dst[w_size - 1] != '/')
+			strlcat(dst, "/", dst_size);
+
+		w_size = strlcat(dst, path, dst_size);
+	}
+
+	va_end(path_list);
+	return w_size;
+}
diff --git a/file.h b/file.h
index f6aac76..1ede156 100644
--- a/file.h
+++ b/file.h
@@ -76,4 +76,18 @@ file_get_basename(const struct file *);
 size_t
 file_get_data_path(const struct file *, char *, size_t, char *);
 
+/*
+ * Build a path from the provided components.
+ * The first argument is the destination buffer where the path will be written.
+ * It is required.
+ * The second argument is the size of the destination buffer. It is required.
+ * The remaining arguments are the path parts which should end with a NULL
+ * component.
+ * Returns the number of characters written to the destination buffer. If it
+ * is equal to the destination buffer size, it means the buffer was too small
+ * and the resulting path has been truncated.
+ */
+size_t
+path_build(char *, size_t,...);
+
 #endif				/* FILE_H */
diff --git a/http.c b/http.c
index 5bad1d8..4dbeff2 100644
--- a/http.c
+++ b/http.c
@@ -34,7 +34,7 @@
 
 #include "http.h"
 #include "mime.h"
-#include "str.h"
+#include "url.h"
 
 bool
 http_open_file(struct kreq * r, enum khttp code, const struct file * f)
@@ -49,13 +49,9 @@ http_open_file(struct kreq * r, enum khttp code, const struct file * f)
 		return false;
 
 	/* file name needs to be url encoded for special chars */
-	filename = khttp_urlencode(filename);
-	if (filename == NULL)
+	if ((filename = url_encode(filename)) == NULL)
 		return false;
 
-	/* but for some reason, spaces should remain spaces... */
-	str_replace(filename, '+', ' ');
-
 	khttp_head(r, kresps[KRESP_STATUS], "%s", khttps[code]);
 	khttp_head(r, kresps[KRESP_CONTENT_DISPOSITION],
 		   "attachment;filename=\"%s\"", filename);
diff --git a/test.c b/test.c
index b3cf87a..fe1703a 100644
--- a/test.c
+++ b/test.c
@@ -4,12 +4,15 @@
 #include "browse.h"
 #include "http.h"
 #include "upload.h"
+#include "url.h"
 
 /* minunit, see https://jera.com/techinfo/jtns/jtn002 */
 #define mu_assert(message, test) do { if (!(test)) return message; } while (0)
 #define mu_run_test(test) do { char* message = test(); tests_run++; \
 	if(message) return message; } while (0)
 
+#define BUF_SZ 1000
+
 int 	tests_run;
 
 static char *
@@ -88,10 +91,41 @@ test_upload_post()
 }
 
 static char *
+test_url_build()
+{
+	char 	dst[BUF_SZ], *expected;
+	size_t 	dst_len;
+
+	expected = "/vault/browse";
+	dst_len = url_build(dst, BUF_SZ, "/vault/browse", "", NULL);
+	mu_assert("test_url_build failed", dst_len > 0 && dst_len < BUF_SZ);
+	mu_assert("test_url_build failed", strncmp(expected, dst, BUF_SZ) == 0);
+
+	expected = "/vault/browse/hello/world";
+	dst_len = url_build(dst, BUF_SZ, "/vault/browse", "hello", "world",
+			    NULL);
+	mu_assert("test_url_build failed", dst_len > 0 && dst_len < BUF_SZ);
+	mu_assert("test_url_build failed", strncmp(expected, dst, BUF_SZ) == 0);
+
+	expected = "/vault/browse";
+	dst_len = url_build(dst, BUF_SZ, "/vault/", "browse/", NULL);
+	mu_assert("test_url_build failed", dst_len > 0 && dst_len < BUF_SZ);
+	mu_assert("test_url_build failed", strncmp(expected, dst, BUF_SZ) == 0);
+
+	dst_len = url_build(dst, BUF_SZ, "", "", NULL);
+	mu_assert("test_url_build failed", dst_len == 0);
+
+	dst_len = url_build(dst, BUF_SZ, NULL);
+	mu_assert("test_url_build failed", dst_len == 0);
+	return 0;
+}
+
+static char *
 all_tests()
 {
 	mu_run_test(test_browse_invalid_traversal);
 	mu_run_test(test_browse_path_too_long);
+	mu_run_test(test_url_build);
 	mu_run_test(test_upload_post);
 	return 0;
 }
diff --git a/upload.c b/upload.c
index 765e694..ed9ec4d 100644
--- a/upload.c
+++ b/upload.c
@@ -32,6 +32,7 @@
 #include <kcgi.h>
 #include <kcgihtml.h>
 #include <limits.h>
+#include <stdio.h>
 #include <string.h>
 
 #include "browse.h"
diff --git a/url.c b/url.c
index adcc95d..977edea 100644
--- a/url.c
+++ b/url.c
@@ -28,13 +28,18 @@
  * POSSIBILITY OF SUCH DAMAGE.
  */
 
+#include <sys/types.h>
+
 #include <assert.h>
 #include <limits.h>
 #include <stdarg.h>
+#include <stdint.h>
 #include <stdlib.h>
+#include <kcgi.h>
 #include <string.h>
 
 #include "config.h"
+#include "str.h"
 #include "url.h"
 
 bool
@@ -74,20 +79,35 @@ check_request_path(const char *path, const char *suffix)
 	return true;
 }
 
+char   *
+url_encode(const char *str)
+{
+	char   *result;
+
+	if ((result = khttp_urlencode(str)) == NULL)
+		return NULL;
+
+	str_replace(result, '+', ' ');
+	return result;
+}
+
 size_t
-url_build(char *dst, size_t dst_size,...)
+url_build(char *dst, size_t dst_sz,...)
 {
 	va_list path_list;
-	const char *path;
-	size_t 	w_size;
+	char 	p [PATH_MAX];
+	char   *encoded, *path, *slash;
+	size_t 	c_len, p_len, w_len;
 
 	dst[0] = '\0';
-	w_size = 0;
-	va_start(path_list, dst_size);
+	w_len = 0;
+	va_start(path_list, dst_sz);
 	while ((path = va_arg(path_list, char *)) != NULL) {
 		/* no more space in dst buffer */
-		if (w_size >= dst_size)
-			break;
+		if (w_len >= dst_sz) {
+			w_len = 0;
+			goto end;
+		}
 
 		if (path[0] == '\0')
 			continue;
@@ -95,12 +115,50 @@ url_build(char *dst, size_t dst_size,...)
 		if (path[0] == '.' && path[1] == '\0')
 			continue;
 
-		if (path[0] != '/' && w_size > 0 && dst[w_size - 1] != '/')
-			strlcat(dst, "/", dst_size);
+		while (path[0] != '\0') {
+			/* no more space in dst buffer */
+			if (w_len >= dst_sz) {
+				w_len = 0;
+				goto end;
+			}
+
+			/* skip heading slashes */
+			slash = strchr(path, '/');
+			if (slash == path) {
+				path++;
+				continue;
+			}
+
+			/* copy string to encode into buffer
+			 * if no slash found, copy everything, otherwise stop
+			 * before the slash */
+			c_len = slash == NULL ? strlen(path) :
+			    (size_t) (slash - path);
+			if (c_len >= PATH_MAX) {
+				w_len = 0;
+				goto end;
+			}
+			p_len = strlcpy(p, path, c_len + 1);
+			if (p_len < c_len) {
+				w_len = 0;
+				goto end;
+			}
+
+			/* encode string */
+			if ((encoded = url_encode(p)) == NULL) {
+				w_len = 0;
+				goto end;
+			}
 
-		w_size = strlcat(dst, path, dst_size);
+			/* append it to the path */
+			strlcat(dst, "/", dst_sz);
+			w_len = strlcat(dst, encoded, dst_sz);
+			free(encoded);
+			path += c_len;
+		}
 	}
 
+end:
 	va_end(path_list);
-	return w_size;
+	return w_len;
 }
diff --git a/url.h b/url.h
index e91925b..d1d638b 100644
--- a/url.h
+++ b/url.h
@@ -32,7 +32,6 @@
 #define URL_H
 
 #include <stdbool.h>
-#include <stdio.h>
 
 /*
  * Checks that the path can be safely processed. Namely, it should not contain
@@ -42,13 +41,20 @@
  */
 bool
 check_request_path(const char *, const char *);
+/*
+ * Encode a string to use in an HTTP context.
+ * Returns NULL in case of error, or the encoded string in case of success.
+ */
+char *
+url_encode(const char *);
 
 /*
  * Build an URL from the provided components.
  * The first argument is the destination buffer where the URL will be written.
  * It is required.
  * The second argument is the size of the destination buffer. It is required.
- * The remaining arguments are the url parts
+ * The remaining arguments are the url parts which should end with a NULL
+ * component.
  * Returns the number of characters written to the destination buffer. If it
  * is equal to the destination buffer size, it means the buffer was too small
  * and the resulting URL has been truncated.