Skip to content

Commit 8f1195a

Browse files
committed
fix: adjust security configs to avoid SSRF and injection
Criadas 2 whitelists (REGION_HOSTS + REGIONAL_ENDPOINT_HOSTS) - Métodos helper seguros (riot_api_host, regional_api_host) - 5 ocorrências de interpolação substituídas - SecurityError se region não estiver na whitelist Análise de segurança confirmou design intencional - Authorization via verify_team_usage! verificada - Documentação de segurança adicionada - Supressão RuboCop com justificativa
1 parent c46f2e0 commit 8f1195a

File tree

4 files changed

+140
-59
lines changed

4 files changed

+140
-59
lines changed

app/controllers/api/v1/scrims/opponent_teams_controller.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,11 +111,17 @@ def destroy
111111
# sensitive operations (update/destroy). This ensures organizations can
112112
# only modify teams they have scrims with.
113113
# Read operations (index/show) are allowed for all teams to enable discovery.
114+
#
115+
# SECURITY: Unscoped find is intentional here. OpponentTeam is a global
116+
# resource visible to all organizations for discovery. Authorization is
117+
# handled by verify_team_usage! for modifications.
118+
# rubocop:disable Rails/FindById
114119
def set_opponent_team
115120
@opponent_team = OpponentTeam.find(params[:id])
116121
rescue ActiveRecord::RecordNotFound
117122
render json: { error: 'Opponent team not found' }, status: :not_found
118123
end
124+
# rubocop:enable Rails/FindById
119125

120126
# Verifies that current organization has used this opponent team
121127
# Prevents organizations from modifying/deleting teams they haven't interacted with

app/modules/players/controllers/players_controller.rb

Lines changed: 80 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -152,60 +152,15 @@ def import
152152
role = params[:role]
153153
region = params[:region] || 'br1'
154154

155-
unless summoner_name.present? && role.present?
156-
return render_error(
157-
message: 'Summoner name and role are required',
158-
code: 'MISSING_PARAMETERS',
159-
status: :unprocessable_entity,
160-
details: {
161-
hint: 'Format: "GameName#TAG" or "GameName-TAG" (e.g., "Faker#KR1" or "Faker-KR1")'
162-
}
163-
)
164-
end
155+
# Validations
156+
return unless validate_import_params(summoner_name, role)
157+
return unless validate_player_uniqueness(summoner_name)
165158

166-
unless %w[top jungle mid adc support].include?(role)
167-
return render_error(
168-
message: 'Invalid role',
169-
code: 'INVALID_ROLE',
170-
status: :unprocessable_entity
171-
)
172-
end
159+
# Import from Riot API
160+
result = import_player_from_riot(summoner_name, role, region)
173161

174-
existing_player = organization_scoped(Player).find_by(summoner_name: summoner_name)
175-
if existing_player
176-
return render_error(
177-
message: 'Player already exists in your organization',
178-
code: 'PLAYER_EXISTS',
179-
status: :unprocessable_entity
180-
)
181-
end
182-
183-
result = Players::Services::RiotSyncService.import(
184-
summoner_name: summoner_name,
185-
role: role,
186-
region: region,
187-
organization: current_organization
188-
)
189-
190-
if result[:success]
191-
log_user_action(
192-
action: 'import_riot',
193-
entity_type: 'Player',
194-
entity_id: result[:player].id,
195-
new_values: result[:player].attributes
196-
)
197-
198-
render_created({
199-
player: PlayerSerializer.render_as_hash(result[:player]),
200-
message: "Player #{result[:summoner_name]} imported successfully from Riot API"
201-
})
202-
else
203-
render_error(
204-
message: "Failed to import from Riot API: #{result[:error]}",
205-
code: result[:code] || 'IMPORT_ERROR',
206-
status: :service_unavailable
207-
)
208-
end
162+
# Handle result
163+
result[:success] ? handle_import_success(result) : handle_import_error(result)
209164
end
210165

