Harden OpenStream controls and polish Android UX#5
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f70aa26791
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val expectedToken = authTokenProvider()?.takeIf(::isValidAuthToken) ?: return false | ||
| val supplied = suppliedToken?.takeIf(::isValidAuthToken) ?: return false |
There was a problem hiding this comment.
When OBS chooses a discovered phone from the source properties, the phone has only advertised itself and has not opened an OBS pairing URL, so MainActivity.controlAuthToken is still null. This early return false makes the very first authenticated /reserve return 401, which breaks the documented OBS-selected-phone pairing path unless the user first taps the slot on the phone.
Useful? React with 👍 / 👎.
| while (response.size() < 8192) { | ||
| const int received = recv(sock, buffer, sizeof(buffer), 0); | ||
| if (received <= 0) break; | ||
| response.append(buffer, static_cast<size_t>(received)); | ||
| if (response.find("\r\n\r\n") != std::string::npos) break; |
There was a problem hiding this comment.
For a busy phone, /reserve returns HTTP 200 with {"ok":false} in the body, but this loop stops as soon as it sees the header terminator. If TCP delivers headers separately from the body, send_control_command() never sees "ok":false and OBS treats the rejected reservation as successful, then tries to connect to a phone that did not accept the slot.
Useful? React with 👍 / 👎.
| fun dispose() { | ||
| unregister() | ||
| executor.shutdownNow() |
There was a problem hiding this comment.
MainActivity posts a delayed appUpdater.checkForUpdates() during onCreate and never removes it. If the activity is destroyed within that delay, dispose() shuts the executor down here; when the pending UI-thread runnable later calls checkForUpdates(), executor.execute throws RejectedExecutionException, so quickly leaving the app can crash it.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
android/app/src/main/res/layout/activity_main.xml (1)
193-203: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
@dimen/os_control_btn_sizefor port button height.
btnPortDownandbtnPortUpuse hardcoded48dpfor both width and height, while all other converted buttons use@dimen/os_control_btn_sizefor height. Using the dimension resource for the height keeps touch-target sizing centralized if the value changes in the future.♻️ Optional refactor for port button height
<Button android:id="@+id/btnPortDown" android:layout_width="48dp" - android:layout_height="48dp" + android:layout_height="`@dimen/os_control_btn_size`" android:gravity="center"<Button android:id="@+id/btnPortUp" android:layout_width="48dp" - android:layout_height="48dp" + android:layout_height="`@dimen/os_control_btn_size`" android:gravity="center"Also applies to: 219-229
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/src/main/res/layout/activity_main.xml` around lines 193 - 203, Update the port control buttons in the activity_main layout so their height uses the shared dimension resource instead of a hardcoded value. In the btnPortDown and btnPortUp button definitions, replace the fixed 48dp height with `@dimen/os_control_btn_size` to keep sizing consistent with the other converted controls. Keep the existing width and other attributes unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/android.yml:
- Around line 18-19: The checkout step in the Android workflow should disable
persisted Git credentials to match the hardened setup used elsewhere. Update the
actions/checkout usage in the test job to set persist-credentials to false,
keeping the job’s pytest-only behavior intact while preventing unnecessary token
persistence.
In @.github/workflows/release.yml:
- Around line 77-89: The Android APK normalization step is vulnerable because
VERSION_NAME is interpolated directly into the shell script from
inputs.tag/github.ref_name, allowing command substitution. Move the tag/ref
value into the step’s env block and read it from a shell variable inside the
script, keeping the JSON generation logic in the Normalize Android APK artifact
step unchanged apart from sourcing VERSION_NAME safely.
In `@android/app/src/main/java/dev/openstream/app/update/AppUpdater.kt`:
- Around line 206-223: `hasExpectedApkDigest()` is doing full APK SHA-256 work
synchronously on the main thread, which can block UI during
`installDownloadedApk()`. Move the digest computation off the UI thread by
running the `MessageDigest`/stream read logic on `executor`, then post the
result back to the activity thread before continuing the install or failure
path. Keep the fix localized around `hasExpectedApkDigest()`,
`installDownloadedApk()`, `downloadReceiver.onReceive()`, and
`resumePendingInstallIfAllowed()`.
In `@docs/protocol.md`:
- Around line 24-26: The fenced protocol examples in the docs are missing
language tags, which triggers markdownlint warnings and inconsistent rendering.
Update the unlabeled code fences in the protocol examples to use a plain text
fence, and make the same change for each matching example in the protocol
documentation so the snippets remain readable without implying a programming
language.
---
Nitpick comments:
In `@android/app/src/main/res/layout/activity_main.xml`:
- Around line 193-203: Update the port control buttons in the activity_main
layout so their height uses the shared dimension resource instead of a hardcoded
value. In the btnPortDown and btnPortUp button definitions, replace the fixed
48dp height with `@dimen/os_control_btn_size` to keep sizing consistent with the
other converted controls. Keep the existing width and other attributes
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e0a96c5-3f6e-4516-b96e-b06e4fc09d0f
📒 Files selected for processing (16)
.github/workflows/android.yml.github/workflows/release.ymlandroid/app/src/main/AndroidManifest.xmlandroid/app/src/main/java/dev/openstream/app/MainActivity.ktandroid/app/src/main/java/dev/openstream/app/control/CameraControlServer.ktandroid/app/src/main/java/dev/openstream/app/update/AppUpdater.ktandroid/app/src/main/res/layout/activity_main.xmlandroid/app/src/main/res/values/colors.xmlandroid/app/src/main/res/values/dimens.xmlandroid/app/src/main/res/values/strings.xmldocs/protocol.mddocs/release-notes-template.mddocs/release.mdobs-plugin/src/openstream-source.cpptests/test_production_contracts.pytests/test_repo_contract.py
| - name: Check out repository | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set persist-credentials: false on this checkout.
The publish-android-update job already hardens its checkout (Line 99), but this test job leaves the default credential persistence enabled, which zizmor flags (artipacked). Since the job only runs pytest, disabling credential persistence is safe.
🔒 Proposed hardening
- name: Check out repository
uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Check out repository | |
| uses: actions/checkout@v4 | |
| - name: Check out repository | |
| uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 18-19: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/android.yml around lines 18 - 19, The checkout step in the
Android workflow should disable persisted Git credentials to match the hardened
setup used elsewhere. Update the actions/checkout usage in the test job to set
persist-credentials to false, keeping the job’s pytest-only behavior intact
while preventing unnecessary token persistence.
Source: Linters/SAST tools
| - name: Normalize Android APK artifact | ||
| run: | | ||
| mkdir -p dist | ||
| if [ -f android/app/build/outputs/apk/release/app-release.apk ]; then | ||
| cp android/app/build/outputs/apk/release/app-release.apk dist/openstream-android.apk | ||
| printf 'signed-release\n' > dist/android-signing.txt | ||
| else | ||
| cp android/app/build/outputs/apk/debug/app-debug.apk dist/openstream-android.apk | ||
| printf 'debug-signed-beta\n' > dist/android-signing.txt | ||
| fi | ||
| cp android/app/build/outputs/apk/release/app-release.apk dist/openstream-android.apk | ||
| sha256sum dist/openstream-android.apk > dist/openstream-android.apk.sha256 | ||
| VERSION_NAME="${{ inputs.tag || github.ref_name }}" | ||
| VERSION_NAME="${VERSION_NAME#v}" | ||
| printf '{\n "versionName": "%s",\n "versionCode": %s,\n "apkAsset": "openstream-android.apk"\n}\n' \ | ||
| APK_SHA256="$(cut -d ' ' -f 1 dist/openstream-android.apk.sha256)" | ||
| printf '{\n "versionName": "%s",\n "versionCode": %s,\n "apkAsset": "openstream-android.apk",\n "apkSha256": "%s"\n}\n' \ | ||
| "$VERSION_NAME" \ | ||
| "${{ steps.version.outputs.code }}" \ | ||
| "$APK_SHA256" \ | ||
| > dist/openstream-android-update.json |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant workflow section with line numbers.
file=".github/workflows/release.yml"
if [ -f "$file" ]; then
nl -ba "$file" | sed -n '70,95p'
else
echo "Missing file: $file"
fi
# Show surrounding references to inputs.tag and github.ref_name in the workflow.
rg -n 'inputs\.tag|github\.ref_name|steps\.version\.outputs\.code' .github/workflows/release.ymlRepository: YashasVM/OpenStream
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".github/workflows/release.yml"
echo '--- lines 70-95 ---'
sed -n '70,95p' "$file" | cat -n
echo
echo '--- occurrences ---'
rg -n 'inputs\.tag|github\.ref_name|steps\.version\.outputs\.code' "$file"Repository: YashasVM/OpenStream
Length of output: 1975
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".github/workflows/release.yml"
echo '--- lines 1-80 ---'
sed -n '1,80p' "$file" | cat -n
echo
echo '--- workflow triggers and inputs ---'
rg -n 'on:|workflow_dispatch|workflow_call|inputs:|tag:' "$file"Repository: YashasVM/OpenStream
Length of output: 3911
Template-injection risk in VERSION_NAME.
inputs.tag is a workflow_dispatch string input, and github.ref_name is expanded into the shell script before execution. A value containing $(...) would execute here. Pass it through env: and read it from a shell variable instead.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 82-82: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 82-82: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 87-87: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 77 - 89, The Android APK
normalization step is vulnerable because VERSION_NAME is interpolated directly
into the shell script from inputs.tag/github.ref_name, allowing command
substitution. Move the tag/ref value into the step’s env block and read it from
a shell variable inside the script, keeping the JSON generation logic in the
Normalize Android APK artifact step unchanged apart from sourcing VERSION_NAME
safely.
Source: Linters/SAST tools
| private fun hasExpectedApkDigest(): Boolean { | ||
| val expected = pendingRelease?.apkSha256 ?: return false | ||
| val apkUri = downloadManager.getUriForDownloadedFile(pendingDownloadId) ?: return false | ||
| val actual = runCatching { | ||
| activity.contentResolver.openInputStream(apkUri)?.use { input -> | ||
| val digest = MessageDigest.getInstance("SHA-256") | ||
| val buffer = ByteArray(DEFAULT_BUFFER_SIZE) | ||
| while (true) { | ||
| val read = input.read(buffer) | ||
| if (read < 0) break | ||
| digest.update(buffer, 0, read) | ||
| } | ||
| digest.digest().joinToString("") { byte -> "%02x".format(byte) } | ||
| } | ||
| }.getOrNull() | ||
| return actual != null && actual.equals(expected, ignoreCase = true) | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm installDownloadedApk call sites all run on the main thread.
rg -nP -C3 'installDownloadedApk\s*\(' android/app/src/main/java/dev/openstream/app/update/AppUpdater.ktRepository: YashasVM/OpenStream
Length of output: 851
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=android/app/src/main/java/dev/openstream/app/update/AppUpdater.kt
# Map the file structure first, then inspect the relevant sections.
ast-grep outline "$file" --view expanded
printf '\n--- relevant lines ---\n'
sed -n '1,260p' "$file" | nl -ba | sed -n '1,260p'
printf '\n--- executor / threading references ---\n'
rg -n 'executor|Handler|Looper|runOnUiThread|Coroutine|Dispatchers|onReceive|onResume|registerReceiver|installDownloadedApk|hasExpectedApkDigest' "$file"Repository: YashasVM/OpenStream
Length of output: 1530
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=android/app/src/main/java/dev/openstream/app/update/AppUpdater.kt
printf '%s\n' '--- install/download flow ---'
sed -n '31,110p' "$file" | cat -n
printf '%s\n' '--- install + digest implementation ---'
sed -n '146,230p' "$file" | cat -n
printf '%s\n' '--- registration / receiver setup ---'
sed -n '1,70p' "$file" | cat -nRepository: YashasVM/OpenStream
Length of output: 10985
Move APK digest hashing off the UI thread. installDownloadedApk() is reached from downloadReceiver.onReceive() and resumePendingInstallIfAllowed(), both on the main thread here. Hashing a full APK synchronously in hasExpectedApkDigest() can block the UI long enough to trigger an ANR; compute the digest on executor and post the install/failure path back to the activity thread.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@android/app/src/main/java/dev/openstream/app/update/AppUpdater.kt` around
lines 206 - 223, `hasExpectedApkDigest()` is doing full APK SHA-256 work
synchronously on the main thread, which can block UI during
`installDownloadedApk()`. Move the digest computation off the UI thread by
running the `MessageDigest`/stream read logic on `executor`, then post the
result back to the activity thread before continuing the install or failure
path. Keep the fix localized around `hasExpectedApkDigest()`,
`installDownloadedApk()`, `downloadReceiver.onReceive()`, and
`resumePendingInstallIfAllowed()`.
| ``` | ||
| srt://<obs-pc-ip>:9000?mode=caller&latency=120 | ||
| srt://<phone-ip>:9000?mode=caller&latency=120 | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to these fenced examples.
The unlabeled fences trigger the markdownlint warning and render less consistently. text is enough here.
Suggested fix
-```
+```text
srt://<phone-ip>:9000?mode=caller&latency=120@@
- +text
srt://0.0.0.0:9000?mode=listener&latency=120
@@
-```
+```text
openstream://connect?slotId=<slot-id>&slotLabel=<slot-label>&sourceInstanceId=<source-id>&controlToken=<64-hex-token>&host=<obs-ip>&port=<port>&latency=<ms>&name=...
</details>
Also applies to: 30-32, 136-138
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 24-24: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @docs/protocol.md around lines 24 - 26, The fenced protocol examples in the
docs are missing language tags, which triggers markdownlint warnings and
inconsistent rendering. Update the unlabeled code fences in the protocol
examples to use a plain text fence, and make the same change for each matching
example in the protocol documentation so the snippets remain readable without
implying a programming language.
</details>
<!-- fingerprinting:phantom:triton:quartz -->
<!-- cr-indicator-types:potential_issue -->
<!-- cr-comment:v1:e6428657747f766e1faae8a1 -->
_Source: Linters/SAST tools_
<!-- This is an auto-generated comment by CodeRabbit -->
# Conflicts: # android/app/src/main/java/dev/openstream/app/MainActivity.kt # android/app/src/main/res/layout/activity_main.xml # android/app/src/main/res/values/colors.xml # android/app/src/main/res/values/dimens.xml # android/app/src/main/res/values/strings.xml
1 participant
Summary
Validation
python -m pytest -q(30 passed):app:compileDebugKotlinwith Android Studio JBR 21git diff --check@codex Please review the control-token pairing flow, release-update verification, and Android UI/lifecycle integration.
Summary by CodeRabbit
New Features
Bug Fixes