Skip to content

Commit 8da6215

Browse files
committed
Merge TASK-021: http_resource allow-mask via method_set
Replaces std::map<std::string, bool> method_state with method_set bitmask member; is_allowed/get_allowed_methods are now const noexcept; string-based public method-allow API removed; internal callers migrated to http_method enum with single decode-at-boundary in webserver_impl::answer_to_connection. Validation: 8 reviewers, 2 iterations, 3 majors fixed (security count_ sentinel guard, hardcoded bitmask assertion → behavior-based, missing review record).
2 parents d2e137f + 6a0dcb1 commit 8da6215

14 files changed

Lines changed: 395 additions & 216 deletions

examples/allowing_disallowing_methods.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ int main() {
3434

3535
hello_world_resource hwr;
3636
hwr.disallow_all();
37-
hwr.set_allowing("GET", true);
37+
hwr.set_allowing(httpserver::http_method::get, true);
3838
ws.register_resource("/hello", &hwr);
3939
ws.start(true);
4040

examples/custom_error.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ int main() {
4444

4545
hello_world_resource hwr;
4646
hwr.disallow_all();
47-
hwr.set_allowing("GET", true);
47+
hwr.set_allowing(httpserver::http_method::get, true);
4848
ws.register_resource("/hello", &hwr);
4949
ws.start(true);
5050

specs/tasks/M4-handlers/TASK-021.md

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,12 @@
88
Replace `http_resource`'s `std::map<std::string, bool> method_state` with a `method_set` bitmask, shrink `sizeof(http_resource)`, and make `is_allowed`/`get_allowed_methods` const.
99

1010
**Action Items:**
11-
- [ ] Replace `std::map<std::string, bool> method_state` with `method_set methods_allowed_;` member.
12-
- [ ] `bool is_allowed(http_method m) const noexcept` returns `methods_allowed_.contains(m)`.
13-
- [ ] `method_set get_allowed_methods() const noexcept` returns `methods_allowed_` by value.
14-
- [ ] `void set_allowing(http_method m, bool allow) noexcept` (mutator stays non-const).
15-
- [ ] `void allow_all() noexcept;` `void disallow_all() noexcept;`
16-
- [ ] Convert internal v1 callers that passed method names as strings to use `http_method` enum values; provide a string→enum helper if existing user-facing setters need to keep their string form.
11+
- [x] Replace `std::map<std::string, bool> method_state` with `method_set methods_allowed_;` member.
12+
- [x] `bool is_allowed(http_method m) const noexcept` returns `methods_allowed_.contains(m)`.
13+
- [x] `method_set get_allowed_methods() const noexcept` returns `methods_allowed_` by value.
14+
- [x] `void set_allowing(http_method m, bool allow) noexcept` (mutator stays non-const).
15+
- [x] `void allow_all() noexcept;` `void disallow_all() noexcept;`
16+
- [x] Convert internal v1 callers that passed method names as strings to use `http_method` enum values. The string-based public API (`set_allowing(const std::string&, bool)` / `is_allowed(const std::string&)` / vector-returning `get_allowed_methods()`) was removed outright on this branch (Decision 1 in plan). The wire-string-to-enum decode happens once at `webserver_impl::answer_to_connection` and the result is recorded on `modded_request::method_enum`; no public string-to-enum helper is shipped.
1717

1818
**Dependencies:**
1919
- Blocked by: TASK-005
@@ -29,4 +29,4 @@ Replace `http_resource`'s `std::map<std::string, bool> method_state` with a `met
2929
**Related Requirements:** PRD-REQ-REQ-002, PRD-REQ-REQ-003
3030
**Related Decisions:** DR-006, §4.4
3131

32-
**Status:** Not Started
32+
**Status:** Done

specs/tasks/_index.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ Nominally: **13 sequential tasks**, each S–XL. Most other tasks parallelize of
103103
| TASK-018 | `http_request` single-key getters return `string_view`, all const | M3 | Done | TASK-015, TASK-016 |
104104
| TASK-019 | High-level GnuTLS accessors replacing `gnutls_session_t` | M3 | Done | TASK-015 |
105105
| TASK-020 | Final public-header backend-include sweep | M3 | Done | TASK-014, TASK-015, TASK-019 |
106-
| TASK-021 | `http_resource` allow-mask via `method_set` | M4 | Not Started | TASK-005 |
106+
| TASK-021 | `http_resource` allow-mask via `method_set` | M4 | Done | TASK-005 |
107107
| TASK-022 | Snake_case `render_*` overrides on `http_resource` | M4 | Not Started | TASK-021 |
108108
| TASK-023 | Smart-pointer `register_resource` overloads | M4 | Not Started | TASK-014 |
109109
| TASK-024 | `register_path` and `register_prefix` (replace `bool family`) | M4 | Not Started | TASK-023 |
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
# Unworked Review Issues
2+
3+
**Run:** 2026-05-07 23:16:42
4+
**Task:** TASK-021
5+
**Total:** 7 (0 critical, 3 major, 4 minor)
6+
7+
## Major
8+
9+
1. [x] **security-reviewer** | `src/httpserver/http_resource.hpp:148` | insecure-design
10+
`set_allowing(http_method::count_, true)` was accepted by the public API without a guard. `method_bit(count_)` = bit 9 = 0x200, outside `valid_method_mask` (0x1FF). If a user called this, `is_allowed(count_)` returned true, enabling a null-callback invocation path for unrecognized HTTP verbs — undefined behaviour / DoS vector. CWE-476, CWE-20.
11+
*Fixed in validation loop iter 1:* Added `if (method == http_method::count_) return;` guard at the top of `set_allowing`. Unit test `set_allowing_count_sentinel_has_no_effect` added.
12+
13+
2. [x] **housekeeper** | `specs/unworked_review_issues/` | documentation-stale
14+
No review record file existed for TASK-021 despite the task being marked Done in both TASK-021.md and _index.md.
15+
*Fixed in validation loop iter 1:* This file created.
16+
17+
3. [x] **code-simplifier** | `test/unit/http_resource_test.cpp:87` | code-structure
18+
`allow_some_methods` test asserted an exact bit-pattern using hardcoded magic numbers `(std::uint32_t{1} << 0) | (std::uint32_t{1} << 2)` with a comment re-stating enum ordinals. If a new method is inserted before `post`, this assertion fails with a misleading message.
19+
*Fixed in validation loop iter 1:* Replaced with `method_set{}.set(http_method::get).set(http_method::post).bits` — self-documenting and enum-ordinal-independent.
20+
21+
## Minor
22+
23+
4. [ ] **security-reviewer** | `src/webserver.cpp:1519` | insecure-design
24+
Allow-header serialization loop iterates from 0 to `count_ - 1` using a runtime cast back to `http_method`. If a developer inserts an enumerator and forgets to update `to_string()`, a silent empty token appears in the Allow header. A compile-time method array would make the divergence a compile error.
25+
*Recommendation:* Replace raw loop with `constexpr std::array<http_method, N> kAllMethods = {...}` and iterate over it.
26+
27+
5. [ ] **code-simplifier** | `src/httpserver/detail/modded_request.hpp:49` | naming
28+
Comment blocks on `method_enum` are tagged `// TASK-021:` — task-tracker references suitable for PR descriptions but not permanent source. Same pattern in `src/httpserver/detail/webserver_impl.hpp:99` and `src/webserver.cpp:303,369`.
29+
*Recommendation:* Strip `TASK-021:` prefix from all four comments, leaving only explanatory prose.
30+
31+
6. [ ] **code-simplifier** | `src/httpserver/http_resource.hpp:214` | code-structure
32+
`static_assert` references `PRD-REQ-REQ-002 / PRD-REQ-REQ-003` without a path or context. Readers cannot verify the requirement without external context.
33+
*Recommendation:* Either remove requirement references or add a brief inline note explaining the constraint.
34+
35+
7. [ ] **security-reviewer** | `src/webserver.cpp:1519` | insecure-design
36+
(Duplicate of item 4 above — same finding categorised as separate minor.)

0 commit comments

Comments
 (0)