Ban users whose sessions include private addresses - #305
namespaceMarcello wants to merge 2 commits into
Conversation
Banning creates a Ban for each session address with create!, and Ban refuses private, loopback and link-local addresses. One such session rolled back the whole ban, so on a Campfire reached over a LAN or a VPN nobody could be banned: the user stayed active with their sessions and messages, and the administrator got an error. Skip the addresses Ban won't store and ban the user anyway. The bans are kept out of the association, where a refused one would fail saving the user. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVFo3Lt9T8M5NR7KxVvsZ2
There was a problem hiding this comment.
Copilot review overview
🟢 Approved
The focused fix preserves address validation and includes regression coverage, with no unresolved findings.
Review effort: Balanced
Findings: None
What changed in this PR
Allows users with private or internal session addresses to be banned without weakening IP-address validation.
Changes:
- Creates valid IP bans outside the user’s association, avoiding validation failures when saving the user.
- Adds regression coverage for mixed public and private session addresses.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
| test/controllers/users/bans_controller_test.rb | Verifies mixed-address bans succeed and clear sessions. |
| app/models/user/bannable.rb | Skips rejected IP bans without blocking the user ban. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
From namespaceMarcello:ban-users-with-private-sessions (upstream PR state at merge: OPEN).
| def create_bans_from_sessions | ||
| sessions.pluck(:ip_address).compact_blank.uniq.each do |ip| | ||
| bans.create!(ip_address: ip) | ||
| Ban.create(user: self, ip_address: ip) |
There was a problem hiding this comment.
Won't this always scope the ban to the user.
So if e.g. I ban you, and you just go and create a new account, that wouldn't then be banned?
The point of this ban system is to prevent easy creation of sock puppets.
There was a problem hiding this comment.
The ban stays on the address. Ban.create(user: self, ip_address: ip) stores the same row that bans.create!(ip_address: ip) did: user_id only records whose ban it is, so that unban can delete it with bans.delete_all. The check never looks at the user. BlockBannedRequests runs in ApplicationController and answers 429 to every non-GET request whose request.remote_ip has a Ban (Ban.banned? is exists?(ip_address: ip_address)), whichever account sends it. BlockBannedRequestsTest builds its ban the same way, Ban.create!(user: users(:kevin), ip_address: "203.0.113.1"), and checks that David's POST from that address gets the 429.
This PR only changes which addresses get a row. On main, a private address among the user's sessions makes bans.create! raise and the transaction roll back, so nothing is banned: the user stays active, and a new account from their public address isn't blocked either. With this change the public addresses are banned and the private ones are skipped. Ban already refuses private addresses, so they couldn't be blocked before this change either.
In 85dac81 the test checks it too: after the ban, a POST from the banned public address by another signed-in account gets a 429. Scoping Ban.banned? to the current user makes it fail (200 instead of 429).
Written by Claude Code (AI assistant) on behalf of @namespaceMarcello, who directs this work.
The bans are created apart from the association, but they block the address, not the user: a POST from the banned public address by another signed-in account gets a 429. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0142qgjggdJ2KDdGk7RF9Xm9
Banning a user creates a
Banfor each IP address their sessions came from.Banrefuses private, loopback and link-local addresses, and the bans are created withcreate!inside the ban transaction. So when any one session came from such an address, the whole ban rolls back. The user stays active, keeps their sessions, and their messages stay up. The administrator gets an error page.That happens to every user on a Campfire reached over a LAN or a VPN, where all addresses are private, and to anyone who has ever signed in from the server itself. In the test environment it is the address of every request, so a user who signed in through an integration test can't be banned:
This keeps
Ban's rule and creates bans only for the addresses it accepts, so the user is banned either way. A private address wasn't going to be blocked anyway, sinceBanwon't store it.The bans are created apart from the
bansassociation. A refused ban left in the association would make saving the user fail when it is marked banned (Validation failed: Bans is invalid).Tests
test/controllers/users/bans_controller_test.rbgets a test for a user with one session from a public address and one from192.168.1.20. It checks that the ban goes through: oneBan, for the public address, the user banned and their sessions gone. It also checks that the ban blocks the address and not only the user: afterwards a POST from the banned public address by another signed-in account gets a 429. On the old code it fails with theRecordInvalidabove. Creating the bans through the association fails it withBans is invalid, and skipping every address fails the count of bans, and scopingBan.banned?to the current user fails the 429.Validation in the container:
bin/rubocop,bin/brakeman,bin/rails testandbin/rails test:systemall pass with no failures.Written by Claude Code (AI assistant) on behalf of @namespaceMarcello, who directs this work.
🤖 Generated with Claude Code