Skip to content

fix: join duplicate header values with "," rather than ", " - #1340

Merged
quinnj merged 4 commits into
masterfrom
jq/appendheader-comma-no-ows
Jul 29, 2026
Merged

fix: join duplicate header values with "," rather than ", "#1340
quinnj merged 4 commits into
masterfrom
jq/appendheader-comma-no-ows

Conversation

@quinnj

@quinnj quinnj commented Jul 29, 2026

Copy link
Copy Markdown
Member

appendheader merges an adjacent duplicate header entry into the one before it, joining the values with ", ". It should join with ",".

HTTP.Request("GET", "/", ["My-Header1" => "value2", "My-Header1" => "value1"])
# before: "My-Header1" => "value2, value1"
# after:  "My-Header1" => "value2,value1"

Why

RFC 9110 §5.3 permits either form when combining repeated field lines — "separated by a comma (,) and optional whitespace (OWS)" — so both are legal on the wire. But AWS SigV4 and Azure SharedKey request canonicalization both require exactly "," with no whitespace.

The important part is where this runs: mkheaders calls appendheader on the request construction path, not just when parsing responses. So this is not a cosmetic difference in received headers — any library that signs cloud requests using HTTP.jl computes an incorrect Authorization signature whenever the outgoing request carries duplicate headers. The signer cannot correct for it after the fact, because by the time it sees the request the values are already merged.

Evidence

Checked against the official AWS SigV4 conformance suite. The two duplicate-header cases produce the published expected signatures with "," and do not with ", ":

case ", " ","
get-header-key-duplicate signature mismatch matches published vector
get-header-value-order signature mismatch matches published vector

Scope

Deliberately unchanged:

  • Set-Cookie remains exempt from merging.
  • Merging is still only performed for an immediately adjacent duplicate — non-adjacent entries are left as separate entries, as before.

Tests

Existing assertions that encoded the ", " form are updated — all of them are direct expectations of merged header values, listed here so they can be checked rather than taken on trust:

  • test/http_core_tests.jlX-Test merge, two x-forwarded-for assertions, and the Accept comma-merge case
  • test/http1_wire_tests.jl — the X-Forwarded-For wire assertions

Added a regression testset covering two adjacent duplicates, three adjacent duplicates, the mkheaders path (both the vector-of-pairs and vector-valued-dict forms), non-adjacent duplicates staying unmerged, and Set-Cookie never merging.

CI hardening

The CI failures were unrelated to the header separator. This PR now hardens three independent timing and error-shape assumptions.

Reseau timeout wrapper

CI now resolves Reseau 1.3.2 or later. These releases preserve a caller-owned TLS deadline as a nested TLSError with a DeadlineExceededError cause. HTTP then wraps that error in HTTP.TLSHandshakeError.

The timeout test helper already followed the Reseau wrapper, but it stopped at the HTTP wrapper. It now follows both layers. This keeps the test compatible with the direct HTTP.TimeoutError from Reseau 1.3.1 and the nested deadline error from Reseau 1.3.2 or later.

Cold precompile workload

The local precompile workload used 1-second and 5-second request deadlines. Those deadlines include cold native compilation. A fresh macOS ARM runner needed more than 5 seconds to compile and run the first local TLS path.

The workload now uses finite 60-second deadlines. This keeps the anti-hang protection while allowing slow cold compilation.

Retry refund test

The retry refund test used a 1-second request deadline. Cold Windows compilation could consume that deadline before the first local response. Its retry delay was also random from 0 to 60 seconds, so the test did not always force deadline preemption.

The test now returns Retry-After: 60 and uses a 10-second request deadline. The retry layer rejects the fixed delay immediately because it exceeds the remaining deadline. The test does not wait for 60 seconds.

Local validation:

  • test/http_core_tests.jl
  • test/http1_wire_tests.jl
  • test/http_client_tests.jl
  • test/precompile_tests.jl, three consecutive runs
  • test/http_retry_tests.jl, five consecutive runs
  • Full Pkg.test(), including all 63 trim-compilation checks

