Conversation
28593db to
5f492a2
Compare
kaiitunnz
left a comment
There was a problem hiding this comment.
Several comments. PTAL.
| @@ -0,0 +1,84 @@ | |||
| """Redaction of API credentials before a task record is serialized. | |||
There was a problem hiding this comment.
This module can be moved to src/shared/utils.
| api = spec.api | ||
| if api is None: | ||
| return False | ||
| headers = api.get("headers") |
There was a problem hiding this comment.
This method detects only redacted credentials in "headers", but redact_api redacts credentials in ("headers", "params", "body", "json", "data"). Also add unit tests to guard against this error.
| @model_serializer(mode="wrap") | ||
| def _serialize(self, handler: SerializerFunctionWrapHandler) -> dict[str, Any]: | ||
| data = handler(self) | ||
| data["raw_yaml"] = redact_raw_yaml(self.raw_yaml) |
There was a problem hiding this comment.
raw_yaml always get redacted. n8n json gets converted into yaml, and yaml comments are removed, making it not raw anymore.
| data = handler(self) | ||
| data["raw_yaml"] = redact_raw_yaml(self.raw_yaml) | ||
| spec = self.task.spec | ||
| api = getattr(spec, "api", None) |
There was a problem hiding this comment.
Use isinstance instead of getattr.
| if not has_credential: | ||
| raise ExecutionError( | ||
| "spec.api.url names a custom endpoint but no credential was " | ||
| "supplied; set an Authorization header. The Nebula token is " | ||
| "never sent to an endpoint the caller chose." | ||
| ) |
There was a problem hiding this comment.
Why must we enforce auth on custom URL? Can we allow opting out?
|
|
||
| token = os.getenv("NEBULA_API_TOKEN") | ||
| if token and not any(k.lower() == "authorization" for k in headers): | ||
| has_credential = any(k.lower() == "authorization" for k in headers) |
There was a problem hiding this comment.
For custom URL, we only check the Authorization header, but some endpoints use different headers, e.g., "X-API-Key". I think we should be more comprehensive.
| return self.failed_workers[-1] if self.failed_workers else None | ||
|
|
||
| @model_serializer(mode="wrap") | ||
| def _serialize(self, handler: SerializerFunctionWrapHandler) -> dict[str, Any]: |
There was a problem hiding this comment.
This redacting logic runs on every TaskRecord serialization, which occurs very often, e.g., when task state changes. I suggest redacting only once when persisting the record.
…erence Extend the API executor with a general, endpoint-agnostic credential path while keeping the legacy Nebula path intact. spec.api.url wins over NEBULA_API_BASE_URL; spec.api.auth.credential_env wins over NEBULA_API_TOKEN. The secret never travels in the workflow spec — only the env-var reference does. A call with no credential fails closed unless spec.api.auth.mode: none opts out explicitly, so a forgotten credential errors instead of silently going out anonymous. Co-Authored-By: Claude Code <noreply@anthropic.com> Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
…dential Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
…om url Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
Moving the Nebula token injection inside the "no custom url" branch left headers read before it was assigned, so every Nebula-path call raised UnboundLocalError. Restore the ordering and gate on a custom_url flag. A caller-supplied spec.api.url now requires its own Authorization header and fails closed without one, rather than calling the endpoint anonymously. The Nebula token is never sent to an endpoint the caller chose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
5f492a2 to
000af3e
Compare
|
Also, two more issues that are worth follow-up PRs to sweep:
|
Purpose
Two ways a user API credential could escape:
maininjectsNEBULA_API_TOKENinto any request, including one aimed at a caller-suppliedspec.api.url— so pointing a task at a third-party endpoint sends it our Nebula credential.REDIS_URLdefaults to an unauthenticatedredis://with no TLS.Changes
src/worker/executors/api_executor.py— the Nebula token is injected only when the request is going to the Nebula endpoint. A caller-suppliedspec.api.urlmust carry its ownAuthorizationheader and fails closed without one.src/server/task/redact.py— new. Recursive, key-name-based redaction of credentials, through nested mappings and lists, applied to both the parsed api spec and the raw workflow YAML.src/server/task/models.py— a serializer onTaskRecordapplies the redaction, so the in-memory record keeps the real credential for dispatch while every dump is redacted. This covers all four existing Redis dump sites and any added later.src/server/dispatcher/base.py— a task rehydrated from a redacted dump cannot authenticate, so it fails withcredential_not_retainedrather than dispatching[REDACTED]as a bearer token.tests/worker/test_api_executor.py,tests/server/test_redact.py— cover the credential matrix and the redaction, including the three defects below.Design
Redaction happens at serialization, not construction. Redacting on the way in would mean the dispatcher no longer has the credential; redacting on the way out means one hook covers every dump site, current and future, instead of an enumeration that can miss one.
raw_yamlis parsed, redacted as a tree with the same recursive rule as the spec, and re-emitted. A line-based scan was tried first and is structurally unable to see a value written as a block (|) or folded (>-) scalar — both ordinary YAML, and both natural ways to write a long bearer token. Re-emitting drops comments and key order;raw_yamlis a stored record that is never re-parsed, so that cost is accepted deliberately. YAML that fails to parse is redacted wholesale rather than stored un-analysed.Three defects were found and fixed during review, each reproduced against the code rather than reasoned about:
json.auth.tokenstayed in plaintext while the header was redactedmonkey,turkey,keyword,keysall matched, destroying unrelated stored configAuthorization: |and>-leaked the value on the following lineKnown limit, stated rather than implied: redaction is by key name. A credential stored under a name that does not look like one is not detected.
Behaviour change worth calling out: an API task carrying a user credential can no longer be re-dispatched after a server restart, because the credential is deliberately not retained. It fails with a clear resubmit error.
Test Plan
Test Result
tests/server— 676 passedtests/server/test_redact.py— 19 passedtests/worker/test_api_executor.py— 5 passedpre-commit run --all-files— all hooks pass, mypy included