Types for policies

[Stoyz]

Hi all!

I was wondering, what is the reason for checking the allowed policies at runtime over a string rather than setting a type for the specific policy itself?
This thought came to me looking at the code of the crossOriginEmbedderPolicy middleware.

Couldn't it be better to just set a custom type for the allowed policies?
Maybe something like this:

export type CrossOriginEmbedderPolicy = "require-corp" | "credentialless";

export interface CrossOriginEmbedderPolicyOptions {
  policy?: CrossOriginEmbedderPolicy;
}

In this way it could help the user to know in advance which policies are allowed or managed by your middlewares without incurring in runtime errors.

Or maybe rather than removing the runtime it could just be added the type as showed above, if the allowed policies check is needed for people using JS.

Thank you and sorry in case I missed a discussion about this topic.

[EvanHahn]

There are two reasons:

1. Lots of people use Helmet without TypeScript, so I want to have runtime checks.

2. TypeScript doesn't always infer things properly, and you need to do additional work to make things type-check. For example, this doesn't typecheck:

   const options = { policy: "require-corp" };
   crossOriginEmbedderPolicy(options);

   See this TypeScript playground for more details.

Neither of these reasons are iron-clad, though. What do you think?

[Stoyz]

Definitely not how I expected it to work.

Maybe an enum could be used for policies instead of a type, but this would be a breaking change for those using the library with TypeScript.
Those using Helmet with JavaScript wouldn't notice the difference because a runtime check considering the enum values could continue to occur.

Please see the implementation example below.

export enum CrossOriginEmbedderPolicy {
    RequireCorp = "require-corp",
    Credentialless = "credentialless",
}

export interface CrossOriginEmbedderPolicyOptions {
    policy?: CrossOriginEmbedderPolicy;
}

function getHeaderValueFromOptions({
    policy = CrossOriginEmbedderPolicy.RequireCorp,
}: Readonly<CrossOriginEmbedderPolicyOptions>): string {
    // Checking directly on the values of the enum
    if (Object.values(CrossOriginEmbedderPolicy).includes(policy)) {
        return policy;
    } else {
        throw new Error(
            `Cross-Origin-Embedder-Policy does not support the ${JSON.stringify(policy)} policy`
        );
    }
}

I also understand that there may be a bit of work to be done for all policies considering that tests should be updated too.
What do you think about it?

[EvanHahn]

This is a good idea. I plan to add this to Helmet v6, the next major version.

I implemented this in the v6 branch. Follow along with the release at #370.

[Stoyz]

I know you closed the issue but may I know why you avoided using enums?

[EvanHahn]

A few reasons.

1. I want to minimize breaking changes, and this would introduce one for TypeScript users. Helmet is designed to be low-effort, and I've regretted larger breaking changes. I'm happy to make breaking changes if there's a good reason, but I don't think this is a good enough reason to introduce one.
2. Helmet would have to export these enums. Importing them might be a bit tricky. For example, I think it's easier to write policy: "require-corp" instead of policy: helmet.crossOriginEmbedderPolicy.Policy.RequireCorp, or something.
3. Enums produce compiled code, which I want to avoid as it increases the size of the bundle. (I could use const enums to avoid that, but in my experience, not all compilers support this.)

Hope this helps!

[Stoyz]

Okay I understand, thanks!

Anyway I have created a branch in my forked repo with an alternative solution to the changes you already made trying to remove duplicate strings for some policies. Please have a look at this commit if you mind.

[EvanHahn]

I'll take a look. Thanks!