#36637 torcontrol: send -torpassword hex-encoded to avoid escaping bugs
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36637 torcontrol: send -torpassword hex-encoded to avoid escaping bugs | Ready | 1 (+1) | Strong | 3 + 39 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Sends the -torpassword configuration argument as a hex-encoded string in the Tor control port AUTHENTICATE command instead of a double-quoted string. It also adds a functional test verifying that passwords containing backslashes and quotes authenticate successfully against a mock Tor control server.
Problem: Tor control passwords containing backslashes fail authentication or are mangled. Bitcoin Core previously sent the password as a QuotedString escaping only double quotes, causing Tor's control port parser to misinterpret backslash sequences or reject strings as unterminated.
Evidence of importance:
- PR description: 'The Tor password was sent as a QuotedString with only double quotes escaped; as Tor interprets backslash escapes inside it, any password containing a backslash was mangled (or left the string unterminated). The Tor Control spec deliberately allows the token to be sent as unquoted hex "to avoid escaping issues"...'
Risk of a mistake: low (other, tests). A mistake in encoding the authentication command would cause Tor control port authentication to fail, preventing automatic Tor onion service configuration.
Changed since the previous assessment: None substantive; the approach and test coverage remain the same.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- author: address suggestion to update or remove the password-authentication example comment 2026-10-26
- author: address suggestion to check received hex in mock Tor server 2026-10-26
Agreement
Strong
substantive support, no open objection (winterrdog, vasild, winterrdog)
Thread notes: Recorded winterrdog's Tested ACK on Windows 10/MSVC (r:5665548329). Open suggestions from maflcko and winterrdog remain awaiting author response.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| vasild (member) | suggestion, style | use default empty list instead of None for extra_args | answered · author noted that mutable default argument would fail linter in CI | answered | yes | 2026-10-24: 'def restart_with_mock(self, mock_tor, extra_args=[]):' Status from: 2026-10-24: jpk68: 'Using `extra_args=[]` would cause this to fail `lint_py_lint` in CI, though, yes?' |
| maflcko (member) | suggestion, maintenance | update or remove the password-authentication example comment so it matches the wire format | open | answered | no | 2026-10-26: 'Update the example or remove it so the authentication summary matches the wire format.' |
| winterrdog (contributor) | suggestion, maintenance | have the mock Tor server verify the received hex and return 515 Bad authentication on mismatch | open | answered | no | 2026-10-26: 'the mock could explicitly check the received hex and reply with a `515 Bad authentication` if it does not match, rather than accepting any `AUTHENTICATE`' |
Support:
- winterrdog (contributor) (Concept ACK, 2026-10-24) [read]: hex-encoding removes the need for escaping entirely, is explicitly allowed by Tor spec, and is preferred over #36374
- vasild (member) (ACK, 2026-10-24) [read]: verified against the Tor control spec documentation
- winterrdog (contributor) (Tested ACK, 2026-10-27) [tested]: successfully built and tested on Windows 10 / MSVC 19.51.36257 / x86_64
Participants:
- vasild (member, support, 2 statements 2026-10-24 to 2026-10-24): ACK referencing Tor spec; suggested nit on test helper default argument
- winterrdog (contributor, support, 3 statements 2026-10-24 to 2026-10-27): Tested ACK; Concept ACK; suggested verifying password in test mock
- maflcko (member, neutral, 1 statements 2026-10-26 to 2026-10-26): suggested updating or removing the password-authentication example comment
Files
41 lines under test/bench/ci.
- test/functional/feature_torcontrol.py +39/-2
- src/torcontrol.cpp +3/-4
Card
The short text the ranking pass compares across PRs.
Encodes -torpassword as unquoted hexadecimal when authenticating to the Tor control port instead of escaping quoted strings. Currently, passwords containing backslashes fail authentication or get mangled because Bitcoin Core only escapes double quotes while Tor parses backslash escapes. The Tor control specification explicitly permits hex-encoded tokens to avoid escaping issues, and this PR adds a functional test covering backslashes and quotes.
Data
thread read through 2026-10-27 (7 statements), head a1903e3abe
labels: none
Categories
P2P
P3 · bug fix (ranking pass: Same Tor-auth bug as #36374 and the approach the control spec endorses, so it leads that pair.)
Fixes authentication failures for Tor control passwords containing backslashes. As noted in the PR description, sending passwords as quoted strings caused Tor to misinterpret backslash escapes or reject them as unterminated, which hex-encoding resolves per the Tor specification.
Membership: Changes src/torcontrol.cpp to hex-encode Tor control passwords, falling squarely within P2P's Tor integration scope.
Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 0