Mask sensitive values on their way out of a service - #635
Merged
Conversation
elias-ba
force-pushed
the
echo-mask-payload
branch
from
August 16, 2026 04:15
0333c78 to
6ebf049
Compare
elias-ba
changed the title
1 task
elias-ba
force-pushed
the
echo-mask-payload
branch
4 times, most recently
from
August 16, 2026 20:02
0cf5e8f to
d4ba7eb
Compare
elias-ba
force-pushed
the
cancel-abandoned-runs
branch
from
August 16, 2026 20:15
a670b4d to
8e25fe6
Compare
elias-ba
force-pushed
the
echo-mask-payload
branch
from
August 16, 2026 20:19
d4ba7eb to
90c3844
Compare
echo returned its input verbatim and logged it. A payload reaching a service can carry values the server set rather than the caller, and logger output is forwarded to the client as SSE log events, so anything in there leaves twice. It now runs the payload through mask_secrets before returning or logging it. That helper already existed for Langfuse traces and does the same job here, so there is no second one to keep in step. The mask itself was narrower than what the server can fill in: it listed three field names and recognised one provider's key format. It now covers the fields for the other providers too, and matches both key shapes, so a value under a name nobody listed is still caught. Follow-ups in #634.
A payload reaching a service carries values the server put there rather than the caller. Three routes led back out: echo returned its input verbatim, any service logging its payload reached the caller too (logger output is forwarded as SSE log events), and the error envelope returns the exception text. The logger is the one that matters, because it needs nothing of the service: vocab_mapper logs its whole payload on the first line of main. Masking in create_logger covers that and anything written later without each service having to remember. Also masks the request context search_adaptor_docs sends to Sentry, which the three sibling services already stripped. The mask itself was narrower than what the server can fill in, so it now lists the other providers' fields. Widening its value pattern to a general sk- shape first made it match ordinary words - task-, risk-, disk- and friends - which would have quietly corrupted every Langfuse trace, since this is the same function used as the export mask. It is anchored now, with tests pinning both directions. The instance-auth tests read a masked field off echo's response, which they can no longer do. They assert that the request is accepted and that what the caller sent does not come back; what the server substitutes is covered directly against InstanceAuth.authenticate.
Review of the masking itself found four ways round it. The error envelope masked two branches and not the one services take. Nearly all of them catch broadly and rewrap as ApolloError(500, str(e)), which entry.py returned untouched - the same disclosure, by the sibling branch of the same case statement. Masking now happens once at the exit, so it covers the message, the details, and whatever gets added later. The log filter was attached to each logger built by create_logger, and a filter on a logger only runs for records emitted through it. A plain getLogger, or a third-party logger like httpx, writes to the same stdout handler and sailed past - and vocab_mapper already silences httpx precisely because it reaches that stream. It sits on the handler now, so what a service uses to log makes no difference. Rendering the message inside the filter moved %-formatting out of the handler, where a bad format string is caught and reported, and into the caller's own logging call, where it is not. A cosmetic typo in a log line would have failed the request. And a message that was neither string nor container passed through unmasked, because the formatter calls str() on it after the filter has run. Sentry saw everything unmasked: it scrubs frame locals by name, but not exception text and not a set_context payload. A before_send hook covers both, and replaces the per-service discipline three services each hand-rolled differently. Also: the lookbehind excluded a leading hyphen or underscore, which are legitimately in front of a key; api-key and apiKey were not recognised alongside api_key, and x-api-key stopped being recognised when the names were normalised; the recursion had no depth bound, which a deep payload could turn into a failed request; and two except clauses captured an exception variable they never bound, raising NameError instead of the 500 they meant to return.
The follow-up from #634, done at the boundary rather than per service. Driven through an unmounted probe that reflects its payload and masks nothing itself, plus one that raises with the payload in scope, since pointing these at echo would only prove echo masks. All three fail if the mask at entry.call's exit is removed.
The exit mask is now a function every path calls, including the two that returned early. That also settles a conflict with the cancellation stack, which restructured the same branches the other way: both now write the output file on every path and mask on every path, so the two agree instead of one silently winning. Sentry got an allowlist of sections and breadcrumbs were not on it. Every INFO record becomes a breadcrumb, and they ride along on the next error event - so a key logged through a handler the filter had not reached left that way. Masking the whole event covers the sections nobody listed, and transactions get the same hook, since before_send is for errors only. The log filter went on at the first create_logger call, over the handlers that existed by then. Langfuse attaches one to the httpx logger during import, before any service module runs, and it writes to stderr, which the bridge forwards to the caller line for line. The sweep now runs at import and again once the service module is loaded, and covers handlers on other loggers rather than only root. The value pattern matched sk- followed by any hyphenated words, so sk-antelope-migration-plan came back redacted. Now that the same function masks what a service returns, that silently corrupts a caller's own data. It anchors on the prefixes providers use, or wants an unbroken run long enough not to be a name. Also drops the server's absolute path from the input-not-found message.
Events reach the caller by a third route: not the result, not the log stream, so neither of those masks sees them. Nothing puts a key in one today, which is the moment to close it rather than after something does.
Three services each stripped api_key from their Sentry context by name, in three slightly different ways, and one of them missed nested values and key-shaped strings entirely. They call the shared mask now. The stream manager is still dropped by name, because that is an object rather than data and not a secret at all. Also removes set_log_output and the filename it sets: nothing has read either since logging moved to stdout.
The regex had three comments for one pattern, and the longest recounted the two shapes I tried before this one rather than explaining the one that is there. The depth guard said the same thing twice, once beside the constant and once in its test. Also drops a phrase that named what the temp payload holds - the same thing the commit messages were careful about, missed one layer down in a public repo.
elias-ba
force-pushed
the
echo-mask-payload
branch
from
August 16, 2026 20:48
90c3844 to
6c5cc66
Compare
Live testing found the close handler was aborting nothing. Elysia builds a fresh wrapper object for each websocket event, so the one the close handler receives is never the one the message handler stored against - the lookup missed every time, and the child kept generating. Keyed on the underlying socket now, which is what the two events share. Nothing caught this: the run settles on its own eventually, so the only signal was a python process still alive after the socket went away. The test looks for exactly that, and closes early enough that echo has not finished by itself - a longer wait passes whether or not the fix is there, which is how the first version of it fooled me.
Masking echo took away the only test of this. The five instance-auth rows used to read the substituted value back off the response; with everything coming back "[REDACTED]" a swap that wrote the wrong value, or forwarded the caller's own, would look exactly like a correct one. applyResolvedKey is now its own exported function and asserted directly, so the substitution is checked without a service having to hand a value back to prove it. Paired with the InstanceAuth rows that pin which resolution a credential produces, that covers the ground the round trip did. Checked by breaking it both ways: forwarding the caller's credential fails two of the five, and blanking the field rather than dropping it fails one.
Contributor
|
Looks great, thank you Elias! I added a tiny change to copy the logger registry before walking it |
Collaborator
Author
|
Thanks @hanna-paasivirta that sounds great, nice catch. Please feel free to merge PRs when you happy with them |
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
2 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
A payload reaching a service carries values the server put there rather than the caller, and services could hand them back out. This masks them at the boundary instead of trusting each service to remember.
Stacked on #632 → #631 → #630 → #629. Review those first; this branch contains them.
Implementation Details
Three routes led back to the caller:
echoreturned its input verbatim, which is what started thislogevents — andvocab_mapperlogs its whole payload on the first line ofmainApolloError(500, str(e))with the request in scopeRather than patch three call sites, masking moved to places a service cannot bypass:
create_loggerinstalls a filter on the log handler, so what a service uses to log makes no differenceentry.callbefore_sendhook masks what Sentry receives — it scrubs frame locals by name, but not exception text, contexts or breadcrumbsmask_secretsalready existed for Langfuse traces, so it is reused rather than duplicated. Doing so showed it was narrower than what the server can fill in: it now covers the other providers' field names, normalised soapi-key,apiKeyandX-Api-Keyall match, and has a depth bound so a deeply nested payload cannot turn into aRecursionError.The value pattern needed care in both directions, because the same function passes over workflow YAML, job code and service results. A loose version ate
task-,risk-anddisk-prefixed names; a different loose version atesk-antelope-migration-plan. It anchors on the prefixes providers actually use, or wants a run too long and unbroken to be a name, andtest_mask_secrets.pypins both directions.Additional notes for the reviewer
Five instance-auth tests changed, and the coverage they had is restored elsewhere. They read the substituted value off
echo's response, which masking makes impossible — with everything coming back[REDACTED], a swap that wrote the wrong value, or forwarded the caller's own, would look identical to a correct one. SoapplyResolvedKeyis now its own exported function and asserted directly: the substitution is checked without a service having to hand a value back to prove it. Paired with theInstanceAuthrows that pin which resolution a given credential produces, that covers the same ground the round trip did. Verified by breaking it both ways — forwarding the caller's own credential fails two of the five new assertions, blanking the field rather than dropping it fails one.A websocket fix rides along. Live testing found
closewas aborting nothing: Elysia builds a fresh wrapper object per event, so the object the close handler receives is never the one the message handler stored against. Keyed on the underlying socket now, with a test that fails without the fix.Closes #634
AI Usage