fix: ignoring response header field space #1921
Conversation
| for _, ch := range s.key { | ||
| if !validHeaderFieldByte(ch) { | ||
| if ch == ' ' { | ||
| spaceIncluded = true | ||
| break |
There was a problem hiding this comment.
If there are invalid characters after space, the validHeaderField will be skipped. I think break should be changed to continue.
There was a problem hiding this comment.
Yes, you're right. I didn't think that...
There was a problem hiding this comment.
Changed! Thank you!
There was a problem hiding this comment.
Wait a minute... Come to think of it, I'm going to ignore it anyway, but do we still need validation? If we need validation of the header key, don't we need validation of the header value?
There was a problem hiding this comment.
I think there are two choices, one is to validate both the key and the value because if there is the wrong byte, it should fail, and the other is not to do it after have checked the space. And wouldn't it be right not to validate it for fasthttp? I think it would be better to skip the minor verification.
There was a problem hiding this comment.
In net/http, they handle headers even if they contain spaces. I just realized that you directly ignore this header field. Although I think we should follow the behavior with net/http instead of skipping this header.
We can ask @erikdubbelboer for his opinion.
There was a problem hiding this comment.
I wanted to follow that action if possible, but net/http doesn't canonicalize the header afterwards, but fasthttp normalizes it through disableNormalizing, so I decided to just ignore it. This is also because it is a common behavior for Internet browsers. If you have any ideas, please let me know.
There was a problem hiding this comment.
I also prefer it if we follow the behavior of net/http as that is really well tested.
prepaser
force-pushed
the
ignore-header-field-spaces
branch
from
419d178 to
1c9a91e
Compare
Closes #1917
Ignore when the header key contains spaces.