Lex v2 SessionID out of spec in some cases

[t-jones]

Recently saw this issue with a colleague using document chaining. Qnabot uses the username passed from cognito as the lex session ID when it makes calls to lex v2. The format of this value is specified in the lex documentation as [0-9a-zA-Z._:-]+. In this specific case, users are being federated into cognito and have their email as a username, so the request to lex contains @ in the sessionid and subsequently throws a validation error, causing the flow to fail.

To Reproduce

• Require authentication for the qnabot client.
• Create a user in cognito with an @ in their username.
• Set up and test the Document Chaining demo.

Expected behavior It should succeed without error.

Please complete the following information about the solution:

• [x] Version: 5.2.0
• [x] Region: us-west-2
• [x] Was the solution modified from the version published on this repository? no
• [x] Have you checked your service quotas for the services this solution uses? N/A
• [x] Were there any errors in the CloudWatch Logs? Yes

2022-12-03T22:58:01.376Z	fcc96118-64e6-4bc0-9e08-730378cf8864	ERROR	Invoke Error 	{
    "errorType": "Error",
    "errorMessage": "Lex client request error:ValidationException: 1 validation error detected: Value '[email protected]' at 'sessionId' failed to satisfy constraint: Member must satisfy regular expression pattern: [0-9a-zA-Z._:-]+",
    "stack": [
        "Error: Lex client request error:ValidationException: 1 validation error detected: Value '[email protected]' at 'sessionId' failed to satisfy constraint: Member must satisfy regular expression pattern: [0-9a-zA-Z._:-]+",
        "    at intoError (file:///var/runtime/index.mjs:46:16)",
        "    at postError (file:///var/runtime/index.mjs:711:51)",
        "    at callback (file:///var/runtime/index.mjs:727:11)",
        "    at file:///var/runtime/index.mjs:782:20",
        "    at router.start (/var/task/lib/router/index.js:24:17)",
        "    at processTicksAndRejections (node:internal/process/task_queues:96:5)"
    ]
}

Additional context

The value is set on ln 131: https://github.com/aws-solutions/qnabot-on-aws/blob/6e59da9d5e911c5c01146202422eabffc85d97f8/lambda/fulfillment/lib/middleware/lexRouter.js#L131 On the next line there is a length check done, so some validation is already happening: https://github.com/aws-solutions/qnabot-on-aws/blob/6e59da9d5e911c5c01146202422eabffc85d97f8/lambda/fulfillment/lib/middleware/lexRouter.js#L132 As a temporary patch, I added a string replace of @ with _ right after this. A more robust solution would be to replace any character not in the spec with an _ but not sure if this would have downstream consequences. Another option: use a different field as the lex sessionid such as req._userId which is set to the cognito session ID (is this unique for anonymous users?). Advise and I'll send you a PR.

[rstrahan]

Hi @t-jones - Thanks for catching this one.

Another option: use a different field as the lex sessionid such as req._userId which is set to the cognito session ID (is this unique for anonymous users?).

That would work. However, using the _userInfo.UserId value was intended to add value by allowing the nested V2 bot to identify an authenticated user and potentially use the username to return differentiated content. Probably a better (more robust) approach would be to do as you suggest, and simply relay req._userId to the nested bot, and perhaps add JSON serialized _userInfo as a session attribute for any nested bot that needs to identify users.

[t-jones]

@rstrahan Should we relay req._userid to the nested bot only in the case where sessionid fails validation? Otherwise it could break existing users is my thinking. At the same time, adding JSON serialized _userinfo as a session attribute. Then remove _userInf.UserId with 6.x.

[rstrahan]

Agreed.. That's a good approach to avoid possibility of backward compatability probs. Tx.

[bios6]

Hi @t-jones ,

Thanks for raising this issue, we will look into this.

[fhoueto-amz]

This is an enhancement

[fhoueto-amz]

@t-jones We will include this in our next major release.