018 audit bug security fixes

.github/copilot-instructions.md

@@ -1,6 +1,6 @@
 # blockchain Development Guidelines
 
-Auto-generated from all feature plans. Last updated: 2026-04-12
+Auto-generated from all feature plans. Last updated: 2026-04-13
 
 ## Active Technologies
 - C++20 (`-std=c++20`) + Boost (Asio, Serialization), OpenSSL (SHA-256 via EVP), nlohmann/json (vendored `src/json.hpp`) (002-consensus-mechanism)
@@ -49,9 +49,9 @@ C++20 (`-std=c++20`): Follow standard conventions
 - Do not include task numbers (e.g. T001, T010) in code comments. Comments should describe *what* or *why*, not reference planning artifacts.
 
 ## Recent Changes
+- 018-audit-bug-security-fixes: Added C++20 (`-std=c++20`) + Boost (Asio, Serialization), OpenSSL (EVP SHA-256), nlohmann/json (vendored `src/json.hpp`)
 - 017-blockchain-module-split: Added C++20 (`-std=c++20`) + Boost (Asio, Serialization), OpenSSL (EVP SHA-256), nlohmann/json (vendored `src/json.hpp`)
 - 016-audit-remediation: Added C++20 (`-std=c++20`) + Boost (Asio, Serialization), OpenSSL (EVP SHA-256), nlohmann/json (vendored `src/json.hpp`)
-- 015-compile-time-optimization: Added C++20 (`-std=c++20`) + Boost (Asio, Serialization, Program Options), OpenSSL, nlohmann/json (vendored), Catch2 (test)
 
 
 <!-- MANUAL ADDITIONS START -->

.specify/feature.json

@@ -1,3 +1,3 @@
 {
-  "feature_directory": "specs/017-blockchain-module-split"
+  "feature_directory": "specs/018-audit-bug-security-fixes"
 }

docs/ROADMAP.md

@@ -1,6 +1,6 @@
 # Roadmap
 
-Last updated: 2026-04-12
+Last updated: 2026-04-13
 
 ## Completed
 
@@ -23,6 +23,7 @@ Last updated: 2026-04-12
 | 015  | Compile-Time Optimization | Shared static archive (`libblockchain_core.a`) eliminates redundant compilation of 11 core source files across 14 build targets, reducing compilation units from ~177 to ~34 (~81% reduction), clean build time from ~16 min to ~2.5 min |
 | 016  | Code Audit Remediation | Fixed 5 bugs (block count, dirty flag, merkle root, IPv6 parsing, pending pool), cached difficulty per boundary with ChunkRetainGuard, extracted shared utilities (chunkFilename, parsePeerKey, RPC helpers, send_to_peers), added TestHelpers.hpp, single-threaded io_context enforcement |
 | 017  | Blockchain Module Split | Split monolithic Blockchain.cpp (1,019 lines) into four focused modules: ChainPersistence (379 lines), DifficultyEngine (95 lines), MerkleProofService (60 lines), and slimmed Blockchain core (624 lines); composition-based ownership; zero API changes; 3 new focused test suites |
+| 018  | Audit Bug & Security Fixes | Fixed 4 bugs (sync response block append, RPC getBlockByIndex bounds check, getBlockByIndex resize chunk IDs, recovery triple-load) and 2 security issues (port range validation in parsePeerKey, seed node input validation in main); single-pass recovery optimization |
 
 ## Suggested Specs
 

specs/018-audit-bug-security-fixes/checklists/requirements.md

@@ -0,0 +1,36 @@
+# Specification Quality Checklist: Audit Bug & Security Fixes
+
+**Purpose**: Validate specification completeness and quality before proceeding to planning
+**Created**: 2026-04-13
+**Feature**: [spec.md](../spec.md)
+
+## Content Quality
+
+- [x] No implementation details (languages, frameworks, APIs)
+- [x] Focused on user value and business needs
+- [x] Written for non-technical stakeholders
+- [x] All mandatory sections completed
+
+## Requirement Completeness
+
+- [x] No [NEEDS CLARIFICATION] markers remain
+- [x] Requirements are testable and unambiguous
+- [x] Success criteria are measurable
+- [x] Success criteria are technology-agnostic (no implementation details)
+- [x] All acceptance scenarios are defined
+- [x] Edge cases are identified
+- [x] Scope is clearly bounded
+- [x] Dependencies and assumptions identified
+
+## Feature Readiness
+
+- [x] All functional requirements have clear acceptance criteria
+- [x] User scenarios cover primary flows
+- [x] Feature meets measurable outcomes defined in Success Criteria
+- [x] No implementation details leak into specification
+
+## Notes
+
+- All items pass. Spec is ready for `/speckit.clarify` or `/speckit.plan`.
+- The spec covers all 4 bugs (§2.1–2.4) and 2 security issues (§3.1–3.2) from the audit report.
+- No [NEEDS CLARIFICATION] markers were needed — the audit report provides precise descriptions of each issue including file locations and expected fixes.

