Skip to content

doc work #262

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.

Sign up for GitHub

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

Open
vinniefalco wants to merge 5 commits into
base: develop
Choose a base branch
from
+1,437 −162
Open

doc work #262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
26e2a87
Tidy up file placement
vinniefalco Feb 8, 2026
3ee2f01
Add test_worker
vinniefalco Feb 8, 2026
705e5f7
more http_worker tests for unhandled routes
vinniefalco Feb 8, 2026
1a19de1
cors tests
vinniefalco Feb 8, 2026
b443381
additional router OPTIONS tests
vinniefalco Feb 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions include/boost/http/server/router.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
#include <boost/http/detail/config.hpp>
#include <boost/http/server/route_handler.hpp>
#include <boost/http/server/detail/router_base.hpp>
#include <boost/http/field.hpp>
#include <boost/http/method.hpp>
#include <boost/http/status.hpp>
#include <boost/url/url_view.hpp>
#include <boost/mp11/algorithm.hpp>
#include <boost/assert.hpp>
Expand Down Expand Up @@ -598,6 +600,13 @@ class router : public detail::router_base
router_options options = {})
: detail::router_base(options.v_)
{
set_options_handler(
[](P& rp, std::string_view allow) -> route_task {
rp.status(status::no_content);
rp.res.set(field::allow, allow);
(void)(co_await rp.send());
co_return route_done;
});
Comment on lines +603 to +609
Copy link

@coderabbitai coderabbitai bot Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Default OPTIONS handler discards send error.

Same pattern as in http_worker.cpp — if rp.send() fails, the error is silently discarded and route_done is returned. This means the caller won't know the response wasn't actually sent. Low severity since OPTIONS responses are lightweight, but worth noting for consistency.

🤖 Prompt for AI Agents 
In `@include/boost/http/server/router.hpp` around lines 603 - 609, The OPTIONS
handler currently ignores the result of co_await rp.send(), so send failures are
discarded; update the lambda passed to set_options_handler (the handler taking
P& rp) to capture the outcome of co_await rp.send(), check for an error, and
only co_return route_done on success—otherwise propagate or return an
error/failed route result so callers know the response wasn't sent (i.e., handle
rp.send()'s failure instead of discarding it after rp.status(status::no_content)
and rp.res.set(field::allow, allow)).

}

/** Construct a router from another router with compatible types.
Expand Down
31 changes: 22 additions & 9 deletions src/server/any_router.cpp → src/server/detail/router_base.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@
// Official repository: https://github.com/cppalliance/http
//

#include "src/server/detail/any_router.hpp"
#include "src/server/detail/router_base.hpp"
#include <boost/http/server/detail/router_base.hpp>
#include <boost/http/detail/except.hpp>
#include <boost/http/error.hpp>
#include <boost/http/field.hpp>
#include <boost/http/status.hpp>
#include <boost/url/grammar/ci_string.hpp>
#include <boost/url/grammar/hexdig_chars.hpp>
#include "src/server/detail/pct_decode.hpp"
Expand Down Expand Up @@ -206,8 +208,8 @@ dispatch_loop(route_params& p, bool is_options) const
std::size_t last_matched = SIZE_MAX;
std::uint32_t current_depth = 0;

std::uint64_t options_methods = 0;
std::vector<std::string> options_custom_verbs;
std::uint64_t matched_methods = 0;
std::vector<std::string> matched_custom_verbs;

std::size_t path_stack[router_base::max_path_depth];
path_stack[0] = 0;
Expand Down Expand Up @@ -286,12 +288,12 @@ dispatch_loop(route_params& p, bool is_options) const
if(!ancestors_ok)
continue;

// Collect methods from matching end-route matchers for OPTIONS
if(is_options && m.end_)
// Collect methods from matching end-route matchers
if(m.end_)
{
options_methods |= m.allowed_methods_;
matched_methods |= m.allowed_methods_;
for(auto const& v : m.custom_verbs_)
options_custom_verbs.push_back(v);
matched_custom_verbs.push_back(v);
}
Comment on lines +291 to 297
Copy link

@coderabbitai coderabbitai bot Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Method collection runs unconditionally for every matching end-route matcher.

matched_methods and matched_custom_verbs accumulate from every end-route matcher whose path matches, even when the handler subsequently matches the method and runs. This is fine for the Allow header (you want all methods), but matched_custom_verbs is a std::vector<std::string> that may accumulate duplicates across matchers. The build_allow_header function doesn't deduplicate (unlike rebuild_global_allow_header which sorts/uniques). For routes with many custom verbs or overlapping matchers, the Allow header could contain duplicates.

Proposed fix — deduplicate before building 
     if(is_options && matched_methods != 0 && options_handler_)
     {
+        std::sort(matched_custom_verbs.begin(), matched_custom_verbs.end());
+        matched_custom_verbs.erase(
+            std::unique(matched_custom_verbs.begin(), matched_custom_verbs.end()),
+            matched_custom_verbs.end());
         std::string allow = build_allow_header(matched_methods, matched_custom_verbs);
         co_return co_await options_handler_->invoke(p, allow);
     }

     // 405 fallback: path matched but method didn't
     if(!is_options &&
         (matched_methods != 0 || !matched_custom_verbs.empty()))
     {
+        std::sort(matched_custom_verbs.begin(), matched_custom_verbs.end());
+        matched_custom_verbs.erase(
+            std::unique(matched_custom_verbs.begin(), matched_custom_verbs.end()),
+            matched_custom_verbs.end());
         std::string allow = build_allow_header(matched_methods, matched_custom_verbs);
🤖 Prompt for AI Agents 
In `@src/server/detail/router_base.cpp` around lines 291 - 297,
matched_custom_verbs is being appended from every matching end-route matcher
(inside the m.end_ branch) and can accumulate duplicates, causing duplicate
verbs in the Allow header because build_allow_header doesn't deduplicate; update
the logic after collecting from matchers (or immediately before calling
build_allow_header) to deduplicate matched_custom_verbs (e.g., sort and
std::unique or use an unordered_set) so only unique custom verbs are passed to
build_allow_header; keep collecting matched_methods as-is but ensure
matched_custom_verbs is de-duplicated similarly to rebuild_global_allow_header
to avoid repeated verbs in the header.


if(m.end_ && !e.match_method(
Expand Down Expand Up @@ -364,12 +366,23 @@ dispatch_loop(route_params& p, bool is_options) const
co_return route_error(pv.ec_);

// OPTIONS fallback
if(is_options && options_methods != 0 && options_handler_)
if(is_options && matched_methods != 0 && options_handler_)
{
std::string allow = build_allow_header(options_methods, options_custom_verbs);
std::string allow = build_allow_header(matched_methods, matched_custom_verbs);
co_return co_await options_handler_->invoke(p, allow);
}

// 405 fallback: path matched but method didn't
if(!is_options &&
(matched_methods != 0 || !matched_custom_verbs.empty()))
{
std::string allow = build_allow_header(matched_methods, matched_custom_verbs);
p.res.set(field::allow, allow);
p.res.set_status(status::method_not_allowed);
(void)(co_await p.send());
co_return route_done;
}
Comment on lines +375 to +384
Copy link

@coderabbitai coderabbitai bot Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

cat -n src/server/detail/router_base.cpp | sed -n '350,400p'

Repository: cppalliance/http

Length of output: 1718

🏁 Script executed:

# Search for matched_methods initialization and usage
rg -n 'matched_methods' src/server/detail/router_base.cpp -B2 -A2

Repository: cppalliance/http

Length of output: 1049

🏁 Script executed:

# Look for route_next and 405 related logic
rg -n 'route_next\|method_not_allowed' src/server/detail/router_base.cpp -B3 -A3

Repository: cppalliance/http

Length of output: 42

🏁 Script executed:

# Search for any tests related to this scenario
find . -name "*test*.cpp" -o -name "*_test.cpp" | xargs grep -l "route_next\|method_not_allowed" 2>/dev/null | head -5

Repository: cppalliance/http

Length of output: 42

🏁 Script executed:

cat -n src/server/detail/router_base.cpp | sed -n '200,310p'

Repository: cppalliance/http

Length of output: 4102

🏁 Script executed:

# Search for where the current request method is checked against allowed methods
rg -n 'pv\.verb_|allowed_methods' src/server/detail/router_base.cpp -B2 -A2

Repository: cppalliance/http

Length of output: 1227

🏁 Script executed:

# Look for the handler invocation and route_next handling
rg -n 'handler.*invoke|route_next' src/server/detail/router_base.cpp -B3 -A3

Repository: cppalliance/http

Length of output: 913

🏁 Script executed:

cat -n src/server/detail/router_base.cpp | sed -n '300,375p'

Repository: cppalliance/http

Length of output: 2412

🏁 Script executed:

# Look for where handlers are invoked in the loop
rg -n '\.invoke\(p' src/server/detail/router_base.cpp -B5 -A5

Repository: cppalliance/http

Length of output: 42

🏁 Script executed:

find . -type f -name "*.cpp" | xargs grep -l "405\|method_not_allowed" 2>/dev/null | head -10

Repository: cppalliance/http

Length of output: 327

🏁 Script executed:

# Search for test files related to router
find . -type f \( -name "*test*.cpp" -o -name "test_*.cpp" \) | head -10

Repository: cppalliance/http

Length of output: 88

🏁 Script executed:

# Look for tests that cover route_next behavior
rg -l 'route_next' --type=cpp | grep -i test | head -5

Repository: cppalliance/http

Length of output: 227

🏁 Script executed:

# Search for 405 tests in router test files
rg -n '405\|method_not_allowed' test/unit/server/router.cpp -B3 -A3

Repository: cppalliance/http

Length of output: 42

🏁 Script executed:

# Look for tests with route_next and method handling
rg -n 'route_next' test/unit/server/router.cpp -B5 -A5 | head -50

Repository: cppalliance/http

Length of output: 1615

🏁 Script executed:

# Search for test cases that test 405 or method_not_allowed behavior
rg -n 'TEST\|BOOST_AUTO_TEST\|method_not_allowed\|405' test/unit/server/router.cpp | head -30

Repository: cppalliance/http

Length of output: 42

🏁 Script executed:

# Look for tests combining route_next with method-specific routes
rg -n 'on.*GET\|on.*POST.*h_next\|\.get\(' test/unit/server/router.cpp -B2 -A2 | head -40

Repository: cppalliance/http

Length of output: 42

🏁 Script executed:

# Get an overview of router test structure
cat -n test/unit/server/router.cpp | head -150

Repository: cppalliance/http

Length of output: 5283

🏁 Script executed:

# Search for any tests involving method selection and route_next
rg -n 'get\(|post\(|put\(' test/unit/server/router.cpp | head -20

Repository: cppalliance/http

Length of output: 42

🏁 Script executed:

cat -n test/unit/server/router.cpp | sed -n '150,400p'

Repository: cppalliance/http

Length of output: 10382

🏁 Script executed:

# Look for test methods that test method-based routing
rg -n 'BOOST_FIXTURE_TEST_CASE|void|auto.*test' test/unit/server/router.cpp | grep -A2 'test' | head -30

Repository: cppalliance/http

Length of output: 604

🏁 Script executed:

cat -n test/unit/server/router.cpp | sed -n '394,470p'

Repository: cppalliance/http

Length of output: 2933

🏁 Script executed:

# Look for any 405-related test cases
rg -n 'status.*method\|method.*status\|405\|method_not_allowed' test/unit/server/router.cpp

Repository: cppalliance/http

Length of output: 42

🏁 Script executed:

rg -n '405\|method_not_allowed' test/unit/server/detail/router_base.cpp

Repository: cppalliance/http

Length of output: 42

🏁 Script executed:

# Check if there are tests that specifically test the scenario described in the bug
# (handler matches both path and method, returns route_next, then falls through)
cat -n test/unit/server/detail/router_base.cpp | head -100

Repository: cppalliance/http

Length of output: 3321

🏁 Script executed:

# Let me trace through the exact scenario: what is pv.verb_ and how is it set?
rg -n 'pv.verb_.*=' src/server/detail/router_base.cpp

Repository: cppalliance/http

Length of output: 190

🏁 Script executed:

# Examine the match_method function to understand how method matching works
rg -n 'match_method' src/server/detail/router_base.cpp -B5 -A10

Repository: cppalliance/http

Length of output: 509

🏁 Script executed:

# Let me understand the complete picture of route entry and what e.match_method does
rg -n 'match_method' src/server/ -B2 -A5 --type=cpp | head -50

Repository: cppalliance/http

Length of output: 967

🏁 Script executed:

# Find the router_entry structure to understand match_method
rg -n 'struct.*router_entry\|bool match_method' src/server/ --type=cpp -A5 | head -40

Repository: cppalliance/http

Length of output: 42

🏁 Script executed:

cat -n src/server/detail/router_base.hpp | sed -n '68,100p'

Repository: cppalliance/http

Length of output: 1066

🏁 Script executed:

# Get the complete router_entry structure
rg -n 'struct router_entry' src/server/detail/router_base.hpp -A20

Repository: cppalliance/http

Length of output: 42

🏁 Script executed:

# Now let me verify the complete picture: after handler returns route_next, is there any way
# the current request's method could be saved or flagged?
rg -n 'route_what::next' src/server/detail/router_base.cpp -B10 -A10

Repository: cppalliance/http

Length of output: 807

🏁 Script executed:

# Check if there's any tracking of whether current method was matched
rg -n 'current.*method\|method.*matched' src/server/detail/router_base.cpp -i

Repository: cppalliance/http

Length of output: 42

🏁 Script executed:

# Now let me verify the logic of the proposed fix by understanding pv.verb_ and matched_methods structure
cat -n src/server/detail/router_base.cpp | sed -n '206,220p'

Repository: cppalliance/http

Length of output: 625

🏁 Script executed:

# Understand the bit manipulation: how are methods stored in matched_methods?
rg -n '1ULL << static_cast' src/server/ --type=cpp -B2 -A2

Repository: cppalliance/http

Length of output: 1237

🏁 Script executed:

# Verify the complete scenario by creating a simple trace
cat > /tmp/verify_405.txt << 'EOF'
Scenario: GET handler at "/" returns route_next

1. Request: GET /
2. Path matching succeeds
3. matched_methods gets 0 (initially)
4. Loop iteration 1: find handler for GET /
   - Path matcher matches (end_ = true)
   - Line 294: matched_methods |= m.allowed_methods_ (now has GET bit set)
   - Line 299-304: e.match_method() returns true (GET == GET)
   - Line 319: Handler invoked, returns route_next
   - Line 330-333: Loop continues (++i)
5. No more handlers match
6. Loop exits
7. Line 376-384: 405 check fires
   - matched_methods != 0 (true - has GET bit)
   - is_options is false (true)
   - Condition is true, sends 405 Method Not Allowed
   - BUT: the request method WAS in matched_methods!

The bug: The 405 check never verifies that the CURRENT REQUEST'S METHOD
is absent from matched_methods. It only checks that SOME method was matched.
EOF
cat /tmp/verify_405.txt

Repository: cppalliance/http

Length of output: 910

🏁 Script executed:

# Verify the proposed fix checks the correct logic
cat > /tmp/verify_fix.txt << 'EOF'
Proposed fix logic:
1. method_is_matched = check if pv.verb_ is in matched_methods
2. For custom verbs: check if pv.verb_str_ is in matched_custom_verbs
3. Handle "all methods" case: matched_methods == ~0ULL
4. Only fire 405 if:
   - NOT is_options AND
   - NOT method_is_matched AND  <-- NEW: ensure current method isn't matched
   - (matched_methods != 0 OR !matched_custom_verbs.empty())

This is correct!
EOF
cat /tmp/verify_fix.txt

Repository: cppalliance/http

Length of output: 468

Bug: 405 fires incorrectly even when the request method IS in the matched set.

The 405 fallback at lines 376-384 checks only that matched_methods != 0 || !matched_custom_verbs.empty() but never verifies that the current request's method is absent from these sets. Consider a scenario where a GET handler for "/" returns route_next: the path matches, matched_methods accumulates the GET bit (line 294), the handler passes the method check (line 299), returns route_next (line 330), and the loop continues. After all entries are processed, the 405 check fires even though GET was in matched_methods — the response incorrectly sends 405 Method Not Allowed instead of falling through to route_next.

The condition must additionally verify that the current request's method is not in the matched set before returning 405.

Proposed fix 
     // 405 fallback: path matched but method didn't
-    if(!is_options &&
-        (matched_methods != 0 || !matched_custom_verbs.empty()))
+    bool method_is_matched = false;
+    if(pv.verb_ != http::method::unknown)
+        method_is_matched = (matched_methods & (1ULL << static_cast<unsigned>(pv.verb_))) != 0;
+    else
+    {
+        for(auto const& v : matched_custom_verbs)
+        {
+            if(v == pv.verb_str_)
+            {
+                method_is_matched = true;
+                break;
+            }
+        }
+    }
+    // Also treat "all methods" as matching
+    if(matched_methods == ~0ULL)
+        method_is_matched = true;
+
+    if(!is_options && !method_is_matched &&
+        (matched_methods != 0 || !matched_custom_verbs.empty()))
     {
🤖 Prompt for AI Agents 
In `@src/server/detail/router_base.cpp` around lines 375 - 384, The 405 fallback
fires even when the incoming request's method is present in matched_methods or
matched_custom_verbs; change the condition in the block that builds the Allow
header to also verify the current request method is NOT present before returning
405. Concretely, compute the method bit for the current request (e.g. method_bit
= method_to_bit(p.req.method) or similar) and only trigger the 405 when
!is_options && (matched_methods != 0 || !matched_custom_verbs.empty()) &&
((matched_methods & method_bit) == 0) && (matched_custom_verbs does not contain
the request method string); then call build_allow_header(matched_methods,
matched_custom_verbs), p.res.set_status(status::method_not_allowed) and send as
before (return route_done).


co_return route_next;
}

Expand Down
4 changes: 2 additions & 2 deletions src/server/detail/any_router.hpp → src/server/detail/router_base.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
// Official repository: https://github.com/cppalliance/http
//

#ifndef BOOST_HTTP_SRC_SERVER_DETAIL_ANY_ROUTER_HPP
#define BOOST_HTTP_SRC_SERVER_DETAIL_ANY_ROUTER_HPP
#ifndef BOOST_HTTP_SRC_SERVER_DETAIL_ROUTER_BASE_HPP
#define BOOST_HTTP_SRC_SERVER_DETAIL_ROUTER_BASE_HPP

#include <boost/http/server/detail/router_base.hpp>
#include <boost/http/detail/except.hpp>
Expand Down
14 changes: 12 additions & 2 deletions src/server/http_worker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,20 @@ do_http_session()

{
auto rv = co_await fr.dispatch(rp.req.method(), rp.url, rp);

if(rv.what() == route_what::close)
break;

if(rv.what() == route_what::next)
{
rp.status(http::status::not_found);
(void)(co_await rp.send());
Copy link

@coderabbitai coderabbitai bot Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Discarded send errors may leave the connection in a bad state.

Both (void)(co_await rp.send()) calls discard the error code. If the send fails (e.g., broken pipe), the loop proceeds to check keep_alive() and may attempt another request/response cycle on a broken stream. Consider breaking out of the loop on send failure, similar to how route handlers use co_return route_error(ec).

Also applies to: 90-90

🤖 Prompt for AI Agents 
In `@src/server/http_worker.cpp` at line 84, The two discarded send results of
(co_await rp.send()) must be checked and handled: capture the await result into
an error variable when calling rp.send(), and if it indicates failure, break out
of the request loop or return early (e.g., co_return route_error(ec)) instead of
continuing to check keep_alive() and reuse the stream; apply this change to both
occurrences of rp.send() so failed sends do not allow another request/response
cycle on a broken connection.

}

if(rv.failed())
{
// VFALCO log rv.error()
break;
rp.status(http::status::internal_server_error);
(void)(co_await rp.send());
}

if(! rp.res.keep_alive())
Expand Down
Loading
Loading