fix: fetch_url must not follow redirects past the approval gate (desktop audit #4490)

[praisonai-triage-agent]

Summary

Addresses the highest-severity, fully self-contained finding from the desktop audit tracking issue #4504: #4490 — fetch_url follows redirects, so the approval card authorises one URL and the engine fetches another (audit rank 1 of 16, high).

urllib.request.urlopen follows 3xx redirects by default. After the user approves http://example.com/blog, a 302 Location: http://127.0.0.1:<engine port>/chats/<id> sends the fetch to an unapproved loopback service and returns its body into the model's context — bypassing the app's only human-in-the-loop control.

Change

• engine/server.py — fetch through build_opener(_NoRedirect); a redirect now raises a clear HTTPError ("redirect to … was not approved") that reaches the model, so it can re-request the new URL and get a second approval card.

Test

• engine/test_chat_stream.py::FetchUrlDoesNotFollowRedirects — stands up two loopback servers (A 302→ B, B returns a secret), sets approval_mode: never to allow the gate, fetches A, and asserts the secret does not come back. A second test asserts a direct (non-redirecting) fetch still works.
• Mutation-verified: reverting the one-line fix makes the test fail (the secret leaks); with the fix it passes.
• Full engine suite (test_chat_stream, test_portability): 64 passed, 0 regressions.

Scope note

#4504 tracks 14 separate audit defects. Per the repo's "minimal, focused fix" guidance, this PR fixes only the #1-ranked security finding. The remaining 13 remain tracked in their own issues.

Fixes #4490. Part of #4504.

Generated with Claude Code

[praisonai-triage-agent]

@coderabbitai review

[praisonai-triage-agent]

/review

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[greptile-apps]

Greptile Summary

The PR prevents fetch_url from following HTTP redirects beyond the URL approved by the user.

• Adds a redirect-denying HTTPRedirectHandler to the desktop engine’s built-in fetch tool.
• Strengthens the regression test to verify that an unapproved redirect target receives no request.
• Preserves direct, non-redirecting fetch behavior.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
src/praisonai-desktop/engine/server.py	Adds a redirect handler that rejects 3xx traversal after approval while retaining ordinary direct fetches.
src/praisonai-desktop/engine/test_chat_stream.py	The revised regression test records requests at the redirect target and now directly verifies the previously requested no-contact invariant.

_{Reviews (3): Last reviewed commit: "test: assert unapproved redirect target ..." | Re-trigger 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.

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/

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. If all issues are resolved, approve the PR / close the Issue
12. If blocking issues remain, request changes / leave clear action items

[praisonai-triage-agent]

Claude finished @MervinPraison's task —— View job

---

Final Architecture Review — PR #4522

• Read AGENTS.md & changed files
• Phase 1: Architecture review
• Phase 2: Evaluate reviewer feedback (Greptile)
• Phase 2: Fix valid issue (strengthen redirect test)
• Test (mutation-verified, full suite)
• Push & final verdict

---

Phase 1 — Architecture review

The production change in server.py:614-621 is correct, minimal, and well-scoped:

• build_opener(_NoRedirect) raises a clear HTTPError on any 3xx before the redirected request is issued — the model gets an actionable error and can re-request the new URL for a second approval card.
• Stdlib-only (no new deps), consistent with the engine's "starts in milliseconds" design.
• Desktop-scoped (src/praisonai-desktop/engine/) — does not touch praisonaiagents/praisonai core, no Agent params added, no hot-path impact.
• Backward compatible — direct (non-redirecting) fetches still work.

Protocol-driven ✅ · Backward compatible ✅ · No perf regression ✅ · Genuine security value (closes a real human-in-the-loop bypass) ✅ · No Agent bloat ✅

Phase 2 — Reviewer feedback

Greptile (P2, valid): the regression test asserted only that the secret body was absent from the result — so a broken impl that contacts the unapproved target and discards its response would still pass. Since the request itself is a side effect (can trigger actions, logging, rate limits), this needed to assert zero reachability.

Fix applied to test_chat_stream.py:

• Server B now records each request in b_hits.
• test_a_redirect_does_not_reach_the_unapproved_target asserts self.b_hits == [] in addition to the body check.

Mutation-verified: reverting the one-line server.py fix (build_opener(_NoRedirect) → urlopen) makes the strengthened test fail — it catches both the leaked body and the recorded hit. Restoring the fix → all green.

Test results: test_chat_stream 17 passed · test_portability 47 passed — 64 total, 0 regressions.

• Qodo: reviews paused (no actionable content).
• CodeRabbit/Gemini/Copilot: no substantive findings posted.

Files modified on this branch

File	Change
src/praisonai-desktop/engine/test_chat_stream.py	+9: record B's hits, assert the unapproved redirect target is never contacted

Skipped: server.py needed no change — the production fix is already correct and minimal.

Phase 3 — Verdict

✅ Approve (informally). The security fix is sound and the one valid reviewer concern is resolved with a mutation-verified test. No blocking issues remain. (I can't submit a formal GitHub approval for security reasons — please merge when ready.)

Job run · Branch claude/issue-4504-20260827-1352

[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]

Claude finished @MervinPraison's task —— View job

---

Rebase & Conflict Resolution — PR #4522 ✅

• Setup git identity & auth
• Fetch latest main & rebase onto origin/main
• Resolve conflicts (keep PR intent + newer main logic)
• Run targeted tests — 66 passed, 0 regressions
• Mutation-verified redirect test (b_hits == [] reachability assertion intact)
• Force-push with --force-with-lease

---

Key finding during rebase

While this PR was open, main merged #4506 (claude/issue-4490) — an identical _NoRedirect fix for the same issue #4490, touching the same two files. That's what caused the DIRTY state. Both conflicts were resolved keeping this PR's superior version:

File	Conflict	Resolution
engine/server.py	main + this PR both add the same _NoRedirect handler	Kept the identical fix plus this PR's 5-line explanatory comment (documents why redirects are refused). No functional duplication — net delta vs main is just the comment.
engine/test_chat_stream.py	both define FetchUrlDoesNotFollowRedirects (would duplicate the class)	Kept this PR's stronger test, dropped main's weaker copy. This PR's version adds the Greptile-requested reachability assertion (self.b_hits == [] — proves the unapproved target is never contacted, not just that its body is absent) and a positive direct-fetch test. main's version had neither.

Verification

• test_chat_stream + test_portability: 66 passed in 3.0s.
• FetchUrlDoesNotFollowRedirects: both tests pass, including the b_hits zero-reachability check.
• Final history is linear on top of 660bd52ce (current main).

Scope

Desktop-engine only (src/praisonai-desktop/engine/). No changes to praisonaiagents/praisonai core, no Agent params, no new deps — nothing added beyond resolving the conflict in this PR's favour.

Skipped: no logic merged in from main's duplicate — it was strictly a subset of this PR's work, so there was nothing newer to fold in.

Job run · Branch claude/issue-4504-20260827-1352

[praisonai-triage-agent]

Merge gate scan — eligible for assessment. Claude merge gate will assess and may auto-merge if MERGE_GATE_VERDICT: APPROVE.

[praisonai-triage-agent]

Merged by Claude PR merge gate (claude-merge-gate.yml).
Verdict: MERGE_GATE_VERDICT: APPROVE
SHA: 8eeb595
Method: merge

[praisonai-triage-agent]

Merge gate scan — not eligible for auto-merge.

• not open
• already merged by gate
• mergeState=UNKNOWN

Actions: wait for CI and the Claude review chain, or add label needs-manual-review and merge manually.
Opt out: label no-auto-merge.