"onWatermarkStatusChanged" event is not fired when calling "org.rdk.C… - #232
Open
rekhap2kandhavelan wants to merge 2 commits into
Open
"onWatermarkStatusChanged" event is not fired when calling "org.rdk.C…#232rekhap2kandhavelan wants to merge 2 commits into
rekhap2kandhavelan wants to merge 2 commits into
Conversation
…ontentProtection.openDrmSession"api
|
I have read the CLA Document and I hereby sign the CLA suryaiyappan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
It introduces logging/printing of highly sensitive DRM materials (access tokens, license requests, full request payloads) and adds GLib debug output (g_print) in production paths.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aims to ensure watermark-related status events are delivered to the correct ContentSecurityManager instance by tracking “owned” DRM session IDs and filtering incoming watermark events to only those sessions.
Changes:
- Track DRM session IDs created by each
SecManagerThunder/ContentProtectionFireboltinstance and ignore watermark events for sessions not owned by that instance. - Add defensive checks to ignore watermark events that are missing
sessionId. - Add GLib usage (
g_print) and related includes (noted as problematic in review comments due to sensitive logging).
File summaries
| File | Description |
|---|---|
| externals/contentsecuritymanager/SecManagerThunder.h | Adds owned-session tracking members and isOwnedSession() declaration. |
| externals/contentsecuritymanager/SecManagerThunder.cpp | Records/clears owned session IDs and filters watermark event handlers by session ownership. |
| externals/contentsecuritymanager/IFirebolt/ContentProtectionFirebolt.h | Adds owned-session tracking members and isOwnedSession() declaration for Firebolt implementation. |
| externals/contentsecuritymanager/IFirebolt/ContentProtectionFirebolt.cpp | Tracks owned sessions and filters watermark events by ownership; adds additional logging/printing. |
Review details
Suppressed comments (2)
externals/contentsecuritymanager/SecManagerThunder.cpp:205
- These g_print statements log the raw licenseRequest/accessToken/contentMetadata, which can expose secrets/PII in logs; remove them (or gate behind a secure redaction mechanism).
g_print("surya_secman: licenseRequestStr: %s", licenseRequestStr.c_str());
g_print("surya_secman: accessTokenStr: %s", accessTokenStr.c_str());
g_print("surya_secman: contentMetaDataStr: %s", contentMetaDataStr.c_str());
externals/contentsecuritymanager/SecManagerThunder.cpp:213
- Logging the full SecManager request payload (initData) can leak sensitive fields (e.g., accessToken/licenseRequest); avoid emitting it at WARN level.
std::string initData = param.print_UnFormatted();
MW_LOG_WARN("surya_secman: SecManager %s param: %s",apiName, initData.c_str());
g_print("surya_secman: initData: %s", initData.c_str());
- Files reviewed: 4/4 changed files
- Comments generated: 7
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+288
to
+290
| g_print("surya: licenseRequestStr: %s", licenseRequestStr.c_str()); | ||
| g_print("surya: accessTokenStr: %s", accessTokenStr.c_str()); | ||
| g_print("surya: contentMetaDataStr: %s", contentMetaDataStr.c_str()); |
Comment on lines
296
to
+298
| std::string initData = param.print_UnFormatted(); | ||
| MW_LOG_WARN("ContentProtection %s param: %s",apiName, initData.c_str()); | ||
| g_print("surya: initData: %s", initData.c_str()); |
Comment on lines
+224
to
+225
| MW_LOG_INFO("surya: AcquireLicenseOpenOrUpdate called with clientId: %s, appId: %s", clientId.c_str(), appId.c_str()); | ||
| g_print("surya: AcquireLicenseOpenOrUpdate called with clientId: %s, appId: %s", clientId.c_str(), appId.c_str()); |
Comment on lines
+35
to
+37
| #include <set> | ||
| #include <memory> | ||
| #include <glib.h> |
Comment on lines
189
to
196
| void HandleWatermarkEvent(const std::string& sessionId, const std::string& statusStr, const std::string& appId); | ||
| /** | ||
| * @brief Check if a DRM session belongs to this ContentProtectionFirebolt instance | ||
| * @param sessionId DRM session ID to validate | ||
| * @return true when the session is owned by this instance | ||
| */ | ||
| bool isOwnedSession(int64_t sessionId); | ||
| private: |
Comment on lines
+140
to
+141
| MW_LOG_INFO("surya: AcquireLicenseOpenOrUpdate called with clientId: %s, appId: %s", clientId.c_str(), appId.c_str()); | ||
| g_print("surya_secman: AcquireLicenseOpenOrUpdate called with clientId: %s, appId: %s", clientId.c_str(), appId.c_str()); |
Comment on lines
+38
to
+40
| #include <set> | ||
| #include <vector> | ||
| #include <glib.h> |
for watermark 69591 ticket
suryaiyappan2k
force-pushed
the
feature/watermark-69591
branch
from
September 10, 2026 17:53
faf8e98 to
8384bfa
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The newly added stdout/log statements expose sensitive DRM payloads (e.g., access tokens/license requests) and should be removed or fully redacted before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (6)
externals/contentsecuritymanager/SecManagerThunder.h:40
<glib.h>is included in this public header but no GLib types/APIs are used here; this adds an unnecessary dependency and increases compile surface for all includers.
#include <set>
#include <vector>
#include <glib.h>
externals/contentsecuritymanager/IFirebolt/ContentProtectionFirebolt.h:37
<glib.h>is included in this header but no GLib types/APIs are used; please drop it to avoid pulling GLib into every translation unit that includes this header.
#include <set>
#include <memory>
#include <glib.h>
externals/contentsecuritymanager/IFirebolt/ContentProtectionFirebolt.cpp:290
- These
g_printstatements output the license request, access token, and content metadata to stdout, which can expose secrets/PII. Remove them or log only non-sensitive metadata (e.g., payload lengths).
g_print("surya: licenseRequestStr: %s", licenseRequestStr.c_str());
g_print("surya: accessTokenStr: %s", accessTokenStr.c_str());
g_print("surya: contentMetaDataStr: %s", contentMetaDataStr.c_str());
externals/contentsecuritymanager/SecManagerThunder.cpp:141
- Avoid printing potentially sensitive identifiers via ad-hoc debug tags and stdout;
g_printalso bypasses the project logging system. If a log is needed, keep it generic and use the existing logger.
MW_LOG_INFO("surya: AcquireLicenseOpenOrUpdate called with clientId: %s, appId: %s", clientId.c_str(), appId.c_str());
g_print("surya_secman: AcquireLicenseOpenOrUpdate called with clientId: %s, appId: %s", clientId.c_str(), appId.c_str());
externals/contentsecuritymanager/IFirebolt/ContentProtectionFirebolt.cpp:225
- Avoid ad-hoc debug tags and stdout printing in production code; keep logs within the project logging system and don’t include personal prefixes.
MW_LOG_INFO("surya: AcquireLicenseOpenOrUpdate called with clientId: %s, appId: %s", clientId.c_str(), appId.c_str());
g_print("surya: AcquireLicenseOpenOrUpdate called with clientId: %s, appId: %s", clientId.c_str(), appId.c_str());
externals/contentsecuritymanager/IFirebolt/ContentProtectionFirebolt.cpp:298
initDatacontains accessToken/contentMetadata and should not be logged verbatim at WARN level (or printed to stdout). Log only length/diagnostic fields or guard behind a secure debug flag with redaction.
std::string initData = param.print_UnFormatted();
MW_LOG_WARN("ContentProtection %s param: %s",apiName, initData.c_str());
g_print("surya: initData: %s", initData.c_str());
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
Comment on lines
+203
to
+205
| g_print("surya_secman: licenseRequestStr: %s", licenseRequestStr.c_str()); | ||
| g_print("surya_secman: accessTokenStr: %s", accessTokenStr.c_str()); | ||
| g_print("surya_secman: contentMetaDataStr: %s", contentMetaDataStr.c_str()); |
| } | ||
|
|
||
| ContentProtectionFirebolt::ContentProtectionFirebolt() : mInitialized(false), mSpeedStateMutex(), mContentProtectionMutex(), mFireboltInitMutex() | ||
| ContentProtectionFirebolt::ContentProtectionFirebolt() : mInitialized(false), mSpeedStateMutex(), mContentProtectionMutex(), mFireboltInitMutex(), mOwnedSessionsMutex(), mOwnedSessions() |
Comment on lines
39
to
+40
| SecManagerThunder::SecManagerThunder() : mSecManagerObj(SECMANAGER_CALL_SIGN), mSecMutex(), mSchedulerStarted(false), | ||
| mRegisteredEvents(), mWatermarkPluginObj(WATERMARK_PLUGIN_CALLSIGN), mWatMutex(), mSpeedStateMutex() | ||
| mRegisteredEvents(), mWatermarkPluginObj(WATERMARK_PLUGIN_CALLSIGN), mWatMutex(), mSpeedStateMutex(), mOwnedSessionsMutex(), mOwnedSessions() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.