[PWA-680] Webpagetest security score is too low #2548

tjwiebell · 2020-07-10T19:14:01Z

Description

Steps -

Run webpagetest for https://develop.pwa-venia.com/ WPT URL - https://www.webpagetest.org/

Expected - New Security should show better results.
Actual - Security score is F.

Related Issue

[PWA-680] Webpagetest security score is too low

Acceptance

Verification Stakeholders

Specification

Verification Steps

Passing score on WPT

https://snyk.io/test/website-scanner/?test=200713_3N_d910d2505fdbd03725cba0a30172769c&utm_medium=referral&utm_source=webpagetest&utm_campaign=website-scanner

Since we're introducing a very opinionated CSP rule in this PR, it would also be good to verify this plays nicely with various setups (Both UPWARDs, onboard/backend image opt).

Screenshots / Screen Captures (if appropriate)

Checklist

I have added tests to cover my changes, if necessary.

I have updated the documentation accordingly, if necessary.

PWAStudioBot · 2020-07-10T19:24:15Z

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Associated JIRA tickets: PWA-680.

Generated by 🚫 dangerJS against 680f8f9

devops-pwa-codebuild · 2020-07-10T19:31:54Z

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2548.pwa-venia.com : LH Performance Expected 0.85 Actual 0.51, LH Best Practices Expected 1 Actual 0.92, WPT Cache Expected 90 Actual 83.333333333333
https://pr-2548.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.34, LH Best Practices Expected 1 Actual 0.92
https://pr-2548.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.41, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92, WPT Cache Expected 65 Actual 52.333333333333

jimbo · 2020-07-10T21:35:48Z

packages/venia-ui/upward.yml

+                x-frame-options:
+                  inline: SAMEORIGIN
+                x-xss-protection:
+                  inline: '1; mode=block'


This may be unnecessary. Does WPT expect it?

The ones included are the five specific headers WPT looks for, dings for each missing one. The browsers that don't support CSP and need this rule we probably don't support anyway, so that's a fair point. We have the option to not include them and defend whatever score WPT decides to give us.

As long as WPT cares, we can include them.

- Write an interceptor that injects them into the app shell

zetlen

LGTM, this is a perfect target-plus-interceptor kind of implementation. The future is now.

Possible improvements for later:

An UPWARD MergeResolver, so that other definitions could reference these headers, instead of this extension injecting them into Venia
Env var definitions for controlling values like max-age, or additional domains for CSP to allow
Unit tests, a README for the extension, etc

None are blockers for now. Approved.

zetlen · 2020-07-14T21:37:07Z

packages/venia-concept/package.json

    "@magento/peregrine": "~6.0.0",
    "@magento/pwa-buildpack": "~5.1.1",
    "@magento/upward-js": "~4.0.1",
+    "@magento/upward-security-headers": "*",


This probably should follow the pattern of other deps and be ~0.0.1.

Fixed to this constraint in 27e45b1

packages/extensions/upward-security-headers/intercept.js

Co-authored-by: James Zetlen <[email protected]>

zetlen

Hey, thank you for troubleshooting the scaffolding stuff. Your fixes work, but might need future maintenance. I had a couple of ideas to simplify the code--let me know if you want to apply them.

zetlen · 2020-07-22T19:02:14Z

packages/pwa-buildpack/lib/cli/create-project.js

    if (process.env.DEBUG_PROJECT_CREATION) {
        prettyLogger.info('Debug: Removing generated tarballs');
        const pkgDir = require('pkg-dir');
-        const monorepoDir = resolve(pkgDir.sync(__dirname), '../../');
+        const packagesDirectory = resolve(pkgDir.sync(__dirname), '../');
        prettyLogger.info(
-            execa.shellSync('rm -v packages/*/*.tgz', { cwd: monorepoDir })
-                .stdout
+            execa.shellSync('find . -name "*.tgz" -delete -print', {
+                cwd: packagesDirectory
+            }).stdout
        );
    }


I don't know why I didn't think of this before, but...what if we just put tarballs in package roots in .gitignore? I just tested it and it worked fine. We could just remove this section in create-project.js entirely. Which would be nice, since now we're not looking for DEBUG_PROJECT_CREATION in two places.

Gitignore additions:

## May temporarily create tarballs during scaffolding debug packages/*/*.tgz packages/extensions/*/*.tgz

Got rid of cleanup and added a gitignore rule in 9a0ce84

zetlen · 2020-07-22T19:23:45Z

packages/venia-concept/_buildpack/create.js

    const workspaceDir = resolve(__dirname, '../../');
