Skip to content

Commit 57b3b2b

Browse files
Add qldoc + exclude empty methods
1 parent 582c4a7 commit 57b3b2b

File tree

1 file changed

+28
-3
lines changed

1 file changed

+28
-3
lines changed

csharp/ql/lib/semmle/code/csharp/security/auth/MissingFunctionLevelAccessControlQuery.qll

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,12 @@ import semmle.code.csharp.frameworks.microsoft.AspNetCore
55
import semmle.code.csharp.frameworks.system.web.UI
66
import semmle.code.asp.WebConfig
77

8+
/** A method representing an action for a web endpoint. */
89
abstract class ActionMethod extends Method {
10+
/**
11+
* Gets a string that can indicate what this method does to determine if it should have an auth check;
12+
* such as its method name, class name, or file path.
13+
*/
914
string getADescription() {
1015
result =
1116
[
@@ -14,23 +19,31 @@ abstract class ActionMethod extends Method {
1419
]
1520
}
1621

22+
/** Holds if this method may need an authorization check. */
1723
predicate needsAuth() {
1824
this.getADescription()
1925
.regexpReplaceAll("([a-z])([A-Z])", "$1_$2")
26+
// separate camelCase words
2027
.toLowerCase()
2128
.regexpMatch(".*(edit|delete|modify|admin|superuser).*")
2229
}
2330

31+
/** Gets a callable for which if it contains an auth check, this method should be considered authenticated. */
2432
Callable getAnAuthorizingCallable() { result = this }
2533

34+
/**
35+
* Gets a possible url route that could refer to this action,
36+
* which would be covered by `<location>` configurations specifying a prefix of it.
37+
*/
2638
string getARoute() { result = this.getDeclaringType().getFile().getRelativePath() }
2739
}
2840

41+
/** An action method in the MVC framework. */
2942
private class MvcActionMethod extends ActionMethod {
3043
MvcActionMethod() { this = any(MicrosoftAspNetCoreMvcController c).getAnActionMethod() }
31-
// override string getARoute() { none() }
3244
}
3345

46+
/** An action method on a subclass of `System.Web.UI.Page`. */
3447
private class WebFormActionMethod extends ActionMethod {
3548
WebFormActionMethod() {
3649
this.getDeclaringType().getBaseClass*() instanceof SystemWebUIPageClass and
@@ -56,6 +69,11 @@ private class WebFormActionMethod extends ActionMethod {
5669
}
5770
}
5871

72+
/**
73+
* Holds if `virtualRoute` is a URL path
74+
* that can map to the corresponding `physicalRoute` filepath
75+
* through a call to `MapPageRoute`
76+
*/
5977
private predicate virtualRouteMapping(string virtualRoute, string physicalRoute) {
6078
exists(MethodCall mapPageRouteCall, StringLiteral virtualLit, StringLiteral physicalLit |
6179
mapPageRouteCall
@@ -69,6 +87,7 @@ private predicate virtualRouteMapping(string virtualRoute, string physicalRoute)
6987
)
7088
}
7189

90+
/** Holds if the filepath `route` can refer to `actual` after expanding a '~". */
7291
bindingset[route, actual]
7392
private predicate physicalRouteMatches(string route, string actual) {
7493
route = actual
@@ -103,16 +122,20 @@ predicate hasAuthViaCode(ActionMethod m) {
103122
)
104123
}
105124

125+
/** An `<authorization>` XML element that */
106126
class AuthorizationXmlElement extends XmlElement {
107127
AuthorizationXmlElement() {
108128
this.getParent() instanceof SystemWebXmlElement and
109129
this.getName().toLowerCase() = "authorization"
110130
}
111131

132+
/** Holds if this element has a `<deny>` element to deny access to a resource. */
112133
predicate hasDenyElement() { this.getAChild().getName().toLowerCase() = "deny" }
113134

135+
/** Gets the physical filepath of this element. */
114136
string getPhysicalPath() { result = this.getFile().getParentContainer().getRelativePath() }
115137

138+
/** Gets the path specified by a `<location>` tag containing this element, if any. */
116139
string getLocationTagPath() {
117140
exists(LocationXmlElement loc, XmlAttribute path |
118141
loc = this.getParent().(SystemWebXmlElement).getParent() and
@@ -122,6 +145,7 @@ class AuthorizationXmlElement extends XmlElement {
122145
)
123146
}
124147

148+
/** Gets a route prefix that this configuration can refer to. */
125149
string getARoute() {
126150
result = this.getLocationTagPath()
127151
or
@@ -134,7 +158,6 @@ class AuthorizationXmlElement extends XmlElement {
134158

135159
/**
136160
* Holds if the given action has an xml `authorization` tag that refers to it.
137-
* TODO: Currently only supports physical paths, however virtual paths defined by `AddRoute` can also be used.
138161
*/
139162
predicate hasAuthViaXml(ActionMethod m) {
140163
exists(AuthorizationXmlElement el, string rest |
@@ -143,6 +166,7 @@ predicate hasAuthViaXml(ActionMethod m) {
143166
)
144167
}
145168

169+
/** Holds if the given action has an `Authorize` attribute. */
146170
predicate hasAuthViaAttribute(ActionMethod m) {
147171
[m.getAnAttribute(), m.getDeclaringType().getAnAttribute()]
148172
.getType()
@@ -154,5 +178,6 @@ predicate missingAuth(ActionMethod m) {
154178
m.needsAuth() and
155179
not hasAuthViaCode(m) and
156180
not hasAuthViaXml(m) and
157-
not hasAuthViaAttribute(m)
181+
not hasAuthViaAttribute(m) and
182+
exists(m.getBody().getAChildStmt()) // exclude empty methods
158183
}

0 commit comments

Comments
 (0)