Skip to content

Commit 753d2ad

Browse files
committed
fix(webapp): keep the impersonation deep link and honor the view-as-user toggle
The consent page's form had no `action`, so React Router resolved it to the matched route's `pathnameBase`. Without `future.v3_relativeSplatPath` that excludes the splat, so the form posted to `/@/orgs/<slug>`, the action saw an empty splat, and the admin landed on the organization root instead of the deep link the page had just named. The form now posts to an explicit absolute path. The query string was dropped too. A `/@/runs/<id>` link redirects to a run path carrying `?span=`, which selects the span to open, so both the displayed destination and the final redirect now carry the incoming search. The debug tooltip and the "Debug run" button still checked `!hasAdminAccess && !isImpersonating`, which stays true with the toggle on, so the two most visible admin-only affordances kept rendering. `useHasAdminAccess` already folds in impersonation and the toggle, so the extra check is gone. The replay dialog's region picker was likewise still keyed off raw impersonation and now uses `hasAdminDisplayAccess`. Also: gate the view-as-user route on a same-origin navigation like the other impersonation routes, stop it flat-route-nesting under `resources.impersonation` (the URL is unchanged), and log consent-page entry at info with only the referer's origin, since it is expected for any link opened outside the app.
1 parent 5612c33 commit 753d2ad

9 files changed

Lines changed: 225 additions & 22 deletions

apps/webapp/app/components/admin/debugRun.tsx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
import { useIsImpersonating } from "~/hooks/useOrganizations";
21
import { useHasAdminAccess } from "~/hooks/useUser";
32
import { Button } from "../primitives/Buttons";
43
import { Dialog, DialogContent, DialogHeader, DialogTrigger } from "../primitives/Dialog";
@@ -12,10 +11,11 @@ import * as Property from "~/components/primitives/PropertyTable";
1211
import { ClipboardField } from "../primitives/ClipboardField";
1312

