ma1 pushed to branch tor-browser-128.3.0esr-14.0-1 at The Tor Project / Applications / Tor Browser

Commits: 42b4d421 by rahulsainani at 2024-10-01T17:58:48+02:00 Bug 1906024 - Format download file names better a=diannaS

Original Revision: https://phabricator.services.mozilla.com/D220559

Differential Revision: https://phabricator.services.mozilla.com/D222254

- - - - - 5928c112 by rahulsainani at 2024-10-01T17:58:55+02:00 Bug 1906024 - Format download file names a=diannaS

Original Revision: https://phabricator.services.mozilla.com/D221771

Differential Revision: https://phabricator.services.mozilla.com/D222259 - - - - - e139ad27 by Nika Layzell at 2024-10-01T18:01:23+02:00 Bug 1911745 - Unify BrowsingContext flag coherency checks, r=mccr8

Previously these checks were largely diagnostic tools for finding bugs in other code as it evolves. This unifies the checks a bit more and makes them stronger for BrowsingContexts created over IPC, providing a place for more coherency checks to be added in the future.

Differential Revision: https://phabricator.services.mozilla.com/D218860 - - - - -

4 changed files:

- docshell/base/BrowsingContext.cpp - docshell/base/BrowsingContext.h - mobile/android/android-components/components/support/ktx/src/main/java/mozilla/components/support/ktx/kotlin/String.kt - mobile/android/android-components/components/support/ktx/src/test/java/mozilla/components/support/ktx/kotlin/StringTest.kt

Changes:

===================================== docshell/base/BrowsingContext.cpp ===================================== @@ -578,9 +578,20 @@ mozilla::ipc::IPCResult BrowsingContext::CreateFromIPC( context->mRequestContextId = aInit.mRequestContextId; // NOTE: Private browsing ID is set by `SetOriginAttributes`.

+ if (const char* failure = + context->BrowsingContextCoherencyChecks(aOriginProcess)) { + mozilla::ipc::IProtocol* actor = aOriginProcess; + if (!actor) { + actor = ContentChild::GetSingleton(); + } + return IPC_FAIL_UNSAFE_PRINTF(actor, "Incoherent BrowsingContext: %s", + failure); + } + Register(context);

- return context->Attach(/* aFromIPC */ true, aOriginProcess); + context->Attach(/* aFromIPC */ true, aOriginProcess); + return IPC_OK(); }

BrowsingContext::BrowsingContext(WindowContext* aParentWindow, @@ -795,8 +806,64 @@ void BrowsingContext::Embed() { } }

