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.

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

Removing the limitation of 256KB for header size #8504

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

gahfy
Copy link

@gahfy gahfy commented Aug 10, 2024

This allows users to not be restricted to the limitation of 256KB for the size of the header, in order to solve #8472 :

How to reproduce

Python3 server code

import socket
import random
import string

def generate_random_string(length):
    return ''.join(random.choices(string.ascii_letters + string.digits, k=length))

random_string = ''.join(random.choices(string.ascii_letters + string.digits, k=256 * 1024 - 48))

# Création d'un socket TCP
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as server_socket:
    server_socket.bind(('127.0.0.1', 8080))
    server_socket.listen()

    while True:
        conn, addr = server_socket.accept()
        with conn:
            response = (
                "HTTP/1.1 200 OK\n"
                "Content-Type: text/plain\n"
                f"MyHeader:{random_string}\n"
                "Content-Length: 14\n"
                "\n"
                "Erreur de requête"
            )
            conn.sendall(response.encode('utf-8'))

Kotlin Client code after/before the PR (Comment/Uncomment lines 10/11)

import okhttp3.OkHttpClient;
import okhttp3.Request;
import okhttp3.Response;
import okhttp3.ResponseBody;
import okhttp3.internal.http1.HeadersReader;

public class OkHttpTest {
  private static final String ENDPOINT = "http://127.0.0.1:8080";

  public static void main(String... args) {
    OkHttpClient client = new OkHttpClient.Builder().build();
    //OkHttpClient client = new OkHttpClient.Builder().headerSizeLimit(HeadersReader.HEADER_NO_LIMIT).build();

    try {
      Request request = new Request.Builder()
        .url(ENDPOINT)
        .build();
      try (Response response = client.newCall(request).execute()) {
        ResponseBody body = response.body();
        System.out.println(body.string());
      }
    } catch(Exception e) {
      e.printStackTrace();
    }
  }
}

Before, you will get an EOFException, after, everything works fine

Removing the limitation of 256KB for header size
Copy link
Collaborator

@yschimke yschimke left a comment

Choose a reason for hiding this comment

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

Seems reasonable. Let me confirm with specs and also my past decisions.

okhttp/src/main/kotlin/okhttp3/OkHttpClient.kt Outdated Show resolved Hide resolved
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.

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)

@yschimke
Copy link
Collaborator

In #3613 we removed a system property lookup. Not sure why we didn't move to a config.

@yschimke
Copy link
Collaborator

My one concern about this is that the current default is way above internet infrastructure. So likely to break on proxies or other clients.

@gahfy
Copy link
Author

gahfy commented Aug 10, 2024

My one concern about this is that the current default is way above internet infrastructure. So likely to break on proxies or other clients.

I agree with that, that is why in the #8472 issue, I suggested also to not change anything, and just update the documentation. Btw, very long headers do not break any RFC 2616 rule. Maybe making sure that we can handle any response which does not break any rule would make the library also usable by people with that weird server response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants