fix: address PR #301 review comments#302
Open
leifj wants to merge 2 commits intoSUNET:masv/infra/releasefrom
Open
fix: address PR #301 review comments#302leifj wants to merge 2 commits intoSUNET:masv/infra/releasefrom
leifj wants to merge 2 commits intoSUNET:masv/infra/releasefrom
Conversation
- Use url.Parse instead of strings.TrimLeft for x509_san_dns client_id - Validate scope against credential constructors in MakeSDJWT (SSRF prevention) - Enforce VCT URL and integrity match configured values - Add validate:required,url to VCTUrl and validate:required to Integrity - Use resolved VCT URL in UIMetadata (prefer GetVCTURL over VCTM.VCT) - Add 30s timeout and 1MiB size limit to fetchVCTM HTTP client - Add SetVCTURL/SetIntegrity methods to CredentialConstructor for testing
leifj
force-pushed
the
fix/review-comments-301
branch
from
e56c070 to
9b63c46
Compare
The issuer and apigw load VCTMs independently, and their cached integrity hashes can diverge when caches refresh at different times. Remove the cross-cache integrity comparison in MakeSDJWT; the scope and VCT URL checks prevent SSRF, and BuildCredentialWithSigner already verifies integrity against the actual fetched VCTM content.
|
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
1 participant
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.
Addresses all 5 Copilot review comments on PR #301:
strings.TrimLeftunsafe for URL scheme stripping (handler_openid4vp.go)url.Parse()+parsedURL.Hostinstead ofstrings.TrimLeftto extract the DNS name for thex509_san_dns:client_id.MakeSDJWTdoesn't validate scope/VCT URL (SSRF risk) (handlers.go)CredentialConstructorbyreq.Scope— reject unknown scopes.vct_urlandintegritymatch the configured values, preventing arbitrary URL injection.vct_url/integritynot validated as required (handlers.go)validate:"required,url"toVCTUrlandvalidate:"required"toIntegrityinCreateCredentialRequest.UIMetadatausesvctm.VCTinstead of resolved URL (handlers_ui.go)GetVCTURL()(the resolved URL used in credentials/DCQL queries), falling back tovctm.VCTonly when the URL is empty.fetchVCTMhas no timeout/size limit (methods.go)http.DefaultClient.io.LimitReadercapped at 1 MiB to prevent excessive memory usage.Also adds
SetVCTURL/SetIntegritymethods toCredentialConstructorand updates issuer tests to populate credential constructors with matching VCT URL/integrity values.