Skip to content

fix(auth): remove dead challenge/response surface (security findings 8+16)#3229

Open
frdomovic wants to merge 3 commits into
masterfrom
fix/remove-dead-challenge-surface
Open

fix(auth): remove dead challenge/response surface (security findings 8+16)#3229
frdomovic wants to merge 3 commits into
masterfrom
fix/remove-dead-challenge-surface

Conversation

@frdomovic

Copy link
Copy Markdown
Member

Security finding

[M] Challenge/response has no replay protection / is decorative (security-review items 8 + 16) — crates/auth/src/auth/token/jwt.rs.

GET /auth/challenge mints a random nonce + jti, but nothing ever stores them and verify_challenge has zero callers — a signed challenge is never demanded by any login flow. The surface is security theater: it suggests proof-of-possession that doesn't exist, and any real fix (a single-use nonce store consumed by a wallet provider's token exchange) would be dead machinery, since core ships no wallet provider — plus a naively-persisted nonce per unauthenticated GET is a storage-write DoS vector.

Decision (team): no NEAR/wallet login this iteration — user_password is the only provider, and password auth needs no PoP step. Remove the dead surface instead of building a nonce store for a provider that doesn't exist.

Before → after

Before After
GET /auth/challenge Public route returning a nonce+jti that nothing verifies (replayable for its whole 5-min "lifetime" — if anything had consumed it). Route removed (404).
TokenManager generate_challenge / verify_challenge (dead) + ChallengeClaims. Removed.
Secrets Three secret types incl. JwtChallenge, rotated forever for nothing. JwtAuth + Csrf only. Legacy system:secrets:jwt_challenge* storage entries are simply never read again (doc-commented on SecretType).
Docs operate/auth.mdx et al. describe challenge lifetime / wallet challenge auth. Updated to the real surface.

Deliberately kept: the TEE/KMS attestation challenge flow (crates/merod/src/kms) and libp2p/sync uses of the word — unrelated subsystems. NodeChallenge DTOs in calimero-server-primitives are unrouted legacy public API — candidate for a separate dead-code sweep.

How to test

cargo test -p mero-auth   # 93 tests green
cargo check -p mero-auth -p calimero-server

Manual: curl http://<node>/auth/challenge → 404; user-password login and token refresh unaffected.

Client follow-ups (separate PRs, non-breaking today since no server ever advertises a wallet provider)

  • mero-js: remove getChallenge() (src/auth-api/auth-client.ts:86), ChallengeResponse type, and the MSW mock — breaking API change for the package, needs a version bump.
  • auth-frontend: remove the dead near_wallet challenge→sign→token branch in LoginView.tsx:277-340 + mocks/fixtures.

Related: #3079/#3080/#3081/#3082/#3083 (the auth security-review stack). Expect a trivial textual conflict with #3083 in jwt.rs/secrets.rs — whichever merges second rebases.

…8+16)

The GET /auth/challenge endpoint minted a signed nonce+jti that nothing
ever stored or consumed, and TokenManager::verify_challenge had zero
callers - the surface was decorative. Since user_password is the only
shipped provider (no NEAR-wallet login this iteration), remove the dead
surface instead of building a nonce store:

- drop TokenManager::generate_challenge / verify_challenge and
  ChallengeClaims
- drop the /challenge route, challenge_handler, and ChallengeResponse
- drop SecretType::JwtChallenge, its storage keys, rotation/initialize
  entries, and get_jwt_challenge_secret; legacy
  system:secrets:jwt_challenge* entries in storage are simply never
  read again
- update operator/contributor docs that described the challenge flow

TEE/KMS attestation challenges (crates/merod/src/kms) and the unrelated
NodeChallenge admin DTOs are untouched.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@meroreviewer meroreviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 MeroReviewer

Reviewed by 2 agents | Quality score: 90% | Review time: 190.4s


✅ No Issues Found

All agents reviewed the code and found no issues. LGTM! 🎉


🤖 Generated by MeroReviewer | Review ID: review-8292737c

@meroreviewer

meroreviewer Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Documentation Review

The following documentation may need updates based on the changes in this PR:

  • 🟡 architecture/crates/auth.html: Files matching crates/auth/** were changed but architecture/crates/auth.html was not updated (per source_to_docs_mapping).

@meroreviewer meroreviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 90% | Review time: 834.1s


✅ No Issues Found

All agents reviewed the code and found no issues. LGTM! 🎉


🤖 Generated by AI Code Reviewer | Review ID: review-aa980757

@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants