From e376dff945e383977310eed48defaed6ae1ba21f Mon Sep 17 00:00:00 2001 From: Michael Bayne Date: Wed, 5 Jun 2019 12:20:30 -0700 Subject: [PATCH 1/3] Reinstate env var substitution in appbase. Also added tests for base init of Application & versioned appbase creation. --- CHANGELOG.md | 8 +- .../threerings/getdown/data/Application.java | 22 +++--- .../com/threerings/getdown/util/Config.java | 77 +++++++++++-------- .../getdown/data/ApplicationTest.java | 75 ++++++++++++++++++ .../getdown/data/EnvConfigTest.java | 4 +- 5 files changed, 142 insertions(+), 44 deletions(-) create mode 100644 core/src/test/java/com/threerings/getdown/data/ApplicationTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index d8eefdc..d9b73af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,12 +1,16 @@ # Getdown Releases +## 1.8.7 - Unreleased + +* Reinstated env var support in `appbase` property. + ## 1.8.6 - June 4, 2019 * Fixed issues with PAC proxy support: added `myIpAddress()`, fixed `dnsResolve()`, fixed crash when detecting PAC proxy. -* Reverted env var support in `appbase` and `latest` properties. It's causing problems that need to - be investigated. +* Reverted env var support in `appbase` property. It's causing problems that need to be + investigated. ## 1.8.5 - May 29, 2019 diff --git a/core/src/main/java/com/threerings/getdown/data/Application.java b/core/src/main/java/com/threerings/getdown/data/Application.java index ab0ec2b..a931225 100644 --- a/core/src/main/java/com/threerings/getdown/data/Application.java +++ b/core/src/main/java/com/threerings/getdown/data/Application.java @@ -605,7 +605,7 @@ public class Application } // check if we're overriding the domain in the appbase, and sub envvars - _appbase = SysProps.overrideAppbase(_appbase); + _appbase = resolveEnvVars(SysProps.overrideAppbase(_appbase)); // make sure there's a trailing slash if (!_appbase.endsWith("/")) { @@ -1130,27 +1130,31 @@ public class Application } } - /** Replaces the application directory and version in any argument. */ + /** Replaces the application directory, version and env vars in any argument. */ protected String processArg (String arg) { arg = arg.replace("%APPDIR%", getAppDir().getAbsolutePath()); arg = arg.replace("%VERSION%", String.valueOf(_version)); + arg = resolveEnvVars(arg); + return arg; + } - // if this argument contains %ENV.FOO% replace those with the associated values looked up - // from the environment - if (arg.contains(ENV_VAR_PREFIX)) { + /** Resolves env var substitutions in {@code text}. */ + protected String resolveEnvVars (String text) { + // if the text contains %ENV.FOO% replace it with FOO looked up in the environment + if (text.contains(ENV_VAR_PREFIX)) { StringBuffer sb = new StringBuffer(); - Matcher matcher = ENV_VAR_PATTERN.matcher(arg); + Matcher matcher = ENV_VAR_PATTERN.matcher(text); while (matcher.find()) { String varName = matcher.group(1), varValue = System.getenv(varName); String repValue = varValue == null ? "MISSING:"+varName : varValue; matcher.appendReplacement(sb, Matcher.quoteReplacement(repValue)); } matcher.appendTail(sb); - arg = sb.toString(); + return sb.toString(); + } else { + return text; } - - return arg; } /** diff --git a/core/src/main/java/com/threerings/getdown/util/Config.java b/core/src/main/java/com/threerings/getdown/util/Config.java index 2978fdf..75357e8 100644 --- a/core/src/main/java/com/threerings/getdown/util/Config.java +++ b/core/src/main/java/com/threerings/getdown/util/Config.java @@ -57,27 +57,11 @@ public class Config } /** - * Parses a configuration file containing key/value pairs. The file must be in the UTF-8 - * encoding. - * + * Parses configuration text containing key/value pairs. * @param opts options that influence the parsing. See {@link #createOpts}. - * * @return a list of {@code String[]} instances containing the key/value pairs in the * order they were parsed from the file. */ - public static List parsePairs (File source, ParseOpts opts) - throws IOException - { - // annoyingly FileReader does not allow encoding to be specified (uses platform default) - try (FileInputStream fis = new FileInputStream(source); - InputStreamReader input = new InputStreamReader(fis, StandardCharsets.UTF_8)) { - return parsePairs(input, opts); - } - } - - /** - * See {@link #parsePairs(File,ParseOpts)}. - */ public static List parsePairs (Reader source, ParseOpts opts) throws IOException { List pairs = new ArrayList<>(); @@ -131,6 +115,21 @@ public class Config return pairs; } + /** + * Parses configuration file containing key/value pairs. + * @param source the file containing the config text. Must be in the UTF-8 encoding. + * @param opts options that influence the parsing. See {@link #createOpts}. + */ + public static List parsePairs (File source, ParseOpts opts) + throws IOException + { + // annoyingly FileReader does not allow encoding to be specified (uses platform default) + try (FileInputStream fis = new FileInputStream(source); + InputStreamReader input = new InputStreamReader(fis, StandardCharsets.UTF_8)) { + return parsePairs(input, opts); + } + } + /** * Takes a comma-separated String of four integers and returns a rectangle using those ints as * the its x, y, width, and height. @@ -166,13 +165,10 @@ public class Config } /** - * Parses a configuration file containing key/value pairs. The file must be in the UTF-8 - * encoding. - * - * @return a map from keys to values, where a value will be an array of strings if more than - * one key/value pair in the config file was associated with the same key. + * Parses the data for a config instance from the supplied {@code source} reader. + * @return a map that can be used to create a {@link #Config}. */ - public static Config parseConfig (File source, ParseOpts opts) + public static Map parseData (Reader source, ParseOpts opts) throws IOException { Map data = new HashMap<>(); @@ -195,15 +191,34 @@ public class Config } } - // special magic for the getdown.txt config: if the parsed data contains 'strict_comments = - // true' then we reparse the file with strict comments (i.e. # is only assumed to start a - // comment in column 0) - if (!opts.strictComments && Boolean.parseBoolean((String)data.get("strict_comments"))) { - opts.strictComments = true; - return parseConfig(source, opts); - } + return data; + } - return new Config(data); + /** + * Parses a configuration file containing key/value pairs. The file must be in the UTF-8 + * encoding. + * + * @return a map from keys to values, where a value will be an array of strings if more than + * one key/value pair in the config file was associated with the same key. + */ + public static Config parseConfig (File source, ParseOpts opts) + throws IOException + { + // annoyingly FileReader does not allow encoding to be specified (uses platform default) + try (FileInputStream fis = new FileInputStream(source); + InputStreamReader input = new InputStreamReader(fis, StandardCharsets.UTF_8)) { + Map data = parseData(input, opts); + + // special magic for the getdown.txt config: if the parsed data contains + // 'strict_comments = true' then we reparse the file with strict comments (i.e. # is + // only assumed to start a comment in column 0) + if (!opts.strictComments && Boolean.parseBoolean((String)data.get("strict_comments"))) { + opts.strictComments = true; + return parseConfig(source, opts); + } + + return new Config(data); + } } public Config (Map data) { diff --git a/core/src/test/java/com/threerings/getdown/data/ApplicationTest.java b/core/src/test/java/com/threerings/getdown/data/ApplicationTest.java new file mode 100644 index 0000000..65960b6 --- /dev/null +++ b/core/src/test/java/com/threerings/getdown/data/ApplicationTest.java @@ -0,0 +1,75 @@ +// +// Getdown - application installer, patcher and launcher +// Copyright (C) 2004-2018 Getdown authors +// https://github.com/threerings/getdown/blob/master/LICENSE + +package com.threerings.getdown.data; + +import java.io.IOException; +import java.io.StringReader; +import java.net.URL; +import java.util.ArrayList; +import java.util.List; + +import org.junit.*; +import static org.junit.Assert.*; + +import com.threerings.getdown.util.Config; + +public class ApplicationTest { + + Application createApp () { + List notes = new ArrayList<>(); + EnvConfig env = EnvConfig.create(new String[0], notes); + EnvConfigTest.checkNoNotes(notes); + return new Application(env); + } + + @Test public void testBaseConfig () throws Exception { + Application app = createApp(); + URL appbase = new URL("https://test.com/foo/bar/"); + Config config = new Config(Config.parseData(toReader( + "appbase", appbase.toString() + ), Config.createOpts(true))); + app.initBase(config); + + assertEquals(appbase, app.getRemoteURL("")); + } + + @Test public void testVersionedBase () throws Exception { + Application app = createApp(); + String rootAppbase = "https://test.com/foo/bar/"; + Config config = new Config(Config.parseData(toReader( + "appbase", rootAppbase + "%VERSION%", + "version", "42" + ), Config.createOpts(true))); + app.initBase(config); + + assertEquals(new URL(rootAppbase + "42/"), app.getRemoteURL("")); + } + + @Test public void testEnvVarBase () throws Exception { + // fiddling to make test work on Windows or Unix + String evar = System.getenv("USER") == null ? "USERNAME" : "USER"; + Application app = createApp(); + String rootAppbase = "https://test.com/foo/%ENV." + evar + "%/"; + Config config = new Config(Config.parseData(toReader( + "appbase", rootAppbase + "%VERSION%", + "version", "42" + ), Config.createOpts(true))); + app.initBase(config); + + String expectAppbase = "https://test.com/foo/" + System.getenv(evar) + "/42/"; + assertEquals(new URL(expectAppbase), app.getRemoteURL("")); + } + + protected static StringReader toReader (String... pairs) + { + StringBuilder builder = new StringBuilder(); + for (int ii = 0; ii < pairs.length; ii += 2) { + builder.append(pairs[ii]).append("=").append(pairs[ii+1]).append("\n"); + } + return new StringReader(builder.toString()); + } + +} diff --git a/core/src/test/java/com/threerings/getdown/data/EnvConfigTest.java b/core/src/test/java/com/threerings/getdown/data/EnvConfigTest.java index 6178651..04a73d3 100644 --- a/core/src/test/java/com/threerings/getdown/data/EnvConfigTest.java +++ b/core/src/test/java/com/threerings/getdown/data/EnvConfigTest.java @@ -19,13 +19,13 @@ public class EnvConfigTest { static String TESTID = "testid"; static String TESTBASE = "https://test.com/test"; - private void debugNotes(List notes) { + private void debugNotes (List notes) { for (EnvConfig.Note note : notes) { System.out.println(note.message); } } - private void checkNoNotes (List notes) { + static void checkNoNotes (List notes) { StringBuilder msg = new StringBuilder(); for (EnvConfig.Note note : notes) { if (note.level != EnvConfig.Note.Level.INFO) { From 157b12dbfa46b66e967cedf89825cb10ddda8109 Mon Sep 17 00:00:00 2001 From: Michael Bayne Date: Wed, 5 Jun 2019 15:44:06 -0700 Subject: [PATCH 2/3] Oops, forgot to hook up myIpAddress(). --- CHANGELOG.md | 2 ++ .../resources/com/threerings/getdown/launcher/PacUtils.js | 4 ++++ .../com/threerings/getdown/launcher/ProxyUtilTest.java | 8 ++++++++ 3 files changed, 14 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d9b73af..ec7a923 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ * Reinstated env var support in `appbase` property. +* Fixed issue with `myIpAddress()` in PAC proxy support. + ## 1.8.6 - June 4, 2019 * Fixed issues with PAC proxy support: added `myIpAddress()`, fixed `dnsResolve()`, fixed crash diff --git a/launcher/src/main/resources/com/threerings/getdown/launcher/PacUtils.js b/launcher/src/main/resources/com/threerings/getdown/launcher/PacUtils.js index 96a644e..db4263b 100644 --- a/launcher/src/main/resources/com/threerings/getdown/launcher/PacUtils.js +++ b/launcher/src/main/resources/com/threerings/getdown/launcher/PacUtils.js @@ -23,6 +23,10 @@ function dnsResolve (host) { return resolver.dnsResolve(host) } +function myIpAddress () { + return resolver.myIpAddress() +} + function isInNet (addrOrHost, pattern, maskstr) { var testRE = /^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/; var test = testRE.exec(addrOrHost); diff --git a/launcher/src/test/java/com/threerings/getdown/launcher/ProxyUtilTest.java b/launcher/src/test/java/com/threerings/getdown/launcher/ProxyUtilTest.java index e60ab90..43ee670 100644 --- a/launcher/src/test/java/com/threerings/getdown/launcher/ProxyUtilTest.java +++ b/launcher/src/test/java/com/threerings/getdown/launcher/ProxyUtilTest.java @@ -6,6 +6,7 @@ package com.threerings.getdown.launcher; import java.io.StringReader; +import java.net.InetAddress; import java.net.URL; import org.junit.Test; @@ -133,5 +134,12 @@ public class ProxyUtilTest { // return 'DIRECT'; // } // } + + String MYIP = + "function FindProxyForURL(url, host) {\n" + + " return 'PROXY ' + myIpAddress() + ':8080';\n" + + "}"; + String myIp = InetAddress.getLocalHost().getHostAddress(); + testPAC(MYIP, "http://testurl.com/", "PROXY " + myIp + ":8080"); } } From e7818030a7181681b7c2a3395810b5ce510eda96 Mon Sep 17 00:00:00 2001 From: Michael Bayne Date: Wed, 5 Jun 2019 16:09:51 -0700 Subject: [PATCH 3/3] Behave more gracefully when patch files do not exist. It is possible to use versioned mode without patch files, though there's no way to specifically tell Getdown whether or not you are using patch files, so it always tries to download them. If they don't exist, it should now log one reasonable warning and then proceed to update based on resource hashes. --- .../com/threerings/getdown/net/Connector.java | 22 +++++++++++++------ .../threerings/getdown/net/Downloader.java | 20 +++++++++++++---- .../threerings/getdown/launcher/Getdown.java | 6 +++++ 3 files changed, 37 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/com/threerings/getdown/net/Connector.java b/core/src/main/java/com/threerings/getdown/net/Connector.java index 544d2cc..f651250 100644 --- a/core/src/main/java/com/threerings/getdown/net/Connector.java +++ b/core/src/main/java/com/threerings/getdown/net/Connector.java @@ -143,15 +143,23 @@ public class Connector { * If the connection failed for proxy related reasons, this changes the state of this connector * to reflect the needed proxy information. */ - public void checkConnectOK (URLConnection conn, String errpre) throws IOException - { - // if it's not an HTTP connection, there's nothing to check - if (!(conn instanceof HttpURLConnection)) return; + public void checkConnectOK (URLConnection conn, String errpre) throws IOException { + int code = checkConnectStatus(conn); + if (code != HttpURLConnection.HTTP_OK) { + throw new IOException(errpre + " [code=" + code + "]"); + } + } + + /** + * Returns the connection status of {@code conn}. If the connection failed for proxy related + * reasons, this changes the state of this connector to reflect the needed proxy information. + */ + public int checkConnectStatus (URLConnection conn) throws IOException { + // if it's not an HTTP connection, we assume it's OK + if (!(conn instanceof HttpURLConnection)) return HttpURLConnection.HTTP_OK; int code = ((HttpURLConnection)conn).getResponseCode(); switch (code) { - case HttpURLConnection.HTTP_OK: - return; case HttpURLConnection.HTTP_FORBIDDEN: case HttpURLConnection.HTTP_USE_PROXY: state = State.NEED_PROXY; @@ -160,7 +168,7 @@ public class Connector { state = State.NEED_PROXY_AUTH; break; } - throw new IOException(errpre + " [code=" + code + "]"); + return code; } /** diff --git a/core/src/main/java/com/threerings/getdown/net/Downloader.java b/core/src/main/java/com/threerings/getdown/net/Downloader.java index 505958a..2298d60 100644 --- a/core/src/main/java/com/threerings/getdown/net/Downloader.java +++ b/core/src/main/java/com/threerings/getdown/net/Downloader.java @@ -140,6 +140,11 @@ public class Downloader */ protected void downloadFailed (Resource rsrc, Exception cause) {} + /** + * Called when a to-be-downloaded resource returns a 404 not found. + */ + protected void resourceMissing (Resource rsrc) {} + /** * Performs the protocol-specific portion of checking download size. */ @@ -150,9 +155,10 @@ public class Downloader if (conn instanceof HttpURLConnection) { ((HttpURLConnection)conn).setRequestMethod("HEAD"); } - // make sure we got a satisfactory response code - _conn.checkConnectOK(conn, "Unable to check up-to-date for " + rsrc.getRemote()); - return conn.getContentLength(); + // if we get a satisfactory response code, return a size; ignore errors as we'll report + // those when we actually attempt to download the resource + int code = _conn.checkConnectStatus(conn); + return code == HttpURLConnection.HTTP_OK ? conn.getContentLength() : 0; } finally { // let it be known that we're done with this connection @@ -232,7 +238,13 @@ public class Downloader protected void download (Resource rsrc) throws IOException { URLConnection conn = _conn.open(rsrc.getRemote(), 0, 0); // make sure we got a satisfactory response code - _conn.checkConnectOK(conn, "Unable to download resource " + rsrc.getRemote()); + int code = _conn.checkConnectStatus(conn); + if (code == HttpURLConnection.HTTP_NOT_FOUND) { + resourceMissing(rsrc); + } else if (code != HttpURLConnection.HTTP_OK) { + throw new IOException( + "Resource returned HTTP error " + rsrc.getRemote() + " [code=" + code + "]"); + } // TODO: make FileChannel download impl (below) robust and allow apps to opt-into it via a // system property diff --git a/launcher/src/main/java/com/threerings/getdown/launcher/Getdown.java b/launcher/src/main/java/com/threerings/getdown/launcher/Getdown.java index 64eabdc..1cc6922 100644 --- a/launcher/src/main/java/com/threerings/getdown/launcher/Getdown.java +++ b/launcher/src/main/java/com/threerings/getdown/launcher/Getdown.java @@ -601,6 +601,8 @@ public abstract class Getdown int ii = 0; for (Resource prsrc : list) { ProgressObserver pobs = pragg.startElement(ii++); try { + // if this patch file failed to download, skip it + if (!prsrc.getLocalNew().exists()) continue; // install the patch file (renaming them from _new) prsrc.install(false); // now apply the patch @@ -663,6 +665,10 @@ public abstract class Getdown log.warning("Download failed", "rsrc", rsrc, e); } + @Override protected void resourceMissing (Resource rsrc) { + log.warning("Resource missing (got 404)", "rsrc", rsrc); + } + /** The last percentage at which we checked for another getdown running, or -1 for not * having checked at all. */ protected int _lastCheck = -1;