Skip to content

Commit

Permalink
feat: new helper functions to replace regexes #807
Browse files Browse the repository at this point in the history
Implemented new StringUtil helper functions: splitCommaSeparated, sanitizeUrlPart, isValidFileName.
Added helper functions to HeaderUtil: matchesAuthorizationScheme, hasScheme.
Added unit tests for the new helper functions.
Refactored codebase to use helper functions instead of regexes if applicable.
  • Loading branch information
wkerckho committed Apr 13, 2022
1 parent 1b7cc1e commit f3e1016
Show file tree
Hide file tree
Showing 18 changed files with 186 additions and 23 deletions.
3 changes: 2 additions & 1 deletion src/authentication/BearerWebIdExtractor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { getLoggerFor } from '../logging/LogUtil';
import type { HttpRequest } from '../server/HttpRequest';
import { BadRequestHttpError } from '../util/errors/BadRequestHttpError';
import { NotImplementedHttpError } from '../util/errors/NotImplementedHttpError';
import { matchesAuthorizationScheme } from '../util/HeaderUtil';
import { CredentialGroup } from './Credentials';
import type { CredentialSet } from './Credentials';
import { CredentialsExtractor } from './CredentialsExtractor';
Expand All @@ -19,7 +20,7 @@ export class BearerWebIdExtractor extends CredentialsExtractor {

public async canHandle({ headers }: HttpRequest): Promise<void> {
const { authorization } = headers;
if (!authorization || !/^Bearer /ui.test(authorization)) {
if (!matchesAuthorizationScheme('Bearer', authorization)) {
throw new NotImplementedHttpError('No Bearer Authorization header specified.');
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/authentication/DPoPWebIdExtractor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { getLoggerFor } from '../logging/LogUtil';
import type { HttpRequest } from '../server/HttpRequest';
import { BadRequestHttpError } from '../util/errors/BadRequestHttpError';
import { NotImplementedHttpError } from '../util/errors/NotImplementedHttpError';
import { matchesAuthorizationScheme } from '../util/HeaderUtil';
import { CredentialGroup } from './Credentials';
import type { CredentialSet } from './Credentials';
import { CredentialsExtractor } from './CredentialsExtractor';
Expand All @@ -27,7 +28,7 @@ export class DPoPWebIdExtractor extends CredentialsExtractor {

public async canHandle({ headers }: HttpRequest): Promise<void> {
const { authorization } = headers;
if (!authorization || !/^DPoP /ui.test(authorization)) {
if (!matchesAuthorizationScheme('DPoP', authorization)) {
throw new NotImplementedHttpError('No DPoP-bound Authorization header specified.');
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/authentication/UnsecureWebIdExtractor.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { getLoggerFor } from '../logging/LogUtil';
import type { HttpRequest } from '../server/HttpRequest';
import { NotImplementedHttpError } from '../util/errors/NotImplementedHttpError';
import { matchesAuthorizationScheme } from '../util/HeaderUtil';
import { CredentialGroup } from './Credentials';
import type { CredentialSet } from './Credentials';
import { CredentialsExtractor } from './CredentialsExtractor';
Expand All @@ -13,7 +14,7 @@ export class UnsecureWebIdExtractor extends CredentialsExtractor {

public async canHandle({ headers }: HttpRequest): Promise<void> {
const { authorization } = headers;
if (!authorization || !/^WebID /ui.test(authorization)) {
if (!matchesAuthorizationScheme('WebID', authorization)) {
throw new NotImplementedHttpError('No WebID Authorization header specified.');
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/http/UnsecureWebSocketsProtocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { getLoggerFor } from '../logging/LogUtil';
import type { HttpRequest } from '../server/HttpRequest';
import { WebSocketHandler } from '../server/WebSocketHandler';
import { parseForwarded } from '../util/HeaderUtil';
import { splitCommaSeparated } from '../util/StringUtil';
import type { ResourceIdentifier } from './representation/ResourceIdentifier';

const VERSION = 'solid-0.1';
Expand Down Expand Up @@ -36,7 +37,7 @@ class WebSocketListener extends EventEmitter {
if (!protocolHeader) {
this.sendMessage('warning', `Missing Sec-WebSocket-Protocol header, expected value '${VERSION}'`);
} else {
const supportedProtocols = protocolHeader.split(/\s*,\s*/u);
const supportedProtocols = splitCommaSeparated(protocolHeader);
if (!supportedProtocols.includes(VERSION)) {
this.sendMessage('error', `Client does not support protocol ${VERSION}`);
this.stop();
Expand Down
6 changes: 5 additions & 1 deletion src/http/input/conditions/BasicConditionsParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type { HttpRequest } from '../../../server/HttpRequest';
import type { BasicConditionsOptions } from '../../../storage/BasicConditions';
import { BasicConditions } from '../../../storage/BasicConditions';
import type { Conditions } from '../../../storage/Conditions';
import { splitCommaSeparated } from '../../../util/StringUtil';
import { ConditionsParser } from './ConditionsParser';

/**
Expand Down Expand Up @@ -58,6 +59,9 @@ export class BasicConditionsParser extends ConditionsParser {
* Undefined if there is no value for the given header name.
*/
private parseTagHeader(request: HttpRequest, header: 'if-match' | 'if-none-match'): string[] | undefined {
return request.headers[header]?.trim().split(/\s*,\s*/u);
const headerValue = request.headers[header];
if (headerValue) {
return splitCommaSeparated(headerValue.trim());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { getLoggerFor } from '../../../../logging/LogUtil';
import type { IdentifierGenerator } from '../../../../pods/generate/IdentifierGenerator';
import type { PodManager } from '../../../../pods/PodManager';
import type { PodSettings } from '../../../../pods/settings/PodSettings';
import { hasScheme } from '../../../../util/HeaderUtil';
import { joinUrl } from '../../../../util/PathUtil';
import type { OwnershipValidator } from '../../../ownership/OwnershipValidator';
import { assertPassword } from '../EmailPasswordUtil';
Expand Down Expand Up @@ -139,7 +140,7 @@ export class RegistrationManager {
// Parse WebID
if (!validated.createWebId) {
const trimmedWebId = this.trimString(webId);
assert(trimmedWebId && /^https?:\/\/[^/]+/u.test(trimmedWebId), 'Please enter a valid WebID.');
assert(trimmedWebId && hasScheme(trimmedWebId, 'http', 'https'), 'Please enter a valid WebID.');
validated.webId = trimmedWebId;
}

Expand Down
3 changes: 2 additions & 1 deletion src/identity/storage/WebIdAdapterFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { getLoggerFor } from '../../logging/LogUtil';
import type { RepresentationConverter } from '../../storage/conversion/RepresentationConverter';
import { createErrorMessage } from '../../util/errors/ErrorUtil';
import { responseToDataset } from '../../util/FetchUtil';
import { hasScheme } from '../../util/HeaderUtil';
import { OIDC } from '../../util/Vocabularies';
import type { AdapterFactory } from './AdapterFactory';

Expand Down Expand Up @@ -42,7 +43,7 @@ export class WebIdAdapter implements Adapter {
// Try to see if valid client metadata is found at the given Client ID.
// The oidc-provider library will check if the redirect_uri matches an entry in the list of redirect_uris,
// so no extra checks are needed from our side.
if (!payload && this.name === 'Client' && /^https?:\/\/.+/u.test(id)) {
if (!payload && this.name === 'Client' && hasScheme(id, 'http', 'https')) {
this.logger.debug(`Looking for payload data at ${id}`);
// All checks based on https://solid.github.io/authentication-panel/solid-oidc/#clientids-webid
if (!/^https:|^http:\/\/localhost(?::\d+)?(?:\/|$)/u.test(id)) {
Expand Down
1 change: 1 addition & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,7 @@ export * from './util/QuadUtil';
export * from './util/RecordObject';
export * from './util/ResourceUtil';
export * from './util/StreamUtil';
export * from './util/StringUtil';
export * from './util/TermUtil';
export * from './util/TimerUtil';
export * from './util/Vocabularies';
3 changes: 2 additions & 1 deletion src/pods/generate/SubdomainIdentifierGenerator.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type { ResourceIdentifier } from '../../http/representation/ResourceIdentifier';
import { ensureTrailingSlash, extractScheme } from '../../util/PathUtil';
import { sanitizeUrlPart } from '../../util/StringUtil';
import type { IdentifierGenerator } from './IdentifierGenerator';

/**
Expand All @@ -15,7 +16,7 @@ export class SubdomainIdentifierGenerator implements IdentifierGenerator {

public generate(name: string): ResourceIdentifier {
// Using the punycode converter is a risk as it doesn't convert slashes for example
const cleanName = name.replace(/\W/gu, '-');
const cleanName = sanitizeUrlPart(name);
return { path: `${this.baseParts.scheme}${cleanName}.${this.baseParts.rest}` };
}
}
3 changes: 2 additions & 1 deletion src/pods/generate/SuffixIdentifierGenerator.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type { ResourceIdentifier } from '../../http/representation/ResourceIdentifier';
import { ensureTrailingSlash } from '../../util/PathUtil';
import { sanitizeUrlPart } from '../../util/StringUtil';
import type { IdentifierGenerator } from './IdentifierGenerator';

/**
Expand All @@ -14,7 +15,7 @@ export class SuffixIdentifierGenerator implements IdentifierGenerator {
}

public generate(name: string): ResourceIdentifier {
const cleanName = name.replace(/\W/gu, '-');
const cleanName = sanitizeUrlPart(name);
return { path: ensureTrailingSlash(new URL(cleanName, this.base).href) };
}
}
4 changes: 2 additions & 2 deletions src/server/middleware/WebSocketAdvertiser.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { addHeader } from '../../util/HeaderUtil';
import { addHeader, hasScheme } from '../../util/HeaderUtil';
import { HttpHandler } from '../HttpHandler';
import type { HttpResponse } from '../HttpResponse';

Expand All @@ -11,7 +11,7 @@ export class WebSocketAdvertiser extends HttpHandler {
public constructor(baseUrl: string) {
super();
const socketUrl = new URL(baseUrl);
socketUrl.protocol = /^(?:http|ws):/u.test(baseUrl) ? 'ws:' : 'wss:';
socketUrl.protocol = hasScheme(baseUrl, 'http', 'ws') ? 'ws:' : 'wss:';
this.socketUrl = socketUrl.href;
}

Expand Down
3 changes: 2 additions & 1 deletion src/storage/IndexRepresentationStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { RepresentationPreferences } from '../http/representation/Represent
import type { ResourceIdentifier } from '../http/representation/ResourceIdentifier';
import { NotFoundHttpError } from '../util/errors/NotFoundHttpError';
import { isContainerIdentifier } from '../util/PathUtil';
import { isValidFileName } from '../util/StringUtil';
import type { Conditions } from './Conditions';
import { cleanPreferences, matchesMediaType } from './conversion/ConversionUtil';
import { PassthroughStore } from './PassthroughStore';
Expand Down Expand Up @@ -31,7 +32,7 @@ export class IndexRepresentationStore extends PassthroughStore {

public constructor(source: ResourceStore, indexName = 'index.html', mediaRange = 'text/html') {
super(source);
assert(/^[\w.-]+$/u.test(indexName), 'Invalid index name');
assert(isValidFileName(indexName), 'Invalid index name');
this.indexName = indexName;
this.mediaRange = mediaRange;
}
Expand Down
3 changes: 2 additions & 1 deletion src/storage/conversion/ErrorToTemplateConverter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { INTERNAL_ERROR } from '../../util/ContentTypes';
import { HttpError } from '../../util/errors/HttpError';
import { resolveModulePath } from '../../util/PathUtil';
import { getSingleItem } from '../../util/StreamUtil';
import { isValidFileName } from '../../util/StringUtil';
import type { TemplateEngine } from '../../util/templates/TemplateEngine';
import { BaseTypedRepresentationConverter } from './BaseTypedRepresentationConverter';
import type { RepresentationConverterArgs } from './RepresentationConverter';
Expand Down Expand Up @@ -63,7 +64,7 @@ export class ErrorToTemplateConverter extends BaseTypedRepresentationConverter {
if (HttpError.isInstance(error)) {
try {
const templateFile = `${error.errorCode}${this.extension}`;
assert(/^[\w.-]+$/u.test(templateFile), 'Invalid error template name');
assert(isValidFileName(templateFile), 'Invalid error template name');
description = await this.templateEngine.render(error.details ?? {},
{ templateFile, templatePath: this.codeTemplatesPath });
} catch {
Expand Down
33 changes: 33 additions & 0 deletions src/util/HeaderUtil.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import type { IncomingHttpHeaders } from 'http';
import escapeStringRegexp from 'escape-string-regexp';
import { getLoggerFor } from '../logging/LogUtil';
import type { HttpResponse } from '../server/HttpResponse';
import { BadRequestHttpError } from './errors/BadRequestHttpError';

const logger = getLoggerFor('HeaderUtil');
// Map used as a simple cache in the helper function matchesAuthorizationScheme.
const authSchemeRegexCache: Map<string, RegExp> = new Map();

// BNF based on https://tools.ietf.org/html/rfc7231
//
Expand Down Expand Up @@ -508,6 +511,7 @@ export function parseForwarded(headers: IncomingHttpHeaders): Forwarded {

/**
* Parses the link header(s) and returns an array of LinkEntry objects.
*
* @param link - A single link header or an array of link headers
* @returns A LinkEntry array, LinkEntry contains a link and a params Record&lt;string,string&gt;
*/
Expand Down Expand Up @@ -547,3 +551,32 @@ export function parseLinkHeader(link: string | string[] = []): LinkEntry[] {
}
return links;
}

/**
* Checks if the value of an HTTP Authorization header matches a specific scheme (e.g. Basic, Bearer, etc).
*
* @param scheme - Name of the authorization scheme (case insensitive).
* @param authorization - The value of the Authorization header (may be undefined).
* @returns True if the Authorization header uses the specified scheme, false otherwise.
*/
export function matchesAuthorizationScheme(scheme: string, authorization?: string): boolean {
const lowerCaseScheme = scheme.toLowerCase();
if (!authSchemeRegexCache.has(lowerCaseScheme)) {
authSchemeRegexCache.set(lowerCaseScheme, new RegExp(`^${escapeStringRegexp(lowerCaseScheme)} `, 'ui'));
}
// Support authorization being undefined (for the sake of usability).
return typeof authorization !== 'undefined' && authSchemeRegexCache.get(lowerCaseScheme)!.test(authorization);
}

/**
* Checks if the scheme part of the specified url matches at least one of the provided options.
*
* @param url - A string representing the URL.
* @param schemes - Scheme value options (the function will check if at least one matches the URL scheme).
* @returns True if the URL scheme matches at least one of the provided options, false otherwise.
*/
export function hasScheme(url: string, ...schemes: string[]): boolean {
const schemeOptions = new Set(schemes.map((item): string => item.toLowerCase()));
const urlSchemeResult = /^(.+?):\/\//u.exec(url);
return urlSchemeResult ? schemeOptions.has(urlSchemeResult[1].toLowerCase()) : false;
}
30 changes: 30 additions & 0 deletions src/util/StringUtil.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/**
* Splits a string by comma.
*
* @param input - String instance to split.
*
* @returns A String array containining the split parts.
*/
export function splitCommaSeparated(input: string): string[] {
return input.split(/\s*,\s*/u);
}

/**
* Sanitizes part of a URL by replacing non-word content with a '-'.
*
* @param urlPart - The URL part to sanitize.
* @returns The sanitized output.
*/
export function sanitizeUrlPart(urlPart: string): string {
return urlPart.replace(/\W/gu, '-');
}

/**
* Checks the validity of a file name. A valid name consists of word characters, '-' or '.'.
*
* @param name - The name of the file to validate.
* @returns True if the filename is valid, false otherwise.
*/
export function isValidFileName(name: string): boolean {
return /^[\w.-]+$/u.test(name);
}
21 changes: 11 additions & 10 deletions test/integration/Middleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import request from 'supertest';
import type { BaseHttpServerFactory } from '../../src/server/BaseHttpServerFactory';
import type { HttpHandlerInput } from '../../src/server/HttpHandler';
import { HttpHandler } from '../../src/server/HttpHandler';
import { splitCommaSeparated } from '../../src/util/StringUtil';
import { getPort } from '../util/Util';
import { getTestConfigPath, instantiateFromConfig } from './Config';

Expand Down Expand Up @@ -96,46 +97,46 @@ describe('An http server with middleware', (): void => {
it('exposes the Accept-[Method] header via CORS.', async(): Promise<void> => {
const res = await request(server).get('/').expect(200);
const exposed = res.header['access-control-expose-headers'];
expect(exposed.split(/\s*,\s*/u)).toContain('Accept-Patch');
expect(exposed.split(/\s*,\s*/u)).toContain('Accept-Post');
expect(exposed.split(/\s*,\s*/u)).toContain('Accept-Put');
expect(splitCommaSeparated(exposed)).toContain('Accept-Patch');
expect(splitCommaSeparated(exposed)).toContain('Accept-Post');
expect(splitCommaSeparated(exposed)).toContain('Accept-Put');
});

it('exposes the Last-Modified and ETag headers via CORS.', async(): Promise<void> => {
const res = await request(server).get('/').expect(200);
const exposed = res.header['access-control-expose-headers'];
expect(exposed.split(/\s*,\s*/u)).toContain('ETag');
expect(exposed.split(/\s*,\s*/u)).toContain('Last-Modified');
expect(splitCommaSeparated(exposed)).toContain('ETag');
expect(splitCommaSeparated(exposed)).toContain('Last-Modified');
});

it('exposes the Link header via CORS.', async(): Promise<void> => {
const res = await request(server).get('/').expect(200);
const exposed = res.header['access-control-expose-headers'];
expect(exposed.split(/\s*,\s*/u)).toContain('Link');
expect(splitCommaSeparated(exposed)).toContain('Link');
});

it('exposes the Location header via CORS.', async(): Promise<void> => {
const res = await request(server).get('/').expect(200);
const exposed = res.header['access-control-expose-headers'];
expect(exposed.split(/\s*,\s*/u)).toContain('Location');
expect(splitCommaSeparated(exposed)).toContain('Location');
});

it('exposes the MS-Author-Via header via CORS.', async(): Promise<void> => {
const res = await request(server).get('/').expect(200);
const exposed = res.header['access-control-expose-headers'];
expect(exposed.split(/\s*,\s*/u)).toContain('MS-Author-Via');
expect(splitCommaSeparated(exposed)).toContain('MS-Author-Via');
});

it('exposes the WAC-Allow header via CORS.', async(): Promise<void> => {
const res = await request(server).get('/').expect(200);
const exposed = res.header['access-control-expose-headers'];
expect(exposed.split(/\s*,\s*/u)).toContain('WAC-Allow');
expect(splitCommaSeparated(exposed)).toContain('WAC-Allow');
});

it('exposes the Updates-Via header via CORS.', async(): Promise<void> => {
const res = await request(server).get('/').expect(200);
const exposed = res.header['access-control-expose-headers'];
expect(exposed.split(/\s*,\s*/u)).toContain('Updates-Via');
expect(splitCommaSeparated(exposed)).toContain('Updates-Via');
});

it('sends incoming requests to the handler.', async(): Promise<void> => {
Expand Down
Loading

0 comments on commit f3e1016

Please sign in to comment.