authhelper: support single-input TOTP fields and improve field interaction by sarathivengadesh · Pull Request #7259 · zaproxy/zap-extensions

sarathivengadesh · 2026-04-07T14:50:43Z

Overview

The TOTP_FIELD step previously called element.sendKeys() directly, which has two problems:

  - Single vs split input — sites like GitHub and Codeberg use a single input for the full 6-digit OTP code. The old code always sent one character per field, so single-input TOTP fields received only one digit and authentication failed.
  
  - JS framework compatibility — sendKeys() alone does not fire input/change events, so React and Angular apps never registered the typed value, causing username and password fields to appear empty on submit.

Changes:

  - TOTP_FIELD now detects whether there is one field (sends full 6-digit code) or multiple fields (sends one character each), by counting enabled TOTP_FIELD steps
  - Added fillFieldWithEvents() which fires input and change JS events after sendKeys(), fixing field registration in React/Angular/Vue apps and Shadow DOM components
  - TOTP code is pre-generated once before stepping through split fields, preventing a 30-second boundary crossing mid-flow from producing a mismatched code
  - MsLoginAuthenticator updated to use fillFieldWithEvents() consistently

…nteraction - Add TOTP_FIELD support for single combined input (e.g. GitHub, Codeberg style) in addition to existing per-character input boxes - Replace fillField with fillFieldWithEvents to trigger JS events so React/Angular apps register typed values correctly - Add precomputedTotpCode overload to avoid clock skew across multiple steps - Fix MsLoginAuthenticator to use updated DefaultAuthenticator signature Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

github-actions · 2026-04-07T14:51:01Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

psiinon · 2026-04-07T15:01:57Z

Checkmarx One – Scan Summary & Details – a752d2f5-29b2-4ae5-b4cd-c0ef9279baeb

Great job! No new security vulnerabilities introduced in this pull request

Use @Checkmarx to interact with Checkmarx PR Assistant.
Examples:
@Checkmarx how are you able to help me?
@Checkmarx rescan this PR

sarathivengadesh · 2026-04-07T15:03:43Z

I have read the CLA Document and I hereby sign the CLA

thc202 · 2026-04-07T15:06:55Z

This needs tests.

thc202 · 2026-04-07T15:17:02Z

Also examples where the current sendKeys fails, that sounds more like a bug in the WebDriver spec since they are supposed to behave like user input.

sarathivengadesh · 2026-04-08T14:53:51Z

#7259 (comment)

@thc202 Thanks for the feedback! To clarify, there are two separate issues addressed in this PR:

Split TOTP input boxes (core bug fix)
Some sites (e.g. Accuknox.com MFA) render the 6-digit OTP as 6 separate boxes instead of a single combined field. With the old code, each TOTP_FIELD step received sendKeys("123456") — but since each box has maxlength=1, only "1" was accepted per box:

Old behavior: [1][1][1][1][1][1] — authentication fails
- If only first field referenced: [123456][ ][ ][ ][ ][ ] — authentication fails
- Single combined input: [123456] — works correctly (unaffected)
- This is not a sendKeys() limitation — the problem is that we were sending the wrong value (full code) to each single-character box. The fix detects when multiple TOTP_FIELD steps exist, pre-computes the TOTP code once (to avoid clock-boundary drift), then sends code.charAt(i) to each field in order, resulting in [1][2][3][4][5][6].

Tested against:
- A site with 6 separate maxlength=1 OTP input boxes (split-field style) using Accuknox — ✅ fixed
- Sites with a single combined OTP input field using https://codeberg.org/— ✅ still working
fillFieldWithEvents (separate enhancement)
React and Angular use synthetic event listeners (input/change) to track field state. sendKeys() fills the value visually, but may not fire these events, causing framework-managed validation (e.g. "Next" button) to remain disabled. Firing those events via JS after sendKeys() ensures compatibility with such frameworks.

thc202 · 2026-04-08T15:09:57Z

I understand what the PR is attempting to do.

…add tests - Revert MsLoginAuthenticator PROOF_TOTP handling back to original clear()+sendKeys() as the existing code was working correctly - Add AuthenticationStepUnitTest covering split-TOTP charIndex logic, single combined field path, precomputed code, and TotpSupport fallback - Add FillFieldWithEventsTest in AuthUtilsUnitTest covering fill, clear-before-fill, JS event dispatch, and non-JS driver robustness - Fix DefaultAuthenticator: remove unnecessary .toString() on String return

sarathivengadesh · 2026-04-09T04:25:04Z

#7259 (comment)
@thc202 Added AuthenticationStepUnitTest with 9 tests covering split-TOTP char index logic, single combined field, precomputed code, and TotpSupport fallback. Also added FillFieldWithEventsTest (4 tests) in AuthUtilsUnitTest for the fillFieldWithEvents method. All 13 tests pass.

thc202 · 2026-04-10T11:04:01Z

It's not necessary to ask for a review, I get notifications of all comments and pushes.

thc202 · 2026-04-11T08:01:17Z

+        // Pre-generate TOTP code once so all 6 character fields use the same code,
+        // avoiding clock-window drift if a 30s boundary crosses during step execution.
+        String precomputedTotpCode =
+                splitTotpFields ? TotpSupport.getCode(credentials) : null;


This should be generated when needed not beforehand, the code will be outdated if the steps before the TOTP_FIELD step take more time than the defined period. This also fails to validate the required number of digits.

The change itself is also out of place, this breaks the step implementation isolation, better to provide a state object for the steps than add ad-hoc step specific logic here.

