Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Travis CI webhooks shouldn't be static #898

Closed
mleanos opened this issue Sep 8, 2015 · 16 comments
Closed

Travis CI webhooks shouldn't be static #898

mleanos opened this issue Sep 8, 2015 · 16 comments
Assignees
Milestone

Comments

@mleanos
Copy link
Member

mleanos commented Sep 8, 2015

The webhooks defined here are static. This will cause activity notifications, for repo's originating from this codebase, to be sent to the meanjs/mean Gitter room. https://github.com/meanjs/mean/blob/master/.travis.yml#L19

If a user of this project doesn't change the settings in travis.yml, their project will use this webhook whenever it runs through the Travis CI build process. I don't think this is a desirable behavior.

I'm not too familiar with Travis CI, but I did some research on this subject. This led me to two possible options to solve this issue...

  1. Use an environment variable to define the Gitter webhook. This may be useful to provide separate settings for the different environment configs. Perhaps something like this in the config/env settings...
travis: {
  gitter: {
    webhook: 'https://webhooks.gitter.im/e/249daf9851ea4776f34e'
  }
}
  1. Use custom variables, and set them in the Travis CI Repository Settings as described here. http://docs.travis-ci.com/user/environment-variables/#Defining-Variables-in-Repository-Settings I'm not sure about this solution though.

I don't really have a way (that I know of) to test these types of settings/variables for Travis CI. I'm hoping someone might have some insight into this issue.

@lirantal @ilanbiala @codydaig @rhutchison Any thoughts/ideas?

@lirantal
Copy link
Member

lirantal commented Sep 9, 2015

Good point @mleanos

Yeah @ilanbiala I actually don't think we even need it because it already exists in gitter's integration area, and if not we can add it manually from gitter.

@lirantal lirantal added this to the 0.4.2 milestone Sep 9, 2015
@ilanbiala
Copy link
Member

@lirantal I'm pretty sure Gitter's integration menu says to add that in, but if you see somewhere in the settings that says otherwise, go for it.

@rhutchison
Copy link
Contributor

I would think this is required, but I am not sure. Technically the only time this would come in to play is when someone opens a PR. Yes, it might show events if someone opens a PR to another fork, but I don't think it would happen that often.

I guess the only thing we should be concerned about is if we are ok with this webhook being public?

@mleanos
Copy link
Member Author

mleanos commented Sep 11, 2015

@rhutchison You're right about when this would show up, as a notification in the Gitter room. Whenever someone has that webhook in their travis.yml file, it will trigger the event notifications if their repo is ran through the Travis CI build process.

I'm not sure if the only likely cause would be from PR's from a fork. What happens if someone is running their codebase through Travis CI by other means.

My concern is mostly with the privacy of the user's of this project. A few times this week, I saw notifications coming into the Gitter room from a fork. The user probably didn't even know this was happening; and I think this could have privacy implications.

@rhutchison
Copy link
Contributor

imho, we should leave this as-is. I value information over any privacy concerns.

RE: security, if someone takes the webhook and we start to see unrelated project activity on this webhook, then the project owners can just delete the webhook.

@lirantal lirantal self-assigned this Sep 11, 2015
@lirantal
Copy link
Member

I'll take a look and see if we can somehow integrate it without sharing the webhooks key on the repository

@lirantal
Copy link
Member

Guys, I couldn't find anything specific about the webhooks integration. I've taken a look at other projects and they also specify the webhooks key in their .travis.yml.

I'm fine if you want to research this a bit more, otherwise I'll close it.

@mleanos
Copy link
Member Author

mleanos commented Sep 11, 2015

@lirantal What about exploring the use of env/config's for the webhook?

Or what is described here.. http://docs.travis-ci.com/user/environment-variables/#Defining-Variables-in-Repository-Settings

@lirantal
Copy link
Member

I'll try that and we'll see if it works.

@codydaig
Copy link
Member

@mleanos @lirantal Whats the status of this issue?

@mleanos
Copy link
Member Author

mleanos commented Oct 17, 2015

@codydaig I'm resolved to think this isn't really an issue; at least not as much as I thought it was initially.

In order for this to cause issues for a user, they'd have to enable the Travis Web Hooks on their repository. So that seems like a conscious effort on their part.

@ilanbiala
Copy link
Member

@lirantal do we want to bother with this or should we just close?

lirantal added a commit to lirantal/meanjs that referenced this issue Oct 18, 2015
@lirantal
Copy link
Member

PR for it: #1004 - I noticed that when the build triggers then it sends information to the gitter room. I'm not sure if that's because travis is parsing the .travis.yml file from the PR or from the master branch.

I created a variable in travis-ci.org for us, and added it to the .travis.yml file.
WDYT?

@ilanbiala
Copy link
Member

@lirantal if it works, sounds good!

@mleanos
Copy link
Member Author

mleanos commented Oct 18, 2015

@lirantal I couldn't quite figure out if Travis parsing the .yml file from the PR or master branch either. I'll try to test this here in a bit with my fork.

@lirantal
Copy link
Member

Ok, so I'll go ahead and merge.
If there will be any issues I'll revert back.

AlexBoYang added a commit to AlexBoYang/mean that referenced this issue Oct 27, 2015
# By Liran Tal (89) and others
# Via Liran Tal (159) and others
* 'master' of https://github.com/meanjs/mean: (306 commits)
  Enable log options for Morgan
  Lock mongoose version to a working version
  updating profile upload with a new version of multer
  fixes meanjs#898 - addressing the issue where the webhook API is hard-coded into the travis config file
  Favicon invalid path
  [docs] Information about Contributing
  Update package.json
  adding karma coverage for grunt
  Added Ruby and Sass to requirements with install directions
  removing keepAlive conf to fail travis if e2e tests fail
  Add ESLint support
  Format code according to ESLint rules
  adding tests for directives
  renaming strength meter, hiding  when password field is empty, and refactoring directives to use $validators
  updating travis to support installing a local mail server
  adding more API tests
  Global Mocha timeout
  Synchronous tests
  Formatting and Indentation
  Seed options - logResults
  ...

Conflicts:
	config/config.js
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants