THREESCALE-15550: Upgrade uri gem by madnialihussain · Pull Request #4356 · 3scale/porta

madnialihussain · 2026-07-24T09:50:50Z

What this PR does / why we need it

Upgrades the uri gem from 0.13.3 to 1.1.1 with a full RFC3986 migration. All URI::DEFAULT_PARSER, URI::RFC2396_PARSER, and URI::REGEXP references have been eliminated.

Key Changes & Migrations

make_regexp → UriValidator: Updated in authentication_provider.rb, web_hook.rb, and proxy_rule.rb. Replaces unanchored regex matching with URI.parse-backed validation; correctly rejects URLs with leading/trailing whitespace.
URI::DEFAULT_PARSER.pattern / URI::REGEXP::PATTERN::* → App-owned constants: Updated in proxy.rb and proxy_rule.rb. Inlined PCHAR, SEGMENT, ABS_PATH, QUERY, UNRESERVED, ESCAPED, and RESERVED as local variables/constants (I checked its same as originals).
URI.decode / URI::RFC2396_PARSER.unescape → CGI.unescapeURIComponent: Updated across 3 files. Both produce identical output for percent-decoding; + is preserved as a literal (not converted to space).
make_regexp text scanning → %r{https?://[^\s)\]>]+}: Updated in messages_helper.rb. URL detection replaced with a simple regex; existing trailing-punctuation stripping handles over-matching.
RFC2396 HOSTNAME regex → RFC3986 reg-name regex: Updated in proxy.rb. Widened hostname validation to allow underscores (_).

Bug Fixes & Refactoring

UriValidator cleanup (uri_validator.rb):
- Removed registry and opaque from DEFAULT_PERMISSIONS_OF_PARTS (RFC2396 artifacts; always nil under RFC3986).
- Fixed a pre-existing bug where generic_error_message attr_reader was never initialized (returning nil on blank URIs). Hardcoded :invalid and removed the dead attr_reader.
Strong Params Fix (service_discovery/auth_controller.rb):
- Replaced params[:referrer] with params.permit(:referrer)[:referrer].

Behavioral Changes

API Error Messages: AuthenticationProvider URL validation errors changed from "Invalid URL format" to "invalid" in JSON API responses (switched from :invalid_url on format: to UriValidator's default :invalid).
Whitespace URLs: URLs with leading/trailing whitespace (e.g., ' http://example.com ') now produce two validation errors (["invalid", "can't contain whitespaces"]) instead of passing unanchored validation.
Underscore Hostnames: hostname_rewrite in proxy.rb now accepts hostnames containing underscores (e.g., my_proxy.internal).

Which issue(s) this PR fixes

Fixes THREESCALE-15550

Verification Steps

All test should pass:

akostadinov · 2026-07-24T10:18:21Z

I'm ok with keeping RFC2396, but iassume many people hit these changes. What is the rationale not to switch to the new parser everywhere with whatever migration path is common for these many users? Like the URL pattern, what is upstream project recommending as a migration to these?

madnialihussain · 2026-07-24T12:16:01Z

I'm ok with keeping RFC2396, but iassume many people hit these changes. What is the rationale not to switch to the new parser everywhere with whatever migration path is common for these many users? Like the URL pattern, what is upstream project recommending as a migration to these?

Thanks for the review @akostadinov.

I investigated for full RFC3986 migration but it's not viable for us

RFC3986_PARSER doesn't provide equivalents for the APIs we use. Its make_regexp, unescape, escape, extract all delegate to RFC2396_PARSER and its deprecation warnings saying "Use URI::RFC2396_PARSER explicitly".

For .pattern and PATTERN::* constants, there are no RFC3986 equivalents at all.

Other projects hit the same issue and took the same approach, CarrierWave (#2760) and Capybara (#2781).

akostadinov · 2026-07-24T16:30:43Z

To me this PR as a quick compatibility patch up and not a proper migration. What I believe the issue is about is understanding the implications of using one parsing method or another and use a future proof method.

This is a test script to demonstrate what presently the different approaches result into:

#!/usr/bin/env ruby
# frozen_string_literal: true
# Run with: ruby uri_parser_repro.rb (NOT bundle exec)

require 'rubygems'
gem 'uri', '0.13.0'
require 'uri'

TEST_URLS = [
  'http://my_proxy.internal:8080',
  'https://foo_bar.example.com',
  'http://exa_mple',
  'http://example.com',
  'http://bad host'
].freeze

puts "RUBY=#{RUBY_VERSION}"
puts "URI_GEM=#{Gem.loaded_specs.fetch('uri').version}"
puts "DEFAULT_PARSER_CLASS=#{URI::DEFAULT_PARSER.class}"
puts "Available PARSER constants: #{URI.constants.grep(/PARSER/).sort.join(', ')}"

default_regexp = URI::DEFAULT_PARSER.make_regexp(%w[http https])

TEST_URLS.each do |url|
  puts
  puts "URL=#{url}"

  begin
    parsed = URI.parse(url)
    puts "URI.parse: ok host=#{parsed.host.inspect}"
  rescue => e
    puts "URI.parse: #{e.class}: #{e.message}"
  end

  begin
    parsed = URI::DEFAULT_PARSER.parse(url)
    puts "DEFAULT_PARSER.parse: ok host=#{parsed.host.inspect}"
  rescue => e
    puts "DEFAULT_PARSER.parse: #{e.class}: #{e.message}"
  end

  puts "DEFAULT_PARSER.make_regexp: #{!!(url =~ default_regexp)}"

The result is:

$ ruby uri_parser_repro.rb
RUBY=3.3.1
URI_GEM=0.13.0
DEFAULT_PARSER_CLASS=URI::RFC2396_Parser
Available PARSER constants: DEFAULT_PARSER, RFC3986_PARSER

URL=http://my_proxy.internal:8080
URI.parse: ok host="my_proxy.internal"
DEFAULT_PARSER.parse: URI::InvalidURIError: the scheme http does not accept registry part: my_proxy.internal:8080 (or bad hostname?)
DEFAULT_PARSER.make_regexp: true

URL=https://foo_bar.example.com
URI.parse: ok host="foo_bar.example.com"
DEFAULT_PARSER.parse: URI::InvalidURIError: the scheme https does not accept registry part: foo_bar.example.com (or bad hostname?)
DEFAULT_PARSER.make_regexp: true

URL=http://exa_mple
URI.parse: ok host="exa_mple"
DEFAULT_PARSER.parse: URI::InvalidURIError: the scheme http does not accept registry part: exa_mple (or bad hostname?)
DEFAULT_PARSER.make_regexp: true

URL=http://example.com
URI.parse: ok host="example.com"
DEFAULT_PARSER.parse: ok host="example.com"
DEFAULT_PARSER.make_regexp: true

URL=http://bad host
URI.parse: URI::InvalidURIError: bad URI(is not URI?): "http://bad host"
DEFAULT_PARSER.parse: URI::InvalidURIError: bad URI(is not URI?): http://bad host
DEFAULT_PARSER.make_regexp: true

I'm not fully against a 100% compatible quick patch but current PR seems to change the behavior at places to be more strict and reject underscores (where URI.parse was used, now we use the RFC3986_PARSER.parse which rejects them).

I'll ask Bob to draft an alternative approach where our validation is more specific on what we care about. And we can decide whether to try keeping 100% old behavior, keep it vaguely or be more precise with our validation.

akostadinov-bot · 2026-07-24T16:33:03Z

Review: URI gem upgrade needs proper migration, not just compatibility patches

This PR upgrades uri from 0.13.3 to 1.1.1, but the approach is mostly patching for compatibility rather than migrating to proper URL validation.

Core Issues

make_regexp is not a validator: Even in uri 0.13.0, URI::DEFAULT_PARSER.make_regexp accepts invalid URLs like http://bad host and is inconsistent with actual parsing behavior.
RFC3986 vs DNS hostnames: The new default parser (RFC3986) accepts underscore hostnames like http://my_proxy.internal because RFC3986 defines generic URI syntax, not DNS hostname rules. This is a behavioral change from strict RFC2396 parsing.
Parser internals coupling: Files like proxy.rb, proxy_rule.rb, authentication_provider.rb, and web_hook.rb depend on URI::DEFAULT_PARSER.pattern and make_regexp, which are parser implementation details, not stable validation APIs.

Recommended Approach

Replace regex-based validators with parse-based semantic validation plus explicit hostname policy.

Example validator:

class HttpUrlValidator < ActiveModel::EachValidator
  def validate_each(record, attribute, value)
    return if value.blank? && options[:allow_blank]
    
    uri = Addressable::URI.parse(value)
    
    unless %w[http https].include?(uri.scheme)
      record.errors.add(attribute, 'must be http or https')
      return
    end
    
    unless valid_hostname?(uri.host)
      record.errors.add(attribute, 'has invalid hostname')
    end
  rescue Addressable::URI::InvalidURIError
    record.errors.add(attribute, 'is not a valid URL')
  end
  
  private
  
  def valid_hostname?(host)
    return false if host.blank?
    return true if host =~ /\A\d+\.\d+\.\d+\.\d+\z/ # IPv4
    return true if host =~ /\A\[.*\]\z/ # IPv6
    return true if host == 'localhost'
    
    # DNS hostname validation (RFC 1123)
    labels = host.split('.')
    return false if labels.empty?
    
    labels.all? do |label|
      label.length <= 63 &&
      label =~ /\A[a-z0-9]([a-z0-9-]*[a-z0-9])?\z/i
    end
  end
end

File-by-File Migration Plan

app/models/authentication_provider.rb

Current: format: { with: URI::DEFAULT_PARSER.make_regexp(%w[http https]) }
Needed: Full HTTP(S) URL with valid DNS hostname
Migration: validates :site, :token_url, :authorize_url, :user_info_url, http_url: true

app/models/web_hook.rb

Current: format: { with: URI::DEFAULT_PARSER.make_regexp(%w[http https]) }
Needed: Full HTTP(S) URL with valid DNS hostname
Migration: validates :url, http_url: true, if: :active

app/models/proxy.rb

Current: Mix of uri: true and URI::DEFAULT_PARSER.pattern regex composition
Needed:
- endpoint, sandbox_endpoint: full HTTP(S) URLs (already using uri: true, keep it)
- api_test_path: absolute path + optional query (custom validator)
- hostname_rewrite: hostname[:port] (custom validator)
- oauth_login_url: HTTP(S) URL without oauth params (custom validator)
Migration: Keep uri: true for endpoints, create app-owned validators for components

app/models/proxy_rule.rb

Current: URI::REGEXP::PATTERN::* for custom route syntax with {var} placeholders
Needed: App-specific route pattern grammar
Migration: Define Porta-owned pattern parser, stop depending on URI::REGEXP::PATTERN constants. For redirect_url, use http_url: true.

app/helpers/messages_helper.rb

Current: URI.regexp(%w(http https)) for URL detection in text
Needed: Tolerant URL linkification
Migration: Use explicit tokenization + parse-based validation, or a dedicated linkifier library

Why This Matters

make_regexp and parser internals are not designed as validation APIs
Keeping uri < 1.0.0 postpones the problem but does not fix the design issue
Porta already uses addressable in several places; we should standardize on parse-based validation
Explicit hostname policy is clearer and more maintainable than relying on parser regex behavior

Reproduction

See uri_parser_repro.rb in the repo root demonstrating the make_regexp vs parse inconsistency even in uri 0.13.0.

akostadinov · 2026-07-24T16:45:09Z

As an answer to @akostadinov-bot comment,

I'm fine to use URI.parse or Addressable::URI.parse, doesn't matter much to me given we perform further validation anyway
for app/models/proxy_rule.rb I'm fine with keeping the current regexp approach (although also fine if we come up with a better implementation).

To reiterate, I am interested in getting the question better understood and that we make a decision for the long-term. Not merely coming up with some implementation that tolerates uri gem 1.0.0+ and tests pass. So treat the above as a point of a discussion, not as something I necessarily want to implemented.

jlledom

I agree with @akostadinov. I think the point of this issue is to migrate the whole application to RFC3986, othwerwise we are making the tech debt deeper. For instance, in thie PR we are explicitly calling the old parser when needed, but the latest uri gem is using RFC3986 as the default parser internally, this could cause inconsistencies in how the same data is treated. This mix of RFCs will be a source of problems for sure.

The best way to solve this is to ensure everything is properly covered by tests, then bump the gem and fix the broken scenarios. If some method definitions don't exist, we can figure out other ways to do the same. For instance we might not cal make_regexp but we can write a new validator method or class to perform the validation. The proxy rule pattern validator might need special attention as well.

I checked the production DB and all urls we have stored in many columns in different tables, all of them are already RFC3986 valid urls. So switching the new RFC shouldn't break anything.

madnialihussain · 2026-07-27T14:40:37Z

Thanks @akostadinov and @jlledom for the feedback. That makes sense. I'll rework this PR

….1.1 upgrade

qltysh · 2026-07-30T12:44:57Z

❌ 12 blocking issues (12 total)

Tool	Category	Rule	Count
reek	Lint	Proxy::PortGenerator#call refers to 'uri' more than self (maybe move it to another class?)	8	❌
rubocop	Style	Prefer string interpolation to string concatenation.	2	❌
reek	Lint	UriPatterns has 7 constants	1	❌
rubocop	Style	Freeze mutable objects assigned to constants.	1	❌

…ndAttributeAccessor, SpaceInsideArrayLiteralBrackets

akostadinov-bot

Overall the PR looks sound and properly addresses the review feedback — all RFC2396 references eliminated, validation moved from make_regexp to parse-based UriValidator, clean URI.decode → CGI.unescapeURIComponent migration, and app-owned regex constants for path/query patterns. A few minor questions inline.

akostadinov-bot · 2026-07-31T12:20:28Z

-  (provider.web_hook || provider.build_web_hook).update!(attrs)
+  hook = provider.web_hook || provider.build_web_hook
+  hook.assign_attributes(attrs)
+  hook.save!(validate: false)


Could the fixture URL be fixed to pass validation instead of skipping it? save!(validate: false) removes incidental validation coverage from these Cucumber scenarios — if a future change breaks webhook URL validation, these tests won't catch it. Not blocking, just wondering if there's a reason the fixture URLs don't pass the new uri: validator.

I agree with the bot. Instead of modifying this, which will affect other tests, just fix the test. In the same cucumber scenario, instead of:

Hostname not supplied: 'http:://banana'

We'll see:

Must be a valid URL such as http://example.com

Fixed in 4cb28f4

akostadinov-bot · 2026-07-31T12:20:28Z


  errors:
    messages:
+      invalid_url: "Invalid URL format"


Now that all validators use :invalid_url, are the old :invalid keys for the same attributes still reachable? If not, could they be removed to avoid the duplication (5 attributes have both invalid: and invalid_url: with identical messages)? If some code path still hits :invalid, keeping both is fine — just worth confirming.

Removed in 794a67c

akostadinov-bot · 2026-07-31T12:20:28Z

+    url = params.permit(:referrer)[:referrer]
    if url
-      URI.decode(url)
+      CGI.unescapeURIComponent(url)


Minor: CGI.unescapeURIComponent raises ArgumentError on malformed percent-encoding (e.g. %ZZ), whereas the old URI.decode silently passed them through. Since referrer URLs originate from the app this is very unlikely, but is it worth a rescue here to fall back to the raw URL on ArgumentError?

The referrer URL is generated by our app and passed through the OAuth redirect flow. Malformed percent encoding would only occur if the user tampered with their own callback URL, which isn't a scenario we need to guard against

akostadinov-bot · 2026-07-31T12:20:28Z

-    ops.validates :site
+  with_options uri: { path: true, query: true, fragment: true }, allow_blank: true do |ops|
+    # Skip uri: for whitespace URLs — keycloak/auth0 whitespace validator handles these
+    ops.validates :site, unless: -> { site.to_s.match?(/\s/) }


The UX improvement is nice (one error instead of two for whitespace URLs). Was stripping whitespace with .strip before validation considered as an alternative? That way " http://example.com " would pass validation and be stored clean, rather than relying on the user to fix the whitespace themselves.

akostadinov-bot · 2026-07-31T12:20:37Z

-  (provider.web_hook || provider.build_web_hook).update!(attrs)
+  hook = provider.web_hook || provider.build_web_hook
+  hook.assign_attributes(attrs)
+  hook.save!(validate: false)


Could the fixture URL be fixed to pass validation instead of skipping it? save!(validate: false) removes incidental validation coverage from these Cucumber scenarios — if a future change breaks webhook URL validation, these tests won't catch it. Not blocking, just wondering if there's a reason the fixture URLs don't pass the new uri: validator.

akostadinov-bot · 2026-07-31T12:20:38Z


  errors:
    messages:
+      invalid_url: "Invalid URL format"


Now that all validators use :invalid_url, are the old :invalid keys for the same attributes still reachable? If not, could they be removed to avoid the duplication (5 attributes have both invalid: and invalid_url: with identical messages). If some code path still hits :invalid, keeping both is fine — just worth confirming.

akostadinov-bot · 2026-07-31T12:20:41Z

+    url = params.permit(:referrer)[:referrer]
    if url
-      URI.decode(url)
+      CGI.unescapeURIComponent(url)


Minor: CGI.unescapeURIComponent raises ArgumentError on malformed percent-encoding (e.g. %ZZ), whereas the old URI.decode silently passed them through. Since referrer URLs originate from the app this is very unlikely, but is it worth a rescue here to fall back to the raw URL on ArgumentError?

akostadinov-bot · 2026-07-31T12:20:42Z

-    ops.validates :site
+  with_options uri: { path: true, query: true, fragment: true }, allow_blank: true do |ops|
+    # Skip uri: for whitespace URLs — keycloak/auth0 whitespace validator handles these
+    ops.validates :site, unless: -> { site.to_s.match?(/\s/) }


The UX improvement is nice (one error instead of two for whitespace URLs). Was stripping whitespace with .strip before validation considered as an alternative? That way " http://example.com " would pass validation and be stored clean, rather than relying on the user to fix the whitespace themselves.

akostadinov · 2026-07-31T12:21:33Z

+  query = "(?:[\\-_.!~*'()a-zA-Z\\d;/?:@&=+$,\\[\\]]|%[a-fA-F\\d]{2})*"
+  optional_query = "(?:\\?(#{query}))?"
+  URI_PATH_PART = Regexp.new('\A' + abs_path + optional_query + '\z')
+  HOST = /\A(?:[a-zA-Z0-9\-._]|%\h\h)+(?::\d+)?\z/


Could you explain how did you come up with these and how do we know these are correct? If this is a copy of what we previously used, that's also fine but we need to be sure that what we don't us some AI generated potentially buggy stuff.

I see you said you verified it's the same as in old parser.

Yes, its copy of original, and i verified them all in rails console, only difference is HOST, just added _ a for RFC3986 compliance

But I don't agree that we should start allowing underscores in hostnames. I think we should keep rejecting these. Is there a reason to allow them. If there is, I'm open to change my mind. Here and in the other places.

akostadinov · 2026-07-31T12:21:54Z

-    param = /
+    UNRESERVED = "\\-_.!~*'()a-zA-Z\\d"
+    ESCAPED    = "%[a-fA-F\\d]{2}"
+    RESERVED   = ";/?:@&=+$,\\[\\]"


~~same here~~
I assume you verified these as well.

p URI::RFC2396_REGEXP::PATTERN::UNRESERVED etc

akostadinov · 2026-07-31T19:21:12Z

-      #$& contains the whole match of the regural expression
-      url = $&.sub(/\.$/, '').sub(/\:$/,'')
+    text.scan(%r{https?://[^\s)\]>]+}) do
+      url = Regexp.last_match(0).sub(/[.:]+\z/, '')


this seems to be a behavior change. Previously only .: at the end was removed, now any sequence of . and : is removed. I suggest to leave this change for a separate PR. This one is already too tricky.

Reverted in commit 4d98455, I also reworked the regex to accept ), ], > as valid URL characters per RFC3986.

jlledom · 2026-08-04T12:32:34Z

-    text.scan(URI.regexp(%w(http https))) do
-      #$& contains the whole match of the regural expression
-      url = $&.sub(/\.$/, '').sub(/\:$/,'')
+    text.scan(%r{https?://[^\s)\]>]+}) do


Ideally we should implement an RFC3986 linkifyer that actually matches valid RFC3986 urls, the uri gem provides some constants with regexps we could use. I've been trying a bit and doesn't seem to be an easy task. I'd say you should try. If you think it's not worth it. then better use %r{https?://[^\s]+} as a pattern, since ), ] and > are valid characters according to RFC 3896 AFAIK.

I tried %r{https?://[^\s]+} but it captures surrounding punctuation e.g. in "Check this (http://example.com) for details", the closing ) becomes part of the URL.

Implemented a balanced-bracket regex instead: It matches broadly but handles () and [] as balanced groups. All existing tests pass. 4b7f655

OK, it won't be perfect but it should work for most cases

jlledom · 2026-08-04T12:55:29Z

+  pchar = "(?:[\\-_.!~*'()a-zA-Z\\d:@&=+$,]|%[a-fA-F\\d]{2})"
+  segment = "#{pchar}*(?:;#{pchar}*)*"
+  abs_path = "/#{segment}(?:/#{segment})*"
+  query = "(?:[\\-_.!~*'()a-zA-Z\\d;/?:@&=+$,\\[\\]]|%[a-fA-F\\d]{2})*"
+  optional_query = "(?:\\?(#{query}))?"
+  URI_PATH_PART = Regexp.new('\A' + abs_path + optional_query + '\z')
+  HOST = /\A(?:[a-zA-Z0-9\-._]|%\h\h)+(?::\d+)?\z/


About this, can't we use the constants from URI::RFC3986_PARSER.regexp?

I don't think it makes sense to eliminate all references to RFC2396 but then hardcode a copy of its patterns. In fact, we are actually using RFC2396 anyway, just in a more complicated way.

If these values are exactly the same they were in RFC2396, then I think it's better to just mention RFC2396 and take them from the gem. If they are now different and meet RFC3986, then better create a helper module with all this regexps and use it from here or the validator.

jlledom · 2026-08-05T07:56:12Z

-  (provider.web_hook || provider.build_web_hook).update!(attrs)
+  hook = provider.web_hook || provider.build_web_hook
+  hook.assign_attributes(attrs)
+  hook.save!(validate: false)


I agree with the bot. Instead of modifying this, which will affect other tests, just fix the test. In the same cucumber scenario, instead of:

Hostname not supplied: 'http:://banana'

We'll see:

Must be a valid URL such as http://example.com

jlledom · 2026-08-05T09:01:39Z

Not sure this is the proper place, but we should somewhere add tests for URLs including the characters that are treated differently in RFC 3986: ! ' ( ) *

Both RFC2396 and RFC3986 accept ! ' ( ) * in URLs, the classification is changed (unreserved → sub-delimiters) but validity didn't change.

However... even if validity doesn't change, we don't have a test to assert this validity

added in 2ed9e4c

jlledom · 2026-08-17T13:03:08Z

-    text.scan(URI.regexp(%w(http https))) do
-      #$& contains the whole match of the regural expression
-      url = $&.sub(/\.$/, '').sub(/\:$/,'')
+    text.scan(%r{https?://[^\s)\]>]+}) do


OK, it won't be perfect but it should work for most cases

…iders

qltysh · 2026-08-18T12:02:04Z

+# constants. RFC3986 also defines different character sets (e.g. UNRESERVED
+# excludes !*'(), QUERY rejects []) which would change validation behavior.
+# These RFC2396 patterns are used intentionally to preserve compatibility.
+module UriPatterns


UriPatterns has 7 constants [reek:TooManyConstants]

qltysh · 2026-08-18T12:02:04Z

  validates :error_status_no_match, :error_status_auth_missing, :error_status_auth_failed, :error_status_limits_exceeded, presence: true

-  uri_pattern = URI::DEFAULT_PARSER.pattern
+  OPTIONAL_QUERY_FORMAT = "(?:\\?(#{UriPatterns::QUERY}))?"


Freeze mutable objects assigned to constants. [rubocop:Style/MutableConstant]

qltysh · 2026-08-18T12:02:04Z


-  uri_pattern = URI::DEFAULT_PARSER.pattern
+  OPTIONAL_QUERY_FORMAT = "(?:\\?(#{UriPatterns::QUERY}))?"
+  URI_PATH_PART = Regexp.new('\A' + UriPatterns::ABS_PATH + OPTIONAL_QUERY_FORMAT + '\z')


Prefer string interpolation to string concatenation. [rubocop:Style/StringConcatenation]

qltysh · 2026-08-18T12:02:04Z

-  uri_pattern = URI::DEFAULT_PARSER.pattern
+  OPTIONAL_QUERY_FORMAT = "(?:\\?(#{UriPatterns::QUERY}))?"
+  URI_PATH_PART = Regexp.new('\A' + UriPatterns::ABS_PATH + OPTIONAL_QUERY_FORMAT + '\z')
+  HOST = Regexp.new('\A' + UriPatterns::HOSTNAME + '(:\d+)?' + '\z')


Prefer string interpolation to string concatenation. [rubocop:Style/StringConcatenation]

qltysh · 2026-08-18T12:02:04Z

      begin
        uri = URI.parse(attribute_value)
-        value = URI::Generic.new(uri.scheme, uri.userinfo, uri.host, uri.port, uri.registry, uri.path, uri.opaque, uri.query, uri.fragment).to_s
+        value = URI::Generic.new(uri.scheme, uri.userinfo, uri.host, uri.port, nil, uri.path, uri.opaque, uri.query, uri.fragment).to_s


Found 8 issues:

1. Proxy::PortGenerator#call refers to 'uri' more than self (maybe move it to another class?) [reek:FeatureEnvy]

2. Proxy::PortGenerator#call refers to 'uri' more than self (maybe move it to another class?) [reek:FeatureEnvy]

3. Proxy::PortGenerator#call refers to 'uri' more than self (maybe move it to another class?) [reek:FeatureEnvy]

4. Proxy::PortGenerator#call refers to 'uri' more than self (maybe move it to another class?) [reek:FeatureEnvy]

5. Proxy::PortGenerator#call refers to 'uri' more than self (maybe move it to another class?) [reek:FeatureEnvy]

6. Proxy::PortGenerator#call refers to 'uri' more than self (maybe move it to another class?) [reek:FeatureEnvy]

7. Proxy::PortGenerator#call refers to 'uri' more than self (maybe move it to another class?) [reek:FeatureEnvy]

8. Proxy::PortGenerator#call refers to 'uri' more than self (maybe move it to another class?) [reek:FeatureEnvy]

jlledom · 2026-08-19T08:52:23Z

+# constants. RFC3986 also defines different character sets (e.g. UNRESERVED
+# excludes !*'(), QUERY rejects []) which would change validation behavior.


Then, shouldn't we define UNRESERVED and QUERY as RFC3986 defines them? that's the point of the PR: migrate to RFC3986. Are there breaking changes?

The only breaking change is QUERY rejecting [], ?ids[]=1 would fail api_test_path validation. But looking at the pr again that it was already confirmed production URLs are all RFC3986 valid, that should be safe.

But the blocker is that the gem doesn't provide UNRESERVED or QUERY as composable strings for RFC3986 — RFC3986_PARSER only exposes compiled anchored Regexps (.regexp), not string patterns (.pattern only exists on RFC2396). I explored alternatives with the help of claude (Addressable::URI::CharacterClasses, URI::RFC3986_Parser::SEG, .source extraction) but none are clean.

I have updated the comment

jlledom · 2026-08-19T09:07:27Z

 class AuthenticationProvider::GitHub < AuthenticationProvider
  self.authorization_scope = :branding

+  validates :site, format: { without: /\s/, message: :contains_whitespace }, allow_blank: true


I'm not sure we need allow_blank here. We are already setting it in authentication_provider.rb, in the call to with_options. Right?

I assume, if the Keycloack and Auth0 subclasses validate presence on :site is precisely because not doing it would allow blank values.

remove in f720960

Upgrade uri gem from 0.13.3 to 1.1.1 and fix all incompatible call sites

09fa2ae

akostadinov requested a review from jlledom July 24, 2026 16:45

jlledom reviewed Jul 27, 2026

View reviewed changes

Eliminate all RFC2396 references and migrate to RFC3986 for uri gem 1…

361e3b9

….1.1 upgrade

qltysh Bot reviewed Jul 30, 2026

View reviewed changes

Comment thread app/helpers/messages_helper.rb Outdated

Comment thread app/models/proxy.rb

Comment thread app/models/proxy_rule.rb

Comment thread app/models/proxy_rule.rb

Comment thread app/validators/uri_validator.rb

Comment thread test/unit/proxy_test.rb Outdated

madnialihussain requested review from akostadinov and jlledom July 30, 2026 12:45

madnialihussain added 2 commits July 30, 2026 13:58

Fix qlty/rubocop issues: PerlBackrefs, ConstantRegexp, EmptyLinesArou…

46b5aff

…ndAttributeAccessor, SpaceInsideArrayLiteralBrackets

Fix validation errors after replacing make_regexp with uri: validator

47252e4

akostadinov-bot Bot reviewed Jul 31, 2026

View reviewed changes

akostadinov reviewed Jul 31, 2026

View reviewed changes

jlledom reviewed Aug 5, 2026

View reviewed changes

madnialihussain added 5 commits August 12, 2026 23:42

fix webhook scenario to test validation error for invalid URL

4cb28f4

remove underscore from HOST regex

8641c06

revert sub chain consolidation in messages_helper

4d98455

remove double-escaping test from messages_helper_test

9f32039

add test for + in path for redhat auth callback

b216d16

madnialihussain added 3 commits August 13, 2026 00:32

add service_discovery auth callback referrer URL tests

5b5bc85

remove dead i18n :invalid entries for uri-validated fields

794a67c

revert inlined patterns to URI::RFC2396 references

493a7cf

qltysh Bot reviewed Aug 14, 2026

View reviewed changes

Comment thread app/helpers/messages_helper.rb Outdated

madnialihussain added 2 commits August 14, 2026 09:31

fix rubocop RedundantRegexpEscape for colon in messages_helper

3dd8111

replace URI.regexp with balanced-bracket regex in linkifier

4b7f655

jlledom reviewed Aug 17, 2026

View reviewed changes

jlledom reviewed Aug 18, 2026

View reviewed changes

Comment thread app/models/authentication_provider.rb

madnialihussain added 2 commits August 18, 2026 11:41

extract UriPatterns module for RFC2396 pattern constants

6400cf4

add whitespace validation for site URL on Custom and Github auth prov…

99b10e6

…iders

qltysh Bot reviewed Aug 18, 2026

View reviewed changes

madnialihussain requested review from akostadinov and jlledom August 18, 2026 12:27

jlledom reviewed Aug 19, 2026

View reviewed changes

madnialihussain added 3 commits August 19, 2026 15:10

fix misleading comment in UriPatterns module

e47b127

Remove unnecessary allow_blank from whitespace validators

f720960

Add test for sub-delimiter characters in URI validation

2ed9e4c

madnialihussain requested a review from jlledom August 19, 2026 15:01

		# constants. RFC3986 also defines different character sets (e.g. UNRESERVED
		# excludes !*'(), QUERY rejects []) which would change validation behavior.

Uh oh!

Conversation

madnialihussain commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it

Key Changes & Migrations

Bug Fixes & Refactoring

Behavioral Changes

Which issue(s) this PR fixes

Verification Steps

Uh oh!

akostadinov commented Jul 24, 2026

Uh oh!

madnialihussain commented Jul 24, 2026

Uh oh!

akostadinov commented Jul 24, 2026

Uh oh!

akostadinov-bot Bot commented Jul 24, 2026

Review: URI gem upgrade needs proper migration, not just compatibility patches

Core Issues

Recommended Approach

File-by-File Migration Plan

Why This Matters

Reproduction

Uh oh!

akostadinov commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jlledom left a comment

Choose a reason for hiding this comment

Uh oh!

madnialihussain commented Jul 27, 2026

Uh oh!

qltysh Bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akostadinov-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

akostadinov-bot Bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

jlledom Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akostadinov-bot Bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akostadinov-bot Bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akostadinov-bot Bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

akostadinov-bot Bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

akostadinov-bot Bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

akostadinov-bot Bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

akostadinov-bot Bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

akostadinov Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

madnialihussain commented Jul 24, 2026 •

edited

Loading

akostadinov commented Jul 24, 2026 •

edited

Loading

qltysh Bot commented Jul 30, 2026 •

edited

Loading

jlledom Aug 5, 2026 •

edited

Loading

akostadinov Jul 31, 2026 •

edited

Loading

akostadinov Jul 31, 2026 •

edited

Loading

jlledom Aug 17, 2026 •

edited

Loading

jlledom Aug 5, 2026 •

edited

Loading

jlledom Aug 17, 2026 •

edited

Loading