Skip to content

Commit a5bfad5

Browse files
paulstnopensearch-changeset-bot[bot]
paulstn
and
opensearch-changeset-bot[bot]
authored
Avoids URIError in @osd/std url methods (#10915)
* wrap osd-std url methods in try catch to avoid throwing errors from parse Signed-off-by: Paul Sebastian <[email protected]> * Changeset file for PR #10915 created/updated * change getUrlOrigin fallback to null instead of url Signed-off-by: Paul Sebastian <[email protected]> --------- Signed-off-by: Paul Sebastian <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
1 parent 67ab52c commit a5bfad5

File tree

4 files changed

+81
-49
lines changed

4 files changed

+81
-49
lines changed

‎changelogs/fragments/10915.yml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
fix:
2+
- @osd-std package url parse method does not throw page crashing errors ([#10915](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10915))

‎packages/osd-std/src/url.test.ts‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,6 @@
3131
import { modifyUrl, isRelativeUrl, getUrlOrigin } from './url';
3232

3333
describe('modifyUrl()', () => {
34-
test('throws an error with invalid input', () => {
35-
expect(() => modifyUrl(1 as any, () => ({}))).toThrowError();
36-
expect(() => modifyUrl(undefined as any, () => ({}))).toThrowError();
37-
expect(() => modifyUrl('http://localhost', undefined as any)).toThrowError();
38-
});
39-
4034
test('supports returning a new url spec', () => {
4135
expect(modifyUrl('http://localhost', () => ({}))).toEqual('');
4236
});
@@ -79,6 +73,12 @@ describe('modifyUrl()', () => {
7973
})
8074
).toEqual('mail:localhost');
8175
});
76+
77+
test('does not throw URIError for malformed urls', () => {
78+
expect(() => {
79+
modifyUrl('http://user:%E0%[email protected]', () => {});
80+
}).not.toThrowError();
81+
});
8282
});
8383

8484
describe('isRelativeUrl()', () => {
@@ -93,6 +93,11 @@ describe('isRelativeUrl()', () => {
9393
expect(isRelativeUrl('///evil.com')).toBe(false);
9494
expect(isRelativeUrl(' //evil.com')).toBe(false);
9595
});
96+
test('does not throw URIError for malformed urls', () => {
97+
expect(() => {
98+
isRelativeUrl('http://user:%E0%[email protected]');
99+
}).not.toThrowError();
100+
});
96101
});
97102

98103
describe('getOrigin', () => {
@@ -105,6 +110,11 @@ describe('getOrigin', () => {
105110
it('return origin with port when the url does have a port', () => {
106111
expect(getUrlOrigin('http://example.com:80/path/to/file')).toEqual('http://example.com:80');
107112
});
113+
it('does not throw URIError for malformed urls', () => {
114+
expect(() => {
115+
getUrlOrigin('http://user:%E0%[email protected]');
116+
}).not.toThrowError();
117+
});
108118
});
109119
describe('when passing a non absolute url', () => {
110120
it('returns null for relative url', () => {

‎packages/osd-std/src/url.ts‎

Lines changed: 55 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -83,36 +83,40 @@ export function modifyUrl(
8383
url: string,
8484
urlModifier: (urlParts: URLMeaningfulParts) => Partial<URLMeaningfulParts> | void
8585
) {
86-
const parsed = parseUrl(url, true) as URLMeaningfulParts;
86+
try {
87+
const parsed = parseUrl(url, true) as URLMeaningfulParts;
8788

88-
// Copy over the most specific version of each property. By default, the parsed url includes several
89-
// conflicting properties (like path and pathname + search, or search and query) and keeping track
90-
// of which property is actually used when they are formatted is harder than necessary.
91-
const meaningfulParts: URLMeaningfulParts = {
92-
auth: parsed.auth,
93-
hash: parsed.hash,
94-
hostname: parsed.hostname,
95-
pathname: parsed.pathname,
96-
port: parsed.port,
97-
protocol: parsed.protocol,
98-
query: parsed.query || {},
99-
slashes: parsed.slashes,
100-
};
89+
// Copy over the most specific version of each property. By default, the parsed url includes several
90+
// conflicting properties (like path and pathname + search, or search and query) and keeping track
91+
// of which property is actually used when they are formatted is harder than necessary.
92+
const meaningfulParts: URLMeaningfulParts = {
93+
auth: parsed.auth,
94+
hash: parsed.hash,
95+
hostname: parsed.hostname,
96+
pathname: parsed.pathname,
97+
port: parsed.port,
98+
protocol: parsed.protocol,
99+
query: parsed.query || {},
100+
slashes: parsed.slashes,
101+
};
101102

102-
// The urlModifier modifies the meaningfulParts object, or returns a new one.
103-
const modifiedParts = urlModifier(meaningfulParts) || meaningfulParts;
103+
// The urlModifier modifies the meaningfulParts object, or returns a new one.
104+
const modifiedParts = urlModifier(meaningfulParts) || meaningfulParts;
104105

105-
// Format the modified/replaced meaningfulParts back into a url.
106-
return formatUrl({
107-
auth: modifiedParts.auth,
108-
hash: modifiedParts.hash,
109-
hostname: modifiedParts.hostname,
110-
pathname: modifiedParts.pathname,
111-
port: modifiedParts.port,
112-
protocol: modifiedParts.protocol,
113-
query: modifiedParts.query,
114-
slashes: modifiedParts.slashes,
115-
} as UrlObject);
106+
// Format the modified/replaced meaningfulParts back into a url.
107+
return formatUrl({
108+
auth: modifiedParts.auth,
109+
hash: modifiedParts.hash,
110+
hostname: modifiedParts.hostname,
111+
pathname: modifiedParts.pathname,
112+
port: modifiedParts.port,
113+
protocol: modifiedParts.protocol,
114+
query: modifiedParts.query,
115+
slashes: modifiedParts.slashes,
116+
} as UrlObject);
117+
} catch (error) {
118+
return url; // Safe fallback to original url
119+
}
116120
}
117121

118122
/**
@@ -122,28 +126,36 @@ export function modifyUrl(
122126
* @public
123127
*/
124128
export function isRelativeUrl(candidatePath: string) {
125-
// validate that `candidatePath` is not attempting a redirect to somewhere
126-
// outside of this OpenSearch Dashboards install
127-
const all = parseUrl(candidatePath, false /* parseQueryString */, true /* slashesDenoteHost */);
128-
const { protocol, hostname, port } = all;
129-
// We should explicitly compare `protocol`, `port` and `hostname` to null to make sure these are not
130-
// detected in the URL at all. For example `hostname` can be empty string for Node URL parser, but
131-
// browser (because of various bwc reasons) processes URL differently (e.g. `///abc.com` - for browser
132-
// hostname is `abc.com`, but for Node hostname is an empty string i.e. everything between schema (`//`)
133-
// and the first slash that belongs to path.
134-
if (protocol !== null || hostname !== null || port !== null) {
135-
return false;
129+
try {
130+
// validate that `candidatePath` is not attempting a redirect to somewhere
131+
// outside of this OpenSearch Dashboards install
132+
const all = parseUrl(candidatePath, false /* parseQueryString */, true /* slashesDenoteHost */);
133+
const { protocol, hostname, port } = all;
134+
// We should explicitly compare `protocol`, `port` and `hostname` to null to make sure these are not
135+
// detected in the URL at all. For example `hostname` can be empty string for Node URL parser, but
136+
// browser (because of various bwc reasons) processes URL differently (e.g. `///abc.com` - for browser
137+
// hostname is `abc.com`, but for Node hostname is an empty string i.e. everything between schema (`//`)
138+
// and the first slash that belongs to path.
139+
if (protocol !== null || hostname !== null || port !== null) {
140+
return false;
141+
}
142+
return true;
143+
} catch (error) {
144+
return false; // Safe fallback to "not relative"
136145
}
137-
return true;
138146
}
139147

140148
/**
141149
* Returns the origin (protocol + host + port) from given `url` if `url` is a valid absolute url, or null otherwise
142150
*/
143151
export function getUrlOrigin(url: string): string | null {
144-
const obj = parseUrl(url);
145-
if (!obj.protocol && !obj.hostname) {
146-
return null;
152+
try {
153+
const obj = parseUrl(url);
154+
if (!obj.protocol && !obj.hostname) {
155+
return null;
156+
}
157+
return `${obj.protocol}//${obj.hostname}${obj.port ? `:${obj.port}` : ''}`;
158+
} catch (error) {
159+
return null; // Safe fallback to null
147160
}
148-
return `${obj.protocol}//${obj.hostname}${obj.port ? `:${obj.port}` : ''}`;
149161
}

‎src/core/server/http/base_path_service.test.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,14 @@ describe('BasePath', () => {
117117

118118
expect(basePath.prepend('http://localhost:5601/a/b')).toBe('http://localhost:5601/a/b');
119119
});
120+
121+
it('returns path unchanged for prepend with malformed auth in url', () => {
122+
const basePath = new BasePath('a/b');
123+
124+
expect(basePath.prepend('http://user:%E0%[email protected]')).toBe(
125+
'http://user:%E0%[email protected]'
126+
);
127+
});
120128
});
121129

122130
describe('#remove()', () => {

0 commit comments

Comments
 (0)