Skip to content

Commit

Permalink
Remove forge.util.parseUrl.
Browse files Browse the repository at this point in the history
- Switch URL parsing to the WHATWG URL Standard `URL` API.
- Older browser or Node.js usage of related code might now require a URL
  polyfill.
  • Loading branch information
davidlehn committed Jan 4, 2022
1 parent e1a740d commit db8016c
Show file tree
Hide file tree
Showing 7 changed files with 45 additions and 74 deletions.
16 changes: 16 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,22 @@ Forge ChangeLog
maintainers for internal project debug purposes and was never intended to be
used with untrusted user inputs. This API was not documented or advertised
and is being removed rather than fixed.
- **SECURITY**, **BREAKING**: Remove `forge.util.parseUrl()` (and
`forge.http.parseUrl` alias) and use the [WHATWG URL
Standard](https://url.spec.whatwg.org/). `URL` is supported by modern browers
and modern Node.js. This change is needed to address URL parsing security
issues. If `forge.util.parseUrl()` is used directly or through `forge.xhr` or
`forge.http` APIs, and support is needed for environments without `URL`
support, then a polyfill must be used.
- **BREAKING**: Remove `forge.task` API. This API was never used, documented,
or advertised by the maintainers. If anyone was using this API and wishes to
continue development it in other project, please let the maintainers know.
Due to use in the test suite, a modified version is located in
`tests/support/`.

### Changed
- **BREAKING**: Increase supported Node.js version to 6.13.0 for URL support.

### Added
- OIDs for `surname`, `title`, and `givenName`.

Expand All @@ -23,6 +33,12 @@ Forge ChangeLog
Depending on how applications used this id to name association it could cause
compatibility issues.

### Notes
- The URL related changes may expose bugs in some of the networking related
code (unrelated to the much wider used cryptography code). The automated and
manual test coverage for this code is weak at best. Issues or patches to
update the code or tests would be appriciated.

## 0.10.0 - 2020-09-01

### Changed
Expand Down
4 changes: 0 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1968,10 +1968,6 @@ var nodeBuffer = Buffer.from(forgeBuffer.getBytes(), 'binary');
// make sure you specify the encoding as 'binary'
var nodeBuffer = Buffer.from('CAFE', 'hex');
var forgeBuffer = forge.util.createBuffer(nodeBuffer.toString('binary'));

// parse a URL
var parsed = forge.util.parseUrl('http://example.com/foo?bar=baz');
// parsed.scheme, parsed.host, parsed.port, parsed.path, parsed.fullHost
```

<a name="log" />
Expand Down
39 changes: 16 additions & 23 deletions lib/http.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ var _getStorageId = function(client) {
// browsers (if this is undesirable)
// navigator.userAgent
return 'forge.http.' +
client.url.scheme + '.' +
client.url.host + '.' +
client.url.protocol.slice(0, -1) + '.' +
client.url.hostname + '.' +
client.url.port;
};

Expand Down Expand Up @@ -121,7 +121,7 @@ var _doRequest = function(client, socket) {
// connect
socket.options.request.connectTime = +new Date();
socket.connect({
host: client.url.host,
host: client.url.hostname,
port: client.url.port,
policyPort: client.policyPort,
policyUrl: client.policyUrl
Expand Down Expand Up @@ -310,7 +310,7 @@ var _initSocket = function(client, socket, tlsOptions) {
// prime socket by connecting and caching TLS session, will do
// next request from there
socket.connect({
host: client.url.host,
host: client.url.hostname,
port: client.url.port,
policyPort: client.policyPort,
policyUrl: client.policyUrl
Expand Down Expand Up @@ -405,7 +405,7 @@ var _readCookies = function(client, response) {
*
* @param options:
* url: the url to connect to (scheme://host:port).
* socketPool: the flash socket pool to use.
* socketPool: the flash socket pool to use.
* policyPort: the flash policy port to use (if other than the
* socket pool default), use 0 for flash default.
* policyUrl: the flash policy file URL to use (if provided will
Expand Down Expand Up @@ -441,8 +441,10 @@ http.createClient = function(options) {
// get scheme, host, and port from url
options.url = (options.url ||
window.location.protocol + '//' + window.location.host);
var url = http.parseUrl(options.url);
if(!url) {
var url;
try {
url = new URL(options.url);
} catch(e) {
var error = new Error('Invalid url.');
error.details = {url: options.url};
throw error;
Expand All @@ -469,7 +471,7 @@ http.createClient = function(options) {
// idle sockets
idle: [],
// whether or not the connections are secure
secure: (url.scheme === 'https'),
secure: (url.protocol === 'https:'),
// cookie jar (key'd off of name and then path, there is only 1 domain
// and one setting for secure per client so name+path is unique)
cookies: {},
Expand Down Expand Up @@ -497,7 +499,7 @@ http.createClient = function(options) {
if(depth === 0 && verified === true) {
// compare common name to url host
var cn = certs[depth].subject.getField('CN');
if(cn === null || client.url.host !== cn.value) {
if(cn === null || client.url.hostname !== cn.value) {
verified = {
message: 'Certificate common name does not match url host.'
};
Expand All @@ -512,7 +514,7 @@ http.createClient = function(options) {
tlsOptions = {
caStore: caStore,
cipherSuites: options.cipherSuites || null,
virtualHost: options.virtualHost || url.host,
virtualHost: options.virtualHost || url.hostname,
verify: options.verify || _defaultCertificateVerify,
getCertificate: options.getCertificate || null,
getPrivateKey: options.getPrivateKey || null,
Expand Down Expand Up @@ -552,7 +554,7 @@ http.createClient = function(options) {
client.send = function(options) {
// add host header if not set
if(options.request.getField('Host') === null) {
options.request.setField('Host', client.url.fullHost);
options.request.setField('Host', client.url.origin);
}

// set default dummy handlers
Expand Down Expand Up @@ -1307,15 +1309,6 @@ http.createResponse = function() {
return response;
};

/**
* Parses the scheme, host, and port from an http(s) url.
*
* @param str the url string.
*
* @return the parsed url object or null if the url is invalid.
*/
http.parseUrl = forge.util.parseUrl;

/**
* Returns true if the given url is within the given cookie's domain.
*
Expand All @@ -1336,11 +1329,11 @@ http.withinCookieDomain = function(url, cookie) {
// ensure domain starts with a '.'
// parse URL as necessary
if(typeof url === 'string') {
url = http.parseUrl(url);
url = new URL(url);
}

// add '.' to front of URL host to match against domain
var host = '.' + url.host;
// add '.' to front of URL hostname to match against domain
var host = '.' + url.hostname;

// if the host ends with domain then it falls within it
var idx = host.lastIndexOf(domain);
Expand Down
37 changes: 0 additions & 37 deletions lib/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -2258,43 +2258,6 @@ util.clearItems = function(api, id, location) {
_callStorageFunction(_clearItems, arguments, location);
};

/**
* Parses the scheme, host, and port from an http(s) url.
*
* @param str the url string.
*
* @return the parsed url object or null if the url is invalid.
*/
util.parseUrl = function(str) {
// FIXME: this regex looks a bit broken
var regex = /^(https?):\/\/([^:&^\/]*):?(\d*)(.*)$/g;
regex.lastIndex = 0;
var m = regex.exec(str);
var url = (m === null) ? null : {
full: str,
scheme: m[1],
host: m[2],
port: m[3],
path: m[4]
};
if(url) {
url.fullHost = url.host;
if(url.port) {
if(url.port !== 80 && url.scheme === 'http') {
url.fullHost += ':' + url.port;
} else if(url.port !== 443 && url.scheme === 'https') {
url.fullHost += ':' + url.port;
}
} else if(url.scheme === 'http') {
url.port = 80;
} else if(url.scheme === 'https') {
url.port = 443;
}
url.full = url.scheme + '://' + url.fullHost;
}
return url;
};

/* Storage for query variables */
var _queryVariables = null;

Expand Down
14 changes: 8 additions & 6 deletions lib/xhr.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ xhrApi.init = function(options) {
getPrivateKey: options.getPrivateKey,
getSignature: options.getSignature
});
_clients[_client.url.full] = _client;
_clients[_client.url.origin] = _client;

forge.log.debug(cat, 'ready');
};
Expand Down Expand Up @@ -380,18 +380,20 @@ xhrApi.create = function(options) {
// use default
_state.client = _client;
} else {
var url = http.parseUrl(options.url);
if(!url) {
var url;
try {
url = new URL(options.url);
} catch(e) {
var error = new Error('Invalid url.');
error.details = {
url: options.url
};
}

// find client
if(url.full in _clients) {
if(url.origin in _clients) {
// client found
_state.client = _clients[url.full];
_state.client = _clients[url.origin];
} else {
// create client
_state.client = http.createClient({
Expand All @@ -409,7 +411,7 @@ xhrApi.create = function(options) {
getPrivateKey: options.getPrivateKey,
getSignature: options.getSignature
});
_clients[url.full] = _state.client;
_clients[url.origin] = _state.client;
}
}

Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
"dist/*.min.js.map"
],
"engines": {
"node": ">= 6.0.0"
"node": ">= 6.13.0"
},
"keywords": [
"aes",
Expand Down
7 changes: 4 additions & 3 deletions tests/websockets/server-webid.js
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,10 @@ var fetchUrl = function(url, callback, redirects) {
console.log('Fetching URL: \"' + url + '\"');

// parse URL
url = forge.util.parseUrl(url);
var client = http.createClient(
url.port, url.fullHost, url.scheme === 'https');
url = new URL(url);
var client = http.createClient({
url: url
});
var request = client.request('GET', url.path, {
Host: url.host,
Accept: 'application/rdf+xml'
Expand Down

0 comments on commit db8016c

Please sign in to comment.