fix: Make the RouterHandler more robust

It now extracts paths based on the base URL and allows catching all methods.
2024-10-03 14:55:10 +00:00 · 2021-09-15 16:15:57 +02:00
parent 42d3ab0a4c
commit fb0b50c997
6 changed files with 164 additions and 51 deletions
--- a/config/identity/handler/default.json
+++ b/config/identity/handler/default.json
@@ -11,9 +11,11 @@
      "comment": "Routes all IDP related requests to the relevant handlers.",
      "@id": "urn:solid-server:default:IdentityProviderHandler",
      "@type": "RouterHandler",
-      "allowedMethods": [ "GET", "POST", "PUT", "DELETE", "OPTIONS" ],
-      "allowedPathNames": [ "^/idp/.*", "^/\\.well-known/openid-configuration" ],
-      "handler": { "@id": "urn:solid-server:default:IdentityProviderHttpHandler" }
+      "args_baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" },
+      "args_targetExtractor": { "@id": "urn:solid-server:default:TargetExtractor" },
+      "args_allowedMethods": [ "*" ],
+      "args_allowedPathNames": [ "^/idp/.*", "^/\\.well-known/openid-configuration" ],
+      "args_handler": { "@id": "urn:solid-server:default:IdentityProviderHttpHandler" }
    },
    {
      "@id": "urn:solid-server:default:IdentityProviderHttpHandler",
--- a/config/ldp/handler/components/request-parser.json
+++ b/config/ldp/handler/components/request-parser.json
@@ -6,6 +6,7 @@
      "@id": "urn:solid-server:default:RequestParser",
      "@type": "BasicRequestParser",
      "args_targetExtractor": {
+        "@id": "urn:solid-server:default:TargetExtractor",
        "@type": "OriginalUrlExtractor",
        "options_includeQueryString": false
      },
--- a/src/server/util/RouterHandler.ts
+++ b/src/server/util/RouterHandler.ts
@@ -1,41 +1,57 @@
-import { parse } from 'url';
+import type { TargetExtractor } from '../../ldp/http/TargetExtractor';
+import { BadRequestHttpError } from '../../util/errors/BadRequestHttpError';
 import { MethodNotAllowedHttpError } from '../../util/errors/MethodNotAllowedHttpError';
 import { NotFoundHttpError } from '../../util/errors/NotFoundHttpError';
+import { ensureTrailingSlash, getRelativeUrl } from '../../util/PathUtil';
 import type { HttpHandlerInput } from '../HttpHandler';
 import { HttpHandler } from '../HttpHandler';

+export interface RouterHandlerArgs {
+  baseUrl: string;
+  targetExtractor: TargetExtractor;
+  handler: HttpHandler;
+  allowedMethods: string[];
+  allowedPathNames: string[];
+}
+
 /**
 * An HttpHandler that checks if a given method and path are satisfied
 * and allows its handler to be executed if so.
+ *
+ * If `allowedMethods` contains '*' it will match all methods.
 */
 export class RouterHandler extends HttpHandler {
-  protected readonly handler: HttpHandler;
-  protected readonly allowedMethods: string[];
-  protected readonly allowedPathNamesRegEx: RegExp[];
+  private readonly baseUrl: string;
+  private readonly targetExtractor: TargetExtractor;
+  private readonly handler: HttpHandler;
+  private readonly allowedMethods: string[];
+  private readonly allMethods: boolean;
+  private readonly allowedPathNamesRegEx: RegExp[];

-  public constructor(handler: HttpHandler, allowedMethods: string[], allowedPathNames: string[]) {
+  public constructor(args: RouterHandlerArgs) {
    super();
-    this.handler = handler;
-    this.allowedMethods = allowedMethods;
-    this.allowedPathNamesRegEx = allowedPathNames.map((pn): RegExp => new RegExp(pn, 'u'));
+    this.baseUrl = ensureTrailingSlash(args.baseUrl);
+    this.targetExtractor = args.targetExtractor;
+    this.handler = args.handler;
+    this.allowedMethods = args.allowedMethods;
+    this.allMethods = args.allowedMethods.includes('*');
+    this.allowedPathNamesRegEx = args.allowedPathNames.map((pn): RegExp => new RegExp(pn, 'u'));
  }

  public async canHandle(input: HttpHandlerInput): Promise<void> {
-    if (!input.request.url) {
-      throw new Error('Cannot handle request without a url');
+    const { request } = input;
+    if (!request.url) {
+      throw new BadRequestHttpError('Cannot handle request without a url');
    }
-    if (!input.request.method) {
-      throw new Error('Cannot handle request without a method');
+    if (!request.method) {
+      throw new BadRequestHttpError('Cannot handle request without a method');
    }
-    if (!this.allowedMethods.includes(input.request.method)) {
-      throw new MethodNotAllowedHttpError(`${input.request.method} is not allowed.`);
+    if (!this.allMethods && !this.allowedMethods.includes(request.method)) {
+      throw new MethodNotAllowedHttpError(`${request.method} is not allowed.`);
    }
-    const { pathname } = parse(input.request.url);
-    if (!pathname) {
-      throw new Error('Cannot handle request without pathname');
-    }
-    if (!this.allowedPathNamesRegEx.some((regex): boolean => regex.test(pathname))) {
-      throw new NotFoundHttpError(`Cannot handle route ${pathname}`);
+    const pathName = await getRelativeUrl(this.baseUrl, request, this.targetExtractor);
+    if (!this.allowedPathNamesRegEx.some((regex): boolean => regex.test(pathName))) {
+      throw new NotFoundHttpError(`Cannot handle route ${pathName}`);
    }
    await this.handler.canHandle(input);
  }
--- a/src/util/PathUtil.ts
+++ b/src/util/PathUtil.ts
@@ -1,6 +1,9 @@
 import { posix, win32 } from 'path';
 import urljoin from 'url-join';
+import type { TargetExtractor } from '../ldp/http/TargetExtractor';
 import type { ResourceIdentifier } from '../ldp/representation/ResourceIdentifier';
+import type { HttpRequest } from '../server/HttpRequest';
+import { BadRequestHttpError } from './errors/BadRequestHttpError';

 /**
 * Changes a potential Windows path into a POSIX path.
@@ -150,6 +153,24 @@ export function extractScheme(url: string): { scheme: string; rest: string } {
  return { scheme: match[1], rest: match[2] };
 }

+/**
+ * Creates a relative URL by removing the base URL.
+ * Will throw an error in case the resulting target is not withing the base URL scope.
+ * @param baseUrl - Base URL.
+ * @param request - Incoming request of which the target needs to be extracted.
+ * @param targetExtractor - Will extract the target from the request.
+ */
+export async function getRelativeUrl(baseUrl: string, request: HttpRequest, targetExtractor: TargetExtractor):
+Promise<string> {
+  baseUrl = ensureTrailingSlash(baseUrl);
+  const target = await targetExtractor.handleSafe({ request });
+  if (!target.path.startsWith(baseUrl)) {
+    throw new BadRequestHttpError(`The identifier ${target.path} is outside the configured identifier space.`,
+      { errorCode: 'E0001', details: { path: target.path }});
+  }
+  return target.path.slice(baseUrl.length - 1);
+}
+
 /**
 * Creates a regular expression that matches URLs containing the given baseUrl, or a subdomain of the given baseUrl.
 * In case there is a subdomain, the first match of the regular expression will be that subdomain.
--- a/test/unit/server/util/RouterHandler.test.ts
+++ b/test/unit/server/util/RouterHandler.test.ts
@@ -1,17 +1,41 @@
 import { createRequest, createResponse } from 'node-mocks-http';
-import type { AsyncHandler, HttpHandlerInput, HttpRequest, HttpResponse } from '../../../../src';
-import { guardStream } from '../../../../src';
+import type {
+  AsyncHandler,
+  HttpHandlerInput,
+  HttpRequest,
+  HttpResponse,
+  TargetExtractor,
+  ResourceIdentifier,
+  RouterHandlerArgs,
+} from '../../../../src';
+import { guardStream, joinUrl } from '../../../../src';
 import { RouterHandler } from '../../../../src/server/util/RouterHandler';
 import { StaticAsyncHandler } from '../../../util/StaticAsyncHandler';

-describe('RouterHandler', (): void => {
+describe('A RouterHandler', (): void => {
+  const baseUrl = 'http://test.com/foo/';
+  let targetExtractor: jest.Mocked<TargetExtractor>;
  let subHandler: AsyncHandler<any, any>;
  let genericRequest: HttpRequest;
  let genericResponse: HttpResponse;
  let genericInput: HttpHandlerInput;
+  let args: RouterHandlerArgs;

  beforeEach((): void => {
+    targetExtractor = {
+      handleSafe: jest.fn(({ request: req }): ResourceIdentifier => ({ path: joinUrl(baseUrl, req.url!) })),
+    } as any;
+
    subHandler = new StaticAsyncHandler(true, undefined);
+
+    args = {
+      baseUrl,
+      targetExtractor,
+      handler: subHandler,
+      allowedMethods: [],
+      allowedPathNames: [],
+    };
+
    genericRequest = guardStream(createRequest({
      url: '/test',
    }));
@@ -23,12 +47,16 @@ describe('RouterHandler', (): void => {
  });

  it('calls the sub handler when handle is called.', async(): Promise<void> => {
-    const handler = new RouterHandler(subHandler, [ 'GET' ], [ '/test' ]);
+    args.allowedMethods = [ 'GET' ];
+    args.allowedPathNames = [ '/test' ];
+    const handler = new RouterHandler(args);
    expect(await handler.handle(genericInput)).toBeUndefined();
  });

  it('throws an error if the request does not have a url.', async(): Promise<void> => {
-    const handler = new RouterHandler(subHandler, [ 'GET' ], [ '/test' ]);
+    args.allowedMethods = [ 'GET' ];
+    args.allowedPathNames = [ '/test' ];
+    const handler = new RouterHandler(args);
    const request = guardStream(createRequest());
    await expect(handler.canHandle({
      request,
@@ -37,7 +65,9 @@ describe('RouterHandler', (): void => {
  });

  it('throws an error if the request does not have a method.', async(): Promise<void> => {
-    const handler = new RouterHandler(subHandler, [ 'GET' ], [ '/test' ]);
+    args.allowedMethods = [ 'GET' ];
+    args.allowedPathNames = [ '/test' ];
+    const handler = new RouterHandler(args);
    const request = guardStream(createRequest({
      url: '/test',
    }));
@@ -49,45 +79,53 @@ describe('RouterHandler', (): void => {
    })).rejects.toThrow('Cannot handle request without a method');
  });

-  it('throws an error if the request does not have a pathname.', async(): Promise<void> => {
-    const handler = new RouterHandler(subHandler, [ 'GET' ], [ '/test' ]);
-    const request = guardStream(createRequest({
-      url: '?bad=pathname',
-    }));
-    await expect(handler.canHandle({
-      request,
-      response: genericResponse,
-    })).rejects.toThrow('Cannot handle request without pathname');
-  });
-
  it('throws an error when there are no allowed methods or pathnames.', async(): Promise<void> => {
-    const handler = new RouterHandler(subHandler, [], []);
+    args.allowedMethods = [];
+    args.allowedPathNames = [];
+    const handler = new RouterHandler(args);
    await expect(handler.canHandle(genericInput)).rejects.toThrow('GET is not allowed.');
  });

  it('throws an error when there are no allowed methods.', async(): Promise<void> => {
-    const handler = new RouterHandler(subHandler, [], [ '/test' ]);
+    args.allowedMethods = [];
+    args.allowedPathNames = [ '/test' ];
+    const handler = new RouterHandler(args);
    await expect(handler.canHandle(genericInput)).rejects.toThrow('GET is not allowed.');
  });

  it('throws an error when there are no allowed pathnames.', async(): Promise<void> => {
-    const handler = new RouterHandler(subHandler, [ 'GET' ], []);
+    args.allowedMethods = [ 'GET' ];
+    args.allowedPathNames = [];
+    const handler = new RouterHandler(args);
    await expect(handler.canHandle(genericInput)).rejects.toThrow('Cannot handle route /test');
  });

  it('throws an error if the RegEx string is not valid Regex.', async(): Promise<void> => {
-    expect((): RouterHandler => new RouterHandler(subHandler, [ 'GET' ], [ '[' ]))
+    args.allowedMethods = [ 'GET' ];
+    args.allowedPathNames = [ '[' ];
+    expect((): RouterHandler => new RouterHandler(args))
      .toThrow('Invalid regular expression: /[/: Unterminated character class');
  });

  it('throws an error if all else is successful, but the sub handler cannot handle.', async(): Promise<void> => {
-    const rejectingHandler = new StaticAsyncHandler(false, undefined);
-    const handler = new RouterHandler(rejectingHandler, [ 'GET' ], [ '/test' ]);
+    args.handler = new StaticAsyncHandler(false, undefined);
+    args.allowedMethods = [ 'GET' ];
+    args.allowedPathNames = [ '/test' ];
+    const handler = new RouterHandler(args);
    await expect(handler.canHandle(genericInput)).rejects.toThrow('Not supported');
  });

  it('does not throw an error if the sub handler is successful.', async(): Promise<void> => {
-    const handler = new RouterHandler(subHandler, [ 'GET' ], [ '/test' ]);
+    args.allowedMethods = [ 'GET' ];
+    args.allowedPathNames = [ '/test' ];
+    const handler = new RouterHandler(args);
+    expect(await handler.canHandle(genericInput)).toBeUndefined();
+  });
+
+  it('supports * for all methods.', async(): Promise<void> => {
+    args.allowedMethods = [ '*' ];
+    args.allowedPathNames = [ '/test' ];
+    const handler = new RouterHandler(args);
    expect(await handler.canHandle(genericInput)).toBeUndefined();
  });
 });
--- a/test/unit/util/PathUtil.test.ts
+++ b/test/unit/util/PathUtil.test.ts
@@ -1,12 +1,25 @@
 import { existsSync } from 'fs';
+import type { TargetExtractor } from '../../../src/ldp/http/TargetExtractor';
+import type { ResourceIdentifier } from '../../../src/ldp/representation/ResourceIdentifier';
+import type { HttpRequest } from '../../../src/server/HttpRequest';
 import {
-  absoluteFilePath, createSubdomainRegexp,
+  absoluteFilePath,
+  createSubdomainRegexp,
  decodeUriPathComponents,
  encodeUriPathComponents,
-  ensureTrailingSlash, extractScheme, getExtension, getModuleRoot, isContainerIdentifier, isContainerPath,
+  ensureTrailingSlash,
+  extractScheme,
+  getExtension,
+  getModuleRoot,
+  getRelativeUrl,
+  isContainerIdentifier,
+  isContainerPath,
  joinFilePath,
-  normalizeFilePath, resolveAssetPath,
-  toCanonicalUriPath, trimTrailingSlashes,
+  joinUrl,
+  normalizeFilePath,
+  resolveAssetPath,
+  toCanonicalUriPath,
+  trimTrailingSlashes,
 } from '../../../src/util/PathUtil';

 describe('PathUtil', (): void => {
@@ -119,6 +132,28 @@ describe('PathUtil', (): void => {
    });
  });

+  describe('#getRelativeUrl', (): void => {
+    const baseUrl = 'http://test.com/foo/';
+    const request: HttpRequest = { url: '/resource' } as any;
+    let targetExtractor: jest.Mocked<TargetExtractor>;
+
+    beforeEach((): void => {
+      targetExtractor = {
+        handleSafe: jest.fn(({ request: req }): ResourceIdentifier => ({ path: joinUrl(baseUrl, req.url!) })),
+      } as any;
+    });
+
+    it('returns the relative path.', async(): Promise<void> => {
+      await expect(getRelativeUrl(baseUrl, request, targetExtractor)).resolves.toBe('/resource');
+    });
+
+    it('errors if the target is outside of the server scope.', async(): Promise<void> => {
+      targetExtractor.handleSafe.mockResolvedValueOnce({ path: 'http://somewhere.else/resource' });
+      await expect(getRelativeUrl(baseUrl, request, targetExtractor)).rejects
+        .toThrow(expect.objectContaining({ errorCode: 'E0001', details: { path: 'http://somewhere.else/resource' }}));
+    });
+  });
+
  describe('#createSubdomainRegexp', (): void => {
    it('creates a regex to match the URL and extract a subdomain.', async(): Promise<void> => {
      const regex = createSubdomainRegexp('http://test.com/foo/');