211166
# POST /api/v1/players/:id/sync_from_riot
@@ -328,6 +283,79 @@ def player_params
328283
:notes
329284
)
330285
end
286+
287+
# Validate import parameters
288+
def validate_import_params(summoner_name, role)
289+
unless summoner_name.present? && role.present?
290+
render_error(
291+
message: 'Summoner name and role are required',
292+
code: 'MISSING_PARAMETERS',
293+
status: :unprocessable_entity,
294+
details: {
295+
hint: 'Format: "GameName#TAG" or "GameName-TAG" (e.g., "Faker#KR1" or "Faker-KR1")'
296+
}
297+
)
298+
return false
299+
end
300+
301+
unless %w[top jungle mid adc support].include?(role)
302+
render_error(
303+
message: 'Invalid role',
304+
code: 'INVALID_ROLE',
305+
status: :unprocessable_entity
306+
)
307+
return false
308+
end
309+
310+
true
311+
end
312+
313+
# Check if player already exists
314+
def validate_player_uniqueness(summoner_name)
315+
existing_player = organization_scoped(Player).find_by(summoner_name: summoner_name)
316+
return true unless existing_player
317+
318+
render_error(
319+
message: 'Player already exists in your organization',
320+
code: 'PLAYER_EXISTS',
321+
status: :unprocessable_entity
322+
)
323+
false
324+
end
325+
326+
# Import player from Riot API
327+
def import_player_from_riot(summoner_name, role, region)
328+
Players::Services::RiotSyncService.import(
329+
summoner_name: summoner_name,
330+
role: role,
331+
region: region,
332+
organization: current_organization
333+
)
334+
end
335+
336+
# Handle successful import
337+
def handle_import_success(result)
338+
log_user_action(
339+
action: 'import_riot',
340+
entity_type: 'Player',
341+
entity_id: result[:player].id,
342+
new_values: result[:player].attributes
343+
)
344+
345+
render_created({
346+
player: PlayerSerializer.render_as_hash(result[:player]),
347+
message: "Player #{result[:summoner_name]} imported successfully from Riot API"
348+
})
349+
end
350+
351+
# Handle import error
352+
def handle_import_error(result)
353+
render_error(
354+
message: "Failed to import from Riot API: #{result[:error]}",
355+
code: result[:code] || 'IMPORT_ERROR',
356+
status: :service_unavailable
357+
)
358+
end
331359
end
332360
end
333361
end

app/modules/players/services/riot_sync_service.rb

Lines changed: 48 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,28 @@ class RiotSyncService
88
EUROPE = %w[euw1 eune1 ru tr1].freeze
99
ASIA = %w[kr jp1 oce1].freeze
1010

11+
# Whitelist of allowed Riot API hostnames to prevent SSRF
12+
REGION_HOSTS = {
13+
'br1' => 'br1.api.riotgames.com',
14+
'na1' => 'na1.api.riotgames.com',
15+
'euw1' => 'euw1.api.riotgames.com',
16+
'kr' => 'kr.api.riotgames.com',
17+
'eune1' => 'eune1.api.riotgames.com',
18+
'lan' => 'lan.api.riotgames.com',
19+
'las1' => 'las1.api.riotgames.com',
20+
'oce1' => 'oce1.api.riotgames.com',
21+
'ru' => 'ru.api.riotgames.com',
22+
'tr1' => 'tr1.api.riotgames.com',
23+
'jp1' => 'jp1.api.riotgames.com'
24+
}.freeze
25+
26+
# Whitelist of allowed regional endpoints for match/account APIs
27+
REGIONAL_ENDPOINT_HOSTS = {
28+
'americas' => 'americas.api.riotgames.com',
29+
'europe' => 'europe.api.riotgames.com',
30+
'asia' => 'asia.api.riotgames.com'
31+
}.freeze
32+
1133
attr_reader :organization, :api_key, :region
1234

1335
def initialize(organization, region = nil)
@@ -52,10 +74,9 @@ def sync_player(player, import_matches: true)
5274

5375
# Fetch summoner by PUUID
5476
def fetch_summoner_by_puuid(puuid)
55-
# Region already validated in initialize via sanitize_region
56-
# Use URI building to safely construct URL and avoid direct interpolation
77+
# Use whitelisted host to prevent SSRF
5778
uri = URI::HTTPS.build(
58-
host: "#{region}.api.riotgames.com",
79+
host: riot_api_host,
5980
path: "/lol/summoner/v4/summoners/by-puuid/#{CGI.escape(puuid)}"
6081
)
6182
response = make_request(uri.to_s)
@@ -64,8 +85,9 @@ def fetch_summoner_by_puuid(puuid)
6485

