Conversation
server/lambda_handler.py duplicated server/adapters/aws_lambda.py with divergent behavior; it was dead code since the deployed handler is server.adapters.aws_lambda.lambda_handler (wired in terraform/aws/main.tf). The root local_server.py duplicated scripts/local_server.py, which is the maintained version (better logging, /mcp routing, OPENCONTEXT_CONFIG support). Docs updated to point at the maintained scripts/local_server.py. Co-Authored-By: Kimi K2.6 via opencode <noreply@ollama.com> (cherry picked from commit ac5d8ac)
scripts/local_server.py serves MCP requests on /mcp (not /), so the FAQ and QUICKSTART curl examples targeting the root path would 404. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 9a311d0)
The warm-start path was as slow as a cold start because lambda_handler called asyncio.run() per request and _run_with_cleanup's finally block shut down the plugin manager after every invocation. The next request then re-initialized the plugin from scratch, including a live HTTP call to the data portal — defeating Lambda container reuse entirely. The per-request shutdown was a workaround for "Event loop is closed" errors with httpx: asyncio.run() creates and closes a fresh loop each call, so httpx clients created on a previous loop broke on warm starts. Replace this with a single module-level event loop created lazily via _get_loop() and reused with loop.run_until_complete(). httpx clients stay bound to a live loop across invocations, so the plugin manager and MCP server globals in server/http_handler.py persist as intended. All existing behavior (OPTIONS handling, base64 bodies, header lowercasing, error responses) is preserved. Co-Authored-By: GLM 5.2 via opencode <noreply@ollama.com> (cherry picked from commit 30ce67b)
…ginConfig, BaseQueryValidator)
The CKAN, Socrata, and ArcGIS plugins currently copy-paste ~60% of their
scaffolding: HTTP client creation/close, the tenacity retry policy,
HTTPStatusError -> RuntimeError translation, the tool-dispatch if/elif
chain with per-arg "is required" checks, Record-N record formatting, and
the SQL/SoQL/WHERE security validators.
These new core/ base classes centralize that shared behavior so the three
plugins can be migrated onto them in a later stage:
- core/config_base.py: BasePluginConfig(pydantic.BaseModel) with
enabled/city_name/timeout fields, extra='forbid', and a reusable
validate_url classmethod that subclasses attach to their URL fields via
field_validator('base_url', 'portal_url')(BasePluginConfig.validate_url).
- core/query_validator.py: BaseQueryValidator with the union of the
existing forbidden keywords, dangerous-pattern regexes, an
ALLOWED_PREFIXES tuple, a validate_query() entry point, an overridable
extra_checks() hook for provider-specific rules, and a standalone
scan_forbidden_keywords() for WHERE-clause-style inputs (ArcGIS).
- core/base_plugin.py: BaseOpenDataPlugin(DataPlugin) that eagerly builds
plugin_config from config_class, tracks HTTP clients in self._clients
for a single shutdown() path, exposes a module-level HTTP_RETRY
decorator, _raise_http_error() that extracts CKAN/Socrata-style error
messages, a ToolHandler-based execute_tool() dispatcher with required-
arg validation and str/ToolResult return wrapping, format_records(),
and build_where_clause(). initialize(), get_tools(), and health_check()
remain abstract for subclasses.
Tests cover URL validation (good/bad/trailing-slash), extra='forbid',
subclass field_validator reuse; query validator allowed/forbidden/length/
prefix/extra_checks override/scan_forbidden_keywords; base plugin
dispatch, unknown tool, required-arg enforcement, exception wrapping,
str-return wrapping, format_records (empty/header/skip_keys/truncation),
build_where_clause (string escaping/IS NULL/numeric/AND join), and
shutdown closing all clients. All 355 tests pass.
Co-Authored-By: GLM 5.2 via opencode <noreply@ollama.com>
(cherry picked from commit d2dec23)
Migrate the CKAN plugin onto the new shared base classes introduced in
refactor/core-base-classes, removing duplicated boilerplate while keeping
all user-facing text and behavior identical.
- config_schema.py: CKANPluginConfig now subclasses BasePluginConfig, reusing
the shared enabled/city_name/timeout fields and BasePluginConfig.validate_url
via field_validator. The duplicated validate_url body is dropped. The CKAN
timeout default is overridden to 120.0 (base default is 30.0) to preserve
the historical 120-second behavior; the bound is widened to le=300 so
existing 120 configs still validate.
- sql_validator.py: SQLValidator now subclasses BaseQueryValidator, reusing
the shared length/forbidden-keyword/prefix/dangerous-pattern checks.
ALLOWED_PREFIXES=('SELECT','WITH'). The CKAN-specific checks (sqlparse
single-statement + statement-type enforcement, and the double-quoted UUID
resource-id validation) move into extra_checks. MAX_SQL_LENGTH is kept as a
backwards-compat alias of MAX_QUERY_LENGTH. tests/test_sql_validator.py
passes unmodified.
- plugin.py: CKANPlugin now subclasses BaseOpenDataPlugin with
config_class=CKANPluginConfig. The ~160-line execute_tool if/elif ladder is
replaced by tool_handlers() returning ToolHandler entries (search_datasets,
get_dataset [dataset_id], query_data [resource_id], get_schema
[resource_id], execute_sql [sql], aggregate_data [resource_id, metrics]).
The HTTP client is created via _create_http_client (keeping
base_url/headers/timeout) so the base class owns shutdown; the plugin's own
shutdown is removed. _call_ckan_api is decorated with HTTP_RETRY from
core.base_plugin instead of a local retry() copy. _parse_ckan_error and the
CKAN-specific 404 resource/dataset parameter hinting are kept in
_call_ckan_api (provider-specific richness not forced into
_raise_http_error). _format_query_results/_format_sql_results now delegate
to self.format_records(...) with their original header text and max_display
values (5 for query results, 10 for SQL results). aggregate_data uses
BaseOpenDataPlugin.build_where_clause for its WHERE construction.
Also fixed while here: aggregate_data's HAVING clause previously hardcoded
'>' for every condition (f'{expr} > {value}'). having now accepts string
values that carry their own operator (e.g. {"count(*)": ">= 5"}); numeric
values keep '>' as the documented default for backward compatibility. The
aggregate_data tool description documents this.
- tests/test_ckan_plugin.py: updated test_plugin_shutdown_closes_client to
assert the base class clears the tracked client list (plugin._clients == [])
instead of the old plugin.client is None, since shutdown is now owned by
the base. All other behavioral assertions are unchanged; the rest of the
suite (mocking httpx.AsyncClient, which _create_http_client still calls)
passes unmodified.
Co-Authored-By: GLM 5.2 via opencode <noreply@ollama.com>
(cherry picked from commit 2b166e6)
Port this hardening (adapted from upstream thealphacubicle/OpenContext security update #37) so aggregate_data validates all user-supplied SQL fragments before assembling the query, preventing SQL injection through field names, metric aliases/expressions, or order_by. - Add module-level _SAFE_IDENTIFIER = r'^[a-zA-Z_][a-zA-Z0-9_]{0,63}$' and _SAFE_METRIC_EXPR (count(*) | sum/avg/min/max/stddev/variance(field), case-insensitive) compiled patterns with _validate_identifier(name) and _validate_metric_expr(expr) helpers that raise ValueError on mismatch. - In aggregate_data, validate all group_by fields, metric aliases and expressions, filter field names, having expression identifiers, and order_by (stripping a leading '-' for descending order) BEFORE building SQL. Validation failures return {"error": True, "message": ...} so the dict-return contract is preserved. - Add tests/test_ckan_plugin.py::TestAggregateDataHardening covering rejected malicious group_by, metric alias, metric expression, filter field, having expression, and order_by, plus a valid-aggregate and a leading-dash order_by happy path. Ported from thealphacubicle/OpenContext (Feature/security update #37). Co-Authored-By: GLM 5.2 via opencode <noreply@ollama.com> (cherry picked from commit 6b31bb8)
Migrate Socrata plugin onto BaseOpenDataPlugin / BasePluginConfig / BaseQueryValidator following the CKAN reference pattern: - config_schema.py: SocrataPluginConfig now inherits BasePluginConfig; reuse BasePluginConfig.validate_url via field_validator; drop duplicated enabled, city_name, timeout, and the copy-pasted validate_url. - soql_validator.py: SoQLValidator now inherits BaseQueryValidator; move the extra semicolon multiple-statement check into extra_checks; keep public API validate_query(soql) -> (bool, Optional[str]). - plugin.py: SocrataPlugin now inherits BaseOpenDataPlugin; config_class set; replace execute_tool if/elif ladder with tool_handlers(); create both HTTP clients via _create_http_client (tracked for base-class shutdown); replace copy-pasted @Retry blocks with @HTTP_RETRY; use self._raise_http_error for HTTPStatusError translation; replace _format_query_results/_format_sql_results bodies with self.format_records(...); use BaseOpenDataPlugin.build_where_clause in query_data filter compilation; remove redundant app_token emptiness re-check in initialize(). Bug fixes folded in: - _format_categories: fix duplicated fallback key cat.get("count", cat.get("count", "")) to cat.get("count", cat.get("value", "")). - _list_categories: add max-pages guard (20 pages / 10000 results) to the unbounded pagination while-loop, with a warning log when hit. Tests updated minimally: shutdown assertions now check is_initialized only (base class manages client lifecycle via _clients list). Co-Authored-By: Kimi K2.6 via opencode <noreply@ollama.com> (cherry picked from commit 1efa1fc)
…ith plugin contract Migrates the ArcGIS plugin onto the shared base classes introduced in d2dec23, following the patterns established by the CKAN and Socrata migrations. Moved to the base classes: - config_schema.py: ArcGISPluginConfig now subclasses BasePluginConfig. Drops the duplicated enabled/city_name/timeout fields and the copy- pasted validate_url; reuses the base URL validator via field_validator (as CKAN/Socrata do). Preserves the 120s timeout default by overriding the timeout field default to 120.0 (base default is 30.0). portal_url default and token field retained. - where_validator.py: WhereValidator now subclasses BaseQueryValidator and reuses scan_forbidden_keywords for its forbidden-keyword scan. Public API validate(where) -> str is preserved (returns '1=1' for empty, raises ValueError on forbidden keywords). As a result, ArcGIS gains the GRANT/REVOKE/DECLARE/SET keywords it was previously missing versus the other plugins (its old FORBIDDEN_KEYWORDS list omitted them). The error message format changes slightly ('Forbidden keyword detected in WHERE clause: Forbidden keyword: X'). - plugin.py: ArcGISPlugin now subclasses BaseOpenDataPlugin. Config is validated eagerly in __init__ (base behavior) instead of lazily in initialize(); get_tools() uses self.plugin_config.city_name directly (the 'Unknown' fallback is dropped). Both HTTP clients are created via _create_http_client so the base class tracks them for shutdown; the plugin's own shutdown() is removed. The execute_tool if/elif ladder is replaced with tool_handlers(). health_check now relies on raise_for_status() (via _call_hub_api) instead of checking status_code == 200, matching the other plugins. _raise_http_error is used for HTTP status errors on both API paths. An _call_hub_api helper (decorated with @HTTP_RETRY) centralizes hub-client GETs, and _call_feature_service (also @HTTP_RETRY) handles feature-service GETs. ArcGIS was previously the only plugin with NO retry on its API calls; both call paths now retry transient failures (ConnectError, ReadTimeout, etc.) up to 3 attempts with exponential backoff, while HTTPStatusError and RuntimeError are not retried. Contract fixes (DataPlugin interface alignment): a. search_datasets tool input parameter renames from 'q' to 'query' to match CKAN/Socrata and the DataPlugin interface; the tool schema description is updated accordingly. b. Adds a get_schema tool + method: fetches the Feature Service layer metadata (GET {service_url}/0?f=json after _ensure_layer_url) and formats the 'fields' list (name, type, alias) like the other plugins' get_schema. Required arg is dataset_id; the service URL is resolved via get_dataset (two-hop, like query_data). If the dataset has no service URL, a clear ValueError is raised. c. DataPlugin.query_data(resource_id, filters, limit) previously repurposed the filters dict to carry where/out_fields. Restructured: an internal async _query_features(dataset_id, where, out_fields, limit) carries the tool-path behavior (tool args unchanged: where, out_fields, limit), and query_data(resource_id, filters, limit) now implements the interface contract — compiling field:value filters into a WHERE clause via BaseOpenDataPlugin.build_where_clause (validating each field identifier with WhereValidator.scan_forbidden _keywords first), defaulting to '1=1' when no filters are supplied. All other user-facing text (search/dataset/query/aggregation formats) is kept as-is. Tests: tests/test_arcgis_plugin.py is rewritten for the new structure — search_datasets param rename (query vs q), get_schema tests (fields, layer-index append, no-service-URL error), query_data contract tests (filters->WHERE compilation, None->IS NULL, forbidden field rejection, 1=1 default), retry behavior on transient errors (ConnectError retried, HTTPStatusError not retried), health_check uses raise_for_status, eager config validation in __init__, two client creation via _create_http_client, base shutdown closes tracked clients. WhereValidator tests cover the newly-gained GRANT/REVOKE/ DECLARE/SET keywords. 62 tests (up from 22), all passing. Co-Authored-By: GLM 5.2 via opencode <noreply@ollama.com> (cherry picked from commit a8f22c9)
Ported from thealphacubicle/OpenContext (Feature/security update #37). Prevents a crafted dataset record from steering Feature Service queries to arbitrary hosts. Adds a static _validate_feature_url(service_url, portal_url) helper that parses the resolved Feature Service URL and requires the scheme to be http/https and the host to either end with '.arcgis.com' or equal the configured portal host (case-insensitive), raising ValueError otherwise. The guard is invoked on the resolved service_url in both Feature-Service query paths (get_schema and _query_features) before any HTTP request is made. Unit tests cover allowed hosts (arcgis.com subdomains, exact portal host match, case-insensitivity) and rejected hosts (arbitrary domains, localhost, 169.254.169.254 metadata IP, non-http schemes, missing hostname, lookalike hosts). Integration tests verify the guard fires in both get_schema and _query_features. Co-Authored-By: GLM 5.2 via opencode <noreply@ollama.com> (cherry picked from commit 4b9dddb)
…nManager 1. custom_plugins/template/plugin_template.py - Rewrite to inherit from BaseOpenDataPlugin (was raw MCPPlugin) - Define config class subclassing BasePluginConfig, set config_class attr - initialize() uses _create_http_client for tracked HTTP lifecycle - Demonstrate tool_handlers() with required_args via ToolHandler - Provide get_tools(), health_check(), and DataPlugin stub methods - Keep heavily commented as a teaching template 2. server/http_handler.py - Move import-time config/logging load into _configure_logging_from_config() - Call it from _initialize_server() before plugin loading - Module-level default configure_json_logging(level='INFO') keeps imports side-effect-free - Preserve OPENCONTEXT_CONFIG env var behavior 3. scripts/local_server.py - Move import-time config load and logging setup into _load_local_config() - Run both in init_server() instead of at module import - Replace global with updated at init time 4. core/plugin_manager.py - discover_plugins() now returns (name, path, source_package) tuples - _load_plugin_class() receives source_package directly instead of substring-matching paths - Store ToolDefinition in self.tools during registration; value becomes (plugin_name, ToolDefinition) - get_all_tools() projects directly from self.tools, removing re-call to plugin.get_tools() - execute_tool() uses tool_def.name to dispatch, keeping public shapes identical 5. core/validators.py - Remove dead defensive check after validate_plugin_count() - Leave multiple-plugins error text untouched 6. docs/CUSTOM_PLUGINS.md - Update plugin structure bullet to mention BaseOpenDataPlugin - Add tip recommending BaseOpenDataPlugin as the starting point 7. tests/test_plugin_manager.py - Update discovery assertion to expect 3-tuple (name, path, source_package) - Update tool registration assertions to inspect ToolDefinition.name - Update execute_tool manual injection to use ToolDefinition instead of bare string - Update _load_plugin_class call sites for new signature Co-Authored-By: Kimi K2.6 via opencode <noreply@ollama.com> (cherry picked from commit b59c05c)
…ning
Code review of the hardening commit found four behavior regressions
against main, all fixed here:
- order_by "field DESC" was rejected and "-field" produced invalid SQL
("ORDER BY -field"); order_by now accepts "field", "-field", and
"field ASC|DESC", compiling to proper ORDER BY ... [ASC|DESC].
- count(field) and count(distinct field) were banned while sum(field)
was allowed; the metric whitelist now permits them.
- HAVING keys naming a declared metric alias errored; aliases are now
substituted with their expression (PostgreSQL does not allow SELECT
aliases in HAVING).
- String HAVING values were interpolated verbatim ("HAVING count(*) 5"
for "5"); values must now be a number with an optional comparison
operator, and bare numbers default to ">".
Also enforce the search_datasets required 'query' argument at dispatch
(the schema declared it required but nothing checked it).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
(cherry picked from commit 58f7956)
Code review found the helper trusted callers to validate field names (only its docstring warned about it), and the three plugins guarded three different ways — Socrata's query_data not at all. Enforce a safe identifier pattern in the helper itself so every caller, including third-party plugins built from the template, is safe by construction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 113c15f)
Code review of the migration + SSRF commits found four behavior
regressions against main, all fixed here:
- The SSRF allowlist (*.arcgis.com or portal host only) broke Hub
datasets whose Feature Services are self-hosted on city domains,
common in Hub catalogs. New trusted_service_hosts config lists extra
hosts (exact or subdomain match) without reopening arbitrary-host SSRF.
- WhereValidator scanned quoted string literals, rejecting legitimate
values like status = 'SET' or call_type = 'Initial Call'. Literals
are now stripped before the keyword scan; the double-prefixed error
message is also fixed.
- search_datasets declared 'query' required but never enforced it, so
old-schema {"q": ...} calls silently returned an unfiltered catalog
dump as success. The required arg is now enforced at dispatch, and
get_aggregations' remaining 'q' parameter is renamed to 'query' for
consistency.
- _format_query_results hand-rolled an uncapped record loop; it now
uses the shared format_records helper (10-record display cap).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
(cherry picked from commit 7f177f5)
The tool schema declared 'query' required but dispatch never checked it, so calls without it silently searched with an empty query. Found in code review; same gap fixed in the CKAN and ArcGIS plugins. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit c7f5b42)
- Matrix the test suite across Python 3.11 and 3.12 - Add Terraform validate job (fmt -check + validate for aws and bootstrap stacks) - Add Dependabot config for pip, gomod, github-actions, and terraform - Ignore Claude Code local files (CLAUDE.md, .claude/) - terraform fmt fixes for staging.tfvars and variables.tf Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 0b5222b)
…c reuse The field_validator(...)(BasePluginConfig.validate_url) reuse pattern broke on Python 3.11/3.12 (CI) while passing on 3.14: the bound classmethod received pydantic's (value, info) pair positionally, so 'cls' swallowed the value and urlparse got a ValidationInfo. A single-argument staticmethod binds identically on every version. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit ad62d8b)
- CUSTOM_PLUGINS.md: new "Decoupled Base Layer" section (BaseOpenDataPlugin, ToolHandler dispatch, BasePluginConfig, BaseQueryValidator) with examples - ARCHITECTURE.md: updated component tree and shared base layer overview - BUILT_IN_PLUGINS.md: ArcGIS plugin section; CKAN execute_sql and aggregate_data (metrics/having/order_by semantics) - config-example.yaml: document trusted_service_hosts Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 14921aa)
The workflow merges main into a `develop` branch on every push to main, but no `develop` branch exists on this fork or upstream, so the job has failed on every push since it was added. Development lands on main via PRs, so the sync has nothing to do; remove it rather than create an unused branch. Claude-Session: https://claude.ai/code/session_01TXm8YpvmTqNHgpRuoxtpoX Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit fcd1549)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First of a four-PR stack contributed back from the sgarcese/OpenContext fork. This one is the foundation; the other three build on it.
What changes
core/:BaseOpenDataPlugin,BasePluginConfig,BaseQueryValidator,ToolHandler, and a sharedHTTP_RETRY. CKAN, Socrata, and ArcGIS plugins are migrated onto it, which removes most of the copy-pasted client, config, retry, and tool-dispatch code across the three plugins.aggregate_datavalidates identifiers and metric expressions (and a follow-up restores valid inputs the first pass over-rejected).build_where_clausevalidates field identifiers.trusted_service_hosts), closing an SSRF path where a dataset record could steer requests to arbitrary hosts.search_datasetsenforces its requiredqueryargument.query_stringis still forwarded.PluginManagertidied, docs updated to describe the base layer and the ArcGIS plugin.developbranch exists here, so it failed on every push.Verification
uv run pytest -q).ruff check core plugins server tests: 412 findings vs 444 onmain(no new findings)..gitignore,staging.tfvars, andserver/adapters/aws_lambda.py, resolved as described above.Stack
Each later PR is opened against
mainbut contains this PR's commits until it merges; review the later ones by their last few commits.🤖 Generated with Claude Code
https://claude.ai/code/session_01TXm8YpvmTqNHgpRuoxtpoX