Skip to content

Commit 3b3da9d

Browse files
pgomulkapgomulka
authored
Validate Accept and Content-Type header for compatible API (#54103)
Adding validation of Accept And Content-Type headers. The idea is to verify combinations of presence and values of both headers depending if a request has a content, headers are versioned (type/vnd.elasticsearch+subtype;compatible-with) . It also changes the way media type is parsed (previously always assuming application as a type i.e application/json) It should expect different types like text - used in SQL Not adding a compatible headers for _cat api. These in order to return a default text do not expect Accept header (Content-type is not needed as content is not present) See #53228 (comment) for more context
1 parent abb657e commit 3b3da9d

File tree

16 files changed

+478
-61
lines changed

16 files changed

+478
-61
lines changed

‎libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentType.java‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ public XContent xContent() {
117117
};
118118

119119
private static final Pattern COMPATIBLE_API_HEADER_PATTERN = Pattern.compile(
120-
"application/(vnd.elasticsearch\\+)?([^;]+)(\\s*;\\s*compatible-with=(\\d+))?",
120+
"(application|text)/(vnd.elasticsearch\\+)?([^;]+)(\\s*;\\s*compatible-with=(\\d+))?",
121121
Pattern.CASE_INSENSITIVE);
122122

123123
/**
@@ -126,7 +126,9 @@ public XContent xContent() {
126126
* also supports a wildcard accept for {@code application/*}. This method can be used to parse the {@code Accept} HTTP header or a
127127
* format query string parameter. This method will return {@code null} if no match is found
128128
*/
129-
public static XContentType fromMediaTypeOrFormat(String mediaType) {
129+
public static XContentType fromMediaTypeOrFormat(String mediaTypeHeaderValue) {
130+
String mediaType = parseMediaType(mediaTypeHeaderValue);
131+
130132
if (mediaType == null) {
131133
return null;
132134
}
@@ -136,7 +138,7 @@ public static XContentType fromMediaTypeOrFormat(String mediaType) {
136138
}
137139
}
138140
final String lowercaseMediaType = mediaType.toLowerCase(Locale.ROOT);
139-
if (lowercaseMediaType.startsWith("application/*")) {
141+
if (lowercaseMediaType.startsWith("application/*") || lowercaseMediaType.equals("*/*")) {
140142
return JSON;
141143
}
142144

@@ -165,11 +167,12 @@ public static XContentType fromMediaType(String mediaTypeHeaderValue) {
165167
return null;
166168
}
167169

170+
//public scope needed for text formats hack
168171
public static String parseMediaType(String mediaType) {
169172
if (mediaType != null) {
170173
Matcher matcher = COMPATIBLE_API_HEADER_PATTERN.matcher(mediaType);
171174
if (matcher.find()) {
172-
return "application/" + matcher.group(2).toLowerCase(Locale.ROOT);
175+
return (matcher.group(1) + "/" + matcher.group(3)).toLowerCase(Locale.ROOT);
173176
}
174177
}
175178

@@ -179,9 +182,9 @@ public static String parseMediaType(String mediaType) {
179182
public static String parseVersion(String mediaType){
180183
if(mediaType != null){
181184
Matcher matcher = COMPATIBLE_API_HEADER_PATTERN.matcher(mediaType);
182-
if (matcher.find() && "vnd.elasticsearch+".equalsIgnoreCase(matcher.group(1))) {
185+
if (matcher.find() && "vnd.elasticsearch+".equalsIgnoreCase(matcher.group(2))) {
183186

184-
return matcher.group(4);
187+
return matcher.group(5);
185188
}
186189
}
187190
return null;

‎modules/rest-compatibility/src/main/java/org/elasticsearch/rest/compat/version7/RestGetActionV7.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,12 @@ public class RestGetActionV7 extends RestGetAction {
3737
private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(RestGetAction.class));
3838
static final String TYPES_DEPRECATION_MESSAGE = "[types removal] Specifying types in "
3939
+ "document get requests is deprecated, use the /{index}/_doc/{id} endpoint instead.";
40+
4041
@Override
4142
public String getName() {
4243
return "document_get_action_v7";
4344
}
4445

45-
4646
@Override
4747
public List<Route> routes() {
4848
assert Version.CURRENT.major == 8 : "REST API compatibility for version 7 is only supported on version 8";

‎modules/rest-compatibility/src/main/java/org/elasticsearch/rest/compat/version7/RestIndexActionV7.java‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ public boolean compatibilityRequired() {
7474
}
7575

7676
public static class CompatibleCreateHandler extends RestIndexAction.CreateHandler {
77+
7778
@Override
7879
public String getName() {
7980
return "document_create_action_v7";
@@ -100,15 +101,16 @@ public boolean compatibilityRequired() {
100101
}
101102

102103
public static final class CompatibleAutoIdHandler extends RestIndexAction.AutoIdHandler {
103-
@Override
104-
public String getName() {
105-
return "document_create_action_auto_id_v7";
106-
}
107104

108105
public CompatibleAutoIdHandler(Supplier<DiscoveryNodes> nodesInCluster) {
109106
super(nodesInCluster);
110107
}
111108

109+
@Override
110+
public String getName() {
111+
return "document_create_action_auto_id_v7";
112+
}
113+
112114
@Override
113115
public List<Route> routes() {
114116
return singletonList(new Route(POST, "/{index}/{type}"));

‎modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,4 +101,27 @@ public void testInvalidHeaderValue() throws IOException {
101101
assertThat(map.get("type"), equalTo("content_type_header_exception"));
102102
assertThat(map.get("reason"), equalTo("java.lang.IllegalArgumentException: invalid Content-Type header []"));
103103
}
104+
105+
public void testInvalidHeaderCombinations() throws IOException {
106+
final Request request = new Request("GET", "/_cluster/settings");
107+
final RequestOptions.Builder options = request.getOptions().toBuilder();
108+
options.addHeader("Content-Type", "application/vnd.elasticsearch+json;compatible-with=7");
109+
options.addHeader("Accept", "application/vnd.elasticsearch+json;compatible-with=8");
110+
request.setOptions(options);
111+
request.setJsonEntity("{\"transient\":{\"search.*\":null}}");
112+
113+
final ResponseException e = expectThrows(ResponseException.class, () -> client().performRequest(request));
114+
final Response response = e.getResponse();
115+
assertThat(response.getStatusLine().getStatusCode(), equalTo(400));
116+
final ObjectPath objectPath = ObjectPath.createFromResponse(response);
117+
final Map<String, Object> map = objectPath.evaluate("error");
118+
assertThat(map.get("type"), equalTo("compatible_api_headers_combination_exception"));
119+
assertThat(map.get("reason"), equalTo("Content-Type and Accept headers have to match when content is present. " +
120+
"Accept=application/vnd.elasticsearch+json;compatible-with=8 " +
121+
"Content-Type=application/vnd.elasticsearch+json;compatible-with=7 " +
122+
"hasContent=true " +
123+
"path=/_cluster/settings " +
124+
"params={} " +
125+
"method=GET"));
126+
}
104127
}

‎qa/rest-compat-tests/src/main/java/org/elasticsearch/rest/compat/AbstractCompatRestTest.java‎

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import java.util.HashMap;
3434
import java.util.List;
3535
import java.util.Map;
36+
import java.util.function.Consumer;
3637
import java.util.stream.StreamSupport;
3738

3839
/**
@@ -43,7 +44,6 @@ protected AbstractCompatRestTest(@Name("yaml") ClientYamlTestCandidate testCandi
4344
super(testCandidate);
4445
}
4546

46-
4747
private static final Logger staticLogger = LogManager.getLogger(AbstractCompatRestTest.class);
4848

4949
public static final String COMPAT_TESTS_PATH = "/rest-api-spec/test-compat";
@@ -66,38 +66,48 @@ public static Iterable<Object[]> createParameters() throws Exception {
6666
});
6767
finalTestCandidates.add(testCandidates.toArray());
6868
}
69-
localCandidates.keySet().forEach(lc -> finalTestCandidates.add(new Object[]{lc}));
69+
localCandidates.keySet().forEach(lc -> finalTestCandidates.add(new Object[] { lc }));
7070
return finalTestCandidates;
7171
}
7272

73-
7473
private static void mutateTestCandidate(ClientYamlTestCandidate testCandidate) {
75-
testCandidate.getTestSection().getExecutableSections().stream().filter(s -> s instanceof DoSection).forEach(ds -> {
74+
testCandidate.getSetupSection().getExecutableSections().stream().filter(s -> s instanceof DoSection).forEach(updateDoSection());
75+
testCandidate.getTestSection().getExecutableSections().stream().filter(s -> s instanceof DoSection).forEach(updateDoSection());
76+
}
77+
78+
private static Consumer<? super ExecutableSection> updateDoSection() {
79+
return ds -> {
7680
DoSection doSection = (DoSection) ds;
77-
//TODO: be more selective here
81+
// TODO: be more selective here
7882
doSection.setIgnoreWarnings(true);
7983

8084
String compatibleHeader = createCompatibleHeader();
81-
//TODO decide which one to use - Accept or Content-Type
82-
doSection.getApiCallSection()
83-
.addHeaders(Map.of(
84-
CompatibleConstants.COMPATIBLE_HEADER, compatibleHeader,
85-
"Content-Type", compatibleHeader
86-
));
87-
});
85+
// for cat apis accept headers would break tests which expect txt response
86+
if (doSection.getApiCallSection().getApi().startsWith("cat") == false) {
87+
doSection.getApiCallSection()
88+
.addHeaders(
89+
Map.of(
90+
CompatibleConstants.COMPATIBLE_ACCEPT_HEADER,
91+
compatibleHeader,
92+
CompatibleConstants.COMPATIBLE_CONTENT_TYPE_HEADER,
93+
compatibleHeader
94+
)
95+
);
96+
}
97+
98+
};
8899
}
89100

90101
private static String createCompatibleHeader() {
91102
return "application/vnd.elasticsearch+json;compatible-with=" + CompatibleConstants.COMPATIBLE_VERSION;
92103
}
93104

94-
95105
private static Map<ClientYamlTestCandidate, ClientYamlTestCandidate> getLocalCompatibilityTests() throws Exception {
96-
Iterable<Object[]> candidates =
97-
ESClientYamlSuiteTestCase.createParameters(ExecutableSection.XCONTENT_REGISTRY, COMPAT_TESTS_PATH);
106+
Iterable<Object[]> candidates = ESClientYamlSuiteTestCase.createParameters(ExecutableSection.XCONTENT_REGISTRY, COMPAT_TESTS_PATH);
98107
Map<ClientYamlTestCandidate, ClientYamlTestCandidate> localCompatibilityTests = new HashMap<>();
99108
StreamSupport.stream(candidates.spliterator(), false)
100-
.flatMap(Arrays::stream).forEach(o -> localCompatibilityTests.put((ClientYamlTestCandidate) o, (ClientYamlTestCandidate) o));
109+
.flatMap(Arrays::stream)
110+
.forEach(o -> localCompatibilityTests.put((ClientYamlTestCandidate) o, (ClientYamlTestCandidate) o));
101111
return localCompatibilityTests;
102112
}
103113
}

‎server/src/main/java/org/elasticsearch/Version.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -342,6 +342,10 @@ public boolean isCompatible(Version version) {
342342
return compatible;
343343
}
344344

345+
public static Version minimumRestCompatibilityVersion(){
346+
return Version.V_7_0_0;
347+
}
348+
345349
@SuppressForbidden(reason = "System.out.*")
346350
public static void main(String[] args) {
347351
final String versionOutput = String.format(

‎server/src/main/java/org/elasticsearch/http/AbstractHttpServerTransport.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,9 @@ private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChan
352352
} catch (final RestRequest.BadParameterException e) {
353353
badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e);
354354
innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel);
355+
} catch (final RestRequest.CompatibleApiHeadersCombinationException e){
356+
badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e);
357+
innerRestRequest = RestRequest.requestNoValidation(xContentRegistry, httpRequest, httpChannel);
355358
}
356359
restRequest = innerRestRequest;
357360
}
@@ -384,12 +387,11 @@ private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChan
384387
}
385388

386389
private RestRequest requestWithoutContentTypeHeader(HttpRequest httpRequest, HttpChannel httpChannel, Exception badRequestCause) {
387-
HttpRequest httpRequestWithoutContentType = httpRequest.removeHeader("Content-Type");
388390
try {
389-
return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel);
391+
return RestRequest.requestWithoutContentType(xContentRegistry, httpRequest, httpChannel);
390392
} catch (final RestRequest.BadParameterException e) {
391393
badRequestCause.addSuppressed(e);
392-
return RestRequest.requestWithoutParameters(xContentRegistry, httpRequestWithoutContentType, httpChannel);
394+
return RestRequest.requestNoValidation(xContentRegistry, httpRequest, httpChannel);
393395
}
394396
}
395397
}

‎server/src/main/java/org/elasticsearch/rest/CompatibleConstants.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,9 @@ public class CompatibleConstants {
2626
/**
2727
* TODO revisit when https://github.com/elastic/elasticsearch/issues/52370 is resolved
2828
*/
29-
public static final String COMPATIBLE_HEADER = "Accept";
29+
public static final String COMPATIBLE_ACCEPT_HEADER = "Accept";
30+
public static final String COMPATIBLE_CONTENT_TYPE_HEADER = "Content-Type";
3031
public static final String COMPATIBLE_PARAMS_KEY = "Compatible-With";
31-
public static final String COMPATIBLE_VERSION = "" + (Version.CURRENT.major - 1);
32+
public static final String COMPATIBLE_VERSION = String.valueOf(Version.minimumRestCompatibilityVersion().major);
3233

3334
}

0 commit comments

Comments
 (0)