Fix TypeError if no extended DNS error is available

[MichaIng]

By submitting this pull request, I confirm the following: {please fill any appropriate checkboxes, e.g: [X]}

{Please ensure that your pull request is for the 'devel' branch!}

• I have read and understood the contributors guide, as well as this entire template.
• I have made only one major change in my proposed changes.
• I have commented my proposed changes within the code.
• I have tested my proposed changes.
• I am willing to help maintain this change if there are issues with it later.
• I give this submission freely and claim no ownership.
• It is compatible with the EUPL 1.2 license
• I have squashed any insignificant commits. (git rebase)
• I have Signed Off all commits. (git commit --signoff)

---

What does this PR aim to accomplish?:

If no extended DNS errors are available, the "ede" variable is assigned an undefined array value, hence "ede" is undefined. Since the length method is not available on undefined variables, a TypeError is thrown later in the script, which breaks the DNS queries view in the admin panel. It hence needs to be assured that .length is never called on an undefined variable.

How does this PR accomplish the above?:

This commit solves the issue by assigning an empty string, if no EDE is available (data[11] is undefined), which seems to be the easier way, compared to changing all cases where ede.length is called. This way, the ede.length > 0 checks keep returning false, if no data was passed.

What documentation changes (if any) are needed to support this PR?:

None

---

For reference: MichaIng/DietPi#4573 (comment)
Issue was observed on Debian Bullseye. But since this is JavaScript, executed in the browser, we're wondering if or why this did not appear before on Buster. Probably something on Bullseye is the reason why no extended DNS errors data is passed in the first place? However, preventing this possible TypeError is a good thing regardless.

[timocapa]

seems to actually be a dev branch issue?

[MichaIng]

Not sure why you think the DietPi version has anything to do with it? 😄
The DietPi version has no effect on the Lighttpd or PHP version and those two shouldn't have an effect on how browsers interpret and handle JavaScript.

Or do you mean the Pi-hole (AdminLTE) development version? Not sure at all, I think we need someone with more knowledge about where/how the EDE data is retrieved and handled, and how it could be missing in your case.

[timocapa]

Not sure why you think the DietPi version has anything to do with it? 😄

pihole dev whoops

[MichaIng]

I mean you could try to revert to master branch:

pihole checkout master

[timocapa]

i mean yeah that'd fix it but i like being on the dev branch - just picking this to fix it isn't hard :)

[MichaIng]

That is actually the important hint. The DNSSEC reply type does not exist in the master branch yet, it was added here: fb2b87e
The 12th array entry is data[11] (starting with 0).

Then the code to show these EDE, which causes this error, was added here: a120e72

@DL6ER
I guess this feature was then tested with DNSSEC enabled, so that the error did not show up? If there is any related test for this page, it may be good to add it with and without DNSSEC to cover all cases.

And @timocapa is DNSSEC indeed disabled in your case? Just out of curiosity, could you try to enable DNSSEC then, revert the fix and see if the error is still present?

[timocapa]

That is actually the important hint.

Thought you picked up on that when you couldn't find the line and I had sent it in the dev branch, with the commit

I guess this feature was then tested with DNSSEC enabled

I do have DNSSEC enabled IIRC

[DL6ER]

There is no automated testing for the web interface and I don't think it'd be very meaningful to start adding them now given that the web interface is going to be largely rewritten for v6.0 (to remove PHP under the hood) and possibly replaced by a better framework altogether at a later point in time.

[MichaIng]

Thought you picked up on that when you couldn't find the line

Ah lol, this is the reason why I didn't see it, I missed the fact that I had a look into the master branch while you gave a link to the devel branch 😄. Okay, then the mystery is solved why this was no error before.

I do have DNSSEC enabled IIRC

Then we still don't know in which case the value is set and in which case not. However, the fix makes sense regardless. If there is some DNSSEC status missing unexpectedly, then its better to investigate that separately than having a failing query log GUI.

I don't think it'd be very meaningful to start adding them

Makes sense.

[timocapa]

Then we still don't know in which case the value is set and in which case not. However, the fix makes sense regardless. If there is some DNSSEC status missing unexpectedly, then its better to investigate that separately than having a failing query log GUI.

Just double checked with the change reverted, doesn't load with either DNSSEC on or off

[MichaIng]

Hmm, the prettier check still fails. Can the the npm test be changed so that it prints the full prettier output to the workflow console, rather than into a log file?

[PromoFaux]

Hmm, the prettier check still fails.

npm run prettier:fix should sort everything out locally... (should!)

[MichaIng]

I have no Node on the system where the Git push was coming from. I'll try it on another system tomorrow.

[PromoFaux]

       var replyid = parseInt(data[5], 10);
       // DNSSEC status
       var dnssecStatus;
-      var ede = data[11] ? data[11] : '';
+      var ede = data[11] ? data[11] : "";
       switch (data[6]) {
         case "1":
           dnssecStatus = '<br><span class="text-green">SECURE';

[PromoFaux]

Can the the npm test be changed so that it prints the full prettier output to the workflow console, rather than into a log file?

Also... not much use. e.g:

0 verbose cli [
0 verbose cli   '/root/.nvm/versions/node/v15.5.0/bin/node',
0 verbose cli   '/root/.nvm/versions/node/v15.5.0/bin/npm',
0 verbose cli   'run',
0 verbose cli   'prettier:check'
0 verbose cli ]
1 info using [email protected]
2 info using [email protected]
3 timing config:load:defaults Completed in 0ms
4 timing config:load:file:/root/.nvm/versions/node/v15.5.0/lib/node_modules/npm/npmrc Completed in 2ms
5 timing config:load:builtin Completed in 2ms
6 timing config:load:cli Completed in 1ms
7 timing config:load:env Completed in 0ms
8 timing config:load:file:/root/AdminLTE/.npmrc Completed in 0ms
9 timing config:load:project Completed in 0ms
10 timing config:load:file:/root/.npmrc Completed in 0ms
11 timing config:load:user Completed in 1ms
12 timing config:load:file:/root/.nvm/versions/node/v15.5.0/etc/npmrc Completed in 0ms
13 timing config:load:global Completed in 0ms
14 timing config:load:cafile Completed in 0ms
15 timing config:load:validate Completed in 0ms
16 timing config:load:setUserAgent Completed in 0ms
17 timing config:load:setEnvs Completed in 1ms
18 timing config:load Completed in 5ms
19 verbose npm-session 910091150191e025
20 timing npm:load Completed in 10ms
21 timing command:run-script Completed in 2192ms
22 verbose stack Error: command failed
22 verbose stack     at ChildProcess.<anonymous> (/root/.nvm/versions/node/v15.5.0/lib/node_modules/npm/node_modules/@npmcli/promise-spawn/index.js:64:27)
22 verbose stack     at ChildProcess.emit (node:events:376:20)
22 verbose stack     at maybeClose (node:internal/child_process:1063:16)
22 verbose stack     at Process.ChildProcess._handle.onexit (node:internal/child_process:295:5)
23 verbose pkgid [email protected]
24 verbose cwd /root/AdminLTE
25 verbose Linux 5.10.16.3-microsoft-standard-WSL2
26 verbose argv "/root/.nvm/versions/node/v15.5.0/bin/node" "/root/.nvm/versions/node/v15.5.0/bin/npm" "run" "prettier:check"
27 verbose node v15.5.0
28 verbose npm  v7.3.0
29 error code 1
30 error path /root/AdminLTE
31 error command failed
32 error command sh -c prettier -l "style/*.css" "style/themes/*.css" "scripts/pi-hole/**/*.js"
33 verbose exit 1

Further reading prettier/prettier#6885.. but the answer pretty much seems to be "just run prettier before you commit" 😏

[MichaIng]

Lol, seems like a discussion with hardened front lines over there 😄. Main developers of course will have a development system and IDE to efficiently develop and commit. It is the spare time contributors to different projects with different programming languages, different frameworks and different CI stacks, like me, which have an easier life when CI checks print the errors.

Okay now pettier should be pretty happy 🙂.

[DL6ER]

@XhmikosR could we have prettier to commit the necessary changes itself from the CI?

[XhmikosR]

Probably... It needs some changes.

1. run prettier with the write flag
2. use an action which commits modified files

Unsure if this will work with forks and PRs, though.

The current situation is not just a big deal IMHO. I mean, if one decides to contribute, they should do it properly locally or after the CI complains.

Just my 2 cents.

[DL6ER]

Thanks, I can see both worlds. Maybe write flag and then show a diff on the CI. Would be a compromise that should work for all. I'll try to allocate some time to look into this (in a separate PR, obviously).

[MichaIng]

use an action which commits modified files

I know such case from Nextcloud, a compile bot action which listens on post commands, and compiles, commits (optionally amends) JS based on changed framework modules/sources. The PR checks do compiling as well and show a list file changed/added/removed files, in case, so maintainers can trigger the bot. But this may not work for PRs from forks, even with "allow maintainer edits" enabled, as access for maintainers is usually limited to changed files and GitHub website/clients. Let me know if I should post some sources for action/bot.

[pralor-bot]

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-9-web-v5-6-and-core-v5-4-released/49544/1