Skip to content

Get() key search can bleed through levels of JSON hierarchy #5

Description

@daboyuka

I want to first thank you, @buger, for your work on this library. Looking up a few JSON key paths in large JSON blobs is a significant bottleneck in a project I'm working on, and your library could give us a big speedup without changing our data format.

Unfortunately, I've discovered an issue in Get(): when searching for a key, Get() may locate that key outside the current JSON object. Here is an example test case that breaks (written using check.v1):

package jsonparser_test

import (
    "github.com/buger-jsonparser"
    . "gopkg.in/check.v1"
    "testing"
)

func (s *JsonParserTests) TestJsonParserSearchBleed(c *C) {
    killer := []byte(`{
      "parentkey": {
        "childkey": {
          "grandchildkey": 123
        },
        "otherchildkey": 123
      }
    }`)

    var jtype int

    _, jtype, _, _ = jsonparser.Get(killer, "childkey")
    c.Assert(jtype, Equals, jsonparser.NotExist) // fails, returns data parentkey.childkey

    _, jtype, _, _ = jsonparser.Get(killer, "parentkey", "childkey", "otherchildkey")
    c.Assert(jtype, Equals, jsonparser.NotExist) // fails, returns data parentkey.otherchildkey
}

// Boilerplate
func Test(t *testing.T) { TestingT(t) }
type JsonParserTests struct{}
var _ = Suite(&JsonParserTests{})

The issue is that Get() uses bytes.Index() to find the next key it's looking for, but only validates it by checking that it is surrounded by double quotes and followed by a colon. In particular, it does not check whether it has crossed an unmatched sequence of braces, which would indicate transitioning into another JSON object level.

I don't have a great suggestion as to how to fix this, sadly. Best of luck.

Activity

  1. buger commented on Mar 22, 2016

    @buger
    Owner

    Good point, i have few ideas how to fix it, will keep you updated, thanks!

  2. buger commented on Mar 22, 2016

    @buger
    Owner

    Should be fixed now, for each next key it will limit scope to parent key object.

  3. daboyuka commented on Mar 22, 2016

    @daboyuka
    ContributorAuthor

    Sorry to say, your patch only fixes the second failing assertion, but not the first (which I think is the more difficult one, sadly).

  4. buger commented on Mar 22, 2016

    @buger
    Owner

    I see what you mean, thank you for pointing. Yes, it is definitely a bit harder, will try to get it fixed.

  5. reopened this on Mar 22, 2016
  6. daboyuka commented on Mar 22, 2016

    @daboyuka
    ContributorAuthor

    For what it's worth, my suggestion would be to use your parsing functions to more carefully step through the keys in an object. A (very) rough sketch:

    1. nextValue to seek to a key
    2. bytes.Equal to compare the key
    3. if failed, stringEnd to skip the key
    4. skip whitespace, colon, whitespace
    5. check type of value, use trailingBracket to skip object, etc.
    6. Repeat

    Unfortunately this won't be nearly as efficient as bytes.Index, so perhaps there is a more efficient way.

  7. daboyuka commented on Mar 22, 2016

    @daboyuka
    ContributorAuthor

    I found another case that breaks the current parser: key "<anything>\"abc": matches a lookup for key "abc". Here's an expanded test case including that:

    package jsonparser_test
    
    import (
        "github.com/buger-jsonparser"
        . "gopkg.in/check.v1"
        "testing"
    )
    
    func (s *JsonParserTests) TestJsonParserSearchBleed(c *C) {
        //c.Skip("jsonparser is broken as of 2016-03-21")
    
        killer := []byte(`{
              "parentkey": {
                "childkey": {
                  "grandchildkey": 123
                },
                "otherchildkey": 123
              },
              "bad key\"good key": 123,
            }`)
    
        var jtype int
    
        _, jtype, _, _ = jsonparser.Get(killer, "childkey")
        c.Assert(jtype, Equals, jsonparser.NotExist) // fails, returns data from parentkey.childkey
    
        _, jtype, _, _ = jsonparser.Get(killer, "parentkey", "childkey", "otherchildkey")
        c.Assert(jtype, Equals, jsonparser.NotExist) // fails, returns data from parentkey.otherchildkey
    
        _, jtype, _, _ = jsonparser.Get(killer, "good key")
        c.Assert(jtype, Equals, jsonparser.NotExist) // fails, returns data from badkey"goodkey
    }
    
    // Boilerplate
    func Test(t *testing.T) { TestingT(t) }
    type JsonParserTests struct{}
    var _ = Suite(&JsonParserTests{})
    
  8. buger commented on Mar 22, 2016

    @buger
    Owner

    Can you try this branch https://github.com/buger/jsonparser/tree/key-search, i rewrote how key search works. Thanks!

  9. buger commented on Mar 22, 2016

    @buger
    Owner

    (to test branch, create vendor folder and put jsonparser to vendor/github.com/buger/jsonparser, then checkout needed branch there)

  10. daboyuka commented on Mar 22, 2016

    @daboyuka
    ContributorAuthor

    Thanks, I'll check it in a little while. I'll also make a PR with the tests above for your convenience. Thanks for tackling this!

  11. added a commit that references this issue on Mar 22, 2016
  12. daboyuka commented on Mar 22, 2016

    @daboyuka
    ContributorAuthor

    Your key-search branch passes the three cases identified in this issue (test cases added in #7). Thanks!

  13. added a commit that references this issue on Mar 23, 2016
  14. buger commented on Mar 24, 2016

    @buger
    Owner

    Merged to master, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions