Refs #39303 - Narrow redhat_cdn_host? to actual CDN hostnames#11766
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a reusable ChangesRed Hat CDN Host Pattern Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/katello/cdn_configuration.rb`:
- Around line 46-48: The redhat_cdn_host? method currently calls URI.parse(url)
without guarding against nil/blank and only rescues URI::InvalidURIError; update
redhat_cdn_host? to first return false for nil/blank url (e.g., return false if
url.blank? or url.nil?) before calling URI.parse(url). Keep the existing rescue
for URI::InvalidURIError and ensure the method uses REDHAT_CDN_HOST_PATTERN on
the parsed host as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06c52de2-87b0-4647-94b2-cf2b6b5a19eb
📒 Files selected for processing (2)
app/models/katello/cdn_configuration.rbtest/models/cdn_configuration_test.rb
| URI.parse(url).host&.match?(REDHAT_CDN_HOST_PATTERN) | ||
| rescue URI::InvalidURIError | ||
| false |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Ruby's stdlib URI module, what exception does URI.parse raise for nil input, and what exception does it raise for malformed URL strings?
💡 Result:
For nil input: URI.parse(nil) raises NoMethodError (e.g., “undefined method `[]' for nil:NilClass”), instead of the expected URI::InvalidURIError in some cases [1]. For malformed URL strings: URI.parse("...") raises URI::InvalidURIError when the URI string does not match the parser’s expected syntax (e.g., URI::RFC2396_Parser.parse / URI.parse shown in docs to raise URI::InvalidURIError) [2][3].
Citations:
- 1: https://bugs.ruby-lang.org/issues/16830
- 2: https://ruby-doc.org/3.3/stdlibs/uri/URI/RFC2396_Parser.html
- 3: https://github.com/ruby/ruby/blob/v3_0_3/lib/uri/common.rb
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the method definition around the reported lines
FILE="app/models/katello/cdn_configuration.rb"
if [ -f "$FILE" ]; then
echo "== $FILE =="
nl -ba "$FILE" | sed -n '1,140p' | sed -n '35,90p'
else
echo "File not found: $FILE"
fi
# Find references to redhat_cdn_host? and to any method/attribute named url in this context
rg -n "redhat_cdn_host\?" -S .
rg -n "cdn_configuration\.rb" -S .
rg -n "URI\.parse\(url\)" -S .
# Find where `url` is coming from in redhat_cdn_host? (could be an attribute, method, or local)
# Print the whole method if found
rg -n "def redhat_cdn_host\?" -S app/models/katello/cdn_configuration.rbRepository: Katello/katello
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="app/models/katello/cdn_configuration.rb"
echo "== $FILE =="
if [ -f "$FILE" ]; then
# Print lines 35-90 with line numbers using awk (no nl available)
awk 'NR>=35 && NR<=90 {printf "%6d:%s\n", NR, $0}' "$FILE"
else
echo "File not found: $FILE"
fi
echo
echo "== Search: redhat_cdn_host? =="
rg -n "def redhat_cdn_host\?" "$FILE" || true
rg -n "redhat_cdn_host\?" -S app/models/katello || true
echo
echo "== Search: where URI.parse(url) occurs =="
rg -n "URI\.parse\(url\)" -S app/models/katello/cdn_configuration.rb || true
rg -n "URI\.parse\(" -S app/models/katello/cdn_configuration.rb || true
echo
echo "== Search: possible `url` sources in this file =="
rg -n "\burl\b" -S app/models/katello/cdn_configuration.rb || trueRepository: Katello/katello
Length of output: 2395
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="app/models/katello/cdn_configuration.rb"
awk 'NR>=1 && NR<=90 {printf "%6d:%s\n", NR, $0}' "$FILE"
echo
echo "== Call sites for redhat_cdn_host? =="
rg -n "redhat_cdn_host\?" -S . || trueRepository: Katello/katello
Length of output: 4180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect call sites with context
echo "== app/services/katello/pulp3/repository.rb around 500-580 =="
awk 'NR>=500 && NR<=580 {printf "%6d:%s\n", NR, $0}' app/services/katello/pulp3/repository.rb
echo
echo "== app/lib/katello/resources/cdn.rb around 40-95 =="
awk 'NR>=40 && NR<=110 {printf "%6d:%s\n", NR, $0}' app/lib/katello/resources/cdn.rb
echo
echo "== test/models/cdn_configuration_test.rb around 1-90 =="
awk 'NR>=1 && NR<=110 {printf "%6d:%s\n", NR, $0}' test/models/cdn_configuration_test.rbRepository: Katello/katello
Length of output: 11553
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== app/services/katello/pulp3/repository.rb around 500-580 =="
awk 'NR>=500 && NR<=580 {printf "%6d:%s\n", NR, $0}' app/services/katello/pulp3/repository.rb
echo
echo "== app/lib/katello/resources/cdn.rb around 40-110 =="
awk 'NR>=40 && NR<=110 {printf "%6d:%s\n", NR, $0}' app/lib/katello/resources/cdn.rb
echo
echo "== test/models/cdn_configuration_test.rb around 1-110 =="
awk 'NR>=1 && NR<=110 {printf "%6d:%s\n", NR, $0}' test/models/cdn_configuration_test.rbRepository: Katello/katello
Length of output: 11555
Guard redhat_cdn_host? against nil/blank url.
redhat_cdn_host? rescues only URI::InvalidURIError, but URI.parse(url) can raise when url is nil/blank (Ruby commonly raises NoMethodError), so the method may not reliably return false. Add a url.blank?/nil guard (or rescue NoMethodError) to make it defensive.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/models/katello/cdn_configuration.rb` around lines 46 - 48, The
redhat_cdn_host? method currently calls URI.parse(url) without guarding against
nil/blank and only rescues URI::InvalidURIError; update redhat_cdn_host? to
first return false for nil/blank url (e.g., return false if url.blank? or
url.nil?) before calling URI.parse(url). Keep the existing rescue for
URI::InvalidURIError and ensure the method uses REDHAT_CDN_HOST_PATTERN on the
parsed host as before.
|
Actionable comments posted: 0 |
vsedmik
force-pushed
the
fix-39303-redhat-cdn-host-check
branch
from
1e4e65d to
cfe13fb
Compare
|
Actionable comments posted: 0 |
3 participants
Problem
The
*.redhat.comcheck inredhat_cdn_host?is too broad — it matches any host in theredhat.comdomain, including Satellite instances hosted in Red Hat infrastructure. This caused subscription client certificates to be injected when enabling repos via a custom CDN pointing at such a host, which the server rejected withtlsv1 alert unknown ca.Solution
Narrow the check to
cdn.redhat.comandcdn-{region}.redhat.com(e.g.cdn-eu.redhat.com,cdn-us.redhat.com) using a constant regexREDHAT_CDN_HOST_PATTERN. The three tests added by #11732 are unaffected — all usecdn-eu.redhat.comwhich still matches.Testing
Added unit tests for
redhat_cdn_host?covering:cdn.redhat.com,cdn-eu.redhat.com,cdn-us.redhat.com) → match*.redhat.comhosts → no matchExisting tests in
cdn_test.rbandyum_test.rbadded by #11732 pass unchanged.Robottelo test that hit this issue should pass with this fix:
Summary by CodeRabbit
Bug Fixes
Tests