allthingssecurity opened a new pull request, #27757: URL: https://github.com/apache/camel/pull/27757
# Description [CAMEL-25573](https://issues.apache.org/jira/browse/CAMEL-25573) The body of a request received by a netty-http consumer is a `NettyChannelBufferStreamCache` (unless `disableStreamCache` or `httpProxy` is set), and a route step that asks for the body as an `InputStream` gets this object. Its `read()` did not keep the `InputStream` contract. What that means with the default configuration: 1. Loud: `from("netty-http:...").unmarshal().gzipDeflater()` did not work. Every request failed with HTTP 500 and `java.io.IOException: Input is not in the .gz format.` in the log. The same holds for any reader that reads the first bytes one by one (to sniff a header, for example): it got wrong values for bytes from 0x80, or an `IndexOutOfBoundsException` at the end of the body. 2. Silent, and narrower: user code or a library that reads the body byte by byte (`while ((b = in.read()) != -1)`) got only the bytes before the first 0xFF, without an error, and negative values for bytes from 0x80. A byte 0xFF is common in binary payloads and never occurs in UTF-8 text, so text bodies were not cut; for them the loop ended with the `IndexOutOfBoundsException` (loud). Not affected: everything that reads in bulk. The type converters of Camel to `byte[]` and `String` (`StreamCacheConverter.convertToByteArray` calls `writeTo`) and `NettyHttpConverter.toByteBuf` read with `read(byte[], int, int)`, which is why most routes never saw this. Silent loss is not the general case. The causes in the class: - `read()` was `return buffer.readByte();`. `ByteBuf.readByte()` returns a Java `byte` (-128 to 127) and throws `IndexOutOfBoundsException` when nothing is left, while `InputStream.read()` returns "an int in the range 0 to 255" and -1 at the end. So a byte 0xFF was returned as -1 (the end of the stream for every reader), bytes 0x80 to 0xFE as negative values, and at the end of the body the method threw. - `markSupported()` returned true, but `mark(int)` was the empty method of `InputStream`. The gzip reader of commons-compress uses a stream with `markSupported()` as it is, so it read the magic number with `read()` and got -117 for the byte 0x8b. (`java.util.zip.GZIPInputStream` failed with `NettyChannelBufferStreamCache.read() returned value out of range -1..255: -117`.) - `read(byte[], int, int)` took `buffer.capacity()` for the end of the content, and threw `IndexOutOfBoundsException` at the end of a buffer whose capacity is larger than its writer index. The aggregated content of a request has no spare capacity, so the consumer does not reach this one today; it is fixed because the method is being touched and the class takes any `ByteBuf`. This change: - `read()` returns `buffer.readByte() & 0xFF`, and -1 when the buffer has no readable byte. - `markSupported()` returns false. `mark(int)` stays the empty method and `reset()` is not changed: it goes back to the start of the body, as `StreamCache.reset()` needs. A reader that needs mark support wraps the stream (the gzip reader of commons-compress does that by itself with a `BufferedInputStream`). - `read(byte[], int, int)` uses `buffer.readableBytes()`. Why not implement `mark(int)`: `reset()`, `writeTo()` (used by the converters to `byte[]` and `String`) and `NettyHttpConverter.toByteBuf` all go back to the marked reader index of the buffer, which today is always the start because only the constructor marks. If `mark(int)` moved it, a reader that marks in the middle (the gzip reader marks before every buffer of 8192 bytes) would leave the stream cache returning only its tail afterwards. I ran the new tests against that variant: after the gzip unmarshal `reset()` and `writeTo()` returned 3634 of 20028 bytes. A stream cache without mark support whose `reset()` goes back to the start already exists: `FileInputStreamCache` (`FileInputStreamCache.java:59`, no `markSupported`/`mark` override; `reset()` at `:89` drops the open stream so that the next read starts at the beginning of the file). The in-memory stream caches of camel-support differ: they are, or delegate to, a `ByteArrayInputStream`, which supports mark. Behaviour change, with a note in the 4.24 upgrade guide (this creates `camel-4x-upgrade-guide-4_24.adoc` with the header of the 4.23 guide and adds its line to the index; if another pull request creates the file first, only the section is needed): byte-by-byte readers now get 0 to 255 and -1 at the end, and `markSupported()` is false. Code that asks `markSupported()` and then relies on `mark`/`reset` of this stream now takes its other path; in the main sources of Camel the only caller of `markSupported()` on a body stream that can be this class is `BlobClientWrapper` of camel-azure-storage-blob (`:137`, it passes the value on to the Azure SDK; read, not run). Tests: - New `NettyHttpGzipRequestBodyTest` in camel-netty-http (4 tests, real consumer and producer): a gzip request body unmarshalled with `gzipDeflater()`; for a gzip body of 20028 bytes (larger than the 8192 byte buffer of the gzip reader) kept in a property, `reset()` and then a read, and the conversion to `byte[]` (`writeTo`), both after the unmarshal, give the whole body; a binary body with a 0xFF byte counted with `read()` in a processor. camel-zip-deflater is added as a test dependency. - New `NettyChannelBufferStreamCacheTest` (7 tests on the class): unsigned values and -1 from `read()`, the copy loop for binary and for text content, `markSupported()` is false, `reset()` and `writeTo()` give the whole body of 20000 bytes after a reader that calls `mark` before every buffer, a buffer with spare capacity, and `java.util.zip.GZIPInputStream` on the stream. - Without the change (the committed tests with the class of main) 10 of the 11 fail: - `gzipDataFormatUnmarshalsTheRequestBody`, `resetReturnsTheWholeRequestBodyAfterTheGzipUnmarshal`, `writeToReturnsTheWholeRequestBodyAfterTheGzipUnmarshal`: `NettyHttpOperationFailedException ... statusCode: 500`, in the log of the consumer `java.io.IOException: Input is not in the .gz format.` (all three fail at the unmarshal on main) - `processorReadsTheRequestBodyByteByByte`: `expected: <5> but was: <2>` - `readReturnsEveryByteAsUnsignedValue`: `expected: <128> but was: <-128>` - `readLoopCopiesBinaryContent`: `array lengths differ, expected: <5> but was: <2>` - `readLoopEndsAtTheEndOfText`: `IndexOutOfBounds readerIndex(5) + length(1) exceeds writerIndex(5)` - `markIsNotSupported`: `expected: <false> but was: <true>` - `readStopsAtTheLastWrittenByte`: `IndexOutOfBounds readerIndex(0) + length(16) exceeds writerIndex(3)` - `gzipInputStreamReadsItsHeader`: `read() returned value out of range -1..255: -117` - `resetAndWriteToReturnTheWholeBodyAfterAReaderThatMarks` passes on main too (there `mark` does nothing either). It is kept because it fails when `mark(int)` is implemented (`array lengths differ, expected: <20000> but was: <0>`), together with the two route tests above (`expected: <20028> but was: <3634>`). - With the change, camel-netty-http: 312 tests, 0 failures, 8 skipped. Not run: a byte-by-byte reader of a text body in a route (the `IndexOutOfBoundsException` at the end is shown on the class only), a dead letter channel with `useOriginalMessage`, and camel-azure-storage-blob with this body. Related: a separate ticket and pull request make the gzip data format of camel-zip-deflater read the body through a `BufferedInputStream` of its own, for the in-memory stream caches of camel-support. The two are independent, and this one does not rely on it. If that one is merged first, the three gzip route tests should pass without this change too (the gzip reader then reads in bulk; read in the code, not run); the other seven failing tests still need it. The same `read()` pattern (a signed byte returned) is in `CassandraCamelCodec.ByteBufferInputStream` (camel-cassandraql, read only by an `ObjectInputStream`) and in `JsonStream` (camel-jsonpath, for its first four bytes). I found no reader that is affected there, and they are not changed here. Found with a Lean 4 model of `read()` and of the copy loop of a reader: "the reader gets the whole body" fails for every body with a byte 0xFF (the reader ends normally with the bytes before it); a body without one is shown to end with the exception for the tested case. For the fixed `read()` the copy loop is proved to return every body, and main and the fix are proved equal for bytes below 0x80. For `reset()` the model shows that no sequence of read, mark and reset calls of a reader changes where it goes back to (true by construction of the model: `mark` does nothing), and gives the 3634 of 20028 bytes for the variant with a marking `mark(int)`. I then reproduced it with the consumer. # Target - [x] I checked that the commit is targeting the correct branch (Camel 4 uses the `main` branch) # Tracking - [x] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it). # Apache Camel coding standards and style - [x] I checked that each commit in the pull request has a meaningful subject line and body. - [ ] I have run `mvn clean install -DskipTests` locally from root folder and I have committed all auto-generated changes. (I built and tested camel-netty-http, including the formatter and import-sort plugins. No generated file changes. I did not run the full root build.) # AI-assisted contributions - [x] If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., `Co-authored-by` trailers) and the PR description identifies the AI tool used. This PR was prepared with Claude Code (Claude Opus 5.5). The commit carries a `Co-Authored-By` trailer. _Claude Code on behalf of allthingssecurity_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
