Skip to content

Commit

Permalink
Regression(262976@main) Images do not show in Microsoft Word Online
Browse files Browse the repository at this point in the history 
https://bugs.webkit.org/show_bug.cgi?id=261524
rdar://114573089

Reviewed by Brent Fulgham.

Images do not show in Microsoft Word Online since the data URL change in 262976@main.
It is a surprising since 262976@main was meant to align us with other browsers and yet
images show correctly in Chrome & Firefox.

After some investigation, I found that the data URL used by Word Online for images in
Safari have incorrect padding, while the ones it uses in Chrome have correct padding.
As a result, it doesn't appear to be a WebKit bug but rather Safari getting served
different content than Chrome for some reason.

To address the issue, I am introducing a quirk to disable padding validation in data
URLs on Microsoft Word Online.

* Source/WTF/wtf/text/Base64.cpp:
(WTF::base64DecodeInternal):
* Source/WTF/wtf/text/Base64.h:
* Source/WebCore/loader/ResourceLoader.cpp:
(WebCore::ResourceLoader::loadDataURL):
* Source/WebCore/page/Quirks.cpp:
(WebCore::Quirks::shouldDisableDataURLPaddingValidation const):
* Source/WebCore/page/Quirks.h:
* Source/WebCore/platform/network/DataURLDecoder.cpp:
(WebCore::DataURLDecoder::DecodeTask::DecodeTask):
(WebCore::DataURLDecoder::createDecodeTask):
(WebCore::DataURLDecoder::decodeSynchronously):
(WebCore::DataURLDecoder::decode):
* Source/WebCore/platform/network/DataURLDecoder.h:

Canonical link: https://commits.webkit.org/267969@main
  • Loading branch information
@cdumez
cdumez committed Sep 13, 2023
1 parent f15b347 commit 599c092
Show file tree
Hide file tree
Showing 8 changed files with 42 additions and 13 deletions.
2 changes: 1 addition & 1 deletion Source/WTF/wtf/text/Base64.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ static std::optional<Vector<uint8_t, 0, CrashOnOverflow, 16, Malloc>> base64Deco
if (equalsSignCount)
return std::nullopt;
destination[destinationLength++] = decodedCharacter;
} else if (mode != Base64DecodeMode::DefaultValidatePaddingAndIgnoreWhitespace || !isASCIIWhitespace(ch))
} else if ((mode != Base64DecodeMode::DefaultValidatePaddingAndIgnoreWhitespace && mode != Base64DecodeMode::DefaultIgnoreWhitespaceForQuirk) || !isASCIIWhitespace(ch))
return std::nullopt;
}
}
Expand Down
4 changes: 3 additions & 1 deletion Source/WTF/wtf/text/Base64.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ enum class Base64EncodeMode : bool { Default, URL };
// enforce a correct number of trailing equal signs in the input.
// - ::DefaultValidatePaddingAndIgnoreWhitespace ignores ASCII whitespace in
// the input. It matches <https://infra.spec.whatwg.org/#forgiving-base64>.
enum class Base64DecodeMode { DefaultIgnorePadding, DefaultValidatePadding, DefaultValidatePaddingAndIgnoreWhitespace, URL };
// - ::DefaultIgnoreWhitespaceForQuirk, URL ignores ASCII whitespace in the
// input but doesn't validate padding. It is currently only used for quirks.
enum class Base64DecodeMode { DefaultIgnorePadding, DefaultValidatePadding, DefaultValidatePaddingAndIgnoreWhitespace, DefaultIgnoreWhitespaceForQuirk, URL };

