Skip to content
This repository has been archived by the owner on Aug 5, 2021. It is now read-only.

Wrong prefix match #53

Open
sualko opened this issue May 30, 2018 · 3 comments
Open

Wrong prefix match #53

sualko opened this issue May 30, 2018 · 3 comments

Comments

@sualko
Copy link

sualko commented May 30, 2018

isEqual: function(a, b) {
// TODO: Special-case arraybuffers, etc
if (a === undefined || b === undefined) {
return false;
}
a = util.toString(a);
b = util.toString(b);
var maxLength = Math.max(a.length, b.length);
if (maxLength < 5) {
throw new Error("a/b compare too short");
}
return a.substring(0, Math.min(maxLength, a.length)) == b.substring(0, Math.min(maxLength, b.length));
}

I think someone tried here to implement a prefix match, but it should not work, because it's the same as

return a.substring(0, a.length) == b.substring(0, b.length);

Am I right, or did I overlook something? Are you interested in a pr?

@elsehow
Copy link

elsehow commented Jun 1, 2018

I am definitely up for a PR!

In the PR, be sure to make a test that:

  • Fails as it's written currently
  • Explains why the test should pass
  • Passes with your propose implementation

Thank you for reviewing the code!

@sualko
Copy link
Author

sualko commented Jun 1, 2018

The problem is, that I'm not sure if the intended behavior is that this is a prefix match. It would be awesome if a developer could give a hint.

@elsehow
Copy link

elsehow commented Jun 1, 2018

@sualko Indeed. Unfortunately, this code all comes from the libsignal-javascript project.

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

No branches or pull requests

2 participants