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

feat: migrate protocol module to NetworkService (Part 11) #18706

Merged
merged 3 commits into from Jun 11, 2019

Conversation

Projects
None yet
2 participants
@zcbenz
Copy link
Member

commented Jun 10, 2019

Description of Change

Refs #15791.

The registerHttpProtocol API can make arbitrary requests regardless of the CORS settings. To keep this behavior, we need to explicitly download data from URL and then pipe it to NetworkService, like what we did for other types of protocol types.

This PR implements the API by adding a URLPipeLoader class that pipes the data from URL to NetworkService, instead of creating a new loader which was essentially a redirection.

Checklist

Release Notes

Notes: no-notes

zcbenz added some commits Jun 10, 2019

Remove "sending request of http protocol urls" test
Sending request to "http://" in "file://" violates CORS rules and always
fail, before NetworkService somehow Chromium still sent a request even
though the request failed with CORS error, so the test passes while the
test is not valid. With NetworkService no request is sent at all and the
test jsut fails.

So this is an ancient invalid test, as sending http requests have been
fully covered in other tests, I am removing this test.

@zcbenz zcbenz requested a review from deepak1556 Jun 10, 2019

@deepak1556
Copy link
Member

left a comment

LGTM

@@ -86,20 +86,6 @@ describe('chromium feature', () => {
})
})

describe('sending request of http protocol urls', () => {

This comment has been minimized.

Copy link
@deepak1556

deepak1556 Jun 10, 2019

Member

why delete this test ?

This comment has been minimized.

Copy link
@zcbenz

zcbenz Jun 10, 2019

Author Member

This test is essentially sending CORS requests in file:// and the request always fails, the test passes by accident. I found it when testing http protocol code.

The thing it wants to test (that sending http request should not crash) is already covered by other tests, we don't need to keep this invalid test.

@deepak1556

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

As a follow up , I am thinking this behavior should be made opt in, the default behavior of custom schemes should be secure, act more like chrome-extension. But this will be a different effort that aligns with making file scheme less privileged.

@electron-cation electron-cation bot removed the new-pr 🌱 label Jun 11, 2019

@zcbenz zcbenz merged commit 3a1d6d2 into master Jun 11, 2019

11 of 13 checks passed

electron-arm-testing Build #20190610.5 failed
Details
electron-arm64-testing Build #20190610.5 failed
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Jun 11, 2019

No Release Notes

@zcbenz zcbenz deleted the ns-protocol-url-loader branch Jun 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.