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

Jump to bottom

transport/http2: use HTTP 400 for bad requests instead of 500 #5804

Merged
merged 1 commit into from Dec 13, 2022
Merged

transport/http2: use HTTP 400 for bad requests instead of 500 #5804

merged 1 commit into from Dec 13, 2022

Conversation

sjbarag
Copy link
Contributor

@sjbarag sjbarag commented Nov 18, 2022 •
edited by dfawley

Non-servicable HTTP requests from clients previously resulted in an HTTP 500 response, but no error exists within the server; the client simply sent a malformed request. Detect bad requests and return HTTP 400 Bad Request instead of HTTP 500 Internal Server Error.

fixes #5802

RELEASE NOTES:

  • transport (net/http server handler): respond to bad HTTP requests with status 400 (Bad Request) instead of 500 (Internal Server Error).

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 18, 2022 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: sjbarag / name: Sean Barag (4fb92d6)

@sjbarag
Copy link
Contributor Author

sjbarag commented Nov 18, 2022 •
edited

A few notes while CI runs its course:

  • There don't appear to be any tests covering the status code included in the HTTP response, but I'm happy to add some if you can give me an appropriate place to put them!
  • While grpc.Server responds with HTTP 500 for malformed requests instead of 4XX #5802 mentions other 4XX response codes, supporting them would involve a pretty significant refactor of google.golang.org/grpc/transport.NewServerHandlerTransport. HTTP 426 Upgrade Required is only valid if the same request would be valid with a different protocol [1], so the resolution of "which HTTP status code do we return?" would get significantly more complicated. It'd be nice to support the HTTP spec more closely, but HTTP 400 for general "client sent a bad request" appears appropriate.
  • I haven't added a "RELEASE NOTES" line to the description. I imagine that's something y'all want control over, but let me know if that's not the case 🙂

[1] From https://httpwg.org/specs/rfc9110.html#field.upgrade:

A server MUST NOT switch protocols unless the received message semantics can be honored by the new protocol; an OPTIONS request can be honored by any protocol.
https://httpwg.org/specs/rfc9110.html#field.upgrade

@sjbarag
Copy link
Contributor Author

sjbarag commented Nov 28, 2022

Looks like "Testing / tests (tests, 1.19, 386)" will pass once #5812 lands and I rebase.

@dfawley dfawley self-requested a review November 29, 2022 18:44
@dfawley dfawley self-assigned this Nov 29, 2022
@dfawley dfawley removed their assignment Nov 29, 2022
@dfawley dfawley added this to the 1.52 Release milestone Nov 29, 2022
dfawley
dfawley reviewed Nov 30, 2022
View reviewed changes
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Somehow I forgot to send the review comment I wrote, sorry!

server.go Outdated
Comment on lines 1011 to 1015
var status = http.StatusBadRequest
if err == transport.ErrInvalidWriter {
status = http.StatusInternalServerError
}
http.Error(w, err.Error(), status)
Copy link
Member

Choose a reason for hiding this comment

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

Could we move this status code writing fully into the transport layer, to avoid the need to span the logic between the two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries! Great idea — fixed in r2 (6a6c330).

@sjbarag sjbarag force-pushed the respond-with-http-400-for-bad-requests branch from 4fb92d6 to 6a6c330 Compare November 30, 2022 19:13
@github-actions
Copy link

github-actions bot commented Dec 6, 2022

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions github-actions bot added the stale label Dec 6, 2022
@sjbarag
Copy link
Contributor Author

sjbarag commented Dec 6, 2022

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

Erm, that’s odd - I pushed another commit six days ago.

@dfawley if you get a few minutes, could you take another look (or maybe mark this not-stale)?

@github-actions github-actions bot removed the stale label Dec 6, 2022
@arvindbr8 arvindbr8 assigned dfawley and unassigned sjbarag Dec 7, 2022
dfawley
dfawley approved these changes Dec 7, 2022
View reviewed changes
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Erm, that’s odd - I pushed another commit six days ago.

Sorry, I just missed your updates. LGTM now, thank you for the change!

@dfawley dfawley assigned easwars and unassigned dfawley Dec 7, 2022
@dfawley dfawley requested a review from easwars December 7, 2022 23:23
easwars
easwars requested changes Dec 8, 2022
View reviewed changes
internal/transport/handler_server.go Outdated Show resolved Hide resolved
internal/transport/handler_server.go Outdated Show resolved Hide resolved
internal/transport/handler_server.go Show resolved Hide resolved
internal/transport/handler_server.go Outdated Show resolved Hide resolved
@easwars easwars assigned sjbarag and unassigned easwars Dec 8, 2022
@sjbarag sjbarag force-pushed the respond-with-http-400-for-bad-requests branch from 6a6c330 to bd7c5a9 Compare December 8, 2022 22:16
@easwars
Copy link
Contributor

easwars commented Dec 9, 2022

Looks like the tests are failing because they were checking for exact string match. Could you please fix them.

@sjbarag
Copy link
Contributor Author

sjbarag commented Dec 9, 2022

Welllll heck, I’m sorry about that! Influenza brain told me to push without running the tests. I’ll fix that up today.

Thanks for your help and patience, everyone!

@sjbarag
transport/http2: use HTTP 400 for bad requests instead of 500
d789adc 
Non-servicable HTTP requests from clients previously resulted in an HTTP
500 response, but no error exists within the server; the client simply
sent a malformed request. Detect bad requests and return HTTP 400 Bad
Request instead of HTTP 500 Internal Server Error.

fixes grpc#5802
@sjbarag sjbarag force-pushed the respond-with-http-400-for-bad-requests branch from bd7c5a9 to d789adc Compare December 9, 2022 18:00
@sjbarag
Copy link
Contributor Author

sjbarag commented Dec 9, 2022

Tests fixed, sorry again!

@easwars easwars merged commit 2f413c4 into grpc:master Dec 13, 2022
1 check passed
@easwars
Copy link
Contributor

easwars commented Dec 13, 2022

Thank you for your contribution.

@sjbarag
Copy link
Contributor Author

sjbarag commented Dec 13, 2022

Thanks for all your help @easwars and @dfawley ! 🙂

@sjbarag sjbarag deleted the respond-with-http-400-for-bad-requests branch December 13, 2022 21:58
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@easwars easwars easwars approved these changes

@dfawley dfawley dfawley approved these changes

Assignees

@sjbarag sjbarag

Labels
Type: Bug
Projects
None yet
Milestone
1.52 Release
Development

Successfully merging this pull request may close these issues.

grpc.Server responds with HTTP 500 for malformed requests instead of 4XX
4 participants
@sjbarag @easwars @dfawley @arvindbr8