HTTPServerErrorHandler: Improve Error Handling for HTTPParserError #2922

fred-sch · 2022-11-29T08:59:11Z

Invalid HTTP previously just closed the connection, but did not actually handle the errors.

This PR uses an adapted version of https://github.com/apple/swift-nio/blob/main/Sources/NIOHTTP1/HTTPServerProtocolErrorHandler.swift to properly catch exceptions if they occur.

Closes: #2921

Refs: vapor#2921

…0a7bebdc5fda783f08765eb3ff56/Sources/NIOHTTP1/HTTPServerProtocolErrorHandler.swift

Sources/Vapor/HTTP/Server/HTTPServer.swift

fred-sch · 2022-12-12T08:55:14Z

Could you have a look at the proposed changes and trigger the workflow when you have some time @0xTim ?

Sources/Vapor/HTTP/Server/HTTPServer.swift

fred-sch · 2023-01-17T13:59:27Z

I had another look at the code and figured out that I was most likely not adding the HTTPServerErrorHandler at the right place between the different ChannelHandlers (which lead to some problems).

Now I put the HTTPServerErrorHandler always before the HTTPServerResponseEncoder. Also, I added the NIOCloseOnErrorHandler at the very end of the ChannelHandlers. This one should now take care of the role the HTTPServerErrorHandler had previously (closing connections) while the HTTPServerErrorHandler takes care of HTTPParserErrors.

Can one of you possibly take another look at the changes @Joannis @0xTim ?

fred-sch · 2023-02-23T15:18:10Z

