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

Make SnapPass safer #4

Closed
wants to merge 2 commits into from
Closed

Make SnapPass safer #4

wants to merge 2 commits into from

Conversation

RyanGWU82
Copy link

  1. Adds the prefix "snappass:" to all Redis interaction. If your Redis server is shared with other applications, this will prevent someone from using SnapPass to trawl your Redis database. This is backwards incompatible and will break existing links.
  2. Adds validation when accepting a key, to make sure it matches the expected 32 characters of hex.
  3. Adds validation when storing a password, to make sure the password isn't more than 1 MB. This will help prevent someone from using SnapPass to fill your Redis server's RAM (although it will only slow them down, not necessarily stop them).

Ryan Park added 2 commits January 15, 2015 16:52
1. Adds the prefix "snappass:" to all Redis interaction. If your Redis server
is shared with other applications, this will prevent someone from using
SnapPass to trawl your Redis database. *This is backwards incompatible and will
break existing links.*

2. Adds validation when accepting a key, to make sure it matches the expected
32 characters of hex.

3. Adds validation when storing a password, to make sure the password isn't
more than 1 MB. This will help prevent someone from using SnapPass to fill
your Redis server's RAM (although it will only slow them down, not necessarily
stop them).
@yongwen
Copy link
Member

yongwen commented Feb 23, 2016

I am concerning about the "backward incompatibility" regarding adding the "snappass" prefix. Could we add a release note section in the README to make sure the existing users aware of this. thanks!

@nichochar
Copy link
Collaborator

nichochar commented Jul 18, 2016

+1 on @yongwen 's comment.

@RyanGWU82 first of all, thanks for the PR, I think it's a good idea.
To make this backward compatible, could we make the prefix a parameter that only applies if it is not None?

We can then set it to default to something reasonable like the snappass you suggested.

Thanks!

@nichochar
Copy link
Collaborator

This thread seems to be dead. Unless @RyanGWU82 wants to rebase, we may want to close this, and create an issue for adding a prefix. We can then use that issue as a place to hold the discussion about maintaining backwards compatibility. Does this sound good to you @yongwen ?

@yongwen
Copy link
Member

yongwen commented Oct 6, 2016

ok with me. thanks!

On Thu, Oct 6, 2016 at 11:08 AM, Nicholas Charriere <
notifications@github.com> wrote:

This thread seems to be dead. Unless @RyanGWU82
https://github.com/RyanGWU82 wants to rebase, we may want to close
this, and create an issue for adding a prefix. We can then use that issue
as a place to hold the discussion about maintaining backwards
compatibility. Does this sound good to you @yongwen
https://github.com/yongwen ?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAeQY-jMG2NdDzLl7E8z8LKyZEp6Cv-Vks5qxTkhgaJpZM4DTI27
.

@nichochar
Copy link
Collaborator

As explained above, closed in favor of #32 and will reopen with a clean rebase and a strategy for backwards compatibility.

@nichochar nichochar closed this Oct 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants