diff --git a/README.md b/README.md index 470ed97..84c116c 100644 --- a/README.md +++ b/README.md @@ -345,11 +345,17 @@ is filled in for you and their slugs are `google` and `microsoft`: |----------|-------------| | `GOOGLE_CLIENT_ID` | OAuth 2.0 client ID from [Google Cloud Console](https://console.cloud.google.com) | | `MICROSOFT_CLIENT_ID` | Application (client) ID from the [Azure portal](https://portal.azure.com) | -| `MICROSOFT_TENANT_ID` | Tenant ID, or `common` for multi-tenant (default `common`) | +| `MICROSOFT_TENANT_ID` | **Your tenant GUID — required.** `common`/`organizations` are refused | -⚠️ A tenant GUID narrows the accepted issuer to that tenant, so a token from any other tenant is -rejected. `common` accepts any Microsoft account — which is the point of multi-tenant, but make it a -decision rather than a default you inherited. +⚠️ **Multi-tenant Microsoft (`common`) is deliberately refused, and Microsoft sign-in stays disabled +until you set a tenant GUID.** Two reasons that point the same way. It cannot work: Microsoft's +multi-tenant metadata advertises the literal template `https://login.microsoftonline.com/{tenantid}/v2.0`, +so the issuer never matches and every login fails anyway. And the obvious fix is dangerous — accepting +that template means accepting tokens from *every* Azure tenant, which is +[nOAuth](https://www.descope.com/blog/post/noauth): any tenant admin can set an arbitrary, unverified +`email` on one of their own users and be issued a session as that address. Safe multi-tenant support +needs per-tenant pinning (validate `tid` against an allowlist, key accounts on `oid`+`tid` rather than +email) and is not implemented. **Any other provider** is added by slug: @@ -398,6 +404,22 @@ https://yourdomain.com/api/auth/oidc//callback The slug is generated rather than chosen so two customers cannot collide on — or guess — each other's. A domain may be claimed by only one organization; a second claim is refused. +⚠️ **A provider may only authenticate emails inside the domains it registered.** An organization +supplies its own issuer and client ID, so it controls that identity provider completely and could +otherwise assert any address at all — including another company's, or an administrator's. Confining +assertions to registered domains is what makes customer-configurable SSO safe to offer. + +⚠️ **Public email providers cannot be claimed.** `gmail.com`, `outlook.com`, `yahoo.com`, `icloud.com` +and the rest of the consumer mailboxes are refused (`server/lib/public-email-domains.js`). Claiming +one would offer every Gmail user a "sign in with your organization" button pointing at one tenant's +infrastructure — phishing launched from this product's own login page — and would let one account +deny a public domain to everyone else. + +⚠️ **Domain ownership is not yet verified.** A claimed domain currently means "no other organization +had claimed it", not "this organization owns it". The blocklist above removes the mass-abuse case, +but proof of control — a DNS TXT record, or a challenge to `postmaster@` — is still the missing +control, and until it exists a domain claim should be treated as a support-reviewable action. + Signing in through an organization's provider makes the user a member of that organization (`org_member`). Existing members keep whatever role they already have — logging in never promotes or demotes anyone. Client secrets are optional (PKCE), and are stored AES-256-GCM encrypted and never diff --git a/frontend/js/i18n/en.js b/frontend/js/i18n/en.js index 565bd02..24a7d8b 100644 --- a/frontend/js/i18n/en.js +++ b/frontend/js/i18n/en.js @@ -160,6 +160,20 @@ export default { 'sso.f_client_secret_hint': 'Leave blank for a public client — we use PKCE, so a secret is not required. Stored encrypted and never shown again.', 'sso.f_domains': 'Email domains', 'sso.f_domains_hint': 'Comma separated. Anyone with an address at these domains is sent to this provider.', + 'sso.edit': 'Edit', + 'sso.save': 'Save changes', + 'sso.cancel': 'Cancel', + 'sso.secret_set': 'A secret is set — leave blank to keep it', + 'sso.secret_none': 'No secret set (public client)', + 'sso.secret_edit_hint': 'Leave blank to keep the current secret. Type a new one to replace it.', + 'sso.secret_clear': 'Remove the stored secret (use a public client)', + 'sso.test': 'Test', + 'sso.testing': 'Checking the provider…', + 'sso.test_failed': 'Could not reach that provider.', + 'sso.check_discovery': 'OpenID configuration', + 'sso.check_endpoints': 'Authorization and token endpoints', + 'sso.check_signing_keys': 'Signing keys', + 'sso.test_caveat': 'This confirms the provider is reachable and its tokens can be verified. It cannot check the client ID, the secret, or that the redirect URI is registered — only a real sign-in does that.', 'auth.sso_err_expired': 'That sign-in took too long. Please try again.', 'auth.sso_err_bad_state': 'Sign-in could not be verified. Please start again.', 'auth.sso_err_no_code': 'The provider did not return an authorization code.', diff --git a/frontend/js/views/login.js b/frontend/js/views/login.js index 008da5e..4665492 100644 --- a/frontend/js/views/login.js +++ b/frontend/js/views/login.js @@ -1,5 +1,6 @@ import { showToast } from '../components/toast.js'; import { t } from '../i18n.js'; +import { esc } from '../utils.js'; /* @@ -196,10 +197,10 @@ export async function render(container) { self-hoster's Keycloak or Authentik still gets a real-looking button. --> ${(config.providers || []).map((p) => ` ${providerIcon(p.slug)} - ${t('auth.signin_with').replace('{provider}', p.name)} + ${esc(t('auth.signin_with', { provider: p.name }))} `).join('')} @@ -504,10 +505,12 @@ function setupHandlers(config, isSetup) { // Nothing to ask about until there is a domain with a dot in it. if (!domain || !domain.includes('.')) { slot.style.display = 'none'; slot.innerHTML = ''; lastDomainAsked = ''; return; } if (domain === lastDomainAsked) return; - lastDomainAsked = domain; try { const res = await fetch(`/api/auth/sso/discover?email=${encodeURIComponent(email)}`); const data = await res.json(); + // Remembered only after a SUCCESSFUL answer. Recording it before the fetch meant a 5xx or a + // tripped rate limit poisoned that domain for the rest of the page's life. + lastDomainAsked = domain; if (!data.sso) { slot.style.display = 'none'; slot.innerHTML = ''; return; } /* * A FORM, not a link, and a deliberately generic label. @@ -541,23 +544,38 @@ function setupHandlers(config, isSetup) { ssoLookupTimer = setTimeout(() => lookupOrgSso(value), 400); }); + /* + * Completing an SSO login. + * + * The callback no longer hands the session token back in the URL — that was a login-CSRF hole, + * because a crafted link could install an ATTACKER'S token and quietly sign the victim into their + * account. The server now leaves it in a one-shot httpOnly cookie and we exchange it here, which + * a link cannot forge. + * + * Wrapped in an async IIFE because setupHandlers() is not async; `await` at this level is a + * SyntaxError that takes the whole module graph down with it, since app.js imports this file + * statically and there is no bundler to catch it first. + */ const ssoParams = new URLSearchParams((window.location.hash.split('?')[1] || '')); - const ssoToken = ssoParams.get('sso_token'); + const ssoReturning = ssoParams.get('sso') === '1'; const ssoError = ssoParams.get('sso_error'); - if (ssoToken || ssoError) { - history.replaceState(null, '', window.location.pathname + '#/login'); + if (ssoReturning || ssoError) { + // Keep any real query string; only the hash carried the SSO markers. + history.replaceState(null, '', window.location.pathname + window.location.search + '#/login'); } - if (ssoToken) { - try { - const meRes = await fetch('/api/auth/me', { headers: { Authorization: `Bearer ${ssoToken}` } }); - if (!meRes.ok) throw new Error('session rejected'); - const me = await meRes.json(); - onAuthSuccess({ token: ssoToken, user: me.user || me }); - } catch { - showToast(t('auth.sso_failed'), 'error'); - } + if (ssoReturning) { + (async () => { + try { + const res = await fetch('/api/auth/sso/claim', { method: 'POST' }); + if (!res.ok) throw new Error('claim rejected'); + const data = await res.json(); + onAuthSuccess(data); + } catch { + showToast(t('auth.sso_failed'), 'error'); + } + })(); } else if (ssoError) { // Every code the callback can emit has a message; an unknown one still says something true // rather than failing silently, which is how the previous implementation behaved on every click. diff --git a/frontend/js/views/settings.js b/frontend/js/views/settings.js index e0fd4ff..6da3f79 100644 --- a/frontend/js/views/settings.js +++ b/frontend/js/views/settings.js @@ -704,6 +704,8 @@ export async function render(container) {
${esc(t('sso.domains_label'))}: ${esc(p.email_domains || '—')}
+ + @@ -716,6 +718,38 @@ export async function render(container) {
${esc(t('sso.callback_label'))}
${esc(origin + p.callback_url)}
+ + + + `).join(''); listEl.querySelectorAll('[data-sso-toggle]').forEach((btn) => { @@ -723,6 +757,89 @@ export async function render(container) { await ssoRequest('PUT', `/${btn.dataset.ssoToggle}`, { enabled: btn.dataset.enabled !== '1' }); }); }); + listEl.querySelectorAll('[data-sso-test]').forEach((btn) => { + btn.addEventListener('click', async () => { + const id = btn.dataset.ssoTest; + const out = document.getElementById(`ssoTest-${id}`); + if (!out) return; + out.style.display = ''; + out.textContent = t('sso.testing'); + try { + const res = await fetch(`/api/organizations/${orgId}/sso/${id}/test`, { + method: 'POST', + headers: { Authorization: `Bearer ${localStorage.getItem('token')}` }, + }); + const data = await res.json(); + if (!res.ok) { out.textContent = data.error || t('sso.test_failed'); return; } + /* + * Literal keys, never a key built by concatenating a check name. Doing that defeats the + * check in server/test/i18n-keys-exist.js that every key an operator can see is + * translated — and a check name the UI does not know would render as raw key text. The + * fallback keeps an unknown one readable instead. + */ + const CHECK_LABELS = { + discovery: t('sso.check_discovery'), + endpoints: t('sso.check_endpoints'), + signing_keys: t('sso.check_signing_keys'), + }; + const rows = (data.checks || []).map((c) => ` +
${c.ok ? '✅' : '❌'} ${esc(CHECK_LABELS[c.name] || c.name)} — ${esc(c.detail || '')}
`).join(''); + /* + * The caveat is shown on SUCCESS, not tucked away. Discovery and keys prove the provider + * exists and that we could verify a token it signs — they say nothing about whether the + * client id, the secret, or the redirect URI registration are right. A green tick that + * implied "SSO works" would send an admin away from the one thing still to check. + */ + out.innerHTML = rows + (data.ok + ? `
${esc(t('sso.test_caveat'))}
` + : ''); + } catch { + out.textContent = t('sso.test_failed'); + } + }); + }); + listEl.querySelectorAll('[data-sso-edit]').forEach((btn) => { + btn.addEventListener('click', () => { + const panel = document.getElementById(`ssoEdit-${btn.dataset.ssoEdit}`); + if (panel) panel.style.display = panel.style.display === 'none' ? 'block' : 'none'; + }); + }); + listEl.querySelectorAll('[data-sso-cancel]').forEach((btn) => { + btn.addEventListener('click', () => { + const panel = document.getElementById(`ssoEdit-${btn.dataset.ssoCancel}`); + if (panel) panel.style.display = 'none'; + }); + }); + listEl.querySelectorAll('[data-sso-save]').forEach((btn) => { + btn.addEventListener('click', async () => { + const panel = document.getElementById(`ssoEdit-${btn.dataset.ssoSave}`); + if (!panel) return; + const val = (f) => panel.querySelector(`[data-f="${f}"]`)?.value?.trim() ?? ''; + const body = { + name: val('name'), + issuer: val('issuer'), + client_id: val('client_id'), + email_domains: val('email_domains'), + }; + /* + * Three states, and only these three: + * typed a value -> replace the secret + * ticked "remove" -> send '' so the server clears it + * left blank, unticked -> send NOTHING, so the stored secret survives + * Sending '' on every save is the bug this shape exists to avoid. + */ + const typed = panel.querySelector('[data-f="client_secret"]')?.value || ''; + const clearing = panel.querySelector('[data-f="clear_secret"]')?.checked; + if (typed) body.client_secret = typed; + else if (clearing) body.client_secret = ''; + + if (!body.name || !body.issuer || !body.client_id) { + showToast(t('sso.missing_fields'), 'error'); + return; + } + await ssoRequest('PUT', `/${btn.dataset.ssoSave}`, body); + }); + }); listEl.querySelectorAll('[data-sso-delete]').forEach((btn) => { btn.addEventListener('click', async () => { if (!confirm(t('sso.confirm_delete'))) return; diff --git a/server/lib/oidc-providers.js b/server/lib/oidc-providers.js index 8249ee8..5371476 100644 --- a/server/lib/oidc-providers.js +++ b/server/lib/oidc-providers.js @@ -74,20 +74,45 @@ function list(env = process.env) { const msId = (env.MICROSOFT_CLIENT_ID || '').trim(); if (msId) { - // `common` lets any Microsoft account in, which is what multi-tenant means and is the documented - // default. A single-tenant deployment sets the tenant GUID and the issuer narrows with it, so a - // token from another tenant then fails the iss check rather than being silently accepted. - const tenant = (env.MICROSOFT_TENANT_ID || 'common').trim() || 'common'; - out.push({ - slug: 'microsoft', - name: 'Microsoft', - issuer: `https://login.microsoftonline.com/${tenant}/v2.0`, - clientId: msId, - clientSecret: (env.MICROSOFT_CLIENT_SECRET || '').trim() || null, - scopes: DEFAULT_SCOPES, - source: 'env', - }); - seen.add('microsoft'); + /* + * ⚠️ A TENANT GUID IS REQUIRED. `common` and `organizations` are refused, for two reasons that + * point the same way. + * + * It does not work: Microsoft's multi-tenant metadata advertises + * `https://login.microsoftonline.com/{tenantid}/v2.0` — a literal template — so the issuer can + * never equal the configured URL and every login fails at /start regardless. + * + * And the obvious patch is dangerous: loosening the `iss` comparison to accept the template + * means accepting tokens from EVERY Azure tenant, which is nOAuth — an admin of any tenant can + * set an arbitrary, unverified `email` on one of their own users and be issued a session as that + * address here. Doing multi-tenant Microsoft safely needs per-tenant pinning (validate `tid` + * against an allowlist and key the account on `oid`+`tid`, not on email), which is a feature, + * not a relaxed regex. + * + * So: refuse loudly at boot rather than ship a login that either never works or works too well. + */ + const rawTenant = (env.MICROSOFT_TENANT_ID || '').trim().toLowerCase(); + if (!rawTenant || ['common', 'organizations', 'consumers'].includes(rawTenant)) { + if (!list._warned) { + console.warn('[sso] MICROSOFT_CLIENT_ID is set but MICROSOFT_TENANT_ID is missing or multi-tenant ' + + `(${rawTenant || 'unset'}). Microsoft sign-in is DISABLED: set your tenant GUID. See README.`); + list._warned = true; + } + seen.add('microsoft'); + } else { + out.push({ + slug: 'microsoft', + name: 'Microsoft', + // A tenant GUID narrows the issuer to that tenant, so a token from any other tenant fails + // the `iss` check instead of being quietly accepted. + issuer: `https://login.microsoftonline.com/${rawTenant}/v2.0`, + clientId: msId, + clientSecret: (env.MICROSOFT_CLIENT_SECRET || '').trim() || null, + scopes: DEFAULT_SCOPES, + source: 'env', + }); + seen.add('microsoft'); + } } for (const raw of String(env.OIDC_PROVIDERS || '').split(',')) { @@ -143,10 +168,19 @@ function rowToProvider(row, secretbox) { name: row.name, issuer: String(row.issuer).replace(/\/+$/, ''), clientId: row.client_id, - clientSecret: row.client_secret_enc ? secretbox.decrypt(row.client_secret_enc) : null, + /* + * Fail CLOSED. secretbox.decrypt returns null when the key has rotated, which silently turned a + * confidential client into a public one — the login then fails at the provider with an error + * nobody can act on, while the admin screen still says "a secret is set". + */ + clientSecret: row.client_secret_enc + ? (secretbox.decrypt(row.client_secret_enc) ?? (() => { throw new Error('client secret could not be decrypted — re-enter it'); })()) + : null, scopes: row.scopes || DEFAULT_SCOPES, source: 'org', organizationId: row.organization_id, + // Carried so the callback can refuse an assertion outside the domains this customer registered. + emailDomains: row.email_domains || '', }; } @@ -158,7 +192,16 @@ function getOrgProvider(slug) { const row = conn.prepare('SELECT * FROM org_sso_providers WHERE slug = ? AND enabled = 1').get(String(slug)); if (!row) return null; return rowToProvider(row, require('./secretbox')); - } catch { return null; } // table not migrated yet + } catch (e) { + /* + * Only "the table is not there yet" is a null. This catch used to swallow EVERYTHING, which + * turned a secret that could not be decrypted back into a silent success — the exact failure the + * fail-closed check above exists to prevent. Anything else propagates so it is logged and the + * login fails loudly. + */ + if (/no such table/i.test(e.message)) return null; + throw e; + } } /** @@ -179,7 +222,7 @@ function forEmail(email) { const domain = String(email).slice(at + 1).toLowerCase().trim(); if (!domain) return null; try { - const rows = conn.prepare("SELECT * FROM org_sso_providers WHERE enabled = 1 AND email_domains != ''").all(); + const rows = conn.prepare("SELECT * FROM org_sso_providers WHERE enabled = 1 AND email_domains != '' ORDER BY created_at, id").all(); const secretbox = require('./secretbox'); for (const row of rows) { const domains = String(row.email_domains || '').split(',').map((d) => d.trim().toLowerCase()).filter(Boolean); diff --git a/server/lib/oidc.js b/server/lib/oidc.js index 010fc2a..9187dc0 100644 --- a/server/lib/oidc.js +++ b/server/lib/oidc.js @@ -44,12 +44,45 @@ const FETCH_TIMEOUT_MS = 8000; const discoveryCache = new Map(); // issuer -> { at, doc } const jwksCache = new Map(); // jwks_uri -> { at, keys } +/* + * Every URL this module fetches is ultimately chosen by whoever configured the provider — and since + * per-org SSO, that is a CUSTOMER, not the operator. Discovery, JWKS and the token endpoint are + * therefore server-side request forgery primitives unless they are constrained. + * + * Two rules, both cheap: + * https only — an http:// target is a plaintext credential leak as well as a way to reach + * services that never expected a request from inside the network. + * public hosts only — loopback, RFC1918, link-local (169.254.169.254 is cloud metadata) and the + * IPv6 equivalents are refused outright. + * + * ⚠️ This is a literal-address check, not full SSRF protection: a hostname that RESOLVES to a + * private address still passes, because refusing that needs resolve-then-pin plumbing that Node's + * fetch does not expose. It raises the bar from "type an internal URL" to "control public DNS", + * and the README says so rather than implying more. + */ +const BLOCKED_HOST = /^(localhost|.*\.localhost|127\.|0\.0\.0\.0|10\.|192\.168\.|169\.254\.|172\.(1[6-9]|2\d|3[01])\.|\[?::1\]?|\[?f[cd])/i; + +function assertFetchable(url) { + let u; + try { u = new URL(url); } catch { throw new Error(`not a URL: ${url}`); } + if (u.protocol !== 'https:') throw new Error('provider URLs must use https'); + if (BLOCKED_HOST.test(u.hostname)) throw new Error('provider host is not publicly routable'); + return u; +} + /** fetch with a timeout, because a hanging IdP must not hang a login forever. */ async function getJson(url) { + assertFetchable(url); const ctl = new AbortController(); const timer = setTimeout(() => ctl.abort(), FETCH_TIMEOUT_MS); try { - const res = await fetch(url, { signal: ctl.signal, redirect: 'follow' }); + /* + * redirect: 'manual' — following redirects would let an allowlisted host bounce us to a blocked + * one, which defeats the check above entirely. A provider that redirects its own well-known + * document is misconfigured, and saying so is more useful than quietly following it. + */ + const res = await fetch(url, { signal: ctl.signal, redirect: 'manual' }); + if (res.status >= 300 && res.status < 400) throw new Error(`${url} redirected; provider URLs must be final`); if (!res.ok) throw new Error(`${url} responded ${res.status}`); return await res.json(); } finally { @@ -192,7 +225,8 @@ async function exchangeCode({ issuer, clientId, clientSecret, code, redirectUri, const timer = setTimeout(() => ctl.abort(), FETCH_TIMEOUT_MS); let payload; try { - const res = await fetch(doc.token_endpoint, { method: 'POST', headers, body, signal: ctl.signal }); + assertFetchable(doc.token_endpoint); + const res = await fetch(doc.token_endpoint, { method: 'POST', headers, body, signal: ctl.signal, redirect: 'manual' }); payload = await res.json().catch(() => ({})); if (!res.ok) { // The provider's own error is far more useful than "exchange failed" — a wrong redirect_uri @@ -213,8 +247,17 @@ function _resetCaches() { jwksCache.clear(); } +/** + * The provider's published keys, straight from the document. Used by the configuration test so an + * admin learns at setup time that a provider publishes no signing keys, rather than at first login. + */ +async function fetchJwks(jwksUri) { + return getJson(jwksUri); +} + module.exports = { discover, + fetchJwks, verifyIdToken, exchangeCode, createPkce, diff --git a/server/lib/public-email-domains.js b/server/lib/public-email-domains.js new file mode 100644 index 0000000..ec4b64b --- /dev/null +++ b/server/lib/public-email-domains.js @@ -0,0 +1,57 @@ +'use strict'; + +/* + * Email domains nobody may claim for organization SSO. + * + * Per-org SSO routes everyone at a domain to that organization's identity provider. Applied to a + * company domain that is the point. Applied to a CONSUMER domain it is an attack: one tenant claims + * `gmail.com`, and from then on every Gmail user who types their address into this product's login + * page is offered a "sign in with your organization" button that sends them to infrastructure the + * tenant controls — phishing launched from the vendor's own trusted login screen. First-claim-wins + * also lets one cheap account deny a public domain to everyone else, permanently. + * + * ⚠️ This is a floor, not a ceiling. It stops the mass-abuse case; it does NOT stop a tenant + * claiming a domain that belongs to some specific other company. Only proof of control — a DNS TXT + * record, or a challenge to postmaster@ — settles that, and until it exists a claimed domain means + * "nobody else had claimed it", not "they own it". + * + * Kept as data, in one file, because it is a list that will need adding to and that is the cheapest + * possible edit. Matching is exact on the registrable domain, so `mail.google.com` is not blocked by + * `gmail.com` — subdomains of consumer providers are not a realistic sign-in domain anyway. + */ + +const PUBLIC_EMAIL_DOMAINS = new Set([ + // Google + 'gmail.com', 'googlemail.com', + // Microsoft + 'outlook.com', 'outlook.co.uk', 'hotmail.com', 'hotmail.co.uk', 'hotmail.fr', 'hotmail.it', + 'live.com', 'live.co.uk', 'msn.com', 'passport.com', + // Yahoo and friends + 'yahoo.com', 'yahoo.co.uk', 'yahoo.co.jp', 'yahoo.fr', 'yahoo.de', 'yahoo.ca', 'yahoo.com.au', + 'ymail.com', 'rocketmail.com', 'aol.com', 'aim.com', + // Apple + 'icloud.com', 'me.com', 'mac.com', + // Privacy-focused + 'proton.me', 'protonmail.com', 'pm.me', 'tutanota.com', 'tutanota.de', 'tuta.io', 'tuta.com', + 'duck.com', 'hey.com', 'fastmail.com', 'fastmail.fm', + // Other large consumer providers + 'gmx.com', 'gmx.de', 'gmx.net', 'gmx.at', 'gmx.ch', 'web.de', 'mail.com', 'email.com', + 'zoho.com', 'zohomail.com', 'yandex.com', 'yandex.ru', 'ya.ru', 'mail.ru', 'bk.ru', 'inbox.ru', + 'list.ru', 'rambler.ru', + 'qq.com', 'foxmail.com', '163.com', '126.com', 'sina.com', 'sina.cn', 'naver.com', 'daum.net', + 'hanmail.net', 'rediffmail.com', + // ISP-style mailboxes, where the domain belongs to the ISP and not to any customer + 'comcast.net', 'verizon.net', 'att.net', 'sbcglobal.net', 'bellsouth.net', 'cox.net', + 'charter.net', 'earthlink.net', 'juno.com', 'optonline.net', 'roadrunner.com', + 'btinternet.com', 'sky.com', 'virginmedia.com', 'talktalk.net', 'orange.fr', 'wanadoo.fr', + 'free.fr', 'laposte.net', 'libero.it', 'virgilio.it', 'tiscali.it', 'terra.com.br', 'uol.com.br', + 'bol.com.br', 'telus.net', 'shaw.ca', 'rogers.com', 'sympatico.ca', 'bigpond.com', 'optusnet.com.au', + 't-online.de', 'freenet.de', 'arcor.de', +]); + +/** True when this domain is a consumer mailbox provider rather than an organization's own domain. */ +function isPublicEmailDomain(domain) { + return PUBLIC_EMAIL_DOMAINS.has(String(domain || '').trim().toLowerCase()); +} + +module.exports = { PUBLIC_EMAIL_DOMAINS, isPublicEmailDomain }; diff --git a/server/routes/auth.js b/server/routes/auth.js index e6992dc..ea99a0d 100644 --- a/server/routes/auth.js +++ b/server/routes/auth.js @@ -841,6 +841,8 @@ router.post('/accept-invite/:inviteId', requireAuth, (req, res) => { // The transaction is held in a short-lived signed cookie rather than server memory so that a // restart mid-login, or a second server process, does not strand the user on a dead state. const OIDC_TX_COOKIE = 'st_oidc_tx'; +// Holds a completed session for the seconds between the provider redirect and the page claiming it. +const SSO_CLAIM_COOKIE = 'st_sso_claim'; const OIDC_TX_TTL_S = 600; function readCookie(req, name) { @@ -926,9 +928,11 @@ router.get('/oidc/:slug/start', async (req, res) => { const state = oidc.randomToken(); const tx = jwt.sign( - { slug: provider.slug, nonce, verifier: pkce.verifier, state }, + { typ: 'oidc-tx', slug: provider.slug, nonce, verifier: pkce.verifier, state }, config.jwtSecret, - { expiresIn: OIDC_TX_TTL_S }, + // HS256 explicitly, and a `typ` the session verifier does not accept: two token kinds signed + // with one secret must never be interchangeable, even if today only `slug` happens to stop it. + { expiresIn: OIDC_TX_TTL_S, algorithm: 'HS256' }, ); res.cookie(OIDC_TX_COOKIE, tx, { httpOnly: true, @@ -970,7 +974,8 @@ router.get('/oidc/:slug/callback', async (req, res) => { let tx; try { - tx = jwt.verify(raw, config.jwtSecret); + tx = jwt.verify(raw, config.jwtSecret, { algorithms: ['HS256'] }); + if (tx.typ !== 'oidc-tx') throw new Error('not a login transaction'); } catch { return backToApp(res, { sso_error: 'expired' }); } @@ -979,8 +984,18 @@ router.get('/oidc/:slug/callback', async (req, res) => { // Compared in constant time so a wrong state cannot be discovered a character at a time. const got = String(req.query.state || ''); const want = String(tx.state || ''); - const sameLength = got.length === want.length; - if (!sameLength || !crypto.timingSafeEqual(Buffer.from(got), Buffer.from(want))) { + /* + * Compared as BYTES, not characters. + * + * `got.length` is UTF-16 code units; Buffer.from() produces UTF-8 bytes. A state of 43 characters + * containing one multi-byte character is 43 chars but 44 bytes, so the guard passed and + * timingSafeEqual threw ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH — inside an async handler, which + * Express 4 does not catch, which server.js turns into process.exit. One crafted request per + * restart was enough to take an instance down. + */ + const gotBuf = Buffer.from(got, 'utf8'); + const wantBuf = Buffer.from(want, 'utf8'); + if (gotBuf.length !== wantBuf.length || !crypto.timingSafeEqual(gotBuf, wantBuf)) { return backToApp(res, { sso_error: 'bad_state' }); } if (tx.slug !== provider.slug) return backToApp(res, { sso_error: 'bad_state' }); @@ -1008,13 +1023,40 @@ router.get('/oidc/:slug/callback', async (req, res) => { const email = String(claims.email || '').toLowerCase().trim(); if (!email) return backToApp(res, { sso_error: 'no_email' }); + + /* + * ⚠️ AN ORGANIZATION'S PROVIDER MAY ONLY SPEAK FOR ITS OWN DOMAINS. + * + * Without this, per-org SSO is an account-takeover primitive, demonstrated end to end twice in + * review: any org owner can point us at an identity provider they fully control, and such a + * provider can assert ANY email with email_verified:true — including a platform_admin's. Every + * check passes honestly, because the attacker IS the issuer. + * + * Instance-wide providers are exempt: the OPERATOR chose them, which is the trust they have + * always had. An org provider is chosen by a customer, so it is confined to the domains that + * customer registered — and a domain cannot be registered while another organization holds it. + * + * ⚠️ This bounds the damage to domains a tenant claimed; it does NOT prove they own them. + * Claiming an unheld public domain is still possible and needs DNS verification. See the README. + */ + if (provider.organizationId) { + const at = email.lastIndexOf('@'); + const domain = at === -1 ? '' : email.slice(at + 1); + const allowed = String(provider.emailDomains || '').split(',').map((d) => d.trim()).filter(Boolean); + if (!domain || !allowed.includes(domain)) { + console.warn(`[oidc] ${provider.slug} asserted ${email}, outside its domains [${allowed.join(', ')}]`); + return backToApp(res, { sso_error: 'domain_not_allowed' }); + } + } /* * An unverified email is refused. The whole account model keys on email — linking, invites, * password reset — so accepting an address the provider itself will not vouch for would let * anyone who can type an address into a sloppy IdP arrive as its owner. Providers that omit the * claim entirely are treated as "not asserted", which is the same answer. */ - if (claims.email_verified === false) return backToApp(res, { sso_error: 'email_unverified' }); + // `=== false` accepted an OMITTED claim, which is the opposite of what the comment above says and + // what Azure AD v2 actually sends (it omits it). Absent means not asserted, which is not verified. + if (claims.email_verified !== true) return backToApp(res, { sso_error: 'email_unverified' }); try { const result = upsertFederatedUser({ claims, email, provider, req }); @@ -1046,13 +1088,57 @@ router.get('/oidc/:slug/callback', async (req, res) => { const workspaceId = ensureDefaultOrgForUser(user, { allowCreate: config.autoCreateOrgOnSignup }); const token = generateToken(user, workspaceId); if (isNew) sendSignupEmails(user, req); - backToApp(res, { sso_token: token }); + + /* + * ⚠️ The session token is NOT put in the redirect URL. + * + * An earlier version returned it in the fragment. That is a login-CSRF hole: anyone could send + * a victim `/app#/login?sso_token=` and the page would install it, silently + * signing that person into the ATTACKER'S account — after which their uploads, playlists and + * settings all land somewhere the attacker can read. + * + * Instead the token goes into a one-shot httpOnly cookie that only this origin can set, and the + * page exchanges it at /sso/claim. A link cannot forge that cookie, so a token can only be + * claimed by the browser that actually completed the login. + */ + res.cookie(SSO_CLAIM_COOKIE, token, { + httpOnly: true, + sameSite: 'lax', + secure: req.protocol === 'https', + maxAge: 120 * 1000, + path: '/api/auth', + }); + backToApp(res, { sso: '1' }); } catch (err) { console.error(`[oidc] ${provider.slug} sign-in failed:`, err.message); backToApp(res, { sso_error: 'server_error' }); } }); +/* + * Exchange the one-shot cookie for the session token. + * + * POST so it cannot be triggered by a link or an , and the cookie is cleared on the way out so + * a second attempt gets nothing — a token that leaks from a log or a back button is already spent. + */ +router.post('/sso/claim', (req, res) => { + const token = readCookie(req, SSO_CLAIM_COOKIE); + res.clearCookie(SSO_CLAIM_COOKIE, { path: '/api/auth' }); + if (!token) return res.status(401).json({ error: 'No sign-in to complete' }); + + let claims; + try { + claims = jwt.verify(token, config.jwtSecret); + } catch { + return res.status(401).json({ error: 'That sign-in has expired' }); + } + const user = db.prepare('SELECT * FROM users WHERE id = ?').get(claims.id); + if (!user) return res.status(401).json({ error: 'That sign-in has expired' }); + + const { password_hash, totp_secret_enc, totp_last_step, ...safeUser } = user; + res.json({ token, user: safeUser, current_workspace_id: claims.current_workspace_id || null }); +}); + /* * Find or create the account behind a verified set of claims. * @@ -1082,6 +1168,16 @@ function upsertFederatedUser({ claims, email, provider, req }) { if (existing.auth_provider !== provider.slug) { if (existing.password_hash) return { error: 'account_exists_local' }; + /* + * `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 + * last" — fine when the operator chose them all, an account takeover once a customer can add + * one. An ORG provider therefore never adopts an account another provider established; the user + * links it deliberately instead. + */ + if (provider.organizationId && existing.auth_provider && existing.auth_provider !== 'local') { + return { error: 'account_exists_other_provider' }; + } db.prepare('UPDATE users SET auth_provider = ?, provider_id = ?, avatar_url = ? WHERE id = ?') .run(provider.slug, String(claims.sub), claims.picture || existing.avatar_url, existing.id); return { user: db.prepare('SELECT * FROM users WHERE id = ?').get(existing.id), isNew: false }; diff --git a/server/routes/org-sso.js b/server/routes/org-sso.js index d19168e..cf951a8 100644 --- a/server/routes/org-sso.js +++ b/server/routes/org-sso.js @@ -21,6 +21,7 @@ const { resolveTenancy } = require('../lib/tenancy'); const secretbox = require('../lib/secretbox'); const oidc = require('../lib/oidc'); const { logActivity, getClientIp } = require('../services/activity'); +const { isPublicEmailDomain } = require('../lib/public-email-domains'); /* * Only an org owner/admin may configure how their people sign in — it is the most security-relevant @@ -80,6 +81,18 @@ function normaliseDomains(raw) { if (!/^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)+$/.test(d)) { throw new Error(`"${part.trim()}" is not a valid email domain`); } + /* + * A consumer mailbox provider is never an organization's sign-in domain, and claiming one is an + * attack rather than a mistake: every Gmail or Outlook user typing their address into this + * product's login page would be offered a "sign in with your organization" button pointing at + * one tenant's infrastructure. It also lets one cheap account deny a public domain to everyone. + */ + if (isPublicEmailDomain(d)) { + const e = new Error(`${d} is a public email provider and cannot be used as a sign-in domain. ` + + 'Use a domain your organization owns.'); + e.status = 400; + throw e; + } seen.add(d); } return [...seen].join(','); @@ -100,8 +113,13 @@ function assertDomainsFree(domains, orgId, excludeId) { if (row.id === excludeId) continue; const held = String(row.email_domains).split(','); for (const d of wanted) { - if (held.includes(d) && row.organization_id !== orgId) { - const e = new Error(`the domain ${d} is already used for sign-in by another organization`); + if (held.includes(d)) { + // Same-org duplicates were allowed and should not have been: two providers claiming one + // domain makes routing depend on table-scan order, so half a company's staff get sent to an + // identity provider that has never heard of them. + const e = new Error(row.organization_id === orgId + ? `the domain ${d} is already used by another of your providers` + : `the domain ${d} is already used for sign-in by another organization`); e.status = 409; throw e; } @@ -145,14 +163,28 @@ router.post('/:orgId/sso', requireOrgAdmin, async (req, res) => { const id = crypto.randomUUID(); const slug = newSlug(); - db.prepare(` - INSERT INTO org_sso_providers (id, organization_id, slug, name, issuer, client_id, client_secret_enc, scopes, email_domains, enabled) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, 1) - `).run(id, req.orgId, slug, String(name).trim(), String(issuer).trim().replace(/\/+$/, ''), String(clientId).trim(), - clientSecret ? secretbox.encrypt(String(clientSecret)) : null, - String(scopes || 'openid email profile').trim(), cleanDomains); + /* + * Re-check the domains INSIDE the transaction. The first check happened before `await + * oidc.discover()`, which yields the event loop for a network round trip the caller's own IdP + * controls the length of — two admins racing that window both passed and both got the domain, + * after which routing became whichever row the scan reached first. + */ + try { + db.transaction(() => { + assertDomainsFree(cleanDomains, req.orgId, null); + db.prepare(` + INSERT INTO org_sso_providers (id, organization_id, slug, name, issuer, client_id, client_secret_enc, scopes, email_domains, enabled) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, 1) + `).run(id, req.orgId, slug, String(name).trim(), String(issuer).trim().replace(/\/+$/, ''), String(clientId).trim(), + clientSecret ? secretbox.encrypt(String(clientSecret)) : null, + String(scopes || 'openid email profile').trim(), cleanDomains); + })(); + } catch (e) { + return res.status(e.status || 500).json({ error: e.message }); + } - logActivity(req.user.id, 'org_sso_created', `${name} (${slug})`, req.orgId, getClientIp(req)); + // (userId, action, details, deviceId, ipAddress, workspaceId) — the org id is NOT the 4th arg. + logActivity(req.user.id, 'org_sso_created', `${name} (${slug}) org=${req.orgId}`, null, getClientIp(req)); res.status(201).json(toPublic(db.prepare('SELECT * FROM org_sso_providers WHERE id = ?').get(id))); }); @@ -202,15 +234,74 @@ router.put('/:orgId/sso/:id', requireOrgAdmin, async (req, res) => { existing.id, ); - logActivity(req.user.id, 'org_sso_updated', `${existing.name} (${existing.slug})`, req.orgId, getClientIp(req)); + logActivity(req.user.id, 'org_sso_updated', `${existing.name} (${existing.slug}) org=${req.orgId}`, null, getClientIp(req)); res.json(toPublic(db.prepare('SELECT * FROM org_sso_providers WHERE id = ?').get(existing.id))); }); +/* + * Check a provider without making anyone log in. + * + * The overwhelmingly common failure is a configuration one — an issuer that is a company home page + * rather than an OIDC issuer, a provider that is unreachable from the server, a JWKS with no signing + * keys — and every one of those currently surfaces as a user staring at a failed login with an + * error that says nothing useful. This turns that into an answer at configuration time. + * + * ⚠️ It is deliberately honest about its limits. Discovery and JWKS prove the provider EXISTS and + * that we could verify a token it signed. They cannot prove the client id is right, that the secret + * matches, or that the redirect URI is registered — only a real authorization round trip does that, + * and the response says so rather than implying a green tick means "SSO works". + */ +router.post('/:orgId/sso/:id/test', requireOrgAdmin, async (req, res) => { + const row = db.prepare('SELECT * FROM org_sso_providers WHERE id = ? AND organization_id = ?').get(req.params.id, req.orgId); + if (!row) return res.status(404).json({ error: 'Not found' }); + + const checks = []; + let doc = null; + try { + doc = await oidc.discover(row.issuer); + checks.push({ name: 'discovery', ok: true, detail: doc.issuer }); + } catch (e) { + checks.push({ name: 'discovery', ok: false, detail: e.message }); + return res.json({ ok: false, checks }); + } + + checks.push({ + name: 'endpoints', + ok: !!(doc.authorization_endpoint && doc.token_endpoint), + detail: doc.authorization_endpoint || 'missing authorization_endpoint', + }); + + try { + const jwks = await oidc.fetchJwks(doc.jwks_uri); + const signing = (jwks.keys || []).filter((k) => !k.use || k.use === 'sig'); + checks.push({ + name: 'signing_keys', + ok: signing.length > 0, + detail: signing.length ? `${signing.length} key(s)` : 'the provider published no signing keys', + }); + } catch (e) { + // Deliberately generic. `jwks_uri` comes from the CALLER'S OWN discovery document, so echoing + // the upstream status here turned this endpoint into a readable internal port scanner. + checks.push({ name: 'signing_keys', ok: false, detail: 'could not read the provider keys' }); + } + + // What the admin must have registered at the provider — the single most common thing to get + // wrong, and something we can state exactly rather than ask them to guess. + const origin = (process.env.APP_URL || '').trim().replace(/\/+$/, '') || `${req.protocol}://${req.get('host')}`; + res.json({ + ok: checks.every((c) => c.ok), + checks, + redirect_uri: `${origin}/api/auth/oidc/${row.slug}/callback`, + // Said plainly so a passing test is not mistaken for a working login. + note: 'unverifiable_by_test', + }); +}); + router.delete('/:orgId/sso/:id', requireOrgAdmin, (req, res) => { const existing = db.prepare('SELECT * FROM org_sso_providers WHERE id = ? AND organization_id = ?').get(req.params.id, req.orgId); if (!existing) return res.status(404).json({ error: 'Not found' }); db.prepare('DELETE FROM org_sso_providers WHERE id = ?').run(existing.id); - logActivity(req.user.id, 'org_sso_deleted', `${existing.name} (${existing.slug})`, req.orgId, getClientIp(req)); + logActivity(req.user.id, 'org_sso_deleted', `${existing.name} (${existing.slug}) org=${req.orgId}`, null, getClientIp(req)); res.json({ success: true }); }); diff --git a/server/server.js b/server/server.js index 43939a1..acb25b8 100644 --- a/server/server.js +++ b/server/server.js @@ -576,7 +576,12 @@ app.use('/api/auth/resend-verification', rateLimit(60000, 5)); // Domain lookup is unauthenticated by necessity (it runs before login). Rate limited so it // cannot be walked to enumerate which customers use SSO. app.use('/api/auth/sso/discover', rateLimit(60000, 10)); -app.use('/api/auth/sso/start', rateLimit(60000, 10)); +// 10/min was wrong for this one: org SSO is used by companies behind a SINGLE corporate egress IP, +// and this is their only entry point, so the 11th employee of the morning met a raw JSON 429 with no +// login page. It is a redirect, not a credential check. +app.use('/api/auth/sso/start', rateLimit(60000, 120)); +// The OIDC endpoints had no limit at all, which left the callback's parsing as a free amplifier. +app.use('/api/auth/oidc', rateLimit(60000, 120)); // Self-service password reset. The request endpoint is the spray surface (it sends mail to // an address the caller supplies), so it gets the tighter cap; the redeem endpoint is a // 32-byte-token guess, capped mostly to keep the bcrypt work bounded. diff --git a/server/test/oidc-sso.test.js b/server/test/oidc-sso.test.js index adeb917..b006974 100644 --- a/server/test/oidc-sso.test.js +++ b/server/test/oidc-sso.test.js @@ -215,11 +215,9 @@ test('every login gets fresh values', () => { // --------------------------------------------------------------------------------------------- // The provider registry -test('Google and Microsoft register from the variables the README always documented', () => { - const list = providers.list({ GOOGLE_CLIENT_ID: 'g', MICROSOFT_CLIENT_ID: 'm', MICROSOFT_TENANT_ID: 'common' }); - const byslug = Object.fromEntries(list.map((p) => [p.slug, p])); - assert.equal(byslug.google.issuer, 'https://accounts.google.com'); - assert.equal(byslug.microsoft.issuer, 'https://login.microsoftonline.com/common/v2.0'); +test('Google registers from the variable the README always documented', () => { + const [g] = providers.list({ GOOGLE_CLIENT_ID: 'g' }); + assert.equal(g.issuer, 'https://accounts.google.com'); }); test('a single-tenant Microsoft app narrows the issuer, so another tenant fails iss', () => { @@ -227,6 +225,22 @@ test('a single-tenant Microsoft app narrows the issuer, so another tenant fails assert.equal(ms.issuer, 'https://login.microsoftonline.com/abc-123/v2.0'); }); +test('MULTI-TENANT MICROSOFT IS REFUSED, not silently broken', () => { + /* + * Two reasons pointing the same way. It cannot work: Microsoft's `common` metadata advertises the + * literal template `https://login.microsoftonline.com/{tenantid}/v2.0`, so the issuer can never + * equal the configured URL and every login fails at /start anyway. + * + * And the obvious patch is dangerous: loosening the iss comparison accepts tokens from EVERY + * Azure tenant, which is nOAuth — any tenant admin can set an arbitrary unverified `email` on + * their own user and be issued a session as that address here. + */ + for (const tenant of ['common', 'organizations', 'consumers', '']) { + assert.deepEqual(providers.list({ MICROSOFT_CLIENT_ID: 'm', MICROSOFT_TENANT_ID: tenant }), [], + `MICROSOFT_TENANT_ID=${tenant || '(unset)'} must not register a provider`); + } +}); + test('any OIDC provider can be added by env', () => { const list = providers.list({ OIDC_PROVIDERS: 'authentik', @@ -362,3 +376,81 @@ test('no database means no org providers, and no crash', () => { const m = require('../lib/oidc-providers'); assert.doesNotThrow(() => m.publicList({ GOOGLE_CLIENT_ID: 'g' })); }); + + +// --------------------------------------------------------------------------------------------- +// Regressions for defects found in security review. Each one was demonstrated end to end against a +// running server before it was fixed; none of them was hypothetical. + +test('TAKEOVER: an org provider may not assert an email outside its own domains', () => { + /* + * The worst defect in this feature. An org admin supplies the issuer and client id, so they + * control the IdP completely and can mint a token asserting ANY email with email_verified:true — + * including a platform_admin's. Every cryptographic check passes honestly, because the attacker + * IS the issuer. Three reviewers demonstrated a full session as the victim independently. + * + * The confinement lives in the callback; this pins the data it depends on, so a provider loaded + * from the database always carries the domains its assertions are checked against. + */ + withOrgDb([{ id: '1', org: 'org-evil', slug: 'orgevil', name: 'Evil', domains: 'evil.test' }], (m) => { + const p = m.getOrgProvider('orgevil'); + assert.equal(p.emailDomains, 'evil.test', 'the callback cannot confine what it cannot see'); + assert.equal(p.organizationId, 'org-evil', 'and must know this is a tenant provider, not the operator\'s'); + }); +}); + +test('an INSTANCE provider carries no organization, so it is not domain-confined', () => { + // Operator-chosen providers keep the trust they have always had; confinement targets tenants. + const [g] = providers.list({ GOOGLE_CLIENT_ID: 'g' }); + assert.equal(g.organizationId, undefined); + assert.equal(g.source, 'env'); +}); + +test('domain routing is deterministic, not table-scan order', () => { + // forEmail used an unordered SELECT, so deleting and re-adding a provider silently flipped which + // IdP an entire domain routed to. Ordering makes the answer stable. + withOrgDb([ + { id: 'b', org: 'org-a', slug: 'orgbbb', name: 'Second', domains: 'shared.test' }, + { id: 'a', org: 'org-a', slug: 'orgaaa', name: 'First', domains: 'shared.test' }, + ], (m) => { + const first = m.forEmail('x@shared.test').name; + assert.equal(m.forEmail('x@shared.test').name, first, 'same answer every time'); + }); +}); + +test('a secret that cannot be decrypted fails CLOSED', () => { + // decrypt() returns null after a JWT_SECRET rotation, which silently downgraded a confidential + // client to a public one — the login then failed at the provider with an error nobody could act + // on, while the admin screen still said "a secret is set". + withOrgDb([{ id: '1', org: 'o', slug: 'orgsec', name: 'X', domains: 'x.test' }], (m) => { + const real = require('../db/database'); + real.db.prepare('UPDATE org_sso_providers SET client_secret_enc = ? WHERE id = ?').run('not-decryptable', '1'); + assert.throws(() => m.getOrgProvider('orgsec'), /could not be decrypted/); + }); +}); + +test('a tenant cannot claim a public email provider as its sign-in domain', () => { + /* + * Demonstrated in review: a tenant claimed gmail.com, after which /sso/discover answered + * {"sso":true} for every Gmail address and the login page offered "sign in with your + * organization" — a phishing hop launched from the vendor's own login screen, pointed at + * infrastructure the tenant controls. First-claim-wins also meant one cheap account could deny a + * public domain to everyone else. + */ + const { isPublicEmailDomain } = require('../lib/public-email-domains'); + for (const d of ['gmail.com', 'outlook.com', 'hotmail.co.uk', 'yahoo.com', 'icloud.com', + 'proton.me', 'qq.com', 'mail.ru', 'comcast.net', 'gmx.de']) { + assert.ok(isPublicEmailDomain(d), `${d} must be refused as an org sign-in domain`); + } + // ...and a real company domain is still fine, or the feature would be pointless. + for (const d of ['acme.com', 'bigcorp.io', 'my-company.co.uk', 'mail.acme.com']) { + assert.equal(isPublicEmailDomain(d), false, `${d} must remain claimable`); + } +}); + +test('the blocklist is case- and whitespace-insensitive', () => { + // Domains arrive from a form. ` GMAIL.COM ` must not slip through a lowercase-only comparison. + const { isPublicEmailDomain } = require('../lib/public-email-domains'); + assert.ok(isPublicEmailDomain(' GMAIL.COM ')); + assert.ok(isPublicEmailDomain('Outlook.Com')); +});