fix(VideoManager): redact stream URLs in diagnostics

[alireza787b]

Description

Several QGC-owned video diagnostics can expose URL credentials or query and fragment secrets. This change centralizes a logging-safe URL representation while preserving the connection details needed for field debugging.

The helper:

• preserves scheme, host, port, and stream path;
• removes URL user information;
• preserves query keys while replacing every value with REDACTED;
• replaces fragment content with a visible REDACTED marker;
• preserves bare ports and strictly validated scheme-less hostname/IP endpoints, including bracketed IPv6 and ports 1-65535; and
• fails closed for empty, invalid, and credential-shaped malformed endpoints.

The helper is used by VideoSettings, VideoManager, VehicleCameraControl, the GStreamer source factory, and GstVideoReceiver. Automatic QGC CacheLocation pipeline snapshots retain topology, caps, media types, and states while omitting element property values that can contain source credentials. Native dumps explicitly enabled through GST_DEBUG_DUMP_DOT_DIR retain GStreamer's full SHOW_ALL detail for local debugging and are documented as sensitive. Bad operator-entered stream URLs are reported as warnings rather than critical failures.

This does not change stream configuration, receiver lifecycle, or video transport behavior.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change
• Documentation update
• Refactoring
• CI/Build changes
• Other

Testing

• Tested locally
• Added/updated unit tests
• Tested with simulator (not applicable)
• Tested with hardware (no runtime behavior change)

Local checks on current head c8ffeaf59337c8828f7a36c921ff8ac1d9ba66e5:

• a focused Qt probe covered bare ports, valid hostnames, IPv4, bracketed IPv6, user information, query values, fragments, credential-shaped malformed input, and port boundaries;
• focused GStreamer coverage verifies that automatic diagnostic DOT flags retain graph topology while omitting a secret element property;
• clang-format, whitespace/EOF hooks, and git diff --check passed for the review follow-up.

The same cases are covered by the dedicated no-I/O QGCNetworkRedactionTest registered as Unit/Utilities; GStreamerTest covers the automatic DOT-property boundary. Fresh exact-head CI is running after the review follow-up. The previous exact head completed QGC's Linux x64/arm64, unit/integration, sanitizer, coverage, Windows, macOS, iOS, Android, Docker, custom-plugin, and CodeQL checks.

Platforms Tested

• Linux (focused Qt/GStreamer probes)
• Windows (QGC CI build)
• macOS (QGC CI build)
• Android (QGC CI build)
• iOS (QGC CI build)

Flight Stacks Tested

Not applicable; this change does not enter a vehicle command path.

Screenshots

Not applicable; there is no UI change.

Checklist

• I have read the Contribution Guidelines
• I have read the Code of Conduct
• My code follows the project coding standards
• I have added tests that prove the fix
• New and existing unit and integration tests pass at the current exact head (running)

Related Issues

This is the first focused replacement for draft #13594. The HTTP MJPEG and WebSocket JPEG transports remain independent in #14730 and #14731.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the project's dual license (Apache 2.0 and GPL v3).

[codecov]

Codecov Report

❌ Patch coverage is 18.56287% with 136 lines in your changes missing coverage. Please review.
✅ Project coverage is 34.56%. Comparing base (f29efd3) to head (c8ffeaf).
⚠️ Report is 354 commits behind head on master.

Files with missing lines	Patch %	Lines
...anager/VideoReceiver/GStreamer/GstVideoReceiver.cc	0.00%	51 Missing ⚠️
src/Utilities/Network/QGCNetworkHelper.cc	37.17%	7 Missing and 42 partials ⚠️
src/Settings/VideoSettings.cc	12.50%	10 Missing and 4 partials ⚠️
...anager/VideoReceiver/GStreamer/GstSourceFactory.cc	0.00%	1 Missing and 8 partials ⚠️
src/VideoManager/VideoManager.cc	0.00%	8 Missing ⚠️
src/Camera/VehicleCameraControl.cc	0.00%	4 Missing ⚠️
...anager/VideoReceiver/GStreamer/GStreamerHelpers.cc	0.00%	0 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (18.56%) is below the target coverage (30.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14727      +/-   ##
==========================================
+ Coverage   25.47%   34.56%   +9.09%     
==========================================
  Files         769      945     +176     
  Lines       65912    85733   +19821     
  Branches    30495    39538    +9043     
==========================================
+ Hits        16788    29633   +12845     
+ Misses      37285    36377     -908     
- Partials    11839    19723    +7884     

Flag	Coverage Δ
unittests	34.56% <18.56%> (+9.09%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/Utilities/Network/QGCNetworkHelper.h	0.00% <ø> (ø)
...Manager/VideoReceiver/GStreamer/GstVideoReceiver.h	0.00% <ø> (ø)
...anager/VideoReceiver/GStreamer/GStreamerHelpers.cc	38.65% <0.00%> (+1.96%)	⬆️
src/Camera/VehicleCameraControl.cc	10.43% <0.00%> (+8.67%)	⬆️
src/VideoManager/VideoManager.cc	7.96% <0.00%> (+6.06%)	⬆️
...anager/VideoReceiver/GStreamer/GstSourceFactory.cc	31.28% <0.00%> (ø)
src/Settings/VideoSettings.cc	30.89% <12.50%> (+15.10%)	⬆️
src/Utilities/Network/QGCNetworkHelper.cc	8.96% <37.17%> (ø)
...anager/VideoReceiver/GStreamer/GstVideoReceiver.cc	5.30% <0.00%> (+2.40%)	⬆️

... and 654 files with indirect coverage changes

---

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8150b0...c8ffeaf. Read the comment docs.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Build Results

Platform Status

Platform	Status	Details
Linux	Passed	View
Windows	Passed	View
MacOS	Passed	View
Android	Passed	View

All builds passed.

Pre-commit

Check	Status	Details
pre-commit	Failed (non-blocking)	View

Pre-commit hooks: 2 passed, 76 failed, 7 skipped.

Test Results

linux-coverage-integration: 52 passed, 0 skipped
linux-coverage-unit: 186 passed, 0 skipped
linux-sanitizers-integration: 31 passed, 0 skipped
linux-sanitizers-unit: 186 passed, 0 skipped
Total: 455 passed, 0 skipped

Code Coverage

Coverage: 70.7%

No baseline available for comparison

Artifact Sizes

Artifact	Size
QGroundControl	253.11 MB
QGroundControl-aarch64	177.33 MB
QGroundControl-installer-AMD64	128.43 MB
QGroundControl-installer-AMD64-ARM64	70.82 MB
QGroundControl-installer-ARM64	99.55 MB
QGroundControl-linux	84.77 MB
QGroundControl-mac	84.77 MB
QGroundControl-x86_64	189.58 MB
No baseline available for comparison

---

_{Updated: 2026-09-03 04:45:08 UTC • Commit: c8ffeaf • Triggered by: Linux}

[alireza787b]

Coverage clarification for exact head 8e06c53cc: QGCNetworkRedactionTest is registered as a dedicated no-I/O Unit/Utilities test, and the exact-head Linux Test + Coverage workflow passed. The new GStreamer DOT-property regression also passed. Codecov reports 1% patch coverage because the changed lines are dominated by production logging call sites that the suite does not invoke (including 62 lines in GstVideoReceiver), not because the helper test is absent. Both codecov/patch and codecov/project concluded success, the project coverage threshold passed, and no coverage exclusions were added.

[DonLakeFlyer]

Review comments:

Design / debuggability

1. Over-redaction will hurt video debugging — the biggest concern. The helper strips path, query, and fragment, not just credentials. The RTSP path is the stream identity, not a secret in the common case: the #1 RTSP failure mode is a wrong mount point (/stream1 vs /axis-media/media.amp), and after this PR every such log reads rtsp://cam:554/<redacted>. "Invalid RTSP URI:" in GstSourceFactory.cc becomes almost useless — it says the URI is invalid while hiding which part is invalid. These are opt-in qCDebug categories that users enable precisely to produce bug-report logs; that's when you want fidelity. Recommend QUrl::RemoveUserInfo as the baseline (what the existing GStreamer code already did), optionally redacting query values only.

2. Concrete regression: <invalid-url> for the most common auto-stream case. In VideoManager::_updateAutoStream, pInfo->uri() for VIDEO_STREAM_TYPE_RTPUDP MAVLink cameras is frequently a bare port number (e.g. "5600") — see the QStringLiteral("udp://0.0.0.0:%1").arg(pInfo->uri()) fallbacks just below the log line. redactedUrlForLogging("5600") returns <invalid-url>, so the primary "Configure stream" diagnostic for PX4/ArduPilot camera auto-config is destroyed for the most common vehicle setup. Same problem for any scheme-less user-typed URL: the <invalid-url> sentinel hides exactly the malformed string needed to debug a parse failure.

3. VideoSettings changes remove the URL entirely. In the VideoSettings.cc hunks, "Testing configuration for UDP Stream:" loses its value completely. A UDP URL is 0.0.0.0:5600 — nothing secret — and the value is the whole point of the log (catching empty/typo'd config). At minimum log the redacted form, not nothing.

Missed coverage

4. src/Camera/VehicleCameraControl.cc lines 1820 and 1832 log raw pInfo->uri() ("Stopping stream:" / "Starting stream:") — the same URI the PR redacts in VideoManager::_updateAutoStream. If the goal is "QGC-owned URI logging," these are in scope and missed.

5. DOT pipeline dumps still leak credentials. GST_DEBUG_BIN_TO_DOT_FILE calls in GstVideoReceiver serialize element properties, including rtspsrc location=rtsp://user:pass@…. The PR body scopes out GStreamer-generated text, but the highest-fidelity credential leak path (the dumps users are told to attach for pipeline debugging) is untouched — the PR reduces, not eliminates, exposure.

Helper implementation

6. Silent query/fragment removal is asymmetric with path handling. Path redaction leaves a visible /<redacted> marker, but the test expects https://example.com/?token=abc123 → https://example.com/ — no indication anything was removed. A log reader can't distinguish "URL had no query" from "query was redacted," which itself misleads debugging. If query removal stays, add a marker.

7. Dead-store no-op in the helper: redactedUrl.setPath(hadPath ? QString() : redactedUrl.path()) — the else branch assigns the path to itself. Simpler: if (hadPath) { redactedUrl.setPath(QString()); }.

Log severity

8. qCCritical misuse on the touched lines (pre-existing). GstSourceFactory.cc lines 276, 316, 321, 339 use qCCritical for bad user-entered URLs while the identical failure class at line 508 ("Unsupported URI scheme:") uses qCWarning. Since the PR rewrites all four lines anyway, downgrading them to qCWarning here would be a zero-cost consistency fix (or explicitly defer as follow-up to keep scope tight).

GstVideoReceiver

9. _redactedUri() re-parses and redacts on every log call, including the watchdog and per-300-frames flow logs. Since _uri only changes in setUri(), caching the redacted string there would be cheaper and simpler at the call sites. Not blocking (qCDebug args aren't evaluated when the category is off).

Tests

10. Single monolithic test slot. QGCNetworkHelperTest follows one-slot-per-behavior; the new QGCNetworkRedactionTest packs 8+ distinct cases (credentials, encoded userinfo, IPv6, root-path query, invalid input…) into a single _testRedactedUrlForLogging slot, so a failure doesn't tell you which case broke.

11. Missing test cases: empty string, bare host:port without scheme (the comment-2 case — this should be a test, and it would expose the <invalid-url> regression), and the QUrl overload directly.

Summary

Comments 1–3 are the substantive objections (path/query stripping, <invalid-url> swallowing bare-port MAVLink URIs, VideoSettings logging nothing), and comment 4 shows the stated scope isn't fully covered. Suggest: redact userinfo (and at most query values), keep host/port/path, don't replace unparseable input with a sentinel without handling the bare-port case, and cover VehicleCameraControl.

[DonLakeFlyer]

Most of the code review is relevant with my main concern being redacting urls to the point of making remote debugging impossible or less useful.

[alireza787b]

Thanks for the detailed review. I addressed the substantive points in 2d149ee39 and rebased the branch onto current master.

• Stream paths are preserved for mount-point debugging.
• User information is removed; query keys remain visible with each value replaced by REDACTED; fragments retain a visible redaction marker.
• Bare ports and scheme-less host/port values remain visible. Empty and genuinely invalid inputs have distinct sentinels.
• VideoSettings values are present again, and VehicleCameraControl start/stop diagnostics use the same helper.
• QGC DOT dumps now retain topology, media type, caps details, and states while omitting element property values. A GStreamer regression test verifies that a secret source property is absent.
• The touched bad-input diagnostics are warnings rather than critical failures.
• Redaction coverage is split into focused slots in a dedicated no-I/O Unit/Utilities test and includes empty input, bare port, scheme-less host/port, and the QUrl overload.

I left the optional cached redacted-URI suggestion out to avoid adding synchronized state for disabled debug categories; it can remain a separate optimization if profiling ever justifies it. The rebased exact-head workflows are running now.

[alireza787b]

Small follow-up at a16dd0b46: while checking the CI label contract, I found that QGCNetworkHelperTest is labeled Network and is excluded by QGC's normal just test / check-ci path. I moved the same five focused, no-I/O redaction slots into a dedicated QGCNetworkRedactionTest registered as Unit/Utilities. This preserves the reviewed one-slot-per-behavior structure and makes the coverage executable in normal CI; production behavior is unchanged.

[alireza787b]

Exact-head CI caught one consistency gap from the severity change: the existing negative UDP/TCP source-factory tests still ignored these diagnostics as QtCriticalMsg, while the reviewed implementation now emits QtWarningMsg. The new URL-redaction and DOT-property tests themselves passed. Commit 8e06c53cc updates only those strict log expectations, and the refreshed workflow is running now. I propagated the same narrow test correction to #14730 and #14731.

[DonLakeFlyer]

Follow-up Review (Claude Fable, on behalf of @DonLakeFlyer)

Reviewed head 8e06c53 against the prior review round.

Status of prior review items

The rework genuinely addressed most of the earlier round: path preserved (item 1), VideoSettings logs the redacted value (3), VehicleCameraControl covered (4), DOT dumps redacted (5), visible REDACTED markers (6), dead store gone (7), qCCritical → qCWarning downgrade (8), one-slot-per-behavior tests (10), and most missing test cases (11). Declining the cached-URI optimization (9) with the stated rationale is fine — qCDebug args aren't evaluated when the category is off.

Remaining issues

1. <invalid-url> regression is only half-fixed — IP:port still gets destroyed (verified empirically). Probed with Qt 6.9:

"5600"              valid= true   → "5600"
"camera.local:5600" valid= true   → "camera.local:5600"
"0.0.0.0:5600"      valid= false  → <invalid-url>
"192.168.1.10:5600" valid= false  → <invalid-url>

QUrl rejects a relative URL whose first path segment contains : — unless the part before the colon happens to be a valid scheme name, which is why camera.local:5600 "works" (it parses as scheme camera.local, path 5600 — an accident, not a design). host:port with a numeric IP — arguably the most common MAVLink VIDEO_STREAM_INFORMATION URI form alongside bare ports — still logs as <invalid-url> in VideoManager::_updateAutoStream. _testHandlesRelativeAndInvalidInput tests camera.local:5600 but not 0.0.0.0:5600 / 192.168.1.10:5600; adding either would fail today. Suggested fix: before the sentinel, detect host:port shapes without @ (e.g. regex ^[^/@?#\s]+:\d{1,5}$) and return them as-is — the @-exclusion keeps user:pass@host inputs out of the passthrough.

2. <invalid-url> still hides the malformed input with no diagnostic breadcrumb. For genuinely unparseable input, consider appending QUrl::errorString() minus the source text, or at least the input length/scheme — "invalid" with zero characteristics makes a parse-failure bug report undebuggable. Lower priority than 1.

3. The developer opt-in DOT path lost SHOW_ALL too. GST_DEBUG_BIN_TO_DOT_FILE is a no-op unless the developer explicitly sets GST_DEBUG_DUMP_DOT_DIR; that's a deliberate local-debugging opt-in, and element properties (latency, buffer-mode, do-timestamp, caps-filter values) are often exactly what you need there. The credential-leak vector motivating this PR is the automatic writePipelineDot() CacheLocation fallback that ends up in field bug-report bundles. Consider: keep GST_DEBUG_GRAPH_SHOW_ALL for the env-var native dumps, and use kDiagnosticDotGraphDetails only in writePipelineDot(). As-is it's defensible (consistent), but it trades away developer debugging fidelity that nobody asked to protect.

4. Leftover include in QGCNetworkHelperTest.cc. The diff adds #include <QtCore/QUrlQuery> but no code in that file changed to need it — looks like a remnant from before the tests moved to QGCNetworkRedactionTest. Drop it.

5. Pre-commit reported "Failed (non-blocking)" with "0 passed, 0 failed, 0 skipped" in the build-results comment — the hook run appears not to have executed at all rather than passing. Worth a re-run before merge given the amount of touched formatting-sensitive code.

6. Minor, helper semantics: for a URL with a present-but-empty query (http://h/p?), hasQuery() is true with zero items and the code emits ?REDACTED — implying content existed where there was none. Harmless, but clearing the query in that case would be more truthful. Not blocking.

7. Tests are good overall — five focused slots, Unit/Utilities labels so they run in normal CI, and the DOT-property regression test with a planted secret is exactly the right shape. The only gap is the IP:port case from item 1.

Summary

The rework resolved the substantive objections from the first round except one: item 1 is a real, verified remaining regression for IP:port MAVLink stream URIs and should block merge; it's a small fix plus one test line. Items 2–4 are cheap improvements; 3 is a judgment call worth an explicit decision; 5 needs a CI re-check.

[DonLakeFlyer]

@alireza787b Can you take a look at latest review?

[alireza787b]

Addressed the follow-up review in 4e50c5a.

• Numeric IP:port values are preserved by both helper overloads, with regressions for 0.0.0.0:5600 and 192.168.1.10:5600.
• Truly invalid string input now retains a source-length breadcrumb without logging the source itself.
• A present-but-empty query is cleared instead of being shown as redacted content.
• Native dumps explicitly enabled with GST_DEBUG_DUMP_DOT_DIR again use GST_DEBUG_GRAPH_SHOW_ALL; automatic CacheLocation field-report dumps still omit credential-bearing element properties.
• Removed the stale QUrlQuery include.

The changed lines pass the exact clang-format hook in diff mode and a focused Qt URL-behavior probe. The push has started a fresh CI and pre-commit run.

[DonLakeFlyer]

So I was considering this for 5.1 but I think it is too invasive this late in the game to go into 5.1. Also would like bake time on it to make sure it doesn't degrade remote debugging capability.

[Copilot]

Pull request overview

Centralizes video URL redaction to prevent credentials and secrets appearing in diagnostics.

Changes:

• Adds and tests URL-redaction utilities.
• Applies redaction across video settings, management, camera, and GStreamer logs.
• Restricts diagnostic DOT graph details and adjusts invalid-URI severity.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/Utilities/Network/QGCNetworkHelper.h	Declares URL-redaction APIs.
src/Utilities/Network/QGCNetworkHelper.cc	Implements URL sanitization.
src/Settings/VideoSettings.cc	Redacts configured stream logs.
src/VideoManager/VideoManager.cc	Redacts receiver URI diagnostics.
src/Camera/VehicleCameraControl.cc	Redacts stream transition logs.
src/VideoManager/VideoReceiver/GStreamer/GstVideoReceiver.h	Declares receiver redaction helper.
src/VideoManager/VideoReceiver/GStreamer/GstVideoReceiver.cc	Redacts receiver lifecycle logs.
src/VideoManager/VideoReceiver/GStreamer/GstSourceFactory.cc	Redacts URI warnings and lowers severity.
src/VideoManager/VideoReceiver/GStreamer/GStreamerHelpers.h	Defines safe DOT graph flags.
src/VideoManager/VideoReceiver/GStreamer/GStreamerHelpers.cc	Uses restricted DOT details.
src/VideoManager/VideoReceiver/GStreamer/README.md	Documents safe graph output.
test/Utilities/Network/QGCNetworkRedactionTest.h	Declares redaction tests.
test/Utilities/Network/QGCNetworkRedactionTest.cc	Tests URL sanitization behavior.
test/Utilities/Network/CMakeLists.txt	Registers redaction tests.
test/VideoManager/GStreamer/GStreamerTest.h	Declares DOT safety test.
test/VideoManager/GStreamer/GStreamerTest.cc	Tests property omission.
test/VideoManager/GStreamer/SourceFactory/GStreamerSourceFactoryTest.cc	Updates expected warning levels.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[alireza787b]

Fixed in 65c2209. The shortcut now parses a strict authority, rejects userinfo/path/query/fragment, accepts only a valid IP address or DNS hostname, and enforces ports 1-65535. Regression coverage now includes credential-shaped malformed input, bracketed IPv6, and out-of-range ports.

[alireza787b]

I kept the two dump paths deliberately separate. Automatic CacheLocation snapshots use the restricted detail mask and omit element properties. Native GST_DEBUG_DUMP_DOT_DIR dumps are an explicit developer opt-in and retain SHOW_ALL, matching the earlier maintainer request to preserve full local diagnostic detail. 65c2209 updates the README to state that these opt-in native dumps can contain credentials and must be treated as sensitive, removing the previous blanket guarantee.