mirror of
https://github.com/screentinker/screentinker.git
synced 2026-08-13 13:53:12 -06:00
Fix: per-organization SSO was blocked by our own CSP and had never worked in a browser
THE HEADLINE FEATURE COULD NOT RUN.
"Continue with single sign-on" was a <form method="POST"> that redirected on to the
customer's identity provider. Chrome applies `form-action` across the WHOLE redirect
chain, and the dashboard sets `form-action 'self'`, so the hop to the provider was
aborted — silently. The user clicked and nothing happened: no navigation, no toast, no
spinner, a byte-identical page. Combined with SSO-only it was a total lockout: password
login answers 403 "use the single sign-on button", pointing at a button that cannot
work.
Every test I ran on this feature checked the button RENDERED. None clicked it.
The provider origins cannot be allowlisted — customers supply them at runtime. So the
page now fetches the destination and navigates itself; a script-initiated navigation is
not governed by form-action. The redirect answer is kept for a caller without
JavaScript, where the chain stays same-origin until the provider takes over. The slug
in the JSON is not a disclosure: following the old redirect put it in the address bar
and history anyway.
Verified in Chrome: the provider start endpoint is reached, zero CSP violations, zero
aborted requests — where before it was ERR_ABORTED plus a console violation.
STORED XSS IN THE PLATFORM ADMIN'S SESSION
admin.js interpolated user name, email and auth_provider into innerHTML unescaped, and
/register accepted an address whose local part was an img tag with an onerror handler —
no spaces, so it slipped the asserted-email check too. A reviewer registered
anonymously and got script execution on #/admin: the page operators are now emailed to.
Escaped, and registration refuses addresses that are not addresses. (The render bug
predates this branch; the reachability and the significance of that screen do not.)
ALSO
- the org SSO button is secondary while a password still works; two identical blue
buttons stacked sent people to their IdP by muscle memory after typing a password.
1609 tests, three clean runs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bvjey4FNam49MN7ybjcq6A
This commit is contained in:
parent
85febe05c0
commit
94e1273ecd
|
|
@ -348,11 +348,17 @@ async function loadUsers() {
|
||||||
<tbody>
|
<tbody>
|
||||||
${users.map(u => `
|
${users.map(u => `
|
||||||
<tr style="border-bottom:1px solid var(--border)">
|
<tr style="border-bottom:1px solid var(--border)">
|
||||||
<td style="padding:8px"><div style="font-weight:500">${u.name || u.email}</div><div style="font-size:11px;color:var(--text-muted)">${u.email}</div></td>
|
<!-- ESCAPED: these come from self-registration and from an identity provider's
|
||||||
<td style="padding:8px"><span style="background:var(--bg-primary);padding:2px 8px;border-radius:10px;font-size:11px">${u.auth_provider}</span></td>
|
email claim, so they are attacker-chosen. A reviewer registered an address whose
|
||||||
|
local part was an img tag with an onerror handler, anonymously, and got script
|
||||||
|
execution in the PLATFORM ADMIN's session on this page - the very page operators
|
||||||
|
are now emailed to. Note backticks are illegal here: this sits inside a template
|
||||||
|
literal. -->
|
||||||
|
<td style="padding:8px"><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:8px"><span style="background:var(--bg-primary);padding:2px 8px;border-radius:10px;font-size:11px">${esc(u.auth_provider)}</span></td>
|
||||||
<td style="padding:8px;font-size:11px;color:var(--text-muted)">${u.last_login ? new Date(u.last_login * 1000).toLocaleString() : t('common.never')}</td>
|
<td style="padding:8px;font-size:11px;color:var(--text-muted)">${u.last_login ? new Date(u.last_login * 1000).toLocaleString() : t('common.never')}</td>
|
||||||
<td style="padding:8px">
|
<td style="padding:8px">
|
||||||
<select class="input" style="max-width:120px;width:100%;background:var(--bg-input);font-size:12px;padding:4px" data-role-user="${u.id}">
|
<select class="input" style="max-width:120px;width:100%;background:var(--bg-input);font-size:12px;padding:4px" data-role-user="${esc(u.id)}">
|
||||||
${PLATFORM_ROLE_OPTIONS.map(r => `<option value="${r}" ${u.role === r ? 'selected' : ''}>${t('admin.role.' + r)}</option>`).join('')}
|
${PLATFORM_ROLE_OPTIONS.map(r => `<option value="${r}" ${u.role === r ? 'selected' : ''}>${t('admin.role.' + r)}</option>`).join('')}
|
||||||
</select>
|
</select>
|
||||||
</td>
|
</td>
|
||||||
|
|
|
||||||
|
|
@ -601,17 +601,46 @@ function setupHandlers(config, isSetup) {
|
||||||
* mapping again on submit, so the slug is never published to the page. POST keeps the address
|
* mapping again on submit, so the slug is never published to the page. POST keeps the address
|
||||||
* out of the URL, browser history and any Referer the provider's page would send.
|
* out of the URL, browser history and any Referer the provider's page would send.
|
||||||
*/
|
*/
|
||||||
|
/*
|
||||||
|
* A BUTTON that fetches and then navigates — not a form that submits.
|
||||||
|
*
|
||||||
|
* The dashboard's CSP is `form-action 'self'`, and Chrome applies it across the whole
|
||||||
|
* redirect chain, so a form POST that 302s on to the customer's identity provider was
|
||||||
|
* ABORTED with nothing shown to the user at all. The provider origins cannot be allowlisted
|
||||||
|
* because customers supply them. A script-initiated navigation is not covered by
|
||||||
|
* form-action, so the page asks the server where to go and goes there.
|
||||||
|
*
|
||||||
|
* Styled secondary: "Sign In" is the primary action while a password still works, and two
|
||||||
|
* identical blue buttons stacked one above the other sent people to their IdP by muscle
|
||||||
|
* memory after typing a password.
|
||||||
|
*/
|
||||||
slot.innerHTML = `
|
slot.innerHTML = `
|
||||||
<form method="POST" action="/api/auth/sso/start">
|
<button type="button" id="orgSsoBtn" class="btn ${data.required ? 'btn-primary' : 'btn-secondary'}"
|
||||||
<input type="hidden" name="email" value="${esc(email)}">
|
style="width:100%;justify-content:center;padding:10px">
|
||||||
<button type="submit" class="btn btn-primary" style="width:100%;justify-content:center;padding:10px">
|
${t('auth.signin_sso')}
|
||||||
${t('auth.signin_sso')}
|
</button>
|
||||||
</button>
|
|
||||||
</form>
|
|
||||||
<div style="font-size:11px;color:var(--text-muted);margin-top:6px;text-align:center">
|
<div style="font-size:11px;color:var(--text-muted);margin-top:6px;text-align:center">
|
||||||
${t('auth.sso_org_hint')}
|
${t('auth.sso_org_hint')}
|
||||||
</div>`;
|
</div>`;
|
||||||
slot.style.display = '';
|
slot.style.display = '';
|
||||||
|
|
||||||
|
const btn = slot.querySelector('#orgSsoBtn');
|
||||||
|
if (btn) btn.addEventListener('click', async () => {
|
||||||
|
btn.disabled = true;
|
||||||
|
try {
|
||||||
|
const r = await fetch('/api/auth/sso/start', {
|
||||||
|
method: 'POST',
|
||||||
|
headers: { 'Content-Type': 'application/json', Accept: 'application/json' },
|
||||||
|
body: JSON.stringify({ email }),
|
||||||
|
});
|
||||||
|
const body = await r.json().catch(() => ({}));
|
||||||
|
if (!r.ok || !body.start_url) throw new Error(body.error || `start ${r.status}`);
|
||||||
|
window.location.assign(body.start_url);
|
||||||
|
} catch {
|
||||||
|
btn.disabled = false;
|
||||||
|
showError(t('auth.sso_err_provider_unavailable'));
|
||||||
|
}
|
||||||
|
});
|
||||||
} catch {
|
} catch {
|
||||||
// A failed lookup must never block a password login — the form still works, and the password
|
// A failed lookup must never block a password login — the form still works, and the password
|
||||||
// box comes back rather than leaving someone staring at a form with no way to submit it.
|
// box comes back rather than leaving someone staring at a form with no way to submit it.
|
||||||
|
|
|
||||||
|
|
@ -102,6 +102,15 @@ router.post('/register', (req, res) => {
|
||||||
}
|
}
|
||||||
const { email, password, name, createOrg } = req.body;
|
const { email, password, name, createOrg } = req.body;
|
||||||
if (!email || !password) return res.status(400).json({ error: 'Email and password required' });
|
if (!email || !password) return res.status(400).json({ error: 'Email and password required' });
|
||||||
|
/*
|
||||||
|
* Registration accepted anything with an @ in it, so `<img/src=q/onerror=alert(1)>@acme.test`
|
||||||
|
* became a real row — markup with no spaces, which is why it also slipped the asserted-email
|
||||||
|
* check. Rendering is escaped now, but an address that is not an address has no business being
|
||||||
|
* stored: it is displayed on operator screens, put in emails, and compared against domains.
|
||||||
|
*/
|
||||||
|
if (!ASSERTED_EMAIL_RE.test(String(email).toLowerCase()) || /[<>"'`\\]/.test(String(email))) {
|
||||||
|
return res.status(400).json({ error: 'Enter a valid email address' });
|
||||||
|
}
|
||||||
if (password.length < 8) return res.status(400).json({ error: 'Password must be at least 8 characters' });
|
if (password.length < 8) return res.status(400).json({ error: 'Password must be at least 8 characters' });
|
||||||
|
|
||||||
/*
|
/*
|
||||||
|
|
@ -1168,10 +1177,32 @@ router.get('/sso/discover', (req, res) => {
|
||||||
*/
|
*/
|
||||||
router.post('/sso/start', express.urlencoded({ extended: false }), (req, res) => {
|
router.post('/sso/start', express.urlencoded({ extended: false }), (req, res) => {
|
||||||
const provider = oidcProviders.forEmail((req.body && req.body.email) || req.query.email);
|
const provider = oidcProviders.forEmail((req.body && req.body.email) || req.query.email);
|
||||||
// An unknown domain is answered exactly like a known one that is disabled: back to the login page
|
/*
|
||||||
// with nothing learned.
|
* ⚠️ ANSWER WITH JSON when the page asks for it, rather than a redirect.
|
||||||
if (!provider) return res.redirect('/app#/login?sso_error=unknown_provider');
|
*
|
||||||
res.redirect(`/api/auth/oidc/${encodeURIComponent(provider.slug)}/start`);
|
* This used to be a plain <form method="POST"> that 302'd on to the provider. Chrome applies
|
||||||
|
* `form-action` to the WHOLE redirect chain, and the dashboard's CSP sets `form-action 'self'`
|
||||||
|
* (server.js), so the hop to the identity provider was aborted — silently. The user clicked
|
||||||
|
* "Continue with single sign-on" and NOTHING happened: no navigation, no error, an unchanged
|
||||||
|
* page. Per-organization SSO, the whole point of this feature, could never work in a browser.
|
||||||
|
*
|
||||||
|
* The origins cannot simply be allowlisted: they are supplied by customers at runtime. So the
|
||||||
|
* page fetches this, then navigates itself — a script-initiated navigation is not governed by
|
||||||
|
* form-action. The redirect is kept for a caller without JavaScript, where the chain is
|
||||||
|
* same-origin up to the point the provider's own page takes over.
|
||||||
|
*
|
||||||
|
* The slug in the answer is not a disclosure: following the old redirect put it in the address
|
||||||
|
* bar, the network log and history anyway. What stays private is the mapping for a domain the
|
||||||
|
* caller cannot name — an unknown domain answers exactly like a disabled one.
|
||||||
|
*/
|
||||||
|
const wantsJson = String(req.get('accept') || '').includes('application/json');
|
||||||
|
if (!provider) {
|
||||||
|
if (wantsJson) return res.status(404).json({ error: 'unknown_provider', code: 'unknown_provider' });
|
||||||
|
return res.redirect('/app#/login?sso_error=unknown_provider');
|
||||||
|
}
|
||||||
|
const startUrl = `/api/auth/oidc/${encodeURIComponent(provider.slug)}/start`;
|
||||||
|
if (wantsJson) return res.json({ start_url: startUrl });
|
||||||
|
res.redirect(startUrl);
|
||||||
});
|
});
|
||||||
|
|
||||||
router.get('/oidc/:slug/start', asyncRoute(async (req, res) => {
|
router.get('/oidc/:slug/start', asyncRoute(async (req, res) => {
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue