Skip to content

Python: Harden HttpPlugin request validation#13969

Open
SergeyMenshykh wants to merge 5 commits into
microsoft:mainfrom
SergeyMenshykh:fix/python-http-plugin-ssrf
Open

Python: Harden HttpPlugin request validation#13969
SergeyMenshykh wants to merge 5 commits into
microsoft:mainfrom
SergeyMenshykh:fix/python-http-plugin-ssrf

Conversation

@SergeyMenshykh
Copy link
Copy Markdown
Member

Improve input validation and request handling in the Python HttpPlugin.

Changes

  • Add explicit opt-in for unrestricted domain access
  • Tighten URL validation logic
  • Improve redirect handling when domain restrictions are configured
  • Add regression tests

- Deny-all by default: add allow_all_domains flag requiring explicit opt-in
- Block redirects when allowed_domains is set to prevent domain bypass
- Add URL scheme validation (http/https only)
- Fix empty hostname bypass and redirect logic edge cases

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 8, 2026 10:11
@SergeyMenshykh SergeyMenshykh requested a review from a team as a code owner May 8, 2026 10:11
@moonbox3 moonbox3 added the python Pull requests for the Python Semantic Kernel label May 8, 2026
@SergeyMenshykh SergeyMenshykh self-assigned this May 8, 2026
@SergeyMenshykh SergeyMenshykh marked this pull request as draft May 8, 2026 10:12
@SergeyMenshykh SergeyMenshykh moved this to In Review in Agent Framework May 8, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens the Python HttpPlugin by making outbound HTTP requests opt-in (either via an allowlist of domains or an explicit “allow all domains” switch), tightening URL validation to only permit http/https, and adjusting redirect behavior to reduce SSRF bypass risk.

Changes:

  • Change default behavior to block all requests unless allowed_domains is provided or allow_all_domains=True.
  • Add scheme validation (only http/https) and disable redirects when domain restrictions are configured.
  • Update and expand unit tests to cover the new security behavior and regressions.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
python/semantic_kernel/core_plugins/http_plugin.py Enforces default-deny URL policy, restricts URL schemes, and controls redirect behavior based on domain configuration.
python/tests/unit/core_plugins/test_http_plugin.py Updates existing tests for the opt-in behavior and adds regression/security tests for default-deny, scheme filtering, and redirect handling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread python/semantic_kernel/core_plugins/http_plugin.py Outdated
Comment thread python/tests/unit/core_plugins/test_http_plugin.py Outdated
Comment thread python/semantic_kernel/core_plugins/http_plugin.py Outdated
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Automated Code Review

Reviewers: 4 | Confidence: 88%

✓ Correctness

The PR correctly implements default-deny, scheme validation, and an explicit allow_all_domains opt-in. The redirect-disable logic for domain-restricted configurations is sound. However, the allow_redirects expression in all four HTTP methods contains a vestigial or self.allowed_domains is None clause that carries forward old semantics (where allowed_domains=None meant 'allow all'), contradicting the new default-deny model. While currently unreachable because _validate_url blocks requests first, this expression would incorrectly enable redirects if the deny-all path is ever relaxed.

✓ Security Reliability

This PR is well-designed security hardening for the HttpPlugin. The default-deny posture, scheme validation (blocking file:/, ftp://, etc.), redirect disabling with domain restrictions (SSRF protection), and explicit opt-in for unrestricted access are all sound security improvements. The redirect logic (allow_redirects = self.allow_all_domains or self.allowed_domains is None) is correct across all configuration combinations — in the default case where both are falsy/None, _validate_url blocks before the redirect setting is ever used. No security or reliability issues found.

✓ Test Coverage

The PR adds strong regression tests for the new security defaults (deny-all, scheme blocking, redirect disabling). However, there is an asymetry in redirect-behavior coverage: the 'redirects disabled with allowed_domains' path is verified for all four HTTP methods (GET/POST/PUT/DELETE), but the 'redirects allowed with allow_all_domains=True' path is only verified for GET. Since each method independently computes allow_redirects on a separate line, a typo or copy-paste error in POST/PUT/DELETE would go undetected. This is a non-blocking gap.

✗ Design Approach

The redirect hardening mostly goes in the right direction, but the new flag precedence leaves one SSRF bypass open: when both allowed_domains and allow_all_domains=True are set, the request methods re-enable redirect following even though the class-level security contract says redirects must be disabled whenever allowed_domains is configured.


Automated review by SergeyMenshykh's agents

Comment thread python/tests/unit/core_plugins/test_http_plugin.py
SergeyMenshykh and others added 2 commits May 11, 2026 11:42
…26.0

azure-search-documents 12.0.0 removed the internal _endpoint attribute
from SearchIndexClient, breaking AzureAISearchCollection initialization.

onnxruntime 1.26.0 does not ship a macOS ARM64 wheel, breaking CI
on macos-latest runners.
See: microsoft/onnxruntime#28441

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Update Security docstring to clarify redirect behavior when
  allow_all_domains=True is set alongside allowed_domains.
- Centralize allow_redirects logic into _allow_redirects property,
  removing vestigial 'allowed_domains is None' clause.
- Rename test_allowed_domains_none_allows_all to
  test_allow_all_domains_allows_all to match actual behavior.
- Add redirect-allowed tests for POST, PUT, and DELETE methods
  with allow_all_domains=True for symmetric coverage.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@moonbox3
Copy link
Copy Markdown
Collaborator

moonbox3 commented May 11, 2026

Python Test Coverage

Python Test Coverage Report •
FileStmtsMissCoverMissing
core_plugins
   http_plugin.py570100% 
TOTAL28603564580% 

Python Unit Test Overview

Tests Skipped Failures Errors Time
3913 23 💤 0 ❌ 0 🔥 1m 51s ⏱️

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@SergeyMenshykh SergeyMenshykh marked this pull request as ready for review May 11, 2026 11:54
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Automated Code Review

Reviewers: 4 | Confidence: 92% | Result: All clear

Reviewed: Correctness, Security Reliability, Test Coverage, Design Approach


Automated review by SergeyMenshykh's agents

@SergeyMenshykh
Copy link
Copy Markdown
Member Author

Automated Code Review

Reviewers: 4 | Confidence: 88%

✓ Correctness

The PR correctly implements default-deny, scheme validation, and an explicit allow_all_domains opt-in. The redirect-disable logic for domain-restricted configurations is sound. However, the allow_redirects expression in all four HTTP methods contains a vestigial or self.allowed_domains is None clause that carries forward old semantics (where allowed_domains=None meant 'allow all'), contradicting the new default-deny model. While currently unreachable because _validate_url blocks requests first, this expression would incorrectly enable redirects if the deny-all path is ever relaxed.

✓ Security Reliability

This PR is well-designed security hardening for the HttpPlugin. The default-deny posture, scheme validation (blocking file:/, ftp://, etc.), redirect disabling with domain restrictions (SSRF protection), and explicit opt-in for unrestricted access are all sound security improvements. The redirect logic (allow_redirects = self.allow_all_domains or self.allowed_domains is None) is correct across all configuration combinations — in the default case where both are falsy/None, _validate_url blocks before the redirect setting is ever used. No security or reliability issues found.

✓ Test Coverage

The PR adds strong regression tests for the new security defaults (deny-all, scheme blocking, redirect disabling). However, there is an asymetry in redirect-behavior coverage: the 'redirects disabled with allowed_domains' path is verified for all four HTTP methods (GET/POST/PUT/DELETE), but the 'redirects allowed with allow_all_domains=True' path is only verified for GET. Since each method independently computes allow_redirects on a separate line, a typo or copy-paste error in POST/PUT/DELETE would go undetected. This is a non-blocking gap.

✗ Design Approach

The redirect hardening mostly goes in the right direction, but the new flag precedence leaves one SSRF bypass open: when both allowed_domains and allow_all_domains=True are set, the request methods re-enable redirect following even though the class-level security contract says redirects must be disabled whenever allowed_domains is configured.

Automated review by SergeyMenshykh's agents

There are two options for handling the case when both allow_all_domains=True and allowed_domains are set:

Option 1 (current): allow_all_domains=True takes full precedence - requests go to any domain and redirects are allowed. The rationale: if you've explicitly opted into unrestricted access, there's no reason to restrict redirects.
Setting both flags is an unusual configuration, and allow_all_domains is the stronger signal.

Option 2: allowed_domains always disables redirects, even when allow_all_domains=True. The rationale: the presence of allowed_domains means the user cares about domain control, so redirect protection should stay on.

I think option 1 makes more sense - allow_all_domains=True is an explicit "I trust all destinations" declaration, and selectively keeping redirect restrictions contradicts that intent. I've updated the docstring to clearly
document this precedence.

@cherry-bisht
Copy link
Copy Markdown

Hey genuine question. I reported this exact issue (allowed_domains=None allowing all domains by default, inconsistency with the .NET plugin) to MSRC on April 6 , Was this tracked internally from that report, or discovered independently? Just trying to understand how the process works

msrc said the Semantic Kernel Python HttpPlugin offers the ability to restrict the allowed_domains and the decision on which domains should be restricted is left to the user

@moonbox3 moonbox3 added this pull request to the merge queue May 11, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 11, 2026
@SergeyMenshykh SergeyMenshykh added this pull request to the merge queue May 12, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 12, 2026
@SergeyMenshykh SergeyMenshykh added this pull request to the merge queue May 12, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 12, 2026
@SergeyMenshykh SergeyMenshykh enabled auto-merge May 12, 2026 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Pull requests for the Python Semantic Kernel

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

5 participants