crytic / slither-action

GNU Affero General Public License v3.0
128 stars 20 forks source link

Markdown example no longer working #59

Closed TheWisestOne closed 9 months ago

TheWisestOne commented 1 year ago

The example usage of the action given in the docs for creating a comment in PRs using Markdown does not appear to work, as of somewhat recently. Specifically, I get an error along the lines of: Unhandled error: SyntaxError: Invalid left-hand side expression in postfix operation

This appears to be related to the way the value of ${{ steps.slither.outputs.stdout }} is inserted into the code.

I belive the output of the previous step includes multiple lines, and JS does not correctly treat the whole thing as one string in this line of code:

const body = `${{ steps.slither.outputs.stdout }}`

Instead, it tries to execute some of the output from steps.slither.outputs.stdout as though it were JS code. Not sure if something abut the way this code ingetion works has changed recently under the hood, but I have recently started encountering this issue across multiple repositories

elopez commented 1 year ago

Thanks for the report @TheWisestOne! Escaping may be a problem indeed 🤔 There may be a safer way to implement this with environment variables; could you give something like this a try if you have the time?

...
    - name: Create/update checklist as PR comment
      uses: actions/github-script@v6
      if: github.event_name == 'pull_request'
      env:
        REPORT: ${{ steps.slither.outputs.stdout }}
      with:
        script: |
          const script = require('.github/scripts/comment')
          const header = '# Slither report'
          const { REPORT } = process.env
          await script({ github, context, header, REPORT })

@tlvince hi! tagging you for visibility; I recall you originally contributed the example, have you observed this happening recently?

TheWisestOne commented 1 year ago

This appears to have fixed it!

Had to make a corresponding change in comment.js as well, but using the env var was exactly the trick I was looking for. It was unclear to me how to access the github step output from the javascript code, as I'm not that familiar with scripting in github actions

0xGuybrush commented 1 year ago

This appears to have fixed it!

Had to make a corresponding change in comment.js as well, but using the env var was exactly the trick I was looking for. It was unclear to me how to access the github step output from the javascript code, as I'm not that familiar with scripting in github actions

@TheWisestOne would you mind sharing the change you made to comment.js? I'm hitting this issue atm too.

TheWisestOne commented 1 year ago

Just renamed the body argument to REPORT and replaced its usage in the code. Should just require changing the first line (function definiton) and the second line, replacing body with REPORT

Sorry, not on the computer I did this on so I can't share the exact code diff

0xGuybrush commented 1 year ago

Just renamed the body argument to REPORT and replaced its usage in the code. Should just require changing the first line (function definiton) and the second line, replacing body with REPORT

Sorry, not on the computer I did this on so I can't share the exact code diff

Cheers thanks!

I was wondering why it was off, but from your pointer, realised that it was because the shorthand assignment was no longer working for the payload to script, as it expected a property called body but we were now passing in REPORT.

I was able to get it sorted from this change within the YAML (without changing the script itself):

await script({ github, context, header, body: REPORT })
elopez commented 1 year ago

@TheWisestOne @0xGuybrush Thanks for confirming this approach with the env variable works! 👍 I opened PR #62 to update the example in the repo README

0xGuybrush commented 1 year ago

@TheWisestOne @0xGuybrush Thanks for confirming this approach with the env variable works! 👍 I opened PR #62 to update the example in the repo README

Just one downside with this @elopez, I was hitting Argument list too long on some branches. (I think because GH actions is expanding out variables and then getting hit with a really long env to run Node with).

I think it's OK for us now that I know why/what it's doing (if it fails on this, run locally & solve the glut of new issues 🙂), but just an FYI.