Skip to content

[DevOps] Tighten workflow permissions (update-api-docs, update-confluence, ci - #732

Open
lisa-tarbo wants to merge 7 commits into
mainfrom
tighten-workflow-permissions
Open

[DevOps] Tighten workflow permissions (update-api-docs, update-confluence, ci#732
lisa-tarbo wants to merge 7 commits into
mainfrom
tighten-workflow-permissions

Conversation

@lisa-tarbo

@lisa-tarbo lisa-tarbo commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Background

Recently added an AGENTS.md to the github/workflows dir and improved README-Claude-workflows for workflows to assist engineers (and Claude) with improving security on workflows.

Resolves: #694 which was to audit these workflows.

Details

  1. update-api-docs.yml
    a.) removed two dead grants on the default GITHUB_TOKEN as the actual write (peter-evans/create-pull-request), uses the ocs-agent GitHub App token explicitly.
    b) With these permissions removed, now the create-github-app-token step now scopes the minted ocs-agent token to permission-contents: write and permission-pull-requests: write — just what peter-evans/create-pull-request needs to push the branch and open the PR, instead of inheriting every permission the app installation has.
  2. update-confluence.yml - updated for consistent use of env variables to fix direct-interpolation issue of TAG="${{ github.event.inputs.release_tag }}" going straight into the run: script.
  3. update-confluence.yml - fix for Heredoc Command Injection risk by generating random string for Delimiter for release note tag & release details
  4. 3 workflows: Set persist-credentials: false on checkout unless the job runs git against the remote after it.
  5. Added standard header comments to all 3 workflows (triggering, requirements, permissions)

Reviewer Notes

  1. Wont fix concurrency issues in this PR
  2. Not adding timeouts in this PR

Acceptance Testing

  • uv run prek run actionlint --files
  • update-confluence.yml - has a manual run, but needs to be done by someone with Confluence permissions
  • update-api-docs.yml - has manual dispatch run
  • ci.yml - very limited changes. Triggered on push to main. So checked by developer after merge

Comment thread .github/workflows/ci.yml Outdated
@claude

claude Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Review: workflow permission tightening + header comments

Verified the substantive changes against the workflow bodies and the scripts they call — the permission tightening is correct:

  • update-api-docs.ymlcontents: write / pull-requests: write were genuinely dead. peter-evans/create-pull-request@v8 is passed token: ${{ steps.app-token.outputs.token }}, so the branch push and PR creation both go through the ocs-agent app installation, not GITHUB_TOKEN. contents: read still covers actions/checkout, and the cross-repo gh api repos/dimagi/open-chat-studio/... read works because that repo is public.
  • update-confluence.yml — moving github.event.inputs.release_tag into RELEASE_TAG_INPUT closes a real template-injection hole (a crafted tag would previously have been interpolated straight into the shell). Nice catch, and it makes both Resolve release info steps consistent.

Spot-checked every factual claim in the new headers and they hold up: the automated label really does gate claude-review.yml:32; update_confluence_release.py really does only ever insert_row_after_header with no update-in-place path; the trigger and secret lists match.

Worth fixing

  1. ci.yml:9-11 — the concurrency note is backwards; see inline comment.
  2. update-confluence.yml:37contents: read is the only grant across the three files without an inline # needed for ... annotation. Suggest contents: read # needed for checkout and 'gh release view' so the new convention is uniform.

Minor

  1. update-api-docs.yml:33# read permission for checkout restates the key. The terser ci.yml style reads better: # needed for checkout — PR creation uses the ocs-agent app token instead.
  2. update-confluence.yml:51-60 — while you're in this block: it has no set -euo pipefail (unlike the fetch step in update-api-docs.yml). If gh release view fails, jq writes empty values, the step still exits 0 because echo is last, and the failure only surfaces later as a confusing RELEASE_URL environment variable is required. Out of scope if you'd rather keep the diff tight.

Nothing blocking beyond (1).

@lisa-tarbo
lisa-tarbo marked this pull request as ready for review August 26, 2026 14:07
Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/update-confluence.yml
Comment thread .github/workflows/update-api-docs.yml
@claude

claude Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Docs review — three workflow headers + permissions tightening

Solid PR. I traced every factual claim in the new headers against the actual YAML and Python, and they all hold up:

  • update-api-docs.yml — the removed grants really were dead. peter-evans/create-pull-request is passed token: ${{ steps.app-token.outputs.token }}, so contents: write / pull-requests: write on the default GITHUB_TOKEN were never consumed. contents: read covers the checkout. ✅
  • The automated-label claim checks out — claude-review.yml's job if: excludes PRs carrying that label. ✅
  • update-confluence.yml — the duplicate-row note is correct. insert_row_after_header() in scripts/update_confluence_release.py has no dedup; a re-run genuinely appends. Good caveat to capture. The CONFLUENCE_* secrets listed match the script's documented required env vars. ✅
  • ci.yml — no concurrency group, no external secrets, deploy-pages does authenticate off the job's own pages: write / id-token: write. ✅

