diff --git a/datasette/app.py b/datasette/app.py index 2cf980e1..492e24c2 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -394,10 +394,37 @@ class Datasette: config = config or {} config_settings = config.get("settings") or {} - # validate "settings" keys in datasette.json - for key in config_settings: + # Validate settings from config file + for key, value in config_settings.items(): 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 # CLI settings should overwrite datasette.json settings self._settings = dict(DEFAULT_SETTINGS, **(config_settings), **(settings or {})) diff --git a/datasette/default_permissions.py b/datasette/default_permissions.py index 925c7092..e873361c 100644 --- a/datasette/default_permissions.py +++ b/datasette/default_permissions.py @@ -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. """ if action == "execute-sql": - default_allow_sql_setting = datasette.setting("default_allow_sql") - # Handle both boolean False and string "false" (from CLI) - if default_allow_sql_setting in (False, "false"): + if not datasette.setting("default_allow_sql"): return False return None @@ -227,9 +225,7 @@ async def permission_resources_sql(datasette, actor, action): rules.extend(config_rules) # Check default_allow_sql setting for execute-sql action - default_allow_sql_setting = 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"): + if action == "execute-sql" and not datasette.setting("default_allow_sql"): # 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" rules.append( diff --git a/tests/test_cli.py b/tests/test_cli.py index 17f7c1f9..a18c8f09 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -307,7 +307,57 @@ def test_setting_type_validation(): runner = CliRunner() result = runner.invoke(cli, ["--setting", "default_page_size", "dog"]) 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)) diff --git a/tests/test_config_dir.py b/tests/test_config_dir.py index 46a6d341..0598a4a6 100644 --- a/tests/test_config_dir.py +++ b/tests/test_config_dir.py @@ -88,7 +88,7 @@ def test_invalid_settings(config_dir): try: with pytest.raises(StartupError) as ex: 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: (config_dir / "datasette.json").write_text(previous, "utf-8") diff --git a/tests/test_utils_permissions.py b/tests/test_utils_permissions.py index cb923df9..937a7a57 100644 --- a/tests/test_utils_permissions.py +++ b/tests/test_utils_permissions.py @@ -37,7 +37,9 @@ def plugin_allow_all_for_user(user: str) -> Callable[[str], PermissionSQL]: 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: return PermissionSQL( "deny_specific_table", @@ -66,7 +68,9 @@ def plugin_org_policy_deny_parent(parent: str) -> Callable[[str], PermissionSQL] 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: return PermissionSQL( "allow_parent", @@ -81,7 +85,9 @@ def plugin_allow_parent_for_user(user: str, parent: str) -> Callable[[str], Perm 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: return PermissionSQL( "allow_child", @@ -137,7 +143,9 @@ def plugin_conflicting_same_child_rules( 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: if action != allowed_action: return PermissionSQL(