Parse and store URL-decoded query parameters separately from path

[Ericthilen]

Fixes #102

Implemented support for parsing query parameters from the request URI.

• Extracts path separately from query string
• Parses parameters after '?'
• Supports multiple values per key
• Applies UTF-8 URL decoding to keys and values
• Stores query parameters separately in HttpRequest

Summary by CodeRabbit

• New Features

  • HTTP web server with TCP listener and static file serving (includes custom 404)
  • Configuration via YAML/JSON with CLI port override
  • IP-based access control (allowlist/blocklist), request logging, and locale detection (header + cookie)
  • Docker multi-architecture image support

• Documentation

  • Expanded README with Quick Start, Configuration, and examples
  • Swedish PortConfiguration guide

• Tests

  • Extensive unit tests covering parser, handlers, filters, MIME detection, and response builder

• Chores

  • CI and release workflows added (build, test, and publish images)

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a Java HTTP web server with virtual-thread TCP handling, configurable filters, static file serving, configuration loader, response/mime utilities, extensive unit tests, Docker support, and GitHub Actions workflows for CI and release.

Changes

Cohort / File(s)	Summary
CI/CD & Container ​.github/workflows/ci.yml, ​.github/workflows/release.yml, Dockerfile	Added GitHub Actions for Java CI and GHCR multi-arch release; added multi-stage Dockerfile building with Maven and running on a lightweight JRE.
Build & Dependencies pom.xml	Bumped compiler release to 25; added bucket4j, test libraries (mockito, awaitility, jackson), brotli4j; integrated Pitest and Spotless plugins.
Application entry & server src/main/java/org/example/App.java, src/main/java/org/example/TcpServer.java, src/main/java/org/example/ConnectionFactory.java	Entrypoint now loads config and resolves port (CLI > config); new TcpServer using virtual threads and ConnectionFactory abstraction.
Connection & static serving src/main/java/org/example/ConnectionHandler.java, src/main/java/org/example/StaticFileHandler.java	New ConnectionHandler parses requests, applies filters, resolves targets and serves files; StaticFileHandler serves files with traversal protection and 403/404 handling.
Configuration src/main/java/org/example/config/AppConfig.java, src/main/java/org/example/config/ConfigLoader.java, src/main/resources/application.yml, PortConfigurationGuide.md	Introduced AppConfig records with defaults, ConfigLoader with YAML/JSON support and caching/reset for tests, and default application.yml; added port configuration guide.
HTTP parsing & response utilities src/main/java/org/example/httpparser/..., src/main/java/org/example/http/HttpResponseBuilder.java, src/main/java/org/example/http/MimeTypeDetector.java	Added HTTP request parsing classes (request line, headers, HttpRequest), response builder with headers/status/body handling, and MIME type detector.
Filter framework src/main/java/org/example/filter/*	Added Filter contract, FilterChain/FilterChainImpl, IpFilter (allow/block modes), LocaleFilter and LocaleFilterWithCookie, and LoggingFilter.
Tests src/test/java/... (many files)	Added comprehensive unit tests covering port resolution, ConfigLoader, ConnectionHandler, StaticFileHandler, TcpServer error handling, filters (Ip, Locale, Logging), HttpResponseBuilder, MimeTypeDetector, and HTTP parsing.
Docs & static assets README.md, www/index.html, www/pageNotFound.html	Rewrote README with server docs and examples; added index and custom 404 page for static serving.

Sequence Diagram(s)

sequenceDiagram
    actor Client
    participant TcpServer
    participant VirtualThread as Virtual<br/>Thread
    participant ConnectionHandler
    participant FilterChain
    participant Filters
    participant StaticFileHandler
    participant HttpResponseBuilder

    Client->>TcpServer: TCP connect
    TcpServer->>TcpServer: accept socket
    TcpServer->>VirtualThread: spawn handler thread
    VirtualThread->>ConnectionHandler: runConnectionHandler()
    ConnectionHandler->>ConnectionHandler: parse request (method, uri, headers)
    ConnectionHandler->>FilterChain: applyFilters(request)
    FilterChain->>Filters: invoke filters (Ip, Locale, Logging)
    alt filter blocks
        Filters->>HttpResponseBuilder: produce 4xx response
    else filters pass
        ConnectionHandler->>StaticFileHandler: resolve & read target file
        StaticFileHandler->>HttpResponseBuilder: build 200/404 response
    end
    HttpResponseBuilder->>ConnectionHandler: response bytes
    ConnectionHandler->>Client: write HTTP response
    ConnectionHandler->>VirtualThread: close socket

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Poem

🐰 I hopped through ports and parsed each line,
Filters in paw, static files shine.
Virtual threads hum, responses take flight,
Configs and tests tuck in for the night.
A rabbit applauds this server delight! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 11.62% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Parse and store URL-decoded query parameters separately from path' directly describes the main change: extracting and storing query parameters separately from the path with URL decoding.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 17

🧹 Nitpick comments (18)

pom.xml (1)

139-141: JaCoCo is configured but skipped.

JaCoCo coverage is explicitly skipped (<skip>true</skip>). If code coverage reporting is desired, remove or set this to false.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pom.xml` around lines 139 - 141, The JaCoCo plugin configuration currently
disables coverage by setting the <skip>true</skip> flag; to enable reporting
remove the <skip> element or change its value to <skip>false</skip> in the
JaCoCo plugin configuration (the plugin block containing the <configuration> and
<skip> entries, typically the jacoco-maven-plugin /
<artifactId>jacoco-maven-plugin</artifactId>), then rebuild to generate coverage
reports.

README.md (1)

39-44: Add language specifier to code blocks.

Several code blocks lack language specifiers, which affects syntax highlighting and accessibility. Consider adding text or an appropriate language identifier.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 39 - 44, The README has fenced code blocks (e.g., the
ASCII banner block shown) without a language specifier; update those blocks by
adding an appropriate language tag (for plain text or ASCII art use "text" or
"ansi") immediately after the opening triple backticks, e.g., change ``` to
```text so syntax highlighting/accessibility tools pick up the block; search for
other triple-backtick blocks in README.md and add the same fix where missing.

.github/workflows/ci.yml (1)

16-19: Verify Maven availability before JDK setup.

This step runs mvn help:evaluate before setup-java configures the JDK. This works because ubuntu-latest has Maven and a default JDK pre-installed, but it's fragile—if the runner image changes, this could break.

Consider adding a comment noting this dependency, or use a fixed Java version for the evaluation step.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 16 - 19, The workflow currently runs
`mvn help:evaluate` to set JAVA_VERSION (the `JAVA_VERSION` env var computed
from the `mvn help:evaluate "-Dexpression=maven.compiler.release"` command)
before the `setup-java` action configures the JDK, which is fragile if the
runner lacks Maven/JDK; update the CI step to either (A) ensure Maven is
available by adding a short comment documenting the implicit dependency on the
runner having Maven preinstalled, or (B) make the evaluation robust by using a
fixed Java/Maven context (e.g., set a default Java version or invoke the
`setup-java` step prior to running `mvn help:evaluate`) so that `mvn
help:evaluate` reliably succeeds when computing `JAVA_VERSION`.

src/test/java/org/example/httpparser/HttpParserTest.java (1)

16-16: Test method name should start with lowercase.

TestHttpParserForHeaders should be testHttpParserForHeaders per Java naming conventions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/example/httpparser/HttpParserTest.java` at line 16, The
test method name violates Java naming conventions: rename the method
TestHttpParserForHeaders to start with a lowercase letter
(testHttpParserForHeaders). Update the method declaration in HttpParserTest
(method currently named TestHttpParserForHeaders) and any references or
annotations that refer to it so the test framework still runs, keeping the
method signature and throws clause unchanged.

src/main/java/org/example/httpparser/HttpParser.java (1)

53-55: Exposing mutable internal map breaks encapsulation.

getHeadersMap() returns the internal HashMap directly, allowing callers to modify the parser's state unexpectedly. Return an unmodifiable view or a defensive copy.

🔒 Proposed fix

     public Map<String, String> getHeadersMap() {
-        return headersMap;
+        return Collections.unmodifiableMap(headersMap);
     }

Add the import at the top:

import java.util.Collections;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 53 - 55,
The getHeadersMap() method currently returns the internal mutable headersMap
(breaking encapsulation); change it to return either
Collections.unmodifiableMap(headersMap) or a defensive copy like new
HashMap<>(headersMap) and add the required import (java.util.Collections) if
using unmodifiableMap; ensure the method signature and return type remain
Map<String, String> and update any callers that expect to mutate the returned
map.

src/main/java/org/example/httpparser/HttpParseRequestLine.java (2)

14-16: Redundant local variable assignment.

The variable reader on line 15 is unnecessary since br can be used directly.

🔧 Suggested simplification

     public void parseHttpRequest(BufferedReader br) throws IOException {
-        BufferedReader reader = br;
-        String requestLine = reader.readLine();
+        String requestLine = br.readLine();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/httpparser/HttpParseRequestLine.java` around lines
14 - 16, In parseHttpRequest, remove the redundant local variable reader and use
the method parameter br directly; delete the assignment "BufferedReader reader =
br" and update any subsequent uses of reader (e.g., the readLine call) to use br
instead, keeping the method signature and behavior of parseHttpRequest
unchanged.

---

11-12: Unreachable debug logging code.

The debug field is private and initialized to false with no setter, making the debug logging block on lines 34-38 unreachable. Consider either removing the dead code or providing a way to enable debug mode (e.g., constructor parameter or setter).

Also applies to: 34-38

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/httpparser/HttpParseRequestLine.java` around lines
11 - 12, The private boolean field debug in class HttpParseRequestLine is
initialized to false and never mutated, making the conditional logging block
guarded by debug (lines 34-38) unreachable; either remove that dead debug-only
logging block or add a way to enable debugging (for example add a constructor
parameter or a public setDebug(boolean) setter that assigns the debug field) and
update uses accordingly; ensure references to the debug field and the logger
(Logger logger = Logger.getLogger(HttpParseRequestLine.class.getName())) are
updated so the debug logging in the request-line parsing methods can actually
run when debug is enabled.

src/test/java/org/example/httpparser/HttpParseRequestLineTest.java (2)

20-31: Consider adding test for URI with query string.

Given the PR objective to parse query parameters, consider adding a test that verifies URIs like /search?q=test&page=1 are correctly captured in the uri field.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/example/httpparser/HttpParseRequestLineTest.java` around
lines 20 - 31, Update the test method testParserWithTestRequestLine (or add a
new test) to include a request line with a query string such as "GET
/search?q=test&page=1 HTTP/1.1", feed it to the same
httpParseRequestLine.setReader(InputStream) and
httpParseRequestLine.parseRequest(), and assert that
httpParseRequestLine.getUri() returns "/search?q=test&page=1" (also keep
existing assertions for getMethod() and getVersion()); this ensures the parser
captures the full URI including query parameters.

---

24-24: Use explicit charset for getBytes() calls.

getBytes() without a charset parameter uses platform-default encoding, which may cause inconsistent test behavior across environments. Use StandardCharsets.UTF_8 for consistency.

🔧 Example fix for line 24

+import java.nio.charset.StandardCharsets;

-        InputStream in = new ByteArrayInputStream(testString.getBytes());
+        InputStream in = new ByteArrayInputStream(testString.getBytes(StandardCharsets.UTF_8));

Also applies to: 41-41, 53-53, 64-64

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/example/httpparser/HttpParseRequestLineTest.java` at line
24, Replace platform-default getBytes() calls in the HttpParseRequestLineTest
test cases with an explicit charset: change each InputStream creation that uses
new ByteArrayInputStream(testString.getBytes()) (and the other occurrences using
getBytes() at the same class) to use
testString.getBytes(StandardCharsets.UTF_8), and add the import
java.nio.charset.StandardCharsets; so all InputStream constructions in
HttpParseRequestLineTest (look for the variable testString and the
ByteArrayInputStream usages) use UTF_8 explicitly.

src/main/java/org/example/httpparser/HttpRequest.java (1)

43-45: Shallow copy of queryParams may allow external mutation of value lists.

Map.copyOf(queryParams) creates a shallow copy—the map itself is immutable, but the List<String> values remain mutable if the caller passed in mutable lists. Consider performing a deep copy to ensure full immutability.

🔒 Proposed fix for deep immutability

-        this.queryParams = queryParams != null ? Map.copyOf(queryParams) : Collections.emptyMap();
+        this.queryParams = queryParams != null
+                ? queryParams.entrySet().stream()
+                    .collect(Collectors.toUnmodifiableMap(
+                        Map.Entry::getKey,
+                        e -> List.copyOf(e.getValue())))
+                : Collections.emptyMap();

Add the import:

import java.util.stream.Collectors;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/httpparser/HttpRequest.java` around lines 43 - 45,
The constructor currently does Map.copyOf(queryParams) which leaves each
List<String> value mutable; update HttpRequest to perform a deep immutable copy
of queryParams (and similarly headers' value lists if applicable) by replacing
Map.copyOf(queryParams) with code that copies each entry and wraps each list
value in an immutable list (e.g., stream over queryParams.entrySet(), map each
key to List.copyOf(new ArrayList<>(value)) or Collections.unmodifiableList(new
ArrayList<>(value)), and collect into an unmodifiable map) so both the map and
its value lists cannot be mutated; locate the queryParams assignment in the
HttpRequest constructor and apply the deep-copy approach using
java.util.stream.Collectors and List.copyOf/Collections.unmodifiableList.

src/main/java/org/example/filter/LoggingFilter.java (1)

38-41: Fix copy-paste error in comment.

The destroy() method comment says "No initialization needed" but should say "No cleanup needed" or similar.

Proposed fix

     `@Override`
     public void destroy() {
-        //No initialization needed
+        //No cleanup needed
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/filter/LoggingFilter.java` around lines 38 - 41,
The comment above the destroy() method in class LoggingFilter is a copy-paste
error; update the Javadoc/inline comment for the method LoggingFilter.destroy()
to say "No cleanup needed" (or "No resources to clean up") instead of "No
initialization needed" so it correctly reflects that destroy performs no
cleanup.

src/main/java/org/example/App.java (1)

6-6: Remove unused import.

java.net.Socket is not used in this file.

Proposed fix

-import java.net.Socket;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/App.java` at line 6, Remove the unused import
statement for java.net.Socket from the top of the file where class App is
defined; locate the import line "import java.net.Socket;" in App.java and delete
it so only actually used imports remain (no other code changes required).

src/test/java/org/example/TcpServerTest.java (1)

23-23: Prefer importing ByteArrayOutputStream instead of using the fully qualified name.

For consistency with the other imports in this file.

Proposed fix

Add to imports:

import java.io.ByteArrayOutputStream;

Then change line 23:

-        java.io.ByteArrayOutputStream outputStream = new java.io.ByteArrayOutputStream();
+        ByteArrayOutputStream outputStream = new ByteArrayOutputStream();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/example/TcpServerTest.java` at line 23, Replace the
fully-qualified type usage with an import: add "ByteArrayOutputStream" to the
file imports and update the instantiation in TcpServerTest (the variable using
java.io.ByteArrayOutputStream) to use the simple class name
ByteArrayOutputStream so the class compiles consistently with other imports.

src/test/java/org/example/config/ConfigLoaderTest.java (1)

128-140: Fix method name formatting.

The method name has extra spaces before the parentheses which is unconventional.

Proposed fix

-    void invalid_port_should_Throw_Exception    () throws Exception {
+    void invalid_port_should_throw_exception() throws Exception {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/example/config/ConfigLoaderTest.java` around lines 128 -
140, The test method declaration invalid_port_should_Throw_Exception    ()
contains extra spaces before the parentheses; remove the extra spaces so the
method signature reads invalid_port_should_Throw_Exception() and ensure the test
method name follows the project's naming conventions (adjust to camelCase or
consistent underscore style if required) to fix the formatting issue in the test
method.

src/main/java/org/example/ConnectionHandler.java (1)

168-171: Complete filter lifecycle by calling destroy() in close().

Filters are initialized (filter.init()), but never destroyed. Calling destroy() in close() keeps lifecycle behavior consistent.

🔧 Proposed fix

 `@Override`
 public void close() throws Exception {
+    for (Filter filter : filters) {
+        filter.destroy();
+    }
     client.close();
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/ConnectionHandler.java` around lines 168 - 171, The
close() method currently only calls client.close(); update it to also call the
filter's destroy() to complete the lifecycle started by filter.init(): invoke
filter.destroy() as part of close() (ensure you reference the same filter
instance used during initialization) and order/guard calls so destroy() runs
even if client.close() throws (e.g., call filter.destroy() in a finally block or
ensure both are attempted). This will make the lifecycle consistent between
filter.init() and filter.destroy().

src/main/java/org/example/TcpServer.java (2)

50-52: Keep the original exception context when generating 500s.

Line 50 currently swallows the root cause. Add at least error logging before writing the 500 response to preserve diagnostics.

🔧 Proposed fix

 } catch (Exception e) {
+    System.err.println("Request processing failed: " + e.getMessage());
     handleInternalServerError(client);
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/TcpServer.java` around lines 50 - 52, The catch
block in TcpServer that currently calls handleInternalServerError(client)
swallows the exception; update the catch(Exception e) handler to first log the
exception (e.g., logger.error or similar) with the full exception/stacktrace and
contextual information (client id/remote address if available) before calling
handleInternalServerError(client) so the original error context is preserved for
diagnostics; reference the catch block surrounding
handleInternalServerError(...) in class TcpServer and ensure the logger call
passes the throwable (e) to capture stacktrace.

---

26-30: Consider TLS for non-local deployments.

If this server is internet-facing, plaintext ServerSocket traffic should be protected (e.g., TLS termination proxy or SSLServerSocket) to avoid cleartext exposure.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/TcpServer.java` around lines 26 - 30, The current
TcpServer uses a plain ServerSocket (in TcpServer where ServerSocket
serverSocket = new ServerSocket(port) and Thread.ofVirtual().start(() ->
handleClient(clientSocket))) which exposes cleartext traffic; replace the
ServerSocket with an SSLServerSocket created from an SSLServerSocketFactory (or
delegate TLS to a termination proxy) and ensure accepted sockets are treated as
SSLSocket before passing to handleClient, load/configure the server keystore and
TLS parameters (cipher suites/protocols) when creating the SSLServerSocket, and
keep the same accept/dispatch flow (accept -> log remote address -> start
virtual thread -> handleClient) but using the SSL socket types.

src/main/java/org/example/config/AppConfig.java (1)

74-76: Defensively copy list fields to preserve record immutability.

blockedIps/allowedIps are passed through directly. Copying with List.copyOf(...) avoids external mutation after config creation.

🔧 Proposed fix

- java.util.List<String> blocked = (blockedIps == null) ? java.util.List.of() : blockedIps;
- java.util.List<String> allowed = (allowedIps == null) ? java.util.List.of() : allowedIps;
+ java.util.List<String> blocked = (blockedIps == null) ? java.util.List.of() : java.util.List.copyOf(blockedIps);
+ java.util.List<String> allowed = (allowedIps == null) ? java.util.List.of() : java.util.List.copyOf(allowedIps);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/config/AppConfig.java` around lines 74 - 76, The
code currently assigns blocked/allowed as either an empty list or the raw
blockedIps/allowedIps which allows external mutation; update the construction so
you defensively copy the lists using List.copyOf(...) for both blocked and
allowed before passing them into the IpFilterConfig constructor (refer to the
blockedIps and allowedIps variables and the IpFilterConfig(...) call) to ensure
immutability of the created config object.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@PortConfigurationGuide.md`:
- Around line 26-29: The YAML example is invalid because the port key is not
indented under server; update the snippet so the key "port" is nested under
"server" (i.e., change the "port" line to be indented two spaces under the
"server:" mapping) so the "server" and "port" keys form a proper mapping in the
example.

In `@README.md`:
- Line 37: Heading "##  Quick Start" contains two spaces after '##' which breaks
the generated anchor; change the heading to a single space "## Quick Start" so
the anchor becomes `#quick-start` and the existing link to "#quick-start" resolves
correctly.
- Line 48: The README contains a duplicated command string "git clone git clone
https://github.com/ithsjava25/project-webserver-juv25g.git"; replace it with a
single correct invocation "git clone
https://github.com/ithsjava25/project-webserver-juv25g.git" so the clone
instruction is valid and not duplicated.
- Line 6: The README Java badge is incorrect — update the badge string from
"Java-21+" to "Java-25+" to match pom.xml's maven.compiler.release value; locate
the badge image line in README.md (the existing
![Java](https://img.shields.io/badge/Java-21+-orange?...) entry) and change the
displayed version text to "Java-25+" so the documentation reflects the actual
minimum Java requirement.

In `@src/main/java/org/example/App.java`:
- Line 14: The code in App.java currently constructs a build-time Path via
Path.of("src/main/resources/application.yml") which will break when packaged;
modify App to first check for an external config path (e.g., from a CLI arg or
env var) and use that Path if present, otherwise load the bundled
application.yml from the classpath via
App.class.getResourceAsStream("/application.yml"); update ConfigLoader (or the
class that reads config) to accept an InputStream fallback when a Path is not
provided so both external files and classpath resources are supported, and
replace usages of the hardcoded Path (configPath) accordingly.

In `@src/main/java/org/example/config/AppConfig.java`:
- Around line 23-24: Remove the Swedish inline edit notes from the production
code by deleting the comment fragments "← LÄGG TILL" and "← UPPDATERA DENNA RAD"
in the block that constructs IpFilterConfig and returns the AppConfig;
specifically, update the lines that create ipFilterConfig (using
IpFilterConfig.defaults() or ipFilter.withDefaultsApplied()) and the return
statement that instantiates new AppConfig(serverConfig, loggingConfig,
ipFilterConfig) so they contain only valid code and no editorial comments.
- Around line 3-4: AppConfig.java currently imports Jackson v2 annotations
(com.fasterxml.jackson.annotation.JsonIgnoreProperties, JsonProperty) which
mismatches the project's Jackson v3 artifacts; update the imports and annotation
usages in the AppConfig class to the v3 package namespace
(tools.jackson.annotation.* as used by ConfigLoader) or alternatively add the
Jackson v2 dependency to the build, and remove the inline Swedish edit notes "//
← LÄGG TILL" and "// ← UPPDATERA DENNA RAD" in AppConfig (the comments near the
class fields) so the file compiles cleanly.

In `@src/main/java/org/example/config/ConfigLoader.java`:
- Around line 47-50: The load(...) method returns the raw deserialized AppConfig
which can leave nested subsections null; make load(...) mirror loadOnce(...) by
applying nested defaults before returning: after objectMapper.readValue(stream,
AppConfig.class) (in ConfigLoader.load), if the result is null return
AppConfig.defaults(), otherwise merge/apply nested defaults from
AppConfig.defaults() into the deserialized instance (or call a
config.applyDefaults() helper) so all subsections are populated; keep using
objectMapper.readValue and AppConfig.defaults() as the source of defaults.

In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 157-158: The URLDecoder.decode calls in ConnectionHandler (the
lines decoding "key" and "value") can throw IllegalArgumentException for
malformed % sequences; catch IllegalArgumentException around those
URLDecoder.decode(key, StandardCharsets.UTF_8) and URLDecoder.decode(value,
StandardCharsets.UTF_8) calls and convert it into a 400 Bad Request response
instead of letting it propagate as a 500. Modify the surrounding method (in
ConnectionHandler where query parsing happens) to validate/try-catch the decode,
and on catch return or throw the handler's standard 400-level error (HTTP 400)
with a clear message about malformed query encoding.

In `@src/main/java/org/example/filter/FilterChainImpl.java`:
- Around line 17-31: The FilterChainImpl currently uses an instance-level cursor
`index` and drops the request when filters are exhausted; change doFilter to be
stateless and to invoke the terminal request handler when no filters remain:
replace the shared `index` usage by passing a local/current index (e.g., an
overloaded/private method doFilter(HttpRequest,HttpResponseBuilder,int) or use a
local variable) so each invocation advances a local cursor rather than mutating
instance state, and when the cursor >= filters.size() call the terminal
execution path (e.g., execute the actual request/handler or invoke a provided
TerminalHandler) so the request is processed instead of being dropped;
update/rename any helper methods accordingly and remove reliance on the instance
field `index`.

In `@src/main/java/org/example/filter/IpFilter.java`:
- Around line 95-107: The addBlockedIp and addAllowedIp methods currently accept
inputs that normalize to empty strings (e.g., whitespace-only) and end up
storing "" in blockedIps/allowedIps; after calling normalizeIp(ip) check the
result for empty (or blank) and throw IllegalArgumentException with a clear
message if it is empty, otherwise add the normalized value; update both
addBlockedIp and addAllowedIp to validate the normalized value before adding to
the respective collections.

In `@src/main/java/org/example/filter/LoggingFilter.java`:
- Around line 21-26: The catch block in LoggingFilter around
chain.doFilter(request, response) swallows exceptions; modify the catch to log
the exception before any status manipulation by calling the class logger (e.g.,
logger.error("Unhandled exception in doFilter", e)) and then keep the existing
logic that sets
response.setStatusCode(HttpResponseBuilder.SC_INTERNAL_SERVER_ERROR) when
response.getStatusCode() == HttpResponseBuilder.SC_OK; ensure you reference the
same symbols (chain.doFilter, response.setStatusCode,
HttpResponseBuilder.SC_INTERNAL_SERVER_ERROR) and do not remove the existing
status handling.

In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Around line 100-110: The build() method is emitting a body for responses that
must not have one (1xx, 204, 304); detect these status codes in
HttpResponseBuilder.build() (use the response status/statusCode field used in
this class) and treat them as no-body: set contentBody to an empty byte[] and
contentLength to 0, and avoid serializing/appending the body or writing any
Content-Length/Transfer-Encoding headers for those codes; apply the same guard
where body serialization is done elsewhere (the other occurrences around the
build code noted in the comment) so the builder never emits a body for 1xx, 204,
or 304.
- Around line 82-89: The setHeaders and setHeader methods currently accept null
and unvalidated header names/values which can cause NullPointerException and
CR/LF response splitting; update
HttpResponseBuilder.setHeaders(Map<String,String>) to treat a null input as an
empty map, initialize the TreeMap as now, and then validate each entry (reject
null names or values and throw IllegalArgumentException); update
HttpResponseBuilder.setHeader(String,String) to validate name and value before
putting; the validation should explicitly reject any input containing '\r' or
'\n' (and optionally control characters), trim inputs, and document the
IllegalArgumentException behavior so callers cannot inject CR/LF into header
names/values.

In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 63-65: The code in StaticFileHandler currently calls
response.setContentTypeFromFilename(uri) which bases the Content-Type on the
request URI and can be wrong for error fallbacks (e.g., when serving
pageNotFound.html); change the logic to determine MIME from the actual file
being served (use the served filename or a MimeTypeDetector that inspects the
file name/path for the chosen file, or detect from fileBytes if available) and
call response.setContentType(...) with that MIME before calling
response.setBody(fileBytes); additionally, ensure that when sending 403/404
fallback pages (the pageNotFound.html path/name used in this class) you
explicitly set "text/html" if detection is unavailable.

In `@src/test/java/org/example/ConnectionHandlerTest.java`:
- Around line 57-60: The current assertions use substring checks on the whole
response (response/ outputStream.toString()) which can yield false positives;
change them to assert the exact HTTP start-line by extracting the first line
(e.g., split response on "\r\n" or read up to the first newline) and
assertThat(firstLine).isEqualTo("HTTP/1.1 200 OK") (or the expected status
text), replacing the contains/doesNotContain checks in ConnectionHandlerTest
(update the assertions around the response variable at the shown locations and
also the analogous checks at the other two occurrences) so the test verifies the
precise start-line rather than substrings.

In `@src/test/java/org/example/httpparser/HttpParserTest.java`:
- Around line 12-13: The HttpParser instance is shared across tests causing
setReader() to only apply on the first test; change the field declaration to not
instantiate HttpParser at declaration and add a setup method annotated with
`@BeforeEach` that assigns httpParser = new HttpParser(); so each test gets a
fresh HttpParser (referencing the HttpParser field and setReader method) and
test isolation is preserved.

---

Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 16-19: The workflow currently runs `mvn help:evaluate` to set
JAVA_VERSION (the `JAVA_VERSION` env var computed from the `mvn help:evaluate
"-Dexpression=maven.compiler.release"` command) before the `setup-java` action
configures the JDK, which is fragile if the runner lacks Maven/JDK; update the
CI step to either (A) ensure Maven is available by adding a short comment
documenting the implicit dependency on the runner having Maven preinstalled, or
(B) make the evaluation robust by using a fixed Java/Maven context (e.g., set a
default Java version or invoke the `setup-java` step prior to running `mvn
help:evaluate`) so that `mvn help:evaluate` reliably succeeds when computing
`JAVA_VERSION`.

In `@pom.xml`:
- Around line 139-141: The JaCoCo plugin configuration currently disables
coverage by setting the <skip>true</skip> flag; to enable reporting remove the
<skip> element or change its value to <skip>false</skip> in the JaCoCo plugin
configuration (the plugin block containing the <configuration> and <skip>
entries, typically the jacoco-maven-plugin /
<artifactId>jacoco-maven-plugin</artifactId>), then rebuild to generate coverage
reports.

In `@README.md`:
- Around line 39-44: The README has fenced code blocks (e.g., the ASCII banner
block shown) without a language specifier; update those blocks by adding an
appropriate language tag (for plain text or ASCII art use "text" or "ansi")
immediately after the opening triple backticks, e.g., change ``` to ```text so
syntax highlighting/accessibility tools pick up the block; search for other
triple-backtick blocks in README.md and add the same fix where missing.

In `@src/main/java/org/example/App.java`:
- Line 6: Remove the unused import statement for java.net.Socket from the top of
the file where class App is defined; locate the import line "import
java.net.Socket;" in App.java and delete it so only actually used imports remain
(no other code changes required).

In `@src/main/java/org/example/config/AppConfig.java`:
- Around line 74-76: The code currently assigns blocked/allowed as either an
empty list or the raw blockedIps/allowedIps which allows external mutation;
update the construction so you defensively copy the lists using List.copyOf(...)
for both blocked and allowed before passing them into the IpFilterConfig
constructor (refer to the blockedIps and allowedIps variables and the
IpFilterConfig(...) call) to ensure immutability of the created config object.

In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 168-171: The close() method currently only calls client.close();
update it to also call the filter's destroy() to complete the lifecycle started
by filter.init(): invoke filter.destroy() as part of close() (ensure you
reference the same filter instance used during initialization) and order/guard
calls so destroy() runs even if client.close() throws (e.g., call
filter.destroy() in a finally block or ensure both are attempted). This will
make the lifecycle consistent between filter.init() and filter.destroy().

In `@src/main/java/org/example/filter/LoggingFilter.java`:
- Around line 38-41: The comment above the destroy() method in class
LoggingFilter is a copy-paste error; update the Javadoc/inline comment for the
method LoggingFilter.destroy() to say "No cleanup needed" (or "No resources to
clean up") instead of "No initialization needed" so it correctly reflects that
destroy performs no cleanup.

In `@src/main/java/org/example/httpparser/HttpParser.java`:
- Around line 53-55: The getHeadersMap() method currently returns the internal
mutable headersMap (breaking encapsulation); change it to return either
Collections.unmodifiableMap(headersMap) or a defensive copy like new
HashMap<>(headersMap) and add the required import (java.util.Collections) if
using unmodifiableMap; ensure the method signature and return type remain
Map<String, String> and update any callers that expect to mutate the returned
map.

In `@src/main/java/org/example/httpparser/HttpParseRequestLine.java`:
- Around line 14-16: In parseHttpRequest, remove the redundant local variable
reader and use the method parameter br directly; delete the assignment
"BufferedReader reader = br" and update any subsequent uses of reader (e.g., the
readLine call) to use br instead, keeping the method signature and behavior of
parseHttpRequest unchanged.
- Around line 11-12: The private boolean field debug in class
HttpParseRequestLine is initialized to false and never mutated, making the
conditional logging block guarded by debug (lines 34-38) unreachable; either
remove that dead debug-only logging block or add a way to enable debugging (for
example add a constructor parameter or a public setDebug(boolean) setter that
assigns the debug field) and update uses accordingly; ensure references to the
debug field and the logger (Logger logger =
Logger.getLogger(HttpParseRequestLine.class.getName())) are updated so the debug
logging in the request-line parsing methods can actually run when debug is
enabled.

In `@src/main/java/org/example/httpparser/HttpRequest.java`:
- Around line 43-45: The constructor currently does Map.copyOf(queryParams)
which leaves each List<String> value mutable; update HttpRequest to perform a
deep immutable copy of queryParams (and similarly headers' value lists if
applicable) by replacing Map.copyOf(queryParams) with code that copies each
entry and wraps each list value in an immutable list (e.g., stream over
queryParams.entrySet(), map each key to List.copyOf(new ArrayList<>(value)) or
Collections.unmodifiableList(new ArrayList<>(value)), and collect into an
unmodifiable map) so both the map and its value lists cannot be mutated; locate
the queryParams assignment in the HttpRequest constructor and apply the
deep-copy approach using java.util.stream.Collectors and
List.copyOf/Collections.unmodifiableList.

In `@src/main/java/org/example/TcpServer.java`:
- Around line 50-52: The catch block in TcpServer that currently calls
handleInternalServerError(client) swallows the exception; update the
catch(Exception e) handler to first log the exception (e.g., logger.error or
similar) with the full exception/stacktrace and contextual information (client
id/remote address if available) before calling handleInternalServerError(client)
so the original error context is preserved for diagnostics; reference the catch
block surrounding handleInternalServerError(...) in class TcpServer and ensure
the logger call passes the throwable (e) to capture stacktrace.
- Around line 26-30: The current TcpServer uses a plain ServerSocket (in
TcpServer where ServerSocket serverSocket = new ServerSocket(port) and
Thread.ofVirtual().start(() -> handleClient(clientSocket))) which exposes
cleartext traffic; replace the ServerSocket with an SSLServerSocket created from
an SSLServerSocketFactory (or delegate TLS to a termination proxy) and ensure
accepted sockets are treated as SSLSocket before passing to handleClient,
load/configure the server keystore and TLS parameters (cipher suites/protocols)
when creating the SSLServerSocket, and keep the same accept/dispatch flow
(accept -> log remote address -> start virtual thread -> handleClient) but using
the SSL socket types.

In `@src/test/java/org/example/config/ConfigLoaderTest.java`:
- Around line 128-140: The test method declaration
invalid_port_should_Throw_Exception    () contains extra spaces before the
parentheses; remove the extra spaces so the method signature reads
invalid_port_should_Throw_Exception() and ensure the test method name follows
the project's naming conventions (adjust to camelCase or consistent underscore
style if required) to fix the formatting issue in the test method.

In `@src/test/java/org/example/httpparser/HttpParseRequestLineTest.java`:
- Around line 20-31: Update the test method testParserWithTestRequestLine (or
add a new test) to include a request line with a query string such as "GET
/search?q=test&page=1 HTTP/1.1", feed it to the same
httpParseRequestLine.setReader(InputStream) and
httpParseRequestLine.parseRequest(), and assert that
httpParseRequestLine.getUri() returns "/search?q=test&page=1" (also keep
existing assertions for getMethod() and getVersion()); this ensures the parser
captures the full URI including query parameters.
- Line 24: Replace platform-default getBytes() calls in the
HttpParseRequestLineTest test cases with an explicit charset: change each
InputStream creation that uses new ByteArrayInputStream(testString.getBytes())
(and the other occurrences using getBytes() at the same class) to use
testString.getBytes(StandardCharsets.UTF_8), and add the import
java.nio.charset.StandardCharsets; so all InputStream constructions in
HttpParseRequestLineTest (look for the variable testString and the
ByteArrayInputStream usages) use UTF_8 explicitly.

In `@src/test/java/org/example/httpparser/HttpParserTest.java`:
- Line 16: The test method name violates Java naming conventions: rename the
method TestHttpParserForHeaders to start with a lowercase letter
(testHttpParserForHeaders). Update the method declaration in HttpParserTest
(method currently named TestHttpParserForHeaders) and any references or
annotations that refer to it so the test framework still runs, keeping the
method signature and throws clause unchanged.

In `@src/test/java/org/example/TcpServerTest.java`:
- Line 23: Replace the fully-qualified type usage with an import: add
"ByteArrayOutputStream" to the file imports and update the instantiation in
TcpServerTest (the variable using java.io.ByteArrayOutputStream) to use the
simple class name ByteArrayOutputStream so the class compiles consistently with
other imports.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5f18f91 and e4a4f90.

⛔ Files ignored due to path filters (2)

• src/main/resources/test.jpg is excluded by !**/*.jpg
• www/test.jpg is excluded by !**/*.jpg

📒 Files selected for processing (41)

• .github/workflows/ci.yml
• .github/workflows/release.yml
• Dockerfile
• PortConfigurationGuide.md
• README.md
• pom.xml
• src/main/java/org/example/App.java
• src/main/java/org/example/ConnectionFactory.java
• src/main/java/org/example/ConnectionHandler.java
• src/main/java/org/example/StaticFileHandler.java
• src/main/java/org/example/TcpServer.java
• src/main/java/org/example/config/AppConfig.java
• src/main/java/org/example/config/ConfigLoader.java
• src/main/java/org/example/filter/Filter.java
• src/main/java/org/example/filter/FilterChain.java
• src/main/java/org/example/filter/FilterChainImpl.java
• src/main/java/org/example/filter/IpFilter.java
• src/main/java/org/example/filter/LocaleFilter.java
• src/main/java/org/example/filter/LocaleFilterWithCookie.java
• src/main/java/org/example/filter/LoggingFilter.java
• src/main/java/org/example/http/HttpResponseBuilder.java
• src/main/java/org/example/http/MimeTypeDetector.java
• src/main/java/org/example/httpparser/HttpParseRequestLine.java
• src/main/java/org/example/httpparser/HttpParser.java
• src/main/java/org/example/httpparser/HttpRequest.java
• src/main/resources/application.yml
• src/test/java/org/example/AppPortResolutionTest.java
• src/test/java/org/example/ConnectionHandlerTest.java
• src/test/java/org/example/StaticFileHandlerTest.java
• src/test/java/org/example/TcpServerTest.java
• src/test/java/org/example/config/ConfigLoaderTest.java
• src/test/java/org/example/filter/IpFilterTest.java
• src/test/java/org/example/filter/LocaleFilterTest.java
• src/test/java/org/example/filter/LocaleFilterWithCookieTest.java
• src/test/java/org/example/filter/LoggingFilterTest.java
• src/test/java/org/example/http/HttpResponseBuilderTest.java
• src/test/java/org/example/http/MimeTypeDetectorTest.java
• src/test/java/org/example/httpparser/HttpParseRequestLineTest.java
• src/test/java/org/example/httpparser/HttpParserTest.java
• www/index.html
• www/pageNotFound.html

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fix YAML indentation in the example.

The port key should be indented under server: to be valid YAML.

📝 Proposed fix

 ```yaml
 server:
-port: 9090
+  port: 9090

</details>

<!-- suggestion_start -->

<details>
<summary>📝 Committable suggestion</summary>

> ‼️ **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.

```suggestion

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@PortConfigurationGuide.md` around lines 26 - 29, The YAML example is invalid
because the port key is not indented under server; update the snippet so the key
"port" is nested under "server" (i.e., change the "port" line to be indented two
spaces under the "server:" mapping) so the "server" and "port" keys form a
proper mapping in the example.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Update Java version badge to match actual requirement.

The badge shows Java-21+ but pom.xml specifies maven.compiler.release as 25. Update to reflect the actual minimum version.

📝 Proposed fix

-![Java](https://img.shields.io/badge/Java-21+-orange?style=for-the-badge&logo=openjdk)
+![Java](https://img.shields.io/badge/Java-25+-orange?style=for-the-badge&logo=openjdk)

📝 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.

Suggested change

	
	

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@README.md` at line 6, The README Java badge is incorrect — update the badge
string from "Java-21+" to "Java-25+" to match pom.xml's maven.compiler.release
value; locate the badge image line in README.md (the existing
![Java](https://img.shields.io/badge/Java-21+-orange?...) entry) and change the
displayed version text to "Java-25+" so the documentation reflects the actual
minimum Java requirement.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fix extra space in heading.

There are multiple spaces after ## which also breaks the #quick-start anchor link on line 12.

📝 Proposed fix

-##  Quick Start
+## Quick Start

📝 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.

Suggested change

	## Quick Start
	## Quick Start

🧰 Tools

🪛 markdownlint-cli2 (0.21.0)

[warning] 37-37: Multiple spaces after hash on atx style heading

(MD019, no-multiple-space-atx)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@README.md` at line 37, Heading "##  Quick Start" contains two spaces after
'##' which breaks the generated anchor; change the heading to a single space "##
Quick Start" so the anchor becomes `#quick-start` and the existing link to
"#quick-start" resolves correctly.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fix duplicate git clone command.

The clone command has git clone written twice.

📝 Proposed fix

-git clone git clone https://github.com/ithsjava25/project-webserver-juv25g.git
+git clone https://github.com/ithsjava25/project-webserver-juv25g.git

📝 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.

Suggested change

	git clone git clone https://github.com/ithsjava25/project-webserver-juv25g.git
	git clone https://github.com/ithsjava25/project-webserver-juv25g.git

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@README.md` at line 48, The README contains a duplicated command string "git
clone git clone https://github.com/ithsjava25/project-webserver-juv25g.git";
replace it with a single correct invocation "git clone
https://github.com/ithsjava25/project-webserver-juv25g.git" so the clone
instruction is valid and not duplicated.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Hardcoded path won't work in deployed JAR.

The path src/main/resources/application.yml is a build-time source path and won't exist in a packaged JAR. Consider loading from the classpath or accepting an external config path.

Proposed fix using classpath resource or external path

Option 1 - External path with fallback:

-        Path configPath = Path.of("src/main/resources/application.yml");
+        Path configPath = Path.of(
+            System.getProperty("config.path", "application.yml")
+        );

Option 2 - Load from classpath (requires ConfigLoader changes to support InputStream):

InputStream is = App.class.getResourceAsStream("/application.yml");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/App.java` at line 14, The code in App.java
currently constructs a build-time Path via
Path.of("src/main/resources/application.yml") which will break when packaged;
modify App to first check for an external config path (e.g., from a CLI arg or
env var) and use that Path if present, otherwise load the bundled
application.yml from the classpath via
App.class.getResourceAsStream("/application.yml"); update ConfigLoader (or the
class that reads config) to accept an InputStream fallback when a Path is not
provided so both external files and classpath resources are supported, and
replace usages of the hardcoded Path (configPath) accordingly.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Block CRLF header injection and validate header input.

setHeaders/setHeader accept unvalidated names/values, which are written verbatim into the response. A \r/\n in input can inject arbitrary headers/body (response splitting). Also, setHeaders(null) currently throws NullPointerException.

Suggested fix

 import java.nio.charset.StandardCharsets;
+import java.util.Objects;
 import java.util.Map;
 import java.util.TreeMap;
@@
     public void setHeaders(Map<String, String> headers) {
         this.headers = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
-        this.headers.putAll(headers);
+        if (headers == null) {
+            return;
+        }
+        headers.forEach(this::setHeader);
     }
 
     public void setHeader(String name, String value) {
+        validateHeader(name, value);
         this.headers.put(name, value);
     }
+
+    private static void validateHeader(String name, String value) {
+        Objects.requireNonNull(name, "Header name cannot be null");
+        Objects.requireNonNull(value, "Header value cannot be null");
+        if (name.indexOf('\r') >= 0 || name.indexOf('\n') >= 0 ||
+            value.indexOf('\r') >= 0 || value.indexOf('\n') >= 0) {
+            throw new IllegalArgumentException("Header contains CR/LF characters");
+        }
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/http/HttpResponseBuilder.java` around lines 82 -
89, The setHeaders and setHeader methods currently accept null and unvalidated
header names/values which can cause NullPointerException and CR/LF response
splitting; update HttpResponseBuilder.setHeaders(Map<String,String>) to treat a
null input as an empty map, initialize the TreeMap as now, and then validate
each entry (reject null names or values and throw IllegalArgumentException);
update HttpResponseBuilder.setHeader(String,String) to validate name and value
before putting; the validation should explicitly reject any input containing
'\r' or '\n' (and optionally control characters), trim inputs, and document the
IllegalArgumentException behavior so callers cannot inject CR/LF into header
names/values.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Do not emit a body for no-body status codes.

For 1xx, 204, and 304, the builder still serializes and appends body bytes. That violates HTTP semantics and can break clients/intermediaries.

Suggested fix

     public byte[] build() {
         byte[] contentBody;
         int contentLength;
@@
         if (bytebody != null) {
             contentBody = bytebody;
             contentLength = bytebody.length;
         } else {
             contentBody = body.getBytes(StandardCharsets.UTF_8);
             contentLength = contentBody.length;
         }
+
+        if ((statusCode >= 100 && statusCode < 200) || statusCode == SC_NO_CONTENT || statusCode == SC_NOT_MODIFIED) {
+            contentBody = new byte[0];
+            contentLength = 0;
+        }
@@
         byte[] response = new byte[headerBytes.length + contentBody.length];

Also applies to: 123-125, 135-137

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/http/HttpResponseBuilder.java` around lines 100 -
110, The build() method is emitting a body for responses that must not have one
(1xx, 204, 304); detect these status codes in HttpResponseBuilder.build() (use
the response status/statusCode field used in this class) and treat them as
no-body: set contentBody to an empty byte[] and contentLength to 0, and avoid
serializing/appending the body or writing any Content-Length/Transfer-Encoding
headers for those codes; apply the same guard where body serialization is done
elsewhere (the other occurrences around the build code noted in the comment) so
the builder never emits a body for 1xx, 204, or 304.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Content-Type is based on request URI, not the actual response body.

Line 64 can set an incorrect MIME type for 403/404 responses (especially when serving pageNotFound.html as fallback). This leads to mismatched headers and body.

🔧 Proposed fix

 public class StaticFileHandler {
     private final String WEB_ROOT;
     private byte[] fileBytes;
     private int statusCode;
+    private String servedName;
@@
     private void handleGetRequest(String uri) throws IOException {
@@
         if (!file.toPath().startsWith(root.toPath())) {
             fileBytes = "403 Forbidden".getBytes(java.nio.charset.StandardCharsets.UTF_8);
             statusCode = SC_FORBIDDEN;
+            servedName = null;
             return;
         }
@@
         if (file.isFile()) {
             fileBytes = Files.readAllBytes(file.toPath());
             statusCode = SC_OK;
+            servedName = file.getName();
         } else {
             File errorFile = new File(WEB_ROOT, "pageNotFound.html");
             if (errorFile.isFile()) {
                 fileBytes = Files.readAllBytes(errorFile.toPath());
+                servedName = errorFile.getName();
             } else {
                 fileBytes = "404 Not Found".getBytes(java.nio.charset.StandardCharsets.UTF_8);
+                servedName = null;
             }
             statusCode = SC_NOT_FOUND;
         }
     }
@@
         HttpResponseBuilder response = new HttpResponseBuilder();
         response.setStatusCode(statusCode);
-        response.setContentTypeFromFilename(uri);
+        if (servedName != null) {
+            response.setContentTypeFromFilename(servedName);
+        } else {
+            response.setHeader("Content-Type", "text/plain; charset=UTF-8");
+        }
         response.setBody(fileBytes);

📝 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.

Suggested change

	// Use MimeTypeDetector instead of hardcoded text/html
	response.setContentTypeFromFilename(uri);
	response.setBody(fileBytes);
	// Use MimeTypeDetector instead of hardcoded text/html
	response.setStatusCode(statusCode);
	if (servedName != null) {
	response.setContentTypeFromFilename(servedName);
	} else {
	response.setHeader("Content-Type", "text/plain; charset=UTF-8");
	}
	response.setBody(fileBytes);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/StaticFileHandler.java` around lines 63 - 65, The
code in StaticFileHandler currently calls
response.setContentTypeFromFilename(uri) which bases the Content-Type on the
request URI and can be wrong for error fallbacks (e.g., when serving
pageNotFound.html); change the logic to determine MIME from the actual file
being served (use the served filename or a MimeTypeDetector that inspects the
file name/path for the chosen file, or detect from fileBytes if available) and
call response.setContentType(...) with that MIME before calling
response.setBody(fileBytes); additionally, ensure that when sending 403/404
fallback pages (the pageNotFound.html path/name used in this class) you
explicitly set "text/html" if detection is unavailable.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Strengthen status assertions to avoid false positives.

Current checks are substring-based and can pass even if the actual status line is wrong. Assert the exact start-line instead.

✅ Proposed test assertion tightening

-        String response = outputStream.toString();
-        assertThat(response).contains("HTTP/1.1 200 OK");
-        assertThat(response).doesNotContain("404");
+        String response = outputStream.toString();
+        assertThat(response).startsWith("HTTP/1.1 200 OK\r\n");

-        String response = outputStream.toString();
-        assertThat(response).contains("HTTP/1.1 200 OK");
-        assertThat(response).doesNotContain("404");
+        String response = outputStream.toString();
+        assertThat(response).startsWith("HTTP/1.1 200 OK\r\n");

-        String response = outputStream.toString();
-        assertThat(response).contains("404");
+        String response = outputStream.toString();
+        assertThat(response).startsWith("HTTP/1.1 404 Not Found\r\n");

Also applies to: 82-85, 104-106

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/example/ConnectionHandlerTest.java` around lines 57 - 60,
The current assertions use substring checks on the whole response (response/
outputStream.toString()) which can yield false positives; change them to assert
the exact HTTP start-line by extracting the first line (e.g., split response on
"\r\n" or read up to the first newline) and
assertThat(firstLine).isEqualTo("HTTP/1.1 200 OK") (or the expected status
text), replacing the contains/doesNotContain checks in ConnectionHandlerTest
(update the assertions around the response variable at the shown locations and
also the analogous checks at the other two occurrences) so the test verifies the
precise start-line rather than substrings.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Shared parser instance causes test isolation failure.

The HttpParser instance is created once at field initialization and reused across tests. Since setReader() only sets the reader if it's currently null, subsequent tests will silently use the first test's reader. Move initialization to a @BeforeEach method to ensure each test gets a fresh instance.

🐛 Proposed fix

+import org.junit.jupiter.api.BeforeEach;

 class HttpParserTest {
-    private HttpParser httpParser = new HttpParser();
+    private HttpParser httpParser;
+
+    `@BeforeEach`
+    void setUp() {
+        httpParser = new HttpParser();
+    }

📝 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.

Suggested change

	class HttpParserTest {
	private HttpParser httpParser = new HttpParser();
	import org.junit.jupiter.api.BeforeEach;
	class HttpParserTest {
	private HttpParser httpParser;
	`@BeforeEach`
	void setUp() {
	httpParser = new HttpParser();
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/example/httpparser/HttpParserTest.java` around lines 12 -
13, The HttpParser instance is shared across tests causing setReader() to only
apply on the first test; change the field declaration to not instantiate
HttpParser at declaration and add a setup method annotated with `@BeforeEach` that
assigns httpParser = new HttpParser(); so each test gets a fresh HttpParser
(referencing the HttpParser field and setReader method) and test isolation is
preserved.

[coderabbitai]

🧹 Nitpick comments (1)

src/main/java/org/example/ConnectionHandler.java (1)

25-28: Consider improving field encapsulation and removing mutable state.

• Fields client and webRoot should be private final since they're set in constructors and shouldn't change.
• The uri field is used as mutable shared state between resolveTargetFile() and runConnectionHandler(). Consider returning the resolved path from resolveTargetFile() instead.

♻️ Suggested improvement

-    Socket client;
-    String uri;
+    private final Socket client;
     private final List<Filter> filters;
-    String webRoot;
+    private final String webRoot;

And refactor resolveTargetFile to return the value:

private String resolveTargetFile(String path) {
    if (path == null || "/".equals(path)) {
        return "index.html";
    }
    return path.startsWith("/") ? path.substring(1) : path;
}

Then in runConnectionHandler:

String resolvedUri = resolveTargetFile(request.getPath());
sfh.sendGetRequest(client.getOutputStream(), resolvedUri);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/ConnectionHandler.java` around lines 25 - 28, Make
client and webRoot immutable and remove shared mutable uri: change the fields
client and webRoot in ConnectionHandler to private final and eliminate the
mutable uri field; refactor resolveTargetFile() to accept the raw path and
return the resolved String (e.g., return "index.html" for null or "/" and strip
leading '/'), then update runConnectionHandler() to call
resolveTargetFile(request.getPath()) and use the returned resolvedUri when
calling sfh.sendGetRequest(client.getOutputStream(), resolvedUri). Ensure all
constructors initialize the new final fields and remove any remaining uses of
the old uri field.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 25-28: Make client and webRoot immutable and remove shared mutable
uri: change the fields client and webRoot in ConnectionHandler to private final
and eliminate the mutable uri field; refactor resolveTargetFile() to accept the
raw path and return the resolved String (e.g., return "index.html" for null or
"/" and strip leading '/'), then update runConnectionHandler() to call
resolveTargetFile(request.getPath()) and use the returned resolvedUri when
calling sfh.sendGetRequest(client.getOutputStream(), resolvedUri). Ensure all
constructors initialize the new final fields and remove any remaining uses of
the old uri field.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e4a4f90 and c0c9ca6.

📒 Files selected for processing (1)

• src/main/java/org/example/ConnectionHandler.java