Repository navigation
fix(abstractions): normalize allowed hosts, reject userinfo, and validate URL structure - #824
Conversation
…date URL structure
|
Hi Vincent Biret (@baywet), opened this PR to normalize allowed hosts, reject URLs with userinfo, and validate URL structure. Unit tests and SonarCloud analysis are all passing. Whenever you have time, please review. Thank you! |
| raise ValueError("Allowed host value cannot contain 'https://' or 'http://' prefix") | ||
|
|
||
| self.allowed_hosts: set[str] = {x.lower() for x in allowed_hosts} | ||
| self.allowed_hosts: set[str] = set() |
There was a problem hiding this comment.
why are we initializing the backing field here since we're already doing this in the setter?
There was a problem hiding this comment.
Good catch. Removed the redundant self.allowed_hosts = set() backing-field initialization from init, relying directly on self.set_allowed_hosts(allowed_hosts) to canonicalize and initialize self.allowed_hosts. Verified in commit 12a6da5.
| if cleaned_host: | ||
| normalized_hosts.add(lower_host) |
There was a problem hiding this comment.
Addressed. The validator now raises ValueError for blank or whitespace-only normalized entries ([""], [" "], and mixed lists with blank entries) rather than silently discarding them. Only an explicitly empty list [] enables unrestricted host fallback semantics. Added regression tests test_raises_on_empty_string_entry, test_raises_on_whitespace_only_entry, and test_raises_on_mixed_valid_and_blank_entry. Verified in commit 12a6da5.
| # query='query', fragment='fragment') | ||
| o = urlparse(url) | ||
| if not all([o.scheme, o.netloc]): | ||
| o = urlparse(cleaned_url) |
There was a problem hiding this comment.
Addressed. Wrapped urlparse(cleaned_url) in a try...except ValueError block returning False to prevent malformed URL exceptions from escaping out of the validator. Added regression test test_returns_false_for_unmatched_ipv6_bracket demonstrating safe rejection for both configured and empty allowlists. Verified in commit 12a6da5.
| o = urlparse(url) | ||
| if not all([o.scheme, o.netloc]): | ||
| o = urlparse(cleaned_url) | ||
| if not (o.scheme and o.netloc and o.hostname) or o.username or o.password: |
There was a problem hiding this comment.
Addressed. Updated userinfo validation to check o.username is not None or o.password is not None, ensuring empty userinfo strings (https://@example.com/path and https://:@example.com/path) are rejected. Added regression tests test_returns_false_for_explicitly_empty_userinfo and test_returns_false_for_userinfo_even_when_allowed_hosts_empty. Verified in commit 12a6da5.
| if not (o.scheme and o.netloc and o.hostname) or o.username or o.password: | ||
| return False |
There was a problem hiding this comment.
Addressed. Updated test_get_authorization_token_invalid_url and test_derived_scope_rejects_url_without_hostname to assert that validator-rejected URLs return an empty token ("") without calling the credential. Split userinfo and port testing into test_userinfo_url_is_rejected_without_requesting_token (asserting token == "" and received_scopes == []) and test_derived_scope_strips_port (asserting proper scope derivation on legitimate URLs with port). All 23 Azure provider tests pass. Verified in commit 12a6da5.
…azure provider tests
|
|
Thanks for the review. I've pushed commit Changes
Validation
I've also updated the inline review threads with individual replies. When you have a chance, could you please take another look? Thank you for your time and feedback! |
Description
This PR strengthens host validation and normalization in
AllowedHostsValidator:Host normalization & prefix validation:
AllowedHostsValidator.__init__now delegates toset_allowed_hoststo maintain a single normalization path.host.strip()) before storing and checking prefixes.http://andhttps://prefixes (e.g., catchingHTTP://...orhttp://...), raisingValueErrorconsistently.isinstance(host, str)), raisingTypeErrorif a non-string entry is provided.URL validation & security hardening:
o.username or o.password) outright to prevent host spoofing and credential leakage (aligning with the TypeScript implementation).scheme,netloc, andhostname) before consideringallowed_hostsempty fallback, ensuring malformed URLs are rejected even when allowed hosts are not configured.Test coverage:
cc Vincent Biret (@baywet)