Repository navigation
fix(branch-protection): add security checks to required status checks - #1232
Merged
Merged
Conversation
get_required_status_checks() built the required-context list sent to GitHub branch protection but never read the security-checks config, so security-suspicious-paths and security-committer-identity were never marked required. The server-side gate in all_required_status_checks() already honoured them, so merge was only blocked transitively via can-be-merged and the GitHub UI showed both checks as optional. security-checks is read through the repository-scoped Config rather than the raw repositories entry, so repo-local .github-webhook-server.yaml overrides win. A repo setting security-checks.mandatory: false locally would otherwise still have its checks marked required. The appends land before the exclude-runs removal loop, so protected-branches exclude-runs can still drop them. Closes #1231 Assisted-by: PI (gpt-6-luna) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
Collaborator
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
PR Summary by QodoRequire built-in security checks in GitHub branch protection
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
AGENTS.md documented `uv run mypy webhook_server/` as a Definition of Done gate, but that invocation cannot pass. mypy is not a declared dependency in pyproject.toml while the types-* stubs are, and the command runs over webhook_server/tests/ which the prek mypy hook excludes. It reports 943 errors on an unmodified main, almost entirely import-not-found on missing github and pytest stubs, so the gate was unsatisfiable and never measured anything real. `prek run --all-files` runs every hook in .pre-commit-config.yaml — ruff, ruff-format, mypy, flake8, gitleaks, detect-secrets, eslint — in isolated envs with their own declared dependencies. It is green on the current tree, mypy included. The [tool.mypy] block in pyproject.toml is unchanged and prek\x27s hook still reads it, so strictness is preserved; only the way it is invoked is fixed. Assisted-by: PI (gpt-6-luna) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
|
Code review by qodo was updated up to the latest commit cc09d4d |
get_required_status_checks() built the required-context list sent to GitHub branch protection but never read the security-checks config, so security-suspicious-paths and security-committer-identity were never marked required. The server-side gate in all_required_status_checks() already honoured them, so merge was only blocked transitively via can-be-merged and the GitHub UI showed both checks as optional. security-checks is read through the repository-scoped Config rather than the raw repositories entry, so repo-local .github-webhook-server.yaml overrides win. A repo setting security-checks.mandatory: false locally would otherwise still have its checks marked required. The security check names are also applied when protected-branches sets include-runs, which previously short-circuited past them entirely. Closes #1231 Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
Eight documents had been reduced to a "Documentation generation failed" placeholder. Recreated them from the codebase: automation-recipes, environment-variables, log-viewer-and-mcp-api, run-pull-request-commands, set-up-checks-and-release-workflows, set-up-owners-and-reviewers, supported-github-events, webhook-and-health-api. Also repaired pre-existing corruption present on main since 9b6469b: - four files were missing the opening fence on their first code block, which shifted every later block and rendered pages as run-on text - the same four had no H1, so pages opened on a bare "config.yaml" line - configuration-reference.md was byte-duplicated; now 595 lines, one copy - docs cited port 500; the real default is 5000 The protected-branches.include-runs row now states that security checks governed by security-checks.mandatory are added on top of the explicit list, which is what the code now does. Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
docs/*.html, search-index.json, llms.txt and llms-full.txt are generated from docs/*.md, but the generator was external and the committed HTML had drifted badly out of sync with the markdown — eight pages rendered empty. Vendor the deterministic rendering half of myk-org/docsfy into scripts/docs_render/ so the site can be rebuilt from this repo alone, and add scripts/generate_docs.py as the entry point. Rendering is idempotent and never writes a markdown file. Local divergence from upstream: _indent_fenced_blocks indents fenced code bodies in llms-full.txt only, so that YAML comments such as "# config.yaml" are not mistaken for headings when an LLM reads the file as plain text. The HTML path is byte-identical to upstream. Adds markdown and jinja2 to the dev group; pygments and pyyaml were already present. Two supporting changes so the vendored package passes the existing gate: [tool.mypy] namespace_packages = false stops mypy mapping scripts/docs_render/renderer.py under both docs_render and scripts.docs_render, and types-Markdown joins the prek mypy hook's additional_dependencies alongside the other stub packages. AGENTS.md gains a Documentation section covering the markdown-only source of truth, the rebuild command, and the structural rules the renderer depends on. Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
Rebuilt with scripts/generate_docs.py. All fifteen pages now render from the corrected markdown; the eight previously empty pages have real content, and configuration-reference.html drops from 146KB to 83KB with the duplicated copy gone. Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
The process_webhook docstring claimed HTTPException 401 for a failed signature check, but app_utils.py raises 403 in both paths (missing X-Hub-Signature-256 header, and HMAC mismatch). The behaviour was correct; only the documented status was wrong. /automerge ignored the `remove` flag that every other label command honours, so `/automerge cancel` added the automerge label instead of removing it. It now calls _remove_label when cancel is given, matching the verified pattern. The maintainer/approver permission check still gates both add and remove. Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
scripts/docs_render/renderer.py enables markdown's codehilite extension, which imports pygments at render time. Pygments was never declared; it reached the environment transitively through ipython (dev group), pytest (tests group) and rich. A production or --no-dev install would fail to render the documentation site. Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
set_repository() passed exclude_status_checks only to get_required_status_checks(). When protected-branches set include-runs, that call was skipped entirely, so exclude-runs was silently discarded and the exclusions never reached GitHub. This affected every check, not just the security ones, and examples/config.yaml shipped exactly the pattern that does not work. exclude-runs is now subtracted from the assembled list on the include-runs path as well, after the security checks are added. The auto path already subtracted inside get_required_status_checks() and is unchanged, so the exclusion is applied exactly once per branch. An explicit exclude-runs entry is honoured even for the security checks; security-checks.mandatory remains the switch for not requiring them at all. Verified against the real set_repository() call: include [a,b] with exclude [b] previously yielded [a,b] and now yields [a] plus the security checks. Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
3 tasks
|
Code review by qodo was updated up to the latest commit c78d654 |
Three defects in the security-checks startup path, found by Qodo review. The warning branches read repo.name from a live PyGithub Repository. Those warnings only fire on malformed config, so any API access was unretried and outside the github_api_call wrapper the project requires. The helper now takes a plain repository identifier string. get_security_status_checks() called config.get_value() with no extra_dict. Config.repository_data is the config.yaml repository entry, not the repo-local .github-webhook-server.yaml — the local file is only read via repository_local_data(), which needs a GitHub API and previously ran solely at webhook time. Startup branch protection therefore never saw the local file, so a repo disabling a check there had it written into branch protection anyway while webhook processing treated it as advisory, leaving the PR mergeable in one place and blocked in the other. The local config is now fetched during settings synchronisation and passed as extra_dict, and invalid YAML is logged and ignored rather than aborting startup. suspicious-paths was tested for truthiness without normalising, so a schema-valid [""] added security-suspicious-paths to the required contexts. github_api.py strips blanks, so the runner saw an empty list and never reported the check, leaving the PR pending forever. Both places now normalise identically. Closes the findings from Qodo on #1232. Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
Collaborator
Author
|
/qodo The following review comments were reviewed and a decision was made:
|
|
Code review by qodo was updated up to the latest commit ad7eb05 |
These docs told maintainers things the code does not do. Each one misleads in a way that costs real time when followed literally. - `prek` and `tox` were documented as bare commands but are not declared dependencies, so the Definition of Done was unrunnable on a fresh checkout; both now go through `uvx` - the WEBHOOK_SERVER_DATA_DIR example created an empty directory and ran entrypoint.py, which raises FileNotFoundError without config.yaml - the security checks are active by default (mandatory and committer-identity-check both default true); the docs said opt-in, so maintainers would wrongly believe their checks were inactive - /logs/api/* routes are registered unconditionally and gated per request with a 404, not registration-gated; the "exactly two routes" claim contradicted the rest of the same file - an approver is only required when the OWNERS path configures at least one Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
Three renderer functions tracked fenced code blocks by looking only for backtick fences, but the Markdown converter also accepts ~~~ fences. A tilde block containing a list item or blockquote had a blank line inserted into the displayed code by _ensure_blank_lines(), and its contents escaped indentation in _indent_fenced_blocks(), which builds llms-full.txt — a YAML # comment inside a ~~~ block read as a heading. A shared _FENCE_RE and _fence_marker() helper now recognises both characters. A fence closes only on a run of the same character that is at least as long as the opening one, so ~~~ can no longer close a ``` block or the reverse. Backtick behaviour is unchanged. Verified: 1840 passed, coverage 90.35%, uvx tox both envs OK, prek clean. Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev> Signed-off-by: OPENAI <noreply@openai.com>
Regenerated HTML, search index and llms-full.txt after the source corrections in 2c33325. Verified idempotent: a second generator run produces no further diff, so these artifacts are in sync with docs/*.md and not carrying unrelated drift. Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
|
Code review by qodo was updated up to the latest commit 9891e16 |
The vendored docsfy sanitizer stripped dangerous HTML with regexes, and four separate bypasses walked through it: <base href>, a </script > with a space before the closing bracket, an unclosed <iframe srcdoc>, and md.toc being returned unsanitised while the template injected it with |safe. Patching the regexes was not converging, so the mechanism changed. A subclass of stdlib html.parser.HTMLParser now re-serialises the document against an explicit allowlist. Attributes are deny-by-default, which covers on* handlers plus style/srcdoc/formaction without an enumeration that could miss a variant. URLs are restricted to http, https, mailto, fragments and absolute paths. Tag handling is three-way, and getting this wrong is what made the first attempt unusable: - raw-text carriers (script, style, iframe, object, svg, ...) drop their contents as well as the tag - known dangerous void tags (base, embed, link, meta, input) drop silently — printing an escaped <meta http-equiv=refresh> into a published page as literal prose helps nobody - genuinely unknown tags are re-emitted as escaped text, so a placeholder like "No <name> configured" still reads as a placeholder The middle rule is the fix for a regression the first attempt caused: dropping the CONTENTS of every unknown tag deleted 155 lines of real documentation from run-pull-request-commands, because python-markdown passes raw HTML through and "<name>" in prose parses as an unrecognised start tag. The drop-contents rule is now limited to elements that actually hold code or markup. md.toc is sanitised like the body, closing the fourth bypass. Verified: 1897 passed, coverage 90.35%, uvx tox both envs OK, prek clean, and the regenerated page is confirmed to keep its table and its full prose. Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
Upstream docsfy excludes its generation-failure stub page from llms.txt and llms-full.txt so a failed generation is never published as if it were real documentation. That filter was lost when the renderer and generator were vendored, so a stub brought into docs/ would be presented to AI readers as documentation. Both index builders now filter through a single predicate that reuses the renderer's existing _FAILURE_STUB_RE rather than keeping a second copy of the pattern that could drift. Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
Two round 7 accuracy fixes. /test-oracle was documented as always running on draft PRs. It is not. GithubWebhook.process returns early for a draft whenever allow-commands-on-draft-prs is absent, so the webhook is dropped before any command is evaluated and the per-command exemption in user_commands is never reached. The exception only applies when the key is set, even to an empty list. The AGENTS.md preview command used bare python3, which contradicts the rule the same file states two sections earlier: Python runs through uv. A contributor following it needed a separate Python on PATH. Regenerated the site output for the changed page. Verified idempotent. One Qodo finding from this round was checked and REJECTED as a false positive rather than actioned: it claimed custom check commands do not run through a shell, but runner_handler.py wraps them in /bin/sh -c via shlex.quote() before run_command splits them, added deliberately in a05ed3b. The existing documentation was correct. Verified: 1897 passed, coverage 90.35%, uvx tox both envs OK, prek clean. Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
The log API guide claimed malformed JSON workflow summaries fail fast with 500 Malformed log entry. They do not, and there are three distinct behaviours rather than one: - A summary line that is not valid JSON never reaches the error path. get_raw_json_entry() catches JSONDecodeError and returns None, so a corrupt line is silently indistinguishable from a missing one. - A structurally invalid summary does raise 500 Malformed log entry in get_workflow_steps_json(), but get_workflow_steps() catches HTTPException without qualification, so that 500 is swallowed and the request falls through to text-log parsing. The client gets a reconstructed timeline, or a 404 if the text logs do not cover the delivery. - 500 Malformed log entry is only reachable through the step-logs endpoint, which calls get_workflow_steps_json() directly. The bare except HTTPException is what makes the documented fail-fast unreachable. It is doing double duty: a 404 "no JSON summary, try the legacy path" is reasonable backward compatibility, but absorbing a 500 "this entry is corrupt" into the same handler is a separate question this change deliberately does not answer. Only the documentation is corrected here. Verified: 1897 passed, coverage 90.35%, uvx tox both envs OK, prek clean. Assisted-by: PI (Space Bunny) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
|
Code review by qodo was updated up to the latest commit 0e9854f |
… report A malformed repository-local .github-webhook-server.yaml was logged and ignored, and startup fell back to the security-checks defaults. That made branch protection require security-suspicious-paths and security-committer-identity. The runtime path reads the same file with raise_on_error=True and propagates the error, so neither check is ever reported. The repository was left requiring two checks that could never pass, making every pull request permanently unmergeable with no visible cause. A parse failure is not "no config", it is UNKNOWN, and it is now treated as such: the error is logged at ERROR with the repository name and the underlying exception, and branch protection is left untouched rather than written from defaults nobody can trust. Startup still does not abort, and labels and other repository settings are still applied. When the config parses, behaviour is unchanged - mandatory and committer-identity-check both still default to true. Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
_is_safe_url() rejected protocol-relative URLs only when the value literally started with "//". Browsers normalise backslashes to forward slashes while parsing an authority, so \\evil.example, /\evil.example and /\/evil.example navigate off-site exactly like //evil.example, but none of them start with a literal "//" and all of them passed. Backslashes are now folded to forward slashes before the policy checks. The order is fixed and documented: HTML-entity unescape first (so //host is judged as the //host it is), then noise stripping, then the backslash fold, then the checks. Doing it in any other order lets a value change what it looks like after the check that should have rejected it. Relative links the docs depend on - quick-start.html, ../README.md and /assets/... - still pass. Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
The guide showed allow-commands-on-draft-prs in a repository-local .github-webhook-server.yaml. Both sites that read it - GithubWebhook.process in github_api.py and user_commands in issue_comment_handler.py - call config.get_value() without an extra_dict, and get_value() consults repository-local data only when extra_dict is passed. A repository-local value was silently ignored and draft PRs stayed blocked. The guide now shows the key in config.yaml, globally or under repositories.<repo>, and states plainly that a repository-local copy has no effect. Both schema locations were checked: root and repositories.<repo> are both reachable through get_value's repository_data/root_data fallback. Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev> Signed-off-by: Meni Yakove <myakove@gmail.com>
|
Code review by qodo was updated up to the latest commit ea4ac0f |
exclude-runs was removing security status contexts from the required
branch-protection list, in both get_required_status_checks() and the
include-runs path in set_repository(). The runtime never applies
exclude-runs to security contexts - the string does not appear in
github_api.py at all - so the two paths disagreed about the same
configuration:
- branch protection stopped requiring the check, so GitHub treated it
as optional
- the pull-request handler still queued it
- can-be-merged still required it
The required-check list now excludes only non-security checks, and the
security contexts are appended afterwards. security-checks.mandatory:
false remains the supported way to turn them off, since
get_security_status_checks() already returns an empty list there and
cannot re-add anything.
This also corrects an earlier decision recorded in the code as
deliberate ("exclude-runs wins over include-runs, security checks
included"). It was never verified against the runtime, and it made
branch protection report a weaker guarantee than the server actually
enforces.
Also switches the invalid-repo-local-YAML log to LOGGER.exception so
the traceback is recorded, per the project's logging convention, and
drops the now-unused exception binding.
Verified: 1936 passed, coverage 90.35%, uvx tox both envs OK, prek clean.
Assisted-by: PI (stealth/space-bunny-alpha) <noreply@pi.dev>
Signed-off-by: Meni Yakove <myakove@gmail.com>
|
Code review by qodo was updated up to the latest commit 4093c37 |
myakove
deleted the
fix/issue-1231-security-checks-branch-protection
branch
September 29, 2026 19:59
Collaborator
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
3 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.
Closes #1231
The original problem
security-suspicious-pathsandsecurity-committer-identityran on every PR but were never marked required in GitHub. The Checks tab showed both as optional, and a maintainer using the native merge box got no signal untilcan-be-mergedhad already flipped to failure.There are two independent notions of "required", and only one was wired up:
Server-side (worked).
all_required_status_checks()inwebhook_server/libs/handlers/check_run_handler.pyappends both names whensecurity_mandatoryis true, feedingrequired_check_failed_or_no_status()and blockingcan-be-merged. Thesecurity-checks.mandatoryconfig key already existed with atruedefault.GitHub branch protection (missing).
get_required_status_checks()inwebhook_server/utils/github_repository_settings.pybuilds the context list applied to protected branches at startup. It handledtox,verified-job,container,pypi,pre-commit,conventional-titleandpre-commit.ci - pr— but never readsecurity-checks, so both names were absent. Confirmed against the live repo before the fix:Merge was only blocked transitively via
can-be-merged.Note: branch protection is applied during server startup, not per webhook. This takes effect on the next restart that runs repository setup, not on merge.
What is in here
2b412f2,a903a1cinclude-runspaths (#1231)cc09d4dprek run --all-filesreplaces standaloneuv run mypyas the documented lint gatedee49fc945e4aascripts/;AGENTS.mddocuments the docs workflow (#1233)2fc2d5a177efda/automerge cancelnow removes the label; 401→403 docstring (#1234)b0e7f74pygmentsdeclared — the docs renderer imported it but relied on it transitively (#1233)c78d654exclude-runsis now applied to theinclude-runslistChanges worth reviewing carefully
exclude-runssemantics changed — this is a behaviour change, not just a bug fix.set_repository()passedexclude_status_checksonly toget_required_status_checks(). Whenprotected-branchessetinclude-runs, that call was skipped entirely, soexclude-runswas silently discarded and the exclusions never reached GitHub. This affected every check, not only the security ones, andexamples/config.yamlshipped exactly the pattern that does not work.exclude-runsis now subtracted from the assembled list on theinclude-runspath too. Verified against the realset_repository()call:include: [a, b]withexclude: [b]previously yielded[a, b]and now yields[a]plus the security checks. The auto path already subtracted insideget_required_status_checks()and is unchanged, so exclusion is applied exactly once per branch.An explicit
exclude-runsentry is honoured even for the security checks.security-checks.mandatoryremains the switch for not requiring them at all.security-checksis read through the repository-scopedConfig, not the rawrepositoriesentry, which required a keyword-onlyconfigparameter onget_required_status_checks(). Deliberate: reading from the raw entry would ignore a repo's local.github-webhook-server.yaml, so a repo settingsecurity-checks: {mandatory: false}there would still have its checks marked required. For a security gate, silently over-blocking is the wrong failure direction.pygmentswas never declared.scripts/docs_render/renderer.pyenables markdown'scodehiliteextension, which imports pygments at render time. It reached the environment only transitively viaipython(dev group),pytest(tests group) andrich. Now declared in the dev group. The docs site is built ahead of time and never rendered at build or runtime — theDockerfileandentrypoint.pydo not reference it — so the dev group is the correct home.Docs
docs/*.mdis the source of truth.*.html,search-index.json,llms.txtandllms-full.txtare generated byscripts/generate_docs.py, which vendors the deterministic rendering half of myk-org/docsfy so the site rebuilds from this repo alone. Rendering is idempotent and never writes a markdown file.One local divergence from upstream:
_indent_fenced_blocksindents fenced code bodies inllms-full.txtonly, so YAML comments like# config.yamlare not mistaken for headings when an LLM reads the file as plain text. The HTML path is byte-identical to upstream.AGENTS.mddocuments the workflow, including that navigation is not auto-discovered — a new page must be added to the list ingenerate_docs.pyor it will not appear in the sidebar or search.Verification
prek run --all-files— all 22 hooks passeduv run --group tests pytest -n auto— 1805 passed, coverage 90.31% (fail_under = 90), stable across repeated parallel and serial runsscripts/generate_docs.py— idempotent, byte-identical output on re-run/automerge canceltest was confirmed to fail against the pre-fix codeexclude-runsfix was verified by exercising the realset_repository()path, not by reading the codeStill open
exclude-runsis now honoured, butexamples/config.yamlshipped the broken pattern for a long time, so any repo copied from it may have believed it was excluding checks that were never excluded. Those exclusions need re-checking by hand — this PR cannot tell which repos were affected.