Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Remove use of eval in non-legacy rollup builds #1760

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 35 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
"eslint-import-resolver-typescript": "^2.2.0",
"eslint-plugin-import": "^2.22.1",
"eslint-plugin-svelte3": "^2.7.3",
"helmet": "4.4.1",
"kleur": "^4.0.0",
"mime": "^2.4.4",
"mocha": "^8.0.0",
Expand Down
2 changes: 1 addition & 1 deletion runtime/src/server/middleware/get_page_handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ export function get_page_handler(
const legacy_main = `${req.baseUrl}/client/legacy/${build_info.legacy_assets.main}`;
script += `(function(){try{eval("async function x(){}");var main="${main}"}catch(e){main="${legacy_main}"};var s=document.createElement("script");try{new Function("if(0)import('')")();s.src=main;s.type="module";s.crossOrigin="use-credentials";}catch(e){s.src="${req.baseUrl}/client/shimport@${build_info.shimport}.js";s.setAttribute("data-main",main);}document.head.appendChild(s);}());`;
} else {
script += `var s=document.createElement("script");try{new Function("if(0)import('')")();s.src="${main}";s.type="module";s.crossOrigin="use-credentials";}catch(e){s.src="${req.baseUrl}/client/shimport@${build_info.shimport}.js";s.setAttribute("data-main","${main}")}document.head.appendChild(s)`;
Copy link
Member

Choose a reason for hiding this comment

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

what was this try / catch trying to detect? I feel like it was to see what browser you were using and can't simply be removed

Copy link
Author

Choose a reason for hiding this comment

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

Looks like it was trying to detect "import" support to add the module script, otherwise it added "shimport".

Copy link
Member

Choose a reason for hiding this comment

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

If there's a way to detect ESM/import/import() support without a try-catch, then I'd be all for switching to that. Otherwise, as I indicated below, I don't think we can just do this without it being a breaking change. (And it'd be a breaking change that I don't think could be easily worked around for people that need to support these browsers. They'd be stuck on the old version.)

Copy link
Author

Choose a reason for hiding this comment

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

@Conduitry Agreed. I'll have a look 👍

script += `var s=document.createElement("script");s.src="${main}";s.type="module";s.crossOrigin="use-credentials";document.head.appendChild(s)`;
}
} else {
script += `</script><script${nonce_attr} src="${main}" defer>`;
Expand Down
58 changes: 58 additions & 0 deletions test/apps/cspnonce-with-helmet/rollup.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import resolve from '@rollup/plugin-node-resolve';
import replace from '@rollup/plugin-replace';
import svelte from 'rollup-plugin-svelte';

const mode = process.env.NODE_ENV;
const dev = mode === 'development';

const config = require('../../../config/rollup.js');

export default {
client: {
input: config.client.input(),
output: config.client.output(),
plugins: [
replace({
'process.browser': true,
'process.env.NODE_ENV': JSON.stringify(mode)
}),
svelte({
dev,
hydratable: true,
emitCss: true
}),
resolve()
]
},

server: {
input: config.server.input(),
output: config.server.output(),
plugins: [
replace({
'process.browser': false,
'process.env.NODE_ENV': JSON.stringify(mode)
}),
svelte({
generate: 'ssr',
dev
}),
resolve({
preferBuiltins: true
})
],
external: ['helmet', 'sirv', 'polka']
},

serviceworker: {
input: config.serviceworker.input(),
output: config.serviceworker.output(),
plugins: [
resolve(),
replace({
'process.browser': true,
'process.env.NODE_ENV': JSON.stringify(mode)
})
]
}
};
9 changes: 9 additions & 0 deletions test/apps/cspnonce-with-helmet/src/client.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import * as sapper from '@sapper/app';

window.start = () => sapper.start({
target: document.querySelector('#sapper')
});

window.prefetchRoutes = () => sapper.prefetchRoutes();
window.prefetch = href => sapper.prefetch(href);
window.goto = href => sapper.goto(href);
8 changes: 8 additions & 0 deletions test/apps/cspnonce-with-helmet/src/routes/_error.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<script>
export let status;
export let error;
</script>

<h1>{status}</h1>

<p>{error.message}</p>
7 changes: 7 additions & 0 deletions test/apps/cspnonce-with-helmet/src/routes/index.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<script>
let test = undefined;
</script>

<input bind:value={test} type=text>

<span>{test || '-'}</span>
25 changes: 25 additions & 0 deletions test/apps/cspnonce-with-helmet/src/server.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import helmet from 'helmet';
import polka from 'polka';
import * as sapper from '@sapper/server';

import { start } from '../../common.js';

const app = polka()
.use((req, res, next) => {
res.locals = { nonce: 'rAnd0m123' };
next();
})
.use(
helmet({
contentSecurityPolicy: {
directives: {
defaultSrc: ["'self'"],
scriptSrc: ["blob: 'self'", (_req, res) => `'nonce-${res.locals.nonce}'`],
connectSrc: ["'self'", 'http://localhost:10000']
}
}
}),
sapper.middleware()
);

start(app);
82 changes: 82 additions & 0 deletions test/apps/cspnonce-with-helmet/src/service-worker.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import * as sapper from '@sapper/service-worker';

const ASSETS = `cache${sapper.timestamp}`;

// `shell` is an array of all the files generated by webpack,
// `files` is an array of everything in the `static` directory
const to_cache = sapper.shell.concat(sapper.files);
const cached = new Set(to_cache);

self.addEventListener('install', event => {
event.waitUntil(
caches
.open(ASSETS)
.then(cache => cache.addAll(to_cache))
.then(() => {
self.skipWaiting();
})
);
});

self.addEventListener('activate', event => {
event.waitUntil(
caches.keys().then(async keys => {
// delete old caches
for (const key of keys) {
if (key !== ASSETS) await caches.delete(key);
}

self.clients.claim();
})
);
});

self.addEventListener('fetch', event => {
if (event.request.method !== 'GET') return;

const url = new URL(event.request.url);

// don't try to handle e.g. data: URIs
if (!url.protocol.startsWith('http')) return;

// ignore dev server requests
if (url.hostname === self.location.hostname && url.port !== self.location.port) return;

// always serve assets and webpack-generated files from cache
if (url.host === self.location.host && cached.has(url.pathname)) {
event.respondWith(caches.match(event.request));
return;
}

// for pages, you might want to serve a shell `index.html` file,
// which Sapper has generated for you. It's not right for every
// app, but if it's right for yours then uncomment this section
/*
if (url.origin === self.origin && routes.find(route => route.pattern.test(url.pathname))) {
event.respondWith(caches.match('/index.html'));
return;
}
*/

if (event.request.cache === 'only-if-cached') return;

// for everything else, try the network first, falling back to
// cache if the user is offline. (If the pages never change, you
// might prefer a cache-first approach to a network-first one.)
event.respondWith(
caches
.open(`offline${sapper.timestamp}`)
.then(async cache => {
try {
const response = await fetch(event.request);
cache.put(event.request, response.clone());
return response;
} catch (err) {
const response = await cache.match(event.request);
if (response) return response;

throw err;
}
})
);
});
14 changes: 14 additions & 0 deletions test/apps/cspnonce-with-helmet/src/template.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<!doctype html>
<html lang="en">
<head>
<meta charset='utf-8'>

%sapper.base%
%sapper.styles%
%sapper.head%
</head>
<body>
<div id='sapper'>%sapper.html%</div>
%sapper.scripts%
</body>
</html>
30 changes: 30 additions & 0 deletions test/apps/cspnonce-with-helmet/test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import * as assert from 'assert';
import { build } from '../../../api';
import { AppRunner } from '../AppRunner';

describe('cspnonce-with-helmet', function() {
this.timeout(10000);

let r: AppRunner;

// hooks
before('build app', () => build({ cwd: __dirname }));
before('start runner', async () => {
r = await new AppRunner().start(__dirname);
});

after(() => r && r.end());

// without the fix in runtime/src/server/middleware/get_page_handler.ts
// this will fail as the script to do the updates will be blocked
it('does not prevent bindings from working', async () => {
await r.load('/');
await r.sapper.start();

assert.equal(await r.text('span'), '-');

await r.page.type('input[type="text"]', 'text');

assert.equal(await r.text('span'), 'text');
});
});