Skip to content

Commit 3a2a28d

Browse files
php-coderphp-coder
committed
Merge remote-tracking branch 'AleksSPb/gh251_suspicious_activities_data_truncation'
Fix #251
2 parents 4daf895 + 7c0ef64 commit 3a2a28d

File tree

3 files changed

+128
-9
lines changed

3 files changed

+128
-9
lines changed

‎src/main/java/ru/mystamps/web/entity/SuspiciousActivity.java

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,11 @@
3636
@Setter
3737
public class SuspiciousActivity {
3838

39-
public static final int PAGE_URL_LENGTH = 100;
40-
public static final int IP_LENGTH = 15;
41-
public static final int METHOD_LENGTH = 7;
39+
public static final int PAGE_URL_LENGTH = 100;
40+
public static final int METHOD_LENGTH = 7;
41+
public static final int IP_LENGTH = 15;
42+
public static final int REFERER_PAGE_LENGTH = 255;
43+
public static final int USER_AGENT_LENGTH = 255;
4244

4345
@Id
4446
@GeneratedValue
@@ -62,10 +64,10 @@ public class SuspiciousActivity {
6264
@Column(length = IP_LENGTH, nullable = false)
6365
private String ip;
6466

65-
@Column(name = "referer_page", nullable = false)
67+
@Column(name = "referer_page", length = REFERER_PAGE_LENGTH, nullable = false)
6668
private String refererPage;
6769

68-
@Column(name = "user_agent", nullable = false)
70+
@Column(name = "user_agent", length = USER_AGENT_LENGTH, nullable = false)
6971
private String userAgent;
7072

7173
}

‎src/main/java/ru/mystamps/web/service/SiteServiceImpl.java

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222
import org.apache.commons.lang3.StringUtils;
2323
import org.apache.commons.lang3.Validate;
2424

25+
import org.slf4j.Logger;
26+
import org.slf4j.LoggerFactory;
2527
import org.springframework.scheduling.annotation.Async;
2628
import org.springframework.transaction.annotation.Transactional;
2729

@@ -35,6 +37,8 @@
3537
@RequiredArgsConstructor
3638
public class SiteServiceImpl implements SiteService {
3739

40+
private static final Logger LOG = LoggerFactory.getLogger(SiteServiceImpl.class);
41+
3842
// see initiate-suspicious_activities_types-table changeset
3943
// in src/main/resources/liquibase/initial-state.xml
4044
private static final String PAGE_NOT_FOUND = "PageNotFound";
@@ -94,16 +98,48 @@ private void logEvent(
9498
activity.setType(activityType);
9599

96100
activity.setOccurredAt(date == null ? new Date() : date);
97-
activity.setPage(page);
101+
activity.setPage(abbreviatePage(page));
98102
activity.setMethod(method);
99103

100104
activity.setUser(user);
101105

102106
activity.setIp(StringUtils.defaultString(ip));
103-
activity.setRefererPage(StringUtils.defaultString(referer));
104-
activity.setUserAgent(StringUtils.defaultString(agent));
107+
activity.setRefererPage(abbreviateRefererPage(StringUtils.defaultString(referer)));
108+
activity.setUserAgent(abbreviateUserAgent(StringUtils.defaultString(agent)));
105109

106110
suspiciousActivities.add(activity);
107111
}
108112

113+
private static String abbreviatePage(String page) {
114+
return abbreviateIfLengthGreaterThan(page, SuspiciousActivity.PAGE_URL_LENGTH, "page");
115+
}
116+
117+
private static String abbreviateRefererPage(String referer) {
118+
// CheckStyle: ignore LineLength for next 1 lines
119+
return abbreviateIfLengthGreaterThan(referer, SuspiciousActivity.REFERER_PAGE_LENGTH, "referer_page");
120+
}
121+
122+
private static String abbreviateUserAgent(String agent) {
123+
// CheckStyle: ignore LineLength for next 1 lines
124+
return abbreviateIfLengthGreaterThan(agent, SuspiciousActivity.USER_AGENT_LENGTH, "user_agent");
125+
}
126+
127+
// CheckStyle: ignore LineLength for next 1 lines
128+
private static String abbreviateIfLengthGreaterThan(String text, int maxLength, String fieldName) {
129+
if (text == null || text.length() <= maxLength) {
130+
return text;
131+
}
132+
133+
// TODO(security): fix possible log injection
134+
LOG.warn(
135+
"Length of value for '{}' field ({}) exceeds max field size ({}): '{}'",
136+
fieldName,
137+
text.length(),
138+
maxLength,
139+
text
140+
);
141+
142+
return StringUtils.abbreviate(text, maxLength);
143+
}
144+
109145
}

‎src/test/groovy/ru/mystamps/web/service/SiteServiceImplTest.groovy

Lines changed: 82 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,21 @@ class SiteServiceImplTest extends Specification {
8989
return true
9090
})
9191
}
92-
92+
93+
def "logAboutAbsentPage() should pass abbreviated page when it's too long"() {
94+
given:
95+
String longPageUrl = '/long/url/' + ('x' * SuspiciousActivity.PAGE_URL_LENGTH)
96+
and:
97+
String expectedPageUrl = longPageUrl.take(SuspiciousActivity.PAGE_URL_LENGTH - 3) + '...'
98+
when:
99+
service.logAboutAbsentPage(longPageUrl, TEST_METHOD, null, null, null, null)
100+
then:
101+
1 * suspiciousActivityDao.add({ SuspiciousActivity activity ->
102+
assert activity?.page == expectedPageUrl
103+
return true
104+
})
105+
}
106+
93107
@Unroll
94108
def "logAboutAbsentPage() should pass method to dao"(String expectedMethod) {
95109
when:
@@ -155,6 +169,19 @@ class SiteServiceImplTest extends Specification {
155169
return true
156170
})
157171
}
172+
def "logAboutAbsentPage() should pass abbreviated referer when it's too long"() {
173+
given:
174+
String longRefererUrl = '/long/url/' + ('x' * SuspiciousActivity.REFERER_PAGE_LENGTH)
175+
and:
176+
String expectedRefererUrl = longRefererUrl.take(SuspiciousActivity.REFERER_PAGE_LENGTH - 3) + '...'
177+
when:
178+
service.logAboutAbsentPage(TEST_PAGE, TEST_METHOD, null, null, longRefererUrl, null)
179+
then:
180+
1 * suspiciousActivityDao.add({ SuspiciousActivity activity ->
181+
assert activity?.refererPage == expectedRefererUrl
182+
return true
183+
})
184+
}
158185

159186
def "logAboutAbsentPage() should pass empty string to dao for unknown referer"() {
160187
when:
@@ -175,6 +202,19 @@ class SiteServiceImplTest extends Specification {
175202
return true
176203
})
177204
}
205+
def "logAboutAbsentPage() should pass abbreviated user agent when it's too long"() {
206+
given:
207+
String longUserAgent = 'Mozilla/5.0 (Windows NT 6.1) AppleWebKit/' + ('x' * SuspiciousActivity.USER_AGENT_LENGTH)
208+
and:
209+
String expectedUserAgent = longUserAgent.take(SuspiciousActivity.USER_AGENT_LENGTH - 3) + '...'
210+
when:
211+
service.logAboutAbsentPage(TEST_PAGE, TEST_METHOD, null, null, null, longUserAgent)
212+
then:
213+
1 * suspiciousActivityDao.add({ SuspiciousActivity activity ->
214+
assert activity?.userAgent == expectedUserAgent
215+
return true
216+
})
217+
}
178218

179219
def "logAboutAbsentPage() should pass empty string to dao for unknown user agent"() {
180220
when:
@@ -247,6 +287,19 @@ class SiteServiceImplTest extends Specification {
247287
return true
248288
})
249289
}
290+
def "logAboutFailedAuthentication() should pass abbreviated page when it's too long"() {
291+
given:
292+
String longPageUrl = '/long/url/' + ('x' * SuspiciousActivity.PAGE_URL_LENGTH)
293+
and:
294+
String expectedPageUrl = longPageUrl.take(SuspiciousActivity.PAGE_URL_LENGTH - 3) + '...'
295+
when:
296+
service.logAboutFailedAuthentication(longPageUrl, TEST_METHOD, null, null, null, null, null)
297+
then:
298+
1 * suspiciousActivityDao.add({ SuspiciousActivity activity ->
299+
assert activity?.page == expectedPageUrl
300+
return true
301+
})
302+
}
250303

251304
def "logAboutFailedAuthentication() should pass null to dao for unknown user"() {
252305
when:
@@ -310,6 +363,20 @@ class SiteServiceImplTest extends Specification {
310363
})
311364
}
312365

366+
def "logAboutFailedAuthentication() should pass abbreviated referer when it's too long"() {
367+
given:
368+
String longRefererUrl = '/long/url/' + ('x' * SuspiciousActivity.REFERER_PAGE_LENGTH)
369+
and:
370+
String expectedRefererUrl = longRefererUrl.take(SuspiciousActivity.REFERER_PAGE_LENGTH - 3) + '...'
371+
when:
372+
service.logAboutFailedAuthentication(TEST_PAGE, TEST_METHOD, null, null, longRefererUrl, null, null)
373+
then:
374+
1 * suspiciousActivityDao.add({ SuspiciousActivity activity ->
375+
assert activity?.refererPage == expectedRefererUrl
376+
return true
377+
})
378+
}
379+
313380
def "logAboutFailedAuthentication() should pass user agent to dao"() {
314381
when:
315382
service.logAboutFailedAuthentication(TEST_PAGE, TEST_METHOD, null, null, null, TEST_USER_AGENT, null)
@@ -330,4 +397,18 @@ class SiteServiceImplTest extends Specification {
330397
})
331398
}
332399

400+
def "logAboutFailedAuthentication() should pass abbreviated user agent when it's too long"() {
401+
given:
402+
String longUserAgent = 'Mozilla/5.0 (Windows NT 6.1) AppleWebKit/' + ('x' * SuspiciousActivity.USER_AGENT_LENGTH)
403+
and:
404+
String expectedUserAgent = longUserAgent.take(SuspiciousActivity.USER_AGENT_LENGTH - 3) + '...'
405+
when:
406+
service.logAboutFailedAuthentication(TEST_PAGE, TEST_METHOD, null, null, null, longUserAgent, null)
407+
then:
408+
1 * suspiciousActivityDao.add({ SuspiciousActivity activity ->
409+
assert activity?.userAgent == expectedUserAgent
410+
return true
411+
})
412+
}
413+
333414
}

0 commit comments

Comments
 (0)