feat(cards): positive field whitelist for client-manager writes
This commit is contained in:
parent
d14694ceab
commit
6a6df8076b
3 changed files with 432 additions and 37 deletions
|
|
@ -1556,48 +1556,121 @@ export function BoardColumn({
|
|||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 3: Create the board route**
|
||||
- [ ] **Step 3: Create the client card controls**
|
||||
|
||||
`src/app/p/[clientSlug]/[projectSlug]/page.tsx`:
|
||||
`src/components/board/client-card-controls.tsx`:
|
||||
|
||||
```tsx
|
||||
import { notFound, redirect } from "next/navigation";
|
||||
"use client";
|
||||
|
||||
import { AppShell } from "@/components/shell/app-shell";
|
||||
import { TopBar } from "@/components/shell/top-bar";
|
||||
import { Board } from "@/components/board/board";
|
||||
import { getSession } from "@/lib/auth/session";
|
||||
import { loadBoard } from "@/lib/data/board";
|
||||
import { useState } from "react";
|
||||
|
||||
export default async function BoardPage({
|
||||
params,
|
||||
import {
|
||||
Dropdown,
|
||||
DropdownContent,
|
||||
DropdownItem,
|
||||
DropdownLabel,
|
||||
DropdownTrigger,
|
||||
} from "@/components/ui/dropdown";
|
||||
import type { CardRecord, Priority } from "@/lib/pb/types";
|
||||
|
||||
const PRIORITIES: Array<{ value: Priority; label: string }> = [
|
||||
{ value: "critical", label: "Critical" },
|
||||
{ value: "high", label: "High" },
|
||||
{ value: "medium", label: "Medium" },
|
||||
{ value: "low", label: "Low" },
|
||||
];
|
||||
|
||||
/**
|
||||
* The only two fields a client manager may change.
|
||||
*
|
||||
* Both go through PATCH /api/client/cards/:id, never straight to PocketBase.
|
||||
* Their token cannot write to the cards collection at all — see Plan 1a — so
|
||||
* a direct SDK call from here would simply fail. The route is the path.
|
||||
*/
|
||||
export function ClientCardControls({
|
||||
card,
|
||||
onPatched,
|
||||
}: {
|
||||
params: Promise<{ clientSlug: string; projectSlug: string }>;
|
||||
card: CardRecord;
|
||||
onPatched: (card: CardRecord) => void;
|
||||
}) {
|
||||
const session = await getSession();
|
||||
if (!session) redirect("/login");
|
||||
const [saving, setSaving] = useState(false);
|
||||
const [error, setError] = useState<string | null>(null);
|
||||
|
||||
const { clientSlug, projectSlug } = await params;
|
||||
const board = await loadBoard(clientSlug, projectSlug);
|
||||
async function patch(body: Record<string, unknown>) {
|
||||
setSaving(true);
|
||||
setError(null);
|
||||
|
||||
// loadBoard returns null both for "does not exist" and "not yours". Rendering
|
||||
// the same 404 for both is deliberate: a distinct 403 would confirm to one
|
||||
// client that another client's project is real.
|
||||
if (!board) notFound();
|
||||
const previous = { ...card };
|
||||
onPatched({ ...card, ...body } as CardRecord);
|
||||
|
||||
const response = await fetch(`/api/client/cards/${card.id}`, {
|
||||
method: "PATCH",
|
||||
headers: { "content-type": "application/json" },
|
||||
body: JSON.stringify(body),
|
||||
});
|
||||
|
||||
setSaving(false);
|
||||
|
||||
if (!response.ok) {
|
||||
onPatched(previous);
|
||||
setError("Could not save");
|
||||
return;
|
||||
}
|
||||
|
||||
const { card: updated } = await response.json();
|
||||
onPatched({ ...card, ...updated } as CardRecord);
|
||||
}
|
||||
|
||||
const currentPriority =
|
||||
PRIORITIES.find((p) => p.value === card.priority) ?? PRIORITIES[2];
|
||||
|
||||
return (
|
||||
<AppShell
|
||||
role={session.role}
|
||||
currentPath={`/p/${clientSlug}/${projectSlug}`}
|
||||
>
|
||||
<TopBar title={board.project.name} subtitle={board.client.name} />
|
||||
<Board
|
||||
role={session.role}
|
||||
projectId={board.project.id}
|
||||
columns={board.columns}
|
||||
initialCards={board.cards}
|
||||
/>
|
||||
</AppShell>
|
||||
<div className="mt-3 flex items-center gap-2 border-t border-border-subtle pt-2.5">
|
||||
<Dropdown>
|
||||
<DropdownTrigger asChild>
|
||||
<button
|
||||
type="button"
|
||||
disabled={saving}
|
||||
className="rounded-md px-1.5 py-1 text-[11px] font-medium text-text-secondary hover:bg-surface-raised disabled:opacity-50"
|
||||
>
|
||||
{currentPriority.label}
|
||||
</button>
|
||||
</DropdownTrigger>
|
||||
<DropdownContent>
|
||||
<DropdownLabel>Priority</DropdownLabel>
|
||||
{PRIORITIES.map((option) => (
|
||||
<DropdownItem
|
||||
key={option.value}
|
||||
active={option.value === card.priority}
|
||||
onSelect={() => patch({ priority: option.value })}
|
||||
>
|
||||
{option.label}
|
||||
</DropdownItem>
|
||||
))}
|
||||
</DropdownContent>
|
||||
</Dropdown>
|
||||
|
||||
<label className="ml-auto flex items-center gap-1.5">
|
||||
<span className="sr-only">Target date</span>
|
||||
<input
|
||||
type="date"
|
||||
disabled={saving}
|
||||
value={card.target_date ? card.target_date.slice(0, 10) : ""}
|
||||
onChange={(event) =>
|
||||
patch({ target_date: event.target.value || null })
|
||||
}
|
||||
className="tabular rounded-md border border-border-subtle bg-surface px-1.5 py-1 text-[11px] text-text-secondary disabled:opacity-50"
|
||||
/>
|
||||
</label>
|
||||
|
||||
{error && (
|
||||
<span role="alert" className="text-[11px] text-accent">
|
||||
{error}
|
||||
</span>
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
```
|
||||
|
|
@ -1605,20 +1678,123 @@ export default async function BoardPage({
|
|||
- [ ] **Step 4: Commit**
|
||||
|
||||
```bash
|
||||
git add src/components/board src/app/p
|
||||
git commit -m "feat(board): column and card tile presentation"
|
||||
git add src/components/board
|
||||
git commit -m "feat(board): column, card tile and client control presentation"
|
||||
```
|
||||
|
||||
The build will fail until Task 10 creates `Board`. That is expected — commit the presentation layer on its own so it can be reviewed separately.
|
||||
|
||||
---
|
||||
|
||||
## Task 10: Drag, drop and persistence
|
||||
## Task 10: Owner card move route
|
||||
|
||||
**Files:**
|
||||
- Create: `src/app/api/owner/cards/[id]/move/route.ts`
|
||||
|
||||
**Why this route exists.** Plan 1a stores the auth token in an **httpOnly** cookie, which JavaScript cannot read. That is the right call for a token, and it has a consequence worth stating plainly: the browser-side PocketBase SDK has no credentials and cannot write anything. An owner drag that called `pbBrowser.collection("cards").update(...)` would fail silently with a 403 and the card would snap back.
|
||||
|
||||
The two ways out are to make the cookie readable by JavaScript, or to move the write server-side. The first hands a long-lived token to any XSS on the page; the second costs one small route. This plan takes the second.
|
||||
|
||||
`src/lib/pb/browser.ts` from Plan 1a therefore stays unauthenticated and is used for nothing in this plan. Leave it in place — Plan 1c may authenticate it deliberately for realtime, with that tradeoff made explicitly.
|
||||
|
||||
- [ ] **Step 1: Write the route**
|
||||
|
||||
`src/app/api/owner/cards/[id]/move/route.ts`:
|
||||
|
||||
```ts
|
||||
import { getServerPb } from "@/lib/pb/server";
|
||||
import { requireSession } from "@/lib/auth/session";
|
||||
import { AccessError } from "@/lib/auth/access";
|
||||
import { jsonError, jsonOk } from "@/lib/http/json";
|
||||
import type { CardRecord } from "@/lib/pb/types";
|
||||
|
||||
/**
|
||||
* PATCH /api/owner/cards/:id/move — the owner drops a card in a new place.
|
||||
*
|
||||
* Unlike the /api/client/* routes, this one uses the caller's OWN PocketBase
|
||||
* token, not a superuser's. The owner-only write rule from Plan 1a is what
|
||||
* authorises the update, so a client manager reaching this route is refused by
|
||||
* the database even though the handler itself is simple.
|
||||
*
|
||||
* That is why there is no field whitelist here: PocketBase is already
|
||||
* enforcing who may write. Only the two positional fields are forwarded
|
||||
* regardless, so a stray body field cannot ride along.
|
||||
*/
|
||||
export async function PATCH(
|
||||
request: Request,
|
||||
context: { params: Promise<{ id: string }> }
|
||||
): Promise<Response> {
|
||||
try {
|
||||
const session = await requireSession();
|
||||
const { id } = await context.params;
|
||||
|
||||
if (session.role !== "owner") {
|
||||
return jsonError("Card not found", 404);
|
||||
}
|
||||
|
||||
let body: unknown;
|
||||
try {
|
||||
body = await request.json();
|
||||
} catch {
|
||||
return jsonError("Invalid request body", 400);
|
||||
}
|
||||
|
||||
const record = body as Record<string, unknown> | null;
|
||||
const column = record?.column;
|
||||
const position = record?.position;
|
||||
|
||||
if (typeof column !== "string" || !column) {
|
||||
return jsonError("A destination column is required", 400);
|
||||
}
|
||||
if (typeof position !== "number" || !Number.isFinite(position)) {
|
||||
return jsonError("A numeric position is required", 400);
|
||||
}
|
||||
|
||||
const pb = await getServerPb();
|
||||
|
||||
const updated = await pb
|
||||
.collection("cards")
|
||||
.update<CardRecord>(id, { column, position });
|
||||
|
||||
return jsonOk({
|
||||
card: {
|
||||
id: updated.id,
|
||||
column: updated.column,
|
||||
position: updated.position,
|
||||
},
|
||||
});
|
||||
} catch (error) {
|
||||
if (error instanceof AccessError) {
|
||||
return jsonError(error.message, error.status);
|
||||
}
|
||||
console.error("[owner-move] failed", error);
|
||||
return jsonError("Could not move the card", 500);
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Verify the build**
|
||||
|
||||
```bash
|
||||
npm run build
|
||||
```
|
||||
|
||||
Expected: build succeeds; the route list includes `/api/owner/cards/[id]/move`.
|
||||
|
||||
- [ ] **Step 3: Commit**
|
||||
|
||||
```bash
|
||||
git add src/app/api/owner
|
||||
git commit -m "feat(api): owner card move route using the caller's own token"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 11: Drag, drop and persistence
|
||||
|
||||
**Files:**
|
||||
- Create: `src/components/board/board.tsx`
|
||||
- Create: `src/app/p/[clientSlug]/[projectSlug]/page.tsx`
|
||||
|
||||
**Who may drag.** The owner only. A client manager reads the board and edits two fields through the controls in Task 11; the spec is explicit that they may not move cards between columns. `@dnd-kit` is given no sensors at all for a client manager, so there is nothing to disable and nothing to bypass. This is presentation convenience, not the security boundary — the boundary is the PocketBase write rule from Plan 1a, which refuses their token outright.
|
||||
**Who may drag.** The owner only. A client manager reads the board and edits two fields through the controls from Task 9; the spec is explicit that they may not move cards between columns. `@dnd-kit` is given no sensors at all for a client manager, so there is nothing to disable and nothing to bypass. This is presentation convenience, not the security boundary — the boundary is the PocketBase write rule from Plan 1a, which refuses their token outright.
|
||||
|
||||
- [ ] **Step 1: Install @dnd-kit**
|
||||
|
||||
|
|
|
|||
94
src/lib/cards/client-patch.test.ts
Normal file
94
src/lib/cards/client-patch.test.ts
Normal file
|
|
@ -0,0 +1,94 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import {
|
||||
CLIENT_EDITABLE_CARD_FIELDS,
|
||||
pickClientCardPatch,
|
||||
pickClientCardCreate,
|
||||
} from "@/lib/cards/client-patch";
|
||||
|
||||
describe("pickClientCardPatch", () => {
|
||||
it("accepts priority and target_date", () => {
|
||||
const { patch, dropped } = pickClientCardPatch({
|
||||
priority: "high",
|
||||
target_date: "2026-10-01",
|
||||
});
|
||||
expect(patch).toEqual({ priority: "high", target_date: "2026-10-01" });
|
||||
expect(dropped).toEqual([]);
|
||||
});
|
||||
|
||||
it("drops every field outside the whitelist", () => {
|
||||
const { patch, dropped } = pickClientCardPatch({
|
||||
priority: "low",
|
||||
column: "col_done",
|
||||
title: "hacked",
|
||||
accepted: true,
|
||||
archived: true,
|
||||
scope_rule: "stock",
|
||||
});
|
||||
expect(patch).toEqual({ priority: "low" });
|
||||
expect(dropped.sort()).toEqual(
|
||||
["accepted", "archived", "column", "scope_rule", "title"].sort()
|
||||
);
|
||||
});
|
||||
|
||||
it("drops a priority outside the allowed set", () => {
|
||||
const { patch, dropped } = pickClientCardPatch({ priority: "urgent" });
|
||||
expect(patch).toEqual({});
|
||||
expect(dropped).toEqual(["priority"]);
|
||||
});
|
||||
|
||||
it("accepts null target_date as clearing the date", () => {
|
||||
const { patch } = pickClientCardPatch({ target_date: null });
|
||||
expect(patch).toEqual({ target_date: "" });
|
||||
});
|
||||
|
||||
it("drops a malformed target_date", () => {
|
||||
const { patch, dropped } = pickClientCardPatch({ target_date: "next tuesday" });
|
||||
expect(patch).toEqual({});
|
||||
expect(dropped).toEqual(["target_date"]);
|
||||
});
|
||||
|
||||
it("returns an empty patch for a non-object body", () => {
|
||||
expect(pickClientCardPatch(null).patch).toEqual({});
|
||||
expect(pickClientCardPatch("nope").patch).toEqual({});
|
||||
expect(pickClientCardPatch(42).patch).toEqual({});
|
||||
});
|
||||
|
||||
it("exposes exactly two editable fields", () => {
|
||||
expect([...CLIENT_EDITABLE_CARD_FIELDS]).toEqual(["priority", "target_date"]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("pickClientCardCreate", () => {
|
||||
it("accepts title and description only", () => {
|
||||
const { input, dropped } = pickClientCardCreate({
|
||||
title: "Add a POS system",
|
||||
description: "We need card payments at the counter.",
|
||||
});
|
||||
expect(input).toEqual({
|
||||
title: "Add a POS system",
|
||||
description: "We need card payments at the counter.",
|
||||
});
|
||||
expect(dropped).toEqual([]);
|
||||
});
|
||||
|
||||
it("drops column, accepted and priority on create", () => {
|
||||
const { input, dropped } = pickClientCardCreate({
|
||||
title: "x",
|
||||
column: "col_inprogress",
|
||||
accepted: true,
|
||||
priority: "critical",
|
||||
});
|
||||
expect(input).toEqual({ title: "x", description: "" });
|
||||
expect(dropped.sort()).toEqual(["accepted", "column", "priority"].sort());
|
||||
});
|
||||
|
||||
it("rejects a missing or blank title", () => {
|
||||
expect(pickClientCardCreate({ description: "no title" }).input).toBeNull();
|
||||
expect(pickClientCardCreate({ title: " " }).input).toBeNull();
|
||||
});
|
||||
|
||||
it("trims the title and caps it at 200 characters", () => {
|
||||
const { input } = pickClientCardCreate({ title: ` ${"a".repeat(250)} ` });
|
||||
expect(input!.title).toHaveLength(200);
|
||||
});
|
||||
});
|
||||
125
src/lib/cards/client-patch.ts
Normal file
125
src/lib/cards/client-patch.ts
Normal file
|
|
@ -0,0 +1,125 @@
|
|||
/**
|
||||
* The field whitelist for client-manager writes.
|
||||
*
|
||||
* This module is the entire reason /api/client/* exists. PocketBase API rules
|
||||
* are record-level: there is no rule that says "this user may update priority
|
||||
* but not column". So a client manager is denied all writes at the database,
|
||||
* and their requests pass through here instead, where only known-good fields
|
||||
* survive.
|
||||
*
|
||||
* The whitelist is positive, never a blocklist. A new card field added in a
|
||||
* later phase is inaccessible to clients by default, which is the failure
|
||||
* direction we want.
|
||||
*/
|
||||
|
||||
import type { Priority } from "@/lib/pb/types";
|
||||
|
||||
export const CLIENT_EDITABLE_CARD_FIELDS = ["priority", "target_date"] as const;
|
||||
|
||||
const PRIORITIES: readonly Priority[] = ["critical", "high", "medium", "low"];
|
||||
|
||||
/** PocketBase stores dates as strings; "" means unset. */
|
||||
const ISO_DATE = /^\d{4}-\d{2}-\d{2}$/;
|
||||
|
||||
export interface ClientCardPatch {
|
||||
priority?: Priority;
|
||||
target_date?: string;
|
||||
}
|
||||
|
||||
export interface ClientCardCreate {
|
||||
title: string;
|
||||
description: string;
|
||||
}
|
||||
|
||||
function asRecord(body: unknown): Record<string, unknown> | null {
|
||||
if (typeof body !== "object" || body === null || Array.isArray(body)) {
|
||||
return null;
|
||||
}
|
||||
return body as Record<string, unknown>;
|
||||
}
|
||||
|
||||
/**
|
||||
* Split an untrusted PATCH body into the fields we will apply and the names
|
||||
* of everything we refused. Dropped names are returned so the caller can log
|
||||
* them — a client repeatedly sending `column` is worth noticing.
|
||||
*/
|
||||
export function pickClientCardPatch(body: unknown): {
|
||||
patch: ClientCardPatch;
|
||||
dropped: string[];
|
||||
} {
|
||||
const record = asRecord(body);
|
||||
if (!record) return { patch: {}, dropped: [] };
|
||||
|
||||
const patch: ClientCardPatch = {};
|
||||
const dropped: string[] = [];
|
||||
|
||||
for (const [key, value] of Object.entries(record)) {
|
||||
if (key === "priority") {
|
||||
if (typeof value === "string" && PRIORITIES.includes(value as Priority)) {
|
||||
patch.priority = value as Priority;
|
||||
} else {
|
||||
dropped.push(key);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
if (key === "target_date") {
|
||||
if (value === null || value === "") {
|
||||
patch.target_date = "";
|
||||
} else if (typeof value === "string" && ISO_DATE.test(value)) {
|
||||
patch.target_date = value;
|
||||
} else {
|
||||
dropped.push(key);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
dropped.push(key);
|
||||
}
|
||||
|
||||
return { patch, dropped };
|
||||
}
|
||||
|
||||
/**
|
||||
* Split an untrusted create body the same way.
|
||||
*
|
||||
* Note what is absent: no column, no priority, no accepted. The route handler
|
||||
* sets those itself — Inbox, medium, false — so a client cannot create a card
|
||||
* straight into In Progress at critical priority.
|
||||
*
|
||||
* `input` is null when the body has no usable title.
|
||||
*/
|
||||
export function pickClientCardCreate(body: unknown): {
|
||||
input: ClientCardCreate | null;
|
||||
dropped: string[];
|
||||
} {
|
||||
const record = asRecord(body);
|
||||
if (!record) return { input: null, dropped: [] };
|
||||
|
||||
const dropped: string[] = [];
|
||||
let title = "";
|
||||
let description = "";
|
||||
|
||||
for (const [key, value] of Object.entries(record)) {
|
||||
if (key === "title") {
|
||||
if (typeof value === "string") title = value.trim().slice(0, 200);
|
||||
else dropped.push(key);
|
||||
continue;
|
||||
}
|
||||
|
||||
if (key === "description") {
|
||||
if (typeof value === "string") description = value.slice(0, 50000);
|
||||
else dropped.push(key);
|
||||
continue;
|
||||
}
|
||||
|
||||
// `project` is read from the body by the route handler before calling
|
||||
// this, so it is expected here and is not a violation.
|
||||
if (key === "project") continue;
|
||||
|
||||
dropped.push(key);
|
||||
}
|
||||
|
||||
if (!title) return { input: null, dropped };
|
||||
return { input: { title, description }, dropped };
|
||||
}
|
||||
Loading…
Reference in a new issue