WPTT / WPThemeReview

PHP_CodeSniffer rules (sniffs) to enforce WordPress theme review coding conventions
MIT License
209 stars 37 forks source link

[New sniff ?] Have either UNIX or DOS line endings for TXT files #4

Closed jrfnl closed 6 years ago

jrfnl commented 8 years ago

Rule:

Have either UNIX or DOS line endings, not both. This rule gets applied to PHP, CSS, JS and TXT files.

Ref: https://make.wordpress.org/themes/handbook/review/required/theme-check-plugin/#line-endings

Theme check file covering this rule:

https://github.com/Otto42/theme-check/blob/master/checks/lineendings.php

Decision needed:

Currently the Theme Check plugin check also checks the line endings in .txt files. This is not covered by the existing Generic.Files.LineEndings sniff and quite likely can't be covered by it as no tokenizer for .txt files is available within PHPCS.

A decision is needed on if and if so, how to continue checking the line endings for .txt files.

To do:

Related: #3

grappler commented 8 years ago

We may have some other sniffs in the future for text files. As the text file would normally only contain text we would not need to tokenize but process it differently. I am also thinking about the readme validator for plugins https://wordpress.org/plugins/about/validator/.

joyously commented 8 years ago

Is there a possibility for txt files to get renamed to php or js, or would this be found by the File System functions check?

jrfnl commented 8 years ago

Is there a possibility for txt files to get renamed to php or js, or would this be found by the File System functions check?

@joyously I'm not sure what you mean. Could you elaborate a bit ?

joyously commented 8 years ago

I was just thinking that a file could be named with a txt extension, but have code in it. I didn't know if all the code checks would be run on the txt file, or if the check for File System calls (such as rename) would catch code that tries to rename one of the theme files.

jrfnl commented 8 years ago

I was just thinking that a file could be named with a txt extension, but have code in it.

Files with a .txt extension will not be executed by the server, so we don't really need to concern ourselves with those for the code specific checks. Similarly, a browser won't execute js code found in a .txt file. (well, ok, they could be executed, but only if you apply some very dirty .htaccess hacks which will get you banned from any reputable webhost. Actually - it might not be a bad idea to check that a theme does not contain any .htaccess files for that matter).

if the check for File System calls (such as rename) would catch code that tries to rename one of the theme files

Interesting thought (and scary). I'd have to check what the specific rule is here about file system calls, but if they are forbidden, then yes, they would (should) catch attempts to rename files. But a theme - or plugin - for that matter, renaming files within their own installed code base is something I've not come across before and sounds wickedly evil.

jrfnl commented 8 years ago

Oh and @joyously - if you believe either of those points should be turned into sniffs, please open separate issues for them. This issue is specifically about the line ending check for txt files.

Pross commented 8 years ago

(dot) files are blocked by the theme uploader already. see https://github.com/Otto42/theme-check/blob/master/checks/filenames.php

joyously commented 8 years ago

@jrfnl Somehow when I read this sniff, my mind returned to an old article I read recently http://ottopress.com/2010/anatomy-of-a-theme-malware/ and other articles I've read about putting data on the end of a jpg, and I wondered if you could trust file extensions, and whether these sniffs are run only on certain files(according to file extensions). And it seems that even going through the WordPress File System would allow you to rename(move) a file. I'm not devious enough to be a hacker, and I don't know enough about this sniff code to know how to contribute except to ask my original questions.

jrfnl commented 8 years ago

There are ways to check files for mimetype - that would give some indication if for instance a .zip file would have been renamed to .txt, it could flag that. But this is not a catch-all (though could catch a lot).

ginsterbusch commented 8 years ago

Possibly related with @joyously 's comment : Also think about things like include('my_php_file_with_wrong_file_extension.txt').

With all the ugly spaghetti horror scenarios I've had to work with in the last 2 years - mostly takeover jobs - I see that as a definite possibilty. Yep, its insane, but there are so many insane "btw: I'm originally a print graphic designer and never properly learned PHP"-self declared developers that have no clue what they're doing, mostly copy-and-pasting themselves through "programming" .. I'd call it a given.

cu, w0lf.

ps: maybe it'd help gathering all those nut jobs / nut cases once in a while and then use that to create better testing routines? Its mostly crappy premium themes, too many folks having worked (or ARE working) on the same job, or above described apocalyptic scenario.

jrfnl commented 8 years ago

maybe it'd help gathering all those nut jobs / nut cases once in a while and then use that to create better testing routines?

Creating new unit tests for sniffs is incredibly easy. Please feel invited to start creating/adding them. The better the test cases we have, the better sniffs we can create ;-)

grappler commented 7 years ago

This should be solved by #3

jrfnl commented 7 years ago

This should be solved by #3

Actually, no, it shouldn't. #3 deals with PHP, CSS and JS files. This issue is about the same for TXT files.

jrfnl commented 6 years ago

While this may be needed for the wp.org markdown parser, this is not something which can be checked for by PHPCS. If we tell PHPCS to parse txt files as PHP, it would possibly also trigger on example code contained in the readme, thus creating false positives we don't want.

This would be better solved from within the Theme Check plugin itself, just scanning the readme.txt file.