rt/wrolpi
1
0
Fork 0
mirror of https://github.com/lrnselfreliance/wrolpi.git synced 2026-10-26 10:51:36 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 5

bookmarks: parse URLs instead of matching a prefix; validate the whole tree on every write

Browse source 
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 <noreply@anthropic.com>
This commit is contained in:
Roland 2026-09-26 12:57:21 -06:00
parent 88b261ae77
commit 2ab817eb83
No known key found for this signature in database
GPG key ID: 1BA620CE6C9A4680
5 changed files with 311 additions and 35 deletions
Download patch file Download diff file Expand all files Collapse all files

65
app/src/components/Bookmarks.js
View file

@ -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}) {
: <Icon name={node.new_tab ? 'external' : 'linkify'} size='large'/>}
</Table.Cell>
<Table.Cell style={{paddingLeft: depth ? `calc(0.75em + ${depth * 1.5}em)` : undefined}}>
{directory
? <b>{node.name}</b>
: <a href={href} target={node.new_tab ? '_blank' : undefined} rel='noopener noreferrer'>
{directory && <b>{node.name}</b>}
{!directory && (href
? <a href={href} target={node.new_tab ? '_blank' : undefined} rel='noopener noreferrer'>
{node.name}
</a>}
</a>
: <span>{node.name}</span>)}
{!directory && <Text size='xs' c='dimmed' truncate='end'>{node.url}</Text>}
</Table.Cell>
{editable && <Table.Cell>
@ -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);
}
};

41
app/src/components/Bookmarks.test.js
View file

@ -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(<BookmarkTable nodes={nodes} actions={actions()}/>);
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(<BookmarkTable nodes={tree} actions={a}/>);
@ -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(<BookmarkAddForms nodes={tree} onAddBookmark={onAddBookmark} onAddDirectory={jest.fn()}/>);
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(<BookmarkAddForms nodes={tree} onAddBookmark={jest.fn()} onAddDirectory={onAddDirectory}/>);

57
app/src/contexts/BookmarksContext.js
View file

@ -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;
}
}

95
wrolpi/bookmarks.py
View file

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

88
wrolpi/test/test_bookmarks.py
View file

@ -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']