nginx / njs

A subset of JavaScript language to use in nginx
http://nginx.org/en/docs/njs/
BSD 2-Clause "Simplified" License
1.14k stars 148 forks source link

NJS call wrong function. #700

Closed mingkyme closed 5 months ago

mingkyme commented 6 months ago

When use same variable name, it called wrong function.

Here is an example.

server {
  server_name a;
  listen 80;
  js_set $value main.A;
  add_header X-A-Value $value;
  location / {
    return 200;
  }
}

server {
  server_name b;
  listen 80;
  js_set $value main.B;
  add_header X-B-Value $value;
  location / {
    return 200;
  }
}
function A(r){
  return 1;
}

function B(r){
  return 2;
}
export default { A,B }
curl -I 0 -H "Host: a"
HTTP/1.1 200 OK
Server: nginx/1.24.0
Date: Wed, 03 Apr 2024 04:14:50 GMT
Content-Type: application/octet-stream
Content-Length: 0
Connection: keep-alive
X-A-Value: 2
xeioex commented 6 months ago

Hi @mingkyme,

js_set variables (as all nginx variables) are global, so basically $value is declared twice and the second declaration shadows the first one. js_set is different from ordinary set because js_set does not init the variable.

I am going to add some checks though to detect situations like this.

zsteinkamp commented 6 months ago

Would be nice to have at least server{} block scoped vars. It's surprising that they're global. If this is impossible, then I guess the next best thing is to write a warning into the error log.

xeioex commented 6 months ago

@arut Roman, do you have an opinion about server{} scope nginx variables?

arut commented 5 months ago

The variables are global by design. It's hardly possible to change it without significant refactoring of nginx. To avoid such misconfigurations, it should be advised to place js_set at http{] level. These variables hold different values, then why do they have the same name?

NetForce1 commented 5 months ago

@arut I don't know about @mingkyme , but for me the reason for wanting server-scoped variables is to have generic NJS functions that need different configuration per server. Like in njs-acme: https://github.com/nginx/njs-acme/issues/57.

Without server-scoped variable, the only proper way to do that would be to create a separate function per server that calls the generic function with the server-specific config?

zsteinkamp commented 5 months ago

Heya @NetForce1 -- A colleague suggested I explore using a map keyed by $host. I will hopefully have time to dig into this before the end of the week and will report back here how it might work with njs-acme.

NetForce1 commented 5 months ago

Hi @zsteinkamp, that might work, but has to be combined with $ssl_server_name for selecting the certificate for the tls connection, as $host is not yet set at that point.