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

Lua filter: support encoding binary data via base64Escape #13057

Merged
merged 3 commits into from
Sep 14, 2020

Conversation

lhluo
Copy link
Contributor

@lhluo lhluo commented Sep 11, 2020

Commit Message: Support binary data in Lua Base64 encoding
Additional Description: base64Escape was introduced in #12552 to allow Lua filters to Base64 encode strings. The input is stored in string_view which assumes a null-terminated string by default. However, raw binary data is not supported because it can have null bytes anywhere. This PR fixes the issue by setting the input_size explicitly.

Risk Level: Low
Testing: Added a test case in unit tests
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Lihao Luo [email protected]

Comment on lines 2220 to 2221
EXPECT_CALL(*filter_, scriptLog(spdlog::level::trace, StrEq("YmFyZm9v")));
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, true));
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep a test with a simple string as input as well?


EXPECT_CALL(*filter_, scriptLog(spdlog::level::trace, StrEq("YmFyZm9v")));
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, true));
// base64 encoding should also work for binary data
Copy link
Member

Choose a reason for hiding this comment

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

Super nit.

// Base64 encoding should also work for binary data.

Copy link
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Thanks for catching this. Few comments. 👆🏽

@lhluo
Copy link
Contributor Author

lhluo commented Sep 11, 2020

@dio thanks for reviewing! Updated PR to address your comments.

Signed-off-by: Lihao Luo <[email protected]>
@lhluo lhluo force-pushed the binary-lua-b64-encode branch from cfa5e3d to 962affd Compare September 11, 2020 19:22
@lhluo
Copy link
Contributor Author

lhluo commented Sep 11, 2020

Force push to fix DCO

@dio
Copy link
Member

dio commented Sep 11, 2020

@lhluo could you merge master? Thanks! Seems like there is a fix for that coverage failure.

…code

* upstream/master:
  lint: add more linters for using absl:: over std:: (envoyproxy#13043)
  udpa: filesystem list collection support for inline entries. (envoyproxy#13028)
  filter: http: jwt: implement matching for HTTP CONNECT (envoyproxy#13064)
  [fuzz] split http filter logic into a fuzzing class (envoyproxy#13016)
  xds: allow empty delta update (envoyproxy#12699)
  CacheFilter: parses the allowed_vary_headers from the cache config. (envoyproxy#12928)
  router: extend HTTP CONNECT route matching criteria (envoyproxy#13056)
  docs: clarify use of Extended CONNECT for h/2 (envoyproxy#13051)
  build: shellcheck tools/ (envoyproxy#13007)
  [fuzz] Refactored Health Checker Impl Tests (envoyproxy#13017)

Signed-off-by: Lihao Luo <[email protected]>
@lhluo
Copy link
Contributor Author

lhluo commented Sep 12, 2020

@dio done!

Copy link
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Thanks.

@lhluo
Copy link
Contributor Author

lhluo commented Sep 14, 2020

hi @dio could you help merge these changes today?

Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing!

@snowp snowp merged commit 64bd631 into envoyproxy:master Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants