[webkit-reviews] review granted: [Bug 192074] [GTK][WPE] Implement HSTS for the soup network backend : [Attachment 376885] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Claudio Saavedra
<csaavedra at igalia.com>'s request for review:
Bug 192074: [GTK][WPE] Implement HSTS for the soup network backend
https://bugs.webkit.org/show_bug.cgi?id=192074

Attachment 376885: Patch

https://bugs.webkit.org/attachment.cgi?id=376885&action=review




--- Comment #26 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 376885
  --> https://bugs.webkit.org/attachment.cgi?id=376885
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376885&action=review

> Source/WebCore/platform/network/soup/GUniquePtrSoup.h:36
> +WTF_DEFINE_GPTR_DELETER(SoupHSTSPolicy, soup_hsts_policy_free);

Remove the trailing ;

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:182
> +void SoupNetworkSession::setHSTSPersistentStorage(const CString& directory)
> +{
> +    hstsStorageDirectory() = directory;
> +}

The problem I see with this is that is a global setting, so it will be used for
ephemeral sessions too.

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:184
> +void SoupNetworkSession::setupHSTSEnforcer()

So, we could either receive here the session ID and check both
hstsStorageDirectory().isNull() || session.isEphemeral() to use a session
enforcer, or save the sessionID received in the constructor as a member to
check it here.

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:215
> +    for (GList* iter = domains.get(); iter != nullptr; iter = iter->next) {

iter != nullptr -> iter

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:217
> +	   hostNames.add(domain.get());

String::fromUTF8(domain.get())

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:230
> +    if (!soup_session_has_feature(m_soupSession.get(),
SOUP_TYPE_HSTS_ENFORCER))
> +	   return;
> +
> +    SoupHSTSEnforcer* enforcer =
SOUP_HSTS_ENFORCER(soup_session_get_feature(m_soupSession.get(),
SOUP_TYPE_HSTS_ENFORCER));

Do we need to check first? can't we just get the feature and null check to
return early?

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:231
> +    for (auto& hostName : hostNames) {

const auto&

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:232
> +	   GUniquePtr<SoupHSTSPolicy>
policy(soup_hsts_policy_new(hostName.utf8().data(),
SOUP_HSTS_POLICY_MAX_AGE_PAST, false));

FALSE

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:246
> +    if (!soup_session_has_feature(m_soupSession.get(),
SOUP_TYPE_HSTS_ENFORCER))
> +	   return;
> +
> +    SoupHSTSEnforcer* enforcer =
SOUP_HSTS_ENFORCER(soup_session_get_feature(m_soupSession.get(),
SOUP_TYPE_HSTS_ENFORCER));

SAme here about has + get

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:247
> +    GUniquePtr<GList> policies(soup_hsts_enforcer_get_policies(enforcer,
false));

FALSE

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:252
> +	       GUniquePtr<SoupHSTSPolicy>
newPolicy(soup_hsts_policy_new(policy.get()->domain,
SOUP_HSTS_POLICY_MAX_AGE_PAST, false));

FALSE

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.h:116
> +    bool shouldAllowHSTSPolicySetting();

const

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.h:117
> +    bool shouldAllowHSTSProtocolUpgrade();

const

> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:87
> +    PROP_HSTS_DIRECTORY,

I would call this PROP_HSTS_CACHE_DIRECTORY, and the same for the property name
and variable member.

> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:348
> +	* The directory where the HTTP Strict-Transport-Security (HSTS)
database will be stored.

s/database/cache/ the fact that the cache is a db is an impl detail.

> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:360
> +	       static_cast<GParamFlags>(WEBKIT_PARAM_READWRITE |
G_PARAM_CONSTRUCT_ONLY | G_PARAM_DEPRECATED)));

DEPRECATED? :-)

> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:645
> + * webkit_website_data_manager_get_hsts_directory:

cache_directory

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebsiteData.cpp:510
> +    // We wait 5 seconds before adding more entries so that we can test the
clear API with a time other than zero.
> +    sleep(5);

5 seconds is too long for a test. maybe it's fine to only test the clear api
with 0.