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

fix: add nginx support for webp images in hidden directory, sync nginx conf #5580

Merged
merged 2 commits into from Mar 5, 2024

Conversation

stephanie-ehrling
Copy link
Contributor

@stephanie-ehrling stephanie-ehrling commented Nov 28, 2023

Added webp support for images in hidden directories.

The Issue

Webp images are not supported for magento 2 projects if the image is in a hidden directory. A request to a webp image will result in a HTTP 403 Error.

How This PR Solves The Issue

Changed nginx config to allow webp files as images in hidden directories.

Manual Testing Instructions

Add an image in webp format in a hidden directory i.e. https://test.ddev.site/media/.renditions/test.webp and request that image. Before the changes there is an error "403 Forbidden". After the changes the image is shown in the browser.

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

@stephanie-ehrling stephanie-ehrling requested a review from a team as a code owner November 28, 2023 11:39
@stephanie-ehrling stephanie-ehrling changed the title Update nginx-site-magento2.conf fix: added webp to nginx-site-magento2.conf Nov 28, 2023
Copy link

github-actions bot commented Nov 28, 2023

@rfay
Copy link
Member

rfay commented Nov 28, 2023

Please update the OP provide manual testing instructions. I assume you have already experimented with this using .ddev/nginx-full/nginx-site.conf?

@rfay rfay changed the title fix: added webp to nginx-site-magento2.conf fix: added webp support for magento2 (nginx-site-magento2.conf) Nov 28, 2023
@stephanie-ehrling
Copy link
Contributor Author

Please update the OP provide manual testing instructions. I assume you have already experimented with this using .ddev/nginx-full/nginx-site.conf?

Right, I tested it with my changes in .ddev/nginx-full/nginx-site.conf and now the images can be displayed.

@rfay
Copy link
Member

rfay commented Nov 28, 2023

Please test manually with the artifacts here, #5580 (comment)

Don't forget to remove your .ddev/nginx-full/nginx-site.conf before testing.

I note that TYPO3 config does this differently, https://github.com/ddev/ddev/blob/8b714e13632069299a9f3e3dbebc3c8442f8baf5/pkg/ddevapp/webserver_config_assets/nginx-site-typo3.conf

I also note that only TYPO3 currently has explicit webp support. Would you consider sorting it out for the other project types? https://github.com/ddev/ddev/tree/master/pkg/ddevapp/webserver_config_assets

@stephanie-ehrling
Copy link
Contributor Author

I tested it with the ddev-macos-arm64.zip and it worked.
For the other images I have no possibilities to test.

@rfay
Copy link
Member

rfay commented Nov 28, 2023

They're all the same, thanks for testing.

Would love to have you improve webp for the other project types as well :)

@stephanie-ehrling
Copy link
Contributor Author

They're all the same, thanks for testing.

Would love to have you improve webp for the other project types as well :)

Maybe I can spend some time on this in the next week.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

I tested this, it works fine.

However, I used upstream/master (v1.22.6) and it also worked fine without this patch.

Could you please re-check and make sure that this is actually needed? Is it possible you hit the wrong URL when testing or something?

I also checked drupal10 project type and it worked fine opening webp files with no modifications of any kind.

I rebased this and you can download and test artifacts at #5580 (comment)

@stephanie-ehrling
Copy link
Contributor Author

stephanie-ehrling commented Dec 21, 2023

I re-checked it with ddev 1.22.6 and can confirm it is needed. I used the same URL with a Magento 2 project. I did not check it with drupal.

Before:

Request URL: https://someproject.ddev.site/media/wysiwyg/someimage.webp
Request Method: GET
Status Code: 403 Forbidden

After:

Request URL: https://someproject.ddev.site/media/wysiwyg/someimage.webp
Request Method: GET
Status Code: 200 OK

@stephanie-ehrling stephanie-ehrling marked this pull request as ready for review January 12, 2024 07:51
@rfay
Copy link
Member

rfay commented Feb 4, 2024

Testing again with magento 2 2.4.6-p3, DDEV v1.22.6, macOS arm64. Test image attached here. Using Magento 2 quickstart from https://ddev.readthedocs.io/en/latest/users/quickstart/#magento, Chrome browser

Following the testing instructions here, I copied testimage.webp to pub/media and then accessed https://ddev-magento2.ddev.site/media/testimage.webp and it showed just fine.

image

testimage.zip

Closing this, happy to reopen, happy to continue the conversation if you can give a more complete test scenario. I am unable to reproduce your situation. Is it possibly a different browser?

@rfay rfay closed this Feb 4, 2024
@sebastian-ehrling
Copy link

@rfay i've got the same error on my local env.

I've found the following error in the nginx error.log:
2024/02/29 19:00:48 [error] 2323#2323: *11 access forbidden by rule, client: 172.24.0.9, server: , request: "GET /media/.renditions/wysiwyg/startseite/test_pc.webp HTTP/1.1", host: "test.ddev.site"

Is it possible to reopen the issue?

