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

Ruby: Avoid double escaping path items #3093

Merged
merged 1 commit into from
Jun 5, 2019
Merged

Ruby: Avoid double escaping path items #3093

merged 1 commit into from
Jun 5, 2019

Conversation

ccouzens
Copy link
Contributor

@ccouzens ccouzens commented Jun 4, 2019

URI.encode is obsolete. CGI.escape, URI.encode_www_form or
URI.encode_www_form_component are recommended instead.
https://ruby-doc.org/stdlib-2.6/libdoc/uri/rdoc/URI/Escape.html#method-i-escape

URI.encode has different behaviour to CGI.escape:

URI.encode('hello/world?test%string')
=> "hello/world?test%25string"
CGI.escape('hello/world?test%string')
=> "hello%2Fworld%3Ftest%25string"

I recently raised pull request #3039
201cbdc

That pull request escapes path items at insertion.

Before either pull request, the path item 'hello?world' would go into
the URL as 'hello?world'. That behaviour was insecure as if an attacker
could control the path item value, they could change the URL the
application connected to.

After #3039 'hello?world' would go in as 'hello%253Fworld'. This was
safer than before, but it's still not correct.
If I'd realised at the time, I would have made it correct at the time.

What this pull request does is make it go in as 'hello%35world', which
is correct.

ApiClient::build_request_url was URI.encoding the whole path.
This wasn't protecting against all undesirable characters in the path
items, but was escaping % characters a 2nd time which was unhelpful.

I have additionally removed URI.encode from Configuration::base_url as I
can't see any benefit it could be bringing.
There is no justification for it in the commit where it was originally
added: 47c8597

CC: ruby technical committee @cliffano @zlx @autopp

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

see above

`URI.encode` is obsolete. `CGI.escape`, `URI.encode_www_form` or
`URI.encode_www_form_component` are recommended instead.
https://ruby-doc.org/stdlib-2.6/libdoc/uri/rdoc/URI/Escape.html#method-i-escape

URI.encode has different behaviour to CGI.escape:

```ruby
URI.encode('hello/world?test%string')
=> "hello/world?test%25string"
CGI.escape('hello/world?test%string')
=> "hello%2Fworld%3Ftest%25string"
```

I recently raised pull request #3039
201cbdc

That pull request escapes path items at insertion.

Before either pull request, the path item 'hello?world' would go into
the URL as 'hello?world'. That behaviour was insecure as if an attacker
could control the path item value, they could change the URL the
application connected to.

After #3039 'hello?world' would go in as 'hello%253Fworld'. This was
safer than before, but it's still not correct.
If I'd realised at the time, I would have made it correct at the time.

What this pull request does is make it go in as 'hello%35world', which
is correct.

ApiClient::build_request_url was URI.encoding the whole path.
This wasn't protecting against all undesirable characters in the path
items, but was escaping % characters a 2nd time which was unhelpful.

I have additionally removed URI.encode from Configuration::base_url as I
can't see any benefit it could be bringing.
There is no justification for it in the commit where it was originally
added: 47c8597
Copy link
Contributor

@autopp autopp left a comment

Choose a reason for hiding this comment

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

@ccouzens LGTM 👍
I have confirmed that the problem has been fixed by running the generated petstore client's Petstore::UserApi#get_user_by_name (in samples/openapi3/client/petstore/ruby).

BTW, as a confirmation just in case, is 'hello%35world' in the description of PR a mistake of 'hello%3Fworld'?

@ccouzens
Copy link
Contributor Author

ccouzens commented Jun 5, 2019

Thank you @autopp

BTW, as a confirmation just in case, is 'hello%35world' in the description of PR a mistake of 'hello%3Fworld'?

Yes, I should have put 'hello%3Fworld'

@autopp autopp merged commit 4e9d226 into OpenAPITools:master Jun 5, 2019
@autopp
Copy link
Contributor

autopp commented Jun 5, 2019

@ccouzens The PR is merged. Thanks 👍

jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Jun 6, 2019
* master:
  [Java][okhttp-gson] Update dependencies (OpenAPITools#3103)
  Link query parameter to model object (OpenAPITools#2710)
  scala-play-server: fix enum names for reserved words (OpenAPITools#3080)
  Add @Sunn to openapi generator core team (OpenAPITools#3105)
  fix NPE in go generator (OpenAPITools#3104)
  scala-play-server: fix API doc url (OpenAPITools#3096)
  [maven-plugin] fix strictSpec parameter without alias (OpenAPITools#3095)
  Ruby: Avoid double escaping path items (OpenAPITools#3093)
  [Golang][client] Allow generating go client code as a submodule. (OpenAPITools#3012)
  [CI] Test maven plugin in Travis, move jobs from Shippable to Circle CI (OpenAPITools#3087)
  general support to add scopes for bearer auth too (OpenAPITools#1984)
  feat(java-jersey2): Making response headers case-insensitive (OpenAPITools#3072)
  [KOTLIN Spring] fix generation with modelNamePrefix/Suffix (OpenAPITools#3038)
  Mark nodejs-server as deprecated (OpenAPITools#3083)
  Use 4.0.2-SNAPSHOT version in gradle samples (OpenAPITools#3085)
fantavlik added a commit to fantavlik/openapi-generator that referenced this pull request Jun 17, 2019
…to inline-resolver

* 'master' of github.com:OpenAPITools/openapi-generator: (213 commits)
  Idiomatic Rust returns for Error conversions (OpenAPITools#2812)
  Add API timeout handling (OpenAPITools#3078)
  Import inner items for map (OpenAPITools#3123)
  update core team in pom.xml (OpenAPITools#3126)
  [gradle] Document consuming via gradle plugin portal (OpenAPITools#3125)
  Bump up babel-cli version to fix security alert (OpenAPITools#3121)
  [C++] [cpprestsdk] Add examples and test for cpprestsdk (OpenAPITools#3109)
  Add enum support to `rust` and skip none option serialization in clients (OpenAPITools#2244)
  Add/update new core team member: etherealjoy (OpenAPITools#3116)
  Gradle sample on travis (OpenAPITools#3114)
  [typescript-fetch] add bearer token support (OpenAPITools#3097)
  Add Q_DECLARE_METATYPE to the generated models and remove ref in signals (OpenAPITools#3091)
  [Java][okhttp-gson] Update dependencies (OpenAPITools#3103)
  Link query parameter to model object (OpenAPITools#2710)
  scala-play-server: fix enum names for reserved words (OpenAPITools#3080)
  Add @Sunn to openapi generator core team (OpenAPITools#3105)
  fix NPE in go generator (OpenAPITools#3104)
  scala-play-server: fix API doc url (OpenAPITools#3096)
  [maven-plugin] fix strictSpec parameter without alias (OpenAPITools#3095)
  Ruby: Avoid double escaping path items (OpenAPITools#3093)
  ...

# Conflicts:
#	modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java
#	modules/openapi-generator/src/test/java/org/openapitools/codegen/InlineModelResolverTest.java
@wing328 wing328 added this to the 4.0.2 milestone Jun 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants