Repository navigation
fix: replace assert guards, add an exception message, and stop a password leak - #282
Conversation
|
Warning Review limit reachedNext included review available in 31 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Remove committed Python bytecode artifacts
Remove this file and the other two added .pyo files under test/__pycache__; they are generated CPython 3.12/pytest bytecode rather than test sources, are interpreter-specific, and embed the developer's absolute local workspace path. Keeping them tracked adds repository churn and leaks local build metadata without contributing runnable source.
AGENTS.md reference: AGENTS.md:L12-L12
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return "{}_{}_{}".format(self.server, self._username, self._password) | ||
| # Never include the password. This string reaches log lines, the repr of | ||
| # a server list, and any traceback that prints a Protocol object. | ||
| return "{}_{}".format(self.server, self._username) |
There was a problem hiding this comment.
For every DistributedClient, HashRing(self._servers) uses each Protocol object's string form as its node identity, so this changes every identity even without authentication (server_None_None becomes server_None). Upgrading a multi-server deployment therefore routes most existing keys to different servers, causing widespread cache misses and a potential backend load spike; keep the legacy hash identity internally while exposing a password-free representation.
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
|
Tick the box to add this pull request to the merge queue (same as
|
…word leak
Five defects. Each one is small and independent.
Defect 1: an assert guarded the wire protocol. The magic byte check at
bmemcached/protocol.py was the only check on a memcached response header.
"python -O" strips an assert, so the client then processed a corrupt stream
as valid data. The assert also raised a bare AssertionError, which the
surrounding "except socket.error" did not catch. The header was read. The
body was not. The socket stayed open in a desynchronized state, and the next
call on that thread read the leftover body as a header. The check now raises
a MemcachedException and calls self.disconnect() first.
Defect 2: two asserts guarded input validation. A zero CAS value now raises
ValueError. The wire protocol reads cas=0 as "no CAS check", so a stripped
assert turned a compare-and-swap into an unconditional overwrite. An empty
server list now raises ValueError at construction, in place of an unclear
failure later.
Defect 3: MemcachedException had no usable message. __init__ never called
super().__init__, and the class defined no __str__, so str(exc) returned the
args tuple repr. str(MemcachedException('boom', 1)) returned "('boom', 1)".
It now returns "boom (code 1)". AuthenticationNotSupported and
InvalidCredentials inherit the fix.
Defect 4: Protocol.__str__ printed the SASL password. The plaintext password
reached any log line, any repr of a server list, and any traceback that
printed a Protocol object. The password is out of the string.
Defect 5: a fragile circular import. bmemcached/client/distributed.py read
SOCKET_TIMEOUT back off the partly initialized package module. The import
worked only because of the line order in bmemcached/client/__init__.py. A
reorder raised ImportError at import time, and no test caught it. The module
now imports from bmemcached.client.constants, which is the form
bmemcached/client/mixin.py already used.
test/test_server_parsing.py expected AssertionError for an empty server
list. It now expects ValueError.
Seven new tests cover the four behavior changes. Two of them also pass under
"python -O", which is the case the asserts failed.
Refs #273
Protocol.__str__ no longer holds the password. DistributedClient passed Protocol objects to HashRing, and uhashring hashes str(node). The node identity changed from "server_username_password" to "server_username". About 65% of keys moved to a different server after the change, with or without authentication. DistributedClient now hashes Protocol._hash_ring_node. This property keeps the legacy identity. A dict maps each identity back to its Protocol object. The str() output still holds no password. A new test compares key placement with a ring built from the legacy identities. It checks 1000 keys with and without credentials. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Protocol.__str__ removed the password, but the string gave no sign of a password. A reader of a log line could not tell if authentication was on. The string now ends with "password=set" or "password=unset". The password value stays out of the string. The hash ring identity does not change. A new test checks the "password=unset" form. The existing test also checks the "password=set" form. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5ee0ff0 to
9a3f461
Compare
1 participant
What changed
Five defect fixes and seven new tests. One commit. Each fix is independent of
the others and of the Python 2 cleanup in #278.
assert. A bad magic byte ina response header now raises a
MemcachedException. The code callsself.disconnect()before it raises.assert. A CAS value of 0raises
ValueError. An empty server list raisesValueError.MemcachedExceptionhas a usable message.__init__callssuper().__init__, and the class defines__str__.Protocol.__str__no longer holds the password.bmemcached/client/distributed.pyimportsSOCKET_TIMEOUTfrombmemcached.client.constants.test/test_server_parsing.py::testNoServersSuppliedexpectedAssertionError.It now expects
ValueError.Why
python -Ostrips everyassert. Three of these guards were the only checkon their path, so an optimized interpreter silently dropped them.
The magic byte guard was the worst of the three. It raised a bare
AssertionError, which the surroundingexcept socket.errordid not catch.The header was read and the body was not, so the socket stayed open in a
desynchronized state and the next call on that thread read the leftover body
as a header. A malformed header from a proxy wedged the client instead of
disconnecting and retrying.
The CAS guard was the second. The wire protocol reads
cas=0as "no CAScheck", so a stripped assert turned a caller's compare-and-swap into an
unconditional overwrite.
Separately,
MemcachedException.__init__never calledsuper().__init__, andthe class defined no
__str__.str(MemcachedException('boom', 1))returned"('boom', 1)". Every caller and log line that calledstr(exc)got a tuplerepr rather than the message. And
Protocol.__str__put the plaintext SASLpassword into any log line, any repr of a server list, and any traceback that
printed a
Protocolobject.Verification
Result:
268 passed. The baseline onmainis261 passed. This pullrequest adds seven tests and changes no existing count.
Result: 0 errors.
The new tests, run alone:
Result:
7 passed.The
assertcases, under an optimized interpreter:Result:
2 passed. Both fail onmainunder-O, because the interpreterstrips the guard.
Direct check of the exception message:
Result:
'boom (code 1)'. Onmainthis returns"('boom', 1)".Risks
A reviewer must check three points.
cas(key, value, 0)raisedAssertionErrorand now raisesValueError. An empty server list did thesame. A caller that catches
AssertionErroraround either call needs anupdate. Neither was a documented contract, and
python -Oalready madeboth unreliable.
_get_responsepreviously let the
AssertionErrorescape uncaught. It now raises aMemcachedExceptionand drops the socket first. Callers that alreadyhandle
MemcachedExceptionneed no change. The one behavior difference areviewer should confirm is the
disconnect(): it is deliberate, becausethe body was never read and a reused socket would misread it as the next
header.
str(MemcachedException)changes format. It returns"boom (code 1)"in place of"('boom', 1)". Anything that parsed the oldtuple repr breaks.
exc.messageandexc.codeare unchanged.Protocol.__str__keeps the server and the username. Only the password isgone.
Closes #273