fix: security audit remediation#226
Merged
Merged
Conversation
This reverts commit c0203cb.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 Walkthrough</review_stack_artifact_start --> WalkthroughSecurity and reliability hardening across Taskito: atomic Redis lock TTL operations and owner masking, HMAC-signed payloads with size limits, webhook delivery-time validation and no-redirects, dashboard role/token/config protections, safer type/file reconstruction, scheduler claim error handling, mutex-poison recovery, and supporting docs/tests. ChangesDistributed Lock Expiry & Owner Privacy
Payload Integrity & Size Control
Webhook SSRF Closure & Validation
Dashboard Security (Auth, Roles, Tokens, Settings)
Proxy & Interception Type Safety & Signing
Scheduler & Storage Robustness
Result Reporting & Service Defaults
Comprehensive Security Guide & Operations Docs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/dashboard/test_dashboard_settings.py (1)
97-105: 💤 Low valueConsider adding explicit tests for protected key rejection.
The test correctly verifies that
auth:keys are hidden from the list endpoint. For completeness, consider adding tests that verify:
GET /api/settings/auth:usersreturns 404PUT /api/settings/auth:testreturns 400 with "setting key is reserved"DELETE /api/settings/auth:usersreturns 404🧪 Example additional test cases
def test_get_protected_setting_returns_404(dashboard_server: tuple[AuthedClient, Queue]) -> None: client, _ = dashboard_server with pytest.raises(urllib.error.HTTPError) as exc_info: client.get("/api/settings/auth:users") assert exc_info.value.code == 404 def test_put_protected_setting_returns_400(dashboard_server: tuple[AuthedClient, Queue]) -> None: client, _ = dashboard_server with pytest.raises(urllib.error.HTTPError) as exc_info: client.put("/api/settings/auth:test", {"value": "hacked"}) assert exc_info.value.code == 400 def test_delete_protected_setting_returns_404(dashboard_server: tuple[AuthedClient, Queue]) -> None: client, _ = dashboard_server with pytest.raises(urllib.error.HTTPError) as exc_info: client.delete("/api/settings/auth:users") assert exc_info.value.code == 404🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/dashboard/test_dashboard_settings.py` around lines 97 - 105, Add explicit tests to ensure protected "auth:" keys are rejected: create three new test functions similar to test_get_settings_hides_auth_keys that call the settings endpoints for an auth key (e.g., GET /api/settings/auth:users, PUT /api/settings/auth:test with a payload, and DELETE /api/settings/auth:users) and assert the correct HTTP error codes (GET and DELETE return 404, PUT returns 400 with the "setting key is reserved" condition). Use the same dashboard_server fixture/AuthedClient as in test_get_settings_hides_auth_keys and assert the raised urllib.error.HTTPError.code values accordingly.py_src/taskito/webhooks.py (1)
41-53: ⚡ Quick winRefine urllib redirect-refusal guidance in
_NoRedirect(py_src/taskito/webhooks.py:41-53)Returning
Nonefrom_NoRedirect.redirect_requestalready causesurllibto raiseurllib.error.HTTPErrorfor 302 redirects (tested viaopener.open(.../redirect/1)yielding HTTPError code302), so the “may vary / continue error-handling chain” concern for 302 is overstated. If you want uniform refusal behavior across other 3xx codes (301/303/307/308), add small regression tests for each status (or explicitly handle the relevanthttp_error_3xxmethods).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@py_src/taskito/webhooks.py` around lines 41 - 53, The comment says returning None from _NoRedirect.redirect_request already causes urllib to raise HTTPError for 302 but may not uniformly refuse all 3xx codes; update the implementation or tests accordingly: either add explicit http_error_3xx/http_error_301/http_error_303/http_error_307/http_error_308 handlers on the _NoRedirect class to consistently refuse all 3xx responses, or add small regression tests exercising opener.open(...) via _NO_REDIRECT_OPENER against endpoints returning 301/303/307/308 to assert HTTPError is raised; reference _NoRedirect.redirect_request and the module-level _NO_REDIRECT_OPENER when adding the handlers/tests so behavior is uniform and covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@py_src/taskito/proxies/reconstruct.py`:
- Around line 27-42: The unsigned-recipe warning is never emitted because the
code only handles the signed path where signing_secret is present; update the
branch that currently checks signing_secret (the same place handling the signed
verification path) to add an else branch that calls _warn_unsigned_once() when
signing_secret is absent so the warning is emitted exactly once; keep using the
existing _warn_unsigned_once() helper (and do not duplicate its logic) and
ensure you call it in the same function that currently processes the signed path
(the code block that references signing_secret) so unsigned reconstructions are
logged.
---
Nitpick comments:
In `@py_src/taskito/webhooks.py`:
- Around line 41-53: The comment says returning None from
_NoRedirect.redirect_request already causes urllib to raise HTTPError for 302
but may not uniformly refuse all 3xx codes; update the implementation or tests
accordingly: either add explicit
http_error_3xx/http_error_301/http_error_303/http_error_307/http_error_308
handlers on the _NoRedirect class to consistently refuse all 3xx responses, or
add small regression tests exercising opener.open(...) via _NO_REDIRECT_OPENER
against endpoints returning 301/303/307/308 to assert HTTPError is raised;
reference _NoRedirect.redirect_request and the module-level _NO_REDIRECT_OPENER
when adding the handlers/tests so behavior is uniform and covered.
In `@tests/dashboard/test_dashboard_settings.py`:
- Around line 97-105: Add explicit tests to ensure protected "auth:" keys are
rejected: create three new test functions similar to
test_get_settings_hides_auth_keys that call the settings endpoints for an auth
key (e.g., GET /api/settings/auth:users, PUT /api/settings/auth:test with a
payload, and DELETE /api/settings/auth:users) and assert the correct HTTP error
codes (GET and DELETE return 404, PUT returns 400 with the "setting key is
reserved" condition). Use the same dashboard_server fixture/AuthedClient as in
test_get_settings_hides_auth_keys and assert the raised
urllib.error.HTTPError.code values accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9061650-318e-4fcf-9bcb-b656de46fec9
📒 Files selected for processing (33)
crates/taskito-core/src/scheduler/poller.rscrates/taskito-core/src/storage/postgres/mod.rscrates/taskito-core/src/storage/redis_backend/locks.rscrates/taskito-python/src/prefork/slot.rscrates/taskito-python/src/py_queue/workflow_ops/fan_out.rsdocs/content/docs/guides/dashboard/authentication.mdxdocs/content/docs/guides/extensibility/events-webhooks.mdxdocs/content/docs/guides/extensibility/serializers.mdxdocs/content/docs/guides/operations/meta.jsondocs/content/docs/guides/operations/security.mdxdocs/content/docs/guides/reliability/locking.mdxpy_src/taskito/__init__.pypy_src/taskito/app.pypy_src/taskito/cli.pypy_src/taskito/dashboard/auth.pypy_src/taskito/dashboard/handlers/settings.pypy_src/taskito/dashboard/routes.pypy_src/taskito/dashboard/server.pypy_src/taskito/interception/converters.pypy_src/taskito/locks.pypy_src/taskito/proxies/handlers/file.pypy_src/taskito/proxies/reconstruct.pypy_src/taskito/proxies/signing.pypy_src/taskito/result.pypy_src/taskito/scaler.pypy_src/taskito/serializers.pypy_src/taskito/webhooks.pytests/core/test_result.pytests/core/test_serializers.pytests/dashboard/test_dashboard_settings.pytests/observability/test_webhooks.pytests/resources/test_interception.pytests/resources/test_proxies.py
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.
Summary
Hardens taskito against the issues found in a full security audit: untrusted-payload
RCE, dashboard authz gaps, webhook SSRF, proxy/interception abuse, and several DoS /
info-leak vectors. 29 focused commits grouped into phases; each fix is self-contained
with tests where applicable.
The central change in posture: storage write-access is no longer implicitly equivalent
to worker RCE. The new opt-in
SignedSerializerand a documented trust model make thatboundary explicit.
What changed
Dashboard / API
auth:keys (password hashes,sessions, CSRF secret).
Secure;/metrics+/readinesscan be gated behindTASKITO_DASHBOARD_METRICS_TOKEN. Bootstrap admin password is popped from the env.errors endpoint.
Serialization / proxies / interception
SignedSerializer(inner, key)— HMAC-SHA256 over payloads, rejectingforged bytes before deserialization.
type_pathreconstructionrestricted to safe kinds (no
os.system-style targets).Webhooks
redirects are no longer followed.
Locks / limits / Rust core
DistributedLock.info()masks a foreign holder'sowner_id.max_payload_bytes, default 1 MiB); NUL idempotencyseparator; scaler binds to localhost by default.
identifier quoted; Redis locks set a native TTL with an atomic reap; prefork recovers
from a poisoned slot mutex; workflow fan-out capped.
Docs
serializers, dashboard-auth, webhooks, and locking guides.
Testing
cargo check(default/postgres/redis/workflows) + clippy +cargo test— green.pytest tests/— 1037 passed, 4 skipped, 0 failed.Known gaps / follow-ups (not in this PR)
max_spawn_per_tickcap is not yet implemented (Low-severity DoS).max_retries=0still falls back to the policy default (needs a None/0 contract changeacross the PyO3 boundary).
default-denied); EncryptedSerializer AAD binding not added.
Summary by CodeRabbit
New Features
Bug Fixes
Security & Hardening
Documentation
Tests