From 78e598d2284717f4155f824c9722271b863be20a Mon Sep 17 00:00:00 2001 From: Nikoloz Turazashvili <74835523+turazashvili@users.noreply.github.com> Date: Thu, 24 Sep 2026 19:32:42 +0700 Subject: [PATCH] fix(articles): allow removing cover video from articles (#23118) (#23855) The web and API param whitelists only permitted `video_source_url` when it was present, so a blank/null value sent by the editor's "Remove" button was silently dropped and the cover video stayed on the article. - Permit `video_source_url` when the key is present and blank, so the cover video can be removed. The host whitelist is unchanged. - Extract the whitelist into `Article::LINKED_VIDEO_SOURCE_PATTERNS` / `Article.permitted_video_source_url?` so both controllers share it. - In `generate_video_embed_url`, clear the derived `video` (and the Mux-derived thumbnail) when a previously set source URL is removed, without touching articles whose `video_source_url` was already nil. - Regression specs for the model, web and API update paths. Fixes #23118 --- app/controllers/articles_controller.rb | 11 +-- .../concerns/api/articles_controller.rb | 11 +-- app/models/article.rb | 29 ++++++- spec/models/article_spec.rb | 82 +++++++++++++++++++ spec/requests/api/v1/articles_spec.rb | 32 ++++++++ .../requests/articles/articles_update_spec.rb | 51 ++++++++++++ 6 files changed, 201 insertions(+), 15 deletions(-) diff --git a/app/controllers/articles_controller.rb b/app/controllers/articles_controller.rb index 98060af9da74f..867bfa3b05bd4 100644 --- a/app/controllers/articles_controller.rb +++ b/app/controllers/articles_controller.rb @@ -353,13 +353,10 @@ def article_params_json ] end - # Allow video_source_url if it's a valid YouTube, Mux, or Twitch URL - video_url = params.dig("article", "video_source_url") - if video_url.present? - youtube_pattern = /\Ahttps?:\/\/(www\.)?(youtube\.com\/watch\?v=|youtu\.be\/)/ - mux_pattern = /\Ahttps?:\/\/player\.mux\.com\// - twitch_pattern = /\Ahttps?:\/\/(www\.)?twitch\.tv\/videos\// - allowed_params << :video_source_url if video_url.match?(youtube_pattern) || video_url.match?(mux_pattern) || video_url.match?(twitch_pattern) + # Allow video_source_url only for supported hosts; a blank value removes the cover video + if params["article"].key?("video_source_url") && + Article.permitted_video_source_url?(params["article"]["video_source_url"]) + allowed_params << :video_source_url end # NOTE: the organization logic is still a little counter intuitive but this should diff --git a/app/controllers/concerns/api/articles_controller.rb b/app/controllers/concerns/api/articles_controller.rb index 29b65bdd562ec..638ee5f322130 100644 --- a/app/controllers/concerns/api/articles_controller.rb +++ b/app/controllers/concerns/api/articles_controller.rb @@ -188,13 +188,10 @@ def article_params ] allowed_params << :ai_disclosure_level if Settings::General.enable_ai_disclosure allowed_params << :organization_id if params.dig("article", "organization_id") && allowed_to_change_org_id? - # allow if a youtube.com, mux.com, or twitch.tv URL - video_url = params.dig("article", "video_source_url") - if video_url.present? - youtube_pattern = /\Ahttps?:\/\/(www\.)?(youtube\.com\/watch\?v=|youtu\.be\/)/ - mux_pattern = /\Ahttps?:\/\/player\.mux\.com\// - twitch_pattern = /\Ahttps?:\/\/(www\.)?twitch\.tv\/videos\// - allowed_params << :video_source_url if video_url.match?(youtube_pattern) || video_url.match?(mux_pattern) || video_url.match?(twitch_pattern) + # allow video_source_url only for supported hosts; a blank value removes the cover video + if params["article"]&.key?("video_source_url") && + Article.permitted_video_source_url?(params["article"]["video_source_url"]) + allowed_params << :video_source_url end if @user.super_admin? allowed_params << :clickbait_score diff --git a/app/models/article.rb b/app/models/article.rb index 88537c190d17d..550e7f1a2b063 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -60,6 +60,15 @@ class Article < ApplicationRecord MAX_TAG_LIST_SIZE = 4 + # Hosts a user may link as a cover video via `video_source_url`. Used by the + # web and API controllers' param whitelists; mirrored on the frontend in + # app/javascript/article-form/components/CoverVideoLink.jsx. + LINKED_VIDEO_SOURCE_PATTERNS = [ + %r{\Ahttps?://(www\.)?(youtube\.com/watch\?v=|youtu\.be/)}, + %r{\Ahttps?://player\.mux\.com/}, + %r{\Ahttps?://(www\.)?twitch\.tv/videos/}, + ].freeze + # Author-visible edits, for the article_updated CDP event. Rows churn on score # recalcs, counter caches and last_comment_at. Mirrors User::SYNC_TRIGGER_KEYS. TRACKABLE_UPDATE_KEYS = %w[ @@ -133,6 +142,14 @@ def self.unique_url_error I18n.t("models.article.unique_url", email: ForemInstance.contact_email) end + # Whether a user-submitted `video_source_url` may be mass-assigned. + # A blank value is allowed so the cover video can be removed. + def self.permitted_video_source_url?(url) + return true if url.blank? + + LINKED_VIDEO_SOURCE_PATTERNS.any? { |pattern| url.to_s.match?(pattern) } + end + enum :type_of, { full_post: 0, status: 1, @@ -1209,7 +1226,17 @@ def set_default_subforem_id end def generate_video_embed_url - return if video_source_url.blank? + if video_source_url.blank? + # The cover video was removed: drop the embed derived from it. Only Mux + # derives the thumbnail from the source URL, so user-provided thumbnails + # are kept. + if video_source_url_was.present? + self.video = nil + self.video_thumbnail_url = nil if video_thumbnail_url&.include?("image.mux.com") + end + self.video_source_url = nil + return + end if video_source_url.include?("youtube.com") || video_source_url.include?("youtu.be") begin diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index 0d7312ed9e2c1..2b3a8a9d8a15e 100644 --- a/spec/models/article_spec.rb +++ b/spec/models/article_spec.rb @@ -1664,6 +1664,88 @@ def build_and_validate_article(*args) expect(article.video).to be_nil end end + + context "when clearing video_source_url" do + it "clears video and video_source_url when set to blank" do + saved_article = create(:article, user: user, video_source_url: "https://www.youtube.com/watch?v=dQw4w9WgXcQ") + expect(saved_article.video).to eq("https://www.youtube.com/embed/dQw4w9WgXcQ") + + saved_article.video_source_url = "" + saved_article.valid? + expect(saved_article.video_source_url).to be_nil + expect(saved_article.video).to be_nil + end + + it "clears video_thumbnail_url for Mux video when cleared" do + saved_article = create(:article, user: user, video_source_url: "https://player.mux.com/nw5QrgIQS02FEx5BJEQH8CdcLmXXRvCNACZKQ01kLoKEI") + expect(saved_article.video_thumbnail_url).to be_present + + saved_article.video_source_url = nil + saved_article.valid? + expect(saved_article.video_source_url).to be_nil + expect(saved_article.video).to be_nil + expect(saved_article.video_thumbnail_url).to be_nil + end + + it "does not clear an existing video embed on unrelated updates when video_source_url is untouched" do + saved_article = create(:article, user: user, video_source_url: nil) + saved_article.update_column(:video, "https://www.youtube.com/embed/dQw4w9WgXcQ") + + saved_article.title = "Updated title" + saved_article.valid? + expect(saved_article.video).to eq("https://www.youtube.com/embed/dQw4w9WgXcQ") + end + + it "does not clear a legacy uploaded video on unrelated updates" do + saved_article = create(:article, :video, user: user) + expect(saved_article.video).to be_present + expect(saved_article.video_source_url).to include(".m3u8") + + saved_article.title = "Updated title" + saved_article.valid? + expect(saved_article.video).to be_present + expect(saved_article.video_thumbnail_url).to be_present + end + + it "keeps a user-provided thumbnail when a non-Mux video is cleared" do + saved_article = create(:article, user: user, + video_source_url: "https://www.youtube.com/watch?v=dQw4w9WgXcQ", + video_thumbnail_url: "https://i.imgur.com/HPiu7N4.jpg") + + saved_article.video_source_url = "" + saved_article.valid? + expect(saved_article.video).to be_nil + expect(saved_article.video_thumbnail_url).to eq("https://i.imgur.com/HPiu7N4.jpg") + end + + it "does not clear video on a new record with a blank video_source_url" do + new_article = build(:article, user: user, video: "https://www.youtube.com/embed/dQw4w9WgXcQ", + video_source_url: "") + new_article.valid? + expect(new_article.video).to eq("https://www.youtube.com/embed/dQw4w9WgXcQ") + expect(new_article.video_source_url).to be_nil + end + end + end + + describe ".permitted_video_source_url?" do + it "permits blank values so the cover video can be removed" do + expect(described_class.permitted_video_source_url?(nil)).to be(true) + expect(described_class.permitted_video_source_url?("")).to be(true) + end + + it "permits YouTube, Mux and Twitch video URLs" do + expect(described_class.permitted_video_source_url?("https://www.youtube.com/watch?v=dQw4w9WgXcQ")).to be(true) + expect(described_class.permitted_video_source_url?("https://youtu.be/dQw4w9WgXcQ")).to be(true) + expect(described_class.permitted_video_source_url?("https://player.mux.com/abc123")).to be(true) + expect(described_class.permitted_video_source_url?("https://www.twitch.tv/videos/1234567890")).to be(true) + end + + it "rejects other URLs" do + expect(described_class.permitted_video_source_url?("https://example.com/video")).to be(false) + expect(described_class.permitted_video_source_url?("https://www.twitch.tv/somechannel")).to be(false) + expect(described_class.permitted_video_source_url?("javascript:alert(1)")).to be(false) + end end describe "#fetch_video_duration" do diff --git a/spec/requests/api/v1/articles_spec.rb b/spec/requests/api/v1/articles_spec.rb index 7ec932a492bcb..61f20a242cd8f 100644 --- a/spec/requests/api/v1/articles_spec.rb +++ b/spec/requests/api/v1/articles_spec.rb @@ -1209,6 +1209,38 @@ def post_article(**params) expect(article.reload.main_image).to eq("https://dummyimage.com/100x100") end + it "removes video_source_url when given an empty string" do + article.update!(video_source_url: "https://www.youtube.com/watch?v=dQw4w9WgXcQ") + expect(article.reload.video).to be_present + + put_article(video_source_url: "") + expect(response).to have_http_status(:ok) + article.reload + expect(article.video_source_url).to be_nil + expect(article.video).to be_nil + end + + it "removes video_source_url when given nil" do + article.update!(video_source_url: "https://www.youtube.com/watch?v=dQw4w9WgXcQ") + expect(article.reload.video).to be_present + + put_article(video_source_url: nil) + expect(response).to have_http_status(:ok) + article.reload + expect(article.video_source_url).to be_nil + expect(article.video).to be_nil + end + + it "does not touch video_source_url when the key is absent" do + article.update!(video_source_url: "https://www.youtube.com/watch?v=dQw4w9WgXcQ") + + put_article(title: "New title") + expect(response).to have_http_status(:ok) + article.reload + expect(article.video_source_url).to eq("https://www.youtube.com/watch?v=dQw4w9WgXcQ") + expect(article.video).to be_present + end + it "updates the tags" do expect do put_article( diff --git a/spec/requests/articles/articles_update_spec.rb b/spec/requests/articles/articles_update_spec.rb index 7730046f5739f..da682f5381f68 100644 --- a/spec/requests/articles/articles_update_spec.rb +++ b/spec/requests/articles/articles_update_spec.rb @@ -194,6 +194,57 @@ expect(article.reload.video_thumbnail_url).to include "https://i.imgur.com/HPiu7N4.jpg" end + it "removes video_source_url and video embed when passed empty string" do + article.update!(video_source_url: "https://www.youtube.com/watch?v=dQw4w9WgXcQ") + expect(article.reload.video).to be_present + + put "/articles/#{article.id}", params: { + article: { video_source_url: "" } + } + expect(response).to have_http_status(:ok) + article.reload + expect(article.video_source_url).to be_nil + expect(article.video).to be_nil + end + + it "removes video_source_url and video embed when passed null via JSON" do + article.update!(video_source_url: "https://www.youtube.com/watch?v=dQw4w9WgXcQ") + expect(article.reload.video).to be_present + + put "/articles/#{article.id}", + params: { article: { video_source_url: nil } }.to_json, + headers: { "Content-Type" => "application/json" } + + expect(response).to have_http_status(:ok) + article.reload + expect(article.video_source_url).to be_nil + expect(article.video).to be_nil + end + + it "keeps video_source_url when the key is absent from the payload" do + article.update!(video_source_url: "https://www.youtube.com/watch?v=dQw4w9WgXcQ") + + put "/articles/#{article.id}", + params: { article: { title: "New title" } }.to_json, + headers: { "Content-Type" => "application/json" } + + expect(response).to have_http_status(:ok) + article.reload + expect(article.video_source_url).to eq("https://www.youtube.com/watch?v=dQw4w9WgXcQ") + expect(article.video).to be_present + end + + it "ignores an unsupported video_source_url" do + article.update!(video_source_url: "https://www.youtube.com/watch?v=dQw4w9WgXcQ") + + put "/articles/#{article.id}", + params: { article: { video_source_url: "https://vimeo.com/123456" } }.to_json, + headers: { "Content-Type" => "application/json" } + + expect(response).to have_http_status(:ok) + expect(article.reload.video_source_url).to eq("https://www.youtube.com/watch?v=dQw4w9WgXcQ") + end + context "when setting published_at in editor v2" do let(:tomorrow) { 1.day.from_now } let(:published_at) { "#{tomorrow.strftime('%d.%m.%Y')} 18:00" }