RFC: Patchset Version Tracking#311
Draft
derekbarbosa wants to merge 8 commits into
Draft
Conversation
…ints Add validation for untrusted input on HTTP ingestion endpoints: - Explicit 2 MiB DefaultBodyLimit on the axum router - Download size cap (10 MiB) and decompression limit (50 MiB) on lore.kernel.org mbox fetches to mitigate gzip bombs - SHA format validation (40 or 64 char hex) in both forge parse_payload methods to reject malformed commit hashes - Repository URL validation with SSRF blocklist (cloud metadata, loopback, localhost) in forge parse_payload - PR/MR number must be positive in both forge providers - Message-ID path separator sanitization in Thread submit handler SHA and URL validation are applied only in forge parse_payload (webhook path), not in submit_patch (CLI path), because the CLI legitimately sends git refs (HEAD, branch names, short SHAs) and local filesystem paths that are resolved server-side via git rev-parse. Assisted-by: claude-opus-4.6 <noreply@anthropic.com> Signed-off-by: derekbarbosa <derekasobrab@gmail.com>
Implement cryptographic verification for incoming forge webhook requests, mitigating forged patchset creation, cost amplification via unauthorized AI review triggers, and SSRF via injected repo URLs. Three authentication modes are supported, selected by header presence: - Standard Webhooks HMAC-SHA256 (GitLab 19.0+ signing token): verifies webhook-signature header over msg_id.timestamp.body - GitHub HMAC-SHA256: verifies X-Hub-Signature-256 header - Legacy GitLab secret token: constant-time comparison of X-Gitlab-Token header All comparisons use constant-time operations via the subtle crate to prevent timing-based secret extraction. The access control flow is revised: when webhook_secret is configured, non-localhost requests are permitted because the signature check is the access control. The --enable-unsafe-all-submit flag is only needed for unauthenticated setups. Startup warnings are emitted for insecure configurations. Secret type auto-detection: whsec_ prefix indicates a Standard Webhooks signing token (base64-decoded); plain strings are used directly for both X-Gitlab-Token and HMAC key bytes. A warning is logged if whsec_ prefix is present but base64 decoding fails. The ForgeProvider trait signature changes to accept body and optional secret, which is a breaking change for downstream trait implementors but not for API consumers. New dependencies: hmac 0.13, base64 0.22, subtle 2. Assisted-by: claude-opus-4.6 <noreply@anthropic.com> Signed-off-by: derekbarbosa <derekasobrab@gmail.com>
Add docs/WEBHOOK_SECURITY.md — a comprehensive guide covering webhook authentication setup for all deployment topologies: - Deployment topology decision tree (public server, tunnel, LAN, script-based) - Authentication method comparison (HMAC signing token, HMAC secret, plain secret token) - Step-by-step setup for GitLab signing tokens, GitLab legacy tokens, and GitHub HMAC - Script-based/cronjob curl examples for both plain and HMAC auth - Production deployment checklist - Reverse proxy examples (nginx with TLS/rate-limiting, Caddy) - Network security (GitLab/GitHub IP ranges, SSRF protections) - Secret management (env vars, file permissions, rotation) - Troubleshooting (every error code) and FAQ Update existing docs to replace placeholder security warnings: - FORGE_SETUP.md: replace security sections with summary + link, update comparison table to mark signature validation as implemented - GITHUB_SETUP.md: replace 'not yet implemented' with setup steps - GITLAB_SETUP.md: replace 'not validated' with signing token setup - configuration.md: add [forge] section and webhook_secret env var Add four topology-specific example configs: - Settings.forge-gitlab-production.toml (signing token + localhost) - Settings.forge-gitlab-simple.toml (plain secret + cronjob) - Settings.forge-github.toml (HMAC secret) - Settings.forge-selfhosted.toml (signing token + LAN bind) Update Settings.toml with documented webhook_secret guidance. Assisted-by: claude-opus-4.6 <noreply@anthropic.com> Signed-off-by: derekbarbosa <derekasobrab@gmail.com>
Add designs/DESIGN_WEBHOOK_INPUT_HARDENING.md per repository convention for auditability. Covers threat model, HMAC signature verification architecture, access control revision, body size limits, input validation, startup warnings, configuration, deployment topologies, and known risks. Assisted-by: claude-opus-4.6 <noreply@anthropic.com> Signed-off-by: derekbarbosa <derekasobrab@gmail.com>
Member
|
Thanks, Derek! One high-level comment: let's drop for now the part which injects prior context into the review context, it's subtle and requires more testing and thinking. It's easy to bias LLMs and make them less good at spotting new issues. The rest overall looks good to me, thank you for working on it. I'll review it a bit more carefully soon. |
Collaborator
Author
|
Hi Roman,
if it helps, we've been seeing it work pretty well at RH. I can try and collect some data by logging into our prod instance. but if it ends up blocking this MR, i'm ok dropping it and hashing out specifics later :) |
Member
|
Yes, let's drop it for now so we can separate infra changes from the review quality changes. Thanks! |
Add version, previous_version_id, and commit_range columns to the patchsets table for tracking patch series version chains. The version number is persisted from parse_subject_version() during patchset creation, defaulting to 1 when no version tag is found. Schema changes: - version INTEGER DEFAULT 1 on patchsets table - previous_version_id INTEGER REFERENCES patchsets(id) - commit_range TEXT for forge-originated commit ranges - Partial index on version for efficient v2+ lookups - All migrations are idempotent via try_add_column PatchsetRow gains version, previous_version_id, and commit_range fields. All SELECT queries returning PatchsetRow are updated to include the new columns. create_fetching_patchset() gains optional version and commit_range parameters. The forge webhook caller passes commit_range; other callers pass None for both. Add sanitize_message_id() to src/patch.rs for stripping null bytes and control characters from message-IDs as defense in depth. This commit provides the data model foundation. Version chain linkage (previous_version_id population) comes in the next commit. Signed-off-by: derekbarbosa <derekasobrab@gmail.com>
Implement version chain linkage for both mailing list and forge webhook paths. Version linking uses signals (thread match, then author + subject match) with no arbitrary time window. If signals cannot confirm a match, the patchset proceeds independently. Mailing list path: - find_previous_version() searches by thread_id first, then by author + stripped subject (version tags removed via strip_subject_version()). No time window constraint. - create_patchset() populates previous_version_id for v2+ patchsets after creation. - Cross-version merge prevention: patchsets with different explicit versions are not merged even in the same thread. Implicit (None) versions still merge with explicit versions in the same thread to handle series where cover letters have version tags but individual patches do not. - Cross-version patch reassignment blocked in create_patch() to prevent patches from being moved between version-tracked patchsets. Forge webhook path: - find_patchset_by_mr_number() looks up the latest version of an existing MR/PR patchset. - When the same MR fires with a different commit_range, a new version is created with previous_version_id linking to the prior version. - When commit_range matches (metadata-only change like Draft-to-Ready), only title/URL metadata is updated without creating a new version. - update_patchset_metadata() for non-versioning metadata changes. New functions: - strip_subject_version() in src/patch.rs - find_previous_version() in src/db.rs - find_patchset_by_mr_number() in src/db.rs - update_patchset_metadata() in src/db.rs Signed-off-by: derekbarbosa <derekasobrab@gmail.com>
Add version and previous_version_id fields to all API JSON responses: - PatchsetRow serialization (list_patchsets) already includes the fields from PLAN-01's struct changes - get_patchset_details() and get_patchset_summary() manual JSON builders updated to include version and previous_version_id - Forge webhook response includes version number Add UI version badge for v2+ patchsets: - .tag-version CSS class with yellow background, light/dark mode - JavaScript renders badge before subsystem tags in the patchset list for versions > 1; v1 patchsets show no badge Add designs/DESIGN_VERSION_TRACKING.md per repository convention for auditability. Signed-off-by: derekbarbosa <derekasobrab@gmail.com>
Fix slug generation in forge_webhook version detection to use repo_url instead of pr_url. The pr_url contains the pull/merge request number as its last path segment (e.g., sashiko-dev/pull/42), which produced incorrect slugs like '42-42-v2' instead of 'repo-42-v2'. Add test coverage identified by adversarial review: - find_previous_version: returns None for v1, cross-thread subject matching via author+subject signal - get_previous_version_findings: returns empty when no prior version - format_previous_findings: basic formatting, empty input, token budget truncation Signed-off-by: derekbarbosa <derekasobrab@gmail.com>
Collaborator
Author
good point. I'll be more discerning of this going forward. |
derekbarbosa
force-pushed
the
feature/version-tracking
branch
from
5c56378 to
ed751db
Compare
Member
|
I'd keep them separate, but not a hard preference. Hardening overall looks good to me. I'll try to merge it soon. |
Collaborator
Author
|
No rush, I want to finish a few more tests. I'll take it out of draft sometime tomorrow. |
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
2 participants
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.
Track version chains for patch series from both mailing lists (NNTP/lore) and forge webhooks (GitHub/GitLab).
When a maintainer posts [PATCH v2 0/5] or pushes new commits to an MR, Sashiko recognizes it as a new version, links it to the prior version, and injects prior review findings into the new review prompt so the AI reviewer can check whether previous issues were addressed.
Schema
Three new columns on the patchsets table: version INTEGER DEFAULT 1, previous_version_id INTEGER REFERENCES patchsets(id), and commit_range TEXT. All migrations are idempotent via try_add_column. Existing patchsets default to version 1 with no chain linkage. A partial index on version > 1 enables efficient lookups.
Version detection
The existing parse_subject_version() function (handles [PATCH v2], [PATCH V3 1/2], [PATCHv5], [PATCH RFC v2 8/8], multi-digit versions) is used to extract version numbers. The parsed version is persisted on patchset creation.
Signal-based chain linkage
find_previous_version() links v2 to v1 using signals, not time windows.
Priority:
(1) same thread_id with version = current - 1
(2) same author + stripped subject match. strip_subject_version() removes version tags from subjects for cross-version comparison.
If no signal confirms a match, the patchset stands alone -- no false positives.
Cross-version merge prevention
Patchsets with different explicit versions are never merged, even in the same thread. Implicit (None) versions still merge with explicit versions in the same thread, preserving the existing behavior where cover letters have [PATCH v6 00/33] but individual patches have [PATCH 01/33]. Patch reassignment between version-tracked patchsets is blocked in create_patch().
Forge versioning
find_patchset_by_mr_number() looks up the latest version for an MR/PR number. When the same MR fires with a different commit_range, a new patchset is created with the version incremented and previous_version_id linking to the prior version. When commit_range matches (metadata-only changes like Draft-to-Ready), only the title and URL are updated without creating a new version.
Prior review context injection
get_previous_version_findings() follows the previous_version_id chain one hop and returns non-preexisting findings sorted by severity. format_previous_findings() produces a structured context string capped at an approximate token limit (2000 tokens, severity-prioritized truncation). The context flows through the reviewer, worker/prompts, and review binary pipeline via the previous_context field on ReviewInput.
API and UI
All API responses include version and previous_version_id fields. The web UI displays a yellow version badge ("v2", "v3") next to patchset names for versions > 1 with light/dark mode support.
Input validation
sanitize_message_id() strips null bytes and control characters from message-IDs as defense in depth.
Depends on: #310 (webhook-input-hardening) -- this adds version chain logic to forge_webhook that trusts webhook payload data (MR numbers, commit ranges) for version decisions. For git-forges, input-hardening's HMAC signature verification and input validation must be in place first to ensure that payload data is authenticated and well-formed before it is used to create version chains or inject prior review findings into AI prompts. This also builds on #310 's revised ForgeProvider trait signature and access control flow.