fix(housekeeping): make route matching deterministic
This commit is contained in:
1 parent
e5c230ba35
commit
0117b45d74
4 files changed
+256
-45
No files matched your search
@@ -138,3 +138,97 @@ src/features/housekeeping/route-handlers.ts
|
|||||||
- Mutation check: dynamic-name normalization removal, regex-style static matching, static-priority removal, decoded-separator acceptance, domain mismatch acceptance, skipped route ACL, context reload inside the handler, missing handler lookup, placeholder handler addition, and empty-domain redirect each break a focused test.
|
- Mutation check: dynamic-name normalization removal, regex-style static matching, static-priority removal, decoded-separator acceptance, domain mismatch acceptance, skipped route ACL, context reload inside the handler, missing handler lookup, placeholder handler addition, and empty-domain redirect each break a focused test.
|
||||||
- The catch-all route imports only housekeeping foundation/manifests/handlers and retains the existing forbidden database/auth/permissions/actions/legacy-page boundary audit.
|
- The catch-all route imports only housekeeping foundation/manifests/handlers and retains the existing forbidden database/auth/permissions/actions/legacy-page boundary audit.
|
||||||
- `apply_patch` created all new files, but the Windows sandbox helper repeatedly failed to read existing files with `apply deny-read ACLs`. Existing-file edits therefore used controller-approved exact-anchor/full-file fallbacks only after resolving absolute paths and validating every target under `E:\Users\simol\Desktop\EpicNext-cms`.
|
- `apply_patch` created all new files, but the Windows sandbox helper repeatedly failed to read existing files with `apply deny-read ACLs`. Existing-file edits therefore used controller-approved exact-anchor/full-file fallbacks only after resolving absolute paths and validating every target under `E:\Users\simol\Desktop\EpicNext-cms`.
|
||||||
|
|
||||||
|
## Fix Round 1 — deterministic encoded route matching
|
||||||
|
|
||||||
|
### Status and scope
|
||||||
|
|
||||||
|
- Fix base: `e5c230ba35aec2b4c471608d32b8a16ecc1ee382`.
|
||||||
|
- Only the two Important matcher blockers were addressed.
|
||||||
|
- The three parked Minor findings remain unchanged; no changed line required an adjustment to them.
|
||||||
|
- Commit message: `fix(housekeeping): make route matching deterministic`.
|
||||||
|
- `.remember/` remained untouched. No worktree, push, PR/MR, database operation, deployment, or Task 10 work was performed.
|
||||||
|
|
||||||
|
### Root-cause evidence
|
||||||
|
|
||||||
|
1. The catch-all receives decoded Next segments and re-encodes them with `encodeURIComponent`. The matcher decoded the request path into `decodedSegments` but compared static route text against `rawSegments`. Therefore literal `a+b[1]` did not equal `a%2Bb%5B1%5D`; the competing `:id` route captured the request and could change the selected capability/handler.
|
||||||
|
2. Candidate sorting used only total dynamic-segment count. Intersecting patterns `/:kind/settings` and `/users/:id` have the same count, so stable sort preserved manifest order and allowed registration order to decide dispatch.
|
||||||
|
|
||||||
|
### RED
|
||||||
|
|
||||||
|
Exact command after test setup was validated:
|
||||||
|
|
||||||
|
```text
|
||||||
|
pnpm exec vitest run --coverage.enabled=false src/features/housekeeping/foundation/routing/match-route.test.ts src/features/housekeeping/foundation/registry.test.ts src/features/housekeeping/route-handlers.test.ts src/features/housekeeping/foundation/preview-route-contract.test.ts
|
||||||
|
```
|
||||||
|
|
||||||
|
Observed exit 1:
|
||||||
|
|
||||||
|
```text
|
||||||
|
Test Files 2 failed | 2 passed (4)
|
||||||
|
Tests 2 failed | 95 passed (97)
|
||||||
|
|
||||||
|
catch-all encoded literal:
|
||||||
|
Expected routeId people.literal-tool with params {}
|
||||||
|
Received routeId people.tool-detail with params { id: "a+b[1]" }
|
||||||
|
|
||||||
|
equal-count specificity:
|
||||||
|
Expected routeId people.user-detail with params { id: "settings" }
|
||||||
|
Received routeId people.kind-settings with params { kind: "users" }
|
||||||
|
```
|
||||||
|
|
||||||
|
The registration-order table exercises both orders. Before production changes the general-first order failed while the reverse order passed, proving that order was the deciding variable.
|
||||||
|
|
||||||
|
An earlier RED attempt exposed a test-table setup error (`manifest.routes is not iterable`); the table was changed from spread array rows to named `{ routes }` rows, then rerun to obtain the behavioral RED above before any production edit.
|
||||||
|
|
||||||
|
### Fix
|
||||||
|
|
||||||
|
- A single `decodeCanonicalSegment` boundary now normalizes request segments and static route-pattern segments exactly once.
|
||||||
|
- Invalid percent encoding, empty values, decoded `/` or `\`, and decoded `.` / `..` remain fail closed.
|
||||||
|
- Dynamic markers retain their parameter names and receive the already-decoded request segment.
|
||||||
|
- Candidate specificity is compared left-to-right. At the earliest static/dynamic difference, the static segment wins; manifest order no longer selects among intersecting patterns.
|
||||||
|
- Existing static-over-dynamic behavior and registry duplicate-shape rejection remain unchanged.
|
||||||
|
|
||||||
|
### GREEN and pre-commit verification
|
||||||
|
|
||||||
|
Targeted command above, exit 0:
|
||||||
|
|
||||||
|
```text
|
||||||
|
Test Files 4 passed (4)
|
||||||
|
Tests 97 passed (97)
|
||||||
|
```
|
||||||
|
|
||||||
|
Full housekeeping suite, exit 0:
|
||||||
|
|
||||||
|
```text
|
||||||
|
pnpm test:housekeeping
|
||||||
|
Test Files 38 passed (38)
|
||||||
|
Tests 352 passed (352)
|
||||||
|
```
|
||||||
|
|
||||||
|
TypeScript, exit 0:
|
||||||
|
|
||||||
|
```text
|
||||||
|
pnpm typecheck
|
||||||
|
$ tsc --noEmit
|
||||||
|
```
|
||||||
|
|
||||||
|
The only output note remained the existing Node warning: local `26.7.0`, package request `>=26.8.1 <27`.
|
||||||
|
|
||||||
|
Exact changed-file Biome, exit 0:
|
||||||
|
|
||||||
|
```text
|
||||||
|
pnpm exec biome check --formatter-enabled=false src/features/housekeeping/foundation/routing/match-route.ts src/features/housekeeping/foundation/routing/match-route.test.ts src/features/housekeeping/foundation/preview-route-contract.test.ts
|
||||||
|
Checked 3 files in 25ms. No fixes applied.
|
||||||
|
```
|
||||||
|
|
||||||
|
`git diff --check` exited 0 before the report update. Cached and committed-tree checks are final staging/commit gates.
|
||||||
|
|
||||||
|
### Exact fix files
|
||||||
|
|
||||||
|
```text
|
||||||
|
src/features/housekeeping/foundation/routing/match-route.ts
|
||||||
|
src/features/housekeeping/foundation/routing/match-route.test.ts
|
||||||
|
src/features/housekeeping/foundation/preview-route-contract.test.ts
|
||||||
|
.superpowers/sdd/2026-08-26-housekeeping-completion/task-9-report.md
|
||||||
|
```
|
||||||
@@ -121,6 +121,24 @@ vi.mock("@/features/housekeeping/manifests", async (importOriginal) => {
|
|||||||
slugs: ["admin.bans.view"],
|
slugs: ["admin.bans.view"],
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
|
{
|
||||||
|
id: "people.tool-detail",
|
||||||
|
labelKey: "pages.housekeeping.domains.people.title",
|
||||||
|
href: "/ase/people/tools/:id" as const,
|
||||||
|
capability: {
|
||||||
|
mode: "any" as const,
|
||||||
|
slugs: ["admin.users.view"],
|
||||||
|
},
|
||||||
|
},
|
||||||
|
{
|
||||||
|
id: "people.literal-tool",
|
||||||
|
labelKey: "pages.housekeeping.domains.people.title",
|
||||||
|
href: "/ase/people/tools/a+b[1]" as const,
|
||||||
|
capability: {
|
||||||
|
mode: "any" as const,
|
||||||
|
slugs: ["admin.users.view"],
|
||||||
|
},
|
||||||
|
},
|
||||||
],
|
],
|
||||||
}
|
}
|
||||||
: manifest,
|
: manifest,
|
||||||
@@ -135,6 +153,14 @@ vi.mock("@/features/housekeeping/route-handlers", () => ({
|
|||||||
render: routeMocks.renderHousekeepingRoute,
|
render: routeMocks.renderHousekeepingRoute,
|
||||||
},
|
},
|
||||||
{ routeId: "people.bans", render: routeMocks.renderHousekeepingRoute },
|
{ routeId: "people.bans", render: routeMocks.renderHousekeepingRoute },
|
||||||
|
{
|
||||||
|
routeId: "people.tool-detail",
|
||||||
|
render: routeMocks.renderHousekeepingRoute,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
routeId: "people.literal-tool",
|
||||||
|
render: routeMocks.renderHousekeepingRoute,
|
||||||
|
},
|
||||||
],
|
],
|
||||||
}));
|
}));
|
||||||
|
|
||||||
@@ -631,6 +657,30 @@ describe("/ase-next/[domain]/[[...segments]] page", () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("keeps catch-all encoding from changing the literal route handler", async () => {
|
||||||
|
const context = capabilityContext([PERMS.USERS_VIEW]);
|
||||||
|
routeMocks.getHousekeepingCapabilityContext.mockResolvedValue(context);
|
||||||
|
|
||||||
|
await renderRoute(
|
||||||
|
AdminNextDomainPage({
|
||||||
|
params: Promise.resolve({
|
||||||
|
domain: "people",
|
||||||
|
segments: ["tools", "a+b[1]"],
|
||||||
|
}),
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(routeMocks.renderHousekeepingRoute).toHaveBeenCalledWith({
|
||||||
|
context,
|
||||||
|
match: {
|
||||||
|
routeId: "people.literal-tool",
|
||||||
|
domain: "people",
|
||||||
|
params: {},
|
||||||
|
canonicalHref: "/ase/people/tools/a%2Bb%5B1%5D",
|
||||||
|
},
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
it("returns 404 for an inaccessible matched route without invoking it", async () => {
|
it("returns 404 for an inaccessible matched route without invoking it", async () => {
|
||||||
routeMocks.getHousekeepingCapabilityContext.mockResolvedValue(
|
routeMocks.getHousekeepingCapabilityContext.mockResolvedValue(
|
||||||
capabilityContext([PERMS.MOD_CFH_VIEW]),
|
capabilityContext([PERMS.MOD_CFH_VIEW]),
|
||||||
|
|||||||
@@ -102,6 +102,35 @@ describe("matchHousekeepingRoute", () => {
|
|||||||
).toBeNull();
|
).toBeNull();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it.each([
|
||||||
|
{
|
||||||
|
routes: [
|
||||||
|
route("people.kind-settings", "/ase/people/:kind/settings"),
|
||||||
|
route("people.user-detail", "/ase/people/users/:id"),
|
||||||
|
],
|
||||||
|
},
|
||||||
|
{
|
||||||
|
routes: [
|
||||||
|
route("people.user-detail", "/ase/people/users/:id"),
|
||||||
|
route("people.kind-settings", "/ase/people/:kind/settings"),
|
||||||
|
],
|
||||||
|
},
|
||||||
|
])(
|
||||||
|
"prefers the earliest static segment regardless of registration order",
|
||||||
|
({ routes }) => {
|
||||||
|
const orderedRegistry = createHousekeepingRegistry([
|
||||||
|
peopleManifest(routes),
|
||||||
|
]);
|
||||||
|
|
||||||
|
expect(
|
||||||
|
matchHousekeepingRoute(orderedRegistry, "/ase/people/users/settings"),
|
||||||
|
)?.toMatchObject({
|
||||||
|
routeId: "people.user-detail",
|
||||||
|
params: { id: "settings" },
|
||||||
|
});
|
||||||
|
},
|
||||||
|
);
|
||||||
|
|
||||||
it.each([
|
it.each([
|
||||||
"/ase/people/unknown",
|
"/ase/people/unknown",
|
||||||
"/ase/economy/users",
|
"/ase/economy/users",
|
||||||
|
|||||||
@@ -10,15 +10,18 @@ export interface HousekeepingRouteMatch {
|
|||||||
}
|
}
|
||||||
|
|
||||||
interface ParsedCanonicalPath {
|
interface ParsedCanonicalPath {
|
||||||
rawSegments: readonly string[];
|
segments: readonly string[];
|
||||||
decodedSegments: readonly string[];
|
}
|
||||||
|
|
||||||
|
interface RoutePatternSegment {
|
||||||
|
literal: string | null;
|
||||||
|
parameterName: string | null;
|
||||||
}
|
}
|
||||||
|
|
||||||
interface RouteCandidate {
|
interface RouteCandidate {
|
||||||
routeId: string;
|
routeId: string;
|
||||||
domain: HousekeepingDomainId;
|
domain: HousekeepingDomainId;
|
||||||
patternSegments: readonly string[];
|
patternSegments: readonly RoutePatternSegment[];
|
||||||
dynamicSegments: number;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
export function matchHousekeepingRoute(
|
export function matchHousekeepingRoute(
|
||||||
@@ -30,24 +33,16 @@ export function matchHousekeepingRoute(
|
|||||||
|
|
||||||
const candidates = registry.domains
|
const candidates = registry.domains
|
||||||
.flatMap((domain) =>
|
.flatMap((domain) =>
|
||||||
domain.routes
|
domain.routes.flatMap((route) => {
|
||||||
.filter((route) =>
|
if (!routeBelongsToDomain(route.href, domain.canonicalHref)) return [];
|
||||||
routeBelongsToDomain(route.href, domain.canonicalHref),
|
|
||||||
)
|
|
||||||
.map((route) => {
|
|
||||||
const patternSegments = route.href.slice(1).split("/");
|
|
||||||
|
|
||||||
return {
|
const patternSegments = parseRoutePattern(route.href);
|
||||||
routeId: route.id,
|
return patternSegments
|
||||||
domain: domain.id,
|
? [{ routeId: route.id, domain: domain.id, patternSegments }]
|
||||||
patternSegments,
|
: [];
|
||||||
dynamicSegments: patternSegments.filter((segment) =>
|
}),
|
||||||
segment.startsWith(":"),
|
|
||||||
).length,
|
|
||||||
};
|
|
||||||
}),
|
|
||||||
)
|
)
|
||||||
.sort((left, right) => left.dynamicSegments - right.dynamicSegments);
|
.sort(compareSpecificity);
|
||||||
|
|
||||||
for (const candidate of candidates) {
|
for (const candidate of candidates) {
|
||||||
const params = matchSegments(candidate, path);
|
const params = matchSegments(candidate, path);
|
||||||
@@ -81,28 +76,71 @@ function parseCanonicalPath(canonicalPath: string): ParsedCanonicalPath | null {
|
|||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
|
||||||
const decodedSegments: string[] = [];
|
const segments: string[] = [];
|
||||||
for (const segment of rawSegments) {
|
for (const segment of rawSegments) {
|
||||||
let decoded: string;
|
const decoded = decodeCanonicalSegment(segment);
|
||||||
try {
|
if (decoded === null) return null;
|
||||||
decoded = decodeURIComponent(segment);
|
segments.push(decoded);
|
||||||
} catch {
|
|
||||||
return null;
|
|
||||||
}
|
|
||||||
|
|
||||||
if (
|
|
||||||
!decoded ||
|
|
||||||
decoded === "." ||
|
|
||||||
decoded === ".." ||
|
|
||||||
decoded.includes("/") ||
|
|
||||||
decoded.includes("\\")
|
|
||||||
) {
|
|
||||||
return null;
|
|
||||||
}
|
|
||||||
decodedSegments.push(decoded);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
return { rawSegments, decodedSegments };
|
return { segments };
|
||||||
|
}
|
||||||
|
|
||||||
|
function parseRoutePattern(
|
||||||
|
routeHref: CanonicalHousekeepingHref,
|
||||||
|
): readonly RoutePatternSegment[] | null {
|
||||||
|
const segments: RoutePatternSegment[] = [];
|
||||||
|
|
||||||
|
for (const segment of routeHref.slice(1).split("/")) {
|
||||||
|
if (segment.startsWith(":")) {
|
||||||
|
const parameterName = segment.slice(1);
|
||||||
|
if (!parameterName) return null;
|
||||||
|
segments.push({ literal: null, parameterName });
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
const literal = decodeCanonicalSegment(segment);
|
||||||
|
if (literal === null) return null;
|
||||||
|
segments.push({ literal, parameterName: null });
|
||||||
|
}
|
||||||
|
|
||||||
|
return segments;
|
||||||
|
}
|
||||||
|
|
||||||
|
function decodeCanonicalSegment(segment: string): string | null {
|
||||||
|
let decoded: string;
|
||||||
|
try {
|
||||||
|
decoded = decodeURIComponent(segment);
|
||||||
|
} catch {
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
|
return !decoded ||
|
||||||
|
decoded === "." ||
|
||||||
|
decoded === ".." ||
|
||||||
|
decoded.includes("/") ||
|
||||||
|
decoded.includes("\\")
|
||||||
|
? null
|
||||||
|
: decoded;
|
||||||
|
}
|
||||||
|
|
||||||
|
function compareSpecificity(
|
||||||
|
left: RouteCandidate,
|
||||||
|
right: RouteCandidate,
|
||||||
|
): number {
|
||||||
|
const sharedLength = Math.min(
|
||||||
|
left.patternSegments.length,
|
||||||
|
right.patternSegments.length,
|
||||||
|
);
|
||||||
|
|
||||||
|
for (let index = 0; index < sharedLength; index += 1) {
|
||||||
|
const leftDynamic = left.patternSegments[index]?.parameterName !== null;
|
||||||
|
const rightDynamic = right.patternSegments[index]?.parameterName !== null;
|
||||||
|
if (leftDynamic === rightDynamic) continue;
|
||||||
|
return leftDynamic ? 1 : -1;
|
||||||
|
}
|
||||||
|
|
||||||
|
return left.patternSegments.length - right.patternSegments.length;
|
||||||
}
|
}
|
||||||
|
|
||||||
function routeBelongsToDomain(
|
function routeBelongsToDomain(
|
||||||
@@ -120,18 +158,18 @@ function matchSegments(
|
|||||||
candidate: RouteCandidate,
|
candidate: RouteCandidate,
|
||||||
path: ParsedCanonicalPath,
|
path: ParsedCanonicalPath,
|
||||||
): Record<string, string> | null {
|
): Record<string, string> | null {
|
||||||
if (candidate.patternSegments.length !== path.rawSegments.length) return null;
|
if (candidate.patternSegments.length !== path.segments.length) return null;
|
||||||
|
|
||||||
const params: Record<string, string> = {};
|
const params: Record<string, string> = {};
|
||||||
for (const [index, patternSegment] of candidate.patternSegments.entries()) {
|
for (const [index, patternSegment] of candidate.patternSegments.entries()) {
|
||||||
if (!patternSegment.startsWith(":")) {
|
const pathSegment = path.segments[index] ?? "";
|
||||||
if (patternSegment !== path.rawSegments[index]) return null;
|
if (patternSegment.parameterName === null) {
|
||||||
|
if (patternSegment.literal !== pathSegment) return null;
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
const parameterName = patternSegment.slice(1);
|
if (patternSegment.parameterName in params) return null;
|
||||||
if (!parameterName || parameterName in params) return null;
|
params[patternSegment.parameterName] = pathSegment;
|
||||||
params[parameterName] = path.decodedSegments[index] ?? "";
|
|
||||||
}
|
}
|
||||||
|
|
||||||
return params;
|
return params;
|
||||||
|
|||||||
Reference in new issue
Block a user