Skip to content

Conversation

@tj098895
Copy link
Contributor

minimize permissions needed, differentiate between event based and reusable workflows

Refs: equinor/ecalc-internal#1253

Type of Work

  • Patch: X.Y.Z+1. NEGLIGIBLE visible changes, does not change input or output - OR changes behaviour. Use chore:, refactor: etc
  • Minor: X.Y+1.Z. Minor changes, might ADD new input (YAML), or other backwards-compatible changes. Use feat:, fix:
  • Major: X+1.Y.Z. Major and most likely BREAKING changes, wo. backwards compatibility, or removing temporary backwards compatibility functionality. Use ! or BREAKING:.

See here (internal): https://github.com/equinor/ecalc-internal/discussions/1044

Have you remembered and considered?

  • IF FEAT: I have remembered to update documentation
  • IF FIX OR FEAT: I have remembered to update manual changelog (docs/drafts/next.draft.md)
  • IF BREAKING: I have remembered to update migration guide (docs/docs/migration_guides/)
  • IF BREAKING: I have committed with BREAKING: in footer or ! in header
  • I have added tests (if not, comment why)
  • I have used conventional commits syntax (if you squash, make sure that conventional commit is used)
  • I have included the Github issue nr in the footer!

What is this PR all about?

What else did you consider?

Between the lines?

@tj098895 tj098895 requested a review from a team as a code owner November 26, 2025 10:25
@tj098895 tj098895 force-pushed the 1253_fix_security_issues_libecalc branch from bbc422c to 86d3d35 Compare November 26, 2025 10:29
@tj098895 tj098895 force-pushed the 1253_fix_security_issues_libecalc branch 2 times, most recently from 09a001d to d92b858 Compare November 26, 2025 10:38
@tj098895 tj098895 requested a review from Copilot November 26, 2025 10:39
@tj098895 tj098895 force-pushed the 1253_fix_security_issues_libecalc branch from d92b858 to ec0b88b Compare November 26, 2025 10:41
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates GitHub Actions workflows to follow new organizational guidelines for workflow security and naming. It implements the principle of least privilege by setting workflow-level permissions to empty (permissions: {}) and granting minimal, specific permissions at the job level. The changes also improve clarity by renaming workflows to better describe their purpose and distinguishing between event-based and reusable workflows.

Key changes:

  • Set explicit minimal permissions at workflow and job levels across all workflow files
  • Renamed workflows and jobs for better clarity (e.g., "Pre CI" → "Ensure Code Quality", "lib-ci" → "test-library")
  • Removed redundant comments explaining workflow triggers

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
.github/workflows/test-library.yml Added workflow-level permissions: {} and job-level contents: read; renamed job from "run-tests" to "test-library"
.github/workflows/test-example-notebooks.yml Added workflow-level permissions: {} and job-level contents: read; renamed job to "test-example-notebooks"
.github/workflows/test-docs.yml Added workflow-level permissions: {} and job-level contents: read; renamed workflow to "Build and Test Docs" and job to "build-docs"
.github/workflows/release-please.yml Added workflow-level permissions: {}, expanded job-level permissions from write-all to specific permissions, added contents: write to create-output-artifact job
.github/workflows/publish-libecalc.yml Added workflow-level permissions: {} and job-level contents: read; renamed jobs and added descriptive comment
.github/workflows/publish-docs.yml Added workflow-level permissions: {} and job-level contents: write with explanatory comment
.github/workflows/on-push-main-branch.yml Added workflow-level permissions: {} and job-level permissions; renamed workflow and jobs to use reusable workflow names
.github/workflows/on-push-any-branch.yml Added workflow-level permissions: {} and renamed jobs to match reusable workflow names
.github/workflows/ensure-code-quality.yml Added workflow-level permissions: {} and job-level contents: read; renamed workflow and job

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

workflow_dispatch: # Workflow dispatch is used for manual triggers.
workflow_dispatch:
workflow_run:
workflows: [ "Main CI/CD" ]
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workflow name reference "Main CI/CD" is outdated. This workflow has been renamed to "On push/merge to main" in this PR (see .github/workflows/on-push-main-branch.yml). Update the reference to match:

workflows: [ "On push/merge to main" ]

Without this change, the release-please workflow will not be triggered after pushes to the main branch.

Suggested change
workflows: [ "Main CI/CD" ]
workflows: [ "On push/merge to main" ]

Copilot uses AI. Check for mistakes.
Comment on lines 31 to 32
permissions:
contents: write
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contents: write permission is unnecessary for this job. The job only creates local files and uploads them as artifacts using actions/upload-artifact@v5, which doesn't require contents: write. The artifact upload action works with default permissions. Consider removing this permission declaration or changing it to contents: read if checkout is added in the future.

Suggested change
permissions:
contents: write
# permissions: block removed; default permissions are sufficient

Copilot uses AI. Check for mistakes.
Comment on lines +21 to +34
ensure-code-quality:
permissions:
contents: read
uses: ./.github/workflows/ensure-code-quality.yml

lib-ci:
uses: ./.github/workflows/lib-ci.yml
test-library:
permissions:
contents: read
uses: ./.github/workflows/test-library.yml

docs-publish:
uses: ./.github/workflows/docs-publish.yml
publish-docs:
permissions:
contents: write
uses: ./.github/workflows/publish-docs.yml
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These permissions declarations are ineffective and misleading. When calling reusable workflows with uses:, permissions set at the job level in the calling workflow are ignored. The reusable workflow's own permissions (defined in the called workflow file) take precedence. Consider removing these declarations to avoid confusion, as they don't actually grant any permissions.

Copilot uses AI. Check for mistakes.
@tj098895 tj098895 force-pushed the 1253_fix_security_issues_libecalc branch 5 times, most recently from 20c1b78 to 9fdede3 Compare November 26, 2025 11:29
minimize permissions needed, differentiate between event based and
reusable workflows

Refs: equinor/ecalc-internal#1253
@tj098895 tj098895 force-pushed the 1253_fix_security_issues_libecalc branch from 9fdede3 to 67dae75 Compare November 26, 2025 11:32
@tj098895 tj098895 merged commit b2630f2 into main Nov 26, 2025
23 checks passed
@tj098895 tj098895 deleted the 1253_fix_security_issues_libecalc branch November 26, 2025 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants