From 72ea4ff0dcd4dbe80ca9f3040fe655c7b875d289 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 18 May 2020 16:56:47 +0200 Subject: [PATCH 01/13] Python: Add more tests of django responses They clearly shouldn't all be XSS sinks --- .../web/django/HttpResponseSinks.expected | 10 +++++ .../test/library-tests/web/django/views_1x.py | 26 ++++++++++++- .../library-tests/web/django/views_2x_3x.py | 38 ++++++++++--------- .../Security/lib/django/http/__init__.py | 2 +- .../Security/lib/django/http/response.py | 38 ++++++++++++++++++- 5 files changed, 93 insertions(+), 21 deletions(-) diff --git a/python/ql/test/library-tests/web/django/HttpResponseSinks.expected b/python/ql/test/library-tests/web/django/HttpResponseSinks.expected index 0180a8726e4..8ecde34d08d 100644 --- a/python/ql/test/library-tests/web/django/HttpResponseSinks.expected +++ b/python/ql/test/library-tests/web/django/HttpResponseSinks.expected @@ -8,6 +8,11 @@ | views_1x.py:45:25:45:70 | django.Response(...) | externally controlled string | | views_1x.py:66:25:66:55 | django.Response(...) | externally controlled string | | views_1x.py:75:25:75:33 | django.Response(...) | externally controlled string | +| views_1x.py:85:25:85:55 | django.Response(...) | externally controlled string | +| views_1x.py:90:25:90:33 | django.Response(...) | externally controlled string | +| views_1x.py:94:25:94:58 | django.Response(...) | externally controlled string | +| views_1x.py:99:33:99:55 | django.Response(...) | externally controlled string | +| views_1x.py:103:33:103:55 | django.Response(...) | externally controlled string | | views_2x_3x.py:8:25:8:63 | django.Response(...) | externally controlled string | | views_2x_3x.py:12:25:12:52 | django.Response(...) | externally controlled string | | views_2x_3x.py:16:25:16:53 | django.Response(...) | externally controlled string | @@ -21,3 +26,8 @@ | views_2x_3x.py:82:25:82:69 | django.Response(...) | externally controlled string | | views_2x_3x.py:85:25:85:64 | django.Response(...) | externally controlled string | | views_2x_3x.py:88:25:88:32 | django.Response(...) | externally controlled string | +| views_2x_3x.py:106:25:106:55 | django.Response(...) | externally controlled string | +| views_2x_3x.py:111:25:111:33 | django.Response(...) | externally controlled string | +| views_2x_3x.py:115:25:115:58 | django.Response(...) | externally controlled string | +| views_2x_3x.py:120:33:120:55 | django.Response(...) | externally controlled string | +| views_2x_3x.py:124:33:124:55 | django.Response(...) | externally controlled string | diff --git a/python/ql/test/library-tests/web/django/views_1x.py b/python/ql/test/library-tests/web/django/views_1x.py index 068f8246210..abc3b716a2f 100644 --- a/python/ql/test/library-tests/web/django/views_1x.py +++ b/python/ql/test/library-tests/web/django/views_1x.py @@ -1,6 +1,6 @@ """test of views for Django 1.x""" from django.conf.urls import patterns, url -from django.http.response import HttpResponse +from django.http.response import HttpResponse, HttpResponseRedirect, JsonResponse, HttpResponseNotFound from django.views.generic import View @@ -77,3 +77,27 @@ def kw_args(request): urlpatterns = [ url(view=kw_args, regex=r'^kw_args$') ] + +# Not an XSS sink, since the Content-Type is not "text/html" +# FP reported in https://github.com/github/codeql-python-team/issues/38 +def fp_json_response(request): + # implicitly sets Content-Type to "application/json" + return JsonResponse({"foo": request.GET.get("foo")}) + +# Not an XSS sink, since the Content-Type is not "text/html" +def fp_manual_json_response(request): + json_data = '{"json": "{}"}'.format(request.GET.get("foo")) + return HttpResponse(json_data, content_type="application/json") + +# Not an XSS sink, since the Content-Type is not "text/html" +def fp_manual_content_type(reuqest): + return HttpResponse('', content_type="text/plain") + +# XSS FP reported in https://github.com/github/codeql/issues/3466 +# Note: This should be a open-redirect sink, but not a XSS sink. +def fp_redirect(request): + return HttpResponseRedirect(request.GET.get("next")) + +# Ensure that subclasses are still vuln to XSS +def tp_not_found(request): + return HttpResponseNotFound(request.GET.get("name")) diff --git a/python/ql/test/library-tests/web/django/views_2x_3x.py b/python/ql/test/library-tests/web/django/views_2x_3x.py index 50fe6d638fe..ce4a565c7a0 100644 --- a/python/ql/test/library-tests/web/django/views_2x_3x.py +++ b/python/ql/test/library-tests/web/django/views_2x_3x.py @@ -1,6 +1,6 @@ """testing views for Django 2.x and 3.x""" from django.urls import path, re_path -from django.http import HttpResponse +from django.http import HttpResponse, HttpResponseRedirect, JsonResponse, HttpResponseNotFound from django.views import View @@ -99,24 +99,26 @@ urlpatterns = [ ] -################################################################################ +# Not an XSS sink, since the Content-Type is not "text/html" +# FP reported in https://github.com/github/codeql-python-team/issues/38 +def fp_json_response(request): + # implicitly sets Content-Type to "application/json" + return JsonResponse({"foo": request.GET.get("foo")}) # TODO +# Not an XSS sink, since the Content-Type is not "text/html" +def fp_manual_json_response(request): + json_data = '{"json": "{}"}'.format(request.GET.get("foo")) + return HttpResponse(json_data, content_type="application/json") # TODO -# We should abort if a decorator is used. As demonstrated below, anything might happen +# Not an XSS sink, since the Content-Type is not "text/html" +def fp_manual_content_type(reuqest): + return HttpResponse('', content_type="text/plain") # TODO -# def reverse_kwargs(f): -# @wraps(f) -# def f_(*args, **kwargs): -# new_kwargs = dict() -# for key, value in kwargs.items(): -# new_kwargs[key[::-1]] = value -# return f(*args, **new_kwargs) -# return f_ +# XSS FP reported in https://github.com/github/codeql/issues/3466 +# Note: This should be a open-redirect sink, but not a XSS sink. +def fp_redirect(request): + return HttpResponseRedirect(request.GET.get("next")) # TODO -# @reverse_kwargs -# def decorators_can_do_anything(request, oof, foo=None): -# return HttpResponse('This is a mess'[::-1]) - -# urlpatterns = [ -# path('rev/', decorators_can_do_anything), -# ] +# Ensure that subclasses are still vuln to XSS +def tp_not_found(request): + return HttpResponseNotFound(request.GET.get("name")) diff --git a/python/ql/test/query-tests/Security/lib/django/http/__init__.py b/python/ql/test/query-tests/Security/lib/django/http/__init__.py index 962077dbad6..f2ac6d2c55b 100644 --- a/python/ql/test/query-tests/Security/lib/django/http/__init__.py +++ b/python/ql/test/query-tests/Security/lib/django/http/__init__.py @@ -1,2 +1,2 @@ -from .response import HttpResponse +from .response import * from .request import HttpRequest diff --git a/python/ql/test/query-tests/Security/lib/django/http/response.py b/python/ql/test/query-tests/Security/lib/django/http/response.py index ae101562df4..96cc2bda9a9 100644 --- a/python/ql/test/query-tests/Security/lib/django/http/response.py +++ b/python/ql/test/query-tests/Security/lib/django/http/response.py @@ -1,2 +1,38 @@ -class HttpResponse(object): +class HttpResponseBase(object): + status_code = 200 + + +class HttpResponse(HttpResponseBase): pass + + +class HttpResponseRedirectBase(HttpResponse): + pass + + +class HttpResponsePermanentRedirect(HttpResponseRedirectBase): + status_code = 301 + + +class HttpResponseRedirect(HttpResponseRedirectBase): + status_code = 302 + + +class HttpResponseNotFound(HttpResponse): + status_code = 404 + + +class JsonResponse(HttpResponse): + + def __init__( + self, + data, + encoder=..., + safe=True, + json_dumps_params=None, + **kwargs + ): + # fake code to represent what is going on :) + kwargs.setdefault("content_type", "application/json") + data = str(data) + super().__init__(content=data, **kwargs) From fa08676a1db08a12ba1c97102268adaff1af8209 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 18 May 2020 17:35:31 +0200 Subject: [PATCH 02/13] Python: Proper redirect taint sinks for Django Also a major restructuring of the code. A bit controversial since it renames/moves classes that are already public. Fixes https://github.com/github/codeql/issues/3466 --- .../src/semmle/python/web/django/Redirect.qll | 27 +++++++++-- .../src/semmle/python/web/django/Response.qll | 39 +++++----------- .../src/semmle/python/web/django/Shared.qll | 45 +++++++++++++++---- .../web/django/HttpRedirectSinks.expected | 6 ++- .../web/django/HttpResponseSinks.expected | 2 - .../library-tests/web/django/views_2x_3x.py | 2 +- 6 files changed, 74 insertions(+), 47 deletions(-) diff --git a/python/ql/src/semmle/python/web/django/Redirect.qll b/python/ql/src/semmle/python/web/django/Redirect.qll index a550088eaf6..ca28a4a32d2 100644 --- a/python/ql/src/semmle/python/web/django/Redirect.qll +++ b/python/ql/src/semmle/python/web/django/Redirect.qll @@ -11,10 +11,29 @@ private import semmle.python.web.django.Shared private import semmle.python.web.Http /** - * Represents an argument to the `django.redirect` function. + * The URL argument for a call to the `django.shortcuts.redirect` function. */ -class DjangoRedirect extends HttpRedirectTaintSink { - override string toString() { result = "django.redirect" } +class DjangoShortcutsRedirectSink extends HttpRedirectTaintSink { + override string toString() { result = "DjangoShortcutsRedirectSink" } - DjangoRedirect() { this = redirect().getACall().getAnArg() } + DjangoShortcutsRedirectSink() { + this = Value::named("django.shortcuts.redirect").(FunctionValue).getArgumentForCall(_, 0) + } +} + +/** + * The URL argument when instantiating a Django Redirect Response. + */ +class DjangoRedirectResponseSink extends HttpRedirectTaintSink { + DjangoRedirectResponseSink() { + exists(CallNode call | + call = any(DjangoRedirectResponse rr).getACall() + | + this = call.getArg(0) + or + this = call.getArgByName("redirect_to") + ) + } + + override string toString() { result = "DjangoRedirectResponseSink" } } diff --git a/python/ql/src/semmle/python/web/django/Response.qll b/python/ql/src/semmle/python/web/django/Response.qll index dc6a3634440..c9e78130d36 100644 --- a/python/ql/src/semmle/python/web/django/Response.qll +++ b/python/ql/src/semmle/python/web/django/Response.qll @@ -4,38 +4,20 @@ import semmle.python.security.strings.Basic private import semmle.python.web.django.Shared private import semmle.python.web.Http -/** - * A django.http.response.Response object - * This isn't really a "taint", but we use the value tracking machinery to - * track the flow of response objects. - */ -class DjangoResponse extends TaintKind { - DjangoResponse() { this = "django.response.HttpResponse" } +/** INTERNAL class used for tracking a django response object. */ +private class DjangoResponseKind extends TaintKind { + DjangoResponseKind() { this = "django.response.HttpResponse" } } -private ClassValue theDjangoHttpResponseClass() { - ( - // version 1.x - result = Value::named("django.http.response.HttpResponse") - or - // version 2.x - // https://docs.djangoproject.com/en/2.2/ref/request-response/#httpresponse-objects - result = Value::named("django.http.HttpResponse") - ) and - // TODO: does this do anything? when could they be the same??? - not result = theDjangoHttpRedirectClass() -} - -/** internal class used for tracking a django response. */ +/** INTENRAL taint-source used for tracking a django response. */ private class DjangoResponseSource extends TaintSource { DjangoResponseSource() { - exists(ClassValue cls | - cls.getASuperType() = theDjangoHttpResponseClass() and + exists(DjangoXSSVulnResponse cls | cls.getACall() = this ) } - override predicate isSourceOf(TaintKind kind) { kind instanceof DjangoResponse } + override predicate isSourceOf(TaintKind kind) { kind instanceof DjangoResponseKind } override string toString() { result = "django.http.response.HttpResponse" } } @@ -45,7 +27,7 @@ class DjangoResponseWrite extends HttpResponseTaintSink { DjangoResponseWrite() { exists(AttrNode meth, CallNode call | call.getFunction() = meth and - any(DjangoResponse response).taints(meth.getObject("write")) and + any(DjangoResponseKind response).taints(meth.getObject("write")) and this = call.getArg(0) ) } @@ -58,9 +40,8 @@ class DjangoResponseWrite extends HttpResponseTaintSink { /** An argument to initialization of a django response, which is vulnerable to external data (xss) */ class DjangoResponseContent extends HttpResponseTaintSink { DjangoResponseContent() { - exists(CallNode call, ClassValue cls | - cls.getASuperType() = theDjangoHttpResponseClass() and - call.getFunction().pointsTo(cls) + exists(CallNode call, DjangoXSSVulnResponse cls | + call = cls.getACall() | call.getArg(0) = this or @@ -75,7 +56,7 @@ class DjangoResponseContent extends HttpResponseTaintSink { class DjangoCookieSet extends CookieSet, CallNode { DjangoCookieSet() { - any(DjangoResponse r).taints(this.getFunction().(AttrNode).getObject("set_cookie")) + any(DjangoResponseKind r).taints(this.getFunction().(AttrNode).getObject("set_cookie")) } override string toString() { result = CallNode.super.toString() } diff --git a/python/ql/src/semmle/python/web/django/Shared.qll b/python/ql/src/semmle/python/web/django/Shared.qll index 669dc5712df..ef3d353b2d2 100644 --- a/python/ql/src/semmle/python/web/django/Shared.qll +++ b/python/ql/src/semmle/python/web/django/Shared.qll @@ -1,12 +1,39 @@ import python -/** django.shortcuts.redirect */ -FunctionValue redirect() { result = Value::named("django.shortcuts.redirect") } - -ClassValue theDjangoHttpRedirectClass() { - // version 1.x - result = Value::named("django.http.response.HttpResponseRedirectBase") - or - // version 2.x - result = Value::named("django.http.HttpResponseRedirectBase") +/** A Class that is a Django Response (subclass of `django.http.HttpResponse`). */ +class DjangoResponse extends ClassValue { + DjangoResponse() { + exists(ClassValue base | + // version 1.x + base = Value::named("django.http.response.HttpResponse") + or + // version 2.x and 3.x + // https://docs.djangoproject.com/en/2.2/ref/request-response/#httpresponse-objects + base = Value::named("django.http.HttpResponse") + | + this.getASuperType() = base + ) + } +} + +/** A Class that is a Django Redirect Response (subclass of `django.http.HttpResponseRedirectBase`). */ +class DjangoRedirectResponse extends DjangoResponse { + DjangoRedirectResponse() { + exists(ClassValue base | + // version 1.x + base = Value::named("django.http.response.HttpResponseRedirectBase") + or + // version 2.x and 3.x + base = Value::named("django.http.HttpResponseRedirectBase") + | + this.getASuperType() = base + ) + } +} + +/** A Class that is a Django Response, and is vulnerable to XSS. */ +class DjangoXSSVulnResponse extends DjangoResponse { + DjangoXSSVulnResponse() { + not this instanceof DjangoRedirectResponse + } } diff --git a/python/ql/test/library-tests/web/django/HttpRedirectSinks.expected b/python/ql/test/library-tests/web/django/HttpRedirectSinks.expected index c47548dd3a5..1c9bcb0cfd5 100644 --- a/python/ql/test/library-tests/web/django/HttpRedirectSinks.expected +++ b/python/ql/test/library-tests/web/django/HttpRedirectSinks.expected @@ -1,2 +1,4 @@ -| test_1x.py:13:21:13:24 | django.redirect | externally controlled string | -| test_2x_3x.py:13:21:13:24 | django.redirect | externally controlled string | +| test_1x.py:13:21:13:24 | DjangoShortcutsRedirectSink | externally controlled string | +| test_2x_3x.py:13:21:13:24 | DjangoShortcutsRedirectSink | externally controlled string | +| views_1x.py:99:33:99:55 | DjangoRedirectResponseSink | externally controlled string | +| views_2x_3x.py:120:33:120:55 | DjangoRedirectResponseSink | externally controlled string | diff --git a/python/ql/test/library-tests/web/django/HttpResponseSinks.expected b/python/ql/test/library-tests/web/django/HttpResponseSinks.expected index 8ecde34d08d..c9c02bc9a12 100644 --- a/python/ql/test/library-tests/web/django/HttpResponseSinks.expected +++ b/python/ql/test/library-tests/web/django/HttpResponseSinks.expected @@ -11,7 +11,6 @@ | views_1x.py:85:25:85:55 | django.Response(...) | externally controlled string | | views_1x.py:90:25:90:33 | django.Response(...) | externally controlled string | | views_1x.py:94:25:94:58 | django.Response(...) | externally controlled string | -| views_1x.py:99:33:99:55 | django.Response(...) | externally controlled string | | views_1x.py:103:33:103:55 | django.Response(...) | externally controlled string | | views_2x_3x.py:8:25:8:63 | django.Response(...) | externally controlled string | | views_2x_3x.py:12:25:12:52 | django.Response(...) | externally controlled string | @@ -29,5 +28,4 @@ | views_2x_3x.py:106:25:106:55 | django.Response(...) | externally controlled string | | views_2x_3x.py:111:25:111:33 | django.Response(...) | externally controlled string | | views_2x_3x.py:115:25:115:58 | django.Response(...) | externally controlled string | -| views_2x_3x.py:120:33:120:55 | django.Response(...) | externally controlled string | | views_2x_3x.py:124:33:124:55 | django.Response(...) | externally controlled string | diff --git a/python/ql/test/library-tests/web/django/views_2x_3x.py b/python/ql/test/library-tests/web/django/views_2x_3x.py index ce4a565c7a0..72371b789e9 100644 --- a/python/ql/test/library-tests/web/django/views_2x_3x.py +++ b/python/ql/test/library-tests/web/django/views_2x_3x.py @@ -117,7 +117,7 @@ def fp_manual_content_type(reuqest): # XSS FP reported in https://github.com/github/codeql/issues/3466 # Note: This should be a open-redirect sink, but not a XSS sink. def fp_redirect(request): - return HttpResponseRedirect(request.GET.get("next")) # TODO + return HttpResponseRedirect(request.GET.get("next")) # Ensure that subclasses are still vuln to XSS def tp_not_found(request): From 37743109853175bb2587b4d88019c177666303d9 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 18 May 2020 19:13:50 +0200 Subject: [PATCH 03/13] Python: Reduce FPs in Django due to bad XSS taint-sinks Fixes https://github.com/github/codeql-python-team/issues/38 --- .../src/semmle/python/web/django/Response.qll | 19 +++++--- .../src/semmle/python/web/django/Shared.qll | 46 +++++++++++++++---- .../web/django/HttpResponseSinks.expected | 8 +--- .../test/library-tests/web/django/views_1x.py | 6 ++- .../library-tests/web/django/views_2x_3x.py | 12 +++-- .../Security/lib/django/http/response.py | 12 ++++- 6 files changed, 73 insertions(+), 30 deletions(-) diff --git a/python/ql/src/semmle/python/web/django/Response.qll b/python/ql/src/semmle/python/web/django/Response.qll index c9e78130d36..b2b2f2e162d 100644 --- a/python/ql/src/semmle/python/web/django/Response.qll +++ b/python/ql/src/semmle/python/web/django/Response.qll @@ -12,7 +12,7 @@ private class DjangoResponseKind extends TaintKind { /** INTENRAL taint-source used for tracking a django response. */ private class DjangoResponseSource extends TaintSource { DjangoResponseSource() { - exists(DjangoXSSVulnResponse cls | + exists(DjangoXSSVulnerableResponse cls | cls.getACall() = this ) } @@ -40,12 +40,17 @@ class DjangoResponseWrite extends HttpResponseTaintSink { /** An argument to initialization of a django response, which is vulnerable to external data (xss) */ class DjangoResponseContent extends HttpResponseTaintSink { DjangoResponseContent() { - exists(CallNode call, DjangoXSSVulnResponse cls | - call = cls.getACall() - | - call.getArg(0) = this - or - call.getArgByName("content") = this + exists(CallNode call, DjangoXSSVulnerableResponse cls | + call = cls.getACall() and + this = cls.getContentArg(call) and + ( + not exists(cls.getContentTypeArg(call)) + or + exists(StringValue s | + cls.getContentTypeArg(call).pointsTo(s) and + s.getText().indexOf("text/html") = 0 + ) + ) ) } diff --git a/python/ql/src/semmle/python/web/django/Shared.qll b/python/ql/src/semmle/python/web/django/Shared.qll index ef3d353b2d2..b4402723273 100644 --- a/python/ql/src/semmle/python/web/django/Shared.qll +++ b/python/ql/src/semmle/python/web/django/Shared.qll @@ -2,38 +2,64 @@ import python /** A Class that is a Django Response (subclass of `django.http.HttpResponse`). */ class DjangoResponse extends ClassValue { + ClassValue base; + DjangoResponse() { - exists(ClassValue base | + ( // version 1.x base = Value::named("django.http.response.HttpResponse") or // version 2.x and 3.x // https://docs.djangoproject.com/en/2.2/ref/request-response/#httpresponse-objects base = Value::named("django.http.HttpResponse") - | - this.getASuperType() = base - ) + ) and + this.getASuperType() = base } } /** A Class that is a Django Redirect Response (subclass of `django.http.HttpResponseRedirectBase`). */ class DjangoRedirectResponse extends DjangoResponse { DjangoRedirectResponse() { - exists(ClassValue base | + exists(ClassValue redirect_base | // version 1.x - base = Value::named("django.http.response.HttpResponseRedirectBase") + redirect_base = Value::named("django.http.response.HttpResponseRedirectBase") or // version 2.x and 3.x - base = Value::named("django.http.HttpResponseRedirectBase") + redirect_base = Value::named("django.http.HttpResponseRedirectBase") | - this.getASuperType() = base + this.getASuperType() = redirect_base ) } } /** A Class that is a Django Response, and is vulnerable to XSS. */ -class DjangoXSSVulnResponse extends DjangoResponse { - DjangoXSSVulnResponse() { +class DjangoXSSVulnerableResponse extends DjangoResponse { + DjangoXSSVulnerableResponse() { + // We want to avoid FPs on subclasses that are not exposed to XSS, for example `JsonResponse`. + // The easiest way is to disregard any subclass that has a special `__init__` method. + // It's not guaranteed to remove all FPs, or not to generate FNs, but compared to our + // previous implementation that would treat 0-th argument to _any_ subclass as a sink, + // this gets us much closer to reality. + this.lookup("__init__") = base.lookup("__init__") and not this instanceof DjangoRedirectResponse } + + // The reason these two method are defined in this class (and no in the Sink + // definition that uses this class), is that if we were to add support for `HttpResponseNotAllowed` + // it would make much more sense to add the custom logic in this class (or subclass), than to handle all of it + // in the sink definition. + + /** Gets the `content` argument of a `call` to the constructor */ + ControlFlowNode getContentArg(CallNode call) { + result = call.getArg(0) + or + result = call.getArgByName("content") + } + + /** Gets the `content_type` argument of a `call` to the constructor */ + ControlFlowNode getContentTypeArg(CallNode call) { + result = call.getArg(1) + or + result = call.getArgByName("content_type") + } } diff --git a/python/ql/test/library-tests/web/django/HttpResponseSinks.expected b/python/ql/test/library-tests/web/django/HttpResponseSinks.expected index c9c02bc9a12..7c9e583095f 100644 --- a/python/ql/test/library-tests/web/django/HttpResponseSinks.expected +++ b/python/ql/test/library-tests/web/django/HttpResponseSinks.expected @@ -8,10 +8,8 @@ | views_1x.py:45:25:45:70 | django.Response(...) | externally controlled string | | views_1x.py:66:25:66:55 | django.Response(...) | externally controlled string | | views_1x.py:75:25:75:33 | django.Response(...) | externally controlled string | -| views_1x.py:85:25:85:55 | django.Response(...) | externally controlled string | -| views_1x.py:90:25:90:33 | django.Response(...) | externally controlled string | -| views_1x.py:94:25:94:58 | django.Response(...) | externally controlled string | | views_1x.py:103:33:103:55 | django.Response(...) | externally controlled string | +| views_1x.py:107:25:107:47 | django.Response(...) | externally controlled string | | views_2x_3x.py:8:25:8:63 | django.Response(...) | externally controlled string | | views_2x_3x.py:12:25:12:52 | django.Response(...) | externally controlled string | | views_2x_3x.py:16:25:16:53 | django.Response(...) | externally controlled string | @@ -25,7 +23,5 @@ | views_2x_3x.py:82:25:82:69 | django.Response(...) | externally controlled string | | views_2x_3x.py:85:25:85:64 | django.Response(...) | externally controlled string | | views_2x_3x.py:88:25:88:32 | django.Response(...) | externally controlled string | -| views_2x_3x.py:106:25:106:55 | django.Response(...) | externally controlled string | -| views_2x_3x.py:111:25:111:33 | django.Response(...) | externally controlled string | -| views_2x_3x.py:115:25:115:58 | django.Response(...) | externally controlled string | | views_2x_3x.py:124:33:124:55 | django.Response(...) | externally controlled string | +| views_2x_3x.py:128:25:128:47 | django.Response(...) | externally controlled string | diff --git a/python/ql/test/library-tests/web/django/views_1x.py b/python/ql/test/library-tests/web/django/views_1x.py index abc3b716a2f..f5476a13cef 100644 --- a/python/ql/test/library-tests/web/django/views_1x.py +++ b/python/ql/test/library-tests/web/django/views_1x.py @@ -98,6 +98,10 @@ def fp_manual_content_type(reuqest): def fp_redirect(request): return HttpResponseRedirect(request.GET.get("next")) -# Ensure that subclasses are still vuln to XSS +# Ensure that simple subclasses are still vuln to XSS def tp_not_found(request): return HttpResponseNotFound(request.GET.get("name")) + +# Ensure we still have a XSS sink when manually setting the content_type to HTML +def tp_manual_response_type(request): + return HttpResponse(request.GET.get("name"), content_type="text/html; charset=utf-8") diff --git a/python/ql/test/library-tests/web/django/views_2x_3x.py b/python/ql/test/library-tests/web/django/views_2x_3x.py index 72371b789e9..d26b7915ef8 100644 --- a/python/ql/test/library-tests/web/django/views_2x_3x.py +++ b/python/ql/test/library-tests/web/django/views_2x_3x.py @@ -103,22 +103,26 @@ urlpatterns = [ # FP reported in https://github.com/github/codeql-python-team/issues/38 def fp_json_response(request): # implicitly sets Content-Type to "application/json" - return JsonResponse({"foo": request.GET.get("foo")}) # TODO + return JsonResponse({"foo": request.GET.get("foo")}) # Not an XSS sink, since the Content-Type is not "text/html" def fp_manual_json_response(request): json_data = '{"json": "{}"}'.format(request.GET.get("foo")) - return HttpResponse(json_data, content_type="application/json") # TODO + return HttpResponse(json_data, content_type="application/json") # Not an XSS sink, since the Content-Type is not "text/html" def fp_manual_content_type(reuqest): - return HttpResponse('', content_type="text/plain") # TODO + return HttpResponse('', content_type="text/plain") # XSS FP reported in https://github.com/github/codeql/issues/3466 # Note: This should be a open-redirect sink, but not a XSS sink. def fp_redirect(request): return HttpResponseRedirect(request.GET.get("next")) -# Ensure that subclasses are still vuln to XSS +# Ensure that simple subclasses are still vuln to XSS def tp_not_found(request): return HttpResponseNotFound(request.GET.get("name")) + +# Ensure we still have a XSS sink when manually setting the content_type to HTML +def tp_manual_response_type(request): + return HttpResponse(request.GET.get("name"), content_type="text/html; charset=utf-8") diff --git a/python/ql/test/query-tests/Security/lib/django/http/response.py b/python/ql/test/query-tests/Security/lib/django/http/response.py index 96cc2bda9a9..1110a3cde19 100644 --- a/python/ql/test/query-tests/Security/lib/django/http/response.py +++ b/python/ql/test/query-tests/Security/lib/django/http/response.py @@ -1,13 +1,21 @@ class HttpResponseBase(object): status_code = 200 + def __init__(self, content_type=None, status=None, reason=None, charset=None): + pass + class HttpResponse(HttpResponseBase): - pass + def __init__(self, content=b"", *args, **kwargs): + super().__init__(*args, **kwargs) + # Content is a bytestring. See the `content` property methods. + self.content = content class HttpResponseRedirectBase(HttpResponse): - pass + def __init__(self, redirect_to, *args, **kwargs): + super().__init__(*args, **kwargs) + ... class HttpResponsePermanentRedirect(HttpResponseRedirectBase): From 6cba2fe4f8d4ba674b5fbb44def20c7e5d97b920 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 26 May 2020 16:45:46 +0200 Subject: [PATCH 04/13] Python: Model Django response sinks that are not vuln to XSS Since HttpResponse is not *only* used for XSS, it is still valuable to know the content is send as part of the response. The *proper* solution to this problem of not all HttpResponses being vulnerable to XSS is probably to define a new abstract class in Http.qll called HttpResponseXSSVulnerableSink (or similar). I would like to model a few more libraries/frameworks before fully comitting to an approach though. --- .../ql/src/Security/CWE-079/ReflectedXss.ql | 7 +- .../src/semmle/python/web/django/Redirect.qll | 2 +- .../src/semmle/python/web/django/Response.qll | 39 ++++++---- .../src/semmle/python/web/django/Shared.qll | 72 ++++++++++--------- .../web/django/HttpResponseSinks.expected | 4 ++ 5 files changed, 76 insertions(+), 48 deletions(-) diff --git a/python/ql/src/Security/CWE-079/ReflectedXss.ql b/python/ql/src/Security/CWE-079/ReflectedXss.ql index dbc02671603..cea41442c5b 100644 --- a/python/ql/src/Security/CWE-079/ReflectedXss.ql +++ b/python/ql/src/Security/CWE-079/ReflectedXss.ql @@ -28,7 +28,12 @@ class ReflectedXssConfiguration extends TaintTracking::Configuration { source instanceof HttpRequestTaintSource } - override predicate isSink(TaintTracking::Sink sink) { sink instanceof HttpResponseTaintSink } + override predicate isSink(TaintTracking::Sink sink) { + sink instanceof HttpResponseTaintSink and + not sink instanceof DjangoResponseContent + or + sink instanceof DjangoResponseContentXSSVulnerable + } } from ReflectedXssConfiguration config, TaintedPathSource src, TaintedPathSink sink diff --git a/python/ql/src/semmle/python/web/django/Redirect.qll b/python/ql/src/semmle/python/web/django/Redirect.qll index ca28a4a32d2..61ee041f904 100644 --- a/python/ql/src/semmle/python/web/django/Redirect.qll +++ b/python/ql/src/semmle/python/web/django/Redirect.qll @@ -27,7 +27,7 @@ class DjangoShortcutsRedirectSink extends HttpRedirectTaintSink { class DjangoRedirectResponseSink extends HttpRedirectTaintSink { DjangoRedirectResponseSink() { exists(CallNode call | - call = any(DjangoRedirectResponse rr).getACall() + call = any(DjangoRedirectResponseClass cls).getACall() | this = call.getArg(0) or diff --git a/python/ql/src/semmle/python/web/django/Response.qll b/python/ql/src/semmle/python/web/django/Response.qll index b2b2f2e162d..cd3b8dc5085 100644 --- a/python/ql/src/semmle/python/web/django/Response.qll +++ b/python/ql/src/semmle/python/web/django/Response.qll @@ -12,7 +12,7 @@ private class DjangoResponseKind extends TaintKind { /** INTENRAL taint-source used for tracking a django response. */ private class DjangoResponseSource extends TaintSource { DjangoResponseSource() { - exists(DjangoXSSVulnerableResponse cls | + exists(DjangoContentResponseClass cls | cls.getACall() = this ) } @@ -37,21 +37,16 @@ class DjangoResponseWrite extends HttpResponseTaintSink { override string toString() { result = "django.Response.write(...)" } } -/** An argument to initialization of a django response, which is vulnerable to external data (xss) */ +/** + * An argument to initialization of a django response. + */ class DjangoResponseContent extends HttpResponseTaintSink { + DjangoContentResponseClass cls; + CallNode call; + DjangoResponseContent() { - exists(CallNode call, DjangoXSSVulnerableResponse cls | - call = cls.getACall() and - this = cls.getContentArg(call) and - ( - not exists(cls.getContentTypeArg(call)) - or - exists(StringValue s | - cls.getContentTypeArg(call).pointsTo(s) and - s.getText().indexOf("text/html") = 0 - ) - ) - ) + call = cls.getACall() and + this = cls.getContentArg(call) } override predicate sinks(TaintKind kind) { kind instanceof StringKind } @@ -59,6 +54,22 @@ class DjangoResponseContent extends HttpResponseTaintSink { override string toString() { result = "django.Response(...)" } } +/** + * An argument to initialization of a django response, which is vulnerable to external data (XSS). + */ +class DjangoResponseContentXSSVulnerable extends DjangoResponseContent { + override DjangoXSSVulnerableResponseClass cls; + + DjangoResponseContentXSSVulnerable() { + not exists(cls.getContentTypeArg(call)) + or + exists(StringValue s | + cls.getContentTypeArg(call).pointsTo(s) and + s.getText().indexOf("text/html") = 0 + ) + } +} + class DjangoCookieSet extends CookieSet, CallNode { DjangoCookieSet() { any(DjangoResponseKind r).taints(this.getFunction().(AttrNode).getObject("set_cookie")) diff --git a/python/ql/src/semmle/python/web/django/Shared.qll b/python/ql/src/semmle/python/web/django/Shared.qll index b4402723273..3413cceb4ab 100644 --- a/python/ql/src/semmle/python/web/django/Shared.qll +++ b/python/ql/src/semmle/python/web/django/Shared.qll @@ -1,25 +1,8 @@ import python -/** A Class that is a Django Response (subclass of `django.http.HttpResponse`). */ -class DjangoResponse extends ClassValue { - ClassValue base; - - DjangoResponse() { - ( - // version 1.x - base = Value::named("django.http.response.HttpResponse") - or - // version 2.x and 3.x - // https://docs.djangoproject.com/en/2.2/ref/request-response/#httpresponse-objects - base = Value::named("django.http.HttpResponse") - ) and - this.getASuperType() = base - } -} - -/** A Class that is a Django Redirect Response (subclass of `django.http.HttpResponseRedirectBase`). */ -class DjangoRedirectResponse extends DjangoResponse { - DjangoRedirectResponse() { +/** A class that is a Django Redirect Response (subclass of `django.http.HttpResponseRedirectBase`). */ +class DjangoRedirectResponseClass extends ClassValue { + DjangoRedirectResponseClass() { exists(ClassValue redirect_base | // version 1.x redirect_base = Value::named("django.http.response.HttpResponseRedirectBase") @@ -32,32 +15,57 @@ class DjangoRedirectResponse extends DjangoResponse { } } +/** + * A class that is a Django Response, which can contain content. + * A subclass of `django.http.HttpResponse` that is not a `DjangoRedirectResponseClass`. + */ +class DjangoContentResponseClass extends ClassValue { + ClassValue base; + + DjangoContentResponseClass() { + ( + // version 1.x + base = Value::named("django.http.response.HttpResponse") + or + // version 2.x and 3.x + // https://docs.djangoproject.com/en/2.2/ref/request-response/#httpresponse-objects + base = Value::named("django.http.HttpResponse") + ) and + this.getASuperType() = base + } + + // The reason these two method are defined in this class (and not in the Sink + // definition that uses this class), is that if we were to add support for + // `django.http.response.HttpResponseNotAllowed` it would make much more sense to add + // the custom logic in this class (or subclass), than to handle all of it in the sink + // definition. + + /** Gets the `content` argument of a `call` to the constructor */ + ControlFlowNode getContentArg(CallNode call) { none() } + + /** Gets the `content_type` argument of a `call` to the constructor */ + ControlFlowNode getContentTypeArg(CallNode call) { none() } +} + /** A Class that is a Django Response, and is vulnerable to XSS. */ -class DjangoXSSVulnerableResponse extends DjangoResponse { - DjangoXSSVulnerableResponse() { +class DjangoXSSVulnerableResponseClass extends DjangoContentResponseClass{ + DjangoXSSVulnerableResponseClass() { // We want to avoid FPs on subclasses that are not exposed to XSS, for example `JsonResponse`. // The easiest way is to disregard any subclass that has a special `__init__` method. // It's not guaranteed to remove all FPs, or not to generate FNs, but compared to our // previous implementation that would treat 0-th argument to _any_ subclass as a sink, // this gets us much closer to reality. this.lookup("__init__") = base.lookup("__init__") and - not this instanceof DjangoRedirectResponse + not this instanceof DjangoRedirectResponseClass } - // The reason these two method are defined in this class (and no in the Sink - // definition that uses this class), is that if we were to add support for `HttpResponseNotAllowed` - // it would make much more sense to add the custom logic in this class (or subclass), than to handle all of it - // in the sink definition. - - /** Gets the `content` argument of a `call` to the constructor */ - ControlFlowNode getContentArg(CallNode call) { + override ControlFlowNode getContentArg(CallNode call) { result = call.getArg(0) or result = call.getArgByName("content") } - /** Gets the `content_type` argument of a `call` to the constructor */ - ControlFlowNode getContentTypeArg(CallNode call) { + override ControlFlowNode getContentTypeArg(CallNode call) { result = call.getArg(1) or result = call.getArgByName("content_type") diff --git a/python/ql/test/library-tests/web/django/HttpResponseSinks.expected b/python/ql/test/library-tests/web/django/HttpResponseSinks.expected index 7c9e583095f..e4e52c97514 100644 --- a/python/ql/test/library-tests/web/django/HttpResponseSinks.expected +++ b/python/ql/test/library-tests/web/django/HttpResponseSinks.expected @@ -8,6 +8,8 @@ | views_1x.py:45:25:45:70 | django.Response(...) | externally controlled string | | views_1x.py:66:25:66:55 | django.Response(...) | externally controlled string | | views_1x.py:75:25:75:33 | django.Response(...) | externally controlled string | +| views_1x.py:90:25:90:33 | django.Response(...) | externally controlled string | +| views_1x.py:94:25:94:58 | django.Response(...) | externally controlled string | | views_1x.py:103:33:103:55 | django.Response(...) | externally controlled string | | views_1x.py:107:25:107:47 | django.Response(...) | externally controlled string | | views_2x_3x.py:8:25:8:63 | django.Response(...) | externally controlled string | @@ -23,5 +25,7 @@ | views_2x_3x.py:82:25:82:69 | django.Response(...) | externally controlled string | | views_2x_3x.py:85:25:85:64 | django.Response(...) | externally controlled string | | views_2x_3x.py:88:25:88:32 | django.Response(...) | externally controlled string | +| views_2x_3x.py:111:25:111:33 | django.Response(...) | externally controlled string | +| views_2x_3x.py:115:25:115:58 | django.Response(...) | externally controlled string | | views_2x_3x.py:124:33:124:55 | django.Response(...) | externally controlled string | | views_2x_3x.py:128:25:128:47 | django.Response(...) | externally controlled string | From daa1b6fc790e5759385917404b377e4c33f1d0c6 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 22 Jun 2020 13:41:03 +0200 Subject: [PATCH 05/13] Python: Fix grammar in QLDoc Co-authored-by: Taus --- python/ql/src/semmle/python/web/django/Response.qll | 2 +- python/ql/src/semmle/python/web/django/Shared.qll | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/python/ql/src/semmle/python/web/django/Response.qll b/python/ql/src/semmle/python/web/django/Response.qll index cd3b8dc5085..a0e07ea4b21 100644 --- a/python/ql/src/semmle/python/web/django/Response.qll +++ b/python/ql/src/semmle/python/web/django/Response.qll @@ -9,7 +9,7 @@ private class DjangoResponseKind extends TaintKind { DjangoResponseKind() { this = "django.response.HttpResponse" } } -/** INTENRAL taint-source used for tracking a django response. */ +/** INTERNAL taint-source used for tracking a django response object. */ private class DjangoResponseSource extends TaintSource { DjangoResponseSource() { exists(DjangoContentResponseClass cls | diff --git a/python/ql/src/semmle/python/web/django/Shared.qll b/python/ql/src/semmle/python/web/django/Shared.qll index 3413cceb4ab..f66acc7fe2d 100644 --- a/python/ql/src/semmle/python/web/django/Shared.qll +++ b/python/ql/src/semmle/python/web/django/Shared.qll @@ -34,7 +34,7 @@ class DjangoContentResponseClass extends ClassValue { this.getASuperType() = base } - // The reason these two method are defined in this class (and not in the Sink + // The reason these two methods are defined in this class (and not in the Sink // definition that uses this class), is that if we were to add support for // `django.http.response.HttpResponseNotAllowed` it would make much more sense to add // the custom logic in this class (or subclass), than to handle all of it in the sink @@ -47,7 +47,7 @@ class DjangoContentResponseClass extends ClassValue { ControlFlowNode getContentTypeArg(CallNode call) { none() } } -/** A Class that is a Django Response, and is vulnerable to XSS. */ +/** A class that is a Django Response, and is vulnerable to XSS. */ class DjangoXSSVulnerableResponseClass extends DjangoContentResponseClass{ DjangoXSSVulnerableResponseClass() { // We want to avoid FPs on subclasses that are not exposed to XSS, for example `JsonResponse`. From 19db418395bf2ead576c527482a47803543d9cda Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Sun, 28 Jun 2020 01:26:11 +0100 Subject: [PATCH 06/13] JS: Add missing store step in Xss query --- .../javascript/security/dataflow/DomBasedXss.qll | 11 +++++++++++ .../test/query-tests/Security/CWE-079/Xss.expected | 12 ++++++++++++ .../ql/test/query-tests/Security/CWE-079/tst.js | 4 ++++ 3 files changed, 27 insertions(+) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll b/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll index 65ab201a720..97e55ca98a2 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll @@ -28,6 +28,17 @@ module DomBasedXss { guard instanceof SanitizerGuard } + override predicate isAdditionalStoreStep( + DataFlow::Node pred, DataFlow::SourceNode succ, string prop + ) { + exists(DataFlow::PropRead read | + pred = read.getBase() and + succ = read and + read.getPropertyName() = "hash" and + prop = urlSuffixPseudoProperty() + ) + } + override predicate isAdditionalLoadStoreStep( DataFlow::Node pred, DataFlow::Node succ, string predProp, string succProp ) { diff --git a/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected b/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected index 7882f28b8b9..9aed8f79822 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected @@ -453,6 +453,12 @@ nodes | tst.js:414:19:414:31 | target.taint8 | | tst.js:415:18:415:30 | target.taint8 | | tst.js:415:18:415:30 | target.taint8 | +| tst.js:422:7:422:46 | payload | +| tst.js:422:17:422:31 | window.location | +| tst.js:422:17:422:31 | window.location | +| tst.js:422:17:422:46 | window. ... bstr(1) | +| tst.js:423:18:423:24 | payload | +| tst.js:423:18:423:24 | payload | | typeahead.js:20:13:20:45 | target | | typeahead.js:20:22:20:38 | document.location | | typeahead.js:20:22:20:38 | document.location | @@ -882,6 +888,11 @@ edges | tst.js:414:19:414:31 | target.taint8 | tst.js:414:19:414:31 | target.taint8 | | tst.js:414:19:414:31 | target.taint8 | tst.js:415:18:415:30 | target.taint8 | | tst.js:414:19:414:31 | target.taint8 | tst.js:415:18:415:30 | target.taint8 | +| tst.js:422:7:422:46 | payload | tst.js:423:18:423:24 | payload | +| tst.js:422:7:422:46 | payload | tst.js:423:18:423:24 | payload | +| tst.js:422:17:422:31 | window.location | tst.js:422:17:422:46 | window. ... bstr(1) | +| tst.js:422:17:422:31 | window.location | tst.js:422:17:422:46 | window. ... bstr(1) | +| tst.js:422:17:422:46 | window. ... bstr(1) | tst.js:422:7:422:46 | payload | | typeahead.js:20:13:20:45 | target | typeahead.js:21:12:21:17 | target | | typeahead.js:20:22:20:38 | document.location | typeahead.js:20:22:20:45 | documen ... .search | | typeahead.js:20:22:20:38 | document.location | typeahead.js:20:22:20:45 | documen ... .search | @@ -1009,6 +1020,7 @@ edges | tst.js:403:18:403:30 | target.taint5 | tst.js:387:16:387:32 | document.location | tst.js:403:18:403:30 | target.taint5 | Cross-site scripting vulnerability due to $@. | tst.js:387:16:387:32 | document.location | user-provided value | | tst.js:412:18:412:30 | target.taint7 | tst.js:387:16:387:32 | document.location | tst.js:412:18:412:30 | target.taint7 | Cross-site scripting vulnerability due to $@. | tst.js:387:16:387:32 | document.location | user-provided value | | tst.js:415:18:415:30 | target.taint8 | tst.js:387:16:387:32 | document.location | tst.js:415:18:415:30 | target.taint8 | Cross-site scripting vulnerability due to $@. | tst.js:387:16:387:32 | document.location | user-provided value | +| tst.js:423:18:423:24 | payload | tst.js:422:17:422:31 | window.location | tst.js:423:18:423:24 | payload | Cross-site scripting vulnerability due to $@. | tst.js:422:17:422:31 | window.location | user-provided value | | typeahead.js:25:18:25:20 | val | typeahead.js:20:22:20:38 | document.location | typeahead.js:25:18:25:20 | val | Cross-site scripting vulnerability due to $@. | typeahead.js:20:22:20:38 | document.location | user-provided value | | v-html.vue:2:8:2:23 | v-html=tainted | v-html.vue:6:42:6:58 | document.location | v-html.vue:2:8:2:23 | v-html=tainted | Cross-site scripting vulnerability due to $@. | v-html.vue:6:42:6:58 | document.location | user-provided value | | winjs.js:3:43:3:49 | tainted | winjs.js:2:17:2:33 | document.location | winjs.js:3:43:3:49 | tainted | Cross-site scripting vulnerability due to $@. | winjs.js:2:17:2:33 | document.location | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/tst.js b/javascript/ql/test/query-tests/Security/CWE-079/tst.js index 3f06a850d9e..30f495739a4 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-079/tst.js @@ -418,3 +418,7 @@ function test() { $('myId').html(target.taint9); // OK } +function hash2() { + var payload = window.location.hash.substr(1); + document.write(payload); // NOT OK +} From 9ca25d5bef94f322f13344323888a8b9daa557e2 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Sun, 28 Jun 2020 01:38:59 +0100 Subject: [PATCH 07/13] JS: Support .hash extraction via a few more methods --- .../security/dataflow/DomBasedXss.qll | 22 ++++++++++++++---- .../query-tests/Security/CWE-079/Xss.expected | 23 +++++++++++++++++++ .../test/query-tests/Security/CWE-079/tst.js | 7 ++++++ 3 files changed, 48 insertions(+), 4 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll b/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll index 97e55ca98a2..f332633dfbc 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll @@ -52,15 +52,29 @@ module DomBasedXss { } override predicate isAdditionalLoadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { - exists(DataFlow::MethodCallNode call, string name | - name = "substr" or name = "substring" or name = "slice" - | - call.getMethodName() = name and + exists(DataFlow::MethodCallNode call | + call.getMethodName() = ["substr", "substring", "slice"] and not call.getArgument(0).getIntValue() = 0 and pred = call.getReceiver() and succ = call and prop = urlSuffixPseudoProperty() ) + or + exists(DataFlow::MethodCallNode call | + call.getMethodName() = "exec" and pred = call.getArgument(0) + or + call.getMethodName() = "match" and pred = call.getReceiver() + | + succ = call and + prop = urlSuffixPseudoProperty() + ) + or + exists(StringSplitCall split | + split.getSeparator() = ["#", "?"] and + pred = split.getBaseString() and + succ = split.getASubstringRead(1) and + prop = urlSuffixPseudoProperty() + ) } override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) { diff --git a/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected b/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected index 9aed8f79822..6624e327f0d 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected @@ -459,6 +459,17 @@ nodes | tst.js:422:17:422:46 | window. ... bstr(1) | | tst.js:423:18:423:24 | payload | | tst.js:423:18:423:24 | payload | +| tst.js:425:7:425:55 | match | +| tst.js:425:15:425:29 | window.location | +| tst.js:425:15:425:29 | window.location | +| tst.js:425:15:425:55 | window. ... (\\w+)/) | +| tst.js:427:20:427:24 | match | +| tst.js:427:20:427:27 | match[1] | +| tst.js:427:20:427:27 | match[1] | +| tst.js:430:18:430:32 | window.location | +| tst.js:430:18:430:32 | window.location | +| tst.js:430:18:430:51 | window. ... '#')[1] | +| tst.js:430:18:430:51 | window. ... '#')[1] | | typeahead.js:20:13:20:45 | target | | typeahead.js:20:22:20:38 | document.location | | typeahead.js:20:22:20:38 | document.location | @@ -893,6 +904,16 @@ edges | tst.js:422:17:422:31 | window.location | tst.js:422:17:422:46 | window. ... bstr(1) | | tst.js:422:17:422:31 | window.location | tst.js:422:17:422:46 | window. ... bstr(1) | | tst.js:422:17:422:46 | window. ... bstr(1) | tst.js:422:7:422:46 | payload | +| tst.js:425:7:425:55 | match | tst.js:427:20:427:24 | match | +| tst.js:425:15:425:29 | window.location | tst.js:425:15:425:55 | window. ... (\\w+)/) | +| tst.js:425:15:425:29 | window.location | tst.js:425:15:425:55 | window. ... (\\w+)/) | +| tst.js:425:15:425:55 | window. ... (\\w+)/) | tst.js:425:7:425:55 | match | +| tst.js:427:20:427:24 | match | tst.js:427:20:427:27 | match[1] | +| tst.js:427:20:427:24 | match | tst.js:427:20:427:27 | match[1] | +| tst.js:430:18:430:32 | window.location | tst.js:430:18:430:51 | window. ... '#')[1] | +| tst.js:430:18:430:32 | window.location | tst.js:430:18:430:51 | window. ... '#')[1] | +| tst.js:430:18:430:32 | window.location | tst.js:430:18:430:51 | window. ... '#')[1] | +| tst.js:430:18:430:32 | window.location | tst.js:430:18:430:51 | window. ... '#')[1] | | typeahead.js:20:13:20:45 | target | typeahead.js:21:12:21:17 | target | | typeahead.js:20:22:20:38 | document.location | typeahead.js:20:22:20:45 | documen ... .search | | typeahead.js:20:22:20:38 | document.location | typeahead.js:20:22:20:45 | documen ... .search | @@ -1021,6 +1042,8 @@ edges | tst.js:412:18:412:30 | target.taint7 | tst.js:387:16:387:32 | document.location | tst.js:412:18:412:30 | target.taint7 | Cross-site scripting vulnerability due to $@. | tst.js:387:16:387:32 | document.location | user-provided value | | tst.js:415:18:415:30 | target.taint8 | tst.js:387:16:387:32 | document.location | tst.js:415:18:415:30 | target.taint8 | Cross-site scripting vulnerability due to $@. | tst.js:387:16:387:32 | document.location | user-provided value | | tst.js:423:18:423:24 | payload | tst.js:422:17:422:31 | window.location | tst.js:423:18:423:24 | payload | Cross-site scripting vulnerability due to $@. | tst.js:422:17:422:31 | window.location | user-provided value | +| tst.js:427:20:427:27 | match[1] | tst.js:425:15:425:29 | window.location | tst.js:427:20:427:27 | match[1] | Cross-site scripting vulnerability due to $@. | tst.js:425:15:425:29 | window.location | user-provided value | +| tst.js:430:18:430:51 | window. ... '#')[1] | tst.js:430:18:430:32 | window.location | tst.js:430:18:430:51 | window. ... '#')[1] | Cross-site scripting vulnerability due to $@. | tst.js:430:18:430:32 | window.location | user-provided value | | typeahead.js:25:18:25:20 | val | typeahead.js:20:22:20:38 | document.location | typeahead.js:25:18:25:20 | val | Cross-site scripting vulnerability due to $@. | typeahead.js:20:22:20:38 | document.location | user-provided value | | v-html.vue:2:8:2:23 | v-html=tainted | v-html.vue:6:42:6:58 | document.location | v-html.vue:2:8:2:23 | v-html=tainted | Cross-site scripting vulnerability due to $@. | v-html.vue:6:42:6:58 | document.location | user-provided value | | winjs.js:3:43:3:49 | tainted | winjs.js:2:17:2:33 | document.location | winjs.js:3:43:3:49 | tainted | Cross-site scripting vulnerability due to $@. | winjs.js:2:17:2:33 | document.location | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/tst.js b/javascript/ql/test/query-tests/Security/CWE-079/tst.js index 30f495739a4..333bcb46e8a 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-079/tst.js @@ -421,4 +421,11 @@ function test() { function hash2() { var payload = window.location.hash.substr(1); document.write(payload); // NOT OK + + let match = window.location.hash.match(/hello (\w+)/); + if (match) { + document.write(match[1]); // NOT OK + } + + document.write(window.location.hash.split('#')[1]); // NOT OK } From 03c91a66c59a6c0da752a1be6e58ffe3dafec913 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Mon, 29 Jun 2020 07:52:25 +0100 Subject: [PATCH 08/13] JS: Update expected output --- .../CWE-079/XssWithAdditionalSources.expected | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/javascript/ql/test/query-tests/Security/CWE-079/XssWithAdditionalSources.expected b/javascript/ql/test/query-tests/Security/CWE-079/XssWithAdditionalSources.expected index 0240433e74a..550d36b9ac2 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/XssWithAdditionalSources.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/XssWithAdditionalSources.expected @@ -453,6 +453,23 @@ nodes | tst.js:414:19:414:31 | target.taint8 | | tst.js:415:18:415:30 | target.taint8 | | tst.js:415:18:415:30 | target.taint8 | +| tst.js:422:7:422:46 | payload | +| tst.js:422:17:422:31 | window.location | +| tst.js:422:17:422:31 | window.location | +| tst.js:422:17:422:46 | window. ... bstr(1) | +| tst.js:423:18:423:24 | payload | +| tst.js:423:18:423:24 | payload | +| tst.js:425:7:425:55 | match | +| tst.js:425:15:425:29 | window.location | +| tst.js:425:15:425:29 | window.location | +| tst.js:425:15:425:55 | window. ... (\\w+)/) | +| tst.js:427:20:427:24 | match | +| tst.js:427:20:427:27 | match[1] | +| tst.js:427:20:427:27 | match[1] | +| tst.js:430:18:430:32 | window.location | +| tst.js:430:18:430:32 | window.location | +| tst.js:430:18:430:51 | window. ... '#')[1] | +| tst.js:430:18:430:51 | window. ... '#')[1] | | typeahead.js:9:28:9:30 | loc | | typeahead.js:9:28:9:30 | loc | | typeahead.js:10:16:10:18 | loc | @@ -886,6 +903,21 @@ edges | tst.js:414:19:414:31 | target.taint8 | tst.js:414:19:414:31 | target.taint8 | | tst.js:414:19:414:31 | target.taint8 | tst.js:415:18:415:30 | target.taint8 | | tst.js:414:19:414:31 | target.taint8 | tst.js:415:18:415:30 | target.taint8 | +| tst.js:422:7:422:46 | payload | tst.js:423:18:423:24 | payload | +| tst.js:422:7:422:46 | payload | tst.js:423:18:423:24 | payload | +| tst.js:422:17:422:31 | window.location | tst.js:422:17:422:46 | window. ... bstr(1) | +| tst.js:422:17:422:31 | window.location | tst.js:422:17:422:46 | window. ... bstr(1) | +| tst.js:422:17:422:46 | window. ... bstr(1) | tst.js:422:7:422:46 | payload | +| tst.js:425:7:425:55 | match | tst.js:427:20:427:24 | match | +| tst.js:425:15:425:29 | window.location | tst.js:425:15:425:55 | window. ... (\\w+)/) | +| tst.js:425:15:425:29 | window.location | tst.js:425:15:425:55 | window. ... (\\w+)/) | +| tst.js:425:15:425:55 | window. ... (\\w+)/) | tst.js:425:7:425:55 | match | +| tst.js:427:20:427:24 | match | tst.js:427:20:427:27 | match[1] | +| tst.js:427:20:427:24 | match | tst.js:427:20:427:27 | match[1] | +| tst.js:430:18:430:32 | window.location | tst.js:430:18:430:51 | window. ... '#')[1] | +| tst.js:430:18:430:32 | window.location | tst.js:430:18:430:51 | window. ... '#')[1] | +| tst.js:430:18:430:32 | window.location | tst.js:430:18:430:51 | window. ... '#')[1] | +| tst.js:430:18:430:32 | window.location | tst.js:430:18:430:51 | window. ... '#')[1] | | typeahead.js:9:28:9:30 | loc | typeahead.js:10:16:10:18 | loc | | typeahead.js:9:28:9:30 | loc | typeahead.js:10:16:10:18 | loc | | typeahead.js:9:28:9:30 | loc | typeahead.js:10:16:10:18 | loc | From 498ee9b5f51c628ac7b70172198cc65149e2f2bb Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Mon, 29 Jun 2020 16:01:16 +0200 Subject: [PATCH 09/13] C#: Factor C++ parts out of autobuilder --- .../BuildScripts.cs | 296 ++++++++++++++++++ .../Semmle.Autobuild.Cpp.Tests.csproj | 25 ++ .../Semmle.Autobuild.Cpp/CppAutobuilder.cs | 23 ++ .../Semmle.Autobuild.Cpp}/Program.cs | 9 +- .../Properties/AssemblyInfo.cs | 8 +- .../Semmle.Autobuild.Cpp.csproj | 28 ++ csharp/CSharp.sln | 28 +- .../BuildScripts.cs | 102 ++---- .../Semmle.Autobuild.CSharp.Tests.csproj} | 4 +- .../AspBuildRule.cs | 4 +- .../CSharpAutobuilder.cs | 127 ++++++++ .../DotNetRule.cs | 3 +- .../Semmle.Autobuild.CSharp/Program.cs | 33 ++ .../Properties/AssemblyInfo.cs | 32 ++ .../Semmle.Autobuild.CSharp.csproj} | 5 +- .../StandaloneBuildRule.cs | 4 +- .../XmlBuildRule.cs | 4 +- .../AutobuildOptions.cs | 21 +- .../Autobuilder.cs | 164 +--------- .../BuildActions.cs | 4 +- .../BuildCommandAutoRule.cs | 16 +- .../BuildCommandRule.cs | 16 +- .../BuildScript.cs | 2 +- .../BuildTools.cs | 2 +- .../CommandBuilder.cs | 4 +- .../Language.cs | 2 +- .../MsBuildRule.cs | 4 +- .../Project.cs | 2 +- .../ProjectOrSolution.cs | 2 +- .../Properties/AssemblyInfo.cs | 8 +- .../Semmle.Autobuild.Shared.csproj | 24 ++ .../Solution.cs | 3 +- 32 files changed, 721 insertions(+), 288 deletions(-) create mode 100644 cpp/autobuilder/Semmle.Autobuild.Cpp.Tests/BuildScripts.cs create mode 100644 cpp/autobuilder/Semmle.Autobuild.Cpp.Tests/Semmle.Autobuild.Cpp.Tests.csproj create mode 100644 cpp/autobuilder/Semmle.Autobuild.Cpp/CppAutobuilder.cs rename {csharp/autobuilder/Semmle.Autobuild => cpp/autobuilder/Semmle.Autobuild.Cpp}/Program.cs (72%) rename {csharp/autobuilder/Semmle.Autobuild.Tests => cpp/autobuilder/Semmle.Autobuild.Cpp}/Properties/AssemblyInfo.cs (83%) create mode 100644 cpp/autobuilder/Semmle.Autobuild.Cpp/Semmle.Autobuild.Cpp.csproj rename csharp/autobuilder/{Semmle.Autobuild.Tests => Semmle.Autobuild.CSharp.Tests}/BuildScripts.cs (91%) rename csharp/autobuilder/{Semmle.Autobuild.Tests/Semmle.Autobuild.Tests.csproj => Semmle.Autobuild.CSharp.Tests/Semmle.Autobuild.CSharp.Tests.csproj} (81%) rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.CSharp}/AspBuildRule.cs (90%) create mode 100644 csharp/autobuilder/Semmle.Autobuild.CSharp/CSharpAutobuilder.cs rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.CSharp}/DotNetRule.cs (99%) create mode 100644 csharp/autobuilder/Semmle.Autobuild.CSharp/Program.cs create mode 100644 csharp/autobuilder/Semmle.Autobuild.CSharp/Properties/AssemblyInfo.cs rename csharp/autobuilder/{Semmle.Autobuild/Semmle.Autobuild.csproj => Semmle.Autobuild.CSharp/Semmle.Autobuild.CSharp.csproj} (79%) rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.CSharp}/StandaloneBuildRule.cs (95%) rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.CSharp}/XmlBuildRule.cs (88%) rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.Shared}/AutobuildOptions.cs (86%) rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.Shared}/Autobuilder.cs (64%) rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.Shared}/BuildActions.cs (98%) rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.Shared}/BuildCommandAutoRule.cs (78%) rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.Shared}/BuildCommandRule.cs (64%) rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.Shared}/BuildScript.cs (99%) rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.Shared}/BuildTools.cs (99%) rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.Shared}/CommandBuilder.cs (99%) rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.Shared}/Language.cs (95%) rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.Shared}/MsBuildRule.cs (98%) rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.Shared}/Project.cs (99%) rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.Shared}/ProjectOrSolution.cs (98%) rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.Shared}/Properties/AssemblyInfo.cs (85%) create mode 100644 csharp/autobuilder/Semmle.Autobuild.Shared/Semmle.Autobuild.Shared.csproj rename csharp/autobuilder/{Semmle.Autobuild => Semmle.Autobuild.Shared}/Solution.cs (98%) diff --git a/cpp/autobuilder/Semmle.Autobuild.Cpp.Tests/BuildScripts.cs b/cpp/autobuilder/Semmle.Autobuild.Cpp.Tests/BuildScripts.cs new file mode 100644 index 00000000000..87390b7bf8f --- /dev/null +++ b/cpp/autobuilder/Semmle.Autobuild.Cpp.Tests/BuildScripts.cs @@ -0,0 +1,296 @@ +using Xunit; +using Semmle.Autobuild.Shared; +using System.Collections.Generic; +using System; +using System.Linq; +using Microsoft.Build.Construction; +using System.Xml; + +namespace Semmle.Autobuild.Cpp.Tests +{ + /// + /// Test class to script Autobuilder scenarios. + /// For most methods, it uses two fields: + /// - an IList to capture the the arguments passed to it + /// - an IDictionary of possible return values. + /// + class TestActions : IBuildActions + { + /// + /// List of strings passed to FileDelete. + /// + public IList FileDeleteIn = new List(); + + void IBuildActions.FileDelete(string file) + { + FileDeleteIn.Add(file); + } + + public IList FileExistsIn = new List(); + public IDictionary FileExists = new Dictionary(); + + bool IBuildActions.FileExists(string file) + { + FileExistsIn.Add(file); + if (FileExists.TryGetValue(file, out var ret)) + return ret; + if (FileExists.TryGetValue(System.IO.Path.GetFileName(file), out ret)) + return ret; + throw new ArgumentException("Missing FileExists " + file); + } + + public IList RunProcessIn = new List(); + public IDictionary RunProcess = new Dictionary(); + public IDictionary RunProcessOut = new Dictionary(); + public IDictionary RunProcessWorkingDirectory = new Dictionary(); + + int IBuildActions.RunProcess(string cmd, string args, string? workingDirectory, IDictionary? env, out IList stdOut) + { + var pattern = cmd + " " + args; + RunProcessIn.Add(pattern); + if (RunProcessOut.TryGetValue(pattern, out var str)) + stdOut = str.Split("\n"); + else + throw new ArgumentException("Missing RunProcessOut " + pattern); + RunProcessWorkingDirectory.TryGetValue(pattern, out var wd); + if (wd != workingDirectory) + throw new ArgumentException("Missing RunProcessWorkingDirectory " + pattern); + if (RunProcess.TryGetValue(pattern, out var ret)) + return ret; + throw new ArgumentException("Missing RunProcess " + pattern); + } + + int IBuildActions.RunProcess(string cmd, string args, string? workingDirectory, IDictionary? env) + { + var pattern = cmd + " " + args; + RunProcessIn.Add(pattern); + RunProcessWorkingDirectory.TryGetValue(pattern, out var wd); + if (wd != workingDirectory) + throw new ArgumentException("Missing RunProcessWorkingDirectory " + pattern); + if (RunProcess.TryGetValue(pattern, out var ret)) + return ret; + throw new ArgumentException("Missing RunProcess " + pattern); + } + + public IList DirectoryDeleteIn = new List(); + + void IBuildActions.DirectoryDelete(string dir, bool recursive) + { + DirectoryDeleteIn.Add(dir); + } + + public IDictionary DirectoryExists = new Dictionary(); + public IList DirectoryExistsIn = new List(); + + bool IBuildActions.DirectoryExists(string dir) + { + DirectoryExistsIn.Add(dir); + if (DirectoryExists.TryGetValue(dir, out var ret)) + return ret; + throw new ArgumentException("Missing DirectoryExists " + dir); + } + + public IDictionary GetEnvironmentVariable = new Dictionary(); + + string? IBuildActions.GetEnvironmentVariable(string name) + { + if (GetEnvironmentVariable.TryGetValue(name, out var ret)) + return ret; + throw new ArgumentException("Missing GetEnvironmentVariable " + name); + } + + public string GetCurrentDirectory = ""; + + string IBuildActions.GetCurrentDirectory() + { + return GetCurrentDirectory; + } + + public IDictionary EnumerateFiles = new Dictionary(); + + IEnumerable IBuildActions.EnumerateFiles(string dir) + { + if (EnumerateFiles.TryGetValue(dir, out var str)) + return str.Split("\n"); + throw new ArgumentException("Missing EnumerateFiles " + dir); + } + + public IDictionary EnumerateDirectories = new Dictionary(); + + IEnumerable IBuildActions.EnumerateDirectories(string dir) + { + if (EnumerateDirectories.TryGetValue(dir, out var str)) + return string.IsNullOrEmpty(str) ? Enumerable.Empty() : str.Split("\n"); + throw new ArgumentException("Missing EnumerateDirectories " + dir); + } + + public bool IsWindows; + + bool IBuildActions.IsWindows() => IsWindows; + + string IBuildActions.PathCombine(params string[] parts) + { + return string.Join(IsWindows ? '\\' : '/', parts.Where(p => !string.IsNullOrWhiteSpace(p))); + } + + string IBuildActions.GetFullPath(string path) => path; + + void IBuildActions.WriteAllText(string filename, string contents) + { + } + + public IDictionary LoadXml = new Dictionary(); + XmlDocument IBuildActions.LoadXml(string filename) + { + if (LoadXml.TryGetValue(filename, out var xml)) + return xml; + throw new ArgumentException("Missing LoadXml " + filename); + } + + public string EnvironmentExpandEnvironmentVariables(string s) + { + foreach (var kvp in GetEnvironmentVariable) + s = s.Replace($"%{kvp.Key}%", kvp.Value); + return s; + } + } + + /// + /// A fake solution to build. + /// + class TestSolution : ISolution + { + public IEnumerable Configurations => throw new NotImplementedException(); + + public string DefaultConfigurationName => "Release"; + + public string DefaultPlatformName => "x86"; + + public string FullPath { get; set; } + + public Version ToolsVersion => new Version("14.0"); + + public IEnumerable IncludedProjects => throw new NotImplementedException(); + + public TestSolution(string path) + { + FullPath = path; + } + } + + public class BuildScriptTests + { + TestActions Actions = new TestActions(); + + // Records the arguments passed to StartCallback. + IList StartCallbackIn = new List(); + + void StartCallback(string s, bool silent) + { + StartCallbackIn.Add(s); + } + + // Records the arguments passed to EndCallback + IList EndCallbackIn = new List(); + IList EndCallbackReturn = new List(); + + void EndCallback(int ret, string s, bool silent) + { + EndCallbackReturn.Add(ret); + EndCallbackIn.Add(s); + } + + CppAutobuilder CreateAutoBuilder(bool isWindows, + string? buildless = null, string? solution = null, string? buildCommand = null, string? ignoreErrors = null, + string? msBuildArguments = null, string? msBuildPlatform = null, string? msBuildConfiguration = null, string? msBuildTarget = null, + string? dotnetArguments = null, string? dotnetVersion = null, string? vsToolsVersion = null, + string? nugetRestore = null, string? allSolutions = null, + string cwd = @"C:\Project") + { + string codeqlUpperLanguage = Language.Cpp.UpperCaseName; + Actions.GetEnvironmentVariable[$"CODEQL_AUTOBUILDER_{codeqlUpperLanguage}_NO_INDEXING"] = "false"; + Actions.GetEnvironmentVariable[$"CODEQL_EXTRACTOR_{codeqlUpperLanguage}_TRAP_DIR"] = ""; + Actions.GetEnvironmentVariable[$"CODEQL_EXTRACTOR_{codeqlUpperLanguage}_SOURCE_ARCHIVE_DIR"] = ""; + Actions.GetEnvironmentVariable[$"CODEQL_EXTRACTOR_{codeqlUpperLanguage}_ROOT"] = $@"C:\codeql\{codeqlUpperLanguage.ToLowerInvariant()}"; + Actions.GetEnvironmentVariable["CODEQL_JAVA_HOME"] = @"C:\codeql\tools\java"; + Actions.GetEnvironmentVariable["SEMMLE_DIST"] = @"C:\odasa"; + Actions.GetEnvironmentVariable["SEMMLE_JAVA_HOME"] = @"C:\odasa\tools\java"; + Actions.GetEnvironmentVariable["SEMMLE_PLATFORM_TOOLS"] = @"C:\odasa\tools"; + Actions.GetEnvironmentVariable["LGTM_INDEX_VSTOOLS_VERSION"] = vsToolsVersion; + Actions.GetEnvironmentVariable["LGTM_INDEX_MSBUILD_ARGUMENTS"] = msBuildArguments; + Actions.GetEnvironmentVariable["LGTM_INDEX_MSBUILD_PLATFORM"] = msBuildPlatform; + Actions.GetEnvironmentVariable["LGTM_INDEX_MSBUILD_CONFIGURATION"] = msBuildConfiguration; + Actions.GetEnvironmentVariable["LGTM_INDEX_MSBUILD_TARGET"] = msBuildTarget; + Actions.GetEnvironmentVariable["LGTM_INDEX_DOTNET_ARGUMENTS"] = dotnetArguments; + Actions.GetEnvironmentVariable["LGTM_INDEX_DOTNET_VERSION"] = dotnetVersion; + Actions.GetEnvironmentVariable["LGTM_INDEX_BUILD_COMMAND"] = buildCommand; + Actions.GetEnvironmentVariable["LGTM_INDEX_SOLUTION"] = solution; + Actions.GetEnvironmentVariable["LGTM_INDEX_IGNORE_ERRORS"] = ignoreErrors; + Actions.GetEnvironmentVariable["LGTM_INDEX_BUILDLESS"] = buildless; + Actions.GetEnvironmentVariable["LGTM_INDEX_ALL_SOLUTIONS"] = allSolutions; + Actions.GetEnvironmentVariable["LGTM_INDEX_NUGET_RESTORE"] = nugetRestore; + Actions.GetEnvironmentVariable["ProgramFiles(x86)"] = isWindows ? @"C:\Program Files (x86)" : null; + Actions.GetCurrentDirectory = cwd; + Actions.IsWindows = isWindows; + + var options = new AutobuildOptions(Actions, Language.Cpp); + return new CppAutobuilder(Actions, options); + } + + void TestAutobuilderScript(Autobuilder autobuilder, int expectedOutput, int commandsRun) + { + Assert.Equal(expectedOutput, autobuilder.GetBuildScript().Run(Actions, StartCallback, EndCallback)); + + // Check expected commands actually ran + Assert.Equal(commandsRun, StartCallbackIn.Count); + Assert.Equal(commandsRun, EndCallbackIn.Count); + Assert.Equal(commandsRun, EndCallbackReturn.Count); + + var action = Actions.RunProcess.GetEnumerator(); + for (int cmd = 0; cmd < commandsRun; ++cmd) + { + Assert.True(action.MoveNext()); + + Assert.Equal(action.Current.Key, StartCallbackIn[cmd]); + Assert.Equal(action.Current.Value, EndCallbackReturn[cmd]); + } + } + + + [Fact] + public void TestDefaultCppAutobuilder() + { + Actions.EnumerateFiles[@"C:\Project"] = ""; + Actions.EnumerateDirectories[@"C:\Project"] = ""; + + var autobuilder = CreateAutoBuilder(true); + var script = autobuilder.GetBuildScript(); + + // Fails due to no solutions present. + Assert.NotEqual(0, script.Run(Actions, StartCallback, EndCallback)); + } + + [Fact] + public void TestCppAutobuilderSuccess() + { + Actions.RunProcess[@"cmd.exe /C C:\odasa\tools\csharp\nuget\nuget.exe restore C:\Project\test.sln"] = 1; + Actions.RunProcess[@"cmd.exe /C CALL ^""C:\Program Files ^(x86^)\Microsoft Visual Studio 14.0\VC\vcvarsall.bat^"" && set Platform=&& type NUL && C:\odasa\tools\odasa index --auto msbuild C:\Project\test.sln /p:UseSharedCompilation=false /t:rebuild /p:Platform=""x86"" /p:Configuration=""Release"" /p:MvcBuildViews=true"] = 0; + Actions.RunProcessOut[@"C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe -prerelease -legacy -property installationPath"] = ""; + Actions.RunProcess[@"C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe -prerelease -legacy -property installationPath"] = 1; + Actions.RunProcess[@"C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe -prerelease -legacy -property installationVersion"] = 0; + Actions.RunProcessOut[@"C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe -prerelease -legacy -property installationVersion"] = ""; + Actions.FileExists[@"C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\vcvarsall.bat"] = true; + Actions.FileExists[@"C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\vcvarsall.bat"] = true; + Actions.FileExists[@"C:\Program Files (x86)\Microsoft Visual Studio 11.0\VC\vcvarsall.bat"] = true; + Actions.FileExists[@"C:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\vcvarsall.bat"] = true; + Actions.FileExists[@"C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe"] = true; + Actions.EnumerateFiles[@"C:\Project"] = "foo.cs\ntest.slx"; + Actions.EnumerateDirectories[@"C:\Project"] = ""; + + var autobuilder = CreateAutoBuilder(true); + var solution = new TestSolution(@"C:\Project\test.sln"); + autobuilder.ProjectsOrSolutionsToBuild.Add(solution); + TestAutobuilderScript(autobuilder, 0, 2); + } + } +} diff --git a/cpp/autobuilder/Semmle.Autobuild.Cpp.Tests/Semmle.Autobuild.Cpp.Tests.csproj b/cpp/autobuilder/Semmle.Autobuild.Cpp.Tests/Semmle.Autobuild.Cpp.Tests.csproj new file mode 100644 index 00000000000..204b6418299 --- /dev/null +++ b/cpp/autobuilder/Semmle.Autobuild.Cpp.Tests/Semmle.Autobuild.Cpp.Tests.csproj @@ -0,0 +1,25 @@ + + + + Exe + netcoreapp3.0 + false + win-x64;linux-x64;osx-x64 + enable + + + + + + + + all + runtime; build; native; contentfiles; analyzers + + + + + + + + diff --git a/cpp/autobuilder/Semmle.Autobuild.Cpp/CppAutobuilder.cs b/cpp/autobuilder/Semmle.Autobuild.Cpp/CppAutobuilder.cs new file mode 100644 index 00000000000..44c34656a2a --- /dev/null +++ b/cpp/autobuilder/Semmle.Autobuild.Cpp/CppAutobuilder.cs @@ -0,0 +1,23 @@ +using Semmle.Autobuild.Shared; + +namespace Semmle.Autobuild.Cpp +{ + public class CppAutobuilder : Autobuilder + { + public CppAutobuilder(IBuildActions actions, AutobuildOptions options) : base(actions, options) { } + + public override BuildScript GetBuildScript() + { + if (Options.BuildCommand != null) + return new BuildCommandRule((_, f) => f(null)).Analyse(this, false); + + return + // First try MSBuild + new MsBuildRule().Analyse(this, true) | + // Then look for a script that might be a build script + (() => new BuildCommandAutoRule((_, f) => f(null)).Analyse(this, true)) | + // All attempts failed: print message + AutobuildFailure(); + } + } +} diff --git a/csharp/autobuilder/Semmle.Autobuild/Program.cs b/cpp/autobuilder/Semmle.Autobuild.Cpp/Program.cs similarity index 72% rename from csharp/autobuilder/Semmle.Autobuild/Program.cs rename to cpp/autobuilder/Semmle.Autobuild.Cpp/Program.cs index e4bccb0e626..3f4627c53d5 100644 --- a/csharp/autobuilder/Semmle.Autobuild/Program.cs +++ b/cpp/autobuilder/Semmle.Autobuild.Cpp/Program.cs @@ -1,6 +1,7 @@ using System; +using Semmle.Autobuild.Shared; -namespace Semmle.Autobuild +namespace Semmle.Autobuild.Cpp { class Program { @@ -10,11 +11,11 @@ namespace Semmle.Autobuild try { var actions = SystemBuildActions.Instance; - var options = new AutobuildOptions(actions); + var options = new AutobuildOptions(actions, Language.Cpp); try { - Console.WriteLine($"Semmle autobuilder for {options.Language}"); - var builder = new Autobuilder(actions, options); + Console.WriteLine("CodeQL C++ autobuilder"); + var builder = new CppAutobuilder(actions, options); return builder.AttemptBuild(); } catch(InvalidEnvironmentException ex) diff --git a/csharp/autobuilder/Semmle.Autobuild.Tests/Properties/AssemblyInfo.cs b/cpp/autobuilder/Semmle.Autobuild.Cpp/Properties/AssemblyInfo.cs similarity index 83% rename from csharp/autobuilder/Semmle.Autobuild.Tests/Properties/AssemblyInfo.cs rename to cpp/autobuilder/Semmle.Autobuild.Cpp/Properties/AssemblyInfo.cs index 778d6305fc5..2d14b0e909d 100644 --- a/csharp/autobuilder/Semmle.Autobuild.Tests/Properties/AssemblyInfo.cs +++ b/cpp/autobuilder/Semmle.Autobuild.Cpp/Properties/AssemblyInfo.cs @@ -4,12 +4,12 @@ using System.Runtime.InteropServices; // General Information about an assembly is controlled through the following // set of attributes. Change these attribute values to modify the information // associated with an assembly. -[assembly: AssemblyTitle("Semmle.Autobuild.Tests")] +[assembly: AssemblyTitle("Semmle.Autobuild.Cpp")] [assembly: AssemblyDescription("")] [assembly: AssemblyConfiguration("")] -[assembly: AssemblyCompany("")] -[assembly: AssemblyProduct("Semmle.Extraction.Tests")] -[assembly: AssemblyCopyright("Copyright © 2018")] +[assembly: AssemblyCompany("GitHub")] +[assembly: AssemblyProduct("CodeQL autobuilder for C++")] +[assembly: AssemblyCopyright("Copyright © GitHub 2020")] [assembly: AssemblyTrademark("")] [assembly: AssemblyCulture("")] diff --git a/cpp/autobuilder/Semmle.Autobuild.Cpp/Semmle.Autobuild.Cpp.csproj b/cpp/autobuilder/Semmle.Autobuild.Cpp/Semmle.Autobuild.Cpp.csproj new file mode 100644 index 00000000000..43e958183ea --- /dev/null +++ b/cpp/autobuilder/Semmle.Autobuild.Cpp/Semmle.Autobuild.Cpp.csproj @@ -0,0 +1,28 @@ + + + + netcoreapp3.0 + Semmle.Autobuild.Cpp + Semmle.Autobuild.Cpp + + Exe + + false + win-x64;linux-x64;osx-x64 + enable + + + + + + + + + + + + + + + + diff --git a/csharp/CSharp.sln b/csharp/CSharp.sln index 78d853a5bbe..7762dd469b9 100644 --- a/csharp/CSharp.sln +++ b/csharp/CSharp.sln @@ -11,8 +11,6 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Semmle.Extraction.CSharp", EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Semmle.Extraction.CIL", "extractor\Semmle.Extraction.CIL\Semmle.Extraction.CIL.csproj", "{399A1579-68F0-40F4-9A23-F241BA697F9C}" EndProject -Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Semmle.Autobuild", "autobuilder\Semmle.Autobuild\Semmle.Autobuild.csproj", "{5131EF00-0BA9-4436-A3B0-C5CDAB4B194C}" -EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Semmle.Extraction.CSharp.Standalone", "extractor\Semmle.Extraction.CSharp.Standalone\Semmle.Extraction.CSharp.Standalone.csproj", "{D00E7D25-0FA0-48EC-B048-CD60CE1B30D8}" EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Semmle.Extraction.CIL.Driver", "extractor\Semmle.Extraction.CIL.Driver\Semmle.Extraction.CIL.Driver.csproj", "{EFA400B3-C1CE-446F-A4E2-8B44E61EF47C}" @@ -23,7 +21,11 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Semmle.Extraction.Tests", " EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Semmle.Util.Tests", "extractor\Semmle.Util.Tests\Semmle.Util.Tests.csproj", "{55A620F0-23F6-440D-A5BA-0567613B3C0F}" EndProject -Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Semmle.Autobuild.Tests", "autobuilder\Semmle.Autobuild.Tests\Semmle.Autobuild.Tests.csproj", "{CE267461-D762-4F53-A275-685A0A4EC48D}" +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Semmle.Autobuild.Shared", "autobuilder\Semmle.Autobuild.Shared\Semmle.Autobuild.Shared.csproj", "{133F2B5B-FD25-4BD9-B34C-062CC6BB4178}" +EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Semmle.Autobuild.CSharp", "autobuilder\Semmle.Autobuild.CSharp\Semmle.Autobuild.CSharp.csproj", "{F3C07863-3759-4A0B-B777-8A0E0FDB1A41}" +EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Semmle.Autobuild.CSharp.Tests", "autobuilder\Semmle.Autobuild.CSharp.Tests\Semmle.Autobuild.CSharp.Tests.csproj", "{34256E8F-866A-46C1-800E-3DF69FD1DCB7}" EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution @@ -47,10 +49,6 @@ Global {399A1579-68F0-40F4-9A23-F241BA697F9C}.Debug|Any CPU.Build.0 = Debug|Any CPU {399A1579-68F0-40F4-9A23-F241BA697F9C}.Release|Any CPU.ActiveCfg = Release|Any CPU {399A1579-68F0-40F4-9A23-F241BA697F9C}.Release|Any CPU.Build.0 = Release|Any CPU - {5131EF00-0BA9-4436-A3B0-C5CDAB4B194C}.Debug|Any CPU.ActiveCfg = Debug|Any CPU - {5131EF00-0BA9-4436-A3B0-C5CDAB4B194C}.Debug|Any CPU.Build.0 = Debug|Any CPU - {5131EF00-0BA9-4436-A3B0-C5CDAB4B194C}.Release|Any CPU.ActiveCfg = Release|Any CPU - {5131EF00-0BA9-4436-A3B0-C5CDAB4B194C}.Release|Any CPU.Build.0 = Release|Any CPU {D00E7D25-0FA0-48EC-B048-CD60CE1B30D8}.Debug|Any CPU.ActiveCfg = Debug|Any CPU {D00E7D25-0FA0-48EC-B048-CD60CE1B30D8}.Debug|Any CPU.Build.0 = Debug|Any CPU {D00E7D25-0FA0-48EC-B048-CD60CE1B30D8}.Release|Any CPU.ActiveCfg = Release|Any CPU @@ -69,10 +67,18 @@ Global {55A620F0-23F6-440D-A5BA-0567613B3C0F}.Debug|Any CPU.ActiveCfg = Debug|Any CPU {55A620F0-23F6-440D-A5BA-0567613B3C0F}.Debug|Any CPU.Build.0 = Debug|Any CPU {55A620F0-23F6-440D-A5BA-0567613B3C0F}.Release|Any CPU.ActiveCfg = Release|Any CPU - {CE267461-D762-4F53-A275-685A0A4EC48D}.Debug|Any CPU.ActiveCfg = Debug|Any CPU - {CE267461-D762-4F53-A275-685A0A4EC48D}.Debug|Any CPU.Build.0 = Debug|Any CPU - {CE267461-D762-4F53-A275-685A0A4EC48D}.Release|Any CPU.ActiveCfg = Release|Any CPU - {CE267461-D762-4F53-A275-685A0A4EC48D}.Release|Any CPU.Build.0 = Release|Any CPU + {133F2B5B-FD25-4BD9-B34C-062CC6BB4178}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {133F2B5B-FD25-4BD9-B34C-062CC6BB4178}.Debug|Any CPU.Build.0 = Debug|Any CPU + {133F2B5B-FD25-4BD9-B34C-062CC6BB4178}.Release|Any CPU.ActiveCfg = Release|Any CPU + {133F2B5B-FD25-4BD9-B34C-062CC6BB4178}.Release|Any CPU.Build.0 = Release|Any CPU + {F3C07863-3759-4A0B-B777-8A0E0FDB1A41}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {F3C07863-3759-4A0B-B777-8A0E0FDB1A41}.Debug|Any CPU.Build.0 = Debug|Any CPU + {F3C07863-3759-4A0B-B777-8A0E0FDB1A41}.Release|Any CPU.ActiveCfg = Release|Any CPU + {F3C07863-3759-4A0B-B777-8A0E0FDB1A41}.Release|Any CPU.Build.0 = Release|Any CPU + {34256E8F-866A-46C1-800E-3DF69FD1DCB7}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {34256E8F-866A-46C1-800E-3DF69FD1DCB7}.Debug|Any CPU.Build.0 = Debug|Any CPU + {34256E8F-866A-46C1-800E-3DF69FD1DCB7}.Release|Any CPU.ActiveCfg = Release|Any CPU + {34256E8F-866A-46C1-800E-3DF69FD1DCB7}.Release|Any CPU.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE diff --git a/csharp/autobuilder/Semmle.Autobuild.Tests/BuildScripts.cs b/csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/BuildScripts.cs similarity index 91% rename from csharp/autobuilder/Semmle.Autobuild.Tests/BuildScripts.cs rename to csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/BuildScripts.cs index 370449de005..25f8600284f 100644 --- a/csharp/autobuilder/Semmle.Autobuild.Tests/BuildScripts.cs +++ b/csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/BuildScripts.cs @@ -1,12 +1,12 @@ using Xunit; -using Semmle.Autobuild; +using Semmle.Autobuild.Shared; using System.Collections.Generic; using System; using System.Linq; using Microsoft.Build.Construction; using System.Xml; -namespace Semmle.Extraction.Tests +namespace Semmle.Autobuild.CSharp.Tests { /// /// Test class to script Autobuilder scenarios. @@ -333,22 +333,21 @@ namespace Semmle.Extraction.Tests Assert.Equal(0, BuildScript.Try(BuildScript.Failure).Run(Actions, StartCallback, EndCallback)); } - Autobuilder CreateAutoBuilder(string lgtmLanguage, bool isWindows, + CSharpAutobuilder CreateAutoBuilder(bool isWindows, string? buildless = null, string? solution = null, string? buildCommand = null, string? ignoreErrors = null, string? msBuildArguments = null, string? msBuildPlatform = null, string? msBuildConfiguration = null, string? msBuildTarget = null, string? dotnetArguments = null, string? dotnetVersion = null, string? vsToolsVersion = null, string? nugetRestore = null, string? allSolutions = null, string cwd = @"C:\Project") { - string codeqlUpperLanguage = lgtmLanguage.ToUpper(); + string codeqlUpperLanguage = Language.CSharp.UpperCaseName; Actions.GetEnvironmentVariable[$"CODEQL_AUTOBUILDER_{codeqlUpperLanguage}_NO_INDEXING"] = "false"; Actions.GetEnvironmentVariable[$"CODEQL_EXTRACTOR_{codeqlUpperLanguage}_TRAP_DIR"] = ""; Actions.GetEnvironmentVariable[$"CODEQL_EXTRACTOR_{codeqlUpperLanguage}_SOURCE_ARCHIVE_DIR"] = ""; - Actions.GetEnvironmentVariable[$"CODEQL_EXTRACTOR_{codeqlUpperLanguage}_ROOT"] = $@"C:\codeql\{lgtmLanguage}"; + Actions.GetEnvironmentVariable[$"CODEQL_EXTRACTOR_{codeqlUpperLanguage}_ROOT"] = $@"C:\codeql\{codeqlUpperLanguage.ToLowerInvariant()}"; Actions.GetEnvironmentVariable["CODEQL_JAVA_HOME"] = @"C:\codeql\tools\java"; Actions.GetEnvironmentVariable["SEMMLE_DIST"] = @"C:\odasa"; Actions.GetEnvironmentVariable["SEMMLE_JAVA_HOME"] = @"C:\odasa\tools\java"; - Actions.GetEnvironmentVariable["LGTM_PROJECT_LANGUAGE"] = lgtmLanguage; Actions.GetEnvironmentVariable["SEMMLE_PLATFORM_TOOLS"] = @"C:\odasa\tools"; Actions.GetEnvironmentVariable["LGTM_INDEX_VSTOOLS_VERSION"] = vsToolsVersion; Actions.GetEnvironmentVariable["LGTM_INDEX_MSBUILD_ARGUMENTS"] = msBuildArguments; @@ -367,8 +366,8 @@ namespace Semmle.Extraction.Tests Actions.GetCurrentDirectory = cwd; Actions.IsWindows = isWindows; - var options = new AutobuildOptions(Actions); - return new Autobuilder(Actions, options); + var options = new AutobuildOptions(Actions, Language.CSharp); + return new CSharpAutobuilder(Actions, options); } [Fact] @@ -396,7 +395,7 @@ namespace Semmle.Extraction.Tests "); Actions.LoadXml["test.csproj"] = xml; - var autobuilder = CreateAutoBuilder("csharp", true); + var autobuilder = CreateAutoBuilder(true); TestAutobuilderScript(autobuilder, 0, 6); } @@ -428,7 +427,7 @@ Microsoft.NETCore.App 2.2.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap "); Actions.LoadXml["test.csproj"] = xml; - var autobuilder = CreateAutoBuilder("csharp", false); + var autobuilder = CreateAutoBuilder(false); TestAutobuilderScript(autobuilder, 0, 7); } @@ -441,47 +440,10 @@ Microsoft.NETCore.App 2.2.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap Actions.EnumerateFiles[@"C:\Project"] = "foo.cs\ntest.cs"; Actions.EnumerateDirectories[@"C:\Project"] = ""; - var autobuilder = CreateAutoBuilder("csharp", false); + var autobuilder = CreateAutoBuilder(false); TestAutobuilderScript(autobuilder, 1, 0); } - - [Fact] - public void TestDefaultCppAutobuilder() - { - Actions.EnumerateFiles[@"C:\Project"] = ""; - Actions.EnumerateDirectories[@"C:\Project"] = ""; - - var autobuilder = CreateAutoBuilder("cpp", true); - var script = autobuilder.GetBuildScript(); - - // Fails due to no solutions present. - Assert.NotEqual(0, script.Run(Actions, StartCallback, EndCallback)); - } - - [Fact] - public void TestCppAutobuilderSuccess() - { - Actions.RunProcess[@"cmd.exe /C C:\odasa\tools\csharp\nuget\nuget.exe restore C:\Project\test.sln"] = 1; - Actions.RunProcess[@"cmd.exe /C CALL ^""C:\Program Files ^(x86^)\Microsoft Visual Studio 14.0\VC\vcvarsall.bat^"" && set Platform=&& type NUL && C:\odasa\tools\odasa index --auto msbuild C:\Project\test.sln /p:UseSharedCompilation=false /t:rebuild /p:Platform=""x86"" /p:Configuration=""Release"" /p:MvcBuildViews=true"] = 0; - Actions.RunProcessOut[@"C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe -prerelease -legacy -property installationPath"] = ""; - Actions.RunProcess[@"C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe -prerelease -legacy -property installationPath"] = 1; - Actions.RunProcess[@"C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe -prerelease -legacy -property installationVersion"] = 0; - Actions.RunProcessOut[@"C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe -prerelease -legacy -property installationVersion"] = ""; - Actions.FileExists[@"C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\vcvarsall.bat"] = true; - Actions.FileExists[@"C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\vcvarsall.bat"] = true; - Actions.FileExists[@"C:\Program Files (x86)\Microsoft Visual Studio 11.0\VC\vcvarsall.bat"] = true; - Actions.FileExists[@"C:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\vcvarsall.bat"] = true; - Actions.FileExists[@"C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe"] = true; - Actions.EnumerateFiles[@"C:\Project"] = "foo.cs\ntest.slx"; - Actions.EnumerateDirectories[@"C:\Project"] = ""; - - var autobuilder = CreateAutoBuilder("cpp", true); - var solution = new TestSolution(@"C:\Project\test.sln"); - autobuilder.ProjectsOrSolutionsToBuild.Add(solution); - TestAutobuilderScript(autobuilder, 0, 2); - } - [Fact] public void TestVsWhereSucceeded() { @@ -538,7 +500,7 @@ Microsoft.NETCore.App 2.2.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap Actions.EnumerateFiles[@"C:\Project"] = "foo.cs\ntest.sln"; Actions.EnumerateDirectories[@"C:\Project"] = ""; - var autobuilder = CreateAutoBuilder("csharp", false, buildless: "true"); + var autobuilder = CreateAutoBuilder(false, buildless: "true"); TestAutobuilderScript(autobuilder, 0, 3); } @@ -552,7 +514,7 @@ Microsoft.NETCore.App 2.2.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap Actions.EnumerateFiles[@"C:\Project"] = "foo.cs\ntest.sln"; Actions.EnumerateDirectories[@"C:\Project"] = ""; - var autobuilder = CreateAutoBuilder("csharp", false, buildless: "true"); + var autobuilder = CreateAutoBuilder(false, buildless: "true"); TestAutobuilderScript(autobuilder, 10, 1); } @@ -568,7 +530,7 @@ Microsoft.NETCore.App 2.2.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap Actions.EnumerateFiles[@"C:\Project"] = "foo.cs\ntest.sln"; Actions.EnumerateDirectories[@"C:\Project"] = ""; - var autobuilder = CreateAutoBuilder("csharp", false, buildless: "true", solution: "foo.sln"); + var autobuilder = CreateAutoBuilder(false, buildless: "true", solution: "foo.sln"); TestAutobuilderScript(autobuilder, 0, 3); } @@ -616,7 +578,7 @@ Microsoft.NETCore.App 2.2.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap SkipVsWhere(); - var autobuilder = CreateAutoBuilder("csharp", false, buildCommand: "./build.sh --skip-tests"); + var autobuilder = CreateAutoBuilder(false, buildCommand: "./build.sh --skip-tests"); TestAutobuilderScript(autobuilder, 0, 4); } @@ -636,7 +598,7 @@ Microsoft.NETCore.App 2.2.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap Actions.RunProcess[@"C:\odasa/tools/odasa index --xml --extensions config csproj props xml"] = 0; Actions.FileExists["csharp.log"] = true; - var autobuilder = CreateAutoBuilder("csharp", false); + var autobuilder = CreateAutoBuilder(false); TestAutobuilderScript(autobuilder, 0, 5); } @@ -655,7 +617,7 @@ Microsoft.NETCore.App 2.2.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap Actions.RunProcessWorkingDirectory[@"C:\odasa/tools/odasa index --auto build.sh"] = ""; Actions.FileExists["csharp.log"] = false; - var autobuilder = CreateAutoBuilder("csharp", false); + var autobuilder = CreateAutoBuilder(false); TestAutobuilderScript(autobuilder, 1, 3); } @@ -674,7 +636,7 @@ Microsoft.NETCore.App 2.2.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap Actions.RunProcessWorkingDirectory[@"C:\odasa/tools/odasa index --auto build.sh"] = ""; Actions.FileExists["csharp.log"] = true; - var autobuilder = CreateAutoBuilder("csharp", false); + var autobuilder = CreateAutoBuilder(false); TestAutobuilderScript(autobuilder, 1, 3); } @@ -691,7 +653,7 @@ Microsoft.NETCore.App 2.2.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap Actions.RunProcess[@"cmd.exe /C C:\odasa\tools\odasa index --xml --extensions config csproj props xml"] = 0; Actions.FileExists["csharp.log"] = true; - var autobuilder = CreateAutoBuilder("csharp", true); + var autobuilder = CreateAutoBuilder(true); TestAutobuilderScript(autobuilder, 0, 3); } @@ -708,7 +670,7 @@ Microsoft.NETCore.App 2.2.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap Actions.RunProcess[@"cmd.exe /C C:\odasa\tools\odasa index --xml --extensions config"] = 0; Actions.FileExists["csharp.log"] = true; - var autobuilder = CreateAutoBuilder("csharp", true, ignoreErrors: "true"); + var autobuilder = CreateAutoBuilder(true, ignoreErrors: "true"); TestAutobuilderScript(autobuilder, 1, 1); } @@ -726,7 +688,7 @@ Microsoft.NETCore.App 2.2.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap Actions.EnumerateFiles[@"C:\Project"] = "foo.cs\ntest.sln"; Actions.EnumerateDirectories[@"C:\Project"] = ""; - var autobuilder = CreateAutoBuilder("csharp", true, buildCommand: "build.cmd --skip-tests", ignoreErrors: "true"); + var autobuilder = CreateAutoBuilder(true, buildCommand: "build.cmd --skip-tests", ignoreErrors: "true"); TestAutobuilderScript(autobuilder, 3, 1); } @@ -751,7 +713,7 @@ Microsoft.NETCore.App 2.2.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap Actions.EnumerateFiles[@"C:\Project"] = "foo.cs\ntest1.cs\ntest2.cs"; Actions.EnumerateDirectories[@"C:\Project"] = ""; - var autobuilder = CreateAutoBuilder("csharp", true, msBuildArguments: "/P:Fu=Bar", msBuildTarget: "Windows", msBuildPlatform: "x86", msBuildConfiguration: "Debug", + var autobuilder = CreateAutoBuilder(true, msBuildArguments: "/P:Fu=Bar", msBuildTarget: "Windows", msBuildPlatform: "x86", msBuildConfiguration: "Debug", vsToolsVersion: "12", allSolutions: "true"); var testSolution1 = new TestSolution(@"C:\Project\test1.sln"); var testSolution2 = new TestSolution(@"C:\Project\test2.sln"); @@ -802,7 +764,7 @@ Microsoft.NETCore.App 2.2.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap "); Actions.LoadXml["test2.csproj"] = csproj2; - var autobuilder = CreateAutoBuilder("csharp", true, msBuildArguments: "/P:Fu=Bar", msBuildTarget: "Windows", msBuildPlatform: "x86", msBuildConfiguration: "Debug", + var autobuilder = CreateAutoBuilder(true, msBuildArguments: "/P:Fu=Bar", msBuildTarget: "Windows", msBuildPlatform: "x86", msBuildConfiguration: "Debug", vsToolsVersion: "12"); TestAutobuilderScript(autobuilder, 0, 6); @@ -824,7 +786,7 @@ Microsoft.NETCore.App 2.2.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap Actions.EnumerateFiles[@"C:\Project"] = "foo.cs\ntest1.cs\ntest2.cs"; Actions.EnumerateDirectories[@"C:\Project"] = ""; - var autobuilder = CreateAutoBuilder("csharp", true, msBuildArguments: "/P:Fu=Bar", msBuildTarget: "Windows", msBuildPlatform: "x86", msBuildConfiguration: "Debug", + var autobuilder = CreateAutoBuilder(true, msBuildArguments: "/P:Fu=Bar", msBuildTarget: "Windows", msBuildPlatform: "x86", msBuildConfiguration: "Debug", vsToolsVersion: "12", allSolutions: "true"); var testSolution1 = new TestSolution(@"C:\Project\test1.sln"); var testSolution2 = new TestSolution(@"C:\Project\test2.sln"); @@ -853,7 +815,7 @@ Microsoft.NETCore.App 2.2.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap Actions.EnumerateFiles[@"C:\Project"] = "foo.cs\ntest1.cs\ntest2.cs"; Actions.EnumerateDirectories[@"C:\Project"] = ""; - var autobuilder = CreateAutoBuilder("csharp", true, msBuildArguments: "/P:Fu=Bar", msBuildTarget: "Windows", + var autobuilder = CreateAutoBuilder(true, msBuildArguments: "/P:Fu=Bar", msBuildTarget: "Windows", msBuildPlatform: "x86", msBuildConfiguration: "Debug", vsToolsVersion: "12", allSolutions: "true", nugetRestore: "false"); var testSolution1 = new TestSolution(@"C:\Project\test1.sln"); @@ -876,7 +838,7 @@ Microsoft.NETCore.App 2.2.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap Actions.EnumerateFiles[@"C:\Project"] = "foo.cs\ntest.sln"; Actions.EnumerateDirectories[@"C:\Project"] = ""; - var autobuilder = CreateAutoBuilder("csharp", false, buildless: "true", solution: "foo.sln", nugetRestore: "false"); + var autobuilder = CreateAutoBuilder(false, buildless: "true", solution: "foo.sln", nugetRestore: "false"); TestAutobuilderScript(autobuilder, 0, 3); } @@ -909,7 +871,7 @@ Microsoft.NETCore.App 2.1.3 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap "); Actions.LoadXml["test.csproj"] = xml; - var autobuilder = CreateAutoBuilder("csharp", false, dotnetArguments: "--no-restore"); // nugetRestore=false does not work for now. + var autobuilder = CreateAutoBuilder(false, dotnetArguments: "--no-restore"); // nugetRestore=false does not work for now. TestAutobuilderScript(autobuilder, 0, 7); } @@ -948,7 +910,7 @@ Microsoft.NETCore.App 3.0.0 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap "); Actions.LoadXml["test.csproj"] = xml; - var autobuilder = CreateAutoBuilder("csharp", false, dotnetVersion: "2.1.3"); + var autobuilder = CreateAutoBuilder(false, dotnetVersion: "2.1.3"); TestAutobuilderScript(autobuilder, 0, 12); } @@ -990,7 +952,7 @@ Microsoft.NETCore.App 2.1.4 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap "); Actions.LoadXml["test.csproj"] = xml; - var autobuilder = CreateAutoBuilder("csharp", false, dotnetVersion: "2.1.3"); + var autobuilder = CreateAutoBuilder(false, dotnetVersion: "2.1.3"); TestAutobuilderScript(autobuilder, 0, 12); } @@ -1024,7 +986,7 @@ Microsoft.NETCore.App 2.1.4 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap "); Actions.LoadXml["test.csproj"] = xml; - var autobuilder = CreateAutoBuilder("csharp", true, dotnetVersion: "2.1.3"); + var autobuilder = CreateAutoBuilder(true, dotnetVersion: "2.1.3"); TestAutobuilderScript(autobuilder, 0, 9); } @@ -1066,7 +1028,7 @@ Microsoft.NETCore.App 2.1.4 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap "); Actions.LoadXml["dirs.proj"] = dirsproj; - var autobuilder = CreateAutoBuilder("csharp", true, msBuildArguments: "/P:Fu=Bar", msBuildTarget: "Windows", msBuildPlatform: "x86", msBuildConfiguration: "Debug", + var autobuilder = CreateAutoBuilder(true, msBuildArguments: "/P:Fu=Bar", msBuildTarget: "Windows", msBuildPlatform: "x86", msBuildConfiguration: "Debug", vsToolsVersion: "12", allSolutions: "true"); TestAutobuilderScript(autobuilder, 0, 4); } @@ -1103,7 +1065,7 @@ Microsoft.NETCore.App 2.1.4 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap "); Actions.LoadXml["dirs.proj"] = dirsproj; - var autobuilder = CreateAutoBuilder("csharp", false); + var autobuilder = CreateAutoBuilder(false); TestAutobuilderScript(autobuilder, 0, 4); } @@ -1125,7 +1087,7 @@ Microsoft.NETCore.App 2.1.4 [/usr/local/share/dotnet/shared/Microsoft.NETCore.Ap "); Actions.LoadXml["dirs.proj"] = dirsproj1; - var autobuilder = CreateAutoBuilder("csharp", false); + var autobuilder = CreateAutoBuilder(false); TestAutobuilderScript(autobuilder, 1, 0); } diff --git a/csharp/autobuilder/Semmle.Autobuild.Tests/Semmle.Autobuild.Tests.csproj b/csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/Semmle.Autobuild.CSharp.Tests.csproj similarity index 81% rename from csharp/autobuilder/Semmle.Autobuild.Tests/Semmle.Autobuild.Tests.csproj rename to csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/Semmle.Autobuild.CSharp.Tests.csproj index 1f0016fc9b0..be45ad8f961 100644 --- a/csharp/autobuilder/Semmle.Autobuild.Tests/Semmle.Autobuild.Tests.csproj +++ b/csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/Semmle.Autobuild.CSharp.Tests.csproj @@ -19,7 +19,7 @@ - + + - diff --git a/csharp/autobuilder/Semmle.Autobuild/AspBuildRule.cs b/csharp/autobuilder/Semmle.Autobuild.CSharp/AspBuildRule.cs similarity index 90% rename from csharp/autobuilder/Semmle.Autobuild/AspBuildRule.cs rename to csharp/autobuilder/Semmle.Autobuild.CSharp/AspBuildRule.cs index f9c690c273b..2f69faeafde 100644 --- a/csharp/autobuilder/Semmle.Autobuild/AspBuildRule.cs +++ b/csharp/autobuilder/Semmle.Autobuild.CSharp/AspBuildRule.cs @@ -1,4 +1,6 @@ -namespace Semmle.Autobuild +using Semmle.Autobuild.Shared; + +namespace Semmle.Autobuild.CSharp { /// /// ASP extraction. diff --git a/csharp/autobuilder/Semmle.Autobuild.CSharp/CSharpAutobuilder.cs b/csharp/autobuilder/Semmle.Autobuild.CSharp/CSharpAutobuilder.cs new file mode 100644 index 00000000000..647a3ad2b4d --- /dev/null +++ b/csharp/autobuilder/Semmle.Autobuild.CSharp/CSharpAutobuilder.cs @@ -0,0 +1,127 @@ +using Semmle.Extraction.CSharp; +using Semmle.Util.Logging; +using Semmle.Autobuild.Shared; + +namespace Semmle.Autobuild.CSharp +{ + public class CSharpAutobuilder : Autobuilder + { + public CSharpAutobuilder(IBuildActions actions, AutobuildOptions options) : base(actions, options) { } + + public override BuildScript GetBuildScript() + { + /// + /// A script that checks that the C# extractor has been executed. + /// + BuildScript CheckExtractorRun(bool warnOnFailure) => + BuildScript.Create(actions => + { + if (actions.FileExists(Extractor.GetCSharpLogPath())) + return 0; + + if (warnOnFailure) + Log(Severity.Error, "No C# code detected during build."); + + return 1; + }); + + var attempt = BuildScript.Failure; + switch (GetCSharpBuildStrategy()) + { + case CSharpBuildStrategy.CustomBuildCommand: + attempt = new BuildCommandRule(DotNetRule.WithDotNet).Analyse(this, false) & CheckExtractorRun(true); + break; + case CSharpBuildStrategy.Buildless: + // No need to check that the extractor has been executed in buildless mode + attempt = new StandaloneBuildRule().Analyse(this, false); + break; + case CSharpBuildStrategy.MSBuild: + attempt = new MsBuildRule().Analyse(this, false) & CheckExtractorRun(true); + break; + case CSharpBuildStrategy.DotNet: + attempt = new DotNetRule().Analyse(this, false) & CheckExtractorRun(true); + break; + case CSharpBuildStrategy.Auto: + var cleanTrapFolder = + BuildScript.DeleteDirectory(TrapDir); + var cleanSourceArchive = + BuildScript.DeleteDirectory(SourceArchiveDir); + var tryCleanExtractorArgsLogs = + BuildScript.Create(actions => + { + foreach (var file in Extractor.GetCSharpArgsLogs()) + try + { + actions.FileDelete(file); + } + catch // lgtm[cs/catch-of-all-exceptions] lgtm[cs/empty-catch-block] + { } + return 0; + }); + var attemptExtractorCleanup = + BuildScript.Try(cleanTrapFolder) & + BuildScript.Try(cleanSourceArchive) & + tryCleanExtractorArgsLogs & + BuildScript.DeleteFile(Extractor.GetCSharpLogPath()); + + /// + /// Execute script `s` and check that the C# extractor has been executed. + /// If either fails, attempt to cleanup any artifacts produced by the extractor, + /// and exit with code 1, in order to proceed to the next attempt. + /// + BuildScript IntermediateAttempt(BuildScript s) => + (s & CheckExtractorRun(false)) | + (attemptExtractorCleanup & BuildScript.Failure); + + attempt = + // First try .NET Core + IntermediateAttempt(new DotNetRule().Analyse(this, true)) | + // Then MSBuild + (() => IntermediateAttempt(new MsBuildRule().Analyse(this, true))) | + // And finally look for a script that might be a build script + (() => new BuildCommandAutoRule(DotNetRule.WithDotNet).Analyse(this, true) & CheckExtractorRun(true)) | + // All attempts failed: print message + AutobuildFailure(); + break; + } + + return + attempt & + (() => new AspBuildRule().Analyse(this, false)) & + (() => new XmlBuildRule().Analyse(this, false)); + } + + /// + /// Gets the build strategy that the autobuilder should apply, based on the + /// options in the `lgtm.yml` file. + /// + CSharpBuildStrategy GetCSharpBuildStrategy() + { + if (Options.BuildCommand != null) + return CSharpBuildStrategy.CustomBuildCommand; + + if (Options.Buildless) + return CSharpBuildStrategy.Buildless; + + if (Options.MsBuildArguments != null + || Options.MsBuildConfiguration != null + || Options.MsBuildPlatform != null + || Options.MsBuildTarget != null) + return CSharpBuildStrategy.MSBuild; + + if (Options.DotNetArguments != null || Options.DotNetVersion != null) + return CSharpBuildStrategy.DotNet; + + return CSharpBuildStrategy.Auto; + } + + enum CSharpBuildStrategy + { + CustomBuildCommand, + Buildless, + MSBuild, + DotNet, + Auto + } + } +} diff --git a/csharp/autobuilder/Semmle.Autobuild/DotNetRule.cs b/csharp/autobuilder/Semmle.Autobuild.CSharp/DotNetRule.cs similarity index 99% rename from csharp/autobuilder/Semmle.Autobuild/DotNetRule.cs rename to csharp/autobuilder/Semmle.Autobuild.CSharp/DotNetRule.cs index 21215af8434..2d365f961da 100644 --- a/csharp/autobuilder/Semmle.Autobuild/DotNetRule.cs +++ b/csharp/autobuilder/Semmle.Autobuild.CSharp/DotNetRule.cs @@ -6,8 +6,9 @@ using System.Collections.Generic; using System.IO; using Semmle.Util; using System.Text.RegularExpressions; +using Semmle.Autobuild.Shared; -namespace Semmle.Autobuild +namespace Semmle.Autobuild.CSharp { /// /// A build rule where the build command is of the form "dotnet build". diff --git a/csharp/autobuilder/Semmle.Autobuild.CSharp/Program.cs b/csharp/autobuilder/Semmle.Autobuild.CSharp/Program.cs new file mode 100644 index 00000000000..ecefdd0efb2 --- /dev/null +++ b/csharp/autobuilder/Semmle.Autobuild.CSharp/Program.cs @@ -0,0 +1,33 @@ +using System; +using Semmle.Autobuild.Shared; + +namespace Semmle.Autobuild.CSharp +{ + class Program + { + static int Main() + { + + try + { + var actions = SystemBuildActions.Instance; + var options = new AutobuildOptions(actions, Language.CSharp); + try + { + Console.WriteLine("CodeQL C# autobuilder"); + var builder = new CSharpAutobuilder(actions, options); + return builder.AttemptBuild(); + } + catch (InvalidEnvironmentException ex) + { + Console.WriteLine("The environment is invalid: {0}", ex.Message); + } + } + catch (ArgumentOutOfRangeException ex) + { + Console.WriteLine("The value \"{0}\" for parameter \"{1}\" is invalid", ex.ActualValue, ex.ParamName); + } + return 1; + } + } +} diff --git a/csharp/autobuilder/Semmle.Autobuild.CSharp/Properties/AssemblyInfo.cs b/csharp/autobuilder/Semmle.Autobuild.CSharp/Properties/AssemblyInfo.cs new file mode 100644 index 00000000000..7314539ace4 --- /dev/null +++ b/csharp/autobuilder/Semmle.Autobuild.CSharp/Properties/AssemblyInfo.cs @@ -0,0 +1,32 @@ +using System.Reflection; +using System.Runtime.InteropServices; + +// General Information about an assembly is controlled through the following +// set of attributes. Change these attribute values to modify the information +// associated with an assembly. +[assembly: AssemblyTitle("Semmle.Autobuild.CSharp")] +[assembly: AssemblyDescription("")] +[assembly: AssemblyConfiguration("")] +[assembly: AssemblyCompany("GitHub")] +[assembly: AssemblyProduct("CodeQL autobuilder for C#")] +[assembly: AssemblyCopyright("Copyright © GitHub 2020")] +[assembly: AssemblyTrademark("")] +[assembly: AssemblyCulture("")] + +// Setting ComVisible to false makes the types in this assembly not visible +// to COM components. If you need to access a type in this assembly from +// COM, set the ComVisible attribute to true on that type. +[assembly: ComVisible(false)] + +// Version information for an assembly consists of the following four values: +// +// Major Version +// Minor Version +// Build Number +// Revision +// +// You can specify all the values or you can default the Build and Revision Numbers +// by using the '*' as shown below: +// [assembly: AssemblyVersion("1.0.*")] +[assembly: AssemblyVersion("1.0.0.0")] +[assembly: AssemblyFileVersion("1.0.0.0")] diff --git a/csharp/autobuilder/Semmle.Autobuild/Semmle.Autobuild.csproj b/csharp/autobuilder/Semmle.Autobuild.CSharp/Semmle.Autobuild.CSharp.csproj similarity index 79% rename from csharp/autobuilder/Semmle.Autobuild/Semmle.Autobuild.csproj rename to csharp/autobuilder/Semmle.Autobuild.CSharp/Semmle.Autobuild.CSharp.csproj index 63aab3b29fb..091f0704ef0 100644 --- a/csharp/autobuilder/Semmle.Autobuild/Semmle.Autobuild.csproj +++ b/csharp/autobuilder/Semmle.Autobuild.CSharp/Semmle.Autobuild.CSharp.csproj @@ -2,8 +2,8 @@ netcoreapp3.0 - Semmle.Autobuild - Semmle.Autobuild + Semmle.Autobuild.CSharp + Semmle.Autobuild.CSharp Exe @@ -24,6 +24,7 @@ + diff --git a/csharp/autobuilder/Semmle.Autobuild/StandaloneBuildRule.cs b/csharp/autobuilder/Semmle.Autobuild.CSharp/StandaloneBuildRule.cs similarity index 95% rename from csharp/autobuilder/Semmle.Autobuild/StandaloneBuildRule.cs rename to csharp/autobuilder/Semmle.Autobuild.CSharp/StandaloneBuildRule.cs index 26bc84bb601..5bfc8c776c4 100644 --- a/csharp/autobuilder/Semmle.Autobuild/StandaloneBuildRule.cs +++ b/csharp/autobuilder/Semmle.Autobuild.CSharp/StandaloneBuildRule.cs @@ -1,4 +1,6 @@ -namespace Semmle.Autobuild +using Semmle.Autobuild.Shared; + +namespace Semmle.Autobuild.CSharp { /// /// Build using standalone extraction. diff --git a/csharp/autobuilder/Semmle.Autobuild/XmlBuildRule.cs b/csharp/autobuilder/Semmle.Autobuild.CSharp/XmlBuildRule.cs similarity index 88% rename from csharp/autobuilder/Semmle.Autobuild/XmlBuildRule.cs rename to csharp/autobuilder/Semmle.Autobuild.CSharp/XmlBuildRule.cs index d9b05dbe0a9..d262ec1f20b 100644 --- a/csharp/autobuilder/Semmle.Autobuild/XmlBuildRule.cs +++ b/csharp/autobuilder/Semmle.Autobuild.CSharp/XmlBuildRule.cs @@ -1,4 +1,6 @@ -namespace Semmle.Autobuild +using Semmle.Autobuild.Shared; + +namespace Semmle.Autobuild.CSharp { /// /// XML extraction. diff --git a/csharp/autobuilder/Semmle.Autobuild/AutobuildOptions.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/AutobuildOptions.cs similarity index 86% rename from csharp/autobuilder/Semmle.Autobuild/AutobuildOptions.cs rename to csharp/autobuilder/Semmle.Autobuild.Shared/AutobuildOptions.cs index 725f2a3644f..28c14be13ff 100644 --- a/csharp/autobuilder/Semmle.Autobuild/AutobuildOptions.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/AutobuildOptions.cs @@ -2,7 +2,7 @@ using System.Linq; using System.Text.RegularExpressions; -namespace Semmle.Autobuild +namespace Semmle.Autobuild.Shared { /// /// Encapsulates build options. @@ -35,7 +35,7 @@ namespace Semmle.Autobuild /// Reads options from environment variables. /// Throws ArgumentOutOfRangeException for invalid arguments. /// - public AutobuildOptions(IBuildActions actions) + public AutobuildOptions(IBuildActions actions, Language language) { RootDirectory = actions.GetCurrentDirectory(); VsToolsVersion = actions.GetEnvironmentVariable(prefix + "VSTOOLS_VERSION"); @@ -53,7 +53,7 @@ namespace Semmle.Autobuild AllSolutions = actions.GetEnvironmentVariable(prefix + "ALL_SOLUTIONS").AsBool("all_solutions", false); NugetRestore = actions.GetEnvironmentVariable(prefix + "NUGET_RESTORE").AsBool("nuget_restore", true); - Language = actions.GetEnvironmentVariable("LGTM_PROJECT_LANGUAGE").AsLanguage(); + Language = language; Indexing = !actions.GetEnvironmentVariable($"CODEQL_AUTOBUILDER_{Language.UpperCaseName}_NO_INDEXING").AsBool("no_indexing", false); } @@ -81,21 +81,6 @@ namespace Semmle.Autobuild } } - public static Language AsLanguage(this string? key) - { - switch (key) - { - case null: - throw new ArgumentException("Environment variable required: LGTM_PROJECT_LANGUAGE"); - case "csharp": - return Language.CSharp; - case "cpp": - return Language.Cpp; - default: - throw new ArgumentException("Language key not understood: '" + key + "'"); - } - } - public static string[] AsListWithExpandedEnvVars(this string? value, IBuildActions actions, string[] defaultValue) { if (value == null) diff --git a/csharp/autobuilder/Semmle.Autobuild/Autobuilder.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/Autobuilder.cs similarity index 64% rename from csharp/autobuilder/Semmle.Autobuild/Autobuilder.cs rename to csharp/autobuilder/Semmle.Autobuild.Shared/Autobuilder.cs index cf6e089d314..e3889c550c9 100644 --- a/csharp/autobuilder/Semmle.Autobuild/Autobuilder.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/Autobuilder.cs @@ -1,16 +1,15 @@ -using Semmle.Extraction.CSharp; -using Semmle.Util.Logging; +using Semmle.Util.Logging; using System; using System.Collections.Generic; using System.IO; using System.Linq; -namespace Semmle.Autobuild +namespace Semmle.Autobuild.Shared { /// /// A build rule analyses the files in "builder" and outputs a build script. /// - interface IBuildRule + public interface IBuildRule { /// /// Analyse the files and produce a build script. @@ -23,7 +22,7 @@ namespace Semmle.Autobuild /// /// Exception indicating that environment variables are missing or invalid. /// - class InvalidEnvironmentException : Exception + public class InvalidEnvironmentException : Exception { public InvalidEnvironmentException(string m) : base(m) { } } @@ -35,7 +34,7 @@ namespace Semmle.Autobuild /// The overall design is intended to be extensible so that in theory, /// it should be possible to add new build rules without touching this code. /// - public class Autobuilder + public abstract class Autobuilder { /// /// Full file paths of files found in the project directory, as well as @@ -126,10 +125,10 @@ namespace Semmle.Autobuild ToArray(); if (matchingFiles.Length == 0) - return null; + return null; if (Options.AllSolutions) - return matchingFiles.Select(p => p.ProjectOrSolution); + return matchingFiles.Select(p => p.ProjectOrSolution); return matchingFiles. Where(f => f.DistanceFromRoot == matchingFiles[0].DistanceFromRoot). @@ -188,9 +187,9 @@ namespace Semmle.Autobuild SemmleDist = Actions.GetEnvironmentVariable("SEMMLE_DIST"); SemmlePlatformTools = Actions.GetEnvironmentVariable("SEMMLE_PLATFORM_TOOLS"); - JavaHome = + JavaHome = Actions.GetEnvironmentVariable("CODEQL_JAVA_HOME") ?? - Actions.GetEnvironmentVariable("SEMMLE_JAVA_HOME") ?? + Actions.GetEnvironmentVariable("SEMMLE_JAVA_HOME") ?? throw new InvalidEnvironmentException("The environment variable CODEQL_JAVA_HOME or SEMMLE_JAVA_HOME has not been set."); Distribution = @@ -209,9 +208,9 @@ namespace Semmle.Autobuild throw new InvalidEnvironmentException($"The environment variable CODEQL_EXTRACTOR_{this.Options.Language.UpperCaseName}_SOURCE_ARCHIVE_DIR or SOURCE_ARCHIVE has not been set."); } - private string TrapDir { get; } + protected string TrapDir { get; } - private string SourceArchiveDir { get; } + protected string SourceArchiveDir { get; } readonly ILogger logger = new ConsoleLogger(Verbosity.Info); @@ -234,6 +233,7 @@ namespace Semmle.Autobuild Log(Severity.Info, $"Working directory: {Options.RootDirectory}"); var script = GetBuildScript(); + if (Options.IgnoreErrors) script |= BuildScript.Success; @@ -253,143 +253,9 @@ namespace Semmle.Autobuild /// /// Returns the build script to use for this project. /// - public BuildScript GetBuildScript() - { - var isCSharp = Options.Language == Language.CSharp; - return isCSharp ? GetCSharpBuildScript() : GetCppBuildScript(); - } + public abstract BuildScript GetBuildScript(); - BuildScript GetCSharpBuildScript() - { - /// - /// A script that checks that the C# extractor has been executed. - /// - BuildScript CheckExtractorRun(bool warnOnFailure) => - BuildScript.Create(actions => - { - if (actions.FileExists(Extractor.GetCSharpLogPath())) - return 0; - - if (warnOnFailure) - Log(Severity.Error, "No C# code detected during build."); - - return 1; - }); - - var attempt = BuildScript.Failure; - switch (GetCSharpBuildStrategy()) - { - case CSharpBuildStrategy.CustomBuildCommand: - attempt = new BuildCommandRule().Analyse(this, false) & CheckExtractorRun(true); - break; - case CSharpBuildStrategy.Buildless: - // No need to check that the extractor has been executed in buildless mode - attempt = new StandaloneBuildRule().Analyse(this, false); - break; - case CSharpBuildStrategy.MSBuild: - attempt = new MsBuildRule().Analyse(this, false) & CheckExtractorRun(true); - break; - case CSharpBuildStrategy.DotNet: - attempt = new DotNetRule().Analyse(this, false) & CheckExtractorRun(true); - break; - case CSharpBuildStrategy.Auto: - var cleanTrapFolder = - BuildScript.DeleteDirectory(TrapDir); - var cleanSourceArchive = - BuildScript.DeleteDirectory(SourceArchiveDir); - var tryCleanExtractorArgsLogs = - BuildScript.Create(actions => - { - foreach (var file in Extractor.GetCSharpArgsLogs()) - try - { - actions.FileDelete(file); - } - catch // lgtm[cs/catch-of-all-exceptions] lgtm[cs/empty-catch-block] - { } - return 0; - }); - var attemptExtractorCleanup = - BuildScript.Try(cleanTrapFolder) & - BuildScript.Try(cleanSourceArchive) & - tryCleanExtractorArgsLogs & - BuildScript.DeleteFile(Extractor.GetCSharpLogPath()); - - /// - /// Execute script `s` and check that the C# extractor has been executed. - /// If either fails, attempt to cleanup any artifacts produced by the extractor, - /// and exit with code 1, in order to proceed to the next attempt. - /// - BuildScript IntermediateAttempt(BuildScript s) => - (s & CheckExtractorRun(false)) | - (attemptExtractorCleanup & BuildScript.Failure); - - attempt = - // First try .NET Core - IntermediateAttempt(new DotNetRule().Analyse(this, true)) | - // Then MSBuild - (() => IntermediateAttempt(new MsBuildRule().Analyse(this, true))) | - // And finally look for a script that might be a build script - (() => new BuildCommandAutoRule().Analyse(this, true) & CheckExtractorRun(true)) | - // All attempts failed: print message - AutobuildFailure(); - break; - } - - return - attempt & - (() => new AspBuildRule().Analyse(this, false)) & - (() => new XmlBuildRule().Analyse(this, false)); - } - - /// - /// Gets the build strategy that the autobuilder should apply, based on the - /// options in the `lgtm.yml` file. - /// - CSharpBuildStrategy GetCSharpBuildStrategy() - { - if (Options.BuildCommand != null) - return CSharpBuildStrategy.CustomBuildCommand; - - if (Options.Buildless) - return CSharpBuildStrategy.Buildless; - - if (Options.MsBuildArguments != null - || Options.MsBuildConfiguration != null - || Options.MsBuildPlatform != null - || Options.MsBuildTarget != null) - return CSharpBuildStrategy.MSBuild; - - if (Options.DotNetArguments != null || Options.DotNetVersion != null) - return CSharpBuildStrategy.DotNet; - - return CSharpBuildStrategy.Auto; - } - - enum CSharpBuildStrategy - { - CustomBuildCommand, - Buildless, - MSBuild, - DotNet, - Auto - } - - BuildScript GetCppBuildScript() - { - if (Options.BuildCommand != null) - return new BuildCommandRule().Analyse(this, false); - - return - // First try MSBuild - new MsBuildRule().Analyse(this, true) | - // Then look for a script that might be a build script - (() => new BuildCommandAutoRule().Analyse(this, true)) | - // All attempts failed: print message - AutobuildFailure(); - } - - BuildScript AutobuildFailure() => + protected BuildScript AutobuildFailure() => BuildScript.Create(actions => { Log(Severity.Error, "Could not auto-detect a suitable build method"); @@ -426,6 +292,6 @@ namespace Semmle.Autobuild /// an odasa --index, unless indexing has been disabled, in which case /// is run directly. /// - internal CommandBuilder MaybeIndex(CommandBuilder builder, string cmd) => Options.Indexing && !(Odasa is null) ? builder.IndexCommand(Odasa, cmd) : builder.RunCommand(cmd); + public CommandBuilder MaybeIndex(CommandBuilder builder, string cmd) => Options.Indexing && !(Odasa is null) ? builder.IndexCommand(Odasa, cmd) : builder.RunCommand(cmd); } } diff --git a/csharp/autobuilder/Semmle.Autobuild/BuildActions.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/BuildActions.cs similarity index 98% rename from csharp/autobuilder/Semmle.Autobuild/BuildActions.cs rename to csharp/autobuilder/Semmle.Autobuild.Shared/BuildActions.cs index 7bc4b9b7591..63cfc3b8145 100644 --- a/csharp/autobuilder/Semmle.Autobuild/BuildActions.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/BuildActions.cs @@ -5,7 +5,7 @@ using System.Diagnostics; using System.IO; using System.Xml; -namespace Semmle.Autobuild +namespace Semmle.Autobuild.Shared { /// /// Wrapper around system calls so that the build scripts can be unit-tested. @@ -124,7 +124,7 @@ namespace Semmle.Autobuild /// /// An implementation of IBuildActions that actually performs the requested operations. /// - class SystemBuildActions : IBuildActions + public class SystemBuildActions : IBuildActions { void IBuildActions.FileDelete(string file) => File.Delete(file); diff --git a/csharp/autobuilder/Semmle.Autobuild/BuildCommandAutoRule.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/BuildCommandAutoRule.cs similarity index 78% rename from csharp/autobuilder/Semmle.Autobuild/BuildCommandAutoRule.cs rename to csharp/autobuilder/Semmle.Autobuild.Shared/BuildCommandAutoRule.cs index 80a819f403e..2ef98609512 100644 --- a/csharp/autobuilder/Semmle.Autobuild/BuildCommandAutoRule.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/BuildCommandAutoRule.cs @@ -1,15 +1,23 @@ -using System.Collections.Generic; +using System; +using System.Collections.Generic; using System.IO; using System.Linq; using Semmle.Util.Logging; -namespace Semmle.Autobuild +namespace Semmle.Autobuild.Shared { /// /// Auto-detection of build scripts. /// - class BuildCommandAutoRule : IBuildRule + public class BuildCommandAutoRule : IBuildRule { + private readonly Func?, BuildScript>, BuildScript> withDotNet; + + public BuildCommandAutoRule(Func?, BuildScript>, BuildScript> withDotNet) + { + this.withDotNet = withDotNet; + } + readonly IEnumerable winExtensions = new List { ".bat", ".cmd", @@ -43,7 +51,7 @@ namespace Semmle.Autobuild string? dir = Path.GetDirectoryName(scriptPath); // A specific .NET Core version may be required - return chmodScript & DotNetRule.WithDotNet(builder, environment => + return chmodScript & withDotNet(builder, environment => { var command = new CommandBuilder(builder.Actions, dir, environment); diff --git a/csharp/autobuilder/Semmle.Autobuild/BuildCommandRule.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/BuildCommandRule.cs similarity index 64% rename from csharp/autobuilder/Semmle.Autobuild/BuildCommandRule.cs rename to csharp/autobuilder/Semmle.Autobuild.Shared/BuildCommandRule.cs index fe91503ec8f..79cdd8c01de 100644 --- a/csharp/autobuilder/Semmle.Autobuild/BuildCommandRule.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/BuildCommandRule.cs @@ -1,17 +1,27 @@ -namespace Semmle.Autobuild +using System; +using System.Collections.Generic; + +namespace Semmle.Autobuild.Shared { /// /// Execute the build_command rule. /// - class BuildCommandRule : IBuildRule + public class BuildCommandRule : IBuildRule { + private readonly Func?, BuildScript>, BuildScript> withDotNet; + + public BuildCommandRule(Func?, BuildScript>, BuildScript> withDotNet) + { + this.withDotNet = withDotNet; + } + public BuildScript Analyse(Autobuilder builder, bool auto) { if (builder.Options.BuildCommand == null) return BuildScript.Failure; // Custom build commands may require a specific .NET Core version - return DotNetRule.WithDotNet(builder, environment => + return withDotNet(builder, environment => { var command = new CommandBuilder(builder.Actions, null, environment); diff --git a/csharp/autobuilder/Semmle.Autobuild/BuildScript.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/BuildScript.cs similarity index 99% rename from csharp/autobuilder/Semmle.Autobuild/BuildScript.cs rename to csharp/autobuilder/Semmle.Autobuild.Shared/BuildScript.cs index e441030ff77..a3ac5b5cbae 100644 --- a/csharp/autobuilder/Semmle.Autobuild/BuildScript.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/BuildScript.cs @@ -2,7 +2,7 @@ using System.Collections.Generic; using System.IO; -namespace Semmle.Autobuild +namespace Semmle.Autobuild.Shared { /// /// A build script. diff --git a/csharp/autobuilder/Semmle.Autobuild/BuildTools.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/BuildTools.cs similarity index 99% rename from csharp/autobuilder/Semmle.Autobuild/BuildTools.cs rename to csharp/autobuilder/Semmle.Autobuild.Shared/BuildTools.cs index ca9285d2c9c..d1aab3c4551 100644 --- a/csharp/autobuilder/Semmle.Autobuild/BuildTools.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/BuildTools.cs @@ -1,7 +1,7 @@ using System.Collections.Generic; using System.Linq; -namespace Semmle.Autobuild +namespace Semmle.Autobuild.Shared { /// /// A BAT file used to initialise the appropriate diff --git a/csharp/autobuilder/Semmle.Autobuild/CommandBuilder.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/CommandBuilder.cs similarity index 99% rename from csharp/autobuilder/Semmle.Autobuild/CommandBuilder.cs rename to csharp/autobuilder/Semmle.Autobuild.Shared/CommandBuilder.cs index 353f132c6ec..ef6eb951449 100644 --- a/csharp/autobuilder/Semmle.Autobuild/CommandBuilder.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/CommandBuilder.cs @@ -2,12 +2,12 @@ using System.Collections.Generic; using System.Linq; using System.Text; -namespace Semmle.Autobuild +namespace Semmle.Autobuild.Shared { /// /// Utility to construct a build command. /// - class CommandBuilder + public class CommandBuilder { enum EscapeMode { Process, Cmd }; diff --git a/csharp/autobuilder/Semmle.Autobuild/Language.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/Language.cs similarity index 95% rename from csharp/autobuilder/Semmle.Autobuild/Language.cs rename to csharp/autobuilder/Semmle.Autobuild.Shared/Language.cs index 7c202f86ed8..c27f42fb7a7 100644 --- a/csharp/autobuilder/Semmle.Autobuild/Language.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/Language.cs @@ -1,4 +1,4 @@ -namespace Semmle.Autobuild +namespace Semmle.Autobuild.Shared { public sealed class Language { diff --git a/csharp/autobuilder/Semmle.Autobuild/MsBuildRule.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/MsBuildRule.cs similarity index 98% rename from csharp/autobuilder/Semmle.Autobuild/MsBuildRule.cs rename to csharp/autobuilder/Semmle.Autobuild.Shared/MsBuildRule.cs index 569f9f183eb..4994eefab06 100644 --- a/csharp/autobuilder/Semmle.Autobuild/MsBuildRule.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/MsBuildRule.cs @@ -1,12 +1,12 @@ using Semmle.Util.Logging; using System.Linq; -namespace Semmle.Autobuild +namespace Semmle.Autobuild.Shared { /// /// A build rule using msbuild. /// - class MsBuildRule : IBuildRule + public class MsBuildRule : IBuildRule { /// /// The name of the msbuild command. diff --git a/csharp/autobuilder/Semmle.Autobuild/Project.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/Project.cs similarity index 99% rename from csharp/autobuilder/Semmle.Autobuild/Project.cs rename to csharp/autobuilder/Semmle.Autobuild.Shared/Project.cs index 415ddcbc0f0..df8fc077145 100644 --- a/csharp/autobuilder/Semmle.Autobuild/Project.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/Project.cs @@ -5,7 +5,7 @@ using System.Linq; using System.Xml; using Semmle.Util.Logging; -namespace Semmle.Autobuild +namespace Semmle.Autobuild.Shared { /// /// Representation of a .proj file, a .csproj file (C#), or a .vcxproj file (C++). diff --git a/csharp/autobuilder/Semmle.Autobuild/ProjectOrSolution.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/ProjectOrSolution.cs similarity index 98% rename from csharp/autobuilder/Semmle.Autobuild/ProjectOrSolution.cs rename to csharp/autobuilder/Semmle.Autobuild.Shared/ProjectOrSolution.cs index 13859a8c0eb..aff44a62540 100644 --- a/csharp/autobuilder/Semmle.Autobuild/ProjectOrSolution.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/ProjectOrSolution.cs @@ -2,7 +2,7 @@ using System.IO; using System.Linq; -namespace Semmle.Autobuild +namespace Semmle.Autobuild.Shared { /// /// A file that can be the target in an invocation of `msbuild` or `dotnet build`. diff --git a/csharp/autobuilder/Semmle.Autobuild/Properties/AssemblyInfo.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/Properties/AssemblyInfo.cs similarity index 85% rename from csharp/autobuilder/Semmle.Autobuild/Properties/AssemblyInfo.cs rename to csharp/autobuilder/Semmle.Autobuild.Shared/Properties/AssemblyInfo.cs index e3da7ca22e9..2eecfca8f52 100644 --- a/csharp/autobuilder/Semmle.Autobuild/Properties/AssemblyInfo.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/Properties/AssemblyInfo.cs @@ -4,12 +4,12 @@ using System.Runtime.InteropServices; // General Information about an assembly is controlled through the following // set of attributes. Change these attribute values to modify the information // associated with an assembly. -[assembly: AssemblyTitle("Semmle.Autobuild")] +[assembly: AssemblyTitle("Semmle.Autobuild.Shared")] [assembly: AssemblyDescription("")] [assembly: AssemblyConfiguration("")] -[assembly: AssemblyCompany("Semmle")] -[assembly: AssemblyProduct("Semmle Visual Studio Autobuild")] -[assembly: AssemblyCopyright("Copyright © Semmle 2017")] +[assembly: AssemblyCompany("GitHub")] +[assembly: AssemblyProduct("CodeQL autobuilder")] +[assembly: AssemblyCopyright("Copyright © GitHub 2020")] [assembly: AssemblyTrademark("")] [assembly: AssemblyCulture("")] diff --git a/csharp/autobuilder/Semmle.Autobuild.Shared/Semmle.Autobuild.Shared.csproj b/csharp/autobuilder/Semmle.Autobuild.Shared/Semmle.Autobuild.Shared.csproj new file mode 100644 index 00000000000..66a5b26098c --- /dev/null +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/Semmle.Autobuild.Shared.csproj @@ -0,0 +1,24 @@ + + + + netcoreapp3.0 + Semmle.Autobuild.Shared + Semmle.Autobuild.Shared + false + win-x64;linux-x64;osx-x64 + enable + + + + + + + + + + + + + + + diff --git a/csharp/autobuilder/Semmle.Autobuild/Solution.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/Solution.cs similarity index 98% rename from csharp/autobuilder/Semmle.Autobuild/Solution.cs rename to csharp/autobuilder/Semmle.Autobuild.Shared/Solution.cs index 0429b9f420c..e6563b31ff6 100644 --- a/csharp/autobuilder/Semmle.Autobuild/Solution.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/Solution.cs @@ -3,11 +3,10 @@ using Microsoft.Build.Exceptions; using System; using System.Collections.Generic; using System.Linq; -using Semmle.Util; using System.IO; using Semmle.Util.Logging; -namespace Semmle.Autobuild +namespace Semmle.Autobuild.Shared { /// /// A solution file, extension .sln. From 398a95c65fb7a03b59f7e22f80d1080e231929b2 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 30 Jun 2020 13:28:04 +0200 Subject: [PATCH 10/13] C#: Remove unused field --- .../autobuilder/Semmle.Autobuild.CSharp.Tests/BuildScripts.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/BuildScripts.cs b/csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/BuildScripts.cs index 25f8600284f..5c6441a7df0 100644 --- a/csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/BuildScripts.cs +++ b/csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/BuildScripts.cs @@ -80,11 +80,9 @@ namespace Semmle.Autobuild.CSharp.Tests } public IDictionary DirectoryExists = new Dictionary(); - public IList DirectoryExistsIn = new List(); bool IBuildActions.DirectoryExists(string dir) { - DirectoryExistsIn.Add(dir); if (DirectoryExists.TryGetValue(dir, out var ret)) return ret; throw new ArgumentException("Missing DirectoryExists " + dir); From 4a7bfbe0918ddf2cb35b91e034a0ce57131933fc Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Thu, 2 Jul 2020 11:43:23 +0200 Subject: [PATCH 11/13] Python: Use .matches instead of .indexOf() = 0 --- python/ql/src/semmle/python/web/django/Response.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/semmle/python/web/django/Response.qll b/python/ql/src/semmle/python/web/django/Response.qll index a0e07ea4b21..5ceef4516f5 100644 --- a/python/ql/src/semmle/python/web/django/Response.qll +++ b/python/ql/src/semmle/python/web/django/Response.qll @@ -65,7 +65,7 @@ class DjangoResponseContentXSSVulnerable extends DjangoResponseContent { or exists(StringValue s | cls.getContentTypeArg(call).pointsTo(s) and - s.getText().indexOf("text/html") = 0 + s.getText().matches("text/html%") ) } } From a947d151e5130c37a386a5333de4b74d2d5ab7d8 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Thu, 2 Jul 2020 11:53:25 +0200 Subject: [PATCH 12/13] Python: Django changes now backwards compatible deprecation --- python/ql/src/semmle/python/web/django/Redirect.qll | 3 +++ python/ql/src/semmle/python/web/django/Response.qll | 10 ++++++++++ python/ql/src/semmle/python/web/django/Shared.qll | 12 ++++++++++++ 3 files changed, 25 insertions(+) diff --git a/python/ql/src/semmle/python/web/django/Redirect.qll b/python/ql/src/semmle/python/web/django/Redirect.qll index 61ee041f904..b3b52f5e03e 100644 --- a/python/ql/src/semmle/python/web/django/Redirect.qll +++ b/python/ql/src/semmle/python/web/django/Redirect.qll @@ -21,6 +21,9 @@ class DjangoShortcutsRedirectSink extends HttpRedirectTaintSink { } } +/** DEPRECATED: Use `DjangoShortcutsRedirectSink` instead. */ +deprecated class DjangoRedirect = DjangoShortcutsRedirectSink; + /** * The URL argument when instantiating a Django Redirect Response. */ diff --git a/python/ql/src/semmle/python/web/django/Response.qll b/python/ql/src/semmle/python/web/django/Response.qll index 5ceef4516f5..df27d319492 100644 --- a/python/ql/src/semmle/python/web/django/Response.qll +++ b/python/ql/src/semmle/python/web/django/Response.qll @@ -4,6 +4,16 @@ import semmle.python.security.strings.Basic private import semmle.python.web.django.Shared private import semmle.python.web.Http +/** + * DEPRECATED: This class is internal to the django library modeling, and should + * never be used by anyone. + * + * A django.http.response.Response object + * This isn't really a "taint", but we use the value tracking machinery to + * track the flow of response objects. + */ +deprecated class DjangoResponse = DjangoResponseKind; + /** INTERNAL class used for tracking a django response object. */ private class DjangoResponseKind extends TaintKind { DjangoResponseKind() { this = "django.response.HttpResponse" } diff --git a/python/ql/src/semmle/python/web/django/Shared.qll b/python/ql/src/semmle/python/web/django/Shared.qll index f66acc7fe2d..b98bd803fcf 100644 --- a/python/ql/src/semmle/python/web/django/Shared.qll +++ b/python/ql/src/semmle/python/web/django/Shared.qll @@ -1,5 +1,17 @@ import python +/** DEPRECATED: Use `Value::named("django.shortcuts.redirect")` instead. */ +deprecated FunctionValue redirect() { result = Value::named("django.shortcuts.redirect") } + +/** DEPRECATED: Use `DjangoRedirectResponseClass` instead. */ +deprecated ClassValue theDjangoHttpRedirectClass() { + // version 1.x + result = Value::named("django.http.response.HttpResponseRedirectBase") + or + // version 2.x + result = Value::named("django.http.HttpResponseRedirectBase") +} + /** A class that is a Django Redirect Response (subclass of `django.http.HttpResponseRedirectBase`). */ class DjangoRedirectResponseClass extends ClassValue { DjangoRedirectResponseClass() { From 9a829271877efd74a5f7851c0ece3f692b905fe9 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Thu, 2 Jul 2020 11:54:41 +0200 Subject: [PATCH 13/13] Python: Autoformat --- python/ql/src/semmle/python/web/django/Redirect.qll | 4 +--- python/ql/src/semmle/python/web/django/Response.qll | 6 +----- python/ql/src/semmle/python/web/django/Shared.qll | 3 +-- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/python/ql/src/semmle/python/web/django/Redirect.qll b/python/ql/src/semmle/python/web/django/Redirect.qll index b3b52f5e03e..ad46d985b8c 100644 --- a/python/ql/src/semmle/python/web/django/Redirect.qll +++ b/python/ql/src/semmle/python/web/django/Redirect.qll @@ -29,9 +29,7 @@ deprecated class DjangoRedirect = DjangoShortcutsRedirectSink; */ class DjangoRedirectResponseSink extends HttpRedirectTaintSink { DjangoRedirectResponseSink() { - exists(CallNode call | - call = any(DjangoRedirectResponseClass cls).getACall() - | + exists(CallNode call | call = any(DjangoRedirectResponseClass cls).getACall() | this = call.getArg(0) or this = call.getArgByName("redirect_to") diff --git a/python/ql/src/semmle/python/web/django/Response.qll b/python/ql/src/semmle/python/web/django/Response.qll index df27d319492..a44af076213 100644 --- a/python/ql/src/semmle/python/web/django/Response.qll +++ b/python/ql/src/semmle/python/web/django/Response.qll @@ -21,11 +21,7 @@ private class DjangoResponseKind extends TaintKind { /** INTERNAL taint-source used for tracking a django response object. */ private class DjangoResponseSource extends TaintSource { - DjangoResponseSource() { - exists(DjangoContentResponseClass cls | - cls.getACall() = this - ) - } + DjangoResponseSource() { exists(DjangoContentResponseClass cls | cls.getACall() = this) } override predicate isSourceOf(TaintKind kind) { kind instanceof DjangoResponseKind } diff --git a/python/ql/src/semmle/python/web/django/Shared.qll b/python/ql/src/semmle/python/web/django/Shared.qll index b98bd803fcf..ea856ddf65a 100644 --- a/python/ql/src/semmle/python/web/django/Shared.qll +++ b/python/ql/src/semmle/python/web/django/Shared.qll @@ -51,7 +51,6 @@ class DjangoContentResponseClass extends ClassValue { // `django.http.response.HttpResponseNotAllowed` it would make much more sense to add // the custom logic in this class (or subclass), than to handle all of it in the sink // definition. - /** Gets the `content` argument of a `call` to the constructor */ ControlFlowNode getContentArg(CallNode call) { none() } @@ -60,7 +59,7 @@ class DjangoContentResponseClass extends ClassValue { } /** A class that is a Django Response, and is vulnerable to XSS. */ -class DjangoXSSVulnerableResponseClass extends DjangoContentResponseClass{ +class DjangoXSSVulnerableResponseClass extends DjangoContentResponseClass { DjangoXSSVulnerableResponseClass() { // We want to avoid FPs on subclasses that are not exposed to XSS, for example `JsonResponse`. // The easiest way is to disregard any subclass that has a special `__init__` method.