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 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)