Motivation
When we receive a HEAD response, it's possible that the response
contains a content-length. llhttp has a bug
(https://github.com/nodejs/llhttp/issues/202) that prevents it from
properly managing that issue, which causes us to incorrectly parse
responses.
Modifications
Forcibly set llhttp's content-length value to 0.
Result
Correctly handle HTTP framing around llhttp's issues.
Motivation
HTTP headers are prevented from containing certain characters that can
potentially affect parsing or interpretation. Inadequately policing this
can lead to vulnerabilities in web applications, most notably HTTP
Response Splitting.
NIO was insufficiently policing the correctness of the header fields we
emit in HTTP/1.1. We've therefore added a new handler that is
automatically added to channel pipelines that will police the validity
of header fields.
For projects that are already running the validation themselves, this
can be easily disabled. Note that by default NIO does not validate
content length is correctly calculated, so applications can have their
framing fall out of sync unless they appropriately calculate this
themselves or use chunked transfer encoding.
Modifications
- Add thorough unit testing to confirm we will not emit invalid header
fields.
- Error if a user attempts to send an invalid header field.
Result
NIO applications are no longer vulnerable to response splitting by CRLF
injection by default.
Motivation:
We should better tolerate LLHTTP status codes we don't yet know about.
Modifications:
- Added support for the status codes that currently exist
- Add a fallback to the RAW case for the future.
Result:
Better management of LLHTTP status codes
Motivation:
The node.js HTTP parser library that we use has been unmaintained for some time. We should move to the maintained replacement, which is llhttp. This patch will update our dependency and bring us over to the new library, as well as make any changes we need.
Modifications:
This patch comes in 4 parts, each contained in a separate commit in the PR.
The first commit drops the existing http_parser code and updates some of the repo state for using llhttp.
The second commit rewrites the update script to bring in llhttp instead of http_parser.
The third runs the actual script. You can skip reviewing this except to sanity check the outcome.
The fourth commit updates the NIO code and the tests to get everything working.
In general the substance of the product modifications was minimal. The logic around keeping track of where we are in the buffer and how upgrades work has changed a bit, so that required some fiddling. I also had to add an error reporting path for the delegates to be able to throw specific errors that llhttp no longer checks for. Finally, I removed two tests that were a little overzealous and that llhttp does not police.
Result:
Back on the supported path.
Motivation:
- Currently http 1xx response heads are handled incorrectly
Modifications:
- Add an `InformationalResponseStrategy` that allows the user to specify whether http 1xx responses shall be forwarded or dropped.
- Implement the necessary cases in `HTTPDecoder` and `BetterHTTPParser`
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
As we've largely completed our move to split out our core abstractions,
we now have an opportunity to clean up our dependencies and imports. We
should arrange for everything to only import NIO if it actually needs
it, and to correctly express dependencies on NIOCore and NIOEmbedded
where they exist.
We aren't yet splitting out tests that only test functionality in
NIOCore, that will follow in a separate patch.
Modifications:
- Fixed up imports
- Made sure our protocols only require NIOCore.
Result:
Better expression of dependencies.
Co-authored-by: George Barnett <gbarnett@apple.com>
Motivation:
I'm sick of typing `.init(major: 1, minor: 1)`.
Modifications:
- Added static vars for common HTTP versions.
Result:
Maybe I'll never type `.init(major: 1, minor: 1)` ever again.
Motivation:
There are multiple sub-optimal ByteBuffer creation patterns that occur
in the wild. Most often they happen when people don't actually have
access to a `Channel` just want to "convert" a `String` into a
`ByteBuffer`. To do this, they are forced to type
var buffer = ByteBufferAllocator().buffer(capacity: string.utf8.count)
buffer.writeString(string)
Sometimes, they don't get the capacity calculation right or just put a
`0`.
Similar problems happen if NIO users want to cache a ByteBuffer in their
`ChannelHandler`. You will then find this code:
```swift
if self.buffer == nil {
self.buffer = receivedBuffer
} else {
var receivedBuffer = receivedBuffer
self.buffer!.writeBuffer(&receivedBuffer)
}
```
And lastly, sometimes people want to append one `ByteBuffer` to another
without mutating the appendee. That's also cumbersome because we only
support a mutable version of `writeBuffer`.
Modifications:
- add `ByteBuffer` convenience initialisers
- add convenience `writeBuffer` methods to `Optional<ByteBuffer>`
- add `writeBufferImmutable` which doesn't mutate the appendee.
Result:
More convenience.
Co-authored-by: Cory Benfield <lukasa@apple.com>
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
The
do {
try someOperation()
XCTFail("should throw") // easy to forget
} catch error as SomethingError {
XCTAssertEqual(.something, error as? SomethingError)
} catch {
XCTFail("wrong error")
}
pattern is not only very long, it's also very error prone. If you forget
any of the XCTFails, you might not tests what it looks like
XCTAssertThrowsError(try someOperation) { error in
XCTAssertEqual(.something, error as? SomethingError)
}
is much safer and shorter.
Modifcations:
Do many of the above replaces.
Result:
Cleaner, shorter, and safer tests.
Motivation:
Was talking about chunked encoding with @adtrevor, so I thought let's
actually capture it in a test.
Modifications:
- add a test
Result:
- more tests
Motivation:
http-parser shipped a patche for node.js CVE-2019-15605, which allowed
HTTP request smuggling. This affected SwiftNIO as well, and so we need
to immediately ship an update to help protect affected users.
A CVE for SwiftNIO will follow, but as this patch is in the wild and
SwiftNIO is known to be affected we should not delay shipping this fix.
Modifications:
- Update http-parser.
- Add regression tests to validate this behaviour.
Result:
Close request smugging vector.
Motivation:
Despite the fact that we already stopped delivering unsolicited
responses for the HTTP response decoder, there was a window where we
would deliver a second .head for a response that come without a request.
Modifications:
Don't deliver a second .head even if send together with a first, legit
response.
Result:
Being a HTTP client using NIO is now easier.
Motivation:
Right now NIO clients will crash in the (admittedly unlikely) event that
a HTTP server sends an unsolicited extra response on a connection. This
seems pretty unnecessary, and fairly graceless.
Modifications:
- Replace a forcible pop with an optional one.
- Added NIOHTTPDecoderError type.
Result:
Removed a crash that we definitely don't want.
Motivation:
Previously we thought that if we have some bytes left that belong to an
upgraded protocol, we should deliver those as an error. This is
implemented on `master` but not in any released NIO version.
However, no other handler sends errors on unclean shutdown, so it feels
wrong to do it in this one very specific case (EOF on inflight upgrade
with data for the upgraded protocol)
Modifications:
Remove the error again.
Result:
Less behaviour change to the last released NIO version.
Motivation:
Do as we documented and invoke the leftovers strategy only when the
handler is removed.
Modifications:
Don't invoke leftovers on EOF (which isn't something the user can
control).
Result:
- Fewer crashes
- should fix https://github.com/IBM-Swift/Kitura-WebSocket-NIO/issues/35
Motivation:
When writing B2MDs, there are a couple of scenarios that always need to
be tested: firehose feeding, drip feeding, many messages, ...
It's tedious writing those tests over and over again for every B2MD.
Modifications:
- Add a simple B2MD verifier that users can use in unit tests.
- Add a new, public `NIOTestUtils` module which contains utilities
mostly useful for testing. Crucially however, it does not depend on
`XCTest` so it can be used it all targets.
Result:
Hopefully fewer bugs in B2MDs.
Motivation:
With HTTP/1.0 and EOF framing it's possible to have a HTTP response that
doesn't have any headers. HTTPDecoder unfortunately fell flat on its
face and crashed the program if that happened.
Modifications:
- don't expect headers on responses
- add tests for that
- rename the state machine cases to reflect a request and response
parsing
Result:
fewer crashes.
Motivation:
The HTTPDecoder is a complex object that has very careful state management goals. One source of this
complexity is that it is fed a stream of bytes with arbitrary chunk sizes, but needs to produce a
collection of objects that are contiguous in memory. For example, each header field name and value
must be turned into a String, which requires a contiguous sequence of bytes to do.
As a result, it is quite common to have a situation where the HTTPDecoder has only *part* of an
object that must be emitted atomically. In this situation, the HTTPDecoder would like to instruct
its ByteToMessageHandler to keep hold of the bytes that form the beginning of that object. To avoid
asking http_parser to parse those bytes twice, the HTTPDecoder uses a value called httpParserOffset
to keep track.
As an example, consider what would happen if the "Connection: keep-alive\r\n" header field was delivered
in two chunks: first "Connection: keep-al", and then "ive\r\n". The header field name can be emitted in
its entirety, but the partial field value must be preserved. To achieve this, the HTTPDecoder will store
an offset internally to keep track of which bytes have been parsed. In this case, the offset will be set
to 7: the number of bytes in "keep-al". It will then tell the rest of the code that only 12 bytes of the
original 19 byte message were consumed, causing the ByteToMessageHandler to preserve those 7 bytes.
However, when the next chunk is received, the ByteToMessageHandler will *replay* those bytes to
HTTPDecoder. To avoid parsing them a second time, HTTPDecoder keeps track of how many bytes it is
expecting to see replayed. This is the value in httpParserOffset.
Due to a logic error in the HTTPDecoder, the httpParserOffset field was never returned to zero.
This field would be modified whenever a partial field was received, but would never be returned
to zero when a complete message was parsed. This would cause the HTTPDecoder to unnecessarily keep
hold of extra bytes in the ByteToMessageHandler even when they were no longer needed. In some cases
the number could get smaller, such as when a new partial field was received, but it could never drop
to zero even when a complete HTTP message was receivedincremented.
Happily, due to the rest of the HTTPDecoder logic this never produced an invalid message: while
ByteToMessageHandler was repeatedly producing extra bytes, it never actually passed them to http_parser
again, or caused any other issue. The only situation in which a problem would occur is if the HTTPDecoder
had a RemoveAfterUpgradeStrategy other than .dropBytes. In that circumstance, decodeLast would not
consume any extra bytes, but those bytes would have remained in the buffer passed to decodeLast, which
would then incorrectly *forward them on*. This is the only circumstance in which this error manifested,
and in most applications it led to surprising and irregular crashes on connection teardown. In all
other applications the only effect was unnecessarily preserving a few tens of extra bytes on
some connections, until receiving EOF caused us to drop all that memory anyway.
Modifications:
- Return httpParserOffset to 0 when a full message has been delivered.
Result:
Fewer weird crashes.
Motivation:
The old HTTP parser tried to convert `http_parser` to a one-shot parser
by pausing it constantly. That was done to be resilient against
re-entrancy. But now, we have the new `ByteToMessageDecoder` which
protects against that so HTTP decoder can just become a real
`ByteToMessageDecoder`. It also serves as a great testbed for
`ByteToMessageDecoder`.
Modifications:
- make the HTTP decoder a `ByteToMessageDecoder`
- refactor the implementation
Result:
cleaner code, more use of `ByteToMessageDecoder`
Motivation:
readInbound/Outbound will soon throw errors if the type isn't right.
Modifications:
prepare tests for throwing readIn/Outbound
Result:
@ianpartridge's PR should pretty much merge after this.
Motivation:
There's just too much test code that has
(channel.eventLoop as! EmbeddedEventLoop).run()
Modifications:
have EmbeddedChannel expose its EmbeddedEventLoop
Result:
easier test code
Motivation:
`ctx` was always an abbreviation was 'context` and in Swift we don't
really use abbreviations, so let's fix it.
Modifications:
- rename all instances of `ctx` to `context`
Result:
- fixes#483
Motivation:
- `ChannelPipeline.add(name:handler:...)` had a strange order of arguments
- `remove(handler:)` and `remove(ctx:)` both remove `ChannelHandler`s
but they read like they remove different things
So let's just fix the argument order and name them `addHandler` and
`removeHandler` making clear what they do.
Modifications:
- rename all `ChannelPipeline.add(name:handler:...)`s to `ChannelPipeline.addHandler(_:name:...)`
- rename all `ChannelPipeline.remove(...)`s to `ChannelPipeline.removeHandler(...)`
Result:
more readable and consistent code
Motivation:
Some of the ChannelPipeline removal methods did not go through the
RemovableChannelHandler API.
Modifications:
make all removal APIs go through the RemovableChannelHandler API
Result:
fewer bugs
Motivation:
ByteBuffer methods like `set(string:)` never felt very Swift-like and
also didn't look the same as their counterparts like `getString(...)`.
Modifications:
- rename all `ByteBuffer.set/write(<type>:,...)` methods to
`ByteBuffer.set/write<Type>(...)`
- polyfill the old spellings in `_NIO1APIShims`
Result:
code more Swift-like
Motivation:
If ChannelHandler removal worked correctly, it was often either by
accident or by intricate knowledge about the implementation of the
ChannelHandler that is to be removed. Especially when it comes to
re-entrancy it mostly didn't work correctly.
Modifications:
- introduce a `RemovableChannelHandler` API
- raise allocation limit per HTTP connection by 1
(https://bugs.swift.org/browse/SR-9905)
Result:
Make things work by contruction rather than accident
Add deadlines as an alternative to timeouts
Motivation:
Address #603
Modifications:
- Add NIODeadline type
- Add func scheduleTask<T>(deadline: NIODeadline, _ task: @escaping () throws -> T) -> Scheduled<T>
- Reduce the use of DispatchTime in favor of Time
- Replace timeout calls with deadline calls where it makes sense
Result:
Tasks can be scheduled after an amount of time has passed, or at a certain time.
Motivation:
ELF's API should be as close as possible to the new Result's API.
Therefore, we should rename `then` to `flatMap`
Modifications:
- renamed `then` to `flatMap`
- renamed `thenIfError` to `flatMapError`
- renamed ELF's generic parameter from `T` to `Value`
Result:
- more like Result
- fixes#688
Motivation:
When we write unit tests, we generally like them to actually test the
thing they claim to. This test never ran, and the assertions in place
to verify that it ran also never ran, so we didn't notice the test
not running.
Modifications:
- Moved the assertion that validates the test ran to a place where
it will definitely execute.
- Ensured the EmbeddedChannel gets marked as active, so that we
actually deliver the bytes from the HTTPDecoder.
Result:
This test will run, and actually pass.
Motivation:
There was an outstanding change in http_parser.c that we did not yet
have in our copy.
Modifications:
run the update http parser script and add changes as well as a test case
designed to hit the change.
Result:
ship the latest and greatest
Motivation:
Our copy of http_parser has become almost a year old now, and there are
both some bugfixes that improve correctness and some performance
improvements in http_parser that we should pick up.
Modifications:
- Updated http_parser
- Added tests to confirm we don't suffer from
https://github.com/nodejs/http-parser/pull/432
- Added tests that we didn't get broken by SOURCE verb addition.
Result:
Shiny new http_parser!
Motivation:
Swift has a rather unhelpful warning
warning: treating a forced downcast to 'EmbeddedEventLoop' as optional will never produce 'nil'
when assigning that to a variable of type `EmbeddedEventLoop!`. The
warning is accurate but obviously we know that can never be `nil`. The
way to silence this warning is to put parenthesis around the expression.
Modifications:
added parenthesis around force casts that are assigned to IUOs
Result:
fewer warnings with 4.2
Motivation:
d53ee6dafc introduced a new constructor to HTTPRequestDecoder which allows to change the behaviour of handling left over bytes when an upgrade was detected and the decoder was removed. This was done as an internal init block as we wanted to to do as part of a patch release.
Modifications:
Change internal to public init
Result:
More flexible configuration possible.
Motivation:
398b950eee introduced a change which forwarded bytes that were buffered in HTTPDecoder after it was removed when an upgrade was detected. This may be risky if we dont know that the pipeline can handle it.
Modifications:
- Add new initializer that allow us to configure if we should forward bytes or not.
- Automatically use this if we know that we added an upgrader
- Add unit tests.
Result:
Less risky implementation.
Motivation:
When discardReadBytes() was called and we still did not pass the Head to the user it was quite possible that the headers / URI could be corrupted as the stored readerIndex did not matchup anymore.
Modifications:
- Override most of the functionality of ByteToMessageDecoder in HTTPDecoder to better work with the provided state machine of http_parser
- Remove allocations by removing the pendingInOut Array as its not needed anymore.
- Add guards against re-entrance calls
- Add unit test that shows that everything works as expected now.
Result:
Fixed bug and reduced allocations (8% - 10% perf win).
Motivation:
Our HTTP code handles only HTTP/1.X. There is no reason to support
HTTP/0.9, and we cannot safely handle a major protocol higher than 1 in
this code, so we should simply treat requests/responses claiming to be
of those protocols as errors.
Modifications:
HTTPDecoder now checks the major version is equal to 1 before it
continues with parsing. If it hits an error, that error will be propagated
out to the user.
Result:
Better resilience against bad HTTP messages.
Cory Benfield