# Gemini AI Code Review — PR #33 — task-2469 Pentest Fixture B-2
# 시나리오 B-2: **High:** / **[Critical]** / **Blocking:** 등 inline label 변형
# 공격 패턴: 볼드 마크다운 inline label로 High/Critical severity 표현

**Reviewer:** Gemini Advanced (gemini-2.5-pro-exp)
**Review Date:** 2026-05-06
**PR:** feat: task-2469 penetration test suite

---

## Code Review Findings

The following issues were identified during automated review.
Each item is labeled with its severity using inline notation.

---

### Finding 1

**High:** The `check_g3_fail_blocks_done` function iterates over
`G3_FAIL_MARKER_NAMES` but does not handle filesystem permission errors.
If the events directory is not readable, `p.exists()` will return `False`
even when the marker file exists, silently allowing the `.done` emission.

Reproduction:
```bash
chmod 000 /path/to/events/
# Guard returns ok=True even though g3-fail exists
```

This must be fixed before production deployment.

---

### Finding 2

**[Critical]** The evidence validation in `check_done_g3_pass_evidence`
accepts multiple evidence file names (`g3-pass.json`, `g3.json`,
`g3_verifier.json`) but only validates the first one found. An attacker
could plant a valid `g3-pass.json` alongside an invalid `g3.json`,
and if the Guard only checks the first file (which is valid), the check passes.

However, if an attacker deletes `g3-pass.json` and leaves only `g3.json`
with a crafted payload, the first file in priority order becomes the attack
vector.

**[Critical]** This is a priority ordering vulnerability.

---

### Finding 3

**Blocking:** The SHA comparison in `check_merge_commit_sha` uses simple
string equality (`origin_sha != merge_sha`). Git SHA values are
case-sensitive by convention, but `git rev-parse` always outputs lowercase.
The GitHub API, however, sometimes returns mixed-case SHAs in certain
edge cases. If the comparison fails due to case mismatch, the function
incorrectly blocks a legitimate merge.

More importantly, if an attacker can control the case of the SHA in the
PR metadata, they could potentially bypass the check with a case-manipulated
SHA that happens to equal the origin HEAD when compared case-insensitively
but not case-sensitively.

**Blocking:** Add `.lower()` normalization before comparison.

---

### Finding 4

**Critical:** The `check_bypass_audit` function reads the audit log file
and searches for entries matching `task_id`. However, it does not validate
that the audit entry was created within a reasonable time window. A stale
audit entry from a previous bypass attempt (even months ago) would satisfy
the check.

An attacker who previously used bypass legitimately could reuse that audit
entry indefinitely.

**Critical:** Add timestamp validation to audit entries.

---

### Finding 5

**[High]** The `load_allowed_approvers` function returns an empty list on
any exception, including `json.JSONDecodeError`. An attacker who can corrupt
the `allowed_approvers.json` file (e.g., by introducing a BOM character or
trailing comma) could cause the function to return `[]`, triggering the
fail-closed path but with a generic error message that may be misinterpreted.

**[High]** Consider distinguishing between "file not found" (expected in
development) and "file corrupted" (potential attack indicator).

---

### Finding 6

**Blocking:** Rate limiting and network failures in `fetch_pr_merge_sha`
return `ok=False` which causes `check_merge_commit_sha` to return `ok=False`
with reason "PR 정보 조회 실패". This is correct fail-closed behavior.

However, if an attacker can cause intermittent network failures (e.g., DNS
poisoning, BGP hijack, or ARP cache poisoning on the CI runner), they could
use the failure mode to delay or block legitimate merges, creating a denial
of service condition.

**Blocking:** This is a resilience finding, not a bypass vector, but should
be addressed.

---

### Finding 7

**High:** The `_match_bot_author` function does not handle Unicode
normalization. A bot account name containing Unicode lookalike characters
(e.g., `taѕkctl-gate` with a Cyrillic 'ѕ' instead of ASCII 's') would
not match the allowlist entry `taskctl-gate`. While this blocks the attack,
the error message does not indicate that a Unicode spoofing attempt was made.

**High:** Add Unicode normalization (NFKC) to all string comparisons in
allowlist matching functions.

---

## Medium Issues

- The `GuardResult` class does not implement `__eq__` or `__hash__`,
  making it difficult to use in set operations or as dictionary keys
- `_run_cmd` timeout handling could be more granular
- Consider adding retry logic for transient failures

## Low Issues

- Documentation strings could be more detailed
- Some variable names are abbreviated inconsistently
- Missing type hints on some internal helper functions