write multi dim array to valid ini #16
Conversation
src/IniWriter.php
Outdated
| if (is_int($key)) { | ||
| $ini .= $option . '[] = ' . $this->encodeValue($currentValue) . "\n"; | ||
| } else { | ||
| $ini .= $option . '[' . $key . '] = ' . $this->encodeValue($currentValue) . "\n"; |
There was a problem hiding this comment.
I haven't properly reviewed but wondering if $key would need to be encoded as well maybe? Just thinking randomly if $key was to be supplied by user input then not sure if you could break something or cause various security issues eg by defining a key like "] = 'xyz';\n foo =".
There was a problem hiding this comment.
I'll check it up and write a test :)
There was a problem hiding this comment.
encodeValue() is not needed for the $key. It's just for booleans and quoted strings as value.
There was a problem hiding this comment.
I think what @tsteur means is, can you write a test where the key is defined as "] = 'xyz';\n foo =" and make sure it doesn't inject something into the INI file?
There was a problem hiding this comment.
@diosmosis, I'll write this test today.
There was a problem hiding this comment.
The $key will be sanitized now. But you can still break the INI by injecting malicious crap "] = 'xyz';\n foo =" to the INI section or as value :/
There was a problem hiding this comment.
Thanks for writing the test @typomedia! I left one other comment, after that it will be good to merge. If you'd also like to make sure this doesn't happen for sections or values, we'd certainly appreciate it! Otherwise we'll take a look ourselves after merging this PR.
src/IniWriter.php
Outdated
| */ | ||
| private function encodeKey($key) | ||
| { | ||
| $key = preg_replace('/[^A-Za-z0-9\-]/', '', $key); |
There was a problem hiding this comment.
I'd maybe allow underscores here as well.
|
Looks good @typomedia! Thanks again for the contribution and thanks for adding tests! |
unit test included ;)