chriszarate / supergenpass

A free bookmarklet password generator.
https://chriszarate.github.io/supergenpass/
GNU General Public License v2.0
418 stars 160 forks source link

Use of @ in Domain/URL field #49

Closed pr0j3kt closed 9 years ago

pr0j3kt commented 10 years ago

I have tried a few times, but it appears the use of "@" in the Domain/URL field is no longer being accepted. I had some cached versions in browsers that were still accepting, but clearing out cache on the working browsers results in that no longer working as well. I was using "user@url" for a password policy to generate passwords for certain users. Did something change that stripped that out? Do I need to download a previous version and run my own moving forward to allow that? Thanks for listening!

chriszarate commented 10 years ago

Yes, it changed when we created supergenpass-lib and standardized the approaches to hashing and URL validation.

This is a tough one. The @ is a reserved character in URLs and it is used to indicate an authenticating user. I added a test for a pretty high-profile use case: Git URLs, e.g., git://git@github.com:user/repo.git. Granted, that's not something that would appear in the browser, but it is something that might be input into other implementations of SGP.

I consider this a long-standing bug that it would be harmful to unfix, but I'm interested in other's opinions.

In the meantime, you can access an older version without this fix in the repo's history: https://rawgit.com/chriszarate/supergenpass/80b10db99ca63de9d64e86fe3a240beeeb2ec7af/mobile.html

basile-laderchi commented 10 years ago

@pr0j3kt: maybe instead of using "user@url" for password policy you could change to using "user.url" and disabling the TLD? (instead of the "at" symbol use a "dot")

Just tested it on the mobile version and it worked just fine.

cmcnulty commented 10 years ago

Tangentially related, have you considered out-sourcing the sub-domain removal to a separate project, namely: https://github.com/oncletom/tld.js - it keeps the list up-to-date, while providing the same functionality. I'd be happy to create a pull request if it's something you'd be inclined to accept.

chriszarate commented 10 years ago

@cmcnulty, I decided a long time ago never to update the TLD list, because it is directly tied to password generation. I imagine that every time it is updated, someone out there will all of a sudden experience unexpectedly different password generation on an affected site and either have to change their password or manually turn off domain isolation every time they log in to that site.

I feel bad enough that fixing a bug can lead to password generation problems, like in this ticket. Affecting password generation usually trumps feature requests, for me.

denis-sokolov commented 10 years ago

The original issue is an unfortunate case of people relying on a feature that was never intended there in the first place. I am sorry to hear we broke it, but the domain/url field has very straight-forward semantics - it's for the domain and the url. May I suggest you to use the unfortunately named "Salt" feature to distinguish multiple users? (Even better, different master passwords).

I can't imagine the feature with the @ character in the domain field to be ever supported again, unless there was a massive public outcry about it, thus I can only suggest to switch to another method and change your passwords. Again, I am sorry.

pr0j3kt commented 10 years ago

I ended up downloading one of the previous releases and storing a copy for myself elsewhere, that way I can still authenticate any passwords I have that use my old password policy. Will probably change up my password policy to be more in-line with current versions.