feat(security): bound credential guessing and close the backend audit - #145
Merged
Conversation
A manual audit of the externally reachable surfaces found one real gap. The
rest held: the outbound HTTP REAction pins its resolved address against DNS
rebinding, the WebSocket checks Team membership before granting a watch,
authorization is resolved in the use-case layer on every Team route, secrets
never leave the server, and passwords use Argon2.
The gap: no rate limiting anywhere, in the application or at the ingress.
POST /api/auth/sign-in was an unbounded guessing loop.
- Add an in-process token-bucket limiter on the three unauthenticated
/api/auth/* routes, keyed by the caller address that `resolve_client_ip`
already derives, so the configured proxy depth decides how far an
X-Forwarded-For chain is trusted. Default 20 attempts per 5 minutes,
overridable; denials answer 429 with Retry-After.
Written rather than pulled in: `tower_governor` would add four transitive
crates to a curated tree for one middleware. State is per replica, which
bounds the loop without pretending to meter quota.
- Document the collaboration limits in the README as VIGIL requires. The six
reaction emojis were claimed as documented but never listed, and the 2 000
character private-message limit only lived in the portal catalogue.
- Delete two dead endpoints. `PUT /service-connections/http` had no consumer
anywhere; `PUT /service-connections/github` was reached only by the demo
seed script. Both duplicate `by-service/{service}`, which dispatches to the
same use-case methods, so callers move over with no behaviour change.
329 unit tests including seven for the limiter, plus two integration tests
proving a spent budget answers 429 with a usable Retry-After and that one
address cannot lock out another. No new dependency: cargo deny stays green.
RomeoCavazza
force-pushed
the
hardening/auth-rate-limit
branch
from
August 3, 2026 16:18
3fa346b to
e35c635
Compare
Merged
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
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.
Why
A manual audit of every externally reachable backend surface found one real gap. Everything else held:
resolve_to_addrs(defeats rebinding),redirect::Policy::none()Watchverifies Team membership before joining a watcher roster;StatusTypingadditionally requires thecan_signal_typingcapabilityOsRngsaltsqlx::query!Validation::default()pins HS256 and validatesexp; server-side revocation on logoutThe gap: no rate limiting anywhere, application or ingress (no
tower_governor, no Traefik middleware ink8s/).POST /api/auth/sign-inwas an unbounded guessing loop.Changes
Rate limiting on
/api/auth/*. An in-process token bucket keyed by the caller address thatresolve_client_ipalready derives — soOPSWARDEN_TRUSTED_PROXY_HOPSdecides how far anX-Forwarded-Forchain is trusted. Without that, one reverse proxy would share a single bucket across every user, and a spoofed header would mint a fresh budget per request. Defaults to 20 attempts per 5 minutes; denials answer429withRetry-After.Written rather than pulled in:
tower_governoradds four transitive crates to a deliberately curated tree gated bytooling/deny.toml. ~100 owned lines for one middleware is the better trade. State is per replica — the goal is to bound a guessing loop, not to meter quota, and that limitation is documented in the module.README collaboration limits. VIGIL requires the reaction emoji set and the private-message length to be documented in the README. The six emojis were claimed as documented but never listed; the 2 000-character limit lived only in the portal catalogue. Both are now in a table, with the new auth budget.
Two dead endpoints deleted.
PUT /service-connections/httphad no consumer anywhere in the repo;PUT /service-connections/githubwas reached only bytooling/seed_demo.sh. Both duplicateby-service/{service}, which dispatches to the same use-case methods — callers move over with no behaviour change.Verification
Retry-Afteraccuracy, no over-refill after idling, and pruning so attacker-generated keys cannot grow the map.429with a usableRetry-After, and one address cannot lock out another.cargo fmt --check,clippy --all-targets --all-features -D warnings,cargo deny(advisories, bans, licenses, sources all ok), offlineSQLX_OFFLINEbuild,cargo test --workspace.Not in this PR
The orphan Channels feature (backend complete, zero frontend) is deliberately left alone — whether the war room becomes the channel is a product decision that belongs with the frontend mechanics model, not a backend cleanup.