Repository navigation
Security hardening pass - #14
Merged
Merged
Conversation
Findings and rationale from the security review (plan: analyze-this- project-from-jolly-fountain): - Supply chain (High): the vcpkg checkout was cloned at master HEAD and auto-`git pull`ed on every configure, executing unpinned upstream code at configure time. The local .vcpkg clone is now pinned (detached HEAD) to VCPKG_PINNED_COMMIT — deliberately the same commit as vcpkg.json's builtin-baseline, so tool, scripts, and port baseline come from one audited revision. A user-set VCPKG_ROOT checkout is treated as user-managed and gets no git operations. - Worker-process DoS (High): the adapters allocate per request, so std::bad_alloc could unwind through OnBeginRequest into the IIS pipeline and crash w3wp.exe. OnBeginRequest now has a try/catch(...) boundary returning RQ_NOTIFICATION_CONTINUE (fail-open is a documented, deliberate tradeoff for this demo rule). - CI supply chain (Medium): all actions pinned by commit SHA (checkout v4, cache v4, get-cmake v4.4.2), cmakeVersion pinned to 4.4.2, and a Dependabot config added so the pins receive update PRs. - Binary hardening (Medium): /guard:cf (compile+link, cl and clang-cl) and /sdl (MSVC only) applied globally so every TU linked into the network-facing DLL is instrumented; verified CF-instrumented via dumpbin /loadconfig. - Hygiene (Low): .gitgnore renamed to .gitignore so the ignore rules actually apply, and the machine-specific .vscode/ untracked accordingly; register_module.cpp no longer leaks the factory when SetRequestNotifications fails; ModuleHandler's suffix match carries a SECURITY NOTE marking it demo-only (not an ACL primitive). CLAUDE.md documents the vcpkg bump procedure (baseline + pin move together), the no-throw boundary contract, and the SHA-pinning convention for new actions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Remediation of the findings from a security review of the project (module code, build/supply chain, CI, repo hygiene), ordered by severity.
High
.vcpkgclone was at master HEAD and auto-git pulled on every configure — executing unpinned upstream code on each dev machine and CI runner. It is now checked out (detached HEAD) atVCPKG_PINNED_COMMIT, deliberately the same commit asvcpkg.json'sbuiltin-baseline, so the tool, its scripts, and the port baseline all come from one audited revision. A user-setVCPKG_ROOTcheckout is treated as user-managed: no git operations at all. Bump procedure (move baseline + pin together) is documented in CLAUDE.md.OnBeginRequest(worker-process DoS). The adapters allocate per request, sostd::bad_alloccould unwind through the notification into the IIS pipeline and crash w3wp.exe — taking the whole app pool down. The body is now wrapped intry/catch(...)returningRQ_NOTIFICATION_CONTINUE; the comment documents the IIS no-throw contract and the deliberate fail-open tradeoff for this demo rule.Medium
actions/checkoutv4,actions/cachev4,lukka/get-cmakev4.4.2 — previously@latest, a floating tag);cmakeVersionpinned to exact4.4.2; new.github/dependabot.yml(github-actions ecosystem, weekly) keeps the pins from rotting./guard:cf(compile+link; cl and clang-cl) and/sdl(MSVC only) applied globally so every TU linked into the network-facing DLL is instrumented./GS, ASLR, DEP were already MSVC defaults.Low / hygiene
.gitgnore→.gitignore: the ignore rules were inert due to the typo (_build/was committable and nearly got committed once); the machine-specific.vscode/is untracked accordingly (kept locally).register_module.cppno longer leaks the factory whenSetRequestNotificationsfails.ModuleHandler's suffix match now carries a SECURITY NOTE marking it demo-only — suffix matching doesn't block a subtree (/forbidden/xpasses) andstd::towloweris locale-dependent, so it must not become the template for a real ACL. Semantics change deliberately left out (product decision).Verification
_build/+.vcpkg/wiped): first configure clones vcpkg and checks outd92484ed…; second configure performs zero git operations and holds the pinVCPKG_ROOTset (user-managed checkout): configure leaves it untoucheddumpbin /loadconfigon the Release DLL: CF instrumented with populated guard function tabledumpbin -exports: still exactly one export,RegisterModulegit statusno longer lists_build/🤖 Generated with Claude Code