Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reject bulk requests with invalid actions #5302

Merged
merged 3 commits into from
Nov 21, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
### Fixed
- Fix 'org.apache.hc.core5.http.ParseException: Invalid protocol version' under JDK 16+ ([#4827](https://github.com/opensearch-project/OpenSearch/pull/4827))
- Fixed compression support for h2c protocol ([#4944](https://github.com/opensearch-project/OpenSearch/pull/4944))
- Add jvm option to allow security manager ([#5194](https://github.com/opensearch-project/OpenSearch/pull/5194))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be included in the CHANGELOG as per the guidelines, removing it.

- Reject bulk requests with invalid actions ([#5299](https://github.com/opensearch-project/OpenSearch/issues/5299))

### Security

## [Unreleased 2.x]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,33 @@ public final class BulkRequestParser {
private static final ParseField IF_PRIMARY_TERM = new ParseField("if_primary_term");
private static final ParseField REQUIRE_ALIAS = new ParseField(DocWriteRequest.REQUIRE_ALIAS);

private enum Action {
adnapibar marked this conversation as resolved.
Show resolved Hide resolved
CREATE,
DELETE,
INDEX,
UPDATE;

private static final Map<String, Action> VALID_ACTIONS = Map.of(
"create",
CREATE,
"delete",
DELETE,
"index",
INDEX,
"update",
UPDATE
);

static Action of(String name, int line) {
if (name != null && VALID_ACTIONS.containsKey(name)) {
return VALID_ACTIONS.get(name);
}
throw new IllegalArgumentException(
"Unknown action line [" + line + "], expected one of [create, delete, index, update]" + " but found [" + name + "]"
adnapibar marked this conversation as resolved.
Show resolved Hide resolved
);
}
}

private static int findNextMarker(byte marker, int from, BytesReference data) {
final int res = data.indexOf(marker, from);
if (res != -1) {
Expand Down Expand Up @@ -176,7 +203,7 @@ public void parse(
+ "]"
);
}
String action = parser.currentName();
Action action = Action.of(parser.currentName(), line);
Copy link
Member

@andrross andrross Nov 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could maybe refactor this as Action.consume(parser, line) so that you can handle the failure case above in the same place. I think it currently prints "...expected FIELD_NAME but found [x]" and that could be improved as well to say the "expected one of ..." thing.

Feel free to think of a better name than "consume"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consume sounds more like it doesn't return anything, let me think of a better name.
the error message does say ... expected one of [create, delete, index, update] but found ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, your new error message says that. I was suggesting to make it say that for the case on line 195 where a non-field name is encountered as well.

Action.parse might work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On line 195, the check is the type of token encountered, so I think we can keep the error message as is.


String index = defaultIndex;
String id = null;
Expand Down Expand Up @@ -272,7 +299,7 @@ public void parse(
);
}

if ("delete".equals(action)) {
if (action == Action.DELETE) {
deleteRequestConsumer.accept(
new DeleteRequest(index).id(id)
.routing(routing)
Expand All @@ -290,7 +317,7 @@ public void parse(

// we use internalAdd so we don't fork here, this allows us not to copy over the big byte array to small chunks
// of index request.
if ("index".equals(action)) {
if (action == Action.INDEX) {
if (opType == null) {
indexRequestConsumer.accept(
new IndexRequest(index).id(id)
Expand All @@ -317,7 +344,7 @@ public void parse(
.setRequireAlias(requireAlias)
);
}
} else if ("create".equals(action)) {
} else if (action == Action.CREATE) {
indexRequestConsumer.accept(
new IndexRequest(index).id(id)
.routing(routing)
Expand All @@ -330,7 +357,7 @@ public void parse(
.source(sliceTrimmingCarriageReturn(data, from, nextMarker, xContentType), xContentType)
.setRequireAlias(requireAlias)
);
} else if ("update".equals(action)) {
} else if (action == Action.UPDATE) {
if (version != Versions.MATCH_ANY || versionType != VersionType.INTERNAL) {
throw new IllegalArgumentException(
"Update requests do not support versioning. " + "Please use `if_seq_no` and `if_primary_term` instead"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -234,4 +234,27 @@ public void testParseDeduplicatesParameterStrings() throws IOException {
assertSame(first.getPipeline(), second.getPipeline());
assertSame(first.routing(), second.routing());
}

public void testFailOnUnsupportedAction() {
BytesArray request = new BytesArray("{ \"baz\":{ \"_id\": \"bar\" } }\n{}\n");
BulkRequestParser parser = new BulkRequestParser();

IllegalArgumentException ex = expectThrows(
IllegalArgumentException.class,
() -> parser.parse(
request,
"foo",
null,
null,
null,
true,
false,
XContentType.JSON,
req -> fail(),
req -> fail(),
req -> fail()
)
);
assertEquals("Unknown action line [1], expected one of [create, delete, index, update] but found [baz]", ex.getMessage());
}
}