feat(backend): security hardening + provider_api invariant + expanded API surface + alembic migrations#276
Merged
Merged
Conversation
Contributor
Test Results Summary 6 files ± 0 6 suites ±0 12m 10s ⏱️ - 6m 15s For more details on these failures, see this check. Results for commit 7a8f1c6. ± Comparison against base commit 3738fee. This pull request removes 25 and adds 781 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
3712731 to
05d7ad5
Compare
fgogolli
added a commit
that referenced
this pull request
Jun 26, 2026
…osemgrep urllib Second pass on the GitHub code-quality bot's findings on PR #276: server_daemon.py: - Add module-level ``logger = get_logger(__name__)`` so debug diagnostics can run without re-resolving the logger in every function. - _redirect_stdio: flush/dup2/devnull-redirect except blocks now log at debug level with a comment explaining the best-effort intent (daemonisation continues on failure rather than aborting startup). - daemonise(): every ``except: pass`` site now carries a comment explaining the intentional swallow — PID-file unlink races, best-effort pipe writes for parent readiness signalling, fd close races during interpreter teardown. server_runtime.py: - _terminate_group + signal handler install/remove blocks log at debug with explanatory comments. Signal forwarding losses on loops that don't support add_signal_handler are non-fatal. server_command_handlers.py + server_daemon.py: - semgrep's dynamic-urllib rule was not suppressed by ``# nosec`` alone (semgrep ignores Bandit annotations). Add proper ``# nosemgrep: python.lang.security.audit.dynamic-urllib-use-detected.dynamic-urllib-use-detected`` markers on the urlopen / Request lines. URLs are operator- controlled config-derived; not user input at the HTTP boundary.
550614e to
ab65369
Compare
- Initial Alembic migration with machines/requests/templates + indexes - Alembic auto-stamp on pre-existing installs (concurrent-safe) - SQLAlchemy Core count_by_column with column allowlist - RepositoryQueryError surfaced at application/ports layer - Safe-deserialize + strict-deserialize variants for list vs deprovisioning paths - Nullable JSON coercion with fail-loud invariants for return requests
- DashboardSummaryOrchestrator (single-UoW snapshot) - CleanupDatabase / WipeDatabase admin services - Machine sync + provisioning orchestration services - Query handlers with per-task timeout + BaseException guard - Return-request status: positive-termination-evidence guard
- SecurityHeadersMiddleware (unconditional CSP/XFO/CTO/HSTS) - RateLimitMiddleware with rate-limit-on-by-default + burst - AuditLogMiddleware with sanitized correlation ID + control-char stripping - LoggingMiddleware redacts sensitive query params (token/api_key/password) - ReadOnlyMiddleware for maintenance mode - Shared trusted-proxy X-Forwarded-For resolution (right-to-left) - Loopback admin token: constant-time compare + non-ASCII crash guard, moved to class attribute to survive module reloads - Multi-worker rate-limit warning at startup - Anonymous identity falls back to viewer role (not admin)
New routers:
- admin: init/reload-config + wipe/cleanup with executor offload
- config: get/set + save with path-traversal guard (TOCTOU-safe)
- events: SSE with asyncio.Lock subscribers + sequence_id + replay_truncated sentinel
- me / observability / providers / system
Existing routers hardened:
- require_role("viewer") on every read endpoint (requests/machines/templates)
- POST /templates/validate protected
- /admin/init returns generic error + correlation_id (no exc leak)
- providers.py replaces bare except with WARNING + exc_info
- SecurityHeadersMiddleware / RateLimitMiddleware / AuditLogMiddleware wired via server.py
…ty release policy Handler contracts: - Rename fulfillment_final to intent-specific keys (requires_async_polling / fulfillment_complete) - Typed FleetCapacityFulfilment dataclass across EC2Fleet/SpotFleet/ASG - Consolidate _resolve_provider_api in AWSHandler base with _default_provider_api hook - ASG marked requires_async_polling=True (create returns group id, not instances) Fleet release: - FleetCapacityInput dataclass passes WEIGHTED capacity to release policy - BaseFleetReleaseManager for shared release flow across EC2Fleet + SpotFleet - Never cancel/delete request-type fleets on capacity arithmetic alone; fleet-empty check gated on requires_capacity_reduction + explicit describe - Legitimate divergence preserved: delete_fleets vs cancel_spot_fleet_requests, fleet-membership tag keys
…lifecycle - reload IPC via http.client with bearer token from <work_dir>/server/orb-server.token (mode 0o600) - Daemon double-fork keeps PID lock fd held across parent → intermediate → grandchild - PID + token files created via os.open with mode 0o600 (no TOCTOU) - Server startup warns on non-loopback bind with auth disabled - start() control-flow terminal on every branch; foreground awaits runtime directly - CLI: orb storage migrate stamp/upgrade + subcommand exit-codes
…health probe + Go SDK fixes - Python SDK dynamic discovery unwraps Paginated - Health check surfaces per-entity deserialize skip counter - Go SDK uses "orb server start" instead of non-existent "system serve"
…ANIFEST for wheel - config/default_config.json: rate_limiting on with conservative defaults - pyproject.toml: alembic + sqlalchemy runtime deps, psutil for health probe - MANIFEST.in: ship rxconfig.py and alembic migrations in the wheel - rxconfig.py: package-level Reflex config (embedded UI)
…on paths - Full dispatch-matrix tests for AuditLogMiddleware, ReadOnlyMiddleware, SecurityHeadersMiddleware, RateLimitMiddleware, AuthMiddleware trusted-proxy - test_role_enforcement dynamically enumerates routes with min-count floor - Router coverage for events / me / observability / providers / system + admin/init generic-error - SSE reconnect + replay_truncated sentinel test - Config router path-traversal (null byte + broken symlink) - SIGHUP reload + server-runtime signal-handler tests - Integration security-features tests updated for SecurityHeadersMiddleware
…ce in live tests - assert_terminal_ok live helper: accept complete_with_error partials when fulfilled >= 1 AND message names shortfall - Multi-* termination tests assert fulfilled_units >= target_units (weighted-capacity contract, not headcount) - _PARTIAL_PATTERN regex anchored to canonical AWS prefixes - Live capacity helper: fail loud on 0/0 bogus response - describe_instances retry-on-NotFound for RunInstances propagation window - Unit tests: fleet release policy weighted arithmetic + base-release manager - Unit tests: consolidated _resolve_provider_api across all handlers
- Traditional / docker deployment guides with systemd + launchd notes - logrotate copytruncate sample for orb-server.log - ORB_ env-var prefix throughout (replacing legacy HF_ references) - CHANGELOG entries for orb-server.token, allow_destructive_admin, alembic migrations - API reference + user guide updates for new routers
…ncellation - Unit / integration / infrastructure / providers via pytest-xdist (-n 2 --dist=loadscope) - providers-tests-serial job for @pytest.mark.serial tests on every PR - e2e job pairs continue-on-error with GITHUB_STEP_SUMMARY failure annotation - Concurrency: fresh push cancels in-flight runs (auto-format job opts out) - Docker compose + entrypoint aligned with new server_daemon lifecycle - OpenAPI spec export via dev-tools - .gitignore: per-package coverage-*.xml artifacts
…rectness - Configuration adapter: SecretStr serialisation for dumps - Bearer-token auth: constant-time compare + ASCII guard on candidate - Logger: file-handler for audit sink when audit_log_file configured - Scheduler: response formatter default spreads input dict; docstring fix - Template repository: safe deserialize path - Utilities: collection validation + json_utils typing
2ea96a7 to
7a8f1c6
Compare
fgogolli
added a commit
that referenced
this pull request
Jul 6, 2026
…r rebase The rebase onto post-#276 main removed src/orb/interface/serve_command_handler.py and moved the daemon-services init call into server_command_handlers._initialize_application. Redirect the test at the new target and drop unused sys/types imports.
fgogolli
added a commit
that referenced
this pull request
Jul 6, 2026
…r rebase The rebase onto post-#276 main removed src/orb/interface/serve_command_handler.py and moved the daemon-services init call into server_command_handlers._initialize_application. Redirect the test at the new target and drop unused sys/types imports.
fgogolli
added a commit
that referenced
this pull request
Jul 6, 2026
…r rebase The rebase onto post-#276 main removed src/orb/interface/serve_command_handler.py and moved the daemon-services init call into server_command_handlers._initialize_application. Redirect the test at the new target and drop unused sys/types imports.
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
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.
Description
Backend hardening for ORB: security posture, data-safety on upgrade, runtime correctness, and consolidated AWS provider contracts. 19 grouped commits, 7663 unit tests passing, zero regressions. UI + UX flow ships in a companion PR.
Highlights
Security + Auth
viewer, neveradmin. The previous anonymous-admin fallback is gone — auth-disabled deployments no longer hand out admin to network callers, and the server warns at startup if it binds to a non-loopback host with auth off.require_role("viewer")enforced on every read endpoint under/requests,/machines,/templates,/events,/me,/providers,/observability, and/system. A dynamic route-enumeration test guards against future regressions.check_destructive_admin_allowedis a public Pydantic-aware dependency; it refuses when auth is disabled (fail-closed) and is no longer a private symbol reached via cross-router imports./providers/healthno longer echoes provider error strings to the client (account IDs / ARNs stay server-side); failures log at WARNING withexc_info=True./admin/initreturns a generic error message with a correlation ID (freshuuid4) — neverstr(exc), no filesystem or traceback leak.POST /config/saverefuses paths that escape the configured config directory. UsesPath.resolve()+.is_relative_to()for containment; passes the resolved path tosave_configso the check and the write hit the same target (TOCTOU-safe)./api/v1/config,/api/v1/admin, and/api/v1/meso secret-read attempts leave a trace.X-Correlation-IDsanitized against C0 + C1 controls and U+2028/U+2029 (log-injection safe), 128-char cap.token=,api_key=,access_token=, andpassword=query parameters before writing to debug logs.unsafe-inlinefor scripts; HSTS only emitted whenrequire_https=true;X-XSS-Protectionremoved;/infono longer reports the auth strategy name. Security headers live in an unconditionalSecurityHeadersMiddleware— present regardless of auth state.0o600. Daemon writes a per-instance bearer token to<work_dir>/server/orb-server.token(mode0o600at creation, no post-write chmod window).orb server reloadsendsAuthorization: Bearer <token>so reload works under bearer-token auth. Token comparison usessecrets.compare_digest, ASCII-encoded with non-ASCII crash guard. Token set is a class attribute so it survives test-suite module reloads.get_real_client_iphelper with AuthMiddleware — walksX-Forwarded-Forright-to-left, skipping trusted proxies (spoof-resistant). Startup warns whenworkers > 1because per-process buckets sum multiplicatively.X-ORB-Schedulerheader only honoured for operator-or-higher roles.Data + Upgrade Safety
head; ships schema + indexes onmachines.statusandmachines.provider_apiin one migration. Downgrade raisesNotImplementedError(data-preserving guardrail — restore from backup).headon first boot when tables exist without analembic_versionrow. Multi-worker concurrent startup serialised viaBEGIN IMMEDIATE(SQLite) /SERIALIZABLE(Postgres) transaction; losers log and exit cleanly.RepositoryQueryError(defined inapplication/ports) is raised bycount_by_columnon SQL errors instead of silently falling through to a 100k-row list scan. Dashboard orchestrator catches it explicitly and degrades to empty counts with WARNING.tags,metadata,provider_data,security_group_ids,health_checks) coerced to empty containers beforemodel_validate— legacy rows survive reads. Coercion runs ahead of theschema_versionfast-path.provider_apiis required at the domain boundary (Field(..., min_length=1)); empty-string sentinel rows are logged at ERROR and skipped by_safe_deserialize_iteron list endpoints. Single-id lookups propagate the ValidationError so callers see a real 404.find_by_return_request_id) use_iter_deserialized_strict— a bad row raises rather than silently dropping, so a return request cannot appear "successful" against a partially-deserialized set.machine_idsinvariant: NULL is aValueErrorat read time. Prevents the class of bug where an emptymachine_idscollapses with an emptydb_machinesand stamps the request COMPLETED with instances still running.Requestreturn status now requires positive termination evidence:COMPLETEDfires only when we had machines and provider now reports none.empty AND emptyreturnsIN_PROGRESSand continues polling._safe_deserialize_iterlogs at ERROR with explicit "do not wipe" guidance and a per-entity skip counter surfaced via astorage.deserializehealth check.default_factoryPydantic fields vs nullable SQL columns.orb storage migrate stamp <revision>exposes alembic stamp via the CLI.Runtime Correctness
compute_fleet_release_decisionnow takes aFleetCapacityInputdataclass carrying weighted capacity (instance_weighted_capacity_units), not just headcount. Release managers readWeightedCapacityfrom AWSdescribe_fleet_instances/describe_spot_fleet_instancesand pass it through, so weighted templates (e.g. t3.medium weight=4) no longer trigger false-full-return arithmetic.is_full_return AND requires_capacity_reduction; for request fleets a positive_fleet_has_no_remaining_instancesdescribe is required. Fixes thecancelled_runningstate observed under weighted partial return.fulfillment_finalis renamed to intent-specific keys:requires_async_polling(handler → provisioning orchestrator) andfulfillment_complete(strategy → status polling). Each has a documented truth table; no more inverse-meaning collisions.FleetCapacityFulfilmentdataclass returned by_fetch_ec2_fleet_capacity/_fetch_spot_fleet_capacity/_fetch_asg_capacity. Downstream consumers use attribute access, not.get()with string keys.BaseFleetReleaseManagerconsolidates the shared release flow across EC2Fleet + SpotFleet. Legitimate divergence preserved:delete_fleetsvscancel_spot_fleet_requests, tag keys._resolve_provider_apiconsolidated in the AWS handler base with a documented priority order and per-handler_default_provider_api()hook.requires_async_polling=True(create returns group ID, not instances) so the polling loop observes instance state transitions before stamping COMPLETED.Machine.from_provider_formatpopulates bothprovider_apiandprovider_namefrom snake_case or camelCase keys; raisesValueErrorwith a clear message when either is missing (no more silent Pydantic ValidationError landmine).DashboardSummaryOrchestratoropens a single UoW covering counts + recent activity so all figures reflect one snapshot.count_by_status/count_by_provider_apiGROUP BY queries instead of listing withlimit=100_000against handlers that clamp at 1000.RequestStatus.is_terminal()so timeout state closes the SSE stream.is_activeagainst domain DTOs beforefilter_expressionsconverts items to dicts.cleanup_databasetreats rows with NULLcreated_atas too-recent-to-purge instead of letting them slip past the cutoff.wipe_databaseslow path skips entities with no primary key the same way the fast path does.system_handlers.__init__no longer double-assignssuper().__init__andself.container.IPC + Async Hygiene
reload-configclassified as non-destructive — admin role alone is sufficient; noallow_destructive_admin=Truerequired._loopback_reload_requestruns viaasyncio.to_threadso the async CLI handler no longer blocks the event loop.handle_storage_migrateawaitscreate_subprocess_execand scrubs the DB URL password from captured output.WipeDatabaseService.execute,CleanupDatabaseService.bulk_cleanup, andopen(config_file)all wrapped inrun_in_executor— long-running admin routes no longer block the API event loop.ListActiveRequestsconcurrent sync wraps each per-request AWS call inasyncio.wait_for(timeout=30s)(configurable) so one stuck DescribeInstances can't hang a whole page.gatherusesreturn_exceptions=Truewith an explicit re-raise pass forBaseExceptionsubclasses (CancelledError,KeyboardInterrupt).repository.save()no longer creates a throwaway event loop withasyncio.runwhen called outside a running loop; it logs a warning and skips the publish so SSE subscriber queues stay valid.SSE
_SseEventBus._subscribersis asetprotected by anasyncio.Lock; concurrent subscribe/unsubscribe cannot corrupt the set.unsubscribeis O(1) viadiscard.sequence_id.history_since_seq(since_seq)emits areplay_truncatedsentinel when the deque cannot serve the requested history — including after server restart (empty deque + non-zero since_seq) and when the client claims a seq_id ahead of anything issued this process lifetime.collections.deque(maxlen=512)so publishes no longer pay an O(n) slice cost.CI + Ops + Docs
--foreground; daemon double-fork would otherwise look like immediate exit to the supervisor.rxconfig.pylives atsrc/orb/ui/rxconfig.pyand ships in the wheel; embedded-UI runtime pointsreflex runat that directory. Repo-root re-export keeps local dev workflows working.orb-server.tokenfile, theallow_destructive_adminfield, and the initial Alembic migration.server_daemon.tail_logdocstring no longer claims automatic rotation; deployment docs include alogrotate copytruncatesample.ci-testsgained aproviders-tests-serialjob so tests marked@pytest.mark.serialactually run on every PR.concurrency:block so a fresh push cancels in-flight runs; the auto-format push job opts out.-n 2 --dist=loadscope(matches GitHub runner CPU count).continue-on-errorwith an explicit failure annotation inGITHUB_STEP_SUMMARY.orb.auditlogger gains a dedicated rotating file handler when an audit log path is configured..gitignoreextended to exclude per-packagecoverage-*.xmlartifacts.Test Coverage
events,me,observability,providers,system.test_role_enforcement.pydynamically enumerates every route with arequire_roledependency and asserts anonymous → 403. A floor guard (>= 7) prevents the enumeration from silently returning empty on future refactors.DashboardSummaryOrchestrator: happy / empty / sub-orchestrator-raises paths,recent_activitycap, ISO helpers on None, count-by-status fast paths, single-UoW snapshot invariant.server_command_handlers: start/stop/status/restart/reload/logs all unit-tested with the daemon module mocked; reload covers loopback success, loopback failure → SIGHUP fallback, ValueError fallback, bearer-token header presence vs absence.server_runtime: signal handler installation, SIGHUP reload semantics (assertscm.reloadis actually called), embedded entrypoint wiring of cwd + env + signal forwarding.AuditLogMiddlewareandReadOnlyMiddlewarefull dispatch-matrix tests; audit log verified to strip credentials from query strings._safe_deserialize_iter: skip-on-error semantics, ERROR-level log, per-entity skip counter exposure.assert_terminal_okacceptscomplete_with_errorpartials whenfulfilled_units >= 1AND message names the shortfall (regex anchored to canonical AWS prefixes); rejects 0/0 bogus responses; retriesdescribe_instancesonInvalidInstanceID.NotFoundfor AWS control-plane propagation.fulfilled_units >= target_units(weighted-capacity contract), not headcount._fleet_has_no_remaining_instancesdescribe verification._resolve_provider_apicovered across all 4 handlers.Other Polish
_SseEventBushistory backed bycollections.deque(maxlen)so publishes no longer pay an O(n) slice cost.Optional[dict]to a realRateLimitConfigPydantic model; middleware consumes the typed shape; default config files seeded with a safe rate-limit block.tsand a separate monotonic delta for latency.Type of Change
Related Issues
N/A — post-audit follow-up; no linked issue.
How Has This Been Tested?
Local:
pytest tests/unit tests/api tests/infrastructure tests/integration tests/interface tests/providers/aws/unit→ 7663 passed, 88 skipped, 0 failed. Pyright clean on all touched files. Ruff lint + format clean. Bootstrap smoke test (register core + server services on a fresh DI container) boots clean. Alembic auto-stamp tested against a pre-existing SQLite database withoutalembic_version. Full AWS live-test suite run against the branch; remaining transient failures fixed by describe_instances retry + positive-termination-evidence guard.Test Configuration
alembic,sqlalchemy,psutil. Theuiextra is not added in this PR; it ships with the companion UI PR.Checklist
Performance Impact
Performance improved
ListActiveRequestsparallel sync (Semaphore(8)) with per-task 30s timeout — a 50-request page no longer pays 50× the AWS round-trip latency and one stuck call can't hang the whole page.MachineSerializerschema_versionfast-path skips legacy fixup on every hot read (now with safe NULL coercion ahead of the fast-path).Dashboard by-status counts use SQL
GROUP BYinstead of listing 100k rows and bucketing in Python.SSE history switched from O(n)-per-publish list slicing to
deque(maxlen=...)constant-time append.Indexes on
machines.statusandmachines.provider_apiland with the initial schema.Admin
wipe/cleanupand blocking config-file reads offloaded torun_in_executor— the API event loop stays responsive during long-running admin operations.Security Considerations
A default install no longer hands out admin to anonymous network callers, secret-bearing config endpoints require admin, audit logs capture sensitive reads without leaking credentials from URLs or
X-Correlation-ID, path-traversal on/config/saveis refused (TOCTOU-safe), the destructive-admin gate fails closed under auth-disabled, loopback token comparison is constant-time and ASCII-safe, rate limiting is on by default with proxy-aware IP resolution, and security headers are emitted unconditionally regardless of auth state.Dependencies
alembic,sqlalchemy(new) — required by the SQL storage backend + migration.psutil(new) — required by the system health check (CPU/memory).ui/ reflex extra is intentionally not added here; it ships with the companion UI PR.Migration Guide
Existing JSON backend deployments
No action required.
Existing SQL backend deployments (pre-Alembic)
The SQL strategy auto-stamps
headon first boot when tables exist without analembic_versionrow. No manualalembic stampneeded for the common case.For explicit control:
Machines with
provider_apiNULL or empty appear as skipped rows in the health probe (storage.deserialize); fix by updating the row or re-running the source-request backfill query documented in the migration comments.Reload IPC
POST /admin/reload-configis the single reload endpoint; callers of the oldPOST /config/reloadneed to update.Loopback admin token
The daemon writes
<work_dir>/server/orb-server.tokenon start (mode0o600). The reload CLI reads it forAuthorization: Bearer …against the API. No manual setup required; the file is auto-cleaned on stop.Rate limiting
Now enabled by default. If your deployment relies on unrestricted request volume, set
rate_limiting.enabled=falsein your config, or raiserequests_per_minuteandburst.Deployment Notes
config/default_config.json(rate_limiting,allow_destructive_admin,audit_log_file, etc.). Deployments with a pinned config file should merge in the new keys before rollout.orb server start --foregroundwithType=simple. The daemon form (no--foreground) double-forks and detaches and will look like an immediate exit to the service manager.proxy_buffering offon the/api/v1/events/location — sample block is indocs/root/deployment/traditional.md.workers > 1the rate limiter's per-process buckets sum multiplicatively; scalerequests_per_minuteaccordingly or move to a shared backend before deploying multi-worker.Reviewers
@finos/open-resource-broker-maintainers