refactor: update get x509 certs logic#352
Conversation
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR refactors trust-service authentication from a CREDEBL platform-based model (with dedicated/shared credentials) to an explicit auth-type-driven configuration supporting Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
review @coderabbitai |
|
✅ Actions performedReview triggered.
|
tipusinghaw
changed the title
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.sample:
- Line 55: Remove the redundant TRUST_LIST_URL entry: keep the primary
documented TRUST_LIST_URL definition and delete the duplicate TRUST_LIST_URL
line (the leftover from the refactor) so the environment sample contains only
one TRUST_LIST_URL key; verify surrounding comments remain accurate after
removal.
In `@src/cliAgent.ts`:
- Around line 282-295: In getTrustedCertificatesForVerification, don't silently
swallow exceptions; either re-throw configuration/IO errors or at minimum log
full error context before returning an empty array—capture the thrown error from
getAuthType / getX509CertsByClientToken / getX509CertsByUrl, inspect its
type/message, and: for configuration/network errors (e.g., missing tenantId,
HTTP failures) re-throw or return a distinct sentinel error so callers can
handle it, otherwise log the full error stack plus a clear message including
authType and tenantId and then return []; update the catch block accordingly to
use the error object (not just error.message) and consider throwing a custom
error for misconfiguration cases.
In `@src/utils/helpers.ts`:
- Line 4: Remove the unused import symbol X509Certificate from the import
statement in src/utils/helpers.ts; locate the line importing JsonEncoder,
JsonTransformer, X509Certificate and delete only X509Certificate so the file
imports just the used symbols (e.g., JsonEncoder and JsonTransformer) to satisfy
the linter and SonarCloud.
In `@src/utils/oid4vc-agent.ts`:
- Around line 300-306: The trust-list validation in getX509CertsByUrl is too
lax: after confirming data is an array, validate each element is a non-empty
string before casting; update the logic in getX509CertsByUrl to filter/map data
to only string entries (e.g., typeof item === 'string' && item.trim().length >
0), log the number of valid certificates, and if no valid string entries remain
throw a clear Error('[getX509CertsByUrl] no valid string certificates in trust
list'); return the filtered string[] instead of blindly casting.
- Around line 6-7: The file has import ordering violations and uses console.*
which trips no-console; reorder imports in src/utils/oid4vc-agent.ts so
third-party, then internal, then relative imports follow the project's
import/order rule and move the lines importing checkX509Certificates and
validateAuthConfig into the correct group, and replace every
console.log/console.error/console.warn call (at the spots you changed around
lines 258, 262, 279, 290, 304) with the shared logger used elsewhere in the
project (or remove them if logging is unnecessary) by calling the logger methods
(e.g., logger.info/logger.error/logger.warn) so ESLint no-console no longer
fails. Ensure you import the shared logger where required and run the linter to
confirm import/order and no-console issues are resolved.
- Line 292: The fetch call "const response = await fetch(trustListUrl)" blocks
without a timeout; wrap this fetch with an AbortController and a timeout
(consistent with the repo's webhook timeout pattern) by creating an
AbortController, starting a setTimeout that calls controller.abort() after the
configured timeout, passing controller.signal into fetch(trustListUrl, { signal
}), and clearing the timer once fetch completes; also handle the abort case
(treat AbortError as a timeout and surface a clear error) so certificate
resolution won't hang indefinitely.
- Around line 247-263: Change getTrustedCerts to require certificateChain in its
signature and validate tenantId at the function boundary: make the signature
accept certificateChain: X509Certificate[] (non-optional) and validate tenantId
up-front when the agent is shared (detected via '!('tenants' in
agent.modules)'); if tenantId is missing for a shared agent throw a clear error.
Keep the downstream call to verifyX509CertificateTrust but remove reliance on it
for contract validation. Update callers (e.g. getX509CertsByClientToken) to pass
both tenantId and certificateChain into the new required parameters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52b47622-9144-4079-acb8-cbf7c0e0b717
📒 Files selected for processing (11)
.env.demo.env.samplesrc/cliAgent.tssrc/controllers/auth/AuthController.tssrc/routes/routes.tssrc/routes/swagger.jsonsrc/server.tssrc/utils/auth.tssrc/utils/constant.tssrc/utils/helpers.tssrc/utils/oid4vc-agent.ts
💤 Files with no reviewable changes (2)
- src/routes/swagger.json
- src/routes/routes.ts
|
|
||
| console.log('[getX509CertsByUrl] fetching trust list from:', trustListUrl) | ||
|
|
||
| const response = await fetch(trustListUrl) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Surface Node runtime hints so we can confirm whether AbortSignal.timeout()
# is available in the deployed runtime; otherwise use AbortController + setTimeout.
fd '^(package\.json|Dockerfile|\.nvmrc|\.node-version)$' -HI -t f -E node_modules -E dist -E build \
-x sh -c 'echo "== $1 =="; sed -n "1,120p" "$1"; echo' sh {}Repository: credebl/agent-controller
Length of output: 5077
🏁 Script executed:
#!/bin/bash
# First, check if the file exists and examine the code around line 292
wc -l src/utils/oid4vc-agent.ts && sed -n '280,305p' src/utils/oid4vc-agent.tsRepository: credebl/agent-controller
Length of output: 881
🏁 Script executed:
#!/bin/bash
# Search for other fetch() calls to see if timeouts are being used elsewhere
rg "fetch\(" --no-heading -B 2 -A 2 | head -100Repository: credebl/agent-controller
Length of output: 576
🏁 Script executed:
#!/bin/bash
# Search for existing HTTP timeout patterns (axios, AbortController, etc.)
rg "AbortSignal|AbortController|timeout" -t ts --no-heading -B 1 -A 1 | head -80Repository: credebl/agent-controller
Length of output: 1571
🏁 Script executed:
#!/bin/bash
# Check the WebhookEvent.ts to understand the full timeout pattern
sed -n '1,50p' src/events/WebhookEvent.tsRepository: credebl/agent-controller
Length of output: 1138
🏁 Script executed:
#!/bin/bash
# Search for any HTTP client utility or helper functions
rg "export.*fetch|function.*fetch|const.*fetch" -t ts --no-heading -B 1 -A 3Repository: credebl/agent-controller
Length of output: 1110
Add a deadline to the trust-list fetch.
fetch() has no default timeout, so a slow or unreachable trust-list endpoint can block certificate resolution indefinitely. Pass an abort signal or use a timeout pattern consistent with the repo's existing webhook timeout handling.
⏱️ Suggested fix
- const response = await fetch(trustListUrl)
+ const response = await fetch(trustListUrl, {
+ signal: AbortSignal.timeout(5000),
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await fetch(trustListUrl) | |
| const response = await fetch(trustListUrl, { | |
| signal: AbortSignal.timeout(5000), | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/oid4vc-agent.ts` at line 292, The fetch call "const response =
await fetch(trustListUrl)" blocks without a timeout; wrap this fetch with an
AbortController and a timeout (consistent with the repo's webhook timeout
pattern) by creating an AbortController, starting a setTimeout that calls
controller.abort() after the configured timeout, passing controller.signal into
fetch(trustListUrl, { signal }), and clearing the timer once fetch completes;
also handle the abort case (treat AbortError as a timeout and surface a clear
error) so certificate resolution won't hang indefinitely.
| if (!Array.isArray(data) || data.length === 0) { | ||
| throw new Error('[getX509CertsByUrl] trust list is empty or invalid') | ||
| } | ||
|
|
||
| console.log('[getX509CertsByUrl] fetched certificates count:', data.length) | ||
|
|
||
| return data as string[] |
There was a problem hiding this comment.
Validate each trust-list entry before casting.
Array.isArray(data) accepts [{}], [123], and [""]. Right now those values are cast to string[] and will only fail later when the x509 layer tries to parse them.
🛡️ Suggested fix
- if (!Array.isArray(data) || data.length === 0) {
+ if (
+ !Array.isArray(data) ||
+ data.length === 0 ||
+ data.some((item) => typeof item !== 'string' || item.trim().length === 0)
+ ) {
throw new Error('[getX509CertsByUrl] trust list is empty or invalid')
}
console.log('[getX509CertsByUrl] fetched certificates count:', data.length)
return data as string[]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!Array.isArray(data) || data.length === 0) { | |
| throw new Error('[getX509CertsByUrl] trust list is empty or invalid') | |
| } | |
| console.log('[getX509CertsByUrl] fetched certificates count:', data.length) | |
| return data as string[] | |
| if ( | |
| !Array.isArray(data) || | |
| data.length === 0 || | |
| data.some((item) => typeof item !== 'string' || item.trim().length === 0) | |
| ) { | |
| throw new Error('[getX509CertsByUrl] trust list is empty or invalid') | |
| } | |
| console.log('[getX509CertsByUrl] fetched certificates count:', data.length) | |
| return data as string[] |
🧰 Tools
🪛 ESLint
[error] 304-304: Unexpected console statement.
(no-console)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/oid4vc-agent.ts` around lines 300 - 306, The trust-list validation
in getX509CertsByUrl is too lax: after confirming data is an array, validate
each element is a non-empty string before casting; update the logic in
getX509CertsByUrl to filter/map data to only string entries (e.g., typeof item
=== 'string' && item.trim().length > 0), log the number of valid certificates,
and if no valid string entries remain throw a clear Error('[getX509CertsByUrl]
no valid string certificates in trust list'); return the filtered string[]
instead of blindly casting.
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
|
2 participants
Summary by CodeRabbit
Configuration Changes
TRUST_SERVICE_AUTH_TYPEto support different authentication modes and conditional configuration fields.Removed Features
Improvements