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.

Sign up for GitHub

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

Jump to bottom

With Directory listing turned on query parameters cause 500 errors #164

Closed
coreyfellows opened this issue Aug 14, 2023 · 5 comments
Closed

With Directory listing turned on query parameters cause 500 errors #164

coreyfellows opened this issue Aug 14, 2023 · 5 comments
Assignees
@dekobon
Labels
bug Something isn't working

Comments

@coreyfellows
Copy link

Describe the bug
With Directory listing turned on query parameters cause 500 errors

To Reproduce
Steps to reproduce the behavior:

  1. Start container with APPEND_SLASH_FOR_POSSIBLE_DIRECTORY=true and ALLOW_DIRECTORY_LIST=true with an S3 bucket with an empty index.html at /test/index.html
  2. Run the container and navigate to /test/?asdf=asdf
  3. 500 error at s3PreListing

Expected behavior
Should show the content of index.html

Your environment
Container is the latest release
Deployed on AWS EC2 with EKS with AWS S3 as the backend
@dekobon dekobon added the bug Something isn't working label Aug 16, 2023
@dekobon
Copy link
Collaborator

dekobon commented Aug 16, 2023

I've verified this issue as reproducible. I hope to be able to do a root cause analysis soon. If anyone has insights into the cause of the issue, posting them in the comments would be quite welcome.

@coreyfellows
Copy link
Author

I suspect the issue is using request_uri here. I believe it includes the query parameters and that wrecks it when appending the index.html.

https://github.com/nginxinc/nginx-s3-gateway/blob/7e35275d04e6da0198958b2344da75e41c6940ed/common/etc/nginx/include/s3gateway.js#L363C66-L363C66

dekobon added a commit to dekobon/nginx-s3-gateway that referenced this issue Aug 17, 2023
@dekobon
Fixes nginxinc#164
ed48f0f
@dekobon dekobon mentioned this issue Aug 17, 2023
Merged
@dekobon
Copy link
Collaborator

dekobon commented Aug 17, 2023

@coreyfellows Can you verify if the PR #167 fixes the issue for you in your environment?

@dekobon dekobon self-assigned this Aug 17, 2023
4141done added a commit that referenced this issue Aug 18, 2023
@4141done
fix: Attempts a fix for issue #164
29e8973
@4141done 4141done mentioned this issue Aug 18, 2023
Closed
@4141done
Copy link
Collaborator

@coreyfellows Quick question for you. Given your expectation described in the bug report, do you also have PROVIDE_INDEX_PAGE enabled? I've written up my understanding of your expectations in this draft PR I made while reviewing #167.

My understanding is that without PROVIDE_INDEX_PAGE we'd expect to see a directory list with index.html as an item in this case.

You can see it here: #169

dekobon added a commit to dekobon/nginx-s3-gateway that referenced this issue Aug 22, 2023
@dekobon @4141done
Fixes nginxinc#164
012a8a8 
Co-authored-by: Javier Evans <[email protected]>
@4141done
Copy link
Collaborator

@coreyfellows A fix for this bug report has been merged. If the problem persists in the new version please reopen this issue. Thank you for your contribution to the project.

elJosho pushed a commit to elJosho/nginx-s3-gateway that referenced this issue Oct 25, 2023
@dekobon @4141done @elJosho
Fixes nginxinc#164 (nginxinc#167)
83d7612 
This PR fixes the issue nginxinc#164 

## Expectations
This solution assumes that the following configuration options are set to true
* `APPEND_SLASH_FOR_POSSIBLE_DIRECTORY`
* `PROVIDE_INDEX_PAGE`
* `ALLOW_DIRECTORY_LIST`

Given a folder `test` **with** an `index.html` present in the directory, the `index.html` should be served for:
* `/test`
* `/test/`
* `/test?foo=bar`
* `/test/?foo=bar`

Given a folder `test` **WITHOUT** an `index.html` present in the directory, files in the directory should be listed for:
* `/test`
* `/test/`
* `/test?foo=bar`
* `/test/?foo=bar`

## Notes
* The `@trailslash` was rewriting to `$request_uri` with a trailing slash on the end.  In the case of `/test?foo=bar` we'd wind up with `/test?foo=bar/` which when combined with the other changes led to a rewrite loop
* Changed the check for directory or file to consider the path without anchor or querystring
* Added yet another integration test suite and shuffled around the conditionals that maybe make the tests even more confusing - but do cover this case.

Co-authored-by: Javier Evans <[email protected]>

* make the isDirectory check simpler and more inclusive of error states

* Allow useful output from curl and enable timeouts

This change adds three new flags when using curl
to hit endpoints as part of integration tests:
--connect-timeout 3 --max-time 30 --no-progress-meter

---------

Co-authored-by: Javier Evans <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dekobon dekobon

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dekobon @4141done @coreyfellows