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

feat(customProbes): inject custom probes as param for AstAnalyser #250

Merged
merged 9 commits into from
Mar 28, 2024

Conversation

tchapacan
Copy link
Contributor

@tchapacan tchapacan commented Mar 9, 2024

fix #221

Inject custom probes as param for AstAnalyser

const { AstAnalyser, JsSourceParser } = require("@nodesecure/js-x-ray");

const analyser = new AstAnalyser({
  parser: new JsSourceParser(),
  customProbes,
  skipDefaultProbes: true
});

Allow developers to inject new custom probe using the AstAnalyser

Purpose of this PR is to propose a fix/feat for the issue #221 by modifying the constructor of some of the relevant class such as (ProbeRunner, ASTAnalyzer, SourceFile) :

  • code
  • tests
  • docs

I tried to keep it as simple as possible without modifying that much what was already done (i haven't change the ProbeRunner instanciation from SourceFile), but I'm open to suggestion.

Don't hesitate to give any feedback/suggestion/proposition to improve this PR

Result after (yes this probe below is perfectible ahah) :

const kIncriminedCodeSample = "const danger = 'danger';";

const customProbes = [
  {
    name: "customProbeUnsafeDanger",
    validateNode: (node, sourceFile) => [true]
    ,
    main: (node, options) => {
      const { sourceFile, data: calleeName } = options;
      if (node.declarations[0].init.value === "danger") {
        sourceFile.addWarning("unsafe-danger", calleeName, node.loc);
  
        return ProbeSignals.Skip;
      }
  
      return null;
    }
  }
];

const analyser = new AstAnalyser({
  parser: new JsSourceParser(),
  customProbes,
  skipDefaultProbes: true
});

const result = analyser.analyse(kIncriminedCodeSample);
console.log(result);
➜  js-x-ray git:(fix/221) ✗ node example.js
{
  idsLengthAvg: 0,
  stringScore: 0,
  warnings: [ { kind: 'unsafe-danger', location: [Array], source: 'JS-X-Ray' } ],
  dependencies: Map(0) {},
  isOneLineRequire: false
}

src/SourceFile.js Outdated Show resolved Hide resolved
Copy link
Contributor

@jean-michelet jean-michelet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few questions, congratulations on the work accomplished.

src/AstAnalyser.js Outdated Show resolved Hide resolved
src/SourceFile.js Outdated Show resolved Hide resolved
src/SourceFile.js Outdated Show resolved Hide resolved
test/utils/index.js Outdated Show resolved Hide resolved
@tchapacan tchapacan force-pushed the fix/221 branch 2 times, most recently from 901f01b to 0115836 Compare March 11, 2024 18:00
src/AstAnalyser.js Outdated Show resolved Hide resolved
types/api.d.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@jean-michelet jean-michelet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some toughs, maybe @PierreDemailly can review my suggestions before you continue.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
src/AstAnalyser.js Outdated Show resolved Hide resolved
src/AstAnalyser.js Show resolved Hide resolved
types/api.d.ts Outdated Show resolved Hide resolved
src/AstAnalyser.js Outdated Show resolved Hide resolved
src/AstAnalyser.js Outdated Show resolved Hide resolved
@jean-michelet

This comment was marked as resolved.

Copy link
Contributor

@jean-michelet jean-michelet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After that, should be ok 👍

src/AstAnalyser.js Outdated Show resolved Hide resolved
src/AstAnalyser.js Outdated Show resolved Hide resolved
test/AstAnalyser.spec.js Outdated Show resolved Hide resolved
test/issues/221-inject-custom-probes.spec.js Outdated Show resolved Hide resolved
@tchapacan tchapacan marked this pull request as ready for review March 19, 2024 19:33
Copy link
Contributor

@jean-michelet jean-michelet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, with a nit

test/AstAnalyser.spec.js Outdated Show resolved Hide resolved
@tchapacan
Copy link
Contributor Author

I have proposed a quick update of the readme, if you think it needs more details i can try create a dedicated .md file for the customProbe part ? and refer it in the readme briefly ?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@PierreDemailly
Copy link
Member

I have proposed a quick update of the readme, if you think it needs more details i can try create a dedicated .md file for the customProbe part ? and refer it in the readme briefly ?

The update is fine 👍 I don't think we need a dedicated doc for the moment

@tchapacan
Copy link
Contributor Author

I have proposed a quick update of the readme, if you think it needs more details i can try create a dedicated .md file for the customProbe part ? and refer it in the readme briefly ?

The update is fine 👍 I don't think we need a dedicated doc for the moment

Thanks for the review, I have fix the little typo in the readme in latest commit, lmk if it seems good enough

Copy link
Member

@PierreDemailly PierreDemailly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.

Inject custom probes in AstAnalyser class
4 participants