8
\$\begingroup\$

I wrote a very simple CSS parser and would appreciate some general feedback regarding the security, code style etc...

Basically, this does the following:

  1. Take user's uploaded CSS file
  2. Save it to disc
  3. Parse it and return some styles
  4. Save a JSON formatted file to disc

Processing class

<?php
//error_reporting(E_ERROR | E_PARSE);
class process{

    protected $message;
    protected $newFileName;
    protected $css_array = array(); // master array to hold all values

    public function upload(){

        // If no errors, run some security checks, move file and set message
        if($_FILES['css_file']['error'] == 0){

            //Check MIME type. Not using $_FILES['xxx']['type'] because it can't be trusted.
            $mimeType = finfo_file(finfo_open(FILEINFO_MIME_TYPE), $_FILES['css_file']['tmp_name']);

            //Storing all the user uploads outside the web root
            $destination = '/var/www/example.com/parse-css-upload/';
            $this->newFileName = md5(uniqid(rand(), true)) . $_FILES['css_file']['name'];

            //Some severs are having an issue detecting the MIME type of CSS files. This is a quick and dirt hack
            if($mimeType == "text/plain" || "text/x-asm"){

                //Move the file to it's destination. Also renaming the file and setting for a little extra security
                $result = move_uploaded_file($_FILES['css_file']['tmp_name'], $destination . $this->newFileName);

                if($result){
                    $message = "The file was uploaded!";
                } else {
                    $message = "Sorry, there was a problem uploading the file.";
                }

            } else {
                $message = "Sorry, that file type is not allowed.";
            }

        } else {
            //If there is an error, capture it...
            switch($_FILES['css_file']['error']){
                case 2: 
                    $message = "The file was uploaded successfully.";
                    break;
                case 4: 
                    $message = "The file is too big to upload.";
                    break;
            }   
        }

        return $message;

    }

    public function parse(){

        $file = file_get_contents('/var/www/example.com/parse-css-upload/' . $this->newFileName);
        $element = explode('}', $file);
        $element = explode('}', $file);

        foreach ($element as $element) {

            // get the name of the CSS element
            $a_name = explode('{', $element);
            $name = trim($a_name[0]);

            // get all the key:value pair styles
            $a_styles = explode(';', $element);

            // remove element name from first property element
            $a_styles[0] = str_replace($name . '{', '', $a_styles[0]);

            // loop through each style and split apart the key from the value
            $count = count($a_styles);
            for ($a=0; $a<=$count; $a++) {
                if (trim($a_styles[$a]) != '') {
                    $a_key_value = explode(':', $a_styles[$a]);
                    // build the master css array
                    $this->css_array[$name][trim($a_key_value[0])] = trim($a_key_value[1]);
                }
            }               
        }

        //json encode and write a new file
        file_put_contents('/var/www/example.com/parse-css-upload/' . $this->newFileName . ".json", json_encode($this->css_array));

        return $this->css_array;
    }

    public function findElements($element){
        $allElements = array();

        foreach($this->css_array as $value){
            foreach($value as $key => $value){
                if($key == $element){
                    $allElements[] = $value;
                }
            }
        }
        return $allElements;
    }
}


?>

Front end:

<?php require_once('process.php'); ?>
<html>
<head>
    <link rel="stylesheet" type="text/css" href="css/reset.css">
    <link rel="stylesheet" type="text/css" href="css/style.css">
    <link href='http://fonts.googleapis.com/css?family=Open+Sans:300italic,400italic,600italic,400,600,300,700, Cardo, Gentium+Book+Basic:400,400italic,700,700italic' rel='stylesheet' type='text/css'>

    <title>CSS Visual Report</title>
</head>
<body>

    <header>
        <section class="wrapper nobg">
            <h1>CSS Visual Report</h1>
            <h2><span>Upload a css file and see what it contains</span></h2>
        </section>

    </header>

    <section class="wrapper">
        <article class="main">
            <form id="upload_form" enctype="multipart/form-data" method="post" action="">
                <h1 id="label"><label for="css_file">Please select a css file</label></h1>

                <div id="fileSelect">
                    <input type="file" name="css_file" id="css_file" />
                </div>

                <div id="submitButton">
                    <input type="submit" value="upload" name="upload" id="upload" />
                </div>

            </form>
        </article>

        <?php 
            if(isset($_POST['upload'])): 
            $process = new process();
            echo $process->upload();
            $css = $process->parse();
        ?>

        <div class="report">
            <h1 id="reportHeadline">Your CSS file contains:</h1>

            <div id="reportColors">
                <h3>h1 Colors:</h3>
                <h1 style="color:<?php echo $css['h1']['color']; ?>">This is a sample of color <?php echo $css['h1']['color']; ?> </h1>
            </div>

            <div id="reportFonts">
                <h3>Fonts:</h3>
                    <?php 
                        $fonts = $process->findElements('font-family');

                        foreach($fonts as $font){
                            echo "<p style='font-family:" . $font . "'>This is a sample of " . $font . "</p>";
                        }
                    ?>
            </div>

            <div id="">
                <h3>Fonts sizes:</h3>
                    <?php 
                        $fontSizes = $process->findElements('font-size');

                        foreach($fontSizes as $fontSize){
                            echo "<p style='font-size:" . $fontSize . "'>This is a sample of " . $fontSize . "</p>";
                        }
                    ?>
            </div>
        </div>

        <?php endif ?>

    </section>

</body>
</html>
\$\endgroup\$
3
  • \$\begingroup\$ Keep in mind that if you want to adhere to the specification, there are some (quite specific) rules you must follow to parse it correctly. \$\endgroup\$
    – icktoofay
    Jul 26, 2014 at 3:15
  • \$\begingroup\$ May I ask why you decided to write a CSS parser? \$\endgroup\$
    – Toby Allen
    Jul 27, 2014 at 7:18
  • \$\begingroup\$ @TobyAllen - A specific part of a project called for it. \$\endgroup\$ Jul 29, 2014 at 0:47

2 Answers 2

7
\$\begingroup\$

I'll mostly judge the actual parsing part, and not that much the boilerplate stuff around it

Why explode the file twice?

$element should be called $elements, and this loop won't really work as expected:

foreach ($element as $element)

There can be multiple selectors for one block:

h2, b {
    ...

Currently that counts as only one in your code.

What about comments?

/* This is for bold text*/ 
b {

gives /* This is for bold text*/ LINEBREAK b as name in your example. Certainly not right.

When you try to remove the element name from the key:values what happens if I put a space before the {? Actually, why not split $a_name[1]?

Counting to < count($styles) is enough (that also saves you the trimming); PHP indexes arrays from 0 on.

Then you never check for errors. If it doesn't have any { in it, it can't be a CSS file, no need to parse further. Or what if your $a_key_value has something but 2 elements? That's an error and should be noted somehow.

And so on for basically every step you do.

Read the specs when doing this. E.g. did you know about the many types of selectors (I only knew 4 of those and when I had written this parser I would totally have rejected E + F and similar)

For some security: An attacker could use your file_get_contents() call to open any file on your server. All he needs to do is set the file name to "/../../../../../etc/passwd". You prepend some hash and some paths so your $this->newFileName becomes /var/www/example.com/HASH/../../../../etc/passwd. The ../ means go one directory up so your end up with just /etc/passwd.

And then you try to parse that as CSS. It's highly unlikely that it will contain valid CSS but as you currently don't try to stop the parsing, some information from it can be read.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the comments. Yeah, the parser is really weak right now. It's more of a proof of concept. Thanks for pointing out the element vs elements typo. RE the file_get_contents() can you elaborate on how an attacker could open any file? I'm renaming the file here $this->newFileName = md5(uniqid(rand(), true)) . $_FILES['css_file']['name']; then storing it outside of the web root. So, as far as I know, the attacker wouldn't have the ability to choose a file name. \$\endgroup\$ Jul 26, 2014 at 4:16
  • \$\begingroup\$ @PaulDessert I added an example as the attacker \$\endgroup\$
    – user45891
    Jul 26, 2014 at 12:12
4
\$\begingroup\$

About your HTML

You are missing the DOCTYPE. As you are using HTML5, it is:

<!DOCTYPE html>

Unless you set the character encoding on the HTTP level, you should add a meta element. In case of UTF-8:

<meta charset="utf-8">

(It should come before any element possibly containing non-ASCII, so place it as the first child of head.)

Your first section is wrong (i.e., it creates a wrong document outline). As "CSS Visual Report" seems to be the document heading, it should belong to body, not to a sectioning element.

Don’t use a heading (h2) for the slogan/subtitle ("Upload a css file and see what it contains"). While HTML5 had introduced the hgroup element for such case, it is now obsolete. Instead, you should simply use p . For reference, see Subheadings, subtitles, alternative titles and taglines in the HTML5 spec.

Using article for the upload form is not wrong, but I’d go with a section here (because the whole page is an app for uploading and parsing the CSS, and having the upload form stand "on its own" doesn’t seem to make sense).

The section with the wrapper class doesn’t seem to make sense; it should be a div if you need it only for styling reasons.

The div with the report class could be a section instead.

The three "result" div elements could be section element as you are using headings anyway. In any case, their headings should not skip one level: either use h1 everywhere (if you use sectioning elements!), or use the logical order.

The color result should not use h1 for "This is a sample of color […]", as it creates a wrong document outline.

You have a div with an empty id (id="").

So, omitting everything not relevant for the mentioned points, it would be:

<!DOCTYPE html>
<html>

<head>
  <meta charset="utf-8">
  <title>…</title>
</head>

<body>

  <header>
    <h1>CSS Visual Report</h1>
    <p>Upload a css file and see what it contains</p>
  </header>

  <section>
    <h1>Please select a CSS file</h1>
  </section>

  <section>
    <h1>Your CSS file contains:</h1>

    <section>
      <h2>Colors:</h2>
    </section>

    <section>
      <h2>Fonts:</h2>
    </section>

    <section>
      <h2>Fonts sizes:</h2>
    </section>

  </section>

</body>
</html>
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.