Add support for space in get request #5495
Conversation
ph
force-pushed
the
fix/packetbeat-support-space-get-request
branch
from
16fd62d to
a72b29a
Compare
|
@urso This is the PR I want to discuss with you, this change for accuracy and supporting badly form URI, add some slowness to that part of the code. It add bit more allocation, but It should be faster if the URI has more space in it than using This PRmaster |
| m.method = common.NetString(slices[0]) | ||
| m.requestURI = common.NetString(slices[1]) | ||
| m.method = common.NetString(fline[:afterMethodIdx]) | ||
| m.requestURI = common.NetString(fline[afterMethodIdx+1 : afterRequestURIIdx]) |
There was a problem hiding this comment.
if there is more than one space between URI and HTTP_VERSION it will be stored into requestURI. I think you should check for that (bytes.Fields did skip consecutive separators)
There was a problem hiding this comment.
I gave some thoughts about it, I don't think we should check for that, if we do it it would mean that we could potentially normalize the query string parameters.
If we look at the following case:
GET /e/b?movie=a brand new world HTTP/1.1 => '/e/b?movie=a brand new world'
GET /e/b?movie=a brand new world HTTP/1.1 =>
/elastic/beats?movie=a brand new world '
When we extract the parameters that should gives us 2 distinct values for the movie field: a brand new world and a brand new world (not the space at the end of world)
WDYT?
There was a problem hiding this comment.
I ran a quick check and the server did skip those extra spaces.
Now I was testing again and see that it's more common for servers to return a 400 Bad Request.
So I guess you can just ignore the comment. It's not common at all and spaces are escaped to %20 or + anyway
|
|
||
| if bytes.Equal(slices[2][:5], []byte("HTTP/")) { | ||
| if bytes.Equal(fline[afterRequestURIIdx+1:afterRequestURIIdx+6], constHTTPVersion) { |
There was a problem hiding this comment.
Suggestion
+len(constHTTPVersion)+1 instead of +6
and :len(constHTTPVersion) in the line above
|
@adriansr thanks, I will do another check. |
ph
force-pushed
the
fix/packetbeat-support-space-get-request
branch
from
a72b29a to
68b6066
Compare
|
@adriansr I have updated the code with the change with |
ph
force-pushed
the
fix/packetbeat-support-space-get-request
branch
from
68b6066 to
4a2fa7f
Compare
|
This can safely go into 6.0.1, I have created the label for it. |
This fix an issue when the http request contains a space instead of
breaking the line with `bytes.fields` we are finding the start and the end
of the URI using the `METHOD` verb and the `HTTP/{VERSION}`. This will
allow packet beat to record theses request instead of ignoring them.
Fixes: #4974
ph
force-pushed
the
fix/packetbeat-support-space-get-request
branch
from
4a2fa7f to
af9f703
Compare
This fix an issue when the http request contains a space instead of
breaking the line with `bytes.fields` we are finding the start and the end
of the URI using the `METHOD` verb and the `HTTP/{VERSION}`. This will
allow packet beat to record theses request instead of ignoring them.
Fixes: elastic#4974
This fix an issue when the http request contains a space instead of
breaking the line with `bytes.fields` we are finding the start and the end
of the URI using the `METHOD` verb and the `HTTP/{VERSION}`. This will
allow packet beat to record theses request instead of ignoring them.
Fixes: #4974
This fix an issue when the http request contains a space instead of
breaking the line with
bytes.fieldswe are finding the start and the endof the URI using the
METHODverb and theHTTP/{VERSION}. This willallow packet beat to record theses request instead of ignoring them.
Fixes: #4974