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

Inja transformation preserve escaped json #258

Merged
merged 9 commits into from
Jul 21, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -204,9 +204,9 @@ message TransformationTemplate {
InjaTemplate value = 2;
}

// Use this attribute to transform request/response headers. It consists of
// an array of string/template objects. Use this attribute to define multiple
// templates for a single header. Header template(s) defined here will be appended to any
// Use this attribute to transform request/response headers. It consists of
// an array of string/template objects. Use this attribute to define multiple
// templates for a single header. Header template(s) defined here will be appended to any
// existing headers with the same header name, not replace existing ones.
repeated HeaderToAppend headers_to_append = 10;

Expand Down Expand Up @@ -255,6 +255,11 @@ message TransformationTemplate {
}
// Use this field to set Dynamic Metadata.
repeated DynamicMetadataValue dynamic_metadata_values = 9;

// Use to render the output of a body transformation as JSON. This will cause
// rendered string values to be escaped in order to make valid JSON strings
bool render_body_as_json = 12;
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should rename this. perhaps this has similar utility for folks who want the body rendered for, say, yaml

Copy link
Contributor

Choose a reason for hiding this comment

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

say escape_characters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it. I renamed the field


}

// Defines an [Inja template](https://github.com/pantor/inja) that will be
Expand Down
5 changes: 4 additions & 1 deletion bazel/repository_locations.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ REPOSITORY_LOCATIONS = dict(
remote = "https://github.com/envoyproxy/envoy",
),
inja = dict(
commit = "5a18986825fc7e5d2916ff345c4369e4b6ea7122", # v3.4 + JSON Pointers
# Includes unmerged modifications for
# - JSON pointer syntax support
# - Allowing escaped strings
commit = "3aa95b8b58a525f86f79cb547bf937176c9cc7ff", # v3.4.0-patch1
jbohanon marked this conversation as resolved.
Show resolved Hide resolved
remote = "https://github.com/solo-io/inja", # solo-io fork including the changes
),
json = dict(
Expand Down
11 changes: 11 additions & 0 deletions changelog/v1.26.3-patch2/inja-escaped-body.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
changelog:
- type: DEPENDENCY_BUMP
dependencyOwner: solo-io
dependencyRepo: inja
dependencyTag: v3.4.0-patch2 # not yet released DO NOT MERGE
- type: NEW_FEATURE
issueLink: https://github.com/solo-io/solo-projects/issues/5155
resolvesIssue: false
description: >-
Inja callback `raw_string` added to permit fine-grained escaping of input strings.
Also, configuration added to escape all strings for a given transformation.
32 changes: 32 additions & 0 deletions source/extensions/filters/http/transformation/inja_transformer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,9 @@ TransformerInstance::TransformerInstance(ThreadLocal::Slot &tls, Envoy::Random::
env_.add_callback("replace_with_random", 2, [this](Arguments &args) {
return replace_with_random_callback(args);
});
env_.add_callback("raw_string", 1, [this](Arguments &args) {
return raw_string_callback(args);
});
}

json TransformerInstance::header_callback(const inja::Arguments &args) const {
Expand Down Expand Up @@ -343,6 +346,32 @@ std::string& TransformerInstance::random_for_pattern(const std::string& pattern)
return found->second;
}

json TransformerInstance::raw_string_callback(const inja::Arguments &args) const {
// inja::Arguments is a std::vector<const json *>, so we can get the json
// value from the args directly.
const auto& input = args.at(0);
nfuden marked this conversation as resolved.
Show resolved Hide resolved

// make sure to bail if we're not working with a raw string value
if(!input->is_string()) {
return input->get_ref<const std::string&>();
}

auto val = input->dump();
Comment on lines +356 to +361
Copy link
Contributor

Choose a reason for hiding this comment

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


// This block makes it such that a template must have surrounding " characters
// around the raw string. This is reasonable since we expect the value we get out of the
// context (body) to be placed in exactly as-is. HOWEVER, the behavior of the jinja
// filter is such that the quotes added by .dumps() are left in. For that reason,
// this callback is NOT named to_json to avoid confusion with that behavior.

// strip the leading and trailing " characters that are added by dump()
// if C++20 is adopted, val.starts_with and val.ends_with would clean this up a bit
val = val.substr(0,1) == "\"" && val.substr(val.length()-1,1) == "\""
? val.substr(1, val.length()-2)
: val;
return val;
nfuden marked this conversation as resolved.
Show resolved Hide resolved
}

// parse calls Inja::Environment::parse which uses non-const references to member
// data fields. This method is NOT SAFE to call outside of the InjaTransformer
// constructor since doing so could cause Inja::Environment member fields to be
Expand Down Expand Up @@ -372,12 +401,15 @@ InjaTransformer::InjaTransformer(const TransformationTemplate &transformation,
passthrough_body_(transformation.has_passthrough()),
parse_body_behavior_(transformation.parse_body_behavior()),
ignore_error_on_parse_(transformation.ignore_error_on_parse()),
render_body_as_json_(transformation.render_body_as_json()),
tls_(tls.allocateSlot()),
instance_(std::make_unique<TransformerInstance>(*tls_, rng)) {
if (advanced_templates_) {
instance_->set_element_notation(inja::ElementNotation::Pointer);
}

instance_->set_escape_strings(render_body_as_json_);

tls_->set([](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr {
return std::make_shared<ThreadLocalTransformerContext>();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ class TransformerInstance {
void set_element_notation(inja::ElementNotation notation) {
env_.set_element_notation(notation);
};
void set_escape_strings(bool escape_strings) {
nfuden marked this conversation as resolved.
Show resolved Hide resolved
env_.set_escape_strings(escape_strings);
};

private:
// header_value(name)
Expand All @@ -62,6 +65,7 @@ class TransformerInstance {
nlohmann::json substring_callback(const inja::Arguments &args) const;
nlohmann::json replace_with_random_callback(const inja::Arguments &args);
std::string& random_for_pattern(const std::string& pattern);
nlohmann::json raw_string_callback(const inja::Arguments &args) const;

inja::Environment env_;
absl::flat_hash_map<std::string, std::string> pattern_replacements_;
Expand Down Expand Up @@ -119,6 +123,7 @@ class InjaTransformer : public Transformer {
envoy::api::v2::filter::http::TransformationTemplate::RequestBodyParse
parse_body_behavior_;
bool ignore_error_on_parse_;
bool render_body_as_json_{};

absl::optional<inja::Template> body_template_;
bool merged_extractors_to_body_{};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1147,6 +1147,70 @@ TEST_F(InjaTransformerTest, ParseUsingJsonPointerSyntax) {
EXPECT_EQ(body.toString(), "online--slash--tilde");
}

TEST_F(InjaTransformerTest, RenderBodyAsJson) {
Http::TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/foo"}};
TransformationTemplate transformation;
transformation.mutable_body()->set_text(
R"EOF({"Value":"{{ value }}"})EOF");
transformation.set_render_body_as_json(true);
InjaTransformer transformer(transformation, rng_, google::protobuf::BoolValue(), tls_);

NiceMock<Http::MockStreamDecoderFilterCallbacks> callbacks;

Buffer::OwnedImpl body(R"({"value":"\"foo\""})"_json.dump());
nfuden marked this conversation as resolved.
Show resolved Hide resolved
auto expected_body = R"({"Value":"\"foo\""})"_json.dump();
transformer.transform(headers, &headers, body, callbacks);
EXPECT_EQ(body.toString(), expected_body);
}

TEST_F(InjaTransformerTest, RenderBodyAsJsonDoubleEscapedInput) {
Http::TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/foo"}};
TransformationTemplate transformation;
transformation.mutable_body()->set_text(
R"EOF({"Value":"{{ value }}"})EOF");
transformation.set_render_body_as_json(false);
InjaTransformer transformer(transformation, rng_, google::protobuf::BoolValue(), tls_);

NiceMock<Http::MockStreamDecoderFilterCallbacks> callbacks;

Buffer::OwnedImpl body(R"({"value":"\\\"foo\\\""})"_json.dump());
auto expected_body = R"({"Value":"\"foo\""})"_json.dump();
transformer.transform(headers, &headers, body, callbacks);
EXPECT_EQ(body.toString(), expected_body);
}

TEST_F(InjaTransformerTest, RawStringCallback) {
Http::TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/foo"}};
TransformationTemplate transformation;
transformation.mutable_body()->set_text(
R"EOF({"Value":"{{ raw_string(value) }}"})EOF");
transformation.set_render_body_as_json(false);
InjaTransformer transformer(transformation, rng_, google::protobuf::BoolValue(), tls_);

NiceMock<Http::MockStreamDecoderFilterCallbacks> callbacks;

Buffer::OwnedImpl body(R"({"value":"\"foo\""})"_json.dump());
auto expected_body = R"({"Value":"\"foo\""})"_json.dump();
transformer.transform(headers, &headers, body, callbacks);
EXPECT_EQ(body.toString(), expected_body);
}

TEST_F(InjaTransformerTest, RenderBodyAsJsonRawStringCallback) {
Http::TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/foo"}};
TransformationTemplate transformation;
transformation.mutable_body()->set_text(
R"EOF({"Value":"{{ raw_string(value) }}"})EOF");
transformation.set_render_body_as_json(true);
InjaTransformer transformer(transformation, rng_, google::protobuf::BoolValue(), tls_);

NiceMock<Http::MockStreamDecoderFilterCallbacks> callbacks;

Buffer::OwnedImpl body(R"({"value":"\"foo\""})"_json.dump());
auto expected_body = R"({"Value":"\\\"foo\\\""})"_json.dump();
jbohanon marked this conversation as resolved.
Show resolved Hide resolved
transformer.transform(headers, &headers, body, callbacks);
EXPECT_EQ(body.toString(), expected_body);
}

} // namespace Transformation
} // namespace HttpFilters
} // namespace Extensions
Expand Down
Loading