Motivation
We shouldn't crash on somewhat likely user error.
Modifications
Pass on writes after close(mode: .output) instead of crashing.
Result
User code is more robust to weird edge cases.
Motivation:
Currently the server pipeline handler ignores close. This generally works,
except in the rare case that the user calls close(mode: .output). In this
instance they have signalled that they'll never write again, and they're
likely expecting a final close shortly after.
However, it is possible that the pipeline handler has suspended reads
at the same time. On Linux this isn't an issue, because we'll still be told
about the eventual socket close. However, on Apple platforms we won't: we've
masked off the reads, and we can't listen to EVFILT_EXCEPT due to some
other issues. This means that on Apple platforms the server pipeline handler
can accidentally wedge the Channel open and prevent it from closing.
We should take this opportunity to have the server pipeline handler be smart
about close(mode: .output). What _should_ happen here is that the pipeline
handler should immediately refuse to deliver further requests on the Channel.
If one is in-flight, it can continue, but everything else should be dropped.
This is because the server cannot possibly respond to further requests.
Modifications:
- Add new states to the server pipeline handler
- Drop buffered requests and new data after close(mode: .output)
- Add tests
Result:
Server pipeline handler behaves way better.
Motivation:
#fileID introduced in Swift 5.3, so no longer need to use #file anywhere
Modifications:
Changed #file to #filePath or #fileID depending on the situation
Motivation:
The `HTTPServerUpgradeHandler` removes itself from the pipeline after an
upgrade and unbuffers any unconsumed reads. If an upgrade starts but
does not complete successfully then the pipeline may be left in an
unknown state.
If, for example, the failure occurs before the user provided upgrade
handler is run then unbuffered writes may be unwrapped by the wrong
channel handler as the wrong type leading to a crash.
Modifications:
- Only remove the upgrade handler and forward buffered writes if all
parts of the upgrade complete successfully.
- If part of the upgrade fails then fire an error into the channel
pipeline without removing the server upgrade handler.
- Remove a few unnecessary `map`s.
- Make `httpEncoder` non-optional since and remove associated dead code
since it can never be `nil`.
Result:
Upgrade handling is safer.
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.
Workaround for https://github.com/apple/swift/issues/59911
```swift
/src/Tests/NIOHTTP1Tests/HTTPTest.swift:148:21: warning: 'buf' mutated after capture by sendable closure
buf.writeString("\(c)")
```
The variable `buf` is mutated but only before being capture by the sendable closure.
All other warnings/errors are fixed with the latest Swift 5.7 toolchain
* Adopt `Sendable` for closures in `HTTPPipelineSetup.swift`
* make `UnsafeTransfer` and `UnsafeMutableTransferBox` available in Swift 5.4 too
* fix code duplication
* Copy `UnsafeTransfer` to `NIOHTTP1Tests` to be able to remove `@testable` from `NIOCore` import
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:
Follow up to https://github.com/apple/swift-nio/pull/1952#discussion_r707351015
The OWS rule from the HTTP semantics draft only considers 'SP' and
'HTAB' to be whitespace. We also (unnecessarily) consider CR and LF to
be whitespace.
Modifications:
- Remove CR and LF from the characters we consider to be whitespace
- Update tests
Result:
We no longer consider CR or LF to be whitespace when trimming
whitespace when producing the canonical form of header values.
Motivation:
The remaining NIO code really conceptually belongs in a module called
NIOPosix, and NIOCore should really be called NIO. We can't really do
that last step, but we can prepare by pushing the bulk of the remaining
code into a module called NIOPosix.
Modifications:
- Move NIO to NIOPosix
- Make NIO an umbrella module.
Result:
NIOPosix exists.
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>
* issue-1036: Fix for dates on Linux test generation script.
Motivation:
Fix for issue-1036 to add a date or date range for the header.
These dates are derived from the git log on the file.
Modifications:
Parsing method to extract the date or date range from the git log
of the file. This was added to generate_linux_tests.
Result:
Dynamic date or date range added to the file header.
* issue-1036: Added revised headers from generation script
* Regenerating dates on XCTests.
Motivation:
Addressing feedback from PR.
Modifications:
All of the XCTests now have -2021 as a header date attached to them.
Result:
Dates modified on the XCTests.
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
The HTTP server upgrade tests had unncessarily wide and also verbose
test assertions.
Modifications:
Rewrote them much simpler and narrowed what states are allowed.
Result:
- Neater code
- Works around https://bugs.swift.org/browse/SR-14253
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.
* HTTPServerObjectAggregator for requests
* Apply suggestions from code review
Co-authored-by: Cory Benfield <lukasa@apple.com>
* most of code review comments addressed
* tidy up state machine close
* bad line breaks
* more verbose error reporting
* removes expect:continue functionality due to #1422
* public type naming and error handling tweaks
* wrong expectation in test
* do not quietly swallow unexpected errors in channelRead
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
The fullFilePath() hack doesn't actually work
(https://bugs.swift.org/browse/SR-12934). This is expected behaviour so
currently, the only workaround would be to `#if` whole function bodies
which is a lot of work. Given that in Swift 5.3 `#file` == `#filePath`
still, we can just silence the warning for now.
Modifications:
- Replace `fullFilePath()` by `(#file)`.
Result:
- file locations working properly again.
Co-authored-by: George Barnett <gbarnett@apple.com>
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:
Very recent versions of the Swift compiler complain if you pass `#file`
to a parameter that is defaulted to `#filePath`. You can silence the
warning by using `(#file)`. We cannot migrate to `#filepath` because we
support Swift 5.0 and 5.1 which don't have `#filepath` yet.
Modifications:
- replace all occurances of `#file` by a special (test `internal`)
function `fullFilePath()` which gives you the full path and is defined
differently depending on the compiler version.
Result:
No warnings even on the latest compiler versions.
Motivation:
Previously, if the user sent us a 0 length chunk, we would encode it as
`0\r\n\r\n` which means the _end_ of the body in chunked encoding. But
the end in NIO is send by sending `.end(...)`.
Modifications:
Don't write anything for 0 length writes.
Result:
More correct behaviour with 0 length body chunks.
Motivation:
The names of most of the BSD socket option values were brought over into
their NIO helper properties, with their namespace removed. This vastly
increases the risk of collision, particularly for things like TCP_INFO,
which just became .info.
Modifications:
- Added the prefixes back, e.g. `.info` is now `.tcp_info`.
- Renamed socket type `.dgram` to `.datagram`, as this is the nice clean
API and we should use nice clean names.
Result:
Better APIs
* NIO: Add new `BSDSocket` namespace
This starts the split of the POSIX/Linux/Darwin/BSD interfaces and the
BSD Socket interfaces in order to support Windows. The constants on
Windows are not part of the C standard library and need to be explicitly
prefixed. Use the import by name to avoid the `#if` conditions on the
wrapped versions. This will allow us to cleanly cleave the dependency
on the underlying C library.
This change adds a `BSDSocket.OptionLevel` and `BSDSocket.Option` types
which allow us to get proper enumerations for these values and pipe them
throughout the NIO codebase.
* Apply suggestions from code review
Co-Authored-By: Cory Benfield <lukasa@apple.com>
Motivation:
It's not always possible to initialize `HTTPHeaders` with all the headers one needs to set. In such case, the underlying array will be expanded as and when needed upon headers being appended. `Array` provides the means to reserve capacity to optimize such use case. This PR exposes this functionality in `HTTPHeaders`.
Modifications:
Expose method `reserveCapacity(_ minimumCapacity: Int)` and computed variable `capacity: Int` in `HTTPHeaders` and added some tests.
Result:
Developers will be able to control the capacity of the underlying headers array as such:
```
var headers = HTTPHeaders()
headers.reserveCapacity(5)
```
Co-authored-by: Ludovic Dewailly <ldewailly@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:
When initializing an `HTTPMethod` with a `String` we currently switch over all expected values to pick an explicit value, only to overwrite our found explicit value with a catchall `RAW(value: String)`. This doesn’t make a ton of sense. This went unnoticed because we use the conditional conformance on `RawRepresentable` to be `Equatable`. But `switch`es don’t use the `Equatable` protocol to match enums. The issue is visible there.
Modifications:
Added a testcase with a switch that fails, if the `.RAW(value: String)` value is used for `”GET”`. Removed the final overwrite in HTTPMethod(rawValue: String). Therefore the test passes.
Result:
We can initialize the HTTPMethod with a String and get an explicit value.
Motivation:
We incorrectly asserted that HEAD and DELETE may not have request
bodies, but they can, it's just unlikely.
Modifications:
Change the categorisation of HEAD and DELETE.
Result:
Better behaviour in unusual cases.
Motivation:
In some cases you only expect (or care about) a single value for a
header. However, getting this value without allocating an array or
traversing the whole array requres using first(where:) provided by
Sequence. This is not very ergonomic as it requires users to do the
case-insensitive comparison and then exctact the value from the
HTTPHeaders. Additionally, users cannot use NIO's case insensitive
ASCII comparison.
Modifications:
- Provide a first(name:) method on HTTPHeaders which returns the value
from the first header whose name is a case insentive match of the
subscript parameter.
Result:
It's easier to get the first value from HTTPHeaders matching a given
name.
Motivation:
Fixes the bug described in #1279
Modifications:
Moved state change in NIOHTTPClientUpgradeHandler.performUpgrade to after firing buffered messages.
Result:
- The upgradeState will now only change to upgradeComplete after buffered reads are handeld.
- Fixes#1279
Motivation:
It's important to also test deprecated functionliaty. One way of
achieving this without warnings is to also deprecate the tests that test
this deprecated functionality. Unfortunately, on Linux we need to
generate lists of tests which would then reference deprecated tests
(which gives us a warning).
Modifications:
Deprecate test suites and the main test runner all the way to the top so
never get warnings.
Result:
Possible to test deprecated functionlity without warnings.
Motivation:
writeWithUnsafeMutableBytes cannot tolerate arbitrarily large writes because it is not possible to resize the buffer by the time body receives the buffer pointer. This change provides an option for the user to reserve writable capacity before the pointer is passed to body.
Modifications:
Added a backward compatible optional parameter to writeWithUnsafeMutableBytes which can reserve writable capacity before the pointer is passed to body.
Result:
Additive change only. Users can optionally specify a minimum writable capacity when calling writeWithUnsafeMutableBytes.
Motivation:
I ran across a case in a test where `HTTPResponseStatus(statusCode: 418)`
unexpectedly returned a `.custom` response status.
Modifications:
- Add a missing `case` for creating a status from `418`
- Shuffled some of cases around so that they are correctly listed in the
correct 2xx, 3xx, 4xx etc. sections
Result:
- `HTTPResponseStatus(statusCode: 418)` now returns `.imATeapot`
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:
There is currently a pair of windows in which it is possible for the
HTTPServerUpgradeHandler to buffer some data without replaying it. This
can happen if it is reentrantly called just after it finishes
unbuffering data, which is not great.
To avoid this, we move to execute this data delivery while we're
removing ourselves. This ensures that we are confident that we cannot be
reentrantly called after our last outcall, meaning we can be confident
of 100% data delivery.
Modifications:
- Moved delayed data delivery to removeHandler.
Result:
Less chance of quiet data loss
Motivation:
Without niling out it may linger around once the last test has
completed, and keep some messages in memory which will not be inspected
anymore
Modifications:
nil out the writeRecorder same as all other fields are currently nilled
out
Result:
In case XCTest keeps the class around, not keeping around the messages
stored by the writeRecorder
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.
Cory Benfield