Security fix for self-comment-ci.yml
#35548
Conversation
…)" This reverts commit ccc4a5a.
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
| id: get_sha | ||
| env: | ||
| PR_NUMBER: ${{ needs.get-pr-number.outputs.PR_NUMBER }} | ||
| COMMENT_DATE: ${{ github.event.comment.created_at }} |
There was a problem hiding this comment.
COMMENT_DATE: ${{ github.event.comment.created_at }}
new for security
| git fetch origin refs/pull/$PR_NUMBER/merge:refs/remotes/pull/$PR_NUMBER/merge | ||
| git checkout refs/remotes/pull/$PR_NUMBER/merge | ||
| PR_MERGE_COMMIT_TIMESTAMP=$(git log -1 --date=unix --format=%cd) | ||
| echo "PR_MERGE_COMMIT_TIMESTAMP: $PR_MERGE_COMMIT_TIMESTAMP" | ||
| COMMENT_TIMESTAMP=$(date -d "${COMMENT_DATE}" +"%s") | ||
| echo "PR_HEAD_SHA: $COMMENT_DATE" | ||
| echo "COMMENT_TIMESTAMP: $COMMENT_TIMESTAMP" | ||
| if [ $COMMENT_TIMESTAMP -le $PR_MERGE_COMMIT_TIMESTAMP ]; then | ||
| echo "Last commit on the pull request is newer than the issue comment triggering this run! Abort!"; | ||
| exit -1; | ||
| fi |
There was a problem hiding this comment.
new for security
from the line git fetch origin refs/pull/$PR_NUMBER/merge:refs/remotes/pull/$PR_NUMBER/merge to here
There was a problem hiding this comment.
Currently I am using the merge commit.
(merge commit = PR head commit merged with main )
Merge commit might change if there is a push or merge to the main branch. So it would be better to use the head commit of the PR instead.
There was a problem hiding this comment.
btw, I am not sure $COMMENT_TIMESTAMP -le $PR_MERGE_COMMIT_TIMESTAMP check 100% safe, the commit date might be manually specified to be in the past 🥲
There was a problem hiding this comment.
however, this approach with comment message should be safer than triggering with contributor's commit message, because we control it
There was a problem hiding this comment.
Hi @qubvel This PR doesn't use users' input for date calculation. The merge commit refs/remotes/pull/$PR_NUMBER/merge is prepared by github itself.
But even if it's the date from the user (i.e. the actual commit from the contributor), would that date be manipulated?
There was a problem hiding this comment.
This PR doesn't use users' input for date calculation. The merge commit refs/remotes/pull/$PR_NUMBER/merge is prepared by github itself.
Looks good then, thanks for clarification!
But even if it's the date from the user (i.e. the actual commit from the contributor), would that date be manipulated?
I hope no! But security issue was an initial concern to disable the feature, wasn't it?
There was a problem hiding this comment.
I hope no! But security issue was an initial concern to disable the feature, wasn't it?
Yes. At that time, there is no check on date at all. But now the concern is about if the commit date could be manipulated.
Anyway, if we use merge commit as mentioned above, it's fine :-)
There was a problem hiding this comment.
Hmm... if merge commit is created when CI is started wouldn't it be always later than the comment?
-> user commit
-> message to trigger CI (COMMENT_TIMESTAMP)
-> CI triggered
-> merge commit created (PR_MERGE_COMMIT_TIMESTAMP)
There was a problem hiding this comment.
Sorry, I should be more clear. The merge commit is prepared by github when a commit is pushed to the PR branch or the target branch of the PR (usually main) is updated.
So the flow is
-> user commit (pushed)
-> merge commit created (PR_MERGE_COMMIT_TIMESTAMP)
-> message to trigger CI (COMMENT_TIMESTAMP)
-> CI triggered
|
a failing run looks like this |
| id: get_sha | ||
| env: | ||
| PR_NUMBER: ${{ needs.get-pr-number.outputs.PR_NUMBER }} | ||
| COMMENT_DATE: ${{ github.event.comment.created_at }} |
No description provided.