Implement securityheadersfilter to harden http responses

[johanbriger]

This PR introduces a new global filter, SecurityHeadersFilter, to the server's pipeline. The purpose is to strengthen the security of all outgoing HTTP responses by injecting standard security headers.

Changes:
SecurityHeadersFilter: A new filter class that adds protection against common web vulnerabilities.

Pipeline Integration: Registered as a global filter in App.java to ensure it covers all routes.

Headers Added:
X-Content-Type-Options: nosniff – Prevents MIME-type sniffing.

X-Frame-Options: DENY – Protects against Clickjacking.

X-XSS-Protection: 1; mode=block – Enables the browser's cross-site scripting filter.

Referrer-Policy: no-referrer – Protects privacy by limiting referrer information.

How to Test:
Start the server.

Visit any route (e.g., localhost:8080/).

Open Browser Developer Tools -> Network Tab.

Inspect the Response Headers for the request and verify the new headers are present.

Summary by CodeRabbit

• New Features

  • Full HTTP server with middleware pipeline, global/route filters, static file serving, MIME detection, redirects, IP filtering, rate limiting, security headers, and request logging.
  • YAML-driven configuration and improved server startup behavior.

• Infrastructure & Deployment

  • Docker support and multi-arch image publishing.
  • CI workflow for build, test, and formatting checks.

• Documentation

  • Major README overhaul and new ADRs/templates and usage guides.

• Tests

  • Extensive unit and integration tests added.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR converts the repo into a full Java HTTP server: adds HTTP parsing/serialization, connection handling using virtual threads, a filter pipeline with multiple filters (security, logging, rate-limiting, redirects, IP controls), router/plugin model, static file serving, CI/CD and Docker workflows, Maven wrapper, extensive docs, and comprehensive tests.

Changes

