along with tests, but no implementations (to ease reviewing).
---
I've put quite some thinking into what to call our concept for this.
[JS has `CookieDefinition`](581f4ed757/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll (L148-L187)), but I couldn't find a matching concept in any other languages.
We used to call this [`CookieSet`](f07a7bf8cf/python/ql/src/semmle/python/web/Http.qll (L76)) (and had a corresponding `CookieGet`).
But for headers, [Go calls this `HeaderWrite`](cd1e14ed09/ql/src/semmle/go/concepts/HTTP.qll (L97-L131)) and [JS calls this `HeaderDefinition`](581f4ed757/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll (L23-L46))
I think it would be really cool if we have a naming scheme that means the name for getting the value of a header on a incoming request is obvious. I think `HeaderWrite`/`HeaderRead` fulfils this best. We could go with `HeaderSet`/`HeaderGet`, but they feel a bit too vague to me. For me, I'm so used to talking about def-use, that I would immediately go for `HeaderDefinition` and `HeaderUse`, which could work, but is kinda strange.
So in the end that means I went with `CookieWrite`, since that allows using a consistent naming scheme for the future :)
I made `FileSystemWriteAccess` be a subclass of `FileSystemAccess` (like in [JS](64001cc02c/javascript/ql/src/semmle/javascript/Concepts.qll (L68-L74))), but then I started wondering about how I could give a good result for `getAPathArgument`, and what would a good result even be? The argument to the `open` call, or the object that the `write` method is called on? I can't see how doing either of these enables us to do anything useful...
So I looked closer at how JS uses `FileSystemWriteAccess`:
1. as sink for zip-slip: 7c51dff0f7/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlipCustomizations.qll (L121)
2. as sink for downloading unsafe files (identified through their extension) through non-secure connections: 89ef6ea4eb/javascript/ql/src/semmle/javascript/security/dataflow/InsecureDownloadCustomizations.qll (L134-L150)
3. as sink for writing untrusted data to a local file 93b1e59d62/javascript/ql/src/semmle/javascript/security/dataflow/HttpToFileAccessCustomizations.qll (L43-L46)
for the 2 first sinks, it's important that `getAPathArgument` has a proper result... so that solves the problem, and highlights that it _can_ be important to give proper results for `getAPathArgument` (if possible).
So I'm trying to do best effort for `f = open(...); f.write(...)`, but with this current code we won't always be able to give a result (as highlighted by the tests). It will also be the case that there are multiple `FileSystemAccess` with the same path-argument, which could be a little strange.
overall, I'm not super confident about the way this new concept and implementation turned out, but it also seems like the best I could come up with right now...
The obvious alternative solution is to NOT make `FileSystemWriteAccess` a subclass of `FileSystemAccess`, but I'm not very tempted to go down this path, given the examples of this being useful above, and just the general notion that we should be able to model writes as being a specialized kind of `FileSystemAccess`.
Problematic part is
```codeql
/** A escape from string format with `markupsafe.Markup` as the format string. */
private class MarkupEscapeFromStringFormat extends MarkupSafeEscape, Markup::StringFormat {
override DataFlow::Node getAnInput() {
result in [this.getArg(_), this.getArgByName(_)] and
not result = Markup::instance()
}
override DataFlow::Node getOutput() { result = this }
}
```
since the char-pred still holds even if `getAnInput` has no results...
I will say that doing it this way feels kinda dirty, and we _could_ fix
this by including the logic in `getAnInput` in the char-pred as well.
But as I see it, that would just lead to a lot of code duplication,
which isn't very nice.
Since expectation tests had so many changes from ConceptsTest, I'm going
to do the changes for that on in a separate commit. The important part
is the changes to taint-tracking, which is highlighted in this commit.
Since we want to be able to easy select request-handlers that are not
set up as part of a view-class, we need to easily be able to identify
those. To handle cases like the one below, we _can't_ just define these
to be all the async functions that are not methods on a class :(
```py
# see https://docs.aiohttp.org/en/stable/web_quickstart.html#organizing-handlers-in-classes
class MyCustomHandlerClass:
async def foo_handler(self, request): # $ MISSING: requestHandler
return web.Response(text="MyCustomHandlerClass.foo")
my_custom_handler = MyCustomHandlerClass()
app.router.add_get("/MyCustomHandlerClass/foo", my_custom_handler.foo_handler) # $ routeSetup="/MyCustomHandlerClass/foo"
```
So it seemed easiest to narrow down the route-setups, but that means we
want both refinement and extensibility... so `::Range` pattern to the
rescue 🎉
The important piece of code that still works after this commit, but
which hasn't been changed, is the one below:
```codeql
/**
* A parameter that will receive a `aiohttp.web.Request` instance when a request
* handler is invoked.
*/
class AiohttpRequestHandlerRequestParam extends Request::InstanceSource, RemoteFlowSource::Range,
DataFlow::ParameterNode {
AiohttpRequestHandlerRequestParam() {
exists(Function requestHandler |
requestHandler = any(AiohttpCoroutineRouteSetup setup).getARequestHandler() and
```
