0
\$\begingroup\$

Object Property Sanitization

I'm learning to code servers using JavaScript, Node, and Express. While writing controllers that create new entries in the database, the need to sanitize the user input arised. Take this post controller as an example.

const Model = require('./Model')

const post = (req, res, nxt) => 
  Model
    .create(req.body)
    .then(createdItem => res.json(createdItem))
    .catch(err => nxt(err))

Ideally, we'd like to sanitize the req.body object (that is parsed from JSON, and is provided by the user) before passing it to the create method of the model, to only include specific properties, so the users can't mess with the database schema or change values they aren't supposed to.

I experimented a little, and came up with four different solutions, and a test suite for them.

I'm using Jest as test framework, Standard as code style, and JSDoc for documentation comments. Here is the code:

ops-for-each.js

/**
 * Creates a new object with the specified properties of the
 * input object, if they are present.
 * @param {object} object Object to sanitize.
 * @param {string[]} properties Properties to copy.
 * @returns Object with the specified properties if present in
 * original object.
 */
const sanitizeObjectProperties = (object, properties) => {
  const accumulator = {}

  properties.forEach(property => {
    if (property in object) accumulator[property] = object[property]
  })

  return accumulator
}

module.exports = {
  fun: sanitizeObjectProperties,
  id: 'for each'
}

ops-reduce.js

/**
 * Creates a new object with the specified properties of the
 * input object, if they are present.
 * @param {object} object Object to sanitize.
 * @param {string[]} properties Properties to copy.
 * @returns Object with the specified properties if present in
 * original object.
 */
const sanitizeObjectProperties = (object, properties) => {
  const reducer = (copy, property) => {
    if (property in object) copy[property] = object[property]
    return copy
  }

  return properties.reduce(reducer, {})
}

module.exports = {
  fun: sanitizeObjectProperties,
  id: 'reduce'
}

ops-filter-reduce.js

/**
 * Creates a new object with the specified properties of the
 * input object, if they are present.
 * @param {object} object Object to sanitize.
 * @param {string[]} properties Properties to copy.
 * @returns Object with the specified properties if present in
 * original object.
 */
const sanitizeObjectProperties = (object, properties) =>
  properties
    .filter(property => property in object)
    .reduce((copy, property) => {
      copy[property] = object[property]
      return copy
    }, {})

module.exports = {
  fun: sanitizeObjectProperties,
  id: 'filter reduce'
}

ops-entries-filter-map.js

/**
 * Creates a new object with the specified properties of the
 * input object, if they are present.
 * @param {object} object Object to sanitize.
 * @param {string[]} properties Properties to copy.
 * @returns Object with the specified properties if present in
 * original object.
 */
const sanitizeObjectProperties = (object, properties) =>
  Object.fromEntries(
    properties
      .filter(property => property in object)
      .map(property => [property, object[property]])
  )

module.exports = {
  fun: sanitizeObjectProperties,
  id: 'Object.fromEntries filter map'
}

ops.test.js

const testSubjects = [
  require('./ops-for-each'),
  require('./ops-reduce'),
  require('./ops-filter-reduce'),
  require('./ops-entries-filter-map')
]

testSubjects.forEach(({ fun, id }) => {
  test(`${id} sanitizes properly`, () => {
    const object = { title: 'One', year: 2021, hack: true }
    const properties = ['title', 'year']
    const sanitizedCopy = fun(object, properties)

    // Must not have the 'hack' property
    expect(sanitizedCopy).toEqual({ title: 'One', year: 2021 })
  })
})

Specific Review

I have some specific questions, but you can safely ignore some or all of them if you have something else to share about this code.

  • Is this code readable? If not, what would you change to make it more readable?
  • Is this code maintainable? If not, what would you change to make it more maintainable?
  • Is there a more efficient solution?
  • Is there a more concise or idiomatic alternative to these solutions? Perhaps using Object.assign, or object deconstruction, or other techniques?
  • Which solution do you prefer, in terms of readability and/or efficiency and/or conciseness?
  • Are the tests written correctly? Should other test scenarios be considered?
  • Are the documentation comments written correctly?
  • Are there any anti-patterns, code smells, or anything that I should not be doing? Am I missing any best practices?
  • Would you approach this problem in a different way?
  • I'm not mutating the original object, but creating a new one and inserting the specified properties. Is that ok? Are there advantages to mutating the original object instead?

General Review

While I have some specific questions, I want this to be an open review. Please share anything you'd do differently, maybe as an improvement, or maybe as an alternative worth considering. I'm interested in observations of the code in general, including the solutions, the tests, and the documentation comments.

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

I prefer either the reduce or fromEntries approaches. The first, ops-for-each.js, is really just an awkward reduce.

You should think about whether you really need the "filter" concept at all. Since you have a list of acceptable properties, as long as you are iterating through those, you will get the right answer. Consider the two cases of:

if (property in object) copy[property] = object[property]

The first, is where property is present, in which case the property is copied. Otherwise, it is not assigned, and therefore undefined. You'll notice you get the same answer without the if statement, as in simply copy[property] = object[property].

Consider

  • anonymous functions that are only called once
  • shorter variable names if the function is small
  • more standard variable names. I found it helpful to use accumulator in the first function, but was surprised when it wasn't named that in the reduce function, where that's the normal name. I find the following easier to get through than the more verbose one:
const sanitizeObjectProperties = (obj, props) => {
  return props.reduce((acc, p) => {
    acc[p] = obj[p]
    return acc
  }, {})
}

As long as you're making just one pass through the properties, you should be relatively good in terms of efficiency. The test looks good, and by having tests, you are improving your maintainability. There are a couple other tests: empty object and pre-sanitized object, which you'll want to have. Your approach of creating a new object is solid and safer than removing extra properties.

It's Javascript, so there's lots of ways to do the same thing. There's probably a clever way to do this with rest/spread, but what you have done (#2 or #4) are pretty clear.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.