fix(vcr): bound negative OpenID4VCI identifier caching, guard against empty offers - #4470
Open
reinkrul wants to merge 3 commits into
Open
fix(vcr): bound negative OpenID4VCI identifier caching, guard against empty offers#4470reinkrul wants to merge 3 commits into
reinkrul wants to merge 3 commits into
Conversation
…fier tlsIdentifierResolver cached a resolution result as soon as it succeeded without erroring, including an empty string when no base-URL service or TLS-derived candidate could be found. Once that happened, every later Resolve() call for that DID short-circuited on the cached empty value for the life of the process, even after the missing node-http-services-baseurl service was added, permanently breaking OpenID4VCI credential-offer delivery until restart. Only cache a non-empty identifier, and only treat a non-empty cached value as a hit, so resolution is retried on every call until it actually succeeds. Assisted by AI
13 tasks
Contributor
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (3)
🛟 Help
|
Address review feedback on the previous commit: caching nothing at all for an unresolved identifier would re-run resolution (including the DID document lookup) on every single call for a DID that never gets fixed, since this resolver can be invoked on every OpenID4VCI request. That negative caching was likely deliberate, not a bug. Reuse the existing lastAttempt/tlsAttemptInterval throttle (already used to rate-limit the expensive TLS-certificate-derived resolution) to also bound how long an empty result is treated as cached, instead of adding a separate cache window. A successful (non-empty) identifier is still cached indefinitely, unchanged. Also close the actual sending-side gap: nothing previously stopped an OpenID4VCI credential offer from being sent with an empty `credential_issuer`, which is what produced the receiver-side "empty Credential Issuer Identifier" rejection in the first place. Introduce openid4vci.ErrIdentifierNotConfigured, returned when resolution succeeds but yields no identifier, and have issueUsingOpenID4VCI treat it like an unsupported wallet (quiet fallback to the network, no error) instead of constructing and sending a broken offer. Assisted by AI
Unlike an unsupported wallet (the other party's problem, no action for this operator), a missing node-http-services-baseurl service is this node's own misconfiguration and needs the operator's attention. Log a Warn pointing them at what to search the documentation for, rather than staying silent like the unsupported-wallet case. Assisted by AI
reinkrul
marked this pull request as ready for review
September 4, 2026 08:22
reinkrul
requested review from
Dirklectisch,
JorisHeadease,
gerardsn,
stevenvegt and
woutslakhorst
as code owners
September 4, 2026 08:22
3 tasks
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
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.

Related: #4469
Problem
tlsIdentifierResolver(vcr/openid4vci/identifiers.go) cached its resolution result as soon as it completed without erroring — including an empty string when neither the DID document nor TLS-certificate-derived candidates yielded a base URL. Once that happened, every laterResolve()call for that DID short-circuited on the cached empty value for the life of the process, even after the missingnode-http-services-baseurlservice was added, permanently breaking OpenID4VCI credential-offer delivery until restart.Observed on the sender side (issuer), for a
credential_offersent withcredential_issuer:"":Observed on the receiver side, rejecting that same offer:
Fix
Bound the negative cache instead of removing it. An earlier version of this fix stopped caching empty results at all, but this resolver can be called on every OpenID4VCI request, so re-running resolution (including the DID document lookup) on every single call for a DID that never gets fixed isn't free — the original negative caching was likely deliberate. Instead, reuse the existing
lastAttempt/tlsAttemptIntervalthrottle (already used to rate-limit the expensive TLS-certificate-derived resolution) to also bound how long an empty result is treated as cached, rather than adding a separate cache window. A successful (non-empty) identifier is still cached indefinitely, unchanged.Guard against sending a broken offer. Nothing previously stopped an OpenID4VCI credential offer from being sent with an empty
credential_issuer— which is what produced the receiver-side "empty Credential Issuer Identifier" rejection shown above. Introducedopenid4vci.ErrIdentifierNotConfigured, returned when resolution succeeds but yields no identifier, andissueUsingOpenID4VCInow treats it like an unsupported wallet (fallback to the network) instead of constructing and sending the broken offer.Warn the operator, unlike the unsupported-wallet case. An unsupported wallet is the other party's problem — nothing this operator can act on, so it stays silent. A missing
node-http-services-baseurlservice is this node's own misconfiguration, so it now logs aWarnpointing the operator at what to search the documentation for, instead of failing the same silent way.Test plan
TestTLSIdentifierResolver/empty_result_is_cached_briefly,_not_forever(new) — asserts a cache hit on an immediate second call, then a re-check once the throttle window elapses.Test_vcr_GetOIDCIssuer/found_DID,_owned,_but_no_identifier_configured(new) — assertsErrIdentifierNotConfiguredwhen resolution yields an empty identifier.Test_issuer_Issue/OpenID4VCI/ok_-_OpenID4VCI_issuer_identifier_not_(yet)_configured_-_fallback_to_network(new) — asserts a quiet fallback to network publish, no offer sent, and the new Warn log.vcr,vcr/issuer,vcr/openid4vcisuites still pass.Assisted by AI