fix: TypeError when listing non-TOTP user auth factors

[jeffnv]

Description

Fixes DAAP-3034 (Sentry API-2FR): multiFactorAuth.listUserAuthFactors() threw TypeError: Cannot read properties of undefined (reading 'issuer') for any user with a non-TOTP factor, failing the entire list call.

GET /user_management/users/:id/auth_factors returns factors of type totp, sms, or generic_otp, but only TOTP factors include a totp object (SMS factors carry sms: { phone_number } instead). The user-management deserializeFactor called deserializeTotp(factor.totp) unconditionally:

- totp: deserializeTotp(factor.totp),
+ ...(factor.sms ? { sms: deserializeSms(factor.sms) } : {}),
+ ...(factor.totp ? { totp: deserializeTotp(factor.totp) } : {}),

This matches the existing (correct) sibling deserializer in src/multi-factor-auth/serializers/factor.serializer.ts; the divergence between the two was the whole bug.

Type fix (why the compiler never caught this)

The AuthenticationFactor/AuthenticationFactorResponse interfaces hard-coded type: 'totp' and a required totp object — true of the enroll endpoint, false of list. They now use the shared FactorType union ('sms' | 'totp' | 'generic_otp', exported from the MFA module so there's a single source of truth) with optional sms? / totp?.

Judgment calls:

• Semver: widening type and making totp optional on the list path is technically breaking for TypeScript consumers who destructure factor.totp — but the type was lying; that code already crashes at runtime for exactly the users this affects. Shipping as a fix (patch) since it corrects the types to the API's actual, long-standing response contract.
• WithSecrets (enroll path) stays narrow: EnrollAuthFactorOptions.type is 'totp' only, so createUserAuthFactor cannot return a non-TOTP factor; deserializeFactorWithSecrets keeps the unguarded call so a contract-violating response fails loudly rather than silently returning undefined (per review).
• sms field: now surfaced on the returned list object (previously dropped), matching the MFA sibling. Additive.

Test coverage: both list-factors.json fixtures now include sms and generic_otp factors, exercised through the listUserAuthFactors spec and a new authentication-factor.serializer.spec.ts — either would have caught the bug.

Note: the production consumer (WorkOS dashboard) is on SDK v8.10.0 (userManagement.listAuthFactors()); there is no 8.x maintenance branch, so it will pick this up on upgrade to v9+/v10 where the method is multiFactorAuth.listUserAuthFactors().

Documentation

Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.

[ ] No

Link to Devin session: https://app.devin.ai/sessions/7fa3a604bb6d415ab2989a8e59fcf403
Requested by: @jeffnv

[linear-code]

DAAP-3034

[devin-ai-integration]

Original prompt from Devin Bot

Repo: workos/workos-node

Fix a production bug: listing a user's auth factors throws a TypeError for any user who has a non-TOTP authentication factor. Fix the deserializer, and fix the type that let it ship. This is tracked by Linear issue DAAP-3034 (Sentry issue API-2FR): https://linear.app/workos/issue/DAAP-3034/typeerror-cannot-read-properties-of-undefined-reading-issuer

#``# What's happening (fact-checked against main as of 2026-07-24)

GET /user_management/users/:id/auth_factors can return factors of type generic_otp, sms, or totp. Only TOTP factors carry a totp object in the response; for an SMS factor the API omits the totp key entirely and instead includes an sms: { phone_number } object.

The user-management deserializer calls deserializeTotp(factor.totp) unconditionally, and deserializeTotp reads totp.issuer — so the call throws TypeError: Cannot read properties of undefined (reading 'issuer'). It fails the whole list call, not just the offending row. This is live in production against real users today (the WorkOS dashboard hit it via SDK v8.10.0's userManagement.listAuthFactors()).

IMPORTANT naming note: on current main (v9.x), userManagement.listAuthFactors() no longer exists — it was renamed to workos.multiFactorAuth.listUserAuthFactors() (see docs/V9_MIGRATION_GUIDE.md). The buggy user-management serializer is still the one used: src/multi-factor-auth/multi-factor-auth.ts (listUserAuthFactors at ~:193 and createUserAuthFactor at ~:163) imports deserializeFactor/deserializeFactorWithSecrets from src/user-management/serializers/authentication-factor.serializer.ts. Fix on main; there is no visible 8.x maintenance branch, so don't attempt a backport — just note in the PR that the production consumer is on 8.10.0 and will pick this up on upgrade.

#``# Landmarks (verified)

• src/user-management/serializers/authentication-factor.serializer.ts — the unguarded deserializer. Both deserializeFactor and `deserializeFa... (3309 chars truncated...)

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment, CI, and merge conflict monitoring

[greptile-apps]

Greptile Summary

This PR fixes a TypeError: Cannot read properties of undefined (reading 'issuer') that crashed multiFactorAuth.listUserAuthFactors() whenever a user had a non-TOTP factor (SMS or generic_otp). The root cause was deserializeFactor in the user-management serializer calling deserializeTotp(factor.totp) unconditionally, while the API only sets totp on TOTP-typed factors.

• Core fix (authentication-factor.serializer.ts): replaces the unconditional totp: deserializeTotp(factor.totp) with conditional spreading for both sms and totp, matching the already-correct sibling in multi-factor-auth/serializers/factor.serializer.ts.
• Interface fix (authentication-factor.interface.ts): widens type from 'totp' to AuthenticationFactorType and makes sms?/totp? optional on AuthenticationFactor and AuthenticationFactorResponse; the WithSecrets variants are intentionally left unchanged since enrollment currently only supports TOTP.
• Test coverage: a new authentication-factor.serializer.spec.ts exercises deserializeFactor directly against the updated user-management/fixtures/list-factors.json (all three factor types), and the existing multiFactorAuth spec is extended to assert the full deserialized output for all three factor types.

Confidence Score: 5/5

Safe to merge — the change is a targeted, well-tested fix to a confirmed runtime crash with no side effects on adjacent code paths.

The fix is minimal and correct: conditional spreading for sms/totp matches the already-proven pattern in the sibling MFA serializer. The WithSecrets path is unchanged and remains safe. New unit tests directly exercise all three factor types through both the user-management serializer spec and the expanded multiFactorAuth integration test. The type widening accurately reflects the existing API contract rather than introducing new behavior.

Files Needing Attention: No files require special attention.

Important Files Changed

Filename	Overview
src/user-management/serializers/authentication-factor.serializer.ts	Core fix: replaces unconditional deserializeTotp(factor.totp) with conditional spreading for sms and totp; deserializeFactorWithSecrets is correctly left unconditional since its response type still requires totp.
src/user-management/interfaces/authentication-factor.interface.ts	Widens AuthenticationFactor and AuthenticationFactorResponse to allow all three factor types with optional sms?/totp?; WithSecrets variants intentionally unchanged (enroll is TOTP-only).
src/user-management/serializers/authentication-factor.serializer.spec.ts	New spec that directly tests deserializeFactor against the user-management fixture for all three factor types; addresses the previously missing coverage gap.
src/multi-factor-auth/interfaces/factor.interface.ts	Exports FactorType so it can be re-used in the user-management module; purely additive change.
src/multi-factor-auth/multi-factor-auth.spec.ts	Extends the listUserAuthFactors test to assert SMS and generic_otp factors; uses toMatchObject and routes through the user-management serializer (which does produce userId), so expectations are correct.
src/user-management/fixtures/list-factors.json	Adds SMS and generic_otp factor entries alongside TOTP; now imported by the new authentication-factor.serializer.spec.ts.
src/multi-factor-auth/fixtures/list-factors.json	Mirrors the user-management fixture update — adds SMS and generic_otp entries and user_id to the TOTP entry, consumed by the updated listUserAuthFactors spec.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["GET /user_management/users/:id/auth_factors"] --> B["AuthenticationFactorResponse[]"]
    B --> C["deserializeFactor()"]
    C --> D{factor.sms?}
    D -- yes --> E["deserializeSms(factor.sms)"]
    D -- no --> F[skip sms]
    C --> G{factor.totp?}
    G -- yes --> H["deserializeTotp(factor.totp)"]
    G -- no --> I[skip totp]
    E & F & H & I --> J["AuthenticationFactor"]

Loading

_{Reviews (3): Last reviewed commit: "Narrow WithSecrets types to TOTP and reu..." | Re-trigger Greptile}

[devin-ai-integration]

Addressed in 6cf80fd: added src/user-management/serializers/authentication-factor.serializer.spec.ts, which imports src/user-management/fixtures/list-factors.json and exercises deserializeFactor against all three factor types, so the fixture is no longer dead test data.

[jeffnv]

Reviewed the full branch with the PR checked out locally.

Verdict: correct fix, ship it after one type decision

The core change is right, and the test genuinely pins the bug — I verified that rather than trusting it. Reverting just the serializer hunk to the pre-fix form and re-running the spec reproduces the exact production failure:

TypeError: Cannot read properties of undefined (reading 'issuer')
  > 10 |     issuer: totp.issuer,
  at src/multi-factor-auth/serializers/totp.serializer.ts:10:18
Tests: 1 failed, 13 passed

Restored, it passes. Also confirmed locally: tsc --noEmit clean, 902 tests pass across 48 suites, all 7 CI checks green.

One thing the diff obscures and that's worth stating for future readers: listUserAuthFactors lives in the MFA module but imports the user-management deserializer (multi-factor-auth.ts:36, aliased deserializeUMFactor). So the test added to multi-factor-auth.spec.ts really does cover the fixed code path — it isn't exercising the sibling by mistake.

One finding I'd act on

The WithSecrets widening is a consumer break with no correctness benefit. Details inline, but in short: EnrollAuthFactorOptions.type is 'totp' and nothing else (enroll-auth-factor.interface.ts:3), so createUserAuthFactor cannot return a non-TOTP factor. Making totp optional there forces every enroll consumer to null-check an impossible case.

Compiling a representative consumer against the new types:

probe.ts(3,69): error TS18048: 'f.totp' is possibly 'undefined'.   // enroll: f.totp.qrCode
probe.ts(5,60): error TS18048: 'f.totp' is possibly 'undefined'.   // list:   f.totp.issuer

The list break is justified — that code is already crashing at runtime for SMS users, so the compile error surfaces a real bug. The enroll break is pure cost, and it hits the consumers reading .totp.secret / .qrCode / .uri, which is most of them.

The PR description draws this distinction correctly ("true of the enroll endpoint, false of list") and then widens both anyway for consistency — trading an accurate type for a uniform one.

Two nits

Both inline: a duplicated 'sms' | 'totp' | 'generic_otp' union, and a dead fixture file.

Operational note (not a defect — the PR discloses it)

This doesn't reach the affected customer. Production is on SDK 8.10.0 via userManagement.listAuthFactors(), there is no 8.x maintenance branch, and the method has since moved to multiFactorAuth.listUserAuthFactors(). The dashboard only picks this up on a v9/v10 major upgrade.

So the workos/workos-side change is still the one that actually unblocks them, and the three unguarded call sites in user-authentication-factors.resolver.ts (:87 list, :272 enroll, :337 delete) remain live regardless of what happens here.

[jeffnv]

I'd keep the WithSecrets pair narrow — type: 'totp' with totp: TotpWithSecrets required — and only widen the non-secrets pair.

EnrollAuthFactorOptions.type is 'totp' and nothing else (enroll-auth-factor.interface.ts:3), so createUserAuthFactor provably cannot return a non-TOTP factor. Widening here doesn't fix a bug; it just makes every enroll consumer null-check an impossible case before reading .totp.secret / .qrCode / .uri. Verified it's a real compile break: error TS18048: 'f.totp' is possibly 'undefined'.

The fair counter-argument: with the guard added to deserializeFactorWithSecrets, that function genuinely can now return an object without totp, so the optional type is honest about it. If you want the narrow type back, either drop the guard on the WithSecrets variant only — an enroll response missing its TOTP payload is a contract violation you'd rather throw on than silently return undefined for — or keep the guard and throw explicitly.

Same applies to AuthenticationFactorWithSecretsResponse at lines 52-54.

[devin-ai-integration]

Done in fc2d3da — reverted the WithSecrets pair to type: 'totp' with required totp/TotpWithSecrets(Response), and dropped the guard in deserializeFactorWithSecrets so an enroll response missing its TOTP payload fails loudly (contract violation) rather than silently returning undefined. Only the non-secrets list-path pair stays widened.

[jeffnv]

This union already exists as FactorType in multi-factor-auth/interfaces/factor.interface.ts:9 with identical members. Two independently-maintained copies of one union, across these exact two modules, is the divergence that caused this incident in the first place.

Worth exporting FactorType and reusing it here rather than declaring a second copy that can drift when a fifth factor type shows up.

[devin-ai-integration]

Done in fc2d3da — exported FactorType from multi-factor-auth/interfaces/factor.interface.ts and made AuthenticationFactorType an alias of it, so there's a single union to update when a new factor type ships.

[jeffnv]

This fixture is unreferenced — the only list-factors import in the tree is multi-factor-auth.spec.ts:16 pulling ./fixtures/list-factors.json, which resolves to the MFA copy.

Updating it reads as added coverage but contributes none. Worth deleting the file, or leaving a note on why it's kept.

[devin-ai-integration]

As of 6cf80fd the fixture is no longer unreferenced — the new src/user-management/serializers/authentication-factor.serializer.spec.ts imports it and exercises deserializeFactor against all three factor types, so it now contributes coverage of the fixed deserializer directly.