Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Large diffs are not rendered by default.

94 changes: 93 additions & 1 deletion workers/admin-panel/src/diff.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,98 @@ describe('diffDocuments', () => {
{ routes: [{ id: 'a', timeoutMs: 8000 }] },
);
expect(entries).toHaveLength(1);
expect(entries[0]).toMatchObject({ kind: 'changed', path: 'routes.a.timeoutMs' });
expect(entries[0]).toMatchObject({
kind: 'changed',
path: 'routes.a.timeoutMs',
routeId: 'a',
field: 'timeoutMs',
});
expect(entries[0]?.risk).toBeUndefined();
});

it('names the owning route even when the id contains dots', () => {
const entries = diffDocuments(
{ routes: [{ id: 'foo.bar', timeoutMs: 1000 }] },
{ routes: [{ id: 'foo.bar', timeoutMs: 2000 }] },
);
// `routes.foo.bar.timeoutMs` is unparseable without the id list; the entry
// carries both halves so no reader has to guess where the id ends.
expect(entries[0]).toMatchObject({ routeId: 'foo.bar', field: 'timeoutMs' });
});

it('stamps the danger classification the publish gate would give the `to` side', () => {
const flipped = diffDocuments(
{ routes: [{ id: 'a', allowPrivateUpstream: false }] },
{ routes: [{ id: 'a', allowPrivateUpstream: true }] },
);
expect(flipped[0]?.risk).toMatchObject({ path: 'allowPrivateUpstream', level: 'high' });

// `allowPrivateUpstream` is a presence rule, so the row stays flagged even
// turning it back off: on a history surface "somebody touched this switch"
// is the thing worth seeing, and the publish dialog says the same.
const restored = diffDocuments(
{ routes: [{ id: 'a', allowPrivateUpstream: true }] },
{ routes: [{ id: 'a', allowPrivateUpstream: false }] },
);
expect(restored[0]?.risk).toMatchObject({ path: 'allowPrivateUpstream' });

// A guarded rule only fires on the state that qualifies, so the safe
// direction leaves the row plain — again matching the publish gate.
const disarmed = diffDocuments(
{ routes: [{ id: 'a', mirror: { includeBody: true } }] },
{ routes: [{ id: 'a', mirror: { includeBody: false } }] },
);
expect(disarmed[0]?.risk).toBeUndefined();
});

it('lets a nested field inherit the risk its ancestor names', () => {
const entries = diffDocuments(
{ routes: [{ id: 'a', bodyRewrite: { inject: { head: '<b>' } } }] },
{ routes: [{ id: 'a', bodyRewrite: { inject: { head: '<script>' } } }] },
);
expect(entries[0]).toMatchObject({ field: 'bodyRewrite.inject.head' });
expect(entries[0]?.risk).toMatchObject({ path: 'bodyRewrite.inject' });
});

it('counts the dangerous switches an added route arrives with', () => {
const entries = diffDocuments(
{ routes: [] },
{
routes: [
{
id: 'a',
allowPrivateUpstream: true,
upstreamHeaders: { authorization: 'x' },
timeoutMs: 1000,
},
],
},
);
expect(entries[0]).toMatchObject({ kind: 'added', routeId: 'a', riskCount: 2 });
// The high-level rule is the one worth naming on a single row.
expect(entries[0]?.risk).toMatchObject({ level: 'high' });
});

it('classifies defaults with the same rules — they fill the same fields', () => {
const entries = diffDocuments(
{ defaults: { allowPrivateUpstream: false }, routes: [{ id: 'a' }] },
{ defaults: { allowPrivateUpstream: true }, routes: [{ id: 'a' }] },
);
expect(entries[0]).toMatchObject({
path: 'defaults.allowPrivateUpstream',
field: 'allowPrivateUpstream',
});
expect(entries[0]?.routeId).toBeUndefined();
expect(entries[0]?.risk).toMatchObject({ level: 'high' });
});

