openfoodfacts / openfoodfacts-server

Open Food Facts database, API server and web interface - 🐪🦋 Perl, CSS and JS coders welcome 😊 For helping in Python, see Robotoff or taxonomy-editor
http://openfoodfacts.github.io/openfoodfacts-server/
GNU Affero General Public License v3.0
660 stars 389 forks source link

[NGINX] If Is Evil #1538

Open NerOcrO opened 6 years ago

NerOcrO commented 6 years ago

Subject: https://www.nginx.com/resources/wiki/start/topics/tutorials/config_pitfalls/#using-if

Todo:

Solutions:

if ($host = android.openfoodfacts.org) {
  return 302 https://play.google.com/store/apps/details?id=org.openfoodfacts.scanner;
}

=>

server {
  server_name android.openfoodfacts.org;

  return 302 https://play.google.com/store/apps/details?id=org.openfoodfacts.scanner;
}

and

if ($test = ABC) {
  return 301 https://$host$request_uri;
}

Nobody use http yet?

EDIT1: The good way and

location /data/ {
  if ($request_method = 'OPTIONS') {
    ...
   }
  ...
}

Not sure but I do this within my Docker:

  location = /data/top_translators.csv {
    add_header Access-Control-Allow-Origin *;
    add_header Access-Control-Allow-Headers If-None-Match,Range;

    proxy_pass http://127.0.0.1:8001;
  }
hangy commented 6 years ago

Agreed for cases that can easily be replaced, but I'm not sure about $request_method.

location /data/ {
  if ($request_method = 'OPTIONS') {
    ...
   }
  ...
}

Nearly every online guide for CORS with nginx says that the CORS headers should only be set for the OPTIONS method. I believe this could be related to security issues. This is also mentioned as en exception in https://www.nginx.com/resources/wiki/start/topics/depth/ifisevil/

NerOcrO commented 6 years ago

This is also mentioned as en exception in https://www.nginx.com/resources/wiki/start/topics/depth/ifisevil/

Where?

hangy commented 6 years ago

There are cases where you simply cannot avoid using an if, for example, if you need to test a variable which has no equivalent directive.

if ($request_method = POST ) {
  return 405;
}
if ($args ~ post=140){
  rewrite ^ http://example.com/ permanent;
}

It seems that there is no alternative to the if-check when the HTTP method is significant. They don't mention CORS specifically.

stephanegigandet commented 6 years ago

We can change the simple redirects, thanks.

For this one: if ($test = ABC) { return 301 https://$host$request_uri; } Nobody use http yet?

--> we do want to allow API access through http, all the old reuses, apps etc. make requests to the http version of the API, and redirecting them will break some of them (e.g. apps that do POST requests on http)

NerOcrO commented 6 years ago

Yep but can we contact them to change there API? No one should use http yet... security risk on POST! And I guess, more speed with http2? And more consistent. And remove this legacy line.

VaiTon commented 5 years ago

Any news on this?

CharlesNepote commented 5 years ago

@cquest your skills are requested ;-)

CharlesNepote commented 4 years ago

Also, current configuration of CORS headers avoids javascript to request data from other domains. Example: on openfoodfacts.org we can't request data from openbeautyfacts.org.

It seems possible to allow CORS for our different domains without using "if" method which is to be avoid.

A simple solution seems to use map, see:

I'm not sure but this solution might be useful for other "if".

map $http_origin $cors_origin_header {
    default "";
    "~(^|^http:\/\/)(localhost$|localhost:[0-9]{1,4}$)" "$http_origin";
    "~^https://(?:[^/]*\.)?openfoodfacts\.(?:org|dev|local))(?::[0-9]+)?$" "$http_origin";
    "~^https://(?:[^/]*\.)?openbeautyfacts\.(?:org|dev|local))(?::[0-9]+)?$" "$http_origin";
    "~^https://(?:[^/]*\.)?openpetfoodfacts\.(?:org|dev|local))(?::[0-9]+)?$" "$http_origin";
}

map $http_origin $cors_cred {
    default "";
    "~(^|^http:\/\/)(localhost$|localhost:[0-9]{1,4}$)" "true";
    "~^https://(?:[^/]*\.)?openfoodfacts\.(?:org|dev|local))(?::[0-9]+)?$" "$http_origin";
    "~^https://(?:[^/]*\.)?openbeautyfacts\.(?:org|dev|local))(?::[0-9]+)?$" "$http_origin";
    "~^https://(?:[^/]*\.)?openpetfoodfacts\.(?:org|dev|local))(?::[0-9]+)?$" "$http_origin";
}

server {
    listen 443 ssl http2;
    server_name openfoodfacts.org;

    include ssl/wildcard;

    add_header Access-Control-Allow-Origin $cors_origin_header always;
    add_header Access-Control-Allow-Credentials $cors_cred;
    add_header "Access-Control-Allow-Methods" "GET, POST, OPTIONS, HEAD";
    add_header "Access-Control-Allow-Headers" "Authorization, Origin, X-Requested-With, Content-Type, Accept";

    if ($request_method = 'OPTIONS' ) {
      return 204 no-content;
    }