Skip to content

Conditionally whitelist datetime.datetime and add tests #767

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 15, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Conditionally whitelist datetime.datetime and add tests
  • Loading branch information
dandavison committed Feb 14, 2025
commit fe6edf210a55e38558dbd76e2a32275e30cd4171
24 changes: 11 additions & 13 deletions temporalio/worker/workflow_sandbox/_restrictions.py
Original file line number Diff line number Diff line change
Expand Up @@ -953,19 +953,17 @@ def r_op(obj: Any, other: Any) -> Any:


def _is_restrictable(v: Any) -> bool:
return v is not None and not isinstance(
v,
(
bool,
int,
float,
complex,
str,
bytes,
bytearray,
datetime.date, # e.g. datetime.datetime
),
)
do_not_restrict = [bool, int, float, complex, str, bytes, bytearray]
if HAVE_PYDANTIC:
# The datetime validator in pydantic_core
# https://github.com/pydantic/pydantic-core/blob/741961c05847d9e9ee517cd783e24c2b58e5596b/src/input/input_python.rs#L548-L582
# does some runtime type inspection that a RestrictedProxy instance
# fails. For this reason we do not restrict date/datetime instances when
# Pydantic is being used. Other restricted types such as pathlib.Path
# and uuid.UUID which are likely to be used in Pydantic model fields
# currently pass Pydantic's validation when wrapped by RestrictedProxy.
do_not_restrict.append(datetime.date) # e.g. datetime.datetime
return v is not None and not isinstance(v, tuple(do_not_restrict))
Copy link
Contributor Author

@dandavison dandavison Feb 14, 2025

Choose a reason for hiding this comment

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

So datetime.datetime will be whitelisted for Pydantic v2 users, but not for v1 users. However, the __get_pydantic_core_schema__ impl doesn't do anything for v1 users, so they have to disable sandboxing of datetime.datetime entirely (e.g. per the sample), so _is_restrictable is irrelevant for them.

Copy link
Member

@cretz cretz Feb 14, 2025

Choose a reason for hiding this comment

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

Works for me. Though I know we don't prematurely optimize, I wonder if there are performance concerns for making list, appending to list, and turning to tuple every time this call is made if you also happen to have pydantic v2 in your venv. We always made the tuple before, so maybe it doesn't matter. Could make this tuple global if you wanted, but I'm ok with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I agree. Performance or not that code looked a bit silly. d915d5b



class _RestrictedProxy:
Expand Down
62 changes: 58 additions & 4 deletions tests/contrib/pydantic/test_pydantic.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import dataclasses
import datetime
import os
import pathlib
import uuid
from datetime import datetime

import pydantic
import pytest
Expand All @@ -9,6 +11,11 @@
from temporalio.client import Client
from temporalio.contrib.pydantic import pydantic_data_converter
from temporalio.worker import Worker
from temporalio.worker.workflow_sandbox._restrictions import (
RestrictionContext,
SandboxMatcher,
_RestrictedProxy,
)
from tests.contrib.pydantic.models import (
PydanticModels,
PydanticModelWithStrictField,
Expand Down Expand Up @@ -103,7 +110,7 @@ async def test_round_trip_misc_objects(client: Client):
{"7": 7.0},
[{"7": 7.0}],
({"7": 7.0},),
datetime(2025, 1, 2, 3, 4, 5),
datetime.datetime(2025, 1, 2, 3, 4, 5),
uuid.uuid4(),
)

Expand Down Expand Up @@ -262,7 +269,9 @@ async def test_datetime_usage_in_workflow(client: Client):

def test_pydantic_model_with_strict_field_outside_sandbox():
_test_pydantic_model_with_strict_field(
PydanticModelWithStrictField(strict_field=datetime(2025, 1, 2, 3, 4, 5))
PydanticModelWithStrictField(
strict_field=datetime.datetime(2025, 1, 2, 3, 4, 5)
)
)


Expand All @@ -276,7 +285,9 @@ async def test_pydantic_model_with_strict_field_inside_sandbox(client: Client):
workflows=[PydanticModelWithStrictFieldWorkflow],
task_queue=tq,
):
orig = PydanticModelWithStrictField(strict_field=datetime(2025, 1, 2, 3, 4, 5))
orig = PydanticModelWithStrictField(
strict_field=datetime.datetime(2025, 1, 2, 3, 4, 5)
)
result = await client.execute_workflow(
PydanticModelWithStrictFieldWorkflow.run,
orig,
Expand Down Expand Up @@ -324,3 +335,46 @@ async def test_validation_error(client: Client):
task_queue=task_queue_name,
result_type=tuple[int],
)


class RestrictedProxyFieldsModel(BaseModel):
path_field: pathlib.Path
uuid_field: uuid.UUID
datetime_field: datetime.datetime


def test_model_instantiation_from_restricted_proxy_values():
restricted_path_cls = _RestrictedProxy(
"Path",
pathlib.Path,
RestrictionContext(),
SandboxMatcher(),
)
restricted_uuid_cls = _RestrictedProxy(
"uuid",
uuid.UUID,
RestrictionContext(),
SandboxMatcher(),
)
restricted_datetime_cls = _RestrictedProxy(
"datetime",
datetime.datetime,
RestrictionContext(),
SandboxMatcher(),
)

restricted_path = restricted_path_cls("test/path")
restricted_uuid = restricted_uuid_cls(bytes=os.urandom(16), version=4)
restricted_datetime = restricted_datetime_cls(2025, 1, 2, 3, 4, 5)

assert type(restricted_path) is _RestrictedProxy
assert type(restricted_uuid) is _RestrictedProxy
assert type(restricted_datetime) is not _RestrictedProxy
p = RestrictedProxyFieldsModel(
path_field=restricted_path, # type: ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The restricted pathlib.Path instance is accepted by pydantic but the restricted datetime.datetime is not:

E       pydantic_core._pydantic_core.ValidationError: 1 validation error for RestrictedProxyFieldsModel
E       datetime_field
E         Input should be a valid datetime [type=datetime_type, input_value=datetime.datetime(2025, 1, 2, 3, 4, 5), input_type=_RestrictedProxy]
E           For further information visit https://errors.pydantic.dev/2.10/v/datetime_type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to be due to the fact that datetime has a custom validation method in Pydantic's Rust layer: https://github.com/pydantic/pydantic-core/blob/741961c05847d9e9ee517cd783e24c2b58e5596b/src/input/input_python.rs#L548-L582
and _RestrictedProxy does not pass the self.downcast::<PyDateTime> test which leads to the validation failure.

UUID does also: https://github.com/pydantic/pydantic-core/blob/741961c05847d9e9ee517cd783e24c2b58e5596b/src/validators/uuid.rs#L95-L154 but _RestrictedProxy does not cause it to fail. And pathlib.Path does not seem to have a dedicated validator in pydantic_core.

uuid_field=restricted_uuid, # type: ignore
datetime_field=restricted_datetime, # type: ignore
)
assert p.path_field == restricted_path
assert p.uuid_field == restricted_uuid
assert p.datetime_field == restricted_datetime
Loading