it('leaves `version` unowned — it is not a route and not a default', () => {
const entries = diffDocuments(
{ version: 1, routes: [{ id: 'a' }] },
{ version: 2, routes: [{ id: 'a' }] },
);
expect(entries[0]).toMatchObject({ path: 'version', kind: 'changed' });
expect(entries[0]?.routeId).toBeUndefined();
expect(entries[0]?.field).toBeUndefined();
});
});
132 changes: 124 additions & 8 deletions workers/admin-panel/src/diff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,15 @@
* arrays are compared whole, except route arrays, which match by `id` so a
* route that moved reports as `moved` rather than as a rewrite of every later
* element.
*
* Every entry that belongs to a route or to `defaults` is stamped with its
* owner (`routeId`), its in-definition `field`, and — when the `to` state trips
* a rule — the `dangerFlags` classification. The panel's danger vocabulary is
* therefore delivered with the diff instead of re-derived from path strings in
* the browser: one classifier, one answer, and it is the classifier the publish
* gate already runs.
*/
import { dangerFlags, type FieldRisk } from './danger.js';
import { canonicalize } from './fingerprint.js';

export interface DiffEntry {
Expand All @@ -25,6 +33,30 @@ export interface DiffEntry {
/** 0-based positions for `moved` entries. */
readonly fromPosition?: number;
readonly toPosition?: number;
/**
* Owning route id, resolved here rather than parsed from `path`: a route id
* may contain dots, so `routes.foo.bar` is ambiguous to every reader except
* the one holding the id list. Absent for `defaults` and `version`.
*/
readonly routeId?: string;
/**
* Path inside the route definition (or inside `defaults`) — the same shape
* `dangerFlags` classifies. Absent for whole-route and top-level entries.
*/
readonly field?: string;
/**
* Danger classification of the `to` state, from the same `dangerFlags` the
* publish gate runs. Sent with the diff so the history view never has to
* re-derive "is this field dangerous" — one classifier, one answer.
*
* The rule's own shape decides what that answer means: presence rules
* (`allowPrivateUpstream`) flag a field the publish touched at all, guarded
* rules (`mirror.includeBody`) only flag the state that qualifies. Both are
* what the publish dialog would have said about the same document.
*/
readonly risk?: FieldRisk;
/** For an added route: how many dangerous switches the new definition carries. */
readonly riskCount?: number;
}

/**
Expand All @@ -43,6 +75,65 @@ const routeIdOf = (route: unknown): string | undefined =>
? ((route as Record<string, unknown>)['id'] as string)
: undefined;

/**
* Danger rules that fire on one side's definition, keyed by field path.
*
* `cors.origins (absent)` is reported by `dangerFlags` under a path that names
* its own absence; the diff keys by the field the operator sees, so the suffix
* is stripped here and nowhere else.
*/
const riskIndex = (definition: unknown): ReadonlyMap<string, FieldRisk> => {
const index = new Map<string, FieldRisk>();
if (typeof definition !== 'object' || definition === null || Array.isArray(definition)) {
return index;
}
for (const risk of dangerFlags(definition as Record<string, unknown>)) {
index.set(risk.path.replace(' (absent)', ''), risk);
}
return index;
};

/** The risk a field inherits: itself, or the closest ancestor a rule names. */
const riskFor = (index: ReadonlyMap<string, FieldRisk>, field: string): FieldRisk | undefined => {
let probe = field;
for (;;) {
const hit = index.get(probe);
if (hit !== undefined) {
return hit;
}
const cut = probe.lastIndexOf('.');
if (cut < 0) {
return undefined;
}
probe = probe.slice(0, cut);
}
};

/**
* Stamps ownership and risk onto entries a generic walker produced.
*
* `prefix` is the path the walker was seeded with, so `field` is what remains
* after it — never a re-parse of the id, which may itself contain dots.
*/
const attribute = (
entries: readonly DiffEntry[],
prefix: string,
index: ReadonlyMap<string, FieldRisk>,
routeId?: string,
): DiffEntry[] =>
entries.map((entry) => {
const field = entry.path.startsWith(`${prefix}.`)
? entry.path.slice(prefix.length + 1)
: undefined;
const risk = field === undefined ? undefined : riskFor(index, field);
return {
...entry,
...(routeId === undefined ? {} : { routeId }),
...(field === undefined ? {} : { field }),
...(risk === undefined ? {} : { risk }),
};
});

/** Recursively compares two JSON values; leaf differences yield `changed`. */
const diffValue = (path: string, from: unknown, to: unknown, out: DiffEntry[]): void => {
if (sameValue(from, to)) {
Expand Down Expand Up @@ -72,7 +163,9 @@ const diffValue = (path: string, from: unknown, to: unknown, out: DiffEntry[]):
}
};

/** Diff for the table-wide defaults block; plain recursive key comparison. */
/** Diff for the table-wide defaults block; plain recursive key comparison.
* Defaults are definition-shaped and fill per-field gaps, so a dangerous
* switch is exactly as dangerous here as inside a route — same classifier. */
const diffDefaults = (from: unknown, to: unknown, out: DiffEntry[]): void => {
const f =
typeof from === 'object' && from !== null && !Array.isArray(from)
Expand All @@ -90,13 +183,15 @@ const diffDefaults = (from: unknown, to: unknown, out: DiffEntry[]): void => {
out.push({ path: 'defaults', kind: 'changed', from, to });
return;
}
diffValue('defaults', f, tt, out);
const local: DiffEntry[] = [];
diffValue('defaults', f, tt, local);
out.push(...attribute(local, 'defaults', riskIndex(tt)));
};

/** Diffs the routes array by `id`, the merge key the proxy itself resolves by.
* (A route id may itself contain dots — `routes.foo.bar` is route `foo.bar`'s
* subtree, not a nested path under route `foo`; clients match on the id after
* the first segment, the same way the compiler keyed the document.) */
* Each entry carries its `routeId` and in-route `field` explicitly, because a
* route id may itself contain dots — `routes.foo.bar` is unparseable without
* the id list, and only this function holds it. */
const diffRoutes = (
fromRoutes: readonly unknown[],
toRoutes: readonly unknown[],
Expand All @@ -120,7 +215,13 @@ const diffRoutes = (
for (const [id, entry] of fromById) {
const target = toById.get(id);
if (target === undefined) {
out.push({ path: `routes.${id}`, kind: 'removed', from: entry.route, to: undefined });
out.push({
path: `routes.${id}`,
kind: 'removed',
from: entry.route,
to: undefined,
routeId: id,
});
continue;
}
if (sameValue(entry.route, target.route)) {
Expand All @@ -132,26 +233,41 @@ const diffRoutes = (
kind: 'moved',
fromPosition: entry.position,
toPosition: target.position,
routeId: id,
});
}
continue;
}
// Content changed and it may also have moved. diffValue reports the field
// changes; the moved entry is emitted alongside so "this route was also
// re-ordered" is never lost behind the field diff.
diffValue(`routes.${id}`, entry.route, target.route, out);
const local: DiffEntry[] = [];
diffValue(`routes.${id}`, entry.route, target.route, local);
out.push(...attribute(local, `routes.${id}`, riskIndex(target.route), id));
if (entry.position !== target.position) {
out.push({
path: `routes.${id}`,
kind: 'moved',
fromPosition: entry.position,
toPosition: target.position,
routeId: id,
});
}
}
for (const [id, entry] of toById) {
if (!fromById.has(id)) {
out.push({ path: `routes.${id}`, kind: 'added', from: undefined, to: entry.route });
// A whole new route: its dangerous switches are the operator's business
// even though no single field row exists to hang them on.
const risks = [...riskIndex(entry.route).values()];
const worst = risks.find((risk) => risk.level === 'high') ?? risks[0];
out.push({
path: `routes.${id}`,
kind: 'added',
from: undefined,
to: entry.route,
routeId: id,
...(worst === undefined ? {} : { risk: worst, riskCount: risks.length }),
});
}
}
};
Expand Down
43 changes: 27 additions & 16 deletions workers/admin-panel/src/history.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@
beforeEach(async () => {
await applyD1Migrations(testEnv.DB, TEST_MIGRATIONS);
for (const table of ['audit_log', 'routes', 'settings', 'users', 'revisions']) {
await testEnv.DB.prepare(`DELETE FROM ${table}`).run();

Check warning on line 121 in workers/admin-panel/src/history.test.ts

View workflow job for this annotation

GitHub Actions / check

eslint(no-await-in-loop)

workers/admin-panel/src/history.test.ts:121:7: Unexpected `await` inside a loop.
}
await testEnv.CONFIG_KV.delete('routes');
__resetConfigCache();
Expand Down Expand Up @@ -181,22 +181,33 @@
expect(res.status).toBe(200);
const body = await res.json();
const changed = body.entries.filter((e: any) => e.kind === 'changed');
expect(changed).toContainEqual({
path: 'routes.alpha.upstream',
kind: 'changed',
from: 'a.internal.example.com',
to: 'a2.internal.example.com',
});
expect(body.entries.filter((e: any) => e.kind === 'added')).toContainEqual({
path: 'routes.beta',
kind: 'added',
to: {
id: 'beta',
match: { host: 'b.example.com', path: '/' },
upstream: 'b.internal.example.com',
timeoutMs: 5000,
},
});
// Ownership travels with the entry: the client never parses `path`, because
// a route id may itself contain dots.
expect(changed).toContainEqual(
expect.objectContaining({
path: 'routes.alpha.upstream',
kind: 'changed',
from: 'a.internal.example.com',
to: 'a2.internal.example.com',
routeId: 'alpha',
field: 'upstream',
}),
);
expect(body.entries.filter((e: any) => e.kind === 'added')).toContainEqual(
expect.objectContaining({
path: 'routes.beta',
kind: 'added',
routeId: 'beta',
to: {
id: 'beta',
match: { host: 'b.example.com', path: '/' },
upstream: 'b.internal.example.com',
timeoutMs: 5000,
},
}),
);
// Nothing here trips a danger rule, so no entry carries a classification.
expect(body.entries.every((e: any) => e.risk === undefined)).toBe(true);

// Reverse direction is free — the rollback dialog asks exactly this.
const back = await get('/api/revisions/diff?from=3&to=1', auth);
Expand Down Expand Up @@ -441,8 +452,8 @@
const auth = await signInAdmin();
// KEEP_REVISIONS is 50; 52 publishes must leave 3..52.
for (let i = 1; i <= 52; i += 1) {
await putRoute(auth, 'churn', routeFor('c.example.com', `c${i}.internal.example.com`));

Check warning on line 455 in workers/admin-panel/src/history.test.ts

View workflow job for this annotation

GitHub Actions / check

eslint(no-await-in-loop)

workers/admin-panel/src/history.test.ts:455:7: Unexpected `await` inside a loop.
const res = await publish(auth);

Check warning on line 456 in workers/admin-panel/src/history.test.ts

View workflow job for this annotation

GitHub Actions / check

eslint(no-await-in-loop)

workers/admin-panel/src/history.test.ts:456:19: Unexpected `await` inside a loop.
expect(res.status).toBe(200);
}
const body = await (await get('/api/revisions?limit=200', auth)).json();
Expand Down
7 changes: 2 additions & 5 deletions workers/admin-panel/web/src/components/publish-dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { Switch } from '@/components/ui/switch';
import { Textarea } from '@/components/ui/textarea';
import { ApiError, NetworkError, api, type PreviewResult } from '@/lib/api';
import { t } from '@/lib/messages';
import { DANGER_REASONS, LIMITS, type FieldRisk } from '@/lib/types';
import { LIMITS, dangerReason, type FieldRisk } from '@/lib/types';

/**
* 发布弹窗 —— 整个面板里唯一一处真正改变线上流量的按钮。
Expand All @@ -37,9 +37,6 @@ interface PublishDialogProps {
readonly onPublished: (revision: number) => void;
}

/** 服务端的 reason 是英文;DANGER_REASONS 是面板自己的说法,优先用它。 */
const reasonOf = (risk: FieldRisk): string => DANGER_REASONS[risk.path] ?? risk.reason;

/**
* 409 响应体里的 dangers 是未知形状(来自服务端,且版本可能不同)。这里只做
* 轻量守卫,不合格的条目丢掉 —— 宁可少列一行也不把 undefined 渲染出来。
Expand Down Expand Up @@ -179,7 +176,7 @@ export const PublishDialog = ({ open, onOpenChange, preview, onPublished }: Publ
<code className="font-mono text-xs">
{routeId}.{risk.path}
</code>
<span className="text-muted-foreground text-xs">{reasonOf(risk)}</span>
<span className="text-muted-foreground text-xs">{dangerReason(risk)}</span>
</li>
)),
)}
Expand Down
Loading
Loading