From 5e46aee485641c251fdaeb9a05bd9977c711def8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Fri, 3 Jul 2026 18:24:04 +0200 Subject: [PATCH 1/2] review-pr: Avoid OIDC for direct API key reviews MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Direct API-key callers do not need AWS-backed credential setup, but the reusable workflow unconditionally requested id-token: write and ran setup-credentials. Organizations that disable OIDC for reusable workflows were blocked at the permission-validation step. Drop the top-level and per-job permissions blocks so the workflow inherits from the caller. When a model key secret is supplied, skip the OIDC credential fetch and authorize using the caller's GITHUB_TOKEN by checking repository write permission instead of Docker org membership. setup-credentials/action.yml gains a fetch-credentials input (default true). When false, DOCKER_AGENT_ACTION_ROOT is exported and Node.js is set up, but the OIDC exchange is skipped. The review job always runs setup-credentials (for DOCKER_AGENT_ACTION_ROOT and Node) and passes fetch-credentials dynamically, so direct-key callers pay no OIDC cost. The reply-to-feedback and reply-to-mention jobs skip setup-credentials entirely when a direct key is present — they use bash gh-api calls and uses: actions, not dist/ scripts, so neither DOCKER_AGENT_ACTION_ROOT nor an explicit Node install is needed. src/check-org-membership adds checkRepositoryWritePermission (write/maintain/admin collaborator check) and an isAuthorizedUser helper that dispatches to org membership (when ORG_MEMBERSHIP_TOKEN is set) or the repo permission check (when it is not). The CLI entry point now accepts GITHUB_TOKEN as a fallback for GITHUB_APP_TOKEN. The mention-reply handler and the reply-to-feedback auth step receive the same treatment: org-membership-token is now optional; when absent, authorization falls back to checkRepositoryWritePermission. The HAS_DIRECT_API_KEY guard that silently dis Signed-off-by: Paweł Gronowski --- .github/actions/mention-reply/action.yml | 5 +- .github/workflows/review-pr.yml | 122 ++++++------------ README.md | 4 +- review-pr/README.md | 19 ++- setup-credentials/action.yml | 16 ++- .../__tests__/check-org-membership.test.ts | 108 ++++++++++++++++ src/check-org-membership/index.ts | 72 +++++++++-- .../__tests__/mention-reply.test.ts | 60 +++++++++ src/mention-reply/index.ts | 21 +-- 9 files changed, 314 insertions(+), 113 deletions(-) diff --git a/.github/actions/mention-reply/action.yml b/.github/actions/mention-reply/action.yml index 8bc164f..e2a3344 100644 --- a/.github/actions/mention-reply/action.yml +++ b/.github/actions/mention-reply/action.yml @@ -8,8 +8,9 @@ inputs: description: 'GitHub token for posting reactions and rejection replies' required: true org-membership-token: - description: 'PAT with read:org scope for docker org membership checks' - required: true + description: 'PAT with read:org scope for docker org membership checks. Optional: when absent, falls back to checking repository write permission using github-token.' + required: false + default: '' outputs: should-reply: description: "'true' if the reply agent should run; 'false' if the event was skipped or rejected" diff --git a/.github/workflows/review-pr.yml b/.github/workflows/review-pr.yml index 7dc1e6b..a1be1df 100644 --- a/.github/workflows/review-pr.yml +++ b/.github/workflows/review-pr.yml @@ -78,13 +78,6 @@ on: description: "Exit code from the review" value: ${{ jobs.review.outputs.exit-code }} -permissions: - contents: read - pull-requests: write - issues: write - id-token: write - actions: read # download-artifact across workflow_run boundary - # Rate-anomaly safeguard: serialize same-trigger bursts on a PR (e.g. rapid # force-pushes firing repeated auto-reviews) instead of running N in parallel. # The group is keyed per PR AND per trigger intent: the comment id (or event @@ -114,25 +107,13 @@ jobs: comment-is-review-cmd: ${{ steps.read.outputs.comment-is-review-cmd }} comment-author-type: ${{ steps.read.outputs.comment-author-type }} steps: - - name: Setup credentials - uses: docker/docker-agent-action/setup-credentials@774b6e0e60d6c648b0f2dc43bd5221377a0a7420 # v2.0.2 - - - name: Verify token for cross-run artifact download - shell: bash - run: | - if [ -z "$GITHUB_APP_TOKEN" ]; then - echo "::error::GITHUB_APP_TOKEN is not set. setup-credentials may have failed (check OIDC and AWS Secrets Manager configuration)." - echo "::error::Cross-run artifact download requires a token with actions:read scope." - exit 1 - fi - - name: Download trigger context uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: pr-review-context path: /tmp/context run-id: ${{ inputs.trigger-run-id }} - github-token: ${{ env.GITHUB_APP_TOKEN }} + github-token: ${{ github.token }} - name: Read context id: read @@ -184,12 +165,8 @@ jobs: ) runs-on: ubuntu-latest timeout-minutes: 50 - permissions: - contents: read - pull-requests: write - issues: write - id-token: write - checks: write + env: + HAS_DIRECT_API_KEY: ${{ secrets.ANTHROPIC_API_KEY != '' || secrets.OPENAI_API_KEY != '' || secrets.GOOGLE_API_KEY != '' || secrets.AWS_BEARER_TOKEN_BEDROCK != '' || secrets.XAI_API_KEY != '' || secrets.NEBIUS_API_KEY != '' || secrets.MISTRAL_API_KEY != '' }} outputs: exit-code: ${{ steps.run-review.outputs.exit-code }} @@ -332,6 +309,8 @@ jobs: EVENT_NAME: ${{ github.event_name }} EVENT_ACTION: ${{ github.event.action }} REQUESTER: ${{ github.event.sender.login }} + GITHUB_TOKEN: ${{ github.token }} + AGENT_LOGIN: docker-agent run: node "$DOCKER_AGENT_ACTION_ROOT/dist/check-org-membership.js" # Rate-anomaly safeguard: count how many docker-agent reviews and replies @@ -466,29 +445,10 @@ jobs: (needs.resolve-context.result == 'success' && needs.resolve-context.outputs.trigger-event == 'pull_request_review_comment' && needs.resolve-context.outputs.comment-in-reply-to-id != '' && needs.resolve-context.outputs.comment-author != 'docker-agent' && needs.resolve-context.outputs.comment-author-type != 'Bot' && needs.resolve-context.outputs.comment-author != 'docker-agent[bot]') ) runs-on: ubuntu-latest - permissions: - contents: read - pull-requests: write - issues: write - id-token: write - actions: read # download cross-run artifacts + env: + HAS_DIRECT_API_KEY: ${{ secrets.ANTHROPIC_API_KEY != '' || secrets.OPENAI_API_KEY != '' || secrets.GOOGLE_API_KEY != '' || secrets.AWS_BEARER_TOKEN_BEDROCK != '' || secrets.XAI_API_KEY != '' || secrets.NEBIUS_API_KEY != '' || secrets.MISTRAL_API_KEY != '' }} steps: - - - name: Setup credentials - if: inputs.trigger-run-id != '' - uses: docker/docker-agent-action/setup-credentials@774b6e0e60d6c648b0f2dc43bd5221377a0a7420 # v2.0.2 - - - name: Verify token for cross-run artifact download - if: inputs.trigger-run-id != '' - shell: bash - run: | - if [ -z "$GITHUB_APP_TOKEN" ]; then - echo "::error::GITHUB_APP_TOKEN is not set. setup-credentials may have failed (check OIDC and AWS Secrets Manager configuration)." - echo "::error::Cross-run artifact download requires a token with actions:read scope." - exit 1 - fi - - name: Download trigger context # Bypass secret-masking: instead of consuming comment-json from # resolve-context job outputs (which GitHub Actions silently drops when @@ -499,7 +459,7 @@ jobs: name: pr-review-context path: /tmp/context run-id: ${{ inputs.trigger-run-id }} - github-token: ${{ env.GITHUB_APP_TOKEN }} + github-token: ${{ github.token }} - name: Parse comment context id: feedback @@ -599,7 +559,7 @@ jobs: fi - name: Setup credentials - if: steps.check.outputs.is_agent == 'true' + if: steps.check.outputs.is_agent == 'true' && env.HAS_DIRECT_API_KEY != 'true' uses: docker/docker-agent-action/setup-credentials@774b6e0e60d6c648b0f2dc43bd5221377a0a7420 # v2.0.2 - name: Check authorization @@ -607,40 +567,37 @@ jobs: id: auth shell: bash env: - GH_TOKEN: ${{ env.ORG_MEMBERSHIP_TOKEN }} + GH_TOKEN: ${{ env.ORG_MEMBERSHIP_TOKEN || github.token }} ORG: docker + REPO: ${{ github.repository }} USERNAME: ${{ steps.feedback.outputs.comment-author }} + HAS_ORG_TOKEN: ${{ env.ORG_MEMBERSHIP_TOKEN != '' }} run: | - if [ -z "$GH_TOKEN" ]; then - echo "::error::ORG_MEMBERSHIP_TOKEN is not set — org membership check cannot run (ensure id-token: write permission is configured)" - echo "authorized=false" >> $GITHUB_OUTPUT - exit 0 - fi - # Check org membership with explicit error handling - if ! RESPONSE=$(gh api "orgs/$ORG/members/$USERNAME" --silent -i 2>/dev/null); then - echo "authorized=false" >> $GITHUB_OUTPUT - echo "⏭️ API call failed or @$USERNAME is not a $ORG org member — not authorized" - exit 0 - fi - # Verify response starts with HTTP status line before parsing - if ! echo "$RESPONSE" | head -1 | grep -q '^HTTP/'; then - echo "::warning::Unexpected API response format" - echo "authorized=false" >> $GITHUB_OUTPUT - exit 0 - fi - # Extract status code from HTTP/1.1 204 No Content format - STATUS=$(echo "$RESPONSE" | head -1 | grep -oP 'HTTP/[0-9.]+ \K[0-9]+') - if [ -z "$STATUS" ]; then - echo "::warning::Failed to extract HTTP status code" - echo "authorized=false" >> $GITHUB_OUTPUT + if [ "$HAS_ORG_TOKEN" = "true" ]; then + # Check org membership with explicit error handling. + if ! RESPONSE=$(gh api "orgs/$ORG/members/$USERNAME" --silent -i 2>/dev/null); then + echo "authorized=false" >> $GITHUB_OUTPUT + echo "⏭️ API call failed or @$USERNAME is not a $ORG org member — not authorized" + exit 0 + fi + STATUS=$(echo "$RESPONSE" | head -1 | grep -oP 'HTTP/[0-9.]+ \K[0-9]+' || true) + if [ "$STATUS" = "204" ]; then + echo "authorized=true" >> $GITHUB_OUTPUT + echo "✅ @$USERNAME is a $ORG org member — authorized" + else + echo "authorized=false" >> $GITHUB_OUTPUT + echo "⏭️ @$USERNAME is not a $ORG org member — not authorized" + fi exit 0 fi - if [ "$STATUS" = "204" ]; then + + PERMISSION=$(gh api "repos/$REPO/collaborators/$USERNAME/permission" --jq '.permission' 2>/dev/null || echo "none") + if [ "$PERMISSION" = "admin" ] || [ "$PERMISSION" = "maintain" ] || [ "$PERMISSION" = "write" ]; then echo "authorized=true" >> $GITHUB_OUTPUT - echo "✅ @$USERNAME is a $ORG org member — authorized" + echo "✅ @$USERNAME has $PERMISSION access to $REPO — authorized" else echo "authorized=false" >> $GITHUB_OUTPUT - echo "⏭️ @$USERNAME is not a $ORG org member — not authorized" + echo "⏭️ @$USERNAME does not have write access to $REPO — not authorized" fi - name: Notify unauthorized user @@ -656,7 +613,7 @@ jobs: run: | jq -n \ - --arg body "Sorry @$AUTHOR, conversational replies are currently available to repository collaborators only. Your feedback has still been captured and will be used to improve future reviews. + --arg body "Sorry @$AUTHOR, conversational replies are currently available to authorized contributors only. Your feedback has still been captured and will be used to improve future reviews. " \ --argjson reply_to "$ROOT_COMMENT_ID" \ @@ -829,15 +786,12 @@ jobs: needs.resolve-context.outputs.comment-author-type != 'Bot') ) runs-on: ubuntu-latest - permissions: - contents: read - pull-requests: write - issues: write - id-token: write - actions: read # download cross-run artifacts + env: + HAS_DIRECT_API_KEY: ${{ secrets.ANTHROPIC_API_KEY != '' || secrets.OPENAI_API_KEY != '' || secrets.GOOGLE_API_KEY != '' || secrets.AWS_BEARER_TOKEN_BEDROCK != '' || secrets.XAI_API_KEY != '' || secrets.NEBIUS_API_KEY != '' || secrets.MISTRAL_API_KEY != '' }} steps: - name: Setup credentials + if: env.HAS_DIRECT_API_KEY != 'true' uses: docker/docker-agent-action/setup-credentials@774b6e0e60d6c648b0f2dc43bd5221377a0a7420 # v2.0.2 - name: Download trigger context @@ -847,7 +801,7 @@ jobs: name: pr-review-context path: /tmp/context run-id: ${{ inputs.trigger-run-id }} - github-token: ${{ env.GITHUB_APP_TOKEN }} + github-token: ${{ env.GITHUB_APP_TOKEN || github.token }} - name: Synthesize mention-reply event context if: inputs.trigger-run-id != '' @@ -904,7 +858,7 @@ jobs: - name: Run mention-reply handler id: mention-context if: steps.resolve-event.outputs.path != '' - uses: docker/docker-agent-action/.github/actions/mention-reply@774b6e0e60d6c648b0f2dc43bd5221377a0a7420 # v2.0.2 + uses: docker/docker-agent-action/.github/actions/mention-reply@896eb4c0871ac3e755d51a14a08d7fa551b87853 # direct API key OIDC fix env: GITHUB_EVENT_PATH: ${{ steps.resolve-event.outputs.path }} GITHUB_EVENT_NAME: ${{ steps.resolve-event.outputs.name }} diff --git a/README.md b/README.md index 66ff8e8..63dcf05 100644 --- a/README.md +++ b/README.md @@ -179,9 +179,11 @@ permissions: pull-requests: write # Post review comments and approve/request changes issues: write # Create security incident issues if secrets are detected in output checks: write # (Optional) Show review progress as a check run on the PR - id-token: write # Required for OIDC authentication to AWS Secrets Manager ``` +Add `id-token: write` only when you use the optional AWS Secrets Manager credential setup. +It is not required when you pass a model API key directly, such as `anthropic-api-key`. + ## Examples ### Multiple Agents in a Workflow diff --git a/review-pr/README.md b/review-pr/README.md index bca7370..c5da683 100644 --- a/review-pr/README.md +++ b/review-pr/README.md @@ -33,7 +33,7 @@ jobs: pull-requests: write # Post review comments issues: write # Create security incident issues if secrets detected checks: write # (Optional) Show review progress as a check run - id-token: write # Required for OIDC authentication to AWS Secrets Manager + id-token: write # Preferred: OIDC authentication to AWS Secrets Manager actions: read # Required by reusable workflow for artifact operations ``` @@ -111,12 +111,27 @@ jobs: pull-requests: write # Post review comments issues: write # Create security incident issues if secrets detected checks: write # (Optional) Show review progress as a check run - id-token: write # Required for OIDC authentication to AWS Secrets Manager + id-token: write # Preferred: OIDC authentication to AWS Secrets Manager actions: read # Required by reusable workflow for artifact operations; also needed to download trigger artifacts with: trigger-run-id: ${{ github.event_name == 'workflow_run' && format('{0}', github.event.workflow_run.id) || '' }} ``` +The preferred setup uses OIDC to fetch Docker-managed credentials from AWS Secrets Manager. +If your repository provides a model API key directly instead, pass it through `secrets` and you may omit `id-token: write`: +`actions: read` is always required — the workflow downloads event-context artifacts across runs regardless of which credential path is used. + +```yaml + permissions: + contents: read + pull-requests: write + issues: write + checks: write + actions: read + secrets: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} +``` + #### How the two workflows interact ``` diff --git a/setup-credentials/action.yml b/setup-credentials/action.yml index f9c129c..23cbf0b 100644 --- a/setup-credentials/action.yml +++ b/setup-credentials/action.yml @@ -3,24 +3,36 @@ name: 'Setup Credentials' description: 'Fetch credentials from AWS Secrets Manager via OIDC' +inputs: + fetch-credentials: + description: 'Fetch AWS-backed credentials. Set to false when the caller supplies credentials directly.' + required: false + default: 'true' runs: using: composite steps: + - name: Export action root + shell: bash + run: echo "DOCKER_AGENT_ACTION_ROOT=$(cd "$GITHUB_ACTION_PATH/.." && pwd)" >> "$GITHUB_ENV" + - name: Setup Node.js + # Always runs — even when fetch-credentials is false — because callers + # invoke dist/ scripts (check-org-membership.js, rate-limit.js) via + # `node "$DOCKER_AGENT_ACTION_ROOT/dist/..."` after this action returns. uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 with: node-version: '24' - name: Fetch credentials + if: inputs.fetch-credentials == 'true' shell: bash run: node "$GITHUB_ACTION_PATH/../dist/credentials.js" - name: Verify credentials were obtained + if: inputs.fetch-credentials == 'true' shell: bash run: | if [ -z "$GITHUB_APP_TOKEN" ]; then echo "::error::GITHUB_APP_TOKEN was not set — setup-credentials failed silently." exit 1 fi - # Export the repo root so callers can reach dist/ bundles via $DOCKER_AGENT_ACTION_ROOT - echo "DOCKER_AGENT_ACTION_ROOT=$(cd "$GITHUB_ACTION_PATH/.." && pwd)" >> "$GITHUB_ENV" diff --git a/src/check-org-membership/__tests__/check-org-membership.test.ts b/src/check-org-membership/__tests__/check-org-membership.test.ts index d6ec667..de0a1b1 100644 --- a/src/check-org-membership/__tests__/check-org-membership.test.ts +++ b/src/check-org-membership/__tests__/check-org-membership.test.ts @@ -7,6 +7,7 @@ vi.mock('@actions/core'); const { mockCheckMembershipForUser, + mockGetCollaboratorPermissionLevel, mockGetPull, mockListEventsForTimeline, mockPaginate, @@ -14,6 +15,9 @@ const { constructorTokens, } = vi.hoisted(() => { const mockCheckMembershipForUser = vi.fn().mockResolvedValue({}); // 204 = member + const mockGetCollaboratorPermissionLevel = vi + .fn() + .mockResolvedValue({ data: { permission: 'write' } }); const mockGetPull = vi.fn().mockResolvedValue({ data: { user: { login: 'bob' } } }); const mockListEventsForTimeline = vi.fn(); const mockPaginate = vi.fn().mockResolvedValue([]); @@ -28,6 +32,7 @@ const { orgs: { checkMembershipForUser: mockCheckMembershipForUser }, pulls: { get: mockGetPull }, issues: { listEventsForTimeline: mockListEventsForTimeline }, + repos: { getCollaboratorPermissionLevel: mockGetCollaboratorPermissionLevel }, }; constructor({ auth }: { auth: string }) { constructorTokens.push(auth); @@ -36,6 +41,7 @@ const { return { mockCheckMembershipForUser, + mockGetCollaboratorPermissionLevel, mockGetPull, mockListEventsForTimeline, mockPaginate, @@ -48,6 +54,7 @@ vi.mock('@octokit/rest', () => ({ Octokit: MockOctokit })); import { checkOrgMembership, + checkRepositoryWritePermission, evaluateMembership, type MembershipInputs, resolvePrAuthor, @@ -204,6 +211,43 @@ describe('resolvePrAuthor', () => { }); }); +// --------------------------------------------------------------------------- +// checkRepositoryWritePermission +// --------------------------------------------------------------------------- + +describe('checkRepositoryWritePermission', () => { + it('returns true for write-level repository permission', async () => { + mockGetCollaboratorPermissionLevel.mockResolvedValueOnce({ data: { permission: 'maintain' } }); + + const ok = await checkRepositoryWritePermission(REPO_TOKEN, 'docker', 'myrepo', 'maintainer'); + + expect(ok).toBe(true); + expect(mockGetCollaboratorPermissionLevel).toHaveBeenCalledWith({ + owner: 'docker', + repo: 'myrepo', + username: 'maintainer', + }); + }); + + it('returns false for read-only repository permission', async () => { + mockGetCollaboratorPermissionLevel.mockResolvedValueOnce({ data: { permission: 'read' } }); + + const ok = await checkRepositoryWritePermission(REPO_TOKEN, 'docker', 'myrepo', 'reader'); + + expect(ok).toBe(false); + }); + + it('returns false for users without repository permission', async () => { + mockGetCollaboratorPermissionLevel.mockRejectedValueOnce( + Object.assign(new Error('Not Found'), { status: 404 }), + ); + + const ok = await checkRepositoryWritePermission(REPO_TOKEN, 'docker', 'myrepo', 'external'); + + expect(ok).toBe(false); + }); +}); + // --------------------------------------------------------------------------- // resolveReviewRequester (trusted, timeline-derived) // --------------------------------------------------------------------------- @@ -429,6 +473,29 @@ describe('evaluateMembership', () => { expect(decision).toEqual({ isMember: false, subject: 'outsider', via: 'none' }); }); + it('review_requested (direct): authorizes requester by repo permission without org token', async () => { + mockGetPull.mockResolvedValueOnce({ data: { user: { login: 'ext-author' } } }); + mockGetCollaboratorPermissionLevel + .mockResolvedValueOnce({ data: { permission: 'read' } }) + .mockResolvedValueOnce({ data: { permission: 'write' } }); + + const decision = await evaluateMembership( + inputs({ orgToken: '', eventAction: 'review_requested', trustedRequester: 'maintainer' }), + ); + + expect(decision).toEqual({ isMember: true, subject: 'maintainer', via: 'requester' }); + expect(mockGetCollaboratorPermissionLevel).toHaveBeenNthCalledWith(1, { + owner: 'docker', + repo: 'myrepo', + username: 'ext-author', + }); + expect(mockGetCollaboratorPermissionLevel).toHaveBeenNthCalledWith(2, { + owner: 'docker', + repo: 'myrepo', + username: 'maintainer', + }); + }); + it('review_requested (fork/trigger): authorizes via the timeline-derived requester', async () => { membersAre('maintainer'); mockGetPull.mockResolvedValueOnce({ data: { user: { login: 'ext-author' } } }); @@ -463,6 +530,26 @@ describe('evaluateMembership', () => { expect(decision).toEqual({ isMember: false, subject: 'ext-author', via: 'none' }); }); + it('fork/trigger review_requested: authorizes timeline requester by repo permission without org token', async () => { + mockGetPull.mockResolvedValueOnce({ data: { user: { login: 'ext-author' } } }); + mockGetCollaboratorPermissionLevel + .mockResolvedValueOnce({ data: { permission: 'read' } }) + .mockResolvedValueOnce({ data: { permission: 'maintain' } }); + mockPaginate.mockResolvedValueOnce([reviewRequestedEvent('maintainer')]); + + const decision = await evaluateMembership( + inputs({ + orgToken: '', + prSource: 'trigger', + eventName: 'workflow_run', + eventAction: 'completed', + prNumber: 7, + }), + ); + + expect(decision).toEqual({ isMember: true, subject: 'maintainer', via: 'requester' }); + }); + it('fork/trigger: a forged timeline requester is still validated against real org membership', async () => { // Even if the (fork-influenced) PR claimed a member, the requester is taken // from the trusted timeline AND re-checked against the org. A non-member there @@ -528,6 +615,27 @@ describe('evaluateMembership', () => { expect(decision).toEqual({ isMember: false, subject: 'ext', via: 'none' }); }); + it('issue_comment: authorizes commenter by repo permission without org token', async () => { + mockGetCollaboratorPermissionLevel.mockResolvedValueOnce({ data: { permission: 'write' } }); + + const decision = await evaluateMembership( + inputs({ + orgToken: '', + eventName: 'issue_comment', + eventAction: 'created', + commentAuthor: 'collaborator', + }), + ); + + expect(decision).toEqual({ isMember: true, subject: 'collaborator', via: 'comment' }); + expect(mockGetPull).not.toHaveBeenCalled(); + expect(mockGetCollaboratorPermissionLevel).toHaveBeenCalledWith({ + owner: 'docker', + repo: 'myrepo', + username: 'collaborator', + }); + }); + it('throws on an invalid PR number for PR-driven paths', async () => { await expect(evaluateMembership(inputs({ prNumber: Number.NaN }))).rejects.toThrow( /Invalid pr-number/, diff --git a/src/check-org-membership/index.ts b/src/check-org-membership/index.ts index 65f7ab6..53f0234 100644 --- a/src/check-org-membership/index.ts +++ b/src/check-org-membership/index.ts @@ -21,6 +21,7 @@ * Exported functions: * checkOrgMembership(orgToken, org, username) → boolean * resolvePrAuthor(repoToken, owner, repo, prNumber) → string + * checkRepositoryWritePermission(repoToken, owner, repo, username) → boolean * resolveReviewRequester(repoToken, owner, repo, prNumber, reviewerLogin) → string * evaluateMembership(inputs) → { isMember, subject, via } * @@ -103,6 +104,40 @@ export async function resolvePrAuthor( return pr.user?.login ?? ''; } +// --------------------------------------------------------------------------- +// Core function: repository permission check +// --------------------------------------------------------------------------- + +/** + * Check whether `username` has write-level permission on the repository. + * + * This is the API-key-only fallback when Docker's AWS-backed org membership + * token is unavailable. + * It keeps fork PRs safe by authorizing only repository collaborators or + * maintainers, including review-requested runs where the requester is resolved + * from the trusted PR timeline. + */ +export async function checkRepositoryWritePermission( + repoToken: string, + owner: string, + repo: string, + username: string, +): Promise { + const octokit = new Octokit({ auth: repoToken }); + try { + const { data } = await octokit.rest.repos.getCollaboratorPermissionLevel({ + owner, + repo, + username, + }); + return ['admin', 'maintain', 'write'].includes(data.permission ?? ''); + } catch (err: unknown) { + const status = (err as { status?: number }).status; + if (status === 404) return false; + throw err; + } +} + // --------------------------------------------------------------------------- // Core function: review-requester resolution (trusted, server-side) // --------------------------------------------------------------------------- @@ -249,12 +284,26 @@ async function resolveTrustedRequester( return ''; } +async function isAuthorizedUser( + inputs: MembershipInputs, + owner: string, + repo: string, + username: string, +): Promise { + if (!username) return false; + if (inputs.orgToken && inputs.org) { + return checkOrgMembership(inputs.orgToken, inputs.org, username); + } + return checkRepositoryWritePermission(inputs.repoToken, owner, repo, username); +} + /** * Decide whether the review is authorized, returning the subject and the path * that granted it. See the module header for the two authorization paths. */ export async function evaluateMembership(inputs: MembershipInputs): Promise { - const { orgToken, org, prSource, eventName, commentAuthor } = inputs; + const { prSource, eventName, commentAuthor } = inputs; + const { owner, repo } = parseRepository(inputs.repository); // Comment-driven triggers (e.g. /review) authorize the commenter. EVENT_NAME may // be absent when called by an older caller; fall back to the presence of a @@ -263,25 +312,24 @@ export async function evaluateMembership(inputs: MembershipInputs): Promise { const orgToken = process.env.ORG_MEMBERSHIP_TOKEN ?? ''; - const repoToken = process.env.GITHUB_APP_TOKEN ?? ''; + const repoToken = process.env.GITHUB_APP_TOKEN || process.env.GITHUB_TOKEN || ''; const org = process.env.ORG ?? ''; - if (!orgToken) { - core.setFailed('ORG_MEMBERSHIP_TOKEN is not set — ensure setup-credentials ran successfully.'); - return; - } if (!repoToken) { - core.setFailed('GITHUB_APP_TOKEN is not set — ensure setup-credentials ran successfully.'); + core.setFailed('GITHUB_APP_TOKEN or GITHUB_TOKEN is not set — cannot authorize the review.'); return; } @@ -328,8 +372,8 @@ async function main(): Promise { if (decision.isMember) { const reason = decision.via === 'requester' - ? `review requested by ${org} org member @${decision.subject}` - : `@${decision.subject} is a ${org} org member`; + ? `review requested by authorized user @${decision.subject}` + : `@${decision.subject} is authorized`; core.info(`✅ ${reason} — proceeding with review`); } else { core.info( @@ -346,7 +390,7 @@ async function main(): Promise { ); } else { core.setFailed( - `Failed to check org membership: ${err instanceof Error ? err.message : String(err)}`, + `Failed to authorize the review: ${err instanceof Error ? err.message : String(err)}`, ); } } diff --git a/src/mention-reply/__tests__/mention-reply.test.ts b/src/mention-reply/__tests__/mention-reply.test.ts index fb0cca4..f75cd02 100644 --- a/src/mention-reply/__tests__/mention-reply.test.ts +++ b/src/mention-reply/__tests__/mention-reply.test.ts @@ -15,12 +15,14 @@ vi.mock('@actions/core'); const { mockAddReaction, mockCheckOrgMembership, + mockCheckRepositoryWritePermission, mockPostComment, mockPostReviewCommentReply, mockGetPrMeta, } = vi.hoisted(() => ({ mockAddReaction: vi.fn().mockResolvedValue(undefined), mockCheckOrgMembership: vi.fn().mockResolvedValue(true), + mockCheckRepositoryWritePermission: vi.fn().mockResolvedValue(true), mockPostComment: vi.fn().mockResolvedValue(undefined), mockPostReviewCommentReply: vi.fn().mockResolvedValue(undefined), mockGetPrMeta: vi.fn().mockResolvedValue({ @@ -34,6 +36,7 @@ const { vi.mock('../../add-reaction/index.js', () => ({ addReaction: mockAddReaction })); vi.mock('../../check-org-membership/index.js', () => ({ checkOrgMembership: mockCheckOrgMembership, + checkRepositoryWritePermission: mockCheckRepositoryWritePermission, })); vi.mock('../../post-comment/index.js', () => ({ postComment: mockPostComment, @@ -156,6 +159,7 @@ beforeEach(() => { // Re-apply defaults after clearAllMocks mockAddReaction.mockResolvedValue(undefined); mockCheckOrgMembership.mockResolvedValue(true); + mockCheckRepositoryWritePermission.mockResolvedValue(true); mockPostComment.mockResolvedValue(undefined); mockPostReviewCommentReply.mockResolvedValue(undefined); mockGetPrMeta.mockResolvedValue({ @@ -172,6 +176,7 @@ afterEach(() => { delete process.env.GITHUB_EVENT_NAME; delete process.env.GITHUB_APP_TOKEN; delete process.env.ORG_MEMBERSHIP_TOKEN; + delete process.env.GITHUB_TOKEN; }); // --------------------------------------------------------------------------- @@ -744,3 +749,58 @@ describe('run() — pull_request_review_comment', () => { expect(prompt).toContain('IN_REPLY_TO_ID=77'); }); }); + +// --------------------------------------------------------------------------- +// run() — repo write-permission fallback (no ORG_MEMBERSHIP_TOKEN) +// --------------------------------------------------------------------------- + +describe('run() — repo write-permission fallback', () => { + beforeEach(() => { + delete process.env.ORG_MEMBERSHIP_TOKEN; + process.env.GITHUB_TOKEN = 'fake-github-token'; + }); + + it('calls checkRepositoryWritePermission instead of checkOrgMembership when no org token', async () => { + await run(); + + expect(mockCheckOrgMembership).not.toHaveBeenCalled(); + expect(mockCheckRepositoryWritePermission).toHaveBeenCalledWith( + 'fake-app-token', + 'docker', + 'myrepo', + 'alice', + ); + expect(core.setOutput).toHaveBeenCalledWith('should-reply', 'true'); + }); + + it('posts rejection when commenter lacks write permission', async () => { + mockCheckRepositoryWritePermission.mockResolvedValueOnce(false); + + await run(); + + expect(mockCheckRepositoryWritePermission).toHaveBeenCalled(); + expect(mockCheckOrgMembership).not.toHaveBeenCalled(); + expect(mockPostComment).toHaveBeenCalledWith( + 'fake-app-token', + 'docker', + 'myrepo', + 42, + expect.stringContaining(''), + ); + expect(core.setOutput).toHaveBeenCalledWith('should-reply', 'false'); + }); + + it('uses GITHUB_TOKEN as the repo token when GITHUB_APP_TOKEN is also absent', async () => { + delete process.env.GITHUB_APP_TOKEN; + + await run(); + + expect(mockCheckRepositoryWritePermission).toHaveBeenCalledWith( + 'fake-github-token', + 'docker', + 'myrepo', + 'alice', + ); + expect(core.setOutput).toHaveBeenCalledWith('should-reply', 'true'); + }); +}); diff --git a/src/mention-reply/index.ts b/src/mention-reply/index.ts index e6b3dd8..a99ce8b 100644 --- a/src/mention-reply/index.ts +++ b/src/mention-reply/index.ts @@ -28,7 +28,10 @@ import { readFileSync } from 'node:fs'; import * as core from '@actions/core'; import { addReaction, type CommentType } from '../add-reaction/index.js'; -import { checkOrgMembership } from '../check-org-membership/index.js'; +import { + checkOrgMembership, + checkRepositoryWritePermission, +} from '../check-org-membership/index.js'; import { getPrMeta, type PrMeta } from '../get-pr-meta/index.js'; import { postComment, postReviewCommentReply } from '../post-comment/index.js'; @@ -256,14 +259,16 @@ export async function run(): Promise { // Use the correct API endpoint based on comment type. await addReaction(token, ctx.owner, ctx.repo, ctx.commentId, 'eyes', ctx.commentType); - // 5. Org membership check - const orgToken = process.env.ORG_MEMBERSHIP_TOKEN ?? core.getInput('org-membership-token'); - if (!orgToken) throw new Error('ORG_MEMBERSHIP_TOKEN or org-membership-token input is required'); + // 5. Auth check: org membership when available, repo write permission as fallback + // for callers that supply a direct model API key instead of AWS-backed creds. + const orgToken = process.env.ORG_MEMBERSHIP_TOKEN || core.getInput('org-membership-token'); + const isMember = orgToken + ? await checkOrgMembership(orgToken, 'docker', ctx.commentAuthor) + : await checkRepositoryWritePermission(token, ctx.owner, ctx.repo, ctx.commentAuthor); - const isMember = await checkOrgMembership(orgToken, 'docker', ctx.commentAuthor); if (!isMember) { - core.info(`⏭️ ${ctx.commentAuthor} is not a docker org member — posting rejection`); - const rejectionBody = `Sorry @${ctx.commentAuthor}, I can only respond to Docker org members.\n\n`; + core.info(`⏭️ ${ctx.commentAuthor} is not authorized — posting rejection`); + const rejectionBody = `Sorry @${ctx.commentAuthor}, I can only respond to authorized contributors.\n\n`; try { // Reply in the same inline thread when triggered from an inline comment; // fall back to a PR-level Issues comment otherwise. @@ -287,7 +292,7 @@ export async function run(): Promise { core.setOutput('should-reply', 'false'); return; } - core.info(`✅ ${ctx.commentAuthor} is a docker org member`); + core.info(`✅ ${ctx.commentAuthor} is authorized`); // 6. Fetch PR metadata const pr = await getPrMeta(token, ctx.owner, ctx.repo, ctx.prNumber); From 9e7255deb415b8fab7bd201a8f7e14e5a7ece5d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Fri, 3 Jul 2026 18:51:45 +0200 Subject: [PATCH 2/2] github: Pin direct API key workflow actions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Paweł Gronowski --- .github/workflows/review-pr.yml | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/.github/workflows/review-pr.yml b/.github/workflows/review-pr.yml index a1be1df..219e93c 100644 --- a/.github/workflows/review-pr.yml +++ b/.github/workflows/review-pr.yml @@ -288,7 +288,9 @@ jobs: if: | steps.command.outputs.is_review != 'false' && steps.draft.outputs.skip != 'true' - uses: docker/docker-agent-action/setup-credentials@774b6e0e60d6c648b0f2dc43bd5221377a0a7420 # v2.0.2 + uses: docker/docker-agent-action/setup-credentials@5e46aee485641c251fdaeb9a05bd9977c711def8 # direct API key OIDC fix + with: + fetch-credentials: ${{ env.HAS_DIRECT_API_KEY != 'true' && 'true' || 'false' }} - name: Check if org member id: membership @@ -560,7 +562,9 @@ jobs: - name: Setup credentials if: steps.check.outputs.is_agent == 'true' && env.HAS_DIRECT_API_KEY != 'true' - uses: docker/docker-agent-action/setup-credentials@774b6e0e60d6c648b0f2dc43bd5221377a0a7420 # v2.0.2 + uses: docker/docker-agent-action/setup-credentials@5e46aee485641c251fdaeb9a05bd9977c711def8 # direct API key OIDC fix + with: + fetch-credentials: 'true' - name: Check authorization if: steps.check.outputs.is_agent == 'true' @@ -792,7 +796,9 @@ jobs: steps: - name: Setup credentials if: env.HAS_DIRECT_API_KEY != 'true' - uses: docker/docker-agent-action/setup-credentials@774b6e0e60d6c648b0f2dc43bd5221377a0a7420 # v2.0.2 + uses: docker/docker-agent-action/setup-credentials@5e46aee485641c251fdaeb9a05bd9977c711def8 # direct API key OIDC fix + with: + fetch-credentials: 'true' - name: Download trigger context if: inputs.trigger-run-id != '' @@ -858,7 +864,7 @@ jobs: - name: Run mention-reply handler id: mention-context if: steps.resolve-event.outputs.path != '' - uses: docker/docker-agent-action/.github/actions/mention-reply@896eb4c0871ac3e755d51a14a08d7fa551b87853 # direct API key OIDC fix + uses: docker/docker-agent-action/.github/actions/mention-reply@5e46aee485641c251fdaeb9a05bd9977c711def8 # direct API key OIDC fix env: GITHUB_EVENT_PATH: ${{ steps.resolve-event.outputs.path }} GITHUB_EVENT_NAME: ${{ steps.resolve-event.outputs.name }}