fix: introduce security related linter and fix warnings #106

magicmatatjahu · 2020-07-06T14:05:08Z

Description

Add eslint-plugin-security package and configure it in project,
Bump eslint package to 7.X.X,
Introduce getMapKey util function for retrieve data by key from object - use it in models,
Fix warnings returned by eslint-plugin-security.

Related issue(s)
Resolves #103

derberg

Awesome stuff mate, just a few questions to understand some decisions

lib/models/operation.js

derberg · 2020-07-07T06:10:28Z

lib/models/schema.js

@@ -240,6 +230,7 @@ class Schema extends Base {
   * @returns {Object<string, Schema|string[]>}
   */
  dependencies() {
+    /* eslint-disable security/detect-object-injection */


why disable, we we cannot just use Map here. Which part linter doesn't like?

Linter doesn't like the key variable. If you use Object.keys() then you have 100% sure, than variable is string and we don't do things like _json.dependencies[key](), so we avoid eval malicious code, so I decided to disable plugin in those lines. I could use utils helper createMapOfType, but as you can see, in one case we create Schema Type, in another we write raw value.

I don't like to disable rules unless it is a test or there is really nothing else we can do.
values are accessed by a key and with Map it is not the case so it is better, so I don't really get your explanation about _json.dependencies[key]()

derberg · 2020-07-07T06:13:51Z

lib/parser.js

@@ -154,6 +154,7 @@ async function customDocumentOperations(js, asyncapiYAMLorJSON, initialFormat, o

  validateChannelParams(js, asyncapiYAMLorJSON, initialFormat);

+  /* eslint-disable security/detect-object-injection */


js.channels can be converted into Map -> https://github.com/asyncapi/parser-js/pull/105/files#diff-c3d5db07c30dd8c16e1da041b6f90850R115

of course depends what part is a problem for the linter

The problem is with retrieving value by channelName and opName variable. As you see, this variable will be in 100% strings.

And as I remember forEach method from ES6 Map cannot handle async functions, and in this function we must handle async conversion of Messages.

you are right about forEach but then you can just use for of

thing is that we should not assume that what we have now (100% string) will always be like that if the future, so better just to fix than ignore

derberg · 2020-07-07T06:17:02Z

lib/utils.js

+};
+
+utils.getMapKey = (obj, key) => {
+  return utils.getMapKeyOfType(obj, key);
 };

 utils.addExtensions = (obj) => {
  obj.prototype.extensions = function () {
    const result = {};
    Object.keys(this._json).forEach(key => {


same as with others, we could make this._json a Map

But then users will have to get data via the get () method and not via the array index, and for me this is a breaking change. If we decided to use Map here, then we should also adjust whole methods, which return plain JS Object, to Map. Or probably I don't understand your suggestion :D

you change it to a map only for forEach but in 193 still return pure json

derberg · 2020-07-07T06:34:13Z

lib/utils.js

@@ -157,31 +159,38 @@ utils.createMapOfType = (obj, Type) => {
  if (!obj) return result;

  Object.keys(obj).forEach(key => {
+    // eslint-disable-next-line security/detect-object-injection
    result[key] = new Type(obj[key]);
  });

  return result;
 };

 utils.getMapKeyOfType = (obj, key, Type) => {


I have some naming issues here, now this function is basically not only for getting a typed map, so name is no longer getMapKeyOfType and hiding it with getMapKey seems strange too me 🤔
why not try renaming getMapKeyOfType to something more generic? with good jsdoc?

I will try make it more generic :)

Also in 162 line, without eslint-disable, we should cast key to String like result[String(key)] = new Type(obj[String(key)]);

yeap, do the casting

did you make changes here? I don't think so

derberg

@magicmatatjahu added a few comments

derberg · 2020-07-07T14:03:24Z

lib/models/schema.js

@@ -240,6 +230,7 @@ class Schema extends Base {
   * @returns {Object<string, Schema|string[]>}
   */
  dependencies() {
+    /* eslint-disable security/detect-object-injection */


I don't like to disable rules unless it is a test or there is really nothing else we can do.
values are accessed by a key and with Map it is not the case so it is better, so I don't really get your explanation about _json.dependencies[key]()

derberg · 2020-07-07T14:07:59Z

lib/parser.js

@@ -154,6 +154,7 @@ async function customDocumentOperations(js, asyncapiYAMLorJSON, initialFormat, o

  validateChannelParams(js, asyncapiYAMLorJSON, initialFormat);

+  /* eslint-disable security/detect-object-injection */


you are right about forEach but then you can just use for of

thing is that we should not assume that what we have now (100% string) will always be like that if the future, so better just to fix than ignore

derberg · 2020-07-07T14:09:31Z

lib/utils.js

@@ -157,31 +159,38 @@ utils.createMapOfType = (obj, Type) => {
  if (!obj) return result;

  Object.keys(obj).forEach(key => {
+    // eslint-disable-next-line security/detect-object-injection
    result[key] = new Type(obj[key]);
  });

  return result;
 };

 utils.getMapKeyOfType = (obj, key, Type) => {


yeap, do the casting

did you make changes here? I don't think so

derberg · 2020-07-07T14:12:45Z

lib/utils.js

+};
+
+utils.getMapKey = (obj, key) => {
+  return utils.getMapKeyOfType(obj, key);
 };

 utils.addExtensions = (obj) => {
  obj.prototype.extensions = function () {
    const result = {};
    Object.keys(this._json).forEach(key => {


you change it to a map only for forEach but in 193 still return pure json

derberg

WELL DONE 👏

sonarqubecloud · 2020-07-09T14:38:16Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

asyncapi-bot · 2020-07-09T15:05:30Z

🎉 This PR is included in version 0.25.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

magicmatatjahu added the enhancement New feature or request label Jul 6, 2020

derberg requested changes Jul 7, 2020

View reviewed changes

derberg reviewed Jul 7, 2020

View reviewed changes

magicmatatjahu force-pushed the 103-security-linter branch from e48965f to 4d7cafe Compare July 8, 2020 10:50

derberg previously approved these changes Jul 9, 2020

View reviewed changes

magicmatatjahu added 5 commits July 9, 2020 16:31

chore: introduce security related linter and fix warnings

e05797c

Disable one of warnings by comment

5333142

After review v1

e9f6233

After review v2

ca033ff

After review v3

dc45451

magicmatatjahu dismissed derberg’s stale review via dc45451 July 9, 2020 14:32

magicmatatjahu force-pushed the 103-security-linter branch from 4cb89df to dc45451 Compare July 9, 2020 14:32

After rebase

3032815

derberg approved these changes Jul 9, 2020

View reviewed changes

derberg merged commit 9c5f891 into asyncapi:master Jul 9, 2020

asyncapi-bot added the released label Jul 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: introduce security related linter and fix warnings #106

fix: introduce security related linter and fix warnings #106

magicmatatjahu commented Jul 6, 2020

derberg left a comment

derberg Jul 7, 2020

magicmatatjahu Jul 7, 2020

derberg Jul 7, 2020

derberg Jul 7, 2020

magicmatatjahu Jul 7, 2020 •

edited

Loading

magicmatatjahu Jul 7, 2020

derberg Jul 7, 2020

derberg Jul 7, 2020

magicmatatjahu Jul 7, 2020

derberg Jul 7, 2020

derberg Jul 7, 2020

magicmatatjahu Jul 7, 2020

magicmatatjahu Jul 7, 2020

derberg Jul 7, 2020

derberg left a comment

derberg Jul 7, 2020

derberg Jul 7, 2020

derberg Jul 7, 2020

derberg Jul 7, 2020

derberg left a comment

sonarqubecloud bot commented Jul 9, 2020

asyncapi-bot commented Jul 9, 2020

		@@ -154,6 +154,7 @@ async function customDocumentOperations(js, asyncapiYAMLorJSON, initialFormat, o

		validateChannelParams(js, asyncapiYAMLorJSON, initialFormat);

		/* eslint-disable security/detect-object-injection */

fix: introduce security related linter and fix warnings #106

fix: introduce security related linter and fix warnings #106

Conversation

magicmatatjahu commented Jul 6, 2020

derberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magicmatatjahu Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derberg left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jul 9, 2020

asyncapi-bot commented Jul 9, 2020

magicmatatjahu Jul 7, 2020 •

edited

Loading