1413
export function AdminDebugRun({ friendlyId }: { friendlyId: string }) {
14+
// `useHasAdminAccess` already folds in impersonation and the "view as user"
15+
// toggle, so this one check is enough.
1516
const hasAdminAccess = useHasAdminAccess();
16-
const isImpersonating = useIsImpersonating();
1717

18-
if (!hasAdminAccess && !isImpersonating) {
18+
if (!hasAdminAccess) {
1919
return null;
2020
}
2121

apps/webapp/app/components/admin/debugTooltip.tsx

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,16 @@ import {
88
TooltipTrigger,
99
} from "~/components/primitives/Tooltip";
1010
import { useOptionalEnvironment } from "~/hooks/useEnvironment";
11-
import { useIsImpersonating, useOptionalOrganization } from "~/hooks/useOrganizations";
11+
import { useOptionalOrganization } from "~/hooks/useOrganizations";
1212
import { useOptionalProject } from "~/hooks/useProject";
1313
import { useHasAdminAccess, useUser } from "~/hooks/useUser";
1414

1515
export function AdminDebugTooltip({ children }: { children?: React.ReactNode }) {
16+
// `useHasAdminAccess` already folds in impersonation and the "view as user"
17+
// toggle, so this one check is enough.
1618
const hasAdminAccess = useHasAdminAccess();
17-
const isImpersonating = useIsImpersonating();
1819

19-
if (!hasAdminAccess && !isImpersonating) {
20+
if (!hasAdminAccess) {
2021
return null;
2122
}
2223

apps/webapp/app/models/admin.server.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
import { authenticator } from "~/services/auth.server";
1212
import { requireUser } from "~/services/session.server";
1313
import { extractClientIp } from "~/utils/extractClientIp.server";
14+
import { impersonationDestinationPath } from "~/utils/pathBuilder";
1415

1516
const pageSize = 20;
1617

@@ -298,6 +299,10 @@ export async function findImpersonationTarget(
298299
* Starts impersonating the organization's first confirmed member and lands on
299300
* the requested path with the `/@` prefix stripped. Shared by the same-origin
300301
* loader path and the consent page's POST so there is one implementation.
302+
*
303+
* The destination keeps the incoming query string: both entry points are served
304+
* at the `/@`-prefixed URL, so `request.url` carries the same search the link
305+
* arrived with (for example the `?span=` a `/@/runs/<id>` link redirects with).
301306
*/
302307
export async function startImpersonation(
303308
request: Request,
@@ -319,7 +324,7 @@ export async function startImpersonation(
319324
return redirectWithImpersonation(
320325
request,
321326
target.userId,
322-
`/orgs/${organizationSlug}/${path}`,
327+
impersonationDestinationPath(organizationSlug, path, new URL(request.url).search),
323328
currentUser,
324329
clients.write
325330
);

apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,10 @@ import {
1414
} from "~/models/admin.server";
1515
import { logger } from "~/services/logger.server";
1616
import { requireUser } from "~/services/session.server";
17+
import {
18+
impersonationConsentPostBackPath,
19+
impersonationDestinationPath,
20+
} from "~/utils/pathBuilder";
1721
import { isSameOriginNavigation } from "~/utils/sameOriginNavigation";
1822

1923
// Everything this route's loader and action touch on the server lives in
@@ -57,25 +61,42 @@ export async function loader({ request, params }: LoaderFunctionArgs) {
5761
throw await startImpersonation(request, organizationSlug, path, user);
5862
}
5963

60-
logger.warn("Cross-site impersonation entry, showing consent page", {
64+
// Expected for any link opened outside the app (address bar, bookmark, a link
65+
// shared elsewhere), so this is routine rather than suspicious. Only the
66+
// referer's origin is logged — the full referer can carry another site's path
67+
// and query string.
68+
logger.info("Impersonation entry outside the app, showing consent page", {
6169
userId: user.id,
6270
organizationSlug,
63-
referer: request.headers.get("referer"),
71+
refererOrigin: refererOrigin(request),
6472
secFetchSite: request.headers.get("sec-fetch-site"),
6573
});
6674

6775
// Read-only on purpose: nothing is written and no impersonation cookie is set
6876
// until the admin confirms with the POST below.
6977
const target = await findImpersonationTarget(organizationSlug);
7078

79+
const search = new URL(request.url).search;
80+
7181
return typedjson({
7282
organizationSlug,
7383
organizationName: target.success ? target.organizationName : undefined,
74-
destinationPath: `/orgs/${organizationSlug}/${path}`,
84+
destinationPath: impersonationDestinationPath(organizationSlug, path, search),
85+
postBackPath: impersonationConsentPostBackPath(organizationSlug, path, search),
7586
canImpersonate: target.success,
7687
});
7788
}
7889

90+
function refererOrigin(request: Request): string | undefined {
91+
const referer = request.headers.get("referer");
92+
if (!referer) return undefined;
93+
try {
94+
return new URL(referer).origin;
95+
} catch {
96+
return undefined;
97+
}
98+
}
99+
79100
export async function action({ request, params }: ActionFunctionArgs) {
80101
if (request.method.toLowerCase() !== "post") {
81102
return new Response("Method not allowed", { status: 405 });
@@ -105,13 +126,14 @@ export async function action({ request, params }: ActionFunctionArgs) {
105126
return clearImpersonation(request, "/admin");
106127
}
107128

108-
// The form has no `action`, so it posts to the current URL and the
109-
// organization slug plus the splat path arrive here unchanged.
129+
// The consent form posts to an explicit absolute path (see
130+
// `impersonationConsentPostBackPath`), so the organization slug, the splat
131+
// path and the query string all arrive here intact.
110132
return startImpersonation(request, organizationSlug, params["*"] ?? "", user);
111133
}
112134

113135
export default function Page() {
114-
const { organizationSlug, organizationName, destinationPath, canImpersonate } =
136+
const { organizationSlug, organizationName, destinationPath, postBackPath, canImpersonate } =
115137
useTypedLoaderData<typeof loader>();
116138

117139
return (
@@ -125,7 +147,7 @@ export default function Page() {
125147
<span className="text-text-bright">{organizationName ?? organizationSlug}</span> and
126148
open <span className="text-text-bright">{destinationPath}</span>.
127149
</Paragraph>
128-
<Form method="post" reloadDocument>
150+
<Form method="post" action={postBackPath} reloadDocument>
129151
<Button type="submit" variant="primary/medium" fullWidth shortcut={{ key: "enter" }}>
130152
Impersonate
131153
</Button>

apps/webapp/app/routes/resources.impersonation.view-as.ts renamed to apps/webapp/app/routes/resources.impersonation_.view-as.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
import { redirect, type ActionFunctionArgs } from "@remix-run/server-runtime";
22
import { z } from "zod";
3+
import { env } from "~/env.server";
34
import { commitImpersonationSession, setViewingAsUser } from "~/services/impersonation.server";
5+
import { logger } from "~/services/logger.server";
46
import { requireUser } from "~/services/session.server";
57
import { sanitizeRedirectPath } from "~/utils";
8+
import { isSameOriginNavigation } from "~/utils/sameOriginNavigation";
69

710
const FormSchema = z.object({
811
viewAsUser: z.enum(["true", "false"]),
@@ -16,6 +19,17 @@ export async function action({ request }: ActionFunctionArgs) {
1619

1720
const user = await requireUser(request);
1821

22+
// The toggle is submitted from our own side menu, so this holds. Applied here
23+
// for the same reason as the other impersonation routes: no other site gets to
24+
// drive this state change.
25+
if (!isSameOriginNavigation(request, env.LOGIN_ORIGIN)) {
26+
logger.warn("Refusing cross-site view-as-user submission", {
27+
userId: user.id,
28+
secFetchSite: request.headers.get("sec-fetch-site"),
29+
});
30+
return redirect("/");
31+
}
32+
1933
const payload = Object.fromEntries(await request.formData());
2034
const parsed = FormSchema.safeParse(payload);
2135
const redirectTo = sanitizeRedirectPath(parsed.success ? parsed.data.redirectTo : undefined);

apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { $replica, prisma } from "~/db.server";
88
import { redirectWithErrorMessage, redirectWithSuccessMessage } from "~/models/message.server";
99
import { displayableEnvironment } from "~/models/runtimeEnvironment.server";
1010
import { logger } from "~/services/logger.server";
11-
import { requireUser } from "~/services/session.server";
11+
import { hasAdminDisplayAccess, requireUser } from "~/services/session.server";
1212
import { dashboardAction } from "~/services/routeBuilders/dashboardBuilder";
1313
import { sortEnvironments } from "~/utils/environmentSort";
1414
import { v3RunSpanPath } from "~/utils/pathBuilder";
@@ -169,7 +169,7 @@ export async function loader({ request, params }: LoaderFunctionArgs) {
169169
new RegionsPresenter().call({
170170
userId,
171171
projectSlug,
172-
isAdmin: user.admin || user.isImpersonating,
172+
isAdmin: hasAdminDisplayAccess(user),
173173
}),
174174
]);
175175

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
import { matchRoutes, resolveTo } from "@remix-run/router";
2+
import { readFileSync } from "node:fs";
3+
import { join } from "node:path";
4+
import { describe, expect, it } from "vitest";
5+
import { impersonationConsentPostBackPath, impersonationDestinationPath } from "./pathBuilder";
6+
7+
// The route that renders the impersonation consent page, as the flat-route
8+
// convention compiles `_app.@.orgs.$organizationSlug.$.tsx`.
9+
const CONSENT_ROUTES = [
10+
{
11+
id: "routes/_app",
12+
children: [
13+
{
14+
id: "routes/_app.@.orgs.$organizationSlug.$",
15+
path: "@/orgs/:organizationSlug/*",
16+
},
17+
],
18+
},
19+
];
20+
21+
/**
22+
* What the consent route's `action` would receive for a POST to `pathname`:
23+
* the organization slug and the splat, straight off the router.
24+
*/
25+
function actionParamsFor(pathname: string) {
26+
const matches = matchRoutes(CONSENT_ROUTES, pathname);
27+
const leaf = matches?.[matches.length - 1];
28+
return {
29+
organizationSlug: leaf?.params.organizationSlug,
30+
splat: leaf?.params["*"],
31+
};
32+
}
33+
34+
/**
35+
* What `<Form method="post">` with NO `action` prop resolves to, reproducing
36+
* `useFormAction()`: `resolveTo(".", …)` over the matched routes' contributing
37+
* pathnames. This app does not enable `future.v3_relativeSplatPath`, so each
38+
* contributing match supplies its `pathnameBase`.
39+
*/
40+
function relativeFormAction(pathname: string) {
41+
const matches = matchRoutes(CONSENT_ROUTES, pathname) ?? [];
42+
const contributing = matches.filter((m, i) => i === 0 || Boolean(m.route.path));
43+
const routePathnames = contributing.map((m) => m.pathnameBase);
44+
return resolveTo(".", routePathnames, pathname, false).pathname;
45+
}
46+
47+
describe("impersonation consent post-back path", () => {
48+
const slug = "acme-inc";
49+
// A `/@/runs/<id>` link redirects here: a deep run path plus the `?span=`
50+
// that selects which span to open.
51+
const splat = "projects/proj_123/env/prod/runs/run_abc";
52+
const search = "?span=span_xyz";
53+
54+
it("keeps the splat and the query string", () => {
55+
expect(impersonationConsentPostBackPath(slug, splat, search)).toBe(
56+
`/@/orgs/${slug}/${splat}${search}`
57+
);
58+
expect(impersonationDestinationPath(slug, splat, search)).toBe(
59+
`/orgs/${slug}/${splat}${search}`
60+
);
61+
});
62+
63+
it("omits the query string when there isn't one", () => {
64+
expect(impersonationConsentPostBackPath(slug, splat)).toBe(`/@/orgs/${slug}/${splat}`);
65+
expect(impersonationDestinationPath(slug, splat)).toBe(`/orgs/${slug}/${splat}`);
66+
});
67+
68+
it("round-trips the slug and splat back to the action", () => {
69+
const postBackPath = impersonationConsentPostBackPath(slug, splat, search);
70+
71+
expect(actionParamsFor(new URL(postBackPath, "http://localhost").pathname)).toEqual({
72+
organizationSlug: slug,
73+
splat,
74+
});
75+
});
76+
77+
it("the destination the action redirects to matches the one the page displayed", () => {
78+
const postBackPath = impersonationConsentPostBackPath(slug, splat, search);
79+
const url = new URL(postBackPath, "http://localhost");
80+
const params = actionParamsFor(url.pathname);
81+
82+
// What `startImpersonation` builds from the action's params + request URL.
83+
expect(impersonationDestinationPath(params.organizationSlug!, params.splat!, url.search)).toBe(
84+
impersonationDestinationPath(slug, splat, search)
85+
);
86+
});
87+
88+
// The regression this file exists for. A relative `<Form>` action drops the
89+
// splat, so the action would start impersonation and land the admin on the
90+
// organization root rather than the deep link the consent page promised.
91+
it("a relative form action would drop the splat, so the path must be explicit", () => {
92+
const consentUrl = `/@/orgs/${slug}/${splat}`;
93+
94+
expect(relativeFormAction(consentUrl)).toBe(`/@/orgs/${slug}`);
95+
expect(actionParamsFor(relativeFormAction(consentUrl))).toEqual({
96+
organizationSlug: slug,
97+
splat: "",
98+
});
99+
100+
// The explicit path does not.
101+
expect(impersonationConsentPostBackPath(slug, splat)).toBe(consentUrl);
102+
});
103+
104+
// Ideally this would render the route and read the emitted `action`
105+
// attribute, but the route module imports `~/env.server`, which validates the
106+
// full server environment at import time and so cannot be pulled into a unit
107+
// test. Asserting on the source is the next best guard: it fails if the
108+
// `action` prop is ever dropped, which is the whole bug.
109+
it("the consent form names its action explicitly", () => {
110+
const source = readFileSync(
111+
join(__dirname, "../routes/_app.@.orgs.$organizationSlug.$.tsx"),
112+
"utf8"
113+
);
114+
115+
const form = source.match(/<Form\b[^>]*>/);
116+
expect(form).not.toBeNull();
117+
expect(form![0]).toMatch(/action=\{postBackPath\}/);
118+
});
119+
});

apps/webapp/app/utils/pathBuilder.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,41 @@ export function impersonate(path: string) {
6262
return `/@${path}`;
6363
}
6464

65+
/**
66+
* Where a `/@/orgs/<slug>/<splat>` impersonation link lands once impersonation
67+
* has started: the same deep link with the `/@` prefix stripped.
68+
*
69+
* `search` must be carried through explicitly. A `/@/runs/<id>` link redirects
70+
* to a `v3RunSpanPath`, whose `?span=<spanId>` selects the span to open, so
71+
* dropping it lands the admin on the run with nothing selected.
72+
*/
73+
export function impersonationDestinationPath(
74+
organizationSlug: string,
75+
splatPath: string,
76+
search: string = ""
77+
) {
78+
return `/orgs/${organizationSlug}/${splatPath}${search}`;
79+
}
80+
81+
/**
82+
* Where the impersonation consent page's form must POST back to.
83+
*
84+
* The form has to name this path explicitly. A `<Form>` with no `action`
85+
* resolves to `useResolvedPath(".")`, and because this app does not enable
86+
* `future.v3_relativeSplatPath`, that resolves to the matched route's
87+
* `pathnameBase` — which excludes the splat. The form would post to
88+
* `/@/orgs/<slug>`, the action would see an empty splat, and the admin would
89+
* land on the organization root instead of the deep link the consent page just
90+
* promised them.
91+
*/
92+
export function impersonationConsentPostBackPath(
93+
organizationSlug: string,
94+
splatPath: string,
95+
search: string = ""
96+
) {
97+
return impersonate(impersonationDestinationPath(organizationSlug, splatPath, search));
98+
}
99+
65100
export function accountPath() {
66101
return `/account`;
67102
}

apps/webapp/test/impersonationConsent.test.ts

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -61,20 +61,27 @@ describe("impersonation consent page", () => {
6161
expect(await prisma.impersonationAuditLog.count()).toBe(0);
6262

6363
// The consent page's POST: same-origin, and it does start impersonation.
64+
// The `?span=` is what a `/@/runs/<id>` link redirects with, so it has to
65+
// survive to the destination or the run opens with no span selected.
6466
const response = await startImpersonation(
65-
new Request(`http://localhost:3030/@/orgs/${org.slug}/projects/p/runs/run_123`, {
66-
method: "POST",
67-
headers: { "sec-fetch-site": "same-origin" },
68-
}),
67+
new Request(
68+
`http://localhost:3030/@/orgs/${org.slug}/projects/p/runs/run_123?span=span_abc`,
69+
{
70+
method: "POST",
71+
headers: { "sec-fetch-site": "same-origin" },
72+
}
73+
),
6974
org.slug,
7075
"projects/p/runs/run_123",
7176
{ id: admin.id, admin: true },
7277
{ read: prisma, write: prisma }
7378
);
7479

7580
expect(response.status).toBe(302);
76-
// The splat path is preserved, with the `/@` prefix stripped.
77-
expect(response.headers.get("location")).toBe(`/orgs/${org.slug}/projects/p/runs/run_123`);
81+
// The splat path and query string are preserved, `/@` prefix stripped.
82+
expect(response.headers.get("location")).toBe(
83+
`/orgs/${org.slug}/projects/p/runs/run_123?span=span_abc`
84+
);
7885
expect(response.headers.get("set-cookie")).toContain("__impersonate=");
7986

8087
const auditLogs = await prisma.impersonationAuditLog.findMany();

0 commit comments

Comments
 (0)