check v1 header bounds before reading #70
Conversation
| if err != nil { | ||
| // The header must be terminated by CRLF and cannot be more than 107 bytes long. | ||
| buf, err := reader.Peek(107) | ||
| if err != nil && err != io.EOF { |
There was a problem hiding this comment.
Per docs:
If Peek returns fewer than n bytes, it also returns an error explaining why the read is short.
Is the errorio.EOF? It's unclear from the docs and the code didn't immediately tell me it is.
There was a problem hiding this comment.
So there could be different errors - e.g. network read error. However, io.EOF is returned when there is less than 107 bytes available in the stream at that time, based on my tests. I will add some unit tests to cover various cases to validate this.
| if err != nil { | ||
| // The header must be terminated by CRLF and cannot be more than 107 bytes long. | ||
| buf, err := reader.Peek(107) | ||
| if err != nil && err != io.EOF { | ||
| return nil, ErrLineMustEndWithCrlf |
There was a problem hiding this comment.
Please, add a new error here, something like ErrLineSizeOverflow=errors.New("proxyproto: header is invalid, must be of size <= 108 bytes")
There was a problem hiding this comment.
This is to be captured in the tests I asked for.
| // At this point, can't be sure CR precedes LF which will be validated next. | ||
| line, err := reader.ReadString('\n') | ||
| if err != nil { | ||
| // The header must be terminated by CRLF and cannot be more than 107 bytes long. |
There was a problem hiding this comment.
Please, add a note here justifying the 107 bytes long, such as per spec
(...) a 108-byte buffer is always enough to store all the line and a trailing zero for string processing.
| return nil, ErrLineMustEndWithCrlf | ||
| } | ||
| if !strings.HasSuffix(line, crlf) { | ||
| pos := bytes.Index(buf, []byte(crlf)) | ||
| if pos == -1 { |
There was a problem hiding this comment.
The existing tests already capture this.
There was a problem hiding this comment.
Not clear what you mean by this. The Peek can return a buffer without CRLF at this point. It can also return a buffer that extends beyond the CRLF (e.g. short header, but more bytes available in the stream). So HasSuffix does not work here. Perhaps I've misunderstood your point though...
There was a problem hiding this comment.
Just a comment that existing tests (in main branch) already capture a scenario where the string read has LF but not CRLF.
|
I'll close this PR as the suggested updates don't resolve all problems and I want to base the fix on the commits in #71. I'll submit new fixes shortly. |
Resolves #69.