Skip to content

Commit ed4461d

Browse files
committed
fix(webapp): carry the pages a switch can still open
A custom dashboard is looked up by its friendly id scoped to the organization rather than the environment, so every environment of the project opens the same dashboard at the same address. Truncating `dashboards/custom/<id>` to the dashboard list on an environment switch lost the user's place for no reason; it now travels like the slug-addressed pages, under the same guard on the trailing segment, while a project or organization switch still truncates it. Integrations is gated on the caller's role rather than on an organization feature flag, and a role differs between organizations, so carrying that page across an organization switch could land on the permission panel where the switch used to land on Tasks. It joins the pages an organization switch drops, and the manifest-derived check now reads a role gate on a loader as well as a feature flag, so a future one cannot slip in unnoticed. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 00174eb commit ed4461d

2 files changed

Lines changed: 132 additions & 15 deletions

File tree

apps/webapp/app/utils/pageSwitching.test.ts

Lines changed: 104 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import {
88
ENVIRONMENT_MATCH_ID,
99
ENVIRONMENT_PORTABLE_PAGES,
1010
environmentPortablePage,
11+
ORGANIZATION_ADDRESSED_PAGES,
1112
ORGANIZATION_PORTABLE_PAGES,
1213
ORGANIZATION_SPECIFIC_PAGES,
1314
organizationPortablePage,
@@ -56,17 +57,38 @@ const GATE_REJECTS_VIA_FLAG = new RegExp(
5657
String.raw`const canAccess = await ${ORGANIZATION_GATE};\s*if \(!canAccess\) \{\s*throw`
5758
);
5859

60+
// A role the caller holds in one organization but not the next: an `authorization` block on the
61+
// loader, or the denial thrown directly for a check the block cannot express.
62+
const GATE_REJECTS_ON_ROLE = /throwPermissionDenied\(|authorization: \{/;
63+
64+
/** The loader's half of a route module, so a gate on its action is not read as a gate on landing. */
65+
function loaderSource(source: string): string {
66+
const start = source.search(/^export (?:const|async function) loader\b/m);
67+
if (start < 0) return "";
68+
69+
const loaderOnwards = source.slice(start);
70+
const next = loaderOnwards.slice(1).search(/^export (?:const|async function|default)/m);
71+
72+
return next < 0 ? loaderOnwards : loaderOnwards.slice(0, next + 1);
73+
}
74+
5975
/**
60-
* The page a loader turns you away from when an organization-scoped check says no, whether it
61-
* sends you home or 404s. A gate that only covers one value of a route param names that page; an
62-
* unconditional gate on a route that takes a resource id names nothing, since a page with an id in
63-
* it is never portable anyway.
76+
* The page a loader turns you away from when a check the organization answers says no, whether it
77+
* sends you home, 404s or renders the permission panel. A gate that only covers one value of a
78+
* route param names that page; a gate on a route that takes a resource id names nothing, since a
79+
* page with an id in it is never portable anyway.
6480
*/
6581
function organizationGatedPage(suffix: string, file: string): string | undefined {
6682
const source = readFileSync(join(APP_DIR, file), "utf8");
6783
const guarded = GATE_REJECTS.exec(source);
6884

69-
if (guarded === null) return GATE_REJECTS_VIA_FLAG.test(source) ? suffix : undefined;
85+
if (guarded === null) {
86+
if (GATE_REJECTS_VIA_FLAG.test(source)) return suffix;
87+
88+
return GATE_REJECTS_ON_ROLE.test(loaderSource(source)) && !suffix.includes(":")
89+
? suffix
90+
: undefined;
91+
}
7092

7193
const [, param, key] = guarded;
7294
if (param === undefined) return suffix.includes(":") ? undefined : suffix;
@@ -80,11 +102,19 @@ const belowEnvironment = Object.values(compiledRoutes)
80102
const suffix = compiledUrl(route.id).slice(ENVIRONMENT_URL.length).replace(/^\//, "");
81103
return {
82104
suffix,
105+
file: route.file,
83106
rendersAPage: rendersAPage(route.file),
84107
organizationGatedPage: organizationGatedPage(suffix, route.file),
85108
};
86109
});
87110

111+
function sourceOf(suffix: string): string {
112+
const route = belowEnvironment.find((route) => route.suffix === suffix);
113+
if (!route) throw new Error(`no route below the environment at ${suffix}`);
114+
115+
return readFileSync(join(APP_DIR, route.file), "utf8");
116+
}
117+
88118
const environmentRoutes = [...new Set(belowEnvironment.map((route) => route.suffix))];
89119

90120
// Streams and Slack callbacks sit below an environment without being pages a user lands on.
@@ -102,7 +132,11 @@ function listAbove(page: string): string {
102132
}
103133

104134
const slugAddressedProbes = probes.filter((page) => SLUG_ADDRESSED_PAGES.includes(listAbove(page)));
105-
const idAddressedProbes = probes.filter((page) => !SLUG_ADDRESSED_PAGES.includes(listAbove(page)));
135+
const organizationAddressedProbes = probes.filter((page) =>
136+
ORGANIZATION_ADDRESSED_PAGES.includes(listAbove(page))
137+
);
138+
const environmentKeptProbes = [...slugAddressedProbes, ...organizationAddressedProbes];
139+
const idAddressedProbes = probes.filter((page) => !environmentKeptProbes.includes(page));
106140

107141
function matchesARoute(page: string): boolean {
108142
const wanted = page === "" ? [] : page.split("/");
@@ -257,16 +291,30 @@ describe("pages an organization switch cannot carry", () => {
257291
expect(gated).toEqual([...ORGANIZATION_SPECIFIC_PAGES].sort());
258292
});
259293

260-
it("are read from both ways a loader turns you away, so neither stops being noticed", () => {
294+
it("are read from every way a loader turns you away, so none stops being noticed", () => {
261295
const gatedPage = (suffix: string) =>
262296
belowEnvironment.find((route) => route.suffix === suffix)?.organizationGatedPage;
263297

264298
expect(gatedPage("logs")).toBe("logs");
265299
expect(gatedPage("dashboards/:dashboardKey")).toBe("dashboards/queues");
300+
expect(gatedPage("settings/integrations")).toBe("settings/integrations");
301+
expect(gatedPage("bulk-actions/:bulkActionParam")).toBeUndefined();
266302
expect(gatedPage("queues/:queueParam")).toBeUndefined();
267303
expect(gatedPage("apikeys")).toBeUndefined();
268304
});
269305

306+
it("read a role gate off the loader, not off an action the page never runs on landing", () => {
307+
const gatedAction = [
308+
"export const loader = dashboardLoader({ params: Schema }, async () => {});",
309+
"",
310+
'export const action = dashboardAction({ authorization: { action: "write" } }, async () => {});',
311+
].join("\n");
312+
313+
expect(GATE_REJECTS_ON_ROLE.test(gatedAction)).toBe(true);
314+
expect(GATE_REJECTS_ON_ROLE.test(loaderSource(gatedAction))).toBe(false);
315+
expect(GATE_REJECTS_ON_ROLE.test(loaderSource(sourceOf("settings/integrations")))).toBe(true);
316+
});
317+
270318
it("still travel with an environment or project switch, which stay in the same organization", () => {
271319
for (const page of ORGANIZATION_SPECIFIC_PAGES) {
272320
expect(ENVIRONMENT_PORTABLE_PAGES.has(page)).toBe(true);
@@ -277,7 +325,16 @@ describe("pages an organization switch cannot carry", () => {
277325
}
278326
});
279327

280-
it("stay put when only the environment changes", () => {
328+
it("stay put when only the environment or project changes", () => {
329+
expect(projectPortablePage("settings/integrations")).toBe("settings/integrations");
330+
expect(
331+
pathForEnvironmentSwitch({
332+
location: locationOn("settings/integrations"),
333+
environmentPathname: environmentLocation.pathname,
334+
environmentSlug: "prod",
335+
})
336+
).toBe("/orgs/acme/projects/api/env/prod/settings/integrations");
337+
281338
expect(
282339
pathForEnvironmentSwitch({
283340
location: locationOn("dashboards/queues", "?period=1d"),
@@ -311,6 +368,7 @@ describe("pages an organization switch cannot carry", () => {
311368
expect(read("?page=logs")).toBe("");
312369
expect(read("?page=query")).toBe("");
313370
expect(read("?page=dashboards/queues")).toBe("dashboards");
371+
expect(read("?page=settings/integrations")).toBe("settings");
314372
expect(read("?page=apikeys")).toBe("apikeys");
315373
});
316374
});
@@ -422,6 +480,43 @@ describe("pages named after something the environment did not issue", () => {
422480
});
423481
});
424482

483+
describe("pages named after an id the organization issued", () => {
484+
it("are the ones a route below them takes an id its organization, not its environment, holds", () => {
485+
expect(organizationAddressedProbes.length).toBeGreaterThan(0);
486+
expect([...new Set(organizationAddressedProbes.map(listAbove))].sort()).toEqual(
487+
[...ORGANIZATION_ADDRESSED_PAGES].sort()
488+
);
489+
490+
for (const page of organizationAddressedProbes) {
491+
expect(environmentPortablePage(page)).toBe(page);
492+
}
493+
});
494+
495+
it("stay open when only the environment changes, since the same id opens them there", () => {
496+
expect(
497+
pathForEnvironmentSwitch({
498+
location: locationOn("dashboards/custom/dashboard_123", "?period=1d"),
499+
environmentPathname: environmentLocation.pathname,
500+
environmentSlug: "prod",
501+
})
502+
).toBe("/orgs/acme/projects/api/env/prod/dashboards/custom/dashboard_123?period=1d");
503+
});
504+
505+
it("fall back to the dashboard list when the project or organization changes", () => {
506+
expect(projectPortablePage("dashboards/custom/dashboard_123")).toBe("dashboards");
507+
expect(organizationPortablePage("dashboards/custom/dashboard_123")).toBe("dashboards");
508+
});
509+
510+
it("keep nothing but a single plain id in that last segment", () => {
511+
expect(environmentPortablePage("dashboards/custom/..%2f..%2flogin")).toBe("dashboards");
512+
expect(environmentPortablePage("dashboards/custom/../../login")).toBe("dashboards");
513+
expect(environmentPortablePage("dashboards/custom/%2e%2e")).toBe("dashboards");
514+
expect(environmentPortablePage("dashboards/custom/..")).toBe("dashboards");
515+
expect(environmentPortablePage("dashboards/custom/")).toBe("dashboards");
516+
expect(environmentPortablePage("dashboards/custom/dashboard_123/extra")).toBe("dashboards");
517+
});
518+
});
519+
425520
describe("a page suffix that is not a plain relative page", () => {
426521
it("falls back to the environment root rather than being sanitised into one", () => {
427522
expect(projectPortablePage("/apikeys")).toBe("");
@@ -458,6 +553,7 @@ describe("a page suffix that is not a plain relative page", () => {
458553
"tasks/standard/../../login",
459554
"agents/..%2f..%2flogin",
460555
"models/%2f%2fevil.example.com",
556+
"dashboards/custom/..%2f..%2flogin",
461557
];
462558

463559
for (const attempt of attempts) {

apps/webapp/app/utils/pageSwitching.ts

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,16 @@ const NESTED_PORTABLE_PAGES = [
2323
/** The branch lists render under any environment of their project, but not in every project. */
2424
export const PROJECT_SPECIFIC_PAGES = ["branches", "dev-branches"];
2525

26-
/** Gated by an organization feature flag, so their loaders turn you away in an organization without it. */
27-
export const ORGANIZATION_SPECIFIC_PAGES = ["logs", "query", "dashboards/queues"];
26+
/**
27+
* Gated on the organization — by a feature flag, or by the role the caller holds there — so their
28+
* loaders turn you away in an organization that answers differently.
29+
*/
30+
export const ORGANIZATION_SPECIFIC_PAGES = [
31+
"logs",
32+
"query",
33+
"dashboards/queues",
34+
"settings/integrations",
35+
];
2836

2937
/**
3038
* Pages whose last segment is a name the user's code or the model catalog decides, rather than an
@@ -41,6 +49,13 @@ export const SLUG_ADDRESSED_PAGES = [
4149
"test/tasks",
4250
];
4351

52+
/**
53+
* Pages whose last segment is an id the organization issued rather than one environment, so the
54+
* same address names the same resource in every environment of the project. Another organization
55+
* never issued that id, so only an environment switch carries it.
56+
*/
57+
export const ORGANIZATION_ADDRESSED_PAGES = ["dashboards/custom"];
58+
4459
/** Every page below an environment that names no resource, so any environment can render it. */
4560
export const ENVIRONMENT_PORTABLE_PAGES: ReadonlySet<string> = new Set(
4661
[
@@ -76,12 +91,18 @@ function nearestPage(suffix: string, pages: ReadonlySet<string>): string {
7691
}
7792

7893
/**
79-
* `suffix` itself when it is a slug-addressed page, as long as the slug is a single plain segment —
80-
* a traversal or an encoded path in its place falls through to the list page above it.
94+
* `suffix` itself when its last segment names the same thing in every environment, as long as that
95+
* segment is a single plain one — a traversal or an encoded path in its place falls through to the
96+
* list page above it.
8197
*/
82-
function slugAddressedPage(suffix: string): string | undefined {
98+
function environmentNeutralPage(suffix: string): string | undefined {
8399
const boundary = suffix.lastIndexOf("/");
84-
if (boundary < 1 || !SLUG_ADDRESSED_PAGES.includes(suffix.slice(0, boundary))) return undefined;
100+
if (boundary < 1) return undefined;
101+
102+
const list = suffix.slice(0, boundary);
103+
if (!SLUG_ADDRESSED_PAGES.includes(list) && !ORGANIZATION_ADDRESSED_PAGES.includes(list)) {
104+
return undefined;
105+
}
85106

86107
let slug: string;
87108
try {
@@ -95,7 +116,7 @@ function slugAddressedPage(suffix: string): string | undefined {
95116

96117
/** The page to keep when only the environment changes. */
97118
export function environmentPortablePage(suffix: string): string {
98-
return slugAddressedPage(suffix) ?? nearestPage(suffix, ENVIRONMENT_PORTABLE_PAGES);
119+
return environmentNeutralPage(suffix) ?? nearestPage(suffix, ENVIRONMENT_PORTABLE_PAGES);
99120
}
100121

101122
/** The page to keep when the project changes. */

0 commit comments

Comments
 (0)