nginx / unit

NGINX Unit - universal web app server - a lightweight and versatile open source server that simplifies the application stack by natively executing application code across eight different programming language runtimes.
https://unit.nginx.org
Apache License 2.0
5.27k stars 324 forks source link

HTTP: added TSTR validation flag to the rewrite option. #1022

Closed hongzhidao closed 6 months ago

hongzhidao commented 7 months ago

Take the configuration as an example:

{
    "rewrite": "`${foo + "
}

Before it was compiled in the router process with a simple error message:

failed to apply previous configuration

It should be checked in the controller process with the error message:

the previous configuration is invalid: "SyntaxError: Unexpected end of input in default:1" in the "rewrite" value.
ac000 commented 7 months ago

So just to confirm. The error here is that the rewrite rule is invalid something? What's the value of something? Is it NJS?

This information (once complete) should also go in the commit message.

callahad commented 7 months ago

Yes, it's an unclosed template literal... it's missing both the closing } for variable substitution and the closing backtick for the string itself.

hongzhidao commented 7 months ago

The error here is that the rewrite rule is invalid something?

Yes, the option value is invalid.

What's the value of something? Is it NJS?

It's a JS code that starts with `. Then it's an invalid JS code. We should report the compiled error.

# JS code
${foo + 

With error

SyntaxError: Unexpected end of input in default:1

Btw, we should do it in the controller process, since the related option name needs to be listed.

in the "rewrite" value.

Based on the above, the message below is necessary for users to read.

the previous configuration is invalid: "SyntaxError: Unexpected end of input in default:1" in the "rewrite" value.

This information (once complete) should also go in the commit message.

Makes sense.

ac000 commented 7 months ago

So this new error only applies to invalid JS?.

hongzhidao commented 7 months ago

Correct, internally we call it template string (tstr), that’s why we add the flag here.

ac000 commented 7 months ago

OK, so, if you can update the commit message

$ git commit --amend
$ git push -f <remote> <branch>

Assuming this is the HEAD commit in its own branch.

ac000 commented 7 months ago

Minor nitpick.

Could you change the last two lines of the commit message to

After this change, the user will see this improved error message:

the previous configuration is invalid: "SyntaxError: Unexpected end of input in default:1" in the "rewrite" value.

I.e Separate the error message with a blank line. Just makes it clearer to read.

hongzhidao commented 7 months ago

Sure, what about the other one? Or I'll rewrite it like this. Is that ok?

This is to improve error messages for rewrite configuration.
Take the configuration as an example:
{
    "rewrite": "`${foo + "
}

Previously, when applying it the user would see this error message:

failed to apply previous configuration

After this change, the user will see this improved error message:

the previous configuration is invalid: "SyntaxError: Unexpected end of input in default:1" in the "rewrite" value.
ac000 commented 7 months ago

Oh, yes, that's good, just put a blank line before the config snippet.

(Personally I would also indent the config snippet by a couple of chars, but I'll leave that up to you).

ac000 commented 7 months ago

Heh, OK, miscommunication. This is really what I meant (in fact lets indent the error messages also...)

This is to improve error messages for rewrite configuration.
Take the configuration as an example:

  {
      "rewrite": "`${foo + "
  }

Previously, when applying it the user would see this error message:

  failed to apply previous configuration

After this change, the user will see this improved error message:

  the previous configuration is invalid: "SyntaxError: Unexpected end of input in default:1" in the "rewrite" value.
ac000 commented 7 months ago

The other issue I see is that it's using your @gmail address.

You can fix this by doing in your unit repository

$ git config user.name "Zhidao HONG"
$ git config user.email z.hong@f5.com
$ git commit --amend --no-edit --author="Zhidao HONG <z.hong@f5.com>"

That will set your .git/config user.{name,email} entries and update the HEAD commits 'Author' entry. The 'Commit' entry will update automatically.

You can check the result with

$ git show --pretty=fuller