Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request refactors the static file handler to use constructor-based configuration with a default web root, introduces an in-memory file cache, sanitizes incoming URIs, validates against path traversal attacks, and reorganizes request handling into a stateless, cache-backed flow with standardized error responses. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant StaticFileHandler
participant FileCache
participant Disk
Client->>StaticFileHandler: sendGetRequest(outputStream, uri)
StaticFileHandler->>StaticFileHandler: sanitizeUri(uri)
StaticFileHandler->>StaticFileHandler: isPathTraversal(sanitized)
alt Path Traversal Detected
StaticFileHandler->>StaticFileHandler: writeResponse(403, "Forbidden")
StaticFileHandler->>Client: 403 Response
else Safe Path
StaticFileHandler->>FileCache: get(cacheKey)
alt Cache Hit
FileCache-->>StaticFileHandler: byte[] content
else Cache Miss
StaticFileHandler->>Disk: readFile(webRoot, sanitized)
Disk-->>StaticFileHandler: byte[] content
StaticFileHandler->>FileCache: put(cacheKey, content)
end
alt File Found
StaticFileHandler->>StaticFileHandler: buildResponse(200, content, contentType)
StaticFileHandler->>Client: 200 + File Content
else File Not Found
StaticFileHandler->>StaticFileHandler: writeResponse(404, "Not Found")
StaticFileHandler->>Client: 404 Response
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/main/java/org/example/StaticFileHandler.java (1)
21-21: Consider replacingSystem.out.printlnwith a proper logger.
System.out.printlnon hot request paths adds synchronization overhead (the underlyingPrintStreamis synchronized) and bypasses any logging configuration. Ajava.util.logging.Loggeror SLF4JLoggeris the idiomatic alternative.Also applies to: 25-25
🤖 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 21, Replace the two System.out.println calls in StaticFileHandler with a proper logger: add a static final Logger (e.g., java.util.logging.Logger or org.slf4j.Logger) on the StaticFileHandler class, initialize it with the class name, and change the println statements (the "✓ Cache hit for: " and the other occurrence) to appropriate logger calls (info/debug) using parameterized or concatenated messages; remove the println usages so logging honours configuration and avoids PrintStream synchronization overhead.src/main/java/org/example/FileCache.java (1)
7-7: Unbounded heap growth — no eviction or size cap.Every unique URI populates the cache permanently. A server with many files or long uptime will accumulate all file bytes in heap with no release path. Consider a bounded LRU structure (e.g.
LinkedHashMapwithremoveEldestEntryoverride, or GuavaCacheBuilder.maximumSize()) to set a ceiling and evict least-recently-used entries.Also applies to: 24-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/FileCache.java` at line 7, The cache field in FileCache is an unbounded HashMap (private final Map<String, byte[]> cache) causing uncontrolled heap growth; replace it with a bounded LRU cache implementation (for example, use a LinkedHashMap subclass with accessOrder=true and override removeEldestEntry to enforce a max entries limit, or replace with a Guava Cache created via CacheBuilder.maximumSize(n)) and apply the same change to the related methods that read/write the cache (e.g., put/get/remove logic used around lines referenced) so entries are evicted when the size limit is exceeded and memory is bounded.src/test/java/org/example/FileCacheTest.java (1)
7-116: Well-structured test suite — good coverage of the public API.All nine cases (round-trip, missing-key null, overwrite semantics, clear, size progression, large/empty payloads) are clear and independently isolated via
@BeforeEach. A couple of optional additions would strengthen the suite:
- Aliasing / defensive-copy test — verify that mutating the array returned by
getdoes not change what the cache subsequently returns (this would simultaneously document and catch the mutable-aliasing issue flagged inFileCache):`@Test` void testGetReturnIsIndependentOfCache() { byte[] original = "hello".getBytes(); cache.put("file.txt", original); byte[] fetched = cache.get("file.txt"); fetched[0] = 'X'; // mutate the returned copy assertThat(cache.get("file.txt")).isEqualTo("hello".getBytes()); }
- Concurrent-access test — a simple latch-based test with two threads doing concurrent puts/gets would confirm thread-safety once
ConcurrentHashMapis adopted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/FileCacheTest.java` around lines 7 - 116, Add two tests to FileCacheTest: one named testGetReturnIsIndependentOfCache that verifies get returns a defensive copy by mutating the fetched byte[] and asserting subsequent cache.get("file.txt") still equals the original bytes; and one simple concurrency test (e.g., testConcurrentPutAndGet) that uses a CountDownLatch and two threads to perform concurrent cache.put and cache.get calls to exercise thread-safety (use FileCache instance, assert final size/contents as expected). Ensure both tests are annotated with `@Test` and use the existing cache field initialized in setUp().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/FileCache.java`:
- Line 7: The cache field uses a non-thread-safe HashMap and stores mutable
byte[] references; replace the Map implementation with a thread-safe
ConcurrentHashMap for the field (FileCache.cache) and make defensive copies of
byte arrays on entry and exit: when storing (e.g., FileCache.put or wherever
cache.put is called) copy the incoming byte[] (Arrays.copyOf) before placing it
into the map, and when returning data (e.g., FileCache.get) return a copy of the
stored byte[] rather than the internal reference; this prevents concurrent
corruption and aliasing of mutable arrays.
In `@src/main/java/org/example/StaticFileHandler.java`:
- Line 14: The instance field fileBytes in StaticFileHandler causes a data race
between concurrent requests; change handleGetRequest to return a byte[] instead
of writing to the instance field and remove the fileBytes field entirely, then
update sendGetRequest to call handleGetRequest and store the returned byte[] in
a local variable before using it to write the response; ensure all references to
the instance field (fileBytes) are replaced with the local variable and adjust
method signatures for handleGetRequest accordingly.
- Around line 26-27: The code creates File file = new File(WEB_ROOT, uri) and
reads it directly (Files.readAllBytes) allowing path traversal; fix by
canonical-path jail: resolve both WEB_ROOT and the resolved file via
getCanonicalPath() (and URL-decode/normalize uri beforehand), then verify the
fileCanonicalPath startsWith webRootCanonicalPath; if not, reject the request
(return 403 or equivalent) instead of calling Files.readAllBytes and caching
fileBytes. Apply this check in the StaticFileHandler where File file, fileBytes,
and Files.readAllBytes are used.
- Line 15: The FileCache instance is currently an instance field on
StaticFileHandler (private final FileCache cache = new FileCache()) so a new
empty cache is created per connection; change it to a shared cache so it
survives requests by making the field static final (e.g. private static final
FileCache CACHE = new FileCache()) or accept a FileCache via constructor
injection from the server so ConnectionHandler.runConnectionHandler() (invoked
by TcpServer.handleClient()) reuses the same cache; update all references to the
instance field to use the new static field name or the injected field and ensure
thread-safety in FileCache if needed.
---
Nitpick comments:
In `@src/main/java/org/example/FileCache.java`:
- Line 7: The cache field in FileCache is an unbounded HashMap (private final
Map<String, byte[]> cache) causing uncontrolled heap growth; replace it with a
bounded LRU cache implementation (for example, use a LinkedHashMap subclass with
accessOrder=true and override removeEldestEntry to enforce a max entries limit,
or replace with a Guava Cache created via CacheBuilder.maximumSize(n)) and apply
the same change to the related methods that read/write the cache (e.g.,
put/get/remove logic used around lines referenced) so entries are evicted when
the size limit is exceeded and memory is bounded.
In `@src/main/java/org/example/StaticFileHandler.java`:
- Line 21: Replace the two System.out.println calls in StaticFileHandler with a
proper logger: add a static final Logger (e.g., java.util.logging.Logger or
org.slf4j.Logger) on the StaticFileHandler class, initialize it with the class
name, and change the println statements (the "✓ Cache hit for: " and the other
occurrence) to appropriate logger calls (info/debug) using parameterized or
concatenated messages; remove the println usages so logging honours
configuration and avoids PrintStream synchronization overhead.
In `@src/test/java/org/example/FileCacheTest.java`:
- Around line 7-116: Add two tests to FileCacheTest: one named
testGetReturnIsIndependentOfCache that verifies get returns a defensive copy by
mutating the fetched byte[] and asserting subsequent cache.get("file.txt") still
equals the original bytes; and one simple concurrency test (e.g.,
testConcurrentPutAndGet) that uses a CountDownLatch and two threads to perform
concurrent cache.put and cache.get calls to exercise thread-safety (use
FileCache instance, assert final size/contents as expected). Ensure both tests
are annotated with `@Test` and use the existing cache field initialized in
setUp().
| import java.util.Map; | ||
|
|
||
| public class FileCache { | ||
| private final Map<String, byte []> cache = new HashMap<>(); |
There was a problem hiding this comment.
Thread-unsafe HashMap + mutable byte-array aliasing will corrupt the cache under concurrent load.
Two distinct problems compound each other:
HashMapis not thread-safe. A web server dispatches requests on multiple threads; concurrentput/get/containscalls can trigger a resize loop, returnnullfor existing keys, or silently lose entries.putstores the caller's array reference as-is, andgetreturns the internal reference directly. Any caller that later overwrites the returned array also overwrites the cached content.
🔒 Proposed fix — ConcurrentHashMap + defensive copies
-import java.util.HashMap;
-import java.util.Map;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
public class FileCache {
- private final Map<String, byte []> cache = new HashMap<>();
+ private final Map<String, byte[]> cache = new ConcurrentHashMap<>();
public byte[] get(String key) {
- return cache.get(key);
+ byte[] value = cache.get(key);
+ return value != null ? value.clone() : null;
}
public void put(String key, byte[] value) {
- cache.put(key, value);
+ cache.put(key, value.clone());
}Also applies to: 13-15, 17-19
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/FileCache.java` at line 7, The cache field uses a
non-thread-safe HashMap and stores mutable byte[] references; replace the Map
implementation with a thread-safe ConcurrentHashMap for the field
(FileCache.cache) and make defensive copies of byte arrays on entry and exit:
when storing (e.g., FileCache.put or wherever cache.put is called) copy the
incoming byte[] (Arrays.copyOf) before placing it into the map, and when
returning data (e.g., FileCache.get) return a copy of the stored byte[] rather
than the internal reference; this prevents concurrent corruption and aliasing of
mutable arrays.
viktorlindell12
left a comment
viktorlindell12
left a comment
There was a problem hiding this comment.
Bra jobbat.
Koden är tydlig och lätt att följa, bra uppdelning mellan de olika delarna.
Just nu konverteras byte[] till String i build(). Detta kan bli problem senare om servern ska hantera binära filer, till exempel bilder eller PDF:er. Ett alternativ kan vara att behålla body som byte[] hela vägen och skriva ut bytes direkt till output-streamen istället.
|
What do you think about converting this into a Filter instead of changing StaticFileHandler? The reason for this would be that we can enable/disable cache by adding or removing the filter and it will also work for content not returned from StaticFileHandler in the future? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/org/example/CacheFilter.java (1)
10-13: ReplaceSystem.out.printlnwith a proper logging framework.Bare
System.out.printlnwith Unicode symbols is not suitable for production. Usejava.util.loggingor SLF4J at an appropriate level (e.g.,FINE/DEBUGfor cache diagnostics). This also avoids I/O contention onstdoutunder load.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/CacheFilter.java` around lines 10 - 13, In CacheFilter, replace the two System.out.println calls around the cache.get(uri) return (the "✓ Cache hit for" and "✗ Cache miss for" prints) with a proper logger: add a private static final org.slf4j.Logger (or java.util.logging.Logger) instance at class scope and log the messages at DEBUG/FINE level (e.g., logger.debug("Cache hit for: {}", uri) / logger.debug("Cache miss for: {}", uri)); remove Unicode glyphs or keep them only if your logging policy allows, and update imports accordingly so you no longer use System.out.println in this class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/CacheFilter.java`:
- Around line 5-6: The current CacheFilter creates an unbounded FileCache
(private final FileCache cache = new FileCache()) which can leak memory; update
the cache to enforce size/TTL eviction. Replace or change the FileCache
instantiation in CacheFilter to use a bounded cache implementation (e.g., a
LinkedHashMap-based LRU with removeEldestEntry, or integrate Caffeine/Guava and
call Caffeine.newBuilder().maximumSize(...).expireAfterAccess(...).build()) and
ensure CacheFilter uses that bounded instance for get/put operations;
alternatively, add eviction logic inside FileCache (maxSize and
expireAfterAccess/expireAfterWrite) so CacheFilter keeps the same field but the
underlying implementation enforces limits.
---
Duplicate comments:
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 17-19: The lambda passed to cacheFilter.getOrFetch uses the raw
user-controlled path and creates new File(WEB_ROOT, path) without
canonicalization or containment checks; canonicalize both WEB_ROOT and the
resolved file (via File.getCanonicalPath or similar) inside the lambda used by
cacheFilter.getOrFetch, verify the resolved path startsWith the canonical
WEB_ROOT path, and if it does not throw/return a safe error (e.g.,
403/IllegalArgument) rather than reading the file; update the lambda referenced
in cacheFilter.getOrFetch, avoid using the raw path directly with
Files.readAllBytes, and ensure the check prevents traversal before calling
Files.readAllBytes.
- Line 14: The cache is currently instance-scoped (private final CacheFilter
cacheFilter in StaticFileHandler) so each request gets a fresh FileCache; change
this to a shared cache by making the CacheFilter or its underlying FileCache
static final or by wiring a singleton/shared CacheFilter into StaticFileHandler
(e.g., accept a CacheFilter in the constructor and have
ConnectionHandler.runConnectionHandler() pass a long-lived/shared instance).
Update the field declaration and constructor usages to use the shared instance
and remove per-request instantiation so cached entries survive across requests.
---
Nitpick comments:
In `@src/main/java/org/example/CacheFilter.java`:
- Around line 10-13: In CacheFilter, replace the two System.out.println calls
around the cache.get(uri) return (the "✓ Cache hit for" and "✗ Cache miss for"
prints) with a proper logger: add a private static final org.slf4j.Logger (or
java.util.logging.Logger) instance at class scope and log the messages at
DEBUG/FINE level (e.g., logger.debug("Cache hit for: {}", uri) /
logger.debug("Cache miss for: {}", uri)); remove Unicode glyphs or keep them
only if your logging policy allows, and update imports accordingly so you no
longer use System.out.println in this class.
# Conflicts: # src/main/java/org/example/StaticFileHandler.java
# Conflicts: # src/main/java/org/example/StaticFileHandler.java
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/main/java/org/example/http/HttpResponseBuilder.java (2)
45-48:setBody(byte[])does not defensively copy the input array.If a caller mutates the
byte[]after callingsetBody, the built response will silently contain the mutated data. Consider cloning defensively, especially since this handles binary file content that may come from a shared cache.Proposed defensive copy
public void setBody(byte[] body) { - this.bytebody = body; + this.bytebody = body != null ? body.clone() : null; this.body = ""; }🤖 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 45 - 48, The setBody(byte[] body) method currently stores the caller's array directly in the bytebody field which allows external mutations to alter the response; change setBody to defensively copy the input (e.g., clone or Arrays.copyOf) into this.bytebody and still clear this.body as before, and handle null inputs by setting this.bytebody to null when body is null to avoid NPEs—refer to the setBody(byte[]) method and the bytebody and body fields when making the change.
64-67: Javadoc uses/*instead of/**— won't be recognized as Javadoc.Fix
- /* + /** * Builds the complete HTTP response as a byte array and preserves binary content without corruption. * `@return` Complete HTTP response (headers + body) as byte[] */🤖 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 64 - 67, The comment above the method that builds the HTTP response in class HttpResponseBuilder is a block comment (/* ... */) instead of a Javadoc comment, so replace it with a proper Javadoc block (/** ... */) placed immediately above the response-building method (e.g., buildResponse or build) and keep the existing description and the `@return` tag so the method's documentation is generated correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Line 14: The Dockerfile COPY instruction "COPY /www/ /www/" is copying from
the build context (and uses a non-idiomatic leading slash); change it to copy
from the build stage by using the named stage (e.g., "COPY --from=build www/
/www/") and remove the leading slash on the source, or point to the correct
stage name used in the Dockerfile if it's not "build"; ensure the referenced
build stage exists and that the source path "www/" is present in that stage.
- Around line 11-16: The Dockerfile copies compiled classes to / but the
ENTRYPOINT uses -classpath /app:/dependencies/* so org.example.App won't be
found; fix by ensuring classes are placed under /app or update the classpath:
change the COPY --from=build /build/target/classes/ / to COPY --from=build
/build/target/classes/ /app/ (so WORKDIR /app/ contains the classes) or
alternatively update ENTRYPOINT to use -classpath /:/dependencies/*; modify the
COPY step that currently uses --from=build and the ENTRYPOINT invocation to keep
class file location and classpath consistent with org.example.App.
In `@README.md`:
- Around line 329-335: Update the README section so it no longer calls the
filter chain a "Future Feature" — indicate that a Filter Chain already exists
and that this PR adds CacheFilter; change the heading and prose to mention the
existing Filter Chain and explicitly list CacheFilter as implemented, and keep
other bullets (Request/response filtering, Authentication, Logging, Compression)
as planned/optional future additions; reference the symbols "Filter Chain" and
"CacheFilter" in the updated text so readers know caching is present and other
filters remain forthcoming.
- Around line 48-49: Remove the duplicated "git clone" token and correct the
directory name after cloning: change the first command to a single "git clone
https://github.com/ithsjava25/project-webserver-juv25g.git" and change the
subsequent "cd" target to "project-webserver-juv25g" (or use the repository
basename returned by the clone) so the two README commands are consistent.
---
Nitpick comments:
In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Around line 45-48: The setBody(byte[] body) method currently stores the
caller's array directly in the bytebody field which allows external mutations to
alter the response; change setBody to defensively copy the input (e.g., clone or
Arrays.copyOf) into this.bytebody and still clear this.body as before, and
handle null inputs by setting this.bytebody to null when body is null to avoid
NPEs—refer to the setBody(byte[]) method and the bytebody and body fields when
making the change.
- Around line 64-67: The comment above the method that builds the HTTP response
in class HttpResponseBuilder is a block comment (/* ... */) instead of a Javadoc
comment, so replace it with a proper Javadoc block (/** ... */) placed
immediately above the response-building method (e.g., buildResponse or build)
and keep the existing description and the `@return` tag so the method's
documentation is generated correctly.
Dockerfile
Outdated
| WORKDIR /app/ | ||
| COPY --from=build /build/target/classes/ / | ||
| COPY --from=build /build/target/dependency/ /dependencies/ | ||
| COPY /www/ /www/ |
There was a problem hiding this comment.
COPY /www/ /www/ copies from the build context, not from the build stage.
This COPY has no --from=build, so it pulls from the Docker build context (the repo root). If www/ doesn't exist in the repo at build time, the build will fail. Also, the leading / on the source path is unconventional — Docker resolves it relative to the build context anyway, but COPY www/ /www/ is the idiomatic form.
Proposed fix
-COPY /www/ /www/
+COPY www/ /www/📝 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
| COPY /www/ /www/ | |
| COPY www/ /www/ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` at line 14, The Dockerfile COPY instruction "COPY /www/ /www/" is
copying from the build context (and uses a non-idiomatic leading slash); change
it to copy from the build stage by using the named stage (e.g., "COPY
--from=build www/ /www/") and remove the leading slash on the source, or point
to the correct stage name used in the Dockerfile if it's not "build"; ensure the
referenced build stage exists and that the source path "www/" is present in that
stage.
Comment on lines
+48
to
+49
| git clone git clone https://github.com/ithsjava25/project-webserver-juv25g.git | ||
| cd project-webserver |
There was a problem hiding this comment.
Duplicate git clone and wrong directory name.
Line 48 has git clone repeated twice. Line 49 uses cd project-webserver but the repository name from the URL is project-webserver-juv25g.
Proposed fix
-git clone git clone https://github.com/ithsjava25/project-webserver-juv25g.git
-cd project-webserver
+git clone https://github.com/ithsjava25/project-webserver-juv25g.git
+cd project-webserver-juv25g📝 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 | |
| cd project-webserver | |
| git clone https://github.com/ithsjava25/project-webserver-juv25g.git | |
| cd project-webserver-juv25g |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 48 - 49, Remove the duplicated "git clone" token and
correct the directory name after cloning: change the first command to a single
"git clone https://github.com/ithsjava25/project-webserver-juv25g.git" and
change the subsequent "cd" target to "project-webserver-juv25g" (or use the
repository basename returned by the clone) so the two README commands are
consistent.
Comment on lines
+329
to
+335
| ### Filter Chain (Future Feature) | ||
|
|
||
| The project includes a filter chain interface for future extensibility: | ||
| - Request/response filtering | ||
| - Authentication | ||
| - Logging | ||
| - Compression |
There was a problem hiding this comment.
"Filter Chain (Future Feature)" is misleading — CacheFilter is being added in this PR.
This section labels the filter chain as a future feature, but this PR introduces CacheFilter. Consider updating the description to reflect that a filter chain already exists (with caching) and that additional filters (auth, compression, etc.) are planned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 329 - 335, Update the README section so it no longer
calls the filter chain a "Future Feature" — indicate that a Filter Chain already
exists and that this PR adds CacheFilter; change the heading and prose to
mention the existing Filter Chain and explicitly list CacheFilter as
implemented, and keep other bullets (Request/response filtering, Authentication,
Logging, Compression) as planned/optional future additions; reference the
symbols "Filter Chain" and "CacheFilter" in the updated text so readers know
caching is present and other filters remain forthcoming.
|
En bra början! |
…mproved CacheFilter with LRU eviction; updated StaticFileHandler to use shared cache; updated and added tests accordingly
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
src/test/java/org/example/StaticFileHandlerTest.java (2)
31-31: Unused fieldhandlerat class level.
handleris declared on line 31 but never assigned or used — every test method creates its own localhandler. Remove the field to avoid confusion.🧹 Proposed fix
class StaticFileHandlerTest { - - private StaticFileHandler handler;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/StaticFileHandlerTest.java` at line 31, Remove the unused class-level field "handler" declared in StaticFileHandlerTest; it is never assigned or referenced because each test creates its own local handler, so delete the line "private StaticFileHandler handler;" from the test class (StaticFileHandlerTest) to avoid confusion and keep tests clean, and run tests to confirm no compile errors remain.
206-225: Parameterized sanitization test covers key URI variants well.Covers query string, fragment, and leading slash. Consider adding a percent-encoded variant (e.g.,
%2Findex.html) to match the sanitization concern flagged inStaticFileHandler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/StaticFileHandlerTest.java` around lines 206 - 225, Add a percent-encoded URI variant to the parameterized test so the sanitization behavior is exercised for encoded slashes: update the CsvSource in StaticFileHandlerTest (the sanitized_uris_should_return_200 test) to include a value like "%2Findex.html" (and/or "%2Findex.html?foo=bar") and keep using StaticFileHandler.sendGetRequest to issue the request and assert the response still contains "HTTP/1.1 200 OK"; this ensures StaticFileHandler's sanitization logic is validated for percent-encoded inputs.src/main/java/org/example/CacheFilter.java (2)
11-17: Consider extracting aFileCacheinterface for testability and future flexibility.Both the PR reviewer (kappsegla) and prior learnings recommend making the cache an interface so
StaticFileHandlerdepends on an abstraction rather than the concreteCacheFilter. This enables swapping implementations (e.g., Redis, Caffeine) and simplifies unit-testing with mocks/stubs.Based on learnings: "FileCache should be refactored to be an interface to allow for different cache implementations (e.g., in-memory, Redis) and should be dependency-injected into StaticFileHandler."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/CacheFilter.java` around lines 11 - 17, Extract a FileCache interface and make StaticFileHandler depend on it instead of the concrete CacheFilter: define an interface (e.g., FileCache) exposing the cache operations used by StaticFileHandler (get, put, remove, size/byte accounting if needed), implement that interface with the existing CacheFilter class (rename or adapt methods in CacheFilter to match the interface), and inject FileCache into StaticFileHandler via constructor or setter so tests can substitute mocks/stubs and you can later add other implementations (Redis/Caffeine) without changing StaticFileHandler.
95-115: LRU eviction scans the entire map on every call — O(n) per eviction.
evictLeastRecentlyUsedstreams all entries to find the minimumlastAccessTime. Inside thewhile (shouldEvict)loop this is O(n) per eviction, and multiple evictions may be needed in a singleaddToCachecall, giving O(n²) worst case. For the current max of 100 entries this is tolerable, but aLinkedHashMapin access-order (or a min-heap / priority queue on access time) would make eviction O(1)/O(log n) and scale better if the limit is ever raised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/CacheFilter.java` around lines 95 - 115, evictLeastRecentlyUsed currently scans the entire cache map (cache) each time, causing O(n) per eviction and O(n²) worst-case in addToCache; replace this with an access-ordered LinkedHashMap or a priority queue to avoid full scans: convert the backing cache to a LinkedHashMap with accessOrder=true and implement removal by overriding removeEldestEntry or explicitly remove the first entry when evicting, ensuring access updates (lastAccessTime/accessCount) occur on every get/put so order stays correct; alternatively, maintain a PriorityQueue keyed by lastAccessTime and update the queue on access, then pop to evict; keep currentBytes adjustments (currentBytes.addAndGet) and System.out eviction logging in evictLeastRecentlyUsed (or the new eviction hook) and ensure thread-safety around cache, currentBytes and any queue modifications.src/main/java/org/example/StaticFileHandler.java (1)
14-19: Cache is a static singleton — prevents dependency injection and complicates testing.The
static final SHARED_CACHEfield prevents injecting a different cache implementation (e.g., for tests, Redis, Caffeine) and tightly couplesStaticFileHandlertoCacheFilter. Both the PR reviewer (kappsegla, donne41) and prior learnings recommend moving the cache instantiation to the server class and injecting it via the constructor.♻️ Sketch: constructor-injected cache
public class StaticFileHandler { private static final String DEFAULT_WEB_ROOT = "www"; - private static final CacheFilter SHARED_CACHE = new CacheFilter(); + private final CacheFilter cache; private final String webRoot; - public StaticFileHandler() { - this(DEFAULT_WEB_ROOT); + public StaticFileHandler(CacheFilter cache) { + this(DEFAULT_WEB_ROOT, cache); } - public StaticFileHandler(String webRoot) { + public StaticFileHandler(String webRoot, CacheFilter cache) { this.webRoot = webRoot; + this.cache = cache; }Based on learnings: "FileCache should be refactored to be an interface … and should be dependency-injected into StaticFileHandler."
🤖 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 14 - 19, The SHARED_CACHE static singleton ties StaticFileHandler to CacheFilter and blocks DI/testing; refactor by introducing a FileCache interface (or use existing CacheFilter to implement it), remove the static SHARED_CACHE field, add a FileCache parameter to StaticFileHandler's constructor and store it as an instance field, update all references inside StaticFileHandler to use this instance cache, and modify the server/bootstrap code to instantiate the desired cache implementation (e.g., new CacheFilter() or a test double/Redis/Caffeine implementation) and pass it into the StaticFileHandler constructor so tests can inject mocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/CacheFilter.java`:
- Around line 134-137: clearCache() performs cache.clear() and
currentBytes.set(0) separately, allowing a concurrent addToCache to race and
leave the map and counter inconsistent; protect both operations with the same
lock used by addToCache by wrapping the body of clearCache() in
synchronized(this) so cache.clear() and currentBytes.set(0) are executed
atomically under the same monitor as addToCache.
- Around line 68-90: The addToCache/shouldEvict logic can loop forever when the
incoming data.length > MAX_CACHE_BYTES; update addToCache to guard against
caching oversized items by first checking if data.length > MAX_CACHE_BYTES and
returning early (skip caching) or, alternately, change the eviction loop to
break if evictLeastRecentlyUsed finds the cache empty; specifically, in
addToCache add a pre-check using MAX_CACHE_BYTES and data.length and skip
adding, or after each evictLeastRecentlyUsed call verify cache.isEmpty() and
break to avoid an infinite loop. Ensure you reference addToCache, shouldEvict,
evictLeastRecentlyUsed and MAX_CACHE_BYTES in your change.
- Around line 44-63: The getOrFetch method has a TOCTOU race allowing multiple
threads to fetch the same URI; change it to perform an atomic check-and-populate
so only one thread calls provider.fetch(uri). Replace the containsKey/get
pattern in getOrFetch with a ConcurrentHashMap.computeIfAbsent-based flow (or a
per-key lock/Future pattern) that creates or returns a CacheEntry atomically;
inside the compute function call provider.fetch(uri) and construct the
CacheEntry, then call entry.recordAccess() for the returned entry (and still use
addToCache behavior only if you need eviction logic), ensuring cache
(ConcurrentHashMap), getOrFetch, addToCache, CacheEntry, and provider.fetch are
the touchpoints to update.
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 59-69: sanitizeUri currently never URL-decodes the incoming URI,
so percent-encoded traversal sequences like "%2e%2e" bypass expectations; update
sanitizeUri to first URL-decode the raw URI (e.g., using URLDecoder.decode(uri,
"UTF-8") or equivalent) inside a try/catch to handle malformed-encoding, then
strip query/fragment, null bytes, and leading slashes as before; ensure errors
fall back to a safe rejection (e.g., return null or throw a handled exception)
so downstream checks in isPathTraversal and normalize operate on the decoded
path.
- Around line 39-42: The shared static cache key currently uses only
sanitizedUri which allows different StaticFileHandler instances with different
webRoot values to collide; update the cache key to include a stable identifier
derived from webRoot (e.g., prefix the key with webRoot or its absolute
path/URI) when calling SHARED_CACHE.getOrFetch, and ensure the FileProvider
lambda still resolves the file using the original sanitizedUri and the handler's
webRoot (so strip the prefix or capture sanitizedUri separately). Reference
SHARED_CACHE, sanitizedUri, webRoot, and the StaticFileHandler code path that
calls SHARED_CACHE.getOrFetch; alternatively scope the cache per webRoot
instance if you prefer not to change keys.
- Around line 44-46: StaticFileHandler currently hardcodes the Content-Type
header to "text/html; charset=UTF-8" when building responses; instead call the
existing helper to derive the correct MIME type for the served file (use
MimeTypeDetector or HttpResponseBuilder.setContentTypeFromFilename) and set that
on the HttpResponseBuilder before setBody(fileBytes), ensuring text/* types
include charset=UTF-8 as appropriate.
In `@src/test/java/org/example/StaticFileHandlerTest.java`:
- Around line 104-137: The test spawns threads where assertion failures
(AssertionError) are not caught by the catch(IOException) and thus are lost, so
update testConcurrent_MultipleReads to propagate thread failures to the main
test: replace raw Thread usage with an ExecutorService submitting
Callables/Runnables and collect Futures (or use a shared
AtomicReference<Throwable>) and after starting all tasks call future.get() (or
check AtomicReference and rethrow if non-null) so any AssertionError from
handler.sendGetRequest/assertThat is rethrown in the main thread; reference
testConcurrent_MultipleReads, handler.sendGetRequest, and
StaticFileHandler.getCacheStats() when making the change.
---
Nitpick comments:
In `@src/main/java/org/example/CacheFilter.java`:
- Around line 11-17: Extract a FileCache interface and make StaticFileHandler
depend on it instead of the concrete CacheFilter: define an interface (e.g.,
FileCache) exposing the cache operations used by StaticFileHandler (get, put,
remove, size/byte accounting if needed), implement that interface with the
existing CacheFilter class (rename or adapt methods in CacheFilter to match the
interface), and inject FileCache into StaticFileHandler via constructor or
setter so tests can substitute mocks/stubs and you can later add other
implementations (Redis/Caffeine) without changing StaticFileHandler.
- Around line 95-115: evictLeastRecentlyUsed currently scans the entire cache
map (cache) each time, causing O(n) per eviction and O(n²) worst-case in
addToCache; replace this with an access-ordered LinkedHashMap or a priority
queue to avoid full scans: convert the backing cache to a LinkedHashMap with
accessOrder=true and implement removal by overriding removeEldestEntry or
explicitly remove the first entry when evicting, ensuring access updates
(lastAccessTime/accessCount) occur on every get/put so order stays correct;
alternatively, maintain a PriorityQueue keyed by lastAccessTime and update the
queue on access, then pop to evict; keep currentBytes adjustments
(currentBytes.addAndGet) and System.out eviction logging in
evictLeastRecentlyUsed (or the new eviction hook) and ensure thread-safety
around cache, currentBytes and any queue modifications.
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 14-19: The SHARED_CACHE static singleton ties StaticFileHandler to
CacheFilter and blocks DI/testing; refactor by introducing a FileCache interface
(or use existing CacheFilter to implement it), remove the static SHARED_CACHE
field, add a FileCache parameter to StaticFileHandler's constructor and store it
as an instance field, update all references inside StaticFileHandler to use this
instance cache, and modify the server/bootstrap code to instantiate the desired
cache implementation (e.g., new CacheFilter() or a test double/Redis/Caffeine
implementation) and pass it into the StaticFileHandler constructor so tests can
inject mocks.
In `@src/test/java/org/example/StaticFileHandlerTest.java`:
- Line 31: Remove the unused class-level field "handler" declared in
StaticFileHandlerTest; it is never assigned or referenced because each test
creates its own local handler, so delete the line "private StaticFileHandler
handler;" from the test class (StaticFileHandlerTest) to avoid confusion and
keep tests clean, and run tests to confirm no compile errors remain.
- Around line 206-225: Add a percent-encoded URI variant to the parameterized
test so the sanitization behavior is exercised for encoded slashes: update the
CsvSource in StaticFileHandlerTest (the sanitized_uris_should_return_200 test)
to include a value like "%2Findex.html" (and/or "%2Findex.html?foo=bar") and
keep using StaticFileHandler.sendGetRequest to issue the request and assert the
response still contains "HTTP/1.1 200 OK"; this ensures StaticFileHandler's
sanitization logic is validated for percent-encoded inputs.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/example/CacheFilter.javasrc/main/java/org/example/StaticFileHandler.javasrc/test/java/org/example/StaticFileHandlerTest.java
donne41
requested changes
Feb 23, 2026
donne41
left a comment
donne41
left a comment
There was a problem hiding this comment.
Jag känner mig lite sträng men tycker det är en del som är ändrat som inte behöver ha ändrats. Förutom det så tror jag vi börjar närma oss!
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/main/java/org/example/CacheFilter.java (3)
118-121:⚠️ Potential issue | 🟡 MinorMake
clearCache()atomic to avoid counter drift.
cache.clear()andcurrentBytes.set(0)can race withaddToCache, leaving the map and byte counter inconsistent. Synchronize the method or wrap both operations in a synchronized block.🛡️ Suggested fix
-public void clearCache() { +public synchronized void clearCache() { cache.clear(); currentBytes.set(0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/CacheFilter.java` around lines 118 - 121, The clearCache method can race with addToCache causing the cache map and currentBytes counter to drift; make clearCache atomic by synchronizing it (or wrapping its body in a synchronized block) using the same lock used by addToCache (e.g., a dedicated lock object or synchronized(this)) so that both cache.clear() and currentBytes.set(0) execute under the same lock as addToCache's updates; update clearCache to acquire that lock before clearing to ensure consistency between cache and currentBytes.
68-72:⚠️ Potential issue | 🔴 CriticalGuard against infinite eviction when a single entry exceeds the cache budget.
If
data.length > MAX_CACHE_BYTES,shouldEvictremains true forever andevictLeastRecentlyUsed()can’t make room once the cache is empty. Skip caching oversized entries (or break when empty).🐛 Suggested fix (oversize guard + empty check)
private synchronized void addToCache(String uri, byte[] data) { + if (data.length > MAX_CACHE_BYTES) { + return; // too large to ever fit + } // Kontrollera om vi måste evicta innan vi lägger till while (shouldEvict(data)) { + if (cache.isEmpty()) { + return; + } evictLeastRecentlyUsed(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/CacheFilter.java` around lines 68 - 72, The addToCache method can loop forever if the incoming data is larger than MAX_CACHE_BYTES because shouldEvict stays true even when the cache is empty; update addToCache (and its eviction loop) to first skip caching when data.length > MAX_CACHE_BYTES, and additionally break the eviction loop if the cache is already empty (e.g., after evictLeastRecentlyUsed returns no-op or you detect currentCacheBytes == 0), so that addToCache either abandons oversized entries or exits the loop safely before attempting endless evictions.
44-63:⚠️ Potential issue | 🟠 MajorAvoid TOCTOU on cache miss to prevent duplicate fetches.
containsKey+getis not atomic, so concurrent misses can trigger multipleprovider.fetch(uri)calls for the same resource. Consider a double-checked synchronized block (or acomputeIfAbsentapproach with exception handling) so only one thread fetches and populates.🔧 Suggested fix (double-check under lock)
public byte[] getOrFetch(String uri, FileProvider provider) throws IOException { - // Kontrollera cache (lock-free read) - if (cache.containsKey(uri)) { - CacheEntry entry = cache.get(uri); - if (entry != null) { - entry.recordAccess(); - System.out.println(" Cache hit for: " + uri); - return entry.data; - } - } + CacheEntry entry = cache.get(uri); + if (entry != null) { + entry.recordAccess(); + System.out.println(" Cache hit for: " + uri); + return entry.data; + } // Cache miss - fetch från provider System.out.println(" Cache miss for: " + uri); - byte[] fileBytes = provider.fetch(uri); - - // Lägg till i cache - addToCache(uri, fileBytes); - - return fileBytes; + synchronized (this) { + entry = cache.get(uri); + if (entry != null) { + entry.recordAccess(); + return entry.data; + } + byte[] fileBytes = provider.fetch(uri); + addToCache(uri, fileBytes); + return fileBytes; + } }Based on learnings: Ensure thread-safe FileCache implementations across the codebase. Do not use plain HashMap for shared caches. Use ConcurrentHashMap or proper synchronization to avoid data races when accessed by multiple request threads. Prefer thread-safe APIs (e.g., computeIfAbsent) and guard compound actions with locking if using custom synchronization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/CacheFilter.java` around lines 44 - 63, The getOrFetch method has a TOCTOU race because it uses cache.containsKey + cache.get causing duplicate provider.fetch(uri) under concurrent misses; change the cache to a ConcurrentHashMap (if not already) and replace the contains/get pattern with an atomic computeIfAbsent or a double-checked synchronized block around the cache population so only one thread invokes provider.fetch(uri) for a given uri; ensure you reference the CacheEntry creation path used by addToCache/CacheEntry so computeIfAbsent returns a fully-populated CacheEntry (or have computeIfAbsent store the fetched byte[] wrapped in a new CacheEntry) and make sure to handle and propagate provider.fetch exceptions without inserting null entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/example/CacheFilter.java`:
- Around line 118-121: The clearCache method can race with addToCache causing
the cache map and currentBytes counter to drift; make clearCache atomic by
synchronizing it (or wrapping its body in a synchronized block) using the same
lock used by addToCache (e.g., a dedicated lock object or synchronized(this)) so
that both cache.clear() and currentBytes.set(0) execute under the same lock as
addToCache's updates; update clearCache to acquire that lock before clearing to
ensure consistency between cache and currentBytes.
- Around line 68-72: The addToCache method can loop forever if the incoming data
is larger than MAX_CACHE_BYTES because shouldEvict stays true even when the
cache is empty; update addToCache (and its eviction loop) to first skip caching
when data.length > MAX_CACHE_BYTES, and additionally break the eviction loop if
the cache is already empty (e.g., after evictLeastRecentlyUsed returns no-op or
you detect currentCacheBytes == 0), so that addToCache either abandons oversized
entries or exits the loop safely before attempting endless evictions.
- Around line 44-63: The getOrFetch method has a TOCTOU race because it uses
cache.containsKey + cache.get causing duplicate provider.fetch(uri) under
concurrent misses; change the cache to a ConcurrentHashMap (if not already) and
replace the contains/get pattern with an atomic computeIfAbsent or a
double-checked synchronized block around the cache population so only one thread
invokes provider.fetch(uri) for a given uri; ensure you reference the CacheEntry
creation path used by addToCache/CacheEntry so computeIfAbsent returns a
fully-populated CacheEntry (or have computeIfAbsent store the fetched byte[]
wrapped in a new CacheEntry) and make sure to handle and propagate
provider.fetch exceptions without inserting null entries.
…ization logic, and optimize shared cache usage; streamlined test cases for clarity
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/org/example/StaticFileHandlerTest.java (1)
152-154:⚠️ Potential issue | 🟡 MinorDead test setup:
pageNotFound.htmlis never read by the handler.
StaticFileHandler.sendGetRequestcallssendErrorResponse(outputStream, 404, "Not Found")directly onIOException— it does not look up anypageNotFound.htmlfile from disk. TheFiles.writeString(testFile, "Fallback page")on line 154 has no effect on the test outcome and can be removed to keep the test's intent clear.🧹 Proposed cleanup
- // Pre-create the mandatory error page in the temp directory to prevent NoSuchFileException - Path testFile = tempDir.resolve("pageNotFound.html"); - Files.writeString(testFile, "Fallback page"); - - //Using the new constructor in StaticFileHandler to reroute so the tests uses the temporary folder instead of the hardcoded www StaticFileHandler staticFileHandler = new StaticFileHandler(tempDir.toString());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/StaticFileHandlerTest.java` around lines 152 - 154, The test creates a temp file "pageNotFound.html" but StaticFileHandler.sendGetRequest invokes sendErrorResponse(outputStream, 404, "Not Found") on IOException and never reads that file; remove the dead setup by deleting the Files.writeString(testFile, "Fallback page") call (leave the Path testFile = tempDir.resolve("pageNotFound.html") only if needed for other assertions or remove the Path creation too), so the test reflects the handler behavior in StaticFileHandler.sendGetRequest and no longer writes an unused fallback file.
♻️ Duplicate comments (3)
src/test/java/org/example/StaticFileHandlerTest.java (1)
89-122: Assertions inside threads are silently swallowed — test can falsely pass.
AssertionErrorfromassertThat(out.toString()).contains("200")is not a subtype ofIOException, so thecatch (IOException e)block on line 108 does not catch it. It escapes as an unchecked exception in the child thread;thread.join()does not rethrow it. Every inner assertion can fail while the test reports green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/StaticFileHandlerTest.java` around lines 89 - 122, The assertion inside the spawned threads in testConcurrent_MultipleReads can be lost because AssertionError thrown in child threads won’t propagate to the test thread; update the thread bodies to catch Throwable (not just IOException) and record any failure into a shared thread-safe container (e.g., AtomicReference<Throwable> or a ConcurrentLinkedQueue<Throwable>), then after joining all threads check the container and rethrow the first Throwable (or fail the test) so failures from the invoked handler.sendGetRequest/out assertions surface; alternatively replace raw Threads with an ExecutorService and submit Callables or Futures and call get() to propagate exceptions from sendGetRequest/assertions back to the main test thread.src/main/java/org/example/StaticFileHandler.java (2)
39-41: Cache key does not incorporatewebRoot— different handler instances will return stale/wrong file content.The cache key is only
sanitizedUri(e.g.,"index.html"). TwoStaticFileHandlerinstances with differentwebRootpaths that both serve"index.html"will share the same cache slot; whichever populates it first wins, and subsequent requests from the other root silently receive the wrong bytes.🤖 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 39 - 41, The cache key currently uses only sanitizedUri in SHARED_CACHE.getOrFetch (inside StaticFileHandler) which causes collisions across handlers with different webRoot; change the key to incorporate webRoot (e.g., combine webRoot's canonical/absolute path or realPath with sanitizedUri) so each (webRoot, sanitizedUri) pair is unique, and continue passing the sanitizedUri to the loader (Files.readAllBytes(new File(webRoot, path).toPath())); ensure you normalize/resolve webRoot to avoid equivalent-path mismatches before composing the key.
54-68:sanitizeUriskips URL-decoding — percent-encoded traversal sequences aren't normalized.A URI like
%2e%2e/%2e%2e/etc/passwdpasses through unmodified;normalize()inisPathTraversalnever sees..and cannot collapse the segments. Today this fails safely as a 404 (literal%2e%2edirectory doesn't exist), but the safe failure is incidental, not deliberate.🤖 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 54 - 68, The sanitizeUri method currently slices off query/fragment but never percent-decodes the path, so percent-encoded traversal like "%2e%2e" bypasses normalization; update sanitizeUri to URL-decode the path segment (after removing query/fragment but before the null/leading-slash cleanup) using UTF-8 (e.g., java.net.URLDecoder.decode(..., StandardCharsets.UTF_8)) and handle malformed-encoding exceptions by treating the URI as invalid (return null or equivalent) so that isPathTraversal's normalize/collapse logic sees actual ".." segments and can detect traversal correctly.
🧹 Nitpick comments (2)
src/main/java/org/example/CacheFilter.java (2)
53-53: ReplaceSystem.out.printlndiagnostics with a proper logger.Scattered
System.out.printlncalls throughout hit/miss/eviction paths are not configurable and cannot be suppressed in production or redirected to a log aggregator. Replace withjava.util.logging.Logger(or SLF4J if already on the classpath) atFINE/DEBUGlevel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/CacheFilter.java` at line 53, Replace the System.out.println diagnostics in CacheFilter (the print in the cache hit/miss/eviction paths) with a proper logger: add a class-level java.util.logging.Logger (or org.slf4j.Logger if available) instance in CacheFilter, remove the System.out.println call, and emit the message via logger.fine(...) (or logger.debug(...) for SLF4J) using the same URI context (e.g., "Cache hit for: " + uri) so messages are configurable by logging level and can be redirected in production.
13-13:CacheFiltershould be an interface to support swappable cache implementations.The class is named
CacheFilterbut is a concrete in-memory implementation.StaticFileHandlerhardwiresnew CacheFilter()as astatic finalfield, making it impossible to substitute a different cache backend (e.g., Redis, Caffeine, test-double) without changingStaticFileHandleritself.Extract a
FileCacheinterface exposinggetOrFetch,clearCache, andgetStats, rename this class to something likeInMemoryFileCache, and inject the interface intoStaticFileHandlervia its constructor.Based on learnings from kappsegla: FileCache should be refactored to be an interface to allow for different cache implementations (e.g., in-memory, Redis) and should be dependency-injected into StaticFileHandler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/CacheFilter.java` at line 13, The current concrete class CacheFilter should be extracted into a FileCache interface declaring getOrFetch(...), clearCache(), and getStats(), then rename the existing class to InMemoryFileCache implementing FileCache; update StaticFileHandler to stop using the static final new CacheFilter() and instead accept a FileCache in its constructor (store as a field) so callers can inject different implementations (e.g., RedisFileCache or test doubles), and update all call sites to use the new interface type and methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 69-78: The isPathTraversal method currently resolves symlinks only
for webRoot but not for the requested path, allowing symlink bypass; update
isPathTraversal so that after resolving webRoot via
Paths.get(webRoot).toRealPath() you also call toRealPath() on the resolved
requestedPath (instead of only normalize()) before checking
startsWith(webRootPath); keep the existing catch (IOException e) → return true
behavior to preserve the handling when the target does not exist.
---
Outside diff comments:
In `@src/test/java/org/example/StaticFileHandlerTest.java`:
- Around line 152-154: The test creates a temp file "pageNotFound.html" but
StaticFileHandler.sendGetRequest invokes sendErrorResponse(outputStream, 404,
"Not Found") on IOException and never reads that file; remove the dead setup by
deleting the Files.writeString(testFile, "Fallback page") call (leave the Path
testFile = tempDir.resolve("pageNotFound.html") only if needed for other
assertions or remove the Path creation too), so the test reflects the handler
behavior in StaticFileHandler.sendGetRequest and no longer writes an unused
fallback file.
---
Duplicate comments:
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 39-41: The cache key currently uses only sanitizedUri in
SHARED_CACHE.getOrFetch (inside StaticFileHandler) which causes collisions
across handlers with different webRoot; change the key to incorporate webRoot
(e.g., combine webRoot's canonical/absolute path or realPath with sanitizedUri)
so each (webRoot, sanitizedUri) pair is unique, and continue passing the
sanitizedUri to the loader (Files.readAllBytes(new File(webRoot,
path).toPath())); ensure you normalize/resolve webRoot to avoid equivalent-path
mismatches before composing the key.
- Around line 54-68: The sanitizeUri method currently slices off query/fragment
but never percent-decodes the path, so percent-encoded traversal like "%2e%2e"
bypasses normalization; update sanitizeUri to URL-decode the path segment (after
removing query/fragment but before the null/leading-slash cleanup) using UTF-8
(e.g., java.net.URLDecoder.decode(..., StandardCharsets.UTF_8)) and handle
malformed-encoding exceptions by treating the URI as invalid (return null or
equivalent) so that isPathTraversal's normalize/collapse logic sees actual ".."
segments and can detect traversal correctly.
In `@src/test/java/org/example/StaticFileHandlerTest.java`:
- Around line 89-122: The assertion inside the spawned threads in
testConcurrent_MultipleReads can be lost because AssertionError thrown in child
threads won’t propagate to the test thread; update the thread bodies to catch
Throwable (not just IOException) and record any failure into a shared
thread-safe container (e.g., AtomicReference<Throwable> or a
ConcurrentLinkedQueue<Throwable>), then after joining all threads check the
container and rethrow the first Throwable (or fail the test) so failures from
the invoked handler.sendGetRequest/out assertions surface; alternatively replace
raw Threads with an ExecutorService and submit Callables or Futures and call
get() to propagate exceptions from sendGetRequest/assertions back to the main
test thread.
---
Nitpick comments:
In `@src/main/java/org/example/CacheFilter.java`:
- Line 53: Replace the System.out.println diagnostics in CacheFilter (the print
in the cache hit/miss/eviction paths) with a proper logger: add a class-level
java.util.logging.Logger (or org.slf4j.Logger if available) instance in
CacheFilter, remove the System.out.println call, and emit the message via
logger.fine(...) (or logger.debug(...) for SLF4J) using the same URI context
(e.g., "Cache hit for: " + uri) so messages are configurable by logging level
and can be redirected in production.
- Line 13: The current concrete class CacheFilter should be extracted into a
FileCache interface declaring getOrFetch(...), clearCache(), and getStats(),
then rename the existing class to InMemoryFileCache implementing FileCache;
update StaticFileHandler to stop using the static final new CacheFilter() and
instead accept a FileCache in its constructor (store as a field) so callers can
inject different implementations (e.g., RedisFileCache or test doubles), and
update all call sites to use the new interface type and methods.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/example/CacheFilter.javasrc/main/java/org/example/StaticFileHandler.javasrc/test/java/org/example/StaticFileHandlerTest.java
…roved StaticFileHandler path traversal logic for better security and simpler validation
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/main/java/org/example/StaticFileHandler.java (1)
105-119:⚠️ Potential issue | 🟠 MajorSymlink traversal still bypasses the path-jail check.
The comment on lines 107–108 states that
toAbsolutePath().normalize()handles symlinks safely, but it does the opposite —normalize()only collapses textual..segments and does not resolve symlinks. A symlink insidewebRootpointing to/etc/passwdwill passstartsWith(webRootPath)and thenFiles.readAllByteson line 48 will follow the symlink and serve the target file.The previous review already flagged this and suggested using
toRealPath()on the resolved requested path. The current code explicitly chose not to, citing "to avoid requirements on directory existence," but the try/catch on line 114 already handles theIOExceptionthattoRealPath()throws for non-existent files.🛡️ Proposed fix
private boolean isPathTraversal(String uri) { try { - Path webRootPath = Paths.get(webRoot).toAbsolutePath().normalize(); - Path requestedPath = webRootPath.resolve(uri).normalize(); - - return !requestedPath.startsWith(webRootPath); + Path webRootPath = Paths.get(webRoot).toRealPath(); + Path requestedPath = webRootPath.resolve(uri).toRealPath(); + return !requestedPath.startsWith(webRootPath); } catch (Exception e) { + // toRealPath() throws IOException for non-existent files — safe default return true; } }🤖 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 105 - 119, The isPathTraversal method is still vulnerable to symlink bypass because normalize() doesn't resolve symlinks; change the requestedPath resolution to resolve symlinks by calling toRealPath(...) on the resolved path (e.g., replace requestedPath = webRootPath.resolve(uri).normalize() with requestedPath = webRootPath.resolve(uri).toRealPath() or similar) before performing the startsWith(webRootPath) check so any symlink targets are resolved and caught; keep the existing try/catch (it already handles the IOException from toRealPath) and ensure the final comparison still uses the canonical/real webRootPath if needed.
🧹 Nitpick comments (3)
src/main/java/org/example/CacheFilter.java (2)
36-40: Cached byte arrays are returned by reference — callers can silently corrupt the cache.
CacheEntrystores and exposes the rawdatareference. If any caller mutates the returnedbyte[], the cached content is corrupted for all subsequent requests. Current callers (StaticFileHandler) don't modify the array, so this isn't an active bug, but it's fragile.🛡️ Defensive copy on read
// In getOrFetch, at cache-hit return sites: - return entry.data; + return entry.data.clone();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/CacheFilter.java` around lines 36 - 40, CacheEntry currently stores and exposes the raw byte[] which allows callers to mutate cached content; update CacheEntry to defensively copy data on input and/or on output: in the CacheEntry(byte[] data) constructor copy the array into a new byte[] before assigning to the data field, and ensure any accessor (e.g., getData or usages that return entry.data) returns a copy rather than the internal reference so callers cannot mutate the cache.
52-84: All cache misses serialize onwriteLock, including disk I/O.
provider.fetch(uri)(disk read) executes while holdingwriteLock(line 76). This means two concurrent requests for different uncached URIs block each other. Under a burst of cold-cache requests this becomes a bottleneck.Consider a per-key locking or
Future-based pattern so only requests for the same key wait on each other, while different keys can fetch concurrently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/CacheFilter.java` around lines 52 - 84, The method getOrFetch currently performs provider.fetch(uri) while holding the shared writeLock, causing unrelated cache misses to serialize; change to a per-key in-flight/future pattern so only requests for the same uri block each other: keep the initial lock-free cache.get(uri) check and early return, then use a ConcurrentHashMap<String, CompletableFuture<byte[]>> (e.g., inFlightFetches) and call computeIfAbsent(uri, key -> CompletableFuture.supplyAsync(() -> provider.fetch(key))) to start the fetch outside the global writeLock; when the future completes, if bytes != null call addToCacheUnsafe(uri, bytes) under the writeLock (or via a short synchronized block) and remove the future from inFlightFetches; finally return the future.join() (or get) result and ensure recordAccess() is still called for cache hits (including after concurrent fetch completes).src/main/java/org/example/StaticFileHandler.java (1)
18-34: Shared cache is hardcoded rather than injected — consider constructor injection.
SHARED_CACHEisstatic final new CacheFilter(), which couplesStaticFileHandlerto the concrete implementation. TheFileCacheinterface was introduced specifically to enable pluggable backends. Accepting aFileCachevia constructor (or a static setter for the shared instance) would complete the decoupling and simplify testing.🤖 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 - 34, SHARED_CACHE is tightly coupled to CacheFilter; change StaticFileHandler to accept a FileCache instead of hardcoding new CacheFilter: add a constructor overload StaticFileHandler(FileCache cache, String webRoot) (and a convenience StaticFileHandler(FileCache cache) or static setter if a shared instance is required), store the injected cache in a non-static or static field (replace SHARED_CACHE usage), and update any code paths that reference SHARED_CACHE to use the injected FileCache so tests can pass mock implementations and different backends can be provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/FileCache.java`:
- Around line 57-64: The toString() in CacheStats can produce Infinity/NaN when
maxBytes is zero; update CacheStats.toString() to guard the division by zero
(use a conditional like maxBytes > 0 ? (double) bytes / maxBytes * 100 : 0.0)
when computing utilization so the formatted string shows a sane percentage
(e.g., 0.0%) instead of Infinity/NaN; keep the same String.format call and
replace the utilization expression only, referencing entries, maxEntries, bytes,
maxBytes, and totalAccesses in CacheStats.toString().
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 76-98: In sanitizeUri change the query/fragment index checks so a
delimiter at position 0 is recognized: compute queryIndex and fragmentIndex
using uri.indexOf('?') / uri.indexOf('#') and treat values >= 0 (or use != -1)
instead of using > 0 when building endIndex; update the endIndex calculation
that chooses the minimum of the two adjusted bounds so leading '?' or '#' are
stripped from the returned path (function: sanitizeUri).
- Around line 100-101: The duplicate Javadoc opener in the StaticFileHandler
class leads to malformed documentation; remove the extra `/**` so only a single
Javadoc block surrounds the intended comment (or if the intent was two separate
docs, ensure each has its own opening `/**` and matching `*/`). Locate the
duplicate `/**` near the top of class StaticFileHandler and either delete the
stray opener or add the missing closing `*/` to properly terminate the first
block, ensuring Javadoc syntax is correct for the affected comment block.
In `@src/test/java/org/example/StaticFileHandlerTest.java`:
- Around line 82-128: The testConcurrent_MultipleReads test currently only
records AssertionError from child threads and wraps IOExceptions in
RuntimeException which can terminate threads silently; update the thread run()
catch logic in testConcurrent_MultipleReads so it captures any Throwable
(including IOException/RuntimeException) into the shared assertionErrors slot
(or a new Throwable[] errors) while synchronizing on threads before rethrowing,
so the main thread can detect and rethrow the first child exception after join;
reference handler.sendGetRequest(...) and the threads[]/assertionErrors[] usage
when making this change.
---
Duplicate comments:
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 105-119: The isPathTraversal method is still vulnerable to symlink
bypass because normalize() doesn't resolve symlinks; change the requestedPath
resolution to resolve symlinks by calling toRealPath(...) on the resolved path
(e.g., replace requestedPath = webRootPath.resolve(uri).normalize() with
requestedPath = webRootPath.resolve(uri).toRealPath() or similar) before
performing the startsWith(webRootPath) check so any symlink targets are resolved
and caught; keep the existing try/catch (it already handles the IOException from
toRealPath) and ensure the final comparison still uses the canonical/real
webRootPath if needed.
---
Nitpick comments:
In `@src/main/java/org/example/CacheFilter.java`:
- Around line 36-40: CacheEntry currently stores and exposes the raw byte[]
which allows callers to mutate cached content; update CacheEntry to defensively
copy data on input and/or on output: in the CacheEntry(byte[] data) constructor
copy the array into a new byte[] before assigning to the data field, and ensure
any accessor (e.g., getData or usages that return entry.data) returns a copy
rather than the internal reference so callers cannot mutate the cache.
- Around line 52-84: The method getOrFetch currently performs
provider.fetch(uri) while holding the shared writeLock, causing unrelated cache
misses to serialize; change to a per-key in-flight/future pattern so only
requests for the same uri block each other: keep the initial lock-free
cache.get(uri) check and early return, then use a ConcurrentHashMap<String,
CompletableFuture<byte[]>> (e.g., inFlightFetches) and call computeIfAbsent(uri,
key -> CompletableFuture.supplyAsync(() -> provider.fetch(key))) to start the
fetch outside the global writeLock; when the future completes, if bytes != null
call addToCacheUnsafe(uri, bytes) under the writeLock (or via a short
synchronized block) and remove the future from inFlightFetches; finally return
the future.join() (or get) result and ensure recordAccess() is still called for
cache hits (including after concurrent fetch completes).
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 18-34: SHARED_CACHE is tightly coupled to CacheFilter; change
StaticFileHandler to accept a FileCache instead of hardcoding new CacheFilter:
add a constructor overload StaticFileHandler(FileCache cache, String webRoot)
(and a convenience StaticFileHandler(FileCache cache) or static setter if a
shared instance is required), store the injected cache in a non-static or static
field (replace SHARED_CACHE usage), and update any code paths that reference
SHARED_CACHE to use the injected FileCache so tests can pass mock
implementations and different backends can be provided.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/example/CacheFilter.javasrc/main/java/org/example/FileCache.javasrc/main/java/org/example/StaticFileHandler.javasrc/test/java/org/example/StaticFileHandlerTest.java
Comment on lines
+57
to
+64
| @Override | ||
| public String toString() { | ||
| return String.format( | ||
| "CacheStats{entries=%d/%d, bytes=%d/%d, utilization=%.1f%%, accesses=%d}", | ||
| entries, maxEntries, bytes, maxBytes, | ||
| (double) bytes / maxBytes * 100, totalAccesses | ||
| ); | ||
| } |
There was a problem hiding this comment.
toString() produces Infinity% when maxBytes is zero.
If a CacheStats is ever constructed with maxBytes == 0, the expression (double) bytes / maxBytes * 100 yields Infinity or NaN, producing an unreadable string.
🛡️ Proposed fix
return String.format(
"CacheStats{entries=%d/%d, bytes=%d/%d, utilization=%.1f%%, accesses=%d}",
entries, maxEntries, bytes, maxBytes,
- (double) bytes / maxBytes * 100, totalAccesses
+ maxBytes > 0 ? (double) bytes / maxBytes * 100 : 0.0, totalAccesses
);📝 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
| @Override | |
| public String toString() { | |
| return String.format( | |
| "CacheStats{entries=%d/%d, bytes=%d/%d, utilization=%.1f%%, accesses=%d}", | |
| entries, maxEntries, bytes, maxBytes, | |
| (double) bytes / maxBytes * 100, totalAccesses | |
| ); | |
| } | |
| `@Override` | |
| public String toString() { | |
| return String.format( | |
| "CacheStats{entries=%d/%d, bytes=%d/%d, utilization=%.1f%%, accesses=%d}", | |
| entries, maxEntries, bytes, maxBytes, | |
| maxBytes > 0 ? (double) bytes / maxBytes * 100 : 0.0, totalAccesses | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/FileCache.java` around lines 57 - 64, The
toString() in CacheStats can produce Infinity/NaN when maxBytes is zero; update
CacheStats.toString() to guard the division by zero (use a conditional like
maxBytes > 0 ? (double) bytes / maxBytes * 100 : 0.0) when computing utilization
so the formatted string shows a sane percentage (e.g., 0.0%) instead of
Infinity/NaN; keep the same String.format call and replace the utilization
expression only, referencing entries, maxEntries, bytes, maxBytes, and
totalAccesses in CacheStats.toString().
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/test/java/org/example/StaticFileHandlerTest.java (1)
93-94:⚠️ Potential issue | 🟡 MinorChild-thread
AssertionErroris not captured.Line 108 catches
Exception, but Line 105 throwsAssertionError(Error), so thread assertions can be lost.💚 Proposed fix
- final Exception[] threadError = new Exception[1]; + final Throwable[] threadError = new Throwable[1]; ... - } catch (Exception e) { + } catch (Throwable e) { synchronized (threads) { - threadError[0] = e; + if (threadError[0] == null) { + threadError[0] = e; + } } } ... if (threadError[0] != null) { throw new AssertionError("Tråd-fel: " + threadError[0].getMessage(), threadError[0]); }Also applies to: 105-110, 123-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/StaticFileHandlerTest.java` around lines 93 - 94, The test spawns a child thread but only captures Exception in final Exception[] threadError, so AssertionError (which extends Error/Throwable) can be lost; update the capture to use Throwable (e.g., final Throwable[] threadError or an AtomicReference<Throwable>) and change the thread's catch block to catch Throwable instead of Exception, then after joining rethrow or fail with the captured Throwable (preserving AssertionError) so assertions in the child thread are surfaced; reference the threadError variable and the thread's run/catch block where the error is stored and checked.src/main/java/org/example/StaticFileHandler.java (2)
85-87:⚠️ Potential issue | 🟡 MinorFix off-by-one in query/fragment stripping.
Line 85 and Line 86 should treat index
0as a found delimiter; otherwise URIs like"?x=1"are not sanitized correctly.🐛 Proposed fix
int endIndex = Math.min( - queryIndex > 0 ? queryIndex : uri.length(), - fragmentIndex > 0 ? fragmentIndex : uri.length() + queryIndex >= 0 ? queryIndex : uri.length(), + fragmentIndex >= 0 ? fragmentIndex : uri.length() );🤖 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 85 - 87, The substring used to strip query/fragment in StaticFileHandler is treating index 0 as "not found", so URIs starting with '?' or '#' (e.g., "?x=1") are not sanitized; update the logic that computes the end index for the URI substring (the code using queryIndex and fragmentIndex) to treat an index >= 0 as a found delimiter (i.e., use queryIndex >= 0 ? queryIndex : uri.length() and fragmentIndex >= 0 ? fragmentIndex : uri.length()), then recompute the min of those two values before substringing to ensure leading '?' or '#' are correctly stripped.
108-117:⚠️ Potential issue | 🟠 MajorPath-jail check is still bypassable via symlinks.
Line 112-116 performs a lexical containment check only; a symlink inside
webRootcan still resolve to a target outsidewebRoot.🛡️ Proposed fix
private boolean isPathTraversal(String uri) { try { - Path webRootPath = Paths.get(webRoot).toAbsolutePath().normalize(); - Path requestedPath = webRootPath.resolve(uri).normalize(); + Path webRootPath = Paths.get(webRoot).toFile().getCanonicalFile().toPath(); + Path requestedPath = webRootPath.resolve(uri).toFile().getCanonicalFile().toPath(); return !requestedPath.startsWith(webRootPath); } catch (Exception e) { LOGGER.log(Level.WARNING, "Sökvägstraversalkontroll misslyckades för: " + uri, e); return true; } }🤖 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 108 - 117, The lexical check in isPathTraversal (webRootPath/requestedPath) can be bypassed by symlinks; change the function to compare real (canonical) paths instead: call toRealPath() on the web root (e.g., realWebRoot = Paths.get(webRoot).toRealPath()) and obtain the real path of the requested target by resolving uri against the web root and then calling toRealPath() on the nearest existing ancestor if the final target may not exist (find existing parent via Files.exists and climb up until exists, call toRealPath() on that parent and reapply the non-existing remainder) to reconstruct the canonical requested path; finally return true if the canonical requested path does not startWith the canonical web root. Ensure all references use the isPathTraversal, webRootPath, requestedPath, realWebRoot identifiers.src/main/java/org/example/FileCache.java (1)
62-66:⚠️ Potential issue | 🟡 MinorGuard utilization formatting for
maxBytes == 0.Line 65 can output
Infinity%/NaN%whenmaxBytesis zero.🛡️ Proposed fix
return String.format( "CacheStats{entries=%d/%d, bytes=%s/%s, utilization=%.1f%%, accesses=%d}", entries, maxEntries, bytesFormatted, maxBytesFormatted, - (double) bytes / maxBytes * 100, totalAccesses + maxBytes > 0 ? (double) bytes / maxBytes * 100 : 0.0, totalAccesses );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/FileCache.java` around lines 62 - 66, The CacheStats string formatting in FileCache can produce Infinity/NaN when maxBytes == 0; update the code that builds the utilization value (used inside the String.format call) to guard against division by zero by computing a safe utilization double first (e.g., if maxBytes == 0 then set utilization to 0.0 else compute (double) bytes / maxBytes * 100) and use that local variable in the String.format invocation so the log never prints Infinity/NaN; reference the FileCache method that returns the formatted CacheStats (the String.format call using entries, maxEntries, bytesFormatted, maxBytesFormatted, bytes, maxBytes, totalAccesses).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/CacheFilter.java`:
- Around line 160-163: The getStats() method currently returns null causing
NPEs; update CacheFilter.getStats() to return a non-null FileCache.CacheStats
instance (either by delegating to the underlying cache/stats provider used
elsewhere in CacheFilter or by constructing and returning an empty/default
FileCache.CacheStats) and ensure all other methods in the same block (lines
around the getStats implementation, e.g., methods referenced between lines
166-199) similarly avoid returning null by using the same underlying stats
source or default objects; locate and use the same cache field or stats helper
used elsewhere in CacheFilter to populate the returned stats.
---
Duplicate comments:
In `@src/main/java/org/example/FileCache.java`:
- Around line 62-66: The CacheStats string formatting in FileCache can produce
Infinity/NaN when maxBytes == 0; update the code that builds the utilization
value (used inside the String.format call) to guard against division by zero by
computing a safe utilization double first (e.g., if maxBytes == 0 then set
utilization to 0.0 else compute (double) bytes / maxBytes * 100) and use that
local variable in the String.format invocation so the log never prints
Infinity/NaN; reference the FileCache method that returns the formatted
CacheStats (the String.format call using entries, maxEntries, bytesFormatted,
maxBytesFormatted, bytes, maxBytes, totalAccesses).
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 85-87: The substring used to strip query/fragment in
StaticFileHandler is treating index 0 as "not found", so URIs starting with '?'
or '#' (e.g., "?x=1") are not sanitized; update the logic that computes the end
index for the URI substring (the code using queryIndex and fragmentIndex) to
treat an index >= 0 as a found delimiter (i.e., use queryIndex >= 0 ? queryIndex
: uri.length() and fragmentIndex >= 0 ? fragmentIndex : uri.length()), then
recompute the min of those two values before substringing to ensure leading '?'
or '#' are correctly stripped.
- Around line 108-117: The lexical check in isPathTraversal
(webRootPath/requestedPath) can be bypassed by symlinks; change the function to
compare real (canonical) paths instead: call toRealPath() on the web root (e.g.,
realWebRoot = Paths.get(webRoot).toRealPath()) and obtain the real path of the
requested target by resolving uri against the web root and then calling
toRealPath() on the nearest existing ancestor if the final target may not exist
(find existing parent via Files.exists and climb up until exists, call
toRealPath() on that parent and reapply the non-existing remainder) to
reconstruct the canonical requested path; finally return true if the canonical
requested path does not startWith the canonical web root. Ensure all references
use the isPathTraversal, webRootPath, requestedPath, realWebRoot identifiers.
In `@src/test/java/org/example/StaticFileHandlerTest.java`:
- Around line 93-94: The test spawns a child thread but only captures Exception
in final Exception[] threadError, so AssertionError (which extends
Error/Throwable) can be lost; update the capture to use Throwable (e.g., final
Throwable[] threadError or an AtomicReference<Throwable>) and change the
thread's catch block to catch Throwable instead of Exception, then after joining
rethrow or fail with the captured Throwable (preserving AssertionError) so
assertions in the child thread are surfaced; reference the threadError variable
and the thread's run/catch block where the error is stored and checked.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/example/CacheFilter.javasrc/main/java/org/example/FileCache.javasrc/main/java/org/example/StaticFileHandler.javasrc/test/java/org/example/StaticFileHandlerTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/org/example/StaticFileHandler.java (2)
84-87: Off-by-one:> 0should be>= 0for query/fragment index checks.If the URI begins with
?or#(index 0),> 0evaluates tofalseand the delimiter is not stripped, leaving the query/fragment as part of the filename. Use>= 0(or!= -1).🐛 Proposed fix
int endIndex = Math.min( - queryIndex > 0 ? queryIndex : uri.length(), - fragmentIndex > 0 ? fragmentIndex : uri.length() + queryIndex >= 0 ? queryIndex : uri.length(), + fragmentIndex >= 0 ? fragmentIndex : uri.length() );🤖 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 84 - 87, The endIndex computation in StaticFileHandler is using `queryIndex > 0` and `fragmentIndex > 0`, which misses delimiters at position 0 (e.g., URIs starting with '?' or '#'); update those checks to use `>= 0` (or `!= -1`) so that queryIndex and fragmentIndex at index 0 are treated as present; modify the expression that sets endIndex (and any similar checks around queryIndex/fragmentIndex) to use `>= 0` to ensure the query/fragment are stripped from the filename correctly.
108-122:normalize()alone does not resolve symlinks — symlink-based traversal is still possible.The comment at lines 110-111 acknowledges skipping
toRealPath()to avoid requiring directory existence, but this leaves the door open for symlink-based escapes if an attacker can create a symlink insidewebRootpointing outside it. The actual file read at line 48 (Files.readAllBytes(new File(webRoot, sanitizedUri).toPath())) will follow symlinks.If this is an accepted trade-off, consider documenting it more explicitly (e.g., "symlinks inside webRoot are trusted"). Otherwise, calling
toRealPath()on the resolved path (and falling through to the existingcatch → return true) would close this gap.🤖 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 108 - 122, The isPathTraversal method currently uses Paths.get(...).toAbsolutePath().normalize() which does not resolve symlinks, so a symlink inside webRoot can still escape when the file is later read with Files.readAllBytes(new File(webRoot, sanitizedUri).toPath()); either explicitly resolve symlinks by calling toRealPath() on the resolved path inside isPathTraversal (and let the existing catch return true on failure) or, if symlinks inside webRoot are an accepted risk, add a clear comment/docstring in isPathTraversal and near the file read (Files.readAllBytes(...)) stating that symlinks in webRoot are trusted and this is an intentional trade-off.
🧹 Nitpick comments (4)
src/main/java/org/example/StaticFileHandler.java (1)
18-23:SHARED_CACHEis hardcoded — prevents dependency injection and testability.The
FileCacheinterface is a good step (per reviewer feedback), but instantiating it asstatic final … = new CacheFilter()couples this class to the concrete implementation and makes it impossible to inject alternatives (e.g., for testing or future Redis-backed caches). Consider accepting aFileCachevia constructor parameter and managing the shared instance at the server/wiring level.Based on learnings, FileCache should be dependency-injected into StaticFileHandler rather than instantiated as a static field, to allow for different cache implementations (e.g., in-memory, Redis).
🤖 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 - 23, SHARED_CACHE is currently tightly coupled to CacheFilter because StaticFileHandler instantiates it as a static final; change to accept a FileCache via dependency injection instead: remove the static instantiation of SHARED_CACHE, add a (final) instance field FileCache cache and a constructor StaticFileHandler(FileCache cache) that assigns it, and update any call sites that relied on the static SHARED_CACHE to pass a shared FileCache (created at server/wiring/bootstrap time) so tests can inject mocks or alternate implementations like Redis-backed caches; keep a default constructor only if you provide a reasonable default cache via delegation in wiring, not by hardcoding new CacheFilter() inside StaticFileHandler.src/main/java/org/example/CacheFilter.java (3)
30-45:CacheEntry.datais a mutablebyte[]— callers can corrupt cached content.
CacheEntrystores the rawbyte[]reference. Any caller that mutates the returned array (e.g.,getOrFetch→ consumer modifies the bytes) will silently corrupt the cached data for all subsequent hits. Defensively copy on store and/or on retrieval.♻️ Defensive copy on store
CacheEntry(byte[] data) { - this.data = data; + this.data = data.clone(); this.lastAccessTime = new AtomicLong(System.currentTimeMillis()); this.accessCount = new AtomicLong(0); }Alternatively, clone on retrieval in
getOrFetch(line 58 / 68) — pick whichever trade-off (memory vs. safety) fits your use case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/CacheFilter.java` around lines 30 - 45, CacheEntry currently stores a direct reference to a mutable byte[] (field CacheEntry.data) which allows callers to mutate cached content; to fix, make CacheEntry defensive by copying the array on creation (in CacheEntry(byte[] data)) and/or by returning a copy from the cache access path (e.g., in getOrFetch) so callers never get the original backing array; update CacheEntry constructor to clone the incoming byte[] and ensure any place that returns CacheEntry.data (notably getOrFetch) returns a clone of the stored array to prevent external mutation from corrupting the cache.
156-169:getStats()is not synchronized — snapshot may be inconsistent with concurrent mutations.
cache.size(),currentBytes.get(), and the access-count stream are read without holdingwriteLock, so a concurrentaddToCacheUnsafe/evictcan interleave and produce a snapshot where, e.g.,entriesandbytesdon't correspond. For a diagnostics-only method this is likely acceptable — just flagging for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/CacheFilter.java` around lines 156 - 169, getStats() currently reads cache.size(), currentBytes.get(), and the access-count stream without locking, allowing concurrent addToCacheUnsafe/evict to produce an inconsistent snapshot; fix by acquiring the same lock used for mutations (use writeLock.lock() / writeLock.unlock() around the snapshot read) so the returned FileCache.CacheStats reflects a consistent state—apply this change inside getStats() and reference writeLock, getStats, addToCacheUnsafe, and evict when locating the code to modify.
62-82: All cache misses are serialized through a single global lock — potential throughput bottleneck.
synchronized(writeLock)guards the entire fetch-and-store path. If threads request different, uncached URIs concurrently, they queue behind the same lock — including the disk I/O inprovider.fetch(uri). Under load this serializes all first-time file reads.A per-key locking pattern (e.g.,
ConcurrentHashMap<String, Future<byte[]>>withcomputeIfAbsent) would allow concurrent fetches for distinct URIs while still coalescing duplicate requests for the same URI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/CacheFilter.java` around lines 62 - 82, The current synchronized(writeLock) serializes all cache misses including provider.fetch(uri); replace the global lock with a per-key in-flight map (e.g., ConcurrentHashMap<String, CompletableFuture<byte[]>> or Future) so only identical URIs are coalesced while distinct URIs fetch concurrently: on lookup use cache.get(uri) then if missing use inFlight.computeIfAbsent(uri, k -> CompletableFuture.supplyAsync(() -> { byte[] bytes = provider.fetch(k); if (bytes != null) addToCacheUnsafe(k, bytes); return bytes; })); wait/join the future, remove the future from inFlight on completion/exception, and preserve entry.recordAccess/LOGGER logic when returning cached data; this keeps addToCacheUnsafe, cache, provider.fetch, writeLock (remove or narrow usage) and LOGGER references but moves I/O out of the global synchronized block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 36-61: In sendGetRequest, the catch block logs the raw
user-provided uri which can enable log-forging; update the
LOGGER.log(Level.FINE, ...) call in the catch to use sanitizedUri (or an
escaped/normalized representation of it) instead of the unsanitized uri so only
validated/escaped input is logged; locate the LOGGER.log call in sendGetRequest
and replace the logged variable accordingly (optionally apply a simple escape
function before logging).
---
Duplicate comments:
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 84-87: The endIndex computation in StaticFileHandler is using
`queryIndex > 0` and `fragmentIndex > 0`, which misses delimiters at position 0
(e.g., URIs starting with '?' or '#'); update those checks to use `>= 0` (or `!=
-1`) so that queryIndex and fragmentIndex at index 0 are treated as present;
modify the expression that sets endIndex (and any similar checks around
queryIndex/fragmentIndex) to use `>= 0` to ensure the query/fragment are
stripped from the filename correctly.
- Around line 108-122: The isPathTraversal method currently uses
Paths.get(...).toAbsolutePath().normalize() which does not resolve symlinks, so
a symlink inside webRoot can still escape when the file is later read with
Files.readAllBytes(new File(webRoot, sanitizedUri).toPath()); either explicitly
resolve symlinks by calling toRealPath() on the resolved path inside
isPathTraversal (and let the existing catch return true on failure) or, if
symlinks inside webRoot are an accepted risk, add a clear comment/docstring in
isPathTraversal and near the file read (Files.readAllBytes(...)) stating that
symlinks in webRoot are trusted and this is an intentional trade-off.
---
Nitpick comments:
In `@src/main/java/org/example/CacheFilter.java`:
- Around line 30-45: CacheEntry currently stores a direct reference to a mutable
byte[] (field CacheEntry.data) which allows callers to mutate cached content; to
fix, make CacheEntry defensive by copying the array on creation (in
CacheEntry(byte[] data)) and/or by returning a copy from the cache access path
(e.g., in getOrFetch) so callers never get the original backing array; update
CacheEntry constructor to clone the incoming byte[] and ensure any place that
returns CacheEntry.data (notably getOrFetch) returns a clone of the stored array
to prevent external mutation from corrupting the cache.
- Around line 156-169: getStats() currently reads cache.size(),
currentBytes.get(), and the access-count stream without locking, allowing
concurrent addToCacheUnsafe/evict to produce an inconsistent snapshot; fix by
acquiring the same lock used for mutations (use writeLock.lock() /
writeLock.unlock() around the snapshot read) so the returned
FileCache.CacheStats reflects a consistent state—apply this change inside
getStats() and reference writeLock, getStats, addToCacheUnsafe, and evict when
locating the code to modify.
- Around line 62-82: The current synchronized(writeLock) serializes all cache
misses including provider.fetch(uri); replace the global lock with a per-key
in-flight map (e.g., ConcurrentHashMap<String, CompletableFuture<byte[]>> or
Future) so only identical URIs are coalesced while distinct URIs fetch
concurrently: on lookup use cache.get(uri) then if missing use
inFlight.computeIfAbsent(uri, k -> CompletableFuture.supplyAsync(() -> { byte[]
bytes = provider.fetch(k); if (bytes != null) addToCacheUnsafe(k, bytes); return
bytes; })); wait/join the future, remove the future from inFlight on
completion/exception, and preserve entry.recordAccess/LOGGER logic when
returning cached data; this keeps addToCacheUnsafe, cache, provider.fetch,
writeLock (remove or narrow usage) and LOGGER references but moves I/O out of
the global synchronized block.
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 18-23: SHARED_CACHE is currently tightly coupled to CacheFilter
because StaticFileHandler instantiates it as a static final; change to accept a
FileCache via dependency injection instead: remove the static instantiation of
SHARED_CACHE, add a (final) instance field FileCache cache and a constructor
StaticFileHandler(FileCache cache) that assigns it, and update any call sites
that relied on the static SHARED_CACHE to pass a shared FileCache (created at
server/wiring/bootstrap time) so tests can inject mocks or alternate
implementations like Redis-backed caches; keep a default constructor only if you
provide a reasonable default cache via delegation in wiring, not by hardcoding
new CacheFilter() inside StaticFileHandler.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/example/CacheFilter.javasrc/main/java/org/example/FileCache.javasrc/main/java/org/example/StaticFileHandler.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/FileCache.java
donne41
requested changes
Feb 25, 2026
donne41
left a comment
donne41
left a comment
There was a problem hiding this comment.
Jag testade att göra om den lite grann med att sätta new CacheFilter i TcpServer, och skicka vidare den i kedjan. Då funkar cachen att den sparar och hämtar som den ska najs!
Dock så får jag erkänna att jag glömde vad det stod i Issuen, så efter uppfriskat minne så kan jag säga att den inte gör sitt jobb som den är nu.
Men det är inte långt borta. Förslag till lösning är
flytta var cachen ska leva någonstans
få uri i ett tidigt stadie
om den finns i cachen retunera det direkt
annars får staticFilehandler läsa in det och spara till cache.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/main/java/org/example/StaticFileHandler.java (2)
20-22: Prefer injecting a cache interface instead of using staticFileCachecalls.Current usage hard-couples
StaticFileHandlerto one global in-memory implementation and makes alternative backends/lifecycle control harder.Based on learnings: Refactor FileCache to an interface to support multiple cache implementations and inject it into StaticFileHandler via dependency injection.
Also applies to: 33-38, 85-87
🤖 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 20 - 22, StaticFileHandler is tightly coupled to the global FileCache; refactor to accept a FileCache interface instance via dependency injection: add a constructor parameter (e.g., FileCache cache) and store it as a private final field, replace all static FileCache calls in StaticFileHandler (constructor, methods referenced around lines 33-38 and 85-87) with instance calls on that field (cache.get/put/remove or appropriately named methods), and update callers to pass the desired FileCache implementation (in-memory, noop, or lifecycle-managed implementation). Ensure FileCache is an interface (if not already) with the methods StaticFileHandler uses and update imports/tests accordingly.
66-73:⚠️ Potential issue | 🟠 MajorPath traversal check is still bypassable through symlinks.
normalize()does not resolve symlink targets for the requested path. Resolve both root and requested path withtoRealPath()beforestartsWith(...).🛡️ Proposed fix
private boolean isPathTraversal(String uri) { try { - Path webRootPath = Paths.get(webRoot).toAbsolutePath().normalize(); - Path requestedPath = webRootPath.resolve(uri).normalize(); - return !requestedPath.startsWith(webRootPath); - } catch (Exception e) { + Path webRootPath = Paths.get(webRoot).toRealPath(); + Path resolvedPath = webRootPath.resolve(uri).normalize(); + Path realRequestedPath = resolvedPath.toRealPath(); + return !realRequestedPath.startsWith(webRootPath); + } catch (IOException e) { return true; } }🤖 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 66 - 73, The isPathTraversal method currently uses normalize() which doesn't resolve symlinks; update it to call toRealPath() on both the webRoot (webRootPath) and the resolved requestedPath before comparing with startsWith to ensure symlink targets are resolved; handle IOException thrown by toRealPath() and treat failures as traversal (return true) while keeping the existing exception-safe behavior.src/main/java/org/example/FileCache.java (1)
8-14:⚠️ Potential issue | 🟠 MajorDefensive-copy cached byte arrays to prevent cache data corruption.
putstores caller-ownedbyte[]andgetreturns internal references. Any mutation by callers can silently corrupt cached responses across requests.🛠️ Proposed fix
public static byte[] get(String key) { - return cache.get(key); + byte[] value = cache.get(key); + return value == null ? null : value.clone(); } public static void put(String key, byte[] content) { - cache.put(key, content); + if (content == null) { + cache.remove(key); + return; + } + cache.put(key, content.clone()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/FileCache.java` around lines 8 - 14, FileCache's put and get expose internal byte[] via cache, allowing callers to mutate cached data; make defensive copies by copying the input byte[] in put (store a clone/copy into cache) and returning a copy from get (do not return the internal reference), handle null inputs/returns appropriately; update methods named put and get and the cache usage so the internal cache only holds and returns copies to avoid shared-mutable state.src/test/java/org/example/StaticFileHandlerTest.java (1)
82-114:⚠️ Potential issue | 🟠 MajorConcurrency test does not wait for threads and can miss worker failures.
The test starts threads but never
join()s them, andcatch (Exception)will not captureAssertionError. This can produce false positives.🧪 Proposed fix
- final Exception[] threadError = new Exception[1]; + final Throwable[] threadError = new Throwable[1]; for (int i = 0; i < 10; i++) { threads[i] = new Thread(() -> { try { for (int j = 0; j < 50; j++) { ByteArrayOutputStream out = new ByteArrayOutputStream(); handler.sendGetRequest(out, "shared.html"); String response = out.toString(); if (!response.contains("HTTP/1.1 200") || !response.contains("Data")) { throw new AssertionError("Oväntad response"); } } - } catch (Exception e) { + } catch (Throwable e) { synchronized (threads) { - threadError[0] = e; + if (threadError[0] == null) { + threadError[0] = e; + } } } }); threads[i].start(); } + + for (Thread thread : threads) { + thread.join(); + } + + if (threadError[0] != null) { + fail("Worker thread failed", threadError[0]); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/StaticFileHandlerTest.java` around lines 82 - 114, The test testConcurrent_MultipleReads starts threads but never waits for them and also only catches Exception (missing AssertionError), so update the thread handling in this test: after starting all threads, loop over the threads array and call join() on each to wait for completion; change the thread's catch to capture AssertionError as well (e.g., catch (Throwable t) or catch (Exception | AssertionError t)) and store it into threadError; after all joins check threadError[0] and rethrow or fail the test if non-null so worker failures cause the test to fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/FileCache.java`:
- Line 6: FileCache currently uses a static unbounded ConcurrentHashMap named
cache which can grow until OOM under unique-path traffic; cap memory by
replacing or wrapping cache with a bounded, eviction-capable structure (e.g.,
use a Caffeine cache or implement a LRU using LinkedHashMap with accessOrder
plus size checks) and enforce either maxEntries or maxBytes and an eviction
policy. Modify the FileCache class to initialize cache with a bounded capacity
or a cache builder (respecting eviction on access/write), update any
put/get/remove usage to work with the chosen cache API, and ensure eviction
metrics/logging are present so entries are evicted deterministically instead of
unbounded accumulation.
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 32-49: The current catch around reading, building and writing the
response turns any IOException (including outputStream.write/flush failures)
into a 404; narrow the error handling so only file-missing exceptions produce
404. Surround the file-read/cache logic (use FileCache, Files.readAllBytes and
sanitizedUri) with a try that catches NoSuchFileException/FileNotFoundException
and calls writeResponse(outputStream, 404, "Not Found"); use a separate catch
for other IOExceptions (including failures from HttpResponseBuilder.build(),
outputStream.write(), outputStream.flush()) that logs the error and returns a
500 via writeResponse(outputStream, 500, "Internal Server Error"). Ensure
exceptions are handled at the appropriate scope so write failures are not
reported as missing files.
---
Duplicate comments:
In `@src/main/java/org/example/FileCache.java`:
- Around line 8-14: FileCache's put and get expose internal byte[] via cache,
allowing callers to mutate cached data; make defensive copies by copying the
input byte[] in put (store a clone/copy into cache) and returning a copy from
get (do not return the internal reference), handle null inputs/returns
appropriately; update methods named put and get and the cache usage so the
internal cache only holds and returns copies to avoid shared-mutable state.
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 20-22: StaticFileHandler is tightly coupled to the global
FileCache; refactor to accept a FileCache interface instance via dependency
injection: add a constructor parameter (e.g., FileCache cache) and store it as a
private final field, replace all static FileCache calls in StaticFileHandler
(constructor, methods referenced around lines 33-38 and 85-87) with instance
calls on that field (cache.get/put/remove or appropriately named methods), and
update callers to pass the desired FileCache implementation (in-memory, noop, or
lifecycle-managed implementation). Ensure FileCache is an interface (if not
already) with the methods StaticFileHandler uses and update imports/tests
accordingly.
- Around line 66-73: The isPathTraversal method currently uses normalize() which
doesn't resolve symlinks; update it to call toRealPath() on both the webRoot
(webRootPath) and the resolved requestedPath before comparing with startsWith to
ensure symlink targets are resolved; handle IOException thrown by toRealPath()
and treat failures as traversal (return true) while keeping the existing
exception-safe behavior.
In `@src/test/java/org/example/StaticFileHandlerTest.java`:
- Around line 82-114: The test testConcurrent_MultipleReads starts threads but
never waits for them and also only catches Exception (missing AssertionError),
so update the thread handling in this test: after starting all threads, loop
over the threads array and call join() on each to wait for completion; change
the thread's catch to capture AssertionError as well (e.g., catch (Throwable t)
or catch (Exception | AssertionError t)) and store it into threadError; after
all joins check threadError[0] and rethrow or fail the test if non-null so
worker failures cause the test to fail.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/example/FileCache.javasrc/main/java/org/example/StaticFileHandler.javasrc/test/java/org/example/StaticFileHandlerTest.java
Ericthilen
reviewed
Mar 1, 2026
Ericthilen
left a comment
Ericthilen
left a comment
There was a problem hiding this comment.
Vissa commits är overifierade men det kanske inte behövs då det går att ta bort det kravet.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests