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

The date header breaks the server if used with locale #2215

Open
easymoney322 opened this issue Nov 27, 2024 · 4 comments · May be fixed by #2217
Open

The date header breaks the server if used with locale #2215

easymoney322 opened this issue Nov 27, 2024 · 4 comments · May be fixed by #2217

Comments

@easymoney322
Copy link

Describe the bug

  1. Date header doesn't seem to be in the right format, when the locale is specified somewhere in the program, causing clients not being able to get data.

  2. Both known headers and the headers added with ->addHeader() are all lowercase.

I've a very simple page in Drogon, this is the code

std::setlocale(LC_ALL, "ru-RU");
  #ifdef _WIN32
    std::locale::global(std::locale(std::locale::empty(), new std::codecvt_utf8<wchar_t>));
  #endif _WIN32

trantor::Logger::setLogLevel(trantor::Logger::kTrace);
drogon::app()
    .setLogPath(ServerLogsDir)
    .setLogLevel(trantor::Logger::kTrace)
    .enableDateHeader(true)
    .enableServerHeader(false)
    .enableSendfile(false)
    .addListener(DrogonListenAddress, DrogonPort)
    .setThreadNum(1)
    .registerHandler("/hello", &nodeInfo, { drogon::Get } )
    .run();

and the respective handler:

void nodeInfo(const drogon::HttpRequestPtr& request, Callback&& callback)
{
  drogon::HttpResponsePtr response = drogon::HttpResponse::newHttpResponse(drogon::k200OK, drogon::ContentType::CT_TEXT_PLAIN);
  response->setVersion(drogon::Version::kHttp11);
  response->addHeader("ServerTest", "Drogon");
  response->setBody("Hello\r\n");
  callback(response);
}

My request in Postman:

GET http://127.0.0.1:1780/metrics
Request Headers
User-Agent: PostmanRuntime/7.37.3
Accept: */*
Host: 127.0.0.1:1780
Accept-Encoding: gzip, deflate, br
Connection: keep-alive

My result in Postman:

Response Headers
content-length: 7
content-type: text/plain; charset=utf-8
servertest: Drogon
date: Ñð, 27 íîÿ 2024 07:17:26 GMT⃠

Response Body
Hello↵↵

and when I try to open the page in browser I get "ERR_INVALID_HTTP_RESPONSE".

When I try to open it with CURL I get:

root@rtr-r00:~# curl http://192.168.100.73:1780/hello --output -
curl: (8) Nul byte in header

Commenting out setlocale instantly fixes the issue with the date on all clients, but I need this locale for other parts of the program.

To Reproduce
Steps to reproduce the behavior:

  1. Use the provided code
  2. Connect to the handled page using Postman, CURL and any browser
  3. See the results in Postman's console, Browser's network tab and CURL's stderr

Expected behavior
First of all, I'm expecting date field not to end up with null-byte and to have standard, non-localized RFC822-string format. In this case it should be Wed, 27 Nov 24 07:17:26 GMT. I believe null-byte is a result of a smaller size of a string, that was allocated using null-bytes.

Second, I'm expecting well known headers such as Content-Length to have the same case as stated in stated in the standard.

Third, I'm expecting drogon to respect explicit case in header keys such as "ServerTest", so it won't not to be resolved to the "servertest".

Desktop (please complete the following information):

  • OS: Windows 10 Pro 19045.4894
  • Browser: Chrome
  • Version: 130.0.6723.92
  • Drogon version: drogon[core,ctl]:[email protected] (VCPKG)
  • The OS locale is ru-RU

Additional context
I understand that all the headerkey checks (mostly) MUST be case-insensitive, and making it all lowercase is probably done for optimization reasons, but I've been working with HTTP for the last 3 years, and its the first web-server that is using lowercase headers. Most of documentation is also using Pascal-Train casing.

@rbugajewski
Copy link
Collaborator

Regarding header names:

  • HTTP/1.1 (RFC 2616): Header field names are case-insensitive, as per RFC 2616, Section 4.2. For example, Content-Type, content-type, and CONTENT-TYPE are treated equivalently. Headers are often sent in PascalCase over the wire, but this is not required.

  • HTTP/2 (RFC 7540): Header names must be sent in lowercase, as specified in RFC 7540, Section 8.1.2. Any deviation results in a protocol error.

This aligns with HTTP’s general case-insensitivity for header names but enforces a stricter format in HTTP/2 for optimization and consistency.

@rbugajewski rbugajewski changed the title The date header breaks the server if used with locale, drogon doesn't respect headers casing The date header breaks the server if used with locale Nov 27, 2024
@easymoney322
Copy link
Author

easymoney322 commented Nov 27, 2024

This is true that PascalCase isn't required for 1.1, and I can understand prefering to use lowercase for compatibility reasons with HTTP/2,
But Drogon doesn't even support HTTP/2 right now:

enum class Version
{
    kUnknown = 0,
    kHttp10,
    kHttp11
};

So why force lowercase for even explicitly set PascalCase headers?
Why go for lowercase when the majority of web-servers use PascalCase?
Most clients use PascalCase. Mozila's docs use PascalCase. Even the name of the sections in 1.1 standard uses PascalCase for header fields.

@easymoney322
Copy link
Author

easymoney322 commented Nov 28, 2024

So, the buffer for the date is initialized with nulls-bytes

char *getHttpFullDate(const trantor::Date &date)

After that, there is constantly defined date length

static const size_t httpFullDateStringLength = 29;

As well as some unsafe operations

memcpy((void *)&(*httpString_)[datePos_],

I will look further into it, but I can see two problems right now:
As time string size is dependent on locale (which is not known untill runtime), I think we should just run a simple test on initialization to check the size of date string, based on the first occuring null-byte.

The second problem is, that whatever the localized date string size is, allowing non-ASCII symbols is not a good idea.
While I wasn't able to find explicit restrictions on headers charset in HTTP/1.1 standard, there are such restrictions in HTTP/2. Using locales generates all kind of non-ascii characters, especially in Asian languages.

Solution: Don't use trantor::Date::toCustomFormattedString() for the purpose of generating header fields. Also add asserts in debug-builds for NON-ASCII characters and null-bytes in headers.
I've run all the code in Debug-environment and still wasn't able to pinpoint the problem for two hours or so. Adding asserts would pinpoint the problem straight away.

@easymoney322
Copy link
Author

It looks like std::strftime, which is used for date formating, is locale-dependent in all formats that don't use digits for numbering weekdays\months etc.

So my solution is to construct stream with set en_US locale and use it to construct the date string.

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 a pull request may close this issue.

2 participants