Security Fix PR Report#293
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 26 minutes and 22 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR implements comprehensive security hardening across authentication, supply chain, input validation, and exploit-style regression testing. It introduces Cloud Tasks OIDC callback verification, GitHub Actions SHA pinning, GitHub API SSRF guards, frontend markdown XSS sanitization, and new regression tests covering mass assignment and rate limiting attacks. ChangesSecurity Hardening Implementation
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
backend/app/services/intelligence/github/api_client.py (1)
20-24: 💤 Low value
_REPO_PATTERNallows..as a valid repository name.The pattern
[A-Za-z0-9._-]permits strings like..or...which, while likely rejected by GitHub's API, could cause unexpected URL path normalization (e.g.,/repos/owner/../normalizing to/repos/). Consider explicitly excluding consecutive dots or pure-dot strings.♻️ Optional: Reject consecutive dots in repo names
-_REPO_PATTERN = re.compile(r"^[A-Za-z0-9._-]{1,100}$") +# Reject names that are only dots or contain ".." +_REPO_PATTERN = re.compile(r"^(?!\.+$)(?!.*\.\.)[A-Za-z0-9._-]{1,100}$")🤖 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/app/services/intelligence/github/api_client.py` around lines 20 - 24, The _REPO_PATTERN currently allows strings like ".."; update its regex to explicitly reject consecutive dots and names made only of dots by adding negative lookaheads (e.g., a (?!.*\.\.) to forbid consecutive dots and a (?!^[.]+$) to forbid pure-dot names) while preserving the existing allowed characters and length constraints; modify the constant _REPO_PATTERN in api_client.py (and keep _OWNER_PATTERN unchanged) so repo validation fails for inputs containing ".." or only dots before any URL interpolation.backend/tests/security/test_ssrf_github.py (1)
34-41: 💤 Low valueMinor resource leak: event loop created but never closed.
Line 41 creates a new event loop that remains open until process exit. While not problematic for tests, a cleaner pattern would avoid creating the replacement loop or use
pytest-asynciodirectly.🤖 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/security/test_ssrf_github.py` around lines 34 - 41, The helper _run creates a new event loop then replaces it with another new loop (leaking one); to fix, avoid creating the replacement loop—either capture the original loop before creating the temporary one and restore it after closing (e.g., save original = asyncio.get_event_loop(); set the new loop to run coro; close it; asyncio.set_event_loop(original)), or simply remove the final asyncio.set_event_loop(asyncio.new_event_loop()) and call asyncio.set_event_loop(None) after closing; update the _run function accordingly.backend/app/services/tasks/cloud_tasks.py (1)
28-28: ⚡ Quick win
X-Internal-Secretis dead config unless the receiver enforces it.
CloudTasksDispatchernow sends this secret on every callback, but_verify_request()inbackend/app/routers/internal.pyonly checksX-CloudTasks-QueueNameplus OIDC. Right now this header adds secret-handling overhead without contributing to authorization, so either validate it server-side or drop it from the request contract.Also applies to: 35-38
🤖 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/app/services/tasks/cloud_tasks.py` at line 28, CloudTasksDispatcher is sending X-Internal-Secret but the server-side _verify_request() in backend/app/routers/internal.py doesn't validate it; either remove the header from CloudTasksDispatcher or add validation: read the same INTERNAL_SECRET env var (or internal_secret) in _verify_request() and compare it to the X-Internal-Secret header value (fail the request if missing/mismatched); update the verification path used by _verify_request() to include this header check alongside the existing X-CloudTasks-QueueName and OIDC checks so the header actually contributes to authorization.backend/tests/security/test_admin_authorization.py (1)
91-107: ⚡ Quick winAssert the expected audience in the patched OIDC verifier.
These doubles ignore the
audienceargument, so the tests still pass if_verify_cloud_tasks_oidc()stops forwardingCLOUD_TASKS_SERVICE_URLintoverify_oauth2_token(). That drops coverage for one of the main security properties added in this PR.Suggested fix
monkeypatch.setattr( "app.routers.internal.id_token.verify_oauth2_token", - lambda token, request, audience: { - "iss": "https://accounts.google.com", - "email": "attacker@example.iam.gserviceaccount.com", - "email_verified": True, - }, + lambda token, request, audience: ( + { + "iss": "https://accounts.google.com", + "email": "attacker@example.iam.gserviceaccount.com", + "email_verified": True, + } + if audience == "https://backend.example.com" + else (_ for _ in ()).throw(AssertionError(f"unexpected audience: {audience}")) + ), ) @@ monkeypatch.setattr( "app.routers.internal.id_token.verify_oauth2_token", - lambda token, request, audience: { - "iss": "https://accounts.google.com", - "email": "tasks@example.iam.gserviceaccount.com", - "email_verified": True, - }, + lambda token, request, audience: ( + { + "iss": "https://accounts.google.com", + "email": "tasks@example.iam.gserviceaccount.com", + "email_verified": True, + } + if audience == "https://backend.example.com" + else (_ for _ in ()).throw(AssertionError(f"unexpected audience: {audience}")) + ), )Also applies to: 119-135
🤖 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/security/test_admin_authorization.py` around lines 91 - 107, The patched OIDC verifier used in the tests (monkeypatch.setattr on app.routers.internal.id_token.verify_oauth2_token) ignores the audience argument so the test will pass even if _verify_cloud_tasks_oidc() stops passing CLOUD_TASKS_SERVICE_URL; update the test doubles to assert or validate that the incoming audience equals CLOUD_TASKS_SERVICE_URL (or raise an error if it does not) so the test fails when the verifier is called with the wrong audience, ensuring the CLOUD_TASKS_SERVICE_URL audience check in _verify_cloud_tasks_oidc() is covered.backend/tests/security/test_rate_limit.py (1)
23-35: ⚡ Quick winAlways reset the shared limiter in
finally.These tests mutate global rate-limit state and only clean it up on the success path. If one of the requests or assertions fails early, later tests can inherit a poisoned bucket and start failing nondeterministically.
Suggested fix
- limiter.reset() - statuses: list[int] = [] - for _ in range(8): - resp = client.post( - "/api/github-link/run", - json={"include_forks": False}, - headers=headers, - ) - statuses.append(resp.status_code) - if resp.status_code == 429: - break - assert 429 in statuses, f"429 が観測されなかった: {statuses}" - limiter.reset() + limiter.reset() + try: + statuses: list[int] = [] + for _ in range(8): + resp = client.post( + "/api/github-link/run", + json={"include_forks": False}, + headers=headers, + ) + statuses.append(resp.status_code) + if resp.status_code == 429: + break + assert 429 in statuses, f"429 が観測されなかった: {statuses}" + finally: + limiter.reset() @@ - limiter.reset() - statuses: list[int] = [] - for _ in range(13): - resp = client.post("/api/blog/accounts/nonexistent/sync", headers=headers) - statuses.append(resp.status_code) - if resp.status_code == 429: - break - assert 429 in statuses, f"429 が観測されなかった: {statuses}" - limiter.reset() + limiter.reset() + try: + statuses: list[int] = [] + for _ in range(13): + resp = client.post("/api/blog/accounts/nonexistent/sync", headers=headers) + statuses.append(resp.status_code) + if resp.status_code == 429: + break + assert 429 in statuses, f"429 が観測されなかった: {statuses}" + finally: + limiter.reset()Also applies to: 45-53
🤖 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/security/test_rate_limit.py` around lines 23 - 35, The test mutates global rate-limit state by calling limiter.reset() only on the success path; wrap the request loop and assertions in a try/finally and move the shared limiter.reset() into the finally block so the limiter is always reset even if a request or assertion fails; apply the same change to the second similar block (the one around lines 45-53) that uses client.post, statuses, and limiter.reset so both test sections always call limiter.reset() regardless of exceptions.
🤖 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 @.claude/skills/SEC_review/SKILL.md:
- Around line 34-35: The scan scopes currently limit checks to backend/**
frontend/** infra/** and thus omit workflow files; update the SKILL.md entries
that define the default "差分（デフォルト）" and the "全体（`full`）" scopes to also include
`.github/workflows/**` so GitHub Actions YAMLs are audited for SHA pinning
(i.e., add `.github/workflows/**` alongside backend/** frontend/** infra/** in
both scope definitions referenced in the diff text).
In @.github/workflows/ci.yml:
- Line 311: The workflow step using
actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 (the setup-node step
in the deploy-frontend* jobs) currently enables npm caching which can be
poisoned; remove the npm cache option (omit cache: npm) or explicitly disable
package-manager caching (set package-manager-cache: false) in those setup-node
steps referenced (the uses: actions/setup-node... occurrences at the
deploy-frontend* jobs) so deployment jobs that access secrets do not restore
untrusted cached npm data.
---
Nitpick comments:
In `@backend/app/services/intelligence/github/api_client.py`:
- Around line 20-24: The _REPO_PATTERN currently allows strings like "..";
update its regex to explicitly reject consecutive dots and names made only of
dots by adding negative lookaheads (e.g., a (?!.*\.\.) to forbid consecutive
dots and a (?!^[.]+$) to forbid pure-dot names) while preserving the existing
allowed characters and length constraints; modify the constant _REPO_PATTERN in
api_client.py (and keep _OWNER_PATTERN unchanged) so repo validation fails for
inputs containing ".." or only dots before any URL interpolation.
In `@backend/app/services/tasks/cloud_tasks.py`:
- Line 28: CloudTasksDispatcher is sending X-Internal-Secret but the server-side
_verify_request() in backend/app/routers/internal.py doesn't validate it; either
remove the header from CloudTasksDispatcher or add validation: read the same
INTERNAL_SECRET env var (or internal_secret) in _verify_request() and compare it
to the X-Internal-Secret header value (fail the request if missing/mismatched);
update the verification path used by _verify_request() to include this header
check alongside the existing X-CloudTasks-QueueName and OIDC checks so the
header actually contributes to authorization.
In `@backend/tests/security/test_admin_authorization.py`:
- Around line 91-107: The patched OIDC verifier used in the tests
(monkeypatch.setattr on app.routers.internal.id_token.verify_oauth2_token)
ignores the audience argument so the test will pass even if
_verify_cloud_tasks_oidc() stops passing CLOUD_TASKS_SERVICE_URL; update the
test doubles to assert or validate that the incoming audience equals
CLOUD_TASKS_SERVICE_URL (or raise an error if it does not) so the test fails
when the verifier is called with the wrong audience, ensuring the
CLOUD_TASKS_SERVICE_URL audience check in _verify_cloud_tasks_oidc() is covered.
In `@backend/tests/security/test_rate_limit.py`:
- Around line 23-35: The test mutates global rate-limit state by calling
limiter.reset() only on the success path; wrap the request loop and assertions
in a try/finally and move the shared limiter.reset() into the finally block so
the limiter is always reset even if a request or assertion fails; apply the same
change to the second similar block (the one around lines 45-53) that uses
client.post, statuses, and limiter.reset so both test sections always call
limiter.reset() regardless of exceptions.
In `@backend/tests/security/test_ssrf_github.py`:
- Around line 34-41: The helper _run creates a new event loop then replaces it
with another new loop (leaking one); to fix, avoid creating the replacement
loop—either capture the original loop before creating the temporary one and
restore it after closing (e.g., save original = asyncio.get_event_loop(); set
the new loop to run coro; close it; asyncio.set_event_loop(original)), or simply
remove the final asyncio.set_event_loop(asyncio.new_event_loop()) and call
asyncio.set_event_loop(None) after closing; update the _run function
accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e7b062c9-436b-4bc9-9eec-1ef96a70961b
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
.claude/skills/SEC_apply/SKILL.md.claude/skills/SEC_review/SKILL.md.github/workflows/ci.yml.github/workflows/opentofu-ci.ymlbackend/app/routers/internal.pybackend/app/services/intelligence/github/api_client.pybackend/app/services/tasks/cloud_tasks.pybackend/requirements.txtbackend/tests/security/test_admin_authorization.pybackend/tests/security/test_mass_assignment.pybackend/tests/security/test_rate_limit.pybackend/tests/security/test_ssrf_github.pydocs/api.mdfrontend/package.jsonfrontend/src/components/forms/MarkdownTextarea.test.tsxfrontend/src/components/forms/MarkdownTextarea.tsx
This was referenced Jul 2, 2026
10 tasks
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.
Summary
javascript:URL の回帰テストを追加。==pin 化。X-Internal-Secretと OIDC audience を送るよう補強。Applied Fixes
Critical
High
marked.parse()の HTML をDOMPurify.sanitize()に通してからdangerouslySetInnerHTMLに渡すよう修正。marked@v5+は sanitize オプション廃止のため<script>/onerror/javascript:が実行可能だった。（違反ルール: security.md「Frontend XSS 対策まとめ」「dangerouslySetInnerHTML は原則禁止 / Markdown レンダラーは sanitize 有効化」）dompurify@^3を追加（v3 は型同梱のため@types不要）し、frontend/package-lock.jsonに integrity 付き lock を反映。Medium
actions/checkout/actions/setup-node/actions/upload-artifact/dorny/paths-filter/astral-sh/setup-uv/cloudflare/wrangler-action/google-github-actions/auth/google-github-actions/setup-gcloudを full commit SHA に固定（# vNコメント付与）。actions/checkout/dorny/paths-filter/opentofu/setup-opentofuを full commit SHA に固定。>=を実インストール版へ固定:google-genai>=1.0.0→==1.46.0/pyasn1>=0.6.3→==0.6.3/python-multipart>=0.0.27→==0.0.27。google-cloud-tasks>=2.16,<3は上限ありのため据え置き。Low
_OWNER_PATTERN(^[A-Za-z0-9-]{1,39}$) /_REPO_PATTERN(^[A-Za-z0-9._-]{1,100}$) を追加し、API パス補間前に検証。/internal/tasks/*で Cloud Tasks OIDC token の audience と service account email を検証。X-Internal-Secretと OIDC audience（= CLOUD_TASKS_SERVICE_URL）を明示付与。Design-level Fixes
NonRetryableErrorで拒否し、../../や/を含むパス操作を遮断。対応テスト:test_ssrf_github.py::test_fetch_repos_raw_rejects_bad_username_without_http。test_ssrf_github.py::test_fetch_languages_skips_bad_owner_without_http。follow_redirects=Falseで従前から実質不成立。username は認証ユーザー自身のgithub_login由来で任意入力でないことも確認済み。本変更は多層防御。TASK_RUNNER=cloud_tasksではキューヘッダーだけでなく OIDC を必須化し、共有シークレット単独依存を緩和。対応テスト:test_admin_authorization.py::test_missing_cloud_tasks_oidc_returns_403/test_invalid_cloud_tasks_oidc_claims_return_403/test_valid_cloud_tasks_oidc_reaches_handler。InternalSecretMiddlewareが/health以外の全パスでX-Internal-Secretを timing-safe 検証・fail-closed、test_internal_secret_middleware.pyで担保）。IDOR/OAuth/CSRF/権限分離は従前から良好で実装修正なし。Dependency & Supply Chain
npm audit --audit-level=high/pip-auditで High 以上なし。google-genai==1.46.0/pyasn1==0.6.3/python-multipart==0.0.27（いずれも venv 実バージョンと一致確認済み）。rg -n 'uses: .*@(v[0-9]|main|master)' .github/workflowsは検出なし。全 9 件の SHA が実在し対応 major ref を指すことを RV で再確認（下表）。dompurifyのみ。Exploit Tests Added
javascript:無害化 / 通常 Markdown 描画維持）。NonRetryableError、client.get.assert_not_called()）。/api/github-link/run5/min・/api/blog/accounts/{id}/sync10/min）。user_id偽装しても所有権が移らない 4 ケース（base.createがサーバ側でuser_id=self.user_id固定）。Secrets Remediation
.gitignore追加: なし。Skipped / Follow-ups
get_environment()の fail-closed 化（設計ハードニング）:get_environment()は未設定時"local"を返し、localはInternalSecretMiddlewareを全スキップする fail-open。本番 infra はENVIRONMENTを必ず注入するため現状穴ではないが、設定漏れ時に認証が全無効化される。production既定（fail-closed）化はローカル DX に影響するため別途検討。brace-expansion/postcss/qs/ws経由）: 採用レポートおよび CI 閾値は High 以上のため本 PR では未更新。別 PR で更新検討。requirements.txtの hash 固定（uv lock / pip-tools）: 中期課題。CLOUD_TASKS_SERVICE_URLとCLOUD_TASKS_SERVICE_ACCOUNTが正しく注入されている前提。デプロイ後に Cloud Tasks 実行ログを確認する。Validation
make lint-backend: pass。make test-backend: pass（463 passed）。make lint-frontend: pass。make lint-frontend-messages: pass。make test-frontend: pass。make build-frontend: pass（dompurify 型解決含む）。make infra-fmt-check/make infra-validate（dev / stg / prod）: pass。npm audit --audit-level=high: pass（High 以上なし、moderate 6 件残あり）。pip-audit -r requirements.txt: pass（No known vulnerabilities found）。Review (RV) 結果 — 2026-05-30
統合にあたりワークツリーの実コードを再検証した。
コードレビュー
CLOUD_TASKS_SERVICE_URL）/ issuer（accounts.google.com）/ email（期待 SA 一致）/email_verified is Trueの 4 点を検証し、いずれか欠落で 403。dispatcher 側のoidc_token.audience = self._service_urlと一致しており整合。ValueErrorは warning ログ +False返却で握りつぶしなし。問題なし。fetch_repos_rawは事前 raise、補助系 3 関数は warning + 空返却でパイプライン継続。CLAUDE.md「黙って return 禁止」に対しても logger.warning を残しており準拠。問題なし。INTERNAL_SECRET/TASK_RUNNER/CLOUD_TASKS_SERVICE_URL/CLOUD_TASKS_SERVICE_ACCOUNTの 4 定数がenv_keys.pyに実在。リテラル直書きなし。問題なし。DOMPurify.sanitize()をdangerouslySetInnerHTMLの前段に挿入。問題なし。SHA 実在性検証（1042 が懸念した破損の確認）
git ls-remoteで全 9 件が実在し対応 major ref を指すことを確認（破損なし）:テスト再実行（RV 時）
pytest tests/security/: 104 passed（新規 OIDC / SSRF / rate-limit / mass-assignment 含む）。vitest run MarkdownTextarea.test.tsx: 4 passed。結論
両 PR レポートの記載は実コードと一致し、1042 で延期された 2 件（SHA 固定・OIDC 検証）は 1852 で正しく完了している。残課題は
get_environment()の fail-closed 化のみ。マージ可。Summary by CodeRabbit
Security Enhancements
Tests
Documentation
Dependency Management