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

[React-Linter] Update danger token #12956

Merged
merged 1 commit into from
May 31, 2018
Merged

[React-Linter] Update danger token #12956

merged 1 commit into from
May 31, 2018

Conversation

hramos
Copy link
Contributor

@hramos hramos commented May 31, 2018

Use a new react-linter access token. This is used by Danger in Circle CI to post react-linter's build size metrics whenever a PR is created or updated.

This token is publicly visible on purpose. As Danger runs on forked pull requests, Danger does not have access to the private environment variables in the Circle CI test environment. We therefore use an access token for a GitHub bot account (react-linter) that is, for all intents and purposes, a regular account with no privileged access.

This token will only allow the bot to interact with public repositories the same way any arbitrary GitHub account might. This allows us to add build size metrics as a comment on PRs. Unfortunately, it also means anyone can leave random comments or even open new issues.

The token was revoked earlier today after it was used to open #12953. I'm restoring the token in order to re-enable build size tracking, but further abuse of the token might result in it getting revoked permanently and we'll need to find an alternate means of running the build size tracking script.

@gaearon
Copy link
Collaborator

gaearon commented May 31, 2018

If they decided to spam us with the old token, wouldn't they just do exactly the same thing with the new one?
Seems like this is a bit pointless. :-(

@hramos
Copy link
Contributor Author

hramos commented May 31, 2018

Exactly. I'm just adding back a working token in order to restore the react-linter functionality since I don't think I'll have time to move this elsewhere today.

Alternatively, I can revoke this new token (since it's now public via this PR) and you can all still get the same build size data by visiting Circle CI, as Danger is still running there (see https://circleci.com/gh/facebook/react/10263#tests/containers/2) -- it just won't be able to post comments on the PR itself.

@hramos
Copy link
Contributor Author

hramos commented May 31, 2018

If you're set on using Danger, the proper way of handling this would be to move towards using the hosted version of Danger, Peril. In order to use Peril we'd need to set up a GitHub App on the Facebook GitHub org, something that will need to be approved by the Facebook Open Source team.

@mbrevda
Copy link

mbrevda commented May 31, 2018

"They" aren't spamming. It was just a test to see if the token was active before bothering people with a report.

@mbrevda
Copy link

mbrevda commented May 31, 2018

Perhaps the danger task could be run from something like Lamda or GCF? It would be publicly triggerable (allowing PR's to trigger it), but the env vars could be kept private.

@gaearon
Copy link
Collaborator

gaearon commented May 31, 2018

@mbrevda Thanks for clarifying :-)

@mbrevda
Copy link

mbrevda commented May 31, 2018

I'm sorry for the bother. DM'd you in Twitter a minute later to clarify.

@gaearon
Copy link
Collaborator

gaearon commented May 31, 2018

Ah, I saw your message but didn’t read yet

@hramos
Copy link
Contributor Author

hramos commented May 31, 2018

@mbrevda that would still allow people to trigger the script to run an arbitrary number of times as we would not be able to validate that the call is coming from Circle on an actual PR job. But at least it prevents people from using the token to comment on arbitrary issues.

@gaearon merging this PR would get us back to where we were a few hours ago. This new token is from the same react-linter account, with the same access scope. The old one was revoked and new PRs won't get linted.

@gaearon gaearon merged commit 65ab536 into facebook:master May 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants