Pull request overview

This PR hardens Python declarative workflow state-path handling (primarily to prevent unsafe object attribute traversal) and aligns PowerFx temp-variable naming, with accompanying test updates/additions.

Changes:

• Restricts DeclarativeWorkflowState.get() object-attribute traversal to safe identifier-shaped segments and returns default when encountering unsafe/unreachable segments.
• Tightens interpolate_string() placeholder matching so only fully identifier-shaped dotted paths are interpolated; other braced tokens remain literal.
• Renames PowerFx long-MessageText() temp variables from _TempMessageText{n} to TempMessageText{n} and updates/extends tests accordingly.

Reviewed changes

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

Automated Code Review

Reviewers: 5 | Confidence: 92%

✓ Correctness

This PR correctly hardens the declarative workflow state against path traversal attacks (e.g., __class__.__init__.__globals__ chains) by validating path segments before allowing getattr access. The regex-based guard in get() only applies to the attribute-access path (not dict lookups), the interpolate_string regex is tightened to only match valid identifiers per segment, and the _TempMessageText variable is renamed to TempMessageText to comply with the new rules. The implementation is logically sound and well-tested.

✓ Security Reliability

This PR is a solid security fix that prevents attribute traversal attacks (e.g., obj.class.init.globals) through the declarative workflow state. The implementation correctly distinguishes between safe dict-key lookups (unrestricted) and dangerous getattr paths (validated against [A-Za-z][A-Za-z0-9_]*). The temp variable rename and interpolation regex tightening are consistent follow-ups. No security or reliability issues found.

✓ Test Coverage

The test coverage for this security fix is comprehensive. The new test_declarative_state_path_safety.py file covers: invalid path rejection (dunders, leading underscores, invalid chars, empty paths), valid path access (underscores-inside-identifiers, mixed case, object/dict traversal, UUIDs), set/append with various key types, interpolation with both invalid and valid paths, end-to-end attack scenarios, and PowerFx regressions. The existing test_graph_coverage.py updates correctly reflect the _TempMessageText→TempMessageText rename. No meaningful test coverage gaps were identified.

✓ Failure Modes

This PR is a well-structured security fix. The path-segment validation in get() correctly gates attribute access behind a regex check while allowing arbitrary dict keys. The interpolate_string regex tightening prevents malicious placeholders from being resolved. The _TempMessageText rename is necessary for PowerFx compatibility. No new concrete failure modes were identified.

✗ Design Approach

The path-safety hardening is directionally good, but it stops at reads and interpolation. get() now rejects malformed segments, while set()/append() still accept them and can create state entries that the new get() will never return, so the write-side API remains inconsistent with the safety contract this PR introduces.

Flagged Issues

• DeclarativeWorkflowState.set() still accepts malformed dot paths even though get() now rejects them. get('Local.') returns the default because empty segments are invalid, but set() will still split the same path and write target['] = value. append() inherits the same behavior through its self.set(path, ...) fallback, allowing creation of unreachable state.

Automated review by peibekwe's agents

Automated Code Review

Reviewers: 5 | Confidence: 90%

✓ Correctness

This PR correctly addresses a path-traversal security vulnerability in declarative workflow state access. The changes add validation to prevent getattr chains (e.g. __class__.__init__.__globals__) while preserving arbitrary dict-key access. The _TempMessageText rename (removing the leading underscore) is a necessary consequence of the new safety check. The interpolate_string regex tightening and empty-segment validation in set/append are both sound. No correctness issues found.

✓ Security Reliability

This PR correctly hardens declarative workflow state path resolution against attribute-traversal attacks. The implementation uses defense-in-depth: interpolate_string's regex rejects unsafe segments at parse time, and get() validates segments before any getattr call. Dict-keyed segments remain unrestricted since dict.get() is safe. The temp variable rename (_TempMessageText → TempMessageText) is a necessary follow-on to maintain compatibility with the new identifier rules. No security or reliability issues found.

✓ Test Coverage

The PR adds comprehensive path-safety tests covering attribute-access rejection, empty-segment validation, and security hardening for interpolate_string. However, there's a gap: the tests verify get() works with UUID/hyphenated dict keys but don't document that interpolate_string() intentionally cannot resolve such paths due to its stricter per-segment regex.

✓ Failure Modes

This PR implements security hardening for declarative workflow state paths, preventing attribute-traversal attacks (e.g., class.init.globals) while preserving legitimate dict-key access patterns. The _TempMessageText rename, empty-segment validation, and interpolation regex tightening are all correctly implemented with no new failure modes introduced. The old code silently wrote unreachable empty-key entries on malformed paths; the new code properly raises ValueError before mutation.

✗ Design Approach

The path-safety hardening looks sound overall, but the new interpolate_string matcher is narrower than the state-path contract the same PR establishes. That creates a real regression for template strings that reference dict-keyed paths with UUIDs or hyphens, especially in the SendActivity flow.

Flagged Issues

• _declarative_base.py:900-901 narows {Variable.Path} matching to identifier-only segments, so SendActivityExecutor (_executors_basic.py:246-251) will leave placeholders like {System.conversations.<uuid>.messages} as literal text instead of resolving them. This contradicts the contract documented in this PR (test_declarative_state_path_safety.py:9-23 and _declarative_base.py:337-340) which explicitly allows arbitrary non-empty dict keys such as UUIDs and hyphenated conversation IDs.

Automated review by peibekwe's agents