Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ability to mask URLs in Error.stack. #2021

Merged
merged 1 commit into from Jul 8, 2022
Merged

Add ability to mask URLs in Error.stack. #2021

merged 1 commit into from Jul 8, 2022

Conversation

xeenon
Copy link
Contributor

@xeenon xeenon commented Jul 1, 2022 •
edited by webkit-commit-queue

2aac2a8

Add ability to mask URLs in Error.stack.
https://bugs.webkit.org/show_bug.cgi?id=242278
rdar://81991245

Reviewed by Darin Adler and Yusuke Suzuki.

Have StackFrame's toString consult a new overrideSourceURL() method on VM::ClientData
that is implemented in WebCore's JSVMClientData that uses the new maskedURLForBindingsIfNeeded()
to mask certain URLs from the Error.stack string returned to JavaScript.

* Source/JavaScriptCore/inspector/ScriptCallStackFactory.cpp:
(Inspector::extractSourceInformationFromException): Pass vm to getLineColumnAndSource().
(Inspector::createScriptCallStackFromException): Pass vm to sourceURL().
* Source/JavaScriptCore/runtime/Error.cpp:
(JSC::getLineColumnAndSource): Add vm parameter and pass it to sourceURL().
(JSC::addErrorInfo): Pass vm to getLineColumnAndSource().
* Source/JavaScriptCore/runtime/Error.h:
* Source/JavaScriptCore/runtime/ErrorInstance.cpp:
(JSC::ErrorInstance::computeErrorInfo): Pass vm to getLineColumnAndSource().
* Source/JavaScriptCore/runtime/StackFrame.cpp:
(JSC::StackFrame::sourceURL const): Add vm parameter and call overrideSourceURL() on clientData.
(JSC::StackFrame::functionName const): Add some newlines.
(JSC::StackFrame::toString const): Pass vm to sourceURL().
* Source/JavaScriptCore/runtime/StackFrame.h:
(JSC::StackFrame::codeBlock const): Added.
* Source/JavaScriptCore/runtime/VM.h:
(JSC::VM::ClientData::overrideSourceURL const): Added.
* Source/WebCore/bindings/js/WebCoreJSClientData.cpp:
(WebCore::JSVMClientData::overrideSourceURL const): Added. Use maskedURLForBindingsIfNeeded() to mask sourceURLs.
* Source/WebCore/bindings/js/WebCoreJSClientData.h:
* Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewConfiguration.mm:
(TEST(WebKit, ConfigurationMaskedURLSchemes)): Added user script tests with Error.stack.

Canonical link: https://commits.webkit.org/252253@main

@xeenon xeenon requested review from dcrousso, patrickangle and a team as code owners July 1, 2022 23:55
@xeenon xeenon self-assigned this Jul 1, 2022
@xeenon xeenon added JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. WebKit Local Build labels Jul 1, 2022
darinadler
darinadler approved these changes Jul 3, 2022
View reviewed changes
Copy link
Member

@darinadler darinadler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not thrilled with the way this uses “override” as a sort of verb/noun/adjective combinations, but the mechanism seems sound.

Constellation
Constellation approved these changes Jul 7, 2022
View reviewed changes
Copy link
Member

@Constellation Constellation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with one comment.

Source/JavaScriptCore/runtime/StackFrame.cpp Outdated
Comment on lines 71 to 72
if (vm.clientData) {
String overrideURL = vm.clientData->overrideSourceURL(*this, sourceURL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you filter out http/https URLs to avoid calling a virtual function here?
Right now, only WebKit/WebCore is implementing this interface. And it will limit http/https. So I think we can first filter out them here. If we have any other motivations further not filtering these schemes in JavaScriptCore.framework, I think we can revisit and extend it later.

Copy link
Contributor Author

@xeenon xeenon Jul 7, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you okay doing this (since I wanted to avoid passing the and making a URL in JSC)?

    if (vm.clientData && !sourceURL.startsWithIgnoringASCIICase("http"_s)) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need, URL.h has a protocolIsInHTTPFamily function which takes a StringView, not a URL.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you do a follow-up patch to remove the explicit "http"_s?

@xeenon xeenon added the merge-queue Applied to send a pull request to merge-queue label Jul 8, 2022
@xeenon
Add ability to mask URLs in Error.stack.
2aac2a8 
https://bugs.webkit.org/show_bug.cgi?id=242278
rdar://81991245

Reviewed by Darin Adler and Yusuke Suzuki.

Have StackFrame's toString consult a new overrideSourceURL() method on VM::ClientData
that is implemented in WebCore's JSVMClientData that uses the new maskedURLForBindingsIfNeeded()
to mask certain URLs from the Error.stack string returned to JavaScript.

* Source/JavaScriptCore/inspector/ScriptCallStackFactory.cpp:
(Inspector::extractSourceInformationFromException): Pass vm to getLineColumnAndSource().
(Inspector::createScriptCallStackFromException): Pass vm to sourceURL().
* Source/JavaScriptCore/runtime/Error.cpp:
(JSC::getLineColumnAndSource): Add vm parameter and pass it to sourceURL().
(JSC::addErrorInfo): Pass vm to getLineColumnAndSource().
* Source/JavaScriptCore/runtime/Error.h:
* Source/JavaScriptCore/runtime/ErrorInstance.cpp:
(JSC::ErrorInstance::computeErrorInfo): Pass vm to getLineColumnAndSource().
* Source/JavaScriptCore/runtime/StackFrame.cpp:
(JSC::StackFrame::sourceURL const): Add vm parameter and call overrideSourceURL() on clientData.
(JSC::StackFrame::functionName const): Add some newlines.
(JSC::StackFrame::toString const): Pass vm to sourceURL().
* Source/JavaScriptCore/runtime/StackFrame.h:
(JSC::StackFrame::codeBlock const): Added.
* Source/JavaScriptCore/runtime/VM.h:
(JSC::VM::ClientData::overrideSourceURL const): Added.
* Source/WebCore/bindings/js/WebCoreJSClientData.cpp:
(WebCore::JSVMClientData::overrideSourceURL const): Added. Use maskedURLForBindingsIfNeeded() to mask sourceURLs.
* Source/WebCore/bindings/js/WebCoreJSClientData.h:
* Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewConfiguration.mm:
(TEST(WebKit, ConfigurationMaskedURLSchemes)): Added user script tests with Error.stack.

Canonical link: https://commits.webkit.org/252253@main
@webkit-commit-queue
Copy link
Collaborator

Committed 252253@main (2aac2a8): https://commits.webkit.org/252253@main

Reviewed commits have been landed. Closing PR #2021 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit 2aac2a8 into WebKit:main Jul 8, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 8, 2022
@xeenon xeenon deleted the eng/bug/242278 branch November 28, 2022 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darinadler darinadler darinadler approved these changes

@Constellation Constellation Constellation approved these changes

@dcrousso dcrousso Awaiting requested review from dcrousso dcrousso is a code owner

@patrickangle patrickangle Awaiting requested review from patrickangle patrickangle is a code owner

Assignees

@xeenon xeenon

Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
Milestone
No milestone
5 participants
@xeenon @webkit-commit-queue @Constellation @darinadler @webkit-early-warning-system