chore: harden saml implementation

api/package.json

@@ -53,6 +53,7 @@
     "prom-client": "^15.1.3",
     "qrcode": "^1.5.4",
     "rate-limiter-flexible": "^5.0.5",
+    "@authenio/samlify-node-xmllint": "^2.0.0",
     "samlify": "^2.11.0",
     "seedrandom": "^3.0.5",
     "serialize-javascript": "^7.0.4",

api/src/saml2/service.ts

@@ -7,16 +7,22 @@ import type { SAML2 } from '../../config/type/index.ts'
 import config from '#config'
 import _slug from 'slugify'
 import samlify from 'samlify'
+// @ts-ignore -- no ambient types; see node_modules/@authenio/samlify-node-xmllint/build/index.d.ts
+import * as xsdValidator from '@authenio/samlify-node-xmllint'
 import Debug from 'debug'
 import mongo from '#mongo'
 import { decipher, cipher } from '../utils/cipher.ts'
-import { exec } from 'node:child_process'
+import { execFile } from 'node:child_process'
 import { promisify } from 'node:util'
+import { generateKeyPairSync } from 'node:crypto'
+import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
 import { getSiteBaseUrl, reqSite } from '#services'
 import { type Site } from '#types'
 import eventsLog, { type EventLogContext } from '@data-fair/lib-express/events-log.js'
 
-const execAsync = promisify(exec)
+const execFileAsync = promisify(execFile)
 const debug = Debug('saml')
 const slug = _slug.default
 
@@ -35,14 +41,11 @@ export type Saml2RelayState = {
   adminMode?: boolean
 }
 
-// const validator = require('@authenio/samlify-xsd-schema-validator')
-// samlify.setSchemaValidator(validator)
-// TODO: apply an actual validator cf https://github.com/tngan/samlify#installation
-samlify.setSchemaValidator({
-  validate: (response) => {
-    return Promise.resolve('skipped')
-  }
-})
+// Schema validation is REQUIRED — samlify relies on it to mitigate XML signature-wrapping
+// attacks (e.g. CVE-2025-47949, CVE-2024-45231 / -45239). Do not replace with a pass-through
+// shim under any circumstances: a `validate: () => Promise.resolve('skipped')` reintroduces
+// every known samlify assertion-injection CVE. See security review C-2.
+samlify.setSchemaValidator(xsdValidator)
 
 export const getSamlProviderById = async (req: Request, id: string): Promise<PreparedSaml2Provider | undefined> => {
   const site = await reqSite(req)
@@ -168,6 +171,19 @@ export const init = async () => {
   }
 }
 
+// For test-env DELETE / cleanup: drop the cached SAML cert from mongo, reset the in-memory
+// SP singleton, and re-run init so the next request mints a fresh cert via createCert.
+// Without this, dev/CI cycles reuse whatever cert was minted at first server startup and
+// the createCert path stays untested in the live env even after code changes. Site-level
+// certs share the `saml-certificates*` _id prefix so they're cleaned up too.
+export const resetForTests = async () => {
+  await mongo.saml2RelayStates.deleteMany({})
+  await mongo.secrets.deleteMany({ _id: { $regex: /^saml-certificates/ } })
+  _sp = undefined
+  _globalProviders.length = 0
+  await init()
+}
+
 export const initServiceProvider = async (site?: Site) => {
   const certificates = await initCertificates(site)
   const url = site ? `${getSiteBaseUrl(site)}/simple-directory` : config.publicUrl
@@ -176,27 +192,55 @@ export const initServiceProvider = async (site?: Site) => {
     Location: `${url}/api/auth/saml2-assert`
   }]
   debug('config service provider')
+  // config.saml2.sp is spread FIRST so it can only tune tangential options (NameIDFormat,
+  // signing algorithm hints, etc.) — it cannot override our cert material, entityID,
+  // assertionConsumerService, or flip wantAssertionsSigned off. Order matters (see C-2).
   return samlify.ServiceProvider({
+    ...config.saml2.sp,
     entityID: `${url}/api/auth/saml2-metadata.xml`,
     assertionConsumerService,
     signingCert: certificates.signing.cert,
     privateKey: certificates.signing.privateKey,
     encryptCert: certificates.encrypt.cert,
     encPrivateKey: certificates.encrypt.privateKey,
+    wantAssertionsSigned: true,
     // @ts-ignore if we use a boolean the attribute is set as empty in the xml output, and some IDP don't like that
-    allowCreate: 'false',
-    ...config.saml2.sp
+    allowCreate: 'false'
   })
 }
 
-const createCert = async () => {
-  const subject = `/C=FR/CN=${new URL(config.publicUrl).hostname}`
-  const privateKey = (await execAsync('openssl genpkey -algorithm RSA -pkeyopt rsa_keygen_bits:2048')).stdout
-  const certPromise = execAsync(`echo "${privateKey}" | openssl req -key /dev/stdin -x509 -sha256 -nodes -days 1095 -subj "${subject}"`)
-  certPromise.child.stdin?.write(privateKey)
-  certPromise.child.stdin?.end()
-  const cert = (await certPromise).stdout
-  return { privateKey, cert }
+// Exported for the C-2 regression test in tests/features/saml2-create-cert.unit.spec.ts —
+// the live dev env almost always reads a cached cert from the `secrets` collection, so
+// without a direct call this path would be entirely untested.
+export const createCert = async () => {
+  const hostname = new URL(config.publicUrl).hostname
+  // Reject hostnames that would be parsed as shell metacharacters by openssl's -subj
+  // arg or that contain CR/LF (defense-in-depth; execFile already prevents shell parsing).
+  if (!/^[A-Za-z0-9.\-_:[\]]+$/.test(hostname)) {
+    throw new Error(`refusing to mint SAML certificate for suspicious hostname: ${hostname}`)
+  }
+  const subject = `/C=FR/CN=${hostname}`
+  // Generate the RSA key in-process (no shell, no string interpolation, no stdin race).
+  const { privateKey } = generateKeyPairSync('rsa', {
+    modulusLength: 2048,
+    privateKeyEncoding: { type: 'pkcs8', format: 'pem' },
+    publicKeyEncoding: { type: 'spki', format: 'pem' }
+  })
+  // OpenSSL 3.x's STORE subsystem refuses to read `-key /dev/stdin` from a Node-spawned
+  // pipe (fopen() on a non-seekable fd fails). Stage the key in a 0700 temp directory
+  // so it's owner-only readable for the brief openssl invocation, then unlink in finally.
+  const tmpDir = mkdtempSync(join(tmpdir(), 'sd-saml-'))
+  const keyPath = join(tmpDir, 'key.pem')
+  try {
+    writeFileSync(keyPath, privateKey, { mode: 0o600 })
+    // execFile with argv array — `subject` and `keyPath` never reach a shell.
+    const { stdout: cert } = await execFileAsync('openssl', [
+      'req', '-key', keyPath, '-x509', '-sha256', '-nodes', '-days', '1095', '-subj', subject
+    ])
+    return { privateKey, cert }
+  } finally {
+    rmSync(tmpDir, { recursive: true, force: true })
+  }
 }
 
 // attributes for microsoft entra https://learn.microsoft.com/en-us/entra/identity-platform/reference-saml-tokens

api/src/test-env.ts

@@ -35,6 +35,10 @@ router.delete('/', async (req, res) => {
   await mongo.db.collection('sd-rate-limiter-auth').deleteMany()
   const { getSiteByHost } = await import('./sites/service.ts')
   getSiteByHost.clear()
+  // Force a fresh SAML cert mint on the next request — exercises createCert end-to-end
+  // every cleanup cycle instead of letting a one-time bootstrap cert linger.
+  const { resetForTests: resetSaml } = await import('./saml2/service.ts')
+  await resetSaml()
   res.status(204).send()
 })
 

docs/architecture/email-trust-and-site-isolation.md

@@ -61,6 +61,13 @@ looser IdP.
 - **LinkedIn** — primary email is considered verified upstream.
 - **SAML 2** — no standard verified flag; adding a SAML IdP is an explicit
   trust statement. Site-level IdPs remain confined by user scoping.
+  `api/src/saml2/service.ts` installs `@authenio/samlify-node-xmllint` as the
+  samlify schema validator, which is required to mitigate XML
+  signature-wrapping attacks. The SP also advertises
+  `wantAssertionsSigned: true` in its metadata, and the
+  `config.saml2.sp` operator-config spread cannot override SP cert material,
+  `entityID`, `assertionConsumerService`, or the wantAssertionsSigned flag
+  (the spread is applied first, the trusted fields last).
 
 ### Why OIDC is looser than the OAuth providers
 

tests/features/saml2-create-cert.unit.spec.ts

@@ -0,0 +1,71 @@
+// Regression coverage for the C-2 follow-up: api/src/saml2/service.ts#createCert
+// was rewritten from
+//   exec(`echo "${privateKey}" | openssl req -key /dev/stdin ... -subj "${subject}"`)
+// — a shell pipeline with string interpolation and a stdin race — to
+// crypto.generateKeyPairSync + execFile('openssl', [...]) (no shell, argv array).
+//
+// In production the path runs once per (deployment, site) and the result is cached
+// in mongo's `secrets` collection forever, so the live dev env will normally NOT
+// exercise it after the first restart. This test calls createCert directly so any
+// regression (wrong PEM shape, broken openssl invocation, cert without the right
+// CN) fails CI even when the dev env is happily reading cached certs.
+
+import { strict as assert } from 'node:assert'
+import { test } from '@playwright/test'
+import { createPrivateKey, X509Certificate } from 'node:crypto'
+
+process.env.NODE_CONFIG_DIR = process.env.NODE_CONFIG_DIR || './api/config/'
+process.env.NODE_ENV = process.env.NODE_ENV || 'test'
+process.env.SUPPRESS_NO_CONFIG_WARNING = '1'
+
+test.describe('SAML2 createCert (C-2 follow-up)', () => {
+  let createCert: () => Promise<{ privateKey: string, cert: string }>
+  let expectedHostname: string
+
+  test.beforeAll(async () => {
+    const samlService = await import('../../api/src/saml2/service.ts')
+    createCert = samlService.createCert
+    const config = (await import('../../api/src/config.ts')).default
+    expectedHostname = new URL(config.publicUrl).hostname
+  })
+
+  test('produces a PKCS#8 RSA private key parseable by node:crypto', async () => {
+    const { privateKey } = await createCert()
+    assert.match(privateKey, /^-----BEGIN PRIVATE KEY-----/)
+    assert.match(privateKey, /-----END PRIVATE KEY-----\s*$/)
+    const key = createPrivateKey(privateKey)
+    assert.equal(key.asymmetricKeyType, 'rsa')
+    // 2048-bit modulus per the explicit modulusLength in createCert
+    assert.equal(key.asymmetricKeyDetails?.modulusLength, 2048)
+  })
+
+  test('produces a self-signed X.509 cert with the configured publicUrl hostname as CN', async () => {
+    const { cert } = await createCert()
+    assert.match(cert, /^-----BEGIN CERTIFICATE-----/)
+    assert.match(cert, /-----END CERTIFICATE-----\s*$/)
+    const x509 = new X509Certificate(cert)
+    // openssl emits the subject as 'C=FR, CN=<hostname>' (or with newlines depending on version)
+    assert.ok(
+      x509.subject.includes(`CN=${expectedHostname}`),
+      `cert subject ${JSON.stringify(x509.subject)} does not contain CN=${expectedHostname}`
+    )
+    // self-signed: issuer matches subject
+    assert.equal(x509.issuer, x509.subject)
+    // validity window: 3 years (1095 days) per the -days arg in createCert
+    const validFrom = new Date(x509.validFrom).getTime()
+    const validTo = new Date(x509.validTo).getTime()
+    const days = (validTo - validFrom) / (1000 * 60 * 60 * 24)
+    assert.ok(Math.abs(days - 1095) < 1, `expected ~1095-day validity window, got ${days}`)
+  })
+
+  test('cert and key form a matching keypair (cert public key verifies against private key signature shape)', async () => {
+    const { privateKey, cert } = await createCert()
+    const x509 = new X509Certificate(cert)
+    const keyObj = createPrivateKey(privateKey)
+    // Both should expose the same public modulus length
+    assert.equal(keyObj.asymmetricKeyDetails?.modulusLength, 2048)
+    assert.equal(x509.publicKey.asymmetricKeyDetails?.modulusLength, 2048)
+    // X509Certificate.checkPrivateKey is the canonical "do these belong together" check
+    assert.ok(x509.checkPrivateKey(keyObj), 'cert public key does not match the generated private key')
+  })
+})

tests/features/saml2-schema-validator.unit.spec.ts

@@ -0,0 +1,82 @@
+// Regression guard for C-2 (security-review-2026-04.md):
+// before PR #120 (chore: harden SAML), api/src/saml2/service.ts installed
+//   samlify.setSchemaValidator({ validate: () => Promise.resolve('skipped') })
+// which re-enabled every samlify XML-signature-wrapping CVE because the validator
+// itself is the defense layer. If anyone reverts to that no-op shim, the tests
+// below must fail loudly.
+
+import { strict as assert } from 'node:assert'
+import { test } from '@playwright/test'
+
+// Minimal env bootstrap so `import '#config'` loads the test config without requiring
+// the full in-process server setup.
+process.env.NODE_CONFIG_DIR = process.env.NODE_CONFIG_DIR || './api/config/'
+process.env.NODE_ENV = process.env.NODE_ENV || 'test'
+process.env.SUPPRESS_NO_CONFIG_WARNING = '1'
+
+test.describe('SAML2 schema validator (C-2 regression)', () => {
+  let samlifyCtx: { validate?: (xml: string) => Promise<unknown> }
+
+  test.beforeAll(async () => {
+    // Importing the module triggers `samlify.setSchemaValidator(xsdValidator)`.
+    await import('../../api/src/saml2/service.ts')
+    // samlify's top-level index does not re-export getContext; import it from the
+    // CJS submodule where it lives. Brittle-ish, but the path has been stable
+    // across samlify 2.x.
+    // @ts-ignore -- no declaration file for this submodule
+    const { getContext } = await import('samlify/build/src/api.js') as any
+    samlifyCtx = getContext()
+  })
+
+  test('a validator is wired in', async () => {
+    assert.equal(typeof samlifyCtx.validate, 'function')
+  })
+
+  test('validator is not the "skipped" no-op shim', async () => {
+    // The historical shim resolved every input (including `undefined`) with
+    // the literal string 'skipped'. A real validator either rejects on
+    // invalid SAML XML, or throws synchronously, or resolves with a
+    // different success marker — anything except 'skipped'.
+    let result: unknown
+    try {
+      result = await samlifyCtx.validate!('<not-saml-at-all/>')
+    } catch (err) {
+      result = err
+    }
+    assert.notEqual(result, 'skipped',
+      'C-2: SAML schema validator has been reverted to the no-op shim — see docs/local/security-review-2026-04.md')
+  })
+
+  test('validator rejects garbage (non-XML) input', async () => {
+    await assert.rejects(() => samlifyCtx.validate!('this is not xml at all'))
+  })
+
+  test('validator rejects well-formed XML that is not a SAML protocol envelope', async () => {
+    // Well-formed XML, no SAML namespace, no <saml2p:Response> root — must fail
+    // schema validation. Before the fix, the shim silently accepted this.
+    await assert.rejects(() => samlifyCtx.validate!(
+      '<?xml version="1.0" encoding="UTF-8"?><root xmlns="urn:example"><foo>bar</foo></root>'
+    ))
+  })
+
+  test('validator rejects a Response whose Assertion carries unexpected child elements (signature-wrapping shape)', async () => {
+    // A classic XML-signature-wrapping shape: a <saml2p:Response> root whose
+    // <saml2:Assertion> contains a sibling element that is NOT in the SAML
+    // assertion schema. samlify's lib would extract attributes from the
+    // attacker-supplied element while verifying the signature of the
+    // legitimate one. The schema validator is the defense that stops the
+    // malformed document at the gate. This XML is deliberately crafted to be
+    // well-formed but schema-invalid.
+    const wrapped = `<?xml version="1.0" encoding="UTF-8"?>
+<saml2p:Response xmlns:saml2p="urn:oasis:names:tc:SAML:2.0:protocol"
+                 xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion"
+                 ID="_wrap" Version="2.0" IssueInstant="2026-04-21T00:00:00Z">
+  <saml2:Issuer>https://evil.example/idp</saml2:Issuer>
+  <saml2:Assertion ID="_a" Version="2.0" IssueInstant="2026-04-21T00:00:00Z">
+    <saml2:Issuer>https://evil.example/idp</saml2:Issuer>
+    <evil:InjectedPayload xmlns:evil="urn:example:evil">admin@victim.com</evil:InjectedPayload>
+  </saml2:Assertion>
+</saml2p:Response>`
+    await assert.rejects(() => samlifyCtx.validate!(wrapped))
+  })
+})