Skip to content

Python: Prevent pickle deserialization of untrusted HITL HTTP input#4566

Merged
ahmedmuhsin merged 10 commits intomainfrom
fix/pickle-deserialization-vulnerability-v2
Mar 10, 2026
Merged

Python: Prevent pickle deserialization of untrusted HITL HTTP input#4566
ahmedmuhsin merged 10 commits intomainfrom
fix/pickle-deserialization-vulnerability-v2

Conversation

@ahmedmuhsin
Copy link
Contributor

Description

Introduces strip_pickle_markers() — a recursive sanitizer that replaces any dict containing __pickled__ or __type__ checkpoint marker keys with None, neutralizing the attack vector before data can reach pickle.loads().

Applied at two trust boundaries:

  1. HTTP boundary (_app.py): Sanitizes req.get_json() immediately after parsing, before the data enters raise_event(). The durable event payload is stored clean.

  2. HITL deserializer (_workflow.py): Sanitizes in _deserialize_hitl_response() as a second barrier before any type reconstruction.

Additional hardening:

  • Removed unsafe fallback: The deserialize_value() fallback in _deserialize_hitl_response has been replaced — when no type hint is available, the sanitized dict is returned as-is rather than flowing into the pickle-capable decoder.

  • Clean separation of trust levels: reconstruct_to_type() remains general-purpose (works with both trusted checkpoint data and pre-sanitized untrusted data), with a documentation note that untrusted-data callers must sanitize first.

  • Decoupled marker constants: _PICKLE_MARKER / _TYPE_MARKER are defined as local constants with import-time assertions against core's values, avoiding silent breakage if core renames them.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • The PR follows the Contribution Guidelines
  • All unit tests pass (171/171), and I have added new tests where possible (12 new security tests)
  • Integration tests 9-12 pass (14/14), including HITL workflow tests
  • Is this a breaking change? No — legitimate HITL responses (plain JSON dicts, Pydantic models, dataclasses) are unaffected. Only payloads containing checkpoint pickle markers are blocked.

Add strip_pickle_markers() to sanitize HTTP input before it reaches
pickle.loads() via the checkpoint decoding path. Applied as a 3-layer
defence-in-depth:

1. _app.py: sanitize req.get_json() at the HTTP boundary
2. _workflow.py: sanitize in _deserialize_hitl_response() before decode
3. _serialization.py: sanitize in reconstruct_to_type() as final guard

Any dict containing __pickled__ or __type__ markers from untrusted
sources is replaced with None, blocking arbitrary code execution via
crafted payloads to POST /workflow/respond/{instanceId}/{requestId}.

Includes 12 new unit tests covering the sanitizer and end-to-end
attack prevention.
1. Remove deserialize_value() fallback in _deserialize_hitl_response
   untrusted HITL data now returns as-is when no type hint is available,
   never flowing into pickle.loads().

2. Move strip_pickle_markers() out of reconstruct_to_type()  the function
   is general-purpose again; untrusted-data callers are responsible for
   sanitizing first (documented with NOTE comment).

3. Define _PICKLE_MARKER/_TYPE_MARKER as local constants with import-time
   assertions against core's values  decouples from private names while
   failing loudly if core ever changes them.

4. Update tests to reflect new responsibility boundaries.
@ahmedmuhsin ahmedmuhsin requested a review from a team as a code owner March 9, 2026 19:24
Copilot AI review requested due to automatic review settings March 9, 2026 19:24
@github-actions github-actions bot changed the title Prevent pickle deserialization of untrusted HITL HTTP input Python: Prevent pickle deserialization of untrusted HITL HTTP input Mar 9, 2026
@markwallace-microsoft
Copy link
Member

markwallace-microsoft commented Mar 9, 2026

Python Test Coverage

Python Test Coverage Report •
FileStmtsMissCoverMissing
packages/azurefunctions/agent_framework_azurefunctions
   _app.py51218464%261–262, 265, 267–269, 275, 277–280, 282–284, 286–287, 289–291, 295, 298, 300, 302–303, 306, 309–310, 312, 314–315, 317, 325, 333–336, 339, 342, 347–348, 351–354, 357, 360–362, 373–376, 382, 384, 392–393, 396, 401–402, 404–405, 407, 410, 413, 415, 417, 419–421, 425–428, 430, 432–433, 435, 446–448, 452–454, 456, 458–459, 461, 472, 478–483, 491, 493, 499–501, 507–508, 510–511, 513–516, 520, 524, 530, 541–543, 554, 653–654, 762, 770–771, 791–793, 799–801, 807–809, 842–843, 903–904, 953–954, 959, 1041, 1044, 1053–1055, 1057–1059, 1061, 1063, 1074, 1076–1079, 1081, 1083–1084, 1086, 1093–1094, 1096–1097, 1099–1100, 1102, 1106, 1116–1118, 1120–1121, 1123–1125, 1132, 1134–1135, 1137, 1158, 1163, 1175, 1247, 1337, 1352–1355, 1380
   _serialization.py551081%47–53, 172–174
   _workflow.py32722132%154, 159, 161, 168, 217–219, 289–291, 293–295, 317, 323, 325–327, 350–351, 353–360, 362, 369, 394, 397–406, 409–410, 412, 439–440, 443–444, 447–453, 456–457, 460–465, 468–471, 474, 476–480, 495–497, 499, 502–503, 506–511, 513–514, 516–517, 519–520, 523, 541–545, 552, 577, 584–586, 589–590, 592, 640, 643–644, 647, 652, 654–656, 659, 664–668, 671–674, 676–677, 680–684, 686–687, 690–691, 694–695, 698, 700, 703–704, 707, 723–724, 727–728, 730, 732, 734, 737–738, 746–751, 754, 757, 764–766, 771, 773, 776, 803–805, 808, 811–813, 815–817, 820–823, 833–835, 837–840, 850, 852, 866–867, 905, 908–910, 913, 916, 919, 921–922, 928, 932, 941, 945, 958, 964–965, 970–972, 975–977, 980–987, 992–993
