JS: add js/http-dependency query#7633
Conversation
esbena
left a comment
There was a problem hiding this comment.
I think this query requires better documentation, especially the qhelp. (the experimental-promotion of the java query may have been a bit too fast)
javascript/ql/src/Security/CWE-300/InsecureDependencyResolution.qhelp
Outdated
Show resolved
Hide resolved
javascript/ql/src/Security/CWE-300/InsecureDependencyResolution.qhelp
Outdated
Show resolved
Hide resolved
javascript/ql/src/Security/CWE-300/InsecureDependencyResolution.qhelp
Outdated
Show resolved
Hide resolved
javascript/ql/src/Security/CWE-300/InsecureDependencyResolution.qhelp
Outdated
Show resolved
Hide resolved
javascript/ql/src/Security/CWE-300/InsecureDependencyResolution.ql
Outdated
Show resolved
Hide resolved
javascript/ql/src/Security/CWE-300/InsecureDependencyResolution.ql
Outdated
Show resolved
Hide resolved
|
I've addressed your comments. |
javascript/ql/src/Security/CWE-300/InsecureDependencyResolution.qhelp
Outdated
Show resolved
Hide resolved
|
|
||
| <example> | ||
| <p> | ||
| The below example shows a <code>package.json</code> file that downloads a dependency using unencrypted HTTP. |
There was a problem hiding this comment.
| The below example shows a <code>package.json</code> file that downloads a dependency using unencrypted HTTP. | |
| The below example shows a <code>package.json</code> file that downloads a dependency using the insecure HTTP protocol. |
I think it is preferable to talk about the vaguer secure/insecure instead of encrypted. Technically, you just need the packets from the server to be signed, it is fine if they are transmitted in cleartext.
... which leads me to consider if we should have a line about verifying the md5sum of insecurely downloaded artifacts as an alternative to the secure download.
I do not think we need it here though, that can be added for a overly aggressive query that checks if any md5sum verification is done on any downloaded resources.
There was a problem hiding this comment.
which leads me to consider if we should have a line about verifying the md5sum of insecurely downloaded artifacts as an alternative to the secure download.
Can you do that for dependencies downloaded through a package.json file?
Wouldn't that require downloading the dependency (without running install script), verifying a checksum, and then run the install script.
I think such a setup is sufficiently rare that we don't need to mention it.
It's common on <script/> tags, but that's another matter.
There was a problem hiding this comment.
It's common on <script/> tags, but that's another matter.
Great idea: https://github.com/github/codeql-javascript-team/issues/374
|
Thanks. I have pinged the docs team, my suggested changes should not be blocking them. |
Co-authored-by: Esben Sparre Andreasen <esbena@github.com>
mchammer01
left a comment
There was a problem hiding this comment.
@erik-krogh - LGTM ✨
Spotted an inconsistency with the query ID, and made a few minor comments.
javascript/ql/lib/change-notes/2022-01-08-insecure-dependency-resolution.md
Outdated
Show resolved
Hide resolved
javascript/ql/src/Security/CWE-300/InsecureDependencyResolution.ql
Outdated
Show resolved
Hide resolved
javascript/ql/src/Security/CWE-300/InsecureDependencyResolution.qhelp
Outdated
Show resolved
Hide resolved
javascript/ql/src/Security/CWE-300/InsecureDependencyResolution.qhelp
Outdated
Show resolved
Hide resolved
javascript/ql/src/Security/CWE-300/InsecureDependencyResolution.qhelp
Outdated
Show resolved
Hide resolved
javascript/ql/src/Security/CWE-300/InsecureDependencyResolution.qhelp
Outdated
Show resolved
Hide resolved
Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>
mchammer01
left a comment
There was a problem hiding this comment.
LGTM, thanks for addressing my comments!
Inspired by this java query.
An evaluation looks fine.
This query has extremely few results, but you can find a few using code search.
Here is a LGTM run with some of those projects.