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.

Sign up for GitHub

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

Jump to bottom

[ci] CircleCI Test skipping Danger due to access #1143

Open
janbrasna opened this issue Feb 24, 2022 · 8 comments · May be fixed by #1144
Open

[ci] CircleCI Test skipping Danger due to access #1143

janbrasna opened this issue Feb 24, 2022 · 8 comments · May be fixed by #1144

Comments

@janbrasna
Copy link
Contributor

Missing DANGER_GITHUB_* tokens, Danger fails with:

"Could not set up API to Code Review site for Danger"

results of: bundle exec danger || echo "danger failed, moving on"

e. g. Test3130:

For your GitHub repo, you need to expose: DANGER_GITHUB_API_TOKEN, DANGER_GITHUB_BEARER_TOKEN
You may also need: DANGER_GITHUB_HOST, DANGER_GITHUB_API_BASE_URL, DANGER_OCTOKIT_VERIFY_SSL

Found these keys in your ENV: CIRCLE_WORKFLOW_JOB_ID, CIRCLE_PR_NUMBER, CIRCLE_PULL_REQUESTS, HOSTNAME, JAVA_HOME, SSH_AUTH_SOCK, CIRCLE_REPOSITORY_URL, CIRCLE_WORKING_DIRECTORY, CIRCLE_INTERNAL_CONFIG, CIRCLECI, RUBY_DOWNLOAD_SHA256, CI_PULL_REQUESTS, CIRCLE_PROJECT_REPONAME, CIRCLE_WORKFLOW_UPSTREAM_JOB_IDS, YARN_VERSION, RUBY_VERSION, PWD, CIRCLE_WORKFLOW_ID, BUNDLE_APP_CONFIG, RUBY_MAJOR, CIRCLE_PULL_REQUEST, CIRCLE_USERNAME, CIRCLE_BRANCH, HOME, LANG, CIRCLE_PROJECT_USERNAME, CIRCLE_BUILD_NUM, CA_CERTIFICATES_JAVA_VERSION, CIRCLE_NODE_TOTAL, BUNDLE_SILENCE_ROOT_WARNING, CIRCLE_SHA1, GEM_HOME, NO_PROXY, JAVA_DEBIAN_VERSION, CI_PULL_REQUEST, SHLVL, CIRCLE_BUILD_URL, BASH_ENV, CIRCLE_NODE_INDEX, CIRCLE_COMPARE_URL, CIRCLE_PREVIOUS_BUILD_NUM, CIRCLE_WORKFLOW_WORKSPACE_ID, CIRCLE_PR_USERNAME, CIRCLE_STAGE, CIRCLE_JOB, LC_ALL, CIRCLE_SHELL_ENV, XAR_VERSION, PATH, CIRCLE_INTERNAL_SCRATCH, CI, CIRCLE_INTERNAL_TASK_DATA, CIRCLE_PR_REPONAME, NODE_VERSION, DEBIAN_FRONTEND, JAVA_VERSION, _, BUNDLER_ORIG_BUNDLE_BIN_PATH, BUNDLER_ORIG_BUNDLE_GEMFILE, BUNDLER_ORIG_BUNDLER_VERSION, BUNDLER_ORIG_GEM_HOME, BUNDLER_ORIG_GEM_PATH, BUNDLER_ORIG_MANPATH, BUNDLER_ORIG_PATH, BUNDLER_ORIG_RB_USER_INSTALL, BUNDLER_ORIG_RUBYLIB, BUNDLER_ORIG_RUBYOPT, BUNDLE_BIN_PATH, BUNDLE_GEMFILE, BUNDLER_VERSION, RUBYOPT, RUBYLIB, MANPATH.

Failing the build, Danger cannot run without API access.
You can see more information at https://danger.systems/guides/getting_started.html
danger failed, moving on
CircleCI received exit code 0
@janbrasna
Copy link
Contributor Author

Just an observation. Not sure if anyone relies on it. I first noticed it in #1085 (comment) in what should've triggered a danger, but went silent. 🤷‍♂️

@rogerluan
Copy link
Member

Hey @janbrasna 👋

Interesting find. This was added here: #288 but I don't know the rationale behind it. It probably made sense back then, but it doesn't anymore. I'll invite @nafu to take a look at this and shed some light if he can 🙇

But I think we can safely move forward with the removal of that silent failure.

I checked and current master of this repo passes on Danger 👌

I'll go ahead and remove the fallback that causes the silent failure 😊

Nice spotting!

@rogerluan rogerluan linked a pull request Feb 26, 2022 that will close this issue
Open
@rogerluan
Copy link
Member

Here it is #1144

Thanks for opening this issue @janbrasna ! 🤗

@rogerluan
Copy link
Member

Ah… Now I get what's going on in CI haha nevermind my comments above about it succeeding in danger. I ran a dry_run locally, only.

We'll need to figure out the GitHub Token situation, which's not trivial since this is an open source repo and danger kinda self-protects repos from exposing DANGER_GITHUB_API_TOKEN 😬

Do you know what needs to be changed there? I haven't dug further into danger's set up, but I'm assuming DANGER_GITHUB_API_TOKEN isn't set in our CI for that reason

@janbrasna

This comment was marked as outdated.

Sign in to view
@janbrasna
Copy link
Contributor Author

From what I see in the PRs, those opened by a bot these days have a danger/danger test in the checks explicitly:

Screenshot 2022-07-03 at 22 24 27

others don't:

Screenshot 2022-07-03 at 22 24 13

🤷‍♂️

So maybe the right way is to move it outside the CircleCI and keep it as a separate check only? However I don't have enough visibility into when that check is triggered by the fastlane bot TBH…

@janbrasna
Copy link
Contributor Author

Or actually it seems that PRs opened by @fastlane members have the danger/danger run, PRs opened by others don't… 🤷‍♂️ weird huh?

@rogerluan
Copy link
Member

Hi @janbrasna , since my last comment in this thread I've acquired more knowledge on the topic and am capable of clarifying what we're seeing the behavior you described in your last comment here 😊 ☝️

PRs opened by fastlane members have the danger/danger run, PRs opened by others don't

This is a security feature. The technical difference is "PRs coming from forks" vs "PRs opened within this repo", I think. This prevents users from creating PRs from forks from executing arbitrary code (e.g. introducing a new script that echo $DANGER_GITHUB_API_TOKEN so they can read the env var, just as a simple example).

Each CI has their own mechanism to prevent this from happening, so the solution to this problem will be a solution specific for Circle CI, and not e.g. danger itself.

Looks like this could be a good starting point: https://circleci.com/blog/triggering-trusted-ci-jobs-on-untrusted-forks (I didn't read it yet, I just found this)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

enforce danger success fastlane/docs
2 participants
@janbrasna @rogerluan