cowsay-org / cowsay

apjanke's fork of the classic cowsay project
http://cowsay.diamonds
GNU General Public License v3.0
83 stars 15 forks source link

cowpath.d/*.path files add unwanted entries to cowpath #28

Open ndim opened 2 years ago

ndim commented 2 years ago

If I have a file in /etc/cowsay/cowpath.d/foo.path which is something like

/usr/share/cowsay-foo/cows

/usr/share/cowsay-bar/cows

then the empty line makes cowsay (version 3.7.0) look for cows in the / directory.

I have verified that cowsay looks for cows in / by putting a special cowfile into /xyz.cow and then calling date | cowsay -f xyz and seeing a cow. If I delete the empty line in the *.path file, I can run date | cowsay -f xyz and see that cowsay cannot find the xyz cow.

It appears that if the *.path file contains comment lines like

# Add the foo cow collection
/usr/share/cowsay-foo/cows

# Add the bar cow collection
/usr/share/cowsay-bar/cows

installing a cow file as /# Add the foo cow collection/xyz.cow, then running date | cowsay -f xyz will not produce a cow, even though a print join(", ", @cowpath); after the uniquify_list line will print a path entry "# Add the foo cow collection".

120                 while (my $entry = <$fh>) {
121                     chomp $entry;
122                     push @default_cowpath, $entry;
123                 }

Would it make sense to

?

IIRC this would mean adding two lines after line 121 like

                        next if $entry =~ /^$/;
                        next if $entry =~ /^#/;

Another way this could be accomplished is by only adding $entry values which start with the letter / (after chomping).

Maybe add this after line 121

                         next unless $entry =~ /^\//;

?

apjanke commented 2 years ago

Oops! Blank lines (including nonempty all-whitespace lines, I think) should be ignored.

Let's make "#" a special character that is a comment introducer, and backslash-escape it like "\#" for a literal "#" in the file name.

Would it make sense to... stop pushing empty string value [...and...] stop pushing $entry values starting with a #

Yes. Though I'd do it slightly different like "strip #-introduced comments, and then drop $entry if all that remains is blank (empty or all-whitespace)".

only adding $entry values which start with the letter /

Oh, hmm: This sounds like a good approach too. I think all paths in a cowpath should be absolute. (I'll need to review the code.) I'm not sure it makes sense to have relative/non-absolute paths in it, at least in the parts of cowpath that are defined by system or user level config files.

Maybe we should even support bash-like ~-expansion in cowpath entries, to allow a system to set up a custom location for per-user cowdirs?

ndim commented 2 years ago

Oops! Blank lines (including nonempty all-whitespace lines, I think) should be ignored.

Common config file properties. Agreed.

Let's make "#" a special character that is a comment introducer, and backslash-escape it like "\#" for a literal "#" in the file name.

String escaping always sounds difficult to implement properly. Easier/safer to entirely avoid it.

Would it make sense to... stop pushing empty string value [...and...] stop pushing $entry values starting with a #

Yes. Though I'd do it slightly different like "strip #-introduced comments, and then drop $entry if all that remains is blank (empty or all-whitespace)".

I would keep both the description of the file format and the code handling ist as simple as possible, while allowing for common config file properties, e.g.

This allows common config file properties like comments and spacing via empty lines with really simple code, while leaving some more things undefined for possible future extension.

Possible problem: While Windows supposedly has at least partial support for / as the separator character, this still disallows c:/path/to/cows. Is there a Perl standard library function like path_is_absolute which could be used instead of cowsay looking and testing for / characters? This would change the last file format definition item

only adding $entry values which start with the letter /

Oh, hmm: This sounds like a good approach too. I think all paths in a cowpath should be absolute. (I'll need to review the code.)

I think the code should actively avoid non-absolute entries to the cowpath.

I'm not sure it makes sense to have relative/non-absolute paths in it, at least in the parts of cowpath that are defined by system or user level config files.

Maybe we should even support bash-like ~-expansion in cowpath entries, to allow a system to set up a custom location for per-user cowdirs?

As cowdirs are dirs to be filled with executable code which cowsay will run, I would think long and hard about the security implications of non-absolute and $HOME based cowdirs before introducing them. Post 3.8.0 maybe?

apjanke commented 2 years ago

Okay, I agree. Because it's incorporating executable code, being conservative is probably the right call.

Let's keep it simple, and only "whitelist" known-probably-valid entries like you describe here: "#" is only a comment introducer in column 1; all cowpaths must start with /; ignore everything else (with maybe a warning to console or syslog for invalid non-blank lines).

Windows supposedly has at least partial support for / as the separator character

The Windows kernel and low-level Win32 APIs fully support /. Many (maybe most?) applications and third-party userland libraries do not. But if we don't use \ as an escape character, we could just strrep \ to / before processing, and then convert it back to the native separator afterwards?

But crap... I didn't even think that cowsay supported Windows? (At least, in Windows mode; under Cygwin or WSL should be fine, because that looks like Linux.) Crap, gotta do some reading here.

ndim commented 2 years ago

Okay, I agree. Because it's incorporating executable code, being conservative is probably the right call.

Let's keep it simple, and only "whitelist" known-probably-valid entries like you describe here: "#" is only a comment introducer in column 1; all cowpaths must start with /; ignore everything else (with maybe a warning to console or syslog for invalid non-blank lines).

Sounds good to me, except that syslog is not simple. If it was a new feature, I would die, warn on stderr, or ignore (in order of decreasing preference). It is an old feature, though, so die could be a bit too much incompatibility.

Windows supposedly has at least partial support for / as the separator character

The Windows kernel and low-level Win32 APIs fully support /. Many (maybe most?) applications and third-party userland libraries do not. But if we don't use \ as an escape character, we could just strrep \ to / before processing, and then convert it back to the native separator afterwards?

But crap... I didn't even think that cowsay supported Windows? (At least, in Windows mode; under Cygwin or WSL should be fine, because that looks like Linux.) Crap, gotta do some reading here.

Does cowsay support Windows? I have no idea. I would ask the cowsay maintainer, but you do not have that option.

Is there even a Perl on Windows? My last time on a Windows system was in 2003 using MSYS2, so I am not very up to date.

Anyway, if you say cowsay is for unix-like systems only, the / check is trivial even without an isabsolutepath function.

ndim commented 2 years ago

One strong argument against cowsay supporting Windows is cowsay relying on symlinks in the source tree.

While allegedly Windows has started supporting symlinks, symlinks appear to work only when the Administrator has explicitly enabled them.

apjanke commented 2 years ago

While allegedly Windows has started supporting symlinks, symlinks appear to work only when the Administrator has explicitly enabled them.

Symlinks in Windows are an incredible mess. (I know this because I love me some symlinks and have tried repeatedly to make the work on Windows.) They (and junction links) have been supported in NTFS and the core OS for years and years, but AFAICT they're not something Microsoft actually cares about, and almost no other Windows software, including the "upper" levels of Windows itself, is aware of them, so even if an administrator has enabled them, you just know something's gonna break. I would like to stay far away from symlinks on Windows.

I wish we had some sort of userbase survey so we could tell if anyone's using cowsay on Windows. And how they're getting it to run? The cowsay file itself is a +x mode Perl script with a #!/user/bin/env perl shebang line; pretty sure that mechanism isn't going to work at all.

Googling "cowsay on windows" does produce a bunch of hits. (And cowsay is only $.99 in the Microsoft Store!) We should take a look at those and see what people are doing. https://github.com/cowsay-org/cowsay/issues/30

Sounds good to me, except that syslog is not simple. If it was a new feature, I would die, warn on stderr, or ignore (in order of decreasing preference). It is an old feature, though, so die could be a bit too much incompatibility.

Makes sense. Let's warn on stderr.

ndim commented 2 years ago

I wish we had some sort of userbase survey so we could tell if anyone's using cowsay on Windows. And how they're getting it to run? The cowsay file itself is a +x mode Perl script with a #!/usr/bin/env perl shebang line; pretty sure that mechanism isn't going to work at all.

Googling "cowsay on windows" does produce a bunch of hits. (And cowsay is only $.99 in the Microsoft Store!) We should take a look at those and see what people are doing. https://github.com/cowsay-org/cowsay/issues/30

I would guess that people do the same thing on/for Windows which the Fedora cowsay package is doing pre-PR#26: Do their own system specific install steps.

With the notable difference that I do the Fedora package and upstream its important aspects as PR#26 free of charge, and they charge users 99 cents for the effort and keep their efforts to themselves.

apjanke commented 2 months ago

Scheduling this for 3.9.0. Seems feasible to fix this with a conservative "support blank and comment lines" parsing approach pretty shortly here.

I'm just giving up on doing Windows-specific support here, on the basis that Cowsay is supposed to be fun, and packaging up a Windows Perl app does not sound like fun. Someone else can do that, or folks can get this cowsay through WSL, where symlinks work fine AFAICT. :)