fix(backend): Address CodeRabbit review comments - query scoping, stale payload, and execution validation

- Change get_pending_review_by_node_exec_id to use find_first with userId filter in query (prevents cross-tenant existence leak)
- Add input_data parameter to check_approval() to use current data for auto-approvals instead of stale stored payload
- Validate all reviews in a request belong to the same execution to prevent cross-execution review processing
- Update HITLReviewHelper to pass input_data to check_approval()

Fixes CodeRabbit comments: 2719424510, 2719424508, 2719424506
This commit is contained in:
Zamil Majdy
2026-01-22 22:35:42 -05:00
parent 4c47b4df76
commit a396155d63
3 changed files with 32 additions and 10 deletions

View File

@@ -137,19 +137,28 @@ async def process_review_action(
detail="At least one review must be provided",
)
# Get graph execution ID by directly looking up one of the requested reviews
# Get graph execution ID by looking up all requested reviews
# Use direct lookup to avoid pagination issues (can't miss reviews beyond first page)
# Also validate that all reviews belong to the same execution
matching_review = None
graph_exec_ids: set[str] = set()
for node_exec_id in all_request_node_ids:
review = await get_pending_review_by_node_exec_id(node_exec_id, user_id)
if review:
if not review:
raise HTTPException(
status_code=status.HTTP_404_NOT_FOUND,
detail=f"No pending review found for node execution {node_exec_id}",
)
if matching_review is None:
matching_review = review
break
graph_exec_ids.add(review.graph_exec_id)
if not matching_review:
# Ensure all reviews belong to the same execution
if len(graph_exec_ids) > 1:
raise HTTPException(
status_code=status.HTTP_404_NOT_FOUND,
detail="No pending reviews found for the requested node executions",
status_code=status.HTTP_409_CONFLICT,
detail="All reviews in a single request must belong to the same execution.",
)
graph_exec_id = matching_review.graph_exec_id

View File

@@ -100,6 +100,7 @@ class HITLReviewHelper:
graph_exec_id=graph_exec_id,
node_id=node_id,
user_id=user_id,
input_data=input_data,
):
logger.info(
f"Block {block_name} skipping review for node {node_exec_id} - "

View File

@@ -43,6 +43,7 @@ async def check_approval(
graph_exec_id: str,
node_id: str,
user_id: str,
input_data: SafeJsonData | None = None,
) -> Optional[ReviewResult]:
"""
Check if there's an existing approval for this node execution.
@@ -56,6 +57,7 @@ async def check_approval(
graph_exec_id: ID of the graph execution
node_id: ID of the node definition (not execution)
user_id: ID of the user (for data isolation)
input_data: Current input data (used for auto-approvals to avoid stale data)
Returns:
ReviewResult if approval found (either normal or auto), None otherwise
@@ -80,8 +82,14 @@ async def check_approval(
f"Found {'auto-' if is_auto_approval else ''}approval for node {node_id} "
f"(exec: {node_exec_id}) in execution {graph_exec_id}"
)
# For auto-approvals, use current input_data to avoid replaying stale payload
# For normal approvals, use the stored payload (which may have been edited)
return ReviewResult(
data=existing_review.payload,
data=(
input_data
if is_auto_approval and input_data is not None
else existing_review.payload
),
status=ReviewStatus.APPROVED,
message=(
"Auto-approved (user approved all future actions for this node)"
@@ -233,11 +241,15 @@ async def get_pending_review_by_node_exec_id(
Returns:
The pending review if found and belongs to user, None otherwise
"""
review = await PendingHumanReview.prisma().find_unique(
where={"nodeExecId": node_exec_id}
review = await PendingHumanReview.prisma().find_first(
where={
"nodeExecId": node_exec_id,
"userId": user_id,
"status": ReviewStatus.WAITING,
}
)
if not review or review.userId != user_id or review.status != ReviewStatus.WAITING:
if not review:
return None
return PendingHumanReviewModel.from_db(review)