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: Fix cors headers #7900

Merged
merged 7 commits into from
Jan 6, 2023
Merged

fix: Fix cors headers #7900

merged 7 commits into from
Jan 6, 2023

Conversation

alexgarel
Copy link
Member

@alexgarel alexgarel commented Dec 23, 2022

  • centralize cors headers handling in perl code

  • add Access-Control-Allow-Methods correctly

  • add tests

  • cleaned nginx prod configurations

  • make docker nginx more close to production configuration

should fix:

We want to include PATCH, PUT, etc.
* centralize cors headers handling in perl code
* add Access-Control-Allow-Methods correctly
* add tests
@alexgarel alexgarel requested a review from a team as a code owner December 23, 2022 15:13
@github-actions github-actions bot added API Issues related to the Open Food Facts API. More specific labels exist & should be used (API WRITE…) ✏️ Editing - Auto Suggest Providing autosuggest for taxonomized fields. Mostly used in editing scenarii Display 🖼️ Images multilingual products NGINX 🧴 Open Beauty Facts Our cosmetic analysis project https://world.openbeautyfacts.org 🐾 Open Pet Food Facts Our pet food analysis project https://world.openpetfoodfacts.org 📸 Open Products Facts Our project to increase the lifespan of objects. https://world.openproductsfacts.org 📦 Packaging https://wiki.openfoodfacts.org/Category:Recycling 🔎 Search 🧪 tests 🔐 Auth labels Dec 23, 2022
@teolemon
Copy link
Member

Noice 👍

@alexgarel
Copy link
Member Author

I want a validation of @stephanegigandet on this one, as it is a big refactor !

conf/nginx.conf Outdated Show resolved Hide resolved
@stephanegigandet
Copy link
Contributor

Looks good to me, but I have not tested it. Maybe wait until January until we merge and deploy this.

@stephanegigandet
Copy link
Contributor

Thanks for adding all the tests. :)

@@ -14,6 +14,13 @@
# Default server configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should move this file to conf/nginx/nginx.conf (which currently seems to contain the default nginx.conf)

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I'm confused, this nginx.conf file is only for docker, which does not use the files in sites-available?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes for docker we have a different config than in prod.

So I kept the nginx conf for prod in conf/nginx, while docker simply uses conf/nginx.conf
(a docker nginx instance only serve one application, and obf, opf, etc. are not docker ready yet.)

@stephanegigandet
Copy link
Contributor

Deployed on https://world.openfoodfacts.dev and https://world.pro.openfoodfacts.dev with a few minor changes to add the off:off authentication and change .org to .dev

also removed the const { off } = require("gulp") line

@stephanegigandet
Copy link
Contributor

For some reason, I don't get the access-control-allow-origin: * header for v3 API queries, but I do get it for v2. (same in prod in fact)

$ curl -i -X OPTIONS https://off:off@world.openfoodfacts.dev/api/v3/product/12345678143
HTTP/2 404 
server: nginx/1.10.3
date: Mon, 02 Jan 2023 17:55:09 GMT
content-type: application/json; charset=utf-8
x-frame-options: DENY
x-content-type-options: nosniff
x-download-options: noopen
x-xss-protection: 1; mode=block
x-request-id: cJ90QvVhm9SHiqqk

{"code":"12345678143","errors":[{"field":{"id":"12345678143","value":"12345678143"},"impact":{"id":"failure","lc_name":"Failure","name":"Failure"},"message":{"id":"product_not_found","lc_name":"","name":""}}],"result":{"id":"product_not_found","lc_name":"Product not found","name":"Product not found"},"status":"failure","warnings":[]}

$ curl -i -X OPTIONS https://off:off@world.openfoodfacts.dev/api/v2/product/12345678143
HTTP/2 404 
server: nginx/1.10.3
date: Mon, 02 Jan 2023 17:55:17 GMT
content-type: application/json; charset=utf-8
access-control-allow-origin: *
x-frame-options: DENY
x-content-type-options: nosniff
x-download-options: noopen
x-xss-protection: 1; mode=block
x-request-id: kP8kuV7uX2Gq5ZkN

@alexgarel
Copy link
Member Author

@stephanegigandet

For some reason, I don't get the access-control-allow-origin: * header for v3 API queries, but I do get it for v2. (same in prod in fact)

Are you sure you have the right code ? (it's not just a nginx config)

For you should not only have "access-control-allow-origin: *" but also the "Access-Control-Allow-Methods" header.

@sonarcloud
Copy link

sonarcloud bot commented Jan 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@stephanegigandet
Copy link
Contributor

I redeployed and tested again, all good!

@alexgarel alexgarel merged commit 4aac6f6 into main Jan 6, 2023
@alexgarel alexgarel deleted the build-nginx-cors branch January 6, 2023 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Issues related to the Open Food Facts API. More specific labels exist & should be used (API WRITE…) 🔐 Auth Display ✏️ Editing - Auto Suggest Providing autosuggest for taxonomized fields. Mostly used in editing scenarii 🖼️ Images NGINX 🧴 Open Beauty Facts Our cosmetic analysis project https://world.openbeautyfacts.org 🐾 Open Pet Food Facts Our pet food analysis project https://world.openpetfoodfacts.org 📸 Open Products Facts Our project to increase the lifespan of objects. https://world.openproductsfacts.org 📦 Packaging https://wiki.openfoodfacts.org/Category:Recycling 🔎 Search 🧪 tests
Projects
Status: 👀 PRs
Development

Successfully merging this pull request may close these issues.

None yet

3 participants