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

njs breaks fancyindex after commit a14be61 #783

Closed
2 tasks done
Harry-Chen opened this issue Sep 14, 2024 · 2 comments · Fixed by #786
Closed
2 tasks done

njs breaks fancyindex after commit a14be61 #783

Harry-Chen opened this issue Sep 14, 2024 · 2 comments · Fixed by #786
Assignees
Labels

Comments

@Harry-Chen
Copy link

Describe the bug

Commit a14be61 changed the way of checking subrequest from subrequest. This change of behaviour breaks fancyindex when the configuration issues a subrequest to fetch a template file in fancyindex_header subrequest.

  • The bug is reproducible with the latest version of njs.
  • I minimized the code and NGINX configuration to the smallest
    possible to reproduce the issue.

This was originally reported (in Chinese) at tuna/mirror-web#452. I bisected the code and found the first breaking commit to be a14be61, which indeed is a modification to checking-related code.

To reproduce

Steps to reproduce the behavior:

  • JS script: /path/to/webroot/fancy_index.njs
"use strict";

const exports = {};
export default exports;
exports.fancyIndexBeforeRender = fancyIndexBeforeRender;

function fancyIndexRender(r, templateUrl) {
  r.subrequest(templateUrl, {
    args: "",
    body: "",
    method: "GET"
  }, function (rTmpl) {
    if (rTmpl.status != 200) {
      return r.return(rTmpl.status);
    }
    const result = rTmpl.responseText;
    r.status = 200;
    r.headersOut["Content-Type"] = "text/html";
    r.sendHeader();
    r.send(result);
    r.finish();
  });
}
function fancyIndexBeforeRender(r) {
  return fancyIndexRender(r, "/fancy-index/template.html");
}
  • Empty template file: touch /path/to/webroot/fancy-index/template.html
  • NGINX configuration:
server {
  listen 8000;
  server_name  localhost;
  root /path/to/webroot/;

  fancyindex_header /fancy-index/before;
  location / {
    fancyindex on;
  }
  js_path /path/to/webroot/;
  location /fancy-index {
    internal;
    js_import fancyIndexRender from fancy_index.njs;
    location = /fancy-index/before {
      js_content fancyIndexRender.fancyIndexBeforeRender;
    }
  }
}
  • NGINX logs:
2024/09/14 15:57:50 [error] 407026#0: *1 js exception: Error: subrequest can only be created for the primary request
    at Request.subrequest (native)
    at fancyIndexRender (fancy_index.njs:8)
    at fancyIndexBeforeRender (fancy_index.njs:25)
, client: 127.0.0.1, server: localhost, request: "GET / HTTP/1.1", subrequest: "/fancy-index/before", host: "127.0.0.1:8000"
  • Output of the nginx -V command:
nginx version: nginx/1.27.1
built by gcc 12.2.0 (Debian 12.2.0-14)
built with OpenSSL 3.0.14 4 Jun 2024
TLS SNI support enabled
configure arguments: --prefix=/path/to/nginx_install --with-http_addition_module --add-dynamic-module=/path/to/njs/nginx --add-dynamic-module=/path/to/ngx-fancyindex
  • Exact steps to reproduce the behavior

Run NGINX with the configuration above, and access http://127.0.0.1:8000. Server will reply with 500 Internal Server Error with similar logs as provided.

Expected behavior

fancyindex should be able to fetch and render the template as before.

Your environment

  • Version of njs or specific commit: a14be61
  • List of other enabled nginx modules if applicable: fancy-index
  • OS: Debian 12
@Harry-Chen Harry-Chen added the bug label Sep 14, 2024
@xeioex xeioex self-assigned this Sep 18, 2024
xeioex added a commit to xeioex/njs that referenced this issue Sep 18, 2024
The issue was introduced in a14be61 (0.8.5).

This fixes nginx#783 on Github.
@xeioex xeioex closed this as completed in c2bc8c6 Sep 18, 2024
@xeioex
Copy link
Contributor

xeioex commented Sep 18, 2024

Hi @Harry-Chen,

Thank you for the detailed report. The bug is fixed now.

@Harry-Chen
Copy link
Author

Thanks! I can confirm that the problem is fixed in current master.

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 a pull request may close this issue.

2 participants