Image preview and delete for Laravel blog application

Question

I am working on a blogging application in Laravel 8 .

I have put together a way to preview and remove the main article image.

In the ArticleController.php controller, I have a method for removing the image from the article and deleting the file from the directory:

public function deleteImage($id, $fileName) {
 $article = Article::find($id);
 $article->image = "default.jpg";
 $article->save();
 if (File::exists(public_path('images/articles/' . $fileName))) {
 File::delete(public_path('images/articles/' . $fileName));
 }
}

In resources/views/dashboard/edit-article.blade.php I have the edit article form, where the image preview code is the following:

<form method="POST" action="{{ route('dashboard.articles.update', [$article->id]) }}"
 enctype="multipart/form-data" novalidate>
 @csrf
 <input type="hidden" id="defaultImage" name="defaultImage" value="{{ asset('images/articles/default.jpg') }}" />
 <div class="col-md-12 post-image text-center">
 <img id="imagePreview" class="image-preview large"
 src="{{ asset('images/articles/' . $article->image) }}" alt="{{ $article->title }}">
 @if($article->image !== 'default.jpg')
 <a class="remove-image edit" href="#" id="delete-image" data-uid="{{$article->id}}" title="Remove image" onclick="removeImage(event)">
 <i class="fa fa-remove"></i>
 </a>
 @endif
 </div>
 </div> 
</form>
@include('partials.image-preview-script')

In resources/views/dashboard/eadd-article.blade.php I have the add article form, where the image preview code is the following:

<form method="POST" action="{{ route('dashboard.articles.add') }}" enctype="multipart/form-data" novalidate>
 @csrf
 <input type="hidden" id="defaultImage" name="defaultImage" value="{{ asset('images/articles/default.jpg') }}" /> 
 
 <div class="row mb-3">
 <label for="image" class="col-md-12">{{ __('Article image') }}</label>
 <div class="col-md-12 post-image @error('image') has-error @enderror">
 <input type="file" value="{{ old('image') }}" name="image" id="file"
 class="file-upload-btn" onchange="previewImage(event)">
 @error('image')
 <span class="invalid-feedback" role="alert">
 <strong>{{ $message }}</strong>
 </span>
 @enderror
 </div>
 </div>
 <div class="row mb-3 d-none">
 <div class="col-md-12 post-image text-center">
 <img id="imagePreview" class="image-preview large" src="">
 <a class="remove-image" href="#" title="Remove image" onclick="removeImage(event)">
 <i class="fa fa-remove"></i>
 </a>
 </div>
 </div>
 
</form>
@include('partials.image-preview-script')

In routes/web.php I have added:

Route::post('/delete-image/{id}/{fileName}', [ArticleController::class, 'deleteImage'])->name('dashboard.articles.delete-image');

The script (used by both edit and delete actions):

<script>
 function previewImage(event) {
 var img = document.getElementById('imagePreview');
 var imageContainer = img.parentNode.parentNode;
 var input = event.target;
 var reader = new FileReader();
 reader.onload = function() {
 img.src = reader.result;
 if (window.getComputedStyle(imageContainer).display === 'none') {
 imageContainer.classList.remove('d-none');
 }
 };
 reader.readAsDataURL(input.files[0]);
 }
 function removeImage(event) {
 var defaultImg = document.getElementById('defaultImage').value;
 var input = document.getElementById('file');
 var img = document.getElementById('imagePreview');
 var imageContainer = img.parentNode.parentNode;
 event.preventDefault();
 if (event.currentTarget.classList.contains('edit')) {
 var id = event.currentTarget.dataset.uid;
 var fileName = img.getAttribute('src').split('/').reverse()[0];
 var url = `${APP_URL}/dashboard/articles/delete-image/${id}/${fileName}`;
 if (confirm('This action is irreversible. Are you sure?')) {
 var CSRF_TOKEN = document.querySelectorAll('meta[name="csrf-token"]')[0].getAttribute('content');
 var xmlhttp = new XMLHttpRequest();
 xmlhttp.onreadystatechange = function() {
 if (xmlhttp.readyState == XMLHttpRequest.DONE) {
 if (xmlhttp.status == 200) {
 img.src = defaultImg;
 document.getElementById('delete-image').remove();
 }
 }
 }
 xmlhttp.open('POST', url, true);
 xmlhttp.setRequestHeader("X-CSRF-TOKEN", CSRF_TOKEN);
 xmlhttp.send();
 }
 } else {
 imageContainer.classList.add('d-none');
 img.src = "";
 input.value = "";
 }
 }
</script>

Questions:

1. Is there any redundancy in my code?
2. Do you see any security issues?
3. Do you see any usability or UX issues?

• Sᴀᴍ Onᴇᴌᴀ: Hi- would you be able to edit to include the route file - i.e. which binds the route to ArticleController::deleteImage()?
• Razvan Zamfir: @SᴀᴍOnᴇᴌᴀ Done. Also, the link leads to the pull request for the feature.
• Sᴀᴍ Onᴇᴌᴀ: Kool. Do you run this on localhost without a secure channel? When I do that and attempt to delete an article image it shows a CORS error in the console - see i.sstatic.net/YjYVjv7x.png The OPTIONS request to localhost/dashboard/articles/delete-image/1/… shows a status is a 502 Bad Gateway with no content and nothing is logged in the PHP log or Laravel log 🤔
• Razvan Zamfir: @SᴀᴍOnᴇᴌᴀ I use a virtual host, with HTTPS.
• Sᴀᴍ Onᴇᴌᴀ: oh nice- I'll need to add that to my toolbox

Answer 1

Is there any redundancy in my code?

Not a significant amount is found, though in the Javascript functions previewImage() and removeImage() both contain this line:

 var img = document.getElementById('imagePreview');

DOM queries are expensive and while might not be a major concern with modern browsers and systems with more resources one could still cache references to DOM elements and eliminate redundancies.

Do you see any security issues?

Ensure data coming in to API request is formed as expected

In a previous review there was a suggestion:

An unbreakable security rule is "Never Trust User Input!"

So, any data sent to the server must be sanitized and validated. This is true even for some identifier (like a user_id) that is part of a URL that you put on the screen. With that in mind, you should always (at the very minimum) make sure that any model function that has a $id argument is getting the right datatype. ¹

As you may be aware Laravel offers methods like whereNumber(), whereAlphaNumeric(), etc. While it may not provide much in terms of security one could also add type hints to the parameters in the controller method to ensure the parameters have expected types - e.g. int, string, bool, etc.

Consider using newer AJAX API

One could use the Fetch API, the Axios Http client, or even jQuery's XHR methods to simplify the handling of making the request and handling the response. Doing so can lead to eliminating conditions like these:

 if (xmlhttp.readyState == XMLHttpRequest.DONE) {
 if (xmlhttp.status == 200) {

Consider using the DELETE HTTP verb

It isn't wrong to use a POST request for the AJAX request to /dashboard/articles/delete-image/${id}/${fileName}, however many REST APIs utilize the HTTP verb DELETE when data is to be deleted. The Laravel routing system supports that verb with Route::delete($uri, $callback). Then the URL does not need to contain the word delete.