fix: reject cross-account author on help center articles (#15191)
## Description The help center article endpoints permit `author_id` and render the author through the agent serializer (`_agent.json.jbuilder`), which exposes `email` plus name, role, and availability. `author_id` was never scoped to the current account, so a forged or stale author disclosed the profile of a user in another account. This is the article path of the same serializer disclosure class as the conversation participants fix. The fix has two parts: - **Serializer guard (the disclosure fix).** The article partials now render the author only when they are a member of the current account (`article.author&.account`). This closes the leak on every read and for every row, including articles that already carry a forged or stale out-of-account author, and mirrors how the conversation assignee is already handled. - **Create-only validation.** `author_id` is checked against the account's users on create, so a new article cannot be forged with an out-of-account author. Update needs no guard: a non-member author is simply never rendered, so editing an article whose author has left the account continues to work. Fixes https://linear.app/chatwoot/issue/CW-7665 Related: https://github.com/chatwoot/chatwoot/pull/15180 (https://linear.app/chatwoot/issue/CW-7746).
This commit is contained in:
@@ -3,6 +3,7 @@ class Api::V1::Accounts::ArticlesController < Api::V1::Accounts::BaseController
|
||||
before_action :check_authorization
|
||||
before_action :fetch_article, except: [:index, :create, :reorder]
|
||||
before_action :set_current_page, only: [:index]
|
||||
before_action :validate_author, only: [:create]
|
||||
|
||||
def index
|
||||
@portal_articles = @portal.articles
|
||||
@@ -63,6 +64,14 @@ class Api::V1::Accounts::ArticlesController < Api::V1::Accounts::BaseController
|
||||
@article = @portal.articles.find(params[:id])
|
||||
end
|
||||
|
||||
def validate_author
|
||||
author_id = params.dig(:article, :author_id)
|
||||
return if author_id.blank?
|
||||
return if author_id.to_s.match?(/\A\d+\z/) && Current.account.users.exists?(id: author_id.to_i)
|
||||
|
||||
render json: { error: 'Invalid author ID' }, status: :unprocessable_entity
|
||||
end
|
||||
|
||||
def portal
|
||||
@portal ||= Current.account.portals.find_by!(slug: params[:portal_id])
|
||||
end
|
||||
|
||||
@@ -22,7 +22,7 @@ end
|
||||
|
||||
json.views article.views
|
||||
|
||||
if article.author.present?
|
||||
if article.author&.account
|
||||
json.author do
|
||||
json.partial! 'api/v1/models/agent', formats: [:json], resource: article.author
|
||||
end
|
||||
|
||||
@@ -14,7 +14,7 @@ end
|
||||
|
||||
json.views article.views
|
||||
|
||||
if article.author.present?
|
||||
if article.author&.account
|
||||
json.author do
|
||||
json.partial! 'api/v1/models/agent', formats: [:json], resource: article.author
|
||||
end
|
||||
|
||||
@@ -40,6 +40,40 @@ RSpec.describe 'Api::V1::Accounts::Articles', type: :request do
|
||||
expect(json_response['payload']['position']).to be(3)
|
||||
end
|
||||
|
||||
it 'rejects a cross-account author without exposing their details' do
|
||||
foreign_user = create(:user, account: create(:account), role: :agent)
|
||||
article_params = {
|
||||
article: {
|
||||
category_id: category.id,
|
||||
title: 'MyTitle',
|
||||
slug: 'my-title',
|
||||
content: 'This is my content.',
|
||||
status: :published,
|
||||
author_id: foreign_user.id
|
||||
}
|
||||
}
|
||||
post "/api/v1/accounts/#{account.id}/portals/#{portal.slug}/articles",
|
||||
params: article_params,
|
||||
headers: admin.create_new_auth_token
|
||||
|
||||
expect(response).to have_http_status(:unprocessable_entity)
|
||||
expect(response.parsed_body).to eq('error' => 'Invalid author ID')
|
||||
expect(response.body).not_to include(foreign_user.email)
|
||||
expect(portal.articles.where(author_id: foreign_user.id)).to be_empty
|
||||
end
|
||||
|
||||
it 'rejects a malformed author_id without raising' do
|
||||
[%w[3 4], ' 3 ', '3abc'].each do |bad_author_id|
|
||||
post "/api/v1/accounts/#{account.id}/portals/#{portal.slug}/articles",
|
||||
params: { article: { title: 'MyTitle', slug: 'my-title', content: 'This is my content.', author_id: bad_author_id } },
|
||||
headers: admin.create_new_auth_token,
|
||||
as: :json
|
||||
|
||||
expect(response).to have_http_status(:unprocessable_entity)
|
||||
expect(response.parsed_body).to eq('error' => 'Invalid author ID')
|
||||
end
|
||||
end
|
||||
|
||||
it 'creates article even if category is not provided' do
|
||||
article_params = {
|
||||
article: {
|
||||
@@ -171,6 +205,30 @@ RSpec.describe 'Api::V1::Accounts::Articles', type: :request do
|
||||
expect(json_response['payload']['position']).to eql(article_params[:article][:position])
|
||||
end
|
||||
|
||||
it 'does not expose a cross-account author set through update' do
|
||||
foreign_user = create(:user, account: create(:account), role: :agent)
|
||||
|
||||
put "/api/v1/accounts/#{account.id}/portals/#{portal.slug}/articles/#{article.id}",
|
||||
params: { article: { author_id: foreign_user.id } },
|
||||
headers: admin.create_new_auth_token
|
||||
|
||||
expect(response).to have_http_status(:success)
|
||||
expect(response.body).not_to include(foreign_user.email)
|
||||
expect(response.parsed_body['payload']).not_to have_key('author')
|
||||
end
|
||||
|
||||
it 'allows editing an article whose author is no longer an account member' do
|
||||
foreign_user = create(:user, account: create(:account), role: :agent)
|
||||
article.update!(author_id: foreign_user.id)
|
||||
|
||||
put "/api/v1/accounts/#{account.id}/portals/#{portal.slug}/articles/#{article.id}",
|
||||
params: { article: { title: 'Updated title', author_id: foreign_user.id } },
|
||||
headers: admin.create_new_auth_token
|
||||
|
||||
expect(response).to have_http_status(:success)
|
||||
expect(article.reload.title).to eq('Updated title')
|
||||
end
|
||||
|
||||
it 'stages draft-only fields without bumping updated_at' do
|
||||
expect do
|
||||
put "/api/v1/accounts/#{account.id}/portals/#{portal.slug}/articles/#{article.id}",
|
||||
@@ -329,6 +387,18 @@ RSpec.describe 'Api::V1::Accounts::Articles', type: :request do
|
||||
expect(json_response['payload']['id']).to eq(article2.id)
|
||||
end
|
||||
|
||||
it 'does not expose an author who is not a member of the account' do
|
||||
foreign_user = create(:user, account: create(:account), role: :agent)
|
||||
article.update!(author_id: foreign_user.id)
|
||||
|
||||
get "/api/v1/accounts/#{account.id}/portals/#{portal.slug}/articles/#{article.id}",
|
||||
headers: admin.create_new_auth_token
|
||||
|
||||
expect(response).to have_http_status(:success)
|
||||
expect(response.parsed_body['payload']).not_to have_key('author')
|
||||
expect(response.body).not_to include(foreign_user.email)
|
||||
end
|
||||
|
||||
it 'get associated articles' do
|
||||
root_article = create(:article, category: category, portal: portal, account_id: account.id, author_id: agent.id, associated_article_id: nil)
|
||||
child_article_1 = create(:article, slug: 'child-1', category: category, portal: portal, account_id: account.id, author_id: agent.id,
|
||||
|
||||
Reference in New Issue
Block a user