Skip to content

fix(streaming): let a container's broker_secret override STREAMING_BROKER_SECRET - #5259

Open
LoneAngelFayt wants to merge 2 commits into
rommapp:masterfrom
LoneAngelFayt:fix/streaming-container-secret-precedence
Open

LoneAngelFayt wants to merge 2 commits into
rommapp:masterfrom
LoneAngelFayt:fix/streaming-container-secret-precedence

Conversation

@LoneAngelFayt

@LoneAngelFayt LoneAngelFayt commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Description

STREAMING_BROKER_SECRET replaced every container's broker_secret from config.yml, so a pool of streaming containers with different BROKER_SECRETs couldn't work: RomM sent the env secret to all of them, and every broker that didn't share it answered 403 {"detail":"bad broker secret"}. I hit this adding a second webstation to a dev stack whose .env sets the env var. The per-container broker_secret was ignored and opening its desktop failed.

This swaps the precedence:

  • a container's own broker_secret wins
  • STREAMING_BROKER_SECRET is the fallback for containers that set none
  • a blank broker_secret: (YAML None) now falls back too, instead of becoming the literal secret "None" when no env var is set

env.template and examples/config.example.yml now describe the new order.

Upgrade note: an install that sets the env var and also has a stale broker_secret left in config.yml (e.g. change-me from the example) will start getting 403s, since that value used to be silently ignored. The fix is to remove or correct the config value.

Docs: rommapp/docs#180 updates the setup guide, troubleshooting page and configuration reference to the new order.

AI assistance: Claude Code wrote the change and tests under my direction; I reviewed the diff and reproduced the 403 against a real webstation container.

Checklist
Please check all that apply.

  • I've tested the changes locally
  • I've updated relevant comments
  • I've assigned reviewers for this PR
  • I've added unit tests that cover the changes

Screenshots (if applicable)

n/a, backend only.

…OKER_SECRET

STREAMING_BROKER_SECRET replaced every container's broker_secret, so a pool
of containers with different BROKER_SECRETs could not authenticate: each
broker after the first answered 403 "bad broker secret". A container's own
broker_secret now wins, and the env var is the fallback for containers that
set none. A blank broker_secret also falls back instead of becoming the
literal secret "None".
@greptile-apps

greptile-apps Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 4/5

[Medium impact] The change affects broker authentication within the streaming feature.

The code appears safe to merge, with non-blocking documentation and test coverage gaps.

Fix All in Claude CodeFindings

  1. P2 Published advice uses wrong secret ▶
  2. P2 Blank secret fix lacks coverage ▶
Fix with agent prompt
### Issue 1
backend/handler/streaming/config.py:445
The new order contradicts the published [streaming setup guide](https://github.com/rommapp/docs/blob/HEAD/docs/using/emulator-streaming.md#L178-L182), which says `STREAMING_BROKER_SECRET` always wins. The [troubleshooting page](https://github.com/rommapp/docs/blob/HEAD/docs/troubleshooting/emulator-streaming.md#L69-L71) repeats that advice. Someone fixing a rejected connection could keep changing the environment value while the container's `broker_secret` still overrides it. Update those pages and the configuration reference alongside this change.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

### Issue 2
backend/tests/endpoints/test_streaming.py:353-360
The fallback test always sets `STREAMING_BROKER_SECRET` to a nonempty value. The old code already passed all three fallback cases, so this does not protect the separate fix for blank YAML becoming the literal string `"None"` when the environment secret is unset. Add a case with `broker_secret=None` and `STREAMING_BROKER_SECRET=""`, and assert that the resolved secret is empty.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

Container-level broker_secret now overrides STREAMING_BROKER_SECRET. Missing, null, and empty values fall back to the environment secret.

  • Streaming containers use their own broker secret before the shared fallback.

LoneAngelFayt explicitly acknowledged that stale container secrets can cause 403 responses after upgrading. That intended behavior is not reported as a defect.

Reviews (1) · Last reviewed commit: "fix(streaming): let a container's broker..." · Reviewed by Greptile

Comment thread backend/handler/streaming/config.py
Comment thread backend/tests/endpoints/test_streaming.py
Addresses Greptile review on rommapp#5259: the fallback cases all set the env
secret, so none pinned that YAML null no longer becomes the secret "None".

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant