diff --git a/.changeset/twelve-days-walk.md b/.changeset/twelve-days-walk.md new file mode 100644 index 0000000000..0b2d261c1a --- /dev/null +++ b/.changeset/twelve-days-walk.md @@ -0,0 +1,5 @@ +--- +'@backstage/core-app-api': minor +--- + +URL encode some well known unsafe characters in `RouteResolver` (and therefore `useRouteRef`) diff --git a/packages/core-app-api/package.json b/packages/core-app-api/package.json index 4d9e8f0430..5852076136 100644 --- a/packages/core-app-api/package.json +++ b/packages/core-app-api/package.json @@ -50,6 +50,7 @@ "@types/react": "^16.13.1 || ^17.0.0", "history": "^5.0.0", "i18next": "^22.4.15", + "lodash": "^4.17.21", "prop-types": "^15.7.2", "react-use": "^17.2.4", "zen-observable": "^0.10.0", diff --git a/packages/core-app-api/src/routing/RouteResolver.beta.test.ts b/packages/core-app-api/src/routing/RouteResolver.beta.test.ts index 7cd9eae021..c0ee452d28 100644 --- a/packages/core-app-api/src/routing/RouteResolver.beta.test.ts +++ b/packages/core-app-api/src/routing/RouteResolver.beta.test.ts @@ -364,4 +364,31 @@ describe('RouteResolver', () => { /^Cannot route.*with parent.*as it has parameters$/, ); }); + + it('should encode some characters in params', () => { + const r = new RouteResolver( + new Map([ + [ref2, 'my-parent/:x'], + [ref1, 'my-route'], + ]), + new Map([[ref1, ref2]]), + [ + { + routeRefs: new Set([ref2]), + path: 'my-parent/:x', + ...rest, + children: [ + MATCH_ALL_ROUTE, + { routeRefs: new Set([ref1]), path: 'my-route', ...rest }, + ], + }, + ], + new Map(), + '/base', + ); + + expect(r.resolve(ref2, '/')?.({ x: 'a/#&?b' })).toBe( + '/base/my-parent/a%2F%23%26%3Fb', + ); + }); }); diff --git a/packages/core-app-api/src/routing/RouteResolver.compat.test.ts b/packages/core-app-api/src/routing/RouteResolver.compat.test.ts index 41e5aba712..0b4bf588a2 100644 --- a/packages/core-app-api/src/routing/RouteResolver.compat.test.ts +++ b/packages/core-app-api/src/routing/RouteResolver.compat.test.ts @@ -390,4 +390,33 @@ describe.each(['beta', 'stable'])('react-router %s', rrVersion => { /^Cannot route.*with parent.*as it has parameters$/, ); }); + + it('should encode some characters in params', () => { + const { RouteResolver } = + require('./RouteResolver') as typeof import('./RouteResolver'); + const r = new RouteResolver( + new Map([ + [ref2, 'my-parent/:x'], + [ref1, 'my-route'], + ]), + new Map([[ref1, ref2]]), + [ + { + routeRefs: new Set([ref2]), + path: 'my-parent/:x', + ...rest, + children: [ + MATCH_ALL_ROUTE, + { routeRefs: new Set([ref1]), path: 'my-route', ...rest }, + ], + }, + ], + new Map(), + '/base', + ); + + expect(r.resolve(ref2, '/')?.({ x: 'a/#&?b' })).toBe( + '/base/my-parent/a%2F%23%26%3Fb', + ); + }); }); diff --git a/packages/core-app-api/src/routing/RouteResolver.stable.test.ts b/packages/core-app-api/src/routing/RouteResolver.stable.test.ts index 8c484effae..05f7678078 100644 --- a/packages/core-app-api/src/routing/RouteResolver.stable.test.ts +++ b/packages/core-app-api/src/routing/RouteResolver.stable.test.ts @@ -364,4 +364,31 @@ describe('RouteResolver', () => { /^Cannot route.*with parent.*as it has parameters$/, ); }); + + it('should encode some characters in params', () => { + const r = new RouteResolver( + new Map([ + [ref2, 'my-parent/:x'], + [ref1, 'my-route'], + ]), + new Map([[ref1, ref2]]), + [ + { + routeRefs: new Set([ref2]), + path: 'my-parent/:x', + ...rest, + children: [ + MATCH_ALL_ROUTE, + { routeRefs: new Set([ref1]), path: 'my-route', ...rest }, + ], + }, + ], + new Map(), + '/base', + ); + + expect(r.resolve(ref2, '/')?.({ x: 'a/#&?b' })).toBe( + '/base/my-parent/a%2F%23%26%3Fb', + ); + }); }); diff --git a/packages/core-app-api/src/routing/RouteResolver.ts b/packages/core-app-api/src/routing/RouteResolver.ts index f17c549bc6..11eb753bc6 100644 --- a/packages/core-app-api/src/routing/RouteResolver.ts +++ b/packages/core-app-api/src/routing/RouteResolver.ts @@ -31,6 +31,7 @@ import { SubRouteRef, } from '@backstage/core-plugin-api'; import { joinPaths } from './helpers'; +import mapValues from 'lodash/mapValues'; /** * Resolves the absolute route ref that our target route ref is pointing pointing to, as well @@ -225,7 +226,23 @@ export class RouteResolver { ); const routeFunc: RouteFunc = (...[params]) => { - return joinPaths(basePath, generatePath(targetPath, params)); + // We selectively encode some some known-dangerous characters in the + // params. The reason that we don't perform a blanket `encodeURIComponent` + // here is that this encoding was added defensively long after the initial + // release of this code. There's likely to be many users of this code that + // already encode their parameters knowing that this code didn't do this + // for them in the past. Therefore, we are extra careful NOT to include + // the percent character in this set, even though that might seem like a + // bad idea. + const encodedParams = + params && + mapValues(params, value => { + if (typeof value === 'string') { + return value.replaceAll(/[&?#;\/]/g, c => encodeURIComponent(c)); + } + return value; + }); + return joinPaths(basePath, generatePath(targetPath, encodedParams)); }; return routeFunc; } diff --git a/yarn.lock b/yarn.lock index 77598be80f..a49cbb5c34 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3955,6 +3955,7 @@ __metadata: "@types/zen-observable": ^0.8.0 history: ^5.0.0 i18next: ^22.4.15 + lodash: ^4.17.21 msw: ^1.0.0 prop-types: ^15.7.2 react-router-beta: "npm:react-router@6.0.0-beta.0"