Skip to content

fix: apply SASLprep (RFC 4013) to passwords before SCRAM-SHA-256 PBKDF2#3669

Merged
brianc merged 9 commits into
brianc:masterfrom
avallete:fix/scram-saslprep
May 13, 2026
Merged

fix: apply SASLprep (RFC 4013) to passwords before SCRAM-SHA-256 PBKDF2#3669
brianc merged 9 commits into
brianc:masterfrom
avallete:fix/scram-saslprep

Conversation

@avallete
Copy link
Copy Markdown
Contributor

@avallete avallete commented May 11, 2026

Summary

pg's SCRAM-SHA-256 client passes the raw password into PBKDF2 with no normalization, while PostgreSQL's server (and libpq) apply SASLprep (B.1 mapping → C.1.2 mapping → NFKC → prohibition + bidi check) when computing the stored verifier. Passwords whose NFKC form differs from themselves authenticate against psql/libpq but fail against pg with 28P01.

This bug bites real users: a password like abcd123456789123456789¨‑¼### authenticates fine in psql but fails in any consumer using pg.

Fix

Inline a minimal SASLprep helper in packages/pg/lib/crypto/sasl.js and call it in continueSession immediately before crypto.deriveKey. The helper performs only the three RFC 4013 steps that change the byte content fed to PBKDF2:

  1. RFC 3454 Table C.1.2 (non-ASCII space) → U+0020.
  2. RFC 3454 Table B.1 (commonly mapped to nothing) → empty.
  3. NFKC normalization.

Prohibition (RFC 4013 §2.3) and bidi (RFC 3454 §6) checks are intentionally omitted: libpq's pg_saslprep and Postgres's own saslprep.c are forgiving on those paths for legacy roles, so omitting them keeps existing accounts working without adding complexity. Because the three remaining steps cannot throw on a string input, no try/catch fallback is needed.

flowchart LR
  rawPwd["raw password"]
  saslprep["inline saslprep()<br/>C.1.2 → SPACE<br/>B.1 → empty<br/>NFKC"]
  derive["crypto.deriveKey<br/>PBKDF2-SHA256"]
  scram["SCRAM proof"]

  rawPwd --> saslprep
  saslprep --> derive
  derive --> scram
Loading

Why in-tree, no dependency

Updated in response to review feedback from @brianc and @charmander on this PR:

  • Supply-chain surface: `@mongodb-js/saslprep` ships an unpinned transitive (`sparse-bitfield`); a runtime dep just for SASLprep was disproportionate to the problem.
  • Smaller code than the dep: ~10 LOC of regex + `String.prototype.normalize('NFKC')`. The Unicode tables are inlined as character classes; no external code-points file (so it works under `workerd` without bundler tricks).
  • Charmander's 3-replace shape (review comment) is the basis for the implementation. One small correction applied: U+200B (ZERO WIDTH SPACE) is in RFC 3454 Table B.1 (mapped to nothing), not C.1.2 (non-ASCII space), so it lives only in the second regex matching PostgreSQL's `saslprep.c` and `libpq`. (Cross-checked against the dep, which maps U+200B to SPACE and is therefore the divergent one here.)
  • Prior art: no `brianc/node-postgres` issue/PR mentions `saslprep`, `stringprep`, `NFKC`, RFC 4013, or RFC 5802. `postgres.js` (porsager/postgres) doesn't normalize either and has the same latent bug.

Why no behavior change for existing users

  • ASCII passwords: all three steps are the identity. The existing CI scram test (`'test4scram'`) is byte-for-byte unchanged.
  • ASCII control characters (e.g. BEL, U+0007): unchanged by C.1.2, B.1, and NFKC alone — same bytes go to PBKDF2 as today (the renamed unit test pins this with the same SCRAM signature snapshot the previous fallback path produced).
  • Cloudflare Workers: validated via the existing worker test harness — `SASLprep is engaged on the SCRAM path under workerd` still passes without any external dep.

What changed

