diff --git a/app/src/components/Bookmarks.js b/app/src/components/Bookmarks.js index fb6c2b3e..270d0aef 100644 --- a/app/src/components/Bookmarks.js +++ b/app/src/components/Bookmarks.js @@ -10,10 +10,14 @@ export const EDIT_BOOKMARKS_PATH = '/more/bookmarks'; /** * The props that make a Menu.Item open a bookmark: an in-app Link for a path, a plain - * anchor for everything else, and a new tab when the bookmark asks for one. + * anchor for everything else, and a new tab when the bookmark asks for one. A URL that + * does not resolve (see resolveBookmarkUrl) gets a disabled item, never a raw href. */ function bookmarkItemProps(node) { const {internal, href} = resolveBookmarkUrl(node.url); + if (!href) { + return {disabled: true}; + } const target = node.new_tab ? {target: '_blank', rel: 'noopener noreferrer'} : {}; if (internal) { return {component: Link, to: href, ...target}; @@ -203,11 +207,12 @@ function BookmarkTableRows({nodes, parentId, depth, editable, actions}) { : } - {directory - ? {node.name} - : + {directory && {node.name}} + {!directory && (href + ? {node.name} - } + + : {node.name})} {!directory && {node.url}} {editable && @@ -267,16 +272,26 @@ export function BookmarkAddForms({nodes, onAddBookmark, onAddDirectory}) { const locations = locationOptions(nodes); const parentOf = (value) => value === ROOT ? null : Number(value); + // The form is cleared only once the server has accepted the node. A refused add has + // already shown its toast; the typed URL stays so the user can correct it. const handleAddBookmark = async () => { if (!url.trim() || !name.trim()) return; - await onAddBookmark({name: name.trim(), url: url.trim(), new_tab: newTab, parent_id: parentOf(location)}); + try { + await onAddBookmark({name: name.trim(), url: url.trim(), new_tab: newTab, parent_id: parentOf(location)}); + } catch (e) { + return; + } setUrl(''); setName(''); }; const handleAddDirectory = async () => { if (!directoryName.trim()) return; - await onAddDirectory({name: directoryName.trim(), parent_id: parentOf(directoryLocation)}); + try { + await onAddDirectory({name: directoryName.trim(), parent_id: parentOf(directoryLocation)}); + } catch (e) { + return; + } setDirectoryName(''); }; @@ -375,32 +390,38 @@ export function BookmarksPage() { }, }; + // These reject on failure (the API client has already shown the toast) so the add + // forms know to keep what was typed. const handleAddBookmark = async (bookmark) => { - try { - await addBookmark(bookmark); - await refresh(); - } catch (e) { - } + await addBookmark(bookmark); + await refresh(); }; const handleAddDirectory = async (directory) => { - try { - await addBookmarkDirectory(directory); - await refresh(); - } catch (e) { - } + await addBookmarkDirectory(directory); + await refresh(); }; const handleEditSave = async ({name, url, new_tab, parent_id}) => { const {node, directory, parentId} = modal; try { await updateBookmark(node.id, directory ? {name} : {name, url, new_tab}); - if (parent_id !== parentId) { - await moveBookmark(node.id, parent_id); - } - await refresh(); - setModal(null); } catch (e) { + return; // Nothing changed; the modal stays open with the toast beside it. + } + // The rename is committed even if the move is refused, so the nav must show it + // either way; only a fully successful edit closes the modal. + let moved = true; + if (parent_id !== parentId) { + try { + await moveBookmark(node.id, parent_id); + } catch (e) { + moved = false; + } + } + await refresh(); + if (moved) { + setModal(null); } }; diff --git a/app/src/components/Bookmarks.test.js b/app/src/components/Bookmarks.test.js index 21ce867e..60b12b3a 100644 --- a/app/src/components/Bookmarks.test.js +++ b/app/src/components/Bookmarks.test.js @@ -16,7 +16,7 @@ const tree = [ ]; describe('resolveBookmarkUrl', () => { - const location = {protocol: 'https:', hostname: '10.0.0.2'}; + const location = {protocol: 'https:', hostname: '10.0.0.2', host: '10.0.0.2'}; it.each([ ['/videos', {internal: true, href: '/videos'}], @@ -30,6 +30,26 @@ describe('resolveBookmarkUrl', () => { ])('resolves %s', (url, expected) => { expect(resolveBookmarkUrl(url, location)).toEqual(expected); }); + + it.each([ + // Paths a browser takes off this host: network paths, backslashes, and control + // characters a URL parser strips before deciding. + '//evil.com', '///evil.com', '/\\evil.com', '/\\@evil.com', '/\t/evil.com', '/\n/evil.com', + // Port forms where the host would become credentials. + ':8096@evil.com', 'http://:8096@evil.com', ':8096\t@evil.com', ':8096/x@evil.com', ':8096\\evil.com', + // Not http(s). + 'javascript:alert(1)', 'data:text/html,hi', 'ftp://example.com', 'videos', '', + ])('refuses %j rather than guessing', (url) => { + expect(resolveBookmarkUrl(url, location)).toEqual({internal: false, href: null}); + }); + + it('brackets an IPv6 host for a port bookmark', () => { + // The browser reports an IPv6 hostname bracketed already. + const v6 = {protocol: 'https:', hostname: '[::1]', host: '[::1]'}; + expect(resolveBookmarkUrl(':8096/web', v6)).toEqual({internal: false, href: 'https://[::1]:8096/web'}); + expect(resolveBookmarkUrl(':8096', {protocol: 'http:', hostname: '::1', host: '[::1]'})) + .toEqual({internal: false, href: 'http://[::1]:8096'}); + }); }); describe('directoryOptions', () => { @@ -72,6 +92,13 @@ describe('BookmarkTable', () => { expect(a.move).toHaveBeenCalledWith(tree[1].children[1], 2, 0); }); + it('shows a bookmark whose URL does not resolve as text, not a link', () => { + const nodes = [{id: 1, name: 'Bad', url: '//evil.com', new_tab: false}]; + render(); + expect(screen.queryByRole('link', {name: 'Bad'})).not.toBeInTheDocument(); + expect(screen.getByText('Bad')).toBeInTheDocument(); + }); + it('edits with the parent the node lives in, and deletes a bookmark without asking', async () => { const a = actions(); render(); @@ -108,6 +135,18 @@ describe('BookmarkAddForms', () => { expect(screen.getByLabelText('Bookmark URL')).toHaveValue(''); }); + it('keeps what was typed when the server refuses the bookmark', async () => { + const onAddBookmark = jest.fn().mockRejectedValue(new Error('refused')); + render(); + await userEvent.type(screen.getByLabelText('Bookmark URL'), '//evil.com'); + await userEvent.type(screen.getByLabelText('Bookmark name'), 'Oops'); + await userEvent.click(screen.getAllByRole('button', {name: 'Add'})[0]); + + expect(onAddBookmark).toHaveBeenCalled(); + expect(screen.getByLabelText('Bookmark URL')).toHaveValue('//evil.com'); + expect(screen.getByLabelText('Bookmark name')).toHaveValue('Oops'); + }); + it('adds a directory', async () => { const onAddDirectory = jest.fn(); render(); diff --git a/app/src/contexts/BookmarksContext.js b/app/src/contexts/BookmarksContext.js index 9b1dfa63..e3d639e6 100644 --- a/app/src/contexts/BookmarksContext.js +++ b/app/src/contexts/BookmarksContext.js @@ -38,6 +38,15 @@ export function useBookmarks() { export const isBookmarkDirectory = (node) => Array.isArray(node.children); +// Characters a URL parser strips or that split a URL in surprising places. See the +// same rule in wrolpi/bookmarks.py; the two must agree. +// eslint-disable-next-line no-control-regex -- matching control characters is the point +const FORBIDDEN = /[\x00-\x20\x7f\\]/; +const PORT_FORM = /^(?:(https?):\/\/)?:(\d{1,5})(?=$|[/?#])(.*)$/i; +const PORT_FORM_START = /^(?:https?:\/\/)?:/i; + +const NONE = {internal: false, href: null}; + /** * Where a bookmark URL really points, from the browser's point of view. * @@ -50,16 +59,56 @@ export const isBookmarkDirectory = (node) => Array.isArray(node.children); * The host-relative forms exist because a WROLPi is reached by LAN IP, by mDNS name, and * by its hotspot address, and a service running beside it is reached the same way. A * bookmark with the hostname written in would work from only one of them. + * + * Anything else, including a stored value the server would refuse today, resolves to a + * null href and is rendered as text rather than a link: the form each value claims is + * checked by parsing it, never by its prefix, because `//evil.com` starts like a path and + * `:8096@evil.com` starts like a port, and a browser takes both off this host. */ export function resolveBookmarkUrl(url, location = window.location) { const value = (url || '').trim(); + if (!value || FORBIDDEN.test(value)) { + return NONE; + } + const origin = `${location.protocol}//${location.host}`; + + if (PORT_FORM_START.test(value)) { + const match = value.match(PORT_FORM); + if (!match || match[3].includes('@')) { + return NONE; + } + const [, scheme, port, tail] = match; + // An IPv6 literal has to be bracketed to sit before a port. Browsers report it + // bracketed already; tests and older code may not. + const bare = location.hostname.replace(/^\[|]$/g, ''); + const host = bare.includes(':') ? `[${bare}]` : bare; + const href = `${scheme ? `${scheme.toLowerCase()}:` : location.protocol}//${host}:${port}${tail}`; + const parsed = parse(href); + if (!parsed || parsed.hostname !== host || parsed.port !== port) { + return NONE; + } + return {internal: false, href}; + } + if (value.startsWith('/')) { + const parsed = parse(value, origin); + if (value.startsWith('//') || !parsed || parsed.origin !== origin) { + return NONE; + } return {internal: true, href: value}; } - const hostRelative = value.match(/^(?:(https?:)\/\/)?(:\d+.*)$/i); - if (hostRelative) { - const scheme = hostRelative[1] ? hostRelative[1].toLowerCase() : location.protocol; - return {internal: false, href: `${scheme}//${location.hostname}${hostRelative[2]}`}; + + const parsed = parse(value); + if (!parsed || (parsed.protocol !== 'http:' && parsed.protocol !== 'https:')) { + return NONE; } return {internal: false, href: value}; } + +function parse(value, base) { + try { + return new URL(value, base); + } catch { + return null; + } +} diff --git a/wrolpi/bookmarks.py b/wrolpi/bookmarks.py index a07c35c2..b3e4a34a 100644 --- a/wrolpi/bookmarks.py +++ b/wrolpi/bookmarks.py @@ -13,6 +13,7 @@ A bookmark `url` may take three forms: import re from dataclasses import dataclass, field from typing import List, Optional, Tuple +from urllib.parse import urlsplit from wrolpi.common import ConfigFile, logger from wrolpi.errors import ValidationError @@ -20,8 +21,16 @@ from wrolpi.switches import register_switch_handler, ActivateSwitchMethod logger = logger.getChild(__name__) -# The forms a bookmark URL may take. See the module docstring. -URL_PATTERN = re.compile(r'^(/|:\d+|https?://)', re.IGNORECASE) +# Characters that a URL parser strips or that split a URL in surprising places. Any of +# them in a bookmark is a sign of an attempt to smuggle a host past the checks below. +_FORBIDDEN = re.compile(r'[\x00-\x20\x7f\\]') +# A port on this host: an optional scheme, then `:` and the port, then the end of the URL +# or the start of a path, query or fragment. `:8096@evil.com` does not match, because +# `@` may not follow the port. +_PORT_FORM = re.compile(r'^(?:(https?)://)?:(\d{1,5})(?=$|[/?#])(.*)$', re.IGNORECASE) +_PORT_FORM_START = re.compile(r'^(?:https?://)?:', re.IGNORECASE) + +URL_HELP = 'Bookmark URL must be a path (/videos), a port (:8096/) or an absolute URL (https://)' @dataclass @@ -43,9 +52,34 @@ def walk(nodes: List[dict], parent: Optional[dict] = None): def validate_url(url: str) -> str: + """Return `url` stripped, or raise if it is not one of the three forms. + + The forms are checked by parsing, not by prefix. A prefix check accepted `//evil.com` + as a path and `:8096@evil.com` as a port, both of which a browser takes off this host. + """ url = (url or '').strip() - if not url or not URL_PATTERN.match(url): - raise ValidationError('Bookmark URL must be a path (/videos), a port (:8096/) or an absolute URL (https://)') + if not url or _FORBIDDEN.search(url): + raise ValidationError(URL_HELP) + + if _PORT_FORM_START.match(url): + # A port on this host. Everything after the port must be a path, query or + # fragment; `@` anywhere in it would turn the host into credentials. + match = _PORT_FORM.match(url) + if not match or not 0 < int(match.group(2)) < 65536 or '@' in match.group(3): + raise ValidationError(URL_HELP) + return url + + if url.startswith('/'): + # A path on this WROLPi. A second slash (`//evil.com`) is a network path, which + # leaves this host. `urlsplit` agrees only when the netloc comes back empty. + parts = urlsplit(url) + if url.startswith('//') or parts.scheme or parts.netloc: + raise ValidationError(URL_HELP) + return url + + parts = urlsplit(url) + if parts.scheme.lower() not in ('http', 'https') or not parts.netloc: + raise ValidationError(URL_HELP) return url @@ -56,6 +90,43 @@ def validate_name(name: str) -> str: return name +def validate_tree(nodes) -> None: + """Raise ValidationError unless `nodes` is a well-formed bookmarks tree. + + Applied to every write, including a hand-edited config file and the config API, so + the nav never renders a URL that `validate_url` would have refused. + """ + if not isinstance(nodes, list): + raise ValidationError('bookmarks must be a list') + seen_ids = set() + # Iterative, with the objects on the path tracked, so a cyclic structure (which a + # YAML anchor can build) is rejected rather than recursed into forever. + stack = [(nodes, ())] + while stack: + siblings, path = stack.pop() + if id(siblings) in path: + raise ValidationError('bookmarks tree contains a cycle') + for node in siblings: + if not isinstance(node, dict): + raise ValidationError(f'Bookmark is not a mapping: {node!r}') + node_id = node.get('id') + if not isinstance(node_id, int) or isinstance(node_id, bool) or node_id in seen_ids: + raise ValidationError(f'Bookmark id is missing or duplicated: {node_id!r}') + seen_ids.add(node_id) + validate_name(node.get('name')) + has_url, has_children = 'url' in node, 'children' in node + if has_url == has_children: + raise ValidationError(f'Bookmark {node_id} must have a url or children, not both') + if has_url: + validate_url(node['url']) + if 'new_tab' in node and not isinstance(node['new_tab'], bool): + raise ValidationError(f'Bookmark {node_id} new_tab must be true or false') + else: + if not isinstance(node['children'], list): + raise ValidationError(f'Bookmark {node_id} children must be a list') + stack.append((node['children'], path + (id(siblings),))) + + class BookmarksConfig(ConfigFile): file_name = 'bookmarks.yaml' default_config = dict( @@ -64,7 +135,23 @@ class BookmarksConfig(ConfigFile): ) validator = BookmarksConfigValidator + def validate(self, config: dict) -> bool: + # The dataclass validator sees only that `bookmarks` is a list. Every writer + # (the bookmarks API, the config API, import) comes through here, so the tree + # itself is checked too. + if not super().validate(config): + return False + if 'bookmarks' in config: + try: + validate_tree(config['bookmarks']) + except ValidationError as e: + logger.error(f'{self.file_name}: {e}') + return False + return True + def import_config(self, file=None, send_events=False): + # An invalid file raises here, leaving successful_import False so save() will not + # overwrite it. A missing file is imported as empty, so the first save creates it. super().import_config(file, send_events) self.successful_import = True diff --git a/wrolpi/test/test_bookmarks.py b/wrolpi/test/test_bookmarks.py index 8dd12211..83634105 100644 --- a/wrolpi/test/test_bookmarks.py +++ b/wrolpi/test/test_bookmarks.py @@ -1,8 +1,9 @@ +import pytest import yaml from wrolpi.bookmarks import get_bookmarks_config from wrolpi.common import get_media_directory -from wrolpi.errors import ValidationError +from wrolpi.errors import InvalidConfig def read_config(): @@ -86,10 +87,22 @@ async def test_bookmarks_tree_api(async_client, test_directory): async def test_bookmarks_validation(async_client, test_directory): """Names are required and URLs must be a path, a port, or an absolute URL.""" - for url in ('', 'videos', 'ftp://example.com', 'javascript:alert(1)', '8096/'): + rejected = ( + '', 'videos', 'ftp://example.com', 'javascript:alert(1)', 'data:text/html,hi', '8096/', + # Paths that a browser takes off this host: network paths, backslash tricks, and + # control characters a URL parser strips before deciding. + '//evil.com', '///evil.com', '/\\evil.com', '/\\@evil.com', '/\t/evil.com', '/\n/evil.com', + # Port forms where the host becomes credentials, or where the port is not a port. + ':8096@evil.com', 'http://:8096@evil.com', ':8096\t@evil.com', ':8096/x@evil.com', ':0', ':70000', + ':8096\\evil.com', 'ftp://:8096/', + # An absolute URL with no host, or with a scheme other than http(s). + 'http://', 'https:///x', 'file:///etc/passwd', + ) + for url in rejected: request, response = await async_client.post('/api/bookmarks', json=dict(name='x', url=url)) assert response.status == 400, url - for url in ('/videos', ':8096/', ':8096', 'http://:8096/web', 'https://example.com', 'HTTP://EXAMPLE.COM'): + for url in ('/videos', '/videos?x=1#y', ':8096/', ':8096', ':8096?x=1', 'http://:8096/web', 'HTTP://:80/', + 'https://example.com', 'HTTP://EXAMPLE.COM', 'https://user@example.com/x'): request, response = await async_client.post('/api/bookmarks', json=dict(name='x', url=url)) assert response.status == 201, url @@ -100,7 +113,7 @@ async def test_bookmarks_validation(async_client, test_directory): # Whitespace is trimmed. request, response = await async_client.post('/api/bookmarks', json=dict(name=' Map ', url=' /map ')) - assert response.json['bookmark'] == dict(id=7, name='Map', url='/map', new_tab=False) + assert response.json['bookmark'] == dict(id=11, name='Map', url='/map', new_tab=False) async def test_bookmarks_move_guards(async_client, test_directory): @@ -144,3 +157,70 @@ def test_bookmarks_config_import(test_directory, test_wrolpi_config): # Later additions continue the id sequence past the hand-written ids. node = config.add_bookmark('Docs', '/docs') assert node['id'] == 3 + + +@pytest.mark.parametrize('bookmarks', [ + # A URL the bookmarks API would refuse. + [dict(id=1, name='x', url='javascript:alert(1)')], + [dict(id=1, name='x', url='//evil.com')], + [dict(id=1, name='x', url=':8096@evil.com')], + # Shape problems: duplicate ids, a missing id, a node that is both, a node that is neither. + [dict(id=1, name='x', url='/a'), dict(id=1, name='y', url='/b')], + [dict(name='x', url='/a')], + [dict(id=1, name='x', url='/a', children=[])], + [dict(id=1, name='x')], + [dict(id=1, name='x', children='nope')], + [dict(id=1, name='', url='/a')], + [dict(id=1, name='x', url='/a', new_tab='yes')], + ['not a node'], + 'not a list', +]) +def test_bookmarks_config_import_rejects_bad_tree(test_directory, test_wrolpi_config, bookmarks): + """A hand-edited file that the API would refuse is not imported, and is not overwritten.""" + config_file = test_directory / 'config/bookmarks.yaml' + config_file.parent.mkdir(exist_ok=True) + original = yaml.dump(dict(version=0, bookmarks=bookmarks)) + config_file.write_text(original) + + config = get_bookmarks_config() + with pytest.raises(InvalidConfig): + config.initialize() + with pytest.raises(InvalidConfig): + config.import_config() + assert config.successful_import is False + assert config.bookmarks == [] + # The in-memory tree is empty, and the API refuses to write it over the user's file. + with pytest.raises(RuntimeError): + config.save() + assert config_file.read_text() == original + + +def test_bookmarks_config_import_rejects_cycle(test_directory, test_wrolpi_config): + """A YAML anchor can make a directory contain itself; walking that would never end.""" + config_file = test_directory / 'config/bookmarks.yaml' + config_file.parent.mkdir(exist_ok=True) + config_file.write_text('version: 0\nbookmarks: &root\n- id: 1\n name: Loop\n children: *root\n') + + config = get_bookmarks_config() + with pytest.raises(InvalidConfig): + config.import_config() + assert config.successful_import is False + + +async def test_bookmarks_config_api_rejects_bad_tree(async_client, test_directory): + """The generic config API applies the same rules as the bookmarks API.""" + request, response = await async_client.post('/api/bookmarks', json=dict(name='ok', url='/videos')) + assert response.status == 201 + + bad = dict(version=1, bookmarks=[dict(id=1, name='x', url='javascript:alert(1)')]) + request, response = await async_client.post('/api/config?file_name=bookmarks.yaml', json=dict(config=bad)) + assert response.status == 400, response.json + request, response = await async_client.get('/api/bookmarks') + assert response.json['bookmarks'] == [dict(id=1, name='ok', url='/videos', new_tab=False)] + + good = dict(version=100, bookmarks=[dict(id=5, name='Docs', url='/docs', new_tab=True)]) + request, response = await async_client.post('/api/config?file_name=bookmarks.yaml', json=dict(config=good)) + assert response.status == 204, response.json + request, response = await async_client.get('/api/bookmarks') + assert response.json['bookmarks'] == good['bookmarks'] + assert read_config()['bookmarks'] == good['bookmarks']