From c3e2717e8e2dd13ee0799b7aa011cac0e190b33b Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Mon, 14 Sep 2026 17:00:18 -0400 Subject: [PATCH] fix: close the review findings on the seat experience MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the adversarial review of this PR. The forced password-change gate could deadlock: it POSTed to the cookie-only route with a bare fetch, and both initgui call sites open it before update_auth_data mints the session cookie — a fresh browser with a token URL 401'd every submit inside a non-dismissible loop. It uses the session-cookie retry wrapper now, taking the caller's token because window.auth_token does not exist yet on that path. It also gains the logout footer its sibling gates have; a lost temporary password was a hard lock with devtools as the only exit. Password recovery refused for seats: the address is admin-supplied and never verified, so whoever holds that inbox could take the seat over at any later time. A seat's recovery channel is its admin's reset. change-email gets the same seat guard as change-username and deletion — the address is where admin-issued credentials go. The login response now carries `team` alongside requires_password_change: no-reload logins store that payload as window.user verbatim, and every seat restriction keys on it. Smaller: the team-badge tooltip no longer double-encodes; the create-token hint for an emailless account stops pointing at a verification it can never perform; the quotas doc records the halved org_seat_free allowance; the config template tells upgrading operators how to keep the old flat cap; the SDK suite covers emailless provisioning and the owner-only uuid. --- config.template.jsonc | 4 +- .../controllers/auth/AuthController.test.ts | 46 +++++++++++++++ .../controllers/auth/AuthController.ts | 33 +++++++++++ src/docs/src/rate-limits-and-quotas.md | 4 +- src/gui/src/UI/Dashboard/TabAccount.js | 6 +- src/gui/src/UI/Dashboard/teamBadge.js | 3 +- src/gui/src/UI/Dashboard/teamBadge.test.js | 14 ++++- .../src/UI/UIWindowPasswordChangeRequired.js | 35 +++++++++--- .../UI/UIWindowPasswordChangeRequired.test.js | 56 +++++++++++++++++++ src/gui/src/i18n/translations/en.js | 1 + src/gui/src/initgui.js | 6 ++ src/puter-js/tests/api/suites/teams.suite.ts | 21 +++++++ 12 files changed, 215 insertions(+), 14 deletions(-) diff --git a/config.template.jsonc b/config.template.jsonc index 5e8849815..911ce3ec8 100644 --- a/config.template.jsonc +++ b/config.template.jsonc @@ -289,7 +289,9 @@ // name from the global username pool, so this is what bounds a team's blast // radius. The owner's plan decides which of the two applies; defaults are 4 // and 40. Lowering either below a team's current seat count blocks new - // provisioning and disables nobody. + // provisioning and disables nobody. Deployments upgrading from the old flat + // default of 50: without a billing extension every owner reads as free, so + // set `max_seats_per_team` to keep the old behavior. "max_seats_per_team_free": 4, "max_seats_per_team_paid": 40, // diff --git a/src/backend/controllers/auth/AuthController.test.ts b/src/backend/controllers/auth/AuthController.test.ts index 6ddef1732..972c90134 100644 --- a/src/backend/controllers/auth/AuthController.test.ts +++ b/src/backend/controllers/auth/AuthController.test.ts @@ -5006,6 +5006,32 @@ describe('AuthController password recovery', () => { expect(after!.pass_recovery_token).toBeTruthy(); }); + it('send-pass-recovery-email: refuses a team seat, and writes no token', async () => { + // The seat's address is admin-supplied and unverified; recovery there + // would be a takeover channel. Its recovery is the admin's reset. + const { user: owner } = await makeUserAndActor(); + const { user: seat } = await makeUserAndActor(); + const team = await server.stores.team.create({ + ownerUserId: owner.id, + name: 'Acme', + }); + await server.stores.user.update(seat.id, { password: null }); + await server.stores.team.addMember(team.uid, seat.id, { + orgOwned: true, + }); + + const res = makeRes(); + await controller.handleSendPassRecoveryEmail( + makeReq({ username: seat.username }), + res, + ); + expect((res.body as { message: string }).message).toMatch( + /If that account exists/i, + ); + const after = await server.stores.user.getById(seat.id, { force: true }); + expect(after!.pass_recovery_token).toBeFalsy(); + }); + it('verify-pass-recovery-token: 400 on missing token', async () => { await expect( controller.handleVerifyPassRecoveryToken(makeReq({}), makeRes()), @@ -5253,6 +5279,26 @@ describe('AuthController user-protected mutations (validation paths)', () => { ).rejects.toMatchObject({ statusCode: 400 }); }); + it('change-email: 403 for an account its team provisioned', async () => { + const { user: owner } = await makeUserAndActor(); + const { user: seat, actor } = await makeUserAndActor(); + const team = await server.stores.team.create({ + ownerUserId: owner.id, + name: 'Acme', + }); + await server.stores.user.update(seat.id, { password: null }); + await server.stores.team.addMember(team.uid, seat.id, { + orgOwned: true, + }); + + await expect( + controller.handleChangeEmail( + makeReq({ new_email: `moved_${uniq()}@example.com` }, { actor }), + makeRes(), + ), + ).rejects.toMatchObject({ statusCode: 403 }); + }); + it('change-username: 403 for an account its team provisioned', async () => { // The console lists members by username and the audit log records them // by it; a self-service rename would desync both. diff --git a/src/backend/controllers/auth/AuthController.ts b/src/backend/controllers/auth/AuthController.ts index 973b7c237..33bbf9c26 100644 --- a/src/backend/controllers/auth/AuthController.ts +++ b/src/backend/controllers/auth/AuthController.ts @@ -2293,6 +2293,14 @@ export class AuthController extends PuterController { return; } + // A seat's address is admin-supplied and never verified, so whoever + // holds that inbox could take the seat over. Its recovery channel is + // the team admin's password reset, not this one. + if (await this.stores.team.getOrgSeat(user.id)) { + res.json({ message: genericMessage }); + return; + } + const pass_recovery_token = uuidv4(); await this.stores.user.update(user.id, { pass_recovery_token }); @@ -2628,6 +2636,16 @@ export class AuthController extends PuterController { } async handleChangeEmail(req: Request, res: Response): Promise { + // The address is where admin-issued credentials and team notices go; + // same reasoning as the username and deletion guards above. + if (await this.stores.team.getOrgSeat(req.actor!.user.id!)) { + throw new HttpError( + 403, + 'Your team set this address. Ask a team admin to change it.', + { legacyCode: 'forbidden' }, + ); + } + const { new_email } = req.body ?? {}; if (!new_email || typeof new_email !== 'string') { throw new HttpError(400, '`new_email` is required', { @@ -4875,6 +4893,20 @@ export class AuthController extends PuterController { console.warn('[auth] taskbar_items resolution failed:', e); } + // Same shape as whoami: no-reload logins store this payload as + // window.user verbatim, and every seat restriction keys on `team`. + let team: { uid: string; name: string | null } | undefined; + if (this.config.teams_enabled === true) { + try { + const seat = await this.stores.team.getOrgSeat(user.id); + if (seat) { + team = { uid: seat.team_uid, name: seat.team_name ?? null }; + } + } catch (e) { + console.warn('[auth] team lookup failed:', e); + } + } + // Response body gets the GUI token (client never sees session token) res.json({ proceed: true, @@ -4891,6 +4923,7 @@ export class AuthController extends PuterController { requires_card_verification: user.requires_card_verification, requires_password_change: user.requires_password_change, is_temp: user.password === null && user.email === null, + ...(team ? { team } : {}), taskbar_items, }, }); diff --git a/src/docs/src/rate-limits-and-quotas.md b/src/docs/src/rate-limits-and-quotas.md index 9d7ed6d8b..65feb9e4b 100644 --- a/src/docs/src/rate-limits-and-quotas.md +++ b/src/docs/src/rate-limits-and-quotas.md @@ -195,7 +195,9 @@ Available only where a deployment has turned teams on. Every team route is bound | Seats one team may provision, paying owner | 40 | | Member password resets per day | 20 | -A seat is a real Puter account on the ordinary tier, created by the team and paid for by its owner, so the seat limit is what bounds a team's size. Over it, provisioning fails with `seat_limit_reached`; over the team limit, creation fails with `team_limit_reached`. Both carry the limit in `fields.limit`. +A seat is a real Puter account created by the team and paid for by its owner, so the seat limit is what bounds a team's size. Over it, provisioning fails with `seat_limit_reached`; over the team limit, creation fails with `team_limit_reached`. Both carry the limit in `fields.limit`. + +A seat whose team pays for no tier is on the `org_seat_free` plan: **half** the ordinary free allowance, usage and rate caps alike (a free account's `bySubscription` caps apply to every free plan). Without this, provisioning seats would mint full free tiers nobody pays for. A seat on a paid team tier gets that tier's allowance. A reset returns a temporary password once and never again. It stops working 24 hours after it is issued, so an unused reset expires rather than becoming a standing credential; after that the administrator has to issue a new one. Until the member replaces it, every authenticated request from that account fails with `password_change_required` — signing in works, but nothing else does until they choose their own password. diff --git a/src/gui/src/UI/Dashboard/TabAccount.js b/src/gui/src/UI/Dashboard/TabAccount.js index 42928e0fa..42c64d495 100644 --- a/src/gui/src/UI/Dashboard/TabAccount.js +++ b/src/gui/src/UI/Dashboard/TabAccount.js @@ -127,9 +127,13 @@ const TabAccount = { if ( window.user?.email_confirmed ) { h += ``; } else { + // "Verify your email" is a dead end for an account with none. + const tokenHint = window.user?.email + ? i18n('verify_email_to_create_token') + : i18n('email_needed_to_create_token'); // Disabled buttons have `pointer-events: none`, so the tooltip // lives on a wrapping span that still receives hover. - h += ``; + h += ``; h += ``; h += ''; } diff --git a/src/gui/src/UI/Dashboard/teamBadge.js b/src/gui/src/UI/Dashboard/teamBadge.js index 9dce269da..75b93aced 100644 --- a/src/gui/src/UI/Dashboard/teamBadge.js +++ b/src/gui/src/UI/Dashboard/teamBadge.js @@ -28,7 +28,8 @@ export const teamBadgeHtml = (user) => { const name = user?.team?.name; if ( typeof name !== 'string' || name.trim() === '' ) return ''; const label = window.html_encode(name); - const title = window.html_encode(i18n('teams_account_of', [name])); + // i18n encodes by default; encoding twice shows entities in the tooltip. + const title = window.html_encode(i18n('teams_account_of', [name], false)); return `
${label}
`; }; diff --git a/src/gui/src/UI/Dashboard/teamBadge.test.js b/src/gui/src/UI/Dashboard/teamBadge.test.js index f9069be98..adcc43cde 100644 --- a/src/gui/src/UI/Dashboard/teamBadge.test.js +++ b/src/gui/src/UI/Dashboard/teamBadge.test.js @@ -1,7 +1,11 @@ import { describe, expect, it } from 'vitest'; -globalThis.i18n = (key, args) => `${key}:${(args ?? []).join(',')}`; -globalThis.window = { html_encode: (v) => String(v).replace(/ { + const raw = `${key}:${(args ?? []).join(',')}`; + return encode ? raw.replace(/&/g, '&') : raw; +}; +globalThis.window = { html_encode: (v) => String(v).replace(/&/g, '&').replace(/ { expect(h).toContain('teams_account_of:Acme'); }); }); + +it('does not double-encode the tooltip', () => { + const h = teamBadgeHtml({ team: { uid: 't-1', name: 'R&D' } }); + expect(h).toContain('title="teams_account_of:R&D"'); + expect(h).not.toContain('&'); +}); diff --git a/src/gui/src/UI/UIWindowPasswordChangeRequired.js b/src/gui/src/UI/UIWindowPasswordChangeRequired.js index 90839904a..9672a2026 100644 --- a/src/gui/src/UI/UIWindowPasswordChangeRequired.js +++ b/src/gui/src/UI/UIWindowPasswordChangeRequired.js @@ -18,6 +18,7 @@ */ import check_password_strength from '../helpers/checkPasswordStrength.js'; +import { fetchWithSessionCookieRetry } from '../util/sessionAuth.js'; import UIWindow from './UIWindow.js'; /** Resolves true once a seat replaces the password its administrator chose. */ @@ -43,6 +44,12 @@ function UIWindowPasswordChangeRequired (options) { h += ''; h += ``; h += ''; + // The gate loops; a lost temporary password needs a way out. + if ( options.logout_in_footer ) { + h += '
'; + h += `${i18n('log_out')}`; + h += '
'; + } h += ''; const el_window = await UIWindow({ @@ -87,6 +94,12 @@ function UIWindowPasswordChangeRequired (options) { $(el_window).find('.pcr-current, .pcr-new, .pcr-confirm').attr('disabled', false); }; + $(el_window).find('.pcr-log-out').on('click', function () { + window.logout(); + $(el_window).close(); + resolve(false); + }); + $(el_window).find('form').on('submit', async function (e) { e.preventDefault(); const current_password = $(el_window).find('.pcr-current').val(); @@ -112,16 +125,22 @@ function UIWindowPasswordChangeRequired (options) { $(el_window).find('.pcr-btn').addClass('disabled'); $(el_window).find('.pcr-current, .pcr-new, .pcr-confirm').attr('disabled', true); + // The route is cookie-gated and this gate can open before the + // session cookie exists; the wrapper mints it and retries once. + const send = () => fetch(`${origin}/user-protected/change-password`, { + method: 'POST', + credentials: 'include', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + password: current_password, + new_pass: new_password, + }), + }); let res; try { - res = await fetch(`${origin}/user-protected/change-password`, { - method: 'POST', - credentials: 'include', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ - password: current_password, - new_pass: new_password, - }), + res = await fetchWithSessionCookieRetry(send, { + origin, + authToken: options.auth_token ?? window.auth_token, }); } catch (err) { return fail(err?.message || 'Request failed'); diff --git a/src/gui/src/UI/UIWindowPasswordChangeRequired.test.js b/src/gui/src/UI/UIWindowPasswordChangeRequired.test.js index 02e20591b..456f8fed2 100644 --- a/src/gui/src/UI/UIWindowPasswordChangeRequired.test.js +++ b/src/gui/src/UI/UIWindowPasswordChangeRequired.test.js @@ -58,6 +58,62 @@ describe('the forced password-change gate', () => { fields['.pcr-confirm'] = 'ChosenPass1!'; }); + it('mints the session cookie and retries when the first submit 401s', async () => { + globalThis.window.auth_token = 'tok-1'; + const rejected = { + ok: false, + status: 401, + clone: () => ({ json: async () => ({ code: 'session_required' }) }), + json: async () => ({ code: 'session_required' }), + }; + globalThis.fetch + .mockResolvedValueOnce(rejected) // first submit + .mockResolvedValueOnce({ ok: true, json: async () => ({}) }) // sync-cookie + .mockResolvedValueOnce({ ok: true }); // retried submit + UIWindowPasswordChangeRequired({}); + await new Promise((r) => setTimeout(r)); + await submit(); + const urls = globalThis.fetch.mock.calls.map(([u]) => String(u)); + expect(urls.some((u) => u.includes('/session/sync-cookie'))).toBe(true); + expect(state.closed).toBe(1); + }); + + it('offers a way out: the footer logout closes the gate', async () => { + let clickHandler = null; + const prevFind = globalThis.$; + globalThis.$ = () => ({ + find: (sel) => ({ + val: () => fields[sel], + on: (evt, fn) => { + if (evt === 'submit') submitHandler = fn; + if (evt === 'click' && sel === '.pcr-log-out') clickHandler = fn; + }, + html: () => ({ fadeIn: () => {} }), + hide: () => {}, + fadeIn: () => {}, + addClass: () => {}, + removeClass: () => {}, + attr: () => {}, + get: () => [undefined], + }), + close: () => { state.closed++; }, + }); + globalThis.window.logout = vi.fn(); + const resolved = UIWindowPasswordChangeRequired({ logout_in_footer: true }); + await new Promise((r) => setTimeout(r)); + expect(state.el.body).toContain('pcr-log-out'); + clickHandler(); + expect(globalThis.window.logout).toHaveBeenCalled(); + await expect(resolved).resolves.toBe(false); + globalThis.$ = prevFind; + }); + + it('renders no logout link unless the caller asks for one', async () => { + UIWindowPasswordChangeRequired({}); + await new Promise((r) => setTimeout(r)); + expect(state.el.body).not.toContain('pcr-log-out'); + }); + it('posts to the one route the gate lets through, with credentials', async () => { globalThis.fetch.mockResolvedValue({ ok: true }); const gate = UIWindowPasswordChangeRequired({ show_close_button: false }); diff --git a/src/gui/src/i18n/translations/en.js b/src/gui/src/i18n/translations/en.js index 3c986d631..339748f25 100644 --- a/src/gui/src/i18n/translations/en.js +++ b/src/gui/src/i18n/translations/en.js @@ -527,6 +527,7 @@ const en = { teams_create_team_hint: 'A team pays for the accounts you create in it. You stay its only administrator.', teams_create_team_prompt: 'What should the team be called?', + email_needed_to_create_token: 'This account has no email address, which tokens require.', teams_audit_range: '{{from}}–{{to}} of {{total}}', previous: 'Previous', next: 'Next', diff --git a/src/gui/src/initgui.js b/src/gui/src/initgui.js index 9bb98a701..55e84779e 100644 --- a/src/gui/src/initgui.js +++ b/src/gui/src/initgui.js @@ -1806,10 +1806,14 @@ window.initgui = async function (options) { show_close_button: false, stay_on_top: true, has_head: false, + logout_in_footer: true, + auth_token: query_param_auth_token, window_options: { is_draggable: false, }, }); + // false = logged out; stop looping on a dead session. + if (changed === false && !window.auth_token) return; } while (!changed); } // if user is logging in using an auth token that means it's not their first ever visit to Puter.com @@ -2086,11 +2090,13 @@ window.initgui = async function (options) { show_close_button: false, stay_on_top: true, has_head: false, + logout_in_footer: true, window_options: { is_draggable: false, cover_page: window.is_embedded, }, }); + if (changed === false && !window.auth_token) return; } while (!changed); } await window.update_auth_data( diff --git a/src/puter-js/tests/api/suites/teams.suite.ts b/src/puter-js/tests/api/suites/teams.suite.ts index aca395776..e218df0b6 100644 --- a/src/puter-js/tests/api/suites/teams.suite.ts +++ b/src/puter-js/tests/api/suites/teams.suite.ts @@ -122,6 +122,27 @@ export default suite('teams', { t.assert.equal(member!.orgOwned, true); }, + 'createMember needs no email, and the owner sees each seat uuid': async (t) => { + const team = await makeTeam(t, 'no-email'); + const username = `tsn${tag()}`; + const created = (await t.puter.teams.createMember(team.uid, { + username, + })) as { username: string; temporaryPassword: string }; + t.assert.equal(created.username, username); + t.assert.ok(created.temporaryPassword.length > 0, 'credential still issued'); + + const members = (await t.puter.teams.listMembers(team.uid)) as Array<{ + username: string; orgOwned: boolean; uuid?: string; + }>; + const member = members.find((m) => m.username === username); + t.assert.ok(!!member, 'the emailless seat should be a member'); + // The uuid is what billing keys a seat's plan on; owner-only. + t.assert.ok( + typeof member!.uuid === 'string' && member!.uuid.length > 0, + 'the owner should see the seat uuid', + ); + }, + 'createMember with a taken username rejects': async (t) => { const team = await makeTeam(t, 'taken'); await t.assert.rejects(