[praisonai-triage-agent]

Fixes #5967

Changes

Gap 1: MCP OAuth (praisonaiagents/mcp/)

• oauth_discovery._require_https: still requires HTTPS, but now also runs tools.url_safety.is_safe_http_url on non-loopback hosts. This blocks https://169.254.169.254, https://10.x, https://[::ffff:169.254.169.254], https://0177.0.0.1, etc. Loopback (localhost/127.0.0.1/::1) is still allowed for local testing, as before.
• _get_json follows redirects manually (allow_redirects=False, max 3 hops). Each hop is checked before it is requested, so no request is sent to a disallowed redirect target.
• oauth_registration imports the same _require_https instead of keeping a duplicate copy.
• The three credential POSTs (code exchange, refresh, DCR) pass allow_redirects=False, so a 307/308 can't re-send a refresh token or client secret to another host.

Gap 2: web_crawl urllib fallback (tools/web_crawl_tools.py)

• Uses a no-redirect opener and the same per-hop SSRF loop as the httpx branch.

No new params, exports or dependencies.

Verification

• Issue PoC 1 now prints blocked for all three URLs and internal server was hit: [].
• New regression test test_web_crawl_urllib_fallback_does_not_follow_redirect_to_internal is a real-socket version of PoC 2. It fails on main and passes here.
• New tests for internal HTTPS hosts and pre-request redirect rejection. Existing example.com tests no longer need live DNS.
• tests/unit/mcp, the security tests and the web_crawl tests: 141 passed, 3 skipped. Full tests/unit: the failures on this branch are exactly the ones on main (missing optional deps in this environment); no new failures.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 11249881-85a2-4592-8c71-f0a957e68af8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[MervinPraison]

@coderabbitai review

[MervinPraison]

/review

[qodo-code-review]

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

[coderabbitai]

⚠️ Action not completed

Review skipped.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no new actionable issues were found.

Summary

The PR checks OAuth URLs before fetching metadata and checks each redirect before following it. The urllib crawler fallback now follows the same pattern.

• MCP OAuth checks each destination before contacting it.
• The urllib web crawler checks every redirect before fetching it.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Metadata or crawl URL] --> B{URL allowed?}
    B -->|No| C[Stop with error]
    B -->|Yes| D[Request without automatic redirects]
    D --> E{Redirect with Location?}
    E -->|Yes, within limit| B
    E -->|No| F[Read response]
    G[Credential POST] --> H[Disable redirects]
    H --> I{Response is 2xx?}
    I -->|No| C
    I -->|Yes| J[Read JSON and save result]

Loading

_{Reviews (3) · Last reviewed commit: "test: mock urllib opener in web_crawl si..." · Reviewed by Greptile}

[MervinPraison]

@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Review changes in this PR. Python SDK: praisonaiagents, praisonai. TypeScript SDK: src/praisonai-ts/. Do NOT modify src/praisonai-rust. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding.

MANDATORY READ (before reviewing):

• Always read src/praisonai-agents/AGENTS.md
• If this PR touches src/praisonai-ts/, also read src/praisonai-ts/AGENTS.md §2.1.2 (TS triage + PR review checklist)

Phase 1: Review per AGENTS.md

1. Protocol-driven: check heavy implementations vs core SDK
2. Backward compatible: ensure zero feature regressions
3. Performance: no hot-path regressions
4. SDK value: review in depth whether the change genuinely adds value to the SDK — never add features for the sake of adding them. It must strengthen the SDK (simpler, more user-friendly, robust, world-class, secure). If it does not clearly add value, request changes or recommend rejecting/closing rather than merging scope creep
5. Do not bloat the Agent class with additional params — only if absolutely required; we already support many params.
6. Repo routing: agent-callable tools → PraisonAI-Tools; lifecycle plugins → PraisonAI-Plugins; optional sandbox backends → PraisonAI-Plugins (praisonai.sandbox entry point) — request changes if wrongly added to praisonaiagents/
   6b. External tool examples (examples/tools/external/**): If the diff is confined to that path, mark SDK value ❌ for monorepo merge — redirect the contributor to PraisonAI-Tools; do not Approve for merge unless the PR has the maintainer-accept-example label.
   6c. New vendor *_tools.py in core (src/praisonai-agents/praisonaiagents/tools/): If the PR adds a new *_tools.py for a third-party API/SaaS/dataset (e.g. FXMacroData), mark Repo routing ❌ and SDK value ❌ — redirect to MervinPraison/PraisonAI-Tools. Do not Approve because legacy bundled tools (Tavily/Exa/You.com/etc.) already in core are not precedent for new vendor modules. Request changes or recommend closing unless the PR has maintainer-accept-core-tools.

MANDATORY COMMENT FORMAT — include this Phase 1 table in your review comment:

Phase 1 — AGENTS.md review

Check	Result
Protocol-driven / no heavy impl in core	✅ or ❌ + one-line rationale
Backward compatible	✅ or ❌ + one-line rationale
Performance (hot path)	✅ or ❌ + one-line rationale
SDK value	✅ or ❌ + one-line rationale (explicitly judge whether the change strengthens the SDK)
No Agent param bloat	✅ or ❌ + one-line rationale
Repo routing	✅ or ❌ + one-line rationale

For TypeScript PRs (src/praisonai-ts/), also add:
| TS types / parity / tests | ✅ or ❌ + one-line rationale (npm run build && npm test) |

Phase 2: FIX Valid Issues
7. For any VALID bugs or architectural flaws found by Gemini, CodeRabbit, Qodo, Copilot, or any other reviewer: implement the fix
8. Also independently identify and fix any gaps or issues you find in the changed code — do not rely only on prior reviewer feedback
9. Push all code fixes directly to THIS branch (do NOT create a new PR)
10. Comment a summary of exact files modified and what you skipped

Phase 3: Final Verdict
11. Never Approve or write “Approve for merge” if any Phase 1 row is ❌, if rule 6b/6c applies, or if the change belongs in PraisonAI-Tools/PraisonAI-Plugins. Use Request changes, redirect to the external repo, and recommend closing the monorepo PR when routing is wrong.
12. Approve only when every Phase 1 row is ✅ (including Repo routing and SDK value) and blocking issues are resolved; otherwise request changes with clear action items

[praisonai-triage-agent]

Phase 1 — AGENTS.md review

Check	Result
Protocol-driven / no heavy impl in core	✅ Small guard helpers in existing MCP OAuth/crawl modules; reuses url_safety
Backward compatible	✅ No public API change; only refuses unsafe hosts and non-2xx credential responses
Performance (hot path)	✅ Only on OAuth/crawl network calls; lazy imports kept
SDK value	✅ Closes SSRF and credential-leak paths (#5967)
No Agent param bloat	✅ agent.py untouched
Repo routing	✅ Hardens existing core modules; adds no new tool or plugin

Phase 2 — Fixes pushed (18eba52)

• Greptile P1 (valid): a redirect response overwrote saved tokens. Since allow_redirects=False, a 3xx reply got past raise_for_status() and was parsed as tokens. I added _require_success(resp) in mcp/oauth_discovery.py, next to _require_https. It raises on anything that isn't 2xx. The three credential POSTs now use it:
  • mcp/oauth_provider.py: code exchange and _try_refresh
  • mcp/oauth_registration.py: register_client
• Greptile P2 (valid): a DNS failure could hide a broken check. test_rejects_plaintext_advertised_endpoints now uses the public_dns fixture and asserts match="insecure transport". It can no longer pass just because DNS lookup fails.
• New regression test: test_refresh_redirect_keeps_saved_tokens. It fails without the fix and passes with it.

Files modified: praisonaiagents/mcp/oauth_discovery.py, praisonaiagents/mcp/oauth_provider.py, praisonaiagents/mcp/oauth_registration.py, tests/unit/mcp/test_oauth_discovery.py.

Tests: tests/unit/mcp, tests/unit/tools and test_security_audit_fixes.py give 899 passed. 5 failed, all in test_openapi_toolset_ssrf.py because httpx isn't installed in the runner; that code isn't touched here. The full tests/ -x run stopped at test_a2a because fastapi isn't installed.

Skipped: nothing else was raised. Qodo was billing-blocked and CodeRabbit gave no actionable items.

Phase 3 — Verdict

✅ Approve. Every Phase 1 row passes and both review findings are fixed.

[MervinPraison]

@claude this PR has merge conflicts with main. Please rebase onto latest main, resolve conflicts (keep this PR's intent, merge in newer main logic), run targeted tests, and force-push with --force-with-lease. Comment which files you resolved. Do not bloat the Agent class with additional params — only if absolutely required; we already support many params. Keep the resolution minimal — the aim is a lightweight and powerful package; do not introduce scope creep while resolving conflicts.

[praisonai-triage-agent]

Rebased onto main (after #5971) and resolved conflicts.

Resolved: src/praisonai-agents/praisonaiagents/tools/web_crawl_tools.py. The urllib fallback in this PR walks redirects by hand and checks SSRF on every hop. I kept that, and kept main's 5 MB cap (response.read(_MAX_CRAWL_BYTES)) inside the new loop. The httpx path is main's streaming version as-is; it already checks SSRF on every redirect.

Follow-up test fix: tests/unit/tools/test_web_crawl_size_cap.py::test_urllib_fallback_reads_only_up_to_cap mocked urllib.request.urlopen, which the fallback no longer calls. It now mocks urllib.request.build_opener and still checks that only _MAX_CRAWL_BYTES is read.

Tests: tests/unit/mcp, test_oauth_discovery.py, test_security_audit_fixes.py, test_security_outstanding_ghsas.py, test_web_crawl_size_cap.py and test_web_crawl_markdown.py: 142 passed, 6 skipped.

Skipped: none. The OAuth files rebased cleanly.

[MervinPraison]

@claude CI failed on HEAD ae0c5f26. Please fix the failures below and push to this branch.

Failed checks

• github-actions / main (3.11) — https://github.com/MervinPraison/PraisonAI/actions/runs/40075446564/job/120819565257

Failures (extracted)

1. tests/unit/test_yaml_headless_5826.py::test_no_metrics_output_without_flag — RuntimeError: 3 of 3 task(s) failed
   • Job: main (3.11)
2. tests/unit/test_yaml_headless_5826.py::test_output_file_and_metrics_json — RuntimeError: 3 of 3 task(s) failed
   • Job: main (3.11)

Critical review first

Before changing code or tests, decide which side is wrong:

1. Legitimate feature change — the PR intent is correct but implementation or tests need updating. Preserve SDK guarantees; update tests only when behaviour intentionally changed and document why.
2. Regression / bug in this PR — the failure exposes a real breakage introduced here. Fix the implementation; do not weaken, skip, or delete tests just to go green.
3. Pre-existing flake or unrelated failure — say so explicitly; prefer fixing the root cause over masking it.

SDK guardrails (AGENTS.md):

• Agent-callable tools → PraisonAI-Tools; lifecycle plugins → PraisonAI-Plugins; sandbox backends → PraisonAI-Plugins (praisonai.sandbox) — not praisonaiagents/.
• Do not disturb core SDK contracts to accommodate a wrapper/feature change.
• Tests must continue to guard backward compatibility and hot-path behaviour — passing CI by lowering test standards is not acceptable.
• If the feature does not genuinely add SDK value, recommend reverting or narrowing scope instead of patching around failures.

What to do

1. State your verdict: legitimate fix, regression fix, or needs human review — and why (1–3 sentences).
2. Fix root cause with minimal changes; never bloat the Agent class with extra params.
3. Run failing tests locally, e.g. pytest tests/unit/test_yaml_headless_5826.py::test_no_metrics_output_without_flag -q, plus any related SDK tests touched by the PR.
4. Push to this branch and comment: files changed, review verdict, and why tests still protect SDK behaviour.

[praisonai-triage-agent]

CI failure on ae0c5f26: pre-existing and unrelated to this PR

Verdict: pre-existing failure, not caused by this PR. I changed no code.

Root cause: the two failing tests in tests/unit/test_yaml_headless_5826.py fail on every LLM call with:

cannot import name 'CacheableResult' from 'mcp.types'

• litellm has only a lower bound (>=1.83.14), so CI now installs litellm 1.106.2.
• litellm 1.106.2 imports mcp.types.CacheableResult in litellm/experimental_mcp_client/client.py and in the proxy MCP server modules. That name only exists in mcp 2.x.
• src/praisonai-agents/pyproject.toml and src/praisonai/pyproject.toml both pin mcp<2, so CI gets mcp 1.30.0. The import fails, and all 3 tasks fail.

Proof that it's unrelated: main (3.11) on main fails the same two tests with the same error (run 39935479479, commit e22d74ae). The extended-ollama job on main hits the same import error too (run 40057679329). This PR only changes MCP OAuth, web_crawl_tools.py and their tests. It doesn't touch litellm, mcp pins, YAML or the framework adapter.

Why I didn't fix it here: the fix is a repo-wide dependency decision. One option is to cap litellm below the release that needs mcp 2.x; the other is to move to mcp>=2. Either change affects every package that depends on these, so it doesn't belong in an SSRF security PR. I recommend a separate small PR on main for the pin. I didn't weaken, skip or delete any test, so test_yaml_headless_5826.py keeps guarding that behaviour and will pass again once the pin is fixed.

Files changed: none.