mirror of
https://github.com/yusufipk/OpenFrame.git
synced 2026-09-11 09:36:08 +00:00
fix: close the findings the test suite surfaced
The suite that landed in #43/#44 was written against existing behaviour, so a number of tests pinned bugs rather than asserting correct behaviour. This fixes the production code and moves each of those tests onto the fixed behaviour in the same change. Security: - project-download: derive the archive entry extension from the last path segment and restrict it to a short alphanumeric run, so an extensionless allowlisted url can no longer contribute a path separator; validate the r2 branch against the strict proxy-path pattern instead of a `startsWith`, which let `/api/upload/video/clip.mp4/../../etc/passwd` through verbatim. - rate-limit: hash a key or action wider than its column instead of skipping the query. Both the guard and the failing INSERT used to answer "allowed", so the limit stopped applying entirely. Warn at startup when TRUSTED_PROXY_MODE is unset in production. - video uploads: the file name decides the content type; a client-declared video mime no longer makes `payload.exe` acceptable. - email templates: escape in the helpers rather than relying on every caller, with an explicit `rawEmailHtml()` opt-out for the one call site that builds markup. `escapeHtml` now covers the single quote. - CSP: allow loopback object storage outside production only. - route-access: reach the billing redirect only for the workspace owner. Keying it off the owner's billing status alone made the redirect target an oracle for whose subscription had lapsed, and sent members to a page they cannot act on. - search: carry the same billing condition every other read path carries. - logger: check `err.name` as well as `err.constructor.name`, so a re-thrown, deserialised or minified Prisma error is still redacted. - upload tokens: resolve the signing secret outside the try, so a server booted without one fails loudly instead of reporting every grant as a forgery. - invitations: never downgrade an existing membership, and report a scoped invitation that points at nothing as not_found rather than accepted. - auth: resolve the workspace role for every signed-in caller, so checkProjectAccess and computeProjectAccess stop disagreeing about the owner who also owns the workspace. The `intent` option is gone with it. - r2-media-proxy: validate the object key inside the proxy so the guard travels with the function; delete the unused, unanchored `mediaUrlToR2Key`. - r2: sign the content type into presigned PUT grants. Correctness: - frame rate snapping picks the nearest standard, not the first within tolerance, so 24, 30 and 60 fps are reachable at all. - a version upload registers its Bunny cleanup as soon as bunny-init answers, so a failed tus upload no longer leaves a billed video behind. - deleting videos clears storage before the rows, so a refused DELETE leaves a retryable row rather than an orphaned object. - an expired upload session can be cancelled, which is what releases its quota. - `voice/` joins the delete allowlist, so a voice note can be removed by the module that wrote it. - a failed CORS write propagates instead of being mistaken for an empty config and replacing the bucket's rules. - filtering projects by workspace no longer hides projects the unfiltered call returns. - upload retries skip aborts and permanent 4xx; progress no longer divides by zero. - reply edits no longer clear the comment's tag; optimistic resolve rolls back to the state it replaced; the delete snapshot is captured once. - assorted UI fixes: duplicate React keys, double-click guards reading stale closures, the tag list fetched twice per load, a failed member list rendering as an empty one, a stale "Initializing upload..." beside a failure, and a registration banner pointing at an email that never arrives. Consistency and access: - the two download routes answer 404 for an id belonging to another tenant, as the comment export route already did. A caller who does belong still gets 403. - accessible names for the share-link password field, the guest name gates, the version dialog inputs and the comment-tag controls. Repository health: - the runner image installs production dependencies only. - a setup file for the unit project restores stubbed env centrally. - native tsconfig path resolution replaces vite-tsconfig-paths. - `uploadBytesWithProgress` exists once. - admin stats bill Bunny storage to the workspace owner like every other quota, gate on the configured flag, wire up the single-flight guard and count the statuses that belonged to no bucket. - `r2Client.destroy()` releases the presign client too. - `prepare` tolerates a production install, where husky is absent.
This commit is contained in:
yusufipk
@@ -442,7 +442,10 @@ describe('DELETE /api/videos/[videoId]/assets/[assetId]', () => {
|
||||
// owner has turned exports off. Both are pinned, because merging them would look
|
||||
// like a tidy-up and would quietly hand the files to every viewer.
|
||||
describe('GET /api/videos/[videoId]/assets/[assetId]/download', () => {
|
||||
it('returns 403 to a signed-in stranger', async () => {
|
||||
// 404 rather than 403 for a caller with no relationship to the project: a 403 confirms
|
||||
// the id exists. The comment export route has always answered 404 for the identical
|
||||
// shape, and the three download paths now agree.
|
||||
it('returns 404 to a signed-in stranger', async () => {
|
||||
const fixture = await seedAsset({ allowDownloads: true });
|
||||
await seedProject();
|
||||
const stranger = await createUser();
|
||||
@@ -454,8 +457,7 @@ describe('GET /api/videos/[videoId]/assets/[assetId]/download', () => {
|
||||
{ videoId: fixture.video.id, assetId: fixture.asset.id }
|
||||
);
|
||||
|
||||
expect(response.status).toBe(403);
|
||||
expect(await readError(response)).toContain('Access denied');
|
||||
expect(response.status).toBe(404);
|
||||
});
|
||||
|
||||
it('returns 403 to a project COMMENTATOR when downloads are disabled', async () => {
|
||||
@@ -529,7 +531,7 @@ describe('GET /api/videos/[videoId]/assets/[assetId]/download', () => {
|
||||
expect(response.status).toBe(404);
|
||||
});
|
||||
|
||||
it('returns 403 for a foreign asset reached through its own foreign video id', async () => {
|
||||
it('returns 404 for a foreign asset reached through its own foreign video id', async () => {
|
||||
const mine = await seedAsset({ allowDownloads: true });
|
||||
const theirs = await seedAsset({ allowDownloads: true });
|
||||
signedInAs(mine.owner);
|
||||
@@ -540,7 +542,7 @@ describe('GET /api/videos/[videoId]/assets/[assetId]/download', () => {
|
||||
{ videoId: theirs.video.id, assetId: theirs.asset.id }
|
||||
);
|
||||
|
||||
expect(response.status).toBe(403);
|
||||
expect(response.status).toBe(404);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -765,7 +765,21 @@ const NON_AUTHORIZATION_REFUSALS = new Map<string, string>();
|
||||
* fails and tells you to delete it, so nothing can rot into a permanent
|
||||
* exemption.
|
||||
*/
|
||||
const NOT_FOUND_IS_THE_GUARD = new Map<string, string>();
|
||||
const NOT_FOUND_IS_THE_GUARD = new Map<string, string>([
|
||||
// Both download routes take a bare resource id with no project in the path, so a 403
|
||||
// for an id belonging to another tenant would confirm that the id exists. They answer
|
||||
// 404 to any caller with no relationship to the project, which is what
|
||||
// versions/[versionId]/comments/export has always done for the identical shape.
|
||||
// Somebody who does belong, an owner whose billing lapsed for instance, still gets 403.
|
||||
[
|
||||
'GET versions/[versionId]/download/route.ts',
|
||||
'hides whether the version id exists from a caller with no relationship to it',
|
||||
],
|
||||
[
|
||||
'GET videos/[videoId]/assets/[assetId]/download/route.ts',
|
||||
'hides whether the video id exists from a caller with no relationship to it',
|
||||
],
|
||||
]);
|
||||
|
||||
function discoverRouteModules(): string[] {
|
||||
const apiDir = path.join(REPO_ROOT, 'app', 'api');
|
||||
|
||||
@@ -332,7 +332,10 @@ describe('GET /api/projects/[projectId]/download', () => {
|
||||
// DownloadEgressEvent row was written, because that row is the billing record: a
|
||||
// refusal that still bills the workspace owner would be its own bug.
|
||||
describe('GET /api/versions/[versionId]/download', () => {
|
||||
it('returns 403 to a signed-in stranger and records no egress', async () => {
|
||||
// 404 rather than 403 for a caller with no relationship to the project: a 403 would
|
||||
// confirm the id exists. The comment export route has always answered 404 for the
|
||||
// identical shape, and the three download paths now agree.
|
||||
it('returns 404 to a signed-in stranger and records no egress', async () => {
|
||||
const fixture = await seedDownloadable({ allowDownloads: true });
|
||||
await seedProject();
|
||||
const stranger = await createUser();
|
||||
@@ -344,7 +347,7 @@ describe('GET /api/versions/[versionId]/download', () => {
|
||||
{ versionId: fixture.version.id }
|
||||
);
|
||||
|
||||
expect(response.status).toBe(403);
|
||||
expect(response.status).toBe(404);
|
||||
expect(await db.downloadEgressEvent.count()).toBe(0);
|
||||
});
|
||||
|
||||
@@ -429,9 +432,9 @@ describe('GET /api/versions/[versionId]/download', () => {
|
||||
|
||||
// Straight identifier substitution. There is no projectId in this URL to
|
||||
// cross-check against, so the version id alone decides which project gets
|
||||
// authorized. A caller who owns a perfectly good project of their own gets 403
|
||||
// authorized. A caller who owns a perfectly good project of their own gets 404
|
||||
// for somebody else's version, and never learns whether it exists.
|
||||
it('returns 403 for a version id belonging to another workspace', async () => {
|
||||
it('returns 404 for a version id belonging to another workspace', async () => {
|
||||
const mine = await seedDownloadable({ allowDownloads: true });
|
||||
const theirs = await seedDownloadable({ allowDownloads: true });
|
||||
signedInAs(mine.owner);
|
||||
@@ -442,7 +445,7 @@ describe('GET /api/versions/[versionId]/download', () => {
|
||||
{ versionId: theirs.version.id }
|
||||
);
|
||||
|
||||
expect(response.status).toBe(403);
|
||||
expect(response.status).toBe(404);
|
||||
expect(await db.downloadEgressEvent.count()).toBe(0);
|
||||
});
|
||||
|
||||
|
||||
@@ -450,11 +450,10 @@ describe('acceptInvitationTokenForUser', () => {
|
||||
expect(membership.role).toBe('ADMIN');
|
||||
});
|
||||
|
||||
// The invited role wins over the role the member already holds, so accepting
|
||||
// a COMMENTATOR invitation demotes a sitting workspace ADMIN. Pinned because
|
||||
// it is a privilege change, and a surprising one: the accept link looks like
|
||||
// it can only ever add access.
|
||||
it('applies a COMMENTATOR invitation over an existing ADMIN membership', async () => {
|
||||
// An invitation can only ever add access. Applying the invited role over an existing
|
||||
// membership made it a privilege-change primitive: re-invite a sitting ADMIN at a lower
|
||||
// role, get them to click the link once, and they are demoted.
|
||||
it('leaves an existing ADMIN membership alone for a COMMENTATOR invitation', async () => {
|
||||
const scenario = await seedProject();
|
||||
const member = await createUser({ email: '[email protected]' });
|
||||
await addWorkspaceMember({
|
||||
@@ -480,7 +479,66 @@ describe('acceptInvitationTokenForUser', () => {
|
||||
const membership = await db.workspaceMember.findUniqueOrThrow({
|
||||
where: { workspaceId_userId: { workspaceId: scenario.workspace.id, userId: member.id } },
|
||||
});
|
||||
expect(membership.role).toBe('COMMENTATOR');
|
||||
expect(membership.role).toBe('ADMIN');
|
||||
});
|
||||
|
||||
it('leaves an existing project ADMIN alone for a COMMENTATOR invitation', async () => {
|
||||
const scenario = await seedProject();
|
||||
const member = await createUser({ email: '[email protected]' });
|
||||
await addProjectMember({
|
||||
projectId: scenario.project.id,
|
||||
userId: member.id,
|
||||
role: 'ADMIN',
|
||||
});
|
||||
const invitation = await createInvitation({
|
||||
invitedById: scenario.owner.id,
|
||||
email: '[email protected]',
|
||||
scope: 'PROJECT',
|
||||
projectId: scenario.project.id,
|
||||
role: 'COMMENTATOR',
|
||||
});
|
||||
|
||||
const result = await acceptInvitationTokenForUser({
|
||||
token: invitation.token,
|
||||
userId: member.id,
|
||||
email: member.email!,
|
||||
});
|
||||
|
||||
expect(result).toBe('accepted');
|
||||
const membership = await db.projectMember.findUniqueOrThrow({
|
||||
where: { projectId_userId: { projectId: scenario.project.id, userId: member.id } },
|
||||
});
|
||||
expect(membership.role).toBe('ADMIN');
|
||||
});
|
||||
|
||||
// The other direction still has to work: an invitation is allowed to promote.
|
||||
it('promotes an existing COMMENTATOR to ADMIN for an ADMIN invitation', async () => {
|
||||
const scenario = await seedProject();
|
||||
const member = await createUser({ email: '[email protected]' });
|
||||
await addWorkspaceMember({
|
||||
workspaceId: scenario.workspace.id,
|
||||
userId: member.id,
|
||||
role: 'COMMENTATOR',
|
||||
});
|
||||
const invitation = await createInvitation({
|
||||
invitedById: scenario.owner.id,
|
||||
email: '[email protected]',
|
||||
scope: 'WORKSPACE',
|
||||
workspaceId: scenario.workspace.id,
|
||||
role: 'ADMIN',
|
||||
});
|
||||
|
||||
const result = await acceptInvitationTokenForUser({
|
||||
token: invitation.token,
|
||||
userId: member.id,
|
||||
email: member.email!,
|
||||
});
|
||||
|
||||
expect(result).toBe('accepted');
|
||||
const membership = await db.workspaceMember.findUniqueOrThrow({
|
||||
where: { workspaceId_userId: { workspaceId: scenario.workspace.id, userId: member.id } },
|
||||
});
|
||||
expect(membership.role).toBe('ADMIN');
|
||||
});
|
||||
|
||||
// The owner already outranks any membership row. Writing one would put them
|
||||
@@ -532,11 +590,10 @@ describe('acceptInvitationTokenForUser', () => {
|
||||
);
|
||||
});
|
||||
|
||||
// Pins today's behaviour for a malformed row (scope WORKSPACE with no
|
||||
// workspaceId): the caller is told "accepted" while nothing is granted and
|
||||
// the invitation stays PENDING, so the accept page shows a success screen.
|
||||
// See the report accompanying this suite.
|
||||
it('reports accepted for a scoped invitation that points at nothing', async () => {
|
||||
// A malformed row (scope WORKSPACE with no workspaceId) grants nothing. Reporting
|
||||
// "accepted" for it showed the user a success screen for a no-op they had no way to
|
||||
// detect, while the invitation stayed PENDING for good.
|
||||
it('refuses a scoped invitation that points at nothing', async () => {
|
||||
const scenario = await seedProject();
|
||||
const invitee = await createUser({ email: '[email protected]' });
|
||||
const invitation = await createInvitation({
|
||||
@@ -552,12 +609,32 @@ describe('acceptInvitationTokenForUser', () => {
|
||||
email: invitee.email!,
|
||||
});
|
||||
|
||||
expect(result).toBe('accepted');
|
||||
expect(result).toBe('not_found');
|
||||
expect(await db.workspaceMember.count()).toBe(0);
|
||||
expect((await db.invitation.findUniqueOrThrow({ where: { id: invitation.id } })).status).toBe(
|
||||
'PENDING'
|
||||
);
|
||||
});
|
||||
|
||||
it('refuses a project invitation that points at nothing', async () => {
|
||||
const scenario = await seedProject();
|
||||
const invitee = await createUser({ email: '[email protected]' });
|
||||
const invitation = await createInvitation({
|
||||
invitedById: scenario.owner.id,
|
||||
email: '[email protected]',
|
||||
scope: 'PROJECT',
|
||||
projectId: null,
|
||||
});
|
||||
|
||||
const result = await acceptInvitationTokenForUser({
|
||||
token: invitation.token,
|
||||
userId: invitee.id,
|
||||
email: invitee.email!,
|
||||
});
|
||||
|
||||
expect(result).toBe('not_found');
|
||||
expect(await db.projectMember.count()).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('acceptPendingInvitationsForUser', () => {
|
||||
|
||||
@@ -437,16 +437,18 @@ describe('getCachedBunnyStorageStats', () => {
|
||||
expect(await getCachedBunnyStorageStats()).toEqual({ totalBytes: -1, byVideoId: {} });
|
||||
});
|
||||
|
||||
// The flag defaults to on, so a self-hosted deployment that never configured
|
||||
// Bunny lands here: credentials missing while the feature is nominally
|
||||
// enabled.
|
||||
it('degrades to -1 when the Bunny credentials are not configured', async () => {
|
||||
// The flag defaults to on, so a self-hosted deployment that never configured Bunny
|
||||
// lands here: credentials missing while the feature is nominally enabled. That used to
|
||||
// key on the flag alone, throw "Missing Bunny Stream credentials." out of
|
||||
// getBunnyConfig() and report -1, which reads as "we could not measure" rather than
|
||||
// "there is nothing to measure".
|
||||
it('reports a genuine zero when the Bunny credentials are not configured', async () => {
|
||||
vi.stubEnv('BUNNY_STREAM_API_KEY', undefined);
|
||||
vi.stubEnv('BUNNY_STREAM_LIBRARY_ID', undefined);
|
||||
vi.stubEnv('NEXT_PUBLIC_BUNNY_STREAM_LIBRARY_ID', undefined);
|
||||
const calls = stubBunnyPages([]);
|
||||
|
||||
expect(await getCachedBunnyStorageStats()).toEqual({ totalBytes: -1, byVideoId: {} });
|
||||
expect(await getCachedBunnyStorageStats()).toEqual({ totalBytes: 0, byVideoId: {} });
|
||||
expect(calls.urls).toEqual([]);
|
||||
});
|
||||
|
||||
@@ -875,16 +877,18 @@ describe('getCachedStripeStats', () => {
|
||||
pastDueUsers: 1,
|
||||
canceledUsers: 1,
|
||||
freeUsers: 3,
|
||||
otherStatusUsers: 0,
|
||||
mrrCents: 3800,
|
||||
currency: 'eur',
|
||||
});
|
||||
expect(stripe.retrievedPriceIds).toEqual(['price_admin_stats_test']);
|
||||
});
|
||||
|
||||
// UNPAID, INCOMPLETE and INCOMPLETE_EXPIRED are real values of the enum that
|
||||
// the report has no bucket for. They must not be silently folded into one of
|
||||
// the five that are reported.
|
||||
it('leaves statuses it does not report out of every bucket', async () => {
|
||||
// UNPAID, INCOMPLETE and INCOMPLETE_EXPIRED are real values of the enum that used to
|
||||
// belong to none of the reported buckets, so those users were counted nowhere and the
|
||||
// five totals silently did not add up to the user table. They must not be folded into
|
||||
// one of the five either.
|
||||
it('counts the statuses the five named buckets do not cover', async () => {
|
||||
await createUser({ subscriptionStatus: 'UNPAID' });
|
||||
await createUser({ subscriptionStatus: 'INCOMPLETE' });
|
||||
await createUser({ subscriptionStatus: 'INCOMPLETE_EXPIRED' });
|
||||
@@ -898,6 +902,7 @@ describe('getCachedStripeStats', () => {
|
||||
pastDueUsers: 0,
|
||||
canceledUsers: 0,
|
||||
freeUsers: 0,
|
||||
otherStatusUsers: 3,
|
||||
mrrCents: 0,
|
||||
currency: 'usd',
|
||||
});
|
||||
@@ -912,6 +917,7 @@ describe('getCachedStripeStats', () => {
|
||||
pastDueUsers: 0,
|
||||
canceledUsers: 0,
|
||||
freeUsers: 0,
|
||||
otherStatusUsers: 0,
|
||||
mrrCents: 0,
|
||||
currency: 'usd',
|
||||
});
|
||||
@@ -932,6 +938,7 @@ describe('getCachedStripeStats', () => {
|
||||
pastDueUsers: 0,
|
||||
canceledUsers: 0,
|
||||
freeUsers: 0,
|
||||
otherStatusUsers: 0,
|
||||
mrrCents: 0,
|
||||
currency: 'usd',
|
||||
});
|
||||
|
||||
@@ -41,11 +41,6 @@ import {
|
||||
createWorkspace,
|
||||
} from '../factories';
|
||||
|
||||
type Intent = 'view' | 'manage' | 'delete';
|
||||
|
||||
/** `undefined` stands for a caller that passes no options at all. */
|
||||
const INTENTS: ReadonlyArray<Intent | undefined> = [undefined, 'view', 'manage', 'delete'];
|
||||
|
||||
type Actor =
|
||||
| 'an anonymous caller'
|
||||
| 'an outsider'
|
||||
@@ -252,42 +247,6 @@ const SCENARIOS: readonly Scenario[] = [
|
||||
];
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// The known divergence
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/**
|
||||
* The one place the two functions disagree, spelled out rather than filtered
|
||||
* out.
|
||||
*
|
||||
* `shouldLoadWorkspaceRole` in lib/auth.ts skips the workspace queries for a
|
||||
* project owner on `intent: 'view'`, on the reasoning that an owner passes every
|
||||
* check on their own. That is true of the three permission booleans, and it is
|
||||
* why this is harmless today, but it is not true of the two identity flags:
|
||||
* checkProjectAccess() reports `isWorkspaceMember: false, isWorkspaceAdmin:
|
||||
* false` for an owner who is in fact the workspace owner, where
|
||||
* computeProjectAccess() on the same rows reports true and true.
|
||||
*
|
||||
* Harmless today rests on one fact and not on the design: the only consumer of
|
||||
* `isWorkspaceMember` from checkProjectAccess() is
|
||||
* app/api/versions/[versionId]/approvals/route.ts:37, and its
|
||||
* `isOwner || isProjectMember || isWorkspaceMember` is already satisfied by
|
||||
* `isOwner` for exactly the actor that diverges. Nothing consumes
|
||||
* `isWorkspaceAdmin` from checkProjectAccess() at all; `canEdit` folds it in,
|
||||
* and `isOwner` covers that too. The next reader of either flag inherits the
|
||||
* bug, which is why this is asserted instead of hidden behind a comparison of
|
||||
* `hasAccess` alone.
|
||||
*
|
||||
* If this stops matching, the divergence was closed: delete this function and
|
||||
* the test at the bottom of the file rather than widening either of them.
|
||||
*/
|
||||
function knownDivergence(actor: Actor, intent: Intent | undefined): Partial<ExpectedAccess> {
|
||||
const resolvesWorkspaceRole = intent === 'manage' || intent === 'delete';
|
||||
if (actor === 'a project owner who also owns the workspace' && !resolvesWorkspaceRole) {
|
||||
return { isWorkspaceMember: false, isWorkspaceAdmin: false };
|
||||
}
|
||||
return {};
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Fixtures
|
||||
// ---------------------------------------------------------------------------
|
||||
@@ -428,7 +387,7 @@ for (const scenario of SCENARIOS) {
|
||||
});
|
||||
|
||||
for (const actor of ACTORS) {
|
||||
it(`resolves ${actor} exactly as computeProjectAccess does, at every intent`, async () => {
|
||||
it(`resolves ${actor} exactly as computeProjectAccess does`, async () => {
|
||||
const userId = seeded.userIdFor(actor);
|
||||
const projectId = projectIdFor(actor, seeded);
|
||||
const expected = scenario.expected[actor];
|
||||
@@ -437,20 +396,10 @@ for (const scenario of SCENARIOS) {
|
||||
// The pure half first: given the rows, this is the answer.
|
||||
expect(computeProjectAccess(enriched, userId), 'computeProjectAccess').toEqual(expected);
|
||||
|
||||
// And the querying half, which has to arrive at the same place from the
|
||||
// same rows, whatever the caller says it intends to do.
|
||||
for (const intent of INTENTS) {
|
||||
const checked = await checkProjectAccess(
|
||||
enriched,
|
||||
userId,
|
||||
intent === undefined ? undefined : { intent }
|
||||
);
|
||||
|
||||
expect(checked, `checkProjectAccess with intent ${intent ?? '(default)'}`).toEqual({
|
||||
...expected,
|
||||
...knownDivergence(actor, intent),
|
||||
});
|
||||
}
|
||||
// And the querying half, which has to arrive at the same place from the same rows.
|
||||
// It used to take an `intent` that skipped the workspace queries for an owner at
|
||||
// `view`, which is what made these two disagree; there is one code path now.
|
||||
expect(await checkProjectAccess(enriched, userId), 'checkProjectAccess').toEqual(expected);
|
||||
});
|
||||
}
|
||||
});
|
||||
@@ -460,8 +409,13 @@ for (const scenario of SCENARIOS) {
|
||||
// The divergence, on its own
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('checkProjectAccess and computeProjectAccess disagree in one place', () => {
|
||||
it('hides the workspace role from a project owner who also owns the workspace, on intent view', async () => {
|
||||
describe('checkProjectAccess and computeProjectAccess agree about the workspace role', () => {
|
||||
// This was the one cell of the matrix that diverged, and it is the shape every real
|
||||
// signup produces: app/api/projects/route.ts gives a new project the workspace owner's
|
||||
// id. checkProjectAccess() used to skip both workspace queries for an owner at `view`
|
||||
// intent, so the two identity flags read as if this user were a stranger to the
|
||||
// workspace they own.
|
||||
it('reports a project owner who also owns the workspace as a workspace admin', async () => {
|
||||
const owner = await createUser();
|
||||
const workspace = await createWorkspace({ ownerId: owner.id });
|
||||
const project = await createProject({
|
||||
@@ -471,32 +425,11 @@ describe('checkProjectAccess and computeProjectAccess disagree in one place', ()
|
||||
});
|
||||
|
||||
const enriched = await fetchEnriched(project.id, owner.id);
|
||||
|
||||
const computed = computeProjectAccess(enriched, owner.id);
|
||||
const viewed = await checkProjectAccess(enriched, owner.id, { intent: 'view' });
|
||||
const managed = await checkProjectAccess(enriched, owner.id, { intent: 'manage' });
|
||||
|
||||
// What the rows say: this user owns the workspace the project lives in.
|
||||
expect(computed.isWorkspaceMember).toBe(true);
|
||||
expect(computed.isWorkspaceAdmin).toBe(true);
|
||||
|
||||
// What checkProjectAccess() says on the intent that pages and GET routes
|
||||
// use. `shouldLoadWorkspaceRole` skipped the workspace queries, so the role
|
||||
// was never resolved and the two flags read as if this user were a stranger
|
||||
// to the workspace.
|
||||
expect(viewed.isWorkspaceMember).toBe(false);
|
||||
expect(viewed.isWorkspaceAdmin).toBe(false);
|
||||
|
||||
// Ask the same question with a mutating intent and the same user, on the
|
||||
// same rows, is a workspace owner again.
|
||||
expect(managed.isWorkspaceMember).toBe(true);
|
||||
expect(managed.isWorkspaceAdmin).toBe(true);
|
||||
|
||||
// Why nobody has noticed: every permission the flags feed is already
|
||||
// granted by isOwner, so the divergence stops at the two identity flags.
|
||||
expect(viewed.hasAccess).toBe(true);
|
||||
expect(viewed.canEdit).toBe(true);
|
||||
expect(viewed.canDelete).toBe(true);
|
||||
expect({ ...viewed, isWorkspaceMember: true, isWorkspaceAdmin: true }).toEqual(computed);
|
||||
expect(await checkProjectAccess(enriched, owner.id)).toEqual(computed);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -180,19 +180,20 @@ describe('cancelR2UploadSession', () => {
|
||||
).toBe('FINALIZED');
|
||||
});
|
||||
|
||||
// The `expiresAt: { gt: now }` clause means an expired session cannot be
|
||||
// cancelled at all: the row stays INITIATED and consumedAt stays null. That
|
||||
// is the current contract, and it is why the sweeper rather than the route
|
||||
// has to be the thing that reclaims those reservations. See the report.
|
||||
it('matches nothing once the session has expired, leaving it INITIATED', async () => {
|
||||
// An `expiresAt: { gt: now }` clause used to make this match zero rows once a session
|
||||
// lapsed, so it stayed INITIATED with a null consumedAt for good. The r2-init DELETE
|
||||
// route releases the quota reservation only when the update reports a row, so every
|
||||
// abandoned upload held its reserved bytes against the user's quota permanently.
|
||||
// Cancelling something already expired is the case that most needs to work.
|
||||
it('cancels a session that has already expired', async () => {
|
||||
const { session } = await newSession({ expiresAt: new Date(Date.now() - 60_000) });
|
||||
|
||||
const result = await cancelR2UploadSession(session.id);
|
||||
|
||||
expect(result.count).toBe(0);
|
||||
expect(result.count).toBe(1);
|
||||
const row = await db.videoUploadSession.findUniqueOrThrow({ where: { id: session.id } });
|
||||
expect(row.status).toBe('INITIATED');
|
||||
expect(row.consumedAt).toBeNull();
|
||||
expect(row.status).toBe('CANCELLED');
|
||||
expect(row.consumedAt).not.toBeNull();
|
||||
});
|
||||
|
||||
it('does nothing for an id that matches no row', async () => {
|
||||
|
||||
@@ -18,7 +18,6 @@ import {
|
||||
extractVideoFileNameFromProxyUrl,
|
||||
extractVideoKeyFromProxyUrl,
|
||||
getVideoAssetAccessContext,
|
||||
mediaUrlToR2Key,
|
||||
sanitizeAssetDisplayName,
|
||||
SAFE_BUNNY_VIDEO_ID,
|
||||
} from '@/lib/video-assets';
|
||||
@@ -132,26 +131,25 @@ describe('proxy URL extraction', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('mediaUrlToR2Key', () => {
|
||||
it('derives an image key and a voice key from canonical URLs', () => {
|
||||
expect(mediaUrlToR2Key(IMAGE_URL)).toBe('images/eeeeeeee-eeee-4eee-8eee-eeeeeeeeeee1.png');
|
||||
expect(mediaUrlToR2Key(AUDIO_URL)).toBe('voice/eeeeeeee-eeee-4eee-8eee-eeeeeeeeeee2.webm');
|
||||
});
|
||||
|
||||
it('returns null for a URL that is not an image or audio proxy path', () => {
|
||||
expect(mediaUrlToR2Key(VIDEO_URL)).toBeNull();
|
||||
expect(mediaUrlToR2Key('https://www.youtube.com/watch?v=dQw4w9WgXcQ')).toBeNull();
|
||||
});
|
||||
|
||||
// Documented, not endorsed. Unlike extractImageKeyFromProxyUrl this one
|
||||
// matches on a substring with no shape check, so the key it produces is
|
||||
// attacker-shaped whenever the URL is. The module has no callers today; if
|
||||
// one appears it must use the extract* helpers instead. See the report.
|
||||
it('accepts a substring match that the anchored extractor rejects', () => {
|
||||
// mediaUrlToR2Key used to live here. It matched the proxy prefix as a substring with no
|
||||
// shape check, so `https://evil.test/api/upload/image/../../videos/live.mp4` produced the
|
||||
// key `images/../../videos/live.mp4`. It had no callers, so it was deleted rather than
|
||||
// reimplemented; the anchored extract* helpers below are what a caller should use.
|
||||
describe('the anchored extractors refuse a substring match', () => {
|
||||
it('rejects a hostile url that only contains the proxy prefix', () => {
|
||||
const hostile = 'https://evil.test/api/upload/image/../../videos/live.mp4';
|
||||
|
||||
expect(extractImageKeyFromProxyUrl(hostile)).toBeNull();
|
||||
expect(mediaUrlToR2Key(hostile)).toBe('images/../../videos/live.mp4');
|
||||
expect(extractImageFileNameFromProxyUrl(hostile)).toBeNull();
|
||||
});
|
||||
|
||||
it('still derives keys from canonical urls', () => {
|
||||
expect(extractImageKeyFromProxyUrl(IMAGE_URL)).toBe(
|
||||
'images/eeeeeeee-eeee-4eee-8eee-eeeeeeeeeee1.png'
|
||||
);
|
||||
expect(extractAudioKeyFromProxyUrl(AUDIO_URL)).toBe(
|
||||
'voice/eeeeeeee-eeee-4eee-8eee-eeeeeeeeeee2.webm'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -14,7 +14,7 @@
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
import { revalidatePath } from 'next/cache';
|
||||
import { db } from '@/lib/db';
|
||||
import { deleteProjectVideosWithCleanup } from '@/lib/video-delete';
|
||||
import { deleteProjectVideosWithCleanup, VideoStorageCleanupError } from '@/lib/video-delete';
|
||||
import {
|
||||
createComment,
|
||||
createProject,
|
||||
@@ -372,12 +372,10 @@ describe('deleteProjectVideosWithCleanup and Bunny', () => {
|
||||
});
|
||||
|
||||
describe('deleteProjectVideosWithCleanup when storage fails', () => {
|
||||
// The rows go first and the objects go second, with no transaction spanning
|
||||
// the two. A refused DELETE therefore leaves the object behind with nothing
|
||||
// in the database still pointing at it: the caller cannot retry, because the
|
||||
// video id it would retry with no longer resolves. The result object is the
|
||||
// only trace, which is why the warnings are asserted here.
|
||||
it('still removes the rows and surfaces the orphaned key as a warning', async () => {
|
||||
// Storage runs first and the rows go second. Deleting the rows first left the object in
|
||||
// the bucket with nothing pointing at it and no way to retry, because the video id no
|
||||
// longer resolved. Keeping the rows makes the delete repeatable.
|
||||
it('keeps the rows and reports the failure when a key cannot be deleted', async () => {
|
||||
const scenario = await seedProject();
|
||||
const target = await seedDeletableVideo({
|
||||
projectId: scenario.project.id,
|
||||
@@ -387,22 +385,40 @@ describe('deleteProjectVideosWithCleanup when storage fails', () => {
|
||||
});
|
||||
r2.rejectKeys.add(TARGET_VIDEO_KEY);
|
||||
|
||||
const result = await deleteProjectVideosWithCleanup(scenario.project.id, [target.video.id]);
|
||||
await expect(
|
||||
deleteProjectVideosWithCleanup(scenario.project.id, [target.video.id])
|
||||
).rejects.toBeInstanceOf(VideoStorageCleanupError);
|
||||
|
||||
expect(result.deletedCount).toBe(1);
|
||||
// The row is gone even though its object is not.
|
||||
expect(await db.video.count()).toBe(0);
|
||||
expect(result.cleanupInput.r2).toEqual({
|
||||
attempted: 2,
|
||||
failed: 1,
|
||||
failedKeys: [TARGET_VIDEO_KEY],
|
||||
});
|
||||
expect(result.cleanupWarnings).toEqual({ r2: { attempted: 2, failed: 1 } });
|
||||
// The row is still there, so the caller can try again.
|
||||
expect(await db.video.count()).toBe(1);
|
||||
// The rest of the sweep still ran.
|
||||
expect(r2.deletedKeys).toEqual([TARGET_COMMENT_IMAGE_KEY]);
|
||||
});
|
||||
|
||||
it('reports a Bunny failure without failing the delete', async () => {
|
||||
it('carries the failed keys on the error so the route can log them', async () => {
|
||||
const scenario = await seedProject();
|
||||
const target = await seedDeletableVideo({
|
||||
projectId: scenario.project.id,
|
||||
ownerId: scenario.owner.id,
|
||||
videoUrl: TARGET_VIDEO_URL,
|
||||
commentImageUrl: TARGET_COMMENT_IMAGE,
|
||||
});
|
||||
r2.rejectKeys.add(TARGET_VIDEO_KEY);
|
||||
|
||||
const error = await deleteProjectVideosWithCleanup(scenario.project.id, [
|
||||
target.video.id,
|
||||
]).catch((err: unknown) => err as VideoStorageCleanupError);
|
||||
|
||||
expect(error.cleanupInput.r2).toEqual({
|
||||
attempted: 2,
|
||||
failed: 1,
|
||||
failedKeys: [TARGET_VIDEO_KEY],
|
||||
});
|
||||
});
|
||||
|
||||
// A Bunny video that survives the delete is billed and invisible in the app, so it gets
|
||||
// the same treatment as an orphaned R2 object.
|
||||
it('keeps the rows when Bunny refuses the delete', async () => {
|
||||
vi.stubEnv('BUNNY_STREAM_API_KEY', 'test-bunny-key');
|
||||
vi.stubEnv('BUNNY_STREAM_LIBRARY_ID', '9999');
|
||||
vi.stubGlobal(
|
||||
@@ -417,10 +433,13 @@ describe('deleteProjectVideosWithCleanup when storage fails', () => {
|
||||
providerVideoId: 'bunny-version-id-2',
|
||||
});
|
||||
|
||||
const result = await deleteProjectVideosWithCleanup(scenario.project.id, [video.id]);
|
||||
const error = await deleteProjectVideosWithCleanup(scenario.project.id, [video.id]).catch(
|
||||
(err: unknown) => err as VideoStorageCleanupError
|
||||
);
|
||||
|
||||
expect(await db.video.count()).toBe(0);
|
||||
expect(result.cleanupWarnings).toEqual({ bunny: { attempted: 1, failed: 1 } });
|
||||
expect(error).toBeInstanceOf(VideoStorageCleanupError);
|
||||
expect(error.cleanupInput.bunny).toMatchObject({ attempted: 1, failed: 1 });
|
||||
expect(await db.video.count()).toBe(1);
|
||||
});
|
||||
|
||||
it('reports no warnings when both providers succeed', async () => {
|
||||
|
||||
@@ -151,11 +151,10 @@ describe('GET /api/projects', () => {
|
||||
expect(projects.projects.map((entry) => entry.id)).toEqual([scenario.project.id]);
|
||||
});
|
||||
|
||||
// Documents current behaviour, which looks like a bug. See the note in the
|
||||
// report: the workspace-membership branch of the OR is dropped as soon as
|
||||
// ?workspaceId is supplied, so filtering by workspace hides exactly the
|
||||
// projects the unfiltered call returns.
|
||||
it('stops listing workspace-member projects once ?workspaceId is supplied', async () => {
|
||||
// The workspace-membership branch of the OR used to be dropped as soon as ?workspaceId
|
||||
// was supplied, so filtering by their own workspace showed a member an empty list while
|
||||
// the unfiltered call returned the same project.
|
||||
it('still lists workspace-member projects when ?workspaceId is supplied', async () => {
|
||||
const scenario = await seedProject();
|
||||
const member = await createUser();
|
||||
await addWorkspaceMember({ workspaceId: scenario.workspace.id, userId: member.id });
|
||||
@@ -166,7 +165,7 @@ describe('GET /api/projects', () => {
|
||||
);
|
||||
|
||||
expect(unfiltered.projects.map((entry) => entry.id)).toEqual([scenario.project.id]);
|
||||
expect(filtered.projects).toEqual([]);
|
||||
expect(filtered.projects.map((entry) => entry.id)).toEqual([scenario.project.id]);
|
||||
});
|
||||
|
||||
it('scopes ?workspaceId to that workspace for an owner of several', async () => {
|
||||
|
||||
@@ -165,16 +165,16 @@ describe('checkRateLimit', () => {
|
||||
expect(result.remaining).toBe(RATE_LIMIT_CONFIGS.api.maxRequests - 1);
|
||||
});
|
||||
|
||||
// Defence in depth against oversized values reaching the query. The call is
|
||||
// allowed but nothing is recorded, so an attacker cannot use a huge key to
|
||||
// bloat the table either.
|
||||
it('allows and records nothing for an over-long key or action', async () => {
|
||||
// An oversized value is hashed to fit its column rather than skipped, so it is written
|
||||
// and counted like any other. A huge key cannot bloat the table either: what lands in
|
||||
// the column is a fixed-width digest.
|
||||
it('records an over-long key and an over-long action', async () => {
|
||||
const longKey = await checkRateLimit('x'.repeat(257), 'login', CONFIG);
|
||||
const longAction = await checkRateLimit('1.2.3.4', 'y'.repeat(65), CONFIG);
|
||||
|
||||
expect(longKey.allowed).toBe(true);
|
||||
expect(longAction.allowed).toBe(true);
|
||||
expect(await countRows('rate_limits')).toBe(0);
|
||||
expect(await countRows('rate_limits')).toBe(2);
|
||||
});
|
||||
|
||||
it('records a key of exactly 255 characters, the column width', async () => {
|
||||
@@ -184,27 +184,31 @@ describe('checkRateLimit', () => {
|
||||
expect(await countRows('rate_limits')).toBe(1);
|
||||
});
|
||||
|
||||
// Documents an off-by-one, reported rather than fixed. The guard in
|
||||
// lib/rate-limit.ts rejects `key.length > 256`, but rate_limits.key is
|
||||
// VARCHAR(255), so a 256-character key clears the guard and then fails the
|
||||
// INSERT with P2010. The catch treats any database error as "allow", so such a
|
||||
// key is never counted and the limit silently stops applying to it.
|
||||
//
|
||||
// Not reachable from the product today: every call site builds a key from an
|
||||
// IP, a user id or a 24-character hash. The failure mode is fail-open, so a
|
||||
// future longer key would disable a limit rather than break a page.
|
||||
it('fails open for a 256-character key instead of counting it', async () => {
|
||||
// Kept to four attempts, one past the limit, because each one logs the
|
||||
// swallowed Postgres error and the point is made without ten copies of it.
|
||||
// This is the case that used to fail open twice over: the guard allowed a 256-character
|
||||
// key through, the INSERT then failed with SQLSTATE 22001 against a VARCHAR(255) column,
|
||||
// and the catch answered "allowed" for every attempt. The key is now hashed before it
|
||||
// reaches the query, so the limit applies to it like any other.
|
||||
it('counts a 256-character key and blocks it past the cap', async () => {
|
||||
const key = 'x'.repeat(256);
|
||||
|
||||
for (let attempt = 0; attempt < 4; attempt += 1) {
|
||||
for (let attempt = 0; attempt < CONFIG.maxRequests; attempt += 1) {
|
||||
const result = await checkRateLimit(key, 'login', CONFIG);
|
||||
expect(result.allowed).toBe(true);
|
||||
expect(result.remaining).toBe(CONFIG.maxRequests);
|
||||
}
|
||||
|
||||
expect(await countRows('rate_limits')).toBe(0);
|
||||
const blocked = await checkRateLimit(key, 'login', CONFIG);
|
||||
expect(blocked.allowed).toBe(false);
|
||||
expect(blocked.remaining).toBe(0);
|
||||
|
||||
expect(await countRows('rate_limits')).toBe(1);
|
||||
expect((await db.rateLimit.findFirstOrThrow()).count).toBe(CONFIG.maxRequests + 1);
|
||||
});
|
||||
|
||||
it('keeps two different over-long keys in separate buckets', async () => {
|
||||
await checkRateLimit(`a${'x'.repeat(300)}`, 'login', CONFIG);
|
||||
await checkRateLimit(`b${'x'.repeat(300)}`, 'login', CONFIG);
|
||||
|
||||
expect(await countRows('rate_limits')).toBe(2);
|
||||
});
|
||||
|
||||
it('counts concurrent calls exactly once each', async () => {
|
||||
|
||||
@@ -297,14 +297,14 @@ describe('GET /api/search reaches everything the caller is entitled to', () => {
|
||||
// ---------------------------------------------------------------------------
|
||||
// Billing
|
||||
// ---------------------------------------------------------------------------
|
||||
// Pins current behaviour, and the behaviour is inconsistent. See the report
|
||||
// accompanying this suite: GET /api/projects filters every row through
|
||||
// `workspace.owner: buildBillingAccessWhereInput()`, and `checkProjectAccess()`
|
||||
// makes `hasAccess` false the moment the workspace owner's billing lapses, so
|
||||
// the project itself answers 403. /api/search applies no billing filter at all
|
||||
// and keeps returning the names. Changing that means changing this test.
|
||||
// Search carries the same billing condition every other read path does. It used to carry
|
||||
// none: GET /api/projects filters every row through
|
||||
// `workspace.owner: buildBillingAccessWhereInput()`, and `checkProjectAccess()` makes
|
||||
// `hasAccess` false the moment the workspace owner's billing lapses, so the project
|
||||
// itself answers 403, while search went on returning names, descriptions and video
|
||||
// titles for the same tenant.
|
||||
describe('GET /api/search and lapsed billing', () => {
|
||||
it('keeps returning a project whose workspace owner has lost billing access', async () => {
|
||||
it('hides a project whose workspace owner has lost billing access', async () => {
|
||||
const term = uniqueTerm();
|
||||
const expiredOwner = await createExpiredUser();
|
||||
await seedProject({ ownerUser: expiredOwner, projectName: `${term} lapsed project` });
|
||||
@@ -312,11 +312,22 @@ describe('GET /api/search and lapsed billing', () => {
|
||||
|
||||
const results = await searchFor(term);
|
||||
|
||||
expect(results.projects.map((entry) => entry.name)).toEqual([`${term} lapsed project`]);
|
||||
expect(results.projects).toEqual([]);
|
||||
});
|
||||
|
||||
// The same caller, the same row, through the list endpoint instead. This is
|
||||
// the contrast that makes the case above a finding rather than a preference.
|
||||
// The positive control: the same shape with billing intact still comes back, so the
|
||||
// assertion above is about billing and not about the fixture failing to seed.
|
||||
it('still returns a project whose workspace owner is paying', async () => {
|
||||
const term = uniqueTerm();
|
||||
const scenario = await seedProject({ projectName: `${term} live project` });
|
||||
signedInAs(scenario.owner);
|
||||
|
||||
const results = await searchFor(term);
|
||||
|
||||
expect(results.projects.map((entry) => entry.name)).toEqual([`${term} live project`]);
|
||||
});
|
||||
|
||||
// The same caller, the same row, through the list endpoint instead: the two agree now.
|
||||
it('is hidden from GET /api/projects for the same caller and the same row', async () => {
|
||||
const expiredOwner = await createExpiredUser();
|
||||
await seedProject({ ownerUser: expiredOwner, projectName: 'Lapsed project' });
|
||||
@@ -329,7 +340,7 @@ describe('GET /api/search and lapsed billing', () => {
|
||||
expect(projects).toEqual([]);
|
||||
});
|
||||
|
||||
it('keeps returning a video title from a lapsed workspace to a collaborator', async () => {
|
||||
it('hides a video title from a lapsed workspace, even from a collaborator', async () => {
|
||||
const term = uniqueTerm();
|
||||
const expiredOwner = await createExpiredUser();
|
||||
const { project } = await seedProject({ ownerUser: expiredOwner });
|
||||
@@ -340,6 +351,20 @@ describe('GET /api/search and lapsed billing', () => {
|
||||
|
||||
const results = await searchFor(term);
|
||||
|
||||
expect(results.videos.map((entry) => entry.title)).toEqual([`${term} lapsed cut`]);
|
||||
expect(results.videos).toEqual([]);
|
||||
});
|
||||
|
||||
it('still returns a video title to a collaborator while the owner is paying', async () => {
|
||||
const term = uniqueTerm();
|
||||
const { project } = await seedProject();
|
||||
|
||||
await createVideo({ projectId: project.id, title: `${term} live cut` });
|
||||
const collaborator = await createUser();
|
||||
await addProjectMember({ projectId: project.id, userId: collaborator.id });
|
||||
signedInAs(collaborator);
|
||||
|
||||
const results = await searchFor(term);
|
||||
|
||||
expect(results.videos.map((entry) => entry.title)).toEqual([`${term} live cut`]);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user