The headers also follow the claude-review.yml template faithfully (title → description → Triggered on:Requirements:Notes:), which is what makes this genuinely useful to the next engineer.

Three things, none blocking

  1. ci.yml: fix the deploy race rather than document it. (comment) The other No concurrency group notes sit on workflows where a parallel run is harmless; this one can leave stale content live on the production docs site. It's a three-line guard straight from GitHub's Pages starter workflow. If you'd rather keep this PR permissions-only, file a follow-up and link it from the note.
  2. update-confluence.yml: the env-var hardening is half done. (comment) RELEASE_BODY still goes into $GITHUB_ENV behind a fixed heredoc delimiter in both resolve steps — same injection class as the release_tag fix, different channel. Low likelihood, but release bodies trace back to upstream OCS PR content, so it's not purely maintainer-controlled.
  3. Missing concurrency notes on the other two headers. (comment) ci.yml documents it, update-api-docs.yml and update-confluence.yml don't, and neither has a group — an inconsistency introduced by this PR against a convention the README asserts.

Minor: contents: read # read permission for checkout in update-api-docs.yml restates the key — # needed for checkout matches ci.yml's terser phrasing. And update-confluence.yml's contents: read is the only grant in the PR that didn't get an inline # needed for … comment.

No README-claude-workflows.md update needed — its table is scoped to the Claude workflows, and the automated-label section already covers update-api-docs.yml correctly.

🤖 Generated with Claude Code

@lisa-tarbo
lisa-tarbo requested a review from snopoke August 26, 2026 14:19
@lisa-tarbo
lisa-tarbo marked this pull request as draft September 1, 2026 15:10
@lisa-tarbo
lisa-tarbo force-pushed the tighten-workflow-permissions branch 2 times, most recently from 9a4da6f to deaf42d Compare September 4, 2026 11:59
@lisa-tarbo
lisa-tarbo force-pushed the tighten-workflow-permissions branch from deaf42d to df2578c Compare September 4, 2026 12:29
@lisa-tarbo
lisa-tarbo marked this pull request as ready for review September 4, 2026 13:38
@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 51adcf6c-3147-41bd-89e4-5a7ffb3d7866


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread .github/workflows/update-api-docs.yml Outdated
@claude

claude Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Review: workflow hardening

Re-reviewed at df2578c. The heredoc fix from the earlier round landed — both resolve steps now use openssl rand -hex 16 delimiters, which closes the $GITHUB_ENV injection channel for RELEASE_BODY. Good.

Re-verified every factual claim in the three new headers against the YAML and Python; they all hold:

  • update-api-docs.yml — cron 0 2 * * * is 02:00 UTC ✅; the automated label really does gate claude-review.yml:32 ✅; OCS_AGENT_APP_ID (var) / OCS_AGENT_PRIVATE_KEY (secret) match lines 93–94 ✅
  • update-confluence.ymlinsert_row_after_header() has no update-in-place path, so the duplicate-row caveat is accurate ✅; the three CONFLUENCE_* secrets match ✅
  • ci.yml — no concurrency group, no external secrets, deploy-pages does authenticate off the job's own pages: write / id-token: write

persist-credentials: false is right in all three spots — no job runs git against the remote afterward, and peter-evans/create-pull-request specifically wants it when passed its own token:.

Worth fixing

  1. update-api-docs.yml:33 — PR description item 1b is not in the diff. (inline) The create-github-app-token step still has no permission-contents / permission-pull-requests, so the ocs-agent token inherits the full installation. That's the write path this PR is about, and AGENTS.md:15 mandates scoping it. Two lines.

  2. Missing concurrency notes on two of the three headers. Still open from the last round. claude.yml, claude-dependabot.yml, release.yml, and now ci.yml all carry a "No concurrency group, so …" note; update-api-docs.yml and update-confluence.yml don't, and neither has a group. README-claude-workflows.md:9 asserts "concurrency behavior [is] documented in each workflow's own header comment" — so this PR leaves that statement false for the two files it just added headers to. Documenting the gap is in scope even though fixing it isn't.

    For update-confluence.yml the consequence is concrete and worth a sentence: two runs for the same tag would append two rows.

Minor

  1. update-confluence.yml:37 — the only grant in the PR without an inline "needed for …" annotation. contents: read <!--c-->#<!--c--> needed for checkout and 'gh release view' would make the new convention uniform across all three files.

  2. update-confluence.yml:51-60 — still no set -euo pipefail (the fetch step in update-api-docs.yml has it). If gh release view fails, jq writes empties, the step exits 0 because echo is last, and it surfaces later as a confusing RELEASE_URL environment variable is required. Understood if you'd rather keep the diff tight.

Only (1) is worth blocking on, and only because the description already claims it's done.

🤖 Generated with Claude Code

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.

[DevOps] Audit the 3 workflows (update-api-docs, update-confluence, ci)

2 participants