Repository navigation
fix(serializers): chain the reason of the serializer a token belongs to - #818
Merged
Merged
Conversation
When no serializer could convert a token, the managers chained the exception of a bound or, without one, that of the last serializer they tried. A token rejected by the serializer of its format for another reason was then reported with the exception of a serializer of another format, which had only rejected the token as foreign. A compact token whose protected header is not a JSON object was reported as invalid JSON by the general serializer. A serializer now rejects an input of another serialization with an UnrecognizedSerializationException, a subclass of InvalidSerializationException. That is a compact serialization of another number of parts or whose protected header part is not base64url, or a JSON document that is not valid JSON, not an object, or without the members of the serialization. The managers chain the exception of a bound first, then the last exception without that mark, and the exception of the last serializer otherwise. Messages and codes are unchanged.
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.
Follow-up of #817, whose "Not covered" section listed this.
The defect
When no serializer can convert a token,
JWSSerializerManagerandJWESerializerManagerthrowUnsupported input.and chain one exception, so that the reason remains available. Since #817, that exception is the one that goes back to a bound of the JSON decoder; otherwise, it is the exception of the last serializer tried.The serializers did not tell two cases apart:
So a token rejected by the serializer of its own format, for a reason other than a bound, was reported with the exception of a serializer that had only found it foreign. For example:
Bad protected header., but the manager chained the JSON general serializer'sUnsupported input.: a syntax error, because a compact token is not JSON.Syntax error, but the manager chained the general serializer'sUnsupported input., which only says that thesignaturesmember is missing.The reason was kept only when the serializer of the token happened to be the last one tried.
The fix
A new
Jose\Component\Core\Exception\UnrecognizedSerializationException, which extendsInvalidSerializationException. A serializer throws it when the input is not in its serialization at all:signature,signatures,ciphertext,recipients).BoundedJsonDecoder::decodeSerialization()(@internal) decodes the JSON document of a JSON serializer. It turns invalid JSON into that exception, keeping its message, code and chainedJsonException. A document beyond a bound is still rejected with aJsonBoundExceededException.The managers chain, in this order of priority:
A serializer of another library that marks the inputs it does not recognize is skipped in the same way. Its unmarked exceptions count as those of a serializer that recognized the input; they never hide a bound.
The messages and codes of every exception are unchanged, and so is the exception that the managers throw.
Backward compatibility
API. One new class,
UnrecognizedSerializationException, registered inJoseExceptionTestwith its SPL parent,InvalidArgumentException.BoundedJsonDecoder, which is@internal, gainsdecodeSerialization(). No signature changes.Exception classes. The rejections of an input of another serialization now throw the subclass:
InvalidSerializationException: the parts count of the compact serializers, a JSON document that is not an object, missing members;Jose\Component\Core\Exception\InvalidArgumentExceptionof the JSON decoder: invalid JSON given to a JSON serializer.A
catchofInvalidArgumentException, Jose or SPL, still catches them, and so does acatchofInvalidSerializationExceptionfor the first group. For the second group, acatchofInvalidSerializationExceptionnow catches them too, which it did not before.Managers. The exception they throw is unchanged. The exception they chain changes only when the serializer of the token rejected it and a serializer of another format came after it.
Behaviour. The probe of fix(serializers): chain the bound a token goes beyond in the serializer managers #817 ran the same 819 cases on 4.3.x (69e9709) and on this branch. In the base run, the 4.3.x versions of
BoundedJsonDecoder, of the six serializers and of the two managers were loaded before Composer's autoloader. The cases are the six serializers, the SD-JWT parser, JWK and JWKSet, plus every JWS and JWE token of that corpus through the two managers, with the serializers in three orders.UnrecognizedSerializationException: 20 wereInvalidSerializationExceptions and 12 wereInvalidArgumentExceptions.Unsupported input→Bad protected header.instead ofUnsupported input.→Syntax error.Verification
Signature\JWSSerializerManagerTestandEncryption\JWESerializerManagerTest(34 cases each):JoseExceptionTestregisters the new class.phpunit-11inphpqa:8.4: 2,958 tests, no failure. The 4 skipped and 2 incomplete tests are the PKCS#12 and ML-DSA platform tests.phpqa:8.4. The changed files passphp -lwith PHP 8.2.is_array()check, merged with the check of the members.Not covered