Skip to content

Commit 3cc449f

Browse files
committed
Hold a request to what the page is willing to take apart
Three bounds were missing on the way in. An argument of a call was walked as deep as it happened to be, so the shape of an argument was the server's to choose and the page's to survive. The depth a request asked for was obeyed without being looked at, which left "negative", "not a number", and "one hundred thousand" all as ways past the bound meant to protect the page. And a batched message was carried out however long its list was, spending the page's one turn on an arbitrarily large number of paths. Arguments are now held to the same depth a read is, that depth is read as a suggestion with a ceiling of its own, and whatever a message asks beyond the bound is refused - with an answer, so the server is not left timing out on a client which is wide awake.
1 parent 32a8b04 commit 3cc449f

2 files changed

Lines changed: 178 additions & 9 deletions

File tree

‎src/js/packages/@reactpy/client/src/event-proxy.ts‎

Lines changed: 88 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,46 @@ export const LIVE_OBJECT_TTL_MS = 60_000;
5757
export const MAX_PROPERTIES = 100;
5858
/** How deep a single resolution may nest before references are used */
5959
export const DEFAULT_MAX_DEPTH = 10;
60+
/**
61+
* The deepest a request may ask the client to go, however it asks.
62+
*
63+
* ``maxDepth`` arrives from the server. A value which is negative, not a number at all,
64+
* or simply enormous would otherwise turn the bound which is there to protect the page
65+
* into the way past it, so it is read as a suggestion with a ceiling of its own.
66+
*/
67+
export const MAX_ALLOWED_DEPTH = 128;
68+
/**
69+
* How many requests one batched message may carry out.
70+
*
71+
* The server queues whatever it wants to know during a pass round its loop, which is a
72+
* bounded number of things. A message carrying more than that did not come from a server
73+
* which means it, and carrying it out would be the page's one turn spent on nothing else.
74+
*/
75+
export const MAX_REQUESTS_PER_MESSAGE = 256;
76+
77+
/** Read the depth a request asked for as a sane one, whatever it actually said */
78+
function allowedDepth(requested: unknown): number {
79+
if (typeof requested !== "number" || !Number.isFinite(requested))
80+
return DEFAULT_MAX_DEPTH;
81+
return Math.min(Math.max(Math.floor(requested), 0), MAX_ALLOWED_DEPTH);
82+
}
83+
84+
/**
85+
* Answer a request which is not going to be carried out
86+
*
87+
* Whatever wanted an answer gets one, because leaving it unanswered would have the
88+
* server sit there until its own timeout and count a client which is perfectly awake
89+
* against it.
90+
*/
91+
function refuse(request: EventProxyRequest, error: string): EventProxyResponse {
92+
return {
93+
type: EVENT_PROXY_RESPONSE_TYPE,
94+
requestId: typeof request.requestId === "string" ? request.requestId : "",
95+
ok: false,
96+
error,
97+
code: "access",
98+
};
99+
}
60100

