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

Debt: use ipc to communicate with the auth dialog #97301

Closed
deepak1556 opened this issue May 8, 2020 · 5 comments · Fixed by #100907
Closed

Debt: use ipc to communicate with the auth dialog #97301

deepak1556 opened this issue May 8, 2020 · 5 comments · Fixed by #100907
Assignees
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders sandbox Running VSCode in a node-free environment
Milestone

Comments

@deepak1556
Copy link
Contributor

deepak1556 commented May 8, 2020

Follow-up to #97199 , since executeJavaScript calls are hard to debug and they are just inline script evaluation that can easily break CSP.

  1. We should change BrowserWindowConstructorOptions for the auth window

from

const opts: BrowserWindowConstructorOptions = {
			alwaysOnTop: true,
			skipTaskbar: true,
			resizable: false,
			width: 450,
			height: 220,
			show: true,
			title: 'VS Code',
			webPreferences: {
				nodeIntegration: true,
				webviewTag: true
			}
		};

to

const opts: BrowserWindowConstructorOptions = {
	alwaysOnTop: true,
	skipTaskbar: true,
	resizable: false,
	width: 450,
	height: 220,
	show: true,
	title: localize('authRequire', "Proxy Authentication Required"),
	webPreferences: {
	      sandbox: true,
               contextIsolation: true,
               preload: require.toUrl('vs/code/electron-browser/proxy/auth-preload.js')
	}
};
  1. Setup ipc channel in the preload script

  2. Replace executeJavaScript call in vs/code/electron-main/auth.ts with win.webContents.send

This will align well with #92164

/cc @bpasero once you setup a structure for how preload communication should look, we can adopt it here.

@deepak1556 deepak1556 added this to the May 2020 milestone May 8, 2020
@deepak1556 deepak1556 added debt Code quality issues engineering VS Code - Build / issue tracking / etc. sandbox Running VSCode in a node-free environment and removed engineering VS Code - Build / issue tracking / etc. labels May 8, 2020
@cr1315
Copy link

cr1315 commented May 14, 2020

Could you please change the opts.height to some number bigger by the way, maybe 320?
I can hardly find the submit button in our case..

@bpasero
Copy link
Member

bpasero commented May 27, 2020

As discussed, the auth dialog should use IMainProcessService and channels to communicate with the main side. A good place to learn from is our issue reporter window.

@joaomoreno
Copy link
Member

Let's do this on debt week.

@joaomoreno
Copy link
Member

Pushing out, no rush here.

@deepak1556
Copy link
Contributor Author

deepak1556 commented Jun 30, 2020

thanks @joaomoreno , agreed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders sandbox Running VSCode in a node-free environment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@joaomoreno @bpasero @deepak1556 @cr1315 and others