fix(editor): Stop nefarious URL redirection in editor middleware (#16047)

Author: MarcL

packages/frontend/editor-ui/src/utils/rbac/middleware/guest.test.ts

@@ -8,36 +8,80 @@ vi.mock('@/stores/users.store', () => ({
 }));
 
 describe('Middleware', () => {
+	const ORIGIN_URL = 'https://n8n.local';
+
+	beforeEach(() => {
+		global.window = Object.create(window);
+
+		Object.defineProperty(window, 'location', {
+			value: {
+				href: '',
+				origin: ORIGIN_URL,
+			},
+			writable: true,
+		});
+	});
+
 	describe('guest', () => {
-		it('should redirect to given path if current user is present and valid redirect is provided', async () => {
-			vi.mocked(useUsersStore).mockReturnValue({ currentUser: { id: '123' } } as ReturnType<
-				typeof useUsersStore
-			>);
+		describe('if current user is present', () => {
+			beforeEach(() => {
+				vi.mocked(useUsersStore).mockReturnValue({ currentUser: { id: '123' } } as ReturnType<
+					typeof useUsersStore
+				>);
+			});
 
-			const nextMock = vi.fn();
-			const toMock = { query: { redirect: '/some-path' } } as unknown as RouteLocationNormalized;
-			const fromMock = {} as RouteLocationNormalized;
+			afterEach(() => {
+				vi.clearAllMocks();
+			});
 
-			await guestMiddleware(toMock, fromMock, nextMock, {});
+			it('should redirect to given path if valid redirect is provided', async () => {
+				const nextMock = vi.fn();
+				const toMock = { query: { redirect: '/some-path' } } as unknown as RouteLocationNormalized;
+				const fromMock = {} as RouteLocationNormalized;
 
-			expect(nextMock).toHaveBeenCalledWith('/some-path');
-		});
+				await guestMiddleware(toMock, fromMock, nextMock, {});
 
-		it('should redirect to homepage if current user is present and no valid redirect', async () => {
-			vi.mocked(useUsersStore).mockReturnValue({ currentUser: { id: '123' } } as ReturnType<
-				typeof useUsersStore
-			>);
+				expect(nextMock).toHaveBeenCalledWith('/some-path');
+			});
 
-			const nextMock = vi.fn();
-			const toMock = { query: {} } as RouteLocationNormalized;
-			const fromMock = {} as RouteLocationNormalized;
+			it('should redirect to homepage if no redirect is set', async () => {
+				const nextMock = vi.fn();
+				const toMock = { query: {} } as RouteLocationNormalized;
+				const fromMock = {} as RouteLocationNormalized;
 
-			await guestMiddleware(toMock, fromMock, nextMock, {});
+				await guestMiddleware(toMock, fromMock, nextMock, {});
+
+				expect(nextMock).toHaveBeenCalledWith({ name: VIEWS.HOMEPAGE });
+			});
+
+			it('should redirect to homepage if redirect is not a valid URL', async () => {
+				const nextMock = vi.fn();
+				const toMock = {
+					query: { redirect: 'not-valid-url' },
+				} as unknown as RouteLocationNormalized;
+
+				const fromMock = {} as RouteLocationNormalized;
+
+				await guestMiddleware(toMock, fromMock, nextMock, {});
+
+				expect(nextMock).toHaveBeenCalledWith({ name: VIEWS.HOMEPAGE });
+			});
+
+			it('should redirect to homepage if redirect is not the origin domain', async () => {
+				const nextMock = vi.fn();
+				const toMock = {
+					query: { redirect: 'https://n8n.local.evil.com/some-path' },
+				} as unknown as RouteLocationNormalized;
+
+				const fromMock = {} as RouteLocationNormalized;
+
+				await guestMiddleware(toMock, fromMock, nextMock, {});
 
-			expect(nextMock).toHaveBeenCalledWith({ name: VIEWS.HOMEPAGE });
+				expect(nextMock).toHaveBeenCalledWith({ name: VIEWS.HOMEPAGE });
+			});
 		});
 
-		it('should allow navigation if no current user is present', async () => {
+		it('should not redirect if no current user is present', async () => {
 			vi.mocked(useUsersStore).mockReturnValue({ currentUser: null } as ReturnType<
 				typeof useUsersStore
 			>);
@@ -48,7 +92,7 @@ describe('Middleware', () => {
 
 			await guestMiddleware(toMock, fromMock, nextMock, {});
 
-			expect(nextMock).toHaveBeenCalledTimes(0);
+			expect(nextMock).not.toHaveBeenCalled();
 		});
 	});
 });

packages/frontend/editor-ui/src/utils/rbac/middleware/guest.ts

@@ -10,11 +10,24 @@ export const guestMiddleware: RouterMiddleware<GuestPermissionOptions> = async (
 ) => {
 	const valid = isGuest();
 	if (!valid) {
-		const redirect = to.query.redirect as string;
-		if (redirect && (redirect.startsWith('/') || redirect.startsWith(window.location.origin))) {
+		const redirect = (to.query.redirect as string) ?? '';
+
+		// Allow local path redirects
+		if (redirect.startsWith('/')) {
 			return next(redirect);
 		}
 
+		try {
+			// Only allow origin domain redirects
+			const url = new URL(redirect);
+			if (url.origin === window.location.origin) {
+				return next(redirect);
+			}
+		} catch {
+			// Intentionally fall through to redirect to homepage
+			// if the redirect is an invalid URL
+		}
+
 		return next({ name: VIEWS.HOMEPAGE });
 	}
 };