Skip to content

Commit

Permalink
fix: prevent exposing routing of internal handlers (#40)
Browse files Browse the repository at this point in the history
Co-authored-by: Richard Herman <[email protected]>
  • Loading branch information
GeekyEggo and GeekyEggo authored Jun 3, 2024
1 parent d00e043 commit 45c54af
Show file tree
Hide file tree
Showing 12 changed files with 52 additions and 41 deletions.
19 changes: 10 additions & 9 deletions src/common/logging/__tests__/routing.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ import type { LogEntry } from "..";
import { MessageGateway, MessageResponder, type MessageRequestOptions } from "../../messaging";
import { LogLevel } from "../level";
import { Logger } from "../logger";

import { LOGGING_WRITE_ROUTE, createRoutedLogTarget, registerCreateLogEntryRoute, type JsonSafeLogEntry } from "../routing";
import { createRoutedLogTarget, registerCreateLogEntryRoute, type JsonSafeLogEntry } from "../routing";

jest.mock("../../messaging");

const expectedLoggerWritePath = "internal:logger.write";

describe("createRoutedLogTarget", () => {
it("sends log entry to router", () => {
// Arrange.
Expand All @@ -28,7 +29,7 @@ describe("createRoutedLogTarget", () => {
message: "Hello world",
scope: "Test"
} satisfies JsonSafeLogEntry,
path: LOGGING_WRITE_ROUTE,
path: expectedLoggerWritePath,
unidirectional: true
});
});
Expand All @@ -47,7 +48,7 @@ describe("registerCreateLogEntryRoute", () => {
spyOnRoute.mock.calls[0][1](
{
action: jest.fn(),
path: LOGGING_WRITE_ROUTE,
path: expectedLoggerWritePath,
unidirectional: true,
body: undefined
},
Expand All @@ -56,7 +57,7 @@ describe("registerCreateLogEntryRoute", () => {

// Assert.
expect(spyOnRoute).toHaveBeenCalledTimes(1);
expect(spyOnRoute).toHaveBeenCalledWith(LOGGING_WRITE_ROUTE, expect.any(Function));
expect(spyOnRoute).toHaveBeenCalledWith(expectedLoggerWritePath, expect.any(Function));
expect(responder.fail).toHaveBeenCalledTimes(1);
});

Expand All @@ -71,7 +72,7 @@ describe("registerCreateLogEntryRoute", () => {
spyOnRoute.mock.calls[0][1](
{
action: jest.fn(),
path: LOGGING_WRITE_ROUTE,
path: expectedLoggerWritePath,
unidirectional: true,
body: {
level: undefined
Expand All @@ -82,7 +83,7 @@ describe("registerCreateLogEntryRoute", () => {

// Assert.
expect(spyOnRoute).toHaveBeenCalledTimes(1);
expect(spyOnRoute).toHaveBeenCalledWith(LOGGING_WRITE_ROUTE, expect.any(Function));
expect(spyOnRoute).toHaveBeenCalledWith(expectedLoggerWritePath, expect.any(Function));
expect(responder.fail).toHaveBeenCalledTimes(1);
});
});
Expand All @@ -105,7 +106,7 @@ describe("registerCreateLogEntryRoute", () => {
spyOnRoute.mock.calls[0][1](
{
action: jest.fn(),
path: LOGGING_WRITE_ROUTE,
path: expectedLoggerWritePath,
unidirectional: true,
body: {
level: LogLevel.WARN,
Expand All @@ -118,7 +119,7 @@ describe("registerCreateLogEntryRoute", () => {

// Assert.
expect(spyOnRoute).toHaveBeenCalledTimes(1);
expect(spyOnRoute).toHaveBeenCalledWith(LOGGING_WRITE_ROUTE, expect.any(Function));
expect(spyOnRoute).toHaveBeenCalledWith(expectedLoggerWritePath, expect.any(Function));
expect(spyOnWrite).toHaveBeenCalledTimes(1);
expect(spyOnWrite).toHaveBeenCalledWith<[LogEntry]>({
data: ["Hello world"],
Expand Down
8 changes: 4 additions & 4 deletions src/common/logging/routing.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import type { MessageGateway } from "../messaging";
import { INTERNAL_PATH_PREFIX, type MessageGateway } from "../messaging";
import { stringFormatter } from "./format";
import type { Logger } from "./logger";
import type { LogEntry, LogTarget } from "./target";

export const LOGGING_WRITE_ROUTE = "#internal:logger.write";
const LOGGER_WRITE_PATH = `${INTERNAL_PATH_PREFIX}logger.write`;

/**
* Creates a log target that that sends the log entry to the router.
Expand All @@ -21,7 +21,7 @@ export function createRoutedLogTarget(router: MessageGateway<unknown>): LogTarge
message: format(entry),
scope: entry.scope
} satisfies JsonSafeLogEntry,
path: LOGGING_WRITE_ROUTE,
path: LOGGER_WRITE_PATH,
unidirectional: true
});
}
Expand All @@ -34,7 +34,7 @@ export function createRoutedLogTarget(router: MessageGateway<unknown>): LogTarge
* @param logger Logger responsible for logging log entries.
*/
export function registerCreateLogEntryRoute(router: MessageGateway<unknown>, logger: Logger): void {
router.route<JsonSafeLogEntry>(LOGGING_WRITE_ROUTE, (req, res) => {
router.route<JsonSafeLogEntry>(LOGGER_WRITE_PATH, (req, res) => {
if (req.body === undefined) {
return res.fail();
}
Expand Down
3 changes: 3 additions & 0 deletions src/common/messaging/gateway.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ import { MessageResponder } from "./responder";
*/
const DEFAULT_TIMEOUT = 5000;

export const PUBLIC_PATH_PREFIX = "public:";
export const INTERNAL_PATH_PREFIX = "internal:";

/**
* Message gateway responsible for sending, routing, and receiving requests and responses.
*/
Expand Down
2 changes: 1 addition & 1 deletion src/plugin/ui/__tests__/controller.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,6 @@ describe("UIController", () => {

// Assert.
expect(spyOnRoute).toHaveBeenCalledTimes(1);
expect(spyOnRoute).toHaveBeenCalledWith("/register", handler, options);
expect(spyOnRoute).toHaveBeenCalledWith("public:/register", handler, options);
});
});
4 changes: 2 additions & 2 deletions src/plugin/ui/__tests__/property-inspector.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ describe("PropertyInspector", () => {

// Assert.
expect(spyOnFetch).toBeCalledTimes(1);
expect(spyOnFetch).toHaveBeenLastCalledWith<[string, JsonValue]>("/hello", { name: "Elgato" });
expect(spyOnFetch).toHaveBeenLastCalledWith<[string, JsonValue]>("public:/hello", { name: "Elgato" });
});

/**
Expand All @@ -71,7 +71,7 @@ describe("PropertyInspector", () => {
// Assert.
expect(spyOnFetch).toBeCalledTimes(1);
expect(spyOnFetch).toHaveBeenLastCalledWith<[MessageRequestOptions]>({
path: "/hello",
path: "public:/hello",
body: { name: "Elgato" },
timeout: 1000,
unidirectional: true
Expand Down
18 changes: 9 additions & 9 deletions src/plugin/ui/__tests__/route.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ describe("route", () => {
action.spyOnGetCharacters.mockImplementation(() => awaiter.setResult(true));

// Act.
const res = await piRouter.fetch("/characters", {
const res = await piRouter.fetch("public:/characters", {
game: "World of Warcraft"
});

Expand All @@ -41,7 +41,7 @@ describe("route", () => {
expect(action.spyOnGetCharacters).toHaveBeenLastCalledWith<[MessageRequest<Filter>, MessageResponder]>(
{
action: new Action(ev),
path: "/characters",
path: "public:/characters",
unidirectional: false,
body: {
game: "World of Warcraft"
Expand All @@ -65,7 +65,7 @@ describe("route", () => {
action.spyOnGetCharactersSync.mockImplementation(() => awaiter.setResult(true));

// Act.
const res = await piRouter.fetch("/characters-sync", {
const res = await piRouter.fetch("public:/characters-sync", {
game: "Mario World"
});

Expand All @@ -75,7 +75,7 @@ describe("route", () => {
expect(action.spyOnGetCharactersSync).toHaveBeenLastCalledWith<[MessageRequest<Filter>, MessageResponder]>(
{
action: new Action(ev),
path: "/characters-sync",
path: "public:/characters-sync",
unidirectional: false,
body: {
game: "Mario World"
Expand All @@ -95,14 +95,14 @@ describe("route", () => {
test("void", async () => {
// Arrange, act.
const action = new ActionWithRoutes();
const res = await piRouter.fetch("/save");
const res = await piRouter.fetch("public:/save");

// Assert.
expect(action.spyOnSave).toHaveBeenCalledTimes(1);
expect(action.spyOnSave).toHaveBeenLastCalledWith<[MessageRequest<Filter>, MessageResponder]>(
{
action: new Action(ev),
path: "/save",
path: "public:/save",
unidirectional: false,
body: undefined
},
Expand All @@ -124,7 +124,7 @@ describe("route", () => {
test("async", async () => {
// Arrange, act.
const action = new ActionWithRoutes();
const res = await piRouter.fetch("/characters", {
const res = await piRouter.fetch("public:/characters", {
game: "World of Warcraft"
});

Expand All @@ -141,7 +141,7 @@ describe("route", () => {
test("sync", async () => {
// Arrange, act.
const action = new ActionWithRoutes();
const res = await piRouter.fetch("/characters-sync", {
const res = await piRouter.fetch("public:/characters-sync", {
game: "Mario World"
});

Expand All @@ -158,7 +158,7 @@ describe("route", () => {
test("void", async () => {
// Arrange, act.
const action = new ActionWithRoutes();
const res = await piRouter.fetch("/save");
const res = await piRouter.fetch("public:/save");

// Assert.
expect(action.spyOnSave).toHaveBeenCalledTimes(0);
Expand Down
2 changes: 1 addition & 1 deletion src/plugin/ui/__tests__/router.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ describe("current UI", () => {
// Assert.
expect(spyOnFetch).toBeCalledTimes(1);
expect(spyOnFetch).toHaveBeenCalledWith<[MessageRequestOptions]>({
path: "/test",
path: "public:/test",
timeout: 1,
unidirectional: true
});
Expand Down
4 changes: 2 additions & 2 deletions src/plugin/ui/controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type streamDeck from "../";
import type { DidReceivePropertyInspectorMessage, PropertyInspectorDidAppear, PropertyInspectorDidDisappear } from "../../api";
import type { IDisposable } from "../../common/disposable";
import type { JsonObject, JsonValue } from "../../common/json";
import type { RouteConfiguration } from "../../common/messaging";
import { PUBLIC_PATH_PREFIX, type RouteConfiguration } from "../../common/messaging";
import { Action } from "../actions/action";
import { connection } from "../connection";
import { ActionWithoutPayloadEvent, SendToPluginEvent, type PropertyInspectorDidAppearEvent, type PropertyInspectorDidDisappearEvent } from "../events";
Expand Down Expand Up @@ -80,7 +80,7 @@ class UIController {
handler: MessageHandler<TBody, TSettings>,
options?: RouteConfiguration<Action>
): IDisposable {
return router.route(path, handler, options);
return router.route(`${PUBLIC_PATH_PREFIX}${path}`, handler, options);
}
}

Expand Down
9 changes: 6 additions & 3 deletions src/plugin/ui/property-inspector.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type streamDeck from "../";
import type { ActionIdentifier, DeviceIdentifier } from "../../api";
import type { JsonValue } from "../../common/json";
import type { MessageGateway, MessageRequestOptions, MessageResponse } from "../../common/messaging";
import { PUBLIC_PATH_PREFIX, type MessageGateway, type MessageRequestOptions, type MessageResponse } from "../../common/messaging";
import type { Action } from "../actions/action";
import { ActionContext } from "../actions/context";
import type { SingletonAction } from "../actions/singleton-action";
Expand Down Expand Up @@ -71,9 +71,12 @@ export class PropertyInspector extends ActionContext implements Pick<MessageGate
*/
public async fetch<T extends JsonValue = JsonValue>(requestOrPath: MessageRequestOptions | string, bodyOrUndefined?: JsonValue): Promise<MessageResponse<T>> {
if (typeof requestOrPath === "string") {
return this.router.fetch(requestOrPath, bodyOrUndefined);
return this.router.fetch(`${PUBLIC_PATH_PREFIX}${requestOrPath}`, bodyOrUndefined);
} else {
return this.router.fetch(requestOrPath);
return this.router.fetch({
...requestOrPath,
path: `${PUBLIC_PATH_PREFIX}${requestOrPath.path}`
});
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/plugin/ui/route.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { JsonObject, JsonValue } from "../../common/json";
import { type MessageResponder } from "../../common/messaging";
import { PUBLIC_PATH_PREFIX, type MessageResponder } from "../../common/messaging";
import type { SingletonAction } from "../actions/singleton-action";
import type { MessageHandler, MessageRequest } from "./message";
import { router } from "./router";
Expand All @@ -14,7 +14,7 @@ export function route<TBody extends JsonValue = JsonValue, TSettings extends Jso
): (target: MessageHandler<TBody, TSettings>, context: ClassMethodDecoratorContext<SingletonAction>) => OptionalParameterMessageHandler<TBody, TSettings, TResult> | void {
return function (target: MessageHandler<TBody, TSettings>, context: ClassMethodDecoratorContext<SingletonAction>): void {
context.addInitializer(function () {
router.route(path, target.bind(this), {
router.route(`${PUBLIC_PATH_PREFIX}${path}`, target.bind(this), {
filter: (source) => source.manifestId === this.manifestId
});
});
Expand Down
10 changes: 5 additions & 5 deletions src/ui/__tests__/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ describe("plugin", () => {
payload: {
__type: "request",
id: mockUUID,
path: "/outbound/path-and-body",
path: "public:/outbound/path-and-body",
unidirectional: false,
body: {
name: "Elgato"
Expand Down Expand Up @@ -81,7 +81,7 @@ describe("plugin", () => {
payload: {
__type: "request",
id: mockUUID,
path: "/outbound/request",
path: "public:/outbound/request",
unidirectional: true,
body: {
name: "Elgato"
Expand Down Expand Up @@ -149,7 +149,7 @@ describe("plugin", () => {

// Assert.
expect(spyOnRoute).toHaveBeenCalledTimes(1);
expect(spyOnRoute).toHaveBeenCalledWith("/register", handler, options);
expect(spyOnRoute).toHaveBeenCalledWith("public:/register", handler, options);
});

/**
Expand All @@ -166,7 +166,7 @@ describe("plugin", () => {
payload: {
__type: "request",
id: "abc123",
path: "/receive",
path: "public:/receive",
unidirectional: false,
body: {
name: "Elgato"
Expand All @@ -189,7 +189,7 @@ describe("plugin", () => {
getSettings,
setSettings
},
path: "/receive",
path: "public:/receive",
unidirectional: false,
body: {
name: "Elgato"
Expand Down
10 changes: 7 additions & 3 deletions src/ui/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { IDisposable } from "../common/disposable";
import type { JsonObject, JsonValue } from "../common/json";
import {
MessageGateway,
PUBLIC_PATH_PREFIX,
type MessageRequestOptions,
type MessageResponder,
type MessageResponse,
Expand Down Expand Up @@ -80,9 +81,12 @@ class PluginController {
*/
public async fetch<T extends JsonValue = JsonValue>(requestOrPath: MessageRequestOptions | string, bodyOrUndefined?: JsonValue): Promise<MessageResponse<T>> {
if (typeof requestOrPath === "string") {
return router.fetch(requestOrPath, bodyOrUndefined);
return router.fetch(`${PUBLIC_PATH_PREFIX}${requestOrPath}`, bodyOrUndefined);
} else {
return router.fetch(requestOrPath);
return router.fetch({
...requestOrPath,
path: `${PUBLIC_PATH_PREFIX}${requestOrPath.path}`
});
}
}

Expand Down Expand Up @@ -130,7 +134,7 @@ class PluginController {
handler: MessageHandler<TBody, TSettings>,
options?: RouteConfiguration<Action>
): IDisposable {
return router.route(path, handler, options);
return router.route(`${PUBLIC_PATH_PREFIX}${path}`, handler, options);
}

/**
Expand Down

0 comments on commit 45c54af

Please sign in to comment.