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

CDN not working for addons #2499

Closed
cekvenich opened this issue Oct 22, 2019 · 19 comments
Closed

CDN not working for addons #2499

cekvenich opened this issue Oct 22, 2019 · 19 comments

Comments

@cekvenich
Copy link

I'm loading as so:
'https://cdn.jsdelivr.net/npm/xterm@4.1.0/lib/xterm.js',
'https://cdn.jsdelivr.net/npm/xterm@4.1.0/css/xterm.css',
'https://cdn.jsdelivr.net/npm/xterm-addon-fit@0.2.1/lib/xterm-addon-fit.js',
'https://cdn.jsdelivr.net/npm/xterm-addon-attach@0.3.0/lib/xterm-addon-attach.js'

It can't find constructor for either fit or attach.
So your 'making' process is b0rked.

I'd like to host the libs on CDN so I don't have to make them.

@cekvenich
Copy link
Author

cekvenich commented Oct 23, 2019

@Tyriar
Copy link
Member

Tyriar commented Oct 23, 2019

Are you just importing them wrong? If those come directly from the npm output they definitely work in the way you're meant to import and consume them. They're built as umd modules just like the main lib https://github.com/xtermjs/xterm.js/blob/master/addons/xterm-addon-fit/webpack.config.js

@cekvenich
Copy link
Author

@Tyriar I'm using it just like xterm.js is used without umd modules from the CDN. Right now add-ons I have to compile and I have to put on CDN. I posted the solution of the compiled .ts output - and hosted in on CDN for other users of the lib.

@Tyriar
Copy link
Member

Tyriar commented Oct 23, 2019

@cekvenich I think umd puts it on window if you include the file, so something like this should work.

const t = new window.Terminal();
const a = new FitAddon();
t.loadAddon(a);

But anyway, this is essentially asking the same thing as #2486 so closing as dupe.

@cekvenich
Copy link
Author

Your solution does not work for me. Also the referenced issue is almost opposite of this, and not the same. In any case I have it working and I hope this helps someone that searches for for a working prebuilt files.
ps: it is ok to have a module version, but you must have a non-module version. If you have only one, then non-module should be the one.

@lachdoug
Copy link

lachdoug commented May 1, 2020

@cekvenich try this:

<!DOCTYPE html>
<html>
<head>
	<meta charset="utf-8">
	<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/xterm@4.5.0/css/xterm.css" />
	<script src="https://cdn.jsdelivr.net/npm/xterm@4.5.0/lib/xterm.js"></script> 
	<script src="https://cdn.jsdelivr.net/npm/xterm-addon-fit@0.3.0/lib/xterm-addon-fit.js"></script> 
</head>
<body>
<div id="terminal"></div>
<script>
	const 	t = new Terminal(),
	 	f = new FitAddon.FitAddon();
	t.loadAddon(f);
	t.open(document.getElementById('terminal'));
	t.write('Hello from \x1B[1;3;31mxterm.js\x1B[0m $ ')
</script> 
</body>
</html>

@cekvenich
Copy link
Author

Thanks! I'll try soon.

@jerch
Copy link
Member

jerch commented May 21, 2020

I'd like to host the libs on CDN so I don't have to make them.

@cekvenich
Thats a really bad idea if you run the terminal to connect to real world machines later on and cannot guarantee the integrity of the CDN packages. A terminal is often used for sensitive tasks and should not be loaded from some random internet source.

@cekvenich
Copy link
Author

It is a common practice and has hash code protection.

@jerch
Copy link
Member

jerch commented May 21, 2020

It is a common practice...

There are countries where ppl dont lock the front doors. Its common practice there. But is it any good?

... and has hash code protection.

And who controls the hash? You during upload with strong auth to the CDN?

@cekvenich
Copy link
Author

I understand the risk. Please help.

@jerch
Copy link
Member

jerch commented May 22, 2020

Several options:

  • control the ressource, thus deliver it on your own
  • use secure CDN (where you control the upload and the SRI hash)
  • book a security audit (they gonna tell you basically the same)
  • tell your users not to use the embedded terminal for sensitive tasks

