Fix request body sanitizing

4 files changed, 35 insertions, 21 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/auditlog/AuditLog.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/auditlog/AuditLog.java
index 19fb1b7e1bb..6d011438b10 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/auditlog/AuditLog.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/auditlog/AuditLog.java
@@ -1,7 +1,6 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.auditlog;
 
-import com.google.common.base.CharMatcher;
 import com.google.common.collect.Ordering;
 
 import java.time.Instant;
@@ -10,7 +9,6 @@ import java.util.Comparator;
 import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
-import java.util.function.Predicate;
 
 /**
  * This represents the audit log of a hosted Vespa system. The audit log contains manual actions performed through
@@ -66,7 +64,7 @@ public class AuditLog {
         private final String resource;
         private final Optional<String> data;
 
-        public Entry(Instant at, String principal, Method method, String resource, Optional<String> data) {
+        public Entry(Instant at, String principal, Method method, String resource, byte[] data) {
             this.at = Objects.requireNonNull(at, "at must be non-null");
             this.principal = Objects.requireNonNull(principal, "principal must be non-null");
             this.method = Objects.requireNonNull(method, "method must be non-null");
@@ -112,16 +110,27 @@ public class AuditLog {
             DELETE
         }
 
-        private static Optional<String> sanitize(Optional<String> data) {
-            Objects.requireNonNull(data, "data must be non-null");
-            return data.filter(Predicate.not(String::isBlank))
-                       .filter(CharMatcher.ascii()::matchesAllOf)
-                       .map(v -> {
-                           if (v.length() > maxDataLength) {
-                               return v.substring(0, maxDataLength);
-                           }
-                           return v;
-                       });
+        private static Optional<String> sanitize(byte[] data) {
+            StringBuilder sb = new StringBuilder();
+            for (byte b : data) {
+                char c = (char) b;
+                if (!printableAscii(c) && !tabOrLineBreak(c)) {
+                    return Optional.empty();
+                }
+                sb.append(c);
+                if (sb.length() == maxDataLength) {
+                    break;
+                }
+            }
+            return Optional.of(sb.toString()).filter(s -> !s.isEmpty());
+        }
+
+        private static boolean printableAscii(char c) {
+            return c >= 32 && c <= 126;
+        }
+
+        private static boolean tabOrLineBreak(char c) {
+            return c == 9 || c == 10 || c == 13;
         }
 
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/auditlog/AuditLogger.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/auditlog/AuditLogger.java
index 34e7955e02a..b6782767386 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/auditlog/AuditLogger.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/auditlog/AuditLogger.java
@@ -3,14 +3,12 @@ package com.yahoo.vespa.hosted.controller.auditlog;
 
 import com.yahoo.container.jdisc.HttpRequest;
 import com.yahoo.transaction.Mutex;
-import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
 
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.UncheckedIOException;
 import java.net.URI;
-import java.nio.charset.StandardCharsets;
 import java.security.Principal;
 import java.time.Clock;
 import java.time.Duration;
@@ -70,7 +68,7 @@ public class AuditLogger {
 
         Instant now = clock.instant();
         AuditLog.Entry entry = new AuditLog.Entry(now, principal.getName(), method.get(), pathAndQueryOf(request.getUri()),
-                                                  Optional.of(new String(data, StandardCharsets.UTF_8)));
+                                                  data);
         try (Mutex lock = db.lockAuditLog()) {
             AuditLog auditLog = db.readAuditLog()
                                   .pruneBefore(now.minus(entryTtl))
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializer.java
index c94c04fc244..38b9d994a6d 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializer.java
@@ -8,6 +8,7 @@ import com.yahoo.slime.Slime;
 import com.yahoo.slime.SlimeUtils;
 import com.yahoo.vespa.hosted.controller.auditlog.AuditLog;
 
+import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -57,6 +58,8 @@ public class AuditLogSerializer {
                     methodFrom(entryObject.field(methodField)),
                     entryObject.field(resourceField).asString(),
                     SlimeUtils.optionalString(entryObject.field(dataField))
+                              .map(s -> s.getBytes(StandardCharsets.UTF_8))
+                              .orElseGet(() -> new byte[0])
             ));
         });
         return new AuditLog(entries);
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializerTest.java
index d9e2e61b868..c047f31e171 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializerTest.java
@@ -4,10 +4,10 @@ package com.yahoo.vespa.hosted.controller.persistence;
 import com.yahoo.vespa.hosted.controller.auditlog.AuditLog;
 import org.junit.Test;
 
+import java.nio.charset.StandardCharsets;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.List;
-import java.util.Optional;
 
 import static java.time.temporal.ChronoUnit.MILLIS;
 import static org.junit.Assert.assertEquals;
@@ -28,16 +28,19 @@ public class AuditLogSerializerTest {
         AuditLog log = new AuditLog(List.of(
                 new AuditLog.Entry(i1, "bar", AuditLog.Entry.Method.POST,
                                    "/bar/baz/",
-                                   Optional.of("0".repeat(2048))),
+                                   "0".repeat(2048).getBytes(StandardCharsets.UTF_8)),
                 new AuditLog.Entry(i2, "foo", AuditLog.Entry.Method.POST,
                                    "/foo/bar/",
-                                   Optional.of("{\"foo\":\"bar\"}")),
+                                   "{\"foo\":\"bar\"}".getBytes(StandardCharsets.UTF_8)),
                 new AuditLog.Entry(i3, "baz", AuditLog.Entry.Method.POST,
                                    "/foo/baz/",
-                                   Optional.of("")),
+                                   new byte[0]),
                 new AuditLog.Entry(i4, "baz", AuditLog.Entry.Method.POST,
                                    "/foo/baz/",
-                                   Optional.of("\ufdff\ufeff\uffff")) // non-ascii
+                                   "000\ufdff\ufeff\uffff000".getBytes(StandardCharsets.UTF_8)), // non-ascii
+                new AuditLog.Entry(i4, "quux", AuditLog.Entry.Method.POST,
+                                   "/foo/quux/",
+                                   new byte[]{(byte) 0xDE, (byte) 0xAD, (byte) 0xBE, (byte) 0xEF}) // garbage
         ));
 
         AuditLogSerializer serializer = new AuditLogSerializer();
@@ -58,6 +61,7 @@ public class AuditLogSerializerTest {
         assertEquals(1024, log.entries().get(0).data().get().length());
         assertTrue(log.entries().get(2).data().isEmpty());
         assertTrue(log.entries().get(3).data().isEmpty());
+        assertTrue(log.entries().get(4).data().isEmpty());
     }
 
 }