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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coerce toJSON'ed Buffer to real Buffer #2954

Closed
QuentinFarizon opened this issue Jun 5, 2023 · 11 comments
Closed

Coerce toJSON'ed Buffer to real Buffer #2954

QuentinFarizon opened this issue Jun 5, 2023 · 11 comments
Assignees
Labels
feature New functionality or improvement
Milestone

Comments

@QuentinFarizon
Copy link

QuentinFarizon commented Jun 5, 2023

Hi! 馃憢

Firstly, thanks for your work on this project! 馃檪

I am using joi with Hapi, and I have a route with a payload field defined as serialNumber: Joi.binary()

But when inspecting payload before validation, I see that I receive the data not as a Buffer instance, but as an object with { type: "Buffer", data: } (probably Hapi calling toJSON() on it). I think it's valid to coerce it to a Buffer instance

Here is the diff that solved my problem:

diff --git a/node_modules/joi/lib/types/binary.js b/node_modules/joi/lib/types/binary.js
index 9147166..053d022 100755
--- a/node_modules/joi/lib/types/binary.js
+++ b/node_modules/joi/lib/types/binary.js
@@ -14,13 +14,19 @@ module.exports = Any.extend({
     type: 'binary',
 
     coerce: {
-        from: 'string',
+        from: ['string', 'object'],
         method(value, { schema }) {
 
-            try {
-                return { value: Buffer.from(value, schema._flags.encoding) };
+            if (typeof value === 'string') {
+                try {
+                    return { value: Buffer.from(value, schema._flags.encoding) };
+                }
+                catch (ignoreErr) { }
+            } else if (typeof value === 'object') {
+                if (value.type === 'Buffer' && Array.isArray(value.data)) {
+                    return { value: Buffer.from(value.data) }
+                }
             }
-            catch (ignoreErr) { }
         }
     },
 
@QuentinFarizon QuentinFarizon changed the title Coerce objects of type Buffer to real Buffer Coerce JSON-stringified Buffer to real Buffer Jun 5, 2023
@QuentinFarizon QuentinFarizon changed the title Coerce JSON-stringified Buffer to real Buffer Coerce toJSON'ed Buffer to real Buffer Jun 5, 2023
@Marsup
Copy link
Collaborator

Marsup commented Jun 5, 2023

Can you provide your hapi configuration as well? I'm not aware of it transmitting buffers as objects.

@QuentinFarizon
Copy link
Author

QuentinFarizon commented Jun 5, 2023

Hello @Marsup
I'm not sure what you mean by hapi configuration, here is how I create the server object :

const server: Server = hapiServer({
    host,
    port,
    state: {
      strictHeader: false
    },
    routes: {
      cors: {
        origin: 'ignore',
        additionalHeaders: ['App-Version', 'source']
      },
      validate: {
        failAction: printValidationErrors
      },
      payload: {
        timeout: false
      },
      timeout: {
        socket: 300000,
        server: false
      }
    }
  })

@Marsup
Copy link
Collaborator

Marsup commented Jun 5, 2023

Is this is part of a multipart where you expect a file buffer on that route? Buffers initiated by hapi itself are actual buffers, and you should not have to do this, so unless you're actually serializing buffers as JSON (which would be bad), there must be something else interfering with the payload.

@QuentinFarizon
Copy link
Author

Ok I understand that having defined my route as taking JSON, it is expected to receive it as a JSON object. This is part of a larger JSON object. Still, I think it should be parsed as a native Buffer.

Or am I missing something on how to mix Joi.binary inside of a JSON payload ?

@Marsup
Copy link
Collaborator

Marsup commented Jun 5, 2023

JSON doesn't have semantics for serializing buffers, how is it supposed to pass it as such?

@QuentinFarizon
Copy link
Author

Well, passing it as { type: 'Buffer, data: } worked with my fix for parsing it, and seemed legit :)
But I guess this is a bit weird, indeed

I could instead define pass this Buffer as a serialNumber: Joi.array().items(Joi.number().min(0).max(256)).min(4).max(7).required() and parse it as a Buffer in the handler

@Marsup
Copy link
Collaborator

Marsup commented Jun 5, 2023

This is very inefficient to pass a buffer serialized as JSON, you shouldn't do that, it should be in a multipart field, or as a plain string if doable.

@QuentinFarizon
Copy link
Author

QuentinFarizon commented Jun 5, 2023

This is a serial number of length 4 to 7, so efficiency is not really an issue.

I'm not sure how to pass it as a string, since there could be 0 in it that would be considered as null-terminators

@kassah
Copy link
Contributor

kassah commented Jun 22, 2023

JSON doesn't have semantics for serializing buffers, how is it supposed to pass it as such?

@Marsup Buffer itself encodes to a format automatically: https://nodejs.org/api/buffer.html#buftojson as part of the Node Build-In API for it. However, there is no automatic function that automatically hooks into JSON parsing and converts it back the other way. The example code in the documentation does show that you can use Buffer.from(value) to reinterpret the resulting object that was encoded in Javascript to covert it back.

Since the binary type checker already uses the Buffer API, My suggestion would be to adapt the binary type coerce method to be something like:

    coerce(value, { schema }) {

        if (typeof value === 'string') {
            try {
                return { value: Buffer.from(value, schema._flags.encoding) };
            }
            catch (ignoreErr) { }
        }

        if (typeof value === 'object' && value.type === 'Buffer') {
            try {
                return { value: Buffer.from(value, schema._flags.encoding) };
            }
            catch (ignoreErr) { }
        }

        return { value };
    },

This follows the same logic presented in the code sample in the Node.js documentation of the buf.toJSON()

I put together a minimal reproduction of this issue that utilizes node.js Buffer API:

const Joi = require('joi');
// Setup Schema that requires a binary value.
const schema = Joi.object().keys({
    name: Joi.string().required(),
    content: Joi.binary().required(),
});
// test PNG graphic from Wikipedia PNG article
const testPng = Buffer.from('89504E470D0A1A0A0000000D49484452' +
    '00000001000000010802000000907753' +
    'DE0000000C4944415408D763F8CFC000' +
    '000301010018DD8DB00000000049454E' +
    '44AE426082', "hex");
// This creates a test object
const testObj = {
    name: "test.png",
    content: testPng
};
// Output validate() return prior to JSON encode/decode
console.log("Validation of Input", schema.validate(testObj));
// Covert object to JSON
const jsonOutput = JSON.stringify(testObj);
// Output JSON that was generated
console.log("JSON", jsonOutput);
// Convert JSON back to object
const testObjAfterJSON = JSON.parse(jsonOutput);
// Output restored object validate() return
console.log("Validation of Output", schema.validate(testObjAfterJSON));

Output of test using Joi 17.9.2 and node v20.0.0 without above change:

Validation of Input {
  value: {
    name: 'test.png',
    content: <Buffer 89 50 4e 47 0d 0a 1a 0a 00 00 00 0d 49 48 44 52 00 00 00 01 00 00 00 01 08 02 00 00 00 90 77 53 de 00 00 00 0c 49 44 41 54 08 d7 63 f8 cf c0 00 00 03 ... 19 more bytes>
  }
}
JSON {"name":"test.png","content":{"type":"Buffer","data":[137,80,78,71,13,10,26,10,0,0,0,13,73,72,68,82,0,0,0,1,0,0,0,1,8,2,0,0,0,144,119,83,222,0,0,0,12,73,68,65,84,8,215,99,248,207,192,0,0,3,1,1,0,24,221,141,176,0,0,0,0,73,69,78,68,174,66,96,130]}}
Validation of Output {
  value: { name: 'test.png', content: { type: 'Buffer', data: [Array] } },
  error: [Error [ValidationError]: "content" must be a buffer or a string] {
    _original: { name: 'test.png', content: [Object] },
    details: [ [Object] ]
  }
}

Output of test using Joi 17.9.2 and node v20.0.0 with above change:

Validation of Input {
  value: {
    name: 'test.png',
    content: <Buffer 89 50 4e 47 0d 0a 1a 0a 00 00 00 0d 49 48 44 52 00 00 00 01 00 00 00 01 08 02 00 00 00 90 77 53 de 00 00 00 0c 49 44 41 54 08 d7 63 f8 cf c0 00 00 03 ... 19 more bytes>
  }
}
JSON {"name":"test.png","content":{"type":"Buffer","data":[137,80,78,71,13,10,26,10,0,0,0,13,73,72,68,82,0,0,0,1,0,0,0,1,8,2,0,0,0,144,119,83,222,0,0,0,12,73,68,65,84,8,215,99,248,207,192,0,0,3,1,1,0,24,221,141,176,0,0,0,0,73,69,78,68,174,66,96,130]}}
Validation of Output {
  value: {
    name: 'test.png',
    content: <Buffer 89 50 4e 47 0d 0a 1a 0a 00 00 00 0d 49 48 44 52 00 00 00 01 00 00 00 01 08 02 00 00 00 90 77 53 de 00 00 00 0c 49 44 41 54 08 d7 63 f8 cf c0 00 00 03 ... 19 more bytes>
  }
}

Note that the type attribute isn't present in other JSON encoded values of other Node built in API objects. It appears to be unique to the Buffer API.

Keep in mind Buffer objects are a node.js API, and thus do not exist in browser (i.e. Chrome, Firefox, Safari, etc) based APIs. I don't think this is an issue for Joi since it already uses the Buffer API in the binary type check.

@kassah
Copy link
Contributor

kassah commented Jun 23, 2023

@QuentinFarizon In researching this a bit more, if you have control over the schema, this can be worked around by using alternatives with an object with custom casting.

   Joi.alternatives(
        Joi.binary(),
        Joi.object({
            type: 'Buffer',
            data: Joi.array().items(Joi.number().min(0).max(255))
        }).and('type','data').custom(Buffer.from)
    )

Now Joi 17.9.2 and node v20.0.0 running:

const Joi = require('joi');
// Setup Schema that requires a binary value.
const schema = Joi.object().keys({
    name: Joi.string().required(),
    content: Joi.alternatives(
        Joi.binary(),
        Joi.object({
            type: 'Buffer',
            data: Joi.array().items(Joi.number().min(0).max(255))
        }).and('type','data').custom(Buffer.from)
    ).required(),
});
// test PNG graphic from Wikipedia PNG article
const testPng = Buffer.from('89504E470D0A1A0A0000000D49484452' +
    '00000001000000010802000000907753' +
    'DE0000000C4944415408D763F8CFC000' +
    '000301010018DD8DB00000000049454E' +
    '44AE426082', "hex");
// This creates a test object
const testObj = {
    name: "test.png",
    content: testPng
};
// Output validate() return prior to JSON encode/decode
console.log("Validation of Input", schema.validate(testObj));
// Covert object to JSON
const jsonOutput = JSON.stringify(testObj);
// Output JSON that was generated
console.log("JSON", jsonOutput);
// Convert JSON back to object
const testObjAfterJSON = JSON.parse(jsonOutput);
// Output restored object validate() return
console.log("Validation of Output", schema.validate(testObjAfterJSON));

Results in the output of:

Validation of Input {
  value: {
    name: 'test.png',
    content: <Buffer 89 50 4e 47 0d 0a 1a 0a 00 00 00 0d 49 48 44 52 00 00 00 01 00 00 00 01 08 02 00 00 00 90 77 53 de 00 00 00 0c 49 44 41 54 08 d7 63 f8 cf c0 00 00 03 ... 19 more bytes>
  }
}
JSON {"name":"test.png","content":{"type":"Buffer","data":[137,80,78,71,13,10,26,10,0,0,0,13,73,72,68,82,0,0,0,1,0,0,0,1,8,2,0,0,0,144,119,83,222,0,0,0,12,73,68,65,84,8,215,99,248,207,192,0,0,3,1,1,0,24,221,141,176,0,0,0,0,73,69,78,68,174,66,96,130]}}
Validation of Output {
  value: {
    name: 'test.png',
    content: <Buffer 89 50 4e 47 0d 0a 1a 0a 00 00 00 0d 49 48 44 52 00 00 00 01 00 00 00 01 08 02 00 00 00 90 77 53 de 00 00 00 0c 49 44 41 54 08 d7 63 f8 cf c0 00 00 03 ... 19 more bytes>
  }
}

Which causes it to at least return the buffer how the application expects it.

While I personally think that the type coerce function should be adapted to handle the buffer in object format, this at least gives a workaround for those who need this capability now or if the repository maintainers disagree.

Either way! Great library with great documentation! Thank you!

@Marsup Marsup self-assigned this Aug 27, 2023
@Marsup Marsup added this to the 17.10.0 milestone Aug 27, 2023
@Marsup Marsup added the feature New functionality or improvement label Aug 27, 2023
@Marsup
Copy link
Collaborator

Marsup commented Aug 27, 2023

Implemented in #2960, soon to be released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or improvement
Projects
None yet
Development

No branches or pull requests

3 participants