10000 fix: properly encode url with unicode characters by LinusU · Pull Request #1291 · node-fetch/node-fetch · GitHub
[go: up one dir, main page]

Skip to content

fix: properly encode url with unicode characters #1291

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

Merged
merged 2 commits into from
Sep 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ Changelog

# 2.x release

## v2.6.3

- Fix: properly en 8000 code url with unicode characters

## v2.6.2

- Fix: used full filename for main in package.json
Expand Down
8 changes: 5 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "node-fetch",
"version": "2.6.2",
"version": "2.6.3",
"description": "A light-weight module that brings window.fetch to node.js",
"main": "lib/index.js",
"browser": "./browser.js",
Expand Down Expand Up @@ -36,6 +36,9 @@
"url": "https://github.com/bitinn/node-fetch/issues"
},
"homepage": "https://github.com/bitinn/node-fetch",
"dependencies": {
"whatwg-url": "^5.0.0"

Choose a reason for hiding this comment

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

why this version?
actual is 9.1.0

this breaks our code because we have dependency on whatwg-url v.6.5.0 already

Copy link
Member Author

Choose a reason for hiding this comment

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

why this version?
actual is 9.1.0

This is the latest version that supports Node.js 4.x

this breaks our code because we have dependency on whatwg-url v.6.5.0 already

In which way does your app break? 🤔

Choose a reason for hiding this comment

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

This also breaks my app : for security reasons, we use the --throw-deprecation and --pending-deprecation node flags, and whatwg-url appears to use Buffer() : I get

DeprecationWarningcode: DEP0005 DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
    at showFlaggedDeprecation (buffer.js:194:11)
    at new Buffer (buffer.js:278:3)
    at utf8PercentDecode (/project/node_modules/whatwg-url/lib/url-state-machine.js:105:17)
    at parseHost (/project/node_modules/whatwg-url/lib/url-state-machine.js:400:18)
    at URLStateMachine.parseHostName (/project/node_modules/whatwg-url/lib/url-state-machine.js:851:18)
    at new URLStateMachine (/project/node_modules/whatwg-url/lib/url-state-machine.js:562:44)
    at Object.module.exports.basicURLParse (/project/node_modules/whatwg-url/lib/url-state-machine.js:1258:15)
    at new URLImpl (/project/node_modules/whatwg-url/lib/URL-impl.js:17:27)
    at Object.setup (/project/node_modules/whatwg-url/lib/URL.js:187:17)
    at new URL (/project/node_modules/whatwg-url/lib/URL.js:25:18)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've asked upstream if we can create a 5.0.1 version of whatwg-url that avoids the deprecation warning here: jsdom/whatwg-url#216

But apart from that I'm not really sure how to handle using deprecated APIs 🤔 I don't think that I would consider it a breaking chance, but I'm not sure...

},
"devDependencies": {
"@ungap/url-search-params": "^0.1.2",
"abort-controller": "^1.1.0",
Expand All @@ -60,7 +63,6 @@
"rollup": "^0.63.4",
"rollup-plugin-babel": "^3.0.7",
"string-to-arraybuffer": "^1.0.2",
"teeny-request": "3.7.0",
"whatwg-url": "^5.0.0"
"teeny-request": "3.7.0"
}
}
5 changes: 4 additions & 1 deletion rollup.config.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import isBuiltin from 'is-builtin-module';
import babel from 'rollup-plugin-babel';
import packageJson from './package.json';
import tweakDefault from './build/rollup-plugin';

process.env.BABEL_ENV = 'rollup';

const dependencies = Object.keys(packageJson.dependencies);

export default {
input: 'src/index.js',
output: [
Expand All @@ -18,6 +21,6 @@ export default {
tweakDefault()
],
external: function (id) {
return isBuiltin(id);
return dependencies.includes(id) || isBuiltin(id);
}
};
40 changes: 37 additions & 3 deletions src/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import Url from 'url';
import Stream from 'stream';
import {URL} from 'whatwg-url';
import Headers, { exportNodeCompatibleHeaders } from './headers.js';
import Body, { clone, extractContentType, getTotalBytes } from './body';
8000
Expand All @@ -18,6 +19,39 @@ const INTERNALS = Symbol('Request internals');
const parse_url = Url.parse;
const format_url = Url.format;

/**
* Wrapper around `new URL` to handle arbitrary URLs
*
* @param {string} urlStr
* @return {void}
*/
function parseURL(urlStr) {
/*
Check whether the URL is absolute or not

Scheme: https://tools.ietf.org/html/rfc3986#section-3.1
Absolute URL: https://tools.ietf.org/html/rfc3986#section-4.3
*/
if (/^[a-zA-Z][a-zA-Z\d+\-.]*:/.exec(urlStr)) {
const url = new URL(urlStr);

return {
path: url.pathname,
pathname: url.pathname,
hostname: url.hostname,
protocol: url.protocol,
port: url.port,
hash: url.hash,
search: url.search,
query: url.query,
href: url.href,
}
}

// Fallback to old implementation for arbitrary URLs
return parse_url(urlStr);
}

const streamDestructionSupported = 'destroy' in Stream.Readable.prototype;

/**
Expand Down Expand Up @@ -59,14 +93,14 @@ export default class Request {
// in order to support Node.js' Url objects; though WHATWG's URL objects
// will fall into this branch also (since their `toString()` will return
// `href` property anyway)
parsedURL = parse_url(input.href);
parsedURL = parseURL(input.href);
} else {
// coerce input to a string before attempting to parse
parsedURL = parse_url(`${input}`);
parsedURL = parseURL(`${input}`);
}
input = {};
} else {
parsedURL = parse_url(input.url);
parsedURL = parseURL(input.url);
}

let method = init.method || input.method || 'GET';
Expand Down
8 changes: 7 additions & 1 deletion test/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export default class TestServer {
}

router(req, res) {
let p = parse(req.url).pathname;
let p = decodeURIComponent(parse(req.url).pathname);

if (p === '/hello') {
res.statusCode = 200;
Expand Down Expand Up @@ -384,6 +384,12 @@ export default class TestServer {
});
req.pipe(parser);
}

if (p === '/issues/1290/ひらがな') {
res.statusCode = 200;
res.setHeader('Content-Type', 'text/plain');
res.end('Success');
}
}
}

Expand Down
22 changes: 22 additions & 0 deletions test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2845,3 +2845,25 @@ describe('external encoding', () => {
});
});
});

describe('issue #1290', function() {
it('should handle escaped unicode in URLs', () => {
const url = `${base}issues/1290/%E3%81%B2%E3%82%89%E3%81%8C%E3%81%AA`;
return fetch(url).then((res) => {
expect(res.status).to.equal(200);
return res.text().then(result => {
expect(result).to.equal('Success');
});
});
});

it('should handle unicode in URLs', () => {
const url = `${base}issues/1290/ひらがな`;
return fetch(url).then((res) => {
expect(res.status).to.equal(200);
return res.text().then(result => {
expect(result).to.equal('Success');
});
});
});
});
0