more #245
more #245
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (28)
✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. 📝 WalkthroughWalkthroughAdds many Boost.HTTP server features: async route handler API, CORS middleware updated to coroutines, URL/HTML encoding, ETag generation, freshness checks, MIME database and lookup, Range header parsing, file sending and static-serving middleware, status predicates, and accompanying unit tests and documentation rule. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant serve_static as serve_static
participant FS as File System
participant Mime as MIME DB
participant Range as Range Parser
participant RP as route_params
Client->>serve_static: HTTP GET /path
serve_static->>FS: stat(path)
FS-->>serve_static: (exists,size,mtime)
serve_static->>Mime: lookup(path)
Mime-->>serve_static: content_type
serve_static->>serve_static: generate ETag / set headers
Client->>serve_static: If-None-Match / Range headers
serve_static->>serve_static: is_fresh? (ETag / Last-Modified)
alt fresh (304)
serve_static->>RP: set 304 and end
else if Range present
serve_static->>Range: parse_range(size, header)
Range-->>serve_static: ranges/result
serve_static->>FS: open file (with range)
loop stream chunks
serve_static->>RP: write(chunk) (co_await)
end
serve_static->>RP: end()
else full content
serve_static->>FS: open file
loop stream chunks
serve_static->>RP: write(chunk) (co_await)
end
serve_static->>RP: end()
end
RP-->>Client: HTTP response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 14
🤖 Fix all issues with AI agents
In @.cursor/rules/doc.mdc:
- Around line 1-5: The rule currently has an empty globs array and alwaysApply:
false so it never runs; update the rule by either populating the globs key with
the appropriate file patterns (e.g., doc/Javadoc/API examples patterns) or set
alwaysApply: true if you intend the rule to be global, ensuring the 'globs' and
'alwaysApply' fields in the rule header are adjusted accordingly so the rule
will actually execute (update the 'globs' array or toggle 'alwaysApply' in the
doc.mdc rule definition).
In `@include/boost/http/server/encode_url.hpp`:
- Around line 20-39: The doc for encode_url is ambiguous about component vs
full-URL encoding; update the API so callers aren't misled: either change the
documentation to state clearly that encode_url preserves RFC-reserved characters
(e.g., '/', '?', '#') and is meant for encoding full URLs/locations only, or add
an explicit overload/flag (e.g., encode_url_component(const std::string& url) or
encode_url(const std::string& url, bool encode_reserved)) that percent-encodes
reserved characters for URL components; update the comment block to mention
which characters are preserved and reference the chosen symbol (encode_url or
new encode_url_component/encode_url(..., encode_reserved)) so callers know which
to use.
In `@include/boost/http/server/etag.hpp`:
- Around line 35-40: The documentation example for etag() is inconsistent with
the implementation in etag.cpp: replace the base64/slash-style example with one
that matches the actual output format produced by etag (size in hex, a hyphen,
then the 16-hex-character FNV-1a hash), e.g. something like
"\"<size_hex>-<16_hex_chars>\"" or, alternatively, add a note that the example
is illustrative only; update the docblock around the etag function to reflect
this change.
In `@include/boost/http/server/send_file.hpp`:
- Around line 102-116: The doc example uses rp.status(status::not_modified)
which is inconsistent with the implementation; update the example in the
send_file.hpp snippet so that when send_file_info::result ==
send_file_result::not_modified it calls rp.res.set_status(status::not_modified)
instead of rp.status(...), preserving the co_return co_await rp.send("") flow
and keeping references to send_file_info, send_file_init, and send_file_result
to match the real implementation pattern.
In `@src/server/etag.cpp`:
- Around line 59-66: The append calls that pass C-strings are hitting the
append(const char*) overload and triggering -Wrestrict; update them to use the
length-aware overload instead: when writing the size prefix built into buf (in
the ETag formatting code that uses std::snprintf and buf) call
result.append(buf, std::strlen(buf)) (or capture snprintf's return and append
that many bytes) rather than result.append(buf), and make the same replacement
for the other occurrence in the file (the hex append already uses
result.append(hex, 16) but ensure any other result.append(...) calls that take a
raw char* use the length-aware overload); reference symbols: result.append, buf,
hex, to_hex, body.size(), hash.
In `@src/server/fresh.cpp`:
- Around line 21-58: The etag_matches function currently uses
if_none_match.find(etag_val) which can return false positives; instead parse the
comma-separated if_none_match into distinct tokens, trim whitespace around each
token, remove optional surrounding quotes and the W/ weak prefix via the
existing strip_weak logic, and then compare each token for exact equality
against etag_val (and also compare the stripped entire if_none_match as a
fallback using inm_stripped). Update the logic in etag_matches to iterate tokens
(splitting on ',') and perform exact token comparisons rather than calling find,
ensuring correct handling of weak prefixes via strip_weak and preserving the "*"
special-case handling.
- Around line 61-75: The current not_modified_since(if_modified_since,
last_modified) does a lexicographic compare which fails across RFC7231's three
allowed date formats; replace the string comparison with proper HTTP-date
parsing: implement a helper like parse_http_date(core::string_view) that tries
IMF-fixdate, RFC850 and asctime formats (returning std::optional<time_t> or
chrono::time_point), use it to parse both if_modified_since and last_modified,
and then compare the parsed time values; if either parse fails, return false
(no-match). Ensure the new parse_http_date is used only inside
not_modified_since so the semantics and noexcept handling remain consistent.
In `@src/server/mime_types.cpp`:
- Around line 28-81: The ext_db constexpr array is intended to be sorted by
extension for binary search but "7z" is incorrectly last, breaking ordering;
update ext_db (constexpr ext_entry ext_db[]) by moving the {"7z",
"application/x-7z-compressed"} entry into its proper lexicographic position
among the extension keys (i.e., before the alphabetic entries so digits come
first) so the array remains strictly sorted and binary search will work
correctly.
In `@src/server/range_parser.cpp`:
- Around line 47-187: parse_range_spec currently returns false for both syntax
errors and valid-but-unsatisfiable specs, causing parse_range to misclassify
malformed headers as unsatisfiable; change parse_range_spec to return a
tri-state outcome (e.g., enum ParseOutcome { Malformed, Unsatisfiable, Ok } or
an out-parameter) instead of bool, and update parse_range to inspect that
outcome: if any Malformed -> set result.type = range_result_type::malformed;
otherwise collect ranges for Ok outcomes and treat Unsatisfiable outcomes as
non-satisfiable entries but not as Malformed; ensure functions referenced are
parse_range_spec, parse_range, range_result, and range_result_type so malformed
inputs like "bytes=abc-def" produce malformed (400) while true-but-unsatisfiable
specs produce unsatisfiable (416).
In `@src/server/send_file.cpp`:
- Around line 139-173: The code currently accepts the first range from
parse_range even when multiple ranges are requested; change the logic inside the
Range handling block (the branch using parse_range, range_result_type::ok, and
range_result.ranges) to only honor the Range when range_result.ranges.size() ==
1; if there are >1 ranges, treat the header as absent (do not set info.is_range,
do not set rp.res.set_status(status::partial_content), do not set Content-Range,
and let the normal full-content path run), since multipart/byteranges is not
implemented; keep existing behavior for the single-range case (setting
info.range_start/end and send_file_result::ok).
In `@src/server/serve_static.cpp`:
- Around line 55-67: The is_dotfile function only checks the last path segment
and thus misses dotfiles in earlier segments (e.g., "/.git/config"); update
is_dotfile to scan all path segments of the input core::string_view path and
return true if any segment begins with a '.' (except explicitly allowlisting
".well-known" if that is desired); modify the logic in is_dotfile to iterate
over segments split by '/' (or walk the string looking for '/' boundaries) and
check the first character after each boundary for '.', keeping the function
noexcept and using core::string_view operations so callers like serve_static can
rely on the updated behavior.
- Around line 152-155: The concatenation using path_cat(path, impl_->root,
req_path) lacks normalization and allows directory traversal; after building
path, resolve both impl_->root and the concatenated path to canonical forms
(e.g., std::filesystem::weakly_canonical or equivalent) and verify the resolved
file path is contained within the resolved root before serving; if the resolved
path lies outside the root (or std::filesystem::relative(resolvedPath,
resolvedRoot) begins with ".."), reject the request and return an error. Ensure
this check is performed immediately after path_cat and before any file access in
the serve_static logic.
- Around line 262-273: The loop reading from f.read and writing via rp.write
must detect partial reads or errors and abort the response instead of
unconditionally calling rp.end; after the loop check if ec is set or remaining
!= 0 (or if any rp.write failed/returned an error) and, in that case, propagate
the error by logging and closing/aborting the response/connection (e.g., call
rp.abort() or set an error status) rather than calling rp.end(); update the
logic around the read/write block (references: f.read, remaining, ec, rp.write,
rp.end) to ensure you only finalize the response when remaining == 0 and no
errors occurred.
In `@test/unit/server/statuses.cpp`:
- Around line 34-49: The test suite is missing coverage for HTTP status 305;
update the test_is_redirect() unit test to assert the current behavior of
statuses::is_redirect for 305 by adding a BOOST_TEST( statuses::is_redirect( 305
) ); (place it with the other "Redirect status codes" assertions inside
test_is_redirect) so the test verifies the implementation's handling of the Use
Proxy code.
🧹 Nitpick comments (3)
include/boost/http/server/encode_url.hpp (1)
13-15: Use the public Boost.Core string_view header in a public API.Including
<boost/core/detail/string_view.hpp>in a public header relies on an internal header. Prefer the public header to avoid stability issues across Boost releases.♻️ Proposed change
-#include <boost/core/detail/string_view.hpp> +#include <boost/core/string_view.hpp>include/boost/http/server/serve_static.hpp (1)
13-41: Add explicit includes for used types.This public header relies on transitive includes for
std::uint32_tandcore::string_view. Adding direct includes improves self-containment and reduces fragility.♻️ Suggested include cleanup
`#include` <boost/http/detail/config.hpp> `#include` <boost/http/server/route_handler.hpp> +#include <boost/core/string_view.hpp> +#include <cstdint>src/server/mime_db.cpp (1)
20-72: Guard the MIME db sort invariant used by binary search.
lookupassumes the array is sorted case-insensitively. Consider a small static/assertion test (or unit test) to prevent accidental unsorted inserts from silently breaking lookups.
| --- | ||
| description: Documentation style conventions for code snippets, javadocs, and API documentation examples | ||
| globs: | ||
| alwaysApply: false | ||
| --- |
There was a problem hiding this comment.
Define globs or enable alwaysApply so the rule actually runs.
With alwaysApply: false and an empty globs, this rule likely won’t apply anywhere. Please add explicit doc globs or switch to alwaysApply: true if it should be global.
🤖 Prompt for AI Agents
In @.cursor/rules/doc.mdc around lines 1 - 5, The rule currently has an empty
globs array and alwaysApply: false so it never runs; update the rule by either
populating the globs key with the appropriate file patterns (e.g.,
doc/Javadoc/API examples patterns) or set alwaysApply: true if you intend the
rule to be global, ensuring the 'globs' and 'alwaysApply' fields in the rule
header are adjusted accordingly so the rule will actually execute (update the
'globs' array or toggle 'alwaysApply' in the doc.mdc rule definition).
| @par Example | ||
| @code | ||
| std::string content = "Hello, World!"; | ||
| std::string tag = etag( content ); | ||
| // tag == "\"d-3/1gIbsr1bCvZ2KQgJ7DpTGR3YH\"" | ||
| @endcode |
There was a problem hiding this comment.
Documentation example may not match actual output format.
The example shows "\"d-3/1gIbsr1bCvZ2KQgJ7DpTGR3YH\"" but the implementation in etag.cpp produces a format like "<size_hex>-<16_hex_chars>" using FNV-1a hash converted to hex. The slash character and base64-like encoding shown here don't match the hex-based implementation.
Consider updating the example to reflect the actual output format, or mark it as illustrative only.
🤖 Prompt for AI Agents
In `@include/boost/http/server/etag.hpp` around lines 35 - 40, The documentation
example for etag() is inconsistent with the implementation in etag.cpp: replace
the base64/slash-style example with one that matches the actual output format
produced by etag (size in hex, a hyphen, then the 16-hex-character FNV-1a hash),
e.g. something like "\"<size_hex>-<16_hex_chars>\"" or, alternatively, add a
note that the example is illustrative only; update the docblock around the etag
function to reflect this change.
| @par Example | ||
| @code | ||
| route_task serve( route_params& rp, std::string path ) | ||
| { | ||
| send_file_info info; | ||
| send_file_init( info, rp, path ); | ||
| if( info.result == send_file_result::not_modified ) | ||
| { | ||
| rp.status( status::not_modified ); | ||
| co_return co_await rp.send( "" ); | ||
| } | ||
| if( info.result != send_file_result::ok ) | ||
| co_return route::next; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate route_params and check for any status helper
route_params_files=$(rg -l 'struct route_params|class route_params' --type cpp --type hpp)
echo "Files containing route_params definition:"
echo "$route_params_files"
echo ""
echo "Searching for status-related methods:"
rg -n 'status\s*\(' $route_params_files | head -30Repository: cppalliance/http
Length of output: 2499
🏁 Script executed:
#!/bin/bash
# Search for route_params definition in header files
rg 'route_params' include/ -t rust -A 10 | head -100Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Find route_params definition
find include -name "*.h" -o -name "*.hpp" | xargs grep -l "route_params"
echo ""
echo "Looking for route_params struct/class definition:"
rg 'struct route_params|class route_params' include/
echo ""
echo "Looking for status method in route_params context:"
rg -B 5 -A 15 'struct route_params|class route_params' include/Repository: cppalliance/http
Length of output: 6588
🏁 Script executed:
#!/bin/bash
# Get the full route_params_base class definition
cat include/boost/http/server/router_types.hpp | grep -n "class route_params_base" -A 200 | head -250Repository: cppalliance/http
Length of output: 2797
🏁 Script executed:
#!/bin/bash
# Check route_handler.hpp for status method
sed -n '140,170p' include/boost/http/server/route_handler.hppRepository: cppalliance/http
Length of output: 1003
🏁 Script executed:
#!/bin/bash
# Find route_params class definition (not _base)
rg -n 'class route_params[^_]|struct route_params[^_]' include/ -A 15Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for route_params definition/typedef
rg -n 'using route_params|typedef.*route_params|route_params =' include/
echo ""
echo "Check what route_params is passed to the status() method:"
rg -B 5 -A 10 'status\(http::status' include/boost/http/server/route_handler.hppRepository: cppalliance/http
Length of output: 1024
🏁 Script executed:
#!/bin/bash
# Check the full send_file.hpp file structure
head -50 include/boost/http/server/send_file.hpp
echo "---"
rg -n 'template.*route_params|route_params.*template' include/boost/http/server/ -A 5Repository: cppalliance/http
Length of output: 1298
🏁 Script executed:
#!/bin/bash
# Check the send_file.cpp implementation
grep -n "status" src/server/send_file.cpp | head -20
echo ""
echo "Show context around set_status usage:"
sed -n '150,195p' src/server/send_file.cppRepository: cppalliance/http
Length of output: 1938
🏁 Script executed:
#!/bin/bash
# Check if rp.status() is actually used in implementations
echo "Search for rp.status() usage in source files:"
rg 'rp\.status\s*\(' src/ --type cpp
echo ""
echo "Compare with rp.res.set_status() usage:"
rg 'rp\.res\.set_status\s*\(' src/ --type cppRepository: cppalliance/http
Length of output: 1063
Update doc example to use actual implementation pattern
The example at lines 109–111 uses rp.status(status::not_modified), but the actual implementation consistently uses rp.res.set_status(status::...) throughout send_file.cpp and other implementations. Change the example to:
if( info.result == send_file_result::not_modified )
{
rp.res.set_status( status::not_modified );
co_return co_await rp.send( "" );
}🤖 Prompt for AI Agents
In `@include/boost/http/server/send_file.hpp` around lines 102 - 116, The doc
example uses rp.status(status::not_modified) which is inconsistent with the
implementation; update the example in the send_file.hpp snippet so that when
send_file_info::result == send_file_result::not_modified it calls
rp.res.set_status(status::not_modified) instead of rp.status(...), preserving
the co_return co_await rp.send("") flow and keeping references to
send_file_info, send_file_init, and send_file_result to match the real
implementation pattern.
src/server/etag.cpp
Outdated
| // Format: size-hash | ||
| char buf[32]; | ||
| std::snprintf( buf, sizeof( buf ), "%zx-", body.size() ); | ||
| result.append( buf ); | ||
|
|
||
| char hex[16]; | ||
| to_hex( hash, hex ); | ||
| result.append( hex, 16 ); |
There was a problem hiding this comment.
Resolve CI -Wrestrict failure in ETag formatting.
CI reports a -Werror on std::char_traits::copy triggered by the current append path (Line 61, Line 86). Switching to length-aware append avoids the append(const char*) overload and should silence the overlap warning while keeping the formatting logic intact.
🛠️ Proposed fix (use length-aware append)
- std::snprintf( buf, sizeof( buf ), "%zx-", body.size() );
- result.append( buf );
+ int n = std::snprintf( buf, sizeof( buf ), "%zx-", body.size() );
+ if( n > 0 && n < static_cast<int>( sizeof( buf ) ) )
+ result.append( buf, static_cast<std::size_t>( n ) );
@@
- std::snprintf( buf, sizeof( buf ),
- "%llx-%llx",
- static_cast<unsigned long long>( size ),
- static_cast<unsigned long long>( mtime ) );
- result.append( buf );
+ int n = std::snprintf( buf, sizeof( buf ),
+ "%llx-%llx",
+ static_cast<unsigned long long>( size ),
+ static_cast<unsigned long long>( mtime ) );
+ if( n > 0 && n < static_cast<int>( sizeof( buf ) ) )
+ result.append( buf, static_cast<std::size_t>( n ) );Also applies to: 85-90
🤖 Prompt for AI Agents
In `@src/server/etag.cpp` around lines 59 - 66, The append calls that pass
C-strings are hitting the append(const char*) overload and triggering
-Wrestrict; update them to use the length-aware overload instead: when writing
the size prefix built into buf (in the ETag formatting code that uses
std::snprintf and buf) call result.append(buf, std::strlen(buf)) (or capture
snprintf's return and append that many bytes) rather than result.append(buf),
and make the same replacement for the other occurrence in the file (the hex
append already uses result.append(hex, 16) but ensure any other
result.append(...) calls that take a raw char* use the length-aware overload);
reference symbols: result.append, buf, hex, to_hex, body.size(), hash.
| // Handle Range header | ||
| auto range_header = rp.req.value_or(field::range, ""); | ||
| if(! range_header.empty()) | ||
| { | ||
| auto range_result = parse_range( | ||
| static_cast<std::int64_t>(info.size), | ||
| range_header); | ||
|
|
||
| if(range_result.type == range_result_type::ok && | ||
| ! range_result.ranges.empty()) | ||
| { | ||
| // Use first range only (simplification) | ||
| auto const& range = range_result.ranges[0]; | ||
| info.is_range = true; | ||
| info.range_start = range.start; | ||
| info.range_end = range.end; | ||
|
|
||
| // Set 206 Partial Content | ||
| rp.res.set_status(status::partial_content); | ||
|
|
||
| auto const content_length = | ||
| range.end - range.start + 1; | ||
| rp.res.set_payload_size( | ||
| static_cast<std::uint64_t>(content_length)); | ||
|
|
||
| // Content-Range header | ||
| std::string cr = "bytes " + | ||
| std::to_string(range.start) + "-" + | ||
| std::to_string(range.end) + "/" + | ||
| std::to_string(info.size); | ||
| rp.res.set(field::content_range, cr); | ||
|
|
||
| info.result = send_file_result::ok; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Avoid returning 206 for multi‑range requests unless multipart is implemented.
Right now the first range is served even when the request has multiple ranges. That can mislead clients expecting a multipart/byteranges response. If you don’t plan to implement multipart, ignore the Range header when more than one range is requested and fall back to full content.
🔧 Suggested adjustment
- if(range_result.type == range_result_type::ok &&
- ! range_result.ranges.empty())
+ if(range_result.type == range_result_type::ok &&
+ range_result.ranges.size() == 1)
{
// Use first range only (simplification)
auto const& range = range_result.ranges[0];
info.is_range = true;
info.range_start = range.start;
info.range_end = range.end;
@@
info.result = send_file_result::ok;
return;
}🤖 Prompt for AI Agents
In `@src/server/send_file.cpp` around lines 139 - 173, The code currently accepts
the first range from parse_range even when multiple ranges are requested; change
the logic inside the Range handling block (the branch using parse_range,
range_result_type::ok, and range_result.ranges) to only honor the Range when
range_result.ranges.size() == 1; if there are >1 ranges, treat the header as
absent (do not set info.is_range, do not set
rp.res.set_status(status::partial_content), do not set Content-Range, and let
the normal full-content path run), since multipart/byteranges is not
implemented; keep existing behavior for the single-range case (setting
info.range_start/end and send_file_result::ok).
| bool | ||
| is_dotfile(core::string_view path) noexcept | ||
| { | ||
| auto pos = path.rfind('/'); | ||
| if(pos == core::string_view::npos) | ||
| pos = 0; | ||
| else | ||
| ++pos; | ||
|
|
||
| if(pos < path.size() && path[pos] == '.') | ||
| return true; | ||
|
|
||
| return false; |
There was a problem hiding this comment.
Dotfile detection only checks the final path segment.
Requests like /.git/config will bypass the current check because only the last segment is inspected. Consider scanning all path segments (or explicitly exempting .well-known if that’s intended).
🔧 Suggested fix
bool
is_dotfile(core::string_view path) noexcept
{
- auto pos = path.rfind('/');
- if(pos == core::string_view::npos)
- pos = 0;
- else
- ++pos;
-
- if(pos < path.size() && path[pos] == '.')
- return true;
-
- return false;
+ std::size_t start = 0;
+ while(start <= path.size())
+ {
+ auto end = path.find('/', start);
+ if(end == core::string_view::npos)
+ end = path.size();
+ if(end > start && path[start] == '.')
+ return true;
+ start = end + 1;
+ }
+ return false;
}📝 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.
| bool | |
| is_dotfile(core::string_view path) noexcept | |
| { | |
| auto pos = path.rfind('/'); | |
| if(pos == core::string_view::npos) | |
| pos = 0; | |
| else | |
| ++pos; | |
| if(pos < path.size() && path[pos] == '.') | |
| return true; | |
| return false; | |
| bool | |
| is_dotfile(core::string_view path) noexcept | |
| { | |
| std::size_t start = 0; | |
| while(start <= path.size()) | |
| { | |
| auto end = path.find('/', start); | |
| if(end == core::string_view::npos) | |
| end = path.size(); | |
| if(end > start && path[start] == '.') | |
| return true; | |
| start = end + 1; | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In `@src/server/serve_static.cpp` around lines 55 - 67, The is_dotfile function
only checks the last path segment and thus misses dotfiles in earlier segments
(e.g., "/.git/config"); update is_dotfile to scan all path segments of the input
core::string_view path and return true if any segment begins with a '.' (except
explicitly allowlisting ".well-known" if that is desired); modify the logic in
is_dotfile to iterate over segments split by '/' (or walk the string looking for
'/' boundaries) and check the first character after each boundary for '.',
keeping the function noexcept and using core::string_view operations so callers
like serve_static can rely on the updated behavior.
| // Build the file path | ||
| std::string path; | ||
| path_cat(path, impl_->root, req_path); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the serve_static.cpp file
find . -name "serve_static.cpp" -type fRepository: cppalliance/http
Length of output: 125
🏁 Script executed:
# Read the file to see lines 152-155 and surrounding context
wc -l src/server/serve_static.cppRepository: cppalliance/http
Length of output: 91
🏁 Script executed:
# Get the specific lines in question with surrounding context
sed -n '140,170p' src/server/serve_static.cppRepository: cppalliance/http
Length of output: 995
🏁 Script executed:
# Find the path_cat function definition
rg -A 10 "^\s*void\s+path_cat|^\s*std::string\s+path_cat|^path_cat" src/Repository: cppalliance/http
Length of output: 593
🏁 Script executed:
# Search for any path normalization or canonicalization in the file
rg -i "canonical|normalize|normalizePath" src/server/serve_static.cppRepository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Get more of the path_cat function
sed -n '1,50p' src/server/serve_static.cpp | tail -30Repository: cppalliance/http
Length of output: 723
🏁 Script executed:
# Find where path_cat is defined - search more broadly
rg -B 2 -A 20 "path_cat\(" src/server/serve_static.cpp | head -60Repository: cppalliance/http
Length of output: 1308
🏁 Script executed:
# Check if there are any validation checks after path_cat is called
sed -n '152,200p' src/server/serve_static.cppRepository: cppalliance/http
Length of output: 1396
🏁 Script executed:
# Look for any usage of std::filesystem::canonical or similar normalization
rg "std::filesystem::" src/server/serve_static.cppRepository: cppalliance/http
Length of output: 119
🏁 Script executed:
# Check the includes to see what filesystem functions are available
head -30 src/server/serve_static.cppRepository: cppalliance/http
Length of output: 809
🏁 Script executed:
# Look at the complete path_cat function to see the full implementation
rg -A 30 "void\s+path_cat" src/server/serve_static.cppRepository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Check the send_file_init function to see if it does any path validation
rg -B 2 -A 30 "send_file_init" src/server/serve_static.cpp | head -50Repository: cppalliance/http
Length of output: 1436
🏁 Script executed:
# Search for send_file.hpp to see what validation send_file_init does
find . -name "send_file.hpp" -o -name "send_file.cpp" | head -5Repository: cppalliance/http
Length of output: 128
🏁 Script executed:
# Check if send_file_init does any path validation or canonicalization
rg "send_file_init" src/server/send_file.cpp src/server/send_file.hpp 2>/dev/null | head -20Repository: cppalliance/http
Length of output: 100
🏁 Script executed:
# Look for any access control or path validation in the serve_static implementation
rg -i "access|permission|traverse|escape|outside|root" src/server/serve_static.cppRepository: cppalliance/http
Length of output: 327
🏁 Script executed:
# Get the complete path_cat function
sed -n '24,60p' src/server/serve_static.cppRepository: cppalliance/http
Length of output: 841
🏁 Script executed:
# Look at send_file_init implementation
sed -n '1,100p' src/server/send_file.cppRepository: cppalliance/http
Length of output: 2416
🏁 Script executed:
# Check send_file.hpp for function signature and documentation
cat include/boost/http/server/send_file.hpp | head -80Repository: cppalliance/http
Length of output: 1889
🏁 Script executed:
# Search for any canonicalization or path normalization in send_file
rg "canonical|normalize" src/server/send_file.cppRepository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Check where req_path comes from and if it's validated
rg "req_path" src/server/serve_static.cpp | head -20Repository: cppalliance/http
Length of output: 269
🏁 Script executed:
# Look at the complete operator() function to see the full flow
sed -n '100,160p' src/server/serve_static.cppRepository: cppalliance/http
Length of output: 1531
🏁 Script executed:
# Check if there's any ".." handling or path traversal defense
rg "\\.\\./" src/server/serve_static.cppRepository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Check if req_path is URL decoded and how
rg -B 5 -A 5 "req_path" src/server/serve_static.cpp | head -40Repository: cppalliance/http
Length of output: 1134
🏁 Script executed:
# Check the full operator() function to see if there are any other validations
sed -n '110,280p' src/server/serve_static.cppRepository: cppalliance/http
Length of output: 4666
🏁 Script executed:
# Check the get_file_stats function in send_file.cpp more carefully
sed -n '25,60p' src/server/send_file.cppRepository: cppalliance/http
Length of output: 971
🏁 Script executed:
# Verify: does std::filesystem::is_directory resolve symlinks?
# Let's check if there's any relative_to or other containment check
rg "relative_to|is_within|inside|contains" src/Repository: cppalliance/http
Length of output: 257
🏁 Script executed:
# Check if the path variable is ever validated against root after concatenation
sed -n '150,180p' src/server/serve_static.cppRepository: cppalliance/http
Length of output: 906
Add path containment validation to prevent directory traversal attacks.
path_cat concatenates the root directory and request path without normalization or validation. Request paths containing .. segments or symlinks pointing outside the root can bypass the intended directory boundary. Implement explicit path containment checks before serving any file:
- Resolve the concatenated path to its canonical form
- Verify the resolved path is within the root directory
- Reject requests that attempt to escape the root
The suggested approach using weakly_canonical path comparison is valid, or alternatively use std::filesystem::relative() and validate the result does not start with ...
🤖 Prompt for AI Agents
In `@src/server/serve_static.cpp` around lines 152 - 155, The concatenation using
path_cat(path, impl_->root, req_path) lacks normalization and allows directory
traversal; after building path, resolve both impl_->root and the concatenated
path to canonical forms (e.g., std::filesystem::weakly_canonical or equivalent)
and verify the resolved file path is contained within the resolved root before
serving; if the resolved path lies outside the root (or
std::filesystem::relative(resolvedPath, resolvedRoot) begins with ".."), reject
the request and return an error. Ensure this check is performed immediately
after path_cat and before any file access in the serve_static logic.
| while(remaining > 0) | ||
| { | ||
| auto const to_read = static_cast<std::size_t>( | ||
| (std::min)(remaining, static_cast<std::int64_t>(buf_size))); | ||
|
|
||
| auto const n = f.read(buffer, to_read, ec); | ||
| if(ec || n == 0) | ||
| break; | ||
|
|
||
| co_await rp.write(capy::const_buffer(buffer, n)); | ||
| remaining -= static_cast<std::int64_t>(n); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and examine context around the problematic lines
find . -name "serve_static.cpp" -type fRepository: cppalliance/http
Length of output: 125
🏁 Script executed:
# Get the full context around lines 262-273 to understand Content-Length handling
cat -n src/server/serve_static.cpp | sed -n '240,290p'Repository: cppalliance/http
Length of output: 1421
🏁 Script executed:
# Search for route_params definition and error handling APIs
rg -nP 'struct\s+route_params\b|class\s+route_params\b' -g '*.hpp' -g '*.cpp' -g '*.ipp' | head -20Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Search for error/abort/fail methods
rg -nP '\b(route_params::)?(send|write|end|fail|abort|cancel)\s*\(' -g '*.hpp' -g '*.cpp' -g '*.ipp' | head -30Repository: cppalliance/http
Length of output: 2145
🏁 Script executed:
# Search for Content-Length in the serve_static file
rg -n "Content-Length|content.length" src/server/serve_static.cpp -iRepository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Look at the function signature and beginning to see where Content-Length is set
cat -n src/server/serve_static.cpp | sed -n '100,260p'Repository: cppalliance/http
Length of output: 5553
🏁 Script executed:
# Find route_params class definition more broadly
fd -e hpp -e h | xargs rg -l "route_params" | head -10Repository: cppalliance/http
Length of output: 460
🏁 Script executed:
# Search for route_params definition in those files
fd -e hpp -e h | xargs rg -A 20 "struct\s+route_params|class\s+route_params" | head -100Repository: cppalliance/http
Length of output: 5579
🏁 Script executed:
# Search for send_file_init to understand what headers it sets
rg -n "send_file_init" -A 30 src/server/send_file.cpp | head -100Repository: cppalliance/http
Length of output: 841
🏁 Script executed:
# Find route_params struct definition (not base)
rg -n "^struct route_params|^class route_params" -A 30 include/boost/http/server/router_types.hpp | head -80Repository: cppalliance/http
Length of output: 2279
🏁 Script executed:
# Search for rp.end() and rp.write() in include files to understand the API
rg -n "\.end\(\)|\.write\(" include/boost/http/server/router_types.hpp -B 2 -A 5 | head -100Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Search for route_params with write, end, send methods
rg -n "route_params" include/boost/http/server/route_handler.hpp -A 5 -B 2 | head -150Repository: cppalliance/http
Length of output: 2818
🏁 Script executed:
# Find where Content-Length is actually set during send_file_init
rg -n "Content-Length|content-length|set_header" src/server/send_file.cpp -i -B 2 -A 2 | head -80Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Look at the full send_file_init function
cat -n src/server/send_file.cpp | sed -n '75,250p'Repository: cppalliance/http
Length of output: 4229
🏁 Script executed:
# Find where rp.write() and rp.end() are implemented
rg -n "route_params::write|route_params::end" include/boost/http/server/ -B 2 -A 10 | head -100Repository: cppalliance/http
Length of output: 983
🏁 Script executed:
# Search for write() method in route_handler.hpp
rg -n "write(" include/boost/http/server/route_handler.hpp -B 3 -A 15 | head -150Repository: cppalliance/http
Length of output: 122
🏁 Script executed:
# Look at the implementation of write and end in the header
cat -n include/boost/http/server/route_handler.hpp | sed -n '200,400p'Repository: cppalliance/http
Length of output: 3457
🏁 Script executed:
# Search for set_payload_size to confirm it sets Content-Length
rg -n "set_payload_size" include/boost/http/server/ -B 2 -A 5Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Check if there's any error recovery or validation after the streaming loop
cat -n src/server/serve_static.cpp | sed -n '262,276p'Repository: cppalliance/http
Length of output: 572
🏁 Script executed:
# Search for how write() and end() handle incomplete responses with Content-Length
rg -n "Content-Length|payload_size" src/server/ -B 3 -A 3 | grep -A 5 -B 5 "write\|end"Repository: cppalliance/http
Length of output: 1145
🏁 Script executed:
# Check if route_params has any error/abort mechanism we missed
rg -n "route_result|abort|cancel|fail|error" include/boost/http/server/route_handler.hpp -B 2 -A 3 | head -80Repository: cppalliance/http
Length of output: 1462
🏁 Script executed:
# Check what route_result enum contains
rg -n "enum.*route_result|enum class route_result" include/boost/http/server/ -A 10Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Search for how the response headers are committed/finalized
rg -n "headers.*sent|commit|finalize" src/server/ -i -B 2 -A 2Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Find route_result definition
rg -n "route_result" include/boost/http/server/router_types.hpp -B 2 -A 10 | head -100Repository: cppalliance/http
Length of output: 1082
🏁 Script executed:
# Find route namespace and route::next
rg -n "namespace route|route::next" include/boost/http/server/ -B 2 -A 5 | head -80Repository: cppalliance/http
Length of output: 5615
🏁 Script executed:
# Check what happens with remaining > 0 after the loop breaks
rg -n "remaining" src/server/serve_static.cpp -B 3 -A 3Repository: cppalliance/http
Length of output: 766
🏁 Script executed:
# Check if write() can return errors that should be checked
rg -n "co_await rp.write" src/server/serve_static.cpp -B 2 -A 2Repository: cppalliance/http
Length of output: 214
🏁 Script executed:
# Look at how end() behaves and what happens if write fails
cat -n include/boost/http/server/route_handler.hpp | sed -n '240,290p'Repository: cppalliance/http
Length of output: 1779
🏁 Script executed:
# Check test file to see how errors are handled in streaming scenarios
rg -n "write|error|break" test/unit/server/serve_static.cpp -B 2 -A 2 | head -100Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Check if rp.end() is documented to handle incomplete writes
rg -n "incomplete|truncated|mismatch" include/boost/http/server/ -i -B 2 -A 2Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Look for how other parts of the codebase handle streaming with Content-Length
rg -n "set_payload_size" src/server/ -B 5 -A 15 | head -120Repository: cppalliance/http
Length of output: 2848
🏁 Script executed:
# Check if write() returns are checked anywhere else in codebase
rg -n "co_await.*write\(" src/ -B 1 -A 1 | grep -A 1 -B 1 "if\|error\|ec"Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Check when headers are actually sent/committed
rg -n "write_impl|end()" src/server/ -B 5 -A 15 | grep -A 10 -B 10 "header\|commit\|send" | head -150Repository: cppalliance/http
Length of output: 8067
Check for read errors and validate complete transmission before finalizing the response.
If f.read() sets ec or returns 0 before all bytes are written, the loop exits but the response headers with Content-Length are already committed. Calling rp.end() unconditionally then produces a truncated response that violates the HTTP protocol. Check that remaining == 0 after the loop or propagate the read error; alternatively, detect write failures and abort the response/connection.
🤖 Prompt for AI Agents
In `@src/server/serve_static.cpp` around lines 262 - 273, The loop reading from
f.read and writing via rp.write must detect partial reads or errors and abort
the response instead of unconditionally calling rp.end; after the loop check if
ec is set or remaining != 0 (or if any rp.write failed/returned an error) and,
in that case, propagate the error by logging and closing/aborting the
response/connection (e.g., call rp.abort() or set an error status) rather than
calling rp.end(); update the logic around the read/write block (references:
f.read, remaining, ec, rp.write, rp.end) to ensure you only finalize the
response when remaining == 0 and no errors occurred.
| void | ||
| test_is_redirect() | ||
| { | ||
| // Redirect status codes | ||
| BOOST_TEST( statuses::is_redirect( 300 ) ); | ||
| BOOST_TEST( statuses::is_redirect( 301 ) ); | ||
| BOOST_TEST( statuses::is_redirect( 302 ) ); | ||
| BOOST_TEST( statuses::is_redirect( 303 ) ); | ||
| BOOST_TEST( statuses::is_redirect( 307 ) ); | ||
| BOOST_TEST( statuses::is_redirect( 308 ) ); | ||
|
|
||
| // Not redirects | ||
| BOOST_TEST( ! statuses::is_redirect( 200 ) ); | ||
| BOOST_TEST( ! statuses::is_redirect( 304 ) ); // Not Modified is not a redirect | ||
| BOOST_TEST( ! statuses::is_redirect( 404 ) ); | ||
| } |
There was a problem hiding this comment.
Missing test coverage for status code 305 (Use Proxy).
The implementation in src/server/statuses.cpp returns true for is_redirect(305), but this test doesn't verify that behavior. Consider adding:
BOOST_TEST( statuses::is_redirect( 303 ) );
+BOOST_TEST( statuses::is_redirect( 305 ) );
BOOST_TEST( statuses::is_redirect( 307 ) );📝 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.
| void | |
| test_is_redirect() | |
| { | |
| // Redirect status codes | |
| BOOST_TEST( statuses::is_redirect( 300 ) ); | |
| BOOST_TEST( statuses::is_redirect( 301 ) ); | |
| BOOST_TEST( statuses::is_redirect( 302 ) ); | |
| BOOST_TEST( statuses::is_redirect( 303 ) ); | |
| BOOST_TEST( statuses::is_redirect( 307 ) ); | |
| BOOST_TEST( statuses::is_redirect( 308 ) ); | |
| // Not redirects | |
| BOOST_TEST( ! statuses::is_redirect( 200 ) ); | |
| BOOST_TEST( ! statuses::is_redirect( 304 ) ); // Not Modified is not a redirect | |
| BOOST_TEST( ! statuses::is_redirect( 404 ) ); | |
| } | |
| void | |
| test_is_redirect() | |
| { | |
| // Redirect status codes | |
| BOOST_TEST( statuses::is_redirect( 300 ) ); | |
| BOOST_TEST( statuses::is_redirect( 301 ) ); | |
| BOOST_TEST( statuses::is_redirect( 302 ) ); | |
| BOOST_TEST( statuses::is_redirect( 303 ) ); | |
| BOOST_TEST( statuses::is_redirect( 305 ) ); | |
| BOOST_TEST( statuses::is_redirect( 307 ) ); | |
| BOOST_TEST( statuses::is_redirect( 308 ) ); | |
| // Not redirects | |
| BOOST_TEST( ! statuses::is_redirect( 200 ) ); | |
| BOOST_TEST( ! statuses::is_redirect( 304 ) ); // Not Modified is not a redirect | |
| BOOST_TEST( ! statuses::is_redirect( 404 ) ); | |
| } |
🤖 Prompt for AI Agents
In `@test/unit/server/statuses.cpp` around lines 34 - 49, The test suite is
missing coverage for HTTP status 305; update the test_is_redirect() unit test to
assert the current behavior of statuses::is_redirect for 305 by adding a
BOOST_TEST( statuses::is_redirect( 305 ) ); (place it with the other "Redirect
status codes" assertions inside test_is_redirect) so the test verifies the
implementation's handling of the Use Proxy code.
|
An automated preview of the documentation is available at https://245.http.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-01-19 23:46:27 UTC |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/boost/http/server/cors.hpp (1)
13-16: Add<string>include forstd::stringtypes.The
cors_optionsstruct usesstd::string(lines 26, 29, 32, 35), but this header doesn't explicitly include<string>. While currently available through transitive includes (route_handler.hpp→router_types.hpp→<string>), relying on transitive includes is fragile and violates the principle that headers should explicitly include all dependencies they use.Proposed fix
`#include` <boost/http/server/route_handler.hpp> `#include` <boost/http/status.hpp> `#include` <chrono> +#include <string>
🤖 Fix all issues with AI agents
In `@src/server/send_file.cpp`:
- Around line 187-192: When info.size is 0 the code sets info.range_end to -1
which can break callers; update the block that sets
rp.res.set_payload_size(info.size), info.range_start, info.range_end and
info.result so it special-cases empty files: if info.size == 0 set
rp.res.set_payload_size(0), info.range_start = 0 and info.range_end = 0 (instead
of -1) and leave info.result = send_file_result::ok, otherwise keep the existing
logic; adjust the handling in the send_file_result::ok branch (use symbols
rp.res, info.range_start, info.range_end, info.size, send_file_result::ok) so
callers receive a non-negative range for empty files.
- Around line 48-54: Replace the manual "now-offset" conversion around sctp and
mtime: instead of computing sctp via ftime - file_time_type::clock::now() +
system_clock::now(), use the standard C++20 conversion to system_clock (e.g.,
std::chrono::clock_cast<std::chrono::system_clock>(ftime) or
std::filesystem::file_time_type::clock::to_sys/
std::filesystem::file_clock::to_sys(ftime) depending on your standard library)
and then compute mtime with std::chrono::system_clock::to_time_t(...) from that
converted time_point; update the code that sets sctp and mtime (references:
sctp, mtime, ftime, std::filesystem::file_time_type) to use the
clock_cast/to_sys approach for accurate epoch translation.
In `@src/server/serve_static.cpp`:
- Around line 232-242: The TOCTOU bug occurs because send_file_init performs
statting/headers but the file is actually opened later with capy::file::open in
this block; to fix, open the file before calling send_file_init (or merge open
into send_file_init) so the same file descriptor is used for stat/size and
header generation, and if open fails handle errors consistently (respect
impl_->opts.fallthrough and set rp.res/status before calling rp.send). Update
the flow around send_file_init and the capy::file f.open call (and related error
handling that returns route::next or uses rp.send) so headers are only set after
a successful open or so that open failures roll back any partial response
headers.
♻️ Duplicate comments (10)
include/boost/http/server/encode_url.hpp (1)
20-39: Clarify component-vs-full-URL encoding semantics.The doc still says “URL or URL component,” but reserved characters are preserved, which is incorrect for generic components. Please either document this as full-URL encoding only or provide a component-safe variant.
🔧 Minimal doc clarification (if keeping current behavior)
- `@param` url The URL or URL component to encode. + `@param` url The full URL to encode (reserved characters are preserved).src/server/fresh.cpp (2)
46-56: Substring ETag matching can return false positives.The
find()call on line 49 can match partial tokens (e.g.,"abc"inside"xabc") leading to incorrect 304 responses. This should parse the comma-separated list and perform exact token comparison.
64-76: Lexicographic date comparison does not handle RFC 7231's multiple date formats.RFC 7231 allows three date formats (IMF-fixdate, RFC 850, and asctime). A simple string comparison will produce incorrect results when the client and server use different formats. Proper HTTP date parsing should be implemented.
include/boost/http/server/etag.hpp (1)
35-40: Update ETag example to match hex output.Line 39 shows a base64-like value, but the implementation formats
"size_hex-<16_hex_chars>"(seesrc/server/etag.cpp). Please update the example or mark it illustrative.🛠️ Suggested doc tweak
- // tag == "\"d-3/1gIbsr1bCvZ2KQgJ7DpTGR3YH\"" + // tag == "\"d-<16_hex_chars>\"" // format: "<size_hex>-<hash_hex>"src/server/etag.cpp (1)
60-62: Avoidappend(const char*)to silence -Wrestrict.Lines 61–62 and 90 append
bufvia the C-string overload, which triggers-Wrestrictin CI. Use the length-aware overload withsnprintf’s return.🛠️ Proposed fix (length-aware append)
- std::snprintf( buf, sizeof( buf ), "%zx-", body.size() ); - result.append( buf ); + int n = std::snprintf( buf, sizeof( buf ), "%zx-", body.size() ); + if( n > 0 && n < static_cast<int>( sizeof( buf ) ) ) + result.append( buf, static_cast<std::size_t>( n ) ); @@ - std::snprintf( buf, sizeof( buf ), - "%llx-%llx", - static_cast<unsigned long long>( size ), - static_cast<unsigned long long>( mtime ) ); - result.append( buf ); + int n = std::snprintf( buf, sizeof( buf ), + "%llx-%llx", + static_cast<unsigned long long>( size ), + static_cast<unsigned long long>( mtime ) ); + if( n > 0 && n < static_cast<int>( sizeof( buf ) ) ) + result.append( buf, static_cast<std::size_t>( n ) );Also applies to: 85-90
src/server/mime_types.cpp (1)
27-81: Fixext_dbordering for binary search.Line 80 puts
"7z"after"zip", which breaks the sorted order and can makelookup_extmiss entries.🛠️ Proposed fix (reorder entry)
constexpr ext_entry ext_db[] = { + { "7z", "application/x-7z-compressed" }, { "aac", "audio/aac" }, { "avif", "image/avif" }, @@ - { "zip", "application/zip" }, - { "7z", "application/x-7z-compressed" }, + { "zip", "application/zip" }, };src/server/send_file.cpp (1)
147-173: Avoid returning 206 for multi-range requests unless multipart is implemented.The code serves only the first range even when multiple ranges are requested, which can mislead clients expecting a
multipart/byterangesresponse. Consider restricting to single-range requests only.🔧 Suggested adjustment
- if(range_result.type == range_result_type::ok && - ! range_result.ranges.empty()) + if(range_result.type == range_result_type::ok && + range_result.ranges.size() == 1) { // Use first range only (simplification)src/server/serve_static.cpp (3)
55-68: Dotfile detection only checks the final path segment.Requests like
/.git/configwill bypass the current check because only the last segment is inspected. All path segments should be scanned for dotfiles.
152-155: Add path containment validation to prevent directory traversal attacks.
path_catconcatenates the root directory and request path without normalization or validation. Request paths containing..segments can escape the root directory. Resolve the concatenated path to canonical form and verify it remains within the root.
262-275: Check for read errors and validate complete transmission before finalizing the response.If
f.read()setsecor returns0before all bytes are written, the loop exits but response headers withContent-Lengthare already committed. Callingrp.end()unconditionally produces a truncated response violating HTTP protocol.🔧 Suggested fix
while(remaining > 0) { auto const to_read = static_cast<std::size_t>( (std::min)(remaining, static_cast<std::int64_t>(buf_size))); auto const n = f.read(buffer, to_read, ec); if(ec || n == 0) break; co_await rp.write(capy::const_buffer(buffer, n)); remaining -= static_cast<std::int64_t>(n); } - co_return co_await rp.end(); + // Check if we sent all expected bytes + if(remaining > 0) + { + // Truncated response - connection should be closed + // Log error or handle appropriately + co_return route::error; // or appropriate error handling + } + + co_return co_await rp.end();
🧹 Nitpick comments (5)
include/boost/http/server/statuses.hpp (1)
87-103: LGTM!The selection of 502, 503, and 504 as retryable codes is a sensible conservative choice focusing on gateway/server transient failures.
Optional consideration: Some applications also treat 408 (Request Timeout) and 429 (Too Many Requests) as retryable. If broader retry semantics are desired in the future, these could be considered for inclusion or offered as a separate predicate. The current focused set is reasonable for the stated purpose.
src/server/send_file.cpp (1)
66-70: Consider checkingstrftimereturn value.
std::strftimereturns 0 on failure (buffer too small or encoding error). While the 64-byte buffer is sufficient for this format, defensive code would check the return value.♻️ Suggested defensive check
char buf[64]; - std::strftime(buf, sizeof(buf), + auto len = std::strftime(buf, sizeof(buf), "%a, %d %b %Y %H:%M:%S GMT", &tm); - return std::string(buf); + if(len == 0) + return std::string(); // or a fallback + return std::string(buf, len);include/boost/http/server/serve_static.hpp (1)
79-119: Consider explicitly deleting copy operations for clarity.The class has a move constructor but no copy constructor or assignment operators. While the raw
impl_pointer makes copying unsafe, explicitly deleting copy operations would make the intent clearer and prevent accidental implicit copies.♻️ Suggested addition
/** Move constructor. */ serve_static(serve_static&& other) noexcept; + + /** Deleted copy constructor. + */ + serve_static(serve_static const&) = delete; + + /** Deleted copy assignment. + */ + serve_static& operator=(serve_static const&) = delete; + + /** Move assignment operator. + */ + serve_static& operator=(serve_static&& other) noexcept;src/server/serve_static.cpp (2)
106-111: Move constructor leaves source in potentially dangerous state.While setting
other.impl_tonullptris correct, the destructor will calldelete nullptrwhich is safe. However, the class lacks a move assignment operator, which could lead to resource leaks if assigned post-construction.♻️ Add move assignment operator
serve_static:: serve_static(serve_static&& other) noexcept : impl_(other.impl_) { other.impl_ = nullptr; } + +serve_static& +serve_static:: +operator=(serve_static&& other) noexcept +{ + if(this != &other) + { + delete impl_; + impl_ = other.impl_; + other.impl_ = nullptr; + } + return *this; +}
220-226: Cache-Control header may be set twice.
send_file_initalready setsCache-Control: public, max-age=Nwhenopts.max_age > 0. This code then overwrites it with theimmutablevariant. While functionally correct, the duplicate set operation is inefficient and could be consolidated.Consider passing the
immutableflag tosend_file_initor setting Cache-Control only once aftersend_file_initreturns.
| // Convert to Unix timestamp | ||
| auto const sctp = std::chrono::time_point_cast< | ||
| std::chrono::system_clock::duration>( | ||
| ftime - std::filesystem::file_time_type::clock::now() + | ||
| std::chrono::system_clock::now()); | ||
| mtime = static_cast<std::uint64_t>( | ||
| std::chrono::system_clock::to_time_t(sctp)); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
C++20 std::filesystem::file_time_type to time_t conversion best practice
💡 Result:
Best practice (C++20)
-
Convert file_time_type -> system_clock via file_clock utilities (prefer std::chrono::clock_cast when available), then to time_t:
std::time_t to_time_t(std::filesystem::file_time_type ft)
{
using namespace std::chrono;
auto st = clock_cast<system_clock>(ft); // or file_clock::to_sys(ft)
return system_clock::to_time_t(st);
}
This preserves correct epoch/translation implemented by the library (no manual epoch math). (docs.w3cub.com)
Fallbacks for pre-C++20
- There is no fully portable, guaranteed conversion before C++20. Common pragmatic options are:
- Convert the other way: convert your time_t to file_time_type (safer for comparisons) if your implementation supports it. (stackoverflow.com)
- Use the “now-offset” trick: map ft to system_clock by aligning clocks via their now() values (approximate, not exact). (stackoverflow.com)
- Use implementation-specific APIs (Windows FILETIME mapping, libc++/libstdc++ helpers) if you need exact conversion. (cppstories.com)
References
- cppreference: file_clock / file_time_type (conversion helpers and clock_cast). [online]. (docs.w3cub.com)
- StackOverflow discussion and practical pre-C++20 workarounds. (stackoverflow.com)
- Article and implementation notes about platform specifics (MSVC vs GCC/Clang). (cppstories.com)
Citations:
- 1: https://docs.w3cub.com/cpp/chrono/file_clock/to_from_sys?utm_source=openai
- 2: https://stackoverflow.com/questions/51273205/how-to-compare-time-t-and-stdfilesystemfile-time-type?utm_source=openai
- 3: https://stackoverflow.com/questions/61030383/how-to-convert-stdfilesystemfile-time-type-to-time-t?utm_source=openai
- 4: https://www.cppstories.com/2024/file-time-cpp20/?utm_source=openai
- 5: https://docs.w3cub.com/cpp/chrono/file_clock/to_from_sys?utm_source=openai
- 6: https://stackoverflow.com/questions/61030383/how-to-convert-stdfilesystemfile-time-type-to-time-t?utm_source=openai
- 7: https://www.cppstories.com/2024/file-time-cpp20/?utm_source=openai
🏁 Script executed:
cat -n src/server/send_file.cpp | sed -n '40,65p'Repository: cppalliance/http
Length of output: 905
File time conversion should use C++20 clock_cast for accuracy.
The current code uses the "now-offset" trick (ftime - file_time_type::clock::now() + system_clock::now()), which is an approximate workaround that loses precision by aligning clocks at a single point in time. C++20 provides the proper solution: use std::chrono::clock_cast<std::chrono::system_clock>(ftime) or std::filesystem::file_clock::to_sys(ftime) followed by system_clock::to_time_t(). This avoids clock synchronization assumptions and preserves correct epoch translation implemented by the library.
🤖 Prompt for AI Agents
In `@src/server/send_file.cpp` around lines 48 - 54, Replace the manual
"now-offset" conversion around sctp and mtime: instead of computing sctp via
ftime - file_time_type::clock::now() + system_clock::now(), use the standard
C++20 conversion to system_clock (e.g.,
std::chrono::clock_cast<std::chrono::system_clock>(ftime) or
std::filesystem::file_time_type::clock::to_sys/
std::filesystem::file_clock::to_sys(ftime) depending on your standard library)
and then compute mtime with std::chrono::system_clock::to_time_t(...) from that
converted time_point; update the code that sets sctp and mtime (references:
sctp, mtime, ftime, std::filesystem::file_time_type) to use the
clock_cast/to_sys approach for accurate epoch translation.
| // Full content response | ||
| rp.res.set_status(status::ok); | ||
| rp.res.set_payload_size(info.size); | ||
| info.range_start = 0; | ||
| info.range_end = static_cast<std::int64_t>(info.size) - 1; | ||
| info.result = send_file_result::ok; |
There was a problem hiding this comment.
Potential issue when info.size is 0.
If the file is empty (info.size == 0), then info.range_end becomes -1. This could cause issues in the caller's streaming loop. Consider handling zero-size files explicitly.
🔧 Suggested fix
// Full content response
rp.res.set_status(status::ok);
rp.res.set_payload_size(info.size);
- info.range_start = 0;
- info.range_end = static_cast<std::int64_t>(info.size) - 1;
+ if(info.size > 0)
+ {
+ info.range_start = 0;
+ info.range_end = static_cast<std::int64_t>(info.size) - 1;
+ }
+ else
+ {
+ info.range_start = 0;
+ info.range_end = -1; // Indicates empty file
+ }
info.result = send_file_result::ok;📝 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.
| // Full content response | |
| rp.res.set_status(status::ok); | |
| rp.res.set_payload_size(info.size); | |
| info.range_start = 0; | |
| info.range_end = static_cast<std::int64_t>(info.size) - 1; | |
| info.result = send_file_result::ok; | |
| // Full content response | |
| rp.res.set_status(status::ok); | |
| rp.res.set_payload_size(info.size); | |
| if(info.size > 0) | |
| { | |
| info.range_start = 0; | |
| info.range_end = static_cast<std::int64_t>(info.size) - 1; | |
| } | |
| else | |
| { | |
| info.range_start = 0; | |
| info.range_end = -1; // Indicates empty file | |
| } | |
| info.result = send_file_result::ok; |
🤖 Prompt for AI Agents
In `@src/server/send_file.cpp` around lines 187 - 192, When info.size is 0 the
code sets info.range_end to -1 which can break callers; update the block that
sets rp.res.set_payload_size(info.size), info.range_start, info.range_end and
info.result so it special-cases empty files: if info.size == 0 set
rp.res.set_payload_size(0), info.range_start = 0 and info.range_end = 0 (instead
of -1) and leave info.result = send_file_result::ok, otherwise keep the existing
logic; adjust the handling in the send_file_result::ok branch (use symbols
rp.res, info.range_start, info.range_end, info.size, send_file_result::ok) so
callers receive a non-negative range for empty files.
| // Open and stream the file | ||
| capy::file f; | ||
| system::error_code ec; | ||
| f.open(path.c_str(), capy::file_mode::scan, ec); | ||
| if(ec) | ||
| { | ||
| if(impl_->opts.fallthrough) | ||
| co_return route::next; | ||
| rp.res.set_status(status::internal_server_error); | ||
| co_return co_await rp.send("Internal Server Error"); | ||
| } |
There was a problem hiding this comment.
TOCTOU race between send_file_init and f.open.
The file is stat'd in send_file_init, but opened later. The file could be deleted or modified between these operations, leading to inconsistent behavior (e.g., headers set for a file that no longer exists). Consider opening the file first or handling the open failure more gracefully after headers are partially set.
🤖 Prompt for AI Agents
In `@src/server/serve_static.cpp` around lines 232 - 242, The TOCTOU bug occurs
because send_file_init performs statting/headers but the file is actually opened
later with capy::file::open in this block; to fix, open the file before calling
send_file_init (or merge open into send_file_init) so the same file descriptor
is used for stat/size and header generation, and if open fails handle errors
consistently (respect impl_->opts.fallthrough and set rp.res/status before
calling rp.send). Update the flow around send_file_init and the capy::file
f.open call (and related error handling that returns route::next or uses
rp.send) so headers are only set after a successful open or so that open
failures roll back any partial response headers.
3 participants
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.