Skip to content

Commit

Permalink
Sanitize organization name
Browse files Browse the repository at this point in the history
Closes issue 20376

In this pull request, I addressed an issue related to the HTML rendering
of organization names in the application. The problem manifested when
special characters, such as '&' in the organization name, were not being
properly escaped, leading to unintended rendering in the UI.

To resolve this issue, I utilized the built-in `sanitize` method
provided by Rails. This ensures that organization names are
properly sanitized before being rendered in HTML.
  • Loading branch information
laicuRoot committed Nov 24, 2023
1 parent 1cea64c commit 79af2b3
Show file tree
Hide file tree
Showing 43 changed files with 1,487 additions and 967 deletions.
6 changes: 3 additions & 3 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,7 @@ jobs:
directory: coverage/simplecov
fail_ci_if_error: ${{ github.ref != 'refs/heads/main' }}
attempt_limit: 5
attempt_delay: 30000
attempt_delay: 60000
- uses: Wandalen/wretry.action@master
with:
action: codecov/codecov-action@v3
Expand All @@ -443,7 +443,7 @@ jobs:
directory: coverage/jest
fail_ci_if_error: ${{ github.ref != 'refs/heads/main' }}
attempt_limit: 5
attempt_delay: 30000
attempt_delay: 60000
- uses: Wandalen/wretry.action@master
with:
action: codecov/codecov-action@v3
Expand All @@ -452,7 +452,7 @@ jobs:
directory: coverage/cypress
fail_ci_if_error: ${{ github.ref != 'refs/heads/main' }}
attempt_limit: 5
attempt_delay: 30000
attempt_delay: 60000

CI-status-report:
runs-on: ubuntu-latest
Expand Down
6 changes: 3 additions & 3 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ gem "cld3", "~> 3.5" # Ruby interface for Compact Language Detector v3
gem "cloudinary", "~> 1.23" # Client library for easily using the Cloudinary service
gem "counter_culture", "~> 3.2" # counter_culture provides turbo-charged counter caches that are kept up-to-date
gem "countries", "~> 5.5" # All sorts of useful information about every country packaged as pretty little country objects. It includes data from ISO 3166
gem "ddtrace", "~> 1.15.0" # ddtrace is Datadog’s tracing client for Ruby.
gem "ddtrace", "~> 1.16.2" # ddtrace is Datadog’s tracing client for Ruby.
gem "devise", "~> 4.8" # Flexible authentication solution for Rails
gem "devise_invitable", "~> 2.0.6" # Allows invitations to be sent for joining
gem "dogstatsd-ruby", "~> 4.8" # A client for DogStatsD, an extension of the StatsD metric server for Datadog
gem "dogstatsd-ruby", "~> 5.6" # A client for DogStatsD, an extension of the StatsD metric server for Datadog
gem "email_validator", "~> 2.2" # Email validator for Rails and ActiveModel
gem "emoji_regex", "~> 3.2" # A pair of Ruby regular expressions for matching Unicode Emoji symbols
gem "fastimage", "~> 2.2" # FastImage finds the size or type of an image given its uri by fetching as little as needed.
Expand Down Expand Up @@ -93,7 +93,7 @@ gem "rpush-redis", "~> 1.1" # Redis module capability for rpush library
gem "request_store", "~> 1.5" # RequestStore gives you per-request global storage
gem "reverse_markdown", "~> 2.1" # Map simple html back into markdown
gem "rolify", "~> 6.0" # Very simple Roles library
gem "rouge", "~> 3.30" # A pure-ruby code highlighter
gem "rouge", "~> 4.2" # A pure-ruby code highlighter
gem "rss", "~> 0.2.9" # Ruby's standard library for RSS
gem "rubyzip", "~> 2.3" # Rubyzip is a ruby library for reading and writing zip files
gem "s3_direct_upload", "~> 0.1" # Direct Upload to Amazon S3
Expand Down
16 changes: 8 additions & 8 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -187,11 +187,11 @@ GEM
puma (>= 3.8.0)
railties (>= 5.2.0)
dante (0.2.0)
datadog-ci (0.2.0)
datadog-ci (0.3.0)
msgpack
date (3.3.4)
ddtrace (1.15.0)
datadog-ci (~> 0.2.0)
ddtrace (1.16.2)
datadog-ci (~> 0.3.0)
debase-ruby_core_source (= 3.2.2)
libdatadog (~> 5.0.0.1.0)
libddwaf (~> 1.14.0.0.0)
Expand Down Expand Up @@ -225,7 +225,7 @@ GEM
devise (>= 4.6)
diff-lcs (1.5.0)
docile (1.4.0)
dogstatsd-ruby (4.8.3)
dogstatsd-ruby (5.6.1)
domain_name (0.5.20190701)
unf (>= 0.0.5, < 1.0.0)
dotenv (2.8.1)
Expand Down Expand Up @@ -704,7 +704,7 @@ GEM
rexml (3.2.6)
rice (4.1.0)
rolify (6.0.1)
rouge (3.30.0)
rouge (4.2.0)
rpush (7.0.1)
activesupport (>= 5.2)
jwt (>= 1.5.6)
Expand Down Expand Up @@ -993,12 +993,12 @@ DEPENDENCIES
countries (~> 5.5)
cuprite (~> 0.13)
cypress-rails (~> 0.5)
ddtrace (~> 1.15.0)
ddtrace (~> 1.16.2)
debug (>= 1.0.0)
derailed_benchmarks (~> 2.1)
devise (~> 4.8)
devise_invitable (~> 2.0.6)
dogstatsd-ruby (~> 4.8)
dogstatsd-ruby (~> 5.6)
dotenv-rails (~> 2.8.1)
easy_translate (~> 0.5.1)
email_validator (~> 2.2)
Expand Down Expand Up @@ -1078,7 +1078,7 @@ DEPENDENCIES
request_store (~> 1.5)
reverse_markdown (~> 2.1)
rolify (~> 6.0)
rouge (~> 3.30)
rouge (~> 4.2)
rpush (~> 7.0)
rpush-redis (~> 1.1)
rspec-rails (~> 6.0)
Expand Down
1 change: 1 addition & 0 deletions app/assets/javascripts/base.js.erb
Original file line number Diff line number Diff line change
Expand Up @@ -724,6 +724,7 @@ var instantClick
});

