fix: assert valid jarm response #2117
Conversation
|
|
Maybe we can add a test covering this? |
auer-martin
force-pushed
the
fix/assert-jarm-response-mode
branch
from
5cc8ed1 to
eb2aa20
Compare
|
I added a basic test. It works, but I am not sure if we should include it like this. |
| credentials: selectedCredentials, | ||
| }, | ||
| }) | ||
| ).rejects.toThrow() |
There was a problem hiding this comment.
I think this test makes sense and is a good addition 👍 . Is there anything returned in the error indicating we're missing response encryption? Would be nice if this toThrow contains something that asserts it's the JARM that is failing here. (it also helps the client interacting with Credo as a verifier to see what's missing)
There was a problem hiding this comment.
Nah, there was not. I added a cause property now.
auer-martin
force-pushed
the
fix/assert-jarm-response-mode
branch
from
eb2aa20 to
e96566d
Compare
Signed-off-by: Martin Auer <[email protected]>
Signed-off-by: Martin Auer <[email protected]>
auer-martin
force-pushed
the
fix/assert-jarm-response-mode
branch
from
e96566d to
b27af5a
Compare
| @@ -83,7 +83,7 @@ export function sendErrorResponse( | |||
| error: unknown, | |||
| additionalPayload?: Record<string, unknown> | |||
| ) { | |||
| const body = { error: message, ...additionalPayload } | |||
| const body = { error: message, ...(error instanceof Error && { cause: error.message }), ...additionalPayload } | |||
There was a problem hiding this comment.
This is not compliant with the spec. there is error and error_description. But we should be cautious with just returning the error message for unknown errors.
For oid4vci i reworked it to now throw a Oauth2ServerErrorResponseError and in the place where sendErrorResponse is called i first check for instanceof Oauth2ServerErrorResponseError and then call sendOauth2ErrorResponse. Then we make sure only error messages that should be leaked to the client are leaked. (Because we only construct a Oauth2ServerErrorResponseError if needed. Can you udpate?
TimoGlastra
merged commit 07a60cd
into
openwallet-foundation:main
No description provided.