-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor file resolving #118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
843b34c
2844d6e
0048a05
e422463
4b3267d
907e588
ec9b987
b2ad6ee
5ea159e
af7fcdb
c27a17d
7bbcdd4
5bd97d0
90b6fc4
5962ecb
456cda3
c1321dd
5552e85
a9cffb6
cbb0743
2991058
522ee98
d319eb1
2b3ed4e
6107ef2
8f0f324
41eeeb2
17b914a
737ded2
d02a330
c65b5cc
2123ba6
12b48b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| package org.example; | ||
|
|
||
| import org.example.config.ConfigLoader; | ||
| import org.example.filter.FilterChain; | ||
| import org.example.http.HttpCachingHeaders; | ||
| import org.example.http.HttpResponseBuilder; | ||
| import org.example.httpparser.HttpRequest; | ||
|
|
||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Paths; | ||
|
|
||
| public class FileResolver { | ||
|
|
||
|
|
||
| /** | ||
| * Helper class to resolve HTTP request paths to actual file system paths. | ||
| * This method processes incoming HTTP request paths and converts them into | ||
| * absolute file paths that can be used to locate and serve static files. | ||
| * @param requestPath the HTTP request path (e.g., "/", "/index.html" | ||
| * @return the complete file system path combining the root directory with the processed request path | ||
| */ | ||
|
|
||
| public static String resolvePath(String requestPath) { | ||
| HttpResponseBuilder response = new HttpResponseBuilder(); | ||
| File errorFile = new File("src/main/resources/error.html"); | ||
|
|
||
| if (requestPath == null || requestPath.isEmpty()) { | ||
| requestPath = "/"; | ||
| } | ||
| String path = requestPath.equals("/") ? "index.html" : requestPath.substring(1); | ||
| String rootDir = ConfigLoader.get().server().rootDir(); | ||
|
|
||
| if (path.contains("..")) { | ||
| try { | ||
| response.setBody(Files.readAllBytes(errorFile.toPath())); | ||
| } catch (IOException e) { | ||
| response.setBody("Forbidden".getBytes(StandardCharsets.UTF_8)); | ||
| } | ||
| response.setStatusCode(HttpResponseBuilder.SC_FORBIDDEN); | ||
| return null; | ||
| } | ||
|
Comment on lines
+27
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On Line 27-28, response/error-page handling is created inside a path resolver, but that response object is never returned. On Line 43, returning 🤖 Prompt for AI Agents |
||
|
|
||
| return rootDir + "/" + path; | ||
|
|
||
| } | ||
|
|
||
|
|
||
|
|
||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,100 @@ | ||||||||||||||||
| package org.example.http; | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| import org.example.config.AppConfig; | ||||||||||||||||
| import org.example.config.ConfigLoader; | ||||||||||||||||
| import org.example.filter.Filter; | ||||||||||||||||
| import org.example.filter.FilterChain; | ||||||||||||||||
| import org.example.httpparser.HttpRequest; | ||||||||||||||||
|
|
||||||||||||||||
| import java.io.File; | ||||||||||||||||
| import java.io.ObjectInputFilter; | ||||||||||||||||
| import java.time.Instant; | ||||||||||||||||
| import java.time.format.DateTimeFormatter; | ||||||||||||||||
| import java.util.Map; | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| public class CachingFilter implements Filter { | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| @Override | ||||||||||||||||
| public void init() { | ||||||||||||||||
|
|
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| @Override | ||||||||||||||||
| public void destroy() { | ||||||||||||||||
|
|
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| @Override | ||||||||||||||||
| public void doFilter(HttpRequest request, HttpResponseBuilder response, FilterChain chain) { | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| HttpCachingHeaders cachingHeaders = new HttpCachingHeaders(); | ||||||||||||||||
|
|
||||||||||||||||
| String resolvedPath = request.getResolvedPath(); | ||||||||||||||||
| File file = new File(resolvedPath); | ||||||||||||||||
| if (!file.isFile()) { | ||||||||||||||||
| chain.doFilter(request, response); | ||||||||||||||||
| return; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| Map<String, String> headers = request.getHeaders(); | ||||||||||||||||
|
|
||||||||||||||||
| String modifiedSince = headers.get("If-Modified-Since"); | ||||||||||||||||
| String eTag = generateEtag(file); | ||||||||||||||||
| String quotedEtag = "\"" + eTag + "\""; | ||||||||||||||||
| Instant lastModified = Instant.ofEpochMilli(file.lastModified()); | ||||||||||||||||
|
|
||||||||||||||||
| String ifNoneMatch = headers.get("If-None-Match"); | ||||||||||||||||
|
|
||||||||||||||||
| cachingHeaders.addETagHeader(eTag); | ||||||||||||||||
| cachingHeaders.setLastModified(Instant.ofEpochMilli(file.lastModified())); | ||||||||||||||||
| cachingHeaders.setDefaultCacheControlStatic(); | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| if (ifNoneMatch != null && ifNoneMatch.equals(quotedEtag)) { | ||||||||||||||||
| response.setStatusCode(HttpResponseBuilder.SC_NOT_MODIFIED); | ||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||
| cachingHeaders.getHeaders().forEach(response::addHeader); | ||||||||||||||||
| return; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if (modifiedSince != null) { | ||||||||||||||||
| try { | ||||||||||||||||
| Instant ifModifiedSinceInstant = | ||||||||||||||||
| Instant.from(DateTimeFormatter.RFC_1123_DATE_TIME.parse(modifiedSince)); | ||||||||||||||||
|
|
||||||||||||||||
| if (!lastModified.isAfter(ifModifiedSinceInstant)) { | ||||||||||||||||
| response.setStatusCode(HttpResponseBuilder.SC_NOT_MODIFIED); | ||||||||||||||||
| cachingHeaders.getHeaders().forEach(response::addHeader); | ||||||||||||||||
| return; | ||||||||||||||||
| } | ||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||
|
|
||||||||||||||||
| } catch (Exception e) { | ||||||||||||||||
|
|
||||||||||||||||
| // Ignore malformed If-Modified-Since header; proceed without cache validation | ||||||||||||||||
| // Consider logging: log.debug("Invalid If-Modified-Since header: {}", modifiedSince, e); | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+75
to
+79
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Empty catch block silently swallows parsing errors. If Proposed fix } catch (Exception e) {
-
+ // Ignore malformed If-Modified-Since header; proceed without cache validation
+ // Consider logging: log.debug("Invalid If-Modified-Since header: {}", modifiedSince, e);
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||
|
|
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| chain.doFilter(request, response); | ||||||||||||||||
| cachingHeaders.getHeaders().forEach(response::addHeader); | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| private String generateEtag(File file) { | ||||||||||||||||
| return file.lastModified() + "-" + file.length(); | ||||||||||||||||
|
|
||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| package org.example.http; | ||
|
|
||
| import java.time.Instant; | ||
| import java.time.ZoneOffset; | ||
| import java.time.format.DateTimeFormatter; | ||
| import java.util.LinkedHashMap; | ||
| import java.util.Map; | ||
| // | ||
| /** | ||
| * Helper class for building HTTP response headers | ||
| * Lets the client reuse cached responses | ||
| * Reduces bandwidth | ||
| * Reduces latency | ||
| * Reduces load on your server | ||
| * Ensures webserver and proxies understand caching instructions | ||
| */ | ||
| public class HttpCachingHeaders { | ||
|
|
||
| /** | ||
| * Cache Control helps manage servers and browsers by settings rules | ||
| * ETag helps cache be more efficient and not needing to send a full resend assuming the content has not changed | ||
| * Last-Modified | ||
| */ | ||
|
|
||
| private static final String CACHE_CONTROL = "Cache-Control"; | ||
| private static final String LAST_MODIFIED = "Last-Modified"; | ||
| private static final String ETAG = "ETag"; | ||
|
|
||
| private static final DateTimeFormatter HTTP_DATE_FORMATTER = | ||
| DateTimeFormatter.RFC_1123_DATE_TIME.withZone(ZoneOffset.UTC); | ||
|
|
||
|
|
||
| private final Map<String, String> headers = new LinkedHashMap<>(); | ||
|
|
||
|
|
||
| /** | ||
| * Sets a header | ||
| * @param name Header name eg. Cache-Control | ||
| * @param value Header value eg. public, max-age=3600 | ||
| */ | ||
| public void setHeader(String name, String value) { | ||
| headers.put(name, value); | ||
| } | ||
|
|
||
| /** | ||
| * Helper method for setting ETag header value | ||
| * ETag values must be enclosed in double quotes "123" not 123 | ||
| * @param etag Raw, unquoted ETag token (e.g. {`@code` abc123}); double quotes | ||
| * are added automatically to comply with RFC 7232. | ||
| */ | ||
| public void addETagHeader(String etag) { | ||
| setHeader(ETAG, "\"" + etag + "\""); | ||
| } | ||
|
Comment on lines
+45
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ETag quoting: Javadoc says quotes are added but implementation doesn't add them. The Javadoc states "double quotes are added automatically to comply with RFC 7232", but the implementation just calls 🐛 Fix to match documented behavior public void addETagHeader(String etag) {
- setHeader(ETAG, etag);
+ setHeader(ETAG, "\"" + etag + "\"");
}Alternatively, update the Javadoc if callers are expected to include quotes. 🤖 Prompt for AI Agents |
||
|
|
||
| /** | ||
| * Sets Cache-Control header value | ||
| * @param cacheControl sets rules eg. public, max-age=3600 | ||
| */ | ||
| public void setCacheControl(String cacheControl) { | ||
| setHeader(CACHE_CONTROL, cacheControl); | ||
| } | ||
|
|
||
| /** | ||
| * Helper method for setting Last-Modified header value | ||
| * Formates and sets Last modified based on an instant | ||
| * @param instant Timestamp of the last modification | ||
| */ | ||
| public void setLastModified(Instant instant){ | ||
| setHeader(LAST_MODIFIED, HTTP_DATE_FORMATTER.format(instant)); | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * In case of errors or unexpected behaviour, the cache should be disabled and no data should be saved | ||
| */ | ||
| public void setNoCache() { | ||
| setCacheControl("no-store, no-cache"); | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Copies all configured caching headers into the provided target map eg. HttpReponseBuilder. | ||
| * @param target Map should return generated headers | ||
| */ | ||
| public void applyTo(Map<String,String> target){ | ||
| target.putAll(headers); | ||
| } | ||
|
|
||
| /** | ||
| * Maps all configured caching headers into a new map | ||
| * @return A map which includes all caching headers | ||
| */ | ||
| public Map<String,String> getHeaders() { | ||
| return new LinkedHashMap<>(headers); | ||
| } | ||
|
|
||
|
|
||
|
|
||
| /** | ||
| * Standard settings for caching, 1 hour | ||
| */ | ||
| public void setDefaultCacheControlStatic(){ | ||
| setCacheControl("public, max-age=3600"); | ||
| } | ||
|
|
||
|
|
||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Block request processing when
resolvedPathisnull.FileResolver.resolvePath(...)can returnnullfor forbidden paths, but execution continues and the unsanitized URI is still used for file serving. This can bypass the intended traversal guard.Suggested fix
Also applies to: 93-95
🤖 Prompt for AI Agents