From 4f7d8e8eb04993662aec540d8298512fb847cf1d Mon Sep 17 00:00:00 2001 From: Matthew Trew <9936426+mwtrew@users.noreply.github.com> Date: Tue, 27 Oct 2026 14:22:46 +0000 Subject: [PATCH 1/3] Reject oversized projects --- .env.example | 1 + .../api/scratch/assets_controller.rb | 35 +++++++++++- app/models/scratch_asset.rb | 6 +++ ...eating_and_showing_a_scratch_asset_spec.rb | 54 +++++++++++++++++++ spec/models/scratch_asset_spec.rb | 25 +++++++++ 5 files changed, 119 insertions(+), 2 deletions(-) diff --git a/.env.example b/.env.example index 20465d29b..8f38ef4eb 100644 --- a/.env.example +++ b/.env.example @@ -68,6 +68,7 @@ SALESFORCE_CONNECT_USER=postgres SCRATCH_ASSET_CONFIG_BASE_URL=https://example.com/config/ SCRATCH_ASSET_IMPORT_BASE_URL=https://example.com/assets/ SCRATCH_ASSET_MAX_SIZE_MB=10 +SCRATCH_PROJECT_ASSETS_MAX_SIZE_MB=100 HOSTNAME=localhost LEARNER_EXPERIENCE_TESTS_DISPATCH_TOKEN=changeme diff --git a/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index cb31a19b2..cf7126865 100644 --- a/app/controllers/api/scratch/assets_controller.rb +++ b/app/controllers/api/scratch/assets_controller.rb @@ -6,6 +6,7 @@ module Api module Scratch class AssetsController < ApiController include ActiveStorage::SetCurrent + include RemixSelection prepend_before_action :load_experience_cs_service_user, only: %i[create_global] before_action :authorize_user, except: %i[show] @@ -50,7 +51,8 @@ def create_global def create_asset(reject_conflicting_content: false, **attributes) scratch_asset = ScratchAsset.find_or_initialize_by(attributes) - return reject_oversized_file if !scratch_asset.file.attached? && file_too_big? + reject_upload_over_limits(attributes[:project]) unless scratch_asset.file.attached? + return if performed? scratch_asset = save_new_asset(scratch_asset, attributes) if scratch_asset.new_record? @@ -102,12 +104,20 @@ def file_matches?(scratch_asset) scratch_asset.file.blob.checksum == request_body_checksum end + def reject_upload_over_limits(project) + if file_too_big? + reject_oversized_file + elsif project && project_limit_exceeded?(project) + reject_project_over_limit + end + end + def file_too_big? request.body.size > max_file_size_mb.megabytes end def reject_oversized_file - render json: { error: "The asset was larger than the limit of #{max_file_size_mb} MB" }, + render json: { error: :asset_too_large, detail: "The asset was larger than the limit of #{max_file_size_mb} MB" }, status: :content_too_large end @@ -115,6 +125,27 @@ def max_file_size_mb ENV.fetch('SCRATCH_ASSET_MAX_SIZE_MB', 10).to_i end + def project_limit_exceeded?(project) + current_total = ScratchAsset.project_byte_size( + project_ids: original_and_remix_ids(project), + uploaded_user_id: current_user.id + ) + current_total + request.body.size > max_project_size_mb.megabytes + end + + def original_and_remix_ids(project) + [project.id, remix_for_user(project, current_user)&.id].compact + end + + def reject_project_over_limit + render json: { error: :project_too_large, detail: "The project would exceed the limit of #{max_project_size_mb} MB" }, + status: :unprocessable_content + end + + def max_project_size_mb + ENV.fetch('SCRATCH_PROJECT_ASSETS_MAX_SIZE_MB', 100).to_i + end + def reject_conflicting_file(scratch_asset) scope = scratch_asset.global? ? 'global' : 'project' render json: { error: "Asset content conflicts with the existing #{scope} asset" }, status: :conflict diff --git a/app/models/scratch_asset.rb b/app/models/scratch_asset.rb index e1347089a..e48bb4aaf 100644 --- a/app/models/scratch_asset.rb +++ b/app/models/scratch_asset.rb @@ -25,6 +25,12 @@ def self.find_visible_to_project(project:, user:, filename:) global_assets.find_by(filename:) end + def self.project_byte_size(project_ids:, uploaded_user_id:) + joins(file_attachment: :blob) + .where(project_id: project_ids, uploaded_user_id:) + .sum('active_storage_blobs.byte_size') + end + def global? project_id.nil? end diff --git a/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb b/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb index 6de22ee09..94ce780a0 100644 --- a/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb +++ b/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb @@ -542,6 +542,60 @@ def make_request(body = upload) end end + describe 'enforcing the project asset size limit' do + let(:limit_mb) { 1 } + let(:half_limit) { 'a' * (limit_mb.megabytes / 2) } + let(:request_path) { '/api/scratch/assets/test_image_1.png' } + let(:request_headers) do + { 'Content-Type' => 'application/octet-stream', 'X-Project-ID' => project.identifier }.merge(auth_headers) + end + + def make_request(body) + post request_path, headers: request_headers, params: body + end + + around do |example| + ClimateControl.modify(SCRATCH_PROJECT_ASSETS_MAX_SIZE_MB: limit_mb.to_s) { example.run } + end + + before do + authenticated_in_hydra_as(teacher) + end + + it 'responds with a 422 status code when the upload would take the project over the limit' do + create_uploaded_scratch_asset(filename: 'existing.png', project:, body: half_limit) + + make_request("#{half_limit}a") + + expect(response).to have_http_status(:unprocessable_content) + end + + it 'does not create an asset or file when the project would go over the limit' do + create_uploaded_scratch_asset(filename: 'existing.png', project:, body: half_limit) + + expect { make_request("#{half_limit}a") }.not_to change(ScratchAsset, :count) + expect { make_request("#{half_limit}a") }.not_to change(ActiveStorage::Blob, :count) + end + + it 'accepts an upload that brings the project exactly up to the limit' do + create_uploaded_scratch_asset(filename: 'existing.png', project:, body: half_limit) + + make_request(half_limit) + + expect(response).to have_http_status(:created) + end + + it 'skips the check when the asset already has a file attached' do + create_uploaded_scratch_asset(filename: 'existing.png', project:, body: half_limit) + existing_asset = create_uploaded_scratch_asset(filename:, project:, body: half_limit) + + make_request("#{half_limit}a") + + expect(response).to have_http_status(:created) + expect(existing_asset.reload.file.download).to eq(half_limit) + end + end + describe 'visibility of a migrated project asset' do let(:student) { create(:student, school:) } let(:class_teacher) { create(:teacher, school:) } diff --git a/spec/models/scratch_asset_spec.rb b/spec/models/scratch_asset_spec.rb index dfa9d0492..beb4e6162 100644 --- a/spec/models/scratch_asset_spec.rb +++ b/spec/models/scratch_asset_spec.rb @@ -57,4 +57,29 @@ def create_scratch_asset(project:, uploaded_user_id:) create(:scratch_asset, filename:, project:, uploaded_user_id:) end end + + describe '.total_byte_size' do + let(:user_id) { SecureRandom.uuid } + let(:project) { create(:project, user_id:, project_type: Project::Types::CODE_EDITOR_SCRATCH, locale: nil) } + let(:other_project) { create(:project, user_id:, project_type: Project::Types::CODE_EDITOR_SCRATCH, locale: nil) } + + it 'sums the file sizes of assets uploaded by the user to the given projects' do + create_attached_asset(filename: 'a.png', project:, uploaded_user_id: user_id, body: 'aaa') + create_attached_asset(filename: 'b.png', project: other_project, uploaded_user_id: user_id, body: 'bbbbb') + + expect(described_class.project_byte_size(project_ids: [project.id, other_project.id], uploaded_user_id: user_id)).to eq(8) + end + + it 'ignores assets without an attached file' do + create(:scratch_asset, filename: 'a.png', project:, uploaded_user_id: user_id) + + expect(described_class.project_byte_size(project_ids: [project.id], uploaded_user_id: user_id)).to eq(0) + end + + def create_attached_asset(filename:, project:, uploaded_user_id:, body:) + described_class.create!(filename:, project:, uploaded_user_id:).tap do |asset| + asset.file.attach(io: StringIO.new(body), filename:) + end + end + end end From 0ddee9a7713a20b73646f8c15cf2bec767e94829 Mon Sep 17 00:00:00 2001 From: Matthew Trew <9936426+mwtrew@users.noreply.github.com> Date: Tue, 27 Oct 2026 15:16:10 +0000 Subject: [PATCH 2/3] Use constants instead of env vars As per Chris' suggestion here: https://github.com/RaspberryPiFoundation/editor-api/pull/1051#pullrequestreview-5504572536 --- .env.example | 2 -- app/controllers/api/scratch/assets_controller.rb | 16 ++++------------ app/models/scratch_asset.rb | 3 +++ .../creating_and_showing_a_scratch_asset_spec.rb | 10 ++-------- 4 files changed, 9 insertions(+), 22 deletions(-) diff --git a/.env.example b/.env.example index 8f38ef4eb..2166cd2e3 100644 --- a/.env.example +++ b/.env.example @@ -67,8 +67,6 @@ SALESFORCE_CONNECT_USER=postgres SCRATCH_ASSET_CONFIG_BASE_URL=https://example.com/config/ SCRATCH_ASSET_IMPORT_BASE_URL=https://example.com/assets/ -SCRATCH_ASSET_MAX_SIZE_MB=10 -SCRATCH_PROJECT_ASSETS_MAX_SIZE_MB=100 HOSTNAME=localhost LEARNER_EXPERIENCE_TESTS_DISPATCH_TOKEN=changeme diff --git a/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index cf7126865..bf83b492e 100644 --- a/app/controllers/api/scratch/assets_controller.rb +++ b/app/controllers/api/scratch/assets_controller.rb @@ -113,24 +113,20 @@ def reject_upload_over_limits(project) end def file_too_big? - request.body.size > max_file_size_mb.megabytes + request.body.size > ScratchAsset::MAX_SIZE_MB.megabytes end def reject_oversized_file - render json: { error: :asset_too_large, detail: "The asset was larger than the limit of #{max_file_size_mb} MB" }, + render json: { error: :asset_too_large, detail: "The asset was larger than the limit of #{ScratchAsset::MAX_SIZE_MB} MB" }, status: :content_too_large end - def max_file_size_mb - ENV.fetch('SCRATCH_ASSET_MAX_SIZE_MB', 10).to_i - end - def project_limit_exceeded?(project) current_total = ScratchAsset.project_byte_size( project_ids: original_and_remix_ids(project), uploaded_user_id: current_user.id ) - current_total + request.body.size > max_project_size_mb.megabytes + current_total + request.body.size > ScratchAsset::MAX_PROJECT_SIZE_MB.megabytes end def original_and_remix_ids(project) @@ -138,14 +134,10 @@ def original_and_remix_ids(project) end def reject_project_over_limit - render json: { error: :project_too_large, detail: "The project would exceed the limit of #{max_project_size_mb} MB" }, + render json: { error: :project_too_large, detail: "The project would exceed the limit of #{ScratchAsset::MAX_PROJECT_SIZE_MB} MB" }, status: :unprocessable_content end - def max_project_size_mb - ENV.fetch('SCRATCH_PROJECT_ASSETS_MAX_SIZE_MB', 100).to_i - end - def reject_conflicting_file(scratch_asset) scope = scratch_asset.global? ? 'global' : 'project' render json: { error: "Asset content conflicts with the existing #{scope} asset" }, status: :conflict diff --git a/app/models/scratch_asset.rb b/app/models/scratch_asset.rb index e48bb4aaf..01d99acd0 100644 --- a/app/models/scratch_asset.rb +++ b/app/models/scratch_asset.rb @@ -1,6 +1,9 @@ # frozen_string_literal: true class ScratchAsset < ApplicationRecord + MAX_SIZE_MB = 20 + MAX_PROJECT_SIZE_MB = 100 + belongs_to :project, optional: true has_one_attached :file diff --git a/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb b/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb index 94ce780a0..f663eb8e1 100644 --- a/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb +++ b/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb @@ -495,11 +495,8 @@ def make_request(body = upload) post request_path, headers: request_headers, params: body end - around do |example| - ClimateControl.modify(SCRATCH_ASSET_MAX_SIZE_MB: limit_mb.to_s) { example.run } - end - before do + stub_const('ScratchAsset::MAX_SIZE_MB', limit_mb) authenticated_in_hydra_as(teacher) end @@ -554,11 +551,8 @@ def make_request(body) post request_path, headers: request_headers, params: body end - around do |example| - ClimateControl.modify(SCRATCH_PROJECT_ASSETS_MAX_SIZE_MB: limit_mb.to_s) { example.run } - end - before do + stub_const('ScratchAsset::MAX_PROJECT_SIZE_MB', limit_mb) authenticated_in_hydra_as(teacher) end From f4b7a8de6d9c5d4a909e8ebc5ec71ab4fd0d695b Mon Sep 17 00:00:00 2001 From: Matthew Trew <9936426+mwtrew@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:07:59 +0000 Subject: [PATCH 3/3] Resolve original project before calculating project size --- app/controllers/api/scratch/assets_controller.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index bf83b492e..f6c442dac 100644 --- a/app/controllers/api/scratch/assets_controller.rb +++ b/app/controllers/api/scratch/assets_controller.rb @@ -130,7 +130,12 @@ def project_limit_exceeded?(project) end def original_and_remix_ids(project) - [project.id, remix_for_user(project, current_user)&.id].compact + original = users_remix?(project) ? project.parent : project + [original.id, project.id, remix_for_user(original, current_user)&.id].compact.uniq + end + + def users_remix?(project) + project.parent.present? && project.user_id == current_user.id end def reject_project_over_limit