jm / toml

Parse TOML. Like a bawss.
MIT License
151 stars 38 forks source link

Too accepting of malformed input #5

Open cespare opened 11 years ago

cespare commented 11 years ago

Thanks for the great gem. I'm looking forward to the next gem release because I'm using the git version to get TOML generation.

This parser accepts several kinds of input specifically described as invalid at https://github.com/mojombo/toml. I discovered these errors using toml-test; see the test methodology and output in this gist.

I ran tests against the latest head of master (479353753fc71173e9a8f778a9c3c20a0f92d189)

  1. Accepts arrays with mixed types, which are not valid toml.

    Spec:

    No, you can't mix data types, that's stupid.

    Example TOML:

    arrays-and-ints =  [1, ["Arrays are not integers."]]
  2. Accepts mutually duplicated keys/keygroups.

    Spec:

    Be careful not to overwrite previous keys. That's dumb. And should produce an error.

    Example 1:

    [fruit]
    type = "apple"
    
    [fruit.type]
    apple = "yes"

    Example 2:

    [a]
    [a]

    Example 3:

    dupe = false
    dupe = true
dirk commented 11 years ago

The above commit fixes Example 1 and Example 3 in the duplicate keys problem you brought up. I'm not exactly sure whether or not Example 2 is technically invalid. Has @mojombo clarified this anywhere?

cespare commented 11 years ago

@dirk Example 2. is still a case of a duplicate key, if you take 'key' to mean hash key (in the decoded data structure), which makes sense given that the duplicate key thing is discussed under the Hash section of the spec. IMO it's also "dumb". If you really think that @mojombo intended that to be allowed, I'd be happy to open a ticket against that project asking for clarification.

dirk commented 11 years ago

@cespare My reasoning for not checking for that duplication is that each keygroup specification [a.b] tells the parser what keygroup to go into for setting keys and values. So doing [a] followed by another [a] is just extraneous (telling the parser to go into the keygroup path "a" twice); however, extraneous is not necessarily invalid. I personally don't mind throwing in a check for this as long as it's consistent with other parsers, but this is ultimately @jm and @mojombo's call.

cespare commented 11 years ago

Ah -- you're right, it's not as bad as I had thought. However, it's still extraneous and my hunch is that @mojombo votes no (and I certainly would). I went ahead and filed mojombo/toml#145 to get clarification. Anyway, this one's really minor (and thanks for fixing the other two).

dirk commented 11 years ago

No problem; thanks for pointing these issues out!

cespare commented 11 years ago

@dirk @mojombo clarified this over on mojombo/toml#145 that example 2 is also invalid.

dirk commented 11 years ago

@cespare Okay cool. I'll see if I can fix it this afternoon!