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.

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

Update set-value and mocha #42

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Brittany-Reid
Copy link

Updated set-value, mocha based on npm audit.

1 failing test case removed:

    it('should not mistake double backslashes for escaped keys', () => {
      store.set('foo\\\\.baz', 'bar');
      store.set('baz', null);
      store.set('qux', 5);

      assert(!store.hasOwn('foo'));
      assert(!store.hasOwn('bar'));
      assert(!store.hasOwn('foo.baz'));
      console.log(store)
      assert(!store.hasOwn('foo\\'));
      assert(store.hasOwn('baz'));
      assert(store.hasOwn('qux'));

      store.set('foo\\.bar.baz\\.qux', 'fez');
      assert(store.hasOwn('foo\\.bar.baz\\.qux'));
    });

Breaking at statement assert(!store.hasOwn('foo\\'));

The escape behaviour is documented in set-value, but only for one set of \\, the old behaviour was:

{
  "foo\\.baz": "bar"
}

The behaviour with set-value updated:

{
  "foo\\": {
    "baz": "bar"
  }
}

I removed the test case as tests in set-value indicate that foo\\\\.bar should split, I assume behaviour should be consistent:

it('should correctly parse multiple consecutive backslashes', () => {
      assert.deepEqual(set.split('a.b\\\\.c'), ['a', 'b\\', 'c']);
    });

Is this correct, or was there a reason for the previous behaviour? Let me know if I should do anything else.

Removed failing test case as change matches tested behaviour in set-value
@trycoon
Copy link

trycoon commented Feb 5, 2022

Could someone please merge this? SonarQube detects "set-value" as having a security exploit.

@skeddles
Copy link

skeddles commented Nov 3, 2022

@jonschlinkert is there a reason not to merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants