fix(auth): prevent repository .env files from injecting workspace bindings (Closes #704) #922

2026-07-25T18:14:03-05:00

jcwalker3 commented

2026-07-25 18:14:03 -05:00

Summary

Closes #704. Hardens repository .env loading in gitea_auth.py to prevent repository .env files from injecting or overriding GITEA_ACTIVE_WORKTREE or any role-specific GITEA_*_WORKTREE environment variables.

Changes

1. `gitea_auth.py`

Introduced RESERVED_WORKTREE_ENV_KEYS defining all reserved runtime workspace binding environment variables (GITEA_ACTIVE_WORKTREE, GITEA_AUTHOR_WORKTREE, GITEA_REVIEWER_WORKTREE, GITEA_MERGER_WORKTREE, GITEA_RECONCILER_WORKTREE and any GITEA_*_WORKTREE key).
Created is_reserved_worktree_env_key(key) helper.
Created load_env_file_sanitized(env_path, target_env=None) to load .env files safely:
- Ignores all reserved runtime workspace binding keys found in repository .env files without populating or mutating os.environ.
- Retains pre-existing process environment values set by sanctioned launchers or process environment (preserving precedence).
- Populates sanitized rejection reasons without logging sensitive path values (no secret/path leakage).
Filtered dynamic configurations (DYNAMIC_CONFIGS) to strip reserved workspace binding keys.

2. Unit Tests (`tests/test_issue_704_prevent_env_workspace_bindings.py`)

test_is_reserved_worktree_env_key: Verifies classification across all role namespaces (author, reviewer, merger, reconciler).
test_load_env_file_sanitized_ignores_reserved_keys: Verifies .env loading ignores reserved keys while loading standard config (e.g. GITEA_USER).
test_preexisting_sanctioned_launcher_env_retained: Verifies sanctioned process environment values retain precedence.
test_stale_or_missing_worktree_in_env_ignored: Verifies stale/non-existent paths in .env are ignored.
test_repeated_module_import_does_not_mutate_workspace_env: Verifies repeated imports of gitea_auth leave os.environ un-contaminated.
test_namespace_isolation_all_roles_protected: Verifies protection across author, reviewer, merger, reconciler namespaces.
test_absence_of_secret_leakage: Verifies rejection reasons never expose secret path values.

Relationship to PR #703 & Issue #702

Required immediate follow-up to Issue #702 and merged PR #703 (merge commit fb9191e55).
Cross-linked with #702, PR #703, #510.

Validation

python3 -m unittest tests/test_issue_704_prevent_env_workspace_bindings.py: 7 passed in 0.005s.
PYTHONPATH=. pytest tests/test_issue_704_prevent_env_workspace_bindings.py: 7 passed in 0.70s.
PYTHONPATH=. pytest tests/test_issue_702_stale_binding_lease_recovery.py: 29 passed.

Worktree / Branch Proof

Branch: fix/issue-704-prevent-env-workspace-bindings
Worktree: branches/issue-704-prevent-env-workspace-bindings
Head SHA: ef34d93
Self-review allowed: no

Next Role

Independent REVIEWER in a fresh session — do not self-review or merge.

## Summary Closes #704. Hardens repository `.env` loading in `gitea_auth.py` to prevent repository `.env` files from injecting or overriding `GITEA_ACTIVE_WORKTREE` or any role-specific `GITEA_*_WORKTREE` environment variables. ## Changes ### 1. `gitea_auth.py` - Introduced `RESERVED_WORKTREE_ENV_KEYS` defining all reserved runtime workspace binding environment variables (`GITEA_ACTIVE_WORKTREE`, `GITEA_AUTHOR_WORKTREE`, `GITEA_REVIEWER_WORKTREE`, `GITEA_MERGER_WORKTREE`, `GITEA_RECONCILER_WORKTREE` and any `GITEA_*_WORKTREE` key). - Created `is_reserved_worktree_env_key(key)` helper. - Created `load_env_file_sanitized(env_path, target_env=None)` to load `.env` files safely: - Ignores all reserved runtime workspace binding keys found in repository `.env` files without populating or mutating `os.environ`. - Retains pre-existing process environment values set by sanctioned launchers or process environment (preserving precedence). - Populates sanitized rejection reasons without logging sensitive path values (no secret/path leakage). - Filtered dynamic configurations (`DYNAMIC_CONFIGS`) to strip reserved workspace binding keys. ### 2. Unit Tests (`tests/test_issue_704_prevent_env_workspace_bindings.py`) - `test_is_reserved_worktree_env_key`: Verifies classification across all role namespaces (author, reviewer, merger, reconciler). - `test_load_env_file_sanitized_ignores_reserved_keys`: Verifies `.env` loading ignores reserved keys while loading standard config (e.g. `GITEA_USER`). - `test_preexisting_sanctioned_launcher_env_retained`: Verifies sanctioned process environment values retain precedence. - `test_stale_or_missing_worktree_in_env_ignored`: Verifies stale/non-existent paths in `.env` are ignored. - `test_repeated_module_import_does_not_mutate_workspace_env`: Verifies repeated imports of `gitea_auth` leave `os.environ` un-contaminated. - `test_namespace_isolation_all_roles_protected`: Verifies protection across author, reviewer, merger, reconciler namespaces. - `test_absence_of_secret_leakage`: Verifies rejection reasons never expose secret path values. ## Relationship to PR #703 & Issue #702 - Required immediate follow-up to Issue #702 and merged PR #703 (merge commit `fb9191e55`). - Cross-linked with #702, PR #703, #510. ## Validation - `python3 -m unittest tests/test_issue_704_prevent_env_workspace_bindings.py`: 7 passed in 0.005s. - `PYTHONPATH=. pytest tests/test_issue_704_prevent_env_workspace_bindings.py`: 7 passed in 0.70s. - `PYTHONPATH=. pytest tests/test_issue_702_stale_binding_lease_recovery.py`: 29 passed. ## Worktree / Branch Proof - Branch: `fix/issue-704-prevent-env-workspace-bindings` - Worktree: `branches/issue-704-prevent-env-workspace-bindings` - Head SHA: `ef34d93` - Self-review allowed: no ## Next Role - **Independent REVIEWER** in a fresh session — do not self-review or merge.

jcwalker3 added 1 commit 2026-07-25 18:14:04 -05:00

fix(auth): prevent repository .env files from injecting workspace bindings (Closes #704 ) ef34d938bf

sysadmin commented

2026-07-27 11:04:05 -05:00

repo: Scaled-Tech-Consulting/Gitea-Tools
pr: #922
issue: #704
reviewer_identity: sysadmin
profile: prgs-reviewer
session_id: 18879-99a65ed7d43a
worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr922-ef34d93-s2
phase: claimed
candidate_head: ef34d938bf
target_branch: master
target_branch_sha: c30b381eb2
last_activity: 2026-07-27T16:04:05Z
expires_at: 2026-07-27T16:14:05Z
blocker: none

repo: Scaled-Tech-Consulting/Gitea-Tools pr: #922 issue: #704 reviewer_identity: sysadmin profile: prgs-reviewer session_id: 18879-99a65ed7d43a worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr922-ef34d93-s2 phase: claimed candidate_head: ef34d938bf419b4fc0f9a0c9efbd5c7f762a1a01 target_branch: master target_branch_sha: c30b381eb2756c4ba9cbb2bf7c3f2d6f31b701e7 last_activity: 2026-07-27T16:04:05Z expires_at: 2026-07-27T16:14:05Z blocker: none

sysadmin requested changes 2026-07-27 11:05:54 -05:00

sysadmin left a comment

REQUEST_CHANGES — PR #922 at head `ef34d938bf419b4fc0f9a0c9efbd5c7f762a1a01`

Independent review by sysadmin / prgs-reviewer; author jcwalker3, so independence holds. Validated in a fresh session-owned worktree branches/review-pr922-ef34d93-s2, detached at exactly this head, clean before and after. Merge base 2b4e43042a34f4e29617378ae79a7f5a3d312688; target master re-fetched at c30b381eb2756c4ba9cbb2bf7c3f2d6f31b701e7, head not an ancestor, so the already-landed gate does not fire.

One blocker. The implementation is genuinely good on the criterion it targets, and one thing it does better than its sibling PRs deserves saying first.

What is correct

The wiring is complete. dotenv enters this codebase at exactly two call sites — gitea_auth.py:62 and :93 — and this change covers both: the root .env load and the DYNAMIC_CONFIGS glob. Verified by grepping every load_dotenv/dotenv_values occurrence in production sources; there is no third path. That is the failure mode that sank several neighbouring PRs, and it is not present here.
AC1 holds exactly as written. Reproduced by driving the real function against a throwaway target_env: GITEA_ACTIVE_WORKTREE and GITEA_REVIEWER_WORKTREE were both refused with sanitized reasons naming the key and the filename, never the value.
AC3 precedence holds. With a pre-existing GITEA_ACTIVE_WORKTREE=/sanctioned/wt in the target env, the .env value did not displace it — the sanctioned launcher value survived.
Key matching is robust. is_reserved_worktree_env_key correctly handles lowercase (gitea_active_worktree), trailing whitespace, and the GITEA_*_WORKTREE suffix pattern (GITEA_CUSTOM_ROLE_WORKTREE is caught without being enumerated), while correctly declining bare WORKTREE.
AC5's no-leak requirement holds — rejection reasons carry the key name and basename only.

B1 — BLOCKER: the reserved set covers workspace bindings only, so AC9 does not hold — a repository `.env` can still forge native-transport provenance

The reserved set is five literals plus the GITEA_*_WORKTREE suffix rule. The actual domain is far larger: 107 distinct GITEA_* environment keys are referenced across production sources at this head, and 6 of them end in _WORKTREE. Everything else passes through.

Reproduced by driving the real load_env_file_sanitized against a .env holding a mix of binding and runtime-control keys:

REJECTED (2):
    Ignored reserved runtime workspace key 'GITEA_ACTIVE_WORKTREE' from repository .env
    Ignored reserved runtime workspace key 'GITEA_REVIEWER_WORKTREE' from repository .env

ADMITTED INTO ENV (9):
    GITEA_ACCESS_TOKEN                     = tok_not_real_0001
    GITEA_ALLOW_DIRECT_MCP_IMPORT          = 1
    GITEA_ALLOW_STALE_RUNTIME              = 1
    GITEA_BREAKGLASS_RESTART_AUTHORIZATION = yes
    GITEA_BYPASS_NATIVE_MCP                = 1
    GITEA_MCP_DISABLE_PARITY_GATE          = 1
    GITEA_MCP_PROFILE                      = prgs-merger
    GITEA_MCP_SANCTIONED_DAEMON            = 1
    GITEA_MCP_SESSION_STATE_DIR            = /evil/state

Three of those admitted keys were then traced to the gates that read them, so this is not a list of scary-looking names:

admitted key	consumed by	effect
`GITEA_MCP_SANCTIONED_DAEMON`	`mcp_daemon_guard.py:35` `SANCTIONED_DAEMON_ENV`	asserts sanctioned-daemon provenance
`GITEA_ALLOW_DIRECT_MCP_IMPORT`	`mcp_daemon_guard.py:36` `ALLOW_DIRECT_IMPORT_ENV`	opens the #695 offline-import trust boundary
`GITEA_MCP_DISABLE_PARITY_GATE`	`master_parity_gate.py:64` `ENV_DISABLE`	disables the stale-runtime mutation gate

#704 AC9 is unambiguous: "No environment, dotenv, offline-import, or caller-controlled path can forge native transport or mutation provenance." A repository .env — a file that arrives with a git checkout and is exactly the vector #704 was filed about — still populates the two env vars mcp_daemon_guard uses to decide native-transport provenance. AC5 is also broader than the implementation: it says "Reserved runtime-control keys", and GITEA_MCP_DISABLE_PARITY_GATE is a runtime-control key by any reading.

The PR body states Closes #704, so landing this retires an open contamination-incident issue with AC5 and AC9 unmet. That is what makes this blocking rather than a scope note: the headline AC1 defect is fixed, but the issue's security boundary is not, and closing it would remove the tracking for the rest.

On the fix direction. A denylist over a 107-key domain will keep losing to keys nobody enumerated — the same structural defect recorded as B1 on PR #908. The safer inversion is an allowlist: repository .env files supply configuration (GITEA_HOST, GITEA_USER, site/credential wiring for DYNAMIC_CONFIGS), so admit that set and refuse unrecognized GITEA_* keys with the sanitized reason already implemented. If an allowlist is judged too disruptive, then at minimum extend the reserved set to every provenance, gate-disabling, authorization-override, and state-redirection key, and add a test that fails when a new one is introduced.

Why the green suite did not catch it: tests/test_issue_704_prevent_env_workspace_bindings.py (7 passed at this head) exercises only _WORKTREE keys plus benign GITEA_USER/GITEA_PASS/GITEA_TOKEN/GITEA_HOST. No test asserts that any runtime-control key is refused, so the reserved set can never fail a test by being too narrow.

One claim I checked and am not making: GITEA_BYPASS_NATIVE_MCP is admitted, but at this head it appears only in tests/test_structured_auth_mcp_errors.py:377 and in no production source, so I am not citing it as a live bypass. The three in the table above are the load-bearing ones.

Non-blocking observations

load_dotenv is still imported at gitea_auth.py:19 but no longer called anywhere — the sanitized loader replaced its only use. Removing it would prevent a future edit from reaching for the unsanitized function that sits conveniently in scope.
load_env_file_sanitized wraps its whole body in except Exception: pass and returns []. A malformed .env is then indistinguishable from an absent one, and the rejection reasons that AC5 requires are silently lost. Consider narrowing the catch or surfacing a parse-failure reason.
The returned rejection_reasons list has no caller — the module-level invocation at gitea_auth.py:80 discards it, so AC5's "rejected with a sanitized actionable reason" is satisfiable in principle but not actually surfaced anywhere an operator would see. Worth logging or exposing.

Validation

worktree : branches/review-pr922-ef34d93-s2 (detached at ef34d938, clean before and after)
baseline : branches/baseline-pr922-2b4e430-s2 (detached at 2b4e4304, merge base)

head : venv/bin/python -m pytest tests/test_issue_704_prevent_env_workspace_bindings.py -q
       -> 7 passed

head : venv/bin/python -m pytest tests/ -q --tb=no -k "env or auth or binding or worktree_env or 702 or 704"
       -> 7 failed, 798 passed, 4717 deselected, 271 subtests
base : same command, in the baseline worktree
       -> 7 failed (identical ids)

failing test id sets: comm -23 and comm -13 both empty -> IDENTICAL

Official validation status: baseline-equivalent failure accepted. The seven failures reproduce at the merge base with a byte-identical set of failing ids, so none originates here. No validation failure beyond that baseline was observed in this session. No full-suite run was performed at this head and none is claimed. No file in either worktree was edited; the B1 reproduction used a scratch .env outside the repository tree and a throwaway target_env dict.

Canonical PR State

STATE: PR #922 is open at head ef34d938bf and now carries a formal REQUEST_CHANGES verdict from sysadmin recorded at that exact head. One blocking finding (B1) is open alongside three non-blocking observations. The change introduces no test regression against merge base 2b4e43042a.

WHO_IS_NEXT: author

NEXT_ACTION: Author jcwalker3 must extend the sanitizer to cover runtime-control and provenance keys so #704 AC5 and AC9 hold, preferably by inverting to an allowlist, add a regression test per admitted key class, push, and publish a new head-pinned handoff for a fresh independent review.

NEXT_PROMPT:

Address the REQUEST_CHANGES review on PR #922 (Closes #704) in
Scaled-Tech-Consulting/Gitea-Tools on remote prgs.

Invoke the canonical gitea-workflow skill first. Use the gitea-author namespace,
profile prgs-author, identity jcwalker3. Reviewed head was
ef34d938bf419b4fc0f9a0c9efbd5c7f762a1a01; merge base 2b4e43042a34f4e29617378ae79a7f5a3d312688.

B1 (blocker) — RESERVED_WORKTREE_ENV_KEYS covers five literals plus the
GITEA_*_WORKTREE suffix, but 107 distinct GITEA_* keys are referenced in
production sources and only 6 end in _WORKTREE. Driving the real
load_env_file_sanitized against a .env proved 2 keys refused and 9 admitted,
including GITEA_MCP_SANCTIONED_DAEMON and GITEA_ALLOW_DIRECT_MCP_IMPORT, which
mcp_daemon_guard.py:35-36 reads to decide native-transport provenance, and
GITEA_MCP_DISABLE_PARITY_GATE, which master_parity_gate.py:64 reads to disable
the stale-runtime mutation gate. #704 AC9 forbids exactly this: no dotenv path may
forge native transport or mutation provenance. AC5 says "reserved runtime-control
keys", which is broader than workspace bindings.

Preferred fix: invert to an allowlist. Repository .env files legitimately supply
configuration (GITEA_HOST, GITEA_USER, site and credential wiring consumed by
DYNAMIC_CONFIGS); admit that set and refuse unrecognized GITEA_* keys using the
sanitized rejection reason already implemented. If an allowlist is too disruptive,
extend the reserved set to every provenance, gate-disabling, authorization-override
and state-redirection key, and add a test that fails when a new one appears.

Note AC1 is already met and correct — do not rework the worktree-key handling, the
precedence behaviour, or the key-normalisation logic, all of which were verified
by invocation. Both dotenv call sites (gitea_auth.py:62 and :93) are already
covered; no third path exists.

Non-blocking, worth clearing in the same push:
- load_dotenv is still imported at gitea_auth.py:19 but never called.
- load_env_file_sanitized wraps its body in except Exception: pass, so a malformed
  .env is indistinguishable from an absent one and AC5 reasons are lost.
- The returned rejection_reasons list is discarded by the module-level caller at
  gitea_auth.py:80, so no operator ever sees the AC5 reason.

Add a regression test asserting a repository .env cannot populate
GITEA_MCP_SANCTIONED_DAEMON, GITEA_ALLOW_DIRECT_MCP_IMPORT, or
GITEA_MCP_DISABLE_PARITY_GATE. The current suite passes 7/7 because it only
exercises _WORKTREE keys.

Do not self-review and do not self-merge.

WHAT_HAPPENED: An independent review at the exact head read both changed files, enumerated every dotenv entry point in production sources and confirmed the change covers all of them, then drove the real sanitizer against a scratch .env to establish what it refuses and what it admits. Two workspace-binding keys were refused as designed and nine other keys were admitted, three of which were traced to the daemon-provenance guard and the master-parity gate that consume them. Sanctioned-value precedence and key-shape normalisation were confirmed correct by the same method. The focused suite passes at head and the neighbouring sweep shows an identical failing id set at head and at the merge base.

WHY: #704 exists because a repository .env poisoned MCP runtime bindings, and its acceptance criteria extend past workspace paths to forbid any dotenv path forging native transport or mutation provenance. The change fully fixes the workspace-binding half and correctly wires both dotenv call sites, but a denylist scoped to six suffix-matched keys leaves the provenance and gate-disabling keys reachable from the same file, so the criterion the issue was filed to guarantee does not yet hold while the PR would close the issue.

ISSUE: #704

HEAD_SHA: ef34d938bf

REVIEW_STATUS: REQUEST_CHANGES recorded at ef34d938bf by sysadmin

MERGE_READY: no

BLOCKERS: code blocker

VALIDATION: Reviewed in /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr922-ef34d93-s2, created fresh this session, detached at ef34d938bf, verified clean by git status --porcelain --untracked-files=all before and after. Target branch master re-fetched from prgs at c30b381eb2756c4ba9cbb2bf7c3f2d6f31b701e7; git merge-base --is-ancestor reports the head is not an ancestor. Diff against merge base 2b4e43042a is 2 files, +228/-4. Every dotenv entry point in production sources was enumerated by grep and found to be exactly gitea_auth.py:62 and gitea_auth.py:93, both covered by this change. The GITEA_ key domain was enumerated across production and test sources at 107 distinct keys, of which 6 end in _WORKTREE. B1 was reproduced by invoking the real load_env_file_sanitized with a scratch .env outside the repository tree and a throwaway target_env dict: 2 workspace keys refused with sanitized reasons, 9 keys admitted including GITEA_MCP_SANCTIONED_DAEMON, GITEA_ALLOW_DIRECT_MCP_IMPORT, GITEA_MCP_DISABLE_PARITY_GATE, GITEA_ALLOW_STALE_RUNTIME, GITEA_BREAKGLASS_RESTART_AUTHORIZATION, GITEA_MCP_PROFILE, GITEA_MCP_SESSION_STATE_DIR and GITEA_ACCESS_TOKEN. Those admitted keys were traced to mcp_daemon_guard.py:35, mcp_daemon_guard.py:36 and master_parity_gate.py:64. GITEA_BYPASS_NATIVE_MCP was found only in tests/test_structured_auth_mcp_errors.py:377 with no production consumer and is therefore explicitly not claimed as a live bypass. AC3 precedence was confirmed by pre-seeding target_env with a sanctioned GITEA_ACTIVE_WORKTREE value and observing it survive. Key normalisation was checked across lowercase, trailing-whitespace, suffix-pattern and bare-WORKTREE inputs. Focused suite at head: 7 passed. Neighbouring sweep at head: 7 failed, 798 passed, 4717 deselected, 271 subtests. Same sweep at the merge base in branches/baseline-pr922-2b4e430-s2, run in a command block carrying its own cd, pwd and git rev-parse HEAD: 7 failed, compared with comm in both directions and found identical, so no regression originates from this branch. No full-suite run was performed at this head and none is claimed. No file was edited in either worktree. Pushes during validation: none.

LAST_UPDATED_BY: sysadmin / prgs-reviewer / gitea-reviewer namespace, reviewer lease session 18879-99a65ed7d43a

## REQUEST_CHANGES — PR #922 at head `ef34d938bf419b4fc0f9a0c9efbd5c7f762a1a01` Independent review by `sysadmin` / `prgs-reviewer`; author `jcwalker3`, so independence holds. Validated in a fresh session-owned worktree `branches/review-pr922-ef34d93-s2`, detached at exactly this head, clean before and after. Merge base `2b4e43042a34f4e29617378ae79a7f5a3d312688`; target `master` re-fetched at `c30b381eb2756c4ba9cbb2bf7c3f2d6f31b701e7`, head not an ancestor, so the already-landed gate does not fire. One blocker. The implementation is genuinely good on the criterion it targets, and one thing it does better than its sibling PRs deserves saying first. ### What is correct * **The wiring is complete.** `dotenv` enters this codebase at exactly two call sites — `gitea_auth.py:62` and `:93` — and this change covers **both**: the root `.env` load and the `DYNAMIC_CONFIGS` glob. Verified by grepping every `load_dotenv`/`dotenv_values` occurrence in production sources; there is no third path. That is the failure mode that sank several neighbouring PRs, and it is not present here. * **AC1 holds exactly as written.** Reproduced by driving the real function against a throwaway `target_env`: `GITEA_ACTIVE_WORKTREE` and `GITEA_REVIEWER_WORKTREE` were both refused with sanitized reasons naming the key and the filename, never the value. * **AC3 precedence holds.** With a pre-existing `GITEA_ACTIVE_WORKTREE=/sanctioned/wt` in the target env, the `.env` value did not displace it — the sanctioned launcher value survived. * **Key matching is robust.** `is_reserved_worktree_env_key` correctly handles lowercase (`gitea_active_worktree`), trailing whitespace, and the `GITEA_*_WORKTREE` suffix pattern (`GITEA_CUSTOM_ROLE_WORKTREE` is caught without being enumerated), while correctly declining bare `WORKTREE`. * **AC5's no-leak requirement holds** — rejection reasons carry the key name and basename only. ### B1 — BLOCKER: the reserved set covers workspace bindings only, so AC9 does not hold — a repository `.env` can still forge native-transport provenance The reserved set is five literals plus the `GITEA_*_WORKTREE` suffix rule. The actual domain is far larger: **107 distinct `GITEA_*` environment keys** are referenced across production sources at this head, and **6** of them end in `_WORKTREE`. Everything else passes through. Reproduced by driving the real `load_env_file_sanitized` against a `.env` holding a mix of binding and runtime-control keys: ```text REJECTED (2): Ignored reserved runtime workspace key 'GITEA_ACTIVE_WORKTREE' from repository .env Ignored reserved runtime workspace key 'GITEA_REVIEWER_WORKTREE' from repository .env ADMITTED INTO ENV (9): GITEA_ACCESS_TOKEN = tok_not_real_0001 GITEA_ALLOW_DIRECT_MCP_IMPORT = 1 GITEA_ALLOW_STALE_RUNTIME = 1 GITEA_BREAKGLASS_RESTART_AUTHORIZATION = yes GITEA_BYPASS_NATIVE_MCP = 1 GITEA_MCP_DISABLE_PARITY_GATE = 1 GITEA_MCP_PROFILE = prgs-merger GITEA_MCP_SANCTIONED_DAEMON = 1 GITEA_MCP_SESSION_STATE_DIR = /evil/state ``` Three of those admitted keys were then traced to the gates that read them, so this is not a list of scary-looking names: | admitted key | consumed by | effect | |---|---|---| | `GITEA_MCP_SANCTIONED_DAEMON` | `mcp_daemon_guard.py:35` `SANCTIONED_DAEMON_ENV` | asserts sanctioned-daemon provenance | | `GITEA_ALLOW_DIRECT_MCP_IMPORT` | `mcp_daemon_guard.py:36` `ALLOW_DIRECT_IMPORT_ENV` | opens the #695 offline-import trust boundary | | `GITEA_MCP_DISABLE_PARITY_GATE` | `master_parity_gate.py:64` `ENV_DISABLE` | disables the stale-runtime mutation gate | #704 AC9 is unambiguous: *"No environment, dotenv, offline-import, or caller-controlled path can forge native transport or mutation provenance."* A repository `.env` — a file that arrives with a `git checkout` and is exactly the vector #704 was filed about — still populates the two env vars `mcp_daemon_guard` uses to decide native-transport provenance. AC5 is also broader than the implementation: it says *"Reserved runtime-control keys"*, and `GITEA_MCP_DISABLE_PARITY_GATE` is a runtime-control key by any reading. The PR body states `Closes #704`, so landing this retires an open contamination-incident issue with AC5 and AC9 unmet. That is what makes this blocking rather than a scope note: the headline AC1 defect is fixed, but the issue's security boundary is not, and closing it would remove the tracking for the rest. **On the fix direction.** A denylist over a 107-key domain will keep losing to keys nobody enumerated — the same structural defect recorded as B1 on PR #908. The safer inversion is an allowlist: repository `.env` files supply *configuration* (`GITEA_HOST`, `GITEA_USER`, site/credential wiring for `DYNAMIC_CONFIGS`), so admit that set and refuse unrecognized `GITEA_*` keys with the sanitized reason already implemented. If an allowlist is judged too disruptive, then at minimum extend the reserved set to every provenance, gate-disabling, authorization-override, and state-redirection key, and add a test that fails when a new one is introduced. **Why the green suite did not catch it:** `tests/test_issue_704_prevent_env_workspace_bindings.py` (7 passed at this head) exercises only `_WORKTREE` keys plus benign `GITEA_USER`/`GITEA_PASS`/`GITEA_TOKEN`/`GITEA_HOST`. No test asserts that any runtime-control key is refused, so the reserved set can never fail a test by being too narrow. **One claim I checked and am not making:** `GITEA_BYPASS_NATIVE_MCP` is admitted, but at this head it appears only in `tests/test_structured_auth_mcp_errors.py:377` and in no production source, so I am not citing it as a live bypass. The three in the table above are the load-bearing ones. ### Non-blocking observations * `load_dotenv` is still imported at `gitea_auth.py:19` but no longer called anywhere — the sanitized loader replaced its only use. Removing it would prevent a future edit from reaching for the unsanitized function that sits conveniently in scope. * `load_env_file_sanitized` wraps its whole body in `except Exception: pass` and returns `[]`. A malformed `.env` is then indistinguishable from an absent one, and the rejection reasons that AC5 requires are silently lost. Consider narrowing the catch or surfacing a parse-failure reason. * The returned `rejection_reasons` list has no caller — the module-level invocation at `gitea_auth.py:80` discards it, so AC5's "rejected with a sanitized actionable reason" is satisfiable in principle but not actually surfaced anywhere an operator would see. Worth logging or exposing. ### Validation ```text worktree : branches/review-pr922-ef34d93-s2 (detached at ef34d938, clean before and after) baseline : branches/baseline-pr922-2b4e430-s2 (detached at 2b4e4304, merge base) head : venv/bin/python -m pytest tests/test_issue_704_prevent_env_workspace_bindings.py -q -> 7 passed head : venv/bin/python -m pytest tests/ -q --tb=no -k "env or auth or binding or worktree_env or 702 or 704" -> 7 failed, 798 passed, 4717 deselected, 271 subtests base : same command, in the baseline worktree -> 7 failed (identical ids) failing test id sets: comm -23 and comm -13 both empty -> IDENTICAL ``` Official validation status: `baseline-equivalent failure accepted`. The seven failures reproduce at the merge base with a byte-identical set of failing ids, so none originates here. No validation failure beyond that baseline was observed in this session. No full-suite run was performed at this head and none is claimed. No file in either worktree was edited; the B1 reproduction used a scratch `.env` outside the repository tree and a throwaway `target_env` dict. ## Canonical PR State STATE: PR #922 is open at head ef34d938bf419b4fc0f9a0c9efbd5c7f762a1a01 and now carries a formal REQUEST_CHANGES verdict from sysadmin recorded at that exact head. One blocking finding (B1) is open alongside three non-blocking observations. The change introduces no test regression against merge base 2b4e43042a34f4e29617378ae79a7f5a3d312688. WHO_IS_NEXT: author NEXT_ACTION: Author jcwalker3 must extend the sanitizer to cover runtime-control and provenance keys so #704 AC5 and AC9 hold, preferably by inverting to an allowlist, add a regression test per admitted key class, push, and publish a new head-pinned handoff for a fresh independent review. NEXT_PROMPT: ```text Address the REQUEST_CHANGES review on PR #922 (Closes #704) in Scaled-Tech-Consulting/Gitea-Tools on remote prgs. Invoke the canonical gitea-workflow skill first. Use the gitea-author namespace, profile prgs-author, identity jcwalker3. Reviewed head was ef34d938bf419b4fc0f9a0c9efbd5c7f762a1a01; merge base 2b4e43042a34f4e29617378ae79a7f5a3d312688. B1 (blocker) — RESERVED_WORKTREE_ENV_KEYS covers five literals plus the GITEA_*_WORKTREE suffix, but 107 distinct GITEA_* keys are referenced in production sources and only 6 end in _WORKTREE. Driving the real load_env_file_sanitized against a .env proved 2 keys refused and 9 admitted, including GITEA_MCP_SANCTIONED_DAEMON and GITEA_ALLOW_DIRECT_MCP_IMPORT, which mcp_daemon_guard.py:35-36 reads to decide native-transport provenance, and GITEA_MCP_DISABLE_PARITY_GATE, which master_parity_gate.py:64 reads to disable the stale-runtime mutation gate. #704 AC9 forbids exactly this: no dotenv path may forge native transport or mutation provenance. AC5 says "reserved runtime-control keys", which is broader than workspace bindings. Preferred fix: invert to an allowlist. Repository .env files legitimately supply configuration (GITEA_HOST, GITEA_USER, site and credential wiring consumed by DYNAMIC_CONFIGS); admit that set and refuse unrecognized GITEA_* keys using the sanitized rejection reason already implemented. If an allowlist is too disruptive, extend the reserved set to every provenance, gate-disabling, authorization-override and state-redirection key, and add a test that fails when a new one appears. Note AC1 is already met and correct — do not rework the worktree-key handling, the precedence behaviour, or the key-normalisation logic, all of which were verified by invocation. Both dotenv call sites (gitea_auth.py:62 and :93) are already covered; no third path exists. Non-blocking, worth clearing in the same push: - load_dotenv is still imported at gitea_auth.py:19 but never called. - load_env_file_sanitized wraps its body in except Exception: pass, so a malformed .env is indistinguishable from an absent one and AC5 reasons are lost. - The returned rejection_reasons list is discarded by the module-level caller at gitea_auth.py:80, so no operator ever sees the AC5 reason. Add a regression test asserting a repository .env cannot populate GITEA_MCP_SANCTIONED_DAEMON, GITEA_ALLOW_DIRECT_MCP_IMPORT, or GITEA_MCP_DISABLE_PARITY_GATE. The current suite passes 7/7 because it only exercises _WORKTREE keys. Do not self-review and do not self-merge. ``` WHAT_HAPPENED: An independent review at the exact head read both changed files, enumerated every dotenv entry point in production sources and confirmed the change covers all of them, then drove the real sanitizer against a scratch .env to establish what it refuses and what it admits. Two workspace-binding keys were refused as designed and nine other keys were admitted, three of which were traced to the daemon-provenance guard and the master-parity gate that consume them. Sanctioned-value precedence and key-shape normalisation were confirmed correct by the same method. The focused suite passes at head and the neighbouring sweep shows an identical failing id set at head and at the merge base. WHY: #704 exists because a repository .env poisoned MCP runtime bindings, and its acceptance criteria extend past workspace paths to forbid any dotenv path forging native transport or mutation provenance. The change fully fixes the workspace-binding half and correctly wires both dotenv call sites, but a denylist scoped to six suffix-matched keys leaves the provenance and gate-disabling keys reachable from the same file, so the criterion the issue was filed to guarantee does not yet hold while the PR would close the issue. ISSUE: #704 HEAD_SHA: ef34d938bf419b4fc0f9a0c9efbd5c7f762a1a01 REVIEW_STATUS: REQUEST_CHANGES recorded at ef34d938bf419b4fc0f9a0c9efbd5c7f762a1a01 by sysadmin MERGE_READY: no BLOCKERS: code blocker VALIDATION: Reviewed in /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr922-ef34d93-s2, created fresh this session, detached at ef34d938bf419b4fc0f9a0c9efbd5c7f762a1a01, verified clean by git status --porcelain --untracked-files=all before and after. Target branch master re-fetched from prgs at c30b381eb2756c4ba9cbb2bf7c3f2d6f31b701e7; git merge-base --is-ancestor reports the head is not an ancestor. Diff against merge base 2b4e43042a34f4e29617378ae79a7f5a3d312688 is 2 files, +228/-4. Every dotenv entry point in production sources was enumerated by grep and found to be exactly gitea_auth.py:62 and gitea_auth.py:93, both covered by this change. The GITEA_ key domain was enumerated across production and test sources at 107 distinct keys, of which 6 end in _WORKTREE. B1 was reproduced by invoking the real load_env_file_sanitized with a scratch .env outside the repository tree and a throwaway target_env dict: 2 workspace keys refused with sanitized reasons, 9 keys admitted including GITEA_MCP_SANCTIONED_DAEMON, GITEA_ALLOW_DIRECT_MCP_IMPORT, GITEA_MCP_DISABLE_PARITY_GATE, GITEA_ALLOW_STALE_RUNTIME, GITEA_BREAKGLASS_RESTART_AUTHORIZATION, GITEA_MCP_PROFILE, GITEA_MCP_SESSION_STATE_DIR and GITEA_ACCESS_TOKEN. Those admitted keys were traced to mcp_daemon_guard.py:35, mcp_daemon_guard.py:36 and master_parity_gate.py:64. GITEA_BYPASS_NATIVE_MCP was found only in tests/test_structured_auth_mcp_errors.py:377 with no production consumer and is therefore explicitly not claimed as a live bypass. AC3 precedence was confirmed by pre-seeding target_env with a sanctioned GITEA_ACTIVE_WORKTREE value and observing it survive. Key normalisation was checked across lowercase, trailing-whitespace, suffix-pattern and bare-WORKTREE inputs. Focused suite at head: 7 passed. Neighbouring sweep at head: 7 failed, 798 passed, 4717 deselected, 271 subtests. Same sweep at the merge base in branches/baseline-pr922-2b4e430-s2, run in a command block carrying its own cd, pwd and git rev-parse HEAD: 7 failed, compared with comm in both directions and found identical, so no regression originates from this branch. No full-suite run was performed at this head and none is claimed. No file was edited in either worktree. Pushes during validation: none. LAST_UPDATED_BY: sysadmin / prgs-reviewer / gitea-reviewer namespace, reviewer lease session 18879-99a65ed7d43a

sysadmin commented

2026-07-27 11:07:39 -05:00

repo: Scaled-Tech-Consulting/Gitea-Tools
pr: #922
issue: #704
reviewer_identity: sysadmin
profile: prgs-reviewer
session_id: 18879-99a65ed7d43a
worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr922-ef34d93-s2
phase: released
candidate_head: ef34d938bf
target_branch: master
target_branch_sha: c30b381eb2
last_activity: 2026-07-27T16:07:38Z
expires_at: 2026-07-27T16:17:38Z
blocker: manual-release

repo: Scaled-Tech-Consulting/Gitea-Tools pr: #922 issue: #704 reviewer_identity: sysadmin profile: prgs-reviewer session_id: 18879-99a65ed7d43a worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr922-ef34d93-s2 phase: released candidate_head: ef34d938bf419b4fc0f9a0c9efbd5c7f762a1a01 target_branch: master target_branch_sha: c30b381eb2756c4ba9cbb2bf7c3f2d6f31b701e7 last_activity: 2026-07-27T16:07:38Z expires_at: 2026-07-27T16:17:38Z blocker: manual-release

jcwalker3 added 1 commit 2026-07-28 08:38:34 -05:00

Merge branch 'master' into fix/issue-704-prevent-env-workspace-bindings 0d82f5ab26

You are not authorized to merge this pull request.

This pull request can be merged automatically.

This branch is out-of-date with the base branch

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/issue-704-prevent-env-workspace-bindings:fix/issue-704-prevent-env-workspace-bindings

git checkout fix/issue-704-prevent-env-workspace-bindings

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Scaled-Tech-Consulting/Gitea-Tools#922

fix(auth): prevent repository .env files from injecting workspace bindings (Closes #704) #922

Summary

Changes

1. gitea_auth.py

2. Unit Tests (tests/test_issue_704_prevent_env_workspace_bindings.py)

Relationship to PR #703 & Issue #702

Validation

Worktree / Branch Proof

Next Role

REQUEST_CHANGES — PR #922 at head ef34d938bf419b4fc0f9a0c9efbd5c7f762a1a01

What is correct

B1 — BLOCKER: the reserved set covers workspace bindings only, so AC9 does not hold — a repository .env can still forge native-transport provenance

Non-blocking observations

Validation

Canonical PR State

Checkout

1. `gitea_auth.py`

2. Unit Tests (`tests/test_issue_704_prevent_env_workspace_bindings.py`)

REQUEST_CHANGES — PR #922 at head `ef34d938bf419b4fc0f9a0c9efbd5c7f762a1a01`

B1 — BLOCKER: the reserved set covers workspace bindings only, so AC9 does not hold — a repository `.env` can still forge native-transport provenance