Fix unintentional file globbing during wildcard lookup

[LoganK]

Matching globs are common because the script runs in the certs
directory.

The test uses a suffix match as the test domains don't include
subdomains, although such cases should probably be considered.

Fix the le3.wtf test. The existing add_location_configuration modifies
"default"; a second add is not necessary.

Fixes #763

[LoganK]

Apologies for the re-pushes to trigger tests. It seems they can be a bit flaky with respect to timeouts?

(Everything passes locally.)

[buchdag]

Apologies for the re-pushes to trigger tests. It seems they can be a bit flaky with respect to timeouts?

Actually there's only one test that is super flaky, the default certificate test : https://github.com/nginx-proxy/docker-letsencrypt-nginx-proxy-companion/blob/master/test/tests/default_cert/run.sh

I haven't found why yet, I'm not sure it's just a timeout issue as I saw it happen locally too.

Thanks for the fast PR, could you use / do what Shellcheck suggests before we merge it ?

[LoganK]

So it turns it was really fortunate you pushed back: the test cleanup was incomplete so subdomain.example.com was only passing because I had executed the tests with the broken build. I'm not sure how the GitHub actions were passing before; either they also don't clean up, or I'm getting tired and missing something.

In any case, worth a complete re-review as that triggered a small number of related changes.

Thanks for the fast PR, could you use / do what Shellcheck suggests before we merge it ?

Done.

[buchdag]

You might have gone a bit too complex with the test, I think it should be way easier and probably sufficient to just test the enumerating function with a file that could glob in the current working dir.

[buchdag]

Something like

# check the wildcard location enumeration function
docker exec "$le_container_name" bash -c \
    'source /app/functions.sh; \
    touch wrong.baz.example.com foo.bar.wrong; \
    enumerate_wildcard_locations foo.bar.baz.example.com; \
    rm wrong.baz.example.com foo.bar.wrong'

wrong.baz.example.com and foo.bar.wrong existing in the current working dir shouldn't be lead to them being matched by enumerate_wildcard_locations().

[LoganK]

Interesting comment. Two rebuttals:

• enumerate_wildcard_locations is insufficient because it doesn't actually create files or expose the globbing issue. add_location_configuration is necessary unless we want to completely refactor.
• I didn't add any new tests cases, only fixed two that were already there.

I would argue that the only new complexity was verifying the absence (false_config_path) which also triggered validating the file existence. This was only because the test cases aren't hermetic, and I wanted to make sure that the redirect wasn't left over.

For subdomain.example.com, I can:

• Do the above checks
• Ensure "*.example.com" is empty before calling add_location_configuration. (Similar to other tests.)
• Do nothing. It's crazy to think that *.example.com would be modified by the three tests above it.

It sounds like your preference is the third point, which I can prep easily enough.

[LoganK]

Option 3 pushed. I think it might be the fewest number of changes to properly test this case.

[buchdag]

Okay I think I might have issues wrapping my head around this.

So yeah using a while read loop instead of a for loop actually fix the issue without using set -o noglob / set +o noglob, I did not initially understand that.

enumerate_wildcard_locations is insufficient because it doesn't actually create files or expose the globbing issue. add_location_configuration is necessary unless we want to completely refactor.

Got that too.

I would argue that the only new complexity was verifying the absence (false_config_path) which also triggered validating the file existence. This was only because the test cases aren't hermetic, and I wanted to make sure that the redirect wasn't left over.

Make sense now that I'm not under the false impression that testing enumerate_wildcard_locations would be enough.

Do you wish to roll back the change made to test/tests/location_config/run.sh to 1a74cdc ?

Don't bother force pushing to re trigger the test, I'll do it manually until they pass.

[LoganK]

So yeah using a while read loop instead of a for loop actually fix the issue without using set -o noglob / set +o noglob, I did not initially understand that.

Yeah, using set and restore was pretty obnoxious. read accepts one line of data very neatly ... at the cost of the input being at the end of the loop (which is extra weird).

(I did try to pipe to the read, but it wasn't working and here-string works so well...

Do you wish to roll back the change made to test/tests/location_config/run.sh to 1a74cdc ?

I am around to your way of thinking, now: it's somewhat overkill to check both that it was added to the correct file and not added to the incorrect file. It would make the tests more robust when bad files are left around between test runs (what I originally ran into), but the automated tests and --teardown / --setup both work correctly. In other words, the extra tests work around issues with the current test setup but do not add significant value.

Happy to re-push if you ask again, but I'm pretty happy with this minimal set of changes.

Don't bother force pushing to re trigger the test, I'll do it manually until they pass.

Special powers are nice.

[buchdag]

Merged, thanks @LoganK !