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

Fix: Corrected Typo. Replaces #63 #80

Closed

Conversation

MarkKragerup
Copy link
Contributor

I don't know why #63 has a merge conflict. It does indeed look like a single letter is missing from that string. This PR should be mergeable. @nzakas can you merge and close #63 ?

@nzakas
Copy link
Contributor

nzakas commented Mar 28, 2022

Since this is part of a rule, it seems like this typo should be causing a bug. Can you please create a test that shows the bug is fixed?

@MarkKragerup
Copy link
Contributor Author

MarkKragerup commented Apr 7, 2022

Isn't it hard to show an error that is fixed with a test? i can only assume that the tests still work to show that all intended functionality is still there. That being said, this correction is correct, according to the Node.js documentation on buffer (https://nodejs.org/api/buffer.html#bufreaddoublebeoffset). There is no readDoubleL, but there should definitly be a readDoubleLE. I believe this one should be merged right away, as it removes 1 false positive and 1 false negative.

@MarkKragerup MarkKragerup changed the title Corrected Typo. Replaces #63 Fix: Corrected Typo. Replaces #63 Apr 7, 2022
@nzakas
Copy link
Contributor

nzakas commented Apr 8, 2022

You should be able to write a test that fails without this fix and passes when the fix is applied. Without doing so, we can’t really know if there was a problem with this code or if the proposed fix actually solves a problem.

@MarkKragerup
Copy link
Contributor Author

I can write a test, but since the fix is here it would just pass. Still, i don't agree that we can't really know if it solves a problem, since the string is checked against the official node api's buffer's member expression, and there is no readDoubleL but readDoubleLE is missing. And the tests still pass, so the change should have no breaking impact.
image

@nzakas
Copy link
Contributor

nzakas commented Apr 9, 2022

What I’m describing is common practice in programming. Write a test that fails to makes the bug obvious and then write some code to make the test pass.

In this case, the rule checks the array here:
https://github.com/nodesecurity/eslint-plugin-security/blob/208019bad4f70a142ab1f0ea7238c37cb70d1a5a/rules/detect-buffer-noassert.js#L62

So you should write a test that checks buf.readDoubleLE(0, true) was not flagged as invalid before your fix and is flagged as invalid after your fix.

This is the way we prevent regressions.

@nzakas nzakas closed this in 313c0c6 Apr 18, 2022
@nzakas
Copy link
Contributor

nzakas commented Apr 18, 2022

In the interest of time I fixed this myself.

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

2 participants