Keep job code out of logs, error payloads and Sentry - #660
Open
elias-ba wants to merge 6 commits into
Open
Conversation
Log lines and ApolloError messages both reach the caller: the bridge forwards every Python log line as an SSE event and the error message is returned as the response body. Exception text carries the document that caused it, so a PyYAML mark quotes the workflow and an Anthropic error echoes the prompt. This replaces the exception text with its type across the chat path, drops job code from anything attached to Sentry, and turns off frame locals so the same values do not go out through the stack. The redactor no longer returns its input when it cannot parse or cannot walk a document, which was the leak underneath the rest: it now withholds with a notice the model can read. One tree walker replaces three, so a nested Lightning export cannot carry a body past it. Two guards go with it. A source scan over everything a chat request can reach fails the build when a new call site puts exception or code text on a caller-visible channel, and a second test holds the Sentry request context to drop_code.
2 tasks
elias-ba
changed the title
Four sites still carried client content. The corrector's raw JSON reply was logged whole, and its corrected_old_code is by construction a verbatim slice of the user's job body. The fall-through warning interpolated corrected_new_code, and that warning reaches sentry_sdk.capture_message, so the code became the Sentry issue title and the text of any alert routed off it. global_chat logged the first hundred characters of the chat message, and subagent_caller logged four previews of the instruction and the reply. The guard did not see any of them, which is the more important half. It matched a denylist of thirteen names, so a leak escaped by being called "response" rather than "response_text", and it only read lines inside a sink call, so one assignment hop walked past it. It is default-deny now. Inside a sink every interpolation must be safe by construction, a literal or len() or type().__name__ or a drop_code wrapper, and names assigned a built string carry their taint to the sink that uses them. Turning it round flagged 74 lines; the benign ones are vetted by exact expression in one reviewable block rather than by scattering line-wide markers, and a rename or a moved line fails closed. No new markers. It found four more of the same shape while it was at it, and it still misses a two-hop alias, which is written down where the tracking lives.
Apollo stripped every non-ASCII character out of a step name, so Café became Cafe and 患者確認 became nothing at all. The rule now lives in one place, services/name_rules.py, and everything that states or enforces it reads from there: the sanitizer, the fuzzy lookup, the prompt shown to the model, and the judges. Two modes, chosen by APOLLO_UNICODE_STEP_NAMES. Off, the default, folds accents to ASCII and matches what Lightning validates today. On, anything but a control character survives as typed, ready for Lightning#4577. Both reject the same control set, normalise to NFC and cap at 100 graphemes. The tables are generated from the Elixir that Lightning runs, so tools/unicode_parity probes both sides and fails CI if the two Unicode versions drift apart.
The hand-written composer existed to reproduce Erlang 27's normaliser, because Lightning runs on it and the two have to agree on how a name is spelled. OpenFn/lightning#5109 moves Lightning to Erlang 28, which fixes the bug. Copying it stops being the right thing at that point. Measured against Erlang 28 over the 72,269-row corpus, restricted to inputs that could be a step name: the standard library disagrees on 16,622 and the hand-written composer on 16,898. So it is now marginally worse than one line. What remains in both is a Hangul shape that only a half-finished IME produces. Real Korean, Japanese, French and Vietnamese words normalise identically under Erlang 28, Python and ICU. Sixteen rows of a realistic corpus change, all Bengali and Tamil two-part vowels. They are exactly the rows where Erlang 27 and Erlang 28 disagree, and the new output matches Erlang 28 on every one. That is the bug going away. The grapheme clusterer, the trim set and their parity checks stay. They have nothing to do with composition and they guard a real rejection. One thing this does not do. The grapheme tables are still generated from Erlang 27, so they will need regenerating against 28 once Lightning actually ships it. Doing that now would make Apollo disagree with the Lightning that is running.
Collaborator
|
Is job code considered sensitive? I don't think I've ever been particularly sensitive about where we print job code. If it is sensitive then we might have to go and look at a few things... |
Support special characters in job names
Collaborator
|
Elias's point here is that it might not be appropriate to send customer code to a third party system - especially if its not used, it's just bloat. But we do send job code to other systems (langfuse) and it is very useful to have the job code in sentry while debugging. I think we should have a wider conversation around how we use data in AI. In leiu of that I suggest we close this PR. |
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
3 participants
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.
Short Description
Apollo streams its logs and errors back to the caller while a service runs. When one failed to parse a workflow, the exception text carried the document it choked on, which is the user's job code. Sentry got it twice over, because it attaches every stack frame's local variables by default and those frames hold the workflow.
Error messages and log lines now report the exception type and a size, never the content. Sentry gets
include_local_variables=Falseand adrop_codescrubber that replaces code-bearing payload fields with a length before anything is attached to an event.Fixes #659
Implementation Details
A test enforces this rather than a convention.
test_error_logging.pywalks the import closure of the three chat entry points and fails if anything unsafe reaches a log line, an error payload or a Sentry call.It is default-deny. Inside one of those calls, every interpolated value has to be safe by construction, meaning a literal, a
len(), atype().__name__, or adrop_codewrapper. It also follows a value one assignment back, so building a string on one line and logging it on the next does not get past it. That matters because the first version was a denylist of thirteen variable names and it reported nothing at all on four sites that were genuinely leaking, including one where the user's job code became the Sentry issue title.Expressions that are safe but not obviously so are cleared by hand in one table, 51 of them, each with a reason. A rename or a moved line fails closed. Five lines are exempt by a
safe-error-text:marker, and the count is asserted, so a sixth is a decision someone has to make.redact_job_bodieswalks the document defensively and refuses rather than leaking when it cannot parse, which matters because the input is by definition a document that already failed to parse somewhere else.This branch is the base of #658. It came out of that work because it lives in the same files, and it is split out so it can be reviewed and merged on its own.
One thing left in the wrong place.
stitch_job_codeinyaml_utils.pycarries a leak comment tangled with an unrelated change, so it sits in #658 rather than here. It is a comment, not a behaviour.AI Usage
Please disclose whether you've used AI in this work (it's cool, we just want to
know!):
You can read more details in our
Responsible AI Policy