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

634 double encoding breaks query params #680

Merged
merged 11 commits into from
Aug 12, 2021

Conversation

josiah14
Copy link
Contributor

@josiah14 josiah14 commented Aug 11, 2021

Problem Description

There was an issues with the building of some of the requests to the GitHub API which caused the query parameters to get encoded twice, first by the java.net.URLEncoder class and then by the org.http4s.Uri.encode function. Doing this would cause characters like / to get encoded as %2F and then as %252F, which the GitHub API would then decode only one layer down to %2F, causing inaccurate search results to come back both for the Repos.searchRepos and Issues.searchIssues functions.

Applied Fix Description

Removed all usages of java.net.URLEncoder and allow all encoding to be done by Http4sSyntax for all requests to the Github API.

Other Minor Changes

Some other minor improvements were made as part of this bugfix:

  • Unix utilities work more reliably if files end in a newline character. This character was missing at the end of logback.xml, so it was added.
  • In the testing utilities, githubApiUrl was not using TLS for its requests, so its value was altered to use https instead of http.
  • In the Repos.searchRepos unit test spec, "search/repositories" was being supplied as the url to the mock HttpClient. This to me made the test seem less realistic and could be brittle if validation was ever added within the HttpClient to validate the string received is actually a URL so as not to blindly accept just any string, so I changed the test to use the githubApiUrl value from the test utilities, instead.

@josiah14 josiah14 requested review from sloshy and Daenyth August 11, 2021 15:18
@josiah14 josiah14 linked an issue Aug 11, 2021 that may be closed by this pull request
BenFradet
BenFradet previously approved these changes Aug 12, 2021
Copy link
Contributor

@BenFradet BenFradet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

@josiah14 josiah14 merged commit 7bba931 into main Aug 12, 2021
@josiah14 josiah14 deleted the 634-double-encoding-breaks-query-params branch August 12, 2021 12:12
@josiah14 josiah14 removed request for Daenyth and sloshy August 13, 2021 14:06
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.

"/" not accepted in searchRepos 's query
2 participants