TuxInvader / nginx-dns

Sample Configuration for DNS over HTTPS (DoH/DoT gateway) and GSLB with NGINX
BSD 2-Clause "Simplified" License
194 stars 47 forks source link

byte string methods are deprecated and have been removed from njs 0.8.0 #19

Closed arman-ku closed 10 months ago

arman-ku commented 1 year ago

Well, the title pretty much says it all. https://nginx.org/en/docs/njs/changes.html

As expected, nginx.log shows: 2023/07/15 23:03:33 [error] 20034#20034: *61 js exception: TypeError: (intermediate value)["bytesFrom"] is not a function at module (/etc/nginx/njs.d/dns/dns.js:35)

Any ETA on when the code will be updated? Thanks in advance :)

arman-ku commented 1 year ago

Ok, i just looked further into the changes of 0.8.0, and apparently ALL byte string method have been removed, switching to Buffer.

Since 0.8.0, the support for byte strings and byte string methods were removed. When working with byte sequence, the Buffer object and Buffer properties, such as r.requestBuffer, r.rawVariables, should be used.

https://nginx.org/en/docs/njs/reference.html#string

arnaudpn commented 1 year ago

@arman-ku : thanks for reporting this, I already noticed Tony M. here https://www.nginx.com/blog/using-nginx-as-dot-doh-gateway/ before falling on this thread.

44 lines of codes are impacted by obsoleted methods as of version 0.8.0:

root@XXXBSD:/usr/local/etc/nginx/njs.d # grep -R -E 'bytesFrom|fromBytes|fromUTF8|toBytes|toString|toUTF8' * | wc -l 44

I hope @TuxInvader will be able to patch this :-)

TuxInvader commented 1 year ago

Hi both,

Can you please try the v2 branch (it's been modified to use buffers)? If it works for you, then I'll merge it into master.

Thanks :-)

arnaudpn commented 1 year ago

Hi @TuxInvader ,

Works great ! Thanks for this v2 :-)

I just have an error popping in my logfile at each request: 2023/07/19 12:12:38 [error] 58066#0: *57804 upstream sent invalid header: "X-DNS-Answers: [HTTPS:\x00..." while reading response header from upstream I didn't have this error with the previous versions although code of previous versions also included references to that header.

Thanks again !

arman-ku commented 1 year ago

Same for me, works fine, just generates a few new error lines error.log

Edit: Okay, after a bit of testing, seems like those requests generating the logs do fail. Webbrowsing is working normally, but when i try some other browsers, some of them embedded, some DNS requests don't get served. I suspect the reason is that my webbrowser (Brave with DoH enabled) is caching the requests heavily, while the embedded ones resolve through Windows (Which i have also setup to use DoH) I will for now downgrade njs and keep using the old version of nginx-dns.

arman-ku commented 10 months ago

Issue is fixed since last commit