fix: SamConfig.get_all() mutates shared document, leaking one command's params into another's

[Adityaj0]

Which issue(s) does this change fix?

Fixes #9181

Why is this change necessary?

SamConfig.get_all() merged the current command's section-specific parameters into the [global] section using a live reference into self.document rather than a copy:

global_params = config_content.get(DEFAULT_GLOBAL_CMDNAME, {}).get(section, {})
global_params.update(params.copy())
params = global_params.copy()

global_params here is the actual dict stored inside self.document["<env>"]["global"][section]. Calling .update() on it permanently writes the current command's values into the shared global section. Since self.document is cached across calls (_read() only re-reads from disk when self.document is falsy), this corruption persists in memory for the lifetime of the SamConfig instance.

The result: if a SamConfig object is reused across multiple get_all() calls for different commands (a valid, documented use of the public API — get_all() takes cmd_names per call for exactly this purpose), a later command silently inherits an earlier command's parameter values instead of the true global default.

How does this change work?

Copy the global-section dict before merging into it, instead of mutating the dict stored in self.document:

global_params = dict(config_content.get(DEFAULT_GLOBAL_CMDNAME, {}).get(section, {}))
global_params.update(params)
params = global_params

What tests ran and what were the results?

Added a regression test, test_get_all_does_not_leak_command_params_into_global_across_calls, in tests/unit/lib/samconfig/test_samconfig.py. It fails against the pre-fix code (reproducing the exact leak: build's resolved config wrongly includes deploy's stack_name/region) and passes after the fix.

Full unit suite (tests/unit) passes: 9387 passed, 25 skipped.
mypy on the changed file: clean.

Checklist

• Add/update tests for this change
• make pr passes locally (unit tests + mypy on changed files; full local make pr target run via pytest tests/unit -q and scoped mypy)
• Write a clear PR title and description

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[roger-zhangg]

Thanks for the clear write-up, @Adityaj0, and apologies for the long silence on this one. I checked the branch out locally and worked through the claim; it holds up. Notes below, including two gaps I think are worth a follow-up.

The bug is real

Confirmed both halves of the mechanism:

1. get_all() mutated the document. Pre-fix, global_params at samcli/lib/config/samconfig.py:92 was the live tomlkit table stored at self.document[env]["global"][section], and .update(params.copy()) wrote the current command's values straight into it. The .copy() protected the argument, not the receiver — which is exactly the sort of thing that reads as safe on a skim.
2. The corruption persists. _read() (samcli/lib/config/samconfig.py:185-193) only re-reads from disk when self.document is falsy, so self.document = self._read() at line 87 is a no-op on every call after the first. The polluted [global] section survives for the lifetime of the SamConfig instance.

Your regression test is a genuine one — I reverted just samconfig.py on your branch and it fails with precisely the leak you describe:

AssertionError: {'stack_name': 'global-stack'} != {'stack_name': 'deploy-only-stack', 'region': 'us-east-1'}

With the fix restored it passes, and I reproduced the same clean/dirty behaviour outside the test harness against a real samconfig.toml.

Test results

tests/unit/lib/samconfig                                 44 passed, 7 skipped
tests/unit/lib/samconfig + tests/unit/cli/test_cli_config_file.py   71 passed, 7 skipped

Worth calling out: no existing test had to be changed (+35/-0 on the test file), so nothing in the suite had baked in the buggy behaviour. Good sign for the fix being a pure correctness win.

Severity: latent today, but still worth fixing

I want to be straight about blast radius rather than let this look scarier than it is. In the shipped CLI the corruption isn't currently observable:

• ConfigProvider.__call__ builds a fresh SamConfig per invocation (samcli/cli/cli_config_file.py:79), as does the --save-params path (samcli/cli/cli_config_file.py:363), so get_all() runs at most once per instance.
• The one place that calls get_all() twice on a single instance — samcli/commands/pipeline/init/interactive_init_flow.py:282 and :287 — varies env, not cmd_names, and never calls flush(), so the pollution stays confined and unwritten.

So this is an API-contract bug rather than a live customer-facing one. That's still the right thing to fix: get_all() taking cmd_names per call advertises reuse, a read-only accessor has no business mutating its backing store, and the instant anything reads-then-flush()es on one instance the corruption reaches disk. Fixing it while it's cheap beats discovering it later.

Two gaps I'd like closed (with the fix applied)

Both verified on your branch, so these are additions rather than objections:

1. The no-[global] path still returns a live reference. When the env has no [global] table, line 91's branch is skipped and line 90's value is returned as-is — a live tomlkit.Table:

type returned when no [global]: Table
document after caller mutates result: {'cached': True, 'injected': 'BOOM'}

2. dict() is a shallow copy, so nested tables stay aliased. With tags = {Team = "core"} under [default.global.parameters], the returned tags is still the document's object, and mutating it reproduces the original cross-command leak one level down:

nested 'tags' is same object as document's?   True
document global tags after mutating nested:   {'Team': 'HIJACKED'}
build now sees leaked nested value:           {'region': 'us-west-2', 'tags': {'Team': 'HIJACKED'}, 'cached': True}

Real config keys with table/inline-table values (tags, image_repositories) land in this shape. Neither gap is reachable today — cli_config_file.py:105 does its own dict(...items()) and handle_parse_options only replaces top-level values — but they're the same defect class you're fixing, so it'd be a shame to leave them. A copy.deepcopy() of the return value, applied on both paths, closes both and lets the method be unconditionally safe to hand out. Your call whether that belongs here or in a follow-up; I'm happy either way and won't hold the PR for it.

Sibling methods

I audited the rest of the class for the same aliasing shape. put() (samcli/lib/config/samconfig.py:119-133) and _deduplicate_global_parameters() (:228-245) both hold live references into self.document, but they are writers and their mutations are deliberate — not bugs, and they should stay as they are. get_stage_configuration_names() (:56-59) builds a fresh list. So get_all() was the only unintended mutator; the only remaining exposure is its own uncovered path and copy depth, per above.

One adjacent observation, clearly out of scope for this PR: ConfigProvider.__call__ at samcli/cli/cli_config_file.py:91-92 does if not self.cmd_names: self.cmd_names = cmd_names, caching the first call's command names onto a ConfigProvider instance that's created at import time and lives for the process. Same shared-mutable-state shape, also latent for the same reason. Might be worth its own issue.

On the BLOCKED status

To save the next person the dig — this is not a rebase problem. mergeable: true, branch protection has strict: false, and your parent commit is a clean ancestor of develop (just 58 commits behind, no conflicts). BLOCKED is coming from:

1. The required PR Workflow check has never reported, because as a first-time fork contributor your CI runs are parked at action_required — Build And Test, CodeQL, and Validate Pyinstaller Build all need a maintainer to click approve before they'll run.
2. develop requires 2 approving reviews plus code-owner review, and there are currently 0.

Neither needs anything from you. A maintainer needs to authorize the workflow run and then get a second reviewer on it.

Verdict

The diagnosis is correct, the fix is minimal and right, the regression test genuinely pins the behaviour, and the local suites are green. I'm supportive of merging this once CI is authorized and it picks up its second approval. The deepcopy hardening above is the only thing I'd like to see land eventually, here or separately.

(Leaving this as a comment rather than a formal review approval — I'm one reviewer's opinion, and the second approval still needs to come from someone else.)