feat: check job permissions and warn if not enough - #1410
Conversation
github-actions
Bot
added
the
enhancement
|
@inimaz can you confirm that this works through some minimal testing? FYI, to test:
|
Nice! I didn't know this was possible, thanks on it! |
| case "${scope}" in | ||
| contents) | ||
| probe "repos/${REPO}/contents/" || \ | ||
| echo "::warning::Missing permission '${perm}'. Add 'contents: read' (or higher) to your workflow's permissions block." |
There was a problem hiding this comment.
I was wondering how easy it would be to print an exact warning message i.e. instead of "contents: read (or higher)", "contents: write" is printed (which we could detect from inputs.permissions).
There was a problem hiding this comment.
Indeed! The issue is that we cannot distinguish for now read and write permissions. Since normally read is a prerequisite for write I only test read things in the probe() function, like this I do not write anything. For future versions we can try to find write actions that are harmless per each permission?
There was a problem hiding this comment.
Maybe we should change the wording of the warning message then. For instance, we can add a link pointing to the documentation of the action (which already contains information about the minimum permissions required).
| ;; | ||
| *) | ||
| echo "::notice::Unknown permission scope '${scope}' — skipping verification." | ||
| ;; |
There was a problem hiding this comment.
If any of the checks fail, we can fail the action early by emitting exit code 1, since the action won't work correctly anyways if the permissions aren't set properly.
There was a problem hiding this comment.
Sure!
Up to us to decide. Maybe there are some actions that work with a subset of the permissions?
For example I was using the doc-deploy-dev without the pull-requests:write permission and it was working fine on its own, until I added the doc-deploy-pr that creates comments on prs --> then doc-deploy-dev needs the permission to comment that on a pr to close previously-opened comments .
TLDR: some jobs still work without the permissions because they do just a subset of all the actions, should we fail them and tell them to add the proper permission?
There was a problem hiding this comment.
Based on your explanation in the other comment, until we are able to test write permissions, we cannot fail all workflows.
It can however be done for workflows that require only read permissions, so maybe we do that first and re-iterate in the future like you suggested.
On the doc-deploy-dev case, your observation is correct. We will have to treat it as a special case. For example, we can split this step into multiple steps: the first checks if there is something to be removed and sets a flag, the next then checks for write permissions, and the last does the actually removal. That way, we are able to check for write permissions only when needed. You see what I mean?
But of course, that still depends on being able to test write permissions in the first place.
| return 0 | ||
| } | ||
|
|
||
| IFS=',' read -ra PERMS <<< "${PERMISSIONS}" |
There was a problem hiding this comment.
To condense the repeating echo statements, could you do something like this? I understand if the case structure is more readable though
# Define the endpoints and suggested permissions as associative arrays
declare -A endpoints=(
[contents]="repos/${REPO}/contents/"
[pull-requests]="repos/${REPO}/pulls?per_page=1"
[issues]="repos/${REPO}/issues?per_page=1"
[actions]="repos/${REPO}/actions/runs?per_page=1"
[packages]="repos/${REPO}/packages?package_type=container"
[pages]="repos/${REPO}/pages"
)
declare -A suggestions=(
[contents]="contents: read"
[pull-requests]="pull-requests: read"
[issues]="issues: read"
[actions]="actions: read"
[packages]="packages: read"
[pages]="pages: read"
)
IFS=',' read -ra PERMS <<< "${PERMISSIONS}"
for perm in "${PERMS[@]}"; do
perm=$(echo "${perm}" | xargs)
scope=$(echo "${perm}" | cut -d: -f1 | xargs)
if [[ -n "${endpoints[$scope]}" ]]; then
probe "${endpoints[$scope]}" || \
echo "::warning::Missing permission '${perm}'. Add '${suggestions[$scope]}' (or higher) to your workflow's permissions block."
elif [[ "$scope" == "id-token" ]]; then
[ -z "${ACTIONS_ID_TOKEN_REQUEST_URL:-}" ] && \
echo "::warning::Missing permission '${perm}'. Add 'id-token: write' to your workflow's permissions block."
elif [[ "$scope" == "attestations" ]]; then
[ -z "${ACTIONS_ID_TOKEN_REQUEST_URL:-}" ] && \
echo "::warning::Missing permission '${perm}'. Add 'attestations: write' and 'id-token: write' to your workflow's permissions block."
else
echo "::notice::Unknown permission scope '${scope}' — skipping verification."
fi
done
6 participants
Related to #1391.
probefunction tests the token against an action in the API that uses the given permission.writepermissions not checked for now... Since one has to find a harmless action to run per each permission. We can think of something in future PRs if needed.doc-deploy-devas an example.