improved detection - #1
Conversation
📝 WalkthroughWalkthroughThis pull request introduces HTTP/2 smuggling detection support and a variant capability matrix system. Changes include new scan profiles and confidence modes, transport-specific executors, enhanced reporting of untested variants and skipped checks, and comprehensive documentation updates reflecting variant maturity status. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Engine
participant VariantRegistry as Variant<br/>Registry
participant Detector as Timing/<br/>Differential
participant Executor
participant Network as Network<br/>Client
Client->>Engine: scan(target, variants)
Engine->>VariantRegistry: classify_requested_variants(requested)
VariantRegistry-->>Engine: (enabled, not_tested)
Engine->>Engine: _is_variant_applicable(variant)
alt Variant Applicable
Engine->>Engine: _get_or_create_baseline(endpoint)
Engine->>Detector: measure_baseline(transport)
Detector->>Executor: get_executor(transport)
Executor->>Network: send_simple_request(baseline)
Network-->>Executor: response_time
Executor-->>Detector: BaselineResult
Detector-->>Engine: baseline
Engine->>Detector: detect(payload, baseline)
Detector->>Executor: send_payload(payload)
Executor->>Network: send HTTP/1 or HTTP/2
Network-->>Executor: response
Executor-->>Detector: RawResponse
Detector->>Detector: analyze_responses()
Detector-->>Engine: DetectionResult
Engine->>Engine: _confirm_timing_payload()
Engine-->>Client: Vulnerability(status)
else Not Applicable
Engine->>Engine: _record_skip(endpoint, variant)
Engine-->>Engine: add to not_tested/skipped
end
sequenceDiagram
participant Engine
participant Registry as Variant<br/>Registry
participant Detector as HTTP/2<br/>Differential
participant Http2Client as HTTP/2<br/>Client
Engine->>Registry: get_capability(H2_CL)
Registry-->>Engine: VariantCapability(status, transport)
alt transport == "http2"
Engine->>Detector: detect(payload.transport="http2")
Detector->>Detector: _detect_http2()
Detector->>Http2Client: baseline_request()
Http2Client-->>Detector: h2_response
Detector->>Http2Client: smuggle_request()
Http2Client-->>Detector: h2_response
Detector->>Http2Client: victim_request()
Http2Client-->>Detector: h2_response
Detector->>Detector: _h2_to_raw(responses)
Detector->>Detector: _analyze_responses()
Detector-->>Engine: DetectionResult(http2)
else transport == "http1"
Engine->>Detector: detect(payload.transport="http1")
Detector-->>Engine: DetectionResult(http1)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/TECHNICAL_REFERENCE.md (2)
320-401:⚠️ Potential issue | 🟡 MinorArchitecture tree is stale — missing new modules.
The tree doesn't list
core/variant_registry.pyornetwork/executor.py, both introduced in this PR. Consider updating to avoid confusion for contributors.
409-436:⚠️ Potential issue | 🟡 MinorImplementation plan task statuses out of date.
Tasks 13-16 (H2.CL, H2.TE, H2.CRLF, WS version smuggling) and surrounding items are shown as
🔲 Pendingbut are being delivered in this PR. Updating these statuses would keep the roadmap accurate.http_smuggler/payloads/http2/crlf_injection.py (1)
348-361:⚠️ Potential issue | 🟡 Minor
"\r\n"and"\x0d\x0a"are identical in Python.In Python,
"\r\n"is"\x0d\x0a"— they produce the same bytes. This means the list has a duplicate entry rather than a distinct variant. If the intent was to test a literal backslash-escaped string (e.g., for header-value injection), use a raw string or double-escape.Proposed fix
return [ "\r\n", # Standard CRLF "\n", # LF only "\r", # CR only "\r\n ", # CRLF + space (folding) "\r\n\t", # CRLF + tab (folding) - "\x0d\x0a", # Explicit bytes + "\x00\r\n", # Null byte + CRLF ]http_smuggler/detection/timing.py (1)
411-423:⚠️ Potential issue | 🟠 Major
detect_batchmeasures a single HTTP/1 baseline and reuses it for all payloads, including HTTP/2.
measure_baselineis called without atransportargument (defaults to"http1"), so the resulting timing baseline reflects HTTP/1.1 round-trip characteristics. This baseline is then passed todetect()for every payload — including HTTP/2 ones that may have materially different latency profiles. The mismatch can produce false positives (if HTTP/2 is naturally slower) or false negatives (if HTTP/2 is faster).Consider grouping payloads by transport and measuring a baseline per group, or omitting the shared baseline so
detect()measures an appropriate one per payload.Sketch: per-transport baseline
- # Measure baseline once - baseline = await self.measure_baseline(host, port, use_ssl) + # Measure baseline per transport + baselines: dict[str, BaselineResult] = {} results = [] for payload in timing_payloads: + transport = getattr(payload, "transport", "http1") + if transport not in baselines: + baselines[transport] = await self.measure_baseline( + host, port, use_ssl, transport=transport, + ) result = await self.detect( payload, host, port, use_ssl, - baseline, + baselines[transport], )
🤖 Fix all issues with AI agents
In `@docs/VARIANTS.md`:
- Around line 35-37: The variant table is inconsistent: `H2.Tunnel` appears in
VARIANTS.md but is missing from the HTTP/2 variant tables in
TECHNICAL_REFERENCE.md and README.md; either add `H2.Tunnel` (with the same
columns as `H2.0`/`h2c`: human name, protocol `http2`, status `planned`) to
those other tables, or in VARIANTS.md add a short clarifying note explaining why
`H2.Tunnel` is intentionally excluded from TECHNICAL_REFERENCE and README (e.g.,
experimental/registry-only) so all docs are aligned; update any cross-references
or TOC entries that list HTTP/2 variants to include the change.
In `@http_smuggler/analysis/reporter.py`:
- Line 254: Remove the unnecessary f-string prefixes on static strings added to
the lines list (e.g., the calls to lines.append(f"✅ **Exploitation
Successful**") and the similar lines.append(f"...") around line 259) — change
them to plain string literals (lines.append("✅ **Exploitation Successful**")) in
reporter.py so there is no unused f-string interpolation; update any other
lines.append calls in the same function that use f"" without {…} to simple "" as
well.
In `@http_smuggler/core/engine.py`:
- Around line 574-579: Replace the assert guard with an explicit runtime check:
after the loop that sets best, change "assert best is not None" to an if block
that handles the None case (e.g., raise a clear RuntimeError or return a
sensible default) so the code is robust when running under -O or after future
refactors; ensure you still compute required = 1 if attempts == 1 else (attempts
// 2) + 1 and set best.vulnerable = positives >= required only when best is not
None, referencing the variables best, attempts, positives and required in the
updated logic.
In `@http_smuggler/detection/timing.py`:
- Around line 150-175: The baseline HTTP/2 branch instantiates Http2Executor and
calls send_simple_request without checking use_ssl, which can attempt a TLS
handshake on non-SSL targets; update the baseline loop to mirror send_payload's
behavior by only using Http2Executor/send_simple_request when use_ssl is True
(or delegate to the shared get_executor factory used in detect()), and otherwise
fall back to the AsyncRawHttpClient path; locate the baseline code in timing.py
(the for loop where Http2Executor is created and response.elapsed is read) and
change it to obtain the executor via get_executor(...) or guard the
Http2Executor branch with if use_ssl: so non-SSL targets don’t attempt TLS
handshakes. Ensure response handling and the existing exception handling remain
consistent.
In `@http_smuggler/main.py`:
- Around line 321-323: The current line unconditionally enables exploiting in
aggressive mode and overrides an explicit --no-exploit; change the logic so an
explicit exploit flag wins: set ExploitConfig(enabled = exploit if exploit is
not None else (scan_mode == ScanMode.AGGRESSIVE)), referencing ExploitConfig,
the enabled field, the exploit variable and scan_mode/ScanMode.AGGRESSIVE;
alternatively, if you prefer Option B, update the CLI/help to state that
aggressive mode forces exploitation and auto-set the exploit flag when parsing
the mode instead of silently overriding user input.
- Around line 465-466: Replace the unnecessary defensive getattr calls with
direct attribute access: change uses of getattr(result, "not_tested", []) and
getattr(result, "skipped", []) to result.not_tested and result.skipped
respectively (the table.add_row calls that add "Not Tested Variants" and
"Skipped Checks"). These fields are defined on the ScanResult dataclass
(not_tested and skipped), so access them directly for clarity and type safety.
In `@README.md`:
- Around line 204-206: The README table uses the non-canonical casing
"WS.VERSION"; update that table cell to the canonical "WS.Version" to match
VARIANTS.md and TECHNICAL_REFERENCE.md, and search for any other occurrences of
"WS.VERSION" and replace them with "WS.Version" (ensure references like the
table row containing "WS.VERSION | Fake 101 Server (port 9999) | WebSocket SSRF"
are updated so the variant name matches the registry).
🧹 Nitpick comments (24)
http_smuggler/network/__init__.py (1)
17-23: New executor exports look correct and consistent.Imports and
__all__entries match. One minor organizational nit:inject_http1_contextandHttp1Executorare grouped under the# HTTP/2comment block (Line 42), which is slightly misleading since they are HTTP/1-related. Consider adding a# Transport executorscomment section to separate them from the HTTP/2 protocol primitives above.Also applies to: 48-52
http_smuggler/core/models.py (1)
79-79: Consider documenting thestatusfield contract more formally.The allowed values (
confirmed|likely|requires_victim|not_confirmed) are only documented in a comment. If more statuses are added later, a string literal type or enum would prevent silent misuse. Low priority — the exploit runner centralizes these as class constants already.http_smuggler/network/executor.py (2)
145-160: Missing headers fallback returns a bareRawResponsewithout diagnostic context.Line 150-151: when
headersis falsy, the returnedRawResponsehas no indication of why the response is empty (i.e., "payload has no HTTP/2 headers to send"). This could make debugging difficult if a payload is misconfigured. Consider adding a log message or settingconnection_closed=Trueto distinguish from a real empty response.
219-226: Factory only handles"http2"—"websocket"/"browser"transports fall through toHttp1Executor.This is fine for the current scope since WebSocket and Browser transports are EXPERIMENTAL/PLANNED, but callers passing
transport="websocket"will silently get an HTTP/1 executor. If you'd like fail-fast behavior for unsupported transports, a warning log or a dedicated check could be added later.http_smuggler/core/variant_registry.py (3)
44-193: Registry covers all 16SmugglingVariantmembers — well organized.The capability metadata is thorough. One observation: PLANNED variants (e.g.,
CL_CL,CL_0) haveexploit_support=Trueand populateddetectorstuples. If these describe intended capabilities (when implemented), that's fine, but it could mislead callers who checkexploit_supportwithout also checkingstatus. Worth a docstring clarification onVariantCapability.exploit_support.
236-245: Redundant explicit aliases for "pause" and "csd".Lines 243-244 manually add
"pause"and"csd"aliases, but the loop at Lines 240-241 already adds lowercase versions of every variant value ("Pause".lower()→"pause","CSD".lower()→"csd"). The explicit lines are harmless and arguably serve as documentation of intent, so this is just a nit.
204-206: No immediate issue, but consider adding validation to prevent registry mismatches ifSmugglingVariantis extended in the future.All 16
SmugglingVariantmembers are currently registered inVARIANT_CAPABILITIES, soget_capabilityis safe today. However, there's no test or startup validation to catch the case if a new enum member is added tomodels.pywithout being added to the registry—it would silently raiseKeyErrorat runtime.Consider either:
- Test-based approach (preferred): Add a test that validates all
SmugglingVariantmembers have entries in the registry.- Error messaging: Improve the error message as suggested in the original comment.
A test is preferable because it catches the issue at test time rather than runtime.
http_smuggler/exploits/exploit_runner.py (1)
148-154: Accessing private_active_serversdict bypassesAutoListenerManagerencapsulation.Lines 152–154 and 346 directly access
self._listener_manager._active_serversto retrieve the server object and callget_loot(). This couples the exploit runner to the internal storage mechanism. Instead, add a public method toAutoListenerManagersuch asget_server(listener_type: str) -> Optional[CallbackServer]to safely expose server access, or add a delegating methodget_loot(listener_type: str = "loot")that calls the underlying server'sget_loot().http_smuggler/core/config.py (2)
4-4: Unused importAny.
Anyis imported but never referenced in this file.🧹 Proposed fix
-from typing import Optional, List, Set, Dict, Any +from typing import Optional, List, Set, Dict
163-167:enabled_variantsdefault now driven by registry;not_tested_variantsuses untyped dicts.The default factory change to
get_enabled_variants_for_scancleanly decouples variant selection from the config. However,not_tested_variantsis typed asList[Dict[str, str]]— the entries have an implicit schema (variant,reason,statuskeys) with no enforcement. Consider a small dataclass orTypedDictto prevent key mismatches.tests/test_reporting.py (1)
11-11:datetime.utcnow()is deprecated since Python 3.12.Use
datetime.now(datetime.UTC)instead to avoidDeprecationWarningon Python 3.12+.🧹 Proposed fix
-from datetime import datetime +from datetime import datetime, timezone- now = datetime.utcnow() + now = datetime.now(timezone.utc)tests/test_cli_variants.py (1)
10-17: Tests are tightly coupled to registry data — consider adding a brief comment documenting the assumption.The test assumes
CL.CLandH2.0are bothPLANNED. If their status changes in the registry (e.g., promoted toIMPLEMENTED), these tests will silently fail. A short comment noting the expected registry state would help future maintainers.http_smuggler/payloads/http2/h2_cl.py (2)
75-93: Duplicated header/body data in both top-level fields andmetadata.
http2_headersandhttp2_bodyare now first-classPayloadfields, but the same data is also stored undermetadata["h2_headers"]andmetadata["body"](with inconsistent key names). This duplication creates a risk of drift if one is updated without the other. Consider removing the redundantmetadataentries and having consumers read from the top-level fields instead.
300-301:if body:is falsy forb""— useif body is not None:instead.The method signature accepts
Optional[bytes], sob""(empty body) is a valid input distinct fromNone. The current check would skip appending an empty body, which is correct for this file's callers but incorrect as a general contract.🧹 Proposed fix
- if body: + if body is not None:http_smuggler/payloads/http2/h2_te.py (2)
75-91: Redundant metadata entries alongside first-class fields.
http2_headersandhttp2_bodyare now first-classPayloadfields (lines 81-82), but the same data is duplicated inmetadata(lines 88-90). The differential detector already prefers the first-class fields with metadata as a fallback. Consider removing the redundanth2_headers/bodykeys frommetadataacross all payloads in this file to avoid stale-data risks during future refactors.
43-52: Extract shared helpers to a common HTTP/2 base class.
_extract_host_pathand_serialize_h2_requestare duplicated verbatim acrossH2TEPayloadGenerator,H2CLPayloadGenerator, andH2CRLFPayloadGenerator. Consider extracting them into a sharedH2PayloadGeneratorBaseclass or mixin to eliminate duplication and improve maintainability.http_smuggler/detection/differential.py (2)
293-296: HTTP/2 path uses a fixed 0.5s delay; HTTP/1 path uses adaptive delay up to 5s.The HTTP/1 path (lines 143-146) computes an adaptive delay based on baseline response time:
max(0.5, min(baseline_time * 3, 5.0)). The HTTP/2 path uses a hardcoded0.5seconds. For slower backends or realistic downgrade scenarios, 0.5s may be insufficient for the smuggled request to settle, leading to false negatives.Consider applying similar adaptive logic here, using the baseline HTTP/2 response time.
335-351:_h2_to_rawomits response headers fromraw_data.The synthetic
raw_data(line 344) isHTTP/2 {status}\r\n\r\n{body}— it doesn't include the response headers. If any downstream code ever parsesraw_data(e.g.,RawResponse.from_raw()), the headers would be lost. Currently theheadersfield is set directly, so this is safe, but it's a subtle inconsistency to be aware of.http_smuggler/core/engine.py (1)
180-191: Config mutation during init:enabled_variantsis silently modified.
self.config.payload.enabled_variants.discard(variant)(line 191) mutates the config object'senabled_variantsset. This violates the expectation that passing a config to the engine constructor won't modify it. Although the current call site doesn't re-inspect the config after engine creation, this is fragile API design. Consider storing effective variants in an instance variable instead.Proposed fix
- enabled = set(self.config.payload.enabled_variants) + enabled = set(self.config.payload.enabled_variants) # local working copy ... - self.config.payload.enabled_variants.discard(variant) + enabled.discard(variant) + + # Store the effective set for use in scan without mutating config + self._effective_variants = enabledThen reference
self._effective_variantsinstead ofself.config.payload.enabled_variantsin the scanning pipeline (lines 298 and 355).http_smuggler/detection/timing.py (2)
150-150: Unused loop variablei— rename to_.Per static analysis (Ruff B007),
iis unused.Fix
- for i in range(self.baseline_requests): + for _ in range(self.baseline_requests):
432-449: Convenience function doesn't expose newconfidence_modeornetwork_configparameters.
timing_detectalways constructsTimingDetector(safety_config)with defaults for the newly added parameters. Callers of this public API have no way to configure confidence mode or network settings (custom headers/cookies). Consider widening the signature if this function is part of the intended public surface.http_smuggler/main.py (3)
490-495: Return type annotation is unparameterizedtuple.The return type
tupleloses the structural information for callers. A precise annotation would aid IDE support and static analysis.Fix
+from typing import Optional, List, Dict, Set, Tuple + def _parse_variants( variants_str: Optional[str], http2_only: bool, classic_only: bool, -) -> tuple: +) -> Tuple[Set[SmugglingVariant], List[Dict[str, str]]]:
512-513: Default path skipsclassify_requested_variants, returning a hardcoded emptynot_testedlist.When no
--variantsflag is given, the default set fromget_enabled_variants_for_scan()is returned directly with[]fornot_tested. This is correct only ifDEFAULT_SCAN_VARIANTSis guaranteed to exclude planned variants. If a planned variant is ever added to that default set, it won't be filtered out here.Consider routing through
classify_requested_variantsfor consistency with the other code paths.Fix
if not variants_str: - return get_enabled_variants_for_scan(), [] + return classify_requested_variants(get_enabled_variants_for_scan())
303-316:--timeoutonly setstiming_detection_timeout; network-level timeouts remain at defaults.The
NetworkConfigat Line 303 is created without propagating the user's--timeoutvalue, soconnect_timeout(10s) andread_timeout(30s) remain hardcoded defaults. A user passing--timeout 30may expect all operations to respect it, but only the timing detection timeout (Line 314) is affected.Consider forwarding the timeout to
NetworkConfigas well, or clarifying the--timeouthelp text.
| | `H2.0` | HTTP/2 request tunneling | `http2` | `planned` | | ||
| | `h2c` | h2c cleartext upgrade smuggling | `http2` | `planned` | | ||
| | `H2.Tunnel` | HTTP/2 tunnel abuse | `http2` | `planned` | |
There was a problem hiding this comment.
Minor inconsistency: H2.Tunnel only in this doc.
H2.Tunnel (line 37) appears here but is absent from the TECHNICAL_REFERENCE.md and README.md HTTP/2 variant tables. Consider aligning variant lists across all docs, or noting in the registry why it's excluded elsewhere.
🤖 Prompt for AI Agents
In `@docs/VARIANTS.md` around lines 35 - 37, The variant table is inconsistent:
`H2.Tunnel` appears in VARIANTS.md but is missing from the HTTP/2 variant tables
in TECHNICAL_REFERENCE.md and README.md; either add `H2.Tunnel` (with the same
columns as `H2.0`/`h2c`: human name, protocol `http2`, status `planned`) to
those other tables, or in VARIANTS.md add a short clarifying note explaining why
`H2.Tunnel` is intentionally excluded from TECHNICAL_REFERENCE and README (e.g.,
experimental/registry-only) so all docs are aligned; update any cross-references
or TOC entries that list HTTP/2 variants to include the change.
| status = vuln.exploitation.status.replace("_", " ").title() | ||
| lines.append(f"- Status: **{status}**") | ||
| if vuln.exploitation.successful: | ||
| lines.append(f"✅ **Exploitation Successful**") |
There was a problem hiding this comment.
Remove extraneous f prefix from strings without placeholders.
Lines 254 and 259 use f-strings but contain no {…} interpolation. This is flagged by Ruff (F541).
Proposed fix
- lines.append(f"✅ **Exploitation Successful**")
+ lines.append("✅ **Exploitation Successful**")- lines.append(f"❌ **Exploitation Not Confirmed**")
+ lines.append("❌ **Exploitation Not Confirmed**")Also applies to: 259-259
🧰 Tools
🪛 Ruff (0.14.14)
[error] 254-254: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In `@http_smuggler/analysis/reporter.py` at line 254, Remove the unnecessary
f-string prefixes on static strings added to the lines list (e.g., the calls to
lines.append(f"✅ **Exploitation Successful**") and the similar
lines.append(f"...") around line 259) — change them to plain string literals
(lines.append("✅ **Exploitation Successful**")) in reporter.py so there is no
unused f-string interpolation; update any other lines.append calls in the same
function that use f"" without {…} to simple "" as well.
| await asyncio.sleep(0.1) | ||
|
|
||
| assert best is not None | ||
| required = 1 if attempts == 1 else (attempts // 2) + 1 | ||
| best.vulnerable = positives >= required | ||
| return best |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace assert with an explicit guard for production robustness.
assert statements are stripped when Python runs with the -O flag. While the loop guarantees best is always assigned (since attempts >= 1), a future refactor could break this invariant. Use an explicit if to be resilient.
Proposed fix
- assert best is not None
- required = 1 if attempts == 1 else (attempts // 2) + 1
- best.vulnerable = positives >= required
- return best
+ if best is None:
+ raise RuntimeError("No timing detection results produced")
+ required = 1 if attempts == 1 else (attempts // 2) + 1
+ best.vulnerable = positives >= required
+ return best📝 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.
| await asyncio.sleep(0.1) | |
| assert best is not None | |
| required = 1 if attempts == 1 else (attempts // 2) + 1 | |
| best.vulnerable = positives >= required | |
| return best | |
| await asyncio.sleep(0.1) | |
| if best is None: | |
| raise RuntimeError("No timing detection results produced") | |
| required = 1 if attempts == 1 else (attempts // 2) + 1 | |
| best.vulnerable = positives >= required | |
| return best |
🤖 Prompt for AI Agents
In `@http_smuggler/core/engine.py` around lines 574 - 579, Replace the assert
guard with an explicit runtime check: after the loop that sets best, change
"assert best is not None" to an if block that handles the None case (e.g., raise
a clear RuntimeError or return a sensible default) so the code is robust when
running under -O or after future refactors; ensure you still compute required =
1 if attempts == 1 else (attempts // 2) + 1 and set best.vulnerable = positives
>= required only when best is not None, referencing the variables best,
attempts, positives and required in the updated logic.
| for i in range(self.baseline_requests): | ||
| try: | ||
| async with AsyncRawHttpClient(self.network) as client: | ||
| await client.connect(host, port, use_ssl) | ||
|
|
||
| start = time.monotonic() | ||
| response = await client.send_and_receive( | ||
| baseline_request, | ||
| if transport == "http2": | ||
| h2_executor = Http2Executor(self.network) | ||
| response = await h2_executor.send_simple_request( | ||
| host=host, | ||
| port=port, | ||
| path=path, | ||
| receive_timeout=self.safety.timing_detection_timeout, | ||
| method="GET", | ||
| headers=self.network.request_headers, | ||
| ) | ||
| elapsed = time.monotonic() - start | ||
|
|
||
| if response.status_code and 200 <= response.status_code < 500: | ||
| times.append(elapsed) | ||
| elapsed = response.response_time | ||
| else: | ||
| async with AsyncRawHttpClient(self.network) as client: | ||
| await client.connect(host, port, use_ssl) | ||
|
|
||
| start = time.monotonic() | ||
| response = await client.send_and_receive( | ||
| baseline_request, | ||
| receive_timeout=self.safety.timing_detection_timeout, | ||
| ) | ||
| elapsed = time.monotonic() - start | ||
|
|
||
| if response.status_code and 200 <= response.status_code < 500: | ||
| times.append(elapsed) |
There was a problem hiding this comment.
HTTP/2 baseline path lacks the use_ssl guard present in Http2Executor.send_payload.
Http2Executor.send_payload (executor.py Line 141) returns a safe empty response when use_ssl is False, but send_simple_request (used here for baseline) has no such guard. On a non-SSL target with an HTTP/2 payload, the baseline loop will attempt a TLS handshake and likely raise an unhandled exception that is silently swallowed by the bare except.
Also, consider using the get_executor factory (as detect() already does at Line 237) instead of instantiating Http2Executor directly — this keeps a single point of construction.
Proposed fix
if transport == "http2":
- h2_executor = Http2Executor(self.network)
- response = await h2_executor.send_simple_request(
+ if not use_ssl:
+ break # HTTP/2 requires TLS; skip baseline
+ executor = get_executor("http2", self.network)
+ response = await executor.send_simple_request(
host=host,
port=port,
path=path,🧰 Tools
🪛 Ruff (0.14.14)
[warning] 150-150: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
🤖 Prompt for AI Agents
In `@http_smuggler/detection/timing.py` around lines 150 - 175, The baseline
HTTP/2 branch instantiates Http2Executor and calls send_simple_request without
checking use_ssl, which can attempt a TLS handshake on non-SSL targets; update
the baseline loop to mirror send_payload's behavior by only using
Http2Executor/send_simple_request when use_ssl is True (or delegate to the
shared get_executor factory used in detect()), and otherwise fall back to the
AsyncRawHttpClient path; locate the baseline code in timing.py (the for loop
where Http2Executor is created and response.elapsed is read) and change it to
obtain the executor via get_executor(...) or guard the Http2Executor branch with
if use_ssl: so non-SSL targets don’t attempt TLS handshakes. Ensure response
handling and the existing exception handling remain consistent.
| exploit=ExploitConfig( | ||
| enabled=exploit or mode == "aggressive", | ||
| enabled=exploit or scan_mode == ScanMode.AGGRESSIVE, | ||
| ), |
There was a problem hiding this comment.
--no-exploit is silently overridden in aggressive mode.
enabled=exploit or scan_mode == ScanMode.AGGRESSIVE means explicit --no-exploit --mode aggressive still enables exploitation, which contradicts the user's intent. If aggressive mode should always exploit, the --exploit flag's help text should say so; otherwise, respect the explicit opt-out.
Option A — respect explicit opt-out
exploit=ExploitConfig(
- enabled=exploit or scan_mode == ScanMode.AGGRESSIVE,
+ enabled=exploit,
),Option B — document behavior and auto-set --exploit when aggressive is chosen
+ if scan_mode == ScanMode.AGGRESSIVE:
+ exploit = True
+
config = ScanConfig(
...
exploit=ExploitConfig(
- enabled=exploit or scan_mode == ScanMode.AGGRESSIVE,
+ enabled=exploit,
),📝 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.
| exploit=ExploitConfig( | |
| enabled=exploit or mode == "aggressive", | |
| enabled=exploit or scan_mode == ScanMode.AGGRESSIVE, | |
| ), | |
| exploit=ExploitConfig( | |
| enabled=exploit, | |
| ), |
🤖 Prompt for AI Agents
In `@http_smuggler/main.py` around lines 321 - 323, The current line
unconditionally enables exploiting in aggressive mode and overrides an explicit
--no-exploit; change the logic so an explicit exploit flag wins: set
ExploitConfig(enabled = exploit if exploit is not None else (scan_mode ==
ScanMode.AGGRESSIVE)), referencing ExploitConfig, the enabled field, the exploit
variable and scan_mode/ScanMode.AGGRESSIVE; alternatively, if you prefer Option
B, update the CLI/help to state that aggressive mode forces exploitation and
auto-set the exploit flag when parsing the mode instead of silently overriding
user input.
| table.add_row("Not Tested Variants", str(len(getattr(result, "not_tested", [])))) | ||
| table.add_row("Skipped Checks", str(len(getattr(result, "skipped", [])))) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find ScanResult class definition
rg -n "class ScanResult" --type=py -A 25Repository: pentoshi007/http-smuggler
Length of output: 2044
🏁 Script executed:
#!/bin/bash
# Also search for dataclass or pydantic decorators on ScanResult
rg -n "ScanResult" --type=py | grep -E "(class|@|field)" | head -20Repository: pentoshi007/http-smuggler
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Look for the context around lines 465-466 in main.py
cat -n http_smuggler/main.py | sed -n '450,475p'Repository: pentoshi007/http-smuggler
Length of output: 1280
🏁 Script executed:
#!/bin/bash
# Check for `@dataclass` decorator on ScanResult
cat -n http_smuggler/core/models.py | sed -n '160,180p'Repository: pentoshi007/http-smuggler
Length of output: 861
Replace defensive getattr() calls with direct attribute access.
ScanResult is a dataclass with both not_tested and skipped fields defined with field(default_factory=list) (lines 175-176 in http_smuggler/core/models.py). These fields are guaranteed to exist and never be None. The getattr fallbacks at lines 465-466 are unnecessary defensive code; use result.not_tested and result.skipped directly for clarity and type safety.
🤖 Prompt for AI Agents
In `@http_smuggler/main.py` around lines 465 - 466, Replace the unnecessary
defensive getattr calls with direct attribute access: change uses of
getattr(result, "not_tested", []) and getattr(result, "skipped", []) to
result.not_tested and result.skipped respectively (the table.add_row calls that
add "Not Tested Variants" and "Skipped Checks"). These fields are defined on the
ScanResult dataclass (not_tested and skipped), so access them directly for
clarity and type safety.
| | CL.TE, TE.CL, TE.TE, H2.CL, H2.TE, H2.CRLF | Capture Server (port 8888) | Session hijacking | | ||
| | WS.VERSION | Fake 101 Server (port 9999) | WebSocket SSRF | | ||
| | Client-Side Desync | Loot Server (port 8080) | Cookie exfiltration | |
There was a problem hiding this comment.
Inconsistent casing: WS.VERSION vs WS.Version.
Line 205 uses WS.VERSION (all-caps), while VARIANTS.md (line 43) and TECHNICAL_REFERENCE.md (line 49) use WS.Version. Standardize to match the canonical name used in the variant registry.
🤖 Prompt for AI Agents
In `@README.md` around lines 204 - 206, The README table uses the non-canonical
casing "WS.VERSION"; update that table cell to the canonical "WS.Version" to
match VARIANTS.md and TECHNICAL_REFERENCE.md, and search for any other
occurrences of "WS.VERSION" and replace them with "WS.Version" (ensure
references like the table row containing "WS.VERSION | Fake 101 Server (port
9999) | WebSocket SSRF" are updated so the variant name matches the registry).
Summary by CodeRabbit
Release Notes
New Features
Documentation
Improvements