Security hardening - #207
Open
BentBr wants to merge 78 commits into
Open
Conversation
Adds `guard_url()` in http.rs that resolves hostnames and rejects loopback, private, link-local, and metadata IPs when APP_ENV=production; non-production is a no-op so local Docker setup keeps working. Called from both source and destination URI adapters before issuing any outbound request.
…and redirect bypasses - Part A: split is_blocked_ip into a const is_blocked_ipv4 helper; unwrap IPv4-mapped (::ffff:) and IPv4-compatible (::) IPv6 addresses before running V4 checks, closing the IPv6 bypass (finding #1); add CGNAT 100.64/10 and benchmark 198.18/15 ranges to the V4 deny-list. - Part B: install SsrfResolver (reqwest::dns::Resolve) on the shared HTTP client so every address reqwest actually connects to — including addresses selected after auto-followed redirects — is filtered through is_blocked_ip in strict mode; this closes DNS rebinding TOCTOU (finding #2) and redirect bypass (finding #3). Redirect policy capped at 5 hops.
Locks the XSS fix in SystemLogsViewer: verifies that email_sent detail views render body_html inside an iframe with sandbox="" (all restrictions active), that no live <script> tag appears in the DOM, and that the iframe is absent for non-email log types.
argon2 in persistence was unused — hashing goes through core::crypto::hash_password_argon2. jsonwebtoken in api was unused — JWT handling goes through core::admin_jwt / entity_jwt.
Workflow-level permission was contents:write, applying write to every job. Narrowed to contents:read at the workflow level; both backend-coverage and frontend-coverage get their own contents:write override since each has a badge-publish step that pushes to gh-pages on push-to-main.
Assert account lockout after 5 consecutive failures, counter reset on success, pre-locked account rejection, and inactive-account rejection. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add tests for all field-type validation paths (string, integer, float, boolean, date, datetime, uuid, select, multi-select), required/null checks, numeric min/max/positive-only constraints, string length and pattern constraints, the free-function validate_field dispatcher, validate_entity entry points including violation-message formatting, and validate_parent_path_consistency edge cases.
Splits the 1201-line validator_tests.rs into 7 themed files under validator_tests/ (json_types, scalar_types, datetime_types, select_types, free_fn, entity_validation, violations_detail), each ≤300 lines.
Splits the 802-line loader_tests.rs into 6 themed files under loader_tests/ (license_config, cache_config, worker_config, outbox_config, app_config, maintenance_config), each ≤300 lines. Updates the #[path] attribute in loader.rs to point at the new mod.rs.
The 748-line validation_tests.rs exceeded the 500-line hard cap. Moved each themed test group into its own file under validation_tests/ so every file stays well within the 300-line soft cap.
Add 34 unit tests for EntityDefinition across five themed submodules (constructors, field_ops, validation, sql_gen, serialization), covering get_field hit/miss, add/update/remove field happy+error paths, all validate branches, Default impl, from_params constructor, table-name helpers, SQL generation, and serde round-trips.
…const The SQL-identifier allowlist omitted entity_key, regressing dynamic-entity queries that filter/sort on it (entity path resolution). Hoist the system-field list to a single dynamic_entity_utils::SYSTEM_FIELDS constant consumed by the row mapper, the identifier validator, and the filter query builder, so the three copies can no longer drift.
Replace the VARCHAR status column with an admin_user_status PG enum (snake_case values, matching the other status enums). UserStatus now derives sqlx::Type + TS and is exported; UserResponse.status is typed UserStatus instead of a Debug-formatted String. Regenerated bindings; updated FE mocks to snake_case.
Checking can_login() before verifying the password leaked account existence and lock state (enumeration). Verify the password first; a wrong password returns the generic 401 like an unknown user, so the 403 locked/inactive response is only reachable once the correct password is supplied.
Add migration converting status from TEXT + CHECK to a dedicated outbox_status enum (matching the other status enums). OutboxStatus now derives sqlx::Type and the row record decodes it directly instead of parsing a String. locked_by stays TEXT — it is a worker-lease owner tag, not a user reference.
Add a Playwright spec that drives the per-IP login limit to 429, plus a Redis helper that clears the login_rl:* counters. The spec runs last and the global setup/teardown clear the counter before their logins so a tripped 429 never throttles the shared test-runner IP.
clearLoginRateLimit no longer aborts setup/teardown when Redis is unreachable (CI uses a different host than the compose 'redis'); the rate-limit spec now resets the per-IP counter with a successful admin login so the 10->429 sequence is deterministic without depending on Redis access.
…s clear at localhost Add an e2e check that an unknown user and a wrong password return the same generic 401 (no enumeration) and that error messages never echo the username or reveal account existence/state. Set E2E_REDIS_HOST=localhost in the CI e2e job so the rate-limit counter clear connects to the runner's Redis.
A locked user who reset their password through the app stayed locked (update_admin_user does not touch status/failed_login_attempts). reset_password now clears the lockout (Locked -> Active, counter 0) without reactivating a deactivated account. + integration test.
…cal validator The advanced dynamic-query path (build_where_clause + sort_by) did its own identifier handling: raw-interpolated (unquoted) filter columns and any alphanumeric sort_by. Both now go through dynamic_entity_repository::identifier (allowlist + double-quoting); an unknown sort falls back to the default order. Add tests for unknown/injection sort and unknown filter fields.
guard_url let SSRF_ALLOWED_HOSTS through, but the connect-time resolver filtered purely by is_blocked_ip, so an allowlisted internal host passed pre-flight then was blocked at connect. Extract a shared addr_blocked_for_host decision (strict + allowlist + is_blocked_ip) used by both guard_url and the resolver, with unit tests covering strict/non-strict, allowlisted, and public/private cases.
build_where_clause now errors on a filter key absent from the entity definition instead of silently dropping it, which would broaden a query the caller believed was narrowed. Matches the filter_entities path.
FieldValidator issued an information_schema query directly; it now calls persistence's fetch_valid_columns, keeping all SQL in the data layer.
Asserts core is dependency-free, nothing depends on api/worker, and internal crate dependencies stay within an allowlist.
New ci-gate jobs: cargo-deny (bans/licenses/sources), cargo-machete, cargo +1.96.0 check (MSRV), plus SQL-boundary and file-length (500 non-test hard / 300 soft) scripts. Mark workspace crates publish=false (proprietary) so cargo-deny skips their license/path-dep checks.
Add restriction lints (unwrap_used/expect_used/panic/todo/unimplemented) and deny(unsafe_code) to every production crate root; allow them in tests via clippy.toml. Replace resulting production panics with proper error handling or tightly-scoped, justified allows. Worker config-load failure now returns an error instead of panicking.
Split validator, field validation, config loader and entity-definition SQL generation into focused submodules so each file is under the 500 non-test-line cap. Pure reorganization; public paths preserved via re-exports.
program.rs (720 lines) split into program/{mod,execute,step_helpers,tests} under the 500 non-test-line cap. Public API (crate::dsl::DslProgram) preserved.
entity_definition_repository.rs (568) and dynamic_entity_repository/query.rs (536) split into focused submodules under the 500 non-test-line cap. Public API preserved via re-exports. Regenerated .sqlx offline data (also drops the orphaned entry from the services schema-query move).
role.rs (656) and workflow/service/mod.rs (612) split into focused submodules under the 500 non-test-line cap. Public API preserved via re-exports.
auth, dsl, entity_definitions, system, users, public dynamic-entities and permissions route files (538-830 lines) split into per-concern submodules under the 500 non-test-line cap. Routing, handler paths, DTOs and behavior unchanged; helper visibility narrowed to pub(super) where internal.
The dsl/program split merged sync-transform application into a single apply_sync_transform used by both full execution (execute/apply) and step-by-step (prepare_step). BuildPath must be applied inline during full execution but deferred step-by-step (it can depend on async transform results, applied later via apply_build_path). Add a defer_build_path flag: prepare_step defers, execute/apply apply inline. Adds unit tests pinning both branches converge.
rdt test-fe ran 'docker compose exec node pnpm test'; without -T the exec allocates a TTY, and the package 'test' script is 'vitest' (watch mode), so vitest waited for changes and the hook never returned. Use 'exec -T node pnpm test:run' (no TTY + single run); same -T for lint. Also make the pre-push release_lock EXIT trap always return 0 so it can't override a successful run's exit status.
…loop error test consumer_loop_continues_after_error slept a fixed 2s before asserting the valid run left the queued state, which is flaky on slow CI runners / slow upstream fetches (it failed in CI run 27577685978 but passes locally). Use the existing wait_until_runs_leave_queued_state poll helper (10s timeout) like the sibling tests.
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.
🔀 PR to
mainfromfix/security-hardeningFeatures
Bug Fixes
Code Refactoring
Tests
Build System
Continuous Integration
Chores