fix(aws-sigv4): sign with the injected clock, not new Date() - #667
Merged
Conversation
The signer stamped `x-amz-date` from `amzDateOf(new Date())`, so SigV4 could not be tested on virtual time: 600 virtual seconds moved the shipped stamp 0 seconds, and a default `manualClock()` (which starts at epoch 0) still produced a real-time stamp. Every test wanting to assert anything about signing time had to inject its own clock-reading signer to measure it. #664 put the stitch's resolved `clock` on `AuthContext` for `oauth2`; this is the companion package taking the same seam. The signing timestamp is control-flow time by ADR 0010's own definition — `x-amz-date` is inside the string-to-sign and AWS refuses a stamp more than ~5 minutes out with `RequestTimeTooSkewed` — so it belongs on the clock that already drives retry, throttle, timeout, circuit and token freshness. It reads it through the identical `ctx.clock?.now() ?? Date.now()` fallback core's `auth.ts` uses, so a hand-built `AuthContext` in a custom strategy's unit test still type-checks. Nothing changes on the wire. The engine threads `systemClock` unless a clock was injected, and `systemClock.now()` IS `Date.now()`. Pinned by a test that signs on the wall clock, reads back the instant it stamped, re-signs on a clock pinned to that instant, and asserts the `Authorization` header is byte-identical, plus a golden signature cross-checked against an independent SigV4 implementation that reproduces the official `get-vanilla` vector. Payload hashing and the `signBody` branches are untouched. The mocking guide's clock map moves the SigV4 row from wall-clock to virtual, and the integrations page gains a "Signing time" section. Both note that an unseeded `manualClock()` signs `19700101T000000Z`. The package's `no-empty-function` suppression is dropped rather than raised: the two test contexts now share one non-empty no-op `emit`. Refs #658 (§1 hooks.onRequest ordering and §3 skew-403 circuit accounting are maintainer calls and remain open). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes §2 only of #658 — "SigV4 signs with
new Date()rather than the injected clock".Root cause
packages/aws-sigv4/src/index.ts:301stamped the signature withamzDateOf(new Date()). On real time that is correct, so nothing was ever wrong on thewire — but it put SigV4's one time-driven input outside the
Clockseam, which made thestrategy untestable on virtual time:
manualClock(), 0 of 3 calls were accepted — ~20,670 days of apparentskew, because the virtual clock starts at epoch while the server's validation reads real time.
The signing timestamp is control-flow time by ADR 0010's
own definition:
x-amz-dateis inside the string-to-sign, and AWS refuses a stamp more than~5 minutes out with
RequestTimeTooSkewed. It decides whether the call is accepted, so itbelongs on the same seam that already drives retry, throttle, timeout, circuit and OAuth2 token
freshness.
The fix — core's pattern, mirrored exactly
#664 landed
clock?: ClockonAuthContext, threaded by the engine from the same placeRuntime.clockcomes from. That is the seam this uses.packages/core/src/auth.ts:432consumesit as:
This package now carries the identical helper (
now()is core-internal, and core'snow = () => Date.now(), so the companion inlines that one call rather than spending a coreexport on it):
The whole behavioural change is one line —
amzDateOf(new Date())→amzDateOf(new Date(clockNow(ctx))).ctxwas already in scope. The optional field plus thefallback means a hand-built
AuthContextin a custom strategy's unit test still type-checks.AuthContextis imported as a type, from thestitchapipeer that is already required — nonew dependency, no structural copy of the type (the same reasoning the package's
Secretre-export already documents).
The real-time wire output is unchanged
This is a testability fix, not a wire fix. The engine threads
systemClockunless a clock wasinjected, and
systemClock.now()isDate.now(). Two tests pin it:ctx.clock),read back the instant it actually stamped, then re-sign the identical request on a clock
pinned to that instant.
x-amz-date,x-amz-content-sha256and the fullAuthorizationheader must match exactly. This test passes both before and after the change — that is
the point of it.
was cross-checked against an independent SigV4 implementation written from the AWS spec
against
node:crypto, which first reproduces the officialaws-sig-v4-test-suiteget-vanillavector (5fa00fa3…) to prove the oracle itself, then produces726c5c4879a6b4ccbbd3b24edbd6b8826d34f87450fbbf4e85546fc7ba9c1642for the header set thestrategy attaches. The package agrees.
Payload hashing is untouched, and the
signBodybranches from #401 — includingmultipart +
signBody: truestill throwing — are unchanged and still covered.The failing test
Written first, and confirmed failing against unmodified
src:5 failed | 18 passed. Thefive that failed were the clock assertions; the byte-identical wire pin was among the 18 that
passed, as it must be. After the one-line fix: 23 passed.
Coverage added, in
packages/aws-sigv4/test/sigv4.spec.ts:manualClock()signs19700101T000000Z— the ~20,670-day skew becomes somethinga test can see and assert rather than something it silently suffers.
the stamp is inside the signed canonical request, not cosmetic.
stitch()withclock: manualClock(...)andmockAdapter, asserting thex-amz-datethe transport actually receives, thenclock.advance(600_000)and asserting it moved ten virtual minutes. This is the seam theissue asked for, exercised the way a user would.
Bundle size — nothing moved
git diff origin/main -- packages/coreis empty. Not one byte was spent in core; theAuthContext.clockfield this needs already existed.stitchapi— whole entryimport { stitch }stitchapi/auth— whole surfaceIdentical to
main— same inputs, since core is untouched.@stitchapi/aws-sigv4is acompanion and is not part of core's gated entry. The
bundle-advertised-sizedrift tether isin sync (the pre-push
yakirrun reports 9 tethers, 0 drift).Docs — §2's second clause
§2's second ask was to "audit which time-driven features read the injected clock and which read
Date.now(), and state the answer in the testing guide." #664 already built that table, sothis PR does the remaining concrete thing: moves the SigV4 row from wall-clock to virtual.
The callout below the table still reads "the four core wall-clock rows" and is now more
accurate — before this there were five wall-clock rows, four of them core.
Also added: a Signing time section on the aws-sigv4 integration page with the
manualClockexample, and a note (in both places) that an unseeded
manualClock()signs19700101T000000Z,which a real endpoint answers with
RequestTimeTooSkewed— the same epoch-0trap the guidealready documents for HTTP-date
Retry-After.One non-drive-by cleanup, called out
The new test context has the same no-op
emitshape as the existingfakeCtx, which trips@typescript-eslint/no-empty-function— a rule this package had baselined atcount: 1.Rather than raise the baseline, both contexts now share one non-empty no-op (
(): void => undefined)and the suppression is dropped via
--prune-suppressions. The baseline file shrinks by 5lines; nothing was added to it. (Per #646, this package is now linted.)
What remains open on #658
hooks.onRequestruns after signing. Not touched. Its ask is a docs line plus a"Better:" proposal to emit an
infowhen anonRequesthook exceeds some duration thresholdon an
auth-carrying stitch. The threshold and whether to spend the bytes are maintainer calls.(a "client fault, don't count it" marker vs. documenting the exclusion), and it is entangled
with the absent-flag rule in
Surface.interpretthat the issue tracks across three sightings.refreshcannot see the response that triggered it — a public API change toAuthStrategy.refresh's signature. Out of scope by the "no half-baked API change" bar.it "defensible, and worth documenting". A core-engine docs claim, unrelated to this package;
landing it here would be a drive-by.
backoff.maxdefaults to 10 s — explicitly framed as the fourth silent-clamp in thepass, i.e. a pattern-level design decision, not a point fix.
worth keeping. Nothing to do.
Verification
All from the repo root, all passing:
check:lint(36 packages),check:types,test(1,477 core + 23 sigv4, whole workspace green),
check:format,check:contract,check:unknown-keys,check:types-d,check:size,check:changelog,check:docs-links,check:exports:companions,check:exports. The pre-push hook's full verify sequence also ranclean, including
build-docsand theyakirtethers.Refs #658
🤖 Generated with Claude Code