JS: add taint-step for URL construction in js/request-forgery - #5630

I do not understand this test and the new QL step. Can you elaborate?

My thoughts:

I agree that request.url is user-controlled, but it will only reach this line if there is a route handler that listens at request.url. So I would treat as "mostly sanitized" if the route handler does not listen on * (it does that in his test though).

I can see that this may be vulnerable after all since the new url(/sitelet?url=https%3A%2F%2Fgithub.com%2Fgithub%2Fcodeql%2Fpull%2Frequest.url%2C%2520base) also changes the host of the URL, causing the "mostly sanitized" url to mean something different to a different host. But that subtlety is not reflected in the QL change that do not check for a second argument.

I think you mostly answered your own question.

So I would treat as "mostly sanitized" if the route handler does not listen on * (it does that in his test though).

Yes, it does sound reasonable to treat such urls as mostly sanitized.
But I think it would take some effort to do, as we would need to match handlers with which urls they listen on.
And I think the url can be tainted even if the handler doesn't listen on *, as long as the handler listens on some "blob".
(E.g. "/foo/bar/*" or "/foo/:bar").

But that subtlety is not reflected in the QL change that do not check for a second argument.

I could add a requirement that the second argument is present (we would still flag the CVE).
In the scenario I looked at the problem was that a certain /priv folder on the target server was supposed to be private (all requests were supposed to go to a /pub folder).

But I also think we should keep the one argument var succ = new url(/sitelet?url=https%3A%2F%2Fgithub.com%2Fgithub%2Fcodeql%2Fpull%2Fpred) step, because it is essentially an identity function (that sometimes throws an exception).

I think we might need some advanced flow-label use if we want to keep track of "how unsafe" a given url is.
(Maybe we could do something similar to the js/tainted-path query).

Yes, we risk that the results might become slightly noisy, but I think it seems safe enough to land.

OK. I am inclined to accept the risk of the FPs, @asgerf do you have any additional thoughts on these URL subtleties?

We could get FPs from accessing safe parts of the URL, like new url(/sitelet?url=https%3A%2F%2Fgithub.com%2Fgithub%2Fcodeql%2Fpull%2Ftaint).port, but since it only affects the request forgery query I'm fine with it.