Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

自動マージが動かない問題がおそらく修正されます2 #17

Merged
merged 2 commits into from
Nov 21, 2023
Merged

自動マージが動かない問題がおそらく修正されます2 #17

merged 2 commits into from
Nov 21, 2023

Conversation

umi1299
Copy link
Contributor

@umi1299 umi1299 commented Nov 21, 2023

内容

フォークしたリポジトリからPRが作成されたとき、Actionsのトリガーが pull_request の場合と pull_request_target の場合では実行コンテキストと secrets.GITHUB_TOKEN に付与されるアクセス許可が変わるそうです
アクセス許可が足りなくて実行に失敗していると思われるのでトリガーを pull_request_target に変更しました

参考: GitHub Actionのon: pull_requeston: pull_request_targetの違いMEMO - Madogiwa Blog
ドキュメント: https://docs.github.com/ja/actions/using-workflows/events-that-trigger-workflows#pull_request_target

関連

その他

度々すみませんがマージと #11 のリベースを試していただきたいです🙇‍♂️

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!!

pull_request_targetですが、相当強力な権限を持つのでちょっとセキュリティ周りを気をつけたいと思います。
(悪用されるとGithubのsecretsが流出し、docker imageにウイルスを混入されてしまうなどがありえる)

まずこのファイルは外部ファイルの依存を絶たないといけなそうです。
npm installなどは極力避けるべきだと思います。

actions/checkoutはこちらのリポジトリがチェックアウトされるので、何かよからぬことをするファイルをインジェクションされることはなさそうです(悪意のあるコードがプルリク&マージを通らない限りは)。

あとはこのファイルがそのような権限を持っているということを把握し続ける必要があります。
とりあえずファイル名にDANGEROUSをつけさせていただこうと思います!
正直これは結構難しいです。別の誰かがプルリクエスト送った時などにもし気づいたらご指摘いただけると・・・ 🙇
(例えばDANGEROUSとついているファイルが変更された時に、そのプルリクエストに対して自動で注意してくれるような別のworkflowがあってもいいかも・・・?)

@Hiroshiba Hiroshiba merged commit e0f0069 into VOICEVOX:main Nov 21, 2023
3 checks passed
@umi1299 umi1299 deleted the fix-automerge-2 branch November 21, 2023 16:04
@umi1299
Copy link
Contributor Author

umi1299 commented Nov 21, 2023

#17 (review)

@Hiroshiba
トリガーが pull_request_target のActionはマージ先のブランチに存在するものが実行されるので、それを考えると変なPRをマージすることがなければ大丈夫と思いました
自動マージも第三者からのPRに対しては働かないですし

ファイル名に DANGEROUS を付けること、了解致しました
トリガーを pull_request_target にしていると強い secrets.GITHUB_TOKEN が取れることと secrets.BUMP_CASK_TOKEN も流出したらまずそうなことを考えると、triage.ymlautobump.yml にも付けますかね?

@Hiroshiba
Copy link
Member

あ、こちらのコメントを確認していませんでした!

triage.yml と autobump.yml にも付けますかね?

triage.ymlにもpull_request_targetが使われていたことを留意できていませんでした、ありがとうございます 🙇
こちらは確かに注意が必要だと思うので、↓のPRで勝手ながら追加させていただきました!

autobumpの方はsecretsにアクセスしていますが、secretsへのアクセスが危ないのは結構わかるので大丈夫かなと思いました!
でもこのステップでは強めの権限が渡るので、気をつけないとですね・・・。

ちなみにsecretsへアクセスするにはautobump内のコードのようにコード内に書くか、あとはenv経由で明示的に与えないと使えないので、ステップ内で明示的に与えない限りは流出しない感じです。
あとはまあファイル全体として強い権限があるわけではない(たぶん・・・?)ので、autobumpはDANGEROUS無しでも大丈夫なのかなと思いました!

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