Skip to content

[Push AV]: Add url validation as per RFC3986 #41502

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[Push AV]: Add url validation as per RFC3986 #41502

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c0610c6
Add url validation as per rfc3986
sayondeep Oct 17, 2025
911d797
Add uriparser as third_party dependency
sayondeep Oct 17, 2025
e937127
Build uriparser portably
sayondeep Oct 21, 2025
bd0dd1a
Fix uriparser build for cmake
sayondeep Nov 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions integrations/docker/images/base/chip-build/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ RUN set -x \
libssl-dev \
libtool \
libudev-dev \
liburiparser-dev \
libusb-1.0-0 \
libusb-dev \
libxml2-dev \
Expand Down
1 change: 1 addition & 0 deletions integrations/docker/images/chip-cert-bins/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ RUN set -x \
libssl-dev \
libtool \
libudev-dev \
liburiparser-dev \
libusb-1.0-0 \
libusb-dev \
libxml2-dev \
Expand Down
5 changes: 5 additions & 0 deletions src/app/clusters/push-av-stream-transport-server/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@
import("//build_overrides/build.gni")
import("//build_overrides/chip.gni")

config("uriparser_config") {
libs = [ "uriparser" ]
}

source_set("push-av-stream-transport-server") {
sources = [
"push-av-stream-transport-cluster.cpp",
Expand All @@ -33,4 +37,5 @@ source_set("push-av-stream-transport-server") {
"${chip_root}/zzz_generated/app-common/clusters/PushAvStreamTransport",
]
public_configs = [ "${chip_root}/src:includes" ]
configs += [ ":uriparser_config" ]
}
37 changes: 33 additions & 4 deletions src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-logic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <lib/support/DefaultStorageKeyAllocator.h>
#include <protocols/interaction_model/StatusCode.h>
#include <set>
#include <uriparser/Uri.h>

static constexpr uint16_t kMaxConnectionId = 65535; // This is also invalid connectionID
static constexpr uint16_t kMaxEndpointId = 65534;
Expand All @@ -43,6 +44,34 @@ using namespace chip::app::Clusters::PushAvStreamTransport::Attributes;
using namespace Protocols::InteractionModel;
using chip::Protocols::InteractionModel::Status;

namespace {
bool hasHttpsScheme(const UriUriA & uri)
{
if (!uri.scheme.first || !uri.scheme.afterLast)
return false;

std::string scheme(uri.scheme.first, uri.scheme.afterLast);
return scheme == "https";
}
Comment on lines +48 to +55
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

According to RFC 3986, the scheme component is case-insensitive. The current implementation performs a case-sensitive comparison with "https". This should be changed to a case-insensitive comparison.

Additionally, creating a std::string for comparison is inefficient as it can cause heap allocation. A direct character-by-character comparison can avoid this.

You will need to include <cctype> for the tolower function.

bool hasHttpsScheme(const UriUriA & uri)
{
    if (!uri.scheme.first || !uri.scheme.afterLast)
    {
        return false;
    }

    const char * scheme_start = uri.scheme.first;
    const size_t scheme_len   = static_cast<size_t>(uri.scheme.afterLast - uri.scheme.first);
    constexpr char https_scheme[] = "https";
    constexpr size_t https_scheme_len = sizeof(https_scheme) - 1;

    if (scheme_len != https_scheme_len)
    {
        return false;
    }

    for (size_t i = 0; i < https_scheme_len; ++i)
    {
        if (tolower(scheme_start[i]) != https_scheme[i])
        {
            return false;
        }
    }

    return true;
}

Copy link
Contributor

@tcarmelveilleux tcarmelveilleux Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code from Gemini above is cruddy, but the comment is correct.

Please:

  • Create a new src/lib/support/StringComparison.h and associated unit test
  • Have a new method for string view case insensitive compare:
#include <string_view>
#include <cctype>
#include <algorithm>

//// NAMESPACE IT properly

bool CaseInsensitiveCompare(std::string_view left, std::string_view right) {
    if (left.length() != right.length()) {
        return false; // Lengths differ, so they cannot be equal case-insensitively
    }

    for (size_t i = 0; i < left.length(); ++i) {
        // Convert characters to lowercase for comparison
        if (std::tolower(static_cast<unsigned char>(left[i])) !=
            std::tolower(static_cast<unsigned char>(right[i]))) {
            return false; // Characters differ after case conversion
        }
    }
    return true; // All characters matched after case conversion
}


bool isValidRFC3986Uri(const std::string & uriStr)
{
UriUriA uri;
const char * errorPos;

int result = uriParseSingleUriA(&uri, uriStr.c_str(), &errorPos);
if (result != URI_SUCCESS)
{
return false;
}

bool isHttps = hasHttpsScheme(uri);

uriFreeUriMembersA(&uri);
return isHttps;
}
Comment on lines +57 to +72
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To make the URL validation more efficient, you can extend this function to also check for the presence of a query or fragment, since uriparser already provides this information after parsing. This avoids scanning the URL string again in ValidateUrl. The checks in ValidateUrl for '?' and '#' would then become redundant and could be removed in a follow-up change.

bool isValidRFC3986Uri(const std::string & uriStr)
{
    UriUriA uri;
    const char * errorPos;

    if (uriParseSingleUriA(&uri, uriStr.c_str(), &errorPos) != URI_SUCCESS)
    {
        return false;
    }

    bool isHttps     = hasHttpsScheme(uri);
    bool hasQuery    = (uri.query.first != nullptr);
    bool hasFragment = (uri.fragment.first != nullptr);

    uriFreeUriMembersA(&uri);
    return isHttps && !hasQuery && !hasFragment;
}

Copy link
Contributor

@bzbarsky-apple bzbarsky-apple Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validation has to happen for all URIs, but the query/fragment bits only need to happen for CMAF URIs. So the bot's advice here would only work if this function had a way to configure whether query/fragment are allowed.

} // namespace

namespace chip {
namespace app {
namespace Clusters {
Expand Down Expand Up @@ -234,14 +263,14 @@ void PushAvStreamTransportServerLogic::PushAVStreamTransportDeallocateCallback(S

bool PushAvStreamTransportServerLogic::ValidateUrl(const std::string & url)
{
const std::string https = "https://";

// Check minimum length and https prefix
if (url.size() <= https.size() || url.substr(0, https.size()) != https)
if (!isValidRFC3986Uri(url))
Copy link
Contributor

@vnkavali vnkavali Oct 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

11.7.6.13. TransportOptionsStruct Type
11.7.6.13.5. URL Field
This field SHALL be a valid string in RFC 3986 format representing the upload location. The field
SHALL use the https scheme which will be validated by the underlying TLSEndpointID.

These RFC Checks are for all Transports, while the following invalid character checks are CMAF-specific in the spec.
More details at https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/12442 and Full discussion at #41321 (comment).

Also, heads up - #41321 addresses other issues in this API and also call this only when ContainerType is CMAF. So would need to rebase to it once it's merged.

{
ChipLogError(Camera, "URL is not a valid RFC3986 URI: %s", url.c_str());
return false;
}

const std::string https = "https://";

// Check that URL does not contain fragment character '#'
if (url.find('#') != std::string::npos)
{
Expand Down
Loading