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

Implement reCAPTCHA for abuse prevention #311

Merged
merged 2 commits into from Jun 1, 2017
Merged

Conversation

@chadwhitacre
Copy link
Contributor

@chadwhitacre chadwhitacre commented May 27, 2017

Redux of #234. Initial rebase here drops some README changes in favor of HEAD, but otherwise should match 8330ac6.

@chadwhitacre
Copy link
Contributor Author

@chadwhitacre chadwhitacre commented May 27, 2017

Rebased again to drop the commits that add the dist directory to the repo: f5604a5 04d33c1. That seems a distraction. Was 947d908.

@chadwhitacre
Copy link
Contributor Author

@chadwhitacre chadwhitacre commented May 27, 2017

Alright, squashed all the commits and removed the remaining bits that were changing orthogonal plumbing vs. actually adding the feature in scope. Was c1294cc.

@chadwhitacre
Copy link
Contributor Author

@chadwhitacre chadwhitacre commented May 27, 2017

Testing with Heroku templating ...

@chadwhitacre
Copy link
Contributor Author

@chadwhitacre chadwhitacre commented May 27, 2017

screen shot 2017-05-26 at 9 08 14 pm

@chadwhitacre
Copy link
Contributor Author

@chadwhitacre chadwhitacre commented May 27, 2017

screen shot 2017-05-26 at 9 10 05 pm

@chadwhitacre
Copy link
Contributor Author

@chadwhitacre chadwhitacre commented May 27, 2017

screen shot 2017-05-26 at 9 10 25 pm


screen shot 2017-05-26 at 9 10 43 pm

@chadwhitacre
Copy link
Contributor Author

@chadwhitacre chadwhitacre commented May 27, 2017

Ready for review, @rauchg!

@chadwhitacre
Copy link
Contributor Author

@chadwhitacre chadwhitacre commented May 27, 2017

Test here if you like (will leave up temporarily):

https://immense-forest-91787.herokuapp.com/

@rauchg
Copy link
Owner

@rauchg rauchg commented May 29, 2017

@whit537 thanks a lot! One last thing before we merge this: do you think it's a good idea to enforce Google reCAPTCHA on every single installation, rather than making it optional?

@chadwhitacre
Copy link
Contributor Author

@chadwhitacre chadwhitacre commented May 29, 2017

I think it should be hard to deploy SlackIn insecurely ("secure by default"). If reCAPTCHA is optional then it should be an explicit opt-out with a clear warning about SlackOut.

@chadwhitacre
Copy link
Contributor Author

@chadwhitacre chadwhitacre commented May 29, 2017

... and I guess I would see that as out-of-scope for this PR. Right now SlackIn exposes its users to a non-trivial vulnerability. Reducing that exposure is important. Adding a footgun back into SlackIn can be done later.

@chadwhitacre
Copy link
Contributor Author

@chadwhitacre chadwhitacre commented May 31, 2017

Ready to merge, @rauchg?

@rauchg rauchg merged commit 1dedb2e into rauchg:master Jun 1, 2017
@chadwhitacre chadwhitacre deleted the chadwhitacre:reCAPTCHA branch Jun 1, 2017
@chadwhitacre
Copy link
Contributor Author

@chadwhitacre chadwhitacre commented Jun 1, 2017

develar added a commit to develar/slackin that referenced this pull request Jun 13, 2017
This reverts commit 1dedb2e
@toolmantim
Copy link
Contributor

@toolmantim toolmantim commented Aug 3, 2017

Was this made to also work with the badge version? Because I'm trying to upgrade (for Node security updates) but can't seem to get this to work, and I can't see how it did?

badge-version

When you try to send an invite you get:

TypeError: undefined is not an object (evaluating 'gcaptcha_response.value')

It works fine if you go to it directly:

slackin

@chadwhitacre
Copy link
Contributor Author

@chadwhitacre chadwhitacre commented Aug 4, 2017

I don't recall seeing anything about the badge version when I was cleaning up the PR. I suggest making a new ticket/PR.

@toolmantim toolmantim mentioned this pull request Aug 4, 2017
1 of 2 tasks complete
@toolmantim
Copy link
Contributor

@toolmantim toolmantim commented Aug 4, 2017

@whit537 I've done some digging, and it looks like you might have forgotten to update the badge to support recaptcha. I've opened up a WIP pull request here: #332 Would love your help/thoughts.

@chadwhitacre
Copy link
Contributor Author

@chadwhitacre chadwhitacre commented Aug 5, 2017

you might have forgotten to update the badge to support recaptcha

Sorry about that. :-/

@toolmantim
Copy link
Contributor

@toolmantim toolmantim commented Aug 5, 2017

It happens! 😊

toolmantim added a commit to toolmantim/slackin that referenced this pull request Aug 7, 2017
This reverts commit 1dedb2e.
@Daniel15
Copy link

@Daniel15 Daniel15 commented Oct 20, 2017

Can you please publish this update to npm? slackin on npm is still 0.13.0

This was referenced Nov 15, 2017
@jpoon jpoon mentioned this pull request Dec 3, 2017
@jpoon
Copy link
Contributor

@jpoon jpoon commented Dec 3, 2017

FYI, this PR broke the Azure deployment: #355. Need to update the environment variables here: https://github.com/rauchg/slackin/blob/master/azuredeploy.json#L99

@KrzysztofMadejski
Copy link

@KrzysztofMadejski KrzysztofMadejski commented Dec 6, 2018

These setting for Azure deployment are fixed in #333

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.