Skip to content

Fix protected branch Strix findings - #309

Merged
seonghobae merged 2 commits into
masterfrom
fix/strix-protected-branch-security-findings-20260529
May 29, 2026
Merged

Fix protected branch Strix findings#309
seonghobae merged 2 commits into
masterfrom
fix/strix-protected-branch-security-findings-20260529

Conversation

@seonghobae

@seonghobae seonghobae commented May 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • remove POP3 missing-secret credential-type disclosure from logs and raised errors
  • replace live smoke test url opener usage with explicit HTTP/HTTPS connections
  • pass Strix workflow expression values through step env before shell usage
  • document the repeated Strix bug patterns in AGENTS/README and the dated plan

Verification

  • PYTHONDONTWRITEBYTECODE=1 bash scripts/ci/test_strix_quick_gate.sh
  • PYTHONDONTWRITEBYTECODE=1 python3 -m pytest backend/tests/test_pop3_worker.py backend/tests/live/test_live_api_sequence.py -q
  • PYTHONDONTWRITEBYTECODE=1 python3 -m bandit -r backend/ -x backend/tests/ -q
  • PYTHONDONTWRITEBYTECODE=1 python3 -m bandit -r backend/tests/live/test_live_api_sequence.py --severity-level medium -q
  • PYTHONDONTWRITEBYTECODE=1 python3 -m pytest backend/tests -q
  • git diff --check

Strix Evidence

Addresses protected-branch Strix run 26648235234 on merge commit 1675651, which reported POP3 log disclosure, test url opener usage, and GitHub Actions shell interpolation.

Summary by CodeRabbit

  • Bug Fixes

    • Standardized error/log text to avoid revealing credential-type details.
    • Prevented direct interpolation of workflow expressions into shell conditionals; workflow values are passed via env.
    • Replaced broad URL-opener usage in test helpers with explicit HTTP client handling.
  • Tests

    • Added regression test ensuring broad URL-opener patterns are not used.
    • CI gate added assertions to verify workflow env-passing and safe shell checks.
  • Documentation

    • Updated governance guidance and added security findings and remediation plan; README clarified log wording.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 29, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 799ffa74-c2a5-4097-9628-bac2bc8c59bc

📥 Commits

Reviewing files that changed from the base of the PR and between 8f1e511 and 0646b8e.

📒 Files selected for processing (3)
  • backend/services/pop3_worker.py
  • backend/tests/live/test_live_api_sequence.py
  • backend/tests/test_pop3_worker.py

📝 Walkthrough

Walkthrough

This PR genericizes POP3 missing-credential logs/errors, moves GitHub Actions expression values into step env: before shell use, replaces broad urllib live-test helpers with explicit http.client requests and validation, and updates tests, CI gate checks, policy, and documentation to enforce these changes.

Changes

Security findings remediation: credential logging, workflow expressions, and HTTP client hardening

Layer / File(s) Summary
POP3 credential logging safety
backend/services/pop3_worker.py, backend/tests/test_pop3_worker.py, README.md
Pop3SyncWorker missing-credential branches now log and raise a standardized "POP3 account configuration incomplete" message. Tests assert the generic error message and that logs do not include credential-type labels (case-insensitive).
GitHub Actions shell expression hardening
.github/workflows/strix.yml, scripts/ci/test_strix_quick_gate.sh
Workflow steps export PROVIDER_MODE and IS_PR_EVIDENCE_RUN via env: and scripts consume those shell variables; CI assertions were added to ensure the workflow does not interpolate ${{ ... }} directly inside shell if conditions.
Live test HTTP helper security
backend/tests/live/test_live_api_sequence.py
read_json replaced urllib.request.urlopen with explicit http.client/HTTPSConnection usage, URL scheme/host validation, query-aware request path, timeout/retry handling, and connection cleanup. A regression test asserts absence of urllib.request/urlopen patterns.
Security policies and findings documentation
AGENTS.md, docs/plans/2026-05-29-strix-protected-branch-security-findings.md
Policy guidance expanded to forbid credential-type log labels, disallow interpolating workflow expressions inside run: commands, require passing expressions via env:, and ban broad URL-opener helpers in examples. A new findings doc records the issues and remediation steps.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

🐰 I hopped through logs where labels grew,
I swapped them out for phrases new.
Env-vars whisper, shells stay tame,
URL checks keep smoke tests sane.
A nibble of safety—hooray, we chew!

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'Fix protected branch Strix findings' directly and specifically describes the main change in the changeset—it addresses three security findings identified by Strix (POP3 credential disclosure, live test URL opener usage, and GitHub Actions shell interpolation).
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/strix-protected-branch-security-findings-20260529

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/services/pop3_worker.py`:
- Line 115: The RuntimeError raised in pop3_worker.py uses a credential-specific
message ("Missing POP3 username..."); change this to the same generic error
wording used by the password branch (e.g., "Account configuration incomplete for
user {config.user_id}") so no credential-type or secret-derived text appears in
exception messages; update the RuntimeError raised where config.user_id is
referenced to use the generic phrase to match the password branch's contract.

In `@backend/tests/live/test_live_api_sequence.py`:
- Around line 36-43: The current assert response.status == 200 can raise
AssertionError and bypass the retry except block; change the assertion to a
raised exception type that the retry loop already catches (e.g. raise
http.client.HTTPException(f"unexpected status {response.status}") when
response.status != 200) so transient non-200 responses trigger the existing
retry logic; update the block around response handling (the code that reads
response and closes connection) to perform this status check and raise
http.client.HTTPException instead of using assert so the except (OSError,
http.client.HTTPException) branch will retry.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7fee8247-e94d-4bb0-aa4d-fb1972ddf608

📥 Commits

Reviewing files that changed from the base of the PR and between 1675651 and 8f1e511.

📒 Files selected for processing (8)
  • .github/workflows/strix.yml
  • AGENTS.md
  • README.md
  • backend/services/pop3_worker.py
  • backend/tests/live/test_live_api_sequence.py
  • backend/tests/test_pop3_worker.py
  • docs/plans/2026-05-29-strix-protected-branch-security-findings.md
  • scripts/ci/test_strix_quick_gate.sh

Comment thread backend/services/pop3_worker.py Outdated
Comment thread backend/tests/live/test_live_api_sequence.py Outdated
@github-actions

github-actions Bot commented May 29, 2026

Copy link
Copy Markdown
Contributor

PR governance metadata gate is not ready for 0646b8e5b7cd8aaaae5e85aef3a0b5be0c5181c9:

  • Current-head CodeRabbit issue comment has blocking warning/failure evidence on 0646b8e.

@seonghobae
seonghobae temporarily deployed to fix/strix-protected-branch-security-findings-20260529 - naruon PR #309 May 29, 2026 17:45 — with Render Destroyed
@seonghobae
seonghobae merged commit ab03be5 into master May 29, 2026
16 of 18 checks passed
@seonghobae
seonghobae deleted the fix/strix-protected-branch-security-findings-20260529 branch May 29, 2026 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant