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.

Sign up for GitHub

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

Jump to bottom

loading: false values are lost #17

Open
mal-tee opened this issue Jan 26, 2021 · 4 comments
Open

loading: false values are lost #17

mal-tee opened this issue Jan 26, 2021 · 4 comments
Labels
bug Something isn't working hacktoberfest help wanted Extra attention is needed

Comments

@mal-tee
Copy link
Member

mal-tee commented Jan 26, 2021

E.g. click on the "Edit" link in datenanfragen/data#946
It contains some "optional": falses. But they are not picked up by the cjg:

grafik

Seems to be a bug in brutusin-json-forms and not in our code.. But still quite annoying.
@mal-tee mal-tee added the bug Something isn't working label Jan 26, 2021
@mal-tee mal-tee mentioned this issue Feb 15, 2021
Merged
@mal-tee mal-tee mentioned this issue Mar 20, 2021
Open
@baltpeter baltpeter added hacktoberfest help wanted Extra attention is needed labels Oct 2, 2021
@acedigibits

This comment was marked as resolved.

Sign in to view
@baltpeter

This comment was marked as outdated.

Sign in to view
@baltpeter
Copy link
Member

I had a go at fixing the bug in brutusin/json-forms today. I did find the bug and it would be possible to fork the library and fix the bug for us, but that would probably be more work than switching to another library as we're planning anyway (datenanfragen/website#561).

For the record, here's what I found:

  • There's a render() function that is called for each (sub-)value. It checks whether the value is falsy and changes it to the initial value or the default value if no initial value is provided:

    https://github.com/brutusin/json-forms/blob/44f27b29ef657f545b8a3d162c2b9c90ef137dbc/src/js/brutusin-json-forms.js#L1200-L1206

    Because the value you provide is false, it fulfills this condition and gets replaced with its initial value. The problem disappears if we change the condition to value !== undefined && value !== null.

    But I'm not sure if that's correct because I'm not even sure what the code in this if statement is supposed to do in the first place. As far as I can tell, this render() function is only called on the initial render of the form, and value is the initial data provided (so, in our case, the unchanged company record). But why would we want to change falsy values to their initial value, then? This shouldn't do anything, should it (value = value)?

  • If this code shouldn't do anything, why does the problem occur, then? Well, because getInitialValue() doesn't work properly.

    Here's the function:

    https://github.com/brutusin/json-forms/blob/44f27b29ef657f545b8a3d162c2b9c90ef137dbc/src/js/brutusin-json-forms.js#L1243-L1251

    initialValue is the company record in our case, and id is the path (looks like JsonPath, but not sure whether it actually is) to the (sub-)value we're interested in, so something like $.required-elements[0].optional in our case.

    Despite the use of eval() being less than ideal (and causing XSS if the attacker controls the schema…), this does work for most cases. The path provided is almost JS, so eval()ing it works in most cases.

    However, it breaks in our case, because required-elements contains a dash and initialValue.required-elements isn't valid JS (it should be initialValue['required-elements']). Thus, an exception is thrown, and we land in the catch, which returns null, causing the value to be lost.

    To fix this problem, we would have to write a whole JsonPath (or whatever variant is being used here) implementation to replace the getInitialValue() function.

That doesn't seem worth, especially since I'm not exactly confident in the quality of the library after looking at its code (did I mention the glaring XSS vuln that even the simplest automated scanner could spot?).

@baltpeter
Copy link
Member

For the record, should anyone want to dig into this further, here's how I got a working dev env for brutusin/json-forms:

You can't build the library with a modern version of Node. I switched to Node 8 using nvm (nvm install 8; nvm use 8).

Then, install the dependencies using npm i (there were node-gyp errors for me, but I just ignored them and that didn't seem to cause a problem.

The library is built using gulp (oh, "fun" memories). I used npx gulp-cli.

Here's the MRE test page I used:

<!DOCTYPE html>
<html>
    <body>
        <div id="form"></div>
        <button id="submit">Log data</button>
        <script type="module">
            require('../dist/js/brutusin-json-forms.min.js');

            const schema = {
                "$schema": "http://json-schema.org/draft-07/schema#",
                "type": "object",
                "properties": {
                    "mybool": {
                        "type": "boolean",
                        "default": true
                    },
                    "my-bool": {
                        "type": "boolean",
                    },
                    "mybool2": {
                        "type": "boolean",
                    },
                    "my-array": {
                        "type": "array",
                        "items": {
                            "type": "object",
                            "properties": {
                                "my-bool": {
                                    "type": "boolean",
                                }
                            },
                        }
                    },
                },
            };

            const initialData = {
                "mybool": true,
                "my-bool": false,
                "mybool2": true,
                "my-array": [
                  {
                    "my-bool": false
                  }
                ]
              };

            const BrutusinForms = brutusin['json-forms'];
            bf = BrutusinForms.create(schema);
            bf.render(document.getElementById('form'), initialData);

            document.getElementById('submit').addEventListener('click', () => {
                console.log(JSON.stringify(bf.getData(), null, 2));
            });
        </script>
    </body>
</html>

I put that into a new folder, switched back to modern Node for that folder and built it using npx parcel <file.html>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working hacktoberfest help wanted Extra attention is needed
Milestone
No milestone
Development

No branches or pull requests
3 participants
@baltpeter @acedigibits @mal-tee