Skip to content

Reject carried-forward receipts by default - #47

Open
jamilahmadzai wants to merge 1 commit into
permission-protocol:mainfrom
jamilahmadzai:fix/reject-carried-forward-receipts
Open

Reject carried-forward receipts by default#47
jamilahmadzai wants to merge 1 commit into
permission-protocol:mainfrom
jamilahmadzai:fix/reject-carried-forward-receipts

Conversation

@jamilahmadzai

Copy link
Copy Markdown

Summary

  • reject carried-forward receipt responses by default
  • add an explicit allow-carry-forward opt-in for repos that knowingly want PR-level approval carry-forward
  • document the stricter default in the README and install guide

Why

A carried-forward approval can keep the gate green after the PR head changes. Since the action cannot prove which head SHA the prior approval covered, the safer default is to fail closed and require a fresh approval for the current head unless carry-forward is explicitly enabled.

Related: #46
Bounty: #36

Validation

  • `ruby -ryaml -e 'YAML.load_file("action.yml"); puts "action.yml ok"'
  • extracted the embedded action script with GitHub expressions substituted and ran bash -n
  • git diff --check

Payout

PayPal: jamilurrehman722@gmail.com

@AAH20 AAH20 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Clean and well-scoped. The core change — inserting a CARRIED_FORWARD check between the API response parse and the set_output calls — is exactly where it belongs.

Key observations:

  1. Placement after VALID and before outputs — the guard at line ~375 flips VALID=false and sets PP_CARRY_FORWARD_DISABLED only when all three conditions hold (VALID=true, CARRIED_FORWARD=true, opt-in not enabled). This means a genuinely fresh receipt is never touched, and a failed/denied receipt is never double-errored.

  2. PP_CARRY_FORWARD_DISABLED error-code block — follows the same pattern as SIGNATURE_INVALID/KEY_REVOKED: prints details, calls upsert_pr_comment, exits 1. Consistent with the rest of the action.

  3. README ## Carry-forward approvals section — good call documenting the version boundary (v2.2.0) so users upgrading know when the behavior changed.

  4. One nit (non-blocking): the CARRIED_FORWARD variable is parsed from the API response with a fallback of "false". If the API ever returns the boolean false rather than the string "false", jq -r renders it as false which matches, so this is fine — just noting for posterity.

LGTM. Approving.

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