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

[API] Encode URI before doing URL substitutions #2394

Closed
mnquintana opened this issue Jun 11, 2015 · 9 comments
Closed

[API] Encode URI before doing URL substitutions #2394

mnquintana opened this issue Jun 11, 2015 · 9 comments
Milestone

Comments

@mnquintana
Copy link
Contributor

I'm trying to connect to a remote API that requires a query string with characters that need to be properly URI-encoded. I discovered that Semantic UI's API behavior module currently doesn't URI encode the url settings at all, which is a bit of an issue, because I can't encode the query string myself since I need to use a URL substitution to capture the user's query (if I do, it'll try to encode the curly brackets from {query}, which will cause URL substitution to be skipped. Would it be possible to URI encode the url before performing the substitutions?

(I'm on the next branch)

@jlukic
Copy link
Member

jlukic commented Jun 11, 2015

The URL is encoded during the $.ajax call. I'm not sure theres an issue there.

Try going to the search component page, for example, and typing in the prompt áááá. You will see in inspector/firebug that the URL is http://api.semantic-ui.com/search/%C3%A1%C3%A1%C3%A1

@tirdadc
Copy link

tirdadc commented Jun 21, 2015

Try this out with the @ symbol, it doesn't get encoded to %40 which causes 502 bad gateway errors for some results on your link. Also worth trying out with /, which doesn't get encoded either.

@jlukic
Copy link
Member

jlukic commented Jul 13, 2015

Searching for @ and @@ both work for me and return normal responses. The API endpoint sometimes gives 502 bad gateway because the nginx server cant handle too many simultaneous requests.

@jlukic jlukic closed this as completed Jul 13, 2015
@tirdadc
Copy link

tirdadc commented Jul 13, 2015

I see that now, but it's still not getting escaped. Try with % and it'll return a 400 Bad Request. Point is you should be consistently seeing escaped characters like %40 and %25. I can't manually work around this either with encodeURIComponent() because {query} doesn't seem to get interpolated when I do this:

apiSettings: {
  url: '/users/autocomplete/email/' + encodeURIComponent('{query}')
}

@jlukic
Copy link
Member

jlukic commented Jul 13, 2015

You would have to do as

apiSettings: {
  url: '/users/autocomplete/email/{query}',
  urlVariables: {
    query: encodeURIComponent(query)
  })
}

I can see both sides here. My fear is double encoding which becomes an issue when blackboxed JS code (like this API module) happens to modify your strings when they are passed along to the server. I guess the argument for is processData is on by default in jQuery.

I'll muddle this over.

@jlukic jlukic reopened this Jul 13, 2015
@jlukic jlukic added this to the 2.1 milestone Jul 13, 2015
@daneren2005
Copy link

For anyone else who comes across this, it is fairly easy to fix on the calling end with:

apiSettings: {
    url: 'get.php?term={query}',
    beforeSend: function(data) {
        data.urlData.query = encodeURIComponent(data.urlData.query);
    }
}

If it is encoding some special characters like mentioned above, it may cause other issues for some people. But I'm guessing the vast majority just care about ?, +, etc being encoded so this should work for them.

@jlukic
Copy link
Member

jlukic commented Jul 20, 2015

I'll be revisiting this shortly in 2.1

@jlukic
Copy link
Member

jlukic commented Jul 20, 2015

Added in 2.0.5, it will also check if value is already encoded to avoid issues with double encoding during replacement.

Keep in mind if you were encoding in beforeSend like @daneren2005 's example, you may be double encoding after 2.0.5

@jlukic
Copy link
Member

jlukic commented Aug 11, 2015

Added encodeParameters setting in 2.1

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

No branches or pull requests

4 participants