Skip to content

feat: replace trusted-bot-app-id with trusted-bot-token PAT input#175

Merged
derekmisler merged 5 commits into
docker:mainfrom
docker-agent:feat/remove-trusted-bot-app-id
May 5, 2026
Merged

feat: replace trusted-bot-app-id with trusted-bot-token PAT input#175
derekmisler merged 5 commits into
docker:mainfrom
docker-agent:feat/remove-trusted-bot-app-id

Conversation

@docker-agent
Copy link
Copy Markdown
Contributor

Summary

Follow-up to #174. Replaces the App-ID-based trusted-bot bypass with a PAT-based one, and cleans up remaining GitHub App credential references throughout the non-src/ codebase.

Refs docker/gordon#500

Motivation

trusted-bot-app-id relied on comment.performed_via_github_app.id — only fires when the comment was posted by a GitHub App. With PAT-based credentials, comments are posted by a regular bot account, so performed_via_github_app is null and the bypass never triggered.

Changes

Core: trusted-bot bypass rewritten

File Change
action.yml trusted-bot-app-id input → trusted-bot-token; env var renamed; bypass block uses gh api /user to resolve login, compares comment.user.login; token masked at top of step
review-pr/action.yml Input renamed; both forwarding calls updated
.github/workflows/review-pr.yml trusted-bot-token: ${{ env.GITHUB_APP_TOKEN }} (was trusted-bot-app-id: ${{ env.GITHUB_APP_ID }})

Cleanup: deprecated inputs and scripts

File Change
.github/workflows/review-pr.yml Removed deprecated CAGENT_REVIEWER_APP_ID, CAGENT_REVIEWER_APP_PRIVATE_KEY, CAGENT_ORG_MEMBERSHIP_TOKEN workflow_call inputs
.github/actions/setup-credentials/action.yml Description updated (removed "generate GitHub App token")
scripts/act-local.sh Rewritten: reads PAT from 1Password (op://Team AI Agent/Docker Agent GitHub Action/pat) directly; removes @octokit/auth-app token-minting step
scripts/debug-permissions.ts Deleted (App-specific debug tool, no longer relevant)
README.md, review-pr/README.md trusted-bot-token row added to inputs table

Validation

  • pnpm build
  • pnpm biome ci .
  • pnpm tsc --noEmit
  • pnpm test — 103/103 unit tests pass ✅

Replaces the App-ID-based trusted-bot bypass with a PAT-based one.

Old approach: `trusted-bot-app-id` (integer) checked
`comment.performed_via_github_app.id` — only works for GitHub App identities.

New approach: `trusted-bot-token` (PAT) resolves the associated login via
`gh api /user` and checks `comment.user.login` — works for any bot account
including PAT-posting bots.

Changes:
- action.yml: input renamed; env var renamed; bypass block rewired
- review-pr/action.yml: input renamed; forwarding calls updated
- .github/workflows/review-pr.yml: trusted-bot-token wired to
  env.GITHUB_APP_TOKEN; deprecated CAGENT_REVIEWER_APP_ID /
  CAGENT_REVIEWER_APP_PRIVATE_KEY / CAGENT_ORG_MEMBERSHIP_TOKEN
  workflow_call inputs removed
- .github/actions/setup-credentials/action.yml: description updated
- scripts/act-local.sh: rewritten to read PAT from 1Password directly
  (removes @octokit/auth-app token-minting step)
- scripts/debug-permissions.ts: deleted (App-specific debug tool)
- README.md, review-pr/README.md: trusted-bot-token row added

Refs docker/gordon#500

Assisted-By: docker-agent
@docker-agent docker-agent requested a review from a team as a code owner May 5, 2026 15:37
@docker-agent docker-agent requested a review from derekmisler May 5, 2026 15:37
Copy link
Copy Markdown
Contributor

@docker-agent docker-agent Bot left a comment

Choose a reason for hiding this comment

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

Assessment: 🟡 NEEDS ATTENTION

One medium-severity finding in the new PAT-based trusted-bot bypass logic. The bypass itself is functionally correct when the token is valid, but silent failure on token resolution errors reduces observability.

Comment thread action.yml Outdated
Copy link
Copy Markdown
Contributor Author

@docker-agent docker-agent left a comment

Choose a reason for hiding this comment

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

Assessment: 🟢 APPROVE

PAT migration from GitHub App ID to trusted-bot-token looks correct and complete. The security model is sound — comment.user.login is server-set and not spoofable. Two minor findings below.

Comment thread action.yml Outdated
Comment thread .github/workflows/review-pr.yml Outdated
Assisted-By: docker-agent
Removes github-app-id, github-app-private-key, and trusted-bot-token inputs
from both action.yml and review-pr/action.yml. The resolved token is now
always: inputs.github-token if provided, else github.token.

The trusted-bot bypass is rewritten to always use that resolved token —
it resolves the GitHub login via `gh api /user` and compares against
comment.user.login. If login resolution fails, a ::warning:: is emitted.

Changes:
- action.yml: removed 3 inputs; removed create-github-app-token step;
  simplified resolve-token to explicit-vs-default; inlined token
  resolution in auth step for trusted-bot bypass
- review-pr/action.yml: same input/step removals; removed trusted-bot-token
  from both cagent-action call sites
- .github/workflows/review-pr.yml: removed trusted-bot-token line (github-token
  already set on the same step)
- README.md, review-pr/README.md: removed github-app-id, github-app-private-key,
  trusted-bot-token rows

Assisted-By: docker-agent
Copy link
Copy Markdown
Contributor Author

@docker-agent docker-agent left a comment

Choose a reason for hiding this comment

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

Assessment: 🟡 NEEDS ATTENTION

Auth bypass design concerns — 2 medium-severity findings about the new trusted-bot bypass logic.

The new mechanism (resolve token → compare login) is simpler and removes the App ID secret requirement, but loses the explicit Bot-type guard from the old logic. The attack surface for GITHUB_TOKEN is protected by GitHub's reserved [bot] suffix, but PAT-based deployments may exhibit unexpected bypass behavior.

Comment thread action.yml
TRUSTED_BOT_LOGIN=$(GH_TOKEN="$ACTIVE_TOKEN" gh api /user --jq '.login' 2>/tmp/gh_api_err || echo "")
if [ -z "$TRUSTED_BOT_LOGIN" ]; then
echo "::warning::Could not resolve bot login from github-token ($(cat /tmp/gh_api_err)); trusted-bot bypass will not apply"
elif [ "$COMMENT_USER_LOGIN" = "$TRUSTED_BOT_LOGIN" ]; then
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[MEDIUM] Trusted-bot bypass semantics shift: PAT identity conflates bot with human author

The new bypass logic resolves gh api /user with the active token and compares the returned login against COMMENT_USER_LOGIN. When github-token is a PAT, the resolved login is the PAT owner's own human username. If that person comments on a PR, the bypass fires — they skip org-membership and author_association checks entirely.

This is a semantic change from the old design, where bypass required an explicit TRUSTED_BOT_APP_ID secret and the comment had to carry a matching numeric App ID. The new design makes bypass implicit for whoever owns the token.

Concrete scenario: if github-token is a PAT owned by alice, and alice (a non-member) comments /review on a PR, she bypasses auth unconditionally — no org check, no association check.

For GITHUB_TOKEN (no explicit github-token): the resolved login is github-actions[bot]. GitHub reserves the [bot] suffix, so human accounts cannot collide with this name — that specific attack path is blocked. But the design still implicitly trusts any account whose login matches the token's resolved identity, which is a wider surface than the old opt-in App ID check.

Suggested fix: add a guard to verify comment.user.type == "Bot" before allowing the bypass (restoring the original type check), or document clearly that github-token must not be a human PAT when bot-bypass is intended.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The github-token here is owned by a machine user account (type User, not Bot). Adding a comment.user.type == "Bot" guard would silently break the bypass for machine user deployments — their comments have user.type = "User" and would never match.

Login equality is sufficient as the security boundary: GitHub enforces global username uniqueness, so no human account can register the same login as the machine user. The [bot] suffix protection applies to the github.token fallback path (where the actor is github-actions[bot]); for custom PATs the equivalent protection is username uniqueness.

Added an inline comment in 17b6a5f explaining this.

Comment thread action.yml
# Always attempt trusted-bot bypass using the resolved github-token.
# Resolves the token's GitHub login via the API, then compares against the comment author.
ACTIVE_TOKEN="${INPUT_GITHUB_TOKEN:-$DEFAULT_GH_TOKEN}"
TRUSTED_BOT_LOGIN=$(GH_TOKEN="$ACTIVE_TOKEN" gh api /user --jq '.login' 2>/tmp/gh_api_err || echo "")
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[MEDIUM] Bot-type guard removed — bypass now reachable by human token owners

The old logic enforced comment.user.type == "Bot" as an explicit gate before the App ID comparison. This meant: even if someone somehow had a matching login, the bypass only fired if GitHub's own event payload marked the account as a Bot type.

The new code has no user.type check. For GITHUB_TOKEN (github-actions[bot]) the [bot] suffix reservation protects against impersonation by human accounts — that's fine. But for a custom github-token (PAT or App installation token where the login doesn't carry [bot]), there is no type gate, and the bypass fires purely on login-string equality.

Defense-in-depth concern: consider re-adding the type check from the event payload:

COMMENT_USER_TYPE=$(jq -r '.comment.user.type // empty' "$GITHUB_EVENT_PATH")
if [ "$COMMENT_USER_TYPE" = "Bot" ] && [ "$COMMENT_USER_LOGIN" = "$TRUSTED_BOT_LOGIN" ]; then

This costs one extra jq call and restores the original defence against human accounts inadvertently or intentionally matching the token identity.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The github-token here is owned by a machine user account (type User, not Bot). Adding a comment.user.type == "Bot" guard would silently break the bypass for machine user deployments — their comments have user.type = "User" and would never match.

Login equality is sufficient as the security boundary: GitHub enforces global username uniqueness, so no human account can register the same login as the machine user. The [bot] suffix protection applies to the github.token fallback path (where the actor is github-actions[bot]); for custom PATs the equivalent protection is username uniqueness.

Added an inline comment in 17b6a5f explaining this.

@derekmisler derekmisler merged commit eb8e137 into docker:main May 5, 2026
10 checks passed
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.

2 participants