Error on startup if invalid setting types

This commit is contained in:
Simon Willison 2025-10-24 00:14:28 -07:00
commit 23715d6c00
5 changed files with 96 additions and 15 deletions

View file

@ -394,10 +394,37 @@ class Datasette:
config = config or {} config = config or {}
config_settings = config.get("settings") or {} config_settings = config.get("settings") or {}
# validate "settings" keys in datasette.json # Validate settings from config file
for key in config_settings: for key, value in config_settings.items():
if key not in DEFAULT_SETTINGS: if key not in DEFAULT_SETTINGS:
raise StartupError("Invalid setting '{}' in datasette.json".format(key)) raise StartupError(f"Invalid setting '{key}' in config file")
# Validate type matches expected type from DEFAULT_SETTINGS
if value is not None: # Allow None/null values
expected_type = type(DEFAULT_SETTINGS[key])
actual_type = type(value)
if actual_type != expected_type:
raise StartupError(
f"Setting '{key}' in config file has incorrect type. "
f"Expected {expected_type.__name__}, got {actual_type.__name__}. "
f"Value: {value!r}. "
f"Hint: In YAML/JSON config files, remove quotes from boolean and integer values."
)
# Validate settings from constructor parameter
if settings:
for key, value in settings.items():
if key not in DEFAULT_SETTINGS:
raise StartupError(f"Invalid setting '{key}' in settings parameter")
if value is not None:
expected_type = type(DEFAULT_SETTINGS[key])
actual_type = type(value)
if actual_type != expected_type:
raise StartupError(
f"Setting '{key}' in settings parameter has incorrect type. "
f"Expected {expected_type.__name__}, got {actual_type.__name__}. "
f"Value: {value!r}"
)
self.config = config self.config = config
# CLI settings should overwrite datasette.json settings # CLI settings should overwrite datasette.json settings
self._settings = dict(DEFAULT_SETTINGS, **(config_settings), **(settings or {})) self._settings = dict(DEFAULT_SETTINGS, **(config_settings), **(settings or {}))

View file

@ -179,9 +179,7 @@ def permission_allowed_default_allow_sql(datasette, actor, action, resource):
This runs before other permission checks to ensure the setting is respected. This runs before other permission checks to ensure the setting is respected.
""" """
if action == "execute-sql": if action == "execute-sql":
default_allow_sql_setting = datasette.setting("default_allow_sql") if not datasette.setting("default_allow_sql"):
# Handle both boolean False and string "false" (from CLI)
if default_allow_sql_setting in (False, "false"):
return False return False
return None return None
@ -227,9 +225,7 @@ async def permission_resources_sql(datasette, actor, action):
rules.extend(config_rules) rules.extend(config_rules)
# Check default_allow_sql setting for execute-sql action # Check default_allow_sql setting for execute-sql action
default_allow_sql_setting = datasette.setting("default_allow_sql") if action == "execute-sql" and not datasette.setting("default_allow_sql"):
# Handle both boolean False and string "false" (from CLI)
if action == "execute-sql" and default_allow_sql_setting in (False, "false"):
# Return a deny rule for all databases # Return a deny rule for all databases
sql = "SELECT NULL AS parent, NULL AS child, 0 AS allow, 'default_allow_sql is false' AS reason" sql = "SELECT NULL AS parent, NULL AS child, 0 AS allow, 'default_allow_sql is false' AS reason"
rules.append( rules.append(

View file

@ -307,7 +307,57 @@ def test_setting_type_validation():
runner = CliRunner() runner = CliRunner()
result = runner.invoke(cli, ["--setting", "default_page_size", "dog"]) result = runner.invoke(cli, ["--setting", "default_page_size", "dog"])
assert result.exit_code == 2 assert result.exit_code == 2
assert '"settings.default_page_size" should be an integer' in result.stderr assert '"settings.default_page_size" should be an integer' in result.output
def test_setting_boolean_validation_invalid():
"""Test that invalid boolean values are rejected"""
runner = CliRunner()
result = runner.invoke(
cli, ["--setting", "default_allow_sql", "invalid", "--get", "/-/settings.json"]
)
assert result.exit_code == 2
assert (
'"settings.default_allow_sql" should be on/off/true/false/1/0' in result.output
)
@pytest.mark.parametrize("value", ("off", "false", "0"))
def test_setting_boolean_validation_false_values(value):
"""Test that 'off', 'false', '0' work for boolean settings"""
runner = CliRunner()
result = runner.invoke(
cli,
[
"--setting",
"default_allow_sql",
value,
"--get",
"/_memory/-/query.json?sql=select+1",
],
)
# Should be forbidden (setting is false)
assert result.exit_code == 1, result.output
assert "Forbidden" in result.output
@pytest.mark.parametrize("value", ("on", "true", "1"))
def test_setting_boolean_validation_true_values(value):
"""Test that 'on', 'true', '1' work for boolean settings"""
runner = CliRunner()
result = runner.invoke(
cli,
[
"--setting",
"default_allow_sql",
value,
"--get",
"/_memory/-/query.json?sql=select+1&_shape=objects",
],
)
# Should succeed (setting is true)
assert result.exit_code == 0, result.output
assert json.loads(result.output)["rows"][0] == {"1": 1}
@pytest.mark.parametrize("default_allow_sql", (True, False)) @pytest.mark.parametrize("default_allow_sql", (True, False))

View file

@ -88,7 +88,7 @@ def test_invalid_settings(config_dir):
try: try:
with pytest.raises(StartupError) as ex: with pytest.raises(StartupError) as ex:
ds = Datasette([], config_dir=config_dir) ds = Datasette([], config_dir=config_dir)
assert ex.value.args[0] == "Invalid setting 'invalid' in datasette.json" assert ex.value.args[0] == "Invalid setting 'invalid' in config file"
finally: finally:
(config_dir / "datasette.json").write_text(previous, "utf-8") (config_dir / "datasette.json").write_text(previous, "utf-8")

View file

@ -37,7 +37,9 @@ def plugin_allow_all_for_user(user: str) -> Callable[[str], PermissionSQL]:
return provider return provider
def plugin_deny_specific_table(user: str, parent: str, child: str) -> Callable[[str], PermissionSQL]: def plugin_deny_specific_table(
user: str, parent: str, child: str
) -> Callable[[str], PermissionSQL]:
def provider(action: str) -> PermissionSQL: def provider(action: str) -> PermissionSQL:
return PermissionSQL( return PermissionSQL(
"deny_specific_table", "deny_specific_table",
@ -66,7 +68,9 @@ def plugin_org_policy_deny_parent(parent: str) -> Callable[[str], PermissionSQL]
return provider return provider
def plugin_allow_parent_for_user(user: str, parent: str) -> Callable[[str], PermissionSQL]: def plugin_allow_parent_for_user(
user: str, parent: str
) -> Callable[[str], PermissionSQL]:
def provider(action: str) -> PermissionSQL: def provider(action: str) -> PermissionSQL:
return PermissionSQL( return PermissionSQL(
"allow_parent", "allow_parent",
@ -81,7 +85,9 @@ def plugin_allow_parent_for_user(user: str, parent: str) -> Callable[[str], Perm
return provider return provider
def plugin_child_allow_for_user(user: str, parent: str, child: str) -> Callable[[str], PermissionSQL]: def plugin_child_allow_for_user(
user: str, parent: str, child: str
) -> Callable[[str], PermissionSQL]:
def provider(action: str) -> PermissionSQL: def provider(action: str) -> PermissionSQL:
return PermissionSQL( return PermissionSQL(
"allow_child", "allow_child",
@ -137,7 +143,9 @@ def plugin_conflicting_same_child_rules(
return [allow_provider, deny_provider] return [allow_provider, deny_provider]
def plugin_allow_all_for_action(user: str, allowed_action: str) -> Callable[[str], PermissionSQL]: def plugin_allow_all_for_action(
user: str, allowed_action: str
) -> Callable[[str], PermissionSQL]:
def provider(action: str) -> PermissionSQL: def provider(action: str) -> PermissionSQL:
if action != allowed_action: if action != allowed_action:
return PermissionSQL( return PermissionSQL(