Conversation
There was a problem hiding this comment.
Pull request overview
Adds certificate chain validation support and accompanying tests/fixtures to ensure C2PA signing credentials are validated against configured trust anchors.
Changes:
- Introduces a global
TrustListfor configuring trust anchors and a helper to load PEM trust lists in tests. - Updates COSE signature validation to build/verify X.509 chains (leaf → intermediates → trusted root), including loop detection.
- Adds extensive end-to-end tests for chain validation scenarios and new trust-list fixtures.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/utils/testCertificates.ts | Adds trust-list configuration hook and new expected status entry helper variants for chain validation outcomes. |
| tests/utils/set-trust-list.ts | Adds helper to load a PEM trust list fixture and configure global trust anchors for tests. |
| tests/trust-list.test.ts | Adds fixture-driven validation tests with an intentionally “wrong” trust list configured. |
| tests/asset-reading.test.ts | Ensures a default trust list is set before running asset reading/validation tests. |
| tests/certificate-chain.test.ts | Adds an end-to-end certificate chain validation test suite with dynamically generated chains. |
| tests/fixtures/trust-list.pem | Adds a default trust list fixture. |
| tests/fixtures/trust-list-wrong.pem | Adds an alternate trust list fixture used to validate “untrusted” behavior. |
| src/cose/index.ts | Re-exports TrustList from the COSE module. |
| src/cose/TrustList.ts | Adds a global trust anchor store and PEM decoding for bundled certs. |
| src/cose/Signature.ts | Integrates chain validation using TrustList and adds chain-building + verification helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| while ((match = pattern.exec(pem)) !== null) { | ||
| const base64 = match[1].replace(/\r?\n|\s/g, ''); | ||
| try { | ||
| out.push(Uint8Array.fromBase64(base64)); |
There was a problem hiding this comment.
Uint8Array.fromBase64 is not available in standard Node.js runtimes, which will break trust anchor parsing outside Bun. Prefer decoding via Buffer.from(base64, 'base64') (or a small base64 helper) and then converting to Uint8Array.
| out.push(Uint8Array.fromBase64(base64)); | |
| const derBuffer = Buffer.from(base64, 'base64'); | |
| out.push(new Uint8Array(derBuffer)); |
| }; | ||
|
|
||
| beforeAll(async () => { | ||
| await setTrustList('tests/fixtures/trust-list-wrong.pem'); |
There was a problem hiding this comment.
This suite sets a wrong trust list globally, but testFiles includes many cases marked valid: true. If validation now depends on trust anchors, this setup will likely flip previously-valid fixtures to untrusted/invalid and make the expectations inconsistent. Consider using the correct trust list fixture here, or explicitly adjusting expected validity/status codes for the 'wrong trust list' scenario.
| await setTrustList('tests/fixtures/trust-list-wrong.pem'); | |
| await setTrustList('tests/fixtures/trust-list.pem'); |
| export async function setTrustList(trustListFile: string = DefaultTrustListPath): Promise<void> { | ||
| const trustListData = (await fs.readFile(trustListFile)).toString(); | ||
| TrustList.setTrustAnchors([trustListData]); |
There was a problem hiding this comment.
This mutates global trust anchors (TrustList.setTrustAnchors), which can introduce test flakiness if Bun runs test files or describes concurrently. Since the production API now supports ValidationOptions.trustAnchors, prefer passing per-test trust anchors into ManifestStore.validate(asset, { trustAnchors: ... }) (or similar) instead of relying on global mutable state.
| export async function setTrustList(trustListFile: string = DefaultTrustListPath): Promise<void> { | |
| const trustListData = (await fs.readFile(trustListFile)).toString(); | |
| TrustList.setTrustAnchors([trustListData]); | |
| export async function setTrustList(trustListFile: string = DefaultTrustListPath): Promise<string[]> { | |
| const trustListData = (await fs.readFile(trustListFile)).toString(); | |
| return [trustListData]; |
| export const DefaultTrustListPath = 'tests/fixtures/trust-list.pem'; | ||
|
|
||
| export async function setTrustList(trustListFile: string = DefaultTrustListPath): Promise<void> { |
There was a problem hiding this comment.
Constant naming is inconsistent with typical TS conventions. Consider either DEFAULT_TRUST_LIST_PATH (SCREAMING_SNAKE_CASE for constants) or defaultTrustListPath (camelCase) to avoid it looking like a type/class.
| export const DefaultTrustListPath = 'tests/fixtures/trust-list.pem'; | |
| export async function setTrustList(trustListFile: string = DefaultTrustListPath): Promise<void> { | |
| export const DEFAULT_TRUST_LIST_PATH = 'tests/fixtures/trust-list.pem'; | |
| export async function setTrustList(trustListFile: string = DEFAULT_TRUST_LIST_PATH): Promise<void> { |
| extensions: [ | ||
| new BasicConstraintsExtension(false, 2, true), | ||
| new ExtendedKeyUsageExtension([ExtendedKeyUsage.emailProtection], true), | ||
| new KeyUsagesExtension(KeyUsageFlags.digitalSignature, true), | ||
| await SubjectKeyIdentifierExtension.create(intermediateKeys.publicKey, false), | ||
| await AuthorityKeyIdentifierExtension.create(wrongRootKeys.publicKey, false), | ||
| ], |
There was a problem hiding this comment.
In the 'should detect invalid signature' test, the intermediate certificate is also made structurally invalid (e.g., BasicConstraintsExtension(false, ...) and key usages missing CA bits). That can cause the chain to fail for reasons other than the intended signature mismatch, weakening the test's isolation. Consider generating a valid intermediate (e.g., via getIntermediateExtensions(...)) and only altering the signing key / issuer relationship to make the signature invalid.
| extensions: [ | |
| new BasicConstraintsExtension(false, 2, true), | |
| new ExtendedKeyUsageExtension([ExtendedKeyUsage.emailProtection], true), | |
| new KeyUsagesExtension(KeyUsageFlags.digitalSignature, true), | |
| await SubjectKeyIdentifierExtension.create(intermediateKeys.publicKey, false), | |
| await AuthorityKeyIdentifierExtension.create(wrongRootKeys.publicKey, false), | |
| ], | |
| // Use the same extensions as the valid intermediate to keep it structurally correct; | |
| // only the signature (produced with the wrong root key) is invalid. | |
| extensions: intermediateCert.extensions, |
| // Loop detection | ||
| if (seen.has(issuer.subject)) { | ||
| return ValidationStatusCode.SigningCredentialUntrusted; | ||
| } | ||
|
|
||
| seen.add(issuer.subject); | ||
| current = issuer; |
There was a problem hiding this comment.
Loop detection keyed by issuer.subject can produce false positives if different certificates share the same subject DN (common in rotated/intermediate reissues). Use a stable unique identifier instead (e.g., certificate thumbprint, raw DER hash, or SKI value when present) to avoid incorrectly rejecting valid chains.
| private static async validateChainCertificate( | ||
| current: X509Certificate, | ||
| issuer: X509Certificate, | ||
| timestamp: Date, | ||
| ): Promise<boolean> { |
There was a problem hiding this comment.
validateChainCertificate reduces all non-trusted cases to a boolean, which causes validateChain to collapse diverse failures (expired, invalid extensions, etc.) into SigningCredentialUntrusted. Since validateCertificate returns specific ValidationStatusCodes, consider returning/propagating those codes instead of boolean so callers can report more accurate validation outcomes.
| // Validate certificate and timestamp for the issuer | ||
| const validateCertificate = await Signature.validateCertificate(issuer, timestamp, false); | ||
| if (validateCertificate !== ValidationStatusCode.SigningCredentialTrusted) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
validateChainCertificate reduces all non-trusted cases to a boolean, which causes validateChain to collapse diverse failures (expired, invalid extensions, etc.) into SigningCredentialUntrusted. Since validateCertificate returns specific ValidationStatusCodes, consider returning/propagating those codes instead of boolean so callers can report more accurate validation outcomes.
| /** | ||
| * Applies targeted modifications to an extensions array, allowing tests to | ||
| * replace or remove individual extensions by index. | ||
| * | ||
| * - If the value for an index is an {@link Extension}, it **replaces** the | ||
| * extension at that position. | ||
| * - If the value is `undefined`, the extension at that position is **removed** | ||
| * (via `splice`). | ||
| * | ||
| * @param extensions - The original ordered array of extensions. | ||
| * @param changes - A map of `constructor.name → replacement | undefined`. | ||
| * @returns The mutated extensions array. | ||
| */ |
There was a problem hiding this comment.
The docstring says replacements/removals happen 'by index', but the implementation uses constructor.name matching (per changes map). Update the comment to match the actual behavior to avoid confusion in future test additions.
| public static setTrustAnchors(anchors: (string | Uint8Array | X509Certificate)[]): void { | ||
| TrustList.trustAnchors = TrustList.parseTrustAnchors(anchors); | ||
| } | ||
|
|
||
| /** | ||
| * Parses trust anchors from various formats into X509Certificate instances. | ||
| * Accepts PEM strings (single or multiple concatenated certs), DER bytes, or `X509Certificate` instances. | ||
| * @param anchors - Array of trust anchors in various formats | ||
| * @returns Array of parsed X509Certificate instances | ||
| */ | ||
| public static parseTrustAnchors(anchors: (string | Uint8Array | X509Certificate)[]): X509Certificate[] { |
There was a problem hiding this comment.
New parsing behavior (setTrustAnchors/parseTrustAnchors) is security- and correctness-critical, but there are no direct unit tests here. Consider adding focused tests that cover: multiple concatenated PEM certs, DER input, already-constructed X509Certificate, and handling of malformed blocks (and ensuring expected anchors are not silently dropped).
No description provided.