Handle SHC revocation #9
Merged
Conversation
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
Reviewer's GuideAdds SHC revocation handling based on issuer directory/VCI snapshot CRLs, introduces SHCReader configuration validation and new error types, and refactors directory/issuer metadata handling (including tests and docs) to use a simpler Directory API and optional VCI directory usage. Sequence diagram for SHCReader verification with revocation and directory lookupsequenceDiagram
actor User
participant SHCReader
participant Directory
participant VCISnapshot as VCIDirectory
participant Issuer
User->>SHCReader: constructor(config)
alt issuerDirectory and useVciDirectory both set
SHCReader-->>User: throw SHCReaderConfigError
else valid configuration
SHCReader-->>User: SHCReader instance
end
User->>SHCReader: verifySHC(jws)
SHCReader->>SHCReader: decode and validate JWS / VC
alt config.issuerDirectory provided
SHCReader->>Directory: directory = issuerDirectory
else config.useVciDirectory true
SHCReader->>VCISnapshot: Directory.fromVCI()
VCISnapshot-->>SHCReader: Directory instance
SHCReader->>Directory: directory = vciDirectory
else no directory configured
SHCReader->>SHCReader: directory = null
end
alt directory is not null
SHCReader->>Directory: getIssuerByIss(payload.iss)
Directory-->>SHCReader: Issuer or undefined
alt Issuer found and payload.vc.rid and issuer.crls
loop for each CRL in issuer.crls
SHCReader->>Issuer: check crl.rids.includes(vcRid)
alt rid found in CRL
SHCReader-->>User: throw SHCRevokedError
end
end
end
end
SHCReader-->>User: new SHC(jws, originalBundle)
Updated class diagram for SHCReader, Directory, SHC, SHCIssuer, and errorsclassDiagram
class SHCReader {
-config SHCReaderConfigParams
-bundleProcessor FHIRBundleProcessor
-vcProcessor VCProcessor
+SHCReader(config SHCReaderConfigParams)
+fromQRNumeric(qrNumericStrings string[]) Promise~SHC~
+fromJWS(jws string) Promise~SHC~
}
class SHCReaderConfigParams {
+publicKey string
+enableQROptimization boolean
+strictReferences boolean
+verifyExpiration boolean
+issuerDirectory Directory
+useVciDirectory boolean
}
class Directory {
-issuers Issuer[]
+Directory(issuers Issuer[])
+getIssuers() Issuer[]
+getIssuerByIss(iss string) Issuer
+fromVCI() Directory
}
class Issuer {
+iss string
+crls CRL[]
}
class CRL {
+rids string[]
}
class VerifiableCredential {
+vc VerifiableCredentialPayload
}
class VerifiableCredentialPayload {
+rid string
+credentialSubject CredentialSubject
}
class CredentialSubject {
+fhirBundle FHIRBundle
}
class FHIRBundle {
}
class SHC {
-jws string
-originalBundle FHIRBundle
+SHC(jws string, originalBundle FHIRBundle)
+asJWS() string
+asBundle() FHIRBundle
+toQRCodes(config QRCodeConfigParams) Promise~string[]~
}
class SHCIssuer {
-issuer string
-privateKey string
-publicKey string
+SHCIssuer(config SHCIssuerConfig)
+issue(fhirBundle FHIRBundle, config VerifiableCredentialParams) Promise~SHC~
}
class SHCError {
+code string
+message string
}
class SHCReaderConfigError {
+SHCReaderConfigError(message string)
}
class SHCRevokedError {
+SHCRevokedError(message string)
}
class CredentialValidationError {
+CredentialValidationError(message string)
}
class QRCodeConfigParams {
}
class SHCIssuerConfig {
+issuer string
+privateKey string
+publicKey string
}
class VerifiableCredentialParams {
}
SHCReader --> SHCReaderConfigParams
SHCReader --> Directory : uses
SHCReader --> VerifiableCredential : validates
SHCReader --> SHC : returns
Directory --> Issuer : contains
Issuer --> CRL : has
VerifiableCredential --> VerifiableCredentialPayload : vc
VerifiableCredentialPayload --> CredentialSubject : credentialSubject
CredentialSubject --> FHIRBundle : fhirBundle
SHCIssuer --> SHCIssuerConfig
SHCIssuer --> SHC : creates
SHCIssuer --> VerifiableCredentialParams
SHCError <|-- SHCReaderConfigError
SHCError <|-- SHCRevokedError
SHCError <|-- CredentialValidationError
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
SHCReader.fromJWS, consider caching the VCI directory whenuseVciDirectoryis true instead of callingDirectory.fromVCI()on every verification to avoid repeated network fetches and improve performance. - The revocation check in
SHCReader.fromJWSusesissuerCrls.forEachand throws inside the callback; usingsome/findor an early-return loop would make the control flow clearer and avoid iterating after a revocation has already been detected.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SHCReader.fromJWS`, consider caching the VCI directory when `useVciDirectory` is true instead of calling `Directory.fromVCI()` on every verification to avoid repeated network fetches and improve performance.
- The revocation check in `SHCReader.fromJWS` uses `issuerCrls.forEach` and throws inside the callback; using `some`/`find` or an early-return loop would make the control flow clearer and avoid iterating after a revocation has already been detected.
## Individual Comments
### Comment 1
<location> `src/shc/reader.ts:170-171` </location>
<code_context>
if (this.config.issuerDirectory) {
- issuerInfo = this.config.issuerDirectory.getIssuerInfo()
+ directory = this.config.issuerDirectory
+ } else if (this.config.useVciDirectory) {
+ directory = await Directory.fromVCI()
}
- return new SHC(jws, originalBundle, issuerInfo)
</code_context>
<issue_to_address>
**suggestion (performance):** Consider caching the VCI directory per reader instance instead of fetching on every read call.
With `useVciDirectory` enabled, each `read()` call re-runs `Directory.fromVCI()`. If this involves network I/O or heavy parsing, it will hurt throughput. Consider lazily initializing and caching the `Directory` on the `SHCReader` instance (e.g., `this.vciDirectory ??= await Directory.fromVCI()`) so it’s fetched once and reused across reads.
Suggested implementation:
```typescript
let directory: Directory | null = null
if (this.config.issuerDirectory) {
directory = this.config.issuerDirectory
} else if (this.config.useVciDirectory) {
if (!this.vciDirectory) {
this.vciDirectory = await Directory.fromVCI()
}
directory = this.vciDirectory
}
```
To fully implement the caching behavior, the `SHCReader` class (or whatever class this method belongs to) should declare a cached directory field, for example:
- Add a private (or protected) field to the class:
```ts
private vciDirectory: Directory | null = null
```
Place this alongside the other instance fields in `src/shc/reader.ts`. Ensure the type and visibility modifiers match the existing code style (e.g., `private` vs `protected`, use of definite assignment, etc.).
</issue_to_address>
### Comment 2
<location> `src/shc/reader.ts:174-179` </location>
<code_context>
+ const vcRid = payload.vc.rid
+ if (issuerCrls && vcRid) {
+ // TODO: we must also handle the case where the SHC is set to be revoked
+ issuerCrls.forEach(crl => {
+ if (crl.rids.includes(vcRid)) {
+ throw new SHCRevokedError('This SHC has been revoked')
+ }
+ })
+ }
+ }
</code_context>
<issue_to_address>
**suggestion:** Use a more intention-revealing pattern than `forEach` with an internal `throw` when checking CRLs.
This works but makes the control flow less clear, since `forEach` doesn’t short‑circuit. Consider using an existence check instead, e.g. `if (issuerCrls.some(crl => crl.rids.includes(vcRid))) { throw new SHCRevokedError('This SHC has been revoked') }`, which better communicates intent and doesn’t rely on `throw` to end iteration.
```suggestion
if (directory) {
const issuer = directory.getIssuerByIss(payload.iss)
const issuerCrls = issuer?.crls
const vcRid = payload.vc.rid
if (issuerCrls && vcRid) {
// TODO: we must also handle the case where the SHC is set to be revoked
const isRevoked = issuerCrls.some(crl => crl.rids.includes(vcRid))
if (isRevoked) {
throw new SHCRevokedError('This SHC has been revoked')
}
```
</issue_to_address>
### Comment 3
<location> `test/shc/shc.test.ts:169-166` </location>
<code_context>
+ it('should verify SHC when reader created with a directory', async () => {
</code_context>
<issue_to_address>
**issue (testing):** Add tests covering SHCRevokedError when an SHC rid is present in the issuer CRL.
This only exercises the non‑revoked path with an `issuerDirectory`. Please also add tests that issue an SHC with a known `rid`, build a `Directory` whose CRL includes that `rid`, and assert that `readerWithDirectory.fromJWS(...)` rejects with `SHCRevokedError`. Cover both cases: (1) `issuerDirectory` passed directly and (2) `useVciDirectory: true` with a mocked VCI snapshot whose CRL marks the SHC as revoked, so we verify revocation is enforced in all flows.
</issue_to_address>
### Comment 4
<location> `test/shc/shc.test.ts:219-166` </location>
<code_context>
+ it('should verify SHC when reader is created with the VCI snapshot directory', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Cover edge cases for VCI directory usage: issuer not found and SHC without `rid`.
This test only covers the happy path. Please also add cases for:
- An issuer missing from the VCI snapshot (no entry for `payload.iss`), asserting the intended behavior when directory metadata is absent.
- An SHC whose VC payload omits `rid`, asserting that verification succeeds without attempting revocation checks.
These will validate revocation behavior when directory data is incomplete or `rid` is missing.
Suggested implementation:
```typescript
expect(verifiedBundle).toBeDefined()
expect(verifiedBundle).toEqual(validBundle)
})
it('should verify SHC when reader is created with the VCI snapshot directory when issuer is missing from VCI snapshot', async () => {
const healthCard = await issuer.issue(validBundle)
const readerWithDirectory = new SHCReader({
...readerConfig,
useVciDirectory: true,
})
const originalFetch = globalThis.fetch
const fetchMock = vi.fn().mockImplementation((url: string) => {
if (url.includes('vci_snapshot.json')) {
// VCI snapshot without an entry for the issuer in payload.iss
return Promise.resolve({
ok: true,
json: async () =>
({
issuers: [],
}) as unknown,
} as Response)
}
// Defer all other requests to the original implementation
return originalFetch(url)
})
globalThis.fetch = fetchMock as unknown as typeof fetch
const verifiedHealthCard = await readerWithDirectory.fromJWS(healthCard.asJWS())
const verifiedBundle = await verifiedHealthCard.asBundle()
expect(verifiedBundle).toBeDefined()
expect(verifiedBundle).toEqual(validBundle)
globalThis.fetch = originalFetch
})
it('should verify SHC when VC payload omits rid using the VCI snapshot directory', async () => {
const bundleWithoutRid = structuredClone(validBundle)
// Remove rid from the VC payload; adjust the path to match the actual schema.
// This example assumes rid is stored on the first entry's resource.identifier[0].value.
if (
bundleWithoutRid.entry?.[0]?.resource?.identifier &&
Array.isArray(bundleWithoutRid.entry[0].resource.identifier)
) {
delete bundleWithoutRid.entry[0].resource.identifier[0].value
}
const healthCard = await issuer.issue(bundleWithoutRid)
const readerWithDirectory = new SHCReader({
...readerConfig,
useVciDirectory: true,
})
const originalFetch = globalThis.fetch
const fetchMock = vi.fn().mockImplementation((url: string) => {
if (url.includes('vci_snapshot.json')) {
// Minimal VCI snapshot; structure should match what the reader expects.
return Promise.resolve({
ok: true,
json: async () =>
({
issuers: [
// Add an entry if required by the reader implementation.
// The important part for this test is that rid is absent from the VC payload,
// so no revocation check is attempted.
],
}) as unknown,
} as Response)
}
return originalFetch(url)
})
globalThis.fetch = fetchMock as unknown as typeof fetch
const verifiedHealthCard = await readerWithDirectory.fromJWS(healthCard.asJWS())
const verifiedBundle = await verifiedHealthCard.asBundle()
expect(verifiedBundle).toBeDefined()
expect(verifiedBundle).toEqual(bundleWithoutRid)
globalThis.fetch = originalFetch
})
it('should verify SHC when reader is created with the VCI snapshot directory', async () => {
```
1. **Confirm the rid location in the bundle**:
The example removes `rid` from `bundleWithoutRid.entry[0].resource.identifier[0].value`. If your schema stores `rid` elsewhere (e.g., a top-level VC claim or a different resource path), update the deletion logic to match the actual location used in your SHC/VC model so that the issued JWS truly omits `rid`.
2. **Align VCI snapshot structure with the production shape**:
The mocked responses for `vci_snapshot.json` return `{ issuers: [...] }` as a minimal shape. Adjust the structure and inner fields (e.g., `iss`, `name`, `status`, `revocation` metadata) to mirror the real snapshot format expected by `SHCReader` (you can copy this from the existing “happy path” VCI test or a shared fixture).
3. **Optionally assert revocation behavior more directly**:
If `SHCReader` hits a revocation endpoint (or calls a specific helper) when `rid` is present and issuer metadata exists, you can extend the tests to spy on that call and assert that it is **not** invoked in these two new scenarios. That will more explicitly validate “no revocation checks when issuer metadata is missing or rid is absent” instead of inferring it only from overall verification success.
</issue_to_address>
### Comment 5
<location> `test/shc/directory.test.ts:6-7` </location>
<code_context>
-}
+import { SAMPLE_DIRECTORY_JSON } from '../helpers'
function assertDirectoryFromSampleJson(directory: Directory) {
- const issuers = directory.getIssuerInfo()
+ const issuers = directory.getIssuers()
expect(issuers).toHaveLength(4)
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for the new `Directory.getIssuerByIss` helper.
`getIssuerByIss` isn’t covered by tests yet, even though `SHCReader` now depends on it for revocation checks. Please add unit tests that:
- Call `getIssuerByIss` with an `iss` present in `SAMPLE_DIRECTORY_JSON` and assert the matching issuer is returned.
- Call it with an unknown `iss` and assert it returns `undefined`.
This keeps the directory lookup logic validated independently of the reader tests.
Suggested implementation:
```typescript
import { SAMPLE_DIRECTORY_JSON } from '../helpers'
import { Directory } from '../../src/shc/directory'
describe('Directory', () => {
function createDirectoryFromSampleJson(): Directory {
// Adjust this factory if Directory is constructed differently in your codebase
return Directory.fromJson(SAMPLE_DIRECTORY_JSON)
}
describe('getIssuerByIss', () => {
it('returns the matching issuer when iss exists in the directory', () => {
const directory = createDirectoryFromSampleJson()
const knownIss = SAMPLE_DIRECTORY_JSON.issuers[0].iss
const issuer = directory.getIssuerByIss(knownIss)
expect(issuer).toBeDefined()
expect(issuer?.iss).toBe(knownIss)
})
it('returns undefined when iss is not found in the directory', () => {
const directory = createDirectoryFromSampleJson()
const issuer = directory.getIssuerByIss('https://unknown-issuer.example.org')
expect(issuer).toBeUndefined()
})
})
})
```
You may need to make a few adjustments depending on the existing codebase:
1. **Directory import path/name**
- Update `import { Directory } from '../../src/shc/directory'` to match the actual location and export name of your `Directory` class (e.g. `../src/shc/Directory` or a different relative path/casing).
2. **Directory construction API**
- If `Directory` does not provide a `fromJson` static method, replace `Directory.fromJson(SAMPLE_DIRECTORY_JSON)` with the correct construction logic, such as:
- `new Directory(SAMPLE_DIRECTORY_JSON)` or
- a different factory/helper already used elsewhere in your tests.
3. **Sample JSON shape**
- This assumes `SAMPLE_DIRECTORY_JSON.issuers` is an array of issuers with an `iss` field. If the structure differs, adjust the access in `knownIss` and the assertion `issuer?.iss` to match the actual type (e.g. `SAMPLE_DIRECTORY_JSON[0].issuer.iss`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
fjsj
reviewed
Dec 15, 2025
fjsj
reviewed
Dec 15, 2025
fjsj
reviewed
Dec 15, 2025
fjsj
reviewed
Dec 15, 2025
fjsj
reviewed
Dec 15, 2025
fjsj
reviewed
Dec 15, 2025
fjsj
reviewed
Dec 15, 2025
Contributor
Author
|
Latest batch of changes:
|
fjsj
reviewed
Dec 17, 2025
fjsj
reviewed
Dec 17, 2025
fjsj
reviewed
Dec 17, 2025
fjsj
requested changes
Dec 17, 2025
Member
fjsj
left a comment
fjsj
left a comment
There was a problem hiding this comment.
Overall LGTM, check minor comments
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.
Summary by Sourcery
Add VCI directory support and revocation checks to SHC verification while simplifying issuer metadata handling.
New Features:
Enhancements:
Documentation:
Tests:
Closes #2
Closes #4