[fix] Surface malformed secrets.toml parse errors - #17428

Summary

All three expected reviews completed (claude-opus-5-5-medium, gpt-5.6-terra-xhigh, and grok-4.7-xhigh). Each requests changes. They agree the goal is right: a malformed secrets.toml should produce a secret-safe diagnostic at startup, and the server should still start. They agree the current change misses that on the real startup path.

Secrets.load_if_toml_exists() now returns False only for StreamlitSecretNotFoundError with error_id="no-secrets-found" and re-raises every other secrets error, including TOML parse failures, invalid-secrets-path, and secrets-directory-entry-not-folder. prepare_streamlit_environment() already logs that exception and returns. New unit tests cover an absent file, a malformed file (including that a sample secret stays out of the message and os.environ), and successful os.environ promotion.

That helper is also the boolean probe used while the Starlette app is built. get_cookie_secret() runs from session-middleware construction inside server.start(), and bootstrap.run awaits server.start() before _on_server_start() reaches prepare_streamlit_environment(). With a malformed file, streamlit run and st.App exit during app construction, and the existing "Failed to load secrets.toml file" log never runs. Later auth, XSRF, and connection probes that still treat the method as non-throwing raise as well. That availability regression is merge-blocking. The call order and the two viable fixes are in the inline comment.

The reviews differ on external-test risk (resolved below to No) and on how much docstring and PR-description editing to request (non-blocking; the docstring gap is real).

Product Alignment

This is a user-facing bug fix: startup behavior when a secrets file exists but cannot be loaded. It adds no public API, config option, or command. Bug fixes do not need a product spec (specs/AGENTS.md). The merged programmatic-secrets spec (specs/2026-04-15-app-secrets-param) covers secrets passed to st.App in code, and leaves TOML parse failures to this bug fix.

The PR body records three decisions: a missing file still returns False; direct st.secrets access still raises StreamlitSecretNotFoundError; startup logs the failure and continues, because a secrets load failure must not prevent the server from starting. The first two match the code. On streamlit run and st.App, the process exits while session middleware is constructed, before that log-and-continue handler runs. Principle 23 (fail fast, fail helpfully) supports a clear diagnostic. The decision in the PR is to log that diagnostic and still start. Principle 26 (minimize migration distance) also applies: apps that previously started with an unreadable secrets file now fail to boot.

Code Quality

The error_id check is a small, local way to tell "no file" from "path present but unusable," and it reuses the id already set in _parse(). There is no new abstraction or dependency. The problem is the shared helper. Cookie-secret setup, XSRF enablement (when protection is off in config), auth secret lookup, and BaseConnection._secrets all use the boolean return as "secrets available or not" and have no handler for the new raise.

st.connection type inference already ignored that boolean and then subscripted st.secrets, which calls _parse() and already raised StreamlitSecretNotFoundError for a bad file. This PR only moves that particular exception one line earlier.

A design that keeps the boolean probe lenient, and applies the strict check on the startup log path, matches both the existing callers and the PR's startup contract.

Performance

No meaningful runtime or frontend impact. Secrets loading is startup and request-setup work, outside the backend hot paths (element enqueue, ForwardMsg, reruns, caches, Arrow). The success path is still one tomllib.loads under the existing lock. The new branch is a string compare on an exception that already existed. A failed parse is still uncached, which was already true. No benchmark is needed.

Test Coverage

Unit coverage of the helper is solid: an absent file returns False, malformed TOML raises StreamlitSecretNotFoundError with path and parser text and does not promote secrets into os.environ, and a valid file still promotes scalar values. The bootstrap tests show that prepare_streamlit_environment() logs a malformed file and returns.

Those tests patch bootstrap.secrets and call prepare_streamlit_environment() directly, so they never execute get_cookie_secret() or create_streamlit_middleware(). That is why they miss the production order. An end-to-end test is unnecessary once a unit test covers that earlier caller. test_absent_secrets_file_is_non_fatal also asserts that SMTP_PASSWORD is absent from os.environ, which adds little once the file is absent and the logger was not called.

Backwards Compatibility

Apps with no secrets file, and apps with a valid secrets file, behave as before. Direct st.secrets access still raises StreamlitSecretNotFoundError, so existing handlers of that type keep working.