File Why
`packages/pg/lib/crypto/sasl.js` Inline `saslprep(password)` (3 char-class regex passes + NFKC), called before `deriveKey`. No try/catch fallback (no throw path now).
`packages/pg/package.json` No new dependency.
`yarn.lock` Net negative: drops `@mongodb-js/saslprep` and `sparse-bitfield` from the second commit.
`packages/pg/test/unit/client/sasl-scram-tests.js` 4 unit tests: B.1 mapping equivalence (`I\u00ADX` ≡ `IX`), NFKC asymmetry (`\u2168` ≡ `IX`), ASCII-control passthrough (BEL, snapshot), production-bug snapshot (`abcd123456789123456789¨‑¼###`).
`packages/pg/test/integration/client/sasl-scram-tests.js` Gated unicode-password integration block (raw / NFKC-equivalent / wrong).
`packages/pg/test/cloudflare/vitest-cf.test.ts` Worker regression guard exercising `sasl.continueSession` end-to-end under `workerd`.
`.github/workflows/ci.yml` Provision `scram_unicode_test` role with `U&'IX-\2168'` + export env vars.
`CHANGELOG.md` `pg@8.21.0` entry, updated to describe the in-tree implementation and the deliberate simplification trade-off.

Commits

  1. `fix: apply SASLprep (RFC 4013) to passwords before SCRAM-SHA-256 PBKDF2` — original `@mongodb-js/saslprep` wiring and tests.
  2. `fix: inline SASLprep, drop @mongodb-js/saslprep dependency` — review-feedback follow-up. Net diff vs commit 1: −1 dep, −1 transitive, −1 try/catch, +10 LOC inline helper, 1 test renamed (snapshot bytes unchanged).

Happy to squash to one commit on merge if you prefer; I kept them separate so the diff in response to your feedback is easy to read.

Test plan

  • `make test-unit` — full pg unit suite (exit 0; all 4 SASLprep tests pass against the inline impl).
  • `make test-worker` — Cloudflare Workers harness; `SASLprep is engaged on the SCRAM path under workerd ✓` passes without `@mongodb-js/saslprep` (the pre-existing `default` test fails for an unrelated reason — no local Postgres listening on the worker network).
  • `yarn lint` — clean.
  • `yarn build` — clean.
  • Smoke test: every snapshot input fed to the inline impl produces a SCRAM signature byte-identical to the dep-based commit, including `'\u0007abc'` (BEL passthrough) and `'abcd123456789123456789¨‑¼###'` (production-bug case).
  • Full integration suite against a Postgres with both `scram_test` and `scram_unicode_test` roles (runs automatically in CI on this branch across Node 16/18/20/22/24/25).

`pg`'s SCRAM-SHA-256 client passes the raw password into PBKDF2 with no
normalization, while PostgreSQL's server (and libpq) apply SASLprep
(B.1 mapping -> NFKC -> prohibition + bidi check) when computing the
stored verifier. Passwords whose NFKC form differs from themselves
(e.g. containing U+00A8 dieresis, U+2011 non-breaking hyphen, U+00BC
vulgar one quarter, NBSP, soft hyphen) authenticate with psql/libpq
but fail against pg with `28P01`.

Wire `@mongodb-js/saslprep` (the maintained fork used by mongodb's
official Node driver) into `continueSession` before `crypto.deriveKey`,
with a try/catch fallback to the raw password on prohibited / bidi
violations to match `libpq`'s `pg_saslprep` behavior.

Also adds:

- Unit tests covering the soft-hyphen B.1 mapping equivalence, the
  Roman-numeral-IX NFKC asymmetry, the prohibited-char fallback, and a
  deterministic snapshot for the original bug-report password.
- A gated integration test block (SCRAM_TEST_PGUSER_UNICODE /
  SCRAM_TEST_PGPASSWORD_UNICODE) covering raw + NFKC-equivalent + wrong
  password.
- A `scram_unicode_test` role (password `U&'IX-\2168'`) provisioned in
  CI plus matching env vars so the new integration tests run on every
  Node version.
- A Cloudflare Workers regression guard that exercises
  `sasl.continueSession` to ensure `@mongodb-js/saslprep` resolves
  cleanly under workerd.
- A `pg@8.21.0` CHANGELOG entry.
@brianc
Copy link
Copy Markdown
Owner

brianc commented May 11, 2026

If maintainers prefer not to add a runtime dependency, the same wiring works against an in-tree implementation of RFC 4013 (~150–250 LOC using String.prototype.normalize('NFKC') + Unicode property tables).

I'm not sure what you mean exactly by this, but if you're suggesting there's a way to avoid taking a 3rd party dependency into pg I strongly prefer this. Even absolutely pinning the saslprep library, it also has a 3rd party dependency without an absolute version pinned, so there's supply chain vulnerability there I'd rather not deal with.

@charmander
Copy link
Copy Markdown
Collaborator

charmander commented May 11, 2026

Yes, and I think dropping support for detecting/preserving perfect compatibility with prohibited passwords to simplify the implementation is a good trade:

const saslprep = (s: string): string => s
  .replace(/[\u00A0\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200A\u200B\u202F\u205F\u3000]/g, ' ')
  .replace(/[\u00AD\u034F\u1806\u180B\u180C\u180D\u200B\u200C\u200D\u2060\uFE00\uFE01\uFE02\uFE03\uFE04\uFE05\uFE06\uFE07\uFE08\uFE09\uFE0A\uFE0B\uFE0C\uFE0D\uFE0E\uFE0F\uFEFF]/g, '')
  .normalize('NFKC')

@brianc
Copy link
Copy Markdown
Owner

brianc commented May 11, 2026

also...if someone wanted could they install the saslprep library in their own code & run it? seems edge-case and not likely something that'd cause runtime errors after deployment or something. Happy to be proven wrong tho

Per review feedback on brianc#3669: ship the SASLprep step as a small in-tree
function instead of pulling a runtime dep with an unpinned transitive.
The function performs only the three byte-changing steps from RFC 4013
(Table C.1.2 -> SPACE, Table B.1 -> empty, NFKC) and skips the
prohibition (RFC 4013 section 2.3) and bidi (RFC 3454 section 6) checks,
since libpq is forgiving on those paths and Postgres's own SASLprep is
similarly lenient. Removes the try/catch fallback (no code path throws).

The deterministic snapshot tests stay byte-for-byte valid because none
of them touch U+200B, the only edge case where the inline impl diverges
from `@mongodb-js/saslprep`. RFC 3454 places U+200B in Table B.1
(mapped to nothing); the dep maps it to SPACE. PostgreSQL's
saslprep.c follows the RFC, so the inline impl matches libpq more
closely on that codepoint. The B.1 unit-test rename ("passes ASCII
control characters through normalization unchanged") keeps the same
snapshot bytes since BEL is unchanged by all three steps.

Co-authored-by: charmander <charmander@noreply.github.com>
now that the associated dependency isn’t being added.
Comment thread packages/pg/lib/crypto/sasl.js Outdated
Comment thread packages/pg/test/cloudflare/vitest-cf.test.ts Outdated
charmander and others added 3 commits May 12, 2026 08:15
normally added as part of the release process
Updated comments in sasl.js to clarify the password normalization process and removed redundant test cases from vitest-cf.test.ts, streamlining the codebase.
@avallete avallete requested a review from charmander May 12, 2026 15:49
Confirmed in pull request comments that the “macOS/iOS” thing was an AI inventing an unneeded justification, and NFKC is already covered by another test.
As mentioned in the test comment, RFC 3454 defines appendix B for mapping tables and appendix C for prohibition tables. RFC 4013 SASLprep is probably misusing that list of non-ASCII spaces, and says nothing about the overlap. (At least it’s obsoleted.)
@charmander charmander requested a review from brianc May 12, 2026 17:57
@brianc
Copy link
Copy Markdown
Owner

brianc commented May 13, 2026

really nice work trimming this down to small, clean thing that workw! 🙌

@brianc brianc merged commit 0ac3edd into brianc:master May 13, 2026
7 checks passed
avallete added a commit to supabase/node-postgres that referenced this pull request May 13, 2026
* test: Ensure failure to throw at all doesn’t pass (brianc#3671)

* Assorted test fixes and cleanup (brianc#3672)

* cleanup: Remove duplicate test

* cleanup: Remove nonsense test

* cleanup: Simplify promise rejection test

* test: Fix and tighten assertion that would always pass

because of the `SELECTR` typo.

* cleanup: Add missing `await`s when using `assert.rejects` in tests; remove unneeded function wrappers

* ci: Node 26 followup (brianc#3670)

* Revert unneeded pg-native→libpq dependency range adjustment

This reverts part of commit 1025d12.

* dev: Upgrade libpq/nan in lockfile for Node 26 compatibility

* fix: apply SASLprep (RFC 4013) to passwords before SCRAM-SHA-256 PBKDF2 (brianc#3669)

* fix: apply SASLprep (RFC 4013) to passwords before SCRAM-SHA-256 PBKDF2

`pg`'s SCRAM-SHA-256 client passes the raw password into PBKDF2 with no
normalization, while PostgreSQL's server (and libpq) apply SASLprep
(B.1 mapping -> NFKC -> prohibition + bidi check) when computing the
stored verifier. Passwords whose NFKC form differs from themselves
(e.g. containing U+00A8 dieresis, U+2011 non-breaking hyphen, U+00BC
vulgar one quarter, NBSP, soft hyphen) authenticate with psql/libpq
but fail against pg with `28P01`.

Wire `@mongodb-js/saslprep` (the maintained fork used by mongodb's
official Node driver) into `continueSession` before `crypto.deriveKey`,
with a try/catch fallback to the raw password on prohibited / bidi
violations to match `libpq`'s `pg_saslprep` behavior.

Also adds:

- Unit tests covering the soft-hyphen B.1 mapping equivalence, the
  Roman-numeral-IX NFKC asymmetry, the prohibited-char fallback, and a
  deterministic snapshot for the original bug-report password.
- A gated integration test block (SCRAM_TEST_PGUSER_UNICODE /
  SCRAM_TEST_PGPASSWORD_UNICODE) covering raw + NFKC-equivalent + wrong
  password.
- A `scram_unicode_test` role (password `U&'IX-\2168'`) provisioned in
  CI plus matching env vars so the new integration tests run on every
  Node version.
- A Cloudflare Workers regression guard that exercises
  `sasl.continueSession` to ensure `@mongodb-js/saslprep` resolves
  cleanly under workerd.
- A `pg@8.21.0` CHANGELOG entry.

* fix: inline SASLprep, drop @mongodb-js/saslprep dependency

Per review feedback on brianc#3669: ship the SASLprep step as a small in-tree
function instead of pulling a runtime dep with an unpinned transitive.
The function performs only the three byte-changing steps from RFC 4013
(Table C.1.2 -> SPACE, Table B.1 -> empty, NFKC) and skips the
prohibition (RFC 4013 section 2.3) and bidi (RFC 3454 section 6) checks,
since libpq is forgiving on those paths and Postgres's own SASLprep is
similarly lenient. Removes the try/catch fallback (no code path throws).

The deterministic snapshot tests stay byte-for-byte valid because none
of them touch U+200B, the only edge case where the inline impl diverges
from `@mongodb-js/saslprep`. RFC 3454 places U+200B in Table B.1
(mapped to nothing); the dep maps it to SPACE. PostgreSQL's
saslprep.c follows the RFC, so the inline impl matches libpq more
closely on that codepoint. The B.1 unit-test rename ("passes ASCII
control characters through normalization unchanged") keeps the same
snapshot bytes since BEL is unchanged by all three steps.

Co-authored-by: charmander <charmander@noreply.github.com>

* Revert unrelated no-op changes to yarn.lock

now that the associated dependency isn’t being added.

* cleanup: Allow Prettier to format some lines

* cleanup: Remove changelog entry for unreleased pg version

normally added as part of the release process

* refactor: Simplify comments in sasl.js and remove unused test cases

Updated comments in sasl.js to clarify the password normalization process and removed redundant test cases from vitest-cf.test.ts, streamlining the codebase.

* Remove redundant NFKC-only SASLprep test

Confirmed in pull request comments that the “macOS/iOS” thing was an AI inventing an unneeded justification, and NFKC is already covered by another test.

* fix: SASLprep zero-width space the same way PostgreSQL does

As mentioned in the test comment, RFC 3454 defines appendix B for mapping tables and appendix C for prohibition tables. RFC 4013 SASLprep is probably misusing that list of non-ASCII spaces, and says nothing about the overlap. (At least it’s obsoleted.)

* cleanup: Simplify regex character classes with ranges

---------

Co-authored-by: charmander <charmander@noreply.github.com>
Co-authored-by: Charmander <~@charmander.me>

---------

Co-authored-by: Charmander <~@charmander.me>
Co-authored-by: charmander <charmander@noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants