Skip to content

Commit

Permalink
Merge commit from fork
Browse files Browse the repository at this point in the history 
to avoid path traversal attacks
and remove the legacy allowance for fragments in MXCs

Signed-off-by: Michael Telatynski <[email protected]>
  • Loading branch information
@t3chguy
t3chguy authored Nov 12, 2024
1 parent c4048d9 commit 00aba74
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 44 deletions.
39 changes: 25 additions & 14 deletions spec/unit/content-repo.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,20 +63,6 @@ describe("ContentRepo", function () {
);
});

it("should put fragments from mxc:// URIs after any query parameters", function () {
const mxcUri = "mxc://server.name/resourceid#automade";
expect(getHttpUriForMxc(baseUrl, mxcUri, 32)).toEqual(
baseUrl + "/_matrix/media/v3/thumbnail/server.name/resourceid" + "?width=32#automade",
);
});

it("should put fragments from mxc:// URIs at the end of the HTTP URI", function () {
const mxcUri = "mxc://server.name/resourceid#automade";
expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual(
baseUrl + "/_matrix/media/v3/download/server.name/resourceid#automade",
);
});

it("should return an authenticated URL when requested", function () {
const mxcUri = "mxc://server.name/resourceid";
expect(getHttpUriForMxc(baseUrl, mxcUri, undefined, undefined, undefined, undefined, true, true)).toEqual(
Expand All @@ -98,5 +84,30 @@ describe("ContentRepo", function () {
"/_matrix/client/v1/media/thumbnail/server.name/resourceid?width=64&height=64&method=scale&allow_redirect=true",
);
});

it("should drop mxc urls with invalid server_name", () => {
const mxcUri = "mxc://server.name:test/foobar";
expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual("");
});

it("should drop mxc urls with invalid media_id", () => {
const mxcUri = "mxc://server.name/foobar:test";
expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual("");
});

it("should drop mxc urls attempting a path traversal attack", () => {
const mxcUri = "mxc://../../../../foo";
expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual("");
});

it("should drop mxc urls attempting to pass query parameters", () => {
const mxcUri = "mxc://server.name/foobar?bar=baz";
expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual("");
});

it("should drop mxc urls with too many parts", () => {
const mxcUri = "mxc://server.name/foo//bar";
expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual("");
});
});
});
65 changes: 35 additions & 30 deletions src/content-repo.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright 2015 - 2021 The Matrix.org Foundation C.I.C.
Copyright 2015 - 2024 The Matrix.org Foundation C.I.C.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand All @@ -14,7 +14,22 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import { encodeParams } from "./utils.ts";
// Validation based on https://spec.matrix.org/v1.12/appendices/#server-name
// We do not use the validation described in https://spec.matrix.org/v1.12/client-server-api/#security-considerations-5
// as it'd wrongly make all MXCs invalid due to not allowing `[].:` in server names.
const serverNameRegex =
/^(?:(?:\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})|(?:\[[\dA-Fa-f:.]{2,45}])|(?:[A-Za-z\d\-.]{1,255}))(?::\d{1,5})?$/;
function validateServerName(serverName: string): boolean {
const matches = serverNameRegex.exec(serverName);
return matches?.[0] === serverName;
}

// Validation based on https://spec.matrix.org/v1.12/client-server-api/#security-considerations-5
const mediaIdRegex = /^[\w-]+$/;
function validateMediaId(mediaId: string): boolean {
const matches = mediaIdRegex.exec(mediaId);
return matches?.[0] === mediaId;
}

/**
* Get the HTTP URL for an MXC URI.
Expand All @@ -36,7 +51,7 @@ import { encodeParams } from "./utils.ts";
* for authenticated media will *not* be checked - it is the caller's responsibility
* to do so before calling this function. Note also that `useAuthentication`
* implies `allowRedirects`. Defaults to false (unauthenticated endpoints).
* @returns The complete URL to the content.
* @returns The complete URL to the content, may be an empty string if the provided mxc is not valid.
*/
export function getHttpUriForMxc(
baseUrl: string,
Expand All @@ -51,14 +66,19 @@ export function getHttpUriForMxc(
if (typeof mxc !== "string" || !mxc) {
return "";
}
if (mxc.indexOf("mxc://") !== 0) {
if (!mxc.startsWith("mxc://")) {
if (allowDirectLinks) {
return mxc;
} else {
return "";
}
}

const [serverName, mediaId, ...rest] = mxc.slice(6).split("/");
if (rest.length > 0 || !validateServerName(serverName) || !validateMediaId(mediaId)) {
return "";
}

if (useAuthentication) {
allowRedirects = true; // per docs (MSC3916 always expects redirects)

Expand All @@ -67,46 +87,31 @@ export function getHttpUriForMxc(
// callers, hopefully.
}

let serverAndMediaId = mxc.slice(6); // strips mxc://
let prefix: string;
const isThumbnailRequest = !!width || !!height || !!resizeMethod;
const verb = isThumbnailRequest ? "thumbnail" : "download";
if (useAuthentication) {
prefix = "/_matrix/client/v1/media/download/";
prefix = `/_matrix/client/v1/media/${verb}`;
} else {
prefix = "/_matrix/media/v3/download/";
prefix = `/_matrix/media/v3/${verb}`;
}
const params: Record<string, string> = {};

const url = new URL(`${prefix}/${serverName}/${mediaId}`, baseUrl);

if (width) {
params["width"] = Math.round(width).toString();
url.searchParams.set("width", Math.round(width).toString());
}
if (height) {
params["height"] = Math.round(height).toString();
url.searchParams.set("height", Math.round(height).toString());
}
if (resizeMethod) {
params["method"] = resizeMethod;
}
if (Object.keys(params).length > 0) {
// these are thumbnailing params so they probably want the
// thumbnailing API...
if (useAuthentication) {
prefix = "/_matrix/client/v1/media/thumbnail/";
} else {
prefix = "/_matrix/media/v3/thumbnail/";
}
url.searchParams.set("method", resizeMethod);
}

if (typeof allowRedirects === "boolean") {
// We add this after, so we don't convert everything to a thumbnail request.
params["allow_redirect"] = JSON.stringify(allowRedirects);
}

const fragmentOffset = serverAndMediaId.indexOf("#");
let fragment = "";
if (fragmentOffset >= 0) {
fragment = serverAndMediaId.slice(fragmentOffset);
serverAndMediaId = serverAndMediaId.slice(0, fragmentOffset);
url.searchParams.set("allow_redirect", JSON.stringify(allowRedirects));
}

const urlParams = Object.keys(params).length === 0 ? "" : "?" + encodeParams(params);
return baseUrl + prefix + serverAndMediaId + urlParams + fragment;
return url.href;
}

0 comments on commit 00aba74

Please sign in to comment.