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

fix: add port / user information in standard schemes #26803

Closed

Conversation

RangerMauve
Copy link

@RangerMauve RangerMauve commented Dec 2, 2020

Description of Change

Fixes #24725 by registering standard schemes as having host and username fields.

This issue was breaking support for gemini:// URLs

What's the best way to add a test for this use case?

Checklist

Release Notes

Notes: Custom protocol schemes can now have ports and user information

@welcome
Copy link

welcome bot commented Dec 2, 2020

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@RangerMauve RangerMauve changed the title Add port / user information is standard schemes Add port / user information in standard schemes Dec 4, 2020
@RangerMauve
Copy link
Author

I don't think the CI error is related to this PR, but I'm not sure how to verify

not ok 1127 session.serviceWorkers getFromVersionID() should report the correct script url and scope Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (C:\projects\src\electron\spec-main\api-service-workers-spec.ts)

@MarshallOfSound
Copy link
Member

@RangerMauve for some reason CircleCI didn't kick off builds for this PR, can you sign in here --> https://app.circleci.com/pipelines/github/electron/electron

And then push a new commit to this branch to re-trigger CI?

@RangerMauve
Copy link
Author

@MarshallOfSound Logged into the CI and pushed a docs commit. 😁

Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

Requesting changes since a test needs to be added. You can find examples of tests in spec-main/api-protocol-spec.ts.

@zcbenz zcbenz added the semver/patch backwards-compatible bug fixes label Dec 8, 2020
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Dec 8, 2020
@zcbenz zcbenz changed the title Add port / user information in standard schemes fix: add port / user information in standard schemes Dec 8, 2020
@zcbenz zcbenz added the wip ⚒ label Dec 18, 2020
@zcbenz
Copy link
Member

zcbenz commented Jan 19, 2021

I have written a test for this, but unfortunately it crashes Electron. It seems that adding the port to URL would make the origin check fail.

[78372:0119/201820.496315:FATAL:render_frame_host_impl.cc(858)] Check failed: browser_side_origin == renderer_side_origin (port://fake-host vs. port://fake-host)
diff --git a/spec-main/api-protocol-spec.ts b/spec-main/api-protocol-spec.ts
index ddf086023..305255ebd 100644
--- a/spec-main/api-protocol-spec.ts
+++ b/spec-main/api-protocol-spec.ts
@@ -703,7 +703,7 @@ describe('protocol module', () => {
     });
   });
 
-  describe('protocol.registerSchemeAsPrivileged', () => {
+  describe('protocol.registerSchemesAsPrivileged', () => {
     it('does not crash on exit', async () => {
       const appPath = path.join(__dirname, 'fixtures', 'api', 'custom-protocol-shutdown.js');
       const appProcess = ChildProcess.spawn(process.execPath, ['--enable-logging', appPath]);
@@ -721,6 +721,29 @@ describe('protocol module', () => {
       expect(stdout).to.not.contain('VALIDATION_ERROR_DESERIALIZATION_FAILED');
       expect(stderr).to.not.contain('VALIDATION_ERROR_DESERIALIZATION_FAILED');
     });
+
+    it('keeps port information for standard scheme', async () => {
+      const portScheme = (global as any).portScheme;
+      const u = portScheme + '://fake-host:8080/';
+      registerStringProtocol(portScheme, (request, callback) => {
+        console.log(request);
+        if (request.url === u) {
+          callback('<script>function ajax(url) { return fetch(url).then(res => res.text()) }</script>');
+        } else {
+          callback(request.url);
+        }
+      });
+      const contents = (webContents as any).create({ sandbox: true });
+      after(() => {
+        unregisterProtocol(portScheme);
+        contents.destroy();
+      });
+
+      await contents.loadURL(u);
+      const requestUrl = u + 'page';
+      const data = await contents.executeJavaScript(`ajax('${requestUrl}')`);
+      expect(data).to.equal(requestUrl);
+    });
   });
 
   describe.skip('protocol.registerSchemesAsPrivileged standard', () => {
diff --git a/spec-main/index.js b/spec-main/index.js
index febd107f6..c391fb39f 100644
--- a/spec-main/index.js
+++ b/spec-main/index.js
@@ -34,9 +34,11 @@ app.commandLine.appendSwitch('use-fake-device-for-media-stream');
 
 global.standardScheme = 'app';
 global.zoomScheme = 'zoom';
+global.portScheme = 'port';
 protocol.registerSchemesAsPrivileged([
   { scheme: global.standardScheme, privileges: { standard: true, secure: true, stream: false } },
   { scheme: global.zoomScheme, privileges: { standard: true, secure: true } },
+  { scheme: global.portScheme, privileges: { standard: true, secure: true, supportFetchAPI: true } },
   { scheme: 'cors-blob', privileges: { corsEnabled: true, supportFetchAPI: true } },
   { scheme: 'cors', privileges: { corsEnabled: true, supportFetchAPI: true } },
   { scheme: 'no-cors', privileges: { supportFetchAPI: true } },

@zcbenz
Copy link
Member

zcbenz commented Jan 19, 2021

I'm closing this PR since it does not work.

@zcbenz zcbenz closed this Jan 19, 2021
@RangerMauve
Copy link
Author

I'd really like to take another shot at this but sadly I'm having trouble compiling electron from source on Pop OS.

Is there a way to set up build tools without manually setting up depot_tools and the such? maybe a docker container?

@RangerMauve
Copy link
Author

Gonna try again and investigate the error that was found by @zcbenz

@zcbenz
Copy link
Member

zcbenz commented Feb 11, 2021

I'd really like to take another shot at this but sadly I'm having trouble compiling electron from source on Pop OS.

Is there a way to set up build tools without manually setting up depot_tools and the such? maybe a docker container?

You can use this tool to make building electron easier: https://github.com/electron/build-tools

@RangerMauve
Copy link
Author

@zcbenz Thanks! I actually tried that out and it helped, however I ran into this issue due to me running Ubuntu 20. 😅 #27704 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch backwards-compatible bug fixes wip ⚒
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port numbers are lost on standard schemes.
3 participants