feat: add passive DCC support with configurable listen IP and port range#25
Merged
Conversation
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 32 minutes and 22 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughWhen a remote client sends DCC SEND with port 0, the bot can now act as passive receiver: it starts a listening socket, replies with its numeric IP and chosen port via CTCP, and accepts the incoming TCP connection to receive the transfer. ChangesPassive DCC Implementation
Sequence DiagramsequenceDiagram
participant Sender as DCC Sender
participant Bot as IRC Bot
participant Listener as Passive Listener
Sender->>Bot: DCC SEND filename (port=0)
Bot->>Bot: _get_passive_dcc_config()
Bot->>Listener: start listening (bind addr/port range)
Listener-->>Bot: listening on resolved address/port
Bot->>Sender: CTCP SEND reply (our address/port)
Sender->>Listener: connect to listener
Listener->>Bot: dcc_connect event (new transport)
Bot->>Bot: create transfer record
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dccbot/aiodcc.py`:
- Around line 273-275: Run the project's formatter (ruff format) on this file to
fix style issues; specifically reformat the method that mutates text when
self.dcctype == 'chat' and returns self.send_bytes(self.encode(text)) so
whitespace and line breaks match project style (the block using self.dcctype,
text += '\n', and return self.send_bytes(self.encode(text)) and the similar
block earlier around send_bytes/encode should be reformatted). Ensure you run
ruff format and re-check CI before pushing.
- Line 25: The code currently unpacks transport.get_extra_info('peername')
directly into two variables (self.connection.peeraddress,
self.connection.peerport) which breaks on IPv6 (peername can be 4-tuple) and may
be None; similarly for self.localaddress, self.localport =
socket_obj.getsockname(). Change both places to safely handle None and
variable-length tuples by reading the address as peername[0] and port as
peername[1] (only after verifying peername is a sequence with length >=2),
falling back to None/defaults if peername is None or too short; update the
assignments around transport.get_extra_info('peername') and
socket_obj.getsockname() accordingly (use safe indexing and presence checks
instead of direct 2-value unpacking).
In `@dccbot/ircbot.py`:
- Around line 606-614: The passive-DCC branch currently returns early and thus
skips the filename/path and size validations; modify the branch in ircbot.py so
that before calling init_passive_dcc_connection you run the same validation
logic used for active DCC offers (reuse the existing filename/path validation
and max/min size checks), and only proceed to call
init_passive_dcc_connection(nick, filename, size, listen_ip, port_range) if
those validations pass; keep the SSL warning via use_ssl but do not return
before validation and ensure that _get_passive_dcc_config and
init_passive_dcc_connection are invoked only after successful validation.
In `@README.md`:
- Around line 71-73: Rewrite the three README bullets for passive_dcc,
passive_dcc_listen_ip, and passive_dcc_port_range as full sentences (not
fragments): e.g. "passive_dcc: A boolean indicating whether to accept passive
DCC transfers (where the peer sends port=0 and the bot listens for an incoming
connection). This option can also be set per-server." and similarly convert the
other two entries so each ends with a period and uses "This option can also be
set per-server." to replace the fragment "Can also be set per-server."
In `@tests/unit/test_ircbot.py`:
- Around line 784-840: Add two new unit tests mirroring
test_on_dcc_send_passive_enabled but asserting init_passive_dcc_connection is
NOT called for invalid filename and zero/oversized size cases: create bot with
mock_bot_manager.config = {"passive_dcc": True}, set event.arguments to a SEND
line that uses an invalid filename (e.g., 'SEND "" 0 0 1000' or a path-traversal
name) and another with a size of 0 or an excessively large size (e.g., 'SEND
"test.txt" 0 0 0' and 'SEND "test.txt" 0 0 9999999999'), patch
bot.init_passive_dcc_connection and call bot.on_dcc_send(bot.connection, event,
False), then assert mock_init.assert_not_called() (and optionally assert
bot.connection.ctcp_reply was invoked with an error reply) to ensure rejection
happens before init_passive_dcc_connection is invoked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca68b84a-499d-4660-a251-c654af13d0c4
📒 Files selected for processing (7)
README.mdconfig.json.sampledccbot/aiodcc.pydccbot/ircbot.pytests/integration/test_irc_connection.pytests/unit/test_aiodcc.pytests/unit/test_ircbot.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dccbot/ircbot.py (1)
854-866:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRuntime guard is unreachable; move check before
ip_quad_to_numstr.The
RuntimeErrorguard at Lines 865-866 comes afterdcc.localaddressis already used at Line 854. IflocaladdressisNone,ip_quad_to_numstrwill fail first with a less informative error.🛡️ Proposed fix — move check before usage
+ if dcc.localaddress is None or dcc.localport is None: + raise RuntimeError("Passive DCC listen succeeded but localaddress/localport not set") + ip_numstr = irc.client.ip_quad_to_numstr(dcc.localaddress) self.connection.ctcp_reply( nick, " ".join(["DCC", "SEND", '"' + filename.replace('"', "") + '"', str(ip_numstr), str(dcc.localport), str(size)]), ) logger.info("[%s] Passive DCC listening on %s:%d for %s", nick, dcc.localaddress, dcc.localport, filename) local_download_path = os.path.join(self.download_path, filename) if self.config.get("incomplete_suffix"): local_download_path += self.config["incomplete_suffix"] - if dcc.localaddress is None or dcc.localport is None: - raise RuntimeError("Passive DCC listen succeeded but localaddress/localport not set") - now = time.time()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dccbot/ircbot.py` around lines 854 - 866, The guard that raises RuntimeError if dcc.localaddress or dcc.localport is unset must be moved before any use of dcc.localaddress; update the code in ircbot.py so you first check "if dcc.localaddress is None or dcc.localport is None: raise RuntimeError(...)" before calling irc.client.ip_quad_to_numstr(dcc.localaddress) and before using dcc.localport in the CTCP/DCC reply and logger.info; ensure ip_quad_to_numstr, self.connection.ctcp_reply and logger.info remain unchanged except for being after the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dccbot/aiodcc.py`:
- Around line 25-35: The debug log uses a %d format for self.connection.peerport
which can be None after the IPv6-safe indexing change; update the logging call
in the block that handles transport.get_extra_info("peername") so it won’t raise
TypeError—e.g. format both address and port with %s (or coerce peerport to a
safe value) when calling log.debug, and ensure the change is applied where
self.connection.peeraddress and self.connection.peerport are set and before the
irc.client.Event invocation.
- Around line 233-234: disconnect() can raise AttributeError if self.peeraddress
isn't set; change the call that builds the dcc_disconnect event to use a safe
lookup (e.g. peer = getattr(self, "peeraddress", "") or check hasattr(self,
"peeraddress") before using it) and pass that safe value into
reactor._handle_event(self, irc.client.Event("dcc_disconnect", peer, "",
[message])); leave the subsequent reactor._remove_connection(self) as-is.
---
Outside diff comments:
In `@dccbot/ircbot.py`:
- Around line 854-866: The guard that raises RuntimeError if dcc.localaddress or
dcc.localport is unset must be moved before any use of dcc.localaddress; update
the code in ircbot.py so you first check "if dcc.localaddress is None or
dcc.localport is None: raise RuntimeError(...)" before calling
irc.client.ip_quad_to_numstr(dcc.localaddress) and before using dcc.localport in
the CTCP/DCC reply and logger.info; ensure ip_quad_to_numstr,
self.connection.ctcp_reply and logger.info remain unchanged except for being
after the guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e73b1359-b7a2-4f7b-ad90-c45b636d7b05
📒 Files selected for processing (4)
README.mddccbot/aiodcc.pydccbot/ircbot.pytests/unit/test_ircbot.py
✅ Files skipped from review due to trivial changes (1)
- README.md
Comment on lines
+25
to
+35
| peername = transport.get_extra_info("peername") | ||
| if peername and len(peername) >= 2: | ||
| self.connection.peeraddress, self.connection.peerport = peername[0], peername[1] | ||
| else: | ||
| self.connection.peeraddress = None | ||
| self.connection.peerport = None | ||
| log.debug("DCC connection from %s:%d", self.connection.peeraddress, self.connection.peerport) | ||
| self.connection.reactor._handle_event(self.connection, irc.client.Event("dcc_connect", self.connection.peeraddress, None, None)) | ||
| if hasattr(self.connection, "server") and self.connection.server: | ||
| self.connection.server.close() | ||
| return |
There was a problem hiding this comment.
Potential TypeError when logging with None values.
After the safe indexing fix for IPv6, peeraddress and peerport can now be None. Line 31 uses %d format specifier for peerport, which will raise TypeError if peerport is None.
🐛 Proposed fix
- log.debug("DCC connection from %s:%d", self.connection.peeraddress, self.connection.peerport)
+ log.debug("DCC connection from %s:%s", self.connection.peeraddress, self.connection.peerport)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dccbot/aiodcc.py` around lines 25 - 35, The debug log uses a %d format for
self.connection.peerport which can be None after the IPv6-safe indexing change;
update the logging call in the block that handles
transport.get_extra_info("peername") so it won’t raise TypeError—e.g. format
both address and port with %s (or coerce peerport to a safe value) when calling
log.debug, and ensure the change is applied where self.connection.peeraddress
and self.connection.peerport are set and before the irc.client.Event invocation.
Comment on lines
233
to
234
| self.reactor._handle_event(self, irc.client.Event("dcc_disconnect", self.peeraddress, "", [message])) | ||
| self.reactor._remove_connection(self) |
There was a problem hiding this comment.
AttributeError risk when peeraddress is not set.
If disconnect() is called on a passive listener before any peer connects, self.peeraddress may not be defined, causing an AttributeError at Line 233.
🛡️ Proposed fix
- self.reactor._handle_event(self, irc.client.Event("dcc_disconnect", self.peeraddress, "", [message]))
+ self.reactor._handle_event(self, irc.client.Event("dcc_disconnect", getattr(self, "peeraddress", None), "", [message]))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| self.reactor._handle_event(self, irc.client.Event("dcc_disconnect", self.peeraddress, "", [message])) | |
| self.reactor._remove_connection(self) | |
| self.reactor._handle_event(self, irc.client.Event("dcc_disconnect", getattr(self, "peeraddress", None), "", [message])) | |
| self.reactor._remove_connection(self) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dccbot/aiodcc.py` around lines 233 - 234, disconnect() can raise
AttributeError if self.peeraddress isn't set; change the call that builds the
dcc_disconnect event to use a safe lookup (e.g. peer = getattr(self,
"peeraddress", "") or check hasattr(self, "peeraddress") before using it) and
pass that safe value into reactor._handle_event(self,
irc.client.Event("dcc_disconnect", peer, "", [message])); leave the subsequent
reactor._remove_connection(self) as-is.
luni
force-pushed
the
upd/extend-dcc-support
branch
from
3a6c49f to
fae3662
Compare
This PR upgrades our local aiodcc.py with the latest upstream asyncio DCC changes (commits up to fd18778a in python-irc fork) and adds full passive DCC support to the bot. Core changes: - aiodcc.py: add DCCProtocol.connection_made() for passive connections, implement listen() with configurable addr/port/ipv6, update disconnect() to close server and guard transport, add privmsg() for DCC CHAT, remove passive NotImplementedError from process_data(), and improve send_bytes() error handling (AttributeError catch + data param rename) - ircbot.py: add _get_passive_dcc_config() with global/per-server fallback chain, modify on_dcc_send() to support port=0 (passive DCC), add init_passive_dcc_connection() that listens and sends CTCP reply - README.md + config.json.sample: document passive_dcc, passive_dcc_listen_ip, passive_dcc_port_range options Tests: - unit/test_aiodcc.py: add tests for listen, listen_and_accept, disconnect_with_server, privmsg, send_bytes_not_connected - unit/test_ircbot.py: add tests for _get_passive_dcc_config, on_dcc_send passive enabled/disabled, init_passive_dcc_connection, on_dcc_connect - integration/test_irc_connection.py: add test_passive_dcc_listen_setup that connects to real InspIRCd, simulates passive DCC SEND, verifies CTCP reply and socket connectivity Config hierarchy (per-server overrides global): passive_dcc (bool) - enable passive DCC transfers passive_dcc_listen_ip (str) - IP to bind listener passive_dcc_port_range [min, max] - port range to try
…CCConnection overrides The upstream irc library's DCCConnection base class has signatures that are incompatible with our asyncio overrides: - listen() returns Self (DCCConnection) in base, but we need async - send_bytes() uses parameter name "bytes" in base, but we use "data" Add targeted # type: ignore[reportIncompatibleMethodOverride] comments since fixing this properly requires upstream changes.
1. aiodcc.py: safely handle IPv6 peername (4-tuple) and None from
transport.get_extra_info('peername') and socket.getsockname()
by checking length before indexing instead of direct unpacking.
2. ircbot.py: move size and filename validation before the passive
DCC branch so it applies to both active and passive transfers.
Replace assert with RuntimeError to satisfy bandit.
3. README.md: rewrite passive DCC config bullets as full sentences
with proper punctuation.
4. tests/unit/test_ircbot.py: add test_on_dcc_send_passive_enabled_invalid_
filename and test_on_dcc_send_passive_enabled_invalid_size to ensure
passive DCC is rejected for invalid filenames and sizes before
init_passive_dcc_connection is called.
luni
force-pushed
the
upd/extend-dcc-support
branch
from
fae3662 to
c9c3b3a
Compare
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
1 participant
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.
This PR upgrades our local aiodcc.py with the latest upstream asyncio DCC changes (commits up to fd18778a in the python-irc fork) and adds full passive DCC support to the bot.
Core Changes
dccbot/aiodcc.py— upstream syncDCCProtocol.connection_made()— handles passive DCC incoming connectionsAioDCCConnection.listen()— full async implementation with configurableaddr,port(int / tuple range / list), andipv6AioDCCConnection.disconnect()— safely closes listening server and guardstransport.close()AioDCCConnection.privmsg()— new method for DCC CHAT sessionsAioDCCConnection.send_bytes()— improved error handling (catchesAttributeErrorfor unconnected state), renamed param frombytestodataNotImplementedErrorfromprocess_data()dccbot/ircbot.py— passive DCC support_get_passive_dcc_config()— reads config with per-server → global → default fallback chainon_dcc_send()— whenpeer_port == 0, checks if passive DCC is enabled and either rejects or delegates toinit_passive_dcc_connection()init_passive_dcc_connection()— creates a listener, sends a CTCP reply with the bot's IP/port, and registers the transferon_dcc_connect()— handles thedcc_connecteventConfiguration
Three new keys (per-server overrides global):
passive_dccpassive_dcc_listen_ippassive_dcc_port_range[min, max]See
config.json.samplefor examples.Tests
listen,listen_and_accept,disconnect_with_server,privmsg,send_bytes_not_connected_get_passive_dcc_config,on_dcc_sendpassive enabled/disabled,init_passive_dcc_connection,on_dcc_connecttest_passive_dcc_listen_setup— connects to real InspIRCd, simulates passive DCC SEND, verifies CTCP reply and actual socket connectivityLocal Verification
88 unit tests pass.
Summary by CodeRabbit
New Features
Documentation
Tests