fix: don't KeyError when a connection_id is reused before cleanup runs - #8376
Open
citizen204 wants to merge 1 commit into
Open
fix: don't KeyError when a connection_id is reused before cleanup runs#8376citizen204 wants to merge 1 commit into
citizen204 wants to merge 1 commit into
Conversation
Proxyserver.register_connection() unconditionally did `del self.connections[connection_id]` in its finally block. If the same connection_id (e.g. a client address/port 4-tuple, per the ServerManager docstring's own note that this is a 'temporary workaround' for connections without a uuid) gets reused and re-registered before the earlier handler's context manager has unwound, the newer registration overwrites the dict entry, and when the newer handler's own cleanup deletes it, the older handler's later cleanup hits a KeyError on an entry that's already gone. Fixes mitmproxy#6405 ## Changes - `mitmproxy/addons/proxyserver.py`: only delete the dict entry in the finally block if it still points at this handler. - `test/mitmproxy/addons/test_proxyserver.py`: regression test simulating two overlapping registrations under the same connection_id.
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.
Summary
Proxyserver.register_connection()unconditionally doesdel self.connections[connection_id]in itsfinallyblock.connection_idis a client address/port 4-tuple for connections that don't have a uuid
(per
ServerManager's own docstring: "temporary workaround"), so it can bereused before an earlier handler's context manager has unwound — e.g. a
client reconnecting quickly on the same source port. When that happens, the
newer registration overwrites the dict entry, the newer handler's cleanup
deletes it, and the older handler's later cleanup then hits
KeyErroronan entry that's already gone:
Fixes #6405
Changes
mitmproxy/addons/proxyserver.py: only delete the dict entry in thefinallyblock if it still points at this handler (i.e. hasn't beenoverwritten by a newer registration under the same id).
test/mitmproxy/addons/test_proxyserver.py: regression test thatsimulates two overlapping
register_connection()calls under the sameconnection_idand asserts neither raises (verified fails on currentmainwith the exact reportedKeyError, passes with the fix).🤖 Generated with Claude Code