61101
export type EventProxyRequest = {
62102
type: typeof EVENT_PROXY_REQUEST_TYPE;
@@ -484,11 +524,20 @@ export class EventProxyRegistry {
484524
}
485525

486526
const responses: EventProxyResponse[] = [];
487-
for (const request of requests) {
527+
for (let index = 0; index < requests.length; index++) {
528+
const request = requests[index];
488529
// Anything which is not a request at all cannot be answered meaningfully, so it
489530
// is passed over. Everything else is carried out; only the answering is optional.
490531
if (!request || typeof request !== "object") continue;
491-
const response = this.resolve(request);
532+
// Past the bound the page stops taking requests apart and says so, rather than
533+
// spending its one turn on an arbitrarily large list of them.
534+
const response =
535+
index >= MAX_REQUESTS_PER_MESSAGE
536+
? refuse(
537+
request,
538+
`a single message may not ask for more than ${MAX_REQUESTS_PER_MESSAGE} things at once`,
539+
)
540+
: this.resolve(request);
492541
if (request.reply !== false) responses.push(response);
493542
}
494543
if (responses.length > 0) {
@@ -528,11 +577,18 @@ export class EventProxyRegistry {
528577
entry.added = this.clock();
529578

530579
const path = Array.isArray(message.path) ? message.path.map(String) : [];
531-
const options: SerializeOptions = { maxDepth: message.maxDepth };
580+
const maxDepth = allowedDepth(message.maxDepth);
581+
const options: SerializeOptions = { maxDepth };
532582

533583
try {
534584
if (message.call) {
535-
const value = this.call(eventId, entry.value, path, message.args || []);
585+
const value = this.call(
586+
eventId,
587+
entry.value,
588+
path,
589+
message.args || [],
590+
maxDepth,
591+
);
536592
return reply(true, {
537593
value: serializeValue(value, eventId, path, options),
538594
});
@@ -558,7 +614,13 @@ export class EventProxyRegistry {
558614
}
559615
}
560616

561-
private call(eventId: string, root: any, path: string[], args: any[]): any {
617+
private call(
618+
eventId: string,
619+
root: any,
620+
path: string[],
621+
args: any[],
622+
maxDepth: number,
623+
): any {
562624
if (path.length === 0)
563625
throw new ProxyAccessError("the event object itself cannot be called");
564626
const { parent, key } = parentAndKey(root, path);
@@ -567,11 +629,26 @@ export class EventProxyRegistry {
567629
throw new ProxyAccessError(`${JSON.stringify(key)} is not a function`);
568630
return method.apply(
569631
parent,
570-
args.map((argument) => this.readArgument(argument)),
632+
args.map((argument) => this.readArgument(argument, maxDepth, 0)),
571633
);
572634
}
573635

574-
private readArgument(argument: unknown): unknown {
636+
/**
637+
* Turn one argument of a call into the value it stands for.
638+
*
639+
* The shape of an argument is the server's business, not the page's, so it is held to
640+
* the same bound as a read is: whatever nests deeper than the request itself asked for
641+
* is refused instead of walked into.
642+
*/
643+
private readArgument(
644+
argument: unknown,
645+
maxDepth: number,
646+
depth: number,
647+
): unknown {
648+
if (depth > maxDepth)
649+
throw new ProxyAccessError(
650+
`an argument was nested more deeply than ${maxDepth} levels`,
651+
);
575652
const ref = readReference(argument);
576653
if (ref) {
577654
const entry = this.live.get(ref.eventId);
@@ -583,11 +660,13 @@ export class EventProxyRegistry {
583660
return readPath(entry.value, ref.path);
584661
}
585662
if (Array.isArray(argument))
586-
return argument.map((item) => this.readArgument(item));
663+
return argument.map((item) =>
664+
this.readArgument(item, maxDepth, depth + 1),
665+
);
587666
if (argument && typeof argument === "object") {
588667
const value: Record<string, unknown> = {};
589668
for (const [key, item] of Object.entries(argument))
590-
value[key] = this.readArgument(item);
669+
value[key] = this.readArgument(item, maxDepth, depth + 1);
591670
return value;
592671
}
593672
return argument;

‎src/js/packages/@reactpy/client/tests/event-proxy.test.ts‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,9 @@ import {
1414
type EventProxyRequestBatch,
1515
type EventProxyResponse,
1616
LIVE_OBJECT_TTL_MS,
17+
MAX_ALLOWED_DEPTH,
1718
MAX_LIVE_OBJECTS,
19+
MAX_REQUESTS_PER_MESSAGE,
1820
ProxyAccessError,
1921
ProxyNotFoundError,
2022
collectProperties,
@@ -587,6 +589,94 @@ test("an argument reference to a forgotten object is an error", () => {
587589
expect(response.code).toBe("not-found");
588590
});
589591

592+
test("an argument nested past the depth the request asked for is refused", () => {
593+
// An argument is the server's shape rather than the page's. Reading is bounded; an
594+
// argument used to be walked however deep it happened to go.
595+
const { registry } = make_registry();
596+
const eventId = registry.register({
597+
remember(...args: unknown[]) {
598+
return args.length;
599+
},
600+
});
601+
602+
const response = registry.resolve(
603+
request({
604+
eventId,
605+
path: ["remember"],
606+
call: true,
607+
args: [[[[["far"]]]]],
608+
maxDepth: 2,
609+
}),
610+
);
611+
612+
expect(response.ok).toBe(false);
613+
expect(response.code).toBe("access");
614+
expect(response.error).toContain("nested more deeply than 2 levels");
615+
});
616+
617+
test("a request cannot ask the page to recurse further than it is willing to", () => {
618+
// ``maxDepth`` comes from the server, so it is a suggestion rather than an order: a
619+
// number which is negative, not a number, or simply enormous has to leave the page
620+
// standing rather than burying it in a stack it cannot come back out of.
621+
const { registry } = make_registry();
622+
const eventId = registry.register({ deep: nest(MAX_ALLOWED_DEPTH + 20) });
623+
624+
for (const asked of [Number.NaN, -1, 100000, "deep"]) {
625+
const response = registry.resolve(
626+
request({
627+
eventId,
628+
path: ["deep"],
629+
maxDepth: asked as unknown as number,
630+
}),
631+
);
632+
expect(response.ok).toBe(true);
633+
634+
let node: any = response.value;
635+
let depth = 0;
636+
while (node && node[EVENT_REFERENCE_KEY] === undefined && node["next"]) {
637+
node = node["next"];
638+
depth++;
639+
}
640+
expect(depth).toBeLessThanOrEqual(MAX_ALLOWED_DEPTH);
641+
expect(readReference(node)).not.toBe(null);
642+
}
643+
});
644+
645+
test("a message which asks for far too much at once is refused the excess", () => {
646+
// The server asks for what it wants to know during one pass round its loop, which is
647+
// a bounded number of things. Carrying out an arbitrarily long list would spend the
648+
// page's one turn on nothing else, and whatever was refused still gets an answer
649+
// rather than leaving the server to time out on a client which is wide awake.
650+
const { registry, replies } = make_registry();
651+
const eventId = registry.register(make_event());
652+
const requests = Array.from(
653+
{ length: MAX_REQUESTS_PER_MESSAGE + 3 },
654+
(_, i) => request({ requestId: `r${i}`, eventId, path: ["type"] }),
655+
);
656+
657+
registry.handleMessage(batch(...requests));
658+
659+
const answered = replies[0].responses as EventProxyResponse[];
660+
expect(answered).toHaveLength(MAX_REQUESTS_PER_MESSAGE + 3);
661+
expect(answered[MAX_REQUESTS_PER_MESSAGE - 1].ok).toBe(true);
662+
expect(answered[MAX_REQUESTS_PER_MESSAGE].ok).toBe(false);
663+
expect(answered[MAX_REQUESTS_PER_MESSAGE].code).toBe("access");
664+
expect(answered[MAX_REQUESTS_PER_MESSAGE].error).toContain("more than");
665+
expect(answered[MAX_REQUESTS_PER_MESSAGE].requestId).toBe(
666+
`r${MAX_REQUESTS_PER_MESSAGE}`,
667+
);
668+
});
669+
670+
function nest(levels: number): Record<string, unknown> {
671+
const root: Record<string, unknown> = {};
672+
let node = root;
673+
for (let index = 0; index < levels; index++) {
674+
node["next"] = {};
675+
node = node["next"] as Record<string, unknown>;
676+
}
677+
return root;
678+
}
679+
590680
test("handleMessage only answers proxy requests, and only when asked", () => {
591681
const { registry, replies } = make_registry();
592682
const eventId = registry.register(make_event());

0 commit comments

Comments
 (0)