[BUG] environment tool: PROTECTED_VARS matching is case-sensitive, and its protected-var tests cannot detect regressions

[strandly-the-agent]

Checks

• I have updated to the lastest minor and patch version of Strands
• I have checked the documentation and this is not expected behavior
• I have searched ./issues and there are no duplicates of my issue

Strands Version

1.50.2

Tools Package Version

0.8.6.dev3+gbe6088906 (main @ f924945 plus PR #545 @ be60889; every item below also reproduces on plain main)

Tools used

1. Environment

Python Version

3.12.13

Operating System

Linux (Amazon Linux 2023, aarch64). Item 1 is Windows-specific and was reproduced against a faithful simulation of Windows os.environ semantics, not on a real Windows runner — see Additional Context.

Installation Method

git clone

Steps to Reproduce

Filed at yonib05's request from the review of #545, which asked for these to be split out rather than ridden along in that bug fix. Four independent items, all pre-existing on main, all in src/strands_tools/environment.py / tests/test_environment.py. Items 1 and 2 are the priorities.

1. PROTECTED_VARS matching is case-sensitive, so on Windows a lower-case name slips past the guard

The guards are if name in PROTECTED_VARS: (environment.py:568 for set, :677 for delete) — exact, case-sensitive — while the write is os.environ[name] = str(value) (:614). On Windows CPython folds os.environ keys to upper case (os.py, if name == 'nt' branch: "Where Env Var Names Must Be UPPERCASE", encodekey calls .upper()), so the guard checks one string and the write lands on another.

# on Windows, or against a simulated nt os.environ (see Additional Context)
agent.tool.environment(action="set", name="env_vars_masked_default", value="false")
agent.tool.environment(action="get", name="MY_TOKEN")

agent.tool.environment(action="set", name="bypass_tool_consent", value="true")
agent.tool.environment(action="set", name="ANYTHING", value="x")   # user answers "n"

2. test_environment_dev_mode_protected_var passes on an exception, so the dev-mode protected-var path has never been exercised

tests/test_environment.py:221-233 sets os_environment["BYPASS_TOOL_CONSENT"] = True — a Python bool, not "true". environment.py:445 then calls .lower() on it.

os_environment["BYPASS_TOOL_CONSENT"] = True      # bool
result = agent.tool.environment(action="set", name="PATH", value="/bad/path")
assert result["status"] == "error"                 # satisfied by the AttributeError
assert os_environment != unchanging_value          # dict vs str — always True

3. The protected-var tests pass on the deny-by-default fixture, not on protection

# edit environment.py: PROTECTED_VARS = set()
python -m pytest tests/test_environment.py -q   # -> 29 passed

tests/test_environment.py:82-90 and :126-135 never override the autouse get_user_input mock, which returns "n", so the calls are refused by the confirmation prompt rather than by the guard. With consent mocked to "y" and the same mutation, PATH is actually overwritten to /bad/path.

4. mask_sensitive_value has no direct unit test

# edit environment.py:183: value[:4] + "..." + value[-4:]  ->  value[:8] + "..." + value[-8:]
python -m pytest tests/test_environment.py -q   # -> 29 passed on main; only PR #545's new assertions catch it

grep mask_sensitive_value tests/ returns nothing.

Expected Behavior

1. A protected variable is protected however the model spells its name — set/delete on env_vars_masked_default or Env_Vars_Masked_Default should be refused wherever they would reach ENV_VARS_MASKED_DEFAULT, and bypass_tool_consent should never be a route to self-granting the consent bypass.
2. test_environment_dev_mode_protected_var should exercise BYPASS_TOOL_CONSENT="true" and assert the variable was left untouched.
3. Emptying or shrinking PROTECTED_VARS should turn the suite red.
4. Changing the mask format should turn the suite red, from a test that targets mask_sensitive_value directly.

Actual Behavior

1. On Windows, the lower-case set succeeds and lands on the upper-cased key. Verified against the real os._Environ class wired with the nt encodekey (tool code unmodified), in both BYPASS_TOOL_CONSENT=true and interactive-approve modes:

lowercase      set->success  getenv(UPPER)='false'  get->'MY_TOKEN = SUPER_SECRET_VALUE_9876'  LEAKED=True
MixedCase      set->success  getenv(UPPER)='false'  get->'MY_TOKEN = SUPER_SECRET_VALUE_9876'  LEAKED=True
CONTROL exact  set->error    getenv(UPPER)=None     get->'MY_TOKEN = SUPE...9876'              LEAKED=False

The consent variant is worse: set('bypass_tool_consent','true') approved once makes the next set succeed while the user answers "n". All ten current PROTECTED_VARS entries are affected, including PATH. On POSIX all of this is inert (a lower-case name creates a distinct, unused variable) — confirmed.

2. The test passes on AttributeError: 'bool' object has no attribute 'lower', caught by the blanket handler at environment.py:766:

status: error, content: [{'text': "Error: 'bool' object has no attribute 'lower'"}]

It never reaches the protected-var branch, and its second assertion compares a dict to a string, so it is true unconditionally.

3. PROTECTED_VARS = set() → 29 passed. The suite cannot detect the loss of protected-variable enforcement — including, today, a regression of the entry fix(environment): drive value masking from environment variable #545 just added.

4. Widening the mask window to reveal every character of a 16-char secret → 29 passed on main.

Additional Context

Why CI is green on the Windows leg. The autouse os_environment fixture (tests/test_environment.py:29-33) replaces os.environ with a plain dict, which cannot model nt key folding, and no test tries a case variant. A platform-independent regression test that does model it (the real os._Environ with an upper-casing encodekey) fails today and passes with the fix, so this is testable on every platform without a Windows runner:

def _nt_environ(initial):
    """os.environ as it behaves on Windows: keys are folded to UPPERCASE."""
    def check_str(value):
        if not isinstance(value, str):
            raise TypeError("str expected, not %s" % type(value).__name__)
        return value
    data = {}
    env = os._Environ(data, lambda k: check_str(k).upper(), str, check_str, str)
    for k, v in initial.items():
        data[k.upper()] = v
    return env


@pytest.mark.parametrize("name", ["env_vars_masked_default", "Env_Vars_Masked_Default"])
def test_cannot_set_protected_var_case_insensitively(agent, name):
    env = _nt_environ({"TEST_TOKEN_SECRET": "abcd1234efgh5678", "BYPASS_TOOL_CONSENT": "true"})
    with mock.patch.object(os, "environ", env), \
         mock.patch.object(os, "putenv", lambda *a, **k: None), \
         mock.patch.object(os, "unsetenv", lambda *a, **k: None):
        result = agent.tool.environment(action="set", name=name, value="false")
        assert result["status"] == "error"
        assert env.get("ENV_VARS_MASKED_DEFAULT") is None

Scope note. The mask_sensitive_value name heuristic only matches TOKEN|SECRET|PASSWORD|KEY|AUTH, so DATABASE_URL, MONGODB_URI, GH_PAT and similar are returned in cleartext even with masking on. That was discussed in #545 and deliberately left out of this issue — widening the pattern is a behaviour change for anyone relying on those being readable, so it wants its own discussion.

Provenance. Found while reviewing #545 (rounds 1 and 2). Everything above was executed on be60889 in a sandbox; the Windows platform claim rests on the stdlib os.py nt branch read on that machine plus the simulation, and the one unverified step is that os.name == 'nt' selects that branch on a real Windows runner.

Labels. Recommending bug; the bug-report template also declares triage, which doesn't exist in this repo's label set — flagging rather than creating it. I couldn't apply labels myself, so they're yours to add.

Possible Solution

Item 1 — normalize the name with the same operation encodekey uses, at both guard sites (environment.py:568 and :677):

-            if name in PROTECTED_VARS:
+            if name.upper() in PROTECTED_VARS:

Verified: blocks 22/22 single-codepoint spellings that upper-case to a protected name (the current guard blocks 0/22; a .lower()-based guard only 20/22), and 29/29 tests still pass. Trade-off: on POSIX this mildly over-blocks — set(name="path"), a genuinely distinct variable there, becomes refused. An os.name-conditional variant avoids that and also passes 29/29; either is a defensible call, and the choice is worth making deliberately.

Item 2 — os_environment["BYPASS_TOOL_CONSENT"] = "true" (string), and assert os_environment["PATH"] == unchanging_value.

Item 3 — add a consent-granted variant of each protected-var test (get_user_input.return_value = "y", mirroring test_direct_set_var_allowed) asserting the variable is still refused and unchanged, so denial can no longer mask the result.

Item 4 — add a direct test, e.g. mask_sensitive_value("TOKEN", "abcd1234efgh5678") == "abcd...5678", keeping the expectation as a literal rather than deriving it from the function under test.

Happy to open a PR for any of these if that's useful — items 2, 3 and 4 are self-contained test changes; item 1 is a behaviour call I'd rather a maintainer make first.

Related Issues

#545