diff --git a/changelogs/fragments/10915.yml b/changelogs/fragments/10915.yml new file mode 100644 index 000000000000..29d70e9b3224 --- /dev/null +++ b/changelogs/fragments/10915.yml @@ -0,0 +1,2 @@ +fix: +- @osd-std package url parse method does not throw page crashing errors ([#10915](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10915)) \ No newline at end of file diff --git a/packages/osd-std/src/url.test.ts b/packages/osd-std/src/url.test.ts index 56dbde02ae1d..8d766ab557f2 100644 --- a/packages/osd-std/src/url.test.ts +++ b/packages/osd-std/src/url.test.ts @@ -31,12 +31,6 @@ import { modifyUrl, isRelativeUrl, getUrlOrigin } from './url'; describe('modifyUrl()', () => { - test('throws an error with invalid input', () => { - expect(() => modifyUrl(1 as any, () => ({}))).toThrowError(); - expect(() => modifyUrl(undefined as any, () => ({}))).toThrowError(); - expect(() => modifyUrl('http://localhost', undefined as any)).toThrowError(); - }); - test('supports returning a new url spec', () => { expect(modifyUrl('http://localhost', () => ({}))).toEqual(''); }); @@ -79,6 +73,12 @@ describe('modifyUrl()', () => { }) ).toEqual('mail:localhost'); }); + + test('does not throw URIError for malformed urls', () => { + expect(() => { + modifyUrl('http://user:%E0%A4@example.com', () => {}); + }).not.toThrowError(); + }); }); describe('isRelativeUrl()', () => { @@ -93,6 +93,11 @@ describe('isRelativeUrl()', () => { expect(isRelativeUrl('///evil.com')).toBe(false); expect(isRelativeUrl(' //evil.com')).toBe(false); }); + test('does not throw URIError for malformed urls', () => { + expect(() => { + isRelativeUrl('http://user:%E0%A4@example.com'); + }).not.toThrowError(); + }); }); describe('getOrigin', () => { @@ -105,6 +110,11 @@ describe('getOrigin', () => { it('return origin with port when the url does have a port', () => { expect(getUrlOrigin('http://example.com:80/path/to/file')).toEqual('http://example.com:80'); }); + it('does not throw URIError for malformed urls', () => { + expect(() => { + getUrlOrigin('http://user:%E0%A4@example.com'); + }).not.toThrowError(); + }); }); describe('when passing a non absolute url', () => { it('returns null for relative url', () => { diff --git a/packages/osd-std/src/url.ts b/packages/osd-std/src/url.ts index 6a0f9cdb0052..bfb10d4a6207 100644 --- a/packages/osd-std/src/url.ts +++ b/packages/osd-std/src/url.ts @@ -83,36 +83,40 @@ export function modifyUrl( url: string, urlModifier: (urlParts: URLMeaningfulParts) => Partial | void ) { - const parsed = parseUrl(url, true) as URLMeaningfulParts; + try { + const parsed = parseUrl(url, true) as URLMeaningfulParts; - // Copy over the most specific version of each property. By default, the parsed url includes several - // conflicting properties (like path and pathname + search, or search and query) and keeping track - // of which property is actually used when they are formatted is harder than necessary. - const meaningfulParts: URLMeaningfulParts = { - auth: parsed.auth, - hash: parsed.hash, - hostname: parsed.hostname, - pathname: parsed.pathname, - port: parsed.port, - protocol: parsed.protocol, - query: parsed.query || {}, - slashes: parsed.slashes, - }; + // Copy over the most specific version of each property. By default, the parsed url includes several + // conflicting properties (like path and pathname + search, or search and query) and keeping track + // of which property is actually used when they are formatted is harder than necessary. + const meaningfulParts: URLMeaningfulParts = { + auth: parsed.auth, + hash: parsed.hash, + hostname: parsed.hostname, + pathname: parsed.pathname, + port: parsed.port, + protocol: parsed.protocol, + query: parsed.query || {}, + slashes: parsed.slashes, + }; - // The urlModifier modifies the meaningfulParts object, or returns a new one. - const modifiedParts = urlModifier(meaningfulParts) || meaningfulParts; + // The urlModifier modifies the meaningfulParts object, or returns a new one. + const modifiedParts = urlModifier(meaningfulParts) || meaningfulParts; - // Format the modified/replaced meaningfulParts back into a url. - return formatUrl({ - auth: modifiedParts.auth, - hash: modifiedParts.hash, - hostname: modifiedParts.hostname, - pathname: modifiedParts.pathname, - port: modifiedParts.port, - protocol: modifiedParts.protocol, - query: modifiedParts.query, - slashes: modifiedParts.slashes, - } as UrlObject); + // Format the modified/replaced meaningfulParts back into a url. + return formatUrl({ + auth: modifiedParts.auth, + hash: modifiedParts.hash, + hostname: modifiedParts.hostname, + pathname: modifiedParts.pathname, + port: modifiedParts.port, + protocol: modifiedParts.protocol, + query: modifiedParts.query, + slashes: modifiedParts.slashes, + } as UrlObject); + } catch (error) { + return url; // Safe fallback to original url + } } /** @@ -122,28 +126,36 @@ export function modifyUrl( * @public */ export function isRelativeUrl(candidatePath: string) { - // validate that `candidatePath` is not attempting a redirect to somewhere - // outside of this OpenSearch Dashboards install - const all = parseUrl(candidatePath, false /* parseQueryString */, true /* slashesDenoteHost */); - const { protocol, hostname, port } = all; - // We should explicitly compare `protocol`, `port` and `hostname` to null to make sure these are not - // detected in the URL at all. For example `hostname` can be empty string for Node URL parser, but - // browser (because of various bwc reasons) processes URL differently (e.g. `///abc.com` - for browser - // hostname is `abc.com`, but for Node hostname is an empty string i.e. everything between schema (`//`) - // and the first slash that belongs to path. - if (protocol !== null || hostname !== null || port !== null) { - return false; + try { + // validate that `candidatePath` is not attempting a redirect to somewhere + // outside of this OpenSearch Dashboards install + const all = parseUrl(candidatePath, false /* parseQueryString */, true /* slashesDenoteHost */); + const { protocol, hostname, port } = all; + // We should explicitly compare `protocol`, `port` and `hostname` to null to make sure these are not + // detected in the URL at all. For example `hostname` can be empty string for Node URL parser, but + // browser (because of various bwc reasons) processes URL differently (e.g. `///abc.com` - for browser + // hostname is `abc.com`, but for Node hostname is an empty string i.e. everything between schema (`//`) + // and the first slash that belongs to path. + if (protocol !== null || hostname !== null || port !== null) { + return false; + } + return true; + } catch (error) { + return false; // Safe fallback to "not relative" } - return true; } /** * Returns the origin (protocol + host + port) from given `url` if `url` is a valid absolute url, or null otherwise */ export function getUrlOrigin(url: string): string | null { - const obj = parseUrl(url); - if (!obj.protocol && !obj.hostname) { - return null; + try { + const obj = parseUrl(url); + if (!obj.protocol && !obj.hostname) { + return null; + } + return `${obj.protocol}//${obj.hostname}${obj.port ? `:${obj.port}` : ''}`; + } catch (error) { + return null; // Safe fallback to null } - return `${obj.protocol}//${obj.hostname}${obj.port ? `:${obj.port}` : ''}`; } diff --git a/src/core/server/http/base_path_service.test.ts b/src/core/server/http/base_path_service.test.ts index cefa90577389..c53c34b8db9e 100644 --- a/src/core/server/http/base_path_service.test.ts +++ b/src/core/server/http/base_path_service.test.ts @@ -117,6 +117,14 @@ describe('BasePath', () => { expect(basePath.prepend('http://localhost:5601/a/b')).toBe('http://localhost:5601/a/b'); }); + + it('returns path unchanged for prepend with malformed auth in url', () => { + const basePath = new BasePath('a/b'); + + expect(basePath.prepend('http://user:%E0%A4@example.com')).toBe( + 'http://user:%E0%A4@example.com' + ); + }); }); describe('#remove()', () => {