mafintosh / mafintosh/csv-parser

Error on "reserved property names"?

Open
#228 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
1.5k
Forks
143
PR merge metrics
No merged PRs in 30d

Description

  • Operating System: Windows 10
  • Node Version: 19.0.1
  • NPM Version: 9.3.1
  • csv-parser Version: 3.0.0

Minor issue possibly?

Expected Behavior

An error should be able to be thrown if enabled in settings should a "reserved" or potentially problematic header name be specified in the csv contents.

Actual Behavior

Property names specified in csv headers can be set on row objects this could lead to unexpected errors or code problems when calling methods on the objects when they might be considered to be "reserved" or should avoid to be set.

How Do We Reproduce?

code.js

Readable.from(`toString
1`).pipe(csv({ columns: true,  strict: true, from_line: 1 }))
    .on('data', (data) => {
        console.log(data.toString(), data.toString() === '[object Object]')
    })

error TypeError: data.toString is not a function

This could be detected in the emitted error event but records imported could actually be used elsewhere outside of the stream processing.

See also: although it may not be every possible name that should not be used in a header.
https://www.w3schools.com/js/js_reserved.asp

JavaScript Objects, Properties, and Methods
You should also avoid using the name of JavaScript built-in objects, properties, and methods:

In a node.js repl for example, this would break toString on object.

// > o = {}
{}
// > o['toString'] = ""
''
// > o.toString()
Uncaught TypeError: o.toString is not a function

Which should return

'[object Object]'

I'm just wondering if there should be a way to protect against setting property names that could lead to unexpected code issues, devs are not always in control of user uploaded content. Or maybe there already is and I'm not aware?

Headers could be defined in the CSV, I suppose the mapHeaders or a validateHeaders option could be used to prevent this.

A way to specify exact header names and strip all other columns might be nice to have as well where the order of headers may come from the CSV content its self e.g. if a user reorders them legitimately in Excel then exports to CSV we should still be able to import the records into objects.

I'm not aware of real problems atm but prototype poisoning vulnerabilities do exist in other ways for JSON parsing when combined with deep copying, or if this library ever adds a feature like nested values it may be a larger problem
https://medium.com/intrinsic-blog/javascript-prototype-poisoning-vulnerabilities-in-the-wild-7bc15347c96

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by running the code.js reproduction with csv-parser 3.0.0 and review the header handling, including the mapHeaders and validateHeaders options mentioned in the report. Done should be a tested, documented way to reject or safely handle problematic header names, including the reproduced toString case.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
backend
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.