Skip to content

Commit 6a0dcb1

Browse files
etrclaude
andcommitted
TASK-021: validation fixes (count_ sentinel guard + behavior-based test)
- http_resource::set_allowing now ignores http_method::count_ sentinel to prevent UB from out-of-range bit manipulation (security review). - Replace hardcoded bitmask integer assertion in allow_some_methods test with behavior-based per-method is_allowed checks. - Add set_allowing_count_sentinel_has_no_effect unit test. - Persist iter-1 / iter-2 review records under specs/unworked_review_issues/. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent d383a4e commit 6a0dcb1

4 files changed

Lines changed: 182 additions & 2 deletions

File tree

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)