Skip to content
This repository has been archived by the owner on Jun 30, 2020. It is now read-only.

[feature-request] Allow array of domains as options #6

Closed
cristiandouce opened this issue Feb 4, 2016 · 11 comments
Closed

[feature-request] Allow array of domains as options #6

cristiandouce opened this issue Feb 4, 2016 · 11 comments

Comments

@cristiandouce
Copy link

Something like:

app.use(helmet.frameguard("allow-from", ["https://domain1.com", "https://domain2.com"]))

Perhaps also a regex?

@EvanHahn
Copy link
Member

EvanHahn commented Feb 4, 2016

Every spec I found showed that allow-from can only have one domain in it. Have you found something that says we can add multiple origins?

@cristiandouce
Copy link
Author

Have you found something that says we can add multiple origins?

No. Yet it's a common dev issue I've encountered in many projects when you handle different resources.

I was thinking that it could be handled like this internally:

function frameguard (action, options) {
  // ...
  if ('allow-from' === action) {
    let allowFrom = (options.indexOf(req.hostname) >= 0 ? req.hostname : options[0];
    req.setHeader('X-Frame-Options', 'ALLOW-FROM ' + allowFrom);
  }
  // ...
}

for the case options is an array of strings. or some similar validation. I could send a working PR later if you are ok to support such feature.

@cristiandouce cristiandouce changed the title Allow array of domains as options [feature-request] Allow array of domains as options Feb 4, 2016
@EvanHahn
Copy link
Member

EvanHahn commented Feb 5, 2016

This is a good idea.

Two thoughts:

  1. I've tried to shy away from Express-specific features in Helmet, and req.hostname is one of those. I haven't been able to avoid them completely (the HSTS middleware uses a property set by Express). I'm happy to do this, as long as we add a caveat: if you use allow-from with an array, you'll need to set req.hostname.
  2. I'd love to make sure that this works; is req.hostname a property we can rely on? Does it have its own security issues?

I'd definitely love a pull request if you're willing.

@binarykitchen
Copy link

Yes, I need that feature too! Why the wait?

@cristiandouce
Copy link
Author

I've managed to create a PR with this feature. I've been trying to fix the tests. But current impl is not very informative. I could use some help figuring out what is going on with the last 2.

@cristiandouce
Copy link
Author

  15 passing (100ms)
  2 failing
     AssertionError: Missing expected exception..
      at _throws (assert.js:337:5)
      at Function.assert.throws (assert.js:361:3)
      at Context.<anonymous> (test/index.js:140:14)
  2) frameguard with improper input fails with a bad domain if the action is "ALLOW-FROM":
     AssertionError: Missing expected exception..
      at _throws (assert.js:337:5)
      at Function.assert.throws (assert.js:361:3)
      at Context.<anonymous> (test/index.js:155:14)

@cristiandouce
Copy link
Author

Found it! n/m. PR is ready for review 👯

@EvanHahn
Copy link
Member

EvanHahn commented Sep 8, 2016

Those assertion errors should be made more informative.

Could you submit the PR and I'll take a look, even if it's incomplete?

@cristiandouce
Copy link
Author

@EvanHahn It's passing green now! 😄

@EvanHahn
Copy link
Member

EvanHahn commented Sep 8, 2016 via email

@EvanHahn
Copy link
Member

EvanHahn commented Oct 7, 2016

Closing in favor of #12.

@EvanHahn EvanHahn closed this as completed Oct 7, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging a pull request may close this issue.

3 participants