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

gonginx does not seem to catch syntax errors in nginx.conf #31

Closed
kehoecj opened this issue Dec 12, 2023 · 11 comments
Closed

gonginx does not seem to catch syntax errors in nginx.conf #31

kehoecj opened this issue Dec 12, 2023 · 11 comments
Assignees
Labels
bug Something isn't working v2 issues that would be fixed in v2 only

Comments

@kehoecj
Copy link

kehoecj commented Dec 12, 2023

We were hoping to use gonginx in the config-file-validator tool to provide syntax validation for nginx.conf files. During some testing we noticed that gonginx will not throw an error if the syntax of the nginx.conf file is invalid. For example, in this test we intentionally did not close a server block and it parsed without error.

Is this expected behavior, a bug, or are we parsing incorrectly?

@kehoecj
Copy link
Author

kehoecj commented Dec 14, 2023

@tufanbarisyildirim any ideas?

@tufanbarisyildirim
Copy link
Owner

Hello @kehoecj , sorry I have missed this, this looks like an interesting bug, thanks for raising it. I will be back here around weekend.

@tufanbarisyildirim tufanbarisyildirim self-assigned this Dec 14, 2023
@tufanbarisyildirim tufanbarisyildirim added the bug Something isn't working label Dec 14, 2023
@tufanbarisyildirim
Copy link
Owner

Honestly, I didn't expect it to be utilized so extensively, and I absolutely enjoy the BUGS that can offer opportunities to reshape the core logic.

DEAL

@tufanbarisyildirim
Copy link
Owner

tufanbarisyildirim commented Dec 15, 2023

Hello @kehoecj can you please try to see if that version helps you? https://github.com/tufanbarisyildirim/gonginx/tree/v2

That was a simple workaround, I will release a version for you that you can stick with and then jump in refactoring it.

@onlineque
Copy link

onlineque commented Dec 21, 2023

Hi @tufanbarisyildirim,
thanks a lot for the fix, now the missing eof in block seems to be catched properly.

But when I feed the config with some nonsense keyword like "listena" in this example:


#user http;
worker_processes  1;

#error_log  logs/error.log;
#error_log  logs/error.log  notice;
#error_log  logs/error.log  info;

#pid        logs/nginx.pid;


events {
    worker_connections  1024;
}


http {
    include       mime.types;
    default_type  application/octet-stream;

    #log_format  main  '$remote_addr - $remote_user [$time_local] "$request" '
    #                  '$status $body_bytes_sent "$http_referer" '
    #                  '"$http_user_agent" "$http_x_forwarded_for"';

    #access_log  logs/access.log  main;

    sendfile        on;
    #tcp_nopush     on;

    #keepalive_timeout  0;
    keepalive_timeout  65;

    #gzip  on;

    server {
        listen       80;
        server_name  localhost;

        #charset koi8-r;

        #access_log  logs/host.access.log  main;

        location / {
            root   /usr/share/nginx/html;
            index  index.html index.htm;
        }

        #error_page  404              /404.html;

        # redirect server error pages to the static page /50x.html
        #
        error_page   500 502 503 504  /50x.html;
        location = /50x.html {
            root   /usr/share/nginx/html;
        }

        # proxy the PHP scripts to Apache listening on 127.0.0.1:80
        #
        #location ~ \.php$ {
        #    proxy_pass   http://127.0.0.1;
        #}

        # pass the PHP scripts to FastCGI server listening on 127.0.0.1:9000
        #
        #location ~ \.php$ {
        #    root           html;
        #    fastcgi_pass   127.0.0.1:9000;
        #    fastcgi_index  index.php;
        #    fastcgi_param  SCRIPT_FILENAME  /scripts$fastcgi_script_name;
        #    include        fastcgi_params;
        #}

        # deny access to .htaccess files, if Apache's document root
        # concurs with nginx's one
        #
        #location ~ /\.ht {
        #    deny  all;
        #}
    }


    # another virtual host using mix of IP-, name-, and port-based configuration
    #
    server {
        listena       8000;
        # listen       somename:8080;
        # server_name  somename  alias  another.alias;

        location / {
            root   html;
            index  index.html index.htm;
        }
    }

    # HTTPS server
    #
    #server {
    #    listen       443 ssl;
    #    server_name  localhost;

    #    ssl_certificate      cert.pem;
    #    ssl_certificate_key  cert.key;

    #    ssl_session_cache    shared:SSL:1m;
    #    ssl_session_timeout  5m;

    #    ssl_ciphers  HIGH:!aNULL:!MD5;
    #    ssl_prefer_server_ciphers  on;

    #    location / {
    #        root   html;
    #        index  index.html index.htm;
    #    }
    #}

}

The gonginx says it's valid. But it's not. Testing with nginx -t -c nginx-bad-2.conf gives me expected:

2023/12/21 21:50:36 [emerg] 252611#252611: unknown directive "listena" in /etc/nginx/nginx-bad-2.conf:85
nginx: configuration file /etc/nginx/nginx-bad-2.conf test failed

Is it something what can be fixed, too ?

Thanks !

@tufanbarisyildirim
Copy link
Owner

oh hmm, that's an easy one I will add directive validation as well but it actually does not suppose to validate logically because nginx -t can also check the files, permissions etc, but still a good star to add directive validation, will add it.

@tufanbarisyildirim
Copy link
Owner

give it a test!

@onlineque
Copy link

That's perfect, it works ! May I ask you to release a version for us ? Thanks a lot !

tufanbarisyildirim added a commit that referenced this issue Dec 22, 2023
* handle errors go way, init fixing #31
* add option to skip comments
* handle unexpected eof in block parsing
* fix building issues for examples
* validate known directives
* delete duplicate directives, fix tests, make linter happy
@tufanbarisyildirim
Copy link
Owner

That's perfect, it works ! May I ask you to release a version for us ? Thanks a lot !

sure: https://github.com/tufanbarisyildirim/gonginx/releases/tag/2.0.0

@tufanbarisyildirim tufanbarisyildirim added the v2 issues that would be fixed in v2 only label Dec 23, 2023
@onlineque
Copy link

onlineque commented Dec 23, 2023

I think we can close this issue, it's good enough at least for now ;-) Once again thanks a lot for your help, @tufanbarisyildirim !

@tufanbarisyildirim
Copy link
Owner

I think we can close this issue, it's good enough at least for now ;-) Once again thanks a lot for your help, @tufanbarisyildirim !

You are welcome, happy to help. will keep this version as base of v2.x.x, thanks for pointing out issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v2 issues that would be fixed in v2 only
Projects
None yet
Development

No branches or pull requests

3 participants