Implement static file handler (foundation for #18) - #36
Conversation
Core Implementation: - Add StaticFileHandler for serving files from /resources/static/ - Add MimeTypeResolver for Content-Type detection - Add security validation to prevent path traversal attacks Testing: - Add MimeTypeResolverTest (15 test cases) - Add StaticFileHandlerTest (20+ test cases) - All tests passing Example Files: - Add index.html demo page with gradient styling - Add styles.css for professional styling - Add app.js for JavaScript functionality demo Note: Integration with Server/ConnectionHandler will be added after PR #28 merges to avoid conflicts. Foundation work for #18
|
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:
📝 WalkthroughWalkthroughAdds static file serving: a MIME type resolver, a GET-only static file handler with path-traversal protection, a Plugin adapter, static web assets (HTML/CSS/JS), and unit tests for MIME resolution and static serving behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Plugin as StaticFilesPlugin
participant Handler as StaticFileHandler
participant Loader as ResourceLoader
participant Resolver as MimeTypeResolver
participant Response as HttpResponse
Client->>Plugin: HTTP GET /about.html
Plugin->>Handler: handle(request)
Handler->>Handler: isPathSafe(path)
alt path unsafe
Handler->>Response: 403 Forbidden
else path safe
Handler->>Loader: loadResource("/static/about.html")
alt resource found
Loader-->>Handler: bytes
Handler->>Resolver: getMimeType("about.html")
Resolver-->>Handler: "text/html"
Handler->>Response: 200 OK + Content-Type
else resource missing
Loader-->>Handler: null
Handler->>Response: 404 Not Found
end
end
Handler-->>Plugin: HttpResponse
Plugin-->>Client: HttpResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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)
No actionable comments were generated in the recent review. 🎉 Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
|
I see only empty classes. Are you still working on that? |
… ADR process for the team
- Add TEMPLATE for writing future ADRs
- Add ADR-001 documenting static file serving architecture
Closes #16
|
Gotta change folder structure for this, might be issues when compiling with the files inside /resources/ folder direction |
TatjanaTrajkovic
left a comment
TatjanaTrajkovic
left a comment
There was a problem hiding this comment.
Error responses include charset=utf-8, but successful 200 responses do not.
Since the body is encoded using UTF-8, we should append ; charset=utf-8 to text-based MIME types for consistency and better HTTP correctness.
LinusWestling
left a comment
LinusWestling
left a comment
There was a problem hiding this comment.
I think it should work to run, can do improvements later if needed :)
fmazmz
left a comment
fmazmz
left a comment
There was a problem hiding this comment.
Before merging this PR, we should merge the latest changes from Main and integrate the static fileHandler to the server. That way we dont need a new PR to actually use the changes.
An example / suggestion would be to update HelloPlugin.java or create a new Plugin that creates an HttpResponse from using the StaticFileHandler.handle().
For example:
import org.juv25d.handler.StaticFileHandler;
import org.juv25d.http.HttpRequest;
import org.juv25d.http.HttpResponse;
import java.io.IOException;
import java.util.HashMap;
public class StaticFilesPlugin implements Plugin {
@Override
public void handle(HttpRequest req, HttpResponse res) throws IOException {
HttpResponse staticRes = StaticFileHandler.handle(req);
res.setStatus(staticRes.statusCode(), staticRes.statusText());
res.setHeaders(new HashMap<>(staticRes.headers()));
res.setBody(staticRes.body());
}
}then add it to the pipeline in App.java:
pipeline.setPlugin(new StaticFilesPlugin());
…edicated ConnectionHandler. (#28) * Rename SocketServer to Server Move HTTP request handling logic to a dedicated ConnectionHandler. * Convert createSocket to an instance method named start(). * Replace console prints with Logger in ConnectionHandler * Fixed typo in ConnectionHandler * refactor Server and ConnectionHandler: - Inject Logger inside of constructor - Inject handlerFactory into the Server that handles creation of a new ConnectionHandler on each request - Remove HttpParser from Server as it is not handling the parsing of a request * accept ConnectionHandlerFactory and not a specific implementation * normalize handlerFactory name --------- Co-authored-by: WHITEROSE <firasmoussa60@gmail.com>
…tests. (#34) * Add testing for ServerLogging.java. Configure ServerLogging.java to improve its testability. * Update logger_shouldNotAddDuplicateHandlers test to properly test non-inclusion of duplicates. * Update ServerLogging.java to guard against invalid level string entries in configure method (logger.parse => setLevel).
* http parser * Bunny fixes. (only using input stream to recieve requests) * Bunny review improvements * Improved http parser ReadLine helper method to eliminate dependency on mark() and reset(). Implemented handleClient() using socket as a try-with-resources to avoid memory leakage in case of exception thrown by httpparser-methods. * NumberFormatException fix on line 53 -> 60 * Added foundation for Filters and Plugins. Added FilterChain to use created filters, and a Pipeline class to handle the workflow (Client → Filters → Plugin → Response → Client). Modified SocketServer handleClient() to use FilterChain. Added example code in App.java for Pipeline usage. TODO: Initialize HttpResponse class * Fixing build fail * add init and destroy to filter interface * add methods to pipeline class * Add servlet-style filter pipeline with lifecycle support * add documentation about temporary server impl * test: verify filters execute in order and plugin is called last * test: add coverage for filter blocking, lifecycle init, and empty pipeline * add loggingfiltertest * fix: construct HttpResponse with required arguments * fix: construct HttpResponse with required arguments * Update FilterChainImpl tests to use full HttpResponse constructor to ensure compatibility with future implementations * remove duplicated class --------- Co-authored-by: Kristina M <kristina0x7@gmail.com> Co-authored-by: WHITEROSE <firasmoussa60@gmail.com>
|
Integration Complete Hey @LinusWestling @fmazmz @TatjanaTrajkovic Following @fmazmz's suggestion, I've integrated StaticFileHandler directly in this PR. I made some architectural decisions I'd like your feedback on. Files Changed New:
Modified:
Architecture: Pipeline Integration ADR-001 suggested direct integration: if (request.method().equals("GET")) {
HttpResponse response = StaticFileHandler.handle(request);
HttpResponseWriter.write(outputStream, response);
}I used the existing Pipeline pattern instead: Why: Follows existing architecture, more extensible, same as Express/Spring/Django middleware. Question:Is this acceptable, or prefer direct integration? Breaking Change: HttpResponse Mutability Removed // Before
private final int statusCode;
// After
private int statusCode;
public void setStatusCode(int statusCode) { ... }Why: Pipeline pattern requires modifying response object. Each request gets own instance (no sharing), single-threaded (no concurrency). Feedback Needed
Thanks Closes #18 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Fix all issues with AI agents
In `@src/main/java/org/juv25d/ConnectionHandler.java`:
- Around line 27-30: The run() method in ConnectionHandler opens a socket
without a read timeout, allowing slow/malicious clients to hold connections; set
a socket read timeout (e.g., call socket.setSoTimeout(30_000) on the socket
variable before obtaining/using the input stream or before any blocking reads)
and handle the resulting SocketTimeoutException in run() to close the connection
and log/cleanup appropriately; update references in ConnectionHandler.run()
around socket.getInputStream()/socket.getOutputStream() and ensure proper
resource cleanup on timeout.
- Around line 27-49: The run() method currently logs and closes the socket if
httpParser.parse(in) throws, so clients get no HTTP response; update run() to
catch parse failures (exceptions from httpParser.parse(in)) and construct/send a
400 Bad Request HttpResponse via HttpResponseWriter.write(out, response) before
closing the socket, ensuring you obtain the socket output stream (out) even on
parse errors and handle potential IOExceptions from the write; keep the existing
pipeline.createChain()/chain.doFilter path unchanged for successful parses and
reuse HttpResponse class to build the 400 response.
In `@src/main/java/org/juv25d/filter/README.md`:
- Line 27: The README line incorrectly references the plural directory
"src/main/java/.../filters/"; update that line to the correct package path
"src/main/java/org/juv25d/filter/" (singular) and ensure any subsequent
instruction text refers to the package name "filter" and the example package
org.juv25d.filter so readers can create the class in the proper directory.
- Line 32: Update the README entry that registers the filter: replace the
incorrect example path string "src/org.example/App.java" with the actual project
path "src/main/java/org/juv25d/App.java" so the README points to the real
App.java location; edit the README.md line under "Register your filter" to use
the corrected path.
In `@src/main/java/org/juv25d/handler/StaticFileHandler.java`:
- Around line 64-69: The MIME-type charset logic in StaticFileHandler misses XML
types so variable mimeType may lack "; charset=utf-8" for application/xml or
application/xhtml+xml; update the conditional that checks mimeType (the block
where mimeType.startsWith("text/") ... mimeType += "; charset=utf-8";) to also
match XML types (e.g., mimeType.contains("xml") or specifically check for
"application/xml" and "application/xhtml+xml"), and ensure you guard against
appending a charset twice by first verifying mimeType does not already contain a
"charset=" token.
- Around line 151-180: The 404 page in createNotFoundResponse reflects the
untrusted path into HTML causing a reflected XSS; fix by HTML-escaping the path
before embedding it. Add a private helper escapeHtml(String) that replaces &, <,
>, " and ' with their HTML entities (e.g. &, <, >, ", ') and
call it when building the html string (use escapeHtml(path) in place of path).
Ensure the escaped string is used in the formatted HTML passed to the
HttpResponse.
- Around line 89-105: The isPathSafe method is vulnerable to URL-encoded
traversal (e.g., %2e%2e) because it checks only literal patterns; update the
code that calls isPathSafe (or isPathSafe itself) to first URL-decode the
incoming request path (using java.net.URLDecoder with UTF-8) and then validate,
and additionally normalize the decoded path (e.g., via
java.nio.file.Paths.get(decoded).normalize()) before checking for traversal or
absolute escapes; ensure isPathSafe still enforces rejects for "..", "\\" and
double slashes on the normalized/decoded value and that the final check confirms
the normalized path starts with "/" (or is within the intended static root) to
prevent escape after decoding.
In `@src/main/java/org/juv25d/http/HttpResponse.java`:
- Around line 43-45: setStatusText currently assigns statusText without null
checking whereas the constructor uses Objects.requireNonNull; update
setStatusText in class HttpResponse to validate its statusText parameter with
Objects.requireNonNull (using the same error message as the constructor) before
assigning to this.statusText to keep behavior consistent.
- Around line 47-49: The setHeaders method currently assigns a mutable
LinkedHashMap to the headers field, breaking the immutability established by the
constructor and exposing internal state via headers(); update setHeaders to
store an unmodifiable map (e.g., Collections.unmodifiableMap(new
LinkedHashMap<>(headers))) so it matches the constructor behavior, or
alternatively change headers() to return a defensive copy; modify the setHeaders
implementation (method name setHeaders and the headers() accessor/headers field)
accordingly so callers cannot mutate the internal headers map.
In `@src/main/java/org/juv25d/Pipeline.java`:
- Around line 23-25: createChain() can NPE when plugin is null; add a guard at
the start of createChain() to validate that the field plugin (set by setPlugin)
is non-null and throw a clear IllegalStateException (or similar) if it hasn't
been set, then construct and return the chain; also change the return type from
FilterChainImpl to the FilterChain interface to reduce coupling so the method
signature returns FilterChain while still instantiating new
FilterChainImpl(List.copyOf(filters), plugin).
In `@src/main/java/org/juv25d/plugin/README.md`:
- Line 22: The README has an inconsistent directory path: it shows
"src/main/java/.../plugins/" (plural) but the actual package is
org.juv25d.plugin (singular); update the README line to use the correct
directory/package name (e.g., "src/main/java/.../plugin/") so it matches the
package org.juv25d.plugin and any references to creating new classes in that
package.
- Around line 30-45: The README example for RouterPlugin uses non-existent
HttpResponse methods; update the RouterPlugin.handle(HttpRequest req,
HttpResponse res) example to call HttpResponse.setStatusCode(int) instead of
setStatus and to call HttpResponse.setBody(byte[]) by converting strings to
bytes (e.g., "Welcome!".getBytes(StandardCharsets.UTF_8)) or otherwise providing
a byte[]; reference the RouterPlugin class, Plugin interface, handle method, and
HttpResponse.setStatusCode / HttpResponse.setBody(byte[]) when making the change
and add the necessary import for StandardCharsets if used.
- Around line 48-52: Update the README plugin registration example to reference
the correct App class/package (org.juv25d.App) and fix the plugin instantiation:
call Pipeline.setPlugin with a proper HelloPlugin constructor invocation (use
new HelloPlugin()) instead of new HelloPlugin; ensure the code block shows
Pipeline pipeline = new Pipeline(); pipeline.setPlugin(new HelloPlugin()); to
match the actual App class and project package.
In `@src/test/java/org/juv25d/filter/LoggingFilterTest.java`:
- Line 45: Replace the Java language assertion in LoggingFilterTest with a JUnit
assertion so the test fails reliably; locate the line asserting
output.contains("GET /test") and change it to use JUnit (e.g.,
Assertions.assertTrue or Assert.assertTrue depending on your test framework) and
add a helpful failure message, ensuring the appropriate org.junit import is
present (class: LoggingFilterTest, method containing the assertion).
In `@src/test/java/org/juv25d/handler/StaticFileHandlerTest.java`:
- Around line 22-28: Update the assertions in StaticFileHandlerTest to expect
the charset suffix appended by StaticFileHandler.handle: change checks like
assertThat(response.headers()).containsEntry("Content-Type", "text/html") to
assert the header value includes "; charset=utf-8" (e.g., "text/html;
charset=utf-8") for the HTML, CSS, JS, and root-path tests (refer to the test
methods shouldReturnCorrectContentTypeForHtml and the corresponding
CSS/JS/root-path test methods) so they match the behavior of
StaticFileHandler.handle().
🧹 Nitpick comments (18)
src/main/resources/static/css/styles.css (2)
23-23: Use modern CSS color function notation.Stylelint flags the legacy
rgba()notation. Modern CSS usesrgb()with a slash-separated alpha value and percentage notation.♻️ Proposed fix
- box-shadow: 0 10px 30px rgba(0, 0, 0, 0.2); + box-shadow: 0 10px 30px rgb(0 0 0 / 20%);
110-110: Use modern media feature range notation.Stylelint recommends context-based range notation over the legacy
max-widthprefix syntax.♻️ Proposed fix
-@media (max-width: 768px) { +@media (width <= 768px) {src/main/java/org/juv25d/logging/ServerLogging.java (1)
30-36: Inconsistent indentation insidetryblock.Line 31 (
Level level = ...) is not indented relative to thetryblock, while lines 33-35 in thecatchare properly indented.♻️ Proposed fix
try { - Level level = Level.parse(levelName.toUpperCase()); - logger.setLevel(level); + Level level = Level.parse(levelName.toUpperCase()); + logger.setLevel(level); } catch (IllegalArgumentException e) {src/main/java/org/juv25d/App.java (1)
19-23: Remove commented-out code; use documentation or configuration instead.The commented-out HelloPlugin option adds noise. If you want to support swappable plugins, consider a configuration mechanism or document it in the README rather than leaving dead code.
src/main/java/org/juv25d/Server.java (3)
20-33: No graceful shutdown mechanism.The
while (true)loop with no interrupt/shutdown check means the server can only be stopped by killing the process. Consider adding avolatile boolean runningflag and astop()method that closes theServerSocketto break out of theaccept()call. This becomes important for testability and clean shutdown.
9-9: Hardcoded port.Consider making the port configurable (constructor parameter or system property) to support different environments and avoid conflicts during testing.
21-21: Unencrypted server socket.Static analysis flagged this as using a plain
ServerSocket(cleartext). This is fine for local development, but keep in mind that for production use, you'd want TLS (e.g.,SSLServerSocket) or a reverse proxy in front.src/main/java/org/juv25d/plugin/HelloPlugin.java (1)
10-13: Placeholder plugin produces no response.If this plugin is activated (as shown in the commented-out code in
App.java), it will leave theHttpResponseunmodified, resulting in whatever default state it was constructed with. Consider setting a minimal response (e.g., 200 with "Hello, World!") to make it useful for testing the pipeline.src/main/java/org/juv25d/filter/FilterChainImpl.java (2)
16-18: Store a defensive copy of the filters list.The constructor stores the caller's list reference directly. If the caller mutates that list after constructing the chain, behavior becomes unpredictable. A defensive copy is a simple safeguard.
♻️ Proposed fix
public FilterChainImpl(List<Filter> filters, Plugin plugin) { - this.filters = filters; + this.filters = List.copyOf(filters); this.plugin = plugin; }
22-29: Consider guarding against a null plugin.If
pluginis null (e.g., no plugin set on the pipeline), line 27 will throw an NPE when all filters have been processed. Either add a null check or document thatpluginmust not be null.src/main/java/org/juv25d/plugin/Plugin.java (1)
1-10: Clean interface definition.Minor nit: extra space before the parenthesis on Line 9 (
handle (HttpRequest...→handle(HttpRequest...).src/main/java/org/juv25d/filter/LoggingFilter.java (1)
11-14: UseServerLogging.getLogger()instead ofSystem.out.println.The rest of the codebase uses
java.util.logging.LoggerviaServerLogging.getLogger(). ALoggingFilterusing rawSystem.outis inconsistent and bypasses log-level control, formatting, and handler configuration.♻️ Proposed fix
package org.juv25d.filter; import org.juv25d.http.HttpRequest; import org.juv25d.http.HttpResponse; +import org.juv25d.logging.ServerLogging; import java.io.IOException; +import java.util.logging.Logger; public class LoggingFilter implements Filter { + private static final Logger logger = ServerLogging.getLogger(); + `@Override` public void doFilter(HttpRequest req, HttpResponse res, FilterChain chain) throws IOException { - System.out.println(req.method() + " " + req.path()); + logger.info(req.method() + " " + req.path()); chain.doFilter(req, res); } }src/test/java/org/juv25d/logging/ServerLoggingTest.java (1)
40-60: Redundant handler cleanup —setUp()already handles this.Lines 43-46 duplicate the handler-removal logic that
@BeforeEach setUp()already performs before each test. You can drop the manual cleanup and callServerLogging.configure(logger)directly.src/main/java/org/juv25d/plugin/StaticFilesPlugin.java (2)
3-3: Remove unnecessary same-package import.
Pluginis in the same package (org.juv25d.plugin), so the import on Line 3 is redundant.
16-25: Field-by-field copy is fragile ifHttpResponsegains new fields.If
HttpResponseis extended with additional fields (e.g., HTTP version, trailers), this adapter will silently drop them. Consider adding a bulk-copy method onHttpResponse(e.g.,copyFrom(HttpResponse other)) to keep this in sync, or refactorStaticFileHandler.handleto mutate the response directly rather than returning a new one.src/test/java/org/juv25d/handler/StaticFileHandlerTest.java (1)
142-149: Redundant assertion —isNotEmpty()already coverslength > 0.Line 148 (
assertThat(response.body().length).isGreaterThan(0)) is redundant afterassertThat(response.body()).isNotEmpty()on Line 147.src/main/java/org/juv25d/handler/MimeTypeResolver.java (1)
12-42: Consider using an unmodifiable map forMIME_TYPES.The
MIME_TYPESmap isprivateand only populated in the static initializer, so there's no immediate bug, but wrapping it inCollections.unmodifiableMap(or usingMap.ofEntries) would make the intent explicit and guard against accidental mutation in future changes.♻️ Example using Map.ofEntries
- private static final Map<String, String> MIME_TYPES = new HashMap<>(); - - static { - // Text types - MIME_TYPES.put("html", "text/html"); - MIME_TYPES.put("htm", "text/html"); - ... - MIME_TYPES.put("zip", "application/zip"); - } + private static final Map<String, String> MIME_TYPES = Map.ofEntries( + Map.entry("html", "text/html"), + Map.entry("htm", "text/html"), + Map.entry("css", "text/css"), + Map.entry("js", "application/javascript"), + Map.entry("json", "application/json"), + Map.entry("xml", "application/xml"), + Map.entry("txt", "text/plain"), + Map.entry("png", "image/png"), + Map.entry("jpg", "image/jpeg"), + Map.entry("jpeg", "image/jpeg"), + Map.entry("gif", "image/gif"), + Map.entry("svg", "image/svg+xml"), + Map.entry("ico", "image/x-icon"), + Map.entry("webp", "image/webp"), + Map.entry("woff", "font/woff"), + Map.entry("woff2", "font/woff2"), + Map.entry("ttf", "font/ttf"), + Map.entry("otf", "font/otf"), + Map.entry("pdf", "application/pdf"), + Map.entry("zip", "application/zip") + );src/main/java/org/juv25d/ConnectionHandler.java (1)
35-40: Minor: prefer static import over fully qualifiedjava.util.Map.of().
Mapis already imported at the top of the file (viaHttpResponseusage pattern). Add an import forjava.util.Mapand useMap.of()for consistency.
I agree with the pipeline integration, it follows the existing patterns and keeps the architecture consistent. Regarding mutability, it might be better to keep HttpResponse immutable and, as you suggested, introduce a separate MutableHttpResponse. Immutability helps prevent accidental side effects and keeps the core response model safer, especially if we introduce multi-threading in the future. |
|
nice one. This looks good to me. Note regarding HttpResponse classes etc: Once we merge feel free to open a new issue and branch to make any other changes. Otherwise now each PR will be getting a conflict. |
…with usage notes for future integration.
…on; simplify and clarify class documentation.
|
Connected the plugin directly, its one line of code. Another PR will only delay our release further. closes #54 |
7 participants
Changes
Foundation work for #18 - GET handling for static files
What's Added
Core Logic
/resources/static/Security Features
../,//,\Testing (35+ test cases)
MimeTypeResolverTest (15 tests):
StaticFileHandlerTest (20+ tests):
Example Static Files
File Structure
Integration Status
⏸️ Not yet integrated with Server/ConnectionHandler
This PR contains all core logic but avoids modifying
Server/SocketServer/ConnectionHandler to prevent conflicts with PR #28.
Integration Plan (after PR #28 merges):
Architecture
Architecture decisions documented in ADR-001 (see PR #35 ).
Dependencies
Review Focus
Notes
Summary by CodeRabbit
New Features
Tests