fix(avatars): every signed-in account sees other users' pictures
System Health shows every user's name and avatarAt to everyone, but the picture route GET /api/users/:id/avatar sat under the staff-only /users prefix, so a developer got 403 EADMIN and saw only initials. authGuard now lets any signed-in account make that one read (EVERYONE_READS); every other /users path stays staff-only, and the test checks both.
This commit is contained in:
@@ -2,7 +2,7 @@
|
|||||||
* @file UserAvatar.tsx
|
* @file UserAvatar.tsx
|
||||||
* @description An account's round picture: the signed-in account's own
|
* @description An account's round picture: the signed-in account's own
|
||||||
* (`GET /api/me/avatar`) or, with `userId`, another's (`GET
|
* (`GET /api/me/avatar`) or, with `userId`, another's (`GET
|
||||||
* /api/users/:id/avatar`, staff only), versioned by `avatarAt`; without one —
|
* /api/users/:id/avatar`, any signed-in account), versioned by `avatarAt`; without one —
|
||||||
* or when the image fails to load — the first letters of its name. Used by the
|
* or when the image fails to load — the first letters of its name. Used by the
|
||||||
* sidebar's account row, its workspace owner groups, Settings → Account, the
|
* sidebar's account row, its workspace owner groups, Settings → Account, the
|
||||||
* Users page and its editor, the pool board and member picker, and System
|
* Users page and its editor, the pool board and member picker, and System
|
||||||
@@ -23,7 +23,7 @@ export function UserAvatar({
|
|||||||
}: {
|
}: {
|
||||||
name: string;
|
name: string;
|
||||||
avatarAt?: string | null;
|
avatarAt?: string | null;
|
||||||
/** Another account's (staff only); omitted = the signed-in account's own. */
|
/** Another account's; omitted = the signed-in account's own. */
|
||||||
userId?: number;
|
userId?: number;
|
||||||
/** How many letters of the name stand in for a missing picture. */
|
/** How many letters of the name stand in for a missing picture. */
|
||||||
letters?: number;
|
letters?: number;
|
||||||
|
|||||||
@@ -1568,7 +1568,7 @@ export const api = {
|
|||||||
},
|
},
|
||||||
// ───────────────────────── Server mode staff views (routes/users.js) ─────────────────────────
|
// ───────────────────────── Server mode staff views (routes/users.js) ─────────────────────────
|
||||||
users: {
|
users: {
|
||||||
/** URL of one account's avatar (admin/analyst), versioned by `avatarAt` (null = none). */
|
/** URL of one account's avatar (any signed-in account), versioned by `avatarAt` (null = none). */
|
||||||
avatarUrl: (id: number, avatarAt?: string | null) =>
|
avatarUrl: (id: number, avatarAt?: string | null) =>
|
||||||
avatarAt ? `${BASE}/users/${id}/avatar?v=${encodeURIComponent(avatarAt)}` : null,
|
avatarAt ? `${BASE}/users/${id}/avatar?v=${encodeURIComponent(avatarAt)}` : null,
|
||||||
/** GET /api/users — every account with its recent usage (admin/analyst). */
|
/** GET /api/users — every account with its recent usage (admin/analyst). */
|
||||||
|
|||||||
@@ -4,8 +4,9 @@
|
|||||||
* server/lib/avatars.js): an account sets, reads and removes its own picture;
|
* server/lib/avatars.js): an account sets, reads and removes its own picture;
|
||||||
* only PNG/JPEG/GIF/WebP by magic bytes (an SVG is refused), at most 1 MB,
|
* only PNG/JPEG/GIF/WebP by magic bytes (an SVG is refused), at most 1 MB,
|
||||||
* served with `nosniff` and a `sandbox` CSP; `avatarAt` reaches `/api/auth/me`;
|
* served with `nosniff` and a `sandbox` CSP; `avatarAt` reaches `/api/auth/me`;
|
||||||
* another account never sees it through `/api/me`; staff read it at
|
* another account never sees it through `/api/me`; every account reads it at
|
||||||
* `/api/users/:id/avatar` (a developer is refused) and a workspace carries
|
* `/api/users/:id/avatar` (a developer too — only the picture, the rest of
|
||||||
|
* `/users` stays staff-only) and a workspace carries
|
||||||
* `owner_avatar_at` for the sidebar's owner groups. A picture from a link
|
* `owner_avatar_at` for the sidebar's owner groups. A picture from a link
|
||||||
* (`POST /api/me/avatar/url`, server/lib/avatar-url.js) is stored, never
|
* (`POST /api/me/avatar/url`, server/lib/avatar-url.js) is stored, never
|
||||||
* downloaded, and the avatar routes redirect to it; https only, never a
|
* downloaded, and the avatar routes redirect to it; https only, never a
|
||||||
@@ -196,7 +197,7 @@ it("a picture from a link: https only, never a private or local address", async
|
|||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
it("staff read another account's avatar; workspaces carry owner_avatar_at", async () => {
|
it("every account reads another's avatar; workspaces carry owner_avatar_at", async () => {
|
||||||
const ava = await login("ava", "ava-pass-1");
|
const ava = await login("ava", "ava-pass-1");
|
||||||
const ann = await login("ann", "ann-pass-1");
|
const ann = await login("ann", "ann-pass-1");
|
||||||
const bob = await login("bob", "bob-pass-1");
|
const bob = await login("bob", "bob-pass-1");
|
||||||
@@ -210,8 +211,12 @@ it("staff read another account's avatar; workspaces carry owner_avatar_at", asyn
|
|||||||
assert.deepEqual(Buffer.from(await got.arrayBuffer()), PNG);
|
assert.deepEqual(Buffer.from(await got.arrayBuffer()), PNG);
|
||||||
assert.equal(
|
assert.equal(
|
||||||
(await fetch(`${BASE}/api/users/${id}/avatar`, { headers: { cookie: bob } })).status,
|
(await fetch(`${BASE}/api/users/${id}/avatar`, { headers: { cookie: bob } })).status,
|
||||||
403
|
200
|
||||||
);
|
);
|
||||||
|
// Only the picture: the rest of /users stays staff-only for a developer.
|
||||||
|
for (const path of [`/api/users/${id}`, `/api/users/${id}/details`, "/api/users"]) {
|
||||||
|
assert.equal((await fetch(BASE + path, { headers: { cookie: bob } })).status, 403, path);
|
||||||
|
}
|
||||||
const lanes = (await (await fetch(BASE + "/api/lanes", { headers: { cookie: ann } })).json())
|
const lanes = (await (await fetch(BASE + "/api/lanes", { headers: { cookie: ann } })).json())
|
||||||
.lanes;
|
.lanes;
|
||||||
assert.ok(lanes.find((l) => l.cwd === "/w/ava").owner_avatar_at);
|
assert.ok(lanes.find((l) => l.cwd === "/w/ava").owner_avatar_at);
|
||||||
|
|||||||
@@ -128,6 +128,11 @@ const STAFF_ONLY_PREFIXES = [
|
|||||||
// Staff-only paths with an id in the middle, matched on `apiPath()`'s
|
// Staff-only paths with an id in the middle, matched on `apiPath()`'s
|
||||||
// lower-cased form: the host's own AI-tool config files (they may hold keys).
|
// lower-cased form: the host's own AI-tool config files (they may hold keys).
|
||||||
const STAFF_ONLY_PATTERNS = [/^\/ai-tools\/[^/]+\/config(\/|$)/];
|
const STAFF_ONLY_PATTERNS = [/^\/ai-tools\/[^/]+\/config(\/|$)/];
|
||||||
|
// The one read under a staff-only prefix every signed-in user may make: an
|
||||||
|
// account's PICTURE (`GET /users/:id/avatar` — the image or a redirect to its
|
||||||
|
// link, nothing else of the account). System Health shows every user's name and
|
||||||
|
// `avatarAt` to everyone, so a developer refused here saw only initials.
|
||||||
|
const EVERYONE_READS = [/^\/users\/\d+\/avatar$/];
|
||||||
|
|
||||||
const isServerMode = () => process.env.AGENTHUB_MODE === "server";
|
const isServerMode = () => process.env.AGENTHUB_MODE === "server";
|
||||||
|
|
||||||
@@ -789,6 +794,7 @@ function authGuard(req, res, next) {
|
|||||||
const isRead = req.method === "GET" || req.method === "HEAD";
|
const isRead = req.method === "GET" || req.method === "HEAD";
|
||||||
if (
|
if (
|
||||||
user.role === "developer" &&
|
user.role === "developer" &&
|
||||||
|
!(isRead && EVERYONE_READS.some((re) => re.test(lp))) &&
|
||||||
(STAFF_ONLY_PREFIXES.some(under) || STAFF_ONLY_PATTERNS.some((re) => re.test(lp)))
|
(STAFF_ONLY_PREFIXES.some(under) || STAFF_ONLY_PATTERNS.some((re) => re.test(lp)))
|
||||||
) {
|
) {
|
||||||
return deny(res, 403, "EADMIN", "only an admin or analyst may open this");
|
return deny(res, 403, "EADMIN", "only an admin or analyst may open this");
|
||||||
|
|||||||
@@ -21,7 +21,7 @@
|
|||||||
* - POST /api/me/avatar/url (Auth) — the picture as a link, redirected to
|
* - POST /api/me/avatar/url (Auth) — the picture as a link, redirected to
|
||||||
* - GET/PATCH /api/users/{id} (Auth) — the Users page's Edit user form (its `quota` too)
|
* - GET/PATCH /api/users/{id} (Auth) — the Users page's Edit user form (its `quota` too)
|
||||||
* - PUT /api/users/quota-default, GET /api/me/quota (Auth) — spend quotas per user
|
* - PUT /api/users/quota-default, GET /api/me/quota (Auth) — spend quotas per user
|
||||||
* - GET /api/users/{id}/avatar (Auth) — one account's picture (admin/analyst)
|
* - GET /api/users/{id}/avatar (Auth) — one account's picture (every account)
|
||||||
* - GET /api/users/tmux, POST /api/users/{id}/tmux-restart, POST /api/users/tmux-restart-idle
|
* - GET /api/users/tmux, POST /api/users/{id}/tmux-restart, POST /api/users/tmux-restart-idle
|
||||||
* (Auth) — each member's own tmux server and an admin's restart of it (S3)
|
* (Auth) — each member's own tmux server and an admin's restart of it (S3)
|
||||||
* Exports `{ tags, schemas, paths }`, combined into the spec by
|
* Exports `{ tags, schemas, paths }`, combined into the spec by
|
||||||
@@ -731,12 +731,11 @@ const paths = {
|
|||||||
get: {
|
get: {
|
||||||
tags: ["Auth"],
|
tags: ["Auth"],
|
||||||
operationId: "getUserAvatar",
|
operationId: "getUserAvatar",
|
||||||
summary: "One account's picture, for the sidebar's owner groups (admin/analyst)",
|
summary: "One account's picture, for owner groups and System Health (every signed-in account)",
|
||||||
parameters: [{ name: "id", in: "path", required: true, schema: { type: "integer" } }],
|
parameters: [{ name: "id", in: "path", required: true, schema: { type: "integer" } }],
|
||||||
responses: {
|
responses: {
|
||||||
200: { description: "The image (PNG, JPEG, GIF or WebP)" },
|
200: { description: "The image (PNG, JPEG, GIF or WebP)" },
|
||||||
302: { description: "Redirect to that account's avatar link" },
|
302: { description: "Redirect to that account's avatar link" },
|
||||||
403: err("EADMIN — a developer"),
|
|
||||||
404: err("ENOAVATAR"),
|
404: err("ENOAVATAR"),
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
|
|||||||
@@ -23,7 +23,8 @@
|
|||||||
* tool the accounts it may run on (pin, group, quota, its own 5 h / 7 d share
|
* tool the accounts it may run on (pin, group, quota, its own 5 h / 7 d share
|
||||||
* of each — `accountUsage` with `ownerId`) and the ones it actually ran on;
|
* of each — `accountUsage` with `ownerId`) and the ones it actually ran on;
|
||||||
* `GET /:id/avatar` its picture (server/lib/avatars.js), for the sidebar's
|
* `GET /:id/avatar` its picture (server/lib/avatars.js), for the sidebar's
|
||||||
* owner groups — staff only like the rest of `/users`.
|
* owner groups and System Health — the one `/users` read a developer may make
|
||||||
|
* too (`EVERYONE_READS` in server/lib/auth.js).
|
||||||
*
|
*
|
||||||
* The Users page's account changes — `POST /` (create), `PATCH /:id` (the Edit
|
* The Users page's account changes — `POST /` (create), `PATCH /:id` (the Edit
|
||||||
* user form: display name, email, role, disabled, a new password, workspaces
|
* user form: display name, email, role, disabled, a new password, workspaces
|
||||||
|
|||||||
Reference in New Issue
Block a user