-mozilla::ipc::IPCResult BrowsingContext::Attach(bool aFromIPC, - ContentParent* aOriginProcess) { +const char* BrowsingContext::BrowsingContextCoherencyChecks( + ContentParent* aOriginProcess) { +#define COHERENCY_ASSERT(condition) \ + if (!(condition)) return "Assertion " #condition " failed"; + + if (mGroup->IsPotentiallyCrossOriginIsolated() != + (Top()->GetOpenerPolicy() == + nsILoadInfo::OPENER_POLICY_SAME_ORIGIN_EMBEDDER_POLICY_REQUIRE_CORP)) { + return "Invalid CrossOriginIsolated state"; + } + + if (aOriginProcess && !IsContent()) { + return "Content cannot create chrome BCs"; + } + + // LoadContext should generally match our opener or parent. + if (IsContent()) { + if (RefPtr<BrowsingContext> opener = GetOpener()) { + COHERENCY_ASSERT(opener->mType == mType); + COHERENCY_ASSERT(opener->mGroup == mGroup); + COHERENCY_ASSERT(opener->mUseRemoteTabs == mUseRemoteTabs); + COHERENCY_ASSERT(opener->mUseRemoteSubframes == mUseRemoteSubframes); + COHERENCY_ASSERT(opener->mPrivateBrowsingId == mPrivateBrowsingId); + COHERENCY_ASSERT( + opener->mOriginAttributes.EqualsIgnoringFPD(mOriginAttributes)); + } + } + if (RefPtr<BrowsingContext> parent = GetParent()) { + COHERENCY_ASSERT(parent->mType == mType); + COHERENCY_ASSERT(parent->mGroup == mGroup); + COHERENCY_ASSERT(parent->mUseRemoteTabs == mUseRemoteTabs); + COHERENCY_ASSERT(parent->mUseRemoteSubframes == mUseRemoteSubframes); + COHERENCY_ASSERT(parent->mPrivateBrowsingId == mPrivateBrowsingId); + COHERENCY_ASSERT( + parent->mOriginAttributes.EqualsIgnoringFPD(mOriginAttributes)); + } + + // UseRemoteSubframes and UseRemoteTabs must match. + if (mUseRemoteSubframes && !mUseRemoteTabs) { + return "Cannot set useRemoteSubframes without also setting useRemoteTabs"; + } + + // Double-check OriginAttributes/Private Browsing + // Chrome browsing contexts must not have a private browsing OriginAttribute + // Content browsing contexts must maintain the equality: + // mOriginAttributes.mPrivateBrowsingId == mPrivateBrowsingId + if (IsChrome()) { + COHERENCY_ASSERT(mOriginAttributes.mPrivateBrowsingId == 0); + } else { + COHERENCY_ASSERT(mOriginAttributes.mPrivateBrowsingId == + mPrivateBrowsingId); + } +#undef COHERENCY_ASSERT + + return nullptr; +} + +void BrowsingContext::Attach(bool aFromIPC, ContentParent* aOriginProcess) { MOZ_DIAGNOSTIC_ASSERT(!mEverAttached); MOZ_DIAGNOSTIC_ASSERT_IF(aFromIPC, aOriginProcess || XRE_IsContentProcess()); mEverAttached = true; @@ -815,25 +882,15 @@ mozilla::ipc::IPCResult BrowsingContext::Attach(bool aFromIPC, MOZ_DIAGNOSTIC_ASSERT(mGroup); MOZ_DIAGNOSTIC_ASSERT(!mIsDiscarded);

- if (mGroup->IsPotentiallyCrossOriginIsolated() != - (Top()->GetOpenerPolicy() == - nsILoadInfo::OPENER_POLICY_SAME_ORIGIN_EMBEDDER_POLICY_REQUIRE_CORP)) { - MOZ_DIAGNOSTIC_ASSERT(aFromIPC); - if (aFromIPC) { - auto* actor = aOriginProcess - ? static_castmozilla::ipc::IProtocol*(aOriginProcess) - : static_castmozilla::ipc::IProtocol*( - ContentChild::GetSingleton()); - return IPC_FAIL( - actor, - "Invalid CrossOriginIsolated state in BrowsingContext::Attach call"); - } else { - MOZ_CRASH( - "Invalid CrossOriginIsolated state in BrowsingContext::Attach call"); +#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED + // We'll already have checked this if `aFromIPC` is set before calling this + // function. + if (!aFromIPC) { + if (const char* failure = BrowsingContextCoherencyChecks(aOriginProcess)) { + MOZ_CRASH_UNSAFE_PRINTF("Incoherent BrowsingContext: %s", failure); } } - - AssertCoherentLoadContext(); +#endif

// Add ourselves either to our parent or BrowsingContextGroup's child list. // Important: We shouldn't return IPC_FAIL after this point, since the @@ -915,7 +972,6 @@ mozilla::ipc::IPCResult BrowsingContext::Attach(bool aFromIPC, if (XRE_IsParentProcess()) { Canonical()->CanonicalAttach(); } - return IPC_OK(); }

void BrowsingContext::Detach(bool aFromIPC) { @@ -1768,40 +1824,6 @@ nsresult BrowsingContext::SetOriginAttributes(const OriginAttributes& aAttrs) { return NS_OK; }

-void BrowsingContext::AssertCoherentLoadContext() { -#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED - // LoadContext should generally match our opener or parent. - if (IsContent()) { - if (RefPtr<BrowsingContext> opener = GetOpener()) { - MOZ_DIAGNOSTIC_ASSERT(opener->mType == mType); - MOZ_DIAGNOSTIC_ASSERT(opener->mGroup == mGroup); - MOZ_DIAGNOSTIC_ASSERT(opener->mUseRemoteTabs == mUseRemoteTabs); - MOZ_DIAGNOSTIC_ASSERT(opener->mUseRemoteSubframes == mUseRemoteSubframes); - MOZ_DIAGNOSTIC_ASSERT(opener->mPrivateBrowsingId == mPrivateBrowsingId); - MOZ_DIAGNOSTIC_ASSERT( - opener->mOriginAttributes.EqualsIgnoringFPD(mOriginAttributes)); - } - } - if (RefPtr<BrowsingContext> parent = GetParent()) { - MOZ_DIAGNOSTIC_ASSERT(parent->mType == mType); - MOZ_DIAGNOSTIC_ASSERT(parent->mGroup == mGroup); - MOZ_DIAGNOSTIC_ASSERT(parent->mUseRemoteTabs == mUseRemoteTabs); - MOZ_DIAGNOSTIC_ASSERT(parent->mUseRemoteSubframes == mUseRemoteSubframes); - MOZ_DIAGNOSTIC_ASSERT(parent->mPrivateBrowsingId == mPrivateBrowsingId); - MOZ_DIAGNOSTIC_ASSERT( - parent->mOriginAttributes.EqualsIgnoringFPD(mOriginAttributes)); - } - - // UseRemoteSubframes and UseRemoteTabs must match. - MOZ_DIAGNOSTIC_ASSERT( - !mUseRemoteSubframes || mUseRemoteTabs, - "Cannot set useRemoteSubframes without also setting useRemoteTabs"); - - // Double-check OriginAttributes/Private Browsing - AssertOriginAttributesMatchPrivateBrowsing(); -#endif -} - void BrowsingContext::AssertOriginAttributesMatchPrivateBrowsing() { // Chrome browsing contexts must not have a private browsing OriginAttribute // Content browsing contexts must maintain the equality:

===================================== docshell/base/BrowsingContext.h ===================================== @@ -984,7 +984,18 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache { bool aHasPostData);

private: - mozilla::ipc::IPCResult Attach(bool aFromIPC, ContentParent* aOriginProcess); + // Assert that this BrowsingContext is coherent relative to related + // BrowsingContexts. This will be run before the BrowsingContext is attached. + // + // A non-null string return value indicates that there was a coherency check + // failure, which will be handled with either a crash or IPC failure. + // + // If provided, `aOriginProcess` is the process which is responsible for the + // creation of this BrowsingContext. + [[nodiscard]] const char* BrowsingContextCoherencyChecks( + ContentParent* aOriginProcess); + + void Attach(bool aFromIPC, ContentParent* aOriginProcess);

// Recomputes whether we can execute scripts in this BrowsingContext based on // the value of AllowJavascript() and whether scripts are allowed in the @@ -998,10 +1009,6 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache {

void AssertOriginAttributesMatchPrivateBrowsing();

- // Assert that the BrowsingContext's LoadContext flags appear coherent - // relative to related BrowsingContexts. - void AssertCoherentLoadContext(); - friend class ::nsOuterWindowProxy; friend class ::nsGlobalWindowOuter; friend class WindowContext;

===================================== mobile/android/android-components/components/support/ktx/src/main/java/mozilla/components/support/ktx/kotlin/String.kt ===================================== @@ -53,6 +53,8 @@ const val MAX_URI_LENGTH = 25000

private const val FILE_PREFIX = "file://" private const val MAX_VALID_PORT = 65_535 +private const val SPACE = " " +private const val UNDERSCORE = "_"

/** * Shortens URLs to be more user friendly. @@ -316,7 +318,9 @@ fun String.sanitizeFileName(): String { file.name.replace("\.\.+".toRegex(), ".") } else { file.name.replace(".", "") - }.replaceEscapedCharacters() + }.replaceContinuousSpaces() + .replaceEscapedCharacters() + .trim() }

/** @@ -324,8 +328,16 @@ fun String.sanitizeFileName(): String { * and is correctly displayed. */ private fun String.replaceEscapedCharacters(): String { - val controlCharactersRegex = "[\x00-\x13/*"?<>:|\\]".toRegex() - return replace(controlCharactersRegex, "_") + val escapedCharactersRegex = "[\x00-\x13*"?<>:|\\]".toRegex() + return replace(escapedCharactersRegex, UNDERSCORE) +} + +/** + * Replaces continuous spaces with a single space. + */ +private fun String.replaceContinuousSpaces(): String { + val escapedCharactersRegex = "[\p{Z}\s]+".toRegex() + return replace(escapedCharactersRegex, SPACE) }

/**

===================================== mobile/android/android-components/components/support/ktx/src/test/java/mozilla/components/support/ktx/kotlin/StringTest.kt ===================================== @@ -199,11 +199,11 @@ class StringTest { "acknowledge\u0006signal" to "acknowledge_signal", "bell\u0007sound" to "bell_sound", "back\u0008space" to "back_space", - "horizontal\u0009tab" to "horizontal_tab", - "new\u000Aline" to "new_line", - "vertical\u000Btab" to "vertical_tab", - "form\u000Cfeed" to "form_feed", - "return\u000Dcarriage" to "return_carriage", + "horizontal\u0009tab" to "horizontal tab", + "new\u000Aline" to "new line", + "vertical\u000Btab" to "vertical tab", + "form\u000Cfeed" to "form feed", + "return\u000Dcarriage" to "return carriage", "shift\u000Eout" to "shift_out", "shift\u000Fin" to "shift_in", "escape\u0010data" to "escape_data",

View it on GitLab: https://gitlab.torproject.org/tpo/applications/tor-browser/-/compare/689b068...