example-instancescan-checks icon indicating copy to clipboard operation
example-instancescan-checks copied to clipboard

Update check for use of GlideRecordSecure on Client Callable Script Includes

Open niamccash opened this issue 1 year ago • 7 comments

Create new Linter Check type Instance Scan for use of GlideRecord Secure on Client Callable Script Includes

As per https://docs.servicenow.com/bundle/utah-api-reference/page/script/server-scripting/concept/c_ScriptIncludes.html#title_client-callable-script-includes

image

Note: there is already an existing Table Check for this. This issue is for a Linter type check.

niamccash avatar Oct 11 '23 14:10 niamccash

Hi @niamccash I just had a chat with a friend who is deep into the instance scan and he recommended to do this with Scritp only check, rather than Linter check. His arguments are:

  • Linter Check is not specifically Script Include, it will run on all scripts.
  • we can add a condition to check only the script include table, but it will still run all of the time, which is bad practioce.

let me know if you still insist to have a Linter check or I can take it over and create a Script only check. Thank you

MartinStoyanoff avatar Oct 11 '23 19:10 MartinStoyanoff

@MartinStoyanoff That's good to know. Thanks for sharing. Could the Script only check catch instances where GlideRecord use is commented out?

I like Linter checks because it does check for actual code use and ignores things like

// var gr = new GlideRecord('incident');

Where devs may have commented out old code instead of removing.

niamccash avatar Oct 11 '23 19:10 niamccash

Hi @niamccash Should I start with the linter check then?

aman2519 avatar Oct 13 '23 13:10 aman2519

Hi @aman2519.

I'm waiting for @MartinStoyanoff's input but I don't see the harm in having multiple checks. If you want to create the Linter check and submit a PR, I will happily accept.

As there will be multiple check types that scan for the same thing, it would be nice to have some comments in the code to help explain how the Linter check is different than the Table check and a Script Only check.

As an added challenge, see if you can add a condition to your Linter check to only check the script include table.

niamccash avatar Oct 13 '23 14:10 niamccash

@aman2519 you can take it and implement the linter check. @niamccash to your question, it will check commented code as well. What I pointed out in my previous comment is an input from Mark Roethof. I am not an instance scan expert at all.

MartinStoyanoff avatar Oct 16 '23 13:10 MartinStoyanoff

@MartinStoyanoff @niamccash For checking commented code, there are some out-of-the-box checks which look to be something like that... I haven't validated it myself. See for example: https://your-instance.service-now.com/nav_to.do?uri=scan_column_type_check.do?sys_id=dfd6ccb8db796510dd95b7e8f49619eb

markroethof avatar Oct 20 '23 18:10 markroethof

@niamccash Linter Check could still work fine if its for a specific table, then you need to start the check with something like engine.current.getTableName() == 'lalala' else return. Downside... the Linter Check will run for several minutes, while the Script Only Check or Table Check will run only for a few seconds.

markroethof avatar Oct 20 '23 19:10 markroethof