mirror of
https://github.com/HeyPuter/puter.git
synced 2026-10-24 23:58:47 +00:00
fix: charge stat's return_shares against the share-listing budget
`return_shares` on /fs/stat and the legacy /stat runs the same work as GET /share/shares, but was only metered under fs:stat's far more generous limit — and the two scopes stacked instead of sharing one counter. Adds consumeRouteRateLimit(req, spec), an imperative charge that resolves the key and per-subscription limit exactly as rateLimitGate does, so a handler can conditionally spend a second scope when a request flag makes the route expensive. Both stat handlers now charge share:list before doing the listing work; SHARE_LIST_LIMIT moves to a shared share/limits.ts so all callers pin the same spec. Closes PUT-1597.
This commit is contained in:
1 parent
50d3794283
commit
9d311c305f
10 files changed
+253
-36
No files matched your search
@@ -23,9 +23,11 @@ import { v4 as uuidv4 } from 'uuid';
|
||||
import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest';
|
||||
import { makeActor, type Actor } from '../../core/actor.js';
|
||||
import { runWithContext } from '../../core/context.js';
|
||||
import { consumeRouteRateLimit } from '../../core/http/middleware/rateLimit.js';
|
||||
import { PuterServer } from '../../server.js';
|
||||
import { setupTestServer } from '../../testUtil.js';
|
||||
import { generateDefaultFsentries } from '../../util/userProvisioning.js';
|
||||
import { SHARE_LIST_LIMIT } from '../share/limits.js';
|
||||
import type { FSController } from './FSController.js';
|
||||
import type {
|
||||
ClientSignedWriteResponse,
|
||||
@@ -798,6 +800,40 @@ describe('FSController.statEntry', () => {
|
||||
statusCode: 401,
|
||||
});
|
||||
});
|
||||
|
||||
it('draws return_shares from the share-listing budget', async () => {
|
||||
const { actor } = await makeUser();
|
||||
const username = actor.user!.username!;
|
||||
const path = `/${username}/Documents/share-budget`;
|
||||
await withActor(actor, () =>
|
||||
controller.mkdirEntry(makeReq({ body: { path }, actor }), makeRes().res),
|
||||
);
|
||||
|
||||
// Spend the whole share:list bucket, as /share/shares' gate would.
|
||||
const chargeReq = makeReq({ body: {}, actor });
|
||||
for (let i = 0; i < SHARE_LIST_LIMIT.limit; i++) {
|
||||
await consumeRouteRateLimit(chargeReq, SHARE_LIST_LIMIT);
|
||||
}
|
||||
|
||||
await expect(
|
||||
withActor(actor, () =>
|
||||
controller.statEntry(
|
||||
makeReq({ body: { path, return_shares: true }, actor }),
|
||||
makeRes().res,
|
||||
),
|
||||
),
|
||||
).rejects.toMatchObject({
|
||||
statusCode: 429,
|
||||
legacyCode: 'too_many_requests',
|
||||
});
|
||||
|
||||
// A plain stat spends only its own budget, so it still admits.
|
||||
const { res, captured } = makeRes();
|
||||
await withActor(actor, () =>
|
||||
controller.statEntry(makeReq({ body: { path }, actor }), res),
|
||||
);
|
||||
expect((captured.body as { name: string }).name).toBe('share-budget');
|
||||
});
|
||||
});
|
||||
|
||||
// ── /readdir (readdirEntries) ───────────────────────────────────────
|
||||
|
||||
@@ -47,6 +47,8 @@ import {
|
||||
} from '../../util/concurrency.js';
|
||||
import { applyInlineContentSecurity } from '../../util/inlineContentSecurity.js';
|
||||
import { listClientShares } from '../share/clientShare.js';
|
||||
import { SHARE_LIST_LIMIT } from '../share/limits.js';
|
||||
import { consumeRouteRateLimit } from '../../core/http/middleware/rateLimit.js';
|
||||
import { PuterController } from '../types.js';
|
||||
import { STORAGE_OP_COSTS } from '../../services/metering/costs.js';
|
||||
import {
|
||||
@@ -991,6 +993,17 @@ export class FSController extends PuterController {
|
||||
await this.#assertAccess(actor, entry.path, 'see');
|
||||
|
||||
const wantsSize = this.#toBoolean(body.return_size);
|
||||
const wantsShares = this.#toBoolean(body.return_shares);
|
||||
// `return_shares` is the share-listing route's work, so it spends from
|
||||
// the share-listing budget too, not just `fs:stat`'s.
|
||||
if (
|
||||
wantsShares &&
|
||||
!(await consumeRouteRateLimit(req, SHARE_LIST_LIMIT))
|
||||
) {
|
||||
throw new HttpError(429, 'Too many requests.', {
|
||||
legacyCode: 'too_many_requests',
|
||||
});
|
||||
}
|
||||
const [subtreeSize, suggestedApps, shareFlags, shares] =
|
||||
await Promise.all([
|
||||
entry.isDir && wantsSize
|
||||
@@ -998,7 +1011,7 @@ export class FSController extends PuterController {
|
||||
: undefined,
|
||||
this.services.suggestedApps.getSuggestedApps(entry),
|
||||
this.services.share.shareFlags(actor, [entry]),
|
||||
this.#toBoolean(body.return_shares)
|
||||
wantsShares
|
||||
? listClientShares(
|
||||
this.services.share,
|
||||
this.clients.event,
|
||||
@@ -2234,8 +2247,7 @@ export class FSController extends PuterController {
|
||||
// the ActorUser type. Access via the escape hatch until a proper
|
||||
// storage-quota mechanism is in place.
|
||||
const actorUser = req.actor?.user as
|
||||
| Record<string, unknown>
|
||||
| undefined;
|
||||
Record<string, unknown> | undefined;
|
||||
|
||||
const candidates = [
|
||||
this.#toStorageCapacityCandidate(actorUser?.free_storage),
|
||||
|
||||
@@ -23,11 +23,13 @@ import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest';
|
||||
import { v4 as uuidv4 } from 'uuid';
|
||||
import { makeActor, type Actor } from '../../core/actor.js';
|
||||
import { runWithContext } from '../../core/context.js';
|
||||
import { consumeRouteRateLimit } from '../../core/http/middleware/rateLimit.js';
|
||||
import { PuterRouter } from '../../core/http/PuterRouter.js';
|
||||
import { PuterServer } from '../../server.js';
|
||||
import { setupTestServer } from '../../testUtil.js';
|
||||
import { signFile } from '../../util/fileSigning.js';
|
||||
import { generateDefaultFsentries } from '../../util/userProvisioning.js';
|
||||
import { SHARE_LIST_LIMIT } from '../share/limits.js';
|
||||
import type { LegacyFSController } from './LegacyFSController.js';
|
||||
|
||||
// ── Test harness ────────────────────────────────────────────────────
|
||||
@@ -426,6 +428,40 @@ describe('LegacyFSController.stat', () => {
|
||||
statusCode: 401,
|
||||
});
|
||||
});
|
||||
|
||||
it('draws return_shares from the share-listing budget', async () => {
|
||||
const { actor } = await makeUser();
|
||||
const username = actor.user!.username!;
|
||||
const path = `/${username}/Documents/share-budget`;
|
||||
await withActor(actor, () =>
|
||||
controller.mkdir(makeReq({ body: { path }, actor }), makeRes().res),
|
||||
);
|
||||
|
||||
// Spend the whole share:list bucket, as /share/shares' gate would.
|
||||
const chargeReq = makeReq({ body: {}, actor });
|
||||
for (let i = 0; i < SHARE_LIST_LIMIT.limit; i++) {
|
||||
await consumeRouteRateLimit(chargeReq, SHARE_LIST_LIMIT);
|
||||
}
|
||||
|
||||
await expect(
|
||||
withActor(actor, () =>
|
||||
controller.stat(
|
||||
makeReq({ body: { path, return_shares: true }, actor }),
|
||||
makeRes().res,
|
||||
),
|
||||
),
|
||||
).rejects.toMatchObject({
|
||||
statusCode: 429,
|
||||
legacyCode: 'too_many_requests',
|
||||
});
|
||||
|
||||
// A plain stat spends only its own budget, so it still admits.
|
||||
const plain = makeRes();
|
||||
await withActor(actor, () =>
|
||||
controller.stat(makeReq({ body: { path }, actor }), plain.res),
|
||||
);
|
||||
expect(plain.captured.body).toMatchObject({ name: 'share-budget' });
|
||||
});
|
||||
});
|
||||
|
||||
describe('LegacyFSController.delete', () => {
|
||||
|
||||
@@ -50,6 +50,8 @@ import {
|
||||
} from '../../util/hostedAppBacking.js';
|
||||
import { applyInlineContentSecurity } from '../../util/inlineContentSecurity.js';
|
||||
import { listClientShares } from '../share/clientShare.js';
|
||||
import { SHARE_LIST_LIMIT } from '../share/limits.js';
|
||||
import { consumeRouteRateLimit } from '../../core/http/middleware/rateLimit.js';
|
||||
import { PuterController } from '../types.js';
|
||||
import {
|
||||
FS_BATCH_CONCURRENT,
|
||||
@@ -487,6 +489,13 @@ export class LegacyFSController extends PuterController {
|
||||
// callers working. `return_owner` is a no-op — `owner` is always set.
|
||||
if (getBoolean(body, 'return_versions')) shaped.versions = [];
|
||||
if (getBoolean(body, 'return_shares')) {
|
||||
// Share-listing work spends the share-listing budget, not just
|
||||
// `fs:stat`'s.
|
||||
if (!(await consumeRouteRateLimit(req, SHARE_LIST_LIMIT))) {
|
||||
throw new HttpError(429, 'Too many requests.', {
|
||||
legacyCode: 'too_many_requests',
|
||||
});
|
||||
}
|
||||
shaped.shares = await listClientShares(
|
||||
this.services.share,
|
||||
this.clients.event,
|
||||
@@ -859,9 +868,7 @@ export class LegacyFSController extends PuterController {
|
||||
// Trash, and `null`/`{}` when restoring. See
|
||||
// `src/gui/src/helpers.js` → `window.move_items`.
|
||||
newMetadata: (body.new_metadata ?? undefined) as
|
||||
| Record<string, unknown>
|
||||
| null
|
||||
| undefined,
|
||||
Record<string, unknown> | null | undefined,
|
||||
});
|
||||
const oldPath = source.path;
|
||||
await this.#emitGuiEvent('outer.gui.item.moved', moved, {
|
||||
@@ -1308,8 +1315,7 @@ export class LegacyFSController extends PuterController {
|
||||
}
|
||||
|
||||
type SignedOrEmpty =
|
||||
| (SignedFile & { path?: string })
|
||||
| Record<string, never>;
|
||||
(SignedFile & { path?: string }) | Record<string, never>;
|
||||
const result: { signatures: SignedOrEmpty[]; token?: string } = {
|
||||
signatures: [],
|
||||
};
|
||||
@@ -1930,10 +1936,7 @@ export class LegacyFSController extends PuterController {
|
||||
const subjectRef = body.subject;
|
||||
const appRef = body.app;
|
||||
const mode = (getString(body, 'mode') ?? 'read') as
|
||||
| 'see'
|
||||
| 'list'
|
||||
| 'read'
|
||||
| 'write';
|
||||
'see' | 'list' | 'read' | 'write';
|
||||
if (!subjectRef || !appRef)
|
||||
throw new HttpError(400, '`subject` and `app` are required', {
|
||||
legacyCode: 'bad_request',
|
||||
|
||||
@@ -31,28 +31,7 @@ import { toClientShare } from './clientShare.js';
|
||||
import { runWithConcurrencyLimitSettled } from '../../util/concurrency.js';
|
||||
import { normalizeLimit } from '../../util/pagination.js';
|
||||
import { PuterController } from '../types.js';
|
||||
|
||||
/**
|
||||
* Two windows: a burst ceiling, and a daily one so a slow drip can't add up to
|
||||
* a mail-merge. Neither bounds _shares_ — one request carries many — which is
|
||||
* what `ShareService`'s per-day quota is for.
|
||||
*/
|
||||
const SHARE_LIMIT = [
|
||||
{ scope: 'share:mutate', limit: 60, window: 60_000, key: 'user' as const },
|
||||
{
|
||||
scope: 'share:mutate-daily',
|
||||
limit: 500,
|
||||
window: 24 * 60 * 60_000,
|
||||
key: 'user' as const,
|
||||
},
|
||||
];
|
||||
|
||||
const SHARE_LIST_LIMIT = {
|
||||
scope: 'share:list',
|
||||
limit: 600,
|
||||
window: 60_000,
|
||||
key: 'user' as const,
|
||||
};
|
||||
import { SHARE_LIMIT, SHARE_LIST_LIMIT } from './limits.js';
|
||||
|
||||
/** Distinct (holder, item) pairs run together; see the note on grouping below. */
|
||||
const SHARE_CONCURRENCY = 8;
|
||||
|
||||
@@ -0,0 +1,51 @@
|
||||
/*
|
||||
* Copyright (C) 2024-present Puter Technologies Inc.
|
||||
*
|
||||
* This file is part of Puter.
|
||||
*
|
||||
* Puter is free software: you can redistribute it and/or modify
|
||||
* it under the terms of the GNU Affero General Public License as published
|
||||
* by the Free Software Foundation, either version 3 of the License, or
|
||||
* (at your option) any later version.
|
||||
*
|
||||
* This program is distributed in the hope that it will be useful,
|
||||
* but WITHOUT ANY WARRANTY; without even the implied warranty of
|
||||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
|
||||
* GNU Affero General Public License for more details.
|
||||
*
|
||||
* You should have received a copy of the GNU Affero General Public License
|
||||
* along with this program. If not, see <https://www.gnu.org/licenses/>.
|
||||
*/
|
||||
|
||||
import type { RouteRateLimit } from '../../core/http/types';
|
||||
|
||||
// Shared like `fs/limits.ts` is shared: the scope only ties counters
|
||||
// together if every caller passes the same spec.
|
||||
|
||||
/**
|
||||
* Two windows: a burst ceiling, and a daily one so a slow drip can't add up to
|
||||
* a mail-merge. Neither bounds _shares_ — one request carries many — which is
|
||||
* what `ShareService`'s per-day quota is for.
|
||||
*/
|
||||
export const SHARE_LIMIT: RouteRateLimit[] = [
|
||||
{ scope: 'share:mutate', limit: 60, window: 60_000, key: 'user' },
|
||||
{
|
||||
scope: 'share:mutate-daily',
|
||||
limit: 500,
|
||||
window: 24 * 60 * 60_000,
|
||||
key: 'user',
|
||||
},
|
||||
];
|
||||
|
||||
/**
|
||||
* One bucket for every share-listing read. The `/share` routes gate on it, and
|
||||
* `stat` charges it imperatively when `return_shares` asks for the same listing
|
||||
* work — otherwise the flag runs share listings under `fs:stat`'s far more
|
||||
* generous budget.
|
||||
*/
|
||||
export const SHARE_LIST_LIMIT: RouteRateLimit = {
|
||||
scope: 'share:list',
|
||||
limit: 600,
|
||||
window: 60_000,
|
||||
key: 'user',
|
||||
};
|
||||
@@ -709,6 +709,34 @@ export async function checkRateLimit(key, limit, windowMs, backend) {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Charge a route-shaped rate-limit spec from inside a handler.
|
||||
*
|
||||
* `rateLimitGate` takes one static spec per route, which is wrong for a route
|
||||
* whose cost depends on a request parameter: `stat` with `return_shares` does
|
||||
* the same work as the share-listing route, and should spend from the same
|
||||
* budget. This resolves the key and the per-subscription limit exactly as the
|
||||
* gate does — same spec in, same bucket out — so a handler can charge a second
|
||||
* scope conditionally. Returns true if allowed; fails open on backend error.
|
||||
*/
|
||||
export async function consumeRouteRateLimit(req, spec) {
|
||||
const {
|
||||
window: windowMs,
|
||||
key: strategy = 'fingerprint',
|
||||
scope,
|
||||
backend,
|
||||
} = spec;
|
||||
const backendPair = resolveBackend(backend);
|
||||
const key = resolveKey(req, scope ?? req.route?.path ?? 'route', strategy);
|
||||
try {
|
||||
const limit = await resolveSubscriptionLimit(req, spec);
|
||||
return await backendPair.rate(key, limit, windowMs);
|
||||
} catch (err) {
|
||||
console.error('[rate-limit] handler charge failed, failing open:', err);
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Read whether `key` still has budget, without spending any. The twin to
|
||||
* `checkRateLimit` for gates whose budget is consumed by an outcome rather than
|
||||
|
||||
@@ -38,6 +38,7 @@ import {
|
||||
checkDriverRateLimit,
|
||||
checkRateLimit,
|
||||
concurrencyGate,
|
||||
consumeRouteRateLimit,
|
||||
configureRateLimit,
|
||||
listConfiguredRateLimitBackends,
|
||||
peekRateLimit,
|
||||
@@ -1427,6 +1428,77 @@ describe('checkRateLimit', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('consumeRouteRateLimit', () => {
|
||||
beforeEach(() => configureRateLimit());
|
||||
afterEach(() => configureRateLimit());
|
||||
|
||||
const uniqueScope = () => `charge-${Math.random().toString(36).slice(2, 10)}`;
|
||||
|
||||
it('spends from the same bucket the route gate does', async () => {
|
||||
// The whole point of the helper: a handler charge and the gate must
|
||||
// resolve the identical key for the same spec + request.
|
||||
const opts = { limit: 2, window: 60_000, key: 'user', scope: uniqueScope() };
|
||||
const req = { actor: { user: { id: 7 } }, headers: {} };
|
||||
|
||||
expect(await consumeRouteRateLimit(req, opts)).toBe(true);
|
||||
|
||||
const next = vi.fn();
|
||||
await rateLimitGate(opts)(req, {}, next);
|
||||
expect(next.mock.calls[0][0]).toBeUndefined();
|
||||
|
||||
// Gate + helper together exhausted the bucket; either side now rejects.
|
||||
expect(await consumeRouteRateLimit(req, opts)).toBe(false);
|
||||
const rejectedNext = vi.fn();
|
||||
await rateLimitGate(opts)(req, {}, rejectedNext);
|
||||
expect(isHttpError(rejectedNext.mock.calls[0][0])).toBe(true);
|
||||
});
|
||||
|
||||
it("buckets per user under the 'user' strategy", async () => {
|
||||
const opts = { limit: 1, window: 60_000, key: 'user', scope: uniqueScope() };
|
||||
const reqFor = (id) => ({ actor: { user: { id } }, headers: {} });
|
||||
expect(await consumeRouteRateLimit(reqFor(1), opts)).toBe(true);
|
||||
expect(await consumeRouteRateLimit(reqFor(1), opts)).toBe(false);
|
||||
expect(await consumeRouteRateLimit(reqFor(2), opts)).toBe(true);
|
||||
});
|
||||
|
||||
it('applies bySubscription overrides via the wired metering service', async () => {
|
||||
configureRateLimit({
|
||||
metering: {
|
||||
getActorSubscription: async (actor) => ({
|
||||
id: actor.user.uuid === 'free' ? 'user_free' : 'other',
|
||||
}),
|
||||
},
|
||||
});
|
||||
const opts = {
|
||||
limit: 5,
|
||||
window: 60_000,
|
||||
bySubscription: { user_free: 1 },
|
||||
key: 'user',
|
||||
scope: uniqueScope(),
|
||||
};
|
||||
const freeReq = { actor: { user: { id: 1, uuid: 'free' } }, headers: {} };
|
||||
expect(await consumeRouteRateLimit(freeReq, opts)).toBe(true);
|
||||
expect(await consumeRouteRateLimit(freeReq, opts)).toBe(false);
|
||||
});
|
||||
|
||||
it('fails open when the backend throws', async () => {
|
||||
configureRateLimit({
|
||||
default: 'redis',
|
||||
redis: {
|
||||
multi: () => {
|
||||
throw new Error('redis exploded');
|
||||
},
|
||||
},
|
||||
});
|
||||
const spy = vi.spyOn(console, 'error').mockImplementation(() => {});
|
||||
const opts = { limit: 1, window: 60_000, key: 'ip', scope: uniqueScope() };
|
||||
const req = { ip: '5.5.5.5', headers: {} };
|
||||
expect(await consumeRouteRateLimit(req, opts)).toBe(true);
|
||||
expect(spy).toHaveBeenCalled();
|
||||
spy.mockRestore();
|
||||
});
|
||||
});
|
||||
|
||||
describe('peekRateLimit', () => {
|
||||
beforeEach(() => configureRateLimit());
|
||||
afterEach(() => configureRateLimit());
|
||||
|
||||
@@ -39,7 +39,7 @@ A `Promise` that resolves to the [`FSItem`](/Objects/fsitem) object of the speci
|
||||
|
||||
The item carries `is_shared`: `true` when it has been shared with someone, `false` when it has not, and `null` when the item is not yours — whether someone else's file has other recipients is not yours to see. It covers shares granted by anyone holding `manage` on the item, not only your own, the same way [`getShares()`](/FS/getShares/) does. Only shares **on the item itself** count. A file inside a folder you shared is reachable through that folder without being shared itself, so it reports `false`; `getShares()` is what reports inherited access.
|
||||
|
||||
With `returnShares: true`, the result also carries `shares` — an array of the same share objects [`getShares()`](/FS/getShares/) returns, including access inherited from a parent folder and unclaimed invitations. It is empty unless you own the item or hold `manage` on it, so asking for it never fails a `stat()` you were otherwise allowed to make.
|
||||
With `returnShares: true`, the result also carries `shares` — an array of the same share objects [`getShares()`](/FS/getShares/) returns, including access inherited from a parent folder and unclaimed invitations. It is empty unless you own the item or hold `manage` on it, so asking for it never fails a `stat()` you were otherwise allowed to make. Because it does `getShares()`'s work, it also spends from the [share-read limit](/rate-limits-and-quotas/#sharing) on top of `stat()`'s own.
|
||||
|
||||
## Examples
|
||||
|
||||
|
||||
@@ -166,7 +166,7 @@ Sharing is bounded twice: on the calls, and on how many people one account can r
|
||||
| Recipients per request | 10 |
|
||||
| Items per request | 50 |
|
||||
|
||||
The read limit is one bucket shared by every share-listing call, so polling one of them spends budget the others need.
|
||||
The read limit is one bucket shared by every share-listing call, so polling one of them spends budget the others need. `stat()` with `returnShares` does the same listing work, so it spends from this bucket too, on top of its own `stat` budget.
|
||||
|
||||
A "new share" is one that gives someone access they didn't already have. Changing the mode on an existing share, or re-sharing an item the recipient already has, costs nothing. Over the daily limit, `share` fails with `share_daily_limit_reached`.
|
||||
|
||||
|
||||
Reference in new issue
Block a user