Apps whose configured secrets path exists but cannot be loaded no longer start. That includes malformed TOML, an invalid secrets path, and a directory entry that is not a folder. Previously load_if_toml_exists() swallowed every StreamlitSecretNotFoundError, the server came up, and a later os.environ lookup could fail with KeyError. After this change the process dies during middleware setup with an uncaught secrets error, and the "Failed to load secrets.toml file" log never runs. Auth and XSRF lookups, and BaseConnection._secrets, likewise raise where they used to fall back.

Security & Risk

No new dependency, injection, path traversal, or rendering surface. Parser diagnostics stay on the existing StreamlitSecretNotFoundError message (path plus tomllib line/column text). The tests check that a secret on a neighboring line is absent from that message.

The risk is availability. Session-middleware construction calls get_cookie_secret(), which now raises, so the process exits. When startup continues, a visible log is the right signal that [auth].cookie_secret and an [auth] section were not applied and that cookie and XSRF setup fell back to config. That log is what this PR is trying to add, and it does not run on the real startup path.

External test recommendation

• Recommend external_test: No
• Triggered categories: None
• Evidence:
  • lib/streamlit/runtime/secrets.py: distinguishes a missing secrets file from a parse or path error inside the in-process loader. The diff does not change routes, cookie or XSRF handlers, the websocket handshake, headers, storage, or embed behavior.
  • lib/streamlit/web/bootstrap.py: comment-only change around the existing startup try/except that logs a secrets load failure.
• Suggested external_test focus areas:
  • None. A malformed secrets.toml fails the same way locally and behind a proxy or iframe. Cover it with a unit test that builds session middleware.
• Confidence: High
• Assumptions and gaps: get_cookie_secret() and is_xsrf_enabled() call load_if_toml_exists(), so a malformed file changes those paths. gpt-5.6-terra-xhigh counted that as category 2 (auth, cookies, CSRF). The other two reviews did not. Category 2 is for login, OAuth, cookie, or CSRF handling that can diverge under proxies, third-party cookies, or embedding. This failure is an in-process exception during app construction and does not depend on those boundaries, so @pytest.mark.external_test would not catch a hosted-only divergence. If a follow-up changes how cookie or XSRF helpers behave on parse errors beyond logging and falling back, reassess.

Accessibility

No frontend or rendered-UI changes. Accessibility is not applicable.

Readability

gpt-5.6-terra-xhigh found no code or PR-description issues. The other two reviews flagged docstring and description gaps. Checked against the raise sites in _parse_file_path and _parse_directory, the docstring gap is real and non-blocking.

lib/streamlit/runtime/secrets.py

• load_if_toml_exists docstring (lines 177–181): The first sentence is clear. The rest says only a parse error is raised, and that False means no secrets file. The raise also covers invalid-secrets-path and secrets-directory-entry-not-folder. "The parse error is raised" hides the actor.

  Proposed rewrite:

  """Load secrets.toml files from disk if they exist.
  
  Returns False when no secrets file or directory is present. Raises
  ``StreamlitSecretNotFoundError`` when a configured path cannot be
  loaded: the TOML cannot be parsed, the path is neither a ``.toml``
  file nor a directory, or a directory entry is not a folder.
  
  Thread-safe.
  """

  Update this wording if the startup fix keeps the method lenient and moves the strict behavior elsewhere.

• _parse Raises section (lines 303–305): Same gap. The section says the exception means "no file" or "cannot be parsed," and omits the invalid-path and non-folder directory cases that raise the same type.

  Proposed rewrite:

  StreamlitSecretNotFoundError
      Raised when no secrets file exists, or when a configured path
      cannot be loaded (parse error, invalid path, or a directory
      entry that is not a folder).
  

PR title and description

• Title [fix] Surface malformed secrets.toml parse errors stands alone, matches [type] Description, and fits the length limit. No change.

• Description: The opening sentence states the change and why. The following failure-chain paragraph restates that mechanism before the bullets do it again. The bullet "Startup still logs and continues rather than aborting" describes prepare_streamlit_environment in isolation and overstates what streamlit run does until the startup caller is fixed.

  Proposed rewrite of the body (keep the testing plan and license footer), to use once the startup behavior matches it:

  ## Describe your changes
  
  Malformed `secrets.toml` is no longer treated as a missing file. Startup logs the path and TOML parser error, then continues, instead of leaving secrets unloaded so a later `os.environ` lookup raises `KeyError`.
  
  - A missing file (`error_id="no-secrets-found"`) still returns `False`. A file that exists but cannot be loaded is reported. Direct `st.secrets` access still raises `StreamlitSecretNotFoundError`, so existing handlers keep working.
  - Startup logs the failure and continues. A secrets load failure must not prevent the server from starting.
  - Validation reuses the existing `tomllib.loads` pass.

The comment at bootstrap.py:243 accurately describes prepare_streamlit_environment itself.

Recommendations

1. Keep a secrets load failure from aborting streamlit run and st.App, and from crashing the boolean callers (get_cookie_secret, is_xsrf_enabled, get_signing_secret, get_secrets_auth_section, BaseConnection._secrets). Preserve a single logged diagnostic and the existing fallbacks. The inline comment has the call order and the two ways to fix it.
2. Add a regression test that get_cookie_secret() or create_streamlit_middleware() / create_starlette_app() with malformed TOML succeeds, falls back to the configured cookie secret, and logs the parse error once.
3. Optional: extend the load_if_toml_exists and _parse docstrings as in Readability, and share one constant for "no-secrets-found" with the raise in _parse.
4. Optional: shorten the PR body as in Readability after the startup behavior matches that text.

Verdict

CHANGES REQUESTED: Re-raising non-missing secrets errors makes streamlit run and st.App exit during session-middleware setup, so the approved log-and-continue startup behavior never runs.

This is an automated AI review consolidated by grok-4.7-xhigh from claude-opus-5-5-medium, gpt-5.6-terra-xhigh, and grok-4.7-xhigh. All expected reviews completed. Please verify the feedback and use your judgment.

This review also includes 3 inline comment(s) on specific code lines.

Summary

All four expected reviews completed (gpt-5.6-sol-high, claude-opus-5-5-high, gemini-3.8-flash-high, grok-4.7-xhigh). All four approve.

This PR makes a malformed or otherwise unloadable secrets path visible at startup. load_if_toml_exists() stays a non-raising boolean probe, because session middleware calls it through get_cookie_secret() while the app is constructed, before prepare_streamlit_environment(). Startup calls secrets._parse(), logs every StreamlitSecretNotFoundError except error_id="no-secrets-found", and continues. Direct st.secrets access still raises StreamlitSecretNotFoundError. A follow-up commit restored the non-raising probe after an earlier version on the branch would have aborted middleware construction. The merged order matches the code: Server.start() builds middleware, then _on_server_start or the st.App lifespan prepares the environment.

Reviewers agree this is a small, correct bug fix. Missing files stay quiet, valid secrets are still copied into os.environ, the cookie-secret fallback is preserved, secret values are absent from the parser error, and unit tests cover the absent, malformed, valid, and middleware paths. An ordinary e2e test is unnecessary.

They differ on three non-blocking points, resolved in the sections below. Hosted @pytest.mark.external_test coverage is not required. The PR description overstates the fix: a later os.environ KeyError can still happen, and the new behavior is the log that explains it. A new public method on Secrets is unnecessary; calling _parse() is proportionate. The PR also carries the do-not-merge label, which is a process hold and is independent of this code assessment.

Product Alignment

The user-facing impact is a startup log. A configured secrets path that cannot be loaded now produces an error in the server log. There is no new public API, config option, or in-app UI. Bug fixes do not need a product spec (specs/AGENTS.md). The merged specs/2026-04-15-app-secrets-param/product-spec.md covers programmatic st.App secrets and does not describe malformed-file startup behavior. The PR body does not claim a separate product approval.

The implementation matches the behavior the branch actually ships:

• A missing file stays quiet, and load_if_toml_exists() still returns False.
• A parse error, invalid path, or non-folder secrets-directory entry is logged once, and the server still starts.
• st.secrets still raises StreamlitSecretNotFoundError with the path and parser error.

