post-merge Strix 보안 경로 강화 - #209
Closed
seonghobae wants to merge 7 commits into
Closed
Conversation
Contributor
|
Caution Review failedFailed to post review comments Note
|
| Layer / File(s) | Summary |
|---|---|
Configuration & Encryption Key Validation .env.example, backend/core/config.py, backend/db/models.py, backend/tests/test_config.py, backend/tests/test_tenant_config_model.py |
ENCRYPTION_KEY validation added (Fernet-format, length, uniqueness, per-byte entropy, weak-key rejection). ALLOWED_SMTP_HOSTS defaults to a deny-all marker and ALLOWED_SMTP_PORTS are validated. allowed_browser_origins normalization added; Settings model now validates on finalize. get_fernet() validates key before constructing Fernet. .env.example documents key generation. |
Browser Session Auth & CSRF Enforcement backend/api/auth.py, backend/main.py, backend/tests/test_auth_real.py |
get_auth_context accepts Request, Authorization header, and naruon_session_token cookie; enforces Origin/Referer against ALLOWED_BROWSER_ORIGINS for cookie-based unsafe methods; introduces _extract_session_token, refactors signed-session verification, adds AuthContextResponse and GET /api/auth/context. CORS now derives allow_origins from settings. Tests cover cookie auth and origin rules. |
Frontend API Client & Cookie Credentials frontend/src/lib/api-client.ts, frontend/src/lib/api-client.test.ts, frontend/src/app/tasks/page.test.tsx, frontend/src/components/EmailDetail.test.tsx |
ApiClient no longer derives Authorization from stored tokens and always includes credentials: 'include'. getSessionToken()/getCurrentUserId() return null. Tests updated to seed legacy localStorage tokens separately and assert requests use credentials: include and omit Authorization and identity headers. |
Settings Page Auth Context & Layout frontend/src/app/settings/page.tsx, frontend/src/app/settings/page.test.tsx |
SettingsPage rewritten to fetch /api/auth/context, gate personal config load on auth, add AccountField wrapper for SMTP/IMAP inputs, include data-testid hooks and scroll containment inside DashboardLayout. Tests validate rendering and scroll containment. |
SMTP Egress & LLM Prompt Handling backend/services/email_client.py, backend/services/llm_service.py, backend/tests/test_llm_service.py |
Email client filters deny-all marker from allowlist parsing. draft_reply tightened: system prompt clarified to treat inputs as untrusted and avoid hidden-prompt disclosure; user message now JSON-encodes {drafting_instruction,email_body}. Tests verify system/user message separation and JSON serialization of untrusted inputs. |
Strix CI Gate – GitHub Models & Error Handling .github/workflows/strix.yml, scripts/ci/strix_quick_gate.sh |
Default Strix LLM provider set to GitHub Models (github/gpt-5.4 family) when unset; gate secret checks adjusted to enable/disable behavior; THRESHOLD_FINDING_DETECTED flag added to track threshold findings and enforce fail-closed on fallback success; GitHub-specific fallback model handling and GitHub API key propagation to child process when using github/* routes; classification of Gemini/GitHub BadRequest as model-route errors to enable fallback retries; PR-scan retry/batch tightened for pull_request_target. |
Strix Gate Testing – GitHub & Gemini Scenarios scripts/ci/test_strix_quick_gate.sh |
Test harness extended to validate GitHub Models defaults, ensure GitHub API key forwarding to child only, add fake scenarios for GitHub route errors and Gemini BadRequest/threshold variants, and cover PR budget/timeouts and fallback-blocking cases. |
Documentation & Governance Updates AGENTS.md, ARCHITECTURE.md, CHANGELOG.md, README.md, backend/README.md, docs/operations/auth-key-management.md, docs/operations/email-relay-proxy-boundary.md, docs/plans/2026-05-26-branding-settings-source-gap-roadmap.md |
Docs updated to require HttpOnly naruon_session_token cookie with credentials: include, strip caller identity headers and forbid localStorage/sessionStorage session tokens, enforce ALLOWED_BROWSER_ORIGINS for cookie writes, document SMTP deny-by-default and DNS/routability checks, strengthen ENCRYPTION_KEY requirements, and describe Strix GitHub Models routing and threshold rules. New roadmap added for branding/settings. |
Infrastructure & Cleanup .gitignore, Dockerfile, backend/tests/test_dockerfile_security.py |
.gitignore cleaned and duplicate entry removed; Dockerfile removed build deps (gcc,libpq-dev); new test asserts final Dockerfile does not install build-time dependencies. Numerous formatting-only changes across many files and tests are included. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- Seongho-Bae/naruon#205: Related Strix CI workflow PR-target batching and gate semantics changes.
- Seongho-Bae/naruon#161: Overlaps with Settings page and provider-management UI changes.
- Seongho-Bae/naruon#155: Prior ApiClient/session handling changes related to credentials/Authorization behavior.
Poem
🐰 I hopped through code to lock the gate,
Cookies tucked away, Origin checks great,
Fernet keys must sparkle, never weak,
SMTP doors closed until hosts we seek,
Models routed tidy, tests sing—secure and sleek!
✨ 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/postmerge-strix-security-20260522
Contributor
|
PR governance metadata gate is not ready for
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
Author
|
Superseded: Strix security hardening already landed on master and is included in #214. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
backend/core/config.py (1)
109-118: ⚡ Quick winConsider blocking IPv4/IPv6 loopback addresses in SMTP host validation.
The validation blocks
localhostandlocalhost.localdomainbut allows loopback IP addresses like127.0.0.1or::1. This could allow SSRF to local SMTP services.🛡️ Proposed fix to block loopback IPs
+from ipaddress import ip_address as parse_ip_address + +def _is_loopback_address(host: str) -> bool: + try: + return parse_ip_address(host).is_loopback + except ValueError: + return False + def validate_allowed_smtp_hosts_value(value: str) -> None: # ... existing code ... for host_value in host_values: if ( "*" in host_value or "://" in host_value or any(character in host_value for character in " \t\r\n/") or host_value in {"localhost", "localhost.localdomain"} + or _is_loopback_address(host_value) ): raise ValueError( "ALLOWED_SMTP_HOSTS must contain only explicit relay hostnames or public IPs" )🤖 Prompt for 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. In `@backend/core/config.py` around lines 109 - 118, The loop that validates ALLOWED_SMTP_HOSTS (iterate over host_values) currently blocks names like "localhost" but still permits loopback IPs (e.g., 127.0.0.1, ::1); update the validation inside the for host_value in host_values loop to detect and reject loopback IP addresses by attempting to parse host_value with the ipaddress module (ipaddress.ip_address) and, if parsing succeeds and address.is_loopback is True, raise the same ValueError; preserve existing checks for wildcards, schemes, whitespace and literal hostnames by only applying the ipaddress check for parseable IPs and catching ipaddress.AddressValueError for non-IP hostnames.backend/tests/test_dockerfile_security.py (1)
11-14: ⚡ Quick winConsider adding an assertion message for clearer test failures.
The assertion would be more debuggable with a custom error message indicating which package was found.
💬 Proposed enhancement
for package_name in ("gcc", "libpq-dev"): assert not re.search( rf"\bapt-get\s+install\b[^;&\n]*\b{re.escape(package_name)}\b", normalized_dockerfile, - ) + ), f"Found '{package_name}' in Dockerfile apt-get install command"🤖 Prompt for 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. In `@backend/tests/test_dockerfile_security.py` around lines 11 - 14, The test's negative assertion lacks a helpful failure message; modify the assertion around re.search(...) so it reports which package was found and where by capturing the search result (e.g., using a variable like match = re.search(..., normalized_dockerfile)) and then asserting match is None with a message that includes package_name and match.group(0) or normalized_dockerfile snippet; update the assertion in backend/tests/test_dockerfile_security.py that currently references package_name and normalized_dockerfile to provide this informative message.scripts/ci/test_strix_quick_gate.sh (2)
224-231: ⚡ Quick winStrengthen the GitHub child-env leak check.
This only proves
GITHUB_API_KEYis present in the GitHub Models path. A regression that also forwardsLLM_API_KEYor other parent-step secrets on that branch would still pass. Please log/assert the generic key path stays unset here too, since the existingruntime-env-forwardingcase does not exercise the GitHub-specific env wiring.Also applies to: 2112-2117
🤖 Prompt for 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. In `@scripts/ci/test_strix_quick_gate.sh` around lines 224 - 231, The current leak check only logs GITHUB_API_KEY; extend it to also log and assert the generic parent-step secret variables (e.g., LLM_API_KEY and any other upstream secret names used in the pipeline) into the same FAKE_STRIX_RUNTIME_ENV_LOG so regressions that forward parent-step secrets are caught; update the print/append block that references GITHUB_API_KEY and FAKE_STRIX_RUNTIME_ENV_LOG to include LLM_API_KEY (and any other relevant parent secrets) with the same "${VAR:-<unset>}" pattern and add an assertion in the test script after writing the log to fail if those generic secret entries are set, mirroring the existing check for runtime-env-forwarding.
4551-4595: ⚡ Quick winIsolate the Gemini fallback source in the new BadRequest cases.
These cases pass the same fallback list through
fallback_models, which also populatesSTRIX_VERTEX_FALLBACK_MODELS. If the gate accidentally consults the Vertex fallback list for Gemini BadRequest handling, these tests would still go green. Mirror the GitHub-specific setup here by unsetting Vertex/generic fallbacks and setting onlySTRIX_GEMINI_FALLBACK_MODELS.🤖 Prompt for 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. In `@scripts/ci/test_strix_quick_gate.sh` around lines 4551 - 4595, The new Gemini BadRequest test cases reuse the generic fallback list via the fallback_models argument which also populates STRIX_VERTEX_FALLBACK_MODELS—masking whether Gemini-specific fallback logic is used; update the four run_gate_case invocations ("gemini-primary-badrequest-fallback-success", "gemini-badrequest-low-report-reaches-fallback", "gemini-badrequest-threshold-report-blocks-fallback-success", "gemini-badrequest-inline-threshold-blocks-fallback-success") to unset or clear STRIX_VERTEX_FALLBACK_MODELS before running and instead set STRIX_GEMINI_FALLBACK_MODELS to the intended fallback string (e.g., "gemini/fallback-one gemini/fallback-two"), or change the call so it passes an explicit STRIX_GEMINI_FALLBACK_MODELS env value rather than using fallback_models; ensure fallback_models is not used to populate STRIX_VERTEX_FALLBACK_MODELS for these cases so the tests truly validate Gemini-only fallback behavior.
🤖 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.
Nitpick comments:
In `@backend/core/config.py`:
- Around line 109-118: The loop that validates ALLOWED_SMTP_HOSTS (iterate over
host_values) currently blocks names like "localhost" but still permits loopback
IPs (e.g., 127.0.0.1, ::1); update the validation inside the for host_value in
host_values loop to detect and reject loopback IP addresses by attempting to
parse host_value with the ipaddress module (ipaddress.ip_address) and, if
parsing succeeds and address.is_loopback is True, raise the same ValueError;
preserve existing checks for wildcards, schemes, whitespace and literal
hostnames by only applying the ipaddress check for parseable IPs and catching
ipaddress.AddressValueError for non-IP hostnames.
In `@backend/tests/test_dockerfile_security.py`:
- Around line 11-14: The test's negative assertion lacks a helpful failure
message; modify the assertion around re.search(...) so it reports which package
was found and where by capturing the search result (e.g., using a variable like
match = re.search(..., normalized_dockerfile)) and then asserting match is None
with a message that includes package_name and match.group(0) or
normalized_dockerfile snippet; update the assertion in
backend/tests/test_dockerfile_security.py that currently references package_name
and normalized_dockerfile to provide this informative message.
In `@scripts/ci/test_strix_quick_gate.sh`:
- Around line 224-231: The current leak check only logs GITHUB_API_KEY; extend
it to also log and assert the generic parent-step secret variables (e.g.,
LLM_API_KEY and any other upstream secret names used in the pipeline) into the
same FAKE_STRIX_RUNTIME_ENV_LOG so regressions that forward parent-step secrets
are caught; update the print/append block that references GITHUB_API_KEY and
FAKE_STRIX_RUNTIME_ENV_LOG to include LLM_API_KEY (and any other relevant parent
secrets) with the same "${VAR:-<unset>}" pattern and add an assertion in the
test script after writing the log to fail if those generic secret entries are
set, mirroring the existing check for runtime-env-forwarding.
- Around line 4551-4595: The new Gemini BadRequest test cases reuse the generic
fallback list via the fallback_models argument which also populates
STRIX_VERTEX_FALLBACK_MODELS—masking whether Gemini-specific fallback logic is
used; update the four run_gate_case invocations
("gemini-primary-badrequest-fallback-success",
"gemini-badrequest-low-report-reaches-fallback",
"gemini-badrequest-threshold-report-blocks-fallback-success",
"gemini-badrequest-inline-threshold-blocks-fallback-success") to unset or clear
STRIX_VERTEX_FALLBACK_MODELS before running and instead set
STRIX_GEMINI_FALLBACK_MODELS to the intended fallback string (e.g.,
"gemini/fallback-one gemini/fallback-two"), or change the call so it passes an
explicit STRIX_GEMINI_FALLBACK_MODELS env value rather than using
fallback_models; ensure fallback_models is not used to populate
STRIX_VERTEX_FALLBACK_MODELS for these cases so the tests truly validate
Gemini-only fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66ff7d1f-58e9-4acc-9f02-814f50dab44c
⛔ Files ignored due to path filters (29)
frontend/branding/brand_assets/1.pngis excluded by!**/*.pngfrontend/branding/brand_assets/2.pngis excluded by!**/*.pngfrontend/branding/brand_assets/3.pngis excluded by!**/*.pngfrontend/branding/brand_assets/4.pngis excluded by!**/*.pngfrontend/branding/brand_assets/5.pngis excluded by!**/*.pngfrontend/branding/brand_assets/6.pngis excluded by!**/*.pngfrontend/branding/naruon_branding.pngis excluded by!**/*.pngfrontend/branding/uiux/10.pngis excluded by!**/*.pngfrontend/branding/uiux/11.pngis excluded by!**/*.pngfrontend/branding/uiux/12.pngis excluded by!**/*.pngfrontend/branding/uiux/13.pngis excluded by!**/*.pngfrontend/branding/uiux/14.pngis excluded by!**/*.pngfrontend/branding/uiux/15.pngis excluded by!**/*.pngfrontend/branding/uiux/16.pngis excluded by!**/*.pngfrontend/branding/uiux/17.pngis excluded by!**/*.pngfrontend/branding/uiux/18.pngis excluded by!**/*.pngfrontend/branding/uiux/19.pngis excluded by!**/*.pngfrontend/branding/uiux/20.pngis excluded by!**/*.pngfrontend/branding/uiux/21.pngis excluded by!**/*.pngfrontend/branding/uiux/22.pngis excluded by!**/*.pngfrontend/branding/uiux/9.pngis excluded by!**/*.pngfrontend/branding/uiux/uiux1.pngis excluded by!**/*.pngfrontend/branding/uiux/uiux2.pngis excluded by!**/*.pngfrontend/branding/uiux/uiux3.pngis excluded by!**/*.pngfrontend/branding/uiux/uiux4.pngis excluded by!**/*.pngfrontend/branding/uiux/uiux5.pngis excluded by!**/*.pngfrontend/branding/uiux/uiux6.pngis excluded by!**/*.pngfrontend/branding/uiux/uiux7.pngis excluded by!**/*.pngfrontend/branding/uiux/uiux8.pngis excluded by!**/*.png
📒 Files selected for processing (31)
.env.example.github/workflows/strix.yml.gitignoreAGENTS.mdARCHITECTURE.mdCHANGELOG.mdDockerfileREADME.mdbackend/README.mdbackend/api/auth.pybackend/core/config.pybackend/db/models.pybackend/main.pybackend/services/email_client.pybackend/services/llm_service.pybackend/tests/test_auth_real.pybackend/tests/test_config.pybackend/tests/test_dockerfile_security.pybackend/tests/test_llm_service.pybackend/tests/test_tenant_config_model.pydocs/operations/auth-key-management.mddocs/operations/email-relay-proxy-boundary.mddocs/plans/2026-05-26-branding-settings-source-gap-roadmap.mdfrontend/src/app/settings/page.test.tsxfrontend/src/app/settings/page.tsxfrontend/src/app/tasks/page.test.tsxfrontend/src/components/EmailDetail.test.tsxfrontend/src/lib/api-client.test.tsfrontend/src/lib/api-client.tsscripts/ci/strix_quick_gate.shscripts/ci/test_strix_quick_gate.sh
💤 Files with no reviewable changes (1)
- Dockerfile
…-security-20260522 # Conflicts: # .github/workflows/strix.yml # .gitignore # AGENTS.md # backend/main.py # frontend/src/app/settings/page.tsx # frontend/src/app/tasks/page.test.tsx
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| @@ -1,20 +1,21 @@ | |||
| from unittest.mock import AsyncMock, patch | |||
| from unittest.mock import AsyncMock, MagicMock, patch | |||
Contributor
Author
PR 게이트 차단 증거현재 head
보안 check를 숨기거나 admin merge하지 않고, base trusted Strix workflow가 현재 PR의 수정 사항을 반영할 수 없는 bootstrap blocker로 기록합니다. 다음 안전한 해소 경로는 repo 정책에 따른 임시 ruleset 조정 후 merge 및 즉시 복구, 또는 base branch에 동일 Strix workflow fix를 먼저 land하는 것입니다. |
Contributor
Author
|
Closing this PR as stale/unmergeable against current Current evidence:
Preserved follow-up notes:
This is a stale PR cleanup, not a security-check bypass. Required Strix remains enforced on active PRs. |
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No linked issue.
목적
PR #202 병합 이후
master의 Strix 보안 검사 실패를 후속 보완합니다.주요 변경
Authorization대신 HttpOnlynaruon_session_tokencookie +credentials: include를 사용합니다.Authorizationheader를 제거합니다.Origin/Referer가ALLOWED_BROWSER_ORIGINS와 일치해야 하며, production은 localhost/loopback allowlist를 거부합니다./api/auth/context를 추가해 frontend가 header spoofing 없이 현재 user context를 조회합니다.gcc/libpq-dev설치를 제거하고 Dockerfile regression을 추가했습니다.검증
uvx black --check backend/api/auth.py backend/core/config.py backend/main.py backend/services/llm_service.py backend/tests/test_auth_real.py backend/tests/test_llm_service.py backend/tests/test_dockerfile_security.py && uvx ruff check ...passed.DISABLE_BACKGROUND_WORKERS=1 uv run pytest backend/tests/test_llm_service.py backend/tests/test_dockerfile_security.py backend/tests/test_auth_real.py backend/tests/test_llm_api.py→ 60 passed.npm test -- src/lib/api-client.test.ts src/app/tasks/page.test.tsx src/components/EmailDetail.test.tsx && npm run lint -- src/lib/api-client.ts src/lib/api-client.test.ts src/app/tasks/page.test.tsx src/components/EmailDetail.test.tsx src/app/settings/page.tsx && npm run typecheckpassed.docker build --platform linux/amd64 -t naruon-backend:security-check-amd64 -f Dockerfile . && docker run --rm naruon-backend:security-check-amd64 sh -c '! command -v gcc && ! dpkg -s libpq-dev >/dev/null 2>&1'passed.git diff --checkpassed.참고
Failed post-merge Strix run: https://github.com/Seongho-Bae/naruon/actions/runs/26262389253
Summary by CodeRabbit
New Features
Security
Improvements
UI