slack-github-action icon indicating copy to clipboard operation
slack-github-action copied to clipboard

Added checks for bot token and webhook url length

Open koki-develop opened this issue 2 years ago • 5 comments

Summary

Fixed #123

Requirements (place an x in each [ ])

koki-develop avatar Sep 05 '22 08:09 koki-develop

The integration tests using env variables are failing. Need to investigate later

seratch avatar Sep 05 '22 09:09 seratch

Hmmm...

PRs created from Forked repositories cannot see the secret. As a result, empty strings are passed to SLACK_BOT_TOKEN and SLACK_WEBHOOK_URL on the integration test. This PR added a check for empty string, which caused the integration test to fail 🤔

screenshot

koki-develop avatar Sep 05 '22 09:09 koki-develop

The test log console does not display the env variable values for better security.

@stevengill Do you have any thoughts on this? I think that env variables should be correctly set but am still unsure why the length validation that is added in this PR cause the test failures.

seratch avatar Sep 06 '22 03:09 seratch

@seratch

The test log console does not display the env variable values for better security.

Yes, in the GitHub Actions log, the secret will appear as ***, like:

log

The screenshot attached to the previous comment does not even show ***. This means that an empty value has been set.

koki-develop avatar Sep 06 '22 03:09 koki-develop

As described in this document, secrets cannot be referenced from workflows triggered from a forked repository.

Note: With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

https://docs.github.com/en/actions/security-guides/encrypted-secrets#using-encrypted-secrets-in-a-workflow

For reference, if you want to refer to secrets from workflows triggered from a forked repository, you need to use the pull_request_target workflow trigger. However, this may be dangerous from a security standpoint, so care should be taken.

Due to the dangers inherent to automatic processing of PRs, GitHub’s standard pull_request workflow trigger by default prevents write permissions and secrets access to the target repository. However, in some scenarios such access is needed to properly process the PR. To this end the pull_request_target workflow trigger was introduced.

https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

koki-develop avatar Sep 06 '22 09:09 koki-develop

This is a pretty bad experience for security-conscious users who receive PRs from forks and aren't willing to use pull_request_target. Before, there were simply no notifications from forks, which was a feature in our case. After, CI fails on any PR from a fork, which is a breaking change in our case because we use Forking Renovate for dependency bumps.

Kurt-von-Laven avatar May 24 '23 18:05 Kurt-von-Laven

Since this PR changes current behaviour which just skips for missing / invalid tokens, it would be helpful to consider this as breaking change and bump major version...

sarisia avatar Aug 28 '23 06:08 sarisia