mirror of
https://github.com/screentinker/screentinker.git
synced 2026-08-13 13:53:12 -06:00
Close the third QA round: limiter bypass, stored XSS, break-glass, org placement
Four HIGH findings. Two were mine, and one was a composition of two of my own fixes.
ONE EXTRA SLASH DEFEATED EVERY /api/auth LIMITER
`/api/auth//login` still reaches the login handler — Express normalises the mount
boundary for the router — but `app.use('/api/auth/login', rateLimit(...))` does not
match it, so the limiter never runs. A review got a real session after 60 unthrottled
password attempts. Same for //totp/verify (unlimited 6-digit brute force),
//forgot-password (unlimited reset mail to any address) and //sso/discover (the
customer-enumeration cap, gone). Fixing the limiter KEY could never help, because the
middleware was never invoked: the path is now collapsed to one canonical form before
routing. Pre-existing, and it falsified this file's own warning about walking past the
login limiter.
STORED XSS: I ESCAPED ONE COPY OF THE TABLE
My earlier fix patched views/admin.js line 357 and missed line 372 in the same
function — and missed views/settings.js entirely, which renders a SECOND copy of the
platform users table from the same endpoint, including the email in a raw text node.
The write path was `POST /api/admin/users`, whose EMAIL_RE barred only whitespace, so
an org or workspace admin (not a platform admin) could choose an address that executed
in the operator's session. Both tables escaped, both regexes tightened to reject markup
characters, verified against 11 address shapes.
I KILLED THE BREAK-GLASS WHILE CLOSING AN ORACLE
Hoisting the domain check above the account lookup — my fix for the enumeration oracle
— made `user.role !== 'platform_admin'` unreachable for enforced domains. On a
self-host the operator IS the org owner, and my would_lock_out_actor guard GUARANTEES
their address is inside the enforced set, so the recovery loop closed on itself:
approving a removal request needs a signed-in platform admin. Both properties hold now
by letting the operator through on a CORRECT PASSWORD only — every wrong answer is the
identical 403 whether the address exists, does not exist, or is theirs. Verified: 200 /
403 / 403 / 403.
Also fixed: enabling SSO-only locked out every password-holding member including the
admin who pressed the button (password refused by policy, SSO refused by
account_exists_local). An org provider now adopts a password account at a domain it has
PROVED by DNS when the org requires SSO — which is what a verified domain means, and
what every hosted identity product does.
SSO USERS WERE LANDING IN A PERSONAL ORG
The membership write added organization_members but no workspace_members, and
ensureDefaultOrgForUser looks at workspaces — so it minted each SSO user a private
organization and made it their current one. The customer's Members page read
"Members (1)" while their staff signed in successfully and were invisible.
ALSO: bcrypt on a NULL password_hash 500'd with a stack (and was an oracle for accounts
a provider deletion had returned to local); stranded_members was returned by the server
and discarded by the UI; a provider with zero domains was the one useless state with no
warning; two limiter shapes were missing (removal-request shared the garbage bucket —
an unauthenticated flood could deny the SSO break-glass path); doubled mail subject
prefixes; a DELETE that toasted "Saved"; a decided request left in the DOM with live
listeners; and a confirm dialog promising "immediately" when sessions already open
survive.
1609 tests, three clean runs. Limiter, break-glass, oracle parity and null-password all
verified against a running server.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bvjey4FNam49MN7ybjcq6A
This commit is contained in:
ScreenTinker
parent
94e1273ecd
commit
fbf55f842c
|
|
@ -152,12 +152,14 @@ export default {
|
|||
'sso.disabled': 'disabled',
|
||||
'sso.domains_label': 'Email domains',
|
||||
'sso.domains_heading': 'Sign-in domains',
|
||||
'sso.removed': 'Removed.',
|
||||
'sso.only_stranded': 'Single sign-on is now required.\n\nThese members are not at a verified domain, so they can no longer sign in at all:\n\n{list}\n\nVerify their domain, or remove them from this organization.',
|
||||
'sso.only_heading': 'Require single sign-on',
|
||||
'sso.only_help': 'When required, people at your verified domains can only sign in through your identity provider — a password will not work. Your provider keeps control of MFA and of removing access.',
|
||||
'sso.only_on': 'Single sign-on is required for your verified domains.',
|
||||
'sso.only_off': 'Password sign-in is still allowed alongside single sign-on.',
|
||||
'sso.only_enable': 'Require single sign-on',
|
||||
'sso.only_confirm': 'Require single sign-on for everyone at your verified domains?\n\nPasswords will stop working for them immediately. Turning this back off needs approval from the people who run this server, so make sure your identity provider is working first.',
|
||||
'sso.only_confirm': 'Require single sign-on for everyone at your verified domains?\n\nPasswords will stop working for them at their next sign-in; sessions already open continue until they expire. Turning this back off needs approval from the people who run this server, so make sure your identity provider is working first.',
|
||||
'sso.only_needs_domain': 'Verify a sign-in domain first — otherwise nobody would be able to sign in.',
|
||||
'sso.only_remove_help': 'Turning this off re-opens password sign-in, so it needs approval from the people who run this server.',
|
||||
'sso.only_request': 'Request to stop requiring single sign-on',
|
||||
|
|
@ -208,7 +210,7 @@ export default {
|
|||
'auth.sso_err_provider_unavailable': 'That provider is not reachable right now.',
|
||||
'auth.sso_err_unknown_provider': 'That sign-in provider is not configured.',
|
||||
'auth.sso_err_registration_disabled': 'New accounts are disabled on this instance.',
|
||||
'auth.sso_err_account_exists_local': 'An account with this email already exists. Sign in with your password, then link your provider in Settings.',
|
||||
'auth.sso_err_account_exists_local': 'An account with this email already exists and uses a password. Sign in with your password instead.',
|
||||
'auth.sso_err_subject_mismatch': 'This email is already linked to a different account at your provider.',
|
||||
'auth.sso_err_server_error': 'Something went wrong completing sign-in.',
|
||||
// Both of these used to fall through to "please try again", which is advice that can never work:
|
||||
|
|
|
|||
|
|
@ -183,7 +183,9 @@ async function loadSsoOnlyRequests() {
|
|||
section.style.display = 'none';
|
||||
return;
|
||||
}
|
||||
if (!requests.length) { section.style.display = 'none'; return; }
|
||||
// Clear as well as hide: leaving the last decided request in the tree kept its live
|
||||
// Approve/Reject listeners attached to a request that no longer exists.
|
||||
if (!requests.length) { host.innerHTML = ''; section.style.display = 'none'; return; }
|
||||
section.style.display = '';
|
||||
|
||||
host.innerHTML = requests.map((r) => `
|
||||
|
|
@ -369,7 +371,7 @@ async function loadUsers() {
|
|||
</td>
|
||||
${workspaceCell(u)}
|
||||
<td style="padding:8px;white-space:nowrap">
|
||||
${u.auth_provider === 'local' && u.id !== currentUser.id ? `<button class="btn btn-secondary btn-sm" data-reset-pw-user="${u.id}" data-user-email="${u.email}" style="margin-right:4px">${t('admin.reset_password')}</button>` : ''}
|
||||
${u.auth_provider === 'local' && u.id !== currentUser.id ? `<button class="btn btn-secondary btn-sm" data-reset-pw-user="${esc(u.id)}" data-user-email="${esc(u.email)}" style="margin-right:4px">${t('admin.reset_password')}</button>` : ''}
|
||||
${!isPlatformAdmin(u) ? `<button class="btn btn-danger btn-sm" data-delete-user="${u.id}">${t('admin.remove')}</button>` : `<span style="color:var(--text-muted);font-size:11px">${t('admin.owner')}</span>`}
|
||||
</td>
|
||||
</tr>
|
||||
|
|
|
|||
|
|
@ -712,7 +712,7 @@ export async function render(container) {
|
|||
${p.enabled ? '' : `<span style="font-size:11px;color:var(--text-muted)"> — ${esc(t('sso.disabled'))}</span>`}
|
||||
<div style="font-size:12px;color:var(--text-muted);margin-top:2px">${esc(p.issuer)}</div>
|
||||
<div style="font-size:12px;color:var(--text-muted)">${esc(t('sso.domains_label'))}: ${esc(p.email_domains || '—')}</div>
|
||||
${((p.domains || []).some((d) => !d.verified) || ((p.domains || []).length === 0 && p.email_domains))
|
||||
${((p.domains || []).some((d) => !d.verified) || (p.domains || []).length === 0)
|
||||
? `<div style="font-size:12px;color:var(--warning,#b45309);margin-top:2px">⚠️ ${esc(t('sso.unverified_warning'))}</div>`
|
||||
: ''}
|
||||
</div>
|
||||
|
|
@ -843,7 +843,20 @@ export async function render(container) {
|
|||
// Confirmed, because it removes the only way in for everyone at these domains, and the way
|
||||
// back needs the operator rather than this button.
|
||||
if (!window.confirm(t('sso.only_confirm'))) return;
|
||||
if (await post(`/api/organizations/${orgId}/sso-only`)) { showToast(t('sso.only_on'), 'success'); await loadSso(); }
|
||||
const r = await post(`/api/organizations/${orgId}/sso-only`);
|
||||
if (r) {
|
||||
showToast(t('sso.only_on'), 'success');
|
||||
/*
|
||||
* Name the people who just lost their only way in. The server reports them precisely so
|
||||
* the admin finds out HERE rather than from a support ticket — and it was being thrown
|
||||
* away, which made the whole warning pointless.
|
||||
*/
|
||||
const stranded = r.stranded_members || [];
|
||||
if (stranded.length) {
|
||||
window.alert(t('sso.only_stranded', { list: stranded.join('\n') }));
|
||||
}
|
||||
await loadSso();
|
||||
}
|
||||
});
|
||||
|
||||
const reqBtn = box.querySelector('#ssoOnlyRequest');
|
||||
|
|
@ -1000,7 +1013,8 @@ export async function render(container) {
|
|||
// The server's message is the useful one here — a bad issuer or a domain already claimed by
|
||||
// another organization both say exactly what went wrong, and a generic failure would not.
|
||||
if (!res.ok) { showToast(data.error || t('sso.save_failed'), 'error'); return false; }
|
||||
showToast(t('sso.saved'), 'success');
|
||||
// "Saved" for a DELETE read as though nothing had been destroyed.
|
||||
showToast(t(method === 'DELETE' ? 'sso.removed' : 'sso.saved'), 'success');
|
||||
await loadSso();
|
||||
return true;
|
||||
} catch {
|
||||
|
|
@ -1227,25 +1241,30 @@ async function loadUsers() {
|
|||
</thead>
|
||||
<tbody>
|
||||
${users.map(u => `
|
||||
<tr style="border-bottom:1px solid var(--border)" data-user-id="${u.id}">
|
||||
<!-- ESCAPED. A SECOND copy of the platform users table lives here, rendered from the
|
||||
same endpoint as the one in views/admin.js. Escaping only that one left this whole
|
||||
table wide open, including a raw text node for the email - and an org or workspace
|
||||
admin can choose an email, so this executed in the platform admin's session. When
|
||||
you touch one of these tables, touch both. -->
|
||||
<tr style="border-bottom:1px solid var(--border)" data-user-id="${esc(u.id)}">
|
||||
<td style="padding:10px 12px">
|
||||
<div style="font-weight:500">${u.name || u.email}</div>
|
||||
<div style="font-size:11px;color:var(--text-muted)">${u.email}</div>
|
||||
<div style="font-weight:500">${esc(u.name || u.email)}</div>
|
||||
<div style="font-size:11px;color:var(--text-muted)">${esc(u.email)}</div>
|
||||
</td>
|
||||
<td style="padding:10px 12px">
|
||||
<span style="background:var(--bg-primary);padding:2px 8px;border-radius:10px;font-size:11px">${u.auth_provider}</span>
|
||||
<span style="background:var(--bg-primary);padding:2px 8px;border-radius:10px;font-size:11px">${esc(u.auth_provider)}</span>
|
||||
</td>
|
||||
<td style="padding:10px 12px">
|
||||
<span style="color:${isPlatformAdmin(u) ? 'var(--accent)' : 'var(--text-secondary)'}">${u.role}</span>
|
||||
<span style="color:${isPlatformAdmin(u) ? 'var(--accent)' : 'var(--text-secondary)'}">${esc(u.role)}</span>
|
||||
</td>
|
||||
<td style="padding:10px 12px">
|
||||
<select class="input plan-select" data-user-id="${u.id}" style="padding:4px 8px;font-size:12px;width:auto">
|
||||
${plans.map(p => `<option value="${p.id}" ${u.plan_id === p.id ? 'selected' : ''}>${p.display_name}</option>`).join('')}
|
||||
<select class="input plan-select" data-user-id="${esc(u.id)}" style="padding:4px 8px;font-size:12px;width:auto">
|
||||
${plans.map(p => `<option value="${esc(p.id)}" ${u.plan_id === p.id ? 'selected' : ''}>${esc(p.display_name)}</option>`).join('')}
|
||||
</select>
|
||||
</td>
|
||||
<td style="padding:10px 12px;white-space:nowrap">
|
||||
${u.auth_provider === 'local' && u.id !== currentUser.id ? `<button class="btn btn-secondary btn-sm reset-user-pw-btn" data-user-id="${u.id}" data-user-email="${u.email}" style="margin-right:4px">${t('settings.user.reset_password')}</button>` : ''}
|
||||
${u.id !== currentUser.id ? `<button class="btn btn-danger btn-sm delete-user-btn" data-user-id="${u.id}">${t('settings.user.remove')}</button>` : `<span style="color:var(--text-muted);font-size:11px">${t('settings.user.you')}</span>`}
|
||||
${u.auth_provider === 'local' && u.id !== currentUser.id ? `<button class="btn btn-secondary btn-sm reset-user-pw-btn" data-user-id="${esc(u.id)}" data-user-email="${esc(u.email)}" style="margin-right:4px">${t('settings.user.reset_password')}</button>` : ''}
|
||||
${u.id !== currentUser.id ? `<button class="btn btn-danger btn-sm delete-user-btn" data-user-id="${esc(u.id)}">${t('settings.user.remove')}</button>` : `<span style="color:var(--text-muted);font-size:11px">${t('settings.user.you')}</span>`}
|
||||
</td>
|
||||
</tr>
|
||||
`).join('')}
|
||||
|
|
|
|||
|
|
@ -21,7 +21,9 @@ const { platformDefaultRow, HARDCODED_BRANDING, PLATFORM_DEFAULT_ID } = require(
|
|||
// have no user/role-management power (#13).
|
||||
|
||||
// Same email shape the invite-create endpoint validates against (workspaces.js).
|
||||
const EMAIL_RE = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
|
||||
// Markup characters are not legal here. The looser form admitted < > " ' and an admin-
|
||||
// chosen email became stored XSS in the platform admin's user list.
|
||||
const EMAIL_RE = /^[^\s@<>"'`\\;,()\[\]]+@[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)+$/;
|
||||
const WORKSPACE_ROLES = ['workspace_admin', 'workspace_editor', 'workspace_viewer'];
|
||||
// Mirror the server-side minimum enforced by PUT /api/auth/me and register.
|
||||
const MIN_PASSWORD_LENGTH = 8;
|
||||
|
|
|
|||
|
|
@ -206,26 +206,48 @@ router.post('/login', (req, res) => {
|
|||
* anyone — so refusing on the domain alone reveals nothing new, and it reveals it identically
|
||||
* for addresses that exist and addresses that do not.
|
||||
*/
|
||||
/*
|
||||
* SSO-only refusal, arranged so it is neither an account-existence oracle NOR a way to brick the
|
||||
* instance.
|
||||
*
|
||||
* Two constraints pull against each other. Answering 403 only for addresses that EXIST turned
|
||||
* this into an enumeration oracle. But hoisting the check above the account lookup — the obvious
|
||||
* cure — silently killed the platform_admin break-glass, because role is not known until the row
|
||||
* is read. That is worse than it sounds: on a self-hosted instance the operator IS the org owner,
|
||||
* and the guard that stops an admin locking themselves out GUARANTEES their address is inside the
|
||||
* enforced set. Approving a removal request needs a platform admin to be signed in, so the
|
||||
* recovery loop closed on itself and the only way back was a shell.
|
||||
*
|
||||
* Both hold if the operator is let through on a CORRECT PASSWORD and nothing else: every wrong
|
||||
* answer is the identical 403, whether the address exists, does not exist, or belongs to the
|
||||
* operator. The only observable difference needs the password, which an enumerator does not have.
|
||||
*/
|
||||
const domainEnforced = (() => {
|
||||
try { return oidcProviders.ssoOnlyForEmail(email); } catch (e) {
|
||||
console.error('[login] SSO-only status unavailable, refusing password login:', e && e.message);
|
||||
return { unavailable: true };
|
||||
}
|
||||
})();
|
||||
if (domainEnforced) {
|
||||
const ssoRefusal = () => {
|
||||
logFailedLogin(email, getClientIp(req), 'Password login refused: domain requires SSO');
|
||||
return res.status(403).json({
|
||||
error: 'Your organization requires single sign-on. Use the single sign-on button to continue.',
|
||||
code: 'sso_required',
|
||||
sso_start: '/api/auth/sso/start',
|
||||
});
|
||||
}
|
||||
};
|
||||
|
||||
const user = db.prepare('SELECT * FROM users WHERE email = ? AND auth_provider = ?').get(email.toLowerCase(), 'local');
|
||||
if (!user) {
|
||||
// An unknown address at an enforced domain answers exactly like a known one — see above.
|
||||
if (domainEnforced) return ssoRefusal();
|
||||
logFailedLogin(email, getClientIp(req), 'User not found');
|
||||
return res.status(401).json({ error: 'Invalid email or password' });
|
||||
}
|
||||
// The break-glass: the operator may still sign in with a password at an enforced domain, but a
|
||||
// WRONG password answers with the same refusal everyone else gets, so nothing is learned.
|
||||
const breakGlass = domainEnforced && user.role === 'platform_admin' && !domainEnforced.unavailable;
|
||||
if (domainEnforced && !breakGlass) return ssoRefusal();
|
||||
|
||||
/*
|
||||
* SSO-ONLY. The organization that owns this VERIFIED domain requires its identity provider, so a
|
||||
|
|
@ -285,7 +307,13 @@ router.post('/login', (req, res) => {
|
|||
return res.status(401).json({ error: 'Invalid email or password' });
|
||||
}
|
||||
|
||||
if (!bcrypt.compareSync(password, user.password_hash)) {
|
||||
if (!user.password_hash || !bcrypt.compareSync(password, user.password_hash)) {
|
||||
if (breakGlass) {
|
||||
// Same answer as every other address at this domain: the operator's existence is not a fact
|
||||
// this endpoint gives away to someone who cannot type their password.
|
||||
loginLockout.recordFailure(user.id);
|
||||
return ssoRefusal();
|
||||
}
|
||||
const rec = loginLockout.recordFailure(user.id);
|
||||
if (rec.lockedUntil) logActivity(null, 'auth:login_locked', `${email} - locked after repeated failures`, null, getClientIp(req));
|
||||
logFailedLogin(email, getClientIp(req), 'Wrong password');
|
||||
|
|
@ -1379,6 +1407,25 @@ router.get('/oidc/:slug/callback', asyncRoute(async (req, res) => {
|
|||
if (!already) {
|
||||
db.prepare("INSERT INTO organization_members (organization_id, user_id, role) VALUES (?, ?, 'org_member')")
|
||||
.run(provider.organizationId, user.id);
|
||||
/*
|
||||
* ⚠️ And a WORKSPACE, or they land somewhere else entirely.
|
||||
*
|
||||
* ensureDefaultOrgForUser (below) looks for a workspace_members row, not an
|
||||
* organization_members one — so writing only the org membership left it finding nothing and
|
||||
* minting the user a brand-new personal organization, which then became their CURRENT one.
|
||||
* The customer's Members page still read "Members (1)": their staff signed in successfully
|
||||
* and were invisible to the admin, managing a private org of their own. That is precisely
|
||||
* the outcome the comment above says this code exists to prevent.
|
||||
*/
|
||||
const target = db.prepare(
|
||||
'SELECT id FROM workspaces WHERE organization_id = ? ORDER BY created_at LIMIT 1'
|
||||
).get(provider.organizationId);
|
||||
if (target) {
|
||||
db.prepare("INSERT OR IGNORE INTO workspace_members (workspace_id, user_id, role) VALUES (?, ?, 'workspace_viewer')")
|
||||
.run(target.id, user.id);
|
||||
} else {
|
||||
console.warn(`[oidc] org ${provider.organizationId} has no workspace; ${user.email} has no place to land`);
|
||||
}
|
||||
// (userId, action, details, deviceId, ipAddress, workspaceId) — the org id is NOT the 4th
|
||||
// arg; it was landing in device_id, which has no FK to catch it.
|
||||
logActivity(user.id, 'org_sso_joined', `via ${provider.name} org=${provider.organizationId}`, null, getClientIp(req));
|
||||
|
|
@ -1505,7 +1552,33 @@ function upsertFederatedUser({ claims, email, provider, req }) {
|
|||
}
|
||||
|
||||
if (existing.auth_provider !== provider.slug) {
|
||||
if (existing.password_hash) return { error: 'account_exists_local' };
|
||||
/*
|
||||
* An account WITH a password is normally never taken over by an SSO login — the owner proves
|
||||
* control by signing in locally. There is exactly one case where refusing is worse than
|
||||
* adopting, and it is a trap the previous design walked into:
|
||||
*
|
||||
* an organization that REQUIRES single sign-on, asserting an address at a domain it has PROVED
|
||||
* by DNS. There, the password is already refused by policy (403 sso_required), so refusing the
|
||||
* SSO login too shuts both doors — the member cannot sign in by any route, password reset
|
||||
* "succeeds" and changes nothing, and if that member is the last org admin the removal request
|
||||
* that would undo it can never be filed. A review locked an admin out of their own tenant this
|
||||
* way, with no route back short of SQL.
|
||||
*
|
||||
* Adopting is safe precisely because of what the two conditions already establish: the tenant
|
||||
* proved control of the domain (a DNS record they published), and the confinement check above
|
||||
* has already refused anything outside it. This is what every hosted identity product does with
|
||||
* a verified domain, and it is the only reading under which "requires single sign-on" is a
|
||||
* statement about the domain rather than about whoever happened to register first.
|
||||
*/
|
||||
const ssoOnlyAdoption = !!provider.organizationId
|
||||
&& !!oidcProviders.ssoOnlyForEmail(email)
|
||||
&& emailAllowedForProvider(provider, email);
|
||||
if (existing.password_hash && !ssoOnlyAdoption) return { error: 'account_exists_local' };
|
||||
if (existing.password_hash && ssoOnlyAdoption) {
|
||||
// The password is dead by policy; clear it rather than leave a credential nobody may use.
|
||||
db.prepare('UPDATE users SET password_hash = NULL WHERE id = ?').run(existing.id);
|
||||
console.log(`[oidc] ${provider.slug} adopted ${email} (organization requires SSO for its verified domain)`);
|
||||
}
|
||||
/*
|
||||
* `password_hash IS NULL` was the wrong test for "safe to relink". Every SSO-created account has
|
||||
* a null password, so it meant "any federated account may be adopted by whichever provider spoke
|
||||
|
|
|
|||
|
|
@ -262,8 +262,8 @@ function notifyOperatorOfClaim(req, { domains, orgId, providerName }) {
|
|||
'8 hours if it is not proved. No action is needed unless this looks wrong.',
|
||||
].join('\n');
|
||||
const subject = domains.length === 1
|
||||
? `[ScreenTinker] SSO domain claimed: ${domains[0]}`
|
||||
: `[ScreenTinker] ${domains.length} SSO domains claimed`;
|
||||
? `SSO domain claimed: ${domains[0]}`
|
||||
: `${domains.length} SSO domains claimed`; // services/email.js adds the [ScreenTinker] prefix
|
||||
for (const a of admins) {
|
||||
Promise.resolve(emailSvc.sendEmail({ to: a.email, subject, text: body }))
|
||||
.catch((e) => console.error('[org-sso] claim notification failed:', e && e.message));
|
||||
|
|
@ -307,7 +307,7 @@ function notifyOperatorOfRemovalRequest(req, { id, orgId, orgName, reason }) {
|
|||
for (const a of admins) {
|
||||
Promise.resolve(emailSvc.sendEmail({
|
||||
to: a.email,
|
||||
subject: `[ScreenTinker] Approval needed: stop requiring SSO for ${orgName || orgId}`,
|
||||
subject: `Approval needed: stop requiring SSO for ${orgName || orgId}`,
|
||||
text: body,
|
||||
})).catch((e) => console.error('[org-sso] removal notification failed:', e && e.message));
|
||||
}
|
||||
|
|
|
|||
|
|
@ -14,7 +14,9 @@ const { sendEmail } = require('../services/email');
|
|||
const NAME_MAX = 80;
|
||||
const SLUG_MAX = 60;
|
||||
const SLUG_RE = /^[a-z0-9]+(?:-[a-z0-9]+)*$/;
|
||||
const EMAIL_RE = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
|
||||
// Markup characters are not legal here. The looser form admitted < > " ' and an admin-
|
||||
// chosen email became stored XSS in the platform admin's user list.
|
||||
const EMAIL_RE = /^[^\s@<>"'`\\;,()\[\]]+@[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)+$/;
|
||||
const WORKSPACE_ROLES = ['workspace_admin', 'workspace_editor', 'workspace_viewer'];
|
||||
|
||||
// Operational policy - env-configurable with conservative defaults. Restart
|
||||
|
|
|
|||
|
|
@ -184,6 +184,29 @@ app.post('/api/stripe/webhook', express.raw({ type: 'application/json' }), strip
|
|||
// 12mb so AI-designed signs with embedded generated images (base64 data URLs)
|
||||
// can be published. #41 follow-up: upload generated images to the content store
|
||||
// and reference by URL instead of embedding, to keep widget configs small.
|
||||
/*
|
||||
* Collapse duplicate slashes in the PATH before anything routes on it.
|
||||
*
|
||||
* Express normalises the mount boundary for a router, so `/api/auth//login` still reaches the login
|
||||
* handler — but `app.use('/api/auth/login', rateLimit(...))` does NOT match it, so the limiter
|
||||
* never runs. One extra slash therefore removed EVERY per-endpoint limit under /api/auth: unlimited
|
||||
* password guesses (a review got a real session after 60 unthrottled attempts), unlimited TOTP
|
||||
* codes, unlimited password-reset mail to any address, and the SSO discovery cap that exists to
|
||||
* stop customer enumeration. It also made the per-account lockout a denial-of-service tool.
|
||||
*
|
||||
* Fixing it inside the limiter's key is not enough — the middleware is never invoked. The path has
|
||||
* to be one canonical thing before routing, which is what this does. Query and body are untouched.
|
||||
*/
|
||||
app.use((req, res, next) => {
|
||||
const q = req.url.indexOf('?');
|
||||
const path = q === -1 ? req.url : req.url.slice(0, q);
|
||||
if (path.includes('//')) {
|
||||
const collapsed = path.replace(/\/{2,}/g, '/');
|
||||
req.url = q === -1 ? collapsed : collapsed + req.url.slice(q);
|
||||
}
|
||||
next();
|
||||
});
|
||||
|
||||
app.use(express.json({ limit: '12mb' }));
|
||||
const { sanitizeBody } = require('./middleware/sanitize');
|
||||
app.use(sanitizeBody);
|
||||
|
|
@ -551,7 +574,12 @@ const LIMIT_PATH_SHAPES = [
|
|||
[/^\/api\/organizations\/sso-only\/removal-requests\/[^/]+\/[^/]+$/, () => '/api/organizations/sso-only/removal-requests/:id/:decision'],
|
||||
[/^\/api\/organizations\/sso-only\/removal-requests$/, () => '/api/organizations/sso-only/removal-requests'],
|
||||
[/^\/api\/organizations\/[^/]+\/sso-only\/removal-request\/[^/]+$/, () => '/api/organizations/:id/sso-only/removal-request/:id'],
|
||||
[/^\/api\/organizations\/[^/]+\/sso-only\/removal-request$/, () => '/api/organizations/:id/sso-only/removal-request'],
|
||||
[/^\/api\/organizations\/[^/]+\/sso-only$/, () => '/api/organizations/:id/sso-only'],
|
||||
// The reset/target routes mint a bucket per TARGET without this, which is the same
|
||||
// caller-chosen-segment defect, at the mount next door.
|
||||
[/^\/api\/auth\/users\/[^/]+\/(.+)$/, (m) => `/api/auth/users/:id/${m[1]}`],
|
||||
[/^\/api\/content\/[^/]+$/, () => '/api/content/:id'],
|
||||
[/^\/api\/organizations\/[^/]+\/sso\/[^/]+\/domains\/[^/]+\/verify$/, () => '/api/organizations/:id/sso/:id/domains/:domain/verify'],
|
||||
[/^\/api\/organizations\/[^/]+\/sso\/[^/]+\/test$/, () => '/api/organizations/:id/sso/:id/test'],
|
||||
[/^\/api\/organizations\/[^/]+\/sso\/[^/]+$/, () => '/api/organizations/:id/sso/:id'],
|
||||
|
|
|
|||
Loading…
Reference in a new issue