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

[Bug] validateRequest() is not working when a query param includes a single quote (') #1059

Open
leon19 opened this issue Dec 17, 2024 · 6 comments · May be fixed by #1061 or #1064
Open

[Bug] validateRequest() is not working when a query param includes a single quote (') #1059

leon19 opened this issue Dec 17, 2024 · 6 comments · May be fixed by #1061 or #1064

Comments

@leon19
Copy link

leon19 commented Dec 17, 2024

Issue Summary

The validateRequest() function is not working properly when a query param value includes a single quote (') (and probably more special characters)

This bug seems to be introduced after this commit 18c6d6f

Why?

The quote gets escaped when using new URL(), and the Twilio sever seems to generate the signature with an unescaped quote

const a = "https://example.com/path?test=foo'bar"
const b = new URL(a).toString() // 'https://example.com/path?test=foo%27bar'

Steps to Reproduce

  1. Setup a call with a redirect URI that has a query param with a quote in it
  2. When the call is redirected to the server, the validation does not pass
  3. This will also happen if ToState, FromState, or any other query param automatically added by Twilio includes a quote, and the server returns a 307 - Temporary Redirect to a different URL, for example Forli' or Trezzo Sull'Adda
    image

Our use case

  1. A caller starts a call to the state Trezzo Sull'Adda
  2. The caller hangs up
  3. We receive the hang-up command via POST and respond with 307 - Temporary Redirect to Location: https://api.example.com
  4. api.example.com receives the redirect with GET method and body as query param instead
  5. validateRequest() now fails because of the single quote

Code Snippet

const { getExpectedTwilioSignature, validateRequest } = require('twilio/lib/webhooks/webhooks');

const token = 'authToken';
const url = `https://example.com/path?test=param'WithQuote`;
const signature = getExpectedTwilioSignature(token, url, {});
const signatureUsingNewUrl = getExpectedTwilioSignature(token, new URL(url).toString(), {});

console.log(validateRequest(token, signature, new URL(url).toString(), {})); // false
console.log(validateRequest(token, signature, url, {}));  // false

console.log(validateRequest(token, signatureUsingNewUrl, new URL(url).toString(), {})); // true
console.log(validateRequest(token, signatureUsingNewUrl, url, {})); // true

Exception/Log

  • The validation returns false

Technical details:

  • twilio-node version: 5.4.0
  • node version: v22.11.0
@manisha1997
Copy link
Contributor

manisha1997 commented Jan 8, 2025

Hello, The change here has the exact same functionality while encoding and don't feel like is causing this issue.

import Url from "url-parse";
const url = "https://example.com/path?test=param'WithQuote&Body=It's+amazing";

console.log(new Url(url));
Output:
URL {
  href: 'https://example.com/path?test=param%27WithQuote&Body=It%27s+amazing',
  origin: 'https://example.com',
  protocol: 'https:',
  username: '',
  password: '',
  host: 'example.com',
  hostname: 'example.com',
  port: '',
  pathname: '/path',
  search: '?test=param%27WithQuote&Body=It%27s+amazing',
  searchParams: URLSearchParams { 'test' => "param'WithQuote", 'Body' => "It's amazing" },
  hash: ''
}

In built URL

const url = "https://example.com/path?test=param'WithQuote&Body=It's+amazing";
console.log(new URL(url));
URL {
  href: 'https://example.com/path?test=param%27WithQuote&Body=It%27s+amazing',
  origin: 'https://example.com',
  protocol: 'https:',
  username: '',
  password: '',
  host: 'example.com',
  hostname: 'example.com',
  port: '',
  pathname: '/path',
  search: '?test=param%27WithQuote&Body=It%27s+amazing',
  searchParams: URLSearchParams { 'test' => "param'WithQuote", 'Body' => "It's amazing" },
  hash: ''
}

@manisha1997
Copy link
Contributor

Can you elaborate further what is the use case here which is causing problem for you?

@leon19
Copy link
Author

leon19 commented Jan 8, 2025

@manisha1997 I'm a bit confused about the role of url-parse in your reply 🤔. I'd appreciate any clarification.

Use Case:

I’m trying to use the validateRequest() function from the twilio-node package to validate a Twilio signature generated by a Twilio server.

The Scenario That Fails:

  1. I set up a status callback to handle when the caller hangs up: https://api.example.com/status-callback

  2. This status callback receives aPOST request: POST https://api.example.com/status-callback

  3. The status callback returns a 307 - Temporary Redirect with the header Location: https://api.example.com/hang-up

  4. As per the spec, the 307 status code should preserve the original HTTP method (see MDN documentation) but that's a different issue. In this case, Twilio sends a GET request to https://api.example.com/hang-up, with the body parameters converted into a query string.

  5. When I use validateRequest() to validate the x-twilio-signature header, it fails if any of the parameters in the query string contain a single quote ('), such as FromState or ToState (FromState=Trezzo Sull'Adda)

What Seems to Be Happening:

It looks like the single quote (') is escaped when using new URL(), but the Twilio server seems to generate the signature with the unescaped quote, causing the validation to fail.

leon19 added a commit to leon19/twilio-node that referenced this issue Jan 9, 2025
leon19 added a commit to leon19/twilio-node that referenced this issue Jan 9, 2025
leon19 added a commit to leon19/twilio-node that referenced this issue Jan 9, 2025
leon19 added a commit to leon19/twilio-node that referenced this issue Jan 9, 2025
leon19 added a commit to leon19/twilio-node that referenced this issue Jan 9, 2025
@manisha1997 manisha1997 linked a pull request Jan 14, 2025 that will close this issue
8 tasks
@manisha1997
Copy link
Contributor

We found the issue and working on a fix

leon19 added a commit to leon19/twilio-node that referenced this issue Jan 14, 2025
@leon19
Copy link
Author

leon19 commented Jan 14, 2025

@manisha1997 You can take also a look at what I did in 😃 #1061

@manisha1997
Copy link
Contributor

sure, I am taking a look at that too

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