Issue #12396 - fix and re-enable NcsaRequestLogTest #12420
Conversation
Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>
| String log = _entries.poll(5, TimeUnit.SECONDS); | ||
| assertThat(log, containsString("GET http://hostname:8888/foo?name=value")); | ||
| assertThat(log, containsString("GET /foo?name=value")); |
There was a problem hiding this comment.
We don't have access in Jetty 12 to get the URI as it was received, so the request log reconstructs it using HttpURI.getPathQuery().
I think to fix this we would need some new API on Request.
There was a problem hiding this comment.
The "URI as it was received" is a strange one.
We build the URI from the components on the HTTP protocol (host, path, and query), and information from the connector (like isSecure).
What we see down here is the URI after it has possibly been modified (like the RewriteHandler), right?
If we had the raw host/path as seen on the protocol, would that be sufficient? or would that possibly cause NCSA format/syntax issues? (like what if the path was received on HTTP/2 with a space or control character that we stripped out / changed due to a Rewrite Rule?)
There was a problem hiding this comment.
The NCSA combined format can be represented like this ...
%h %l %u %t "%r" %s %b "%{Referer}i" "%{User-agent}i"
%h: the client hostname or IP address
%l: the client identifier or - if none is set
%u: the username used by the client for authentication or - if none is set
%t: the date and time of the HTTP request
%r: the first line of the HTTP request
%s: the status code of the HTTP response
%b: the byte size of the HTTP response
%{Referer}i: the URL that linked to the requested page or - if none is set
%{User-agent}i: the web browser and platform used by the client
I think you are attempting to handle the %r with this, right?
If so, this would really only work for HTTP/1.x, not HTTP/2 or HTTP/3 (as those two don't have the concept of "the first line of the HTTP request")
This "first line of the HTTP request" could also help with your BAD /badRequest HTTP/1.0 issue below.
There was a problem hiding this comment.
I worry that if we implement this, someone nefarious sort could come along and just hammer a server with huge "first line" and blow out the request log and/or server memory (as we would now store that raw "first line" in a way accessible from the Request object).
There was a problem hiding this comment.
I think its probably fine to keep the behavior for this as is. Even though it might not be the exact first line of the request for HTTP/1.x.
There was a problem hiding this comment.
I'm fine with this PR being what it is.
I opened #12423 to cover the other things brought up though.
jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/NcsaRequestLogTest.java
Show resolved
Hide resolved
| @@ -270,7 +295,7 @@ public void testBadVersion(String logType) throws Exception | |||
|
|
|||
| _connector.getResponse("METHOD /foo HTTP/9\n\n"); | |||
| String log = _entries.poll(5, TimeUnit.SECONDS); | |||
| assertThat(log, containsString("\"- - -\"")); | |||
| assertThat(log, containsString("\"BAD /badMessage HTTP/1.0\"")); | |||
There was a problem hiding this comment.
If no URI the HttpConnection creates a fake one to call the ErrorHandler. This was causing this to be GET /badMessage HTTP/1.0.
I have changed HttpConnection to set the Method to BAD so we can differentiate this from a valid request.
There was a problem hiding this comment.
Interesting, not sure about this change.
Can it be empty? or something that couldn't be sent on the protocol? (like a lowercase string with a space, eg: <bad request>).
Putting on my user/operations hat, seeing the hardcoded path /badMessage (or HTTP/1.0) would be a bit odd to see in the output of RequestLog too.
There was a problem hiding this comment.
I agree it is strange to have this in the RequestLog.
There is a test which does _connector.getResponse("XXXXXXXX") and it still shows up in RequestLog as BAD /badMessage HTTP/1.0.
but @gregw said it was necessary for the ErrorHandler.
maybe we could just add some flag to the request so requestLog knows to just display it as "- - -"?
There was a problem hiding this comment.
Based on how NCSA Request Log is loosely defined, I would expect connector.getResponse("XXXXXXX") to result in XXXXXXX showing up in the request log (in place of where BAD /badMessage HTTP/1.0 shows up)
There was a problem hiding this comment.
I'm fine with this PR being what it is.
I opened #12423 to cover the other things brought up though.
issue #12396