Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Removing the limitation of 256KB for header size #8504

Closed
wants to merge 4 commits into from
Closed

Removing the limitation of 256KB for header size #8504

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5b672b
Removing the limitation of 256KB for header size
gahfy Aug 10, 2024
a493ebc
Remove magic number -1L in header limit, and replace by HEADER_NO_LIM…
gahfy Aug 10, 2024
0c32f7c
Fixing spelling error in error message for a wrong `headerSizeLimit`
gahfy Aug 10, 2024
286e151
Bump OkHTTP api
gahfy Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions okhttp/src/main/kotlin/okhttp3/OkHttpClient.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import okhttp3.internal.checkDuration
import okhttp3.internal.concurrent.TaskRunner
import okhttp3.internal.connection.RealCall
import okhttp3.internal.connection.RouteDatabase
import okhttp3.internal.http1.HeadersReader
import okhttp3.internal.immutableListOf
import okhttp3.internal.platform.Platform
import okhttp3.internal.proxy.NullProxySelector
Expand Down Expand Up @@ -264,6 +265,7 @@ open class OkHttpClient internal constructor(

internal val routeDatabase: RouteDatabase = builder.routeDatabase ?: RouteDatabase()
internal val taskRunner: TaskRunner = builder.taskRunner ?: TaskRunner.INSTANCE
internal val headerSizeLimit: Long = builder.headerSizeLimit

@get:JvmName("connectionPool")
val connectionPool: ConnectionPool =
Expand Down Expand Up @@ -626,6 +628,7 @@ open class OkHttpClient internal constructor(
internal var minWebSocketMessageToCompress = RealWebSocket.DEFAULT_MINIMUM_DEFLATE_SIZE
internal var routeDatabase: RouteDatabase? = null
internal var taskRunner: TaskRunner? = null
internal var headerSizeLimit: Long = HeadersReader.HEADER_LIMIT

internal constructor(okHttpClient: OkHttpClient) : this() {
this.dispatcher = okHttpClient.dispatcher
Expand Down Expand Up @@ -661,6 +664,7 @@ open class OkHttpClient internal constructor(
this.minWebSocketMessageToCompress = okHttpClient.minWebSocketMessageToCompress
this.routeDatabase = okHttpClient.routeDatabase
this.taskRunner = okHttpClient.taskRunner
this.headerSizeLimit = okHttpClient.headerSizeLimit
}

/**
Expand Down Expand Up @@ -1387,6 +1391,22 @@ open class OkHttpClient internal constructor(
this.minWebSocketMessageToCompress = bytes
}

/**
* Sets the maximum length (in characters, excluding line breaks) of the header.
*
* Set to [HeadersReader.HEADER_NO_LIMIT] to allow any size fot the header.
*
* [HeadersReader.HEADER_LIMIT] by default.
*/
fun headerSizeLimit(characters: Long) =
apply {
require(characters >= -1) {
"headerSizeLimit must me either positive or equals to -1: $characters"
yschimke marked this conversation as resolved.
Show resolved Hide resolved
}

this.headerSizeLimit = characters
}

fun build(): OkHttpClient = OkHttpClient(this)
}

Expand Down
24 changes: 18 additions & 6 deletions okhttp/src/main/kotlin/okhttp3/internal/http1/HeadersReader.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,26 @@ import okhttp3.Headers
import okio.BufferedSource

/**
* Parse all headers delimited by "\r\n" until an empty line. This throws if headers exceed 256 KiB.
* Parse all headers delimited by "\r\n" until an empty line. This throws if headers exceed `headerLimit`.
*/
class HeadersReader(val source: BufferedSource) {
private var headerLimit = HEADER_LIMIT.toLong()
class HeadersReader(
val source: BufferedSource,
private var headerLimit: Long = HEADER_LIMIT
) {

/** Read a single line counted against the header size limit. */
fun readLine(): String {
val line = source.readUtf8LineStrict(headerLimit)
headerLimit -= line.length.toLong()
val line = if(headerLimit == HEADER_NO_LIMIT)
source.readUtf8LineStrict()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given we now have two codepaths here, and it was already confusing. Should we catch and re throw in all cases with context such as remaining header length/"NO_LIMIT"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure to understant what you mean here ? If there is no limit and we do source.readUtf8LineStrict(headerLimit), it will throw an exception, and the next source.readUtf8LineStrict() will be run on the next line if I understant it well.
So how would you perform the completion of the line in the catch clause ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably worth catching and rethrowing and explaining issue so it's clear why

  • newline not found within limit
  • newline now found by end of stream

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure to really understand what you mean. Can you write 1 or 2 line of pseudo code, to make sure I understand it well (no need to make something correct, it does not need to compile either, it's just in order to get an idea of what you mean)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mainly concerned about the confusion with this limit and the limit used by Okio.

https://github.com/square/okio/blob/597a65527c987d9f221dd5607c49e49f934df2de/okio/src/jvmTest/kotlin/okio/ReadUtf8LineTest.kt#L66

  throw EOFException(
    "\\n not found: limit=" + minOf(buffer.size, limit) +
      " content=" + data.readByteString().hex() + '…'.toString(),
  )

So if I've set the header length limit to 1024, but I might get 0, 10, or 1024 as the limit in the exception message. I'd be confused if I saw a different limit here.

Maybe something like

    val line = try {
      if (headerLimit == HEADER_NO_LIMIT) source.readUtf8LineStrict()
      else source.readUtf8LineStrict(headerLimit)
    } catch (eofe: EOFException) {
      throw EOFException(buildString {
        append("Could not read a complete header")
        if (headerLimit != HEADER_NO_LIMIT) {
          append(" within $headerLimit bytes")
        }
      }).apply {
        initCause(eofe)
      }
    }
``

But I'm open to suggestions.

else
source.readUtf8LineStrict(headerLimit)
if(headerLimit != HEADER_NO_LIMIT) {
if(line.length.toLong() > headerLimit) {
headerLimit = HEADER_NO_LIMIT - 1L
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why -2 instead of zero? Just avoiding -1?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and I'm not sure about the behavior then of source.readUtf8LineStrict(headerLimit) with a negative number in the case of an empty line, which wouldn't break the rule limit. So putting it strictly negative will handle that scenario.

} else {
headerLimit -= line.length.toLong()
}
}
return line
}

Expand All @@ -43,6 +54,7 @@ class HeadersReader(val source: BufferedSource) {
}

companion object {
private const val HEADER_LIMIT = 256 * 1024
const val HEADER_LIMIT: Long = 256L * 1024L
const val HEADER_NO_LIMIT: Long = -1L
}
}
5 changes: 4 additions & 1 deletion okhttp/src/main/kotlin/okhttp3/internal/http1/Http1ExchangeCodec.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,10 @@ class Http1ExchangeCodec(
private val sink: BufferedSink,
) : ExchangeCodec {
private var state = STATE_IDLE
private val headersReader = HeadersReader(source)
private val headersReader = HeadersReader(
source = source,
headerLimit = client?.headerSizeLimit?:HeadersReader.HEADER_LIMIT
)

private val Response.isChunked: Boolean
get() = "chunked".equals(header("Transfer-Encoding"), ignoreCase = true)
Expand Down
Loading