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

Encode query parameters regardless of the browser #104

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

phyllipy
Copy link

Problem: Query parameters are not being encoded as expected

value: this&that

Browser: Firefox 93.0 (64-bit) Mac OS Big Sur

Screen Shot 2021-10-21 at 11 35 40
Screen Shot 2021-10-21 at 11 35 55

Google Chrome 94.0.4606.81 (Official Build) (x86_64) (Big Sur)

Screen Shot 2021-10-21 at 11 37 44

Screen Shot 2021-10-21 at 11 38 02

Solution: Update the parameterize helper to always encode the parameters (unless it is already encoded)

original issue: #103

@gstark
Copy link
Contributor

gstark commented Nov 29, 2021

I have this same issue and would love to see this PR land. However, I wonder if the test of value.indexOf("%") is too simplistic.

MDN gives this method for determining if a string contains encoded components.

function containsEncodedComponents(x) {
  // ie ?,=,&,/ etc
  return (decodeURI(x) !== decodeURIComponent(x));
}

We may also want to add a try/catch for URIError and assume false?

// From: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/decodeURIComponent
//
// Determines if a string already has encoded elements
export function containsEncodedComponents(stringToTest: string) {
  try {
    // ie ?,=,&,/ etc
    return decodeURI(stringToTest) !== decodeURIComponent(stringToTest)
  } catch (URIError) {
    return false
  }
}

@phyllipy
Copy link
Author

phyllipy commented Dec 23, 2021

I have this same issue and would love to see this PR land. However, I wonder if the test of value.indexOf("%") is too simplistic.

MDN gives this method for determining if a string contains encoded components.

function containsEncodedComponents(x) {
  // ie ?,=,&,/ etc
  return (decodeURI(x) !== decodeURIComponent(x));
}

We may also want to add a try/catch for URIError and assume false?

// From: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/decodeURIComponent
//
// Determines if a string already has encoded elements
export function containsEncodedComponents(stringToTest: string) {
  try {
    // ie ?,=,&,/ etc
    return decodeURI(stringToTest) !== decodeURIComponent(stringToTest)
  } catch (URIError) {
    return false
  }
}

Yeah I agree using value.indexOf("%") is a rather naive approach. However, the snippet above doesn't really work in all cases. For example, using comparing the string: http://example.com/api/v1/people?filter[name]=%22Jane%22 (used in one of the tests) we have:

value = "http://example.com/api/v1/people?filter[name]=%22Jane%22"
decodeURIComponent(value)   // =>  "http://example.com/api/v1/people?filter[name]=\"Jane\""
decodeURL(value)  // => "http://example.com/api/v1/people?filter[name]=\"Jane\"" 
decodeURI(value) !== decodeURIComponent(value)  // => false

This would result in a double encoding. But we can use decodeURI(value) and compare it with the original string, if equals then the string is not enconded.

value = "http://example.com/api/v1/people?filter[name]=%22Jane%22"

decodeURI(value) === value  // =>  false

@zailleh
Copy link

zailleh commented Feb 28, 2022

@phyllipy I suspect you don't want to have commited package-lock.json here

mdcarreira pushed a commit to mdcarreira/spraypaint.js that referenced this pull request Jan 26, 2023
Solution was taken from existing PR in original spraypaint repository:
graphiti-api#104
mdcarreira added a commit to mdcarreira/spraypaint.js that referenced this pull request Jan 26, 2023
* Creates test for filtering with spectial characters

* Fixes encoding of special characters.
Solution was taken from existing PR in original spraypaint repository:
graphiti-api#104

Co-authored-by: Micael Carreira <micael.carreira@esab.se>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants