chore(mint): log requested and granted token scope - #1918
Conversation
Site previewPreview: https://03c0d80c-site.fullsend-ai.workers.dev Commit: |
ReviewFindingsMedium
Low
Info
Previous runReviewFindingsMedium
Low
Info
Previous run (2)ReviewFindingsMedium
Low
Info
Previous run (3)ReviewFindingsMedium
Low
Info
Previous run (4)ReviewFindingsMedium
Low
Info
|
fullsend-ai-review
Bot
added
the
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review squad (4 agents: claude-coder, claude-researcher, gemini, cursor) — 3 MEDIUM, 6 LOW, 3 INFO findings after dedup and verification (2 false positives removed). Posting the 2 actionable MEDIUM findings inline.
| MINT_RESPONSE=$(curl -sSf --retry 5 --retry-delay 5 --retry-all-errors \ | ||
| -H "Authorization: Bearer $OIDC_TOKEN" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d "$BODY" \ | ||
| "${MINT_URL}/v1/token" | jq -r '.token') | ||
| "${MINT_URL}/v1/token") | ||
| TOKEN=$(echo "$MINT_RESPONSE" | jq -r '.token') | ||
| if [[ -z "$TOKEN" || "$TOKEN" == "null" ]]; then | ||
| echo "::error::Token mint returned no token for role=$ROLE" | ||
| exit 1 | ||
| fi | ||
| echo "::add-mask::$TOKEN" |
There was a problem hiding this comment.
MEDIUM — Token masking window
The full JSON response (including the raw token) now lives in MINT_RESPONSE before ::add-mask:: is called. Previously the token was extracted inline (curl | jq -r '.token'), so the full response never existed in a named variable.
If ACTIONS_STEP_DEBUG=true, the runner traces shell variable assignments — the raw token in MINT_RESPONSE would appear in debug logs before the mask on line 65 takes effect.
Suggestion: Mask immediately after extraction, before any other processing of MINT_RESPONSE:
MINT_RESPONSE=$(curl -sSf --retry 5 --retry-delay 5 --retry-all-errors \
-H "Authorization: Bearer $OIDC_TOKEN" \
-H "Content-Type: application/json" \
-d "$BODY" \
"${MINT_URL}/v1/token")
TOKEN=$(echo "$MINT_RESPONSE" | jq -r '.token')
echo "::add-mask::$TOKEN" # mask before any further echo of MINT_RESPONSEThis closes the window between storing and masking.
There was a problem hiding this comment.
Pushed b064cf4. Moved the mask right after extraction, before the validity check.
| if granted != nil { | ||
| log.Printf("granted scope: repos=%v permissions=%v repo_selection=%s", | ||
| granted.Repos, granted.Permissions, granted.RepoSelection) | ||
| } |
There was a problem hiding this comment.
MEDIUM — No validation when granted scope diverges from requested scope
The PR's motivation (#1916) is investigating how downstream retro runs post issues cross-org. This logs granted scope but never compares it to requested scope. When repository_selection=all is returned, the token has access to all repos the app is installed on — not just the requested ones. That's the exact privilege escalation signal #1916 needs to detect, yet it's logged identically to a normal selected response.
Similarly, granted.Permissions is never compared against rolePermissions[role], so a permission-level divergence would go unnoticed.
Suggestion: Add comparison and severity-differentiated logging:
if granted != nil {
log.Printf("granted scope: repos=%v permissions=%v repo_selection=%s",
granted.Repos, granted.Permissions, granted.RepoSelection)
if granted.RepoSelection == "all" {
log.Printf("WARNING: token granted with repository_selection=all (requested specific repos: %v)", req.Repos)
}
requested := rolePermissions[req.Role]
for perm, level := range granted.Permissions {
if reqLevel, ok := requested[perm]; !ok {
log.Printf("WARNING: extra permission granted: %s=%s (not requested)", perm, level)
} else if level != reqLevel {
log.Printf("WARNING: permission level mismatch: %s requested=%s granted=%s", perm, reqLevel, level)
}
}
}This turns the observability plumbing into the detection capability #1916 actually needs.
There was a problem hiding this comment.
Pushed b064cf4. Added WARNING logs for repository_selection=all and for any permission that diverges from what the role defines in rolePermissions.
| GRANTED_REPOS=$(echo "$MINT_RESPONSE" | jq -r '.granted_repos // [] | join(",")') | ||
| GRANTED_PERMS=$(echo "$MINT_RESPONSE" | jq -r '.granted_permissions // {} | to_entries | map("\(.key)=\(.value)") | join(",")') | ||
| REPO_SELECTION=$(echo "$MINT_RESPONSE" | jq -r '.repository_selection // "unknown"') | ||
| echo "Granted scope: repos=${GRANTED_REPOS:-none} permissions=${GRANTED_PERMS:-none} repo_selection=${REPO_SELECTION}" |
There was a problem hiding this comment.
Why are you adding what it seems as default values? If the mint does not return anything we shouldn't mask that, it will be misleading.
There was a problem hiding this comment.
Yeah, that was misleading. Switched to // empty so jq produces nothing when the field is absent — the log line just shows blank values instead of fabricated ones.
rh-hemartin
left a comment
rh-hemartin
left a comment
There was a problem hiding this comment.
I agree with @waynesun09 comments, approving because I expect you to address them.
Move ::add-mask:: immediately after token extraction to close the debug-log exposure window. Remove fabricated jq defaults — log only what the mint actually returns. Add WARNING-level logs when GitHub grants repository_selection=all or permissions that diverge from what the role requested. Addresses review feedback from #1918. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Parse repositories, permissions, and repository_selection from GitHub's installation token response. Log both requested and granted scope server-side with WARNING when repository_selection=all or permissions diverge from the role definition. Return granted scope in the mint response so the mint-token action can log it client-side. Mask token immediately after extraction to close the debug-log exposure window. Use jq's // empty instead of fabricated defaults so absent fields produce blank output. Relates-to: #1916 Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
978ecc7 to
b0840dc
Compare
fullsend-ai-review
Bot
added
requires-manual-review
|
Heads up — the mintcore refactor landed on main while this was in review, so I had to rewrite the changes on top of the new structure. The logic is the same but it lives in |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review squad (7 agents) found 1 MEDIUM, 3 LOW, 3 INFO findings. Rebase onto mintcore refactor is clean, prior review issues are resolved, no regressions to identity token flows. The one actionable item is the one-directional permission comparison — adding under-scoping detection would complete the observability for #1916.
| for perm, level := range granted.Permissions { | ||
| if reqLevel, ok := requested[perm]; !ok { | ||
| log.Printf("WARNING: extra permission granted: %s=%s (not requested)", perm, level) | ||
| } else if level != reqLevel { | ||
| log.Printf("WARNING: permission level mismatch: %s requested=%s granted=%s", perm, reqLevel, level) | ||
| } | ||
| } |
There was a problem hiding this comment.
MEDIUM — Permission comparison is one-directional; missing permissions not detected
This loop iterates over granted.Permissions and checks each against requested, which catches extra permissions and level mismatches. However, it does not detect the inverse: a permission that was requested via RolePermissionsFor() but entirely absent from the granted set. If GitHub declines to grant a requested permission, the under-scoping goes unlogged.
Given that #1916 is investigating scope divergence, under-granting is arguably as important as over-granting for the observability picture.
Flagged in the prior review round and still unaddressed — 7/7 review agents agreed on this one.
Suggestion: Add a reverse loop after this block:
for perm, reqLevel := range requested {
if _, ok := granted.Permissions[perm]; !ok {
log.Printf("WARNING: requested permission not granted: %s=%s", perm, reqLevel)
}
}Add a reverse loop after the existing over-grant check to detect permissions that were requested via RolePermissionsFor() but absent from the granted set. This completes the scope divergence observability for #1916 — under-granting is now logged alongside over-granting and level mismatches. Closes review feedback from waynesun09 on PR #1918. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
fullsend-ai-review
Bot
added
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad Report (5 agents: 2x claude-coder, claude-researcher, gemini, cursor)
2 MEDIUM findings posted inline on action.yml. No critical or high issues found. Overall the PR is well-structured — code is consistent across all paired files, tests are thorough, and add-mask timing is improved over main. Both MEDIUM issues are straightforward shell fixes in the action script.
| echo "Requesting token: role=$ROLE repos=$REPOS" | ||
| MINT_RESPONSE=$(curl -sSf --retry 5 --retry-delay 5 --retry-all-errors \ | ||
| -H "Authorization: Bearer $OIDC_TOKEN" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d "$BODY" \ | ||
| "${MINT_URL}/v1/token" | jq -r '.token') | ||
| "${MINT_URL}/v1/token") | ||
| TOKEN=$(echo "$MINT_RESPONSE" | jq -r '.token') | ||
| echo "::add-mask::$TOKEN" | ||
| if [[ -z "$TOKEN" || "$TOKEN" == "null" ]]; then |
There was a problem hiding this comment.
MEDIUM — MINT_RESPONSE variable holds raw token without masking
MINT_RESPONSE holds the full JSON response (including the plaintext token) and is never masked. While TOKEN is masked after extraction, MINT_RESPONSE itself persists for the rest of the step. If ACTIONS_STEP_DEBUG is enabled or a future change echoes this variable, the token could leak. Previously the token was piped directly through jq and never stored in a named variable.
Suggestion — add echo "::add-mask::$MINT_RESPONSE" immediately after the curl, or unset MINT_RESPONSE after extracting all fields:
MINT_RESPONSE=$(curl -sSf --retry 5 --retry-delay 5 --retry-all-errors \
-H "Authorization: Bearer $OIDC_TOKEN" \
-H "Content-Type: application/json" \
-d "$BODY" \
"${MINT_URL}/v1/token")
echo "::add-mask::$MINT_RESPONSE"
TOKEN=$(echo "$MINT_RESPONSE" | jq -r '.token')
echo "::add-mask::$TOKEN"Flagged by 4/5 review agents.
There was a problem hiding this comment.
Fixed both in 7aedecc. Masked MINT_RESPONSE right after curl, and moved the null/empty check before the token mask so we never register "null" as a secret.
| -d "$BODY" \ | ||
| "${MINT_URL}/v1/token" | jq -r '.token') | ||
| "${MINT_URL}/v1/token") | ||
| TOKEN=$(echo "$MINT_RESPONSE" | jq -r '.token') | ||
| echo "::add-mask::$TOKEN" | ||
| if [[ -z "$TOKEN" || "$TOKEN" == "null" ]]; then |
There was a problem hiding this comment.
MEDIUM — Masking "null" string breaks subsequent logging on mint failure
::add-mask::$TOKEN runs before the null/empty check. If the mint returns "null" for the token, the literal string "null" gets registered as a secret, causing all subsequent occurrences of "null" in workflow logs to be replaced with *** for the remainder of the job.
Suggestion — move validation before the mask:
TOKEN=$(echo "$MINT_RESPONSE" | jq -r '.token')
if [[ -z "$TOKEN" || "$TOKEN" == "null" ]]; then
echo "::error::Token mint returned no token for role=$ROLE"
exit 1
fi
echo "::add-mask::$TOKEN"There was a problem hiding this comment.
Covered in the same commit — token validation now runs before the mask.
Mask MINT_RESPONSE immediately after curl to prevent token leakage if ACTIONS_STEP_DEBUG is enabled. Move the null/empty token check before ::add-mask::$TOKEN to avoid masking the literal string "null" which would corrupt all subsequent log output for the job. Both copies (.github/actions/ and scaffold) updated in sync. Addresses review feedback from waynesun09 on PR #1918. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
fullsend-ai-review
Bot
removed
the
requires-manual-review
fullsend-ai-review
Bot
added
the
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Both MEDIUM findings from the initial 5-agent review have been addressed in commit 7aedecc:
- MINT_RESPONSE masking —
::add-mask::$MINT_RESPONSEadded immediately after curl, closing the debug-log exposure window - Null token masking — validation moved before
::add-mask::$TOKEN, preventing the literal "null" from being registered as a secret
Re-reviewed with 2 agents post-fix: no remaining MEDIUM+ issues. All file pairs (source/embed, root/scaffold) are byte-identical. Tests pass. LGTM.
Retro: PR #1918 —
|
3 participants
Summary
repositories,permissions, andrepository_selectionfrom GitHub's installation token response (fields we were discarding)mint-tokenaction can log it in workflow logs tooRelates to #1916 — gets us the observability needed to investigate how downstream retro runs are posting issues cross-org.
Test plan
TestHandler_FullFlowupdated to verify granted scope plumbing end-to-endmake go-testandmake lintpassgranted scope:lineRequesting token:/Granted scope:output🤖 Generated with Claude Code