Implement client-side version checking #79

siggy · 2017-12-20T23:49:17Z

Previously Conduit would render an iframe, received from
versioncheck.conduit.io.

Modify the client to retrieve the latest released version, via CORS.

briansmith · 2017-12-22T18:57:08Z

web/app/js/components/Version.jsx

+      return "Conduit is up to date";
+    } else {
+      return (<div>
+        A new version ({this.state.latest}) is available<br />


I don't know anything about react. What ensures that this is safe w.r.t. xss? I.e. let's say this.state.latest is "<script>alert("hi");</script>", does this syntax do the escaping to make that safe?

Good question!

I modified my test response to be:

{ "version": "<script>alert('hi');</script>" }

...and got:

Is that sufficient? If there's a more malicious way to craft that json, I can try it pretty quickly.

And you're correct @briansmith -- the {...} syntax has built in escaping, documented here: https://reactjs.org/docs/jsx-in-depth.html#string-literals-1

@siggy @klingerf Thanks! I agree that's what's wanted.

rmars

😻 thanks for taking this on!
I haven't done as much investigation into you as this, but I was wondering whether fetch allows us to do this without adding another library?

rmars · 2017-12-21T20:37:17Z

web/app/css/version.css

+    text-decoration: none;
+  }
+  & a.button:active {
+    background-color: #2F80ED;


would advise you to use the color var from styles.css since we're importing that file anyway.... also looking at that file, we have that color defined twice :/ we should probably consolidate. (though if we want to have a separate file for styles directly copied, we can)

rmars · 2017-12-21T20:41:51Z

web/app/js/components/Version.jsx

+  }
+
+  loadFromServer() {
+    if (this.state.done) {


variable naming nit: we've been using pendingRequests for this elsewhere, it'd be nice to use the same name

rmars · 2017-12-21T20:44:53Z

web/app/test/VersionTest.jsx

+  it('renders update message when versions do match', () => {
+    loadFromServer = setResponse(null, {responseText: "{\"version\": \"v2.3.4\"}"});
+
+    // must wrap Version in a BrowserRouter because this test case renders a Link,


@franziskagoltz added a routerWrap function in testHelpers, maybe we can use that?

i could not find a way to make routerWrap work with properties, so leaving this as is.

rmars · 2017-12-21T21:06:45Z

web/app/css/version.css

+
+  /* from https://conduit.io/ */
+
+  & a.button {


Since we probably want to use the same styles everywhere if we add future buttons, I'd put the a.button styles in styles.css (or create a buttons.css, if we want to just have an isolated "coped this from conduit.io styles" file)

rmars · 2017-12-21T21:08:58Z

web/app/test/VersionTest.jsx

+  let loadFromServer;
+
+  function setResponse(err, resp) {
+    return sinon.stub(Version.prototype, 'loadFromServer').callsFake(function fakeFn() {


yup... though removing this because fetch does what we need!

briansmith · 2017-12-23T00:02:38Z

My main interest in this is the security-critical stuff, which seems to have been addressed so +1 from me.

However, I wonder where the best place to format the output really is. It seems like with the proposed change we have less flexibility than if we were to do the formatting on our own host. For example, how would we indicate in the web UI the update's urgency, e.g. a critical security update vs. a "whenever you get around to it" update? It seems like it would be good to let the server give us some free-form text to describe the urgency, at least.

siggy · 2017-12-23T02:31:05Z

@briansmith I agree some kind of richer free-form messaging here is a good idea. I'd like to defer it for a subsequent PR. As our version check endpoint is json (kevin's bright recommendation), we can iterate on it and add fields in a backwards compatible way. I've filed #90 to track this.

xiaods · 2017-12-28T19:07:15Z

versioncheck.conduit.io/update is public domain, could you please consider enterprise-level environment, it can't directly access internet directly.

siggy · 2017-12-28T19:37:57Z

@xiaods fair point. i think for this initial PR we'll hit the public URL. i could imagine future versions supporting some kind of proxy setting to enable outside internet access (or something else entirely). if you have more thoughts, please summarize them in a Github issue so we can consider it for our roadmap.

rmars

🌟 🙌 🚀 yayy! thanks for doing this!

Just had a couple small comments.

rmars · 2018-01-02T21:01:44Z

web/app/js/components/Version.jsx

+    super(props);
+    this.loadFromServer = this.loadFromServer.bind(this);
+    this.handleApiError = this.handleApiError.bind(this);
+    this.state = {err: null, latest: null, loaded: false, pendingRequests: false,};


extra trailing comma

rmars · 2018-01-02T21:10:20Z

web/app/css/version.css

+.version {
+  padding: 0 0 0 9px;
+  font-size: 13px;
+  font-weight: 700;


nit: --font-weight-bold

rmars · 2018-01-02T21:12:05Z

web/app/js/components/Version.jsx

+    this.setState({
+      loaded: true,
+      pendingRequests: false,
+      err: e


nit: in other places we do this we use "error" and I'd prefer us to be consistent

rmars

🐑 thanks for updating!

Previously Conduit would render an iframe, received from versioncheck.conduit.io. Modify the client to retrieve the latest released version, via CORS. Signed-off-by: Andrew Seigner <andrew@sig.gy>

In preparation for further simplifications to HTTP telemetry, this change consolidates all HTTP-specific logic under the `telemetry::http` module. Specifically, the following modules have been moved: - `telemetry::event`; - `telemetry::metrics::labels`; - `telemetry::metrics::record`; - `telemetry::sensors`; and - `telemetry::sensors::http`. This change takes pains to avoid changing any implementation details, so some types and methods have been made public temporarily while the interface boundaries are not well defined. This will be fixed in a subsequent change.

siggy self-assigned this Dec 20, 2017

siggy requested review from klingerf, briansmith and rmars December 20, 2017 23:49

rmars added the area/web label Dec 21, 2017

briansmith reviewed Dec 22, 2017

View reviewed changes

rmars reviewed Dec 22, 2017

View reviewed changes

siggy force-pushed the siggy/version-check branch from d8ea95c to 1d0e6ae Compare December 22, 2017 21:44

siggy mentioned this pull request Dec 23, 2017

implement richer version check messaging #90

Closed

siggy force-pushed the siggy/version-check branch 2 times, most recently from 3d77b9b to b3b9478 Compare December 28, 2017 06:13

rmars approved these changes Jan 2, 2018

View reviewed changes

Implement client-side version checking

5493601

Previously Conduit would render an iframe, received from versioncheck.conduit.io. Modify the client to retrieve the latest released version, via CORS. Signed-off-by: Andrew Seigner <andrew@sig.gy>

siggy force-pushed the siggy/version-check branch from a5a7395 to 5493601 Compare January 2, 2018 22:18

siggy merged commit 449f306 into master Jan 3, 2018

siggy deleted the siggy/version-check branch January 3, 2018 00:07

siggy mentioned this pull request Jan 3, 2018

conduit dashboard ,routes page blocked script . #76

Closed

siggy added this to the 0.1.2 milestone Jan 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement client-side version checking #79

Implement client-side version checking #79

siggy commented Dec 20, 2017

briansmith Dec 22, 2017

siggy Dec 22, 2017

klingerf Dec 22, 2017

briansmith Dec 22, 2017

rmars left a comment

rmars Dec 21, 2017

rmars Dec 21, 2017

rmars Dec 21, 2017

siggy Dec 23, 2017

rmars Dec 21, 2017

rmars Dec 21, 2017

siggy Dec 23, 2017

briansmith commented Dec 23, 2017

siggy commented Dec 23, 2017

xiaods commented Dec 28, 2017

siggy commented Dec 28, 2017

rmars left a comment

rmars Jan 2, 2018

rmars Jan 2, 2018

rmars Jan 2, 2018

rmars left a comment

Implement client-side version checking #79

Implement client-side version checking #79

Conversation

siggy commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansmith commented Dec 23, 2017

siggy commented Dec 23, 2017

xiaods commented Dec 28, 2017

siggy commented Dec 28, 2017

rmars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmars left a comment

Choose a reason for hiding this comment