Strict ACL parsing without a regex #2587
Conversation
|
ttfkam
changed the title
There was a problem hiding this comment.
This definitely looks more performant - a single loop over the string rather than at least 26! However, it's not as clearly correct as it was - the previous code was very obvious and this code is less so - so it will need some unit tests. Just simple assertions that a number of standard permissions strings with/without role, with/without grants parse correctly. Since pg-introspection doesn't seem to have tests already, please use node:test for them (or mocha/chai if you prefer).
In general we trust permission strings to be valid (we trust Postgres) so I'm not too concerned about malicious permission strings, however Postgres does add new permissions from time to time so it's worth ensuring that the code correctly handles (via an error) a new permission character.
I've included a number of TypeScript improvements, all minor.
| } | ||
| i++; | ||
| } // end token processing | ||
| if (i < aclLength) { |
There was a problem hiding this comment.
Previously the granter was required, and in general I think it will always be set. Let's just throw an error if it's the empty string.
Description
Supersedes #2586 per maintainer comments.
Motivation: browsing through the code and saw a simple opportunity.
No new tests. Should only require regression testing.
Performance impact
Minor speed improvement for ACL parsing logic in aggregate due to one less regex.
Security impact
Excludes most invalid ACL input. (Doesn't stop duplicate ACL tokens.)
Checklist
yarn lint:fixpasses.yarn testpasses.RELEASE_NOTES.mdfile (if one exists).