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.

Sign up for GitHub

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

Jump to bottom

[bug fix] Stop manual URI parsing/escaping and use CGI::parse #119

Merged
merged 6 commits into from
Dec 4, 2019
Merged

[bug fix] Stop manual URI parsing/escaping and use CGI::parse #119

merged 6 commits into from
Dec 4, 2019

Conversation

smaeda-ks
Copy link
Collaborator

@smaeda-ks smaeda-ks commented Oct 1, 2019 •
edited

Problem

Sometimes, users need to pass URL strings that contain query strings as a parameter.
E.g., website_url parameter from the POST accounts/:account_id/cards/website endpoint (Ads API).

In this case, they have to escape the URL strings in order to let the parser to parse parameters correctly:

raw: https://developer.twitter.com/params?aaa=bbb&ccc=ddd
parsed: https%3A%2F%2Fdeveloper.twitter.com%2Fparams%3Faaa%3Dbbb%26ccc%3Dddd

$ twurl -XPOST -H ads-api.twitter.com '/6/accounts/18ce54t1ol3/cards/website?media_key=3_1102477685765206016&name=WebsiteCardTest-buggy&website_title=WebsiteCardTest-buggy&website_url=https%3A%2F%2Fdeveloper.twitter.com%2Fparams%3Faaa%3Dbbb%26ccc%3Dddd' | jq

but the problem is, this code block is doing some extra job and hence the parsed parameter I included in the above twurl request gets "double-escaped".
https://github.com/twitter/twurl/blob/v0.9.3/lib/twurl/cli.rb#L161-L167

so the above twurl request will actually fail like this:

$ twurl -XPOST -H ads-api.twitter.com '/6/accounts/18ce54t1ol3/cards/website?media_key=3_1102477685765206016&name=WebsiteCardTest-buggy&website_title=WebsiteCardTest-buggy&website_url=https%3A%2F%2Fdeveloper.twitter.com%2Fparams%3Faaa%3Dbbb%26ccc%3Dddd' | jq
{
  "errors": [
    {
      "code": "INVALID_PARAMETER",
      "message": "Invalid URL encoded params",
      "parameter": "website_url"
    }
  ],
  "request": {
    "params": {
      "name": "WebsiteCardTest-buggy",
      "account_id": "18ce54t1ol3",
      "media_key": "3_1102477685765206016",
      "website_title": "WebsiteCardTest-buggy",
      "website_url": "https%3A%2F%2Fdeveloper.twitter.com%2Fparams%3Faaa%3Dbbb%26ccc%3Dddd"
    }
  }
}

Solution

Just stop doing manual URI parsing and use the CGI::parse method instead.

Result

$ twurl -XPOST -H ads-api.twitter.com '/6/accounts/18ce54t1ol3/cards/website?media_key=3_1102477685765206016&name=WebsiteCardTest-buggy&website_title=WebsiteCardTest-buggy&website_url=https%3A%2F%2Fdeveloper.twitter.com%2Fparams%3Faaa%3Dbbb%26ccc%3Dddd' | jq
{
  "data": {
    "name": "WebsiteCardTest-buggy",
    "website_shortened_url": "https://t.co/Vji3Zyh076",
    "image_display_height": "800",
    "media_url": "https://pbs.twimg.com/media/D0zJnTDUcAA0snp.png",
    "website_display_url": "developer.twitter.com",
    "id": "84w31",
    "website_dest_url": "https://developer.twitter.com/params",
    "media_key": "3_1102477685765206016",
    "created_at": "2019-10-01T02:20:03Z",
    "image_display_width": "800",
    "website_title": "WebsiteCardTest-buggy",
    "card_uri": "card://1178857075960471554",
    "website_url": "https://developer.twitter.com/params?aaa=bbb&ccc=ddd",
    "updated_at": "2019-10-01T02:20:03Z",
    "deleted": false,
    "card_type": "WEBSITE"
  },
  "request": {
    "params": {
      "name": "WebsiteCardTest-buggy",
      "website_shortened_url": "https://t.co/Vji3Zyh076",
      "image_display_height": "800",
      "media_url": "https://pbs.twimg.com/media/D0zJnTDUcAA0snp.png",
      "website_display_url": "developer.twitter.com",
      "account_id": "18ce54t1ol3",
      "website_dest_url": "https://developer.twitter.com/params",
      "media_key": "3_1102477685765206016",
      "image_display_width": "800",
      "website_title": "WebsiteCardTest-buggy",
      "website_url": "https://developer.twitter.com/params?aaa=bbb&ccc=ddd",
      "card_type": "WEBSITE"
    }
  }
}