specs/018-audit-bug-security-fixes/data-model.md

@@ -0,0 +1,76 @@
+# Data Model: 018-audit-bug-security-fixes
+
+**Date**: 2026-04-13  
+**Spec**: [spec.md](spec.md)
+
+## Summary
+
+This feature modifies no data models, schemas, or persistent formats. All
+changes are behavioral fixes to existing code paths. The entities below
+document existing structures referenced by the fixes.
+
+---
+
+## Existing Entities (no changes)
+
+### Block
+
+The fundamental chain unit. No field changes.
+
+| Field | Type | Description |
+|-------|------|-------------|
+| index | `size_t` | Block position in the chain |
+| timestamp | `time_t` | Creation timestamp |
+| prevHash | `std::string` | Hash of the previous block |
+| hash | `std::string` | SHA-256 hash of this block |
+| merkleRoot | `std::string` | Merkle root of entries |
+| entries | `std::vector<StreamEntry>` | Key-value stream data |
+| nonce | `uint64_t` | Proof-of-work nonce |
+| difficulty | `uint32_t` | Mining difficulty target |
+
+### SyncResponse (wire format — unchanged)
+
+| Field | Type | Description |
+|-------|------|-------------|
+| total_chain_height | `uint64_t` | Sender's total block count |
+| chunk_index | `uint64_t` | Which chunk this batch represents |
+| blocks | `std::vector<Block>` | Blocks in this batch |
+
+### PeerAddress
+
+| Field | Type | Description |
+|-------|------|-------------|
+| host | `std::string` | Hostname or IP address |
+| port | `uint16_t` | Port number, valid range [1, 65535] |
+
+---
+
+## Interface Changes
+
+### ChainPersistence::validateChunk()
+
+**Before**: `bool validateChunk(size_t chunkIndex, const ConsensusConfig& config)`
+**After**: `std::optional<ChunkHandler> validateChunk(size_t chunkIndex, const ConsensusConfig& config)`
+
+Returns the loaded and validated chunk on success, `std::nullopt` on failure.
+Only caller is `recoverChain()`.
+
+---
+
+## State Transitions
+
+No new state machines. The sync handler's block-append loop transitions from
+"skip known blocks" to "append new blocks via `bc.appendBlock()`" within the
+existing sync state machine.
+
+---
+
+## Validation Rules (new/changed)
+
+| Rule | Location | Description |
+|------|----------|-------------|
+| Port range [1, 65535] | `parsePeerKey()` | Reject ports outside valid range |
+| Port range [1, 65535] | `main.cpp` seed parsing | Reject invalid CLI seed ports |
+| Block index < chain length | `RpcServer` getBlockByIndex | Reject out-of-range RPC queries |
+| Overlap hash match | `handle_sync_response()` | Abort batch if overlap block hashes mismatch |
+| Empty batch → end-of-sync | `handle_sync_response()` | Stop requesting if batch is empty |

specs/018-audit-bug-security-fixes/plan.md

