Skip to content

Commit b3548a0

Browse files
hahwulhahwul
authored
Improve tagger coverage and accuracy (#1789)
1 parent 9d1706b commit b3548a0

6 files changed

Lines changed: 284 additions & 49 deletions

File tree

‎spec/unit_test/tagger/taggers/file_upload_spec.cr‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,5 +151,55 @@ describe "FileUploadTagger" do
151151
endpoint.tags.size.should eq(1)
152152
endpoint.tags[0].name.should eq("file_upload")
153153
end
154+
155+
it "tags PATCH endpoint with avatar body parameter" do
156+
tagger = FileUploadTagger.new(default_tagger_options)
157+
158+
endpoint = Endpoint.new("/users/me/avatar", "PATCH", [
159+
Param.new("avatar", "", "body"),
160+
])
161+
162+
tagger.perform([endpoint])
163+
164+
endpoint.tags.size.should eq(1)
165+
endpoint.tags[0].name.should eq("file_upload")
166+
end
167+
168+
it "tags multipart content-type header values" do
169+
tagger = FileUploadTagger.new(default_tagger_options)
170+
171+
endpoint = Endpoint.new("/api/profile", "POST", [
172+
Param.new("Content-Type", "multipart/form-data; boundary=abc", "header"),
173+
])
174+
175+
tagger.perform([endpoint])
176+
177+
endpoint.tags.size.should eq(1)
178+
endpoint.tags[0].name.should eq("file_upload")
179+
end
180+
181+
it "does not tag non-multipart content-type headers" do
182+
tagger = FileUploadTagger.new(default_tagger_options)
183+
184+
endpoint = Endpoint.new("/api/profile", "POST", [
185+
Param.new("Content-Type", "application/json", "header"),
186+
])
187+
188+
tagger.perform([endpoint])
189+
190+
endpoint.tags.size.should eq(0)
191+
end
192+
193+
it "does not tag GET media endpoints as uploads" do
194+
tagger = FileUploadTagger.new(default_tagger_options)
195+
196+
endpoint = Endpoint.new("/users/123/avatar", "GET", [
197+
Param.new("avatar", "", "query"),
198+
])
199+
200+
tagger.perform([endpoint])
201+
202+
endpoint.tags.size.should eq(0)
203+
end
154204
end
155205
end

‎spec/unit_test/tagger/taggers/jwt_spec.cr‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,5 +133,43 @@ describe "JwtTagger" do
133133
endpoint.tags.size.should eq(1)
134134
endpoint.tags[0].name.should eq("jwt")
135135
end
136+
137+
it "tags bearer authorization values without a second token parameter" do
138+
tagger = JwtTagger.new(default_tagger_options)
139+
140+
endpoint = Endpoint.new("/api/profile", "GET", [
141+
Param.new("Authorization", "Bearer eyJhbGciOiJIUzI1NiJ9.payload.signature", "header"),
142+
])
143+
144+
tagger.perform([endpoint])
145+
146+
endpoint.tags.size.should eq(1)
147+
endpoint.tags[0].name.should eq("jwt")
148+
end
149+
150+
it "tags auth routes with custom token header names" do
151+
tagger = JwtTagger.new(default_tagger_options)
152+
153+
endpoint = Endpoint.new("/login", "POST", [
154+
Param.new("x-api-token", "", "header"),
155+
])
156+
157+
tagger.perform([endpoint])
158+
159+
endpoint.tags.size.should eq(1)
160+
endpoint.tags[0].name.should eq("jwt")
161+
end
162+
163+
it "does not tag CSRF-only token parameters" do
164+
tagger = JwtTagger.new(default_tagger_options)
165+
166+
endpoint = Endpoint.new("/auth/session", "POST", [
167+
Param.new("csrf_token", "", "form"),
168+
])
169+
170+
tagger.perform([endpoint])
171+
172+
endpoint.tags.size.should eq(0)
173+
end
136174
end
137175
end

‎spec/unit_test/tagger/taggers/oauth_spec.cr‎

Lines changed: 61 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ describe "OAuthTagger" do
5757
it "tags with exactly 3 matching parameters" do
5858
tagger = OAuthTagger.new(default_tagger_options)
5959

60-
endpoint = Endpoint.new("/oauth/token", "POST", [
60+
endpoint = Endpoint.new("/api/integrations/token", "POST", [
6161
Param.new("grant_type", "client_credentials", "form"),
6262
Param.new("client_id", "my-app", "form"),
6363
Param.new("client_secret", "secret", "form"),
@@ -101,10 +101,9 @@ describe "OAuthTagger" do
101101
endpoint2.tags.size.should eq(0)
102102
end
103103

104-
it "is case-sensitive for parameter matching" do
104+
it "normalizes parameter names for matching" do
105105
tagger = OAuthTagger.new(default_tagger_options)
106106

107-
# OAuth parameter names are case-sensitive
108107
endpoint = Endpoint.new("/oauth/token", "POST", [
109108
Param.new("GRANT_TYPE", "authorization_code", "form"),
110109
Param.new("CODE", "abc123", "form"),
@@ -113,7 +112,65 @@ describe "OAuthTagger" do
113112

114113
tagger.perform([endpoint])
115114

116-
# Should not match because case doesn't match
115+
endpoint.tags.size.should eq(1)
116+
endpoint.tags[0].name.should eq("oauth")
117+
end
118+
119+
it "tags OAuth authorization endpoints with OIDC parameters" do
120+
tagger = OAuthTagger.new(default_tagger_options)
121+
122+
endpoint = Endpoint.new("/oauth2/authorize", "GET", [
123+
Param.new("response_type", "code", "query"),
124+
Param.new("client_id", "my-app", "query"),
125+
Param.new("redirect_uri", "https://example.com/callback", "query"),
126+
Param.new("scope", "openid profile", "query"),
127+
Param.new("state", "abc123", "query"),
128+
])
129+
130+
tagger.perform([endpoint])
131+
132+
endpoint.tags.size.should eq(1)
133+
endpoint.tags[0].name.should eq("oauth")
134+
end
135+
136+
it "tags OAuth token endpoints using PKCE verifier without client secret" do
137+
tagger = OAuthTagger.new(default_tagger_options)
138+
139+
endpoint = Endpoint.new("/oauth/token", "POST", [
140+
Param.new("grant-type", "authorization_code", "form"),
141+
Param.new("code_verifier", "pkce-secret", "form"),
142+
])
143+
144+
tagger.perform([endpoint])
145+
146+
endpoint.tags.size.should eq(1)
147+
endpoint.tags[0].name.should eq("oauth")
148+
end
149+
150+
it "does not tag generic token routes with weak OAuth parameters" do
151+
tagger = OAuthTagger.new(default_tagger_options)
152+
153+
endpoint = Endpoint.new("/api/token", "POST", [
154+
Param.new("code", "123456", "form"),
155+
Param.new("state", "ready", "form"),
156+
])
157+
158+
tagger.perform([endpoint])
159+
160+
endpoint.tags.size.should eq(0)
161+
end
162+
163+
it "does not tag non-OAuth endpoints with weak OAuth-like parameter names" do
164+
tagger = OAuthTagger.new(default_tagger_options)
165+
166+
endpoint = Endpoint.new("/api/promotions", "POST", [
167+
Param.new("code", "SPRING", "form"),
168+
Param.new("state", "published", "form"),
169+
Param.new("scope", "regional", "form"),
170+
])
171+
172+
tagger.perform([endpoint])
173+
117174
endpoint.tags.size.should eq(0)
118175
end
119176
end

‎src/tagger/taggers/file_upload.cr‎

Lines changed: 37 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,15 @@ require "../../models/tagger"
22
require "../../models/endpoint"
33

44
class FileUploadTagger < Tagger
5-
WORDS = ["file", "upload", "attachment", "document", "image", "multipart", "content-type", "filename", "content-disposition"]
5+
WORDS = Set{
6+
"file", "files", "upload", "attachment", "attachments", "document", "documents",
7+
"image", "images", "photo", "photos", "avatar", "media", "multipart",
8+
"content_type", "filename", "content_disposition",
9+
}
10+
11+
UPLOAD_PARAM_TYPES = Set{"file", "form", "body", "json"}
12+
UPLOAD_METHODS = Set{"POST", "PUT", "PATCH"}
13+
UPLOAD_PATH_PARTS = Set{"upload", "uploads", "attach", "attachment", "attachments", "import", "imports", "avatar", "photo", "photos", "media"}
614

715
def initialize(options : Hash(String, YAML::Any))
816
super
@@ -11,28 +19,40 @@ class FileUploadTagger < Tagger
1119

1220
def perform(endpoints : Array(Endpoint))
1321
endpoints.each do |endpoint|
14-
tmp_params = [] of String
15-
16-
endpoint.params.each do |param|
17-
tmp_params.push param.name.to_s.downcase
18-
end
19-
20-
# Check URL path for upload indicators
21-
url_lower = endpoint.url.downcase
22-
is_upload_url = url_lower.includes?("upload") || url_lower.includes?("attach") || url_lower.includes?("import")
22+
has_upload_param = endpoint.params.any? { |param| upload_param?(param) }
23+
is_upload_url = upload_url?(endpoint.url)
24+
is_upload_method = UPLOAD_METHODS.includes?(endpoint.method.upcase)
25+
has_multipart_header = endpoint.params.any? { |param| multipart_header?(param) }
2326

24-
words_set = Set.new(WORDS)
25-
tmp_params_set = Set.new(tmp_params)
26-
intersection = words_set & tmp_params_set
27-
28-
# Check that at least one parameter matches and method is POST/PUT or URL indicates upload
29-
is_upload_method = endpoint.method == "POST" || endpoint.method == "PUT"
30-
check = (intersection.size.to_i >= 1 && is_upload_method) || (is_upload_url && is_upload_method)
27+
check = is_upload_method && (has_upload_param || is_upload_url || has_multipart_header)
3128

3229
if check
3330
tag = Tag.new("file_upload", "File upload endpoint potentially vulnerable to unrestricted file upload, path traversal, or malicious file execution.", "FileUpload")
3431
endpoint.add_tag(tag)
3532
end
3633
end
3734
end
35+
36+
private def upload_param?(param : Param) : Bool
37+
name = normalize_param_name(param.name)
38+
return false unless UPLOAD_PARAM_TYPES.includes?(param.param_type)
39+
40+
WORDS.includes?(name) || name.ends_with?("_file") || name.ends_with?("_files")
41+
end
42+
43+
private def multipart_header?(param : Param) : Bool
44+
name = normalize_param_name(param.name)
45+
value = param.value.downcase
46+
(name == "content_type" && value.includes?("multipart/form-data")) ||
47+
(name == "content_disposition" && value.includes?("filename="))
48+
end
49+
50+
private def upload_url?(url : String) : Bool
51+
parts = url.downcase.split(/[\/\-_\.]+/).reject(&.empty?)
52+
parts.any? { |part| UPLOAD_PATH_PARTS.includes?(part) }
53+
end
54+
55+
private def normalize_param_name(name : String) : String
56+
name.downcase.tr("-", "_")
57+
end
3858
end

‎src/tagger/taggers/jwt.cr‎

Lines changed: 45 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,17 @@ require "../../models/tagger"
22
require "../../models/endpoint"
33

44
class JwtTagger < Tagger
5-
WORDS = ["token", "jwt", "bearer", "refresh_token", "access_token", "id_token", "authorization"]
5+
STRONG_NAMES = Set{
6+
"jwt", "bearer", "authorization", "access_token", "refresh_token", "id_token",
7+
"auth_token", "api_token", "x_api_token", "x_access_token",
8+
}
9+
10+
EXCLUDED_TOKEN_NAMES = Set{
11+
"csrf_token", "xsrf_token", "authenticity_token", "anti_csrf_token",
12+
"captcha_token", "recaptcha_token", "turnstile_token",
13+
}
14+
15+
AUTH_PATH_PARTS = Set{"auth", "authenticate", "authentication", "login", "signin", "sign_in", "token", "refresh", "jwt"}
616

717
def initialize(options : Hash(String, YAML::Any))
818
super
@@ -11,27 +21,46 @@ class JwtTagger < Tagger
1121

1222
def perform(endpoints : Array(Endpoint))
1323
endpoints.each do |endpoint|
14-
tmp_params = [] of String
15-
16-
endpoint.params.each do |param|
17-
tmp_params.push param.name.to_s.downcase
18-
end
24+
signals = endpoint.params.count { |param| jwt_signal?(param) }
25+
has_bearer_value = endpoint.params.any? { |param| bearer_or_jwt_value?(param.value) }
26+
is_auth_url = auth_url?(endpoint.url)
1927

20-
# Check URL path for JWT indicators
21-
url_lower = endpoint.url.downcase
22-
is_jwt_url = url_lower.includes?("/token") || url_lower.includes?("/auth") || url_lower.includes?("/jwt") || url_lower.includes?("/refresh")
23-
24-
words_set = Set.new(WORDS)
25-
tmp_params_set = Set.new(tmp_params)
26-
intersection = words_set & tmp_params_set
27-
28-
# Check that at least two parameters match or URL indicates JWT handling
29-
check = intersection.size.to_i >= 2 || (is_jwt_url && intersection.size.to_i >= 1)
28+
# Require either an unmistakable token value, multiple token/auth
29+
# signals, or an auth-like route plus a non-CSRF token parameter.
30+
check = has_bearer_value || signals >= 2 || (is_auth_url && signals >= 1)
3031

3132
if check
3233
tag = Tag.new("jwt", "JWT endpoint for token-based authentication, requiring validation of signature, expiration, and claims.", "JWT")
3334
endpoint.add_tag(tag)
3435
end
3536
end
3637
end
38+
39+
private def jwt_signal?(param : Param) : Bool
40+
name = normalize_param_name(param.name)
41+
return false if EXCLUDED_TOKEN_NAMES.includes?(name)
42+
return true if STRONG_NAMES.includes?(name)
43+
return true if name == "token"
44+
return true if name.ends_with?("_token") && !name.includes?("csrf") && !name.includes?("captcha")
45+
46+
false
47+
end
48+
49+
private def bearer_or_jwt_value?(value : String) : Bool
50+
value_lower = value.downcase
51+
return true if value_lower.starts_with?("bearer ")
52+
53+
# Compact JWTs are three base64url-ish segments. Keep this strict
54+
# enough to avoid tagging arbitrary dotted values.
55+
!!value.match(/\AeyJ[A-Za-z0-9_-]*\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]*\z/)
56+
end
57+
58+
private def normalize_param_name(name : String) : String
59+
name.downcase.tr("-", "_")
60+
end
61+
62+
private def auth_url?(url : String) : Bool
63+
parts = url.downcase.tr("-", "_").split(/[\/\.]+/).reject(&.empty?)
64+
parts.any? { |part| AUTH_PATH_PARTS.includes?(part) }
65+
end
3766
end

0 commit comments

Comments
 (0)