@raxod502
Copy link

raxod502 commented Jun 2, 2020

@jerch I think you misunderstood what @cekvenich is saying. He does not want help with making his application more secure. He wants xterm.js to work properly when he loads it from a CDN. Incidentally, I also want this. At the very least, if xterm.js is not going to support usage via CDN, then it should be removed from locations such as CDNJS -- otherwise it just wastes people's time.

@Tyriar
Copy link
Member

Tyriar commented Jun 2, 2020

@raxod502 we didn't put them there. Some people do use them and security isn't a problem, it's only if you are using xterm.js in a sensitive context like an actual running process that you should consider the security implications.

@raxod502
Copy link

The problem is that it's totally undocumented what interface the addon CDN modules expose, making it really really annoying to use them. I was convinced for a while that they were not usable at all, because none of the examples in the docs worked (no symbols were defined). I just took a closer look, and by staring at the minified export code for a while (ew), I determined that a global binding called attach is established by the attach addon, and the required constructor is somewhere inside a property. But that was very difficult to figure out. I think that xterm.js would benefit a lot from more examples on usage that work out of the box.

@jerch
Copy link
Member

jerch commented Jun 13, 2020

The problem is that it's totally undocumented what interface the addon CDN modules expose...

Again - we did not put it there, thus we dont control what gets parked there or what those modules expose. If the uploader simply put the npm package content there - well then they contain the typical class mangling done by the TS compiler. Additionally you can find library: addonName in the addon webpack config files, which finally leads to the new MyAddon.MyAddon(...) naming indirection.

I think that xterm.js would benefit a lot from more examples on usage that work out of the box.

I partially agree - more examples are always helpful, so feel free to write some. I strongly disagree with the "work out of the box" aspect, especially if it includes insecure CDN usage. First xterm.js is meant as a raw component to be integrated by some app. The preferred way to do that is to pull the packages at npm level, import them as needed and bundle the final thingy. Ideally you do it with Typescript (and the docs are written with TS in mind), but thats not a must. From the source repo side there is no CDN involved at all. If you really want to get a secure CDN solution rolling - feel free to make negotations with some CDN distributors for secure upload and proper SRI support, pull the npm package, rebundle and upload them, use them in your projects. Or if you are keen - get a good laywer, promote the CDN bundles as official, clean and secure and offer it to the public. If you are unsure, why a terminal with proper security measure is needed, read https://xtermjs.org/docs/guides/security/. Thanks.

@lachdoug
Copy link

If we put the CDN security issue aside, and just follow the approach used in the Getting Started section of the docs:

npm install xterm
npm install xterm-addon-fit
<!DOCTYPE html>
<html>
<head>
	<meta charset="utf-8">
	<link rel="stylesheet" href="node_modules/xterm/css/xterm.css" />
	<script src="node_modules/xterm/lib/xterm.js"></script>
	<script src="node_modules/xterm-addon-fit/lib/xterm-addon-fit.js"></script>
</head>
<body>
<div id="terminal"></div>
<script>
	const	term = new Terminal(),
		fit = new FitAddon();
	term.loadAddon(fit);
	term.open(document.getElementById('terminal'));
	term.write('Hello from \x1B[1;3;31mxterm.js\x1B[0m $ ')
</script>
</body>
</html>

This throws an error.

TypeError: FitAddon is not a constructor

The problem is that the xterm-addon-fit.js UMD returns an object when used with <script src="">.

@jerch
Copy link
Member

jerch commented Jun 13, 2020

@lachdoug Hmm yeah, that comes from the additional library setting in the addon webpack config, which "hides" the addon ctor behind AddonName.AddonName. I am not sure, why it was made that way in the first place, maybe to avoid global scope pollution by other exports in the addon code? We have no real coding rules on addons itself, thus this seems like an easy way to avoid namespace pollution. But sure, the name doubling is awkward here.

@Tyriar Should we remove the library rule from webpack config and make addons always export just one main entry point (the ctor itself)?

@Tyriar
Copy link
Member

Tyriar commented Jun 15, 2020

👉 #2941

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants