diff --git a/.changeset/is-expired-fail-closed.md b/.changeset/is-expired-fail-closed.md new file mode 100644 index 00000000..15d0b347 --- /dev/null +++ b/.changeset/is-expired-fail-closed.md @@ -0,0 +1,14 @@ +--- +"@agentcommercekit/vc": patch +--- + +Fix `isExpired` failing open on an unparseable `expirationDate` + +`isExpired` returned `false` (not expired) whenever `credential.expirationDate` +was present but could not be parsed into a valid date. Since `isExpired` is +the check `verifyParsedCredential` uses to reject expired credentials, a +credential with a malformed or malicious `expirationDate` value was treated +as never-expiring instead of being rejected. + +`isExpired` now fails closed: an unparseable `expirationDate` is treated as +expired, matching the safer default for a security-relevant check. diff --git a/packages/vc/src/verification/is-expired.test.ts b/packages/vc/src/verification/is-expired.test.ts index ac456e47..1e6bc808 100644 --- a/packages/vc/src/verification/is-expired.test.ts +++ b/packages/vc/src/verification/is-expired.test.ts @@ -47,9 +47,38 @@ describe("isExpired", () => { expect(isExpired(credential)).toBe(false) }) - it("handles invalid date strings gracefully", () => { + it("treats an unparseable expiration date as expired (fail closed)", () => { const credential = buildCredential("invalid-date") - expect(isExpired(credential)).toBe(false) + expect(isExpired(credential)).toBe(true) + }) + + it("treats a present but empty-string expiration date as expired (fail closed)", () => { + // An empty string is present (not `undefined`) but unparseable + // (`new Date("")` -> `NaN`). It must not be conflated with an absent + // `expirationDate` via a falsy check, or it silently fails open. + const credential = buildCredential("") + + expect(isExpired(credential)).toBe(true) + }) + + it("treats a non-string expiration date as expired (fail closed)", () => { + // `parseJwtCredential` does not validate that `expirationDate` is a + // string, so a malformed or untrusted credential can carry a number + // (or another non-string JSON value) here at runtime, bypassing the + // type system. `new Date(epochMs)` parses to a valid date, so without + // an explicit typeof check, a numeric value corresponding to a *future* + // date would incorrectly pass as "not expired" via the normal + // date-comparison path below - this must be rejected before it gets + // that far, regardless of which date it happens to encode. + const tenYearsFromNowMs = Date.now() + 10 * 365 * 24 * 60 * 60 * 1000 + + // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- models an untyped/malformed JWT payload + const credential = { + ...buildCredential(), + expirationDate: tenYearsFromNowMs, + } as unknown as W3CCredential + + expect(isExpired(credential)).toBe(true) }) }) diff --git a/packages/vc/src/verification/is-expired.ts b/packages/vc/src/verification/is-expired.ts index 1d66ac8e..b912f94f 100644 --- a/packages/vc/src/verification/is-expired.ts +++ b/packages/vc/src/verification/is-expired.ts @@ -3,19 +3,37 @@ import type { W3CCredential } from "../types" /** * Check if a credential is expired * + * Fails closed: a credential with an `expirationDate` that is present but + * cannot be parsed as a valid date is treated as expired, not as + * non-expiring. + * * @param credential - The {@link W3CCredential} to check - * @returns `true` if the credential is expired, `false` otherwise + * @returns `true` if the credential is expired (or has an unparseable + * expiration date), `false` otherwise */ export function isExpired(credential: W3CCredential): boolean { - if (!credential.expirationDate) { + if (credential.expirationDate === undefined) { return false } + // `parseJwtCredential` does not validate that `expirationDate` is a + // string, so a malformed or malicious credential could carry a number + // (which `Date()` accepts as epoch milliseconds) or another non-string + // JSON value here. Reject anything that isn't a string outright, rather + // than letting it reach `new Date()`, which would silently accept types + // the {@link W3CCredential} type only documents as a string. + if (typeof credential.expirationDate !== "string") { + return true + } + const expirationDate = new Date(credential.expirationDate) if (isNaN(expirationDate.getTime())) { - // Expiration date is invalid, so we consider the credential not expired - return false + // Expiration date is present but unparseable. Fail closed: an + // unparseable expiration date must not be treated as "never expires", + // since that would let a malformed or malicious `expirationDate` value + // grant a credential unbounded validity. + return true } return expirationDate < new Date() diff --git a/packages/vc/src/verification/is-revoked.ts b/packages/vc/src/verification/is-revoked.ts index 48255533..974938b9 100644 --- a/packages/vc/src/verification/is-revoked.ts +++ b/packages/vc/src/verification/is-revoked.ts @@ -411,9 +411,12 @@ async function resolveStatusListCredential( ) } - // Check the expiry directly rather than through `isExpired`, which reads an - // unparseable date as "not expired". The expiry is the main bound on status - // list replay, so a malformed one must not quietly remove it. + // Check the expiry directly rather than through `isExpired`: this path + // must throw `undetermined()` with a URL-specific message (isExpired only + // returns a boolean), and must reject a list that expires at exactly `now` + // (`<=`), whereas isExpired's own bound is strict (`<`). The expiry is the + // main bound on status list replay, so a malformed one must not quietly + // remove it. if (verified.expirationDate !== undefined) { const expiresAt = Date.parse(verified.expirationDate)