Introduce TypeScript and an integration test

[dbartholomae]

As discussed, I've moved the code to TypeScript, added linting and prettification as well as an integration test to make it easier for adding more tests.
Happy to also jump onto a call for a sync review if that is easier. If you review this async, I recommend going commit by commit. I've tried to group them in a way that makes it possible to review them one commit at a time.

Some things of note:

1. I've updated the version of the GitHub Action dependencies, as the code was already relying on a new interface, but the dependency in package.json was still on an old version. The released version that is in use in the actual actions is not using what is currently in this repo in the master branch, but after the changes I did, things should work now (as confirmed by the integration test).
2. There was also a missing dependency on request, I assume because it was installed as a global dependency on the machine of the person who built and published this package.
3. I've added TypeScript interfaces for the data structures used. Report, Site, Alert, and Instance are based on what should be in the JSON output file, so it would be great if you could double check them and confirm the following:
   • There is always an array of sites in the report as site
   • Each site always has a @name , but it might not have an array of alerts
   • Each alert always has a name, pluginid, an array of instances and (not yet in this PR but the info will be needed for the next) both riskcode as well as confidence which only can have the values given at https://www.zaproxy.org/docs/constants/

[dbartholomae]

@psiinon

[psiinon]

Woah!
Many thanks!
I'm off for the rest of this week and heading over to SF for OWASP AppSec next week, but I'll aim to have a good look at this Mon/Tues.

[psiinon]

A README.md which gave a quick summary of the commands needed to build / test this repo would be very useful 😁

[kingthorin]

Along with any details about dependency/framework maintenance going forward.

[dbartholomae]

Thanks! I've added a CONTRIBUTING.md and, while I was at it, a workflow to automatically run tests/linting/builds on CI.

Along with any details about dependency/framework maintenance going forward.

Not sure what you would like to see for this. Could you give me some more details? Thanks!

[kingthorin]

I dunno, just whatever dependency hell we might need to know about upgrading jest, npm stuff (just npm audit --force?), etc. Just a line or two for anyone that isn't familiar with the tech(s).

[dbartholomae]

Like running npm audit fix in case of security problems? I can mention that running this should "just work" if that's what you are looking for.
I allso see that dependabot is enabled for this repo - I would suggest to just enable auto merge from dependabot if the build is green, e.g. via mergify. We could even automatically release new versions of the package on each merge to master. Happy to set that up - I would need some help with installing mergify and setting up the related secret for publishing on npm, though :)

[kingthorin]

npm audit fix (with or without force), if that's all that's needed it's probably fine. I was just concerned that there might be something else for jest or the linters, or something ts specific, that I (or someone in the future) might not be familiar with.

[dbartholomae]

Not that I'm aware of. If something comes to my mind over the next days, I'll add it, but the setup is optimized for just working, as this package most likely won't see that much love compared to others :)

[dbartholomae]

@psiinon quick bump

[thc202]

Is it usual that the main code is mixed with test code? Doesn't it allow to split that per dir?

[dbartholomae]

Is it usual that the main code is mixed with test code? Doesn't it allow to split that per dir?

That's a stylistic preference. I have a strong preference for having test files directly next to the files under test, as it makes it easier to find them, and e.g. if a file changes directory it is easier to also relocate the file as there are no two separate folder structures to maintain.
In the past, it was beneficial to have separate directories, because it made it easier to ship only production code and no test code to production. That's not an issue here, as we explicitly define which files to build and distribute in the build config.

[dbartholomae]

@psiinon another reminder

[dbartholomae]

@psiinon Hi Simon, do you know when you will be able to check the PR? Otherwise I would go ahead with a fork and release that for now, and we can potentially move the fork back into the zaproxy organisation some time in the future.

[thc202]

That's not an issue here

I know it's not an issue, it's a matter of organization. No big deal for me.

[thc202]

A test is failing for me locally:

➜  scans git:(pr/45) ✗ npm run test

> @zaproxy/[email protected] test
> jest

 PASS  src/models/FilteredSite.test.ts
 FAIL  src/index.test.ts
  ● Test suite failed to run

    /zaproxy/actions-common/packages/scans/node_modules/jest-worker/build/index.js:1
    {"site":[{"@name":"http://localhost:8080","alerts":[{"instances":[{"uri":"http://localhost:8080/bodgeit/contact.jsp"}],"name":"Cross Site Scripting (Reflected)","pluginid":"40012"}]}]}
           ^

    SyntaxError: Unexpected token ':'

      at _jestWorker (node_modules/jest-runner/build/testWorker.js:37:16)

Test Suites: 1 failed, 1 passed, 2 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        5.855 s
Ran all test suites.

Might be better to create a PR just adding the CI and merge it before this one, to get the checks running here.

[dbartholomae]

Huh, that's weird. I have CI already running on my fork, and it's green. Do you have any global npm packages installed that might interfere with local versions? Which version of Node are you running?
I'll examine a bit closer this weekend.

[dbartholomae]

Could be related to the way that I currently mock the file system. Not sure why it doesn't show up on CI or locally, though, as I work on Windows and CI on Linux, which already covers two file systems.

[dbartholomae]

Might be better to create a PR just adding the CI and merge it before this one, to get the checks running here.

That's basically what this PR is: it is not introducing any new functionality, but adds the functionality to run tests. We could add the workflow file only on a separate branch which would lead to a failing workflow (as there are no tests) so that it can run on this PR if we want.
Since the repo isn't actually in use (the current repo HEAD actually isn't working and is not the version that is used by the actions), it could also be fine to merge and fix on main in case the tests fail (which they shouldn't as I run the CI on my fork).

[thc202]

Do you have any global npm packages installed that might interfere with local versions?

Most likely not, how should I check that? (I don't use npm often.) I followed the steps provided in the docs, if other changes are needed would be better to point that out.

Which version of Node are you running?

v19.1.0

I work on Windows and CI on Linux, which already covers two file systems.

I used macOS.

That's basically what this PR is

No, adding the CI workflow does not require doing any changes nor running the tests (once merged it would run with these changes though, hopefully). Anyway, good to see passing in CI already.

[dbartholomae]

I can confirm that mocking the file system does not seem to work on Mac by running it on Mac in CI. Unfortunately, the current way the script is written requires mocking the file system for the e2e test. This will be less of a problem for future changes, as these can be driven by unit tests instead of e2e tests. To solve it, I now skip the test on Mac only. I've also set up CI to run the repo on Linux, Mac, and Windows to detect these kinds of problems.

[thc202]

Thank you!

[thc202]

Shouldn't the mocks and fixtures being excluded as well?

[thc202]

In any case not a blocker for me, can be tweaked after merge if needed.

[dbartholomae]

Good catch! Will address in next PR to not require an additional review of this one :)

[dbartholomae]

(And, just in case this isn't clear: This change wouldn't affect the users of the GitHub action, as the action is build via a bundler that anyways only bundles files that are used by the actual production code, so at that step all fixtures, tests, and test helpers would be ignored)

[psiinon]

Thanks @dbartholomae and sorry this took so long to review. As you can see we need help with the actions 😁

[dbartholomae]

Thanks! I've written #48 to plan the next steps and would love your feedback on that before I continue :)