Make it more difficult to accidentally log sensitive headers (#6551)

square · Feb 8, 2021 · dcc6483 · dcc6483
1 parent b607bb0
commit dcc6483
Show file tree

Hide file tree

Showing 3 changed files with 99 additions and 26 deletions.
diff --git a/okhttp/src/main/kotlin/okhttp3/Headers.kt b/okhttp/src/main/kotlin/okhttp3/Headers.kt
@@ -28,6 +28,7 @@ import okhttp3.Headers.Builder
 import okhttp3.internal.format
 import okhttp3.internal.http.toHttpDateOrNull
 import okhttp3.internal.http.toHttpDateString
+import okhttp3.internal.isSensitiveHeader
 import org.codehaus.mojo.animal_sniffer.IgnoreJRERequirement
 
 /**
@@ -180,12 +181,27 @@ class Headers private constructor(
 
   override fun hashCode(): Int = namesAndValues.contentHashCode()
 
+  /**
+   * Returns header names and values. The names and values are separated by `: ` and each pair is
+   * followed by a newline character `\n`.
+   *
+   * Since OkHttp 5 this redacts these sensitive headers:
+   *
+   *  * `Authorization`
+   *  * `Cookie`
+   *  * `Proxy-Authorization`
+   *  * `Set-Cookie`
+   *
+   * To get all headers as a human-readable string use `toMultimap().toString()`.
+   */
   override fun toString(): String {
     return buildString {
       for (i in 0 until size) {
-        append(name(i))
+        val name = name(i)
+        val value = value(i)
+        append(name)
         append(": ")
-        append(value(i))
+        append(if (isSensitiveHeader(name)) "██" else value)
         append("\n")
       }
     }
@@ -370,7 +386,7 @@ class Headers private constructor(
       }
 
       // Check for malformed headers.
-      for (i in 0 until namesAndValues.size step 2) {
+      for (i in namesAndValues.indices step 2) {
         val name = namesAndValues[i]
         val value = namesAndValues[i + 1]
         checkName(name)
@@ -420,7 +436,7 @@ class Headers private constructor(
 
     private fun checkName(name: String) {
       require(name.isNotEmpty()) { "name is empty" }
-      for (i in 0 until name.length) {
+      for (i in name.indices) {
         val c = name[i]
         require(c in '\u0021'..'\u007e') {
           format("Unexpected char %#04x at %d in header name: %s", c.toInt(), i, name)
@@ -429,10 +445,11 @@ class Headers private constructor(
     }
 
     private fun checkValue(value: String, name: String) {
-      for (i in 0 until value.length) {
+      for (i in value.indices) {
         val c = value[i]
         require(c == '\t' || c in '\u0020'..'\u007e') {
-          format("Unexpected char %#04x at %d in %s value: %s", c.toInt(), i, name, value)
+          format("Unexpected char %#04x at %d in %s value", c.toInt(), i, name) +
+              (if (isSensitiveHeader(name)) "" else ": $value")
         }
       }
     }

diff --git a/okhttp/src/main/kotlin/okhttp3/internal/Util.kt b/okhttp/src/main/kotlin/okhttp3/internal/Util.kt
@@ -17,25 +17,6 @@
 
 package okhttp3.internal
 
-import okhttp3.EventListener
-import okhttp3.Headers
-import okhttp3.Headers.Companion.headersOf
-import okhttp3.HttpUrl
-import okhttp3.OkHttp
-import okhttp3.OkHttpClient
-import okhttp3.RequestBody.Companion.toRequestBody
-import okhttp3.Response
-import okhttp3.ResponseBody.Companion.toResponseBody
-import okhttp3.internal.http2.Header
-import okio.Buffer
-import okio.BufferedSink
-import okio.BufferedSource
-import okio.ByteString.Companion.decodeHex
-import okio.ExperimentalFileSystem
-import okio.FileSystem
-import okio.Options
-import okio.Path
-import okio.Source
 import java.io.Closeable
 import java.io.FileNotFoundException
 import java.io.IOException
@@ -57,6 +38,25 @@ import java.util.concurrent.ThreadFactory
 import java.util.concurrent.TimeUnit
 import kotlin.text.Charsets.UTF_32BE
 import kotlin.text.Charsets.UTF_32LE
+import okhttp3.EventListener
+import okhttp3.Headers
+import okhttp3.Headers.Companion.headersOf
+import okhttp3.HttpUrl
+import okhttp3.OkHttp
+import okhttp3.OkHttpClient
+import okhttp3.RequestBody.Companion.toRequestBody
+import okhttp3.Response
+import okhttp3.ResponseBody.Companion.toResponseBody
+import okhttp3.internal.http2.Header
+import okio.Buffer
+import okio.BufferedSink
+import okio.BufferedSource
+import okio.ByteString.Companion.decodeHex
+import okio.ExperimentalFileSystem
+import okio.FileSystem
+import okio.Options
+import okio.Path
+import okio.Source
 
 @JvmField
 val EMPTY_BYTE_ARRAY = ByteArray(0)
@@ -249,6 +249,14 @@ fun String.canParseAsIpAddress(): Boolean {
   return VERIFY_AS_IP_ADDRESS.matches(this)
 }
 
+/** Returns true if we should void putting this this header in an exception or toString(). */
+fun isSensitiveHeader(name: String): Boolean {
+  return name.equals("Authorization", ignoreCase = true) ||
+      name.equals("Cookie", ignoreCase = true) ||
+      name.equals("Proxy-Authorization", ignoreCase = true) ||
+      name.equals("Set-Cookie", ignoreCase = true)
+}
+
 /** Returns a [Locale.US] formatted [String]. */
 fun format(format: String, vararg args: Any): String {
   return String.format(Locale.US, format, *args)

diff --git a/okhttp/src/test/java/okhttp3/HeadersTest.java b/okhttp/src/test/java/okhttp3/HeadersTest.java
@@ -25,7 +25,6 @@
 import okhttp3.internal.http.HttpHeaders;
 import okhttp3.internal.http2.Header;
 import okhttp3.internal.http2.Http2ExchangeCodec;
-import org.junit.Ignore;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
@@ -381,6 +380,37 @@ public final class HeadersTest {
     }
   }
 
+  @Test public void sensitiveHeadersNotIncludedInExceptions() {
+    try {
+      new Headers.Builder().add("Authorization", "valué1");
+      fail("Should have complained about invalid name");
+    } catch (IllegalArgumentException expected) {
+      assertThat(expected.getMessage()).isEqualTo(
+          "Unexpected char 0xe9 at 4 in Authorization value");
+    }
+    try {
+      new Headers.Builder().add("Cookie", "valué1");
+      fail("Should have complained about invalid name");
+    } catch (IllegalArgumentException expected) {
+      assertThat(expected.getMessage()).isEqualTo(
+          "Unexpected char 0xe9 at 4 in Cookie value");
+    }
+    try {
+      new Headers.Builder().add("Proxy-Authorization", "valué1");
+      fail("Should have complained about invalid name");
+    } catch (IllegalArgumentException expected) {
+      assertThat(expected.getMessage()).isEqualTo(
+          "Unexpected char 0xe9 at 4 in Proxy-Authorization value");
+    }
+    try {
+      new Headers.Builder().add("Set-Cookie", "valué1");
+      fail("Should have complained about invalid name");
+    } catch (IllegalArgumentException expected) {
+      assertThat(expected.getMessage()).isEqualTo(
+          "Unexpected char 0xe9 at 4 in Set-Cookie value");
+    }
+  }
+
   @Test public void headersEquals() {
     Headers headers1 = new Headers.Builder()
         .add("Connection", "close")
@@ -415,6 +445,24 @@ public final class HeadersTest {
     assertThat(headers.toString()).isEqualTo("A: a\nB: bb\n");
   }
 
+  @Test public void headersToStringRedactsSensitiveHeaders() {
+    Headers headers = new Headers.Builder()
+        .add("content-length", "99")
+        .add("authorization", "peanutbutter")
+        .add("proxy-authorization", "chocolate")
+        .add("cookie", "drink=coffee")
+        .add("set-cookie", "accessory=sugar")
+        .add("user-agent", "OkHttp")
+        .build();
+    assertThat(headers.toString()).isEqualTo(""
+        + "content-length: 99\n"
+        + "authorization: ██\n"
+        + "proxy-authorization: ██\n"
+        + "cookie: ██\n"
+        + "set-cookie: ██\n"
+        + "user-agent: OkHttp\n");
+  }
+
   @Test public void headersAddAll() {
     Headers sourceHeaders = new Headers.Builder()
         .add("A", "aa")