Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
name: Java CI with Maven

on:
push:
branches: [ "main" ]
pull_request:
branches: [ "main" ]

jobs:
build:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v4

- name: Get Java Version
run: |
JAVA_VERSION=$(mvn help:evaluate "-Dexpression=maven.compiler.release" -q -DforceStdout)
echo "JAVA_VERSION=$JAVA_VERSION" >> $GITHUB_ENV

- name: Set up JDK ${{ env.JAVA_VERSION }}
uses: actions/setup-java@v4
with:
java-version: ${{ env.JAVA_VERSION }}
distribution: 'temurin'
cache: maven

- name: Compile with Maven
run: mvn -B compile --file pom.xml

- name: Test with Maven
run: mvn -B test --file pom.xml
Comment on lines +31 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

mvn -B test stops before the verify phase — Spotless and Failsafe never execute in CI.

The spotless:check goal is bound to verify in pom.xml (line 177) and integration tests use maven-failsafe-plugin which also runs in verify. Neither will fire with mvn test. Consider updating to mvn -B verify (or at a minimum mvn -B test spotless:check) to enforce formatting and run integration tests in CI.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 31 - 32, The CI step named "Test with
Maven" currently runs "mvn -B test --file pom.xml", which stops before the
verify phase so plugins bound to verify (e.g., spotless:check and
maven-failsafe-plugin) never run; update the command used in that step to run
the verify phase (e.g., use "mvn -B verify --file pom.xml") or at minimum append
the spotless check ("mvn -B test spotless:check --file pom.xml") so that
spotless:check and integration tests executed by maven-failsafe-plugin are
enforced in CI.

35 changes: 35 additions & 0 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
name: Publish Docker Image to Github Packages on Release
on:
release:
types:
- published
jobs:
publish:
runs-on: ubuntu-latest
permissions:
contents: read
packages: write
steps:
- uses: actions/checkout@v6.0.2
- uses: docker/setup-qemu-action@v3.7.0
- uses: docker/setup-buildx-action@v3.12.0
- name: Log in to GHCR
uses: docker/login-action@v3.7.0
with:
registry: ghcr.io
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
- name: Extract metadata
id: meta
uses: docker/metadata-action@v5.10.0
with:
images: ghcr.io/ithsjava25/webserver
- name: Build and push
uses: docker/build-push-action@v6.18.0
with:
context: .
push: true
platforms: linux/amd64, linux/arm64
tags: ${{ steps.meta.outputs.tags }}
labels: ${{ steps.meta.outputs.labels }}

11 changes: 11 additions & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
FROM maven:3-eclipse-temurin-25-alpine AS build
WORKDIR /build
COPY src/ src/
COPY pom.xml pom.xml
RUN mvn compile
Comment on lines +3 to +5
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

www/ static assets are never copied into the image — StaticFileHandler will always fail.

The build stage only copies src/ and pom.xml, and the runtime stage only copies compiled classes. StaticFileHandler resolves files under the relative path "www" at runtime, but that directory is not present in the image, so every static-file request will throw an IOException.

mvn compile also only compiles sources; dependency JARs remain in ~/.m2 and are not staged under /build/target/. At runtime -classpath /app contains only your compiled classes — any reference to bucket4j or Jackson classes will cause a NoClassDefFoundError at startup.

🔧 Proposed fix
 FROM maven:3-eclipse-temurin-25-alpine AS build
 WORKDIR /build
 COPY src/ src/
+COPY www/ www/
 COPY pom.xml pom.xml
-RUN mvn compile
+RUN mvn package -DskipTests
+RUN mvn dependency:copy-dependencies -DoutputDirectory=target/dependency

 FROM eclipse-temurin:25-jre-alpine
+WORKDIR /app
 RUN addgroup -S appgroup && adduser -S appuser -G appgroup
 COPY --from=build /build/target/classes/ /app/
+COPY --from=build /build/target/dependency/ /app/libs/
+COPY --from=build /build/www/ /app/www/
-ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"]
+ENTRYPOINT ["java", "-classpath", "/app:/app/libs/*", "org.example.App"]
 USER appuser
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Dockerfile` around lines 3 - 5, The Dockerfile fails to include static assets
and dependencies: ensure the build stage packages the app with dependencies and
copies the www static directory into the final image; replace mvn compile with a
full package (e.g., mvn -DskipTests package) so target/*.jar contains all
compiled classes and dependency jars (or use the maven-assembly/shade plugin),
COPY the produced jar into the runtime image, and add COPY www/ www/ (or COPY
--from=build /build/target/www/ www/) so StaticFileHandler can find "www"; also
adjust the runtime classpath to run the packaged jar (java -jar
/app/app-with-deps.jar) instead of relying on /app classpath.


FROM eclipse-temurin:25-jre-alpine
RUN addgroup -S appgroup && adduser -S appuser -G appgroup
COPY --from=build /build/target/classes/ /app/
ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"]
USER appuser
Comment on lines +10 to +11
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

USER placed after ENTRYPOINT — non-idiomatic order.

While Docker applies the final USER instruction as the container's default user regardless of where it appears relative to ENTRYPOINT/CMD, convention is to place USER before ENTRYPOINT for clarity.

🔧 Proposed fix
 COPY --from=build /build/target/classes/ /app/
+USER appuser
 ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"]
-USER appuser
📝 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
ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"]
USER appuser
USER appuser
ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Dockerfile` around lines 10 - 11, Move the USER instruction so it appears
before the ENTRYPOINT for clarity and convention: place USER appuser above
ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"] (ensure any file
permissions or ownership for /app and runtime artifacts are already set for
appuser before switching users). Update the Dockerfile so USER precedes
ENTRYPOINT and verify org.example.App runs correctly as appuser.

67 changes: 66 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,21 @@
<version>1.0-SNAPSHOT</version>

<properties>
<maven.compiler.release>23</maven.compiler.release>
<maven.compiler.release>25</maven.compiler.release>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<junit.jupiter.version>6.0.2</junit.jupiter.version>
<assertj.core.version>3.27.7</assertj.core.version>
<mockito.version>5.21.0</mockito.version>
<bucket4j.version>8.14.0</bucket4j.version>

</properties>