struct Base64Specification {
std::span<const std::byte> input;
Expand Down
10 changes: 9 additions & 1 deletion Source/WebCore/loader/ResourceLoader.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include "DataURLDecoder.h"
#include "DiagnosticLoggingClient.h"
#include "DiagnosticLoggingKeys.h"
#include "Document.h"
#include "DocumentLoader.h"
#include "FrameDestructionObserverInlines.h"
#include "FrameLoader.h"
Expand All @@ -50,6 +51,7 @@
#include "PageConsoleClient.h"
#include "PlatformStrategies.h"
#include "ProgressTracker.h"
#include "Quirks.h"
#include "ResourceError.h"
#include "ResourceHandle.h"
#include "SecurityOrigin.h"
Expand Down Expand Up @@ -290,12 +292,18 @@ void ResourceLoader::loadDataURL()
auto url = m_request.url();
ASSERT(url.protocolIsData());

auto shouldValidatePadding = DataURLDecoder::ShouldValidatePadding::Yes;
if (RefPtr document = m_frame ? m_frame->document() : nullptr) {
if (document->quirks().shouldDisableDataURLPaddingValidation())
shouldValidatePadding = DataURLDecoder::ShouldValidatePadding::No;
}

DataURLDecoder::ScheduleContext scheduleContext;
#if USE(COCOA_EVENT_LOOP)
if (auto page = m_frame->page())
scheduleContext.scheduledPairs = *page->scheduledRunLoopPairs();
#endif
DataURLDecoder::decode(url, scheduleContext, [this, protectedThis = Ref { *this }, url](auto decodeResult) mutable {
DataURLDecoder::decode(url, scheduleContext, shouldValidatePadding, [this, protectedThis = Ref { *this }, url](auto decodeResult) mutable {
if (this->reachedTerminalState())
return;
if (!decodeResult) {
Expand Down
11 changes: 11 additions & 0 deletions Source/WebCore/page/Quirks.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1568,4 +1568,15 @@ bool Quirks::shouldStarBeFeaturePolicyDefaultValue() const
return *m_shouldStarBeFeaturePolicyDefaultValueQuirk;
}

// Microsoft office online generates data URLs with incorrect padding on Safari only (rdar://114573089).
bool Quirks::shouldDisableDataURLPaddingValidation() const
{
if (!needsQuirks())
return false;

if (!m_shouldDisableDataURLPaddingValidation)
m_shouldDisableDataURLPaddingValidation = m_document->url().host().endsWith("officeapps.live.com"_s);

This comment has been minimized.

Sign in to view

Copy link
@karlcow

karlcow Sep 14, 2023

Member

I need to double check if it solves the issue for office.com too.
Also maybe use the new helper. :)

This comment has been minimized.

Sign in to view

Copy link
@cdumez

cdumez Sep 14, 2023

Author Contributor

I indeed didn’t test on office.com. Please let me know if it doesn’t work there.

This comment has been minimized.

Sign in to view

Copy link
@karlcow

karlcow Sep 14, 2023

Member

If I don't do anything wrong. This is still failing for me after compiling the ToT
using the steps in https://bugs.webkit.org/show_bug.cgi?id=261537
with Site Specific Hacks Enabled.

1. Go to https://www.office.com/launch/word?auth=1 (log in if not yet)
2. Open a Word Document
3. Insert Picture
4. Choose from This Device
5. Choose the picture

This comment has been minimized.

Sign in to view

Copy link
@cdumez

cdumez Sep 14, 2023

Author Contributor

Ok, thanks for the repro case. I'll look into it.
return *m_shouldDisableDataURLPaddingValidation;
}

}
2 changes: 2 additions & 0 deletions Source/WebCore/page/Quirks.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ class Quirks {
bool needsResettingTransitionCancelsRunningTransitionQuirk() const;

bool shouldStarBeFeaturePolicyDefaultValue() const;
bool shouldDisableDataURLPaddingValidation() const;

private:
bool needsQuirks() const;
Expand Down Expand Up @@ -235,6 +236,7 @@ class Quirks {
bool m_needsConfigurableIndexedPropertiesQuirk { false };
bool m_needsToCopyUserSelectNoneQuirk { false };
mutable std::optional<bool> m_shouldStarBeFeaturePolicyDefaultValueQuirk;
mutable std::optional<bool> m_shouldDisableDataURLPaddingValidation;
};

} // namespace WebCore
18 changes: 11 additions & 7 deletions Source/WebCore/platform/network/DataURLDecoder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,10 @@ static Result parseMediaType(const String& mediaType)
struct DecodeTask {
WTF_MAKE_FAST_ALLOCATED;
public:
DecodeTask(const URL& url, const ScheduleContext& scheduleContext, DecodeCompletionHandler&& completionHandler)
DecodeTask(const URL& url, const ScheduleContext& scheduleContext, ShouldValidatePadding shouldValidatePadding, DecodeCompletionHandler&& completionHandler)
: url(url.isolatedCopy())
, scheduleContext(scheduleContext)
, shouldValidatePadding(shouldValidatePadding)
, completionHandler(WTFMove(completionHandler))
{
}
Expand Down Expand Up @@ -133,16 +134,18 @@ struct DecodeTask {
StringView encodedData;
bool isBase64 { false };
const ScheduleContext scheduleContext;
ShouldValidatePadding shouldValidatePadding;
DecodeCompletionHandler completionHandler;

Result result;
};

static std::unique_ptr<DecodeTask> createDecodeTask(const URL& url, const ScheduleContext& scheduleContext, DecodeCompletionHandler&& completionHandler)
static std::unique_ptr<DecodeTask> createDecodeTask(const URL& url, const ScheduleContext& scheduleContext, ShouldValidatePadding shouldValidatePadding, DecodeCompletionHandler&& completionHandler)
{
return makeUnique<DecodeTask>(
url,
scheduleContext,
shouldValidatePadding,
WTFMove(completionHandler)
);
}
Expand All @@ -160,7 +163,8 @@ static std::optional<Result> decodeSynchronously(DecodeTask& task)
return std::nullopt;

if (task.isBase64) {
auto decodedData = base64Decode(PAL::decodeURLEscapeSequences(task.encodedData), Base64DecodeMode::DefaultValidatePaddingAndIgnoreWhitespace);
auto mode = task.shouldValidatePadding == ShouldValidatePadding::Yes ? Base64DecodeMode::DefaultValidatePaddingAndIgnoreWhitespace : Base64DecodeMode::DefaultIgnoreWhitespaceForQuirk;
auto decodedData = base64Decode(PAL::decodeURLEscapeSequences(task.encodedData), mode);
if (!decodedData)
return std::nullopt;
task.result.data = WTFMove(*decodedData);
Expand All @@ -171,11 +175,11 @@ static std::optional<Result> decodeSynchronously(DecodeTask& task)
return WTFMove(task.result);
}

void decode(const URL& url, const ScheduleContext& scheduleContext, DecodeCompletionHandler&& completionHandler)
void decode(const URL& url, const ScheduleContext& scheduleContext, ShouldValidatePadding shouldValidatePadding, DecodeCompletionHandler&& completionHandler)
{
ASSERT(url.protocolIsData());

decodeQueue().dispatch([decodeTask = createDecodeTask(url, scheduleContext, WTFMove(completionHandler))]() mutable {
decodeQueue().dispatch([decodeTask = createDecodeTask(url, scheduleContext, shouldValidatePadding, WTFMove(completionHandler))]() mutable {
auto result = decodeSynchronously(*decodeTask);

#if USE(COCOA_EVENT_LOOP)
Expand All @@ -194,11 +198,11 @@ void decode(const URL& url, const ScheduleContext& scheduleContext, DecodeComple
});
}

std::optional<Result> decode(const URL& url)
std::optional<Result> decode(const URL& url, ShouldValidatePadding shouldValidatePadding)
{
ASSERT(url.protocolIsData());

auto task = createDecodeTask(url, { }, nullptr);
auto task = createDecodeTask(url, { }, shouldValidatePadding, nullptr);
return decodeSynchronously(*task);
}

Expand Down
6 changes: 4 additions & 2 deletions Source/WebCore/platform/network/DataURLDecoder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,10 @@ struct ScheduleContext {
#endif
};

WEBCORE_EXPORT void decode(const URL&, const ScheduleContext&, DecodeCompletionHandler&&);
WEBCORE_EXPORT std::optional<Result> decode(const URL&);
enum class ShouldValidatePadding : bool { No, Yes };

WEBCORE_EXPORT void decode(const URL&, const ScheduleContext&, ShouldValidatePadding, DecodeCompletionHandler&&);
WEBCORE_EXPORT std::optional<Result> decode(const URL&, ShouldValidatePadding = ShouldValidatePadding::Yes);

}

Expand Down
2 changes: 1 addition & 1 deletion Source/WebKit/NetworkProcess/NetworkDataTaskDataURL.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ void NetworkDataTaskDataURL::resume()

m_state = State::Running;

DataURLDecoder::decode(firstRequest().url(), { }, [this, protectedThis = Ref { *this }](auto decodeResult) mutable {
DataURLDecoder::decode(firstRequest().url(), { }, DataURLDecoder::ShouldValidatePadding::Yes, [this, protectedThis = Ref { *this }](auto decodeResult) mutable {
if (m_state == State::Canceling || m_state == State::Completed)
return;

Expand Down

0 comments on commit 599c092

Please sign in to comment.