Skip to content

fix: use GitHub Teams for E2E test authorization#1117

Open
notgitika wants to merge 1 commit intomainfrom
fix/e2e-auth-github-teams
Open

fix: use GitHub Teams for E2E test authorization#1117
notgitika wants to merge 1 commit intomainfrom
fix/e2e-auth-github-teams

Conversation

@notgitika
Copy link
Copy Markdown
Contributor

Summary

Replace the AUTHORIZED_USERS secret-based authorization check in the E2E Tests workflow with a GitHub Teams membership check against aws/agentcore-devex-devs.

Problem

The AUTHORIZED_USERS repo secret was silently overwritten on Apr 29, causing all E2E tests to skip on every PR since then with no visible error. The authorize job "passed" but set is_authorized=false, so the e2e job silently skipped. GitHub secrets are write-only — there's no way to see the current value or audit who changed it.

Solution

Use the GitHub Teams API to check if the PR author is a member of aws/agentcore-devex-devs:

  • Team membership is managed by org admins, not repo-level settings
  • Changes are tracked in GitHub's org audit log
  • Cannot be silently overwritten by anyone with repo write access
  • New team members automatically get E2E access without secret updates

Setup Required

A repo admin needs to create a TEAM_CHECK_TOKEN secret with a GitHub PAT that has read:org scope. This token is used to query team membership via the GitHub API.

Changes

  • .github/workflows/e2e-tests.yml — Replace bash secret check with actions/github-script@v7 team membership check

Test plan

  • Create TEAM_CHECK_TOKEN secret with read:org PAT
  • Open a test PR and verify the authorize job checks team membership
  • Verify E2E tests run for team members
  • Verify E2E tests skip for non-team members with a clear message

…RS secret

Replace the AUTHORIZED_USERS secret-based check with a GitHub Teams
membership check against aws/agentcore-devex-devs. The secret was
silently overwritten on Apr 29, causing all E2E tests to skip on PRs
with no visible error.

Team membership is managed by org admins and changes are tracked in
GitHub's org audit log, preventing silent overwrites.

Requires a TEAM_CHECK_TOKEN secret (PAT with read:org scope) to query
team membership via the GitHub API.
@notgitika notgitika requested a review from a team May 4, 2026 22:01
@github-actions github-actions Bot added size/s PR size: S agentcore-harness-reviewing AgentCore Harness review in progress labels May 4, 2026
@agentcore-cli-automation
Copy link
Copy Markdown

Silent-skip failure mode is reintroduced in the error handler

The PR description explains the bug being fixed: when AUTHORIZED_USERS was silently overwritten, the authorize job "passed" but set is_authorized=false, causing E2E tests to silently skip with no visible error. The new implementation re-creates this exact failure mode via the blanket catch block:

} catch (error) {
  core.info(`⏭️ User ${context.actor} is not a member of aws/agentcore-devex-devs — skipping E2E tests.`);
  core.info('ℹ️ External contributors: ask a maintainer to run the E2E tests manually via workflow_dispatch.');
  core.setOutput('is_authorized', 'false');
}

getMembershipForUserInOrg returns 404 when the user is genuinely not a member, but it also throws on:

  • 401 UnauthorizedTEAM_CHECK_TOKEN is missing, revoked, or its read:org scope was removed
  • 401/403 — the PAT expired (fine-grained PATs max out at ~1 year and typically expire sooner)
  • 403 rate-limit exceeded
  • 5xx — transient GitHub API outage
  • 404 — if the team was renamed/deleted, or if the PAT owner is not an org member (some endpoints 404 rather than 403 in that case)

In all of these cases, every E2E run will silently skip for every user — team members included — and the authorize job will report success. That's the exact behavior this PR is trying to prevent, just with a different root cause (PAT lifecycle / misconfig instead of secret overwrite).

Suggested fixes (pick one)

Option A — Discriminate on status code. Treat only error.status === 404 as "not a member" and rethrow (or core.setFailed) on everything else, so a broken token turns the authorize job red instead of silently green:

} catch (error) {
  if (error.status === 404) {
    core.info(`⏭️ User ${context.actor} is not a member of aws/agentcore-devex-devs — skipping E2E tests.`);
    core.info('ℹ️ External contributors: ask a maintainer to run the E2E tests manually via workflow_dispatch.');
    core.setOutput('is_authorized', 'false');
    return;
  }
  core.setFailed(`Team membership check failed (status ${error.status}): ${error.message}. Check that TEAM_CHECK_TOKEN is valid and has read:org scope.`);
}

Option B — Fail-closed with a loud signal. Keep the current skip behavior but core.warning() (or core.error()) on non-404 errors so they surface in the PR checks summary instead of being buried in info logs.

Option C — Use listMembersInOrg / list team members once and cache. Less sensitive to per-user 404 vs. 403 ambiguity, but doesn't solve the expired-PAT problem on its own.

Option A is probably the right call — the whole point of this PR is to make auth failures visible rather than silent.

@agentcore-cli-automation
Copy link
Copy Markdown

github.actor is not the PR author on synchronize/reopened (pre-existing, but worth fixing here)

This workflow runs on pull_request_target and checks out github.event.pull_request.head.sha (attacker-controlled code from the fork) while exposing E2E_AWS_ROLE_ARN, E2E_SECRET_ARN (which contains Anthropic/OpenAI/Gemini API keys), and CDK_REPO_TOKEN. Authorization is the only thing gating those secrets.

github.actor is not the PR author — it's whoever triggered the current event. For pull_request_target that includes synchronize and reopened. So:

  1. External contributor opens a PR from their fork.
  2. A team member (e.g., reviewing and triaging) reopens the PR, or a bot/admin pushes a commit to the fork branch (GitHub supports this via "Allow edits by maintainers").
  3. github.actor = team member → authorize passes → E2E runs against the external contributor's HEAD with full secret access.

This is pre-existing behavior from the AUTHORIZED_USERS implementation, but since this PR is explicitly re-working the authorization model for a workflow that runs untrusted code with privileged credentials, it's worth fixing in the same change.

Suggested fix

Check the PR author, not the event actor. Use github.event.pull_request.user.login when the event is a pull request, and fall back to github.actor for workflow_dispatch:

const username = context.eventName === 'workflow_dispatch'
  ? context.actor
  : context.payload.pull_request.user.login;

const { data } = await github.rest.teams.getMembershipForUserInOrg({
  org: 'aws',
  team_slug: 'agentcore-devex-devs',
  username,
});

This ensures the team-membership check is evaluated against the person whose code is actually being executed, regardless of who happened to trigger the event.

@github-actions github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label May 4, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 43.04% 8989 / 20883
🔵 Statements 42.32% 9544 / 22550
🔵 Functions 39.86% 1550 / 3888
🔵 Branches 39.92% 5794 / 14511
Generated in workflow #2382 for commit 02311be by the Vitest Coverage Report Action

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

Labels

size/s PR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants