slack-github-action
slack-github-action copied to clipboard
Added checks for bot token and webhook url length
Summary
Fixed #123
Requirements (place an x
in each [ ]
)
- [x] I've read and understood the Contributing Guidelines and have done my best effort to follow them.
- [x] I've read and agree to the Code of Conduct.
The integration tests using env variables are failing. Need to investigate later
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 🤔
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
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:

The screenshot attached to the previous comment does not even show ***
.
This means that an empty value has been set.
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 thepull_request_target
workflow trigger was introduced.https://securitylab.github.com/research/github-actions-preventing-pwn-requests/
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.
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...