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

xframe ALLOW-FROM is obsolete in all modern browsers #6766

Open
hkovacs opened this issue Jun 22, 2020 · 10 comments
Open

xframe ALLOW-FROM is obsolete in all modern browsers #6766

hkovacs opened this issue Jun 22, 2020 · 10 comments

Comments

@hkovacs
Copy link

@hkovacs hkovacs commented Jun 22, 2020

Describe the bug
Using xframe ALLOW-FROM throws error in console: Invalid 'X-Frame-Options' header encountered when loading 'https://api.dev.mysite.com/uploads/my-file.pdf': 'ALLOW-FROM dev.mysite.com' is not a recognized directive. The header will be ignored.

https://strapi.io/documentation/v3.x/concepts/middlewares.html#response-middlewares
instructions for xframe indicate ALLOW-FROM which is obsolete per https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Frame-Options

Steps to reproduce the behavior
See docs

Expected behavior
i guess allow-from needs to be changed.

i tried using frame-ancestors but it also wasnt recognized, so i dont have any suggestions at the moment

Screenshots
none

Code snippets

        xframe: {
            enabled: true,
            value: "frame-ancestors dev.mysite.com"
        },

System
Node.js version: v10.16.3
NPM version: 6.9.0
Strapi version: Strapi v3.0.4
Database: mongo v4.0.12
Operating system: ubuntu 18.04

Additional context
Thank you!

@derrickmehaffy
Copy link
Member

@derrickmehaffy derrickmehaffy commented Jun 22, 2020

Talked with @hkovacs via slack and I can confirm this is outdated, we may need to dig into the middleware and see if there is an updated or replacement for it.

@derrickmehaffy
Copy link
Member

@derrickmehaffy derrickmehaffy commented Jun 22, 2020

@lauriejim
Copy link
Member

@lauriejim lauriejim commented Jun 23, 2020

Thank you, I think this is a topic for @alexandrebodin .
If you already investigated and found a solution / alternative, feel free to submit a PR 👨‍💻
Thank you 👍

@hkovacs
Copy link
Author

@hkovacs hkovacs commented Jun 23, 2020

@alexandrebodin

i have started on this, but it is a bit above my level...

i have made the following surface changes:

diff --git a/packages/strapi/lib/middlewares/csp/index.js b/packages/strapi/lib/middlewares/csp/index.js
index e9b58903c..ddcbbd10a 100644
--- a/packages/strapi/lib/middlewares/csp/index.js
+++ b/packages/strapi/lib/middlewares/csp/index.js
@@ -1,9 +1,9 @@
 'use strict';
 
 const convert = require('koa-convert');
-const { csp } = require('koa-lusca');
+const { contentSecurityPolicy } = require('koa-helmet');
 /**
- * CSP hook
+ * contentSecurityPolicy hook
  */
 
 module.exports = strapi => {
@@ -16,7 +16,7 @@ module.exports = strapi => {
       strapi.app.use(async (ctx, next) => {
         if (ctx.request.admin) return await next();
 
-        return await convert(csp(strapi.config.middleware.settings.csp))(
+        return await convert(contentSecurityPolicy(strapi.config.middleware.settings.csp))(
           ctx,
           next
         );
diff --git a/packages/strapi/lib/middlewares/hsts/index.js b/packages/strapi/lib/middlewares/hsts/index.js
index 225ca2356..241cee78d 100644
--- a/packages/strapi/lib/middlewares/hsts/index.js
+++ b/packages/strapi/lib/middlewares/hsts/index.js
@@ -4,7 +4,7 @@
  * Module dependencies
  */
 const convert = require('koa-convert');
-const { hsts } = require('koa-lusca');
+const { hsts } = require('koa-helmet');
 
 /**
  * HSTS hook
diff --git a/packages/strapi/lib/middlewares/p3p/index.js b/packages/strapi/lib/middlewares/p3p/index.js
index 295840da6..24cac44ec 100644
--- a/packages/strapi/lib/middlewares/p3p/index.js
+++ b/packages/strapi/lib/middlewares/p3p/index.js
@@ -4,7 +4,7 @@
  * Module dependencies
  */
 const convert = require('koa-convert');
-const { p3p } = require('koa-lusca');
+const { p3p } = require('koa-helmet');
 /**
  * P3P hook
  */
diff --git a/packages/strapi/lib/middlewares/xframe/index.js b/packages/strapi/lib/middlewares/xframe/index.js
index 4a2074302..be3fb340f 100644
--- a/packages/strapi/lib/middlewares/xframe/index.js
+++ b/packages/strapi/lib/middlewares/xframe/index.js
@@ -1,7 +1,7 @@
 'use strict';
 
 const convert = require('koa-convert');
-const { xframe } = require('koa-lusca');
+const { xframe } = require('koa-helmet');
 
 /**
  * CRON hook
diff --git a/packages/strapi/lib/middlewares/xss/index.js b/packages/strapi/lib/middlewares/xss/index.js
index 5ae3556dc..d2ffccd5f 100644
--- a/packages/strapi/lib/middlewares/xss/index.js
+++ b/packages/strapi/lib/middlewares/xss/index.js
@@ -1,7 +1,7 @@
 'use strict';
 
 const convert = require('koa-convert');
-const { xssProtection } = require('koa-lusca');
+const { xssFilter } = require('koa-helmet');
 
 module.exports = strapi => {
   return {
@@ -11,7 +11,7 @@ module.exports = strapi => {
       strapi.app.use(async (ctx, next) => {
         if (ctx.request.admin) {
           return await convert(
-            xssProtection({
+            xssFilter({
               enabled: true,
               mode: defaults.xss.mode,
             })
@@ -20,7 +20,7 @@ module.exports = strapi => {
 
         const xssConfig = strapi.config.get('middleware.settings.xss');
         if (xssConfig.enabled) {
-          return await convert(xssProtection(xssConfig))(ctx, next);
+          return await convert(xssFilter(xssConfig))(ctx, next);
         }
 
         await next();
diff --git a/packages/strapi/package.json b/packages/strapi/package.json
index fd7b0d3b1..4edc59f4d 100644
--- a/packages/strapi/package.json
+++ b/packages/strapi/package.json
@@ -35,10 +35,10 @@
     "koa-compress": "^3.0.0",
     "koa-convert": "^1.2.0",
     "koa-favicon": "^2.0.0",
+    "koa-helmet": "^5.2.0",
     "koa-i18n": "^2.1.0",
     "koa-ip": "^2.0.0",
     "koa-locale": "~1.3.0",
-    "koa-lusca": "~2.2.0",
     "koa-router": "^7.4.0",
     "koa-session": "^5.12.0",
     "koa-static": "^5.0.0",

i dont know where else to go with these changes, so i am happy for some pointers on where to look next... or perhaps this is all that is needed...

@hkovacs
Copy link
Author

@hkovacs hkovacs commented Jun 23, 2020

i do think that this is a priority high given that the bug means that strapi is not fully secure while using koa-lusca which has been unsupported since before strapi was created and koa-lusca packages are obsolete in modern browsers.

@derrickmehaffy
Copy link
Member

@derrickmehaffy derrickmehaffy commented Jun 24, 2020

I would also class this as a bug @lauriejim (though I wouldn't say high) we do need to find a replacement.

@alexandrebodin
Copy link
Member

@alexandrebodin alexandrebodin commented Jun 24, 2020

It would be fine to migrate to helmet but if we cannot map the old options to the new ones it will be a breaking change so won't happen until v4.

@hkovacs
Copy link
Author

@hkovacs hkovacs commented Jun 24, 2020

It would be fine to migrate to helmet but if we cannot map the old options to the new ones it will be a breaking change so won't happen until v4.

im happy to attempt a try at doing this if someone can tell me where to look. it might take the same amount of time whether i help or wait for v4, so i am willing to try.

@alexandrebodin
Copy link
Member

@alexandrebodin alexandrebodin commented Jun 26, 2020

Just so you have the full picture. v4 will only come at the end of year. If you want to work for the v4 then you can go further and merge the multiple middlewares into one middleware with options for each security layers. This would make it simpler for users to configure all their security middlewares at once.

If you want to work on a v3 comptabile solution then you will need to work on keeping the same options and middlewares :)

@hkovacs
Copy link
Author

@hkovacs hkovacs commented Jun 26, 2020

Just so you have the full picture. v4 will only come at the end of year. If you want to work for the v4 then you can go further and merge the multiple middlewares into one middleware with options for each security layers. This would make it simpler for users to configure all their security middlewares at once.

If you want to work on a v3 comptabile solution then you will need to work on keeping the same options and middlewares :)

with time i will study v3 middleware and get a better grasp. see how it goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants