Dump HTTP request headers #13992

promag · 2018-08-17T13:59:22Z

No description provided.

promag · 2018-08-17T13:59:53Z

src/httpserver.cpp

             RequestMethodString(hreq->GetRequestMethod()), hreq->GetURI(), hreq->GetPeer().ToString());

+    for (auto i : hreq->GetHeaders()) {
+        LogPrint(BCLog::HTTP, "  %s: %s\n", i.first, i.second);


Should redact authorization header?

promag · 2018-08-17T14:01:15Z

Output example when using bitcoin-cli:

2018-08-17T14:00:38Z Received a POST request for / from 127.0.0.1:61509
2018-08-17T14:00:38Z Request headers:
2018-08-17T14:00:38Z   Authorization: Basic X19jb29raWVfXzo3OGYxMzkzMzVmNmZkNjBhZjA5ZjY3MDcyNmM0ZDE0MDRiOGI2MTlmYjYwNDc3NjExYzZmYTQ1Y2E2ZjAxODZh
2018-08-17T14:00:38Z   Connection: close
2018-08-17T14:00:38Z   Content-Length: 37
2018-08-17T14:00:38Z   Host: 127.0.0.1

Output example when running the test framework:

2018-08-17T14:08:37.622063Z Received a POST request for / from 127.0.0.1:61759
2018-08-17T14:08:37.622063Z Request headers:
2018-08-17T14:08:37.623648Z   Accept-Encoding: identity
2018-08-17T14:08:37.623701Z   Authorization: Basic X19jb29raWVfXzoxMmQyMjMxOTY4YzM5NzRkN2ViMDI0ZjA5YzM1M2Y5ZjBkNDUxNjA1MjQ4NDY4NzY5YmJlOTlmMmYxNjY0NWY3
2018-08-17T14:08:37.623795Z   Content-Length: 68
2018-08-17T14:08:37.623844Z   Content-type: application/json
2018-08-17T14:08:37.623875Z   Host: 127.0.0.1
2018-08-17T14:08:37.623895Z   User-Agent: AuthServiceProxy/0.1

practicalswift · 2018-08-17T14:04:53Z

Concept ACK

Perhaps some sanitization or escaping is needed on the adversary controlled input before printing/logging to make sure we’re not opening up to log injection, etc?

promag · 2018-08-17T14:29:35Z

@practicalswift Added a commit that only dumps headers if the request is valid (authorized and method is known).

laanwj · 2018-08-17T14:32:58Z

src/httpserver.cpp

@@ -239,6 +239,12 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
        return;
    }

+    // Dump headers if request is valid
+    LogPrint(BCLog::HTTP, "Request headers:\n",


it should avoid doing this if !LogAcceptCategory(BCLog::HTTP), no need for the overhead to copy the headers when the result is ignored

laanwj · 2018-08-17T14:33:21Z

~~Concept ACK, this could be useful (though is also very verbose)~~

agree that string sanitizing is very important to prevent manipulation with escape codes etc.

EDIT dumping the Authorization header as-is is also not a good idea, it leaks the cleartext username/password !

promag · 2018-08-17T14:44:02Z

@laanwj right, I asked it above #13992 (comment).

laanwj · 2018-08-17T15:04:42Z

Thinking of it, I'm not sure this is worth the added code and complexity.
It's so easy to make tcpdump or wireshark dump this information if you really need it, say for debugging your HTTP implementation.

promag · 2018-08-17T15:28:30Z

Updated to redact authorization header (added EqualHeaders).

Agree with @laanwj and I'm going to close this unless there is a strong reason to keep it.

practicalswift · 2018-08-17T15:36:53Z

src/httpserver.cpp

@@ -646,6 +670,11 @@ HTTPRequest::RequestMethod HTTPRequest::GetRequestMethod()
    }
 }

+bool EqualHeaders(const std::string& h1, const std::string& h2)
+{
+    return evutil_ascii_strcasecmp(h1.c_str(), h2.c_str()) == 0;


Is there a reason for relying on libevent in this function? If not I suggest doing this without relying on third-party code by introducing the required locale independent string helper functions in-tree. That would perhaps also allow us to reduce the number of know locale dependence problems listed here: https://github.com/bitcoin/bitcoin/blob/master/test/lint/lint-locale-dependence.sh#L4

DrahtBot · 2018-08-17T20:43:54Z

Note to reviewers: This pull request conflicts with the following ones:

http: avoid fd exhaustion #12274 (http: avoid fd exhaustion by theuni)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

l2a5b1 · 2018-08-19T10:12:28Z

Hey @promag I was looking at this PR when you closed it.

I agree with @laanwj and you that a web debugging proxy is the right tool for inspecting HTTP requests.

If the PR was still open and you where leaning towards implementing it, I would have suggested to extract the dump headers implementation and make it a freestanding utility function.

But more importantly, IMO the changes where non-const member functions have been changed to const member function are useful.

promag · 2018-08-19T10:45:43Z

Yeah I was going to cherry pick that.

Thanks for reviewing.

promag · 2018-08-20T00:39:23Z

But more importantly, IMO the changes where non-const member functions have been changed to const member function are useful.

@251labs see #14006.

promag commented Aug 17, 2018

View reviewed changes

promag force-pushed the 2017-08-dump-request-headers branch from af855e2 to 8d416e4 Compare August 17, 2018 14:27

laanwj reviewed Aug 17, 2018

View reviewed changes

laanwj added the RPC/REST/ZMQ label Aug 17, 2018

http: Add const modifier to HTTPRequest methods

51f1960

promag force-pushed the 2017-08-dump-request-headers branch from f0e8c1d to 1a4af64 Compare August 17, 2018 14:36

promag added 3 commits August 17, 2018 16:20

http: Add utility function EqualHeaders

ec39a57

http: Add HTTPRequest::GetHeaders

1c130b3

http: Dump HTTP request headers

88f1c9d

promag force-pushed the 2017-08-dump-request-headers branch from 1a4af64 to 88f1c9d Compare August 17, 2018 15:22

practicalswift reviewed Aug 17, 2018

View reviewed changes

promag closed this Aug 19, 2018

promag deleted the 2017-08-dump-request-headers branch August 19, 2018 09:35

promag mentioned this pull request Nov 2, 2018

rpc: Make HTTP RPC debug logging more informative #14618

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Dump HTTP request headers #13992

Dump HTTP request headers #13992

Uh oh!

promag commented Aug 17, 2018

Uh oh!

promag Aug 17, 2018

Uh oh!

promag commented Aug 17, 2018 •

edited

Loading

Uh oh!

practicalswift commented Aug 17, 2018 •

edited

Loading

Uh oh!

promag commented Aug 17, 2018

Uh oh!

laanwj Aug 17, 2018

Uh oh!

laanwj commented Aug 17, 2018 •

edited

Loading

Uh oh!

promag commented Aug 17, 2018

Uh oh!

laanwj commented Aug 17, 2018

Uh oh!

promag commented Aug 17, 2018

Uh oh!

practicalswift Aug 17, 2018 •

edited

Loading

Uh oh!

DrahtBot commented Aug 17, 2018

Uh oh!

l2a5b1 commented Aug 19, 2018

Uh oh!

promag commented Aug 19, 2018

Uh oh!

promag commented Aug 20, 2018

Uh oh!

Uh oh!

Dump HTTP request headers #13992

Dump HTTP request headers #13992

Uh oh!

Conversation

promag commented Aug 17, 2018

Uh oh!

promag Aug 17, 2018

Choose a reason for hiding this comment

Uh oh!

promag commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Aug 17, 2018

Uh oh!

laanwj Aug 17, 2018

Choose a reason for hiding this comment

Uh oh!

laanwj commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Aug 17, 2018

Uh oh!

laanwj commented Aug 17, 2018

Uh oh!

promag commented Aug 17, 2018

Uh oh!

practicalswift Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Aug 17, 2018

Uh oh!

l2a5b1 commented Aug 19, 2018

Uh oh!

promag commented Aug 19, 2018

Uh oh!

promag commented Aug 20, 2018

Uh oh!

Uh oh!

promag commented Aug 17, 2018 •

edited

Loading

practicalswift commented Aug 17, 2018 •

edited

Loading

laanwj commented Aug 17, 2018 •

edited

Loading

practicalswift Aug 17, 2018 •

edited

Loading