-    fs.readdirSync(workspaceDir).forEach(packageDir => {
+    const extensionDir = resolve(workspaceDir, 'extensions');
+
+    const packageDirs = [
+        ...fs.readdirSync(workspaceDir),
+        ...fs.readdirSync(extensionDir).map(path => `extensions/${path}`)
+    ];
+


This works, but I think it would be a little more robust to just ask Yarn what the current packages are.

// Assume we are in the pwa-studio repo and get its root. const monorepoDir = resolve(__dirname, '../../../'); // The Yarn "workspaces info" command outputs JSON as of v1.22.4. // The -s flag suppresses all other non-JSON logging output. const yarnWorkspaceInfoCmd = 'yarn -s workspaces info'; let workspaceInfo = require('child_process').execSync( yarnWorkspaceInfoCmd, { cwd: monorepoDir } ); let packageDirs; try { packageDirs = Object.values(JSON.parse(workspaceInfo)).map( ({ location }) => resolve(monorepoDir, location) ); } catch (e) { // In case Yarn breaks this in the future, throw a more informative // error than your typical JSONError. throw new Error( `Could not parse output of '${yarnWorkspaceInfoCmd}:\n${workspaceInfo}` ); }

Love it, made this change in 9a0ce84.

zetlen · 2020-07-22T19:25:41Z

packages/venia-concept/_buildpack/create.js

        const packagePath = resolve(workspaceDir, packageDir);
        if (!fs.statSync(packagePath).isDirectory()) {
            return;


If you use the Yarn method in my other comment, you don't need to do this resolve and stat check. packageDirs will contain only absolute-path directories.

Much cleaner, implemented in 9a0ce84. This method additionally filters out workspaces without package.json files, so I was able to get rid of that check too.

jimbo

Works for me. 🚀

devpatil7 · 2020-07-24T22:47:02Z

@tjwiebell WPT security score looks great on all configs (img_opt - onboard/backend, upward-php/js). Issue I'm seeing with braintree payment module - Infinite loading indicator with below console error -

Refused to frame 'https://assets.braintreegateway.com/' because it violates the following Content Security Policy directive: "child-src 'self'". Note that 'frame-src' was not explicitly set, so 'child-src' is used as a fallback.

tjwiebell · 2020-07-27T15:47:36Z

@tjwiebell WPT security score looks great on all configs (img_opt - onboard/backend, upward-php/js). Issue I'm seeing with braintree payment module - Infinite loading indicator with below console error -

@dpatil-magento - Added an additional rule in 092cd5a that allows iframes to load from that braintree domain. Good catch!

devpatil7 · 2020-07-27T19:58:56Z

QA Approved.

Test some inline definitions to see if this passes the WPT scan

a976bfb

PWAStudioBot added the pkg:venia-ui label Jul 10, 2020

feat(targets): Add target for editing UPWARD definitions

cae5598

jimbo previously approved these changes Jul 10, 2020

View reviewed changes

m2-community-project bot assigned jimbo Jul 10, 2020

m2-community-project bot added the Progress: approved label Jul 10, 2020

- Move security definitions into a new package

8fe6be9

- Write an interceptor that injects them into the app shell

tjwiebell dismissed jimbo’s stale review via 8fe6be9 July 13, 2020 14:08

Merge branch 'develop' into tommy/wpt-fix-with-upward

a1a2876

tjwiebell marked this pull request as ready for review July 13, 2020 14:15

PWAStudioBot added pkg:pwa-buildpack pkg:venia-concept pkg:extensions labels Jul 13, 2020

m2-community-project bot assigned tjwiebell Jul 13, 2020

m2-community-project bot added Progress: review and removed Progress: approved labels Jul 13, 2020

Add new package to docker configs

b189640

zetlen previously approved these changes Jul 14, 2020

View reviewed changes

m2-community-project bot assigned zetlen Jul 14, 2020

m2-community-project bot added Progress: approved and removed Progress: review labels Jul 14, 2020

Fix to version and add clean script

27e45b1

tjwiebell dismissed zetlen’s stale review via 27e45b1 July 17, 2020 14:25

tjwiebell and others added 4 commits July 17, 2020 09:48

Update packages/extensions/upward-security-headers/intercept.js

9ea1390

Co-authored-by: James Zetlen <[email protected]>

Fix failing test

140cda8

Fix for scaffolding to support packages in extensions directory

4d5b15b

Merge branch 'develop' into tommy/wpt-fix-with-upward

400f3bd

zetlen reviewed Jul 22, 2020

View reviewed changes

Implement PR suggestions

9a0ce84

m2-community-project bot added Progress: review and removed Progress: approved labels Jul 23, 2020

tjwiebell added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Jul 23, 2020

Fixup failing test

caa7c6b

jimbo previously approved these changes Jul 23, 2020

View reviewed changes

devpatil7 added 2 commits July 24, 2020 16:07

Merge branch 'develop' into tommy/wpt-fix-with-upward

a1e66e9

Merge branch 'develop' into tommy/wpt-fix-with-upward

0d872ec

Add Braintree to CSP rule

092cd5a

tjwiebell dismissed jimbo’s stale review via 092cd5a July 27, 2020 15:44

devpatil7 added 2 commits July 27, 2020 11:58

Merge branch 'develop' into tommy/wpt-fix-with-upward

37d5a0b

Merge branch 'develop' into tommy/wpt-fix-with-upward

680f8f9

jimbo approved these changes Jul 27, 2020

View reviewed changes

devpatil7 merged commit 77ab096 into develop Jul 27, 2020

m2-community-project bot removed the Progress: review label Jul 27, 2020

devpatil7 deleted the tommy/wpt-fix-with-upward branch July 27, 2020 20:05

tjwiebell mentioned this pull request Aug 24, 2020

feat(targets): Add target for editing UPWARD definitions (DRAFT) #2459

Closed

Uh oh!

[PWA-680] Webpagetest security score is too low #2548

[PWA-680] Webpagetest security score is too low #2548

Uh oh!

Conversation

tjwiebell commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

Uh oh!

PWAStudioBot commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devops-pwa-codebuild commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zetlen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zetlen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimbo left a comment

Choose a reason for hiding this comment

Uh oh!

devpatil7 commented Jul 24, 2020

Uh oh!

tjwiebell commented Jul 27, 2020

Uh oh!

devpatil7 commented Jul 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tjwiebell commented Jul 10, 2020 •

edited

Loading

PWAStudioBot commented Jul 10, 2020 •

edited

Loading

devops-pwa-codebuild commented Jul 10, 2020 •

edited

Loading