Skip to content

Commit

Permalink
New Root URL: Don't probe URLs entered with full https:// or http:// …
Browse files Browse the repository at this point in the history 
…scheme
  • Loading branch information
@davidfstr
davidfstr committed Jan 13, 2024
2 parents b756828 + 51a4033 commit bee97eb
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 99 deletions.
96 changes: 23 additions & 73 deletions src/crystal/tests/test_addrooturl.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ def test_given_empty_string_then_returns_empty_string() -> None:


@with_subtests
def test_given_any_valid_url_then_returns_that_url_as_first_candidate(subtests: SubtestsContext) -> None:
def test_given_any_valid_url_then_returns_that_url_as_only_candidate(subtests: SubtestsContext) -> None:
VALID_URLS = [
'https://xkcd.com/',
'https://xkcd.com/1/',
Expand All @@ -230,11 +230,11 @@ def test_given_any_valid_url_then_returns_that_url_as_first_candidate(subtests:
with _EXPAND_enabled():
for url in VALID_URLS:
with subtests.test(url=url):
assertEqual(url, EXPAND(url)[0])
assertEqual([url], EXPAND(url))


@with_subtests
def test_given_a_url_that_would_be_valid_with_appended_slash_then_returns_that_modified_url_as_first_candidate(subtests: SubtestsContext) -> None:
def test_given_a_url_that_would_be_valid_with_appended_slash_then_returns_that_modified_url_as_only_candidate(subtests: SubtestsContext) -> None:
VALID_WITH_SLASH_URLS = [
'https://xkcd.com',
'https://www.apple.com',
Expand All @@ -243,14 +243,14 @@ def test_given_a_url_that_would_be_valid_with_appended_slash_then_returns_that_m
with _EXPAND_enabled():
for url in VALID_WITH_SLASH_URLS:
with subtests.test(url=url):
assertEqual(url + '/', EXPAND(url)[0])
assertEqual([url + '/'], EXPAND(url))


@with_subtests
def test_given_url_without_www_prefix_then_returns_that_url_plus_that_url_with_www_as_candidates(subtests: SubtestsContext) -> None:
def test_given_schemaless_url_without_www_prefix_then_returns_that_url_plus_that_url_with_www_as_candidates(subtests: SubtestsContext) -> None:
CASES = [
('https://xkcd.com/', ['https://xkcd.com/', 'https://www.xkcd.com/']),
('https://apple.com/', ['https://apple.com/', 'https://www.apple.com/']),
('xkcd.com/', ['https://xkcd.com/', 'https://www.xkcd.com/']),
('apple.com/', ['https://apple.com/', 'https://www.apple.com/']),
]
with _EXPAND_enabled():
for (input, output) in CASES:
Expand All @@ -259,10 +259,10 @@ def test_given_url_without_www_prefix_then_returns_that_url_plus_that_url_with_w


@with_subtests
def test_given_url_with_www_prefix_then_returns_that_url_plus_that_url_without_www_as_candidates(subtests: SubtestsContext) -> None:
def test_given_schemaless_url_with_www_prefix_then_returns_that_url_plus_that_url_without_www_as_candidates(subtests: SubtestsContext) -> None:
CASES = [
('https://www.xkcd.com/', ['https://www.xkcd.com/', 'https://xkcd.com/']),
('https://www.apple.com/', ['https://www.apple.com/', 'https://apple.com/']),
('www.xkcd.com/', ['https://www.xkcd.com/', 'https://xkcd.com/']),
('www.apple.com/', ['https://www.apple.com/', 'https://apple.com/']),
]
with _EXPAND_enabled():
for (input, output) in CASES:
Expand Down Expand Up @@ -320,10 +320,8 @@ async def test_given_url_input_is_empty_and_focused_when_tab_pressed_then_url_in
@awith_subtests
async def test_given_url_input_is_nonempty_and_focused_when_tab_pressed_then_url_input_unfocused_and_spinner_appears(subtests: SubtestsContext) -> None:
URLS = [
'https://xkcd.com/',
'https://www.apple.com/',
'http://xkcd.com/',
'http://www.apple.com/',
'xkcd.com',
'www.apple.com',
]
async with _add_url_dialog_open() as aud:
with _urlopen_responding_with(_UrlOpenHttpResponse(code=404, url=ANY)):
Expand All @@ -346,15 +344,13 @@ async def test_given_url_input_is_nonempty_and_focused_when_tab_pressed_then_url

@awith_subtests
async def test_given_url_input_is_nonempty_and_did_press_tab_and_spinner_is_visible_when_url_responds_with_http_200_then_spinner_disappears(subtests: SubtestsContext) -> None:
URLS = [
'https://xkcd.com/',
'https://www.apple.com/',
'http://xkcd.com/',
'http://www.apple.com/',
CASES = [
('xkcd.com', 'https://xkcd.com/'),
('www.apple.com', 'https://www.apple.com/'),
]
async with _add_url_dialog_open() as aud:
last_focused = None # type: Optional[wx.Window]
for url in URLS:
for (url, normalized_url) in CASES:
with subtests.test(url=url):
# NOTE: Fail if requests any URL beyond the original one
with _urlopen_responding_with({url: _UrlOpenHttpResponse(code=200, url=url)}):
Expand All @@ -365,13 +361,12 @@ async def test_given_url_input_is_nonempty_and_did_press_tab_and_spinner_is_visi
assertEqual(False, aud.url_field_focused)
assertEqual(True, aud.url_cleaner_spinner.IsShown())
await wait_for(lambda: (False == aud.url_cleaner_spinner.IsShown()) or None)
assertEqual(url, aud.url_field.Value)
assertEqual(normalized_url, aud.url_field.Value)


@awith_subtests
async def test_given_url_input_is_nonempty_without_www_and_did_press_tab_and_spinner_is_visible_when_url_responds_with_http_3xx_to_url_with_www_and_url_with_www_url_responds_with_http_200_then_url_input_replaced_with_url_with_www_and_spinner_disappears(subtests: SubtestsContext) -> None:
CASES = [
('https://apple.com/', 'https://apple.com/', 'https://www.apple.com/'),
('apple.com/mac/', 'https://apple.com/mac/', 'https://www.apple.com/mac/'),
('apple.com/', 'https://apple.com/', 'https://www.apple.com/'),
('apple.com', 'https://apple.com/', 'https://www.apple.com/'),
Expand All @@ -395,7 +390,6 @@ async def test_given_url_input_is_nonempty_without_www_and_did_press_tab_and_spi
@awith_subtests
async def test_given_url_input_is_nonempty_with_www_and_did_press_tab_and_spinner_is_visible_when_url_responds_with_http_3xx_to_url_without_www_and_url_without_www_responds_with_http_200_then_url_input_replaced_with_url_without_www_and_spinner_disappears(subtests: SubtestsContext) -> None:
CASES = [
('https://www.xkcd.com/', 'https://www.xkcd.com/', 'https://xkcd.com/'),
('www.xkcd.com/1/', 'https://www.xkcd.com/1/', 'https://xkcd.com/1/'),
('www.xkcd.com/', 'https://www.xkcd.com/', 'https://xkcd.com/'),
('www.xkcd.com', 'https://www.xkcd.com/', 'https://xkcd.com/'),
Expand All @@ -416,58 +410,14 @@ async def test_given_url_input_is_nonempty_with_www_and_did_press_tab_and_spinne
assertEqual(without_www_url, aud.url_field.Value)


@awith_subtests
async def test_given_url_input_is_nonempty_with_http_and_did_press_tab_and_spinner_is_visible_when_url_responds_with_http_200_then_spinner_disappears(subtests: SubtestsContext) -> None:
URLS = [
'http://captive.apple.com/',
'http://http.badssl.com/',
]
async with _add_url_dialog_open() as aud:
last_focused = None # type: Optional[wx.Window]
for url in URLS:
with subtests.test(url=url):
# NOTE: Fail if requests any URL beyond the original one
with _urlopen_responding_with({url: _UrlOpenHttpResponse(code=200, url=url)}):
last_focused = SetFocus(aud.url_field, last_focused)
aud.url_field.Value = url

last_focused = SetFocus(aud.name_field, last_focused) # simulate press tab
assertEqual(False, aud.url_field_focused)
assertEqual(True, aud.url_cleaner_spinner.IsShown())
await wait_for(lambda: (False == aud.url_cleaner_spinner.IsShown()) or None)
assertEqual(url, aud.url_field.Value)


@awith_subtests
async def test_given_url_input_is_nonempty_with_http_and_did_press_tab_and_spinner_is_visible_when_url_responds_with_http_3xx_to_https_url_then_url_input_replaced_with_https_url_and_spinner_disappears(subtests: SubtestsContext) -> None:
CASES = [
('http://www.themanime.org/', 'https://www.themanime.org/'),
]
async with _add_url_dialog_open() as aud:
last_focused = None # type: Optional[wx.Window]
for (http_url, https_url) in CASES:
with subtests.test(url_input=http_url):
with _urlopen_responding_with({
http_url: _UrlOpenHttpResponse(code=200, url=https_url)}):
last_focused = SetFocus(aud.url_field, last_focused)
aud.url_field.Value = http_url

last_focused = SetFocus(aud.name_field, last_focused) # simulate press tab
assertEqual(False, aud.url_field_focused)
assertEqual(True, aud.url_cleaner_spinner.IsShown())
await wait_for(lambda: (False == aud.url_cleaner_spinner.IsShown()) or None)
assertEqual(https_url, aud.url_field.Value)


@awith_subtests
async def test_given_url_input_is_nonempty_and_did_press_tab_and_spinner_is_visible_when_url_responds_with_http_3xx_to_unrelated_url_then_spinner_disappears(subtests: SubtestsContext) -> None:
CASES = [
('http://contoso.com/', 'https://www.microsoft.com/'),
('https://contoso.com/', 'https://www.microsoft.com/'),
('contoso.com/', 'https://contoso.com/', 'https://www.microsoft.com/'),
]
async with _add_url_dialog_open() as aud:
last_focused = None # type: Optional[wx.Window]
for (start_url, target_url) in CASES:
for (start_url, normalized_start_url, target_url) in CASES:
with subtests.test(url_input=start_url):
with _urlopen_responding_with({
start_url: _UrlOpenHttpResponse(code=200, url=target_url)}):
Expand All @@ -478,7 +428,7 @@ async def test_given_url_input_is_nonempty_and_did_press_tab_and_spinner_is_visi
assertEqual(False, aud.url_field_focused)
assertEqual(True, aud.url_cleaner_spinner.IsShown())
await wait_for(lambda: (False == aud.url_cleaner_spinner.IsShown()) or None)
assertEqual(start_url, aud.url_field.Value)
assertEqual(normalized_start_url, aud.url_field.Value)


# === Test: Concurrent Actions While Resolving URL & Allow Create Root URL ===
Expand Down Expand Up @@ -719,7 +669,7 @@ async def test_given_url_input_matches_existing_root_url_when_press_ok_then_disp
last_focused = None # type: Optional[wx.Window]

last_focused = SetFocus(aud.url_field, last_focused)
aud.url_field.Value = 'https://xkcd.com/'
aud.url_field.Value = 'xkcd.com/'

with patch('crystal.browser.addrooturl.ShowModal', return_value=wx.ID_OK) as show_modal_method:
click_button(aud.ok_button)
Expand All @@ -741,7 +691,7 @@ async def test_given_url_input_matches_existing_root_url_when_press_ok_then_disp
last_focused = None

last_focused = SetFocus(aud.url_field, last_focused)
aud.url_field.Value = 'https://xkcd.com/'
aud.url_field.Value = 'xkcd.com/'

last_focused = SetFocus(aud.name_field, last_focused) # simulate press tab
assertEqual(False, aud.url_field_focused)
Expand All @@ -767,7 +717,7 @@ async def test_given_url_input_matches_existing_root_url_when_press_ok_then_disp
last_focused = None

last_focused = SetFocus(aud.url_field, last_focused)
aud.url_field.Value = 'https://xkcd.com/'
aud.url_field.Value = 'xkcd.com/'

last_focused = SetFocus(aud.name_field, last_focused) # simulate press tab
assertEqual(False, aud.url_field_focused)
Expand Down
57 changes: 31 additions & 26 deletions src/crystal/url_input.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,26 +80,17 @@ def _candidate_urls_from_user_input(url_input: str) -> List[str]:
In the current implementation:
* If no https:// or http:// prefix is given,
try first the former then the latter.
* If no www. domain prefix is given,
* If additionally no www. domain prefix is given,
try first without the prefix then with the prefix.
"""
# While running tests, unless CRYSTAL_URLOPEN_MOCKED=True,
# assume any URL input is already a valid URL without
# performing any (possibly real) network requests
if os.environ.get('CRYSTAL_RUNNING_TESTS', 'False') == 'True':
if os.environ.get('CRYSTAL_URLOPEN_MOCKED', 'False') == 'True':
# OK
pass
else:
# Assume valid, without performing any network requests
return [url_input]

if url_input.strip() == '':
return [url_input]

url_parts = urlparse(url_input)
if url_parts.scheme in ('', 'https', 'http'):
# If missing scheme, try https:// then http://
# If missing scheme:
# 1. try https:// then http://
# 2. try variations on www
if url_parts.scheme == '':
scheme_candidates = ('https', 'http') # type: Tuple[str, ...]

Expand All @@ -116,20 +107,17 @@ def _candidate_urls_from_user_input(url_input: str) -> List[str]:
netloc=new_netloc,
path=new_path
) # reinterpret
elif url_parts.scheme == 'https':
scheme_candidates = ('https', 'http')
elif url_parts.scheme == 'http':
scheme_candidates = ('http', 'https')
else:
raise AssertionError()

# 1. If has www, then try non-www domain if www domain fails
# 2. If missing www, then try it if non-www domain fails
if url_parts.netloc.startswith('www.'):
netloc_candidates = (url_parts.netloc, url_parts.netloc[len('www.'):]) # type: Tuple[str, ...]
elif url_parts.netloc != '' and not url_parts.netloc.startswith('www.'):
netloc_candidates = (url_parts.netloc, 'www.' + url_parts.netloc)

# 1. If has www, then try non-www domain if www domain fails
# 2. If missing www, then try it if non-www domain fails
if url_parts.netloc.startswith('www.'):
netloc_candidates = (url_parts.netloc, url_parts.netloc[len('www.'):]) # type: Tuple[str, ...]
elif url_parts.netloc != '' and not url_parts.netloc.startswith('www.'):
netloc_candidates = (url_parts.netloc, 'www.' + url_parts.netloc)
else:
netloc_candidates = (url_parts.netloc,)
else:
scheme_candidates = (url_parts.scheme,)
netloc_candidates = (url_parts.netloc,)

# If empty path then normalize to /
Expand Down Expand Up @@ -173,6 +161,23 @@ def _resolve_url_from_candidates(
if len(url_candidates) == 1:
return url_candidates[0]

# Disallow network requests while running tests,
# unless CRYSTAL_URLOPEN_MOCKED=True
if os.environ.get('CRYSTAL_RUNNING_TESTS', 'False') == 'True':
if os.environ.get('CRYSTAL_URLOPEN_MOCKED', 'False') == 'True':
# OK
pass
else:
raise AssertionError(
'Attempting to resolve URL candidates with real '
'network requests while automated tests are running. '

'Please either (1) enter URLs with http:// or https:// '
'schema that does not need to be resolved, or (2) '
'mock the urlopen() function with something like '
'_urlopen_responding_with().'
)

# Look for URL candidate which can be fetched successfully
for url_candidate in url_candidates:
if did_cancel_func():
Expand Down

0 comments on commit bee97eb

Please sign in to comment.