Honeybadger.beforeNotify(function(notice) {
if (!notice || typeof notice.message !== 'string') return true
const ignorePatterns = [/ResizeObserver/i, /MetaMask/i, /MtPopUpList/i, /ChunkLoadError/i]
return !(ignorePatterns.some((pattern) => pattern.test(notice.message)));
});
Expand Down
3 changes: 0 additions & 3 deletions app/assets/stylesheets/components/cards.scss
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,6 @@
&__header {
padding: var(--su-3) var(--su-4);
border-bottom: 1px solid var(--body-bg);
display: flex;
justify-content: space-between;
align-items: center;
}

&__body {
Expand Down
4 changes: 1 addition & 3 deletions app/controllers/concerns/api/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@ def show
not_found unless @user.registered
end

def me
render :show
end
def me; end

def unpublish
authorize(@user, :unpublish_all_articles?)
Expand Down
18 changes: 18 additions & 0 deletions app/controllers/sidebars_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ class SidebarsController < ApplicationController

def show
get_latest_campaign_articles
get_active_discussions if user_signed_in?
end

private
Expand All @@ -11,4 +12,21 @@ def get_latest_campaign_articles
@campaign_articles_count = Campaign.current.count
@latest_campaign_articles = Campaign.current.plucked_article_attributes
end

def get_active_discussions
tag_names = current_user.cached_followed_tag_names
languages = current_user.languages.pluck(:language)
languages = [I18n.default_locale.to_s] if languages.empty?
@active_discussions = Article.published
.where("published_at > ?", 1.week.ago)
.where("comments_count > ?", 0)
.with_at_least_home_feed_minimum_score
.cached_tagged_with_any(tag_names)
.where(language: languages)
.or(Article.featured.published.where("published_at > ?", 1.week.ago)
.with_at_least_home_feed_minimum_score)
.order("last_comment_at DESC")
.limit(5)
.pluck(:path, :title, :comments_count, :created_at)
end
end
2 changes: 1 addition & 1 deletion app/javascript/onboarding/components/ProfileForm.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export class ProfileForm extends Component {
const { next } = this.props;
next();
} catch (error) {
Honeybadger.notify(error.statusText);
Honeybadger.notify(error);
let errorMessage = 'Unable to continue, please try again.';
if (error.status === 422) {
// parse validation error messages from UsersController#onboarding
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { axe } from 'jest-axe';
import { ProfileForm } from '../ProfileForm';

global.fetch = fetch;
global.Honeybadger = { notify: jest.fn() };

describe('ProfileForm', () => {
const renderProfileForm = () =>
Expand Down Expand Up @@ -220,4 +221,26 @@ describe('ProfileForm', () => {

expect(getByText(/continue/i)).toBeInTheDocument();
});

it('should render an error message if the request failed', async () => {
const { getByRole, findByText } = render(
<ProfileForm
prev={jest.fn()}
next={jest.fn()}
slidesCount={3}
currentSlideIndex={1}
communityConfig={{ communityName: 'Community' }}
/>,
);
fetch.mockResponse(async () => {
const body = JSON.stringify({ errors: 'Fake Error' });
return new Response(body, { status: 422 });
});

const submitButton = getByRole('button', { name: 'Continue' });
submitButton.click();

const errorMessage = await findByText('An error occurred: Fake Error');
expect(errorMessage).toBeInTheDocument();
});
});
1 change: 1 addition & 0 deletions app/models/settings/general.rb
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ class General < Base
setting :twitter_hashtag, type: :string

# Tags
setting :display_sidebar_active_discussions, type: :boolean, default: true
setting :sidebar_tags, type: :array, default: %w[]

# Broadcast
Expand Down
4 changes: 2 additions & 2 deletions app/views/admin/listings/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
<%= form_tag(admin_listings_path, method: "get", role: "search", class: "crayons-card crayons-card--secondary p-4 flex items-center") do %>
<div class="mr-4">
<%= label_tag(:search, "Keyword", class: "mb-0") %>
<%= text_field_tag(:search, params[:search]) %>
<%= text_field_tag(:search, params[:search], autocomplete: "off") %>
<% if params[:state].present? %>
<%= hidden_field_tag(:state, params[:state]) %>
<% end %>
Expand Down Expand Up @@ -52,7 +52,7 @@
<tr>
<td><%= link_to listing.title, edit_admin_listing_path(listing.id), rel: "noopener" %></td>
<td><%= link_to listing.user.username, admin_user_path(listing.user.id) %></td>
<td><%= link_to listing.organization.name, admin_organization_path(listing.organization_id) if listing.organization_id.present? %></td>
<td><%= link_to sanitize(listing.organization.name), admin_organization_path(listing.organization_id) if listing.organization_id.present? %></td>
<td><%= listing.category %></td>
<td><%= listing.cached_tag_list %></td>
<td><%= listing.published ? "Yes" : "No" %></td>
Expand Down
2 changes: 1 addition & 1 deletion app/views/admin/organizations/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
<tbody class="crayons-card">
<% @organizations.each do |organization| %>
<tr>
<td><%= link_to "@#{organization.name}", admin_organization_path(organization.id) %></td>
<td><%= link_to sanitize("@#{organization.name}"), admin_organization_path(organization.id) %></td>
<td class="whitespace-nowrap"><%= organization.id %></td>
<% if organization.twitter_username %>
<td><%= link_to organization.twitter_username, "https://twitter.com/#{organization.twitter_username}" %></td>
Expand Down
2 changes: 1 addition & 1 deletion app/views/admin/organizations/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
<div class="s:flex items-center gap-5">
<div class="flex flex-1 items-center">
<h1 class="crayons-title lh-tight">
<%= @organization.name %>
<%= sanitize(@organization.name) %>
</h1>
</div>
<div class="flex relative justify-between s:justify-end gap-2 my-2 s:my-0">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
</a>
<h3 class="crayons-subtitle-2">
<a href="<%= org_membership.organization.path %>" target="_blank" rel="noopener" class="c-link">
<%= org_membership.organization.name %>
<%= sanitize(org_membership.organization.name) %>
</a>
<span class="opacity-50">&bull;</span> <%= t("views.admin.users.overview.orgs.type.#{org_membership.type_of_user}") %></span>
</h3>
Expand Down
1 change: 1 addition & 0 deletions app/views/api/v0/users/me.json.jbuilder
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
json.partial! "api/v0/shared/user_show", user: @user
1 change: 1 addition & 0 deletions app/views/api/v1/shared/_user_show_extended.json.jbuilder
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,4 @@ end

json.joined_at I18n.l(user.created_at, format: :json)
json.profile_image user.profile_image_url_for(length: 320)
json.badge_ids user.badge_ids
2 changes: 2 additions & 0 deletions app/views/api/v1/users/me.json.jbuilder
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
json.partial! "api/v1/shared/user_show_extended", user: @user
json.followers_count @user.followers_count
2 changes: 1 addition & 1 deletion app/views/dashboards/following_organizations.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
<div class="pl-4 s:pl-0 self-center">
<h3 class="s:mb-1 p-0">
<a href="<%= organization.path %>">
<%= organization.name %>
<%= sanitize(organization.name) %>
</a>
</h3>

Expand Down
2 changes: 1 addition & 1 deletion app/views/organizations/_liquid.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
</a>
<div class="ltag__user__content">
<h2>
<a href="/<%= organization.slug %>" class="ltag__user__link"><%= organization.name %></a>
<a href="/<%= organization.slug %>" class="ltag__user__link"><%= sanitize(organization.name) %></a>
<%= follow_button(organization, "full", "c-btn--secondary fs-base") %>
</h2>
<div class="ltag__user__summary">
Expand Down
4 changes: 2 additions & 2 deletions app/views/organizations/members.html.erb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<% title @organization.name %>
<% title sanitize(@organization.name) %>
<div class="blank-space"></div>
<main class="org-member-page crayons-layout px-4 m:px-9 gap-0">
<h1 class="crayons-title"><%= @organization.name %> (<%= @members.count %>)</h1>
<h1 class="crayons-title"><%= sanitize(@organization.name) %> (<%= @members.count %>)</h1>
<div class="mt-4 l:mt-7 grid gap-4 grid-cols-1 s:grid-cols-1 m:grid-cols-2 l:grid-cols-3 xl:grid-cols-4 mb-6">
<% @members.each_with_index do |member, index| %>
<div class="crayons-card member-item">
Expand Down
2 changes: 1 addition & 1 deletion app/views/shared/_billboard_header.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
<% else %>
<a href="<%= billboard.organization.path %>" target="_blank" rel="noopener" class="flex">
<img width="24" height="24" class="radius-default crayons-sponsorship__image" src="<%= billboard.organization.profile_image_url_for(length: 64) %>" alt="profile" loading="lazy" />
<div class="crayons-sponsorship__title ml-2 fs-s fw-medium"><%= billboard.organization.name %></div>
<div class="crayons-sponsorship__title ml-2 fs-s fw-medium"><%= sanitize(billboard.organization.name) %></div>
</a>
<% end %>
<% if billboard.type_of == "external" %>
Expand Down
37 changes: 28 additions & 9 deletions app/views/sidebars/_homepage_content.html.erb
Original file line number Diff line number Diff line change
@@ -1,26 +1,45 @@
<aside class="side-bar sidebar-additional showing grid gap-4" id="sidebar-additional">
<% cache(release_adjusted_cache_key("main-article-right-sidebar-discussions-#{params[:timeframe]}-#{user_signed_in?}"), expires_in: (params[:timeframe].blank? ? 120 : 360).seconds) do %>
<% @sidebar_billboard = Billboard.for_display(area: "sidebar_right", user_signed_in: user_signed_in?) %>
<% if @sidebar_billboard %>
<%= render partial: "shared/billboard", locals: { billboard: @sidebar_billboard, data_context_type: BillboardEvent::CONTEXT_TYPE_HOME } %>
<% @sidebar_billboard = Billboard.for_display(area: "sidebar_right", user_signed_in: user_signed_in?) %>
<% if @sidebar_billboard %>
<%= render partial: "shared/billboard", locals: { billboard: @sidebar_billboard, data_context_type: BillboardEvent::CONTEXT_TYPE_HOME } %>
<% end %>
<% if user_signed_in? && @active_discussions.any? %>
<% if Settings::General.display_sidebar_active_discussions %>
<section class="crayons-card crayons-card--secondary crayons-layout__content" id="active-discussions">
<header class="crayons-card__header">
<h3 class="crayons-subtitle-2">
<%= t("views.main.side.active_discussions") %>
</h3>
</header>
<div>
<% @active_discussions.each do |plucked_article| %>
<%= render "articles/widget_list_item", plucked_article: plucked_article, show_comment_count: true %>
<% end %>
</div>
</section>
<% end %>
<% end %>
<% cache(release_adjusted_cache_key("main-article-right-sidebar-discussions-#{params[:timeframe]}-#{user_signed_in?}"), expires_in: (params[:timeframe].blank? ? 120 : 360).seconds) do %>
<%= render "articles/sidebar_campaign" if Campaign.current.show_in_sidebar? %>
<%= render "articles/sidebar_listings" if Listing.feature_enabled? %>
<% Settings::General.sidebar_tags.each do |tag| %>
<% Tag.where(name: Settings::General.sidebar_tags).find_each do |tag| %>
<section class="crayons-card crayons-card--secondary crayons-layout__content">
<header class="crayons-card__header">
<h3 class="crayons-subtitle-2"><a href="/t/<%= tag %>" class="crayons-link">#<%= tag %></a></h3>
<h3 class="crayons-subtitle-2"><a href="/t/<%= tag.name %>" class="crayons-link">#<%= tag.name %></a></h3>
<div class="fs-xs color-base-70">
<%= tag.short_summary %>
</div>
</header>

<div>
<% if tag == "help" %>
<% if tag.name == "help" %>
<% Article.active_help.limit(5).pluck(:path, :title, :comments_count, :created_at).each do |plucked_article| %>
<%= render "articles/widget_list_item", plucked_article: plucked_article, show_comment_count: true %>
<% end %>
<% else %>
<% active_threads(tags: [tag], time_ago: Timeframe.datetime(params[:timeframe]), count: 5).each do |plucked_article| %>
<% active_threads(tags: [tag.name], time_ago: Timeframe.datetime(params[:timeframe]), count: 5).each do |plucked_article| %>
<%= render "articles/widget_list_item", plucked_article: plucked_article, show_comment_count: true %>
<% end %>
<% end %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/users/_org_member.html.erb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<div>
<h2><%= @organization.name %></h2>
<h2><%= sanitize(@organization.name) %></h2>
<div class="grid gap-2">
<% @organization.users.each do |user| %>
<div class="crayons-card crayons-card--secondary p-4">
Expand Down
2 changes: 1 addition & 1 deletion app/views/users/_sidebar.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
<span class="crayons-logo crayons-logo--l mr-2 shrink-0">
<img src="<%= organization.profile_image_url_for(length: 90) %>" alt="<%= organization.name %> profile image" class="crayons-logo__image" loading="lazy" />
</span>
<h4 class="fs-base fw-medium"><%= organization.name %></h4>
<h4 class="fs-base fw-medium"><%= sanitize(organization.name) %></h4>
</a>
<% end %>
</div>
Expand Down
2 changes: 1 addition & 1 deletion app/views/users/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
<div class="profile-header__details" data-url="<%= user_path(@user, format: "json") %>">
<div class="items-center js-username-container mb-2">
<h1 class="crayons-title lh-tight">
<%= @user.name %>
<%= sanitize @user.name %>
</h1>
</div>

Expand Down
1 change: 1 addition & 0 deletions codecov.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# NB: Run the following command to verify this file is valid
# > cat codecov.yml | curl --data-binary @- https://codecov.io/validate

comment: false
codecov:
ci:
- "!buildkite"
Expand Down

0 comments on commit 79af2b3

Please sign in to comment.