@@ -0,0 +1,89 @@
+# Implementation Plan: Audit Bug & Security Fixes
+
+**Branch**: `018-audit-bug-security-fixes` | **Date**: 2026-04-13 | **Spec**: [spec.md](spec.md)
+**Input**: Feature specification from `/specs/018-audit-bug-security-fixes/spec.md`
+
+## Summary
+
+Fix 4 bugs and 2 security vulnerabilities identified in audit sections 2 and 3.
+The sync handler must actually append received blocks (§2.1), chain recovery
+must load each chunk only once (§2.2), the `getBlockByIndex` resize must use
+correct chunk IDs (§2.3), `parsePeerKey()` must validate port range (§2.4),
+seed node CLI parsing must handle invalid input gracefully (§3.1), and the
+`getBlockByIndex` RPC handler must bounds-check the requested index (§3.2).
+
+## Technical Context
+
+**Language/Version**: C++20 (`-std=c++20`)
+**Primary Dependencies**: Boost (Asio, Serialization), OpenSSL (EVP SHA-256), nlohmann/json (vendored `src/json.hpp`)
+**Storage**: Boost.Serialization binary chunk files (`chunk_NNNNNN.dat`)
+**Testing**: Catch2
+**Target Platform**: Linux, macOS, Windows
+**Project Type**: CLI / library (blockchain node)
+**Performance Goals**: Single-pass chain recovery (N chunks → N loads, not 3N)
+**Constraints**: Must not change the existing P2P wire format (`SyncResponse`)
+**Scale/Scope**: 7 source files modified, 10 targeted test additions
+
+## Constitution Check
+
+*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.*
+
+| # | Principle | Status | Notes |
+|---|-----------|--------|-------|
+| I | C++20 | PASS | All changes are standard C++20 |
+| II | Build System (Autotools, `make -j8`) | PASS | Minor Makefile.am update to add `utils_tests.cpp` to `blockchain_tests_SOURCES` |
+| III | Full Test Coverage (Catch2, individual binaries) | PASS | Each fix gets targeted tests |
+| IV | Code Style | PASS | Follows existing conventions |
+| V | Minimal Dependencies | PASS | No new dependencies |
+| VI | Mandatory TLS | PASS | No TLS changes |
+| VII | Cross-Platform | PASS | No platform-specific code |
+| VIII | Feature Branches | PASS | On branch `018-audit-bug-security-fixes` |
+| IX | Pre-1.0 API Stability | PASS | No public API changes |
+| X | Low-Latency Performance | PASS | Recovery optimization improves startup |
+| XI | MIT License | PASS | No third-party code |
+| XII | .gitignore | PASS | No new binaries (tests added to existing binaries) |
+| XIII | Roadmap Currency | REQUIRED | Must update ROADMAP.md on completion |
+
+**Gate result**: PASS — no violations.
+
+## Project Structure
+
+### Documentation (this feature)
+
+```text
+specs/018-audit-bug-security-fixes/
+├── plan.md              # This file
+├── research.md          # Phase 0 output
+├── data-model.md        # Phase 1 output
+├── quickstart.md        # Phase 1 output
+└── tasks.md             # Phase 2 output (created by /speckit.tasks)
+```
+
+### Source Code (files modified)
+
+```text
+src/
+├── network/
+│   ├── PeerClient.cpp       # §2.1 — fix handle_sync_response() to append blocks
+│   └── RpcServer.cpp        # §3.2 — bounds check getBlockByIndex RPC
+├── ChainPersistence.hpp     # §2.2 — change validateChunk() return type
+├── ChainPersistence.cpp     # §2.2 — single-pass recovery
+├── Blockchain.cpp           # §2.3 — fix resize chunk ID
+├── utils.cpp                # §2.4 — port range validation in parsePeerKey()
+└── main.cpp                 # §3.1 — safe seed node port parsing
+
+tests/
+├── sync_tests.cpp             # Tests for §2.1 (sync appends)
+├── chunk_recovery_tests.cpp   # Test for §2.2 (single-pass recovery)
+├── rpc_integration_tests.cpp  # Test for §3.2 (getBlockByIndex bounds)
+├── block_tests.cpp            # Test for §2.3 (resize IDs)
+├── cli_tests.cpp              # Test for §3.1 (seed node CLI parsing)
+└── utils_tests.cpp (NEW)      # Tests for §2.4 (parsePeerKey port range)
+```
+
+**Structure Decision**: One new test file (`utils_tests.cpp`) added to the
+existing `blockchain_tests` binary via `tests/Makefile.am`. No new test binaries.
+
+## Complexity Tracking
+
+No constitution violations — section not applicable.

specs/018-audit-bug-security-fixes/quickstart.md

@@ -0,0 +1,62 @@
+# Quickstart: 018-audit-bug-security-fixes
+
+**Date**: 2026-04-13
+
+## Build
+
+```bash
+cd /home/tim/projects/metaverse-systems/blockchain
+make -j8
+```
+
+## Run Tests
+
+```bash
+./tests/blockchain_tests
+./tests/sync_tests
+./tests/utils_tests
+./tests/rpc_integration_tests
+```
+
+## Verify Individual Fixes
+
+### §2.1 — Sync appends blocks
+
+Run the sync tests and verify the "handle_sync_response appends new blocks"
+test passes:
+
+```bash
+./tests/sync_tests -c "handle_sync_response appends"
+```
+
+### §2.4 — parsePeerKey port validation
+
+```bash
+./tests/utils_tests -c "parsePeerKey"
+```
+
+### §3.1 — Seed node port parsing
+
+Start the node with an invalid seed node and verify it exits gracefully:
+
+```bash
+./src/blockchain --data-dir /tmp/test-node --seed-node "host:abc"
+# Expected: error message, non-zero exit code
+
+./src/blockchain --data-dir /tmp/test-node --seed-node "host:99999"
+# Expected: error message, non-zero exit code
+```
+
+### §3.2 — RPC getBlockByIndex bounds check
+
+Start a node and send an out-of-range RPC request:
+
+```bash
+# In one terminal:
+./src/blockchain --data-dir /tmp/test-node
+
+# In another terminal:
+echo '{"jsonrpc":"2.0","id":1,"method":"getBlockByIndex","params":{"index":999999}}' | \
+  openssl s_client -connect localhost:8332 -quiet 2>/dev/null
+# Expected: JSON-RPC error with code -32001
+```