Python/JS: Share modeling of crypto algorithms#5286
Conversation
Now to be shared accross both languages, with sync-identical-files
I didn't quite know where to place it for JS, so I tried my best :) The canonical Python version might be changed in the future, but I wanted to keep this change small.
asgerf
left a comment
There was a problem hiding this comment.
👍 Definitely a good idea to keep the files in sync, just want to make sure we're also in sync about filenames.
| ], | ||
| "CryptoAlgorithms Python/JS": [ | ||
| "javascript/ql/src/semmle/javascript/security/CryptoAlgorithms.qll", | ||
| "python/ql/src/semmle/crypto/Crypto.qll" |
There was a problem hiding this comment.
Could we have a bit more consistency between the languages here?
Either of these is fine with me:
semmle/<language>/security/CryptoAlgorithms.qll
semmle/crypto/CryptoAlgorithms.qll
There was a problem hiding this comment.
I could have elaborated on that a bit more. As part of an other PR I'm currently working on, I want to place this somewhere else for Python. Due to our deprecation rules, we can't just remove python/ql/src/semmle/crypto/Crypto.qll (at least not for a year to come), but nothing stops us from putting it in a nice place for JS.
My plan of action is to add a CryptoAlgorithms.qll somewhere, and make the existing semmle/crypto/Crypto.qll a dummy module only containing import <new-parent>.CryptoAlgorithms.
Does this sound good enough for you? (File will have same name CryptoAlgorithms.qll, but might be placed in a slightly differently folder-structure)
A more detailed explanation of where it will go for Python: I want to expose the content of CryptoAlgorithms.qll as part of our crypto concepts (defined in python/ql/src/semmle/python/Concepts.qll), so my initial plan was to move it to python/ql/src/semmle/python/concepts/CryptoAlgorithms.qll. That still leaves us with the file in 2 different locations, but I guess that our modeling is still so far from each other that it's difficult to get things matched perfectly. Let me know what you think 😊
While semmle/crypto/CryptoAlgorithms.qll isn't necessarily a bad idea (based on the fact that which crypto algorithms are safe/unsafe is language agnostic I assume), Python seems to be the only language placing significant code outside of semmle/<language>/. I certainly don't want to be accountable for starting to do that, so I would recommend we don't do this.
There was a problem hiding this comment.
Ah, I wasn't aware of the situation in Pythonland. This looks fine then. 👍
While waiting for github#5286 to be approved and merged :)
tausbn
left a comment
There was a problem hiding this comment.
One small comment and a suggestion, otherwise this looks ready to merge.
Co-authored-by: Taus <tausbn@github.com>
I didn't quite know where to place it for JS, so I tried my best 😊 let me know if it should be placed somewhere else 👍