fix: auto-disable XDP for non-conforming apps#1664
Conversation
Disable XDP integration when the appid does not conform xdp id Signed-off-by: reddevillg <reddevillg@gmail.com>
deepin pr auto review你好！我是CodeGeeX。我已仔细审查了你提交的 整体来看，代码逻辑清晰，测试用例覆盖较为全面。但在语法逻辑、代码性能和代码安全方面，还有一些可以改进和优化的空间。以下是详细的审查意见： 1. 语法与逻辑问题：日志提示信息与代码逻辑不匹配
2. 代码性能问题：不必要的内存动态分配
3. 代码安全与健壮性问题：
4. 代码质量问题：头文件包含冗余
💡 改进后的代码建议基于以上分析，我为你重构了 /*
* SPDX-FileCopyrightText: 2026 UnionTech Software Technology Co., Ltd.
*
* SPDX-License-Identifier: LGPL-3.0-or-later
*/
#pragma once
#include <cctype>
#include <string_view>
namespace linglong::utils {
inline bool isValidXdgDesktopPortalId(std::string_view appid) noexcept
{
if (appid.empty()) {
return false;
}
bool has_dot = false;
bool segment_valid = false; // 标记当前分段是否至少有一个有效字符
bool is_last_segment = false;
for (size_t i = 0; i < appid.size(); ++i) {
char c = appid[i];
if (c == '.') {
if (!segment_valid) {
// 遇到 '.' 但前面分段为空 (例如 ".." 或 "org..app" 或 ".app")
return false;
}
has_dot = true;
segment_valid = false; // 重置，准备校验下一个分段
is_last_segment = false;
} else {
// 只要遇到了非 '.' 字符，说明进入了新的分段
if (!segment_valid) {
segment_valid = true;
// 判断是否是最后一个分段：后续字符中不再包含 '.'
is_last_segment = (appid.find('.', i) == std::string_view::npos);
}
bool char_valid = false;
if (std::isalnum(static_cast<unsigned char>(c)) || c == '_') {
char_valid = true;
} else if (c == '-' && is_last_segment) {
// 仅在最后一个分段允许连字符 '-'
char_valid = true;
}
if (!char_valid) {
return false;
}
}
}
// 必须至少包含一个 '.' (保证至少两个分段)，且最后一个分段不能为空
return has_dot && segment_valid;
}
} // namespace linglong::utils关于 CLI 逻辑的修改建议针对 const auto &appid = runContext.getTargetID();
if (!options.disableXdp.has_value() && !utils::isValidXdgDesktopPortalId(appid)) {
LogW("appid '{}' doesn't conform to XDP ID specification, XDP integration is auto-disabled. "
"Use --enable-xdp to override if you know what you are doing.",
appid);
runOptions.disableXdp = true;
}总结你的单元测试写得非常好，覆盖了各种边界情况（空串、单段、多段、特殊字符位置等）。在采用上述重构代码后，你可以直接运行原有的单元测试，它们应该全部通过。重构后的代码在性能上更优，且不依赖标准库容器的堆分配，更适合作为基础工具函数。 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a validation check for XDG Desktop Portal IDs to ensure compatibility before enabling XDP integration. It includes a new utility function isValidXdgDesktopPortalId, unit tests for various ID formats, and logic in the CLI to disable XDP with a warning if the ID is invalid. Feedback suggests optimizing the validation function to avoid heap allocations and locale-dependent checks, while also ensuring strict conformance to D-Bus specifications regarding segments starting with digits.
| #include <algorithm> | ||
| #include <cctype> | ||
| #include <string_view> | ||
| #include <vector> |
There was a problem hiding this comment.
If the optimized implementation of isValidXdgDesktopPortalId (suggested below) is adopted, the <algorithm>, <cctype>, and <vector> headers are no longer necessary, and only <string_view> is required.
| #include <algorithm> | |
| #include <cctype> | |
| #include <string_view> | |
| #include <vector> | |
| #include <string_view> |
| inline bool isValidXdgDesktopPortalId(std::string_view appid) noexcept | ||
| { | ||
| std::vector<std::string_view> segments; | ||
|
|
||
| size_t start = 0; | ||
| for (size_t i = 0; i < appid.size(); ++i) { | ||
| if (appid[i] == '.') { | ||
| segments.push_back(appid.substr(start, i - start)); | ||
| start = i + 1; | ||
| } | ||
| } | ||
| segments.push_back(appid.substr(start)); | ||
|
|
||
| if (segments.size() < 2) { | ||
| return false; | ||
| } | ||
|
|
||
| for (size_t i = 0; i + 1 < segments.size(); ++i) { | ||
| if (segments[i].empty()) { | ||
| return false; | ||
| } | ||
| if (!std::all_of(segments[i].begin(), segments[i].end(), [](char c) { | ||
| return std::isalnum(static_cast<unsigned char>(c)) || c == '_'; | ||
| })) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| const auto &last = segments.back(); | ||
| if (last.empty()) { | ||
| return false; | ||
| } | ||
| return std::all_of(last.begin(), last.end(), [](char c) { | ||
| return std::isalnum(static_cast<unsigned char>(c)) || c == '_' || c == '-'; | ||
| }); | ||
| } |
There was a problem hiding this comment.
The current implementation of isValidXdgDesktopPortalId is inefficient as it allocates a std::vector and performs multiple passes over the string. It can be implemented in a single pass without heap allocations.
Additionally, std::isalnum is locale-dependent. For protocol-level validation like D-Bus/XDP IDs, which are strictly ASCII, it is safer and more idiomatic to use explicit range checks.
Note: According to the D-Bus and Desktop Entry specifications, segments of a well-known name must not begin with a digit. The current implementation allows this (e.g., org.2example.app). If strict conformance is required, you should add a check to ensure segments do not start with a digit (and update the corresponding test case).
inline bool isValidXdgDesktopPortalId(std::string_view appid) noexcept
{
if (appid.empty()) {
return false;
}
size_t segmentCount = 0;
size_t start = 0;
for (size_t i = 0; i <= appid.size(); ++i) {
if (i == appid.size() || appid[i] == '.') {
std::string_view segment = appid.substr(start, i - start);
if (segment.empty()) {
return false;
}
bool isLast = (i == appid.size());
for (char c : segment) {
bool ok = (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || (c == '_');
if (isLast && c == '-') {
ok = true;
}
if (!ok) {
return false;
}
}
segmentCount++;
start = i + 1;
}
}
return segmentCount >= 2;
}
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds a small utility to validate whether an application ID conforms to the expected XDG desktop portal (XDP) ID format, and uses it in ll-cli run to automatically disable XDP integration for non-conforming app IDs unless the user explicitly overrides via CLI flags.
Changes:
- Added
linglong::utils::isValidXdgDesktopPortalId()helper inlibs/utils. - Added unit tests covering valid/invalid XDP IDs.
- Updated
ll-clirun path to auto-disable XDP when the resolved target ID is not a valid XDP ID (unless--enable-xdp/--disable-xdpwas explicitly set).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| libs/utils/src/linglong/utils/xdp.h | Introduces the XDP ID validation helper. |
| libs/utils/CMakeLists.txt | Adds the new header to the utils library sources list. |
| libs/linglong/tests/ll-tests/src/linglong/utils/xdp_test.cpp | Adds gtest coverage for XDP ID validation behavior. |
| libs/linglong/tests/ll-tests/CMakeLists.txt | Registers the new test file in the ll-tests target. |
| libs/linglong/src/linglong/cli/cli.cpp | Disables XDP integration by default when appid is invalid, with a warning and an override hint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, dengbo11, reddevillg The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Disable XDP integration when the appid does not conform xdp id