Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Conversation

@n-riesco
Copy link
Contributor

Fixes plotly/streambed#10963

Closes #433

n-riesco added 2 commits May 17, 2018 20:59
* Don't log requests to and responses from `/static`, `/images` and
  `/settings/urls`.
@n-riesco
Copy link
Contributor Author

@tarzzz In the end, it turned out easier to listen on will-navigate. Would you do the review, please?


I don't think Falcon needs nodeIntegration to be enabled, so I've opened issue #438 to remind us to disable it.

mainWindow.webContents.on('will-navigate', (event, url) => {
if (!url.startsWith(HTTP_URL)) event.preventDefault();
});

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@tarzzz tarzzz left a comment

Choose a reason for hiding this comment

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

Looks good,
💃 after testing to make sure it works as intended, and does not affect OAuth logins.. !!

@tarzzz
Copy link
Contributor

tarzzz commented May 18, 2018

btw, You can cherry-pick bd1df53 here to close #438 as well.. !!

@n-riesco
Copy link
Contributor Author

n-riesco commented May 18, 2018

@tarzzz

  • the reason I opened Disable nodeIntegration #438 is that I don't want to change nodeIntegration in this PR.
  • I can't test the on-prem case, please, could you test this PR doesn't break oauth?
@tarzzz
Copy link
Contributor

tarzzz commented May 18, 2018 via email

@n-riesco
Copy link
Contributor Author

@tarzzz I've just realised that we can't test this PR against on-prem, because oauth in the desktop app is still broken (#306).

I've tested it against Plotly Cloud and it's working:

peek 2018-05-19 07-45

@tarzzz
Copy link
Contributor

tarzzz commented May 22, 2018

Yeah, we should be good to merge it because the issue is only applicable on the electron app. drag-and-drop on browser version will simply load the new url in that tab.

@n-riesco n-riesco merged commit e5d01c9 into plotly:master May 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

2 participants