Addressed. Added AuthenticationContext — a state object created once per authentication attempt and passed to every step. The TOTP code is now generated lazily at the moment the first TOTP_FIELD step executes (not before all steps run), so it stays fresh even if preceding steps take time. All TOTP logic remains inside AuthenticationStep; DefaultAuthenticator has no step-specific knowledge.

thc202 · 2026-04-11T08:02:24Z

    }

+    @Nested
+    class FillFieldWithEventsTest {


These tests are not testing anything (you can change the script and the tests still pass). Please, add proper tests in BrowserTest.

thc202 · 2026-04-11T08:03:31Z

#7259 (comment)
Can you provide the examples?

thc202 · 2026-04-11T10:17:19Z

but since each box has maxlength=1, only "1" was accepted per box

For those cases which we can detect how many digits the field supports we should fill each field automatically instead of requiring the user to split the TOTP into several steps.

…ct split OTP fields, browser tests - Add AuthenticationContext to lazily generate and cache the TOTP code at the moment the first TOTP_FIELD step executes, avoiding stale codes when preceding steps cross a 30-second window boundary - Remove pre-computation of TOTP code from DefaultAuthenticator; all step execution now goes through step.execute(wd, credentials, ctx) - Auto-detect split single-character OTP input boxes at runtime: if the resolved TOTP_FIELD element has maxlength="1", AuthUtils.fillSplitOtpFields locates all sibling inputs via JS and fills each with one digit — no need to configure multiple TOTP_FIELD steps - Replace mock-only FillFieldWithEventsTest (which did not verify script content) with two real BrowserTest methods: one that confirms input/change events fire on a React-style listener, and one that confirms split OTP boxes are auto-filled in DOM order - Update AuthenticationStepUnitTest for the new context-based API

sarathivengadesh · 2026-04-15T05:46:42Z

#7259 (comment)

The concrete reproduction case is app.dev.accuknox.com — its OTP screen uses React-controlled input[maxlength="1"] boxes. Plain sendKeys writes to the raw DOM but React's internal state never updates because React listens to synthetic events, not native DOM mutations. As a result the Submit button stays disabled and the form submits React's last known (empty) value. Dispatching an explicit input event after sendKeys triggers React's onChange handler, syncs the component state, and authentication succeeds. This was verified against that app.

sarathivengadesh · 2026-04-15T05:47:47Z

#7259 (comment)
Implemented. AuthenticationStep now checks element.getDomAttribute("maxlength") at runtime when executing a TOTP_FIELD. If the value is "1", AuthUtils.fillSplitOtpFields uses JavaScript to find all input[maxlength="1"] within the same form or parent container and fills each with one character of the TOTP code. Users now only need a single TOTP_FIELD step pointing to the first digit box — no manual splitting required.

psiinon · 2026-04-15T08:12:28Z

@sarathivengadesh could you create an example test app for this in the dev add-on?
This app contains a set of test apps including test login pages - see https://www.zaproxy.org/docs/desktop/addons/dev-add-on/
An LLM will hopefully be able to create a good example based on the others with minimal effort 😁

thc202 · 2026-04-15T08:18:07Z

#7259 (comment)

Can you share test credentials (or sample page)? I'd like to verify the behaviour.

thc202 changed the title ~~fix(authhelper): support single-input TOTP fields and improve field interaction~~ authhelper: support single-input TOTP fields and improve field interaction Apr 7, 2026

thc202 reviewed Apr 7, 2026

View reviewed changes

Comment thread ...uthhelper/src/main/java/org/zaproxy/addon/authhelper/internal/auth/MsLoginAuthenticator.java Outdated

thc202 mentioned this pull request Apr 8, 2026

authhelper: Handle StaleElementReferenceException in DefaultAuthenticator #7262

Open

sarathivengadesh force-pushed the fix/totp-single-input-auth branch from 4a9bd06 to d7dd821 Compare April 9, 2026 03:14

sarathivengadesh requested a review from thc202 April 9, 2026 04:25

thc202 removed their request for review April 9, 2026 07:50

sarathivengadesh requested a review from thc202 April 10, 2026 04:19

thc202 removed their request for review April 10, 2026 07:48

thc202 reviewed Apr 11, 2026

View reviewed changes

sarathivengadesh force-pushed the fix/totp-single-input-auth branch from 8d1aeb2 to 08da194 Compare April 15, 2026 05:42

sarathivengadesh requested a review from thc202 April 15, 2026 05:47

thc202 removed their request for review April 15, 2026 06:49

Uh oh!

Conversation

sarathivengadesh commented Apr 7, 2026

Overview

Changes:

Uh oh!

github-actions bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psiinon commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Great job! No new security vulnerabilities introduced in this pull request

Uh oh!

sarathivengadesh commented Apr 7, 2026

Uh oh!

thc202 commented Apr 7, 2026

Uh oh!

thc202 commented Apr 7, 2026

Uh oh!

Uh oh!

sarathivengadesh commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thc202 commented Apr 8, 2026

Uh oh!

sarathivengadesh commented Apr 9, 2026

Uh oh!

thc202 commented Apr 10, 2026

Uh oh!

thc202 Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

sarathivengadesh Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

thc202 Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

thc202 commented Apr 11, 2026

Uh oh!

thc202 commented Apr 11, 2026

Uh oh!

sarathivengadesh commented Apr 15, 2026

Uh oh!

sarathivengadesh commented Apr 15, 2026

Uh oh!

psiinon commented Apr 15, 2026

Uh oh!

thc202 commented Apr 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Apr 7, 2026 •

edited

Loading

psiinon commented Apr 7, 2026 •

edited

Loading

sarathivengadesh commented Apr 8, 2026 •

edited

Loading