Resolve MR branch and validate goals for business logic scans

A Business Logic Security Scan started from the API or Duo Chat with a merge request IID scans the default branch instead of that merge request.

Merge order: Targets master directly; it doesn't depend on any other BLSA MR's code. It can merge in any order.

What does this MR do and why?

  • A Business Logic Security Scan started from the API or Duo Chat with a merge request IID now checks out that merge request's source branch. Before, it checked out the default branch.
  • Duo Chat now runs the flow's goal validator. A fork or deletion-only merge request is refused with a 400, as it already is on the workflows API.
  • A merge request IID that matches no merge request is now refused (invalid_goal), from GitLab Duo's review on !257092 (merged).
  • A closed or merged merge request is refused too (invalid_goal, The merge request is not open), and resolves no branch.
  • The branch comes from a new, optional source_branch_resolver on foundational flows. Only this flow declares one.

Part of the BLSA split of !246889. Tracker: https://gitlab.com/gitlab-org/gitlab/-/work_items/630266

Behavior

Start path Goal Before After
API, no source_branch MR IID Checks out the default branch. Whole-repository scan Checks out the MR's source branch. Scans the files the MR adds or modifies (with the full-scan fallbacks from !257465 (merged): BL_TARGET_FILES, more than 400 files, paths that can't be split)
Duo Chat MR IID Checks out the default branch. Whole-repository scan Checks out the MR's source branch. Scans the files the MR adds or modifies (with the full-scan fallbacks from !257465 (merged): BL_TARGET_FILES, more than 400 files, paths that can't be split)
Duo Chat Fork MR IID Not refused. Whole-repository scan of the default branch 400: Merge requests from forks are not scanned
API or Duo Chat Closed, merged or unknown MR IID Not refused. Whole-repository scan of the default branch 400: The merge request is not open / Merge request not found
Duo Chat Deletion-only MR IID Not refused. Whole-repository scan of the default branch 400: The merge request has no added or modified files to scan
Automatic trigger (pipeline) MR IID or pipeline URL Checks out the pipeline's ref Unchanged. The pipeline's ref is passed explicitly and wins
Session restart MR IID Checks out the default branch. Whole-repository scan Checks out the MR's source branch. Scans the files the MR adds or modifies (with the full-scan fallbacks from !257465 (merged): BL_TARGET_FILES, more than 400 files, paths that can't be split)

Changed-files scoping comes from !257465 (merged). Until !257465 (merged) merges, a scan started with an MR IID checks out the MR's source branch and scans the whole repository. Once !257465 (merged) is in, it scans only the files the MR adds or modifies, as the table shows.

An explicit source_branch always wins. A goal that is not an MR IID (a pipeline URL) resolves no branch.

How to review

  1. bl_security/definition.rb: the resolver. Open MR IID to source branch; nil for a fork MR, a non-numeric goal, an unknown IID, or a closed or merged MR. The validator refuses unknown and non-open MRs.
  2. attributes.rb and foundational_flow.rb: the new attribute, its keyword validation, and resolve_source_branch_for.
  3. flows/execute_service.rb and duo_workflow_helpers.rb: the source_branch.presence || resolver fallback. ExecuteService covers the consumer path, Duo Chat and restart. The helper covers the name-started path.
  4. agent_workflows.rb: validate_flow_goal! runs only for the business logic flow, and only after the execute-permission check on both paths.

Backward compatibility / impact

Consumer Change Risk Mitigation
Duo Chat and agent callers of code_review/v1 on POST agent_workflows None A new 400 for group containers, or a normalized goal, if the validator ran for every flow Fix 3: the validator runs only for the business logic flow reference (bl_security/experimental). Other flows are byte-identical to the target branch, whatever the flag. Scoping by flow was the smaller change than scoping by flag
Duo Chat and agent callers of bl_security on POST agent_workflows Unknown, closed, merged, fork and deletion-only MRs get a 400 A caller could probe for merge requests it cannot run the flow on Fix 2: validated after the execute-permission check, so an unauthorized caller gets the normal authorization error first. The validator itself returns nil when bl_security_analyzer is off
Ai::Catalog::Flows::ExecuteService and the helper's start_workflow_params Fall back to the flow's resolved branch when source_branch is blank Another flow checks out an unexpected branch Only this flow declares a source_branch_resolver; every other flow resolves nil. An explicit source_branch always wins
New source_branch_resolver attribute Optional, keyword-validated like its siblings None for existing flows Nil by default

validate_flow_goal! moved into API::Helpers::DuoWorkflowHelpers. Since bd13282c it returns Invalid goal to callers without read_merge_request, on workflows.rb too; otherwise workflows.rb behaves as before.

Design decisions

  • A fork MR resolves no branch. Its branch is not in the target project. The validator refuses those runs anyway.
  • The branch is resolved from the goal, not stored. Restart re-resolves it the same way, so no new column is needed.

Known limitations and follow-ups

  • workflows.rb validates the goal before authorization. Pre-existing; left unchanged here and filed as a follow-up.
  • Session restart or resume drops an explicit source_branch. Pre-existing. Restart re-resolves the branch from the goal, so a changed source branch changes what it checks out.
  • An explicit source_branch that doesn't match the goal's MR still wins. It is not checked against the MR.
  • Possible follow-up: reuse GOAL_RESOLVER for the source branch and set noteable_resolver to link the workflow to its MR.

Preflight fixes (this revision)

  1. Closed or merged MRs are refused (invalid_goal) and resolve no branch, so nothing checks out a stale branch.
  2. On agent_workflows, the goal is validated after the execute-permission check on both paths, so it no longer leaks whether an MR exists.
  3. On agent_workflows, only the business logic flow is validated, so flag-off behaviour for other flows (e.g. code_review/v1 on a group) is unchanged.
File inventory (11 files)
  • Resolver: ee/app/models/ai/catalog/foundational_flow/bl_security/definition.rb, foundational_flow.rb, foundational_flow/attributes.rb
  • Start paths: ee/app/services/ai/catalog/flows/execute_service.rb, ee/lib/api/helpers/duo_workflow_helpers.rb
  • API: ee/lib/api/ai/duo_workflows/agent_workflows.rb, ee/lib/api/ai/duo_workflows/workflows.rb (validator moved to the helper)
  • Specs: duo_workflow_helpers_spec.rb, foundational_flow_spec.rb, agent_workflows_spec.rb, execute_service_spec.rb

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Meir Benayoun

Merge request reports

Loading
Loading