[feature-request] Allow domain to be specified as a function
ruhnowg opened this issue · 22 comments
This would allow users to implement special handling of X-Frame-Options in the case that they want to support many different domains. I was thinking that the api could be something along the lines of a function that returns a string. If a string is returned set allow-from to be that string, otherwise if null is returned DENY. I'm happy to do a pull request for this if people think it is a good idea. I feel like #6 could simply be implemented as a special case of this.
Can you give me an example of a function you'd like to use?
Sure, I was thinking about something along the lines of:
function(req) {
if (/.*foo\.com/.test(req.hostname)) {
return req.hostname;
}
}
This would essentially allow the app to be embedded into any page matching the regex, ie bar.foo.com or baz.foo.com, but not any random site. Anyone consuming this would need to be very careful not to cache the response since the response would be the same but might have different headers depending on where the request was made from.
Edit: Basically I'm attempting to emulate the frame-ancestors option outlined here because it doesn't look like csp 2.0 is supported by ie yet.
True, but req.hostname will be your domain, not the domain of the parent page. See my comment here.
@EvanHahn Oops, you're right. I think req.headers.referer would do the trick though.
If I'm not mistaken, the Referer header isn't always to be trusted, especially with some of these new referrer control policies that browsers are implementing.
That being said, if a developer wants to trust the Referer header despite its inconsistencies, this feature request would allow that.
What do you think?
@EvanHahn I agree. It's possible that the referer header might not be present / be trusted, but this would leave it up to the developer to decide if this is acceptable or not.
Yeah, we totally 'need' this if you want to allow your application to be embedded within multiple sites.
For other visitors who might not immediately understand why This article here https://blogs.msdn.microsoft.com/ieinternals/2010/03/30/combating-clickjacking-with-x-frame-options/ gives the reasoning behind it (the example used there is to pass a query parameter to the app to allow a 'pre-agreed' set of values to be checked off against)
However given the lack of browser support for 'ALLOW-FROM' I totally understand why this might not be prioritised ;)
It's got mixed browser support, but Content Security Policy's frame-ancestors directive might be helpful here. Helmet's Content Security Policy middleware can help with CSP if that's a route you take.
If you want to have multiple ALLOW-FROMs with this module, I can only think of one way to do it, and it involves some "authorization" on the embedder's behalf. Here's how it might work.
Let's say example.com is trying to put one of my pages, evanhahn.com/iframe, in an iframe.
example.com might have the following HTML:
<iframe src="https://evanhahn.com/iframe?domain=https%3A%2F%2Fevanhahn.com"></iframe>evanhahn.com might have the following server-side code:
var ALLOWED_BY = new Set([
'http://evanhahn.com',
'https://evanhahn.com',
// ...
])
app.get('/iframe', function (req, res, next) {
if (ALLOWED_BY.has(req.query.domain)) {
frameguard({
action: 'allow-from',
domain: String(req.query.domain)
})(req, res, function (err) {
if (err) { return next(err) }
// send the iframe HTML here...
res.sendFile('/path/to/iframe.html')
})
} else {
// They didn't provide a domain, so let's do nothing.
// Why send the iframe if they're not authenticated?
res.status(403)
res.end()
}
})You can do this with Frameguard today. (To be fair, I didn't test the code above, so it may not work.)
We could build this functionality into Frameguard, but it probably wouldn't be as powerful as the above. For example, we couldn't elegantly handle the else condition from above.
What do people think? Is this something we should add to this module?
interesting, I didn't think of calling it directly per request (rather than at setup-time) that would suffice for me ;)
If that suffices for everyone, I'll close this issue. If we think Frameguard should support this somehow, I'll leave it open to discuss.
above solution is still a pain when you just want to use app.use(helmet.frameguard({...})) for any requests. still would be good to have support for the initial ticket issue.
@binarykitchen Would something like this work?
var ALLOWED_BY = new Set([
'http://evanhahn.com',
'https://evanhahn.com',
// ...
])
app.use(function (req, res, next) {
if (ALLOWED_BY.has(req.query.domain)) {
frameguard({
action: 'allow-from',
domain: String(req.query.domain)
})(req, res, next)
} else {
next()
}
})Or would you prefer this to be built into Frameguard?
@EvanHahn sorry for late response - yes, better build this into frameguard. make it simple for the programmer to use. something KISS like
app.use(helmet.frameguard({
action: 'allow-from',
domains: ['https://staticxx.facebook.com', 'http://letsencrypt.com']
}))I think the apparent ease of the code snippet you posted is outweighed by the complexity of what's going on, and the code snippet I posted is more straightforward.
My solution seems like it'd work, but I feel weird adding something so nonstandard to this module, especially when the code snippet above is relatively brief. "Consumers" of your iframes will need to set a query string parameter, which feels strange.
Thoughts?
well i leave it to you @EvanHahn to make the final decision. i don't see any disadvantages with my suggestion because it clearly says what it wants and leaves the rest to the black box to solve internally.
just saying that the API of libraries like these should be kept as simple as possible, optimised for the processor in our skulls. we developers already have a too high cognitive load (i can show you slides about that). so again, keep it as simple as possible if you can ...
Added something to the readme about this. Closing.
@EvanHahn thanks!
@aglemann I removed the text from the readme because I no longer think it's a good way to do this. ALLOW-FROM has mixed browser support and this setup is nonstandard.
There's also a much better alternative: Content-Security-Policy's frame-ancestors directive.
Does that answer your question?
Does that answer your question?
Yes tks, is it still considered best practice to use both headers for IE support?
It depends on your desired browser support. I think ALLOW-FROM generally creates more developer headache than it's worth, but your interests may vary.
Does that help?