[Master] Enforce HTTP/2 maxConcurrentStreams limit on server#2637
[Master] Enforce HTTP/2 maxConcurrentStreams limit on server#2637daneshk wants to merge 14 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a configurable ChangesHTTP/2 max active streams configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HttpUtil
participant ListenerConfiguration
participant ServerConnectorBootstrap
participant HttpServerChannelInitializer
participant Http2SourceConnectionHandlerBuilder
HttpUtil->>ListenerConfiguration: setHttp2MaxConcurrentStreams(value)
ServerConnectorBootstrap->>HttpServerChannelInitializer: setHttp2MaxConcurrentStreams(http2MaxActiveStreams)
HttpServerChannelInitializer->>Http2SourceConnectionHandlerBuilder: pass maxActiveStreams
Http2SourceConnectionHandlerBuilder->>Http2SourceConnectionHandlerBuilder: initialSettings().maxConcurrentStreams(maxActiveStreams)
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
daneshk
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (79.32%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #2637 +/- ##
============================================
+ Coverage 79.22% 79.32% +0.09%
- Complexity 0 915 +915
============================================
Files 376 441 +65
Lines 21636 25130 +3494
Branches 3382 4112 +730
============================================
+ Hits 17142 19935 +2793
- Misses 3546 3976 +430
- Partials 948 1219 +271 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
daneshk
requested review from
TharmiganK,
lnash94 and
shafreenAnfar
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
native/src/test/java/io/ballerina/stdlib/http/transport/http2/Http2MaxConcurrentStreamsTestCase.java (1)
59-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftTest only covers the H2C prior-knowledge path.
The client here connects over plaintext TCP and builds a raw
Http2ConnectionHandlerwithout an HTTP/1.1 upgrade, so this only exercisesHttp2WithPriorKnowledgeHandler. Per the PR objectives, the fix is meant to apply "across all HTTP/2 server paths: H2C upgrade, H2C prior-knowledge, and TLS+ALPN," but the H2C-upgrade (Http2ServerUpgradeCodec) and TLS+ALPN (Http2PipelineConfiguratorForServer) code paths — which independently construct their ownHttp2SourceConnectionHandlerBuilder— remain untested for this CVE-relevant setting.Given this is a security-fix regression test, consider adding equivalent assertions for the upgrade and ALPN negotiation paths to ensure the limit is enforced consistently everywhere it's wired.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@native/src/test/java/io/ballerina/stdlib/http/transport/http2/Http2MaxConcurrentStreamsTestCase.java` around lines 59 - 148, The new regression test in Http2MaxConcurrentStreamsTestCase only verifies the prior-knowledge H2C path, so it does not cover the upgrade and TLS+ALPN server paths mentioned in the PR. Extend the test coverage by adding equivalent assertions for the Http2ServerUpgradeCodec flow and the Http2PipelineConfiguratorForServer / ALPN flow, using the same maxConcurrentStreams capture helper or a shared variant. Ensure each path exercises its own Http2SourceConnectionHandlerBuilder wiring so the configured SETTINGS_MAX_CONCURRENT_STREAMS limit is validated consistently across all HTTP/2 server setups.README.md (1)
110-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHeading style inconsistency flagged by markdownlint.
Static analysis flags line 110's
####heading as inconsistent with the document's established heading style (expected setext).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 110 - 129, The README section heading uses an ATX style heading that conflicts with the document’s existing setext style. Update the HTTP/2 Stream Concurrency heading in the README so it matches the surrounding markdown heading convention, keeping the section title and content unchanged while converting it to the expected heading format.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@native/src/main/java/io/ballerina/stdlib/http/api/HttpUtil.java`:
- Around line 1587-1591: The http2MaxActiveStreams handling in HttpUtil silently
casts a long to int in setHttp2MaxConcurrentStreams, which can truncate or
overflow the configured stream limit. Replace the direct cast with validation
consistent with the other config paths in this method and the existing
validateConfig() helper used for maxActiveStreamsPerConnection, rejecting
out-of-range values before applying them to listenerConfiguration. Keep the fix
localized in HttpUtil around the endpointConfig lookup and
listenerConfiguration.setHttp2MaxConcurrentStreams call.
---
Nitpick comments:
In
`@native/src/test/java/io/ballerina/stdlib/http/transport/http2/Http2MaxConcurrentStreamsTestCase.java`:
- Around line 59-148: The new regression test in
Http2MaxConcurrentStreamsTestCase only verifies the prior-knowledge H2C path, so
it does not cover the upgrade and TLS+ALPN server paths mentioned in the PR.
Extend the test coverage by adding equivalent assertions for the
Http2ServerUpgradeCodec flow and the Http2PipelineConfiguratorForServer / ALPN
flow, using the same maxConcurrentStreams capture helper or a shared variant.
Ensure each path exercises its own Http2SourceConnectionHandlerBuilder wiring so
the configured SETTINGS_MAX_CONCURRENT_STREAMS limit is validated consistently
across all HTTP/2 server setups.
In `@README.md`:
- Around line 110-129: The README section heading uses an ATX style heading that
conflicts with the document’s existing setext style. Update the HTTP/2 Stream
Concurrency heading in the README so it matches the surrounding markdown heading
convention, keeping the section title and content unchanged while converting it
to the expected heading format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5da35c71-224b-4740-82c6-cf4b0de23332
📒 Files selected for processing (13)
README.mdballerina/README.mdballerina/http_service_endpoint.balnative/src/main/java/io/ballerina/stdlib/http/api/HttpConstants.javanative/src/main/java/io/ballerina/stdlib/http/api/HttpUtil.javanative/src/main/java/io/ballerina/stdlib/http/transport/contract/config/ListenerConfiguration.javanative/src/main/java/io/ballerina/stdlib/http/transport/contractimpl/DefaultHttpWsConnectorFactory.javanative/src/main/java/io/ballerina/stdlib/http/transport/contractimpl/listener/HttpServerChannelInitializer.javanative/src/main/java/io/ballerina/stdlib/http/transport/contractimpl/listener/ServerConnectorBootstrap.javanative/src/main/java/io/ballerina/stdlib/http/transport/contractimpl/listener/http2/Http2SourceConnectionHandlerBuilder.javanative/src/main/java/io/ballerina/stdlib/http/transport/contractimpl/listener/http2/Http2WithPriorKnowledgeHandler.javanative/src/test/java/io/ballerina/stdlib/http/transport/http2/Http2MaxConcurrentStreamsTestCase.javanative/src/test/resources/testng.xml
…move Java defaults
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
native/src/main/java/io/ballerina/stdlib/http/transport/contract/config/ListenerConfiguration.java (1)
289-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueField/accessor naming mismatch.
Field is
http2MaxActiveStreamsbut the getter/setter are namedgetHttp2MaxConcurrentStreams/setHttp2MaxConcurrentStreams. Both names are used in the codebase (http2MaxActiveStreamsin Ballerina config,MaxConcurrentStreamsin native accessors), which is confusing when tracing the value across layers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@native/src/main/java/io/ballerina/stdlib/http/transport/contract/config/ListenerConfiguration.java` around lines 289 - 296, The HTTP/2 stream limit accessors in ListenerConfiguration are named differently from the backing field, which makes the value hard to trace across layers. Align the naming by either renaming the field to match getHttp2MaxConcurrentStreams/setHttp2MaxConcurrentStreams or renaming the accessors to match http2MaxActiveStreams, and update all call sites consistently so the config name and native accessor name no longer diverge.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@native/src/main/java/io/ballerina/stdlib/http/transport/contract/config/ListenerConfiguration.java`:
- Line 67: `ListenerConfiguration` is still allowing `http2MaxActiveStreams` to
stay at 0, which `DefaultHttpWsConnectorFactory` passes through for HTTP/2
listeners. Restore a Java-side fallback of 100 for this field in
`ListenerConfiguration` (or validate and reject HTTP/2 configs when the value is
unset) so direct callers that only set `version = HTTP_2_0` do not advertise
zero concurrent streams.
---
Nitpick comments:
In
`@native/src/main/java/io/ballerina/stdlib/http/transport/contract/config/ListenerConfiguration.java`:
- Around line 289-296: The HTTP/2 stream limit accessors in
ListenerConfiguration are named differently from the backing field, which makes
the value hard to trace across layers. Align the naming by either renaming the
field to match getHttp2MaxConcurrentStreams/setHttp2MaxConcurrentStreams or
renaming the accessors to match http2MaxActiveStreams, and update all call sites
consistently so the config name and native accessor name no longer diverge.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36fd7e37-4349-46d1-b8d2-f8899f9a91e7
📒 Files selected for processing (3)
ballerina/http_service_endpoint.balnative/src/main/java/io/ballerina/stdlib/http/api/HttpUtil.javanative/src/main/java/io/ballerina/stdlib/http/transport/contract/config/ListenerConfiguration.java
🚧 Files skipped from review as they are similar to previous changes (2)
- native/src/main/java/io/ballerina/stdlib/http/api/HttpUtil.java
- ballerina/http_service_endpoint.bal
Per review feedback, the detailed HTTP/2 stream concurrency explanation (RFC citation, defaults, tuning example) is a spec-level detail and doesn't belong in the READMEs. Document it properly under Listener in docs/spec/spec.md instead, and fold a brief mention into the existing feature-highlight sentence in both READMEs to keep them consistent with the terse style used for the other Listener/Service features. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed in 00d615c: moved the HTTP/2 stream concurrency documentation out of both READMEs into |
|
| * 2.1.1. [Automatically starting the service](#211-automatically-starting-the-service) | ||
| * 2.1.2. [Programmatically starting the service](#212-programmatically-starting-the-service) | ||
| * 2.1.3. [Default listener](#213-default-listener) | ||
| * 2.1.4. [HTTP2 stream concurrency](#214-http2-stream-concurrency) |
There was a problem hiding this comment.
should we keep the terms consistent, with HTTP/2?
| password = "ballerina" | ||
| ``` | ||
|
|
||
| #### 2.1.4. HTTP2 stream concurrency |
| listenerConfiguration.setPipeliningEnabled(true); //Pipelining is enabled all the time | ||
| listenerConfiguration.setHttp2InitialWindowSize(endpointConfig | ||
| .getIntValue(ENDPOINT_CONFIG_HTTP2_INITIAL_WINDOW_SIZE).intValue()); | ||
| listenerConfiguration.setHttp2MaxConcurrentStreams( |
There was a problem hiding this comment.
In the pr description, it says: "Set to -1 for unlimited streams (not recommended for public-facing services)"
Where is this logic handled?
|
|
||
| long maxActiveStreamsPerConnection = | ||
| poolRecord.getIntValue(HttpConstants.CONNECTION_POOLING_MAX_ACTIVE_STREAMS_PER_CONNECTION); | ||
| poolConfiguration.setHttp2MaxActiveStreamsPerConnection( |
There was a problem hiding this comment.
From PR description: Set to -1 for unlimited streams (not recommended for public-facing services)
, why is it handled here?
3 participants
Summary
Fixes CVE-2026-47244: `DefaultHttp2Connection` in Netty prior to 4.1.135 initializes `maxActiveStreams` to `Integer.MAX_VALUE`, allowing a single TCP connection to open unbounded concurrent streams and exhaust heap memory (OOM).
Performance
Tested with
h2load(nghttp2/1.69.0) against a stock H2C listener on localhost. No regression observed — Netty 4.1.135 includes general throughput improvements alongside the security fix.All 200,000 requests succeeded with 0 errors in both cases. Well-behaved clients (which respect
SETTINGS_MAX_CONCURRENT_STREAMS) are unaffected by the enforcement. Only clients that previously exploited the lack of enforcement (sending unlimited streams on one connection) are now correctly rate-limited.Server SETTINGS confirmed via nghttp:
```
recv SETTINGS frame
[SETTINGS_MAX_CONCURRENT_STREAMS(0x03):100]
```
Related Issues
This PR enforces HTTP/2 server-side concurrent stream limits across all HTTP/2 server connection flows and updates the Netty dependency to ensure the limit is applied at the connection level.
Key updates:
http2MaxActiveStreamslistener configuration (default:100) to let each listener tune its per-connection HTTP/2 concurrency limit; supports an “unlimited” mode via-1.SETTINGS_MAX_CONCURRENT_STREAMSvalue to clients.4.1.135.Finalto align server-side stream limit enforcement with the new configuration.docs/spec/spec.md.