Skip to content

Respect job.if event guards in cache safety policies #198

Closed

Description

@kjanat

Problem

cache-call-unrestricted and cache-write-untrusted determine the relevant low-trust events from the workflow's complete on: declaration without considering jobs.<job_id>.if. This produces findings for jobs whose conditions exclude every event the policy is concerned with.

Present in v1.17.0 and current master (cc8b0be), confirmed by inspecting the rule implementation.

Minimal example

.github/workflows/ci.yml:

name: CI
on: [pull_request, pull_request_target]

jobs:
  build:
    if: github.event_name == 'pull_request'
    uses: ./.github/workflows/build.yml

.github/workflows/build.yml:

name: Build
on: workflow_call

jobs:
  build:
    runs-on: ubuntu-latest
    steps:
      - run: echo build

The current cache-call-unrestricted logic reports the calling job because the workflow declares pull_request_target, although that event cannot satisfy the job's condition. Adding cache-mode: write to this guarded calling job instead makes cache-write-untrusted report it for the same unreachable event.

Neither event-dependent finding should apply here. Other declaration, expression and cache-operation checks should continue to apply.

Downstream impact

In Expensify/App#101846, two native-build calls received cache-mode: read despite conditions requiring github.event_name == 'pull_request' and excluding fork PRs. The review discussion raised the resulting loss of cache saves in those build jobs.

Requiring a read-only cap to silence an unreachable-event finding can unnecessarily restrict legitimate caching.

Proposed behaviour

Filter the policy's restricted events per job using the existing expression AST. Remove an event only when the job condition is provably false for that event. A small supported subset covering event-name comparisons and boolean operators is sufficient; unknown terms must remain unknown.

Apply this consistently to both policies, including job-level and inherited workflow-level write grants. A guarded job must not hide a warning for an unguarded sibling inheriting the same workflow-level declaration. Report only restricted events that remain potentially reachable.

Do not infer that pull_request is universally trusted, or that a fork-origin check alone makes a job safe. This change concerns reachability of the events already classified by the cache policies.

Acceptance cases

For on: [pull_request, pull_request_target, issue_comment], let E represent github.event_name and U an otherwise valid condition whose value is unknown:

Job condition Expected event-dependent finding
E == 'pull_request' None
E == 'pull_request' && U None
`E == 'pull_request'
E != 'pull_request_target' Retain for issue_comment
`!(E == 'pull_request_target'
`E == 'pull_request_target'
Missing if, U, or unsupported condition Preserve existing conservative behaviour

Cover both policies, inherited modes and mixed guarded/unguarded jobs. Bare and ${{ }} forms should agree. Follow GitHub expression semantics for parentheses, negation, reversed equality and case-insensitive string comparisons.

Related: #165. GitHub's caller cache-limit semantics still justify requiring explicit limits when a restricted event can actually reach the call.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions