-
Notifications
You must be signed in to change notification settings - Fork 12
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
fix RawHeaders for POST #29
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -111,14 +111,12 @@ func TestRequestHeaderEmptyValueFromString(t *testing.T) { | |
} | ||
|
||
func TestRequestRawHeaders(t *testing.T) { | ||
kvs := "EmptyValue:\r\n" + | ||
"host: foobar\r\n" + | ||
kvs := "host: foobar\r\n" + | ||
"value: b\r\n" + | ||
"\r\n" | ||
t.Run("normalized", func(t *testing.T) { | ||
s := "GET / HTTP/1.1\r\n" + kvs | ||
exp := "Emptyvalue:\r\n" + | ||
"Host: foobar\r\n" + | ||
exp := "Host: foobar\r\n" + | ||
"Value: b\r\n" + | ||
"\r\n" | ||
var h RequestHeader | ||
|
@@ -129,16 +127,54 @@ func TestRequestRawHeaders(t *testing.T) { | |
if string(h.Host()) != "foobar" { | ||
t.Fatalf("unexpected host: %q. Expecting %q", h.Host(), "foobar") | ||
} | ||
v1 := h.Peek("EmptyValue") | ||
if len(v1) > 0 { | ||
t.Fatalf("expecting empty value. Got %q", v1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the need to remove EmptyValue testing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. EmptyValue check is not the purpose if this Test. It has been copied alongside other checks (by me, in #27) from a general headers tests that remains unchanged. In other words EmptyValue is tested elsewhere, the removal is my attempt to DRY my previous PR. |
||
v2 := h.Peek("Value") | ||
if !bytes.Equal(v2, []byte{'b'}) { | ||
t.Fatalf("expecting non empty value. Got %q", v2) | ||
} | ||
if raw := h.RawHeaders(); string(raw) != exp { | ||
t.Fatalf("expected header %q, got %q", exp, raw) | ||
} | ||
}) | ||
t.Run("post", func(t *testing.T) { | ||
s := "POST / HTTP/1.1\r\n" + kvs + "body" | ||
exp := "Host: foobar\r\n" + | ||
"Value: b\r\n" + | ||
"\r\n" | ||
var h RequestHeader | ||
br := bufio.NewReader(bytes.NewBufferString(s)) | ||
if err := h.Read(br); err != nil { | ||
t.Fatalf("unexpected error: %s", err) | ||
} | ||
if string(h.Host()) != "foobar" { | ||
t.Fatalf("unexpected host: %q. Expecting %q", h.Host(), "foobar") | ||
} | ||
v2 := h.Peek("Value") | ||
if !bytes.Equal(v2, []byte{'b'}) { | ||
t.Fatalf("expecting non empty value. Got %q", v2) | ||
} | ||
if raw := h.RawHeaders(); string(raw) != exp { | ||
t.Fatalf("expected header %q, got %q", exp, raw) | ||
} | ||
}) | ||
t.Run("http10", func(t *testing.T) { | ||
s := "GET / HTTP/1.0\r\n" + kvs | ||
exp := "Host: foobar\r\n" + | ||
"Value: b\r\n" + | ||
"\r\n" | ||
var h RequestHeader | ||
br := bufio.NewReader(bytes.NewBufferString(s)) | ||
if err := h.Read(br); err != nil { | ||
t.Fatalf("unexpected error: %s", err) | ||
} | ||
if string(h.Host()) != "foobar" { | ||
t.Fatalf("unexpected host: %q. Expecting %q", h.Host(), "foobar") | ||
} | ||
v2 := h.Peek("Value") | ||
if !bytes.Equal(v2, []byte{'b'}) { | ||
t.Fatalf("expecting non empty value. Got %q", v2) | ||
} | ||
if raw := h.RawHeaders(); string(raw) != exp { | ||
t.Fatalf("unexpected raw header %q, expected %q instead", kvs, raw) | ||
t.Fatalf("expected header %q, got %q", exp, raw) | ||
} | ||
}) | ||
t.Run("non-normalized", func(t *testing.T) { | ||
|
@@ -153,16 +189,12 @@ func TestRequestRawHeaders(t *testing.T) { | |
if string(h.Host()) != "" { | ||
t.Fatalf("unexpected host: %q. Expecting %q", h.Host(), "") | ||
} | ||
v1 := h.Peek("EmptyValue") | ||
if len(v1) > 0 { | ||
t.Fatalf("expecting empty value. Got %q", v1) | ||
} | ||
v2 := h.Peek("value") | ||
if !bytes.Equal(v2, []byte{'b'}) { | ||
t.Fatalf("expecting non empty value. Got %q", v2) | ||
} | ||
if raw := h.RawHeaders(); string(raw) != exp { | ||
t.Fatalf("unexpected raw header %q, expected %q instead", kvs, raw) | ||
t.Fatalf("expected header %q, got %q", exp, raw) | ||
} | ||
}) | ||
t.Run("no-kvs", func(t *testing.T) { | ||
|
@@ -182,7 +214,7 @@ func TestRequestRawHeaders(t *testing.T) { | |
t.Fatalf("expecting empty value. Got %q", v1) | ||
} | ||
if raw := h.RawHeaders(); string(raw) != exp { | ||
t.Fatalf("unexpected raw header %q, expected %q instead", kvs, raw) | ||
t.Fatalf("expected header %q, got %q", exp, raw) | ||
} | ||
}) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is going to work.
parse
is called from tryRead where it's buffer argument comes from mustPeekBuffered which as you can see gets its return value from bufio.Reader.Peek which will change its return value after a next read (for example for the body) makingrawHeaders
contain invalid data.I haven't tested this myself but I see your tests don't cover this case. Can you add a test here a whole POST request including a body of at least 4kb is parsed instead of just the RequestHeader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I have just followed the same trail literally minutes ago, will add a test shortly.