Skip to content

fix: reject proof signed with non-attested key - #504

Merged
vafeini merged 2 commits into
eu-digital-identity-wallet:mainfrom
asserh:fix/error-on-non-attested-proof-key
Feb 16, 2026
Merged

fix: reject proof signed with non-attested key#504
vafeini merged 2 commits into
eu-digital-identity-wallet:mainfrom
asserh:fix/error-on-non-attested-proof-key

Conversation

@asserh

@asserh asserh commented Feb 10, 2026

Copy link
Copy Markdown
Contributor

Calling Flow.first() caused a NoSuchElementException when no matching attested key was found, resulting in a generic error message.

Use firstOrNull() and return an explicit InvalidProof instead.

@asserh

asserh commented Feb 10, 2026

Copy link
Copy Markdown
Contributor Author

Found this subtle issue when working on key attestations for a wallet implementation. The generic error message threw me off, but with this change it should be clearer when the proof validation fails in this exact way.

Really fantastic job on the project by the way! It has been extremely useful for our internal testing and development here at DIGG.

@babisRoutis

Copy link
Copy Markdown
Contributor

Dear @asserh

Thank you for reporting this and for filling the PR.
Please give us some time, to evaluate the issue and the fix.

PS : I also want to thank you for your kind and supportive words. I am really happy to hear that you find it useful.

@babisRoutis
babisRoutis self-requested a review February 10, 2026 19:08
babisRoutis
babisRoutis previously approved these changes Feb 10, 2026

@babisRoutis babisRoutis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor comment to consider
In any case, it looks good.

@babisRoutis
babisRoutis requested a review from vafeini February 10, 2026 19:09
@babisRoutis babisRoutis added this to the pid-issuer v0.7.5 milestone Feb 10, 2026
@babisRoutis babisRoutis added the bug Something isn't working label Feb 10, 2026
@vafeini

vafeini commented Feb 11, 2026

Copy link
Copy Markdown
Contributor

@asserh Thanks for the PR, good catch :D
Can you move the unit test in WalletApiTest.kt. There is dedicated class there (WalletApiEncryptionOptionalKeyAttestationsRequiredTest) that is testing the key_attestation related proofs. There you can find convenient methods (keyAttestationJWT()) like the one you have created for generating key_attestation jwts.

@asserh

asserh commented Feb 11, 2026

Copy link
Copy Markdown
Contributor Author

Absolutely, I will move it over to WalletApiTest instead 👍 Did not even notice there already were existing methods for generating attestations. Will get back to you with an update

@asserh
asserh force-pushed the fix/error-on-non-attested-proof-key branch from 5a01604 to a799412 Compare February 11, 2026 15:38
@asserh

asserh commented Feb 11, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the suggestion, it made a lot of sense to relocate the test. It was a lot easier there with the existing utility functions. Does this look better?

Let me know if you prefer me to squash my commits manually. Although it looks like you already use squash and merge in Github for PR:s.

@babisRoutis
babisRoutis self-requested a review February 12, 2026 16:08
Calling `Flow.first()` caused a NoSuchElementException when no matching
attested key was found, resulting in a generic error message.

Use `firstOrNull()` and return an explicit InvalidProof instead.

Signed-off-by: Asser Hakala <extern.asser.hakala@digg.se>
Revert changes in ValidateJwtProofTest since test is relocated

Signed-off-by: Asser Hakala <extern.asser.hakala@digg.se>
@babisRoutis
babisRoutis force-pushed the fix/error-on-non-attested-proof-key branch from a799412 to c513522 Compare February 16, 2026 07:03
@vafeini

vafeini commented Feb 16, 2026

Copy link
Copy Markdown
Contributor

Thanks for the suggestion, it made a lot of sense to relocate the test. It was a lot easier there with the existing utility functions. Does this look better?

Let me know if you prefer me to squash my commits manually. Although it looks like you already use squash and merge in Github for PR:s.

@asserh Looks good, PR approved, will be merged in main. Thanks for the fix!

@vafeini
vafeini merged commit 91760f5 into eu-digital-identity-wallet:main Feb 16, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Development

Successfully merging this pull request may close these issues.

5 participants