<dependencies>
<dependency>
<groupId>com.bucket4j</groupId>
<artifactId>bucket4j_jdk17-core</artifactId>
<version>${bucket4j.version}</version>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter</artifactId>
Expand All @@ -28,12 +36,36 @@
<version>${assertj.core.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>${mockito.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-junit-jupiter</artifactId>
<version>${mockito.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.awaitility</groupId>
<artifactId>awaitility</artifactId>
<version>4.3.0</version>
<scope>test</scope>
</dependency>

<dependency>
<groupId>tools.jackson.core</groupId>
<artifactId>jackson-databind</artifactId>
<version>3.0.3</version>
</dependency>
<dependency>
<groupId>tools.jackson.dataformat</groupId>
<artifactId>jackson-dataformat-yaml</artifactId>
<version>3.0.3</version>
</dependency>

</dependencies>
<build>
<plugins>
Expand Down Expand Up @@ -118,6 +150,39 @@
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.pitest</groupId>
<artifactId>pitest-maven</artifactId>
<version>1.22.0</version>
<dependencies>
<dependency>
<groupId>org.pitest</groupId>
<artifactId>pitest-junit5-plugin</artifactId>
<version>1.2.2</version>
</dependency>
</dependencies>
</plugin>
<plugin>
<groupId>com.diffplug.spotless</groupId>
<artifactId>spotless-maven-plugin</artifactId>
<version>3.2.1</version>
<configuration>
<java>
<removeUnusedImports/>
<formatAnnotations/>
</java>
</configuration>
<executions>
<execution>
<phase>verify</phase>
<goals>
<goal>
check
</goal>
</goals>
</execution>
</executions>
Comment on lines +175 to +184
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

spotless:check is bound to verify but CI only runs mvn test — style is never enforced.

The Spotless execution is bound to the verify phase (Line 177), but .github/workflows/ci.yml runs mvn -B test which stops before verify. This means Spotless will silently pass on every CI run regardless of formatting violations.

Either bind Spotless to the test phase or update the CI command to mvn -B verify.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pom.xml` around lines 175 - 184, The Spotless execution is bound to the
verify phase (spotless:check goal inside the <execution> with
<phase>verify</phase>) but CI invokes mvn -B test so formatting isn't enforced;
either change the <phase> in the pom.xml execution from verify to test so
spotless:check runs during mvn test, or leave the pom as-is and update the CI
invocation to run mvn -B verify instead; locate the <execution> block containing
the check goal and modify the <phase> value or update the CI command
accordingly.

</plugin>
</plugins>
</build>
</project>
4 changes: 2 additions & 2 deletions src/main/java/org/example/App.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@

public class App {
public static void main(String[] args) {
System.out.println("Hello There!");
new TcpServer(8080).start();
}
}
}
58 changes: 58 additions & 0 deletions src/main/java/org/example/ConnectionHandler.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package org.example;

import org.example.httpparser.HttpParser;

import java.io.IOException;
import java.net.Socket;

public class ConnectionHandler implements AutoCloseable {

Socket client;
String uri;

public ConnectionHandler(Socket client) {
this.client = client;
}

public void runConnectionHandler() throws IOException {
StaticFileHandler sfh = new StaticFileHandler();
HttpParser parser = new HttpParser();
parser.setReader(client.getInputStream());
parser.parseRequest();
parser.parseHttp();

// --- DIN ÄNDRING FÖR ISSUE #75 BÖRJAR HÄR ---
String requestedUri = parser.getUri();
if (requestedUri.equals("/health")) {
String responseBody = "{\"status\": \"ok\"}";
String header = "HTTP/1.1 200 OK\r\n" +
"Content-Type: application/json\r\n" +
"Content-Length: " + responseBody.length() + "\r\n" +
"\r\n";

client.getOutputStream().write(header.getBytes());
client.getOutputStream().write(responseBody.getBytes());
client.getOutputStream().flush();
return; // Avslutar här så vi inte letar efter filer i onödan
}
// --- DIN ÄNDRING SLUTAR HÄR ---
Comment on lines +24 to +38
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove developer scaffolding comments before merge.

The Swedish // --- DIN ÄNDRING ... --- and // Avslutar här ... comments are instructional notes, not production documentation, and should be removed.

🤖 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 24 - 38,
Remove the developer scaffolding and instructional Swedish comments around the
health-check handling in ConnectionHandler; keep the health-check logic intact
(the block that sets requestedUri = parser.getUri(), checks
requestedUri.equals("/health"), builds responseBody/header, writes to
client.getOutputStream(), flushes, and returns) but delete the comment lines
starting with "// --- DIN ÄNDRING..." and the inline "// Avslutar här..." so
only production-relevant comments (if any) remain.


resolveTargetFile(requestedUri);
sfh.sendGetRequest(client.getOutputStream(), uri);
}

private void resolveTargetFile(String uri) {
if (uri.matches("/$")) { //matches(/)
this.uri = "index.html";
} else if (uri.matches("^(?!.*\\.html$).*$")) {
this.uri = uri.concat(".html");
} else {
this.uri = uri;
}
Comment on lines +44 to +51
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Locate StaticFileHandler and inspect how it opens/reads the file
fd -e java -g "StaticFileHandler.java" --exec cat {}

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 1079


Fix path traversal vulnerability in StaticFileHandler.

The vulnerability claim is confirmed. StaticFileHandler.sendGetRequest() constructs a file path using new File(WEB_ROOT, uri) without normalizing or validating the URI. Combined with resolveTargetFile() in ConnectionHandler which adds .html but does not sanitize ../ sequences, a request like GET /../../etc/passwd produces the target path ../../etc/passwd.html, which—when passed to Files.readAllBytes() and resolved by the OS—escapes the web root and exposes arbitrary files.

Resolve this by normalizing the path in StaticFileHandler and rejecting any attempt to traverse outside the web root:

Path normalizedPath = Paths.get(WEB_ROOT).resolve(uri).normalize();
if (!normalizedPath.startsWith(Paths.get(WEB_ROOT))) {
    throw new IOException("Access denied: path traversal detected");
}
File file = normalizedPath.toFile();
🤖 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 44 - 51,
ConnectionHandler.resolveTargetFile currently appends “.html” but does not strip
or validate ../ sequences, and StaticFileHandler.sendGetRequest constructs new
File(WEB_ROOT, uri) without normalization, allowing path traversal; fix by
normalizing and validating the resolved path inside
StaticFileHandler.sendGetRequest (use
Paths.get(WEB_ROOT).resolve(uri).normalize()), verify the normalized path
startsWith(Paths.get(WEB_ROOT)) and reject with an IOException or access-denied
response if it does not, and also ensure ConnectionHandler.resolveTargetFile
normalizes or rejects suspicious URIs (e.g., remove leading slash and disallow
any .. segments) before returning the final uri so both resolveTargetFile and
sendGetRequest cannot be tricked by traversal sequences.

}

@Override
public void close() throws Exception {
client.close();
}
}
35 changes: 35 additions & 0 deletions src/main/java/org/example/StaticFileHandler.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package org.example;

import org.example.http.HttpResponseBuilder;

import java.io.File;
import java.io.IOException;
import java.io.OutputStream;
import java.io.PrintWriter;
import java.nio.file.Files;
import java.util.Map;

public class StaticFileHandler {
private static final String WEB_ROOT = "www";
private byte[] fileBytes;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

fileBytes as an instance field creates a race condition under concurrent virtual threads.

If the same StaticFileHandler instance is reused across multiple connection handlers (e.g., as a shared singleton), two concurrent requests will call handleGetRequest() simultaneously. The second call overwrites fileBytes before the first call's sendGetRequest() finishes reading it — serving the wrong content to the first client.

Move fileBytes to a local variable in sendGetRequest:

♻️ Proposed fix
 public class StaticFileHandler {
     private static final String WEB_ROOT = "www";
-    private byte[] fileBytes;

-    private void handleGetRequest(String uri) throws IOException {
-        File file = new File(WEB_ROOT, uri);
-        fileBytes = Files.readAllBytes(file.toPath());
-    }

     public void sendGetRequest(OutputStream outputStream, String uri) throws IOException {
-        handleGetRequest(uri);
+        File file = new File(WEB_ROOT, uri);
+        byte[] fileBytes = Files.readAllBytes(file.toPath());
         HttpResponseBuilder response = new HttpResponseBuilder();
         response.setHeaders(Map.of("Content-Type", "text/html; 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` at line 14, The instance
field fileBytes in StaticFileHandler causes a race when
handleGetRequest/sendGetRequest are called concurrently; remove the fileBytes
field and instead allocate a local byte[] inside sendGetRequest (and return or
use it directly within that call path), update handleGetRequest/sendGetRequest
signatures/usage as needed so each request works with its own local byte[]
(referencing fileBytes, StaticFileHandler, handleGetRequest, and sendGetRequest
to locate the change), and ensure no other methods rely on the removed instance
field.


public StaticFileHandler(){}

private void handleGetRequest(String uri) throws IOException {

File file = new File(WEB_ROOT, uri);
fileBytes = Files.readAllBytes(file.toPath());

Comment on lines +18 to +22
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Path traversal vulnerability — user-controlled URI is not sanitised before file access.

new File(WEB_ROOT, uri) concatenates the user-supplied URI directly onto the web root. A URI like ../../etc/passwd or %2F..%2F..%2Fetc%2Fpasswd resolves to a path outside www/, giving the caller arbitrary read access to the filesystem.

Always compare the canonical path of the resolved file against the canonical path of WEB_ROOT:

🔒 Proposed fix
 private void handleGetRequest(String uri) throws IOException {
-    File file = new File(WEB_ROOT, uri);
+    File root = new File(WEB_ROOT).getCanonicalFile();
+    File file = new File(root, uri).getCanonicalFile();
+    if (!file.getPath().startsWith(root.getPath() + File.separator)
+            && !file.getPath().equals(root.getPath())) {
+        throw new IOException("Forbidden path: " + uri);
+    }
     fileBytes = Files.readAllBytes(file.toPath());
 }
📝 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
private void handleGetRequest(String uri) throws IOException {
File file = new File(WEB_ROOT, uri);
fileBytes = Files.readAllBytes(file.toPath());
private void handleGetRequest(String uri) throws IOException {
File root = new File(WEB_ROOT).getCanonicalFile();
File file = new File(root, uri).getCanonicalFile();
if (!file.getPath().startsWith(root.getPath() + File.separator)
&& !file.getPath().equals(root.getPath())) {
throw new IOException("Forbidden path: " + uri);
}
fileBytes = Files.readAllBytes(file.toPath());
🤖 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 18 - 22, The
handleGetRequest method currently constructs new File(WEB_ROOT, uri) directly,
allowing path traversal; fix by URL-decoding the incoming uri, resolving the
candidate File's canonical path and comparing it against the canonical path of
WEB_ROOT to ensure the file is inside the web root (reject request if it doesn't
startWith the WEB_ROOT canonical path), and only then read bytes into fileBytes
after verifying file.isFile() and file.canRead(); update handleGetRequest to
return an error/404/403 instead of reading files outside the web root.

}

public void sendGetRequest(OutputStream outputStream, String uri) throws IOException{
handleGetRequest(uri);
HttpResponseBuilder response = new HttpResponseBuilder();
response.setHeaders(Map.of("Content-Type", "text/html; charset=utf-8"));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Content-Type is hardcoded to text/html regardless of file extension.

Every file — CSS, JavaScript, JSON, images — will be served as text/html; charset=utf-8. Browsers will misinterpret non-HTML resources, breaking stylesheets, scripts, and images. Derive the MIME type from the URI extension (e.g., via Files.probeContentType() or a manual lookup map) before setting the Content-Type header.

🤖 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` at line 28, The response
currently hardcodes Content-Type to "text/html; charset=utf-8" in
StaticFileHandler (where response.setHeaders(Map.of(...)) is called); change
this to compute the MIME type from the requested URI/file before calling
response.setHeaders: use java.nio.file.Files.probeContentType(path) (or a small
extension->mime lookup map) to determine the contentType, append a charset
(e.g., "; charset=utf-8") only for text/* types, and fall back to
"application/octet-stream" (or "text/plain; charset=utf-8") if probeContentType
returns null; then set response.setHeaders(Map.of("Content-Type",
computedContentType)).

response.setBody(fileBytes);
PrintWriter writer = new PrintWriter(outputStream, true);
writer.println(response.build());
Comment on lines +30 to +31
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

PrintWriter uses the platform default encoding and println() appends a spurious newline after the response.

Two distinct bugs here:

  1. new PrintWriter(outputStream, true) encodes characters using the JVM's platform default charset (not guaranteed to be UTF-8). If the body or headers contain non-ASCII characters, the bytes written to the socket will not match the Content-Length computed in UTF-8.
  2. writer.println(response.build()) appends a platform line-separator (\n on Linux) after the complete HTTP response, including after the already-written body. This adds stray bytes beyond the declared Content-Length, which can confuse strict HTTP clients.

Write the response directly as UTF-8 bytes instead:

🐛 Proposed fix
-        PrintWriter writer = new PrintWriter(outputStream, true);
-        writer.println(response.build());
+        outputStream.write(response.build().getBytes(java.nio.charset.StandardCharsets.UTF_8));
+        outputStream.flush();
📝 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
PrintWriter writer = new PrintWriter(outputStream, true);
writer.println(response.build());
outputStream.write(response.build().getBytes(java.nio.charset.StandardCharsets.UTF_8));
outputStream.flush();
🤖 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 30 - 31, The
code uses PrintWriter (new PrintWriter(outputStream, true)) and
println(response.build()), which uses platform default encoding and appends a
newline; replace this by encoding the full response String to UTF-8 bytes (e.g.,
response.build().getBytes(StandardCharsets.UTF_8)), write those bytes directly
to outputStream and flush so no extra newline is added, and ensure the
Content-Length calculation (wherever set) uses the UTF-8 byte length to match
what you write; update references in StaticFileHandler where PrintWriter and
println are used.


}

}
36 changes: 36 additions & 0 deletions src/main/java/org/example/TcpServer.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package org.example;

import java.io.IOException;
import java.net.ServerSocket;
import java.net.Socket;

public class TcpServer {

private final int port;

public TcpServer(int port) {
this.port = port;
}

public void start() {
System.out.println("Starting TCP server on port " + port);

try (ServerSocket serverSocket = new ServerSocket(port)) {
while (true) {
Socket clientSocket = serverSocket.accept(); // block
System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress());
Thread.ofVirtual().start(() -> handleClient(clientSocket));
}
} catch (IOException e) {
throw new RuntimeException("Failed to start TCP server", e);
}
}

private void handleClient(Socket client) {
try (ConnectionHandler connectionHandler = new ConnectionHandler(client)) {
connectionHandler.runConnectionHandler();
} catch (Exception e) {
throw new RuntimeException("Error handling client connection " + e);
}
Comment on lines +22 to +34
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Two related exception-handling problems.

  1. Swallowed virtual-thread exceptions (Line 22): A RuntimeException thrown inside the lambda propagates to the virtual thread's uncaught-exception handler, which by default merely prints to stderr. The server continues accepting connections with no observable failure signal. At minimum, set an explicit uncaught-exception handler on the virtual thread.

  2. Lost cause (Line 33): "Error handling client connection " + e converts the exception to its toString() representation and discards the original stack trace. Pass it as the cause argument instead.

🔧 Proposed fix
-                Thread.ofVirtual().start(() -> handleClient(clientSocket));
+                Thread.ofVirtual()
+                        .uncaughtExceptionHandler((t, ex) ->
+                                logger.log(Level.SEVERE, "Unhandled error in client thread", ex))
+                        .start(() -> handleClient(clientSocket));
-            throw new RuntimeException("Error handling client connection " + e);
+            throw new RuntimeException("Error handling client connection", e);
🤖 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 22 - 34, The
virtual-thread lambda started with Thread.ofVirtual().start(() ->
handleClient(clientSocket)) currently lets exceptions go to the default
uncaught-exception handler; change thread creation to set an explicit
uncaught-exception handler (via Thread.ofVirtual().unstarted()/builder or
Thread.currentThread().setUncaughtExceptionHandler on the created thread) that
logs the error and any context so failures are observable, and update the catch
in handleClient(Socket) to throw new RuntimeException("Error handling client
connection", e) (pass the caught exception as the cause rather than
concatenating it) so the original stack trace from
ConnectionHandler.runConnectionHandler() is preserved.

}
}
62 changes: 62 additions & 0 deletions src/main/java/org/example/http/HttpResponseBuilder.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package org.example.http;
//
import java.nio.charset.StandardCharsets;
import java.util.LinkedHashMap;
import java.util.Map;

public class HttpResponseBuilder {

private static final String PROTOCOL = "HTTP/1.1";
private int statusCode = 200;
private String body = "";
private byte[] bytebody;
private Map<String, String> headers = new LinkedHashMap<>();

private static final String CRLF = "\r\n";


public void setStatusCode(int statusCode) {
this.statusCode = statusCode;
}
public void setBody(String body) {
this.body = body != null ? body : "";
}

public void setBody(byte[] body) {
this.bytebody = body;
}
public void setHeaders(Map<String, String> headers) {
this.headers = new LinkedHashMap<>(headers);
}

private static final Map<Integer, String> REASON_PHRASES = Map.of(
200, "OK",
201, "Created",
400, "Bad Request",
404, "Not Found",
500, "Internal Server Error");
public String build(){
StringBuilder sb = new StringBuilder();
int contentLength;
if(body.isEmpty() && bytebody != null){
contentLength = bytebody.length;
setBody(new String(bytebody, StandardCharsets.UTF_8));
}else{
contentLength = body.getBytes(StandardCharsets.UTF_8).length;
}


String reason = REASON_PHRASES.getOrDefault(statusCode, "OK");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Unknown status codes fall back to "OK" — misleading reason phrase.

REASON_PHRASES.getOrDefault(statusCode, "OK") will emit HTTP/1.1 503 OK for any unregistered code. Common missing entries include 301 Moved Permanently, 302 Found, 403 Forbidden, 429 Too Many Requests, and 503 Service Unavailable. Consider using "Unknown" as the fallback and expanding the map, or computing the phrase from a more complete lookup.

🤖 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` at line 49, The code
in HttpResponseBuilder uses REASON_PHRASES.getOrDefault(statusCode, "OK") which
can produce incorrect reason phrases (e.g., "503 OK"); change the default
fallback to a neutral value like "Unknown" and update the REASON_PHRASES map to
include missing common codes (301, 302, 403, 429, 503) or replace the map lookup
with a more complete lookup routine; locate the REASON_PHRASES map and the line
that sets the local variable reason in HttpResponseBuilder and adjust the
getOrDefault fallback and/or enrich the map entries accordingly.

sb.append(PROTOCOL).append(" ").append(statusCode).append(" ").append(reason).append(CRLF);
headers.forEach((k,v) -> sb.append(k).append(": ").append(v).append(CRLF));
sb.append("Content-Length: ")
.append(contentLength);
sb.append(CRLF);
sb.append("Connection: close").append(CRLF);
sb.append(CRLF);
sb.append(body);
return sb.toString();

}
Comment on lines +38 to +60
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

build() has a state-mutating side effect — it is not idempotent.

Line 43 calls setBody(new String(bytebody, StandardCharsets.UTF_8)), which permanently sets this.body. The consequences are:

  1. Calling build() twice behaves differently: the second call skips the if-branch (body is now non-empty), recomputes contentLength from the string's UTF-8 byte length, and may produce a different Content-Length value.
  2. Setting both bodies — if a caller calls setBody(String) first (so body is non-empty) and then setBody(byte[]), the bytebody is silently ignored on build() because the if-guard requires body.isEmpty().

Refactor build() to use local variables only, leaving the builder's state untouched:

♻️ Proposed fix
 public String build(){
     StringBuilder sb = new StringBuilder();
     int contentLength;
+    String bodyToWrite;
     if(body.isEmpty() && bytebody != null){
         contentLength = bytebody.length;
-        setBody(new String(bytebody, StandardCharsets.UTF_8));
+        bodyToWrite = new String(bytebody, StandardCharsets.UTF_8);
     }else{
         contentLength = body.getBytes(StandardCharsets.UTF_8).length;
+        bodyToWrite = body;
     }

     String reason = REASON_PHRASES.getOrDefault(statusCode, "OK");
     sb.append(PROTOCOL).append(" ").append(statusCode).append(" ").append(reason).append(CRLF);
     headers.forEach((k,v) -> sb.append(k).append(": ").append(v).append(CRLF));
     sb.append("Content-Length: ").append(contentLength);
     sb.append(CRLF);
     sb.append("Connection: close").append(CRLF);
     sb.append(CRLF);
-    sb.append(body);
+    sb.append(bodyToWrite);
     return sb.toString();
 }
🤖 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 38 -
60, The build() method currently mutates object state by calling setBody(...)
when bytebody is present; instead, compute local variables (e.g., byte[]
effectiveBytes and String effectiveBody) without altering this.body or
this.bytebody: if bytebody != null use effectiveBytes = bytebody and
effectiveBody = new String(bytebody, StandardCharsets.UTF_8), otherwise use
effectiveBody = this.body and effectiveBytes =
effectiveBody.getBytes(StandardCharsets.UTF_8); compute contentLength from
effectiveBytes.length, write headers and effectiveBody into the StringBuilder,
and never call setBody or modify fields so build() is idempotent (refer to
build(), setBody, body, bytebody, and contentLength in the diff).


}
Loading