Skip to content

Commit

Permalink
Cherry-pick 259548.30@safari-7615-branch (49109db). https://bugs.webk…
Browse files Browse the repository at this point in the history
…it.org/show_bug.cgi?id=250760

    Error object stacktraces may leak sensitive data in URL query parameters
    https://bugs.webkit.org/show_bug.cgi?id=250760
    rdar://104376838

    Reviewed by Patrick Angle.

    If a remote script is delivered after a redirect sensitive data may be present
    in the post-redirect URL. If the script later throws an error the error event
    object will have that post-redirect URL in its stacktrace and sourceURL properties.

    * Source/JavaScriptCore/runtime/Error.cpp:
    (JSC::getLineColumnAndSource):
    * Source/JavaScriptCore/runtime/StackFrame.cpp:
    (JSC::StackFrame::sourceURLStripped const):
        This is a new function which uses the URL class to strip
        potentially sensitive information from the URL of the script
        which contains the code for the current stack frame.
    (JSC::StackFrame::toString const):
    * Source/JavaScriptCore/runtime/StackFrame.h:

    * Source/WTF/wtf/URL.cpp:
    (WTF::URL::strippedForUseAsReport const):
        This is a function similar to strippedForUseAsReferrer except we also remove
        query parameters from the URL while strippedForUseAsReferrer only strips
        user information and fragment.
    * Source/WTF/wtf/URL.h:

    * Source/WebInspectorUI/UserInterface/Base/URLUtilities.js:
        Adds a utility function similar to WTF::URL::strippedForUseAsReport.
    * Source/WebInspectorUI/UserInterface/Models/DebuggerData.js:
    (WI.DebuggerData.prototype.scriptsForURL):
    (WI.DebuggerData.prototype.addScript):
        The Web Inspector debugger maps URLs it knows about to URLs reported
        by the stack frames in an error object's stack trace. This allows one
        to jump to offending source lines in the web inspector. In order to
        correctly map the stripped URL reported in a stack trace we need to key
        the map on the stripped URL as well.

    * Tools/TestWebKitAPI/Tests/WTF/URL.cpp:
    (TestWebKitAPI::TEST_F):
        Adds a unit test for URL::strippedForUseAsReport

    Canonical link: https://commits.webkit.org/259548.30@safari-7615-branch
  • Loading branch information
rreno authored and aperezdc committed Apr 1, 2023
1 parent 1975150 commit 6d228e9
Show file tree
Hide file tree
Showing 8 changed files with 90 additions and 6 deletions.
2 changes: 1 addition & 1 deletion Source/JavaScriptCore/runtime/Error.cpp
Expand Up @@ -192,7 +192,7 @@ bool getLineColumnAndSource(VM& vm, Vector<StackFrame>* stackTrace, unsigned& li
StackFrame& frame = stackTrace->at(i);
if (frame.hasLineAndColumnInfo()) {
frame.computeLineAndColumn(line, column);
sourceURL = frame.sourceURL(vm);
sourceURL = frame.sourceURLStripped(vm);
return true;
}
}
Expand Down
7 changes: 6 additions & 1 deletion Source/JavaScriptCore/runtime/StackFrame.cpp
Expand Up @@ -79,6 +79,11 @@ String StackFrame::sourceURL(VM& vm) const
return emptyString();
}

String StackFrame::sourceURLStripped(VM& vm) const
{
return URL(sourceURL(vm)).strippedForUseAsReport();
}

String StackFrame::functionName(VM& vm) const
{
if (m_isWasmFrame)
Expand Down Expand Up @@ -129,7 +134,7 @@ void StackFrame::computeLineAndColumn(unsigned& line, unsigned& column) const
String StackFrame::toString(VM& vm) const
{
String functionName = this->functionName(vm);
String sourceURL = this->sourceURL(vm);
String sourceURL = this->sourceURLStripped(vm);

if (sourceURL.isEmpty() || !hasLineAndColumnInfo())
return makeString(functionName, '@', sourceURL);
Expand Down
1 change: 1 addition & 0 deletions Source/JavaScriptCore/runtime/StackFrame.h
Expand Up @@ -51,6 +51,7 @@ class StackFrame {
String functionName(VM&) const;
SourceID sourceID() const;
String sourceURL(VM&) const;
String sourceURLStripped(VM&) const;
String toString(VM&) const;

bool hasBytecodeIndex() const { return m_bytecodeIndex && !m_isWasmFrame; }
Expand Down
12 changes: 12 additions & 0 deletions Source/WTF/wtf/URL.cpp
Expand Up @@ -890,6 +890,18 @@ String URL::strippedForUseAsReferrerWithExplicitPort() const
return makeString(StringView(m_string).left(m_hostEnd), ':', static_cast<unsigned>(*port), StringView(m_string).substring(end, m_queryEnd - end));
}

String URL::strippedForUseAsReport() const
{
if (!m_isValid)
return m_string;

unsigned end = credentialsEnd();

if (m_userStart == end && m_pathEnd == m_string.length())
return m_string;

return makeString(StringView(m_string).left(m_userStart), StringView(m_string).substring(end, m_pathEnd - end));
}

bool URL::isLocalFile() const
{
Expand Down
4 changes: 3 additions & 1 deletion Source/WTF/wtf/URL.h
Expand Up @@ -77,9 +77,11 @@ class URL {
WTF_EXPORT_PRIVATE static URL fileURLWithFileSystemPath(StringView);

WTF_EXPORT_PRIVATE String strippedForUseAsReferrer() const;

WTF_EXPORT_PRIVATE String strippedForUseAsReferrerWithExplicitPort() const;

// Similar to strippedForUseAsReferrer except we also remove the query component.
WTF_EXPORT_PRIVATE String strippedForUseAsReport() const;

// Makes a deep copy. Helpful only if you need to use a URL on another
// thread. Since the underlying StringImpl objects are immutable, there's
// no other reason to ever prefer isolatedCopy() over plain old assignment.
Expand Down
29 changes: 29 additions & 0 deletions Source/WebInspectorUI/UserInterface/Base/URLUtilities.js
Expand Up @@ -327,6 +327,35 @@ WI.urlWithoutFragment = function(urlString)
return urlString;
};

WI.urlWithoutUserQueryOrFragment = function(urlString)
{
try {
let url = new URL(urlString);

if (url.username) {
url.username = "";
}
if (url.password) {
url.password = "";
}
if (url.search) {
url.search = "";
}
if (url.hash) {
url.hash = "";
}

let result = url.toString();
if (result.endsWith("#"))
return result.substring(0, result.length - 1);

return result;

} catch { }

return urlString;
}

WI.displayNameForHost = function(host)
{
let extensionName = WI.browserManager.extensionNameForId(host);
Expand Down
7 changes: 4 additions & 3 deletions Source/WebInspectorUI/UserInterface/Models/DebuggerData.js
Expand Up @@ -64,7 +64,7 @@ WI.DebuggerData = class DebuggerData

scriptsForURL(url)
{
return this._scriptContentIdentifierMap.get(url) || [];
return this._scriptContentIdentifierMap.get(WI.urlWithoutUserQueryOrFragment(url)) || [];
}

// Protected (Called by DebuggerManager)
Expand All @@ -79,10 +79,11 @@ WI.DebuggerData = class DebuggerData
this._scriptIdMap.set(script.id, script);

if (script.contentIdentifier) {
let scripts = this._scriptContentIdentifierMap.get(script.contentIdentifier);
let url = WI.urlWithoutUserQueryOrFragment(script.contentIdentifier);
let scripts = this._scriptContentIdentifierMap.get(url);
if (!scripts) {
scripts = [];
this._scriptContentIdentifierMap.set(script.contentIdentifier, scripts);
this._scriptContentIdentifierMap.set(url, scripts);
}
scripts.push(script);
}
Expand Down
34 changes: 34 additions & 0 deletions Tools/TestWebKitAPI/Tests/WTF/URL.cpp
Expand Up @@ -603,6 +603,40 @@ TEST_F(WTF_URL, URLRemoveQueryParameters)
checkRemovedParameters(__LINE__, removedParameters14, { "key1"_s });
}

TEST_F(WTF_URL, URLStrippedForUseAsReport)
{
const auto expectedHTTPSURL = createURL("https://example.com/api"_s);
const auto expectedHTTPURL = createURL("http://example.com/api"_s);

const auto url = createURL("https://example.com/api"_s);
const auto url1 = createURL("https://example.com/api?key=value"_s);
auto url2 = createURL("https://user:password@example.com/api"_s);
auto url3 = createURL("https://user:password@example.com/api?key=value&key1=value1"_s);
auto url4 = createURL("https://example.com/api#fragment"_s);
auto url5 = createURL("https://user:password@example.com/api#fragment"_s);
auto url6 = createURL("https://example.com/api?key=value&key1=value1#fragment"_s);
auto url7 = createURL("https://user:password@example.com/api?key=value#fragment"_s);

const auto url8 = createURL("http://example.com/api#fragment"_s);
auto url9 = createURL("http://example.com/api?key=value#fragment"_s);
const auto url10 = createURL("http://user:password@example.com/api#fragment"_s);
auto url11 = createURL("http://user:password@example.com/api?key=value#fragment"_s);

EXPECT_EQ(expectedHTTPSURL.string(), url.strippedForUseAsReport());
EXPECT_EQ(expectedHTTPSURL.string(), url1.strippedForUseAsReport());
EXPECT_EQ(expectedHTTPSURL.string(), url2.strippedForUseAsReport());
EXPECT_EQ(expectedHTTPSURL.string(), url3.strippedForUseAsReport());
EXPECT_EQ(expectedHTTPSURL.string(), url4.strippedForUseAsReport());
EXPECT_EQ(expectedHTTPSURL.string(), url5.strippedForUseAsReport());
EXPECT_EQ(expectedHTTPSURL.string(), url6.strippedForUseAsReport());
EXPECT_EQ(expectedHTTPSURL.string(), url7.strippedForUseAsReport());

EXPECT_EQ(expectedHTTPURL.string(), url8.strippedForUseAsReport());
EXPECT_EQ(expectedHTTPURL.string(), url9.strippedForUseAsReport());
EXPECT_EQ(expectedHTTPURL.string(), url10.strippedForUseAsReport());
EXPECT_EQ(expectedHTTPURL.string(), url11.strippedForUseAsReport());
}

TEST_F(WTF_URL, IsolatedCopy)
{
URL url1 { "http://www.apple.com"_str };
Expand Down

0 comments on commit 6d228e9

Please sign in to comment.