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

raise ArgumentError when SECRET_KEY_BASE is an integer #22078

Merged
merged 1 commit into from Oct 27, 2015

Conversation

arunagw
Copy link
Member

@arunagw arunagw commented Oct 26, 2015

If SECRET_KEY_BASE or other secret gets passed as other then string
we need to raise ArgumentError to know that it's a wrong argument.

Closes #22072

@schneems
Copy link
Member

Will this change the value returned by generate_key ?

@rafaelfranca
Copy link
Member

And why would secrets.yml have an integer?

@rafaelfranca rafaelfranca self-assigned this Oct 26, 2015
@arunagw
Copy link
Member Author

arunagw commented Oct 26, 2015

@schneems Nope it will not change the value as other specs are fine but I will double check.

@rafaelfranca I saw #22072 and found that it's easy to incorporate

@rafaelfranca
Copy link
Member

It is invalid input. This should be always a string, so it should raise an
argument error if it is not a string, not accept any value. Could you
change your patch to raise the argument error?

On Mon, Oct 26, 2015, 14:52 Arun Agrawal notifications@github.com wrote:

@schneems https://github.com/schneems Nope it will not change the value
as other specs are fine but I will double check.

@rafaelfranca https://github.com/rafaelfranca I saw #22072
#22072 and found that it's easy to
incorporate


Reply to this email directly or view it on GitHub
#22078 (comment).

@arunagw
Copy link
Member Author

arunagw commented Oct 26, 2015

Yes. I can work on this.

@arunagw arunagw changed the title Allow SECRET_KEY_BASE to be an integer raise ArgumentError when SECRET_KEY_BASE is an integer Oct 26, 2015
@arunagw
Copy link
Member Author

arunagw commented Oct 26, 2015

@rafaelfranca I have updated PR with changes. Please have a look. thanks

@@ -8,6 +8,9 @@ module ActiveSupport
# key in multiple incompatible contexts.
class KeyGenerator
def initialize(secret, options = {})
unless secret.kind_of?(String)
raise ArgumentError, "Secret must be a type of String"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should check it were we assign with a proper message pointing to the secrets configuration. I think if we raise here it will not be user friendly.

If `SECRET_KEY_BASE` or other `secret` gets passed as other then string
we need to raise `ArgumentError` to know that it's a wrong argument.

Closes rails#22072
@arunagw
Copy link
Member Author

arunagw commented Oct 27, 2015

@rafaelfranca Yes. make sense to point user into right direction.

I found that we are also doing some secrets.yml raise in LegacyKeyGenerator here. So initially i thought to raise from KeyGenerator.

spastorino added a commit that referenced this pull request Oct 27, 2015
raise `ArgumentError` when `SECRET_KEY_BASE` is an integer
@spastorino spastorino merged commit 7a9ce69 into rails:master Oct 27, 2015
@arunagw
Copy link
Member Author

arunagw commented Oct 27, 2015

@spastorino Should we backport this to 4-2-stable?

@arunagw arunagw deleted the secret-key-as-an-integer branch October 27, 2015 13:36
@spastorino
Copy link
Contributor

@arunagw I wouldn't, I don't think it's that important.

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

4 participants