WebPlatformForEmbedded / WPEWebKit

WPE WebKit port (downstream)
210 stars 135 forks source link

Fix public/private strings in logs #1295

Closed asurdej-comcast closed 4 months ago

asurdej-comcast commented 4 months ago

Continuation of https://github.com/WebPlatformForEmbedded/WPEWebKit/pull/1284 as I can't push changes to that PR.

Initial commit contains exact changes from 1284

asurdej-comcast commented 4 months ago

Next patch will fix raw string literals R"(bla "bla" bla)", like:

-    LOCAL_LOG(R"({ "url": "%{public}s",)", escapedURL.utf8().data());
-    LOCAL_LOG(R"(  "partition": "%{public}s",)", "BLOCKED");
-    LOCAL_LOG(R"(  "hasStorageAccess": %{public}s,)", "false");
-    LOCAL_LOG(R"(  "referer": "%{public}s",)", escapedReferrer.utf8().data());
-    LOCAL_LOG(R"(  "isSameSite": "%{public}s",)", sameSiteInfo.isSameSite ? "true" : "false");
-    LOCAL_LOG(R"(  "isTopSite": "%{public}s",)", sameSiteInfo.isTopSite ? "true" : "false");
+    LOCAL_LOG(R"({ "url": "%)" PUBLIC_LOG_STRING R"(",)", escapedURL.utf8().data());
+    LOCAL_LOG(R"(  "partition": "%)" PUBLIC_LOG_STRING R"(",)", "BLOCKED");
+    LOCAL_LOG(R"(  "hasStorageAccess": %)" PUBLIC_LOG_STRING R"(,)", "false");
+    LOCAL_LOG(R"(  "referer": "%)" PUBLIC_LOG_STRING R"(",)", escapedReferrer.utf8().data());
+    LOCAL_LOG(R"(  "isSameSite": "%)" PUBLIC_LOG_STRING R"(",)", sameSiteInfo.isSameSite ? "true" : "false");
+    LOCAL_LOG(R"(  "isTopSite": "%)" PUBLIC_LOG_STRING R"(",)", sameSiteInfo.isTopSite ? "true" : "false");

It doesn't look particulary clean but using those macros itself introduce a lot of mess.

And add extra PUBLIC/PRIVATE_LOG_PREFIX that is used with non %s placeholders, like %d or %u

Also remove empty strings "" from logs format that was outcome of sed, like LOG("blabla: %" PUBLIC_LOG_STRING "", string);

asurdej-comcast commented 4 months ago

@magomez I can't push any changes to #1284 so created new PR with additional fixes. Fixed warnings "unknown conversion type character '"' in format" and also %{public}d or %{public}u

In general splitting every string with PUBLIC_LOG_STRING don't look particulary pretty but not sure if there is any other way

magomez commented 4 months ago

Awesome work @asurdej-comcast, thanks!!! There are still some minor warnings but they are not related to your patch, but arch issues, like using %lu for a size_t and such. I think this needs a different approach upstream, and we'll try to work on that for the next versions, but for this one it's a good solution. I'll use it for 2.42 as well.