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 #964 Joi wrong value parsing #1097

Merged
merged 4 commits into from
Mar 26, 2017
Merged

Fix #964 Joi wrong value parsing #1097

merged 4 commits into from
Mar 26, 2017

Conversation

EvgenyOrekhov
Copy link
Contributor

Fixes TypeError: Cannot assign to read only property '_$miss$_forbiddenKey|1_$end$_'
when a parameter is given as a JSON string with incorrect property.

lib/errors.js Outdated
@@ -285,6 +285,9 @@ internals.annotate = function (stripColorCodes) {
delete ref[replacement];
}
else {
if (typeof ref === 'string') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably the good fix but in the wrong place. If the error is further down the parsed object, this will fail. This should be in the very 1st if of the loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Marsup Could you provide a failing example?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just add some more nesting on a and cause a deeper error, like `a: "{b:{c:"string"}}". Your code basically only works if the error happens on the object being parsed, not inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Marsup I tried this:

            const schema = {
                a: Joi.object({
                    b: Joi.object({
                        c: Joi.string()
                    })
                })
            };

            const input = {
                a: '{"b":{"d":"string"}}'
            };

            expect(() => {

                Joi.attempt(input, schema);
            }).to.throw(/\"d\" is not allowed/);
            done();

and that:

            const schema = {
                a: Joi.object({
                    b: Joi.object({
                        c: Joi.object({
                            d: Joi.string()
                        })
                    })
                })
            };

            const input = {
                a: '{"b":{"c":{"d":{}}}}'
            };

            expect(() => {

                Joi.attempt(input, schema);
            }).to.throw(/\"d\" must be a string/);
            done();

Both cases are not failing without any changes to the fix.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should have been more accurate, while this is fixing the failure, it's not fixing the issue. If you look at the error annotation, it's showing the list of errors correctly but can't pinpoint it on the input.

Eg. with your 2nd example :

ValidationError: {
  "a": "{\"b\":{\"c\":{\"d\":{}}}}" // There should be a [1] here or on the parsed object
}

[1] "d" must be a string

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Marsup I fixed the issue with the '_$miss$_forbiddenKey|1_$end$_' TypeError.
You are talking about an issue with error annotation.
I added a failing test case for the missing [1], but I don't know how to fix it yet.
Moving the typeof ref === 'string' check to the very 1st if of the loop doesn't help.

@EvgenyOrekhov
Copy link
Contributor Author

@Marsup I fixed the pinpointing of errors. Please review the new changes.

@EvgenyOrekhov
Copy link
Contributor Author

@Marsup What's the hold-up on this PR?
Please review the new changes and let me know if I need to change anything else.

@Marsup Marsup self-assigned this Mar 25, 2017
@Marsup Marsup added the bug Bug or defect label Mar 25, 2017
@Marsup
Copy link
Collaborator

Marsup commented Mar 25, 2017

Lacking time essentially. Adding more tests around this I noticed some nasty bugs that need fixing as well, and it involves a fair amount of rewrite of this algorithm, which is probably more than you'd be willing to do.

I'll merge this PR when I'm sure I can ship the follow up patch with it.

@Marsup Marsup merged commit e349bc8 into hapijs:master Mar 26, 2017
Marsup added a commit that referenced this pull request Mar 26, 2017
@Marsup Marsup added this to the 10.3.1 milestone Mar 26, 2017
@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
bug Bug or defect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants