Skip to content

Commit aa8621b

Browse files
seratchseratch
authored
Fix #1021 bolt-servlet improperly returns 200 OK with empty string to invalid requests (#1022)
1 parent 69be08d commit aa8621b

File tree

10 files changed

+413
-18
lines changed

10 files changed

+413
-18
lines changed

‎bolt-http4k/src/test/java/test_locally/Http4kSlackAppTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ public void slackEvents_commandNotFound() {
139139

140140
@Test
141141
public void workWithEmptyOrNullResponseBody() {
142-
Response expected = Response.Companion.create(Status.OK).header("Content-Type", "plain/text").body("");
142+
Response expected = Response.Companion.create(Status.OK).header("Content-Type", "text/plain").body("");
143143
Response response = simpleSlackApp.invoke(slackEventRequest("command=do-nothing"));
144144
assertThat(response, equalTo(expected));
145145
}

‎bolt-jakarta-servlet/src/main/java/com/slack/api/bolt/jakarta_servlet/SlackAppServlet.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I
4141
resp.setContentType("application/json");
4242
resp.getWriter().write("{\"error\":\"Something is wrong\"}");
4343
}
44+
} else {
45+
adapter.writeResponse(resp, Response.builder().statusCode(400).body("Invalid Request").build());
4446
}
4547
}
4648
}

‎bolt-jakarta-servlet/src/test/java/test_locally/servlet_test/InvalidRequestPatternTest.java‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
package test_locally.servlet_test;
2+
3+
import com.slack.api.app_backend.SlackSignature;
4+
import com.slack.api.bolt.App;
5+
import com.slack.api.bolt.AppConfig;
6+
import com.slack.api.bolt.jakarta_servlet.SlackAppServlet;
7+
import com.slack.api.bolt.middleware.builtin.RequestVerification;
8+
import jakarta.servlet.Servlet;
9+
import jakarta.servlet.annotation.WebServlet;
10+
import lombok.extern.slf4j.Slf4j;
11+
import org.eclipse.jetty.http.HttpTester;
12+
import org.eclipse.jetty.servlet.ServletTester;
13+
import org.junit.After;
14+
import org.junit.Before;
15+
import org.junit.Test;
16+
17+
import java.util.Arrays;
18+
19+
import static org.hamcrest.CoreMatchers.*;
20+
import static org.hamcrest.MatcherAssert.assertThat;
21+
22+
@Slf4j
23+
public class InvalidRequestPatternTest {
24+
25+
private final String secret = "foo-bar-baz";
26+
private final SlackSignature.Generator generator = new SlackSignature.Generator(secret);
27+
private AppConfig appConfig = AppConfig.builder().singleTeamBotToken("xoxb-").signingSecret(secret).build();
28+
private App app;
29+
private Servlet webApp;
30+
31+
@Before
32+
public void setUp() {
33+
// https://api.slack.com/docs/verifying-requests-from-slack
34+
String signingSecret = appConfig.getSigningSecret();
35+
if (signingSecret == null || signingSecret.trim().isEmpty()) {
36+
// This is just a random value to avoid SlackSignature.Generator's initialization error.
37+
// When this App runs through Socket Mode connections, it skips request signature verification.
38+
signingSecret = "---";
39+
}
40+
SlackSignature.Verifier verifier = new SlackSignature.Verifier(new SlackSignature.Generator(signingSecret));
41+
RequestVerification requestVerification = new RequestVerification(verifier);
42+
app = new App(appConfig, Arrays.asList(
43+
requestVerification
44+
));
45+
webApp = new SlackWebApp(app);
46+
app.start();
47+
}
48+
49+
@After
50+
public void tearDown() {
51+
app.stop();
52+
}
53+
54+
@WebServlet(urlPatterns = "/")
55+
public static class SlackWebApp extends SlackAppServlet {
56+
public SlackWebApp(App app) {
57+
super(app);
58+
}
59+
}
60+
61+
@Test
62+
public void valid() throws Exception {
63+
ServletTester tester = TestUtils.getServletTester(webApp);
64+
HttpTester.Request request = TestUtils.prepareRequest();
65+
66+
String requestBody = "{\n" +
67+
" \"token\": \"fixed-value\",\n" +
68+
" \"challenge\": \"challenge-value\",\n" +
69+
" \"type\": \"url_verification\"\n" +
70+
"}";
71+
request.setContent(requestBody);
72+
String timestamp = String.valueOf(System.currentTimeMillis() / 1000);
73+
request.setHeader(SlackSignature.HeaderNames.X_SLACK_REQUEST_TIMESTAMP, timestamp);
74+
request.setHeader(SlackSignature.HeaderNames.X_SLACK_SIGNATURE, generator.generate(timestamp, requestBody));
75+
request.setHeader("Content-Type", "application/json");
76+
77+
HttpTester.Response response = HttpTester.parseResponse(tester.getResponses(request.generate()));
78+
79+
assertThat(response.getStatus(), is(equalTo(200)));
80+
assertThat(response.getContent(), is(equalTo("challenge-value")));
81+
assertThat(response.get("Content-Type"), is(startsWith("text/plain;charset=ISO-8859-1")));
82+
}
83+
84+
@Test
85+
public void nonsense() throws Exception {
86+
ServletTester tester = TestUtils.getServletTester(webApp);
87+
HttpTester.Request request = TestUtils.prepareRequest();
88+
89+
String requestBody = "{\n" +
90+
" \"token\": \"fixed-value\",\n" +
91+
" \"challenge\": \"challenge-value\",\n" +
92+
" \"type\": \"nonsense\"\n" +
93+
"}";
94+
request.setContent(requestBody);
95+
String timestamp = String.valueOf(System.currentTimeMillis() / 1000);
96+
request.setHeader(SlackSignature.HeaderNames.X_SLACK_REQUEST_TIMESTAMP, timestamp);
97+
request.setHeader(SlackSignature.HeaderNames.X_SLACK_SIGNATURE, generator.generate(timestamp, requestBody));
98+
request.setHeader("Content-Type", "application/json");
99+
100+
HttpTester.Response response = HttpTester.parseResponse(tester.getResponses(request.generate()));
101+
102+
assertThat(response.getStatus(), is(equalTo(400)));
103+
assertThat(response.getContent(), is(equalTo("Invalid Request")));
104+
assertThat(response.get("Content-Type"), is(startsWith("text/plain;charset=ISO-8859-1")));
105+
}
106+
107+
@Test
108+
public void invalidSignature() throws Exception {
109+
ServletTester tester = TestUtils.getServletTester(webApp);
110+
HttpTester.Request request = TestUtils.prepareRequest();
111+
112+
String requestBody = "{\n" +
113+
" \"token\": \"fixed-value\",\n" +
114+
" \"challenge\": \"challenge-value\",\n" +
115+
" \"type\": \"url_verification\"\n" +
116+
"}";
117+
request.setContent(requestBody);
118+
String timestamp = String.valueOf(System.currentTimeMillis() / 1000);
119+
request.setHeader(SlackSignature.HeaderNames.X_SLACK_REQUEST_TIMESTAMP, timestamp);
120+
SlackSignature.Generator differentGenerator = new SlackSignature.Generator("a different app's secret");
121+
request.setHeader(SlackSignature.HeaderNames.X_SLACK_SIGNATURE, differentGenerator.generate(timestamp, requestBody));
122+
request.setHeader("Content-Type", "application/json");
123+
124+
HttpTester.Response response = HttpTester.parseResponse(tester.getResponses(request.generate()));
125+
126+
assertThat(response.getStatus(), is(equalTo(401)));
127+
assertThat(response.getContent(), is(equalTo("{\"error\":\"invalid request\"}")));
128+
assertThat(response.get("Content-Type"), is(startsWith("application/json")));
129+
}
130+
}

‎bolt-jakarta-servlet/src/test/java/test_locally/servlet_test/SlashCommandTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ public void echo() throws Exception {
134134

135135
assertThat(response.getContent(), is(equalTo("")));
136136
assertThat(response.getStatus(), is(equalTo(200)));
137-
assertThat(response.get("Content-Type"), is(startsWith("plain/text")));
137+
assertThat(response.get("Content-Type"), is(startsWith("text/plain")));
138138
assertThat(echoCalls.get(), is(1));
139139

140140
verify(echoSenderMock, times(1)).send(Mockito.any(SlashCommandResponse.class));

‎bolt-servlet/src/main/java/com/slack/api/bolt/servlet/SlackAppServlet.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I
4141
resp.setContentType("application/json");
4242
resp.getWriter().write("{\"error\":\"Something is wrong\"}");
4343
}
44+
} else {
45+
adapter.writeResponse(resp, Response.builder().statusCode(400).body("Invalid Request").build());
4446
}
4547
}
4648
}

‎bolt-servlet/src/test/java/test_locally/servlet/SlackOAuthAppServletTest.java‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -26,20 +26,6 @@ public void instantiation() {
2626
assertEquals(app, servlet.getApp());
2727
}
2828

29-
String slashCommandPayload = "token=gIkuvaNzQIHg97ATvDxqgjtO" +
30-
"&team_id=T0001" +
31-
"&team_domain=example" +
32-
"&enterprise_id=E0001" +
33-
"&enterprise_name=Globular%20Construct%20Inc" +
34-
"&channel_id=C2147483705" +
35-
"&channel_name=test" +
36-
"&user_id=U2147483697" +
37-
"&user_name=Steve" +
38-
"&command=/weather" +
39-
"&text=94070" +
40-
"&response_url=https://hooks.slack.com/commands/1234/5678" +
41-
"&trigger_id=13345224609.738474920.8088930838d88f008e0";
42-
4329
static class SimpleServletInputStream extends ServletInputStream {
4430
final ByteArrayInputStream body;
4531

‎bolt-servlet/src/test/java/test_locally/servlet_test/InvalidRequestPatternTest.java‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
package test_locally.servlet_test;
2+
3+
import com.slack.api.app_backend.SlackSignature;
4+
import com.slack.api.bolt.App;
5+
import com.slack.api.bolt.AppConfig;
6+
import com.slack.api.bolt.middleware.builtin.RequestVerification;
7+
import com.slack.api.bolt.servlet.SlackAppServlet;
8+
import lombok.extern.slf4j.Slf4j;
9+
import org.eclipse.jetty.http.HttpTester;
10+
import org.eclipse.jetty.servlet.ServletTester;
11+
import org.junit.After;
12+
import org.junit.Before;
13+
import org.junit.Test;
14+
15+
import javax.servlet.Servlet;
16+
import javax.servlet.annotation.WebServlet;
17+
import java.util.Arrays;
18+
19+
import static org.hamcrest.CoreMatchers.*;
20+
import static org.hamcrest.MatcherAssert.assertThat;
21+
22+
@Slf4j
23+
public class InvalidRequestPatternTest {
24+
25+
private final String secret = "foo-bar-baz";
26+
private final SlackSignature.Generator generator = new SlackSignature.Generator(secret);
27+
private AppConfig appConfig = AppConfig.builder().singleTeamBotToken("xoxb-").signingSecret(secret).build();
28+
private App app;
29+
private Servlet webApp;
30+
31+
@Before
32+
public void setUp() {
33+
// https://api.slack.com/docs/verifying-requests-from-slack
34+
String signingSecret = appConfig.getSigningSecret();
35+
if (signingSecret == null || signingSecret.trim().isEmpty()) {
36+
// This is just a random value to avoid SlackSignature.Generator's initialization error.
37+
// When this App runs through Socket Mode connections, it skips request signature verification.
38+
signingSecret = "---";
39+
}
40+
SlackSignature.Verifier verifier = new SlackSignature.Verifier(new SlackSignature.Generator(signingSecret));
41+
RequestVerification requestVerification = new RequestVerification(verifier);
42+
app = new App(appConfig, Arrays.asList(
43+
requestVerification
44+
));
45+
webApp = new SlackWebApp(app);
46+
app.start();
47+
}
48+
49+
@After
50+
public void tearDown() {
51+
app.stop();
52+
}
53+
54+
@WebServlet(urlPatterns = "/")
55+
public static class SlackWebApp extends SlackAppServlet {
56+
public SlackWebApp(App app) {
57+
super(app);
58+
}
59+
}
60+
61+
@Test
62+
public void valid() throws Exception {
63+
ServletTester tester = TestUtils.getServletTester(webApp);
64+
HttpTester.Request request = TestUtils.prepareRequest();
65+
66+
String requestBody = "{\n" +
67+
" \"token\": \"fixed-value\",\n" +
68+
" \"challenge\": \"challenge-value\",\n" +
69+
" \"type\": \"url_verification\"\n" +
70+
"}";
71+
request.setContent(requestBody);
72+
String timestamp = String.valueOf(System.currentTimeMillis() / 1000);
73+
request.setHeader(SlackSignature.HeaderNames.X_SLACK_REQUEST_TIMESTAMP, timestamp);
74+
request.setHeader(SlackSignature.HeaderNames.X_SLACK_SIGNATURE, generator.generate(timestamp, requestBody));
75+
request.setHeader("Content-Type", "application/json");
76+
77+
HttpTester.Response response = HttpTester.parseResponse(tester.getResponses(request.generate()));
78+
79+
assertThat(response.getStatus(), is(equalTo(200)));
80+
assertThat(response.getContent(), is(equalTo("challenge-value")));
81+
assertThat(response.get("Content-Type"), is(startsWith("text/plain; charset=ISO-8859-1")));
82+
}
83+
84+
@Test
85+
public void nonsense() throws Exception {
86+
ServletTester tester = TestUtils.getServletTester(webApp);
87+
HttpTester.Request request = TestUtils.prepareRequest();
88+
89+
String requestBody = "{\n" +
90+
" \"token\": \"fixed-value\",\n" +
91+
" \"challenge\": \"challenge-value\",\n" +
92+
" \"type\": \"nonsense\"\n" +
93+
"}";
94+
request.setContent(requestBody);
95+
String timestamp = String.valueOf(System.currentTimeMillis() / 1000);
96+
request.setHeader(SlackSignature.HeaderNames.X_SLACK_REQUEST_TIMESTAMP, timestamp);
97+
request.setHeader(SlackSignature.HeaderNames.X_SLACK_SIGNATURE, generator.generate(timestamp, requestBody));
98+
request.setHeader("Content-Type", "application/json");
99+
100+
HttpTester.Response response = HttpTester.parseResponse(tester.getResponses(request.generate()));
101+
102+
assertThat(response.getStatus(), is(equalTo(400)));
103+
assertThat(response.getContent(), is(equalTo("Invalid Request")));
104+
assertThat(response.get("Content-Type"), is(startsWith("text/plain; charset=ISO-8859-1")));
105+
}
106+
107+
@Test
108+
public void invalidSignature() throws Exception {
109+
ServletTester tester = TestUtils.getServletTester(webApp);
110+
HttpTester.Request request = TestUtils.prepareRequest();
111+
112+
String requestBody = "{\n" +
113+
" \"token\": \"fixed-value\",\n" +
114+
" \"challenge\": \"challenge-value\",\n" +
115+
" \"type\": \"url_verification\"\n" +
116+
"}";
117+
request.setContent(requestBody);
118+
String timestamp = String.valueOf(System.currentTimeMillis() / 1000);
119+
request.setHeader(SlackSignature.HeaderNames.X_SLACK_REQUEST_TIMESTAMP, timestamp);
120+
SlackSignature.Generator differentGenerator = new SlackSignature.Generator("a different app's secret");
121+
request.setHeader(SlackSignature.HeaderNames.X_SLACK_SIGNATURE, differentGenerator.generate(timestamp, requestBody));
122+
request.setHeader("Content-Type", "application/json");
123+
124+
HttpTester.Response response = HttpTester.parseResponse(tester.getResponses(request.generate()));
125+
126+
assertThat(response.getStatus(), is(equalTo(401)));
127+
assertThat(response.getContent(), is(equalTo("{\"error\":\"invalid request\"}")));
128+
assertThat(response.get("Content-Type"), is(startsWith("application/json")));
129+
}
130+
}

‎bolt-servlet/src/test/java/test_locally/servlet_test/SlashCommandTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ public void echo() throws Exception {
134134

135135
assertThat(response.getContent(), is(equalTo("")));
136136
assertThat(response.getStatus(), is(equalTo(200)));
137-
assertThat(response.get("Content-Type"), is(startsWith("plain/text")));
137+
assertThat(response.get("Content-Type"), is(startsWith("text/plain")));
138138
assertThat(echoCalls.get(), is(1));
139139

140140
verify(echoSenderMock, times(1)).send(Mockito.any(SlashCommandResponse.class));

‎bolt/src/main/java/com/slack/api/bolt/response/Response.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ public class Response {
2323
@Builder.Default
2424
private Integer statusCode = 200;
2525
@Builder.Default
26-
private String contentType = "plain/text";
26+
private String contentType = "text/plain";
2727
@Builder.Default
2828
private Map<String, List<String>> headers = new HashMap<>();
2929
private String body;

0 commit comments

Comments
 (0)