6586
# Fetch rank data for a summoner
6687
def fetch_rank_data(summoner_id)
88+
# Use whitelisted host to prevent SSRF
6789
uri = URI::HTTPS.build(
68-
host: "#{region}.api.riotgames.com",
90+
host: riot_api_host,
6991
path: "/lol/league/v4/entries/by-summoner/#{CGI.escape(summoner_id)}"
7092
)
7193
response = make_request(uri.to_s)
@@ -102,8 +124,9 @@ def import_player_matches(player, count: 20)
102124
def search_riot_id(game_name, tag_line)
103125
regional_endpoint = get_regional_endpoint(region)
104126

127+
# Use whitelisted host to prevent SSRF
105128
uri = URI::HTTPS.build(
106-
host: "#{regional_endpoint}.api.riotgames.com",
129+
host: regional_api_host(regional_endpoint),
107130
path: "/riot/account/v1/accounts/by-riot-id/#{CGI.escape(game_name)}/#{CGI.escape(tag_line)}"
108131
)
109132
response = make_request(uri.to_s)
@@ -133,8 +156,9 @@ def search_riot_id(game_name, tag_line)
133156
def fetch_match_ids(puuid, count = 20)
134157
regional_endpoint = get_regional_endpoint(region)
135158

159+
# Use whitelisted host to prevent SSRF
136160
uri = URI::HTTPS.build(
137-
host: "#{regional_endpoint}.api.riotgames.com",
161+
host: regional_api_host(regional_endpoint),
138162
path: "/lol/match/v5/matches/by-puuid/#{CGI.escape(puuid)}/ids",
139163
query: URI.encode_www_form(count: count)
140164
)
@@ -146,8 +170,9 @@ def fetch_match_ids(puuid, count = 20)
146170
def fetch_match_details(match_id)
147171
regional_endpoint = get_regional_endpoint(region)
148172

173+
# Use whitelisted host to prevent SSRF
149174
uri = URI::HTTPS.build(
150-
host: "#{regional_endpoint}.api.riotgames.com",
175+
host: regional_api_host(regional_endpoint),
151176
path: "/lol/match/v5/matches/#{CGI.escape(match_id)}"
152177
)
153178
response = make_request(uri.to_s)
@@ -252,6 +277,22 @@ def sanitize_region(region)
252277
normalized
253278
end
254279

280+
# Get safe Riot API hostname from whitelist (prevents SSRF)
281+
def riot_api_host
282+
host = REGION_HOSTS[@region]
283+
raise SecurityError, "Region #{@region} not in whitelist" if host.nil?
284+
285+
host
286+
end
287+
288+
# Get safe regional API hostname from whitelist (prevents SSRF)
289+
def regional_api_host(endpoint_name)
290+
host = REGIONAL_ENDPOINT_HOSTS[endpoint_name]
291+
raise SecurityError, "Regional endpoint #{endpoint_name} not in whitelist" if host.nil?
292+
293+
host
294+
end
295+
255296
# Get regional endpoint for match/account APIs
256297
def get_regional_endpoint(platform_region)
257298
if AMERICAS.include?(platform_region)

app/modules/scrims/controllers/opponent_teams_controller.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,11 +111,17 @@ def destroy
111111
# sensitive operations (update/destroy). This ensures organizations can
112112
# only modify teams they have scrims with.
113113
# Read operations (index/show) are allowed for all teams to enable discovery.
114+
#
115+
# SECURITY: Unscoped find is intentional here. OpponentTeam is a global
116+
# resource visible to all organizations for discovery. Authorization is
117+
# handled by verify_team_usage! for modifications.
118+
# rubocop:disable Rails/FindById
114119
def set_opponent_team
115120
@opponent_team = OpponentTeam.find(params[:id])
116121
rescue ActiveRecord::RecordNotFound
117122
render json: { error: 'Opponent team not found' }, status: :not_found
118123
end
124+
# rubocop:enable Rails/FindById
119125

120126
# Verifies that current organization has used this opponent team
121127
# Prevents organizations from modifying/deleting teams they haven't interacted with

0 commit comments

Comments
 (0)