Skip to content

fix(security): use secrets for verification code and atomic token rotation - #279

Merged
minorcell merged 1 commit into
1024XEngineer:mainfrom
xiaocheny214:fix/security-crypto-token-rotation
Aug 13, 2026
Merged

fix(security): use secrets for verification code and atomic token rotation#279
minorcell merged 1 commit into
1024XEngineer:mainfrom
xiaocheny214:fix/security-crypto-token-rotation

Conversation

@xiaocheny214

Copy link
Copy Markdown
Contributor

问题

用户认证流程中存在两个安全缺陷(#200):

  1. 验证码使用非加密安全的 PRNGrandom.choices 使用 Mersenne Twister,攻击者若能观察足够多的输出可以预测后续验证码
  2. 刷新令牌轮换非原子 — 删除旧 token 和存储新 token 之间存在竞态窗口,可能导致用户意外登出或 token 复用

修复

1. 验证码生成改用 secrets

# 之前
import random
return "".join(random.choices(string.digits, k=6))

# 之后
import secrets
return "".join(secrets.choice(string.digits) for _ in range(6))

secrets.choice 使用操作系统级 CSPRNG,不可预测。

2. Token 轮换改为原子操作

# 之前(非原子)
self.redis.delete(redis_key)          # ← 第一步
self._store_refresh_token(new_jti, user_id)  # ← 第二步

# 之后(原子 MULTI/EXEC)
pipe = self.redis.pipeline()
pipe.delete(redis_key)
pipe.setex(new_redis_key, REFRESH_TOKEN_EXPIRE_SECONDS, str(user_id))
pipe.execute()

Redis pipeline 使用 MULTI/EXEC 保证两个操作在同一事务中执行,消除竞态窗口。

测试

  • 所有 35 个现有测试通过
  • secrets.choice 生成的验证码格式不变(6 位数字)
  • 原子操作不影响 refresh_tokens() 的返回值语义

Closes #200

@vercel

vercel Bot commented Aug 13, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
windup Ignored Ignored Preview Aug 13, 2026 8:17am

@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@fennoai fennoai 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.

The verification-code change is sound, but the refresh-token rotation still has a concurrency flaw: the old token is checked with a standalone GET before the transaction. Two concurrent requests can both observe it as valid and each execute DELETE old + SET new, so the same refresh token can be redeemed more than once. The rotation needs an atomic compare-and-delete/consume step (for example, a Redis WATCH transaction or a Lua script that validates and rotates only if the old key still exists), with a regression test covering concurrent reuse.

Comment thread backend/packages/app/src/windup_app/server/user/service.py Outdated
…ation

- Replace random.choices with secrets.choice for cryptographically secure
  verification code generation (prevents PRNG prediction attacks)
- Use Redis Lua script for atomic check-delete-store token rotation
  (eliminates race condition where concurrent requests could both pass
  the GET check and each successfully rotate the same refresh token)
- Add concurrent reuse regression test

Closes 1024XEngineer#200
@xiaocheny214
xiaocheny214 force-pushed the fix/security-crypto-token-rotation branch from 7ffed3e to 791b9fb Compare August 13, 2026 08:16
@minorcell
minorcell merged commit dfa79b2 into 1024XEngineer:main Aug 13, 2026
5 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.

fix(security): 验证码用 random.choices 生成、刷新令牌轮换非原子

2 participants