fix(sdk): prevent over-redaction of dict keys with keyword substrings - #4581
Draft
all-hands-bot wants to merge 1 commit into
Draft
fix(sdk): prevent over-redaction of dict keys with keyword substrings#4581all-hands-bot wants to merge 1 commit into
all-hands-bot wants to merge 1 commit into
Conversation
PR #4508 added re.IGNORECASE to the dict-entry redaction regex, which caused non-sensitive keys containing KEY/SECRET/TOKEN/PASSWORD as substrings (e.g. monkey, donkey, keyboard, secretary, tokenizer) to have their values incorrectly redacted. The fix splits the pattern into two alternations: - Underscore-delimited components (case-insensitive): api_key, MY_SECRET - PascalCase/camelCase components (case-sensitive): UserPassword, apiKey This preserves the intent of #4508 (matching lowercase api_key, token) while avoiding false positives on common English words. Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
Contributor
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Contributor
Coverage Report •
|
||||||||||||||||||||
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.
HUMAN:
AGENT:
Why
Source PR reviewed: #4508 (case-insensitive dict-entry secret redaction in
redact.py)Applicable risk lens: #10 (Negative-space and silent-success cases) — the PR
introduced over-redaction that silently returns success with incorrect output.
PR #4508 added
re.IGNORECASEto the dict-entry secret redaction regexes inredact_text_secrets(). The pattern[A-Z_]*(?:KEY|SECRET|TOKEN|PASSWORD)[A-Z_]*with
IGNORECASEmatches any key containing those keywords as bare substrings,not just underscore/camelCase-separated components. This causes non-sensitive keys
like
monkey,donkey,keyboard,secretary,keyword, andtokenizertohave their values incorrectly replaced with
<redacted>.Deterministic reproduction and impact:
This over-redaction corrupts log output and telemetry, making non-sensitive data
unreadable and potentially causing debugging confusion when values that are
explicitly NOT secret get masked.
Root cause: The character class
[A-Z_]withre.IGNORECASEmatches allletters (upper and lower). So
[A-Z_]*KEY[A-Z_]*matchesmonkeybecausemonmatches
[A-Z_]*andkeymatchesKEY(case-insensitive). The pre-#4508case-sensitive pattern only matched uppercase compound names like
API_KEY.Why the original tests did not catch the case: The tests in #4508 verified that
lowercase sensitive keys (
api_key,token) ARE redacted — which is correct — butdid not test that non-sensitive keys containing keyword substrings (
monkey,secretary) are NOT redacted. The test for non-sensitive keys only usednameand
path, neither of which contains a keyword substring.Proof that the regression test fails on unmodified main:
Summary
[A-Z_]*(?:KEY|SECRET|TOKEN|PASSWORD)[A-Z_]*pattern (withre.IGNORECASE) with two alternations: an underscore-delimited pattern(case-insensitive, matching
api_key,MY_SECRET,key) and aPascalCase/camelCase pattern (case-sensitive, matching
UserPassword,apiKey).monkey,donkey,keyboard,secretary,keyword, andtokenizerare NOT redacted.Issue Number
N/A — found via weekly regression hunt.
How to Test
Before fix (fails on main):
uv run pytest tests/sdk/utils/test_redact.py::TestRedactTextSecretsDictKeys::test_does_not_over_redact_keys_containing_keyword_substrings -xvs # FAILED — monkey/donkey/keyboard/secretary/keyword/tokenizer values are over-redactedAfter fix (passes):
uv run pytest tests/sdk/utils/test_redact.py -xvs # 39 passedFull edge-case probe (run on main baseline with fix applied):
End-to-end validation:
$ uv run pytest tests/sdk/utils/test_redact.py -v # 39 passed in 0.09sVideo/Screenshots
N/A — text-based redaction logic; reproduction output included above.
Design Doc
N/A — minimal regex fix.
Type
Notes
@chintan-diwakarand approver@enysthave beenrequested as reviewers.
(
api_key,token,password,secret) are still redacted. It only preventsfalse positives on keys where the keyword appears as a bare substring within a
larger word.
system as part of the Weekly High-Impact Regression Hunt.
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:986ede8-pythonRun
All tags pushed for this build
About Multi-Architecture Support
986ede8-python) is a multi-arch manifest supporting both amd64 and arm64986ede8-python-amd64) are also available if needed