Cohort / File(s)	Summary
Build & CI/CD /.dockerignore, /.editorconfig, .github/workflows/ci.yml, .github/workflows/docker-release.yml, /.gitignore, /.mvn/wrapper/maven-wrapper.properties, Dockerfile, mvnw, mvnw.cmd, pom.xml	Adds CI and release workflows, multi-arch Docker build, Maven Wrapper, formatting rules, updated pom (Java 25, new deps/plugins), and repo/build ignore rules. Review CI secrets, GHCR login, and Maven wrapper properties.
Application entry & server bootstrap src/main/java/org/juv25d/App.java, src/main/java/org/juv25d/Server.java, src/main/java/org/juv25d/ConnectionHandler.java, src/main/java/org/juv25d/ConnectionHandlerFactory.java, src/main/java/org/juv25d/DefaultConnectionHandlerFactory.java	Introduces application main, server accept loop (virtual threads), connection handler runnable, and handler factory. Check startup configuration wiring and use of virtual threads.
Pipeline & routing core src/main/java/org/juv25d/Pipeline.java, src/main/java/org/juv25d/router/Router.java, src/main/java/org/juv25d/router/SimpleRouter.java, src/main/java/org/juv25d/filter/FilterRegistration.java	Implements Pipeline with global/route filters, router interface and simple implementation (longest-prefix wildcard). Verify thread-safety, pattern matching, and router assignment validations.
Filter framework src/main/java/org/juv25d/filter/Filter.java, src/main/java/org/juv25d/filter/FilterChain.java, src/main/java/org/juv25d/filter/FilterChainImpl.java, src/main/java/org/juv25d/filter/annotation/...	Adds Filter and FilterChain APIs, chain implementation that delegates to Router, and annotations @Global/@Route. Inspect chain terminal behavior and router integration.
Concrete filters src/main/java/org/juv25d/filter/LoggingFilter.java, .../IpFilter.java, .../RateLimitingFilter.java, .../RedirectFilter.java, .../RedirectRule.java, .../SecurityHeadersFilter.java	Adds logging, IP allow/block, Bucket4J rate limiting, ordered redirects with wildcard support, and security headers filter. Review validation, error responses (403/429), header handling, and regex/wildcard matching.
HTTP model & IO src/main/java/org/juv25d/http/HttpRequest.java, .../HttpResponse.java, .../HttpParser.java, .../HttpResponseWriter.java	Introduces request/response types, parser (request-line, headers, Content-Length handling), and response serializer (computes Content-Length). Verify robustness to malformed input and CRLF handling.
Static file serving src/main/java/org/juv25d/handler/StaticFileHandler.java, .../MimeTypeResolver.java, src/main/resources/static/*	Serves classpath /static resources with path traversal prevention, MIME detection, root index, and static web assets (HTML/CSS/JS). Check path normalization, MIME charset addition, and large-file considerations.
Plugin system src/main/java/org/juv25d/plugin/Plugin.java, .../HelloPlugin.java, .../StaticFilesPlugin.java, .../NotFoundPlugin.java	Adds Plugin contract and implementations for static files, not-found handler, and a placeholder hello plugin. Validate plugin lifecycle expectations and exception propagation.
Configuration & logging src/main/java/org/juv25d/util/ConfigLoader.java, src/main/java/org/juv25d/logging/ServerLogging.java, src/main/resources/application-properties.yml	YAML-backed ConfigLoader singleton and centralized logger setup. Review default values, error wrapping, and environment/system property precedence for log level.
Documentation & ADRs README.md, docs/adr/README.md, docs/adr/TEMPLATE.md, docs/adr/ADR-001-static-file-serving-architecture.md, docs/notes/pipeline-usage.md	Large README overhaul, ADRs and static-file architecture decision, and pipeline/filter usage docs. Review for correctness and alignment with implemented behavior.
Tests src/test/java/org/juv25d/... (many new unit/integration tests), removed src/test/java/org/example/AppIT.java	Adds extensive unit tests (parser, writer, filters, routing, static files, logging) and a Testcontainers integration test. Review test assumptions (container image name, ports) and CI execution.
Removed template src/main/java/org/example/App.java, src/test/java/org/example/AppIT.java	Removes original Hello World template files. Confirm no residual references remain.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Client
    participant Server as Server\n(accept loop)
    participant ConnHandler as ConnectionHandler
    participant Parser as HttpParser
    participant Pipeline as Pipeline
    participant Filters as Global/Route Filters
    participant Router as Router
    participant Plugin as Plugin
    participant Writer as HttpResponseWriter

    Client->>Server: TCP connect / send request bytes
    Server->>ConnHandler: create runnable for socket
    ConnHandler->>Parser: parse(InputStream) -> HttpRequest
    Parser-->>ConnHandler: HttpRequest
    ConnHandler->>Pipeline: createChain(HttpRequest)
    Pipeline->>Filters: assemble sorted global + route filters
    ConnHandler->>Filters: FilterChain.doFilter(request, response)
    loop filter chain
        Filters->>Filters: each Filter.doFilter(...)
    end
    Filters->>Router: resolve(request)
    Router-->>Plugin: selected Plugin
    Plugin->>Plugin: handle(request, response)
    Plugin-->>ConnHandler: response populated
    ConnHandler->>Writer: write(response) -> bytes
    Writer-->>Client: HTTP response bytes

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

• Implement SecurityHeadersFilter to harden HTTP responses project-webserver-juv25d#89: Adds SecurityHeadersFilter and registers it as a global filter in App.java; directly addresses the issue's requirement to inject security response headers.

Poem

🐰 In burrows of bytes I quietly creep,
Filters guard tunnels while I nap and keep,
Static pages hop out, tidy and bright,
Docker crates packed snug for flight,
A tiny server, sprung to life at night.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.46% 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 'Implement securityheadersfilter to harden http responses' directly and accurately describes the main change in the PR: adding a SecurityHeadersFilter to enhance HTTP response security.

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

✨ Finishing Touches

• 📝 Generate docstrings

🧪 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: 2

Note

Due to the large number of review comments, Critical severity comments were prioritized as inline comments.

🟠 Major comments (19)

.mvn/wrapper/maven-wrapper.properties-3-3 (1)

3-3: ⚠️ Potential issue | 🟠 Major

Maven 3.8.7 is end-of-life — upgrade to 3.9.12.

Maven 3.8.9 and earlier has reached end of life and is no longer supported; new plugin releases require Maven 3.9.0 or later. The latest stable release is Maven 3.9.12 (released December 16, 2025). Staying on 3.8.7 means no bug fixes, security patches, or compatibility with new plugin releases.

♻️ Proposed fix

-distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.8.7/apache-maven-3.8.7-bin.zip
+distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.9.12/apache-maven-3.9.12-bin.zip

🤖 Prompt for AI Agents

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

In @.mvn/wrapper/maven-wrapper.properties at line 3, Update the Maven wrapper
distribution to the supported 3.9.12 release by changing the distributionUrl
value in .mvn/wrapper/maven-wrapper.properties (the distributionUrl property) to
point to the apache-maven-3.9.12-bin.zip artifact; ensure the URL uses the
official Maven repository and that any CI or build scripts that rely on the
wrapper are re-run to fetch the updated distribution.

src/test/java/org/juv25d/AppIT.java-31-46 (1)

31-46: ⚠️ Potential issue | 🟠 Major

No test coverage for the security headers — the stated objective of this PR.

The PR's primary goal is to add SecurityHeadersFilter injecting X-Content-Type-Options, X-Frame-Options, X-XSS-Protection, and Referrer-Policy. Neither integration test verifies that these headers are present on responses. Consider adding assertions in shouldReturnIndexHtml (or a dedicated test) to validate the security headers:

assertThat(response.headers().firstValue("X-Content-Type-Options")).hasValue("nosniff");
assertThat(response.headers().firstValue("X-Frame-Options")).hasValue("DENY");
assertThat(response.headers().firstValue("Referrer-Policy")).hasValue("no-referrer");

🤖 Prompt for AI Agents

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

In `@src/test/java/org/juv25d/AppIT.java` around lines 31 - 46, Add assertions to
the integration tests in AppIT to verify the security headers set by
SecurityHeadersFilter: update shouldReturnIndexHtml (or add a separate test) to
assert that response.headers().firstValue("X-Content-Type-Options") has value
"nosniff", response.headers().firstValue("X-Frame-Options") has value "DENY",
response.headers().firstValue("X-XSS-Protection") has the expected value (e.g.,
"1; mode=block"), and response.headers().firstValue("Referrer-Policy") has value
"no-referrer"; use the same HttpResponse<String> response object and AssertJ
style assertions (hasValue / isEqualTo) as used elsewhere in AppIT to keep
consistency with methods shouldReturnIndexHtml and
shouldReturn404ForNonExistentPage.

Dockerfile-11-17 (1)

11-17: ⚠️ Potential issue | 🟠 Major

Container runs as root — add a non-root user.

The runtime stage does not specify a USER directive, so the application runs as root inside the container. This is a security best practice violation flagged by Trivy (DS-0002).

Proposed fix

 FROM eclipse-temurin:25-jre-alpine
 
 WORKDIR /app
 
-# might need to update this later when we have our explicit class names
+RUN addgroup -S appgroup && adduser -S appuser -G appgroup
+
 COPY --from=build /app/target/app.jar app.jar
+
+USER appuser
+
 ENTRYPOINT ["java", "-jar", "app.jar"]

🤖 Prompt for AI Agents

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

In `@Dockerfile` around lines 11 - 17, The container currently runs as root;
create and switch to a non-root user in the runtime Dockerfile: add a non-root
group/user (e.g., "app"), ensure ownership of /app and app.jar is changed to
that user (chown), and add a USER directive before ENTRYPOINT to run the JVM as
that user; reference the Dockerfile WORKDIR, COPY, and ENTRYPOINT lines (and
adjust the COPY destination ownership) so the runtime stage drops root
privileges and the app runs as the non-root "app" user.

src/main/java/org/juv25d/filter/SecurityHeadersFilter.java-20-20 (1)

20-20: ⚠️ Potential issue | 🟠 Major

Remove or disable the deprecated X-XSS-Protection header — it can introduce XSS vulnerabilities.

Even though this feature can protect users of older web browsers that don't support CSP, in some cases, X-XSS-Protection can create XSS vulnerabilities in otherwise safe websites. The OWASP HTTP Headers Cheat Sheet explicitly states: "Do not set this header or explicitly turn it off." It is recommended to use Content-Security-Policy instead of XSS filtering.

Setting 1; mode=block as the value is the exact configuration OWASP warns against. Either remove the line entirely or explicitly set it to X-XSS-Protection: 0:

🔧 Proposed fix

         res.setHeader("X-Content-Type-Options", "nosniff");
         res.setHeader("X-Frame-Options", "DENY");
-        res.setHeader("X-XSS-Protection", "1; mode=block");
+        // X-XSS-Protection is deprecated and can introduce XSS vulnerabilities;
+        // use Content-Security-Policy instead.
+        res.setHeader("X-XSS-Protection", "0");
         res.setHeader("Referrer-Policy", "no-referrer");

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/filter/SecurityHeadersFilter.java` at line 20, The
SecurityHeadersFilter currently sets the deprecated X-XSS-Protection header;
remove that call or change it to explicitly disable the legacy filter (set value
"0") to follow OWASP guidance. Locate the statement setting this header in the
SecurityHeadersFilter (e.g., the res.setHeader("X-XSS-Protection", ...) line
inside the filter method) and either delete that line or replace the value with
"0" so the header is not enabling legacy XSS filtering; ensure any related tests
or header assertions are updated accordingly.

src/main/java/org/juv25d/handler/MimeTypeResolver.java-14-42 (1)

14-42: ⚠️ Potential issue | 🟠 Major

Missing md MIME type will break README.md serving.

app.js calls fetch("/README.md") in initReadme(). With no entry for md, getMimeType falls back to application/octet-stream, which causes browsers to treat the response as a binary download rather than text — preventing DOMPurify/marked from rendering the content.

🐛 Proposed fix

 // Text types
 MIME_TYPES.put("html", "text/html");
 MIME_TYPES.put("htm",  "text/html");
 MIME_TYPES.put("css",  "text/css");
-MIME_TYPES.put("js",   "application/javascript");
+MIME_TYPES.put("js",   "text/javascript");
 MIME_TYPES.put("json", "application/json");
 MIME_TYPES.put("xml",  "application/xml");
 MIME_TYPES.put("txt",  "text/plain");
+MIME_TYPES.put("md",   "text/markdown");

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/handler/MimeTypeResolver.java` around lines 14 - 42,
Add the missing MIME mapping for Markdown files so README.md is served as text:
in the static initializer of MimeTypeResolver (where MIME_TYPES is populated)
add a mapping for the "md" extension to "text/markdown" (e.g.,
MIME_TYPES.put("md", "text/markdown")) so getMimeType returns a text MIME type
instead of falling back to application/octet-stream; update the same static
block alongside other text types.

src/main/resources/static/js/app.js-16-18 (1)

16-18: ⚠️ Potential issue | 🟠 Major

popstate handler calls navigate(), which re-pushes state and breaks back-button navigation.

When the user presses Back/Forward, popstate fires after the browser has already updated window.location. Calling navigate(window.location.pathname) then fetches the page and calls history.pushState() again, inserting a duplicate forward entry and making the back button appear stuck.

The fix is to load the content without pushing state on popstate:

🐛 Proposed fix

+// Separate content-loading from history manipulation
+function loadContent(href) {
+    const main = document.getElementById("main-content");
+    main.classList.add("fade-out");
+    setTimeout(() => {
+        fetch(href)
+            .then(res => {
+                if (!res.ok) throw new Error(`HTTP ${res.status}`);
+                return res.text();
+            })
+            .then(html => {
+                const doc = new DOMParser().parseFromString(html, "text/html");
+                const newMain = doc.querySelector("main");
+                if (!newMain) throw new Error("No <main> found in " + href);
+                main.innerHTML = newMain.innerHTML;
+                route(href);
+                main.classList.remove("fade-out");
+                setTimeout(() => nav.classList.remove("disable-anchors"), 150);
+            })
+            .catch(err => {
+                console.error("Navigation failed:", err);
+                main.classList.remove("fade-out");
+                nav.classList.remove("disable-anchors");
+            });
+    }, 200);
+}
+
 function navigate(href) {
     nav.classList.add("disable-anchors");
-    const main = document.getElementById("main-content");
-    main.classList.add("fade-out");
-    setTimeout(() => {
-        fetch(href)
-            // ...
-            .then(html => {
-                // ...
-                history.pushState(null, "", href);
-                route(href);
-                main.classList.remove("fade-out");
-                setTimeout(() => { nav.classList.remove("disable-anchors"); }, 150);
-            })
-            // ...
-    }, 200);
+    history.pushState(null, "", href);
+    loadContent(href);
 }
 
 window.addEventListener("popstate", () => {
-    navigate(window.location.pathname);
+    loadContent(window.location.pathname);  // restore content without re-pushing state
 });

🤖 Prompt for AI Agents

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

In `@src/main/resources/static/js/app.js` around lines 16 - 18, The popstate
handler currently calls navigate(window.location.pathname), which fetches and
renders the page but also re-pushes history and creates a duplicate entry;
change the handler to load content without pushing state by either calling a
new/no-history variant (e.g., loadPath or renderPath) or by adding a no-push
option to navigate (e.g., navigate(path, {push: false}) or navigate(path,
false)) and use that from the popstate listener so the page is rendered but
history.pushState is not invoked.

src/main/java/org/juv25d/DefaultConnectionHandlerFactory.java-9-21 (1)

9-21: ⚠️ Potential issue | 🟠 Major

this.pipeline is stored but never used — create() always uses the pipeline parameter instead.

The constructor accepts and stores a Pipeline, but the overridden create(Socket, Pipeline) method ignores the stored field and passes the parameter to the new ConnectionHandler. In the current code, both happen to be the same object (the fully-configured pipeline from App.java), but this design is flawed:

1. The stored field is dead code and creates maintenance confusion.
2. The design is error-prone — if the code is refactored or someone passes a different pipeline to create(), the security filters configured at factory construction time are silently discarded.
3. The signature misleadingly suggests the factory owns the pipeline when it doesn't.

Either the factory should use its stored pipeline:

Option 1: Factory owns the pipeline

 `@Override`
 public Runnable create(Socket socket, Pipeline pipeline) {
-    return new ConnectionHandler(socket, httpParser, logger, pipeline);
+    return new ConnectionHandler(socket, httpParser, logger, this.pipeline);
 }

Or remove the constructor parameter and field entirely:

Option 2: Caller supplies the pipeline

 public class DefaultConnectionHandlerFactory implements ConnectionHandlerFactory {
     private final HttpParser httpParser;
     private final Logger logger;
-    private final Pipeline pipeline;
 
-    public DefaultConnectionHandlerFactory(HttpParser httpParser, Logger logger, Pipeline pipeline) {
+    public DefaultConnectionHandlerFactory(HttpParser httpParser, Logger logger) {
         this.httpParser = httpParser;
         this.logger = logger;
-        this.pipeline = pipeline;
     }
 
     `@Override`
     public Runnable create(Socket socket, Pipeline pipeline) {
         return new ConnectionHandler(socket, httpParser, logger, pipeline);
     }
 }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/DefaultConnectionHandlerFactory.java` around lines 9
- 21, The factory stores a Pipeline in this.pipeline but the create(Socket,
Pipeline) method ignores it and uses the method parameter; update
DefaultConnectionHandlerFactory.create to pass the stored this.pipeline into the
ConnectionHandler (i.e., return new ConnectionHandler(socket, httpParser,
logger, this.pipeline)) so the factory-owned pipeline (configured in the
constructor) is always used; ensure the constructor and field names
(DefaultConnectionHandlerFactory, this.pipeline, create, ConnectionHandler)
remain consistent and remove any unused local references to the method parameter
if appropriate.

src/main/java/org/juv25d/handler/StaticFileHandler.java-89-105 (1)

89-105: ⚠️ Potential issue | 🟠 Major

isPathSafe is bypassable via URL-encoded traversal sequences.

The check tests for the literal strings .., //, and \, but an attacker can use percent-encoded equivalents (%2e%2e, %2f, %5c) to pass the check. Depending on how the downstream classpath resource lookup resolves encoded characters, this could allow partial traversal.

Add a URL-decode step before the safety checks:

🔒 Proposed fix

+import java.net.URLDecoder;
+import java.nio.charset.StandardCharsets;

 private static boolean isPathSafe(String path) {
     if (path == null || path.isEmpty()) {
         return false;
     }
+    // Decode percent-encoded sequences before validating
+    String decoded;
+    try {
+        decoded = URLDecoder.decode(path, StandardCharsets.UTF_8);
+    } catch (IllegalArgumentException e) {
+        return false; // malformed encoding
+    }
-    if (path.contains("..") || path.contains("//") || path.contains("\\")) {
+    if (decoded.contains("..") || decoded.contains("//") || decoded.contains("\\")) {
         return false;
     }
-    if (!path.startsWith("/")) {
+    if (!decoded.startsWith("/")) {
         return false;
     }
     return true;
 }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/handler/StaticFileHandler.java` around lines 89 -
105, isPathSafe is vulnerable to encoded traversal because it checks the raw
input; first URL-decode the incoming path (using UTF-8) and store into a local
decodedPath, then run the existing safety checks (null/empty, contains "..",
"//", "\\" and ensure startsWith("/")) against decodedPath instead of the raw
input; if decoding throws or yields a value that fails validation return false.
Ensure you catch invalid-decode exceptions and return false so malformed
percent-encodings cannot bypass the checks.

src/main/java/org/juv25d/handler/StaticFileHandler.java-151-180 (1)

151-180: ⚠️ Potential issue | 🟠 Major

Reflected XSS: path is inserted into HTML without escaping.

createNotFoundResponse formats the raw request path directly into the HTML body (<code>%s</code>). A request for /<script>alert(document.cookie)</script> produces a text/html response that executes the script in the requester's browser.

🔒 Proposed fix — HTML-escape the path before embedding it

 private static HttpResponse createNotFoundResponse(String path) {
+    String safePath = path
+        .replace("&", "&")
+        .replace("<", "&lt;")
+        .replace(">", "&gt;")
+        .replace("\"", "&quot;")
+        .replace("'", "&#x27;");
     String html = """
             ...
-                <p>The requested resource <code>%s</code> was not found on this server.</p>
+                <p>The requested resource <code>%s</code> was not found on this server.</p>
             ...
-            """.formatted(path);
+            """.formatted(safePath);

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/handler/StaticFileHandler.java` around lines 151 -
180, The createNotFoundResponse method injects the raw path into HTML causing
reflected XSS; escape the path before embedding it into the HTML body (e.g.,
HTML-encode &, <, >, ", ' characters) and use the escaped value in the formatted
string. Locate createNotFoundResponse and sanitize the path variable (either via
a utility like StringEscapeUtils.escapeHtml4 or a small helper that replaces the
five special chars) prior to calling html.formatted(path), so the returned
HttpResponse contains the escaped text instead of raw user input.

src/main/java/org/juv25d/App.java-29-48 (1)

29-48: ⚠️ Potential issue | 🟠 Major

All global filters share priority 0 — SecurityHeadersFilter will not run on short-circuited responses.

Every filter is registered with 0 as its priority, and RedirectFilter is added first. If the pipeline processes equal-priority filters in insertion order and RedirectFilter or IpFilter terminates the chain early (by not calling chain.doFilter()), SecurityHeadersFilter never executes. This directly contradicts the PR's stated goal of applying security headers to all outgoing responses.

The documented convention in pipeline-usage.md assigns distinct priorities (100, 200, 300, 400…). SecurityHeadersFilter should run with the lowest priority number (highest precedence) so it can set headers before any filter that might short-circuit.

💡 Proposed fix — assign unique, meaningful priorities

-pipeline.addGlobalFilter(new RedirectFilter(redirectRules), 0);
+pipeline.addGlobalFilter(new SecurityHeadersFilter(), 100); // runs first on every response
 
-pipeline.addGlobalFilter(new IpFilter(
+pipeline.addGlobalFilter(new IpFilter(
     Set.of(),
     Set.of()
-), 0);
+), 200);
 
-pipeline.addGlobalFilter(new LoggingFilter(), 0);
+pipeline.addGlobalFilter(new LoggingFilter(), 300);
 
-pipeline.addGlobalFilter(new SecurityHeadersFilter(), 0);
+pipeline.addGlobalFilter(new RedirectFilter(redirectRules), 400);
 
 if (config.isRateLimitingEnabled()) {
     pipeline.addGlobalFilter(new RateLimitingFilter(
         config.getRequestsPerMinute(),
         config.getBurstCapacity()
-    ), 0);
+    ), 500);
 }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/App.java` around lines 29 - 48, Multiple global
filters are registered with the same priority (0) so short-circuiting filters
like RedirectFilter or IpFilter can prevent SecurityHeadersFilter from running;
update the pipeline.addGlobalFilter calls to use distinct, ordered priorities
per pipeline-usage.md (e.g., lower numbers for higher precedence) and ensure
SecurityHeadersFilter is registered with the highest precedence value so it
always runs before filters that may short-circuit; modify the calls that add
RedirectFilter, IpFilter, LoggingFilter, RateLimitingFilter and
SecurityHeadersFilter accordingly (refer to pipeline.addGlobalFilter and the
classes RedirectFilter, IpFilter, LoggingFilter, SecurityHeadersFilter,
RateLimitingFilter).

src/main/java/org/juv25d/Server.java-21-33 (1)

21-33: ⚠️ Potential issue | 🟠 Major

No graceful shutdown mechanism.

The while (true) loop has no way to be broken — no volatile flag, no shutdown() method, no Runtime.addShutdownHook. The only way to stop the server is to kill the JVM, which drops in-flight requests.

💡 Minimal shutdown approach

+private volatile boolean running = true;

 public void start() {
-    try (ServerSocket serverSocket = new ServerSocket(port, 64)) {
+    try (ServerSocket serverSocket = new ServerSocket(port, 64)) {
         logger.info("Server started at port: " + serverSocket.getLocalPort());

-        while (true) {
+        Runtime.getRuntime().addShutdownHook(Thread.ofVirtual().unstarted(() -> {
+            running = false;
+            try { serverSocket.close(); } catch (IOException ignored) {}
+        }));
+
+        while (running) {
             Socket socket = serverSocket.accept();
             Runnable handler = handlerFactory.create(socket, pipeline);
             Thread.ofVirtual().start(handler);
         }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/Server.java` around lines 21 - 33, Add a graceful
shutdown to Server by introducing a volatile boolean running (or AtomicBoolean)
checked in start() instead of while(true), provide a public shutdown() that
clears running and closes the ServerSocket, and register a
Runtime.addShutdownHook to call shutdown(); update start() to store the created
ServerSocket in a field (instead of only a local variable) so shutdown() can
close it, and ensure accept() loop catches
SocketException/ClosedChannelException to exit cleanly; keep existing handler
creation via handlerFactory.create(socket, pipeline) and continue using
Thread.ofVirtual().start(handler) for request threads.

src/main/java/org/juv25d/http/HttpParser.java-38-48 (1)

38-48: ⚠️ Potential issue | 🟠 Major

Unbounded header parsing is a DoS vector.

Neither the number of headers nor individual header-line length is bounded. A malicious client can send millions of headers or an infinitely long header line, exhausting heap memory. Consider capping both (e.g., 8 KB per line, 100 max headers) and throwing an appropriate error when exceeded.

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/http/HttpParser.java` around lines 38 - 48, The
header parsing in HttpParser (the headers Map and the loop that uses
readLine(in)) is unbounded; add limits to prevent DoS by capping the maximum
header line length (e.g., 8*1024 bytes/chars) and maximum number of headers
(e.g., 100) before inserting into headers; inside the while loop check the line
length returned by readLine(in) and throw an IOException like "Header line too
long" if it exceeds the limit, and before putting into headers increment a
counter (or check headers.size()) and throw an IOException like "Too many
headers" when the max is exceeded; keep error messages descriptive and use the
existing IOException type.

src/main/java/org/juv25d/Pipeline.java-43-65 (1)

43-65: ⚠️ Potential issue | 🟠 Major

createChain will NPE if setRouter was never called.

router is initialized to null (Line 21). If createChain is invoked before setRouter, it passes null to FilterChainImpl, which will throw a NullPointerException when the chain exhausts all filters and calls router.resolve(req). Add a guard.

Proposed fix

 public FilterChainImpl createChain(HttpRequest request) {
+    if (router == null) {
+        throw new IllegalStateException("Router has not been configured. Call setRouter() before creating chains.");
+    }
     List<Filter> filters = new ArrayList<>();

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/Pipeline.java` around lines 43 - 65, createChain
currently passes the potentially null field router into FilterChainImpl which
will NPE if setRouter was never called; add a guard at the start or just before
constructing the FilterChainImpl to check that router != null and throw an
IllegalStateException (or another appropriate runtime exception) with a clear
message like "Router not set" so callers get an explicit error instead of an
NPE; reference the createChain method and the router field (and setRouter) when
making this change so the null-check occurs before new FilterChainImpl(filters,
router) is invoked.

src/main/java/org/juv25d/http/HttpParser.java-66-81 (1)

66-81: ⚠️ Potential issue | 🟠 Major

readLine has no length limit — enables single-line memory exhaustion.

A client can send a multi-gigabyte request line or header line (no \n) to grow the StringBuilder unboundedly. Add a max-line-length guard (e.g., 8192 bytes) and throw if exceeded.

Proposed fix

+    private static final int MAX_LINE_LENGTH = 8192;
+
     private String readLine(InputStream in) throws IOException {
         StringBuilder sb = new StringBuilder();
         int b;
         while ((b = in.read()) != -1) {
+            if (sb.length() >= MAX_LINE_LENGTH) {
+                throw new IOException("Line exceeds maximum allowed length");
+            }
             if (b == '\n') {
                 break;
             }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/http/HttpParser.java` around lines 66 - 81, The
readLine method builds an unbounded StringBuilder from InputStream which allows
a client to exhaust memory; add a max-line-length guard (e.g., MAX_LINE_LENGTH =
8192) inside readLine and increment a counter as bytes are appended, and if the
counter exceeds MAX_LINE_LENGTH throw an IOException (or a specific
ProtocolException) indicating "line too long". Keep the existing handling of
'\r' and '\n' and the null return on EOF when nothing was read; apply the length
check before appending to sb in the readLoop within readLine to prevent
unbounded growth.

src/main/java/org/juv25d/ConnectionHandler.java-27-59 (1)

27-59: ⚠️ Potential issue | 🟠 Major

Uncaught RuntimeException will silently kill the handler thread.

Only IOException is caught. If a filter or plugin throws a RuntimeException (e.g., NullPointerException, IllegalStateException), it propagates uncaught, and the client gets no response while the error goes unlogged. Wrap in a broader catch to ensure errors are always logged and the socket is closed cleanly.

Proposed fix

     `@Override`
     public void run() {
         try (socket) {
             var in = socket.getInputStream();
             var out = socket.getOutputStream();
 
             HttpRequest parsed = httpParser.parse(in);
             String remoteIp = socket.getInetAddress().getHostAddress();
 
             HttpRequest request = new HttpRequest(
                 parsed.method(),
                 parsed.path(),
                 parsed.queryString(),
                 parsed.httpVersion(),
                 parsed.headers(),
                 parsed.body(),
                 remoteIp
             );
 
             HttpResponse response = new HttpResponse(
                 200,
                 "OK",
-                java.util.Map.of(),
+                Map.of(),
                 new byte[0]
             );
 
             var chain = pipeline.createChain(request);
             chain.doFilter(request, response);
 
             HttpResponseWriter.write(out, response);
 
         } catch (IOException e) {
             logger.log(Level.SEVERE, "Error while handling request", e);
+        } catch (Exception e) {
+            logger.log(Level.SEVERE, "Unexpected error while handling request", e);
         }
     }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/ConnectionHandler.java` around lines 27 - 59, The
run() method currently only catches IOException, so any RuntimeException from
httpParser.parse, pipeline.createChain, chain.doFilter, or
HttpResponseWriter.write will escape and kill the thread; modify run() to add a
broader catch (catch Throwable or catch RuntimeException) after the IOException
catch to log the error via logger.log(Level.SEVERE, ...), ensure the socket is
closed (the try-with-resources already closes it) and attempt to send a 500
HttpResponse (use HttpResponse(500,"Internal Server Error", Map.of(), new
byte[0]) and HttpResponseWriter.write) so the client gets a proper response when
pipeline.createChain or chain.doFilter throws.

src/main/java/org/juv25d/filter/RateLimitingFilter.java-25-25 (1)

25-25: ⚠️ Potential issue | 🟠 Major

Unbounded buckets map enables a memory-exhaustion attack.

Every unique client IP causes a new Bucket entry that is never evicted. An attacker spoofing source IPs (or any large NAT / proxy environment with many distinct IPs) can fill the map indefinitely and cause OOM. Replace the ConcurrentHashMap with a bounded cache with TTL-based eviction, e.g. Caffeine:

🛡️ Suggested fix using Caffeine cache

-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;

-private final Map<String, Bucket> buckets = new ConcurrentHashMap<>();
+private final Cache<String, Bucket> buckets = Caffeine.newBuilder()
+    .maximumSize(100_000)
+    .expireAfterAccess(java.time.Duration.ofMinutes(10))
+    .build();

Then update computeIfAbsent:

-Bucket bucket = buckets.computeIfAbsent(clientIp, k -> createBucket());
+Bucket bucket = buckets.get(clientIp, k -> createBucket());

And destroy():

-buckets.clear();
+buckets.invalidateAll();

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/filter/RateLimitingFilter.java` at line 25, The
unbounded Map<String, Bucket> buckets should be replaced with a bounded,
TTL-evicting cache (e.g., Caffeine) to prevent memory-exhaustion; change the
field declaration for buckets from ConcurrentHashMap to a Caffeine Cache<String,
Bucket> configured with a reasonable maximumSize and expireAfterAccess/Write
TTL, update places that use buckets.computeIfAbsent(...) to use cache.get(key, k
-> createBucket(...)) or cache.asMap().computeIfAbsent(...) so bucket creation
remains lazy, and modify destroy() to call cache.invalidateAll() /
cache.cleanUp() (or close the cache if applicable) to ensure proper
eviction/cleanup. Ensure Bucket creation logic and any references to the buckets
variable are updated to work with the Cache API.

src/main/java/org/juv25d/filter/RateLimitingFilter.java-83-91 (1)

83-91: ⚠️ Potential issue | 🟠 Major

Replace deprecated Bandwidth.classic() + Refill.intervally() with modern builder API and fix rate limiting semantics.

Bucket4J 8.16.1 deprecated Bandwidth.classic in v8.5.0. More importantly, the current implementation has a semantic bug: Refill.intervally waits until the full minute elapses before adding all 60 tokens at once, but since capacity = burstCapacity (5 tokens), only 5 tokens can be held. After the burst is exhausted, requests are limited to ~5 per minute instead of the intended 60 per minute.

Use refillGreedy instead, which distributes tokens continuously throughout the period and achieves the smooth 60 req/min throughput:

🔧 Migration to modern API with corrected semantics

 private Bucket createBucket() {
-    Bandwidth limit = Bandwidth.classic(
-        capacity,
-        Refill.intervally(refillTokens, refillPeriod));
-
-    return Bucket.builder()
-        .addLimit(limit)
-        .build();
+    return Bucket.builder()
+        .addLimit(limit -> limit
+            .capacity(capacity)
+            .refillGreedy(refillTokens, refillPeriod))
+        .build();
 }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/filter/RateLimitingFilter.java` around lines 83 -
91, The createBucket method uses the deprecated Bandwidth.classic and
Refill.intervally which causes burst capacity semantics to be wrong; update
createBucket to use the modern Bandwidth builder API and a greedy refill so
tokens are distributed continuously: build a Bandwidth via Bandwidth.builder()
(referencing capacity and burstCapacity if used) and supply
Refill.greedy(refillTokens, refillPeriod) (or refillGreedy equivalent) instead
of Refill.intervally, then add that Bandwidth to Bucket.builder().addLimit(...)
and build — this replaces the deprecated call and fixes the rate-limiting
semantics in createBucket.

src/main/java/org/juv25d/filter/RedirectRule.java-30-35 (1)

30-35: ⚠️ Potential issue | 🟠 Major

Null sourcePath will cause NPE inside matches()

Neither sourcePath nor targetUrl is validated against null in the constructor. A null sourcePath propagates silently to matches() where sourcePath.contains("*") (line 53) throws an NPE at request time. targetUrl being null would produce a malformed Location header.

🐛 Proposed fix — add null guards

 public RedirectRule(String sourcePath, String targetUrl, int statusCode) {
     validateStatusCode(statusCode);
+    Objects.requireNonNull(sourcePath, "sourcePath must not be null");
+    Objects.requireNonNull(targetUrl, "targetUrl must not be null");
     this.sourcePath = sourcePath;
     this.targetUrl = targetUrl;
     this.statusCode = statusCode;
 }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/filter/RedirectRule.java` around lines 30 - 35, The
constructor RedirectRule(String sourcePath, String targetUrl, int statusCode)
does not guard against nulls causing NPEs in matches()
(sourcePath.contains("*")) or malformed Location headers from a null targetUrl;
update the constructor (the RedirectRule(...) method) to validate and reject
null sourcePath and targetUrl (e.g., use Objects.requireNonNull or explicit
checks) before assigning fields and still call validateStatusCode(statusCode);
throw IllegalArgumentException/NullPointerException with a clear message so
invalid inputs fail fast.

src/main/java/org/juv25d/util/ConfigLoader.java-38-43 (1)

38-43: ⚠️ Potential issue | 🟠 Major

Missing defaults when the server section is entirely absent

If serverConfig is null (i.e., the server key is missing from the YAML), the if block is skipped and both fields keep Java's zero-value defaults: port = 0 and rootDirectory = null. A port of 0 causes the OS to bind to a random ephemeral port, and a null root directory will NPE in downstream consumers like StaticFilesPlugin. The intended defaults (8080 / "static") must also apply when the whole section is absent.

🐛 Proposed fix — initialize fields with defaults at declaration

-    private int port;
-    private String logLevel;
-    private String rootDirectory;
+    private int port = 8080;
+    private String logLevel;
+    private String rootDirectory = "static";

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/util/ConfigLoader.java` around lines 38 - 43,
ConfigLoader currently leaves port and rootDirectory at Java defaults when the
"server" map is missing: update the class so port and rootDirectory get the
intended defaults (port = 8080, rootDirectory = "static") even if serverConfig
is null; do this by initializing the fields at declaration or by adding an else
branch after the serverConfig check that assigns these defaults (refer to fields
port and rootDirectory and the local variable serverConfig in ConfigLoader to
locate where to apply the change).

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🌐 Web query:

testcontainers junit-jupiter version compatibility 2.0 1.21

💡 Result:

What “2.0 vs 1.21” means for Testcontainers + JUnit Jupiter

Testcontainers’ JUnit 5 integration has two different artifacts depending on the Testcontainers major line:

Testcontainers line	JUnit Jupiter integration artifact	Notes
1.21.x	org.testcontainers:junit-jupiter:1.21.x	Depends on org.testcontainers:testcontainers:1.21.x (same major/minor line). (mvnrepository.com)
2.0.x	org.testcontainers:testcontainers-junit-jupiter:2.0.x	Depends on org.testcontainers:testcontainers:2.0.x (same major line). (central.sonatype.com)

So, don’t mix …:2.0.x with …:1.21.x—pick the JUnit module that matches your Testcontainers core major version. (central.sonatype.com)

JUnit “2.0” / “Jupiter 2.0” clarification

There isn’t a “JUnit Jupiter 2.0” release line; the current major is JUnit 6.0.0 (released 2025-09-30) with unified versioning across Platform/Jupiter/Vintage. (infoq.com)

Practical compatibility guidance

• org.testcontainers:testcontainers-junit-jupiter:2.0.x is documented as JUnit Jupiter (JUnit 5) integration. (java.testcontainers.org)
• Its published POM shows no pinned dependency on a specific org.junit.jupiter:* version, so you supply your own JUnit Jupiter version (JUnit 5.x, or JUnit 6.x if your build supports it). (central.sonatype.com)

Citations:

• 1: https://mvnrepository.com/artifact/org.testcontainers/junit-jupiter/1.21.4/dependencies
• 2: https://central.sonatype.com/artifact/org.testcontainers/testcontainers-junit-jupiter/2.0.0
• 3: https://central.sonatype.com/artifact/org.testcontainers/testcontainers-junit-jupiter/2.0.0
• 4: https://www.infoq.com/news/2025/10/junit6-java17-kotlin/?utm_source=openai
• 5: https://java.testcontainers.org/test_framework_integration/junit_5/?utm_source=openai
• 6: https://central.sonatype.com/artifact/org.testcontainers/testcontainers-junit-jupiter/2.0.0

---

Testcontainers version mismatch and wrong artifact name: testcontainers 2.0.3 declares junit-jupiter:1.21.4, but 2.0.x requires testcontainers-junit-jupiter.

The org.testcontainers:testcontainers artifact at version 2.0.3 is paired with org.testcontainers:junit-jupiter:1.21.4, which mixes incompatible major versions and uses the wrong module name. In testcontainers 2.0.x, the JUnit 5 integration artifact was renamed to testcontainers-junit-jupiter. Align both artifact name and version: either use testcontainers:2.0.3 + testcontainers-junit-jupiter:2.0.3, or downgrade to testcontainers:1.21.4 + junit-jupiter:1.21.4. Mixing versions will cause classpath conflicts or initialization failures at runtime.

🤖 Prompt for AI Agents

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

In `@pom.xml` around lines 49 - 60, The pom currently mixes
org.testcontainers:testcontainers:2.0.3 with
org.testcontainers:junit-jupiter:1.21.4 which is the wrong artifact name and
mismatched major version; update the dependency block so the JUnit 5 integration
matches the Testcontainers major version—replace
org.testcontainers:junit-jupiter:1.21.4 with
org.testcontainers:testcontainers-junit-jupiter:2.0.3 (or alternatively
downgrade both to 1.21.4 by using org.testcontainers:testcontainers:1.21.4 and
org.testcontainers:junit-jupiter:1.21.4) to ensure artifact name and version
alignment.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

NullPointerException if ConfigLoader.getLogLevel() returns null.

When the logging section is absent from application-properties.yml, ConfigLoader.getLogLevel() returns null. If neither the log.level system property nor the LOG_LEVEL environment variable is set, levelName will be null, causing a NullPointerException at levelName.toUpperCase() on Line 33. The catch block only catches IllegalArgumentException, so the NPE propagates and crashes initialization.

🐛 Proposed fix

 String levelName = System.getProperty(
     "log.level",
     System.getenv().getOrDefault("LOG_LEVEL", ConfigLoader.getInstance().getLogLevel())
 );

+if (levelName == null) {
+    logger.setLevel(Level.INFO);
+    return;
+}
+
 try {
     Level level = Level.parse(levelName.toUpperCase());
     logger.setLevel(level);
 } catch (IllegalArgumentException e) {
     logger.setLevel(Level.INFO);
     logger.warning("Invalid log level: '" + levelName + "', defaulting to INFO");
 }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/logging/ServerLogging.java` around lines 27 - 38,
The code in ServerLogging builds levelName using ConfigLoader.getLogLevel()
which can be null, leading to a NullPointerException at levelName.toUpperCase();
fix by normalizing null/empty values to a safe default before calling
toUpperCase()/Level.parse: e.g., compute a non-null string from
System.getProperty(...), System.getenv().getOrDefault(...), or
ConfigLoader.getLogLevel() using a null-coalescing step (or
Objects.toString(..., "INFO")) so levelName is never null, then call
Level.parse(levelName.toUpperCase()) and keep the existing catch for
IllegalArgumentException; ensure logger.setLevel(Level.INFO) is used when the
resolved value is invalid.

[coderabbitai]

🧹 Nitpick comments (3)

src/main/java/org/juv25d/App.java (3)

6-8: Remove leftover // New import inline comments from import declarations.

These are development-time markers and add no value to the committed code.

♻️ Proposed fix

-import org.juv25d.plugin.NotFoundPlugin; // New import
+import org.juv25d.plugin.NotFoundPlugin;
 import org.juv25d.plugin.StaticFilesPlugin;
-import org.juv25d.router.SimpleRouter; // New import
+import org.juv25d.router.SimpleRouter;

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/App.java` around lines 6 - 8, Remove the leftover
inline comments "// New import" from the import declarations in App.java;
specifically edit the import statements that reference NotFoundPlugin and
SimpleRouter (and any other imports with that comment) so they are plain import
lines without trailing development comments.

---

52-53: Two separate StaticFilesPlugin instances for "/" and "/*" can share one instance.

StaticFilesPlugin is stateless (delegates entirely to StaticFileHandler.handle(request)). Creating two distinct instances is unnecessary.

♻️ Proposed fix

+        StaticFilesPlugin staticFilesPlugin = new StaticFilesPlugin();
-        router.registerPlugin("/", new StaticFilesPlugin()); // Register StaticFilesPlugin for the root path
-        router.registerPlugin("/*", new StaticFilesPlugin()); // Register StaticFilesPlugin for all paths
+        router.registerPlugin("/", staticFilesPlugin);
+        router.registerPlugin("/*", staticFilesPlugin);

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/App.java` around lines 52 - 53, You are registering
two separate StaticFilesPlugin instances for the same behavior; instead create a
single StaticFilesPlugin instance and reuse it for both
router.registerPlugin("/", ...) and router.registerPlugin("/*", ...). Locate the
calls to router.registerPlugin and replace the two new StaticFilesPlugin()
constructions with a single variable (e.g., staticFilesPlugin) instantiated once
and passed to both registrations so the stateless StaticFilesPlugin (which
delegates to StaticFileHandler.handle(request)) is shared.

---

24-48: Assign explicit priorities to control filter execution order; currently all filters have priority 0 which relies on insertion order.

With all filters at priority 0, the execution order depends on insertion order (stable sort), making the sequence implicit and fragile. The current order is:

1. SecurityHeadersFilter
2. RedirectFilter
3. IpFilter
4. LoggingFilter
5. RateLimitingFilter

This causes RateLimitingFilter to run after LoggingFilter, which means rate-limited requests are logged before being rejected. Additionally, security-critical filters should be ordered intentionally rather than by accident. Filters with lower priority values execute first.

♻️ Proposed fix: assign distinct priorities

-        pipeline.addGlobalFilter(new SecurityHeadersFilter(), 0);
+        pipeline.addGlobalFilter(new SecurityHeadersFilter(), 50); // applied last / post-processing

         // Configure redirect rules
         List<RedirectRule> redirectRules = List.of(
             new RedirectRule("/old-page", "/new-page", 301),
             new RedirectRule("/temp", "https://example.com/temporary", 302),
             new RedirectRule("/docs/*", "/documentation/", 301)
         );
-        pipeline.addGlobalFilter(new RedirectFilter(redirectRules), 0);
+        pipeline.addGlobalFilter(new RedirectFilter(redirectRules), 40);

         // IP filter is enabled but configured with open access during development
-        pipeline.addGlobalFilter(new IpFilter(
-            Set.of(),
-            Set.of()
-        ), 0);
+        pipeline.addGlobalFilter(new IpFilter(
+            Set.of(),
+            Set.of()
+        ), 10); // reject blocked IPs first

-        pipeline.addGlobalFilter(new LoggingFilter(), 0);
+        pipeline.addGlobalFilter(new LoggingFilter(), 30); // log after IP/rate checks

         if (config.isRateLimitingEnabled()) {
             pipeline.addGlobalFilter(new RateLimitingFilter(
                 config.getRequestsPerMinute(),
                 config.getBurstCapacity()
-            ), 0);
+            ), 20); // rate-limit after IP check, before logging
         }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/juv25d/App.java` around lines 24 - 48, All filters are
added with priority 0 so execution order is implicit; update the
pipeline.addGlobalFilter calls to use explicit distinct integer priorities so
ordering is intentional: add RateLimitingFilter with the highest precedence
(lowest number) so it runs before LoggingFilter (e.g., priority 10), then
IpFilter (20), then RedirectFilter (30), then LoggingFilter (40), and finally
SecurityHeadersFilter last (e.g., 50) so response headers are applied after
other processing; modify the calls referencing SecurityHeadersFilter,
RedirectFilter, IpFilter, LoggingFilter, and RateLimitingFilter passed to
pipeline.addGlobalFilter to use these explicit priority values.

🤖 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/juv25d/App.java`:
- Around line 6-8: Remove the leftover inline comments "// New import" from the
import declarations in App.java; specifically edit the import statements that
reference NotFoundPlugin and SimpleRouter (and any other imports with that
comment) so they are plain import lines without trailing development comments.
- Around line 52-53: You are registering two separate StaticFilesPlugin
instances for the same behavior; instead create a single StaticFilesPlugin
instance and reuse it for both router.registerPlugin("/", ...) and
router.registerPlugin("/*", ...). Locate the calls to router.registerPlugin and
replace the two new StaticFilesPlugin() constructions with a single variable
(e.g., staticFilesPlugin) instantiated once and passed to both registrations so
the stateless StaticFilesPlugin (which delegates to
StaticFileHandler.handle(request)) is shared.
- Around line 24-48: All filters are added with priority 0 so execution order is
implicit; update the pipeline.addGlobalFilter calls to use explicit distinct
integer priorities so ordering is intentional: add RateLimitingFilter with the
highest precedence (lowest number) so it runs before LoggingFilter (e.g.,
priority 10), then IpFilter (20), then RedirectFilter (30), then LoggingFilter
(40), and finally SecurityHeadersFilter last (e.g., 50) so response headers are
applied after other processing; modify the calls referencing
SecurityHeadersFilter, RedirectFilter, IpFilter, LoggingFilter, and
RateLimitingFilter passed to pipeline.addGlobalFilter to use these explicit
priority values.