From 2ab817eb836fcfe876ce6bef6b2842c11eeba769 Mon Sep 17 00:00:00 2001 From: Roland Date: Sat, 26 Sep 2026 12:57:21 -0600 Subject: [PATCH] bookmarks: parse URLs instead of matching a prefix; validate the whole tree on every write The URL contract was checked by prefix on both sides, and a prefix is not a form: `//evil.com` starts like a path and `:8096@evil.com` starts like a port, and a browser takes both off this host. A path is now a single leading slash that resolves to this origin, a port form is an optional scheme, `:`, digits and then only a path, query or fragment (no `@`, no backslash, no control characters), and anything else must parse as http(s). The same rules live in wrolpi/bookmarks.py and BookmarksContext.js, with the same bypass strings in both test suites. The config's dataclass validator only saw that `bookmarks` was a list, so a hand-edited file or the generic config API could store a `javascript:` URL that the bookmarks API would refuse, and the nav rendered it. The config now validates the tree itself on every write and import: URL forms, exactly one of url/children, unique integer ids, boolean new_tab, and no cycles (a YAML anchor can build one). An invalid file is not imported and, since it was never successfully imported, is never overwritten. On the page, a stored URL that does not resolve is rendered as text, never as an href. A refused add keeps what was typed instead of clearing the form, and an edit whose move is refused still refreshes the nav (the rename was committed) and keeps the modal open. An IPv6 host is bracketed before a port. Co-Authored-By: Claude Fable 5.1 --- app/src/components/Bookmarks.js | 65 ++++++++++++------- app/src/components/Bookmarks.test.js | 41 +++++++++++- app/src/contexts/BookmarksContext.js | 57 +++++++++++++++-- wrolpi/bookmarks.py | 95 ++++++++++++++++++++++++++-- wrolpi/test/test_bookmarks.py | 88 ++++++++++++++++++++++++-- 5 files changed, 311 insertions(+), 35 deletions(-) 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']