That matches principle 23 (fail helpfully) and principle 26 (existing apps keep working). Logging is the right channel: startup runs before the script, so an in-app st.error would not be visible (lib/streamlit/AGENTS.md). Callers that only check the boolean probe still treat a parse failure as unavailable, which keeps middleware construction going. The startup log is the diagnostic for those later failures.

The opening sentence of the PR description can be read as saying the os.environ KeyError no longer happens. Parsing still fails, secrets stay unloaded, and that lookup can still raise. The new behavior is the log. That is an editorial mismatch, covered under Readability, and it does not change the product assessment of the code.

Code Quality

The change is small and follows the startup order. Every reviewer found the complexity proportionate to the bug.

claude-opus-5-5-high preferred a small public method on Secrets so bootstrap would not call private _parse() or branch on the "no-secrets-found" telemetry slug, and so tests would not need "_parse" on Secrets.__setattr__. gemini-3.8-flash-high, gpt-5.6-sol-high, and grok-4.7-xhigh treat the private call and the error_id check as the smaller design. That is the right call. The slug is already the stable identifier for "nothing was configured," the probe contract stays in load_if_toml_exists(), and a new public method would add surface for a single logging call site. The "_parse" allowlist entry is optional cleanup and is called out inline; it matches the existing load_if_toml_exists test hook.

BootstrapSecretsLoadTest uses unittest.TestCase because it needs setUp/tearDown to restore os.environ, which matches lib/tests/AGENTS.md.

Performance

No meaningful impact. Secrets parsing runs at startup and is outside the backend hot paths in lib/streamlit/AGENTS.md. There is no frontend change. A valid file parsed during middleware setup is a cache hit in prepare_streamlit_environment(). A malformed file is read twice at startup because failures are not cached. That cost is one small TOML parse.

Test Coverage

Unit coverage matches the behavior change. Reviewers agree an ordinary e2e test is unnecessary: the user-visible result is a server log plus the existing st.secrets exception.

• secrets_test.py covers a missing file, a malformed file (load_if_toml_exists() returns False and os.environ is unchanged), and a parser error that includes the path and excludes a secret from a well-formed line.
• bootstrap_test.py covers a quiet missing file, a single error log that continues startup, valid scalars promoted to os.environ, and session middleware built on the config cookie secret before the startup log.
• server_util_test.py covers get_cookie_secret() falling back when the file is malformed.

claude-opus-5-5-high noted there is no test for the other non-missing ids (invalid-secrets-path, secrets-directory-entry-not-folder), which this branch also logs. That gap is optional. The malformed-TOML case already exercises the same branch (error_id != "no-secrets-found").

Backwards Compatibility

load_if_toml_exists() keeps its signature and its non-raising boolean contract. The docstring now describes that behavior. Valid and missing-file startups are unchanged. st.secrets still raises the same exception type.

The new startup error log is additive. Default secrets.files entries are secrets.toml paths from get_config_files; a missing default file still raises no-secrets-found and stays silent. A configured non-.toml path that is not a directory (invalid-secrets-path), or a file placed directly in a secrets directory, now logs at every startup. Previously that stayed silent until something accessed st.secrets. That is the intended diagnostic.

Assigning st.secrets._parse now succeeds because that name is on the internal allowlist, in the same set as load_if_toml_exists.

Security & Risk

Secrets handling is the sensitive area. Cookie-secret and XSRF decisions are unchanged: a malformed file still looks unavailable, so get_cookie_secret() falls back to server.cookieSecret, or to a generated key when that option is empty. tomllib.TOMLDecodeError text is positional (line and column) and does not include source lines. The unit test checks that a secret on a valid neighboring line is absent from the exception string and from exec_kwargs["error"]. The logged traceback is Streamlit frames plus that exception.

No new dependencies, routes, headers, eval, or subprocess calls.

External test recommendation

• Recommend external_test: No
• Triggered categories: None
• Evidence:
  • lib/streamlit/runtime/secrets.py: docstring and __setattr__ allowlist only. load_if_toml_exists() still catches StreamlitSecretNotFoundError and returns False. Cookie, XSRF, and auth callers of that probe are unchanged.
  • lib/streamlit/web/bootstrap.py: startup calls secrets._parse() and logs load failures other than no-secrets-found. No route, handshake, cookie, CORS, header, storage, or SiS API changes.
