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

Expect-CT: put max-age first #225

Closed
EvanHahn opened this issue Jun 29, 2020 · 8 comments
Closed

Expect-CT: put max-age first #225

EvanHahn opened this issue Jun 29, 2020 · 8 comments

Comments

@EvanHahn
Copy link
Member

It's a little weird that max-age comes second after enforce:

if (options.enforce) {
directives.push("enforce");
}
directives.push(`max-age=${parseMaxAge(options.maxAge)}`);

We should reorder these lines and update the tests.

@CyberZujo
Copy link
Contributor

CyberZujo commented Jul 26, 2020

 directives.push(`max-age=${parseMaxAge(options.maxAge)}`);

  if (options.enforce) {
    directives.push("enforce");
  }

Did you mean this? Is there any constraint for the max age or just literally to reorder lines?

I would like to claim this one.

@EvanHahn
Copy link
Member Author

EvanHahn commented Jul 27, 2020 via email

@CyberZujo
Copy link
Contributor

CyberZujo commented Jul 28, 2020

` it("can enable enforcement", async () => {
    await check(expectCt({ enforce: true }), {
      "expect-ct": "enforce, max-age=123",
    });
  });

  it("can explicitly disable enforcement", async () => {
    await check(expectCt({ enforce: false }), {
      "expect-ct": "max-age=123",
    });
  });
`

I've found these cases and the previous value of max-age was set to 0, probably now since it's reordered, I have to provide a value, so is that okay or, it is in

test/expect-ct.test.ts

You can assign this to me and I'll make a PR.

@EvanHahn
Copy link
Member Author

I'd make the change and then run npm test to see what breaks, and then fix those tests. At least one of the tests you showed should change, though it's possible that there are others.

I can't assign this issue to you (GitHub doesn't allow me to) but feel free to make a pull request.

@EvanHahn
Copy link
Member Author

@CyberZujo I'm planning to release the next major version of Helmet on Sunday, August 2 (a few days from now). Would you mind waiting until then? No problem if not, but I think it will be less work if we wait.

@CyberZujo
Copy link
Contributor

Yes, no problem, I'll wait until then and then I'll make the PR.

@EvanHahn
Copy link
Member Author

EvanHahn commented Aug 2, 2020

Helmet 4 (and expect-ct@1.0.0) was just released, so feel free to go ahead with your change. No rush, though!

EvanHahn pushed a commit that referenced this issue Sep 28, 2020
See [#264][0] and [#225][1].

[0]: #264
[1]: #225
@EvanHahn
Copy link
Member Author

Closed by #264.

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

No branches or pull requests

2 participants