Fix IndexError in SOCKS proxy parser for malformed credentials #1273
Conversation
- Add safe parsing for username:password@host:port format - Add length checks before accessing list elements - Add default values (empty string for credentials, 127.0.0.1:1080 for host) - Fixes potential IndexError when parsing malformed proxy strings Relates to OWASP#1219
Summary by CodeRabbit
WalkthroughEnhanced SOCKS proxy parsing in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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: 2
🤖 Fix all issues with AI agents
In `@nettacker/core/socks_proxy.py`:
- Line 37: The port parsing currently uses int(host_parts[1]) which raises
ValueError for non-numeric ports; update both occurrences (the port assignment
that uses host_parts and the similar one at the second occurrence) to guard
conversion with a try/except ValueError (or strconv-safe parse) and fall back to
the default port 1080 when conversion fails, ensuring host_parts and port
variables are used unchanged otherwise and the code continues gracefully.
- Line 27: The parsing of proxy strings currently uses socks_proxy.split("@", 1)
which breaks when passwords contain '@'; update the split to use rsplit("@", 1)
so the last '@' is used as the delimiter (modify the line that assigns
auth_part, host_part from socks_proxy), and run/adjust any related tests or
callers that expect correct auth_part/host_part extraction.
🧹 Nitpick comments (2)
nettacker/core/socks_proxy.py (2)
31-31: Redundantlen(...) > 0guards —str.split()always returns at least one element.
auth_parts[0]andhost_parts[0]are always safe to access aftersplit()/rsplit(), so thelen(...) > 0checks on lines 31, 36, and 49 are dead branches. Not harmful, but slightly misleading.♻️ Simplified (line 31 example; same pattern for 36 and 49)
- socks_username = auth_parts[0] if len(auth_parts) > 0 else "" + socks_username = auth_parts[0]Also applies to: 36-36, 49-49
17-17: Missing docstring and type hints onset_socks_proxy.As per coding guidelines, "use type hints where practical, and add docstrings for public APIs." A brief docstring and a return type hint would improve discoverability and maintainability.
- Change split('@', 1) to rsplit('@', 1) to handle @ in passwords
- Add ValueError handling for non-numeric port values
- Remove redundant len() > 0 checks for split()[0]
- Add docstring for set_socks_proxy function
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@nettacker/core/socks_proxy.py`:
- Around line 18-28: The docstring for the SOCKS proxy configuration is
inaccurate—update the docstring in the function that configures SOCKS proxy (the
block that checks url.startswith('socks5://') and falls back to SOCKS4) to
mention both 'socks5://' and 'socks4://' formats, including authenticated
('socks5://username:password@host:port' or
'socks4://username:password@host:port') and unauthenticated
('socks5://host:port' or 'socks4://host:port') forms, and keep the returned
tuple description (socket_class, getaddrinfo_function) unchanged so it matches
the implementation.
🧹 Nitpick comments (2)
nettacker/core/socks_proxy.py (2)
64-73: Duplicate host:port parsing logic could be extracted into a helper.Lines 46–54 and 65–73 are nearly identical. A small helper like
_parse_host_port(host_part, default_port=1080)would reduce duplication and make future changes easier.♻️ Suggested refactor
+def _parse_host_port(host_part, default_port=1080): + """Parse host:port string, returning (hostname, port) with a safe default.""" + parts = host_part.rsplit(":", 1) + hostname = parts[0] + if len(parts) > 1: + try: + port = int(parts[1]) + except ValueError: + port = default_port + else: + port = default_port + return hostname, portThen in both branches:
- host_parts = host_part.rsplit(":", 1) - hostname = host_parts[0] - if len(host_parts) > 1: - try: - port = int(host_parts[1]) - except ValueError: - port = 1080 - else: - port = 1080 + hostname, port = _parse_host_port(host_part)
56-79: Redundantstr()wrapping onhostname.
hostnameis already astr(result ofstr.rsplit), so thestr(hostname)calls on lines 58 and 77 are unnecessary.
|
Closing as duplicate of #1214. I see that @moksha-hub's PR addresses the same SOCKS proxy issue. Will work on a different contribution instead. Thank you for the feedback! |
2 participants
Fixes #1219
Problem
The SOCKS proxy parser in
nettacker/core/socks_proxy.pythrowsIndexErrorwhen parsing malformed proxy strings that don't follow the expected formatusername:password@host:port.Solution
127.0.0.1:1080for host/port@to handle edge cases in credentialsChanges
set_socks_proxy()function innettacker/core/socks_proxy.pyTesting
Tested with various malformed proxy strings:
malformed_proxy(no separators)user@host(missing password and port)user:pass@host(missing port)All cases now handle gracefully without raising
IndexError.Type of change
Checklist
Fix IndexError in SOCKS proxy parser for malformed credentials