Skip to content

Match the documented parameter names to the signatures in NIOHPACK - #562

Merged
glbrntt merged 2 commits into
apple:mainfrom
darkdi:docs-param-names
Sep 1, 2026
Merged

glbrntt merged 2 commits into
apple:mainfrom
darkdi:docs-param-names

Conversation

@darkdi

@darkdi darkdi commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

The base64 helper looks like it came over from Foundation with Data(base64Encoded:options:) still in the doc comment, and the parameter is a Bool now. The other two seem to be plain leftovers. All three are internal, so DocC never builds them and nothing flags the mismatch. Nothing else touched.

@glbrntt glbrntt added the semver/none No version bump required. label Sep 1, 2026
@glbrntt
glbrntt enabled auto-merge (squash) September 1, 2026 05:14
glbrntt pushed a commit that referenced this pull request Sep 1, 2026
`isValidFieldName` validates a field name with `allSatisfy` over the
tchar set. `allSatisfy` returns `true` for an empty collection, so the
empty field name passes.

The file already states the rule it is implementing. The doc comment at
line 312 quotes:

```
token          = 1*tchar
```

The derivation that follows is correct — I expanded `0x21, 0x23...0x27,
0x2a, 0x2b, 0x2d, 0x2e, 0x30...0x39, 0x5e...0x7a, 0x7c, 0x7e` byte by
byte against tchar minus `A-Z` and every one of the four rewriting steps
in lines 315-341 holds, including the `%x5E-60 / %x61-7A` → `%x5E-7A`
collapse. It is the `1*` quantifier that is missing: the code implements
`*tchar`.

### Scope

All three block types accept an empty name today, since they share
`HeaderFieldName.init`:

```
request:  empty name -> accepted
response: empty name -> accepted
trailers: empty name -> accepted
```

An empty name serialises as `: value`, and RFC 9113 § 8.2.1 lists field
names as something a recipient is required to validate and treat as
malformed when the generic syntax is violated. This is a conformance gap
in a validator that already does the rest of that section — the
uppercase check, the CR/LF/NUL value check and the connection-specific
header check are all there.

(`":"` alone is already rejected, though as `unknownPseudoHeader` rather
than a name error, since `dropFirst()` leaves an empty base name that
matches no known pseudo-header. That path is unchanged here.)

### The change

An explicit early return, mirroring the shape swift-http-types uses for
the same production in `tokenValidity`:

```swift
if buffer.isEmpty {
    return .invalid
}
```

### Tests

Added to `HTTP2FramePayloadToHTTP1CodecCRLFTests.swift`, which already
exercises `validateRequestBlock`, `validateResponseBlock` and
`validateTrailersBlock` directly for the § 8.2.1 value rules. Five
cases: the empty name in each of the three block types, plus `"a b"` and
an ordinary `x-custom-header` to keep the surrounding behaviour pinned.

### Verification

**I could not run `swift test` here**: this machine has Command Line
Tools without XCTest, so the test targets fail at `import XCTest` with
`no such module`. Pre-existing and unrelated — it reproduces on an
unmodified checkout (18 occurrences). **CI should be the authority;
treat my local run as unverified.**

`swift build` succeeds. I linked a throwaway executable against
`NIOHTTP2` to exercise the change directly:

```
PASS request:  empty name       -> rejected
PASS response: empty name       -> rejected
PASS trailers: empty name       -> rejected
PASS request:  "a b"            -> rejected
PASS request:  "Host" (uppercase) -> rejected
PASS request:  valid name       -> accepted
PASS request:  pseudo-headers only -> accepted
```

Before the change the first three were accepted.

### Conflicts

No open PR touches `HPACKHeaders+Validation.swift`. #531 and #562 touch
NIOHPACK, and #531 (`+ Fix a bug`) is about `isASCII` in
`HPACKHeader.swift` — no overlap with `isValidFieldName`.
@glbrntt
glbrntt merged commit 0f3e54e into apple:main Sep 1, 2026
53 of 55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver/none No version bump required.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants