Add Directory support #6
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 GuideThis PR introduces directory-based issuer metadata support by implementing a Directory class to fetch and store issuer keys and CRLs, extending the SHCReader and SHCIssuer/SHC classes to propagate issuerInfo, adding new types and configuration parameters, updating tests accordingly, and bumping the Node engine requirement. Sequence diagram for reading a JWS with directory-based issuer metadatasequenceDiagram
participant Reader as SHCReader
participant Directory
participant SHC
Reader->>Directory: getIssuerInfo()
Directory-->>Reader: Issuer[]
Reader->>SHC: new SHC(jws, originalBundle, issuerInfo)
SHC-->>Reader: SHC instance
Sequence diagram for issuing a health card with issuerInfo propagationsequenceDiagram
participant Issuer as SHCIssuer
participant SHC
Issuer->>SHC: new SHC(jws, fhirBundle, issuerInfo)
SHC-->>Issuer: SHC instance
Class diagram for new and updated issuer metadata classesclassDiagram
class Directory {
- issuerInfo: Issuer[]
+ getIssuerInfo(): Issuer[]
+ static fromJSON(dirJson: DirectoryJSON): Directory
+ static fromURLs(issUrls: string[]): Promise<Directory>
}
class SHC {
- jws: string
- originalBundle: FHIRBundle
- issuerInfo: Issuer[]
+ getIssuerInfo(): Issuer[]
}
class SHCReader {
- config: SHCReaderConfig
+ read(jws: string): SHC
}
class SHCIssuer {
+ issue(fhirBundle: FHIRBundle, config: VerifiableCredentialParams, issuerInfo: Issuer[]): Promise<SHC>
}
class Issuer {
+ iss: string
+ keys: IssuerKey[]
+ crls: IssuerCrl[]
}
class IssuerKey {
+ kty: string
+ kid: string
}
class IssuerCrl {
+ kid: string
+ method: string
+ ctr: number
+ rids: string[]
}
class DirectoryJSON {
+ issuerInfo: issuerInfo[]
}
Directory --> Issuer
SHC --> Issuer
SHCReader --> Directory
SHCIssuer --> SHC
Issuer --> IssuerKey
Issuer --> IssuerCrl
DirectoryJSON --> Issuer
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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/shc/directory.ts:20-29` </location>
<code_context>
+ try {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Catching errors inside the loop may mask partial failures.
Move the try/catch block inside the loop so that errors from individual issuers don't prevent processing the rest, and to improve error reporting for each issuer.
Suggested implementation:
```typescript
for (const issUrl of issUrls) {
try {
const issuer: Issuer = {
iss: issUrl,
keys: [],
crls: [],
}
const crls = []
const jwksUrl = `${issUrl}/.well-known/jwks.json`
const jwksResponse = await fetch(jwksUrl)
```
```typescript
try {
for (const issUrl of issUrls) {
const issuer: Issuer = {
iss: issUrl,
keys: [],
crls: [],
}
const crls = []
const jwksUrl = `${issUrl}/.well-known/jwks.json`
const jwksResponse = await fetch(jwksUrl)
} catch (error) {
console.error(`Error processing issuer ${issUrl}:`, error)
// Optionally, you could continue or add partial issuer info to issuersInfo
continue
}
}
```
</issue_to_address>
### Comment 2
<location> `src/shc/directory.ts:29-31` </location>
<code_context>
+ const jwksUrl = `${issUrl}/.well-known/jwks.json`
+ const jwksResponse = await fetch(jwksUrl)
+ if (!jwksResponse.ok) {
+ const errorData = await jwksResponse.json().catch(() => ({ error: 'Unknown error' }))
+ throw new Error(errorData.error || `Failed to fetch jwks at ${jwksUrl}`)
+ }
</code_context>
<issue_to_address>
**suggestion:** Parsing error responses as JSON may not always be reliable.
Some endpoints may return non-JSON errors, leading to unexpected exceptions. Consider verifying the content-type header or providing a fallback error message if JSON parsing fails.
```suggestion
const jwksUrl = `${issUrl}/.well-known/jwks.json`
const jwksResponse = await fetch(jwksUrl)
if (!jwksResponse.ok) {
let errorMessage: string
const contentType = jwksResponse.headers.get('content-type') || ''
if (contentType.includes('application/json')) {
try {
const errorData = await jwksResponse.json()
errorMessage = errorData.error || `Failed to fetch jwks at ${jwksUrl}`
} catch {
errorMessage = `Failed to fetch jwks at ${jwksUrl} (invalid JSON error response)`
}
} else {
errorMessage = `Failed to fetch jwks at ${jwksUrl} (non-JSON error response)`
}
throw new Error(errorMessage)
}
```
</issue_to_address>
### Comment 3
<location> `src/shc/directory.ts:47` </location>
<code_context>
+ continue
+ }
+ const crl = await crlResponse.json()
+ if (crl) crls.push(crl)
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** No validation of CRL structure before adding to array.
Malformed or incomplete CRLs may be included. Please validate the CRL object to ensure it meets expected structure before adding it to the array.
Suggested implementation:
```typescript
// Helper function to validate CRL structure
function isValidCrl(crl: any): boolean {
// Example: require 'id' and 'revoked' fields
return (
typeof crl === 'object' &&
crl !== null &&
typeof crl.id === 'string' &&
Array.isArray(crl.revoked)
)
}
for (const key of issKeys) {
const crlUrl = `${issUrl}/.well-known/crl/${key.kid}.json`
const crlResponse = await fetch(crlUrl)
if (!crlResponse.ok) {
console.debug(
`Failed to fetch crl at ${crlUrl} with status ${crlResponse.status}, skipping.`
)
continue
}
const crl = await crlResponse.json()
if (isValidCrl(crl)) {
crls.push(crl)
} else {
console.warn(`Invalid CRL structure from ${crlUrl}, skipping.`)
}
```
You may need to adjust the `isValidCrl` function to match the exact expected CRL structure for your application. Add or remove required fields as necessary.
</issue_to_address>
### Comment 4
<location> `src/shc/directory.ts:11-13` </location>
<code_context>
+ return this.issuerInfo
+ }
+
+ static fromJSON(dirJson: DirectoryJSON): Directory {
+ const data: Issuer[] = dirJson.issuerInfo.map(({ issuer: { iss }, keys, crls }) => {
+ return { iss, keys, crls } as Issuer
</code_context>
<issue_to_address>
**suggestion:** Type assertion with 'as Issuer' may mask type mismatches.
Explicitly validate the input data or assign properties to ensure type safety instead of relying on type assertion.
```suggestion
const data: Issuer[] = dirJson.issuerInfo.map(({ issuer, keys, crls }) => {
// Validate and assign properties explicitly
const iss = typeof issuer?.iss === 'string' ? issuer.iss : '';
const validKeys = Array.isArray(keys) ? keys : [];
const validCrls = Array.isArray(crls) ? crls : [];
return {
iss,
keys: validKeys,
crls: validCrls
};
})
```
</issue_to_address>
### Comment 5
<location> `test/shc/directory.test.ts:4` </location>
<code_context>
+import type { DirectoryJSON, Issuer } from './types'
+
+export class Directory {
+ constructor(private issuerInfo: Issuer[]) {}
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test cases for error handling in Directory.fromURLs.
Add tests for Directory.fromURLs to cover fetch failures, such as network errors, invalid responses, and malformed JSON, ensuring proper error handling and logging or exception behavior.
</issue_to_address>
### Comment 6
<location> `test/shc/shc.test.ts:61-70` </location>
<code_context>
+ it('should bundle issuerInfo into SHC when reader created with a directory', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding negative and edge case tests for issuerInfo propagation.
Please include tests for empty directories, multiple issuers, and missing keys/CRLs to verify SHCReader and SHC behavior in these scenarios.
Suggested implementation:
```typescript
it('should bundle issuerInfo into SHC when reader created with a directory', async () => {
const { importPKCS8, importSPKI } = await import('jose')
const privateKeyCrypto = await importPKCS8(testPrivateKeyPKCS8, 'ES256')
const publicKeyCrypto = await importSPKI(testPublicKeySPKI, 'ES256')
const configWithCryptoKeys: SHCConfig = {
issuer: 'https://example.com/issuer',
privateKey: privateKeyCrypto,
publicKey: publicKeyCrypto,
expirationTime: null,
}
// ... existing test logic ...
})
it('should not bundle issuerInfo when reader created with an empty directory', async () => {
const emptyDirectory = new Directory([])
const reader = new SHCReader({ directory: emptyDirectory })
const shc = await reader.createSHC(createValidFHIRBundle())
expect(shc.issuerInfo).toBeUndefined()
})
it('should bundle issuerInfo for multiple issuers in directory', async () => {
const { importPKCS8, importSPKI } = await import('jose')
const privateKeyCrypto1 = await importPKCS8(testPrivateKeyPKCS8, 'ES256')
const publicKeyCrypto1 = await importSPKI(testPublicKeySPKI, 'ES256')
const privateKeyCrypto2 = await importPKCS8(testPrivateKeyPKCS8_2, 'ES256')
const publicKeyCrypto2 = await importSPKI(testPublicKeySPKI_2, 'ES256')
const directory = new Directory([
{
issuer: 'https://issuer1.com',
publicKey: publicKeyCrypto1,
crl: ['crl1'],
},
{
issuer: 'https://issuer2.com',
publicKey: publicKeyCrypto2,
crl: ['crl2'],
},
])
const reader = new SHCReader({ directory })
const shc = await reader.createSHC(createValidFHIRBundle())
expect(shc.issuerInfo).toBeDefined()
expect(Array.isArray(shc.issuerInfo)).toBe(true)
expect(shc.issuerInfo.length).toBe(2)
expect(shc.issuerInfo[0].issuer).toBe('https://issuer1.com')
expect(shc.issuerInfo[1].issuer).toBe('https://issuer2.com')
})
it('should handle missing publicKey in directory entry', async () => {
const directory = new Directory([
{
issuer: 'https://issuer-missing-key.com',
// publicKey is missing
crl: ['crl1'],
},
])
const reader = new SHCReader({ directory })
await expect(reader.createSHC(createValidFHIRBundle())).rejects.toThrow(/publicKey/i)
})
it('should handle missing CRL in directory entry', async () => {
const { importSPKI } = await import('jose')
const publicKeyCrypto = await importSPKI(testPublicKeySPKI, 'ES256')
const directory = new Directory([
{
issuer: 'https://issuer-missing-crl.com',
publicKey: publicKeyCrypto,
// crl is missing
},
])
const reader = new SHCReader({ directory })
const shc = await reader.createSHC(createValidFHIRBundle())
expect(shc.issuerInfo[0].crl).toBeUndefined()
})
```
- You may need to define `testPrivateKeyPKCS8_2` and `testPublicKeySPKI_2` for the multiple issuers test, or mock them similarly to your existing test keys.
- Adjust error handling in your SHCReader implementation if it does not currently throw on missing keys.
- Ensure that the Directory and SHCReader classes support these edge cases as expected.
</issue_to_address>
### Comment 7
<location> `test/shc/shc.test.ts:123-59` </location>
<code_context>
+ })
+
+ const verifiedHealthCard = await readerWithDirectory.fromJWS(jws)
+ expect(verifiedHealthCard.getIssuerInfo()).toBe(directory.getIssuerInfo())
+ })
+
it('should issue SMART Health Card with CryptoKey objects', async () => {
</code_context>
<issue_to_address>
**nitpick (testing):** Nitpick: Use deep equality for issuerInfo comparison.
Use 'toEqual' instead of 'toBe' to compare issuerInfo objects, ensuring deep equality rather than reference equality.
</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
Nov 14, 2025
fjsj
reviewed
Nov 14, 2025
fjsj
requested changes
Nov 14, 2025
Member
fjsj
left a comment
fjsj
left a comment
There was a problem hiding this comment.
Please check comments and add documentation.
fjsj
reviewed
Nov 18, 2025
fjsj
reviewed
Nov 18, 2025
fjsj
requested changes
Nov 18, 2025
Member
fjsj
left a comment
fjsj
left a comment
There was a problem hiding this comment.
LGTM, but check the two suggested minor changes
rvlb
force-pushed
the
directories-shc
branch
from
010a8fb to
5e7c85e
Compare
fjsj
approved these changes
Nov 24, 2025
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 support for Directory-based issuer metadata by introducing a Directory class, extending SHCReader and SHC classes to propagate issuerInfo, and updating configuration and APIs accordingly
New Features:
Tests:
fromJSONandfromURLsstatic methodsSummary by Sourcery
Introduce support for directory-based issuer metadata by adding a Directory class, updating SHCIssuer, SHCReader, and SHC to propagate issuerInfo, defining related types, and adding tests for the new functionality
New Features:
Enhancements:
Tests: