4
\$\begingroup\$

I have an image cropper that crops the uploaded image, Before sending it to the server using PHP and AJAX.

Here is a live fiddle to preview a working cropping example.

Here is the code:

// Create cropped part.
function getRoundedCanvas(sourceCanvas) {
  var canvas = document.createElement('canvas');
  var context = canvas.getContext('2d');
  var width = sourceCanvas.width;
  var height = sourceCanvas.height;
  canvas.width = width;
  canvas.height = height;
  context.imageSmoothingEnabled = true;
  context.drawImage(sourceCanvas, 0, 0, width, height);
  context.globalCompositeOperation = 'destination-in';
  context.beginPath();
  context.arc(width / 2, height / 2, Math.min(width, height) / 2, 0, 2 * Math.PI, true);
  context.fill();
  return canvas;
}

// On uploading a file
$("#input").on("change", function(e) {
  var _URL = window.URL || window.webkitURL,
      file = this.files[0],                   
      image = new Image();
  image.src = _URL.createObjectURL(file);    
  image.onload = function(e) {
    var image = document.getElementById('image'),
        button = document.getElementById('button');
    $('#image').attr('src',  _URL.createObjectURL(file));
    $('#image').show();
    $('#button').show();
    var cropper = new Cropper(image, {
      aspectRatio: 1,
      movable: false,
      cropBoxResizable: true,
      dragMode: 'move',
      ready: function () {
        croppable = true;
        button.onclick = function () {
          var croppedCanvas;
          var roundedCanvas;
          var roundedImage;
          if (!croppable) {
            return;
          }
          // Crop
          croppedCanvas = cropper.getCroppedCanvas();
          cropper.getCroppedCanvas({
            fillColor: '#fff',
            imageSmoothingEnabled: true,
            imageSmoothingQuality: 'high',
          });
          // Round
          roundedCanvas = getRoundedCanvas(croppedCanvas);
          // Show
          roundedImage = document.createElement('img');
          roundedImage.src = roundedCanvas.toDataURL();
          result.innerHTML = '';
          result.appendChild(roundedImage);

        }
      }
    });
  }
}); 
#image,
#button{
  display:none
}
<!-- Cropper CSS -->
<link rel="stylesheet" type="text/css" href="https://cdnjs.cloudflare.com/ajax/libs/cropperjs/1.4.1/cropper.min.css">
 
 <!-- Cropper JS -->
 <script src="https://cdnjs.cloudflare.com/ajax/libs/cropperjs/1.4.1/cropper.min.js"></script>
 <script src="http://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.js"></script>
 
 <!-- File input -->
 <input type="file" id="input" name="image">
 <!-- Selected image will be displayed here -->
 <img id="image" src="" alt="Picture">
 
 <!-- Button to scrop the image -->
 <button type="button" id="button">Crop</button>
 
 <!-- Preview cropped part -->
 <div id="result"></div>

Upload an image and click crop, The cropped part would be displayed beneath it.

Then I use the following code to send a blob to PHP using Ajax:

cropper.getCroppedCanvas().toBlob(function (blob) {
  var formData = new FormData();
  formData.append('avatar', blob);
  // Use `jQuery.ajax` method
  $.ajax('upload.php', {
    method: "POST",
    data: formData,
    processData: false,
    contentType: false,
    success: function (response) {
    },
    error: function () {
    }
  });
});

In upload.php:

if( isset($_FILES['avatar']) and !$_FILES['avatar']['error'] ){
  file_put_contents( "uploads/image.png", file_get_contents($_FILES['avatar']['tmp_name']) );
}

Should I make some checks for security or just the dimensions and size checks?

\$\endgroup\$
1
\$\begingroup\$

Should I make some checks for security or just the dimensions and size checks?

It would be simple to check the type of the file uploaded. For example, you could check if the type is an image. For example, if the user uploads a PNG file, $_FILES['avatar']['type'] should be image/png, so a check for that value starting with image would be a simple conditional (e.g. using strpos() and looking for it to return 0, or preg_match('^image/', $_FILES['avatar']['type]).

The HTML could also be updated to only accept certain file types. One technique for this is to add the accept attribute to the input element with a value like image/* to limit the selectable files to images. While this does not prevent a user from selecting any file (or modifying a file to look like an image), it can act like a filter to the file selection dialog if the OS and browser support it. Read more about this in answers to Limit file format when using ?

<input type="file" id="input" name="image" accept="image/*">

One could also add a change handler to check the extension of the file submitted (bear in mind that the user can also change the extension of a file through the OS).

var allowedExtensions = ['jpg', 'gif', 'bmp', 'png', 'tif'];
document.addEventListener('DOMContentLoaded', function() {
  var fileInput = document.getElementById('input');
  fileInput.addEventListener('change', function(e) {
    var parts = this.value.split('.');
    if (allowedExtensions.indexOf(parts[parts.length - 1].toLowerCase()) < 0) {
      this.value = ''; // clear value
      console.log('invalid extension');
    }
  });
});
<input type="file" id="input" name="image" accept="image/*">

Combining one or both of those approaches with the server-side check would be a good approach. Bearing in mind that a user could alter the HTML attributes with tools like browser consoles, as well as submit a form manually with their own page or a tool like Postman, the server-side check should be utilized for optimal protection.

Other Feedback

JavaScript

Variable croppable

It apppears that croppable is set to true in the ready function:

ready: function () {
    croppable = true;
    button.onclick = function () {

While it would be undefined before that line, the onclick function wouldn't be set until after that is set to true, so the following block seems superflous in the onclick function:

if (!croppable) {
    return;
}

This is because by the time the click handler is set on the button, that variable will have been set to true so the condition will never evaluate to a truthy value.

I also noticed in that onclick function that three variables (i.e. croppedCanvas, roundedCanvas, and roundedImage) are declared before the conditional return. There isn't really any reason to declare those variables before the condition. Then those variables can be declared whenever assigned.

Cache DOM references

DOM lookups aren't exactly cheap, so it may be better to store references to elements fetched via document.getElementById() as well as using the jQuery function in variables and referencing those variables later.

Use consistent DOM lookups

Much of the code uses jQuery to look up elements, but then there are other places using document.getElementById(). If you are going to have jQuery, it makes sense to use it for all DOM lookups - for the case of getting a single element (e.g. image and button) .get() can be used to access the element.

So these lines:

var image = document.getElementById('image'),
    button = document.getElementById('button');

Can be simplified using jQuery and .get():

var image = $('#image').get(0),
    button = $('#button').get(0);   

Variable name reused

While moving image and button out of the onload callback I noticed that image was used for an Image object and then inside the callback re-assigned to the element with id attribute image. It is wise not to re-use variables like that... Perhaps a better name for the variable referencing the element would be imageElement.

PHP

You state that upload.php contains the following:

if( isset($_FILES['avatar']) and !$_FILES['avatar']['error'] ){
  file_put_contents( "uploads/image.png", file_get_contents($_FILES['avatar']['tmp_name']) );
}

Is there any error handling/response to the client as to to activity on the server? What happens if the write to "uploads/image.png" would fail?

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for the answer, But I didn't get exactly what you said about croppable, Bothcroppable and onclick are inside the ready function, Do you mean that it will always be true, So I don't need that condition? \$\endgroup\$ – Bon Sep 7 '18 at 4:40
  • \$\begingroup\$ Yes- does the updated answer provide a better explanation? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Sep 7 '18 at 5:15
  • \$\begingroup\$ I copied the code from couple examples from the plugin developer, So it's not ordered/cleaned properly. \$\endgroup\$ – Bon Sep 7 '18 at 18:25
  • \$\begingroup\$ Ah yes... the code looks very similar to the upload-cropped-image-to-server.html. It is a good thing you included your PHP code - otherwise it would be like a review of fengyuanchen's cropperjs code. In the future it might be advisable to disclose the original source of the code \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Sep 7 '18 at 18:37
  • \$\begingroup\$ Actually it's a combination from 3 examples, Modal, Upload to server and Round image, So it's a little messy :) \$\endgroup\$ – Bon Sep 8 '18 at 5:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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