For the context again: If a vapor app is supporting HTTP without TLS/SSL and is running behind nginx proxy, a single faulty (invalid HTTP) request can easily bring down all upstream servers for a time amount of the upstreams fail_timeout if the app is under some load, because there is also a bug in nginx that it will ignore the proxy_next_upstream_tries directive if under load (compare: https://trac.nginx.org/nginx/ticket/2421).

Another side note: we have been using this fix on one of our vapor based apis (approx. 20 Million req/day) for over a month now and have not experienced any problems so far.

Any thoughts or suggestions on this MR are very appreciated!

0xTim

This looks good to me - @Lukasa @weissi are you able to give this a quick once over to ensure the handler stuff is being used correctly?

0xTim · 2023-02-28T19:18:45Z

Sources/Vapor/HTTP/Server/HTTPServer.swift

+        if !self.hasUnterminatedResponse {
+            self.logger.debug("Bad Request - Invalid HTTP: \(error)")
+            let headers = HTTPHeaders([("Connection", "close"), ("Content-Length", "0")])
+            let head = HTTPResponseHead(version: .http1_1, status: .badRequest, headers: headers)


How does this fit in when running Vapor as an HTTP/2 server? Can we get the version from the current request?

Maybe the easiest way to deal with this would be to have a separate HTTP1ServerErrorHandlerand HTTP2ServerErrorHandler that are added in addVaporHTTP1Handlers and addVaporHTTP2Handlers respectively?

As far as I understand getting the HTTP version from the current request will not be easily possible because the error is thrown in the feedInput method of the BetterHTTPParser when the HTTPRequestDecoder is used to transform the InboundIn ByteBuffer to the InboundOut HTTPServerRequestPart (in the Http1 pipeline). So we never actually get to see the HTTPServerRequestPart when an error is thrown.

I am also wondering if this is even possibly happening in the HTTP/2 pipeline, because the HTTP2FramePayloadToHTTP1ServerCodec transforms a HTTP2Frame.FramePayload to a HTTPServerRequestPart. The only errors that are being thrown on the way should be NIOHTTP2Errors, so maybe a separate handler would be needed to handle such errors as well?

Yeah that makes sense. @FranzBusch I'd like to ask your thoughts here as the resident NIO expert! And if you have chance for a once-over in general as well that would be ace. This has been running fine in a prod system so I'm happy with the overall stability but just a sanity check to make sure we're not doing anything silly

This PR is by itself fine and writing out a badRequest in response to seeing an HTTPParserError is good. As you noted above though you also need to handle the HTTP2Errors somewhere. Either in a separate handler or by modifying this one.

I am just having a look at this again. I am wondering if the error handling logic that is implemented in the decodeFrame method and the processFrame method of the NIOHTTP2Handler is not already good enough when running Vapor as an HTTP/2 server?

Any opinions on my above comment @0xTim @FranzBusch ?

fred-sch · 2023-06-13T17:50:37Z

Sources/Vapor/HTTP/Server/HTTPServer.swift

+        let errorHandler = HTTPServerErrorHandler(logger: configuration.logger)
+        handlers.append(errorHandler)
+


Related to the discussion here, this should either be replaced by an appropriate ChannelHandler for HTTP2Errors at the correct place in the ChannelPipeline or be removed entirely

codecov-commenter · 2023-07-26T10:48:02Z

Codecov Report

Merging #2922 (eeb99a7) into main (a4b0715) will increase coverage by 0.10%.
Report is 1 commits behind head on main.
The diff coverage is 93.87%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2922      +/-   ##
==========================================
+ Coverage   76.21%   76.32%   +0.10%     
==========================================
  Files         211      211              
  Lines        7821     7864      +43     
==========================================
+ Hits         5961     6002      +41     
- Misses       1860     1862       +2

Files Changed	Coverage Δ
Sources/Vapor/HTTP/Headers/HTTPHeaders+Name.swift	`74.07% <ø> (ø)`
Sources/Vapor/HTTP/Server/HTTPServer.swift	`82.46% <93.33%> (+1.54%)`	⬆️
Sources/Vapor/Request/Request.swift	`76.34% <100.00%> (+0.78%)`	⬆️

gwynne

My only comment here is the one where I resurrected someone else's old comment 😅 But other than that, LGTM

Sources/Vapor/HTTP/Server/HTTPServer.swift

Issue was addressed

gwynne · 2023-09-22T07:08:51Z

@fred-sch Slightly off-topic, is it intentional that the Vapor CI workflows are being run in your fork? (Such as the API docs CI, although it fails immediately since the credentials secrets obviously aren't available.)

gwynne

This LGTM at this point, though I wouldn't mind at least one more set of eyes on it.

0xTim

I think we're good to merge!

penny-for-vapor · 2023-09-22T13:09:07Z

These changes are now available in 4.83.2

…2922) * fix: HTTPServerErrorHandler catch HTTPParserError Refs: #2921 * add reference to https://github.com/apple/swift-nio/blob/00341c92770e0a7bebdc5fda783f08765eb3ff56/Sources/NIOHTTP1/HTTPServerProtocolErrorHandler.swift * set hasUnterminatedResponse in write * error-handler needs to be before response encoder * add test for invalid http * rephrase and improve comments slightly * remove http1 error handler from http2 pipeline --------- Co-authored-by: Tim Condon <0xTim@users.noreply.github.com> Co-authored-by: Gwynne Raskind <gwynne@vapor.codes>

* Make use of NIOFileHandle safe * Fix sendable closure warnings in FileIO * Make BodyStreamWriter Sendable * Fix Sendable warnings in HTTPServerHandler * Fix Sendable warnings in HTTPServerRequestDecoder * Fix Sendable warnings in ChannelResponseBodyStream * Fix Sendable warnings in HTTPServerUpgradeHandler * Fix warnings in Tests * Fix some test warnings * Make Request+BodyStream sendable * Use LoopBound instead of Locks as we know we're on an EventLoop * Fix some test warnings * Fix some test warnings * Fix the last warnings in the tests * Remove the unchecked Sendable on request and response * Changes from code review * Reduce inits of NIOLoopBound * Remove unnecessary checks on NIOLoopBound * Remove reminders * Revert "Remove unnecessary checks on NIOLoopBound" This reverts commit de0d52f. * Improve routes * Remove unnecessary checks on NIOLoopBound * Show error * Compiles but failing test * Don't make stuff sendable when we don't need to be * Fix code review comments * Fix a couple of logic bugs in response * Fix some outdated comment docs * PR Review * HTTPServerErrorHandler: Improve Error Handling for HTTPParserError (#2922) * fix: HTTPServerErrorHandler catch HTTPParserError Refs: #2921 * add reference to https://github.com/apple/swift-nio/blob/00341c92770e0a7bebdc5fda783f08765eb3ff56/Sources/NIOHTTP1/HTTPServerProtocolErrorHandler.swift * set hasUnterminatedResponse in write * error-handler needs to be before response encoder * add test for invalid http * rephrase and improve comments slightly * remove http1 error handler from http2 pipeline --------- Co-authored-by: Tim Condon <0xTim@users.noreply.github.com> Co-authored-by: Gwynne Raskind <gwynne@vapor.codes> * Fix mismatching manifests * PR feedback * Trigger CodeQL * Trip CI --------- Co-authored-by: Gwynne Raskind <gwynne@vapor.codes> Co-authored-by: fred-sch <73998525+fred-sch@users.noreply.github.com>

fred-sch added 2 commits November 28, 2022 23:22

fix: HTTPServerErrorHandler catch HTTPParserError

e0bdb7d

Refs: vapor#2921

add reference to https://github.com/apple/swift-nio/blob/00341c92770e…

8f0db21

…0a7bebdc5fda783f08765eb3ff56/Sources/NIOHTTP1/HTTPServerProtocolErrorHandler.swift

MrSkwiggs reviewed Nov 29, 2022

View reviewed changes

Sources/Vapor/HTTP/Server/HTTPServer.swift Outdated Show resolved Hide resolved

fred-sch commented Nov 29, 2022

View reviewed changes

Sources/Vapor/HTTP/Server/HTTPServer.swift Outdated Show resolved Hide resolved

0xTim and others added 2 commits January 9, 2023 10:27

Merge branch 'main' into main

68a2802

temporary fix for failing tests

79f0cd3

fred-sch force-pushed the main branch from 476d4d8 to 79f0cd3 Compare January 9, 2023 12:24

Joannis reviewed Jan 14, 2023

View reviewed changes

Sources/Vapor/HTTP/Server/HTTPServer.swift Outdated Show resolved Hide resolved

Sources/Vapor/HTTP/Server/HTTPServer.swift Show resolved Hide resolved

fred-sch added 2 commits January 17, 2023 10:19

set hasUnterminatedResponse in write

0c5f81f

error-handler needs to be before response encoder

20c3325

fred-sch and others added 2 commits January 18, 2023 09:42

add test for invalid http

90cf49d

Merge branch 'main' into main

4ec2e00

0xTim reviewed Mar 1, 2023

View reviewed changes

Merge branch 'main' into main

5183093

0xTim added the semver-patch Internal changes only label Mar 1, 2023

fred-sch commented Jun 13, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main'

bc07b3c

fred-sch requested a review from gwynne as a code owner July 26, 2023 10:42

gwynne previously requested changes Jul 26, 2023

View reviewed changes

Sources/Vapor/HTTP/Server/HTTPServer.swift Outdated Show resolved Hide resolved

fred-sch added 3 commits July 26, 2023 15:54

rephrase and improve comments slightly

d936401

remove http1 error handler from http2 pipeline

9d4dfa8

tiny improvements

4062b94

0xTim and others added 2 commits August 20, 2023 01:16

Merge branch 'main' into main

af985ac

Merge branch 'main' into main

eeb99a7

gwynne requested a review from 0xTim September 22, 2023 06:56

gwynne approved these changes Sep 22, 2023

View reviewed changes

0xTim approved these changes Sep 22, 2023

View reviewed changes

gwynne merged commit 1f2b44b into vapor:main Sep 22, 2023
16 checks passed

grahamburgsma mentioned this pull request Oct 5, 2023

Crash likely from changes in #2922 #3079

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTPServerErrorHandler: Improve Error Handling for HTTPParserError #2922

HTTPServerErrorHandler: Improve Error Handling for HTTPParserError #2922

fred-sch commented Nov 29, 2022

fred-sch commented Dec 12, 2022

fred-sch commented Jan 17, 2023

fred-sch commented Feb 23, 2023

0xTim left a comment

0xTim Feb 28, 2023

fred-sch Mar 2, 2023

0xTim Mar 7, 2023

FranzBusch Apr 27, 2023

fred-sch Jun 13, 2023 •

edited

fred-sch Jul 26, 2023

fred-sch Jun 13, 2023

codecov-commenter commented Jul 26, 2023 •

edited

gwynne left a comment

gwynne commented Sep 22, 2023

gwynne left a comment

0xTim left a comment

penny-for-vapor bot commented Sep 22, 2023

		let errorHandler = HTTPServerErrorHandler(logger: configuration.logger)
		handlers.append(errorHandler)

HTTPServerErrorHandler: Improve Error Handling for HTTPParserError #2922

HTTPServerErrorHandler: Improve Error Handling for HTTPParserError #2922

Conversation

fred-sch commented Nov 29, 2022

fred-sch commented Dec 12, 2022

fred-sch commented Jan 17, 2023

fred-sch commented Feb 23, 2023

0xTim left a comment

Choose a reason for hiding this comment

0xTim Feb 28, 2023

Choose a reason for hiding this comment

fred-sch Mar 2, 2023

Choose a reason for hiding this comment

0xTim Mar 7, 2023

Choose a reason for hiding this comment

FranzBusch Apr 27, 2023

Choose a reason for hiding this comment

fred-sch Jun 13, 2023 • edited

Choose a reason for hiding this comment

fred-sch Jul 26, 2023

Choose a reason for hiding this comment

fred-sch Jun 13, 2023

Choose a reason for hiding this comment

codecov-commenter commented Jul 26, 2023 • edited

Codecov Report

gwynne left a comment

Choose a reason for hiding this comment

gwynne commented Sep 22, 2023

gwynne left a comment

Choose a reason for hiding this comment

0xTim left a comment

Choose a reason for hiding this comment

penny-for-vapor bot commented Sep 22, 2023

fred-sch Jun 13, 2023 •

edited

codecov-commenter commented Jul 26, 2023 •

edited