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: trailers() always returns nil if it's called before reading body #36507

Closed
spacewander opened this issue Oct 9, 2024 · 2 comments · Fixed by #36553
Closed

lua: trailers() always returns nil if it's called before reading body #36507

spacewander opened this issue Oct 9, 2024 · 2 comments · Fixed by #36553

Comments

@spacewander
Copy link
Contributor

If you are reporting any crash or any potential security issue, do not
open an issue in this repo. Please report the issue via emailing
envoy-security@googlegroups.com where the issue will be triaged appropriately.

Title: One line description

lua: trailers() always returns nil if it's called before reading body

Description:

What issue is being seen? Describe what should be happening instead of
the bug, for example: Envoy should not crash, the expected value isn't
returned, etc.

When calling trailers() before reading body, this method will always return nil whether there are trailers or not.

Repro steps:

Include sample requests, environment, etc. All data and inputs
required to reproduce the bug.

With configuration:

                          typed_per_filter_config:
                            envoy.filters.http.lua:
                              "@type": type.googleapis.com/envoy.extensions.filters.http.lua.v3.LuaPerRoute
                              source_code:
                                inline_string: |
                                  function envoy_on_request(handle)
                                     local trailers = handle:trailers()
                                     local always_wrap_body = true
                                     local body = handle:body(always_wrap_body)                  
                                    ...

The returned trailers are nil.

However, if we change the order to:

                                     local always_wrap_body = true
                                     local body = handle:body(always_wrap_body)     
                                     local trailers = handle:trailers()    

Now we can read the trailers.

It's reasonable because Envoy may want to handle the body first and then handle the trailers, which share the same stream. But it would be better to mention this order requirement in the docs: https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/lua_filter#trailers.

@spacewander spacewander added bug triage Issue requires triage labels Oct 9, 2024
@alyssawilk
Copy link
Contributor

cc @mattklein123 @wbpcode

@alyssawilk alyssawilk added area/docs and removed triage Issue requires triage labels Oct 9, 2024
@wbpcode
Copy link
Member

wbpcode commented Oct 10, 2024

I will prefer treat this as an implementation bug. But I have no enough bandwidth to fix it recently.

But at least, an additional mention at docs would be great.

@wbpcode wbpcode added the help wanted Needs help! label Oct 10, 2024
wbpcode pushed a commit that referenced this issue Oct 14, 2024
…36553)

<!--
!!!ATTENTION!!!

If you are fixing *any* crash or *any* potential security issue, *do
not*
open a pull request in this repo. Please report the issue via emailing
envoy-security@googlegroups.com where the issue will be triaged
appropriately.
Thank you in advance for helping to keep Envoy secure.

!!!ATTENTION!!!

For an explanation of how to fill out the fields, please see the
relevant section
in
[PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/main/PULL_REQUESTS.md)
-->

Commit Message: lua: mention that body should be consumed before
fetching trailers
Additional Description:
Risk Level: Zero
Testing: N/A
Docs Changes: lua_filter.rst
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
Fixes #36507
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API
Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

Signed-off-by: spacewander <spacewanderlzx@gmail.com>
grnmeira pushed a commit to grnmeira/envoy that referenced this issue Oct 17, 2024
…nvoyproxy#36553)

<!--
!!!ATTENTION!!!

If you are fixing *any* crash or *any* potential security issue, *do
not*
open a pull request in this repo. Please report the issue via emailing
envoy-security@googlegroups.com where the issue will be triaged
appropriately.
Thank you in advance for helping to keep Envoy secure.

!!!ATTENTION!!!

For an explanation of how to fill out the fields, please see the
relevant section
in
[PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/main/PULL_REQUESTS.md)
-->

Commit Message: lua: mention that body should be consumed before
fetching trailers
Additional Description:
Risk Level: Zero
Testing: N/A
Docs Changes: lua_filter.rst
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
Fixes envoyproxy#36507
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API
Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

Signed-off-by: spacewander <spacewanderlzx@gmail.com>
Signed-off-by: Gustavo <grnmeira@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants