fix: Reject invalid extended CONNECT requests#3385
Draft
larseggert wants to merge 8 commits intomozilla:mainfrom
Draft
fix: Reject invalid extended CONNECT requests#3385larseggert wants to merge 8 commits intomozilla:mainfrom
larseggert wants to merge 8 commits intomozilla:mainfrom
Conversation
larseggert
requested review from
KershawChang,
martinthomson and
mxinden
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the handling of invalid Extended CONNECT requests by replacing unimplemented!() panics with proper HTTP error responses. The server now correctly rejects Extended CONNECT requests with unsupported protocols (501 Not Implemented) or missing :protocol headers (400 Bad Request), as specified in RFC 9220.
Changes:
- Replaced
unimplemented!()macros with proper error handling and logging for invalid Extended CONNECT requests - Added tracking of rejected Extended CONNECT requests with appropriate HTTP status codes (501 for unsupported protocols, 400 for missing
:protocolheader) - Implemented automatic error response mechanism to send status codes back to clients
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| neqo-http3/src/server_connection_events.rs | Added tracking structure for rejected Extended CONNECT requests and logic to assign status codes; includes comprehensive tests for both error cases |
| neqo-http3/src/connection_server.rs | Implements error response sending mechanism that processes rejected requests and transmits appropriate HTTP status codes |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3385 +/- ##
==========================================
- Coverage 94.26% 94.14% -0.12%
==========================================
Files 125 129 +4
Lines 37973 38535 +562
Branches 37973 38535 +562
==========================================
+ Hits 35795 36280 +485
- Misses 1339 1408 +69
- Partials 839 847 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Merging this PR will degrade performance by 8.73%
Performance Changes
Comparing 
Footnotes
|
Member
martinthomson
left a comment
martinthomson
left a comment
There was a problem hiding this comment.
This is probably largely academic, but your focus on extended connect forgets the not-extended version.
mxinden
reviewed
Feb 9, 2026
Member
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
Do we need this server functionality for a test? If not, is this fix worth the effort and complexity?
martinthomson
requested changes
Feb 10, 2026
Member
martinthomson
left a comment
martinthomson
left a comment
There was a problem hiding this comment.
These three comments are each separately problematic.
larseggert
force-pushed
the
fix-rejected_extended_connect
branch
from
5f3522c to
4a3a724
Compare
Member
martinthomson
left a comment
martinthomson
left a comment
There was a problem hiding this comment.
Getting there. Just a potential tweak to one test (for robustness) and a missing test. Other than that, I think the design looks good.
| { | ||
| _ = handler_borrowed.stream_close_send(stream_id, conn, now); | ||
| } else { | ||
| // Ensure the stream doesn't linger if we can't send the response. |
Member
There was a problem hiding this comment.
I like this. It's a very sensible way to avoid sitting on requests.
Comment on lines
+1164
to
+1167
| // Client sends STOP_SENDING in the same flight as the request. | ||
| peer_conn | ||
| .stream_stop_sending(stream_id, Error::HttpNone.code()) | ||
| .unwrap(); |
Member
There was a problem hiding this comment.
What happens if we change our code such that STOP_SENDING ends up causing other frames to not be sent? I mean, that's something we might want to consider doing, isn't it?
(If we had RESET_STREAM_AT, you could commit(), which would make the data you sent relevant. You wouldn't necessarily guarantee that the fin flag makes it through. The write would, so this is probably OK.)
The alternative approach here is to write the STOP_SENDING onto the wire directly, using the test frame writer.
Collaborator
Author
Member
There was a problem hiding this comment.
Waiting on RESET_STREAM_AT seems like a good plan.
| /// When the client sends `STOP_SENDING` before the server processes the | ||
| /// rejection, `send_headers` fails and the server falls back to `cancel_fetch`. | ||
| #[test] | ||
| fn rejected_extended_connect_fallback_on_stop_sending() { |
Member
There was a problem hiding this comment.
I think that there is a missing test: where the server has to reject the stream rather than send a response.
Collaborator
Author
There was a problem hiding this comment.
I'm not quite sure I understand what you mean here. I added a check that the client receives a RESET_STREAM; is this what you meant?
Member
There was a problem hiding this comment.
Yeah. For that you probably need to configure the client with a very small flow control buffer.
Contributor
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
|
Contributor
Client/server transfer resultsPerformance differences relative to cc1d9b5. Transfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Table above only shows statistically significant changes. See all results below. All resultsTransfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Download data for |
Contributor
Benchmark resultsNo significant performance differences relative to cc1d9b5. All resultstransfer/1-conn/1-100mb-resp (aka. Download)/mtu-1504: Change within noise threshold. time: [198.25 ms 198.74 ms 199.33 ms]
thrpt: [501.69 MiB/s 503.17 MiB/s 504.41 MiB/s]
change:
time: [-1.5116% -1.1370% -0.7540] (p = 0.00 < 0.05)
thrpt: [+0.7598% +1.1501% +1.5348]
Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) high mild
2 (2.00%) high severetransfer/1-conn/10_000-parallel-1b-resp (aka. RPS)/mtu-1504: No change in performance detected. time: [282.18 ms 284.54 ms 287.06 ms]
thrpt: [34.836 Kelem/s 35.144 Kelem/s 35.438 Kelem/s]
change:
time: [-0.8118% +0.2051% +1.2107] (p = 0.70 > 0.05)
thrpt: [-1.1962% -0.2047% +0.8185]
No change in performance detected.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severetransfer/1-conn/1-1b-resp (aka. HPS)/mtu-1504: No change in performance detected. time: [38.575 ms 38.743 ms 38.928 ms]
thrpt: [25.689 B/s 25.811 B/s 25.923 B/s]
change:
time: [-0.3247% +0.2959% +0.9287] (p = 0.37 > 0.05)
thrpt: [-0.9201% -0.2950% +0.3258]
No change in performance detected.
Found 9 outliers among 100 measurements (9.00%)
2 (2.00%) high mild
7 (7.00%) high severetransfer/1-conn/1-100mb-req (aka. Upload)/mtu-1504: Change within noise threshold. time: [202.13 ms 202.51 ms 202.92 ms]
thrpt: [492.80 MiB/s 493.80 MiB/s 494.73 MiB/s]
change:
time: [-0.8017% -0.4397% -0.1089] (p = 0.01 < 0.05)
thrpt: [+0.1090% +0.4416% +0.8082]
Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severedecode 4096 bytes, mask ff: No change in performance detected. time: [4.5110 µs 4.5213 µs 4.5331 µs]
change: [-0.4316% -0.0836% +0.2761] (p = 0.67 > 0.05)
No change in performance detected.
Found 12 outliers among 100 measurements (12.00%)
8 (8.00%) high mild
4 (4.00%) high severedecode 1048576 bytes, mask ff: No change in performance detected. time: [1.1578 ms 1.1604 ms 1.1642 ms]
change: [-0.6941% -0.2186% +0.2275] (p = 0.37 > 0.05)
No change in performance detected.
Found 13 outliers among 100 measurements (13.00%)
9 (9.00%) low severe
4 (4.00%) high severedecode 4096 bytes, mask 7f: No change in performance detected. time: [5.7904 µs 5.8002 µs 5.8103 µs]
change: [-0.3062% -0.0148% +0.3000] (p = 0.92 > 0.05)
No change in performance detected.
Found 5 outliers among 100 measurements (5.00%)
4 (4.00%) high mild
1 (1.00%) high severedecode 1048576 bytes, mask 7f: No change in performance detected. time: [1.4860 ms 1.4883 ms 1.4906 ms]
change: [-0.0827% +0.1561% +0.4129] (p = 0.20 > 0.05)
No change in performance detected.decode 4096 bytes, mask 3f: No change in performance detected. time: [5.5353 µs 5.5438 µs 5.5533 µs]
change: [-0.4103% -0.1024% +0.1998] (p = 0.52 > 0.05)
No change in performance detected.
Found 5 outliers among 100 measurements (5.00%)
2 (2.00%) high mild
3 (3.00%) high severedecode 1048576 bytes, mask 3f: No change in performance detected. time: [1.4145 ms 1.4167 ms 1.4191 ms]
change: [-0.1952% +0.0197% +0.2419] (p = 0.87 > 0.05)
No change in performance detected.streams/simulated/1-streams/each-1000-bytes: No change in performance detected. time: [129.68 ms 129.68 ms 129.69 ms]
thrpt: [7.5302 KiB/s 7.5305 KiB/s 7.5307 KiB/s]
change:
time: [-0.0024% +0.0017% +0.0056] (p = 0.43 > 0.05)
thrpt: [-0.0056% -0.0017% +0.0024]
No change in performance detected.
Found 4 outliers among 100 measurements (4.00%)
4 (4.00%) high mildstreams/simulated/1000-streams/each-1-bytes: Change within noise threshold. time: [2.5359 s 2.5361 s 2.5364 s]
thrpt: [394.26 B/s 394.30 B/s 394.34 B/s]
change:
time: [-0.0441% -0.0275% -0.0114] (p = 0.00 < 0.05)
thrpt: [+0.0114% +0.0275% +0.0441]
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildstreams/simulated/1000-streams/each-1000-bytes: No change in performance detected. time: [6.5843 s 6.5916 s 6.5999 s]
thrpt: [147.97 KiB/s 148.15 KiB/s 148.32 KiB/s]
change:
time: [-0.2525% -0.0584% +0.1358] (p = 0.54 > 0.05)
thrpt: [-0.1356% +0.0584% +0.2531]
No change in performance detected.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high severestreams/walltime/1-streams/each-1000-bytes: No change in performance detected. time: [583.61 µs 585.84 µs 588.42 µs]
change: [-0.7669% -0.1564% +0.4051] (p = 0.60 > 0.05)
No change in performance detected.
Found 10 outliers among 100 measurements (10.00%)
10 (10.00%) high severestreams/walltime/1000-streams/each-1-bytes: Change within noise threshold. time: [12.493 ms 12.510 ms 12.527 ms]
change: [+0.9841% +1.2032% +1.4218] (p = 0.00 < 0.05)
Change within noise threshold.streams/walltime/1000-streams/each-1000-bytes: Change within noise threshold. time: [45.175 ms 45.219 ms 45.262 ms]
change: [-0.4363% -0.2953% -0.1629] (p = 0.00 < 0.05)
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildcoalesce_acked_from_zero 1+1 entries: No change in performance detected. time: [92.055 ns 92.384 ns 92.710 ns]
change: [-0.3470% +0.1646% +0.7881] (p = 0.57 > 0.05)
No change in performance detected.
Found 13 outliers among 100 measurements (13.00%)
11 (11.00%) high mild
2 (2.00%) high severecoalesce_acked_from_zero 3+1 entries: No change in performance detected. time: [110.00 ns 110.71 ns 111.77 ns]
change: [-0.4152% +0.1529% +0.7748] (p = 0.63 > 0.05)
No change in performance detected.
Found 15 outliers among 100 measurements (15.00%)
8 (8.00%) high mild
7 (7.00%) high severecoalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [109.20 ns 109.56 ns 110.01 ns]
change: [-0.2086% +0.1951% +0.6611] (p = 0.41 > 0.05)
No change in performance detected.
Found 14 outliers among 100 measurements (14.00%)
5 (5.00%) low severe
2 (2.00%) low mild
2 (2.00%) high mild
5 (5.00%) high severecoalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [94.452 ns 94.585 ns 94.736 ns]
change: [-0.6640% -0.1879% +0.4067] (p = 0.48 > 0.05)
No change in performance detected.
Found 11 outliers among 100 measurements (11.00%)
4 (4.00%) high mild
7 (7.00%) high severeRxStreamOrderer::inbound_frame(): Change within noise threshold. time: [108.97 ms 109.04 ms 109.11 ms]
change: [-0.5147% -0.3180% -0.1608] (p = 0.00 < 0.05)
Change within noise threshold.
Found 29 outliers among 100 measurements (29.00%)
10 (10.00%) low severe
8 (8.00%) high mild
11 (11.00%) high severesent::Packets::take_ranges: No change in performance detected. time: [4.4750 µs 4.5691 µs 4.6559 µs]
change: [-4.4212% -1.5583% +1.5726] (p = 0.31 > 0.05)
No change in performance detected.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildtransfer/simulated/pacing-false/varying-seeds: No change in performance detected. time: [23.941 s 23.941 s 23.941 s]
thrpt: [171.09 KiB/s 171.09 KiB/s 171.09 KiB/s]
change:
time: [+0.0000% +0.0000% +0.0000] (p = NaN > 0.05)
thrpt: [+0.0000% +0.0000% +0.0000]
No change in performance detected.transfer/simulated/pacing-true/varying-seeds: No change in performance detected. time: [23.676 s 23.676 s 23.676 s]
thrpt: [173.01 KiB/s 173.01 KiB/s 173.01 KiB/s]
change:
time: [+0.0000% +0.0000% +0.0000] (p = NaN > 0.05)
thrpt: [+0.0000% +0.0000% +0.0000]
No change in performance detected.transfer/simulated/pacing-false/same-seed: No change in performance detected. time: [23.941 s 23.941 s 23.941 s]
thrpt: [171.09 KiB/s 171.09 KiB/s 171.09 KiB/s]
change:
time: [+0.0000% +0.0000% +0.0000] (p = NaN > 0.05)
thrpt: [+0.0000% +0.0000% +0.0000]
No change in performance detected.transfer/simulated/pacing-true/same-seed: No change in performance detected. time: [23.676 s 23.676 s 23.676 s]
thrpt: [173.01 KiB/s 173.01 KiB/s 173.01 KiB/s]
change:
time: [+0.0000% +0.0000% +0.0000] (p = NaN > 0.05)
thrpt: [+0.0000% +0.0000% +0.0000]
No change in performance detected.transfer/walltime/pacing-false/varying-seeds: Change within noise threshold. time: [23.456 ms 23.487 ms 23.536 ms]
change: [-1.0611% -0.9094% -0.7134] (p = 0.00 < 0.05)
Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severetransfer/walltime/pacing-true/varying-seeds: Change within noise threshold. time: [23.633 ms 23.664 ms 23.712 ms]
change: [-1.3828% -1.2257% -1.0161] (p = 0.00 < 0.05)
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severetransfer/walltime/pacing-false/same-seed: Change within noise threshold. time: [23.513 ms 23.532 ms 23.552 ms]
change: [-0.7734% -0.6640% -0.5571] (p = 0.00 < 0.05)
Change within noise threshold.
Found 6 outliers among 100 measurements (6.00%)
2 (2.00%) low mild
2 (2.00%) high mild
2 (2.00%) high severetransfer/walltime/pacing-true/same-seed: Change within noise threshold. time: [23.876 ms 23.906 ms 23.950 ms]
change: [-0.7617% -0.5956% -0.3919] (p = 0.00 < 0.05)
Change within noise threshold.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) low mild
1 (1.00%) high mild
2 (2.00%) high severeDownload data for |
Collaborator
Author
|
Blocked until #3102 lands, see comment above. |
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
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.
See https://bugzilla.mozilla.org/show_bug.cgi?id=1997780