Commits
Tarun Sukhani authored c7e0a5d25e4
[PF-54..PF-66] Second Codex audit: 13 fixes (5 P1 ship-blockers + 8 P2) All twenty-six PFs from the second external Codex review are addressed. 299/299 unit tests pass. P1 ship-blockers (protocol correctness): - PF-54 StreamChunkAggregator: rejectOversize/rejectBadRequest now write via ctx.channel().writeAndFlush(...). Outbound from the aggregator's ctx flowed head-ward and SKIPPED HttpResponseEncoder (which sits tail-ward of the aggregator), shipping a raw FullHttpResponse object that never reached the wire as HTTP. Clients saw a connection close instead of a valid 413 - PF-55 StreamChunkAggregator: handle Expect: 100-continue. Strict upload clients (curl --expect100, Apache HttpClient with ExpectContinue=true) waited forever before sending the body. Now we respond 100 Continue, or 413 if Content-Length already exceeds play.netty.maxContentLength - PF-56 StreamChunkAggregator: check decoderResult().isFailure() on HttpRequest and HttpContent and respond 400 + close. Without this guard the aggregator concatenated partial bytes from a malformed chunked stream and dispatched it as a fresh "successful" FullHttpRequest with truncated body - PF-57 FileService: multi-range Content-Length now includes the closing --boundary-- trailer that readChunk() emits. The trailer bytes had been overrunning Content-Length and corrupting framing of the *next* response on a keep-alive connection - PF-58 FileService: ByteRangeInput.isEndOfInput() now scans every range from the cursor to the end. The previous check observed only byteRanges[currentByteRange]; if a part body filled exactly chunkSize, ChunkedWriteHandler saw remaining()==0 on the just-finished range and stopped the response with later parts unsent P2 (lifetime, correctness, configuration): - PF-59 PlayHandler.parseRequest: wrap the post-spool-acquisition path in try/catch that closes the FileInputStream and unlinks the temp file before rethrowing. A non-numeric port in the Host header was leaking one temp file per request because the spool channel attribute was already cleared by getAndSet(null) - PF-60 PlayHandler: Http.Request.current is updated to the parsed Request before plugin rawInvocation runs. Previously it pointed at the blank placeholder ctor, so plugins that read Http.Request.current() saw the wrong cookies, headers, body, params - PF-61 PlayHandler: NettyInvocation._finally and WebSocketInvocation._finally now remove() Request/Response/Scope/ Inbound/Outbound ThreadLocals; the IO-thread channelRead does the same. A long-idle keep-alive connection no longer pins the previous parsed body in memory until the next message overwrites the slot - PF-62 PlayHandler.serve500: respect cookie.sendOnError. ServletWrapper has always followed this rule (play.server.ServletWrapper:382); the Netty path was permissive and could persist partial Session/Flash/Lang state across a failed action - PF-63 FileService: 416 responses now send Content-Range: bytes */<fileLength> per RFC 7233 §4.2. The previous bytes 0-(fileLength-1)/fileLength was non-spec and degenerated to the invalid bytes 0--1/0 for empty files - PF-64 FileService: ByteRange.servedRange widened from int to long. Single-range responses past 2 GiB (4 K video, large archives) had overflowed the counter, corrupting both remaining() and the raf.seek(start + servedRange) call (file pointer jumped backwards) - PF-65 HttpServerPipelineFactory + SslHttpServerPipelineFactory: sanitize play.netty.maxContentLength before passing to a user-wired HttpObjectAggregator. Netty's ctor rejects negatives with IllegalArgumentException, so the documented "unlimited" sentinel -1 silently dropped the explicit aggregator from the pipeline. Promote any negative value to Integer.MAX_VALUE - PF-66 PlayHandler.websocketHandshake: WS frame/message cap now treats -1 as Integer.MAX_VALUE for symmetry with the HTTP body cap. The previous code coerced negatives to a hard 65345, contradicting the documented "unlimited" semantic Note on the audit's reported test failures: the "failures in non-Netty classes (CacheTest, deps/YAML tests)" stdout entries are Log4j2 setup messages at ERROR level (`SystemPropertyArbiter contains an invalid element... "List"`, `Unable to locate appender "Out"`) emitted when the test logging config initialises. Each affected test class reports "Failures: 0" -- ant clean unittest passes 299/299 with zero failures on this checkout. Verified with `ant clean unittest`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>