Skip to content

Commit 45dc251

Browse files
pglombardoCopilot
pglombardo
and
Copilot
authored
Security: Fix file upload authentication enforcement (#4381)
* Security: Fix file upload authentication enforcement * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Peter Giacomo Lombardo <pglombardo@hey.com> * Tests: better setting management * Better file param checking * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Peter Giacomo Lombardo <pglombardo@hey.com> * Better semantic check --------- Signed-off-by: Peter Giacomo Lombardo <pglombardo@hey.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1 parent f0ba2b8 commit 45dc251

3 files changed

Lines changed: 172 additions & 10 deletions

File tree

‎app/controllers/api/v1/pushes_controller.rb‎

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -147,13 +147,15 @@ def show
147147
}
148148
EOS
149149
def create
150-
# Require authentication if allow_anonymous is false
151-
# See config/settings.yml
152-
authenticate_user! unless Settings.allow_anonymous
150+
permitted_params = push_params
153151

154-
@push = Push.new(push_params)
152+
# Require authentication if anonymous creation is disabled or
153+
# when creating file pushes / uploading attachments.
154+
authenticate_user! if requires_authentication_for_create?(permitted_params)
155155

156-
if !push_params[:kind].present?
156+
@push = Push.new(permitted_params)
157+
158+
if !permitted_params[:kind].present?
157159
# These are used to determine the default kind based on the request path
158160
# for old push records. Their paths are generated based on their kind.
159161
# And, QR code pushes are created by using `/p/` path.
@@ -162,7 +164,7 @@ def create
162164
"file"
163165
elsif request.path.include?("/r.json")
164166
"url"
165-
elsif request.path.include?("/p.json") && push_params.key?(:files)
167+
elsif request.path.include?("/p.json") && permitted_params.key?(:files)
166168
"file"
167169
else
168170
"text"
@@ -171,8 +173,8 @@ def create
171173

172174
@push.user = current_user if user_signed_in?
173175

174-
assign_deletable_by_viewer(@push, push_params)
175-
assign_retrieval_step(@push, push_params)
176+
assign_deletable_by_viewer(@push, permitted_params)
177+
assign_retrieval_step(@push, permitted_params)
176178

177179
if @push.save
178180
log_creation(@push)
@@ -417,6 +419,18 @@ def expired
417419

418420
private
419421

422+
def requires_authentication_for_create?(permitted_params)
423+
return true unless Settings.allow_anonymous
424+
return true if request.path.start_with?("/f")
425+
426+
# Keep auth semantics aligned with API surface:
427+
# - v1 /p.json treats files key presence as file intent
428+
# - v2 /api/v2/pushes should do the same for auth gating
429+
((request.path.include?("/p.json") || params["controller"] == "api/v2/pushes") &&
430+
permitted_params.key?(:files)) ||
431+
permitted_params[:kind] == "file"
432+
end
433+
420434
# validate_page_parameter
421435
#
422436
# Validates and sanitizes the page parameter for pagination

‎test/controllers/api/base_controller_test.rb‎

Lines changed: 78 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,80 @@ class Api::BaseControllerTest < ActionDispatch::IntegrationTest
277277
assert_not_equal :unauthorized, response.status
278278
end
279279

280+
test "/p/create with files requires authentication even when allow_anonymous is true" do
281+
previous_allow_anonymous = Settings.allow_anonymous
282+
previous_enable_file_pushes = Settings.enable_file_pushes
283+
Settings.allow_anonymous = true
284+
Settings.enable_file_pushes = true
285+
Rails.application.reload_routes!
286+
287+
post "/p.json",
288+
params: {
289+
password: {
290+
payload: "test_secret_file_upload_requires_auth",
291+
files: [fixture_file_upload("monkey.png", "image/jpeg")]
292+
}
293+
},
294+
headers: {
295+
"Accept" => "application/json"
296+
}
297+
298+
assert_response :unauthorized
299+
ensure
300+
Settings.allow_anonymous = previous_allow_anonymous
301+
Settings.enable_file_pushes = previous_enable_file_pushes
302+
Rails.application.reload_routes!
303+
end
304+
305+
test "/p/create with files works with valid token when allow_anonymous is true" do
306+
Settings.allow_anonymous = true
307+
Settings.enable_file_pushes = true
308+
Rails.application.reload_routes!
309+
310+
post "/p.json",
311+
params: {
312+
password: {
313+
payload: "test_secret_file_upload_authenticated",
314+
files: [fixture_file_upload("monkey.png", "image/jpeg")]
315+
}
316+
},
317+
headers: {
318+
"Authorization" => "Bearer valid_token_123",
319+
"Accept" => "application/json"
320+
}
321+
322+
assert_response :created
323+
json = JSON.parse(response.body)
324+
assert json["url_token"].present?
325+
ensure
326+
Settings.allow_anonymous = true
327+
Settings.enable_file_pushes = false
328+
Rails.application.reload_routes!
329+
end
330+
331+
test "/p/create with empty files key requires authentication when allow_anonymous is true" do
332+
Settings.allow_anonymous = true
333+
Settings.enable_file_pushes = true
334+
Rails.application.reload_routes!
335+
336+
post "/p.json",
337+
params: {
338+
password: {
339+
payload: "test_secret_file_key_present_empty",
340+
files: []
341+
}
342+
},
343+
headers: {
344+
"Accept" => "application/json"
345+
}
346+
347+
assert_response :unauthorized
348+
ensure
349+
Settings.allow_anonymous = true
350+
Settings.enable_file_pushes = false
351+
Rails.application.reload_routes!
352+
end
353+
280354
# When allow_anonymous is false, Api::V1::PushesController#create calls
281355
# authenticate_user! — anonymous JSON create must be rejected.
282356
test "/p/create requires authentication when allow_anonymous is false" do
@@ -345,7 +419,8 @@ class Api::BaseControllerTest < ActionDispatch::IntegrationTest
345419
post "/f.json",
346420
params: {
347421
file_push: {
348-
payload: "test"
422+
payload: "test",
423+
files: [fixture_file_upload("monkey.png", "image/jpeg")]
349424
}
350425
},
351426
headers: {
@@ -365,7 +440,8 @@ class Api::BaseControllerTest < ActionDispatch::IntegrationTest
365440
post "/f.json",
366441
params: {
367442
file_push: {
368-
payload: "test"
443+
payload: "test",
444+
files: [fixture_file_upload("monkey.png", "image/jpeg")]
369445
}
370446
},
371447
headers: {

‎test/integration/api/api_v2_pushes_test.rb‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,78 @@ def test_create_with_missing_payload_returns_json_validation_error_without_accep
219219
assert body.key?("payload")
220220
end
221221

222+
def test_create_file_upload_requires_authentication_even_when_allow_anonymous_enabled
223+
previous_allow_anonymous = Settings.allow_anonymous
224+
previous_enable_file_pushes = Settings.enable_file_pushes
225+
226+
Settings.allow_anonymous = true
227+
Settings.enable_file_pushes = true
228+
Rails.application.reload_routes!
229+
230+
post "/api/v2/pushes",
231+
params: {
232+
push: {
233+
payload: "v2-file-push-without-auth",
234+
files: [fixture_file_upload("monkey.png", "image/jpeg")]
235+
}
236+
}
237+
238+
assert_response :unauthorized
239+
ensure
240+
Settings.allow_anonymous = previous_allow_anonymous
241+
Settings.enable_file_pushes = previous_enable_file_pushes
242+
Rails.application.reload_routes!
243+
end
244+
245+
def test_create_file_upload_allows_authenticated_user_when_allow_anonymous_enabled
246+
original_allow_anonymous = Settings.allow_anonymous
247+
original_enable_file_pushes = Settings.enable_file_pushes
248+
Settings.allow_anonymous = true
249+
Settings.enable_file_pushes = true
250+
Rails.application.reload_routes!
251+
user = users(:luca)
252+
253+
post "/api/v2/pushes",
254+
params: {
255+
push: {
256+
payload: "v2-file-push-with-auth",
257+
files: [fixture_file_upload("monkey.png", "image/jpeg")]
258+
}
259+
},
260+
headers: bearer_headers(user)
261+
262+
assert_response :created
263+
body = JSON.parse(response.body)
264+
assert body["url_token"].present?
265+
ensure
266+
Settings.allow_anonymous = original_allow_anonymous
267+
Settings.enable_file_pushes = original_enable_file_pushes
268+
Rails.application.reload_routes!
269+
end
270+
271+
def test_create_with_empty_files_key_requires_authentication_even_when_allow_anonymous_enabled
272+
original_allow_anonymous = Settings.allow_anonymous
273+
original_enable_file_pushes = Settings.enable_file_pushes
274+
Settings.allow_anonymous = true
275+
Settings.enable_file_pushes = true
276+
Rails.application.reload_routes!
277+
278+
post "/api/v2/pushes",
279+
params: {
280+
push: {
281+
payload: "v2-file-key-empty-without-auth",
282+
files: []
283+
}
284+
},
285+
as: :json
286+
287+
assert_response :unauthorized
288+
ensure
289+
Settings.allow_anonymous = original_allow_anonymous
290+
Settings.enable_file_pushes = original_enable_file_pushes
291+
Rails.application.reload_routes!
292+
end
293+
222294
def test_create_with_valid_payload_returns_json_created_without_accept_header
223295
assert_difference("Push.count", 1) do
224296
post "/api/v2/pushes",

0 commit comments

Comments
 (0)