Validate JSONPath reference paths (.$) in Step Functions state machine definitions - #4643
Validate JSONPath reference paths (.$) in Step Functions state machine definitions#4643ChrisJr404 wants to merge 4 commits into
Conversation
…initions Fields whose name ends with '.$' inside a payload template (Parameters, ResultSelector, ItemSelector, Assign) must have a value that is a JSONPath or an intrinsic function call; a literal value is rejected by Step Functions at deploy time. E3601 now walks payload templates and reports these so the error is caught before deployment. Fixes aws-cloudformation#4085
kddejong
left a comment
There was a problem hiding this comment.
The check also fires in JSONata mode. _validate_reference_paths runs inside _validate_step, which executes for every schema variant from _clean_schema, including JSONata. The .$ reference-path convention is JSONPath-only — under QueryLanguage: JSONata a literal .$ field gets flagged with a misleading JSONPath error:
Assign: { "foo.$": "plainliteral" } # QueryLanguage: JSONata → E3601
Gate this to JSONPath/unset query language and add a JSONata case asserting no E3601. Otherwise looks good — inline notes below.
|
|
||
| # Validate that ".$" payload template fields reference a JSONPath or | ||
| # an intrinsic function | ||
| yield from self._validate_reference_paths(value, k) |
There was a problem hiding this comment.
This runs for both the JSONPath and JSONata schema variants. In JSONata mode the .$ convention isn't valid ASL, so literal .$ fields get flagged with a JSONPath-oriented message. Gate the call so it only runs when the resolved QueryLanguage is JSONPath (or unset).
| return True | ||
| if stripped.startswith("$"): | ||
| return True | ||
| if stripped.startswith("States.") and stripped.endswith(")"): |
There was a problem hiding this comment.
Minor: startswith("States.") and endswith(")") will accept malformed strings like States.Foo bar). Fine to keep (errs toward false negatives rather than false positives), but a short comment noting the intentional looseness would help future readers.
The .$ reference-path convention is specific to JSONPath. In JSONata mode a literal .$ field name is legal and has no JSONPath semantics, so running the check there produced a misleading E3601. Thread the resolved query language out of _clean_schema and only validate reference paths for the JSONPath (or unset) variant. Adds a JSONata test asserting no E3601 and a note on the intentionally loose intrinsic check.
|
Good catch, thanks. I threaded the resolved query language out of Added a JSONata test case asserting no E3601 on a literal One thing worth flagging: the gate keys off the top-level |
kddejong
left a comment
There was a problem hiding this comment.
Thanks for the follow-ups — the JSONata gating and the intentional-looseness comment both look good, and I confirmed the core behavior: the #4085 literal .$ case is flagged, and !Sub/!If intrinsics, ${...} substitutions, $$ context refs, and States.Format(...) are correctly left alone. 24 tests pass and existing SFN fixtures are unchanged.
One correctness issue before this can merge: a false positive.
_validate_reference_paths decides in_payload by matching the key name anywhere in the tree (key in _payload_template_fields). But that name isn't necessarily an ASL payload template — it can be a user-chosen state name, or nested literal data. A Pass Result is literal JSON (not a payload template), so a .$ key there is a literal and must not be flagged.
Repro:
Resources:
SM:
Type: AWS::StepFunctions::StateMachine
Properties:
RoleArn: arn:aws:iam::123456789012:role/r
Definition:
StartAt: Parameters
States:
Parameters: # a state that happens to be named "Parameters"
Type: Pass
Result: # Pass Result is literal, NOT a payload template
"literalKey.$": "literal data"
End: trueE3601 'literal data' is not a valid JSONPath string or intrinsic function for 'literalKey.$' at /States/Parameters/Result/literalKey.$
The same misfires for states named Assign/ResultSelector/ItemSelector. Since we treat false positives as worse than false negatives, this should be fixed before merge.
Suggested direction: scope payload detection structurally — only enter payload mode when Parameters/ResultSelector/ItemSelector/Assign appears as a direct field of a state (i.e. walk states and start the payload recursion from those specific fields), rather than matching the key name at any depth. That keeps Pass Result and state names from triggering it. A regression test with a state named Parameters (and a literal .$ under Result) asserting no E3601 would lock it in.
|
Good catch on the false positive. I reworked the payload detection to be structural instead of name-based: it now walks the States map and only starts payload-mode recursion from the payload template fields (Parameters/ResultSelector/ItemSelector/Assign) that appear as a direct field of a state, recursing into Parallel branches and Map ItemProcessor/Iterator so nested states keep their coverage. That means a Pass Result stays literal and a state that merely happens to be named Parameters (or Assign/ResultSelector/ItemSelector) no longer trips E3601. Added a regression test for your repro: a state named Parameters whose Pass Result contains a literal "literalKey.$", asserting no E3601. Existing JSONPath positive/negative and JSONata cases are unchanged. 25 tests pass; ruff and isort are clean. |
kddejong
left a comment
There was a problem hiding this comment.
The structural scoping fix looks good — my earlier false positive (state named after a payload field) is resolved, and nested Map/Parallel payloads are validated correctly.
One remaining false positive: the JSONata gate is only evaluated at the top level. _clean_schema reads QueryLanguage from the definition root, but QueryLanguage can be set per-state — a JSONPath (or unset) machine can have an individual JSONata state for incremental adoption (docs: "If the state machine level QueryLanguage is set to JSONPath, then any individual state-level QueryLanguage can be set to either JSONPath or JSONata"). Such a state still gets the .$ check:
Definition:
StartAt: J
States:
J:
Type: Pass
QueryLanguage: JSONata # valid per-state override
Assign:
foo.$: "plainliteral" # legal in JSONata → wrongly flagged E3601
End: trueThis is the same class you flagged for the top-level JSONata case — just at state granularity.
Suggested fix in _validate_reference_paths: skip a state whose own QueryLanguage is JSONata, while still recursing into nested state machines (each nested state resolves its own):
state_path = deque(path)
state_path.extend(["States", state_name])
# A state may override QueryLanguage to JSONata even when the
# machine default is JSONPath (incremental adoption). The ".$"
# convention is JSONPath-only, so skip its payload fields here.
if state.get("QueryLanguage") != "JSONata":
for field in self._payload_template_fields:
if field in state:
field_path = deque(state_path)
field_path.append(field)
yield from self._validate_payload(state[field], k, field_path)I tried this locally: it clears the FP, keeps the issue repro and the nested-Map cases flagged, and all 25 tests still pass. Worth a regression test for a JSONPath machine + JSONata state asserting no E3601.
|
Good catch on the per-state case. |
Issue #, if available: Fixes #4085
Description of changes:
Adds validation so that E3601 catches
.$reference-path fields whose value is not a valid JSONPath or intrinsic function call, before Step Functions rejects the template at deploy time.In the Amazon States Language, inside a payload template (
Parameters,ResultSelector,ItemSelector, andAssign) a field whose name ends with.$is evaluated as a JSONPath or an intrinsic function rather than as a literal. Using a literal there fails at deploy time with, e.g.:cfn-lint didn't flag this. This change teaches
StateMachineDefinition(E3601) to walk payload templates recursively and report any.$field whose value is a plain string that neither begins with$(JSONPath, including$$context references) nor looks like an intrinsic function call (States.*( ... )).To avoid false positives it only inspects string values: non-string values (a resolved intrinsic function object, for example) and strings containing a
${...}substitution placeholder are left alone, since those can resolve to a valid path at deploy time. Nested objects and arrays inside a payload template are treated as payload templates too, matching the ASL semantics.Testing:
test/unit/rules/resources/stepfunctions/test_state_machine_definition.pycovering a valid mix of JSONPath / context / intrinsic / literal values (no error), the literalAssigncase from the issue, and an invalid path nested insideParameters.pytest test/unit/rules/resources/stepfunctions/passes (23 tests).test/fixtures/templatesthat declares anAWS::StepFunctions::StateMachine; none produce a new E3601, so existing snapshots are unchanged.ruff check,ruff format --check, andmypyare clean on the changed files.