Skip to content

Fix reply-to-feedback authorization api#105

Merged
derekmisler merged 1 commit intodocker:mainfrom
derekmisler:fix-reply-to-feedback-authorization-api
Mar 20, 2026
Merged

Fix reply-to-feedback authorization api#105
derekmisler merged 1 commit intodocker:mainfrom
derekmisler:fix-reply-to-feedback-authorization-api

Conversation

@derekmisler
Copy link
Copy Markdown
Contributor

@derekmisler derekmisler commented Mar 19, 2026

Summary

This PR fixes the authorization check in the reply-to-feedback workflow step. The previous implementation relied solely on author_association from the GitHub event context (which can be unreliable in workflow_call contexts), replacing it with a direct GitHub API call to verify org membership. It also fixes a bug where the jq payload used in_reply_to_id instead of the correct in_reply_to field.

Changes

  • .github/workflows/review-pr.ymlauth step: Replaces the author_association-based authorization with an explicit gh api orgs/$ORG/members/$USERNAME membership check using CAGENT_ORG_MEMBERSHIP_TOKEN. Falls back to author_association if the token is not configured.
  • .github/workflows/review-pr.ymlauth step: Adds robust HTTP response parsing — validates the response starts with an HTTP status line, extracts the status code via grep -oP, and handles unexpected formats gracefully.
  • .github/workflows/review-pr.ymlNotify unauthorized user step: Adds validation that ROOT_COMMENT_ID is set and is a valid integer before passing it to jq/gh api, preventing silent failures.
  • .github/workflows/review-pr.ymlNotify unauthorized user step: Fixes the jq payload key from in_reply_to_id to in_reply_to, which is the correct field name for the GitHub Pull Request Review Comments API.

Breaking Changes

  • The CAGENT_ORG_MEMBERSHIP_TOKEN secret (scoped to org membership read) must be configured for the primary authorization path to work. Without it, the workflow falls back to author_association.
  • The auto-review-org input must be set correctly, as it is now used as the org name in the membership API call.

How to Test

  • Trigger a comment reply on a PR from a user who is a member of the configured org — verify authorized=true is set and the reply workflow proceeds.
  • Trigger a comment reply from a user who is not an org member — verify authorized=false is set and the unauthorized notification comment is posted correctly (and uses in_reply_to in the API payload).
  • Remove or leave CAGENT_ORG_MEMBERSHIP_TOKEN unset and verify the fallback to author_association logic works as expected with a warning logged.

Closes: https://github.com/docker/gordon/issues/260

@derekmisler derekmisler force-pushed the fix-reply-to-feedback-authorization-api branch 2 times, most recently from 399e491 to ef5516f Compare March 19, 2026 15:53
Signed-off-by: Derek Misler <derek.misler@docker.com>
@derekmisler derekmisler force-pushed the fix-reply-to-feedback-authorization-api branch from ef5516f to 5af8e59 Compare March 19, 2026 15:55
@derekmisler
Copy link
Copy Markdown
Contributor Author

/describe

@derekmisler derekmisler marked this pull request as ready for review March 19, 2026 15:55
@derekmisler derekmisler requested a review from a team as a code owner March 19, 2026 15:55
@docker-agent
Copy link
Copy Markdown
Contributor

docker-agent bot commented Mar 19, 2026

✅ PR description has been generated and updated!

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: 🟢 APPROVE

The changes improve authorization logic by switching from author_association to org membership checking with proper fallback. The validation of ROOT_COMMENT_ID and the API field name fix (in_reply_to_idin_reply_to) are good improvements.

Minor observations noted in inline comments for future consideration.

@derekmisler derekmisler enabled auto-merge (squash) March 20, 2026 12:41
@derekmisler derekmisler merged commit 6b67149 into docker:main Mar 20, 2026
15 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