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

graphql: download external assets, use go generate #19727

Closed
wants to merge 3 commits into from

Conversation

kurkomisi
Copy link
Contributor

The graphql explorer relies on external cdn files, which is unsafe.
This PR discards the external dependencies, and embeds their binary version into the assets.go file, which guarantees that the files won't be compromised.

@kurkomisi kurkomisi requested a review from gballet as a code owner June 17, 2019 12:38
@karalabe
Copy link
Member

Please add linter ignores to avoid trigerring the misspell.

@kurkomisi
Copy link
Contributor Author

kurkomisi commented Jun 17, 2019

I tried to generalize the shell commands, but it became complicated, and didn't work properly.

@karalabe
Copy link
Member

I think I just blew this PR. I had one open against GraphQL that I thought we already merged. Now that that's in, you'll need to rebase this, sorry.

@karalabe
Copy link
Member

Hmm, also, you seemed to have only embedded the raw resource files. That's a bit problematic because we've no idea what version they are at or how to update them. Isn't there some npm black magic through which we could ask for a particular version number, or to update to latest or something? This won't be scalable.

@holiman
Copy link
Contributor

holiman commented Jun 20, 2019

An alternative variant is to keep using the CDN, but also use the hash of the file. See https://www.w3.org/TR/SRI/
You can use https://www.srihash.org/ to calculate the correct hashes. Note though, it requires that we link to specific versions, not things like foobar-latest.min.js.

@kurkomisi
Copy link
Contributor Author

Superseded by #19742.

@kurkomisi kurkomisi closed this Jun 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants