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

Upgrading graphql-playground-middleware-express. #2482

Merged

Conversation

revanth0212
Copy link
Contributor

Description

Fixes the graphql-playground-middleware-express security issue.

https://github.com/magento/pwa-studio/network/alert/yarn.lock/graphql-playground-html/open

Related Issue

Closes PWA-678

Acceptance

Gql Playground should work as expected and the GitHub security issue should be resolved.

Verification Stakeholders

@zetlen
@jimbo

Verification Steps

  1. yarn watch:venia
  2. Open the deployed graphiql app in the browser.
  3. Should not fail when an operation is performed.

Checklist

None

@revanth0212 revanth0212 added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Jun 10, 2020
@m2-community-project m2-community-project bot added this to Ready for Review in Pull Request Progress Jun 10, 2020
@revanth0212
Copy link
Contributor Author

@zetlen the security issue also suggests that we use a sanitizer to clean the endpoint before providing it to the playground middleware. We are not doing that in our current implementation of PWADevServer.js. Do you think I should add that as part of this PR?

Here is their suggestion:

const { expressPlayground } = require('graphql-playground-middleware-express');
const { sanitizeUrl } = require('@braintree/sanitize-url');

const qs = require('querystringify');

const { renderPlaygroundPage } = require('graphql-playground-html');

module.exports = (req, res, next) => {
	const { endpoint } = qs.parse(req.url)
	res.html(expressPlayground({endpoint: sanitizeUrl(endpoint) })).status(200)
	next()
}

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Jun 10, 2020

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-678.

Generated by 🚫 dangerJS against 5393553

@devops-pwa-codebuild
Copy link
Collaborator

devops-pwa-codebuild commented Jun 10, 2020

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-2482.pwa-venia.com : LH Performance Expected 0.85 Actual 0.56, LH Best Practices Expected 1 Actual 0.92
https://pr-2482.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.34, LH Best Practices Expected 1 Actual 0.92
https://pr-2482.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.49, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92

@@ -83,6 +83,7 @@
"resolutions": {
"graphql": "~14.3.1",
"**/graphql-cli/npm-run": "~5.0.0",
"**/graphql-cli/graphql-playground-middleware-express": "~1.7.18",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bummer that we can't just update this version everywhere since 1.7.12 is hardcoded in the last patch in v3 of graphql-cli. If they just used ~ then we could have updated this without resolutions!

@m2-community-project m2-community-project bot moved this from Ready for Review to Review in Progress in Pull Request Progress Jun 10, 2020
Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved! The only alternative I could see is upgrading to graphql-cli@4.0.0 but that's a major change and would require more time and attention.

@revanth0212
Copy link
Contributor Author

Approved! The only alternative I could see is upgrading to graphql-cli@4.0.0 but that's a major change and would require more time and attention.

Yeah dude that would be a major change.

@m2-community-project m2-community-project bot moved this from Review in Progress to Reviewer Approved in Pull Request Progress Jun 11, 2020
@dpatil-magento dpatil-magento merged commit 0e6f10b into develop Jun 11, 2020
@dpatil-magento dpatil-magento deleted the revanth/graphql-playground-middleware-security-fix branch June 11, 2020 18:40
@m2-community-project m2-community-project bot moved this from Reviewer Approved to Done in Pull Request Progress Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:pwa-buildpack version: Minor This changeset includes functionality added in a backwards compatible manner.
Development

Successfully merging this pull request may close these issues.

None yet

6 participants