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

Fix handling of config tri-state bool values (like acl_public) #940

Merged
merged 1 commit into from
Mar 3, 2018

Conversation

bcl
Copy link
Contributor

@bcl bcl commented Dec 7, 2017

Commit bc38c2a added the ability to set
acl_public from the config file -- but it did not take into account that
it is a 'tri-state' value so the string is what is written, causing it
to ALWAYS be true.

This results in unexpected and insecure behavior if 'acl_public = False'
is used in the config file.

This patch modifies the Config.update_option to check the option type
AND the value if the original is None. If an option's default is None
and the value is a bool (true, false, yes, no, 0, 1) then it will set it
to a bool value.

@bcl
Copy link
Contributor Author

bcl commented Dec 7, 2017

This should also solve #793 but in a more generic way.

@fviard
Copy link
Contributor

fviard commented Dec 8, 2017

Thank you for having catch that.
And the proposed fix is quite graceful!

But I still see a little drawback, if you can add a fix for that in your PR, that would be awesome:
Now, all the fields having a "None" as default config will be "treated" by your new code.
So 2 sides effects:

  • One that is in fact a text field, might have boolean set for it if by bad luck or else "true" or "1" be used for example. (A rare corner case i guess)
  • Any provided text for such an option will go trough your "is_bool" function. As it is using a "str(...)" construct, chances are that this will crash with utf8 values and python2.7. (That case is likely for someone using content_disposition).

Possible fix:
I see 3 options that are like that in the config: content_disposition, content_type, and upload_id.
A fast lookup of the code let me think that they may have their default value replaced to "".
A few lines of code will have to be updated to not look "is None" but just if "None or empty".
Luckily, i don't see any case where these 3 fields could have a legitimate "" value that has to be used.

Commit bc38c2a added the ability to set
acl_public from the config file -- but it did not take into account that
it is a 'tri-state' value so the string is what is written, causing it
to ALWAYS be true.

This results in unexpected and insecure behavior if 'acl_public = False'
is used in the config file.

This patch modifies the Config.update_option to check the option type
AND the value if the original is None. If an option's default is None
and the value is a bool (true, false, yes, no, 0, 1) then it will set it
to a bool value.
@bcl bcl force-pushed the master-acl_public branch from ea12ce0 to 618d357 Compare December 9, 2017 20:03
@bcl
Copy link
Contributor Author

bcl commented Dec 9, 2017

I've updated the PR with a small change, dropping use of str() and renaming the functions. From what I can see they will always be called with unicode so it can just use .lower() on that. And if it ever gets called with a str, .lower() still works.

I don't think the corner case of using a bool string for one of the other entries is going to be a problem, but if you want to deal with that separately I'll leave that up to you.

@fviard fviard merged commit 8b001f8 into s3tools:master Mar 3, 2018
@fviard
Copy link
Contributor

fviard commented Mar 3, 2018

Finally merged, thank you. Sorry for the long delay.

fviard added a commit that referenced this pull request Mar 3, 2018
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.

2 participants