diff --git a/frontend/apps/web/src/pages/CreatePage.tsx b/frontend/apps/web/src/pages/CreatePage.tsx index d115b17..58c95da 100644 --- a/frontend/apps/web/src/pages/CreatePage.tsx +++ b/frontend/apps/web/src/pages/CreatePage.tsx @@ -20,7 +20,7 @@ import { Breadcrumb, Button, Card, EmptyState } from '@dar/ui'; import { FieldInput } from '@dar/form'; import { RecordSkeleton } from '../components/RecordSkeleton'; -import { slugify } from '../slugify'; +import { applyPrepopulate } from './prepopulate'; import { useModelMeta } from '../useModelMeta'; import { useToast } from '../toast'; import { useUnsavedGuard } from '../useUnsavedGuard'; @@ -184,16 +184,13 @@ function CreateForm({ schema, onCreate, onCancel }: CreateFormProps) { function handleFieldChange(name: string, v: WriteValue): void { setValues((prev) => { - const next = { ...prev, [name]: v }; // A direct edit of a target field opts it out of further auto-fill. if (prepopTargets.has(name)) editedTargets.current.add(name); - for (const [target, sources] of Object.entries(prepopulated)) { - if (editedTargets.current.has(target)) continue; - if (!sources.includes(name)) continue; - const joined = sources.map((s) => (next[s] == null ? '' : String(next[s]))).join(' '); - next[target] = slugify(joined); - } - return next; + return applyPrepopulate({ + values: { ...prev, [name]: v }, + prepopulated, + editedTargets: editedTargets.current, + }); }); } diff --git a/frontend/apps/web/src/pages/DetailPage.tsx b/frontend/apps/web/src/pages/DetailPage.tsx index 33afaf7..b378c09 100644 --- a/frontend/apps/web/src/pages/DetailPage.tsx +++ b/frontend/apps/web/src/pages/DetailPage.tsx @@ -38,6 +38,7 @@ import { type InlineWriteItem, type InlineWritePayload, type ObjectActionDescriptor, + type ObjectActionRunResponse, type WriteValue, } from '@dar/data'; import { detailCollapseKey, usePersistedState } from '@dar/customization'; @@ -48,7 +49,7 @@ import { HistoryModal } from '@dar/history'; import { RecordSkeleton } from '../components/RecordSkeleton'; import { useModelMeta } from '../useModelMeta'; -import { useToast } from '../toast'; +import { toastMessages, useToast } from '../toast'; import { carryPreservedFilters, listPathWithPreservedFilters } from '../changelistFilters'; import { useUnsavedGuard } from '../useUnsavedGuard'; @@ -310,13 +311,21 @@ export function DetailPage({ onRun={() => runObjectAction({ client, appLabel, modelName, pk, name: action.name }) } - onSuccess={async (message, redirect) => { + onSuccess={async ({ message, messages, redirect }) => { if (redirect) { navigate(redirect); return; } await refresh(); - toast.success(message || 'Done'); + // Dispatch by Django level tag (#632) — error / + // warning levels must NOT toast green. Falls back to a + // single-line success toast for v1.4.x APIs that don't + // emit `messages[]`. + if (messages && messages.length > 0) { + toastMessages(toast, messages); + } else { + toast.success(message || 'Done'); + } }} onError={(message) => toast.error(message)} /> @@ -444,8 +453,8 @@ function ObjectActionButton({ onError, }: { action: ObjectActionDescriptor; - onRun: () => Promise<{ ok: boolean; message?: string; redirect?: string }>; - onSuccess: (message: string | undefined, redirect: string | undefined) => Promise | void; + onRun: () => Promise; + onSuccess: (result: ObjectActionRunResponse) => Promise | void; onError: (message: string) => void; }) { const [busy, setBusy] = useState(false); @@ -460,7 +469,7 @@ function ObjectActionButton({ try { const result = await onRun(); if (result.ok) { - await onSuccess(result.message, result.redirect); + await onSuccess(result); } else { onError(result.message || 'The action could not be completed.'); } diff --git a/frontend/apps/web/src/pages/prepopulate.test.ts b/frontend/apps/web/src/pages/prepopulate.test.ts new file mode 100644 index 0000000..50a1ceb --- /dev/null +++ b/frontend/apps/web/src/pages/prepopulate.test.ts @@ -0,0 +1,56 @@ +// Lock the prepopulated_fields behaviour (#245 / #629). Verifies: +// 1. typing in a source field slugifies the target on every keystroke +// 2. multiple sources are joined with a space, then slugified together +// 3. once the operator edits a target by hand, further changes to +// its sources DO NOT overwrite — the manual edit wins +// 4. a target with no source values stays an empty string +import { describe, expect, it } from 'vitest'; + +import { applyPrepopulate } from './prepopulate'; + +describe('applyPrepopulate', () => { + const prepopulated = { slug: ['title'] }; + + it('slugifies the target from a single source on first edit', () => { + const out = applyPrepopulate({ + values: { title: 'Hello World' }, + prepopulated, + editedTargets: new Set(), + }); + expect(out.slug).toBe('hello-world'); + }); + + it('joins multiple sources with a space before slugifying', () => { + const out = applyPrepopulate({ + values: { first: 'Acme', second: 'Holdings Inc' }, + prepopulated: { slug: ['first', 'second'] }, + editedTargets: new Set(), + }); + expect(out.slug).toBe('acme-holdings-inc'); + }); + + it('does not overwrite a target the operator has edited by hand', () => { + const out = applyPrepopulate({ + values: { title: 'New Title', slug: 'custom-slug' }, + prepopulated, + editedTargets: new Set(['slug']), + }); + expect(out.slug).toBe('custom-slug'); + }); + + it('leaves the target as an empty string when every source is empty', () => { + const out = applyPrepopulate({ + values: { title: '' }, + prepopulated, + editedTargets: new Set(), + }); + expect(out.slug).toBe(''); + }); + + it('returns a new object — does not mutate the input', () => { + const values = { title: 'X' }; + const out = applyPrepopulate({ values, prepopulated, editedTargets: new Set() }); + expect(out).not.toBe(values); + expect(values).not.toHaveProperty('slug'); + }); +}); diff --git a/frontend/apps/web/src/pages/prepopulate.ts b/frontend/apps/web/src/pages/prepopulate.ts new file mode 100644 index 0000000..c574cea --- /dev/null +++ b/frontend/apps/web/src/pages/prepopulate.ts @@ -0,0 +1,37 @@ +// prepopulated_fields helper (#245 / #629). Pure function so the +// CreatePage form logic is testable without rendering the page — +// CreatePage owns the per-target "user has edited this" set and +// passes it in; this just applies the slugify-from-sources rule +// to every still-untouched target. +// +// Mirrors Django admin's add-form behaviour: typing in a source +// field auto-fills the target until the operator edits the target +// by hand, after which auto-fill stops for that record. +import type { WriteValue } from '@dar/data'; + +import { slugify } from '../slugify'; + +export interface PrepopulateArgs { + /** Current draft values (the function is non-mutating — returns + * a NEW object with the updated targets). */ + values: Record; + /** From `schema.prepopulated_fields`: `{target: [...sourceFields]}`. */ + prepopulated: Record; + /** Target field names the operator has edited directly — these + * are SKIPPED so a manual edit isn't overwritten. */ + editedTargets: ReadonlySet; +} + +export function applyPrepopulate({ + values, + prepopulated, + editedTargets, +}: PrepopulateArgs): Record { + const next = { ...values }; + for (const [target, sources] of Object.entries(prepopulated)) { + if (editedTargets.has(target)) continue; + const joined = sources.map((s) => (next[s] == null ? '' : String(next[s]))).join(' '); + next[target] = slugify(joined); + } + return next; +} diff --git a/frontend/apps/web/src/toast.test.ts b/frontend/apps/web/src/toast.test.ts new file mode 100644 index 0000000..9671504 --- /dev/null +++ b/frontend/apps/web/src/toast.test.ts @@ -0,0 +1,67 @@ +// Lock the level → toast-method dispatch (#632). The detail page +// previously toasted success-green for every action, including +// `message_user(..., level=messages.ERROR)`, because the legacy +// `runObjectAction` adapter dropped the level field. This test +// pins the per-level routing so a regression turns red, not silent. +import { describe, expect, it, vi } from 'vitest'; + +import { toastMessages } from './toast'; + +function makeToast() { + return { + success: vi.fn<(message: string) => void>(), + error: vi.fn<(message: string) => void>(), + info: vi.fn<(message: string) => void>(), + }; +} + +describe('toastMessages', () => { + it('routes `error` to toast.error (red)', () => { + const t = makeToast(); + toastMessages(t, [{ level: 'error', message: 'boom' }]); + expect(t.error).toHaveBeenCalledWith('boom'); + expect(t.success).not.toHaveBeenCalled(); + expect(t.info).not.toHaveBeenCalled(); + }); + + it('routes `warning` to toast.error (red — closest available)', () => { + const t = makeToast(); + toastMessages(t, [{ level: 'warning', message: 'careful' }]); + expect(t.error).toHaveBeenCalledWith('careful'); + }); + + it('routes `info` to toast.info (blue)', () => { + const t = makeToast(); + toastMessages(t, [{ level: 'info', message: 'fyi' }]); + expect(t.info).toHaveBeenCalledWith('fyi'); + }); + + it('routes `debug` to toast.info (blue)', () => { + const t = makeToast(); + toastMessages(t, [{ level: 'debug', message: 'trace' }]); + expect(t.info).toHaveBeenCalledWith('trace'); + }); + + it('routes `success` (and unknown levels) to toast.success (green)', () => { + const t = makeToast(); + toastMessages(t, [ + { level: 'success', message: 'ok' }, + { level: 'unknown', message: 'mystery' }, + ]); + expect(t.success).toHaveBeenCalledWith('ok'); + expect(t.success).toHaveBeenCalledWith('mystery'); + }); + + it('dispatches every entry in order — no merging, no dedupe', () => { + const t = makeToast(); + toastMessages(t, [ + { level: 'error', message: 'first' }, + { level: 'success', message: 'second' }, + { level: 'error', message: 'third' }, + ]); + expect(t.error).toHaveBeenCalledTimes(2); + expect(t.error).toHaveBeenNthCalledWith(1, 'first'); + expect(t.error).toHaveBeenNthCalledWith(2, 'third'); + expect(t.success).toHaveBeenCalledWith('second'); + }); +}); diff --git a/frontend/apps/web/src/toast.tsx b/frontend/apps/web/src/toast.tsx index b5560b0..3a5994b 100644 --- a/frontend/apps/web/src/toast.tsx +++ b/frontend/apps/web/src/toast.tsx @@ -32,6 +32,27 @@ export function useToast(): ToastApi { return ctx; } +/** + * Dispatch a `message_user`-emitted message list to the toast API, + * picking the colour per Django's level tag (#632 — list AND detail + * page now share this; the detail page previously dropped the level + * via the legacy `runObjectAction` adapter and toasted success-green + * for every action even when the level was `error` / `warning`). + * + * Mirrors the legacy admin: `error` / `warning` → red, `info` / + * `debug` → blue, `success` (and any unknown level) → green. + */ +export function toastMessages( + toast: ToastApi, + messages: ReadonlyArray<{ level: string; message: string }>, +): void { + for (const m of messages) { + if (m.level === 'error' || m.level === 'warning') toast.error(m.message); + else if (m.level === 'info' || m.level === 'debug') toast.info(m.message); + else toast.success(m.message); + } +} + export function ToastProvider({ children }: { children: ReactNode }) { const [toasts, setToasts] = useState([]); const idRef = useRef(0); diff --git a/frontend/packages/api/src/contract.ts b/frontend/packages/api/src/contract.ts index 116cd01..0bba0a9 100644 --- a/frontend/packages/api/src/contract.ts +++ b/frontend/packages/api/src/contract.ts @@ -319,7 +319,13 @@ export type ObjectActionDescriptor = ActionDescriptor; */ export interface ObjectActionRunResponse { ok: boolean; + /** Convenience: first `messages[]` entry's text — kept for back-compat + * with v1.4.x consumers that only ever toasted one line. */ message?: string; + /** Every `message_user` line with its Django level tag (#632 — needed + * so the SPA can pick red / amber / blue / green per level instead of + * always toasting green on the detail page). */ + messages?: UserMessage[]; redirect?: string; } diff --git a/frontend/packages/data/src/mutations.ts b/frontend/packages/data/src/mutations.ts index f07fcd8..1e13bf8 100644 --- a/frontend/packages/data/src/mutations.ts +++ b/frontend/packages/data/src/mutations.ts @@ -89,6 +89,11 @@ export async function runObjectAction( return { ok: res.executed, ...(message !== undefined ? { message } : {}), + // Propagate the full level-tagged messages list (#632) so the + // detail page can pick the toast colour per level — the legacy + // single-`message` field collapses every message into one string + // with no level information. + ...(res.messages !== undefined ? { messages: res.messages } : {}), ...(res.redirect !== undefined ? { redirect: res.redirect } : {}), }; } diff --git a/pyproject.toml b/pyproject.toml index 70b77ad..6287331 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "django-admin-react" -version = "1.4.11" +version = "1.4.12" description = "A drop-in React single-page admin for Django, driven entirely by ModelAdmin." authors = ["django-admin-react contributors"] license = "MIT" diff --git a/tests/test_login_next_redirect.py b/tests/test_login_next_redirect.py new file mode 100644 index 0000000..4ea6c60 --- /dev/null +++ b/tests/test_login_next_redirect.py @@ -0,0 +1,66 @@ +"""Lock the ``?next=`` open-redirect protection on ``DarLoginView`` (#636). + +``DarLoginView`` subclasses ``django.contrib.auth.views.LoginView``, +which defaults ``success_url_allowed_hosts = set()`` and rejects any +``next`` query value that points off-host. The risk: a subclass that +overrides ``get_success_url`` / ``form_valid`` without preserving the +allow-list check would silently re-introduce an open redirect — every +"please log in, then return to " mail-out would become a phishing +weapon. + +This test posts a login with ``?next=https://evil.example.com/`` and +asserts the post-login redirect target is NOT off-host. If a future +change overrides the redirect helpers without the safe-host gate, +this test fails red and the regression doesn't ship. +""" + +from __future__ import annotations + +from urllib.parse import urlparse + +import pytest +from django.contrib.auth import get_user_model +from django.test import Client + +LOGIN_URL = "/admin-react/login/" + + +@pytest.fixture +def staff_user(db): + u = get_user_model().objects.create_user(username="alice", password="pw12345!") + u.is_staff = True + u.is_active = True + u.save() + return u + + +def test_next_param_rejects_external_host(staff_user): + """An off-host ``next`` value MUST NOT survive into the redirect target.""" + c = Client() + response = c.post( + f"{LOGIN_URL}?next=https://evil.example.com/phish", + data={"username": "alice", "password": "pw12345!"}, + ) + # Successful login should redirect to the safe default, not off-host. + assert response.status_code == 302, "Login POST should 302 on success." + target = response.headers["Location"] + parsed = urlparse(target) + # An empty netloc means same-host (a path-only redirect like `/admin/`). + # Any non-empty netloc must NOT be the attacker's host. + assert parsed.netloc in ("", "testserver"), ( + f"Login redirected to off-host URL {target!r} — open redirect." + ) + + +def test_next_param_accepts_same_host_path(staff_user): + """A same-host ``next`` path IS honoured (the legitimate use case).""" + c = Client() + response = c.post( + f"{LOGIN_URL}?next=/admin-react/auth/user/", + data={"username": "alice", "password": "pw12345!"}, + ) + assert response.status_code == 302 + target = response.headers["Location"] + parsed = urlparse(target) + assert parsed.netloc in ("", "testserver") + assert parsed.path == "/admin-react/auth/user/"