Skip to content
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

[POA-1519] Allow HTTP response without reason phrase #214

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

mgritter
Copy link
Contributor

@mgritter mgritter commented Aug 2, 2024

Technically, the RFC requires a space even if the reason phrase is empty, but at least one service has been observed to violate this. Add unit tests for both correct and incorrect blank reason phrase, and a one reproducing the original error to verify that Go's http parser is fine with this.

Technically, the RFC requires a space even if the reason phrase is
empty, but at least one service has been observed to violate this.
Add unit tests for both correct and incorrect blank reason phrase,
and a one reproducing the original error to verify that Go's http
parser is fine with this.
@mgritter mgritter requested a review from shreys7 August 2, 2024 22:20
@shreys7
Copy link
Member

shreys7 commented Aug 5, 2024

Technically, the RFC requires a space even if the reason phrase is empty, but at least one service has been observed to violate this.

Can you add where we observed this and how you found that this was the case? Empty reason phrase sounds skeptical, since I don't think we would have added any special handling in the Application layer for the service to do this.

input: "HTTP/1.1 200\r\n",
expectedDecision: akinet.Accept,
},
// This is technically legal, as the reason string can be zero bytes long.
Copy link
Member

@shreys7 shreys7 Aug 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we can just write This is technically legal as reason string is optional for better readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is less clear what "optional" means than the specific thing this test is demonstrating

@mgritter
Copy link
Contributor Author

mgritter commented Aug 5, 2024

Technically, the RFC requires a space even if the reason phrase is empty, but at least one service has been observed to violate this.

Can you add where we observed this and how you found that this was the case? Empty reason phrase sounds skeptical, since I don't think we would have added any special handling in the Application layer for the service to do this.

The TCPdumps demonstrating the problem are in https://postmanlabs.atlassian.net/browse/POA-1519; I didn't want to be more specific about Postman internal services in a public repository.

@mgritter mgritter merged commit 6896f31 into main Aug 5, 2024
3 checks passed
@mgritter mgritter deleted the mgritter/POA-1519-relax-response-parsing branch August 5, 2024 17:47
mgritter added a commit to postmanlabs/postman-insights-agent that referenced this pull request Aug 5, 2024
@mgritter mgritter restored the mgritter/POA-1519-relax-response-parsing branch October 12, 2024 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants