This repository was archived by the owner on May 4, 2026. It is now read-only.
TCK-00613: FAC gates architecture alignment + security hardening (throughput-profile model)#708
Merged
Merged
Conversation
added 4 commits
February 16, 2026 13:56
Create TCK-00613: migrate all CI from shell scripts to unified Rust gate engine under apm2 fac gates with profile-based gate selection (local-full, quick, workflow-preflight, drift-audit). Close merged tickets: TCK-00534, TCK-00544, TCK-00596, TCK-00597, TCK-00612. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> TCK-00613 close merged TCK-00532 and TCK-00549 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> TCK-00613: squash-prep aggregate branch + working tree
Collaborator
Author
# apm2-review-verdict:v1
schema: apm2.review.verdict.v1
pr: 708
sha: 04a10c79e9da2966ded1139911c5b8f8ab206ff8
updated_at: 2026-02-17T00:59:34Z
dimensions:
code-quality:
decision: deny
reason: 'FAIL: 2 blocker, 2 major findings. Significant architecture divergence (missing Gate Engine, missing Profiles) and security risks (DoS, Path Traversal).'
set_by: ubuntu
set_at: 2026-02-17T00:59:34Z
security:
decision: deny
reason: 'FAIL: 0 blocker, 2 major findings'
set_by: ubuntu
set_at: 2026-02-17T00:58:57Z
findings:
- finding_id: f-708-code_quality-1771289937612561-0
type: code-quality
severity: BLOCKER
summary: 'Missing requirement: S1 Gate Engine architecture (Trait/Registry/Runner)'
risk: Architecture divergence from approved design creates hardcoded maintenance burden and prevents profile-based extensibility.
impact: Violates TCK-00613 Scope S1. The PR implements ad-hoc matching in evidence.rs instead of the required gates_engine module with Gate trait and GateRegistry.
location: crates/apm2-cli/src/commands/fac_review/evidence.rs
body: The ticket explicitly requires a gates_engine/ module with a Gate trait, GateRegistry, and GateRunner. The PR implements gate_checks.rs with standalone functions (run_test_safety_guard) manually matched in evidence.rs. This fails the requirement for a unified gate registry and dynamic profile resolution.
evidence_digest: ccbb073937f827d45b94a540160b7e3b9dee5d03ac87fa55afba0132c949de9d
evidence_pointer: none
timestamp: 2026-02-17T00:58:57Z
- finding_id: f-708-code_quality-1771289942753069-0
type: code-quality
severity: BLOCKER
summary: 'Missing requirement: S2 --profile CLI argument for gate sets'
risk: Inconsistent user experience and failure to deliver requested CI profile capability.
impact: Violates TCK-00613 Scope S2. The PR retains --quick flag and repurposes gate_profile for throughput (concurrency) instead of gate sets (local-full, workflow-preflight).
location: crates/apm2-cli/src/commands/fac_review/gates.rs
body: The ticket requires apm2 fac gates --profile <local-full|quick|...> to control WHICH gates run. The PR implements GateThroughputProfile (Throughput/Balanced/Conservative) which controls parallelism, and retains the boolean --quick flag. The requested gate-set profile logic is missing.
evidence_digest: 6d97b8a4834becea5fb4cff53ee36b9f0778e171ae6723e06a976f8f29eeb5a3
evidence_pointer: none
timestamp: 2026-02-17T00:59:02Z
- finding_id: f-708-code_quality-1771289950328986-0
type: code-quality
severity: MAJOR
summary: Unbounded file read in gate_checks.rs and fac_preflight.rs
risk: Denial of Service (DoS) via OOM if large files are encountered during scanning.
impact: Violates CTR-1603 (Bounded Reads). Processing large files (e.g. accidentally committed binaries or huge logs) will crash the process.
location: crates/apm2-cli/src/commands/fac_review/gate_checks.rs
body: Functions run_test_safety_guard and run_review_artifact_lint call fs::read on all walked files without a size check. Similarly, fac_preflight.rs:scan_regex_matches uses fs::read_to_string on scan paths. Implement read_bounded or check file size before reading.
evidence_digest: cd2d8bae39004184983f03d262783c6c358dbb2a2573e6e631a9818b36925837
evidence_pointer: none
timestamp: 2026-02-17T00:59:10Z
- finding_id: f-708-code_quality-1771289964239148-0
type: code-quality
severity: MAJOR
summary: Arbitrary workspace execution root in fac_worker
risk: Security boundary violation; worker execution in unauthorized paths.
impact: Violates SP-RUNTIME-001 (Sandbox Isolation). fac_worker accepts workspace_root from the job payload and chdirs into it without validation.
location: crates/apm2-cli/src/commands/fac_worker.rs
body: 'resolve_workspace_root accepts any directory path. An attacker could submit a gates job with workspace_root: /etc or other sensitive paths, causing the worker to execute commands in that context. Validate that workspace_root is within APM2_HOME, the repo root, or a specific allowlisted lane directory.'
evidence_digest: 7f4044d1f12ab0eec9cf728d77129b0310f8da1b6faf58d58f1d9584510c27dc
evidence_pointer: none
timestamp: 2026-02-17T00:59:24Z
- finding_id: f-708-code_quality-1771289969529967-0
type: code-quality
severity: MINOR
summary: Missing validation for repository variable in fac_preflight.rs
risk: Potential URL injection or malformed API calls.
impact: Weakens robustness of GitHub API integration.
location: crates/apm2-cli/src/commands/fac_preflight.rs
body: The repository variable used in fetch_pr_json_via_gh is not explicitly validated to match owner/repo format before being used in the gh api URL. Ensure it matches a strict regex.
evidence_digest: bdbc1ae852b986cc90776830f563dcfa0fd99e6f061ae8cec8568054380d641b
evidence_pointer: none
timestamp: 2026-02-17T00:59:29Z
- finding_id: f-708-security-1771289883531374-0
type: security
severity: MAJOR
summary: workflow_dispatch permission can be overridden by env var
risk: Any actor can set APM2_PREFLIGHT_ACTOR_PERMISSION (e.g., workflow code, environment injection in a malicious PR workflow dispatch path, or compromised workflow step) and bypass GitHub permission lookup. The override is trusted before GH API verification and directly drives the dispatch_actor authorization check, creating a privilege bypass path from untrusted workflow input to trusted workflow_dispatch execution.
impact: Attackers can make the pipeline treat a non-admin actor as admin/maintain/write for workflow_dispatch events, potentially skipping intended workflow trust controls and enabling unauthorized high-trust FAC actions.
location: crates/apm2-cli/src/commands/fac_preflight.rs:843
body: Remove env-based permission override from production authorization path. If an override is needed for tests, gate it to test-only builds or explicit debug mode, require signed runtime configuration for overrides, and retain GH permission lookup as the sole source of truth in normal runs.
evidence_digest: cd06401db7689b0c67980a4ba1b950f1e2db4e24a22a1640737006bfcb6db0ef
evidence_pointer: none
timestamp: 2026-02-17T00:58:03Z
- finding_id: f-708-security-1771289888656750-0
type: security
severity: MAJOR
summary: Signing key file permissions are applied after key material is written
risk: When FAC queue state is initialized for the first time, the signing key is written with default process umask and filesystem permissions, then restricted to 0o600. A pre-existing symlink or permissive umask widens exposure/corruption between write and chmod, enabling local adversaries with write access to APM2_HOME to read/replace signing key during initialization.
impact: Compromise of fac/private/fac/signing_key allows forging FAC job signatures or authority tokens, undermining queue admission and trusted dispatch in the FAC runtime.
location: crates/apm2-cli/src/commands/fac_queue_submit.rs:319
body: Create the key file atomically with owner-only permissions from the start (e.g., secure temp/file-open flags or O_NOFOLLOW-compatible helpers), refuse symlink targets, and avoid separate write+chmod step. Then fsync and fsync directory to prevent TOCTOU and residual permission leaks.
evidence_digest: a2104c118028047e12a03c35c1e93787f8a3ec9ad7f1470f451a38902dbc261f
evidence_pointer: none
timestamp: 2026-02-17T00:58:08Z |
Collaborator
Author
# apm2-review-verdict:v1
schema: apm2.review.verdict.v1
pr: 708
sha: a302f44d9c0239d4e148e5d98cb1ddaaa20df107
updated_at: 2026-02-17T01:14:18Z
dimensions:
code-quality:
decision: deny
reason: 'FAIL: 1 blocker, 0 major findings'
set_by: ubuntu
set_at: 2026-02-17T01:14:18Z
security:
decision: deny
reason: 'FAIL: 1 blocker, 2 major findings. Critical thread-safety violation in worker CWD handling (process-global state mutation), credential permission race (TOCTOU), and DoS risk via unbounded file reads.'
set_by: ubuntu
set_at: 2026-02-17T01:08:46Z
findings:
- finding_id: f-708-code_quality-1771290844500912-0
type: code-quality
severity: BLOCKER
summary: 'Missing requirement: test_safety_guard correctly evaluates top-level src/*.rs paths'
risk: Top-level Rust modules under src are excluded from test-safety scanning, so test-only unsafe constructs can be missed in production modules with cfg(test) regions.
impact: Security and compliance checks can silently miss prohibited command patterns in , , etc., violating TCK-00613 DoD requirement on top-level test-safety correctness.
location: crates/apm2-cli/src/commands/fac_review/gate_checks.rs:216
body: The predicate requires for Rust source checks, but top-level crate files use (no leading slash) and are skipped. This is a direct violation of the ticket’s acceptance criterion that correctly evaluates top-level paths. Update the path match to include prefix explicitly (for example ), and add a regression test covering with cfg(test)-scoped command-literal content.
evidence_digest: 1eb0a8a64f33454cf6864c4904f209b45fa6607554f126b269fa88fa886cea02
evidence_pointer: none
timestamp: 2026-02-17T01:14:04Z
- finding_id: f-708-code_quality-1771290851574533-0
type: code-quality
severity: MINOR
summary: test_safety_guard loads entire workspace files without size or directory caps
risk: Workspace scans read every matched file fully into memory, including large artifacts in generated directories, which can cause CI resource spikes and non-deterministic timeouts.
impact: In large repositories or adversarial inputs, gate execution can exceed memory/time budgets, producing false failures and masking true violations.
location: crates/apm2-cli/src/commands/fac_review/gate_checks.rs:145
body: The new test-safety gate scans all non-git entries under workspace and reads entire files with , but there are no path allowlist caps or file-size limits. Without excluding large generated trees (, , caches) or enforcing max file size, one oversized file can allocate large memory and make gate execution flaky for CI. Add bounded traversal + bounded reads (or streamed regex scanning) and explicit exclusion of non-source artifact directories before enforcement.
evidence_digest: 180ac758c3650bfb7850b4bf0b81c4fcc952f7c37d05bd8f30c7ce208c044dfb
evidence_pointer: none
timestamp: 2026-02-17T01:14:11Z
- finding_id: f-708-security-1771290491971095-0
type: security
severity: MAJOR
summary: Race condition in signing key permission restriction
risk: The signing key file is created with default permissions (potentially world-readable depending on umask) before being restricted to 0600. This TOCTOU window allows other users on the system to read the private key.
impact: Compromise of the FAC broker/worker identity, allowing an attacker to forge receipts or impersonate the node.
location: crates/apm2-cli/src/commands/fac_queue_submit.rs:load_or_generate_persistent_signer
body: Use `fs::OpenOptions` with `.mode(0o600)` (via `std::os::unix::fs::OpenOptionsExt`) to create the file with restricted permissions atomically at the filesystem level. Do not use `fs::write` followed by `set_permissions`.
evidence_digest: 4cd9ff58e3801973373da546778b1b3d2dc1f0526c3a60f0f853938c7129f620
evidence_pointer: none
timestamp: 2026-02-17T01:08:11Z
- finding_id: f-708-security-1771290497095485-0
type: security
severity: BLOCKER
summary: Thread-unsafe process-global CWD modification
risk: The worker modifies the process-global Current Working Directory (CWD) using `std::env::set_current_dir` via `CurrentDirGuard`. In a multi-threaded runtime (like Tokio, used by apm2-daemon/worker), this affects all threads simultaneously, causing race conditions for any other file I/O, logging, or process spawning occurring in parallel.
impact: Unpredictable failure of concurrent operations, log misplacement, or incorrect path resolution in parallel tasks. Violates Rust safety expectations for multi-threaded processes.
location: crates/apm2-cli/src/commands/fac_worker.rs:run_gates_in_workspace
body: Remove `CurrentDirGuard` and the call to `std::env::set_current_dir`. Refactor `run_gates_local_worker` and downstream gate logic to accept an explicit `workspace_root` path and resolve all file operations relative to that root. Do not rely on ambient CWD.
evidence_digest: 76d226ad9adf3fac026834bb3ae9dbe37b0b7ec84a1602a7c35c726b87aa1eba
evidence_pointer: none
timestamp: 2026-02-17T01:08:17Z
- finding_id: f-708-security-1771290502218266-0
type: security
severity: MAJOR
summary: Unbounded memory consumption in gate checks
risk: The `run_test_safety_guard` and `run_review_artifact_lint` functions read entire files into memory strings using `fs::read` and `String::from_utf8_lossy` without size limits. Encountering a large file (e.g., generated artifact, large fixture) will cause an OOM crash (DoS).
impact: Denial of Service of the CI/review process. An attacker could commit a large file to crash the review gate.
location: crates/apm2-cli/src/commands/fac_review/gate_checks.rs
body: Enforce a strict file size limit (e.g., 10MB) before reading files. Skip files exceeding the limit or process them in bounded chunks (though regex matching on chunks is harder). For source code and review artifacts, a size limit is appropriate and safe.
evidence_digest: f1ac07c8772794a3396a9de4b4b5edda6ab59c8f79e25cd0cd201070d9d3b398
evidence_pointer: none
timestamp: 2026-02-17T01:08:22Z
- finding_id: f-708-security-1771290507341295-0
type: security
severity: MINOR
summary: Secrets stored in weakly-typed Strings
risk: Credentials (GITHUB_TOKEN) are stored in standard `String` types in `fac_preflight.rs`. This keeps secrets in memory longer than necessary (no zeroize on drop) and risks accidental exposure via `Debug` or panic dumps.
impact: Increased risk of credential leakage in dumps or logs.
location: crates/apm2-cli/src/commands/fac_preflight.rs
body: Use `secrecy::SecretString` for credential variables to ensure redaction in Debug and best-effort zeroization on Drop. Conforms to CTR-2604.
evidence_digest: 65065885ed6badfeaaf7087831516a9b49b801f6bccf0868b8d695a2c8282695
evidence_pointer: none
timestamp: 2026-02-17T01:08:27Z |
Anveio
changed the title
Collaborator
Author
# apm2-review-verdict:v1
schema: apm2.review.verdict.v1
pr: 708
sha: 847f5226e76863728706248036fd772f6f4721b0
updated_at: 2026-02-17T02:32:21Z
dimensions:
code-quality:
decision: approve
reason: 'PASS: no blocker or major findings'
set_by: ubuntu
set_at: 2026-02-17T02:32:21Z
security:
decision: approve
reason: 'PASS: no blocker or major findings (2 minor hardening findings recorded)'
set_by: ubuntu
set_at: 2026-02-17T02:31:00Z
findings:
- finding_id: f-708-security-1771295438921770-0
type: security
severity: MINOR
summary: Gate checks can miss non-UTF8 tracked paths
risk: A malicious contributor can use file names with non-UTF8 bytes so tracked path decoding becomes lossy before path resolution. Those entries may be skipped during safety and integrity scans.
impact: Security gate coverage can be incomplete while reports still look successful, weakening confidence in destructive-pattern detection and workspace integrity checks.
location: crates/apm2-cli/src/commands/fac_review/gate_checks.rs:1017
body: Preserve raw path bytes for NUL-delimited git entries and fail closed when a tracked entry cannot be represented safely. Add a regression test using a non-UTF8 filename to prove the checker scans it or denies the run.
evidence_digest: d203354df78751829b20d1accbf0296419617b6a17cb3d442f74f8e112a21140
evidence_pointer: none
timestamp: 2026-02-17T02:30:38Z
- finding_id: f-708-security-1771295451055879-0
type: security
severity: MINOR
summary: Signing-key parent bootstrap uses create-then-chmod
risk: When the signer parent path is created, directory permissions are tightened after creation. A local attacker with concurrent filesystem access to that parent hierarchy can race this window to interfere with bootstrap state.
impact: This weakens the hard fail-closed posture around key material initialization and can enable local tampering or denial of service during first-time setup.
location: crates/apm2-cli/src/commands/fac_key_material.rs:54
body: Create sensitive directories with restrictive mode at creation time instead of create-and-fix. Reuse the existing FAC permission helper that uses create-time 0700 semantics, and add a regression test that asserts parent directory mode at first creation.
evidence_digest: d42a6016bffb040cb480410e10a681ae6aca85fa59796a47324051c1d17a59dc
evidence_pointer: none
timestamp: 2026-02-17T02:30:51Z |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
FAC Gate Status