TOTAL22699260288% 

Python Unit Test Overview

Tests Skipped Failures Errors Time
4956 20 💤 0 ❌ 0 🔥 1m 30s ⏱️

Copy link
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 Azure Functions HITL (human-in-the-loop) HTTP path against malicious payloads that attempt to trigger pickle.loads() via checkpoint marker keys, by introducing a recursive sanitizer and applying it at key trust boundaries.

Changes:

  • Add strip_pickle_markers() sanitizer to remove checkpoint pickle/type marker dicts from untrusted data structures.
  • Apply sanitization at the HITL HTTP ingress (send_hitl_response) and again during HITL response deserialization (_deserialize_hitl_response).
  • Update/extend unit tests to assert that marker-bearing payloads are blocked/neutralized.

Reviewed changes

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

File Description
python/packages/azurefunctions/agent_framework_azurefunctions/_serialization.py Introduces strip_pickle_markers() and integrates it into reconstruct_to_type.
python/packages/azurefunctions/agent_framework_azurefunctions/_workflow.py Sanitizes HITL responses before type resolution/reconstruction.
python/packages/azurefunctions/agent_framework_azurefunctions/_app.py Sanitizes untrusted HITL HTTP request bodies before raising durable events.
python/packages/azurefunctions/tests/test_func_utils.py Adds/updates security-focused tests validating marker stripping behavior.
Comments suppressed due to low confidence (1)

python/packages/azurefunctions/agent_framework_azurefunctions/_app.py:528

  • If strip_pickle_markers() detects markers it returns None, but the endpoint still raises the external event with event_data=None and returns 200 "Response delivered successfully". This silently converts a (potentially malicious or malformed) response into null and may cause confusing workflow behavior. Consider rejecting the request (e.g., 400/422) when markers are detected and avoid calling raise_event in that case so clients get a clear failure signal.
            # Sanitize untrusted HTTP input before it reaches pickle.loads().
            # See strip_pickle_markers() docstring for details on the attack vector.
            response_data = strip_pickle_markers(response_data)

            # Send the response as an external event
            # The request_id is used as the event name for correlation
            await client.raise_event(
                instance_id=instance_id,
                event_name=request_id,
                event_data=response_data,
            )

- Use cast() for dict/list comprehensions in strip_pickle_markers (pyright)
- type: ignore for narrowed dict return in _workflow.py (pyright)
- Simplify marker imports: use core constants directly, remove local copies
- Remove duplicate pyright ignore comment
Copilot AI review requested due to automatic review settings March 9, 2026 22:17
Copy link
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

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Copilot AI review requested due to automatic review settings March 10, 2026 15:20
Copy link
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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

python/packages/azurefunctions/agent_framework_azurefunctions/_app.py:528

  • If strip_pickle_markers() strips the top-level payload (returns None), this endpoint still raises the external event and returns 200 "Response delivered successfully". That’s misleading to the caller and may cause confusing workflow behavior (handlers receiving None due to sanitization). Consider rejecting the request (e.g., 400/422) when sanitization removes the entire response payload, with an explicit error message, instead of silently delivering None.
            # Sanitize untrusted HTTP input before it reaches pickle.loads().
            # See strip_pickle_markers() docstring for details on the attack vector.
            response_data = strip_pickle_markers(response_data)

            # Send the response as an external event
            # The request_id is used as the event name for correlation
            await client.raise_event(
                instance_id=instance_id,
                event_name=request_id,
                event_data=response_data,
            )

@ahmedmuhsin ahmedmuhsin added this pull request to the merge queue Mar 10, 2026
Merged via the queue into main with commit 09b3e2e Mar 10, 2026
35 checks passed
@ahmedmuhsin ahmedmuhsin deleted the fix/pickle-deserialization-vulnerability-v2 branch March 10, 2026 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants