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

base64 validation too strict; padding is optional #1156

Closed
fluxsauce opened this issue Apr 11, 2017 · 6 comments
Closed

base64 validation too strict; padding is optional #1156

fluxsauce opened this issue Apr 11, 2017 · 6 comments
Assignees
Labels
feature New functionality or improvement
Milestone

Comments

@fluxsauce
Copy link
Contributor

fluxsauce commented Apr 11, 2017

Context

  • node version: 6.9.2
  • joi version: 10.4.1
  • environment node
  • used with standalone:
  • any other relevant information:

What are you trying to achieve or the steps to reproduce ?

Determine if a URL parameter contains valid base64 encoded content.

const schema = Joi.string().base64()

Example values:

  • MTYyMDcxMDozOjE

The problem is that the validation fails on a base64 string that is valid but does not have padding; the issue was dismissed in #1081

I don't agree with that interpretation; it's not invalid, it just doesn't have optional padding.

https://en.wikipedia.org/wiki/Base64#Output_Padding

In theory, the padding character is not needed for decoding, since the number of missing bytes can be calculated from the number of Base64 digits. In some implementations, the padding character is mandatory, while for others it is not used.

The input is not normalized and comes from a variety of sources that I do not have direct control over and I cannot force changes, nor can I just arbitrarily ignore unpadded content.

Attempting to pad the content prior to validation is hacky.

// Joi requires padding.
const paddingLength = working.length % 4;
const workingPadded = paddingLength ? working + '='.repeat(4 - paddingLength) : working;

A more graceful solution would be to make padding optional.

  • options: - an optional object with the following optional keys:
    • paddingRequired: if true, requires that the string include = padding. Defaults to true.

Which result you had ?

message: '"value" must be a valid base64 string',

What did you expect ?

No errors.

@Marsup
Copy link
Collaborator

Marsup commented Apr 25, 2017

Fair enough, happy to take a PR if you're up for it.

@fluxsauce
Copy link
Contributor Author

I am, as I just got hit with a bug on my end because I didn't apply the padding workaround in all the places that I was validating. :-/

Here's a direct link to the code in question

base64() {

I'm going to fix the code on my end first then try to fix this.

@Marsup do you feel it's better to arbitrarily pad and test or just use a different regex?

@WesTyler
Copy link
Contributor

I would vote for a new regex to test against based on the paddingRequired flag.

@fluxsauce
Copy link
Contributor Author

I'm cool with that.

@DavidTPate
Copy link
Contributor

In looking at this, I would think that we would make some modifications to our RegExp to make the padding (=) not required but only in the event that the flag paddingRequired is set to false. For backwards compatibility I would expect paddingRequired to default to true.

Definitely happy to take a look at a PR.

fluxsauce added a commit to fluxsauce/joi that referenced this issue May 2, 2017
fluxsauce added a commit to fluxsauce/joi that referenced this issue May 7, 2017
fluxsauce added a commit to fluxsauce/joi that referenced this issue May 7, 2017
fluxsauce added a commit to fluxsauce/joi that referenced this issue May 22, 2017
@Marsup Marsup closed this as completed in b9a6110 Jun 2, 2017
@Marsup Marsup self-assigned this Jun 15, 2017
@Marsup Marsup added this to the 10.6.0 milestone Jun 15, 2017
@hueniverse hueniverse added feature New functionality or improvement and removed request labels Sep 19, 2019
@lock
Copy link

lock bot commented Jan 9, 2020

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality or improvement
Projects
None yet
Development

No branches or pull requests

5 participants