Clarification Needed on Nonce Validation Logic

[1thrasher]

I'm confused with the nonce validation logic in the validatePayloadNonce method within ToolLaunchValidator.php.

A nonce should be validated to ensure it exists, has not been used, and has not expired. The expected behavior is to invalidate (expire or remove) the nonce after a successful validation to ensure it cannot be reused.

However, the implementation appears to deviate from this expected behavior in the following manner:

• It checks if the nonce claim is missing in the payload and throws an exception if it is. (correct)
• It retrieves the nonce value and searches for it in the repository. (correct)
• If the nonce is found and not expired, an exception is thrown. (Wait, why? Shouldn't it be if it has expired?)
• If the nonce is not found, a new nonce is created and saved, treating the validation as successful. (Again, why? Shouldn't it throw an exception since it cannot validate the payload nonce?)

Perhaps I'm misunderstanding the nonce's purpose, but treating its expiration or absence in the repository as a valid seems to be the opposite of what we want. (See the first 4-minutes of the video here.)

Can you help clarify my confusion here?

Thank you for looking into this.

[wazelin]

These seem to be valid concerns, @1thrasher. Thank you for raising the issue!

We will plan the work on a fix.

[marcovdb]

@wazelin Great! While we're on the subject of nonces, any chance you could you also take a look at #154 and #188? Because that is actually breaking important functionality right now, as far as I can see.

[ddelblanco]

Hello

I'm not a php developer, so please take this suggestion as just a guide of what I think can be changed. (That is why this is not a PR and just a comment)

I suggest to change the validatePayloadNonce to something like this (please refine it)

 /**
     * @throws LtiExceptionInterface
     */
    private function validatePayloadNonce(LtiMessagePayloadInterface $payload, MessagePayloadInterface $state): self
    {
        if (empty($payload->getToken()->getClaims()->get(LtiMessagePayloadInterface::CLAIM_NONCE))) {
            throw new LtiException('ID token nonce claim is missing');
        }

        $nonceValue = $payload->getMandatoryClaim(MessagePayloadInterface::CLAIM_NONCE);

        $nonce = $this->nonceRepository->find($nonceValue);

        if (null !== $nonce) {
            if ($nonce->isExpired()) {
                throw new LtiException('ID token nonce claim is expired');
            }
        } else {
            throw new LtiException('ID token nonce claim does not exist');
        }

        // Check if nonce is the one used in the state token
        if ($this->isStateValidationRequired()) {
            $stateNonce = $state->getMandatoryClaim(MessagePayloadInterface::CLAIM_NONCE);
            if ($stateNonce !== $nonceValue) {
            throw new LtiException('ID token nonce does not match the one used in the state token');
            }
        }

        $this->addSuccess('ID token nonce claim is valid');

        return $this;
    }

NOTE: Not sure is this ($stateNonce !== $nonceValue) will compare the nonce values or not... maybe we need a getValue() there for each... but I will leave that to the PHP experts :)

Where the concerns that @1thrasher are solved and at the same time, there is a new check that controls that the state and the nonce belong to the same original oidc request.

This would imply to change line 94 in the same file to:

if ($this->isNonceValidationRequired()) {
           $this->validatePayloadNonce($payload, $state);
}

In addition to this, the nonce should be deleted once it is used once, and don't rely only on the expiration date. If not, someone could use it more than one time.

As said, I'm not a PHP person, but around line 105, before the return, something like this is needed (again, just a suggestion)

            // delete the nonce
            $this->nonceRepository->delete($payload->getMandatoryClaim(MessagePayloadInterface::CLAIM_NONCE));

I'm not sure if the nonce in the repository is used later (for example in the deep link) but if it is we should only delete it here if it is not a deep link request and then delete it when the deep link request finishes using it.

I hope this helps.