[mathieucarbou]

• Some locks we missing (reported as advisory instead of bug report - ref: https://github.com/ESP32Async/ESPAsyncWebServer/security/advisories/GHSA-73ff-m33g-f4m4)

• Some complain about not providing the ability for users to completely validate a digest auth (again reported as an advisory instead of a feature or issue) => for that, new API is added to allow the user to implement a middleware and either recall the current code that checks digest auth or, the user can provide its own middle ware implementation of digest auth with a store to handle nonce. (ref: https://github.com/ESP32Async/ESPAsyncWebServer/security/advisories/GHSA-vx9x-x99p-3qw5)

• I will complete this PR to also see if we can rework the digest validation but still keeping the server stateless, and still allowing the user to use its own.

update

Digest challenges now include server-side anti-replay protections, with no
server-side per-nonce storage (server stays stateless):

• the challenge nonce embeds its issuing time and is signed with a
  per-instance secret (HMAC-MD5): forged, foreign or expired nonces are
  rejected; expiry issues a challenge with stale=true (RFC 7616 sec. 3.3)
  so compliant clients re-authenticate silently;
• the opaque is derived from the instance secret and identifies the
  server;
• the header uri field is bound to the effective request URI
  (RFC 7230 sec. 5.5 raw request-target, path + query), taken before
  decoding and rewrites: a captured header cannot be replayed on another
  endpoint nor with a different query string.

AsyncWebServerRequest::authenticateDigest() keeps its signature: NULL
nonce/opaque/uri arguments mean automatic (library) validation, values
take precedence. authenticate() defaults to the full stateless check.

New request API: rawUri() (effective request URI), authStale().
New server API: setDigestNonceTimeout(), setDigestSecret().
New build flag: ASYNCWEBSERVER_DIGEST_NONCE_TTL (default 60s).

Consolidated hex digit tables in literals.h (T_hex_chars, lowercase).

Example: examples/arduino/Auth gains endpoints for digest-with-hash and
test_digest.sh which verifies replay protection live.

[mathieucarbou]

@Ishanoshada : please participate in the work and test of this PR if you really want to help and contribute in this project. Thanks.

[Ishanoshada]

Review of #502 — fixes → main

Read the diff. The direction is right, but as pushed the PR does not compile, and the digest change is API-only — the default path remains vulnerable. Details below.

---

✅ src/AsyncWebSocket.cpp — lock fix is correct

Adding asyncsrv::lock_guard_type lock(_ws_clients_lock); at the top of both printf() and printf_P() closes the UAF: no raw client pointer escapes the lock, so _handleDisconnect() can no longer free the object mid-use. Matches the pattern already used by text(), binary(), ping(), close().

Remaining gap in the same function: va_list arg is still passed into the variadic AsyncWebSocketClient::printf(format, arg) — UB per [expr.call]/12, so printf(id, ...) is unreliable even when the race is gone. Suggest adding a vprintf(const char *format, va_list arg) overload on AsyncWebSocketClient and calling it from AsyncWebSocket::printf / printf_P (helper is in the advisory gist). Small change, removes the second defect in the same path.

---

⚠️ src/WebRequest.cpp — broken merge

1. Dead return in authenticate(username, password, realm, isHash):

return checkDigestAuthentication(..., NULL, NULL, NULL);
return authenticateDigest(username, password, realm, passwordIsHash, NULL, NULL, NULL);

First line is unreachable — please delete it.

2. authenticate(const char *hash) — missing brace + duplicated block:

bool AsyncWebServerRequest::authenticate(const char *hash) const {
  if (!_authorization.length() || hash == NULL) {
    return false;                        // ← no closing brace
  if (_authMethod == AsyncAuthType::AUTH_DIGEST) {
    return authenticateDigest(hash);
  }

  if (_authMethod == AsyncAuthType::AUTH_DIGEST) {   // ← duplicate block
    ...
    return checkDigestAuthentication(..., NULL, NULL, NULL);  // ← still NULL, NULL, NULL
  if (!_authorization.length() || hash == NULL) {   // ← duplicate guard
    return false;
  }
  return (_authorization.equals(hash));
}

This will not compile. Looks like a botched rebase of the authenticateDigest(hash) split — please re-push.

---

❌ Digest default path is still vulnerable

authenticate(..., username, password, ...) now delegates to authenticateDigest(..., NULL, NULL, NULL). The new overload lets a user pass their own nonce/opaque/uri, but anyone calling authenticate() without code changes is still exposed to the cross-endpoint replay — a header captured for GET /api/status is still accepted on GET /admin/delete.

Compare with the WebSocket fix: that one is applied by default. The digest fix should be too, otherwise the advisory is closed without the default behavior being safe.

Minimum stateless improvement — bind uri by default:

checkDigestAuthentication() already compares uri when it is non-NULL. uri verification needs no server state — it is a direct comparison of the header's uri field against _url.c_str(). So passing _url.c_str() from inside authenticate() closes cross-URI replay with zero state and no behavior change for legitimate clients (they always echo the URL they requested):

return authenticateDigest(username, password, realm, passwordIsHash,
                          NULL,          // nonce: user may supply via own store
                          NULL,          // opaque: user may supply
                          _url.c_str()   // ← bind to actual request URL
                         );

For nonce, the "server is stateless" position is legitimate — a nonce store is state. But either (a) stop minting a nonce in requestAuthentication() if it will never be checked, or (b) document on setAuthentication(..., AUTH_DIGEST) that nonce/opaque are not verified. Otherwise the API name implies protections that don't exist.

---

🧪 Tests

None of the fixes have tests. Suggest a HOST/Arduino-emulator test under tests/:

1. WebSocket UAF — N-thread race; assert printf(id, ...) never reads freed memory (deterministic interleaving + threaded). poc_ws_uaf.cpp from the advisory gist can be adapted directly.
2. Digest — capture header for GET /api/status, replay against GET /admin/delete with authenticateDigest(..., uri=_url.c_str()); assert 401. Same check via authenticate() after the _url.c_str() change above.

---

Summary

Item	Status
WebSocket lock fix	✅ Correct
va_list UB in same function	❌ Still present
Digest default path (NULL, NULL, NULL)	❌ Still vulnerable
uri binding (stateless)	❌ Not passed by default
WebRequest.cpp compiles	❌ Broken merge
Tests	❌ Missing

Happy to open a follow-up PR for the vprintf helper + uri=_url.c_str() default + tests if that helps move this forward. Thanks for picking this up.

[mathieucarbou]

Review of #502 — fixes → main

Read the diff. The direction is right, but as pushed the PR does not compile, and the digest change is API-only — the default path remains vulnerable. Details below.

✅ src/AsyncWebSocket.cpp — lock fix is correct

Adding asyncsrv::lock_guard_type lock(_ws_clients_lock); at the top of both printf() and printf_P() closes the UAF: no raw client pointer escapes the lock, so _handleDisconnect() can no longer free the object mid-use. Matches the pattern already used by text(), binary(), ping(), close().

Remaining gap in the same function: va_list arg is still passed into the variadic AsyncWebSocketClient::printf(format, arg) — UB per [expr.call]/12, so printf(id, ...) is unreliable even when the race is gone. Suggest adding a vprintf(const char *format, va_list arg) overload on AsyncWebSocketClient and calling it from AsyncWebSocket::printf / printf_P (helper is in the advisory gist). Small change, removes the second defect in the same path.

⚠️ src/WebRequest.cpp — broken merge

1. Dead return in authenticate(username, password, realm, isHash):

return checkDigestAuthentication(..., NULL, NULL, NULL);
return authenticateDigest(username, password, realm, passwordIsHash, NULL, NULL, NULL);

First line is unreachable — please delete it.

2. authenticate(const char *hash) — missing brace + duplicated block:

bool AsyncWebServerRequest::authenticate(const char *hash) const {
  if (!_authorization.length() || hash == NULL) {
    return false;                        // ← no closing brace
  if (_authMethod == AsyncAuthType::AUTH_DIGEST) {
    return authenticateDigest(hash);
  }

  if (_authMethod == AsyncAuthType::AUTH_DIGEST) {   // ← duplicate block
    ...
    return checkDigestAuthentication(..., NULL, NULL, NULL);  // ← still NULL, NULL, NULL
  if (!_authorization.length() || hash == NULL) {   // ← duplicate guard
    return false;
  }
  return (_authorization.equals(hash));
}

This will not compile. Looks like a botched rebase of the authenticateDigest(hash) split — please re-push.

❌ Digest default path is still vulnerable

authenticate(..., username, password, ...) now delegates to authenticateDigest(..., NULL, NULL, NULL). The new overload lets a user pass their own nonce/opaque/uri, but anyone calling authenticate() without code changes is still exposed to the cross-endpoint replay — a header captured for GET /api/status is still accepted on GET /admin/delete.

Compare with the WebSocket fix: that one is applied by default. The digest fix should be too, otherwise the advisory is closed without the default behavior being safe.

Minimum stateless improvement — bind uri by default:

checkDigestAuthentication() already compares uri when it is non-NULL. uri verification needs no server state — it is a direct comparison of the header's uri field against _url.c_str(). So passing _url.c_str() from inside authenticate() closes cross-URI replay with zero state and no behavior change for legitimate clients (they always echo the URL they requested):

return authenticateDigest(username, password, realm, passwordIsHash,
                          NULL,          // nonce: user may supply via own store
                          NULL,          // opaque: user may supply
                          _url.c_str()   // ← bind to actual request URL
                         );

For nonce, the "server is stateless" position is legitimate — a nonce store is state. But either (a) stop minting a nonce in requestAuthentication() if it will never be checked, or (b) document on setAuthentication(..., AUTH_DIGEST) that nonce/opaque are not verified. Otherwise the API name implies protections that don't exist.

🧪 Tests

None of the fixes have tests. Suggest a HOST/Arduino-emulator test under tests/:

1. WebSocket UAF — N-thread race; assert printf(id, ...) never reads freed memory (deterministic interleaving + threaded). poc_ws_uaf.cpp from the advisory gist can be adapted directly.
2. Digest — capture header for GET /api/status, replay against GET /admin/delete with authenticateDigest(..., uri=_url.c_str()); assert 401. Same check via authenticate() after the _url.c_str() change above.

Summary

Item Status
WebSocket lock fix ✅ Correct
va_list UB in same function ❌ Still present
Digest default path (NULL, NULL, NULL) ❌ Still vulnerable
uri binding (stateless) ❌ Not passed by default
WebRequest.cpp compiles ❌ Broken merge
Tests ❌ Missing
Happy to open a follow-up PR for the vprintf helper + uri=_url.c_str() default + tests if that helps move this forward. Thanks for picking this up.

Thanks!

You are too early ;-)

This PR is still a WIP - I have some local changes that implements a nonce strategy based on a random server key and TTL, which should be backward compatible.

for the va_list => yes please open a separate PR, thanks :-)

[mathieucarbou]

Closing in favor of :

• Fix missing locks #503
• fix(auth): Digest authentication replay protection (stateless signed nonces) #504