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

escaping inconsistent #1220

Closed
srl295 opened this issue Jan 10, 2018 · 14 comments · Fixed by #3137
Closed

escaping inconsistent #1220

srl295 opened this issue Jan 10, 2018 · 14 comments · Fixed by #3137
Assignees
Labels
dependencies Pull requests that update a dependency file released type: bug version: 3.x

Comments

@srl295
Copy link
Contributor

srl295 commented Jan 10, 2018

We ran into an issue IBM-Cloud/gp-js-client#113 where a path component (part of a parameter) was for example a ' (A, space, quote). Going into the URL Interceptor so that we can add authorization the path was escaped to a URL such as http://example.com/api/a%20' - so far so good. But then, when the request was actually sent, the Fetch module ran the URL through url.parse again, resulting in http://example.com/api/a%20%27. This results in the same logical behavior, however, the exact URL string is different causing our HMAC calculation to be wrong.

I worked around this in https://github.com/IBM-Cloud/gp-js-client/pull/114/files#diff-38aee4b3886a621f8daddb4104803472R91 by having an interceptor run url.parse() on the URL pre-emptively before HMAC calculation, which makes the later parse a no-op.

I was a little surprised that quote (%27) gets escaped as it is acceptable per HTTP. Probably the larger issue is that the escaping that happens within swagger-js should match the escaping that actually goes out on the wire.

@srl295
Copy link
Contributor Author

srl295 commented Jan 10, 2018

this was observed under 3.3.1 and also master (so 3.4.4+)

@shockey
Copy link
Contributor

shockey commented Jan 10, 2018

Thanks for the report, @srl295!

We've had this issue crop up in the past, looks like we might have a blind spot.

@shockey shockey added this to the January 12, 2018 milestone Jan 10, 2018
@shockey shockey self-assigned this Jan 10, 2018
@srl295
Copy link
Contributor Author

srl295 commented Jan 11, 2018

url.parse("http://example.com/'").href;
// --> http://example.com/%27

@shockey
Copy link
Contributor

shockey commented Jan 12, 2018

Hmm - I'm not able to reproduce this on my end:

What browser are you seeing this in? I'm on macOS Chrome 63.

@srl295
Copy link
Contributor Author

srl295 commented Jan 12, 2018

@shockey this is via node.js API, not browser. And interesting, looking at your inspector… in this case though, the HTTP request isn't wrong, the problem is that what's passed to the interceptor is not what goes out over the wire. The Fetch module does another escaping after the interceptor is done.

I think i can make a standalone case for it.

@shockey
Copy link
Contributor

shockey commented Jan 12, 2018

Ah, got it. My hunch is that this is an implementation difference.

Looking forward to a demo for this (a failing test would be great!)

@webron
Copy link
Contributor

webron commented Feb 20, 2018

@srl295 ping? :)

srl295 added a commit to srl295/swagger-js that referenced this issue Feb 27, 2018
For: swagger-api#1220

`npm t` gives:
>  Error: Expected 'percent-twentyseven' to equal 'quote'

* Request has a quote (') in it
* The interceptor sees req.url ending in quote (') which is a perfectly valid thing for a URL to end in…
* However, the xmock  shows the actual URL ends in /%27
@srl295
Copy link
Contributor Author

srl295 commented Feb 27, 2018

@webron pong ^

srl295 added a commit to srl295/swagger-js that referenced this issue Feb 27, 2018
For: swagger-api#1220

`npm t` gives:
>  Error: Expected 'percent-twentyseven' to equal 'quote'

* Request has a quote (') in it
* The interceptor sees req.url ending in quote (') which is a perfectly valid thing for a URL to end in…
* However, the xmock  shows the actual URL ends in /%27
@srl295
Copy link
Contributor Author

srl295 commented Oct 19, 2018

@shockey ping ^ ?

@shockey
Copy link
Contributor

shockey commented Oct 23, 2018

@srl295 pong! will look at this soon 😄

@char0n
Copy link
Member

char0n commented May 1, 2020

@srl295 thank you for reporting this and for providing a failing test.

The problem is in node-fetch library that we use when swagger-client is run in node.js runtime.

Actual url requested in node.js

fetch("http://localhost:8080/ '") -> url: /%20%27

Actual url requested in browser

fetch("http://localhost:8080/ '") -> url: /%20'

The actual problem lies in node-fetch implementation of fetch interface. The implementation is using legacy API instead of new WHATWG URL API. And that is causing inconsistencies. Fortunately [email protected] swichched to new WHATWG URL API. So the moment we switch to major version 3.x of node-fetch this issue will auto-correct.

I'm leaving this issue open untill we update node-fetch to 3.x branch. Before closing this issue #1252 needs to be incorporated into our tests to verify that the problem is gone.

srl295 added a commit to srl295/swagger-js that referenced this issue May 4, 2020
For: swagger-api#1220

`npm t` gives:
>  Error: Expected 'percent-twentyseven' to equal 'quote'

* Request has a quote (') in it
* The interceptor sees req.url ending in quote (') which is a perfectly valid thing for a URL to end in…
* However, the xmock  shows the actual URL ends in /%27
@char0n char0n self-assigned this Jul 2, 2020
@char0n char0n added the dependencies Pull requests that update a dependency file label Jul 2, 2020
@char0n char0n removed their assignment Dec 14, 2021
@char0n
Copy link
Member

char0n commented Sep 12, 2023

Addressed in f9207bd, which is part of #3137.

Thank you for patience.

@char0n char0n closed this as completed Sep 12, 2023
@char0n char0n self-assigned this Sep 12, 2023
@srl295
Copy link
Contributor Author

srl295 commented Sep 12, 2023

@char0n thanks!

char0n added a commit that referenced this issue Sep 12, 2023
swagger-client requires Node.js >=12.20.0 and uses different fetch 
implementation depending on Node.js version:

>=12.20.0 <16.8 - node-fetch@3
>=16.8 <18 - undici
>=18 - native Node.js fetch

Closes #1220
Closes #2736
Closes #2415
Closes #2381
Closes #2187
Closes #2291
swagger-bot pushed a commit that referenced this issue Sep 12, 2023
# [3.21.0](v3.20.2...v3.21.0) (2023-09-12)

### Features

* replace node-fetch with undici ([#3137](#3137)) ([bc7ca17](bc7ca17)), closes [#1220](#1220) [#2736](#2736) [#2415](#2415) [#2381](#2381) [#2187](#2187) [#2291](#2291)
@swagger-bot
Copy link
Contributor

🎉 This issue has been resolved in version 3.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file released type: bug version: 3.x
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants