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

Provide a way to disable folder listing #370

Closed
chrispiyux opened this issue Jun 27, 2023 · 9 comments
Closed

Provide a way to disable folder listing #370

chrispiyux opened this issue Jun 27, 2023 · 9 comments
Labels
enhancement New feature or request no-issue-activity

Comments

@chrispiyux
Copy link

We are using your component in our application and during a pentest it was hightlighted that XSS vulnerabilities are present in folder listing and 404 template. We were able to modify the templates in order to avoid these but as an additionnal step, we would like to be able to disable folder listing as a global parameter. I could see it was present in your inspiration project pottava/aws-s3-proxy, maybe would be good to re-integrate it.

Describe the solution you'd like
By providing a config parameter or environment variable, folder listing would be disabled and return 403

Describe alternatives you've considered
Today we have updated folder-listing.tpl template and returned HTTP status code 403

Additional context
NA

@chrispiyux chrispiyux added the enhancement New feature or request label Jun 27, 2023
@oxyno-zeta
Copy link
Owner

Hello @chrispiyux ,

Thanks for the issue well described !

The problem I have with this is the following: why disabling the folder listing should return a 403 error ? I'm pretty sure that once this feature will be done like that, someone will say that it should return a 404 (and I'm more aligned with this by the way).

Changing the template and the status code is a good way to go I think.
It allows more things. For example, you can return the folder for you as admin and a 403/404 for other people.

I can make an opinionated feature to return a 404 for sure but I must say I dislike those... A lot of things can be customized in the application for that reason.

I can also write a full documentation page on how to disable file listing with templates.

What do you think about this ?

Oxyno-zeta

@chrispiyux
Copy link
Author

Hi Alexandre,
thanks a lot for your quick answer.
I was proposing 403 because it's what i remember IIS is doing when folder listing is disable, but i'm maybe wrong.
404 would actually be also ok.

As i said, yes we achieved to fix the issue by updating the template, but security has requested also to remove the listBuckets permission for the IAM role used by S3 proxy. Hence instead of reponding with the custom template, S3 proxy throw a 500 error which i'm not fan of.

That's why, if we could manage the directory browsing disabling before S3 proxy tries to list the bucket files it would really be great.
Thanks a lot
Christophe

@oxyno-zeta
Copy link
Owner

Hello Christophe,

Changing the template doesn't exclude the fact the application will make the request. That's why you have a 500 when you remove the right in the policy.

I've a better view now. I will try to work on this in the coming weeks and find a way to avoid having an opinionated answer.

Oxyno-zeta

@chrispiyux
Copy link
Author

Hello Alexandre,
thanks a lot, really looking forward to have this feature.

@oxyno-zeta
Copy link
Owner

Hello @chrispiyux ,

The good news is that I think I found a way to make it available without any opinionated option.
Now I need to find a couple of hours to work on it...

I just wanted to tell you that I don't forget this subject.

@chrispiyux
Copy link
Author

Hi @oxyno-zeta

very good news !
Thanks a lot

oxyno-zeta added a commit that referenced this issue Jul 14, 2023
@oxyno-zeta
Copy link
Owner

oxyno-zeta commented Jul 15, 2023

Hello @chrispiyux ,

As I'm not sure I will be able to finish the new version (I want to change few other things too), I will make you a 4.11 alpha version for you (oxynozeta/s3-proxy:4.11.0-alpha.0-amd64).

I will build it soon.

The documentation is deployed too.

Tell me if it is working well please ;) .

Have a nice day !

@chrispiyux
Copy link
Author

Hello @oxyno-zeta ,

thanks a lot, i will take a look and do some testing and keep you updated.

Thanks

@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request no-issue-activity
Projects
None yet
Development

No branches or pull requests

2 participants