I did spend some time to figure out how to best write `minimumSecureKeySize`
predicate. I wanted to write once and for all the recommended sizes for each
cryptosystem.
I considered making the predicate such as
```codeql
int minimumSecureKeySize() {
this.getName() = "RSA" and result = 2048
or
this.getName() = "DSA" and result = 2048
or
this.getName() = "ECC" and result = 244
}
```
but then it would be impossible to add a new model without also being able to
modify the body of this predicate -- which seems like a bad way to start off a
brand new way of modeling things.
So I considered if we could add it to the non-range class, such as
```codeql
class RSAKeyGeneration extends KeyGeneration {
RSAKeyGeneration() { this.getName() = "RSA" }
override int minimumSecureKeySize() { result = 2048 }
}
```
This has the major problem that when you're writing the models for a new
API (and therefore extending KeyGeneration::Range), there is no way for you to
see that you need to take this extra step :| (also problem about how we should
define `minimumSecureKeySize` on `KeyGeneration` class then, since if we make it
abstract, we effectively disable the ability to refine `KeyGeneration` since any
subclass must provide an implementation.)
So, therefore I ended up with this solution ;)
- Fail the CI check if change note is missing.
- Disregards changes outside of `*/ql/src`.
- Runs the workflow on label changes, and upon moving the PR out of
draft mode.
- Only fails the CI check if the PR is out of draft.
- Changes label to `no-change-note-required`.
Tests working can be verified by running
```
ls ql/python/ql/test/experimental/library-tests/frameworks/crypto*/*.py | xargs -L1 sh -c 'python $0 || exit 255'
```
When I changed the taint modeling in 19b7ea8d85, that obviously also means that
some of the related locations for alerts will change. So that's why all the
examples needs to be updated.
Besides this, I had to fix a minor problem with having too many alerts. If
running a query agaisnt code like in the example below, there would be 3 alerts,
2 of them originating from the import.
```
from flask import Flask, request
app = Flask(__name__)
@app.route("/route")
def route():
SINK(request.args.get['input'])
```
The 2 import sources where:
- ControlFlowNode for ImportMember
- GSSA Variable request
I removed these from being a RemoteFlowSource, as seen in the diff.
I considered restricting `FlaskRequestSource` so it only extends
`DataFlow::CfgNode` (and make the logic a bit simpler), but I wasn't actually
sure if that was safe to do or not... If you know, please let me know :)
We don't actually need it for anything right now, but I have plans for the
future where would need it.
Although it would be nice to have it as an `API::Node`, and we could re-write
implementations so we could provide it in this instance, I'm not convinced we
can do that in general right now.
For example, if <n'th> parameter of a function has to be modeled as belonging to
a certain type, I don't see any way to specify that as an API::Node.
For me, that's ok. Until we _can_ specify things like this as API::Nodes in the
future, I would like to keep things consistent, and use `DataFlow::Node` as the
result type.
This was a good time to do this, so we don't have 2 different ways of doing the
same thing.
I needed to do this to figure out if we should expose
`API::moduleImport("flask").getMember("request")` in a helper predicate or
not. I think I ended up using more refenreces to this in the end. Although it's
not unreasonable to let someone do this themselves, I also think it's reasonable
that we provide a helper predicate for this.
