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

Allow to enter secret as base64 #11

Closed
wants to merge 1 commit into from

Conversation

choffmeister
Copy link

Also some small choring.

@mjamado
Copy link
Owner

mjamado commented Aug 13, 2020

Hello, @choffmeister, thank you for your PR.

However, I can't accept it (like this). Changing the order of the parameters, like you did, will break current installs for everyone who upgrades.

Please figure out a way to either:

  • Put the new setting at the end of the parameters, which will make the settings separate, or;
  • Try to autodetect whether the string is a base64 representation, which will make some plain text secrets unusable (because they will be false-positive-detected).

Neither of the alternatives are perfect, but I'm guessing the first will raise fewer problems.

Anyway, I fail to see this as a relevant feature. Given the general availability of online base64 encoders and decoders, why would you use a base64 representation of the secret, if the plugin supports it in plain text? It's not a security issue (base64 is not an hashing function), it's not even a convenience issue, given the aforementioned availability of tooling.

@choffmeister
Copy link
Author

choffmeister commented Aug 13, 2020

@mjamado Ah, did not know that it breaks existing installs. Was just thinking, that it makes more sense to keep the secret stuff (incl private key) together. But if the order is relevant to existing installs, then I will adjust my PR of course and append the setting to the end.

Insomnia should use IDs to target the settings before they move plugins out of beta though. Without it updating plugins will be a nightmare 😄

@choffmeister
Copy link
Author

@mjamado Sure it is not a security feature. The point is, that converting the base64 manually to a string and then pasting it might cause issues. For example jwt.io also allows to specify it. If one does not want to have a toggle, then I would argue, that one should always enter the secret as base64. This way around you for sure have no problem. And not using a proper binary secret for signing is bad practise anyway (as it is hard to make sure that your secret has the right amount of entropy, if u generate a string secret instead of a binary secret like with openssl rand -base64 32).

@mjamado
Copy link
Owner

mjamado commented Aug 13, 2020

The point is just this one: we can't break the plugin (again). 😄

Insomnia should have life cycle events for plugins, like onPluginUpdate or something, in which we could migrate old settings to new settings. Something to take to Insomnia's issues... someday.

@nijikokun
Copy link
Contributor

Was thinking about this feature when I looked at the plugin!

Also, I've gone ahead and opened an issue with some details from this thread on the Insomnia issues. Feel free to add more insights there 🎉

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