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.

Sign up for GitHub

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

feat: add option to enable cross-origin isolation #234

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat: add option to enable cross-origin isolation #234

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e01f5d9
fix: enable cross-origin isolation
nolanlawson Apr 25, 2022
87e319f
Merge remote-tracking branch 'origin/main' into cross-origin-isolation
nolanlawson Aug 4, 2024
3cd4426
fix: add header and cli option
nolanlawson Aug 4, 2024
0aee155
fix: actually expose the cli option
nolanlawson Aug 4, 2024
0b7e643
docs: update changelog
nolanlawson Aug 5, 2024
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
12 changes: 12 additions & 0 deletions src/cross-origin-isolation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import {Middleware} from 'koa';
AndrewJakubowicz marked this conversation as resolved.
Show resolved Hide resolved

// Enable cross-origin isolation for more precise timers:
// https://developer.chrome.com/blog/cross-origin-isolated-hr-timers/
export function crossOriginIsolation(): Middleware {
// Based on https://github.com/fishel-feng/koa-isolated
return async function isolated(ctx, next) {
ctx.set('Cross-Origin-Opener-Policy', 'same-origin');
ctx.set('Cross-Origin-Embedder-Policy', 'require-corp');
await next();
};
}
2 changes: 2 additions & 0 deletions src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {nodeResolve} from 'koa-node-resolve';

import {BenchmarkResponse, Deferred} from './types';
import {NpmInstall} from './versions';
import {crossOriginIsolation} from './cross-origin-isolation';

export interface ServerOpts {
host: string;
Expand Down Expand Up @@ -88,6 +89,7 @@ export class Server {
this.server = server;
const app = new Koa();

app.use(crossOriginIsolation());
Copy link
Collaborator

@AndrewJakubowicz AndrewJakubowicz Aug 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a chance this change could be breaking in the case where someone is explicitly measuring something where external resources are requested?

In this case, may it be worth putting this change behind a ServerOpts option, where the default is the current behavior (not cross origin isolated), but the headers can be opted in?

Copy link
Author

Choose a reason for hiding this comment

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

Good point! I totally didn't think of that. 😅

I think making this disabled by default sort of defeats the purpose of improving the timing accuracy, but I also admit I don't have any evidence that CORP/COEP actually does this (maybe the browser treats localhost differently? not sure). That said, having the option at least unblocks SharedArrayBuffer and friends.

So sure thing: I added a CLI option, --cross-origin-isolated, which is disabled by default. I think further experimentation would be needed to see if it has any effect on timing accuracy. 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the benefits of having it (even if behind a flag):

  • Safe roll-out of the option. Making it a non-breaking change.
  • The timers should be more accurate when cross origin isolated is enabled.
  • And we could still add the SharedArrayBuffer (and friends) features. They would just have to check the crossOriginIsolated property.

I'm a fan of this change!

app.use(bodyParser());
app.use(mount('/submitResults', this.submitResults.bind(this)));
app.use(this.instrumentRequests.bind(this));
Expand Down
10 changes: 10 additions & 0 deletions src/test/server_test.ts
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for adding tests!

Original file line number Diff line number Diff line change
Expand Up @@ -135,4 +135,14 @@ suite('server', () => {
session = server.endSession();
assert.equal(session.bytesSent, 0);
});

test('enables cross-origin isolation', async () => {
const res = await fetch(`${server.url}/import-bare-module.html`);

assert.equal(res.headers.get('Cross-Origin-Opener-Policy'), 'same-origin');
assert.equal(
res.headers.get('Cross-Origin-Embedder-Policy'),
'require-corp'
);
});
});