As you can see, with this patch,
"website_url": "https://developer.twitter.com/params?aaa=bbb&ccc=ddd",
which is what I expect to see.

Other bug fixes

This PR includes other bug fixes that will resolve the below issues:
#117
#77 (initial PR was: #108)

@smaeda-ks
Stop manual URI parsing/escaping
5bf47da 
and use URI::HTTP standard library.
@coveralls
Copy link

coveralls commented Oct 1, 2019 •
edited

Coverage Status

Coverage decreased (-0.2%) to 87.982% when pulling e236eda on smaeda-ks:smaeda-ks/fix-uri-parse into 7eb9a13 on twitter:master.

@smaeda-ks smaeda-ks mentioned this pull request Oct 1, 2019
Merged
@andypiper andypiper self-assigned this Oct 1, 2019
@andypiper
Copy link
Contributor

Hey @smaeda-ks thank you! i'll connect with you and @dlamacchia as I'm not sure what the best approach is to getting this merged. Appreciate it!

@smaeda-ks
Copy link
Collaborator Author

@andypiper @dlamacchia
Based on the test results, I'm fine with merging this to the master branch and see if other outstanding PRs can pass the test afterward. (I'm not talking about a version release here, but just maintaining the upstream branch. Releasing a new version would be a separate discussion, I guess.)

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@smaeda-ks
Copy link
Collaborator Author

ping @dlamacchia

@smaeda-ks
Copy link
Collaborator Author

Just found some problems with this PR, will add more commits so please hold on. Thanks

@smaeda-ks
Use CGI::parse
5c05a68 
- URI::decode_www_form can't handle multibyte characters
@smaeda-ks smaeda-ks changed the title [bug fix] Stop manual URI parsing/escaping and use URI::HTTP standard library [bug fix] Stop manual URI parsing/escaping and use CGI::parse Nov 27, 2019
@smaeda-ks
Copy link
Collaborator Author

@dlamacchia OK, so I fixed a bug in the latest commit: 5c05a68 and now I think it's finally safe to merge.

cc: @osowskit since you originally gave me an idea of this fix.

@smaeda-ks smaeda-ks mentioned this pull request Nov 28, 2019
Closed
@smaeda-ks smaeda-ks self-assigned this Nov 28, 2019
@smaeda-ks
avoid using set_form_data()
2bcda26 
- as it doesn't seem to be escaping some of the special characters such as '*' (asterisk). Use CGI.escape instead.
- do not parse/escape request POST body in case if "content-type" request header is specified.
@smaeda-ks
Copy link
Collaborator Author

smaeda-ks commented Nov 28, 2019 •
edited

@dlamacchia @andypiper

I updated the description but now this PR includes two more major bug fixes:
#77
#117

Those are also parsing/escaping issues. I'm really eager to merge this fix.

@smaeda-ks smaeda-ks mentioned this pull request Nov 28, 2019
Merged
@smaeda-ks smaeda-ks requested a review from a team November 28, 2019 20:58
@grimreaper grimreaper removed the request for review from a team November 29, 2019 02:48
@smaeda-ks smaeda-ks added this to the Target v0.9.4 milestone Nov 29, 2019
@smaeda-ks
Update .travis.yml
e236eda 
- follow latest Ruby releases
@smaeda-ks smaeda-ks merged commit 5b03096 into twitter:master Dec 4, 2019
@osowskit osowskit mentioned this pull request Aug 31, 2020
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andypiper andypiper andypiper approved these changes

@dlamacchia dlamacchia Awaiting requested review from dlamacchia

Assignees

@smaeda-ks smaeda-ks

Labels
BugFix
Milestone
Target v0.9.4
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@smaeda-ks @coveralls @andypiper @CLAassistant