feat: cap the remaining unthrottled endpoints, record replay outcomes - #32
Merged
Conversation
The OpenAPI register description still said registration_state and oauth_state would be rejected until OAuth login shipped. That flow has been live since the third-party login merge, so the text now describes what the endpoint does: paired-or-omitted, GetDel consumption, the same-transaction account plus binding, new accounts only, and 50300 on a Redis outage. /user/avatar had an empty description while it is the one path in the file with no route registered. It now says so in place, and the info block states that this file is the target contract rather than a route inventory, so a consumer does not have to check each path against the running service. The pg_cron section of the schema design read as though the jobs existed. No migration creates the extension or schedules anything, so expired rows are not being cleaned up; the section is now marked as a design that has not landed. Also corrects "three tightenings" to four in the admin chapter, which already listed four.
GET /oauth/{github,lark} had the same shape as GET /oauth/authorize —
unauthenticated, one Redis oauth_state key written per call — but no cap.
POST /oauth/exchange-code was likewise uncapped, POST /auth/register runs
a 600k-iteration PBKDF2 per accepted call, and GET /card/:id takes a
sequential user ID, so an uncapped endpoint is a scrape of every public
card.
All four follow the existing pattern: a named limiter per endpoint,
enforced in the service, keyed by caller IP (the only key available on
three of them). Fail-open with a WARN, per the PRD's rate-limit class.
No global middleware, so a new route still inherits nothing and must name
its own cap; .env.example now says that where the old global knob was.
Three ordering choices, each covered by a test:
- authorize throttles before resolving the provider, otherwise a disabled
provider's 40000 route is a free probe.
- exchange-code throttles before the empty-code check, since the caller
picks the input and rejecting blanks for free leaves the Redis GetDel
per guess uncapped.
- card throttles before the ID check, because the 404 an invalid ID gets
is itself what an enumerator is reading.
A throttled call also must not spend a one-time credential: rejecting one
leaves the Register-Ticket and the login_code intact, so tripping the
limit cannot burn a victim's live value.
Config drift removed: RATE_LIMIT_GLOBAL_RPS and RATE_LIMIT_CAPTCHA_RPM
were set in .env.example but read nowhere. The first is dropped with the
decision not to add a global limiter; the second duplicated the
send-email pair that already exists. RATE_LIMIT_REGISTER_RPH is now
wired to the code it named.
The four defaults are reasoned from endpoint shape, not measured traffic,
and should be revisited against real numbers before launch.
session.Refresh revokes the whole token family on a replayed refresh token, in two separate branches: an already-revoked token, and a rotation that the repository rejects as a replay. Both wrote an audit row that was indistinguishable from any other refresh failure — same action, same invalid-token error code as an expired token or a client mismatch. That is the one case in the log that means a token leaked and live sessions were cut in response, and it was not searchable. The row now carries an outcome: rotated on success, refresh_replayed when the replay defense fires. The name matches the outcome the oauth service already records on its own token path, so one query covers both. The family_id was already recorded as the resource ID; a test now pins it so the row stays tied to the tokens that were revoked because of it.
Both lines in the implementation checklist were unchecked and described as barely started: rate limiting as "login and two OAuth endpoints done, five pending", audit logging as "login/logout done, the rest to follow". Both were wrong. Rate limiting now covers every endpoint that needed it, and audit rows are written across session, OAuth, third-party login and admin — registration, password change and reset, identity binding and unbinding, and client changes were all already instrumented. Stale checklists are not free: reading them as current is how the last two rounds of work started from a list of things that turned out to be implemented. Each line now states what is actually wired, and the module table says pg_cron's SQL exists only as a design so nobody reads the schema doc as a description of the running database. Also records that limiter defaults were reasoned from endpoint shape rather than measured, and that there is no global middleware — a new route inherits no cap and must name its own.
API文档 §2 still said all five third-party login endpoints were unthrottled, in the same tree where they are now capped. A contract that contradicts the code it describes is worse than one that is merely incomplete, so that note is replaced with the actual quotas. Adds the limit description to §1.3 register, §2 third-party login and §3.4 card, and a 429 response to the five affected paths in the OpenAPI file, reusing the existing TooManyRequests component. Each note records the three things a caller cannot infer from a status code: the env var and default, where the check sits relative to the endpoint's own validation, and that a throttled call does not spend a Register-Ticket or a login_code. GitHub and Lark authorize share one counter — one limiter instance under the endpoint name oauth_login — so exhausting the quota on one also blocks the other; that is stated rather than left to be discovered.
s3loy
previously approved these changes
Aug 1, 2026
RATE_LIMIT_REGISTER_RPH=3 per IP would have blocked enrollment. The campus network NATs a whole building behind one egress address, so three registrations per hour was the quota for every student sharing it — exactly the traffic this endpoint sees when a new cohort signs up. The check also sat above every validation, so requests that never reached PBKDF2 still spent quota: a short password, a bad college, an occupied student ID. A user mistyping their own form locked themselves, and everyone behind their NAT, out for an hour. The cap exists to bound derivation cost, so it is now keyed by Register-Ticket — one verified email, which is the unit that cost should be metered against — and enforced just before the identity resolution and hash, after every free rejection. Ticket acquisition is already capped by the send-email limiters, so the cost stays bounded. It still precedes the registration_state consumption, so a throttled call spends neither one-time credential. The window must not exceed the ticket 5-minute TTL. A longer one is self-defeating: it is still closed when the ticket it throttles has expired, leaving the caller nothing to retry with — which is what the API docs promised. ValidateAPIAuth now rejects that configuration, and a test pins the default against its own ceiling.
The outcome field was justified by saying an expired token, a client mismatch and a replay share one action and one error code, so only the name separates them. That was not true of the code: both non-replay branches returned without writing an audit row at all, so every success=false refresh row was already a replay and the outcome distinguished nothing. That is the weaker half of the claim. A replay row means something because the mundane rejections are on record next to it; with them missing, a quiet log cannot be read as absence of replays — only as absence of writes. Both branches now audit, with expired and client_mismatch outcomes, and the comment describes what the code does. client_mismatch is not reachable through the first-party flow, so it means a misrouted client or a token being probed.
…ard cap
docker-compose.yml enumerates every RATE_LIMIT_* variable explicitly, and
the four added in this branch were missing. The environment block is a
whitelist, so inside the compose stack those values could not be
overridden from .env at all — they were pinned to their defaults. That
hurts most on exactly the two knobs that need field tuning.
RATE_LIMIT_CARD_RPM was 60/60s with a comment about pages that render
several cards, but the endpoint exists for a member wall behind a shared
egress. One visitor loading a few dozen cards would spend the whole NAT
minute and 429 the next person. Raised to 300 and the reasoning corrected:
a cap this loose slows a scrape rather than preventing it, and bulk
public-card reads belong behind the proxy cache, which is where the
capacity defense already lives.
Docs follow the register rekey: the retry the API contract promised
("retry with the same ticket once the window recovers") was impossible
with a 1h window over a 5m ticket. Also records why register keys on the
ticket rather than the IP, so the next reader does not switch it back.
govet shadow flagged the register limiter check: err is already in scope from the student-ID lookup above, and := declared a second one. Harmless as written, but the pattern is worth not keeping — the next statement assigns oauthIdentity, err from the outer err, so a shadow here is one edit away from a check that reads the wrong variable.
s3loy
approved these changes
Aug 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
补齐 PRD §11 剩下的两项：四个仍未限流的端点，以及 refresh 重放事件在审计日志里不可检索的问题。顺带修掉一批与实现矛盾的文档状态。
限流（
ecc9a5f）GET /oauth/{github,lark}/oauth/authorize形状相同：无认证、每次调用写一个oauth_state键，但后者有帽子、这两个没有POST /oauth/exchange-codePOST /auth/registerGET /card/:id沿用既有范式：service 层具名限流器，fail-open + WARN，超限返回
42900带Retry-After。三处检查顺序是刻意的，各有测试锁定：
40000而非 404，若限流在后，它就是不受限的探测面。404本身就是枚举者要读的信号。另有两处保护一次性凭证：被限流的请求不消费 Register-Ticket 和
login_code，否则触发限流即可烧掉他人活跃凭证。配置漂移清理：
RATE_LIMIT_GLOBAL_RPS和RATE_LIMIT_CAPTCHA_RPM此前设在.env.example但代码零引用。前者随"不做全局限流"的决定删除，后者与已实现的 send-email 双键重复。RATE_LIMIT_REGISTER_RPH接上了它本该驱动的代码。审计（
74bae7e）session.Refresh在两条分支上检测重放并级联撤销整个 token family，但写入的审计行与任何普通刷新失败无法区分——同一 action、同一40105，跟过期或客户端不匹配长得一样。日志里唯一意味着「令牌泄露、活跃会话已被切断」的事件，反而筛不出来。现在 detail 带
outcome：成功rotated，重放防御触发refresh_replayed。与 oauth 侧已有的refresh_replayed/code_replayed同名，一条查询覆盖两条 token 路径。文档（
7a51dfd98d895d03f852f）docs/API文档.md§2 此前仍写着「本章 5 个端点目前均未限流」——就在这些端点已被限流的同一棵树里。同一份契约既宣称未限流又存在限流，比只是不完整更糟。其余：OpenAPI 的注册补全描述还说
registration_state会被拒绝（该流程早已上线）；/user/avatar空描述但路由未注册；pg_cron 章节读起来像已部署（实际任何 migration 都没创建它，过期数据目前不会被清理）；PRD 两行追踪状态严重滞后。评审时请留意
限流默认值没有流量依据。 20 / 30 / 3 / 60 是按端点形状推定的，不是实测。上线前应按真实数据校准。
GitHub 与 Lark 共用一个计数桶。 单一 limiter 实例、endpoint 名
oauth_login、仅按 IP。所以同一 IP 把 GitHub 登录用满 20 次后，Lark 登录也会被拒。这是取舍而非疏漏（按 provider 分桶需多一组配置），但值得反对的话请提。没有全局限流中间件，是刻意的。 理由：Caddy 与网络层才是容量防线；全局 RPS 的数值缺少对应物，要么永不触发要么在真实高峰误伤；校园网 NAT 下按 IP 全局限流会让整栋楼共享一个桶。代价是新增路由不继承任何配额，必须显式声明——
.env.example与 PRD 都写明了这一点。admin 组仍未限流（
docs/API文档.md:1277已承认）。keyword会展开三个无法走索引的ILIKE加一次全表COUNT(*)。没纳入本 PR 的理由是：有认证 + 角色门禁的端点属于另一类风险，混进来会让边界模糊。若认为该一并处理，我另开一个。验证
429+Retry-After映射、重放 outcome（两条分支各一）go test -race -shuffle=on -coverprofile -covermode=atomic ./...，含 Testcontainers 集成测试gofmt/go build/go vet干净；openapi.yaml解析通过，37 path 完整