Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue 670. Image Upload Validation. #1164

Merged
merged 12 commits into from
Mar 22, 2025
Merged

Issue 670. Image Upload Validation. #1164

merged 12 commits into from
Mar 22, 2025

Conversation

mattburnett-repo
Copy link
Member

@mattburnett-repo mattburnett-repo commented Mar 17, 2025 •
edited
Loading

Contributor checklist

Description

This PR addresses the issues presented in #670 .

Backend

  • Validates image upload file type and content.
  • Limits uploaded file size to 5MB.
  • Sanitizes uploaded image file names to UUID filenames.
  • Scrubs exif data / meta data from jpeg / png format files.
    • Uses Pillow. More detailed exif scrubbing is possible through the piexif module.
  • Adds pytest tests to content/tests/test_image_upload.py to increase test coverage for image upload.

Frontend

  • Added nuxt-security module and set up first iteration of config.
    • This provides header- and middleware-based protection against OWASP Top Ten threats, as well as:
      • Image file source / location restriction as part of broader Content Security Policy.
      • Rate limiting.
      • Upload file size restriction now also handled on frontend.

To do

  • Backend:
    Decide if we want to implement pyClamd or something similar for image file scanning. This could be run as a service in Docker.

  • Frontend:
    Configure access-control-allow-origin: * header in nuxt-security. Likely need to use VITE_FRONTEND_URL env var.
    Decide if we want to implement DOM purify.
    Needs tests (vitest and Playwright).

  • 'Delete image' functionality (front and backend).

Related issue

@netlify Netlify
Copy link

netlify bot commented Mar 17, 2025 •
edited
Loading

✅ Deploy Preview for activist-org ready!

Name Link
🔨 Latest commit 977f4f8
🔍 Latest deploy log https://app.netlify.com/sites/activist-org/deploys/67ded0547c85d600087d9521
😎 Deploy Preview https://deploy-preview-1164--activist-org.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions GitHub Actions
Copy link
Contributor

Thank you for the pull request! ❤️

The activist team will do our best to address your contribution as soon as we can. If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Development rooms once you're in. Also consider attending our bi-weekly Saturday developer syncs! It'd be great to meet you 😊

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 17, 2025 •
edited by andrewtavis
Loading

Maintainer Checklist

The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

  • The TypeScript, pytest and formatting workflows within the PR checks do not indicate new errors in the files changed

  • The Playwright end to end and Zap penetration tests have been ran and are passing (if necessary)

  • The changelog has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

@andrewtavis andrewtavis self-requested a review March 18, 2025 07:24
@andrewtavis
Copy link
Member

The header validation rules are on me, @mattburnett-repo. We had a dependency issue and the build's a bit weird because of it right now. I'll get it fixed soon.

@andrewtavis andrewtavis mentioned this pull request Mar 18, 2025
Merged
2 tasks
@andrewtavis
Copy link
Member

Frontend tests are going to fail on the above build, @mattburnett-repo. The header rules are fixed, but for some reason we're getting a frontend error in node_modules right now 🤔 If you have any suggestions here, feel free to let us know! Obviously not a part of what you're doing here, but figured I'd ask :)

CC @Abhi-Bohora who I'll also ask about this in #1166

mattburnett-repo and others added 8 commits March 18, 2025 15:09
@mattburnett-repo
WIP - Added UUID check to _create_view test.
feff1df
@andrewtavis
Merge branch 'main' into 670
d3a8c54
@mattburnett-repo
WIP - Cleaned up test_image_upload.py file.
9b0d503
@mattburnett-repo
WIP - Added nuxt-security module.
e63509b 
Added security key to nuxt.config.ts.
Currently configures CSP header, logger and request size.
CSP modified to allow image display/retrieval from api.
@mattburnett-repo
WIP - Scrub meta data.
f1994f4 
Added function scrub_exif to content/serializers.py.
@mattburnett-repo
WIP - Fixed type error.
3b8bdb3 
Fixed Image / ImageFile type mismatch error from deployment check.
@mattburnett-repo
WIP - Exif data scrubbing type error.
438aa08 
Cast line 49 img as PILImage.Image type.
@mattburnett-repo
WIP - Clean up and rate limiter.
8e675eb 
Cleanup for content/serializer.py.
Added rateLimiter to nuxt.config.ts/security settings.
@mattburnett-repo mattburnett-repo self-assigned this Mar 20, 2025
@mattburnett-repo mattburnett-repo added feature New feature or request python Relates to Python code javascript Relates to Javascript code labels Mar 20, 2025
@andrewtavis
Copy link
Member

Let me know when this is ready for review, @mattburnett-repo :) We'll likely look at it during the sync or maybe Saturday beforehand 😊

@andrewtavis
Copy link
Member

Maybe remove the WIP from the title if it is :)

@mattburnett-repo mattburnett-repo changed the title WIP - Issue 670. Image Upload Vallidation. Issue 670. Image Upload Valldation. Mar 20, 2025
@mattburnett-repo mattburnett-repo changed the title Issue 670. Image Upload Valldation. Issue 670. Image Upload Validation. Mar 20, 2025
@mattburnett-repo
Copy link
Member Author

mattburnett-repo commented Mar 20, 2025 •
edited
Loading

Let me know when this is ready for review, @mattburnett-repo :) We'll likely look at it during the sync or maybe Saturday beforehand 😊

There's always more to do (cf. the 'to do' items above) @andrewtavis , but this PR currently covers the important points in #670 and can be considered as done.

andrewtavis
andrewtavis approved these changes Mar 22, 2025
View reviewed changes
Copy link
Member

@andrewtavis andrewtavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All's looking great here, @mattburnett-repo! Thanks for the great work and the nice review session 😊

@andrewtavis andrewtavis merged commit fdd63e7 into activist-org:main Mar 22, 2025
7 checks passed
@mattburnett-repo mattburnett-repo deleted the 670 branch March 22, 2025 15:33
@andrewtavis andrewtavis mentioned this pull request Mar 22, 2025
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewtavis andrewtavis andrewtavis approved these changes

Assignees

@mattburnett-repo mattburnett-repo

Labels
feature New feature or request javascript Relates to Javascript code python Relates to Python code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mattburnett-repo @andrewtavis