Version bumped to 2.6.0 — behaviour-visible, so a minor rather than a patch.

🤖 Generated with Claude Code

Co-authored by Codex

appendheader merges an adjacent duplicate header entry into the preceding one.
It joined the values with ", " - a comma plus optional whitespace. RFC 9110
Section 5.3 permits either form when combining repeated field lines, but AWS
SigV4 and Azure SharedKey request canonicalization both require exactly ","
with no whitespace.

mkheaders calls appendheader on the request construction path, so this affects
outgoing requests, not just parsed responses: any library signing cloud requests
with HTTP.jl computes an incorrect signature whenever a request carries
duplicate headers. Verified against the official AWS SigV4 conformance suite -
the get-header-key-duplicate and get-header-value-order cases produce the
published expected signatures with "," and do not with ", ".

Set-Cookie remains exempt from merging, and merging is still only performed for
an immediately adjacent duplicate; neither behavior changes here.

Existing assertions that encoded the ", " form are updated, and a regression
testset covers two and three adjacent duplicates, the mkheaders path,
non-adjacent duplicates staying unmerged, and Set-Cookie never merging.
@quinnj

quinnj commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

CI update

The original header change was not the cause of the failures. I traced and hardened three independent CI assumptions on this branch:

  • Reseau 1.3.2 or later nests a caller-owned TLS deadline below HTTP.TLSHandshakeError. The timeout helper now follows that wrapper.
  • Cold local TLS compilation on macOS ARM exceeded the precompile workload's 5-second budget. The workload now keeps finite 60-second deadlines.
  • The retry refund test used a 1-second first-response budget and a random 0-to-60-second retry delay. It now uses Retry-After: 60 with a 10-second request budget.

Exact head cfd8ba3e385988f4a731e3ca212048259a245ab5 is green:

  • Documentation
  • Julia 1 on Ubuntu, Windows, and macOS ARM
  • Julia pre on Ubuntu and Windows
  • Codecov patch and project

Local validation also passed:

  • Full Pkg.test(), including all 63 trim-compilation checks
  • Core, wire, and client test files
  • Precompile test file, three consecutive runs
  • Retry test file, five consecutive runs

quinnj added 3 commits July 29, 2026 12:10
Reseau 1.3.2 preserves caller-owned TLS deadline errors as a TLSError. HTTP wraps that error again as TLSHandshakeError, so the timeout test helper must traverse both layers.

Also describe the bare-comma header form as RFC-permitted instead of the RFC canonical form.
Use a fixed 60-second Retry-After value so the abandoned-retry path is deterministic. Give the first local response a 10-second request budget so cold Windows compilation cannot consume the deadline before the behavior under test starts.
@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.28%. Comparing base (cb98f26) to head (cfd8ba3).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1340      +/-   ##
==========================================
+ Coverage   88.20%   88.28%   +0.08%     
==========================================
  Files          30       30              
  Lines       11921    11921              
==========================================
+ Hits        10515    10525      +10     
+ Misses       1406     1396      -10     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@quinnj
quinnj merged commit e7fbc1f into master Jul 29, 2026
8 checks passed
@quinnj
quinnj deleted the jq/appendheader-comma-no-ows branch July 29, 2026 18:53
quinnj added a commit to JuliaServices/CloudBase.jl that referenced this pull request Jul 29, 2026
Bumps the floor to HTTP 2.6, which carries the appendheader fix (JuliaWeb/HTTP.jl#1340)
joining duplicate header values with "," rather than ", ". That was the last blocker
on AWS SigV4 canonicalisation: the full conformance suite now passes, including the
get-header-key-duplicate and get-header-value-order cases.

Also in this commit:
- awssignv2! no longer assigns request.body. HTTP 2 parameterises Request on its body
  type, so the body cannot be swapped in place; the signed form-encoded body is returned
  for the caller to send instead.
- SigV2 GET now emits an origin-form target ("/?query" rather than a bare "?query"),
  which is what a request-target requires. The signature itself is unchanged.
- cloudlayer splits CloudBase's signing kwargs (service, region, x_amz_date, ...) from
  the ones bound for HTTP. HTTP 1's layers absorbed them; HTTP 2 validates its keyword
  arguments and rejects unknown ones.

Still failing: the emulator-backed testsets. minio bucket creation fails, so the live
request path needs another pass - unit-level signing is correct but something in the
end-to-end request differs. Not yet ready to merge.
quinnj added a commit to JuliaServices/CloudBase.jl that referenced this pull request Aug 4, 2026
* fix: AWS SigV4 endpoint parsing and signing, Azure credential and SAS correctness

AWS:
- urlServiceRegion counted labels from the front of the host, so a bucket name
  containing dots was mistaken for the service/region: my.bucket.s3.amazonaws.com
  resolved to service=bucket, region=s3 and signed against the wrong credential
  scope. Matching is now anchored at the amazonaws.com suffix, which also handles
  the bucket.s3.amazonaws.com form that previously returned service=bucket.
- The service===nothing guard constructed an ArgumentError but never threw it, so
  an unrecognised host failed later inside signing as MethodError(lowercase, Nothing).
- awssign!(...; debug=true) referenced access_key_id/secret_access_key/session_token,
  none of which exist in that scope; the debug path always threw UndefVarError.
- The canonical query string sorted by the concatenation of name and value, which
  cannot distinguish ab=c from a=bc, so the same query set could sign two ways.
  Sort by the (name, value) pair as SigV4 specifies.
- loadRoleArn built a Dict{String,String} then assigned an Int (DurationSeconds)
  and file bytes (WebIdentityToken), so both AssumeRole paths threw on convert.
  The web identity path is how EKS pods obtain credentials.
- deduplicateHeaders! indexed first(headers) before checking for emptiness.
- bytes() wrapped a String's buffer with unsafe_wrap and no GC.@preserve, while
  callers pass temporaries such as bytes("AWS4$secret").

Azure:
- azureLoadConfig! passed the raw expires_on value to unix2datetime, but IMDS
  returns it as a JSON string and Figgy parses JSON scalars as Strings, so VM
  credential loading threw. Normalise via azureExpiration.
- reloadAzureVMCredentials!() defaulted vmHost to nothing, which has no matching
  AzureVMCredentialsSource constructor.
- ContentType's field was named rscl, colliding with ContentLanguage: a contentType
  was emitted under the wrong query key and rsct was never emitted, while the
  string-to-sign still signed it as rsct, so signature and query disagreed.
- generateUserDelegationSASToken interpolated the (resource, service) tuple from
  getCanonicalizedResource into the string-to-sign instead of destructuring it.
- The URI/String user-delegation entry points called a (::URI, ::String) method
  that does not exist, so every one of them raised MethodError.
- parseAzureAccountContainerBlob called String(nothing) when the optional service
  group did not participate, e.g. for azure://account/container.

* fix(signing): preserve canonical request semantics

* wip: HTTP 2 migration - trace-based signing/metrics, FunctionWrappers removed

Compat moved to HTTP = "2". CloudBase loads and signs on HTTP 2.5.5 + Reseau.

- cloudsignlayer and cloudmetricslayer are replaced by a single cloudlayer(provider)
  that installs an HTTP 2 trace <subcommand> [options] - record system behavior

trace record: record a trace file
    $ trace record myworkload
        [... Ctrl-C to stop ...]
    $ trace record myworkload --Logging:enable-logs --end-after-duration 5s
    $ trace record myworkload --plan profile --omit Symbolication
    $ trace record myworkload --end-on-notification stop-myworkload-trace
        [... elsewhere `notifyutil -p stop-myworkload-trace` ...]
    $ trace record /tmp/trace-path.atrc --compress

trace amend: add data to a file
    $ trace amend myworkload-003.atrc --add Symbolication

trace trim: trim a file based on kdebug event times
    $ trace trim myworkload-002.atrc --from +1s --to +2s

trace providers: print information about Logging, Symbolication, etc.

trace plans: print detailed information about tracing approaches

See `man trace` for more information. callback. Signing runs on RequestEvent, which HTTP
  emits before *every* attempt, so retries are re-signed with a fresh timestamp - the
  guarantee the old stream layer provided. A caller-supplied trace <subcommand> [options] - record system behavior

trace record: record a trace file
    $ trace record myworkload
        [... Ctrl-C to stop ...]
    $ trace record myworkload --Logging:enable-logs --end-after-duration 5s
    $ trace record myworkload --plan profile --omit Symbolication
    $ trace record myworkload --end-on-notification stop-myworkload-trace
        [... elsewhere `notifyutil -p stop-myworkload-trace` ...]
    $ trace record /tmp/trace-path.atrc --compress

trace amend: add data to a file
    $ trace amend myworkload-003.atrc --add Symbolication

trace trim: trim a file based on kdebug event times
    $ trace trim myworkload-002.atrc --from +1s --to +2s

trace providers: print information about Logging, Symbolication, etc.

trace plans: print detailed information about tracing approaches

See `man trace` for more information. is composed rather
  than displaced.
- HTTP 2's Request has no , so awssign!/awssignv2!/azuresign!/gcpsign! now take
  the URI explicitly; the absolute URL comes from the trace event.
- Request bodies are typed objects and HTTP.isbytes is gone, so signing reads payload
  bytes through a new requestbodybytes helper.
- HTTP.Header no longer exists; canonicalHeader takes a Pair.
- FunctionWrappers is dropped entirely. PREREQUEST_CALLBACK/METRICS_CALLBACK Refs are
  replaced by plain prerequest/metrics functions users add methods to. The metrics
  signature loses the error-category counters and connect/read/write durations, which
  HTTP 2 no longer exposes, rather than reporting them as zeros.

KNOWN ISSUE - not ready to merge. Two AWS SigV4 conformance cases fail
(get-header-key-duplicate, get-header-value-order; 22 pass, 4 pre-existing broken).
HTTP 2 merges duplicate request headers at construction and joins them with ", ",
whereas SigV4 canonicalisation requires ",". deduplicateHeaders! used to perform that
join itself and so controlled the separator. Because the merge happens before CloudBase
sees the request, a genuine single header value containing ", " is now
indistinguishable from two merged headers, so this cannot be fixed by string rewriting
without risking corrupting legitimate values. Needs a way to see pre-merge headers, or
confirmation of how AWS expects an already-merged value to canonicalise.

* HTTP 2.6 migration: SigV4/SigV2 conformance green

Bumps the floor to HTTP 2.6, which carries the appendheader fix (JuliaWeb/HTTP.jl#1340)
joining duplicate header values with "," rather than ", ". That was the last blocker
on AWS SigV4 canonicalisation: the full conformance suite now passes, including the
get-header-key-duplicate and get-header-value-order cases.

Also in this commit:
- awssignv2! no longer assigns request.body. HTTP 2 parameterises Request on its body
  type, so the body cannot be swapped in place; the signed form-encoded body is returned
  for the caller to send instead.
- SigV2 GET now emits an origin-form target ("/?query" rather than a bare "?query"),
  which is what a request-target requires. The signature itself is unchanged.
- cloudlayer splits CloudBase's signing kwargs (service, region, x_amz_date, ...) from
  the ones bound for HTTP. HTTP 1's layers absorbed them; HTTP 2 validates its keyword
  arguments and rejects unknown ones.

Still failing: the emulator-backed testsets. minio bucket creation fails, so the live
request path needs another pass - unit-level signing is correct but something in the
end-to-end request differs. Not yet ready to merge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant