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.

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

Add validation to competition tab content to not allow relative URLs #9792

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions app/models/competition.rb
Original file line number Diff line number Diff line change
Expand Up @@ -643,9 +643,9 @@ def user_can_pre_register?(user)
delegates.include?(user) || trainee_delegates.include?(user) || organizers.include?(user)
end

attr_accessor :being_cloned_from_id
attr_accessor :being_cloned_from_id, :being_cloned_from_cache
def being_cloned_from
Competition.find_by(id: being_cloned_from_id)
@being_cloned_from_cache ||= Competition.find_by(id: being_cloned_from_id)
end

def build_clone
Expand Down Expand Up @@ -709,7 +709,7 @@ def build_clone
# Clone competition tabs.
if clone_tabs
being_cloned_from&.tabs&.each do |tab|
tabs.create(tab.attributes.slice(*CompetitionTab::CLONEABLE_ATTRIBUTES))
tabs.create!(tab.attributes.slice(*CompetitionTab::CLONEABLE_ATTRIBUTES))
end
end
end
Expand Down Expand Up @@ -2449,7 +2449,13 @@ def to_form_data
# It is quite uncool that we have to duplicate the internal form_data formatting like this
# but as long as we let our backend handle the complete error validation we literally have no other choice
def form_errors
return {} if self.valid?
self_valid = self.valid?
# If we're cloning, we also need to check the parent's associations.
# Otherwise, the user may be surprised by a silent fail if some tabs/venues/schedules
# of the parent are invalid. (This can happen if we introduce new validations on old data)
self_valid &= being_cloned_from&.tabs&.all?(&:valid?) if being_cloned_from_id.present?

return {} if self_valid

{
# for historic reasons, we keep 'name' errors listed under ID. Don't ask.
Expand Down Expand Up @@ -2542,14 +2548,18 @@ def form_errors
},
"cloning" => {
"fromId" => errors[:being_cloned_from_id],
"cloneTabs" => errors[:clone_tabs],
"cloneTabs" => being_cloned_from_id.present? ? being_cloned_from&.association_errors(:tabs) : errors[:clone_tabs],
},
"other" => {
"competitionEvents" => errors[:competition_events],
},
}
end

def association_errors(association_name)
self.public_send(association_name).map(&:errors).flat_map(&:to_a)
end

def set_form_data(form_data, current_user)
JSON::Validator.validate!(Competition.form_data_json_schema, form_data)

Expand Down
10 changes: 10 additions & 0 deletions app/models/competition_tab.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,16 @@ def slug
competition.tabs.where("display_order > ?", display_order).update_all("display_order = display_order - 1")
end

validate :verify_if_full_urls
private def verify_if_full_urls
gregorbg marked this conversation as resolved.
Show resolved Hide resolved
content.scan(/\[(.*?)\]\((.*?)\)/).any? do |match|
url = match[1]
unless url.starts_with?('http://', 'https://')
errors.add(:content, I18n.t('competitions.errors.not_full_url', url: url))
end
end
end

def reorder(direction)
current_display_order = display_order
other_display_order = display_order + (direction.to_s == "up" ? -1 : 1)
Expand Down
1 change: 1 addition & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1963,6 +1963,7 @@ en:
series_distance_km: "The competition is located too far away from %{competition}."
series_distance_days: "The competition is scheduled too many days away from %{competition}."
must_ask_about_guests_if_specifying_limit: "Must ask about guests if a guest limit is specified."
not_full_url: "You have a relative URL (%{url}) in the content. Please enter full URLs with prefix http:// or https://."
new:
create_competition: "Create competition"
note_clone: "To clone an existing competition visit the information page for that competition and click the \"Clone\" link in the menu."
Expand Down
14 changes: 14 additions & 0 deletions spec/models/competition_tab_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,18 @@
expect(competition.tabs.to_a).to eq [tab1, tab2, tab3]
end
end

context "#verify_if_full_urls" do
let(:competition_tab) { FactoryBot.build(:competition_tab) }

it "doesn't allow relative URLs" do
competition_tab.update(content: "[Link](/relative)")
expect(competition_tab).to be_invalid
end

it "allows full URLs" do
competition_tab.update(content: "[Link](http://full)")
expect(competition_tab).to be_valid
end
end
end