• Suggested external_test focus areas:
  • None. Unit tests cover the startup log and the cookie-secret fallback.
• Confidence: High
• Assumptions and gaps: Middleware construction still runs before prepare_streamlit_environment() on the CLI and st.App paths (confirmed in starlette_server.py and starlette_app.py). SiS directory secrets use the same parser; this PR does not branch on SiS.

gpt-5.6-sol-high recommended Yes under category 2 (auth, cookies, CSRF, and identity binding) because the tests exercise session-middleware construction and cookie-secret fallback, and suggested a hosted run with a malformed secrets.toml. The other three reviews recommended No. Category 2 applies when the diff touches login/logout, OAuth, _streamlit_xsrf, CSRF/XSRF handling, trusted user headers, or session-to-identity binding. This diff leaves those paths on the existing load_if_toml_exists() probe. That fallback does not depend on a proxy, iframe, or CSP boundary that would diverge from the unit tests. A hosted check would be defense in depth, and the checklist does not require it.

Accessibility

No frontend or UI change. Accessibility is not applicable.

Readability

Line-level comment, docstring, and test wording notes are in the inline comments.

PR title. [fix] Surface malformed secrets.toml parse errors matches [type] Description and is within 63 characters. It stands alone. claude-opus-5-5-high and grok-4.7-xhigh offered an optional tighter title that names the channel: [fix] Log malformed secrets.toml errors at startup. gpt-5.6-sol-high found the current title sufficient. Either title is acceptable.

PR description. The first sentence says a malformed file is no longer treated as missing, then "instead of leaving secrets unloaded so a later os.environ lookup raises KeyError." Parsing still fails, so secrets stay unloaded and that lookup can still raise. The new behavior is the log that explains the failure. Bullets 1 and 2 repeat the probe-versus-startup split. "Validation reuses the existing tomllib.loads pass" describes a step the diff does not add. gpt-5.6-sol-high accepted the description as written. gemini-3.8-flash-high proposed a rewrite that still says the change prevents the KeyError. The testing note should stay.

Proposed description:

## Describe your changes

Startup logs the path and parser error when a configured secrets path exists but cannot be loaded, then continues. A parse failure previously looked like a missing file, so secrets were never copied into `os.environ` and a later lookup raised `KeyError` with no parser diagnostic. Secrets remain unloaded in that case; the log explains the failure.

- Missing files stay quiet. `load_if_toml_exists()` still returns `False` and does not raise, so cookie-secret setup during app construction falls back and the server still starts. XSRF and auth use that same non-raising probe.
- Direct `st.secrets` access still raises `StreamlitSecretNotFoundError`.

## GitHub Issue Link (if applicable)

N/A — observed in a community forum report; present through 1.64.0 and `develop`.

## Testing Plan

- [x] Unit Tests (JS and/or Python)
- [ ] E2E Tests
- [ ] Any manual testing needed?

Python unit tests cover absent vs malformed vs valid secrets, secret-safe diagnostics, `os.environ` promotion during eager startup, and that `get_cookie_secret()` / session-middleware construction fall back instead of aborting.

lib/streamlit/web/bootstrap.py line 227 (docstring bullet outside the diff hunk). The bullet still says secrets load "if it exists." A file can exist and fail to parse.

Proposed rewrite:

- Loads secrets from secrets.toml when parsing succeeds, and logs a load failure without stopping startup

Recommendations

1. Update the PR description so it says a parse failure is logged and that secrets stay unloaded, so a later os.environ lookup can still raise KeyError.
2. Optionally update the prepare_streamlit_environment docstring bullet that still says secrets load "if it exists."
3. Optionally add one unit test that invalid-secrets-path (or a non-folder secrets-directory entry) logs once at startup.

Verdict

APPROVED: All four reviews approve, and the startup log is correct while the non-raising secrets probe, cookie-secret fallback, and st.secrets exception contract stay in place. Remaining notes are optional.

This is an automated AI review consolidated by grok-4.7-xhigh. Please verify the feedback and use your judgment.

This review also includes 7 inline comment(s) on specific code lines.

Auto-approved by AI final review (workflow run).