Skip to content

[DevOps] Tighten security & workflow permissions for claude workflow - #733

Draft
lisa-tarbo wants to merge 6 commits into
mainfrom
security-claude-workflow
Draft

[DevOps] Tighten security & workflow permissions for claude workflow#733
lisa-tarbo wants to merge 6 commits into
mainfrom
security-claude-workflow

Conversation

@lisa-tarbo

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

Copy link
Copy Markdown
Collaborator

Background

This work was done using Claude and the recently updated README-Claude-workflows and this significantly improves Claude's ability to find and fix security issues.

Resolves: #696 which states - claude.yml — highest severity: broad permissions , no trigger gate

  • The issue notes Claudes suggestion of changes - some of which were not done, but commented so Claude does not get confused next time

Details

  1. Removed 3 permissions no needed as workflow uses GitHub App for OCS
  2. Add workflow gate to stop false runs of Claude code with regex for real mention.

Commented on important details that confuse Claude when reviewing in future

  1. Commented why technically difficult to add actor/permission gate and why it relies on claude-code-action's built-in check that the triggering user has write access to this repo. It fails the "Run Claude Code" step with "Action failed with error which is better than being silently skipped.
  2. Commented how claude-code-action picks push target branches - this confused Claude reviews in past.

Acceptance Test

  1. Add and email address for claude.ai as a comment to see that it does not trigger claude code
  2. Test for a user of a fork AND a user who is a maintainer
  3. Test comments in Issue trigger. to confirm it fails silently on incorrect branch, but this is sorted out later (Decided this is OK to keep the workflow simple)
  4. The developer guide is still valid and usable for contributors to the docs repo so double check its accurate

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

claude Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Reviewed the permission removal by tracing every consumer of the default GITHUB_TOKEN in this workflow:

  • Checkout (line 62) passes token: ${{ steps.app-token.outputs.token }} — so the fetch, and the git remote that Bash(git push:*) later uses, are both authenticated by the ocs-agent app, not the default token.
  • Run Claude Code (line 75) passes github_token: ${{ steps.app-token.outputs.token }} — so the tracking comment plus every allowlisted gh pr/gh issue call go through the app token.
  • No other step touches the GitHub API.

That leaves contents: write, pull-requests: write, and issues: write genuinely unused, so dropping them looks correct. id-token: write is rightly kept. This also matches the reasoning already written down in update-changelog.yml — and the difference between the two is real, not an inconsistency: that workflow's checkout has no token:, so its contents: write is still load-bearing.

Two inline notes, neither blocking:

  1. Add the rationale for the removal in the permissions: block, mirroring update-changelog.yml's NOTE — the README's own checklist asks for that trace.
  2. The new header note is accurate but under-states the more surprising behaviour (the ref: fallback to the default branch on issue_comment events).

One housekeeping item: the PR description's Background section describes README improvements ("Improved README for workflows...", "Using the README-Claude-workflows significantly improves Claude's ability to find and fix security issues"), but the diff only touches .github/workflows/claude.yml — no README is changed here. Worth trimming the description to the actual change so the merge commit and issue #696 history stay accurate.

@lisa-tarbo lisa-tarbo changed the title [DevOps] Tightne workflow permissions for claude workflow [DevOps] Tighten security & workflow permissions for claude workflow Aug 27, 2026
@lisa-tarbo
lisa-tarbo marked this pull request as draft August 27, 2026 09:39
@lisa-tarbo
lisa-tarbo marked this pull request as ready for review August 27, 2026 11:51
@coderabbitai

coderabbitai Bot commented Aug 27, 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: Pro Plus

Run ID: 01c2495b-f204-44b5-933d-214990f5c7ba


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/claude.yml Outdated
Comment thread .github/workflows/claude.yml Outdated
Comment thread .github/workflows/claude.yml
Comment thread .github/workflows/claude.yml Outdated
@claude

claude Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Follow-up review covering the two new steps. The permissions trace from my earlier comment still stands — the three removals are correct.

Both new steps are real improvements. The PCRE gate kills the support@claude.ai false-positive class that contains() could not, the untrusted bodies are correctly passed through env: rather than interpolated, and making an issue_comment on a PR check out the PR head instead of silently landing on the default branch fixes a genuine bug.

Four notes inline. Only the fork-PR one is functional — it is a behaviour change worth deciding on deliberately, since acceptance test #2 covers that path. The other three are consistency and maintainability.

One nit not worth an inline: the regex still matches a bare @claude.ai, since . is not in [\w-] and the lookbehind only sees the preceding space. Full addresses like support@claude.ai are correctly excluded, so acceptance test #1 as written passes — add a (?!\.\w) lookahead only if you want bare domain mentions excluded too.

@lisa-tarbo
lisa-tarbo requested a review from snopoke August 27, 2026 12:10
@lisa-tarbo

lisa-tarbo commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Generated by Claude using

Running this test on branch claude-review-workflow to test pr-review.md

cd /home/lisa/open-chat-studio-docs claude -p "/review-pr REPO: dimagi/open-chat-studio-docs PR_NUMBER: 733" \ --allowedTools "Task,Read,Grep,Glob,Bash(gh pr diff:*),Bash(gh pr view:*)" \ --verbose

Workflow security review

Reviewed against .github/workflows/AGENTS.md (this is a workflow/security change, not docs). Overall a solid hardening — nice work.

Good:

  • Dropping contents/pull-requests/issues: write is correct: checkout and claude-code-action both auth with the ocs-agent App token, so the default GITHUB_TOKEN scopes were never consumed (matches the AGENTS.md rule that permissions: is bypassed when the App mints its token).
  • The mention gate handles untrusted text safely — *_BODY/*_TITLE go through env: and are never interpolated into the script.

Should fix — comment references a non-existent step: the new note says the push target is "independent of the ref the Resolve checkout ref step resolves." There is no Resolve checkout ref step here — the ref is resolved inline on the Checkout repository step (ref: github.event.pull_request.head.ref || github.ref). Since these comments orient future reviewers of a security-sensitive file, please point at the actual step name.

Minor / optional — regex edge case: (?<!\w)@claude(?![\w-]) correctly ignores real emails (info@claude.ai → no match) and @claudebot, but a bare @claude.ai (dot terminator) does match. Real emails always have a local part so acceptance-test #1 still passes — flagging for awareness, not a blocker.

id-token: write and the unchanged persist-credentials default both look fine given the App-token flow and that Claude runs git push.

@lisa-tarbo
lisa-tarbo marked this pull request as draft August 31, 2026 15:17
@lisa-tarbo
lisa-tarbo force-pushed the security-claude-workflow branch from e39842b to 425b51e Compare September 4, 2026 12:33
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.

[AgentOps] Tighten permissions for claude.yml workflow

1 participant