@sebastian-ehrling
Copy link

Hi @rfay

please check the following urls. When I put the test.webp in a subdirectory with the name .renditions, it doesn't work anymore.

Works:
https://test.ddev.site/media/test.webp

Doesn't work:
https://test.ddev.site/media/.renditions/test.webp

@rfay
Copy link
Member

rfay commented Feb 29, 2024

Does it work if the directory is not a hidden directory? For example, if it's named renditions instead of .renditions ?

@sebastian-ehrling
Copy link

yes, this work's also.
Example: https://test.ddev.site/media/renditions/test.webp

@rfay
Copy link
Member

rfay commented Feb 29, 2024

So most likely the problem is not following into hidden directories? Can you try it with a jpg file?

@sebastian-ehrling
Copy link

@rfay
Copy link
Member

rfay commented Feb 29, 2024

So

  • a jpg works in a hidden directory
  • A webp does not work in a hidden directory
  • A webp does work in any non-hidden directory

?

@sebastian-ehrling
Copy link

correct!

@rfay
Copy link
Member

rfay commented Feb 29, 2024

And what happens with the changes proposed in this PR?

@sebastian-ehrling
Copy link

I've modfied the nginx config file like the changes from pr. After restart nginx / ddev it work's perfectly

@rfay rfay reopened this Feb 29, 2024
@rfay
Copy link
Member

rfay commented Feb 29, 2024

I'm reopening, but please update the OP to say the specifics of what's going on here.

@stasadev can you understand why this would happen?

Are any changes required for apache-fpm?

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

I've tested the current logic, and when `.webp' is not added to the list, the path goes to the following condition:

location ~* /\.(?!well-known\/) {
    deny all;
}

This PR also adds .webp to /static/ and /media/ conditions, which is not needed. I placed the image to /pub/media/.test/1.webp and /pub/static/test/1.webp and it worked only with this condition:

location ~* \.(?:jpg|jpeg|gif|png|ico|cur|gz|svg|svgz|mp4|ogg|ogv|webm|htc|webp)$

Are any changes required for apache-fpm?

It works fine with apache-fpm.

We need to add .webp to other nginx-site.conf.

If you don't mind, I can do it.

@rfay rfay changed the title fix: added webp support for magento2 (nginx-site-magento2.conf) fix: add nginx support for webp in hidden directory (nginx-site-magento2.conf) Mar 1, 2024
@rfay rfay changed the title fix: add nginx support for webp in hidden directory (nginx-site-magento2.conf) fix: add nginx support for webp images in hidden directory (nginx-site-magento2.conf) Mar 1, 2024
@stasadev stasadev changed the title fix: add nginx support for webp images in hidden directory (nginx-site-magento2.conf) fix: add nginx support for webp images in hidden directory Mar 1, 2024
@rfay
Copy link
Member

rfay commented Mar 1, 2024

I don't think anybody will object if you fix for the other CMS types @stasadev

@stasadev
Copy link
Member

stasadev commented Mar 1, 2024

I added the missing conditions for craftcms and php and synchronized the rest of the nginx-site-*.conf files with the latest list of extensions.

The only files I haven't touched (don't know how to test them):

  • nginx-site-django4.conf
  • nginx-site-python.conf

@stasadev stasadev changed the title fix: add nginx support for webp images in hidden directory fix: add nginx support for webp images in hidden directory, sync nginx conf Mar 1, 2024
@rfay
Copy link
Member

rfay commented Mar 1, 2024

Thanks!
django4: https://ddev.readthedocs.io/en/latest/users/quickstart/#django-4-experimental
python: https://ddev.readthedocs.io/en/latest/users/quickstart/#pythonflask-experimental

We have some explicit examples in ddevapp_test.go as well

I'm not sure it's super necessary to test those though.

@stasadev
Copy link
Member

stasadev commented Mar 1, 2024

I tested it with test-django4-bakerydemo, and it didn't work.
I think it's handled somewhere on the python side, because when I add this condition, all media stops working at all.

So I won't add anything else.

@rfay rfay merged commit 22df941 into ddev:master Mar 5, 2024
19 checks passed
@sebastian-ehrling
Copy link

great!

Happy Wife, Happy Life 👍 @stephanie-ehrling

@rfay
Copy link
Member

rfay commented Apr 13, 2024

I'm afraid this has had unexpected consequences, making Kirby completely fail when used with nginx. It appears that the change made here in https://github.com/ddev/ddev/pull/5580/files#diff-6633a7b4b2209acb43fe88d9cce12c93058ca2a2ff4f712e232bd1cda067c900R55-R63 is the cause of the problem.

Looking back, I don't know how that snuck in here, or why it was related to webp images in hidden directory.

I'm afraid this will need to be reverted, and we can try again.

@rfay
Copy link
Member

rfay commented Apr 13, 2024

I guess the change was made in 6288949 and was done by @stasadev trying to get this ready for pull.

I guess we'll have to take a look and see why this has happened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants