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

sanitization rules aren't applied if a custom exec is used #146

Open
steve-allam opened this issue Mar 30, 2023 · 4 comments
Open

sanitization rules aren't applied if a custom exec is used #146

steve-allam opened this issue Mar 30, 2023 · 4 comments

Comments

@steve-allam
Copy link

steve-allam commented Mar 30, 2023 •
edited by mattwelke

Not sure if this is expected behaviour, as the documentation doesn't say either way (that I can see).

I have pasted an example script below. I am using a custom function 'normalise' to match various options and convert to my required string, but it would appear that when using this - the rules aren't applied. ['trim','lower'] doesn't happen. If you run the script as below, the gender will come out as 'Male', whereas I would expect it to be 'male'.

I have added a line in the normalise function to apply the rules, but I expected them to be applied anyway?

Regards,

Steve
p.s. Many thanks for a useful library!

var inspector = require('schema-inspector');

// Sanitization Schema
var sanitize = {
  type: 'object',
  properties: {
    name: { type: 'string', rules: ['trim', 'title'] },
    gender: { code:'gender', type: 'string', rules: ['trim', 'lower'],
      $normalise: [
        { match: ['m','gent'], convert: 'male' },
        { match: ['f','female','lady'], convert: 'female' }
      ]
    }
  }
};

var customSanitize = {
  normalise: function (schema, candidate) {
    var arr = schema.$normalise
//    if (schema.rules) { candidate = this.rules(schema, candidate); }   /// Line added to apply rules!!
    for (var i=0;i<arr.length;i++)
    {
      var rx = new RegExp(arr[i].match.map((item) => { return '^'+item+'$'}).join('|'));
      if (rx.test(candidate)) { return arr[i].convert; break; }
    }
    return candidate;
  },
};

var data = {
  name: 'fred smith',
  gender: 'Male',
}

let s = inspector.sanitize(sanitize, data, customSanitize);
console.log(s);
@mattwelke
Copy link
Collaborator

mattwelke commented Mar 30, 2023 •
edited

Hmm, good point. We used to use exec at a previous company for validation, but we didn't have any other rules in place besides it. So that explains why I never ran into this before. When I look at the README, it doesn't make it clear whether exec is meant to add to the rules specified or replace any other rules specified. My gut feeling is that the original library author meant for it to be a kind of escape hatch where it would completely override any other rules specified.

I think a good approach would be for us to keep it that way and add docs to make it clear that's how it's supposed to work. We can also add docs for a workaround. I like that you've come up with a workaround, but it does appear to use internal implementation details. A different workaround that I think would be more sustainable long term would be nested use of the library. For example, someone could specify an exec function that did their custom behavior and then used another instance of Schema Inspector within the function body, with another, much smaller schema, to apply a few more Schema Inspector rules. Or, they could do it the other way around. First, the rules, then their custom logic. That workaround would not involve using internal implementation details. It would be 100% public API use.

Alternatively, we could expand the library so that the type of exec becomes object|<existing_function_type> instead. If it's an object, it would be an object with two properties, additive: boolean and fn: <existing_function_type>. That would control the behavior we're talking about here, and would be configurable in a backwards compatible way.

Curious what your thoughts are on these paths.

@steve-allam
Copy link
Author

steve-allam commented Mar 30, 2023 via email

@mattwelke
Copy link
Collaborator

Yeah thinking about this again, my workaround does sound pretty annoying. It sucks to have to use more than one schema when you've done the work to write a schema that can be used for both validation and sanitization, which is literally this library's reason to exist in a world where we have JSON Schema already.

I think my idea for adding support for "additive exec" is the way to go.

Because this library is in maintenance mode right now (I'm the only maintainer and I'm not paid to work on OSS), I wasn't originally planning to add new features, just bug fixes and security vulnerabilities. To me, this feature lacking sounds like a bug. So I'll complete the work when I can. I can't make any promises on timeline though. I recommend implementing the multiple schema workaround I suggested if you need to get around this bug urgently. If you're in no rush, feel free to keep an eye on the releases for the fix.

@steve-allam
Copy link
Author

steve-allam commented Apr 3, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mattwelke @steve-allam