Skip to content

Make verifyTotp total so a malformed 2FA POST cannot kill the gateway - #345

Merged
kh0pper merged 1 commit into
mainfrom
fix/verify-totp-missing-code
Sep 10, 2026
Merged

Make verifyTotp total so a malformed 2FA POST cannot kill the gateway#345
kh0pper merged 1 commit into
mainfrom
fix/verify-totp-missing-code

Conversation

@kh0pper

@kh0pper kh0pper commented Sep 10, 2026

Copy link
Copy Markdown
Owner

The defect

A POST to /dashboard/login/2fa with the totp_code field simply absent reaches verifyTotp(undefined, secret). otpauth reads .length off the token, so it raises TypeError: Cannot read properties of undefined (reading 'length') inside an async route handler. That is an unhandled rejection, which is fatal, so one malformed request takes the whole gateway process down.

Same class as the t()-not-imported defect in #343: a bad request must render an error, never end the process. This one was found the same evening, on the same instance, and offered rather than fixed then.

Reproduced directly against the pinned otpauth:

undefined -> THROWS TypeError: Cannot read properties of undefined (reading 'length')
null      -> THROWS TypeError: Cannot read properties of null (reading 'length')
123456    -> null      (already answered false)
""        -> null
"abc"     -> null
{}        -> null

The secret argument is attacker-shaped too

Three call sites, and the code is not the only field that comes out of a request body:

  • dashboard/index.js:240/dashboard/login/2fa, the code
  • dashboard/index.js:350/dashboard/login/2fa/setup, the code
  • dashboard/settings/sections/two-factor.js:124enable_2fa, code and secret

Secret.fromBase32(undefined) throws Cannot read properties of undefined (reading 'replace'), and a string that is not base32 throws on parse.

The fix

verifyTotp is now total on both arguments. A non-string or blank token, a non-string or blank secret, and a secret that will not parse as base32 each answer false. Guarding in the one function covers all three call sites.

Nothing else changes. A number, an object or a junk string already answered false through otpauth's own null return, so the observable behaviour for every previously-working input is identical. A real code for the current period still verifies.

Tests

Four cases appended to tests/dashboard-2fa-login.test.js, the file #343 added:

  • a missing code (undefined, null) answers false instead of throwing
  • a code that is not a usable string answers false
  • a missing or unparseable secret answers false
  • the real code for the current period still verifies (RFC 6238 SHA1 seed)

The first and third fail on main with a TypeError. All ten in the file pass with the fix, as do csrf-middleware (14) and i18n-global-parity (7).

A POST to /dashboard/login/2fa with the `totp_code` field simply absent
reached verifyTotp(undefined, secret). otpauth reads `.length` off the
token, so the call raised `TypeError: Cannot read properties of
undefined` inside an async route handler — an unhandled rejection, which
is fatal, so one malformed request took the whole gateway process down.
Same class as the t()-not-imported defect in #343: a bad request must
render an error, never end the process.

The secret argument is attacker-shaped too. /dashboard/login/2fa/setup
and the settings enable_2fa action both pass a `secret` straight from the
request body, and Secret.fromBase32(undefined) throws the same way, as
does a string that is not base32 at all.

So verifyTotp is now total on both arguments: a non-string or blank
token, a non-string or blank secret, and a secret that will not parse as
base32 each answer false. Nothing else changes — a number, an object or a
junk string already answered false via otpauth's own null return, and a
real code for the current period still verifies.

Found on the R4 instance while building a client for the perch-interactive
routes; one malformed POST restarted its gateway during #343's own
verification.
@kh0pper
kh0pper merged commit 163f44e into main Sep 10, 2026
3 checks passed
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.

1 participant