Fix #1156 - Adds support for optional base64 padding validation. #1174
Conversation
fluxsauce
changed the title
API.md
Outdated
|
|
||
| Requires the string value to be a valid base64 string. | ||
| * padding - optional parameter defaulting to `true` which will require `=` padding if `true` or make padding optional (`false`). |
There was a problem hiding this comment.
I think that calling this paddingRequired would better represent the functionality of it, I could see a padding option being used for other things.
There was a problem hiding this comment.
If it's better to stick with a single parameter rather than an object, sounds good.
API.md
Outdated
| @@ -1764,12 +1764,23 @@ Requires the string value to be a valid hexadecimal string. | |||
| const schema = Joi.string().hex(); | |||
| ``` | |||
|
|
|||
| #### `string.base64()` | |||
| #### `string.base64([padding])` | |||
There was a problem hiding this comment.
I'm not sure what the consensus is but I'd personally prefer to see this as an options object instead that way we can support other options in the future without making a breaking change.
There was a problem hiding this comment.
I can, but the only other options I can anticipate would be around decoding? Trying to be cognizant of over-engineering.
test/string.js
Outdated
|
|
||
| const rule = Joi.string().base64(true); | ||
| Helper.validate(rule, [ | ||
| ['YW55IGNhcm5hbCBwbGVhc3VyZS4=', true], |
There was a problem hiding this comment.
We should also include a test for 2 bytes of padding (== instead of =). ie. YW55IGNhcm5hbCBwbGVhc3VyZQ==
test/string.js
Outdated
|
|
||
| const rule = Joi.string().base64(false); | ||
| Helper.validate(rule, [ | ||
| ['YW55IGNhcm5hbCBwbGVhc3VyZS4=', true], |
There was a problem hiding this comment.
We should also include a test for 2 bytes of padding (== instead of =). ie. YW55IGNhcm5hbCBwbGVhc3VyZQ==
lib/string.js
Outdated
| if (padding) { | ||
| Hoek.assert(typeof padding === 'boolean', 'padding requirement must be boolean'); | ||
| } | ||
| const paddingRequired = (typeof padding === 'undefined') ? true : padding; |
There was a problem hiding this comment.
This won't catch when a null is passed. In general I would usually handle this with something like the following:
padding = padding === false ? padding : padding || true;There was a problem hiding this comment.
👍 and will add tests.
test/string.js
Outdated
| @@ -3180,6 +3180,42 @@ describe('string', () => { | |||
| ], done); | |||
| }); | |||
|
|
|||
| it('validates an base64 string with explicit padding required', (done) => { | |||
There was a problem hiding this comment.
Could we add some tests around passing in the padding parameter so that we are making sure that it requires a boolean and other inputs are rejected.
|
@DavidTPate Thanks again, I renamed the option but as I didn't get any feedback about additional options for base64 validation, I'm keeping it as boolean for now. I believe all other concerns were addressed. |
|
Also in favor of an object. I'm not dogmatic about the famous boolean trap but I'd say this specifically is a very good case and would contribute in a bad way to the readability of your schemas. |
|
@fluxsauce Your changes & docs look good, I'd just really like to see the option passed as an object as opposed to a single parameter. |
|
I appreciate the feedback and I'll make the changes, I just haven't had the time to sit down and do it properly. Will happen this week. |
|
Sunday counts as this week, yes? :-) Should be set, let me know if you have any feedback! |
|
This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions. |
As discussed in #1156.
Added a single boolean option
padding; if undefined, defaults totrue.My original version was slightly too lenient and allowed too much padding at the end of the statement. I believe this version should take into account only proper amounts of padding.