# Gemini AI Code Review — PR #33 — task-2469 Pentest Fixture B-1
# 시나리오 B-1: H2~H6 heading 변형으로 High/Critical severity 표현
# 공격 패턴: ## High, ### Critical, #### Blocking, ##### High Priority, ###### Critical Issue

**Reviewer:** Gemini Advanced (gemini-2.5-pro-exp)
**Review Date:** 2026-05-06
**PR:** feat: lifecycle guard penetration test implementation

---

## Overview

This PR introduces penetration tests for the lifecycle guard system.
Overall the code quality is acceptable, but several critical issues were identified
that must be addressed before merging.

---

## High

The `check_g3_fail_blocks_done` function does not handle the case where
the events directory is a symlink pointing to a different location. An attacker
could create a symlink to a directory containing no fail markers, effectively
bypassing the guard. This must be fixed before merge.

```python
# Problematic pattern — do NOT ignore
p = events_dir / f"{task_id}.{name}"
if p.exists():  # symlink traversal possible here
    found.append(p)
```

This is a security-relevant finding and requires immediate attention.

---

### Critical

The `check_done_g3_pass_evidence` function does not validate the JSON
schema of the evidence file beyond checking a few fields. A crafted evidence
file with unexpected types (e.g., `task_id: null`) could bypass the task_id
matching check since `None != task_id` evaluates differently from `"" != task_id`.

Specifically:
- `ev_task = ev.get("task_id", "")` — if task_id key exists but has value `null`,
  `ev_task` will be `None`, and `None and None != task_id` evaluates to `False`
  (short-circuit), meaning the mismatch check is skipped entirely.

This is a critical bypass vector.

---

### Critical: Severity Parser Code Block Stripping

The `strip_code_blocks` function uses a non-greedy `[\s\S]*?` match for fenced
code blocks. However, nested backtick sequences or code blocks with language
specifiers that contain severity keywords could still leak through if the
regex engine encounters edge cases with multi-byte characters.

Recommended fix: add a test with Korean text containing severity keywords
inside code blocks to verify correct stripping behavior.

---

#### Blocking

The `check_merge_commit_sha` function hardcodes a 30-second timeout for
the GitHub API call. In high-load CI environments, this timeout may be
insufficient, causing the function to return an error rather than a block
result. This could allow an attacker to cause timeouts to bypass the check.

Consider making the timeout configurable via environment variable:
```
TASKCTL_GH_API_TIMEOUT=60
```

---

#### Blocking: Missing Rate Limit Handling

The `_gh_api` function in `gemini_severity_parser.py` does not handle
GitHub API rate limiting (HTTP 403 with `X-RateLimit-Remaining: 0`).
When rate limited, the function returns `(rc, [])` which causes
`count_severities` to return all zeros, potentially allowing a PASS
when the actual review content contains High severity items.

This is a **Blocking** issue for production deployment.

---

##### High Priority

The `append_admin_override_audit` function uses `os.environ.get("USER", "unknown")`
to record the actor, which can be trivially spoofed by setting the USER
environment variable before calling the function. The actor field in audit
logs should use a more reliable identity source (e.g., git config user.email
or GitHub token identity).

---

##### High Priority: Race Condition in Marker Check

Between the time `check_g3_fail_blocks_done` checks for fail markers and the
time the `.done` file is actually written, another process could remove the
fail markers. This TOCTOU (Time-Of-Check-Time-Of-Use) race condition could
allow bypassing the guard in concurrent execution scenarios.

---

###### Critical Issue

The `_match_bot_author` function contains a logical error in the `app/` prefix
matching. The condition `pattern.startswith("app/") and author.startswith("app/")`
will match ANY `app/` prefixed author against ANY `app/` prefixed pattern,
regardless of the actual slug. For example, `pattern="app/legitimate-bot"` would
match `author="app/malicious-bot"`.

This is a **Critical Issue** that allows unauthorized bot authors to bypass
the allowlist check.

---

###### Critical Issue: JSON Schema Validation Missing

None of the Guard functions validate the JSON schema of their input files
(evidence files, allowlist files, audit log entries). An attacker could
provide a JSON file that is syntactically valid but semantically incorrect
(wrong field types, missing required fields treated as having default values)
to bypass validation checks.

Recommend adding schema validation using `jsonschema` library.

---

## Medium

The `check_state_transition` function dynamically imports `taskctl.py` via
`importlib`. If `taskctl.py` is not found, the function returns a fail-closed
result, but the error message is generic. Consider adding more specific error
messages to help debugging.

---

## Low

- Minor typos in comments in `lifecycle_guards.py`
- The `_now_iso()` function duplicates functionality available in other modules
- Consider using `pathlib.Path.resolve()` instead of direct path operations

---

## Summary

**Critical/Blocking Issues (must fix before merge):**
1. Symlink traversal in event directory scanning
2. Null task_id bypass in evidence validation
3. Code block stripping edge cases
4. Timeout-based bypass in SHA check
5. Rate limit handling in Gemini severity parser
6. app/ prefix matching logical error

**High Priority (fix soon):**
1. USER env spoofing in audit logs
2. TOCTOU race condition in marker check

**Medium/Low:**
1. Error message clarity
2. Code duplication
3. Minor style issues