Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RequestValidator marks valid signature invalid when having multiple values attached to one parameter #613

Closed
hugo-netcraft opened this issue Jul 22, 2022 · 4 comments · Fixed by #615 or #617
Labels
type: bug bug in the library

Comments

@hugo-netcraft
Copy link

hugo-netcraft commented Jul 22, 2022

Issue Summary

In some very specific circumstances, the current signature generation/validation does not match the given valid signature (or Twilio's server's logic for generating a signature), which causes a valid request to be marked as invalid. This may also be an issue in the other language, I have not tested them all.

The circumstance is when using Autopilot, you can produce a request with duplicate keys in the POST parameters. This is done by using the same field to pick up multiple values.

I personally do not know what Twilio's servers actually do to generate the request, so you'll have to look into that yourself. I have already tried brute forcing it to figure out how formats parameters when this happens but I have had no luck in that area.

Steps to Reproduce

  1. Setup a Python server from the code snippet below
  2. Create new Autopilot bot
  3. Add a task called numbers, program the task with the following (replacing the url with your own):
{
	"actions": [
		{
			"redirect": "https://****-****-****-*-****-***-****-****-****.eu.ngrok.io/autopilot"
		}
	]
}
  1. Add sample to the task: {numbers} hi {numbers} hi {numbers}, making sure to make numbers use field type Twilio.NUMBER
  2. Use the simulator to validate the server is marking a normal request as valid, by inputing: 6 hi hi. The server should log Request Valid and say Hello
  3. Now use the simulator to test the edge case 6 hi 4 hi 1, the server will log this as an invalid request

Code Snippet

from flask import Flask, request
from twilio.request_validator import RequestValidator

app = Flask(__name__)

@app.route('/autopilot', methods=['POST'])
def autopilot():
    print(request.form)
    print(request.form.get('Field_number_Value'))

    # Please insert your auth token below
    auth_token='----------------'
    validator = RequestValidator(auth_token)
    sig = request.headers['X-Twilio-Signature']
    # Please insert your ngrok url below if you are using ngrok, otherwise you can change it to request.url
    url = 'https://****-****-****-*-****-***-****-****-****.eu.ngrok.io/autopilot'
    if validator.validate(url, request.form, sig):
        print('Request valid')
    else:
        print('Request invalid')

    return { 'actions': [ { 'say' : 'Hello' } ] }

Exception/Log

The output for the invalid request is:

ImmutableMultiDict([('CurrentTask', 'numbers'), ('Field_number_Value', '6'), ('Field_number_Value', '4'), ('Field_number_Value', '1'), ('Memory', '{"twilio":{"chat":{"ChannelSid":"CHXXXXXXXXXXXXXXXXXXXXXXXXXXXX","AssistantName":"","Attributes":{},"ServiceSid":"ISXXXXXXXXXXXXXXXXXXXXXXXXX","Index":17,"From":"","MessageSid":"IMXXXXXXXXXXXXXXXXXXXXXXXX"}}}'), ('Channel', 'chat'), ('NextBestTask', ''), ('CurrentTaskConfidence', '1.0'), ('AccountSid', 'ACXXXXXXXXXXXXXXXXXXXXXXXXXX'), ('CurrentInput', '6 hi 4 hi 1'), ('DialogueSid', 'UKXXXXXXXXXXXXXXXXXXXXXXXXXX'), ('DialoguePayloadUrl', 'https://autopilot.twilio.com/v1/Assistants/UAXXXXXXXXXXXXXXXXXXXXXXXXXXX/Dialogues/UKXXXXXXXXXXXXXXXXXXXXXXXXXXX'), ('AssistantSid', 'UAXXXXXXXXXXXXXXXXXXXXXXXXXX'), ('Field_number_Type', 'Twilio.NUMBER'), ('UserIdentifier', '')])
6
Request invalid
127.0.0.1 - - [22/Jul/2022 11:29:07] "POST /autopilot HTTP/1.1" 200 -

As you can see from the above, Field_number_Value is duplicated multiple times for each of the values.

Then on the next line you can see when trying to access Field_number_Value, it only returns 6 - this is not the actual problem but helps to explain the next part.

From my testing, I have found that these lines (found here)

for k, v in sorted(params.items()):
    s += k + v

ignore the duplicate keys and only adds the first key in the dictionary. E.g. we have the above, it will only add Field_number_Value6 to s.

Yet again this is also not the issue as I've tried to add a key for each value and it still produces invalid signature, but it is where the logic is differing from Twilio's servers.

Technical details:

  • twilio-python version: 7.11.0
  • python version: 3.6.8
@hugo-netcraft hugo-netcraft changed the title Signature generation does not match Twilio servers when having multiple values for one key RequestValidator marks valid signature invalid when having multiple values attached to one parameter Jul 22, 2022
@childish-sambino
Copy link
Contributor

childish-sambino commented Jul 25, 2022

This issue has been added to our internal backlog to be prioritized. Pull requests and +1s on the issue summary will help it move up the backlog (ref: DI-1002).

@childish-sambino childish-sambino added type: bug bug in the library status: help wanted requesting help from the community labels Jul 25, 2022
@hugo-netcraft
Copy link
Author

Hi there,

I have been able to figure out how the parameters should be formatted to produce a valid signature.

So given ('Field_number_Value', '6'), ('Field_number_Value', '4'), ('Field_number_Value', '1') we must turn it into

Field_number_Value3Field_number_Value4Field_number_Value6, where the values are sorted lexically, so if you have the values 43, 24 and 9 should be turned into Field_number_Value24Field_number_Value43Field_number_Value9

@hugo-netcraft
Copy link
Author

Hi, we have since found another edge case this doesn't work with the fix. When some parameters are the same e.g. 9 hi 9 hi 9 or 8 hi 9 hi 9, it seems we get a signature which is created from a list of parameters which are uniq, so we don't have duplicate 9's. However please test this a bit more as I haven't been able to fully test the theory.

@childish-sambino
Copy link
Contributor

@hugo-netcraft Good catch. Fixed by #617

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug bug in the library
Projects
None yet
2 participants