From 6602e2c812992e3253e2a9cdc7bdad95df68c727 Mon Sep 17 00:00:00 2001 From: soloturn Date: Sat, 26 Sep 2026 22:18:21 +0200 Subject: [PATCH 1/6] feat(reporter): submit crash reports directly to GitHub issues - Pre-fills Terasology's real GitHub issue form with a crash summary, every logged exception (one row each naming its log tab, full stack trace, the 5 log lines preceding it), capped to GitHub's issue-body byte limit. - Adds an alternative direct-submit path via GitHub's device login flow and issue API, so the user isn't required to go through the browser-prefilled form. - Configures the OAuth client ID used for that direct-submit login. Co-Authored-By: soloturn --- cr-core/build.gradle.kts | 3 + .../crashreporter/GlobalProperties.java | 4 + .../terasology/crashreporter/RootPanel.java | 8 +- .../crashreporter/pages/CrashSummary.java | 380 ++++++++++++++++++ .../pages/FinalActionsPanel.java | 43 +- .../pages/GitHubDeviceLogin.java | 144 +++++++ .../pages/GitHubIssueApiClient.java | 68 ++++ .../pages/GitHubIssueLinkBuilder.java | 104 +++++ .../pages/GitHubLoginDialog.java | 176 ++++++++ .../resources/i18n/MessagesBundle.properties | 10 + .../crashreporter/pages/CrashSummaryTest.java | 232 +++++++++++ .../pages/GitHubDeviceLoginTest.java | 110 +++++ .../pages/GitHubIssueApiClientTest.java | 70 ++++ .../pages/GitHubIssueLinkBuilderTest.java | 125 ++++++ .../main/resources/crashreporter.properties | 2 + 15 files changed, 1476 insertions(+), 3 deletions(-) create mode 100644 cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java create mode 100644 cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubDeviceLogin.java create mode 100644 cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueApiClient.java create mode 100644 cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java create mode 100644 cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubLoginDialog.java create mode 100644 cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java create mode 100644 cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubDeviceLoginTest.java create mode 100644 cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueApiClientTest.java create mode 100644 cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java diff --git a/cr-core/build.gradle.kts b/cr-core/build.gradle.kts index 07f937a..06115ca 100644 --- a/cr-core/build.gradle.kts +++ b/cr-core/build.gradle.kts @@ -78,6 +78,9 @@ dependencies { implementation("com.fasterxml.jackson.core:jackson-annotations") implementation("org.apache.httpcomponents:httpclient:4.5.13") implementation("org.apache.httpcomponents:httpmime:4.5.13") + // GitHub issue-creation API request/response bodies (GitHubIssueApiClient) - small, + // dependency-free, no reason to hand-roll JSON escaping/parsing instead. + implementation("org.json:json:20260814") testImplementation("org.junit.jupiter:junit-jupiter-api:5.10.1") testImplementation("org.junit.jupiter:junit-jupiter-params:5.10.1") diff --git a/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java b/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java index 94a062b..11b6429 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java @@ -18,6 +18,10 @@ public enum KEY { SUPPORT_FORUM_LINK, JOIN_DISCORD_LINK, REPORT_ISSUE_LINK, + REPORT_ISSUE_TEMPLATE, + // GitHub OAuth App client ID with Device Flow enabled. Unset by default - see + // GitHubDeviceLogin's javadoc. When unset, the "submit directly" button is hidden. + REPORT_ISSUE_OAUTH_CLIENT_ID, RES_BANNER_IMAGE, RES_SERVER_ICON, diff --git a/cr-core/src/main/java/org/terasology/crashreporter/RootPanel.java b/cr-core/src/main/java/org/terasology/crashreporter/RootPanel.java index 21de16e..19a9904 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/RootPanel.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/RootPanel.java @@ -74,7 +74,13 @@ public String get() { } }); pages.add(uploadPanel); - pages.add(new FinalActionsPanel(properties, new Supplier() { + pages.add(new FinalActionsPanel(properties, exception, new Supplier() { + + @Override + public String get() { + return errorMessagePanel.getLog(); + } + }, new Supplier() { @Override public URL get() { diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java new file mode 100644 index 0000000..64192fb --- /dev/null +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java @@ -0,0 +1,380 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.crashreporter.pages; + +import java.io.PrintWriter; +import java.io.StringWriter; +import java.net.URL; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * Builds a pre-filled GitHub issue title/body from a crash: the exception itself, plus every other + * exception found across the crashed process' own log tabs, and the engine version/active module + * list - the reporter runs in its own JVM (see #52, subprocess isolation) and has no other way to + * reach any of the log-only information. + *

+ * On macOS, that subprocess isolation means the in-process {@code Throwable} passed to + * {@link #extract} is a best-effort reconstruction from just its class name and message (see + * {@code CrashReporter#reconstructThrowable}) - its own stack trace points into the reporter's own + * relaunch machinery, not the real crash site. Whenever the crash was also logged in one of the log + * tabs (the normal case for an engine-level crash handler), the trace captured from that log text is + * the real one and is used instead; the reconstructed exception's own trace is only a fallback for + * when nothing better is available. + *

+ * The version/module regexes here mirror two fixed, narrow log lines the engine emits at startup - + * see {@code TerasologyEngine#logEnvironmentInfo} ({@code TerasologyVersion#toString}'s + * {@code [buildNumber=..., ..., engineVersion=X, displayVersion=Y]} format) and + * {@code RegisterMods} ({@code "Activating module: :"}, once per active module). + * Log formatting is not a published API and can drift; a change there degrades this to a blank + * "unknown"/empty-list extract rather than failing the report itself. + */ +public final class CrashSummary { + + private static final int MAX_STACK_LINES = 15; + private static final int MAX_MODULES_LISTED = 30; + private static final int MAX_TITLE_MESSAGE_LENGTH = 80; + private static final int MAX_EXCEPTIONS_LISTED = 10; + private static final int CONTEXT_LINES_BEFORE = 5; + private static final String NO_TAB_LABEL = "this crash"; + + private static final Pattern ENGINE_VERSION_PATTERN = Pattern.compile("engineVersion=([^,\\]]*)"); + private static final Pattern DISPLAY_VERSION_PATTERN = Pattern.compile("displayVersion=([^,\\]]*)"); + private static final Pattern ACTIVE_MODULE_PATTERN = Pattern.compile("Activating module: (\\S+:\\S+)"); + private static final Pattern LOG_TAB_PATTERN = Pattern.compile("(?m)^=== (.*) ===$"); + // A log-formatted stack trace: a "some.FullyQualified.NameException[: message]" header line + // immediately followed by one or more "at ..."/"Caused by: ..." frame lines - the shape every + // JVM logging framework prints a Throwable in (Logback's %ex, java.util.logging, a raw + // printStackTrace()), regardless of which class emits it. + private static final Pattern STACK_TRACE_HEADER_PATTERN = Pattern.compile( + "(?m)^([\\w$]+(?:\\.[\\w$]+)+(?:Exception|Error))(:[^\\n]*)?\\n((?:[ \\t]*(?:at |Caused by:)[^\\n]*\\n?)+)"); + + private final Throwable exception; + private final List exceptionBlocks; + private final int moreExceptionsCount; + private final String engineVersion; + private final String displayVersion; + private final List activeModules; + + private CrashSummary(Throwable exception, List exceptionBlocks, int moreExceptionsCount, + String engineVersion, String displayVersion, List activeModules) { + this.exception = exception; + this.exceptionBlocks = exceptionBlocks; + this.moreExceptionsCount = moreExceptionsCount; + this.engineVersion = engineVersion; + this.displayVersion = displayVersion; + this.activeModules = activeModules; + } + + public static CrashSummary extract(Throwable exception, String combinedLogText) { + String text = combinedLogText != null ? combinedLogText : ""; + + List blocks = buildExceptionBlocks(exception, text); + int moreCount = 0; + if (blocks.size() > MAX_EXCEPTIONS_LISTED) { + moreCount = blocks.size() - MAX_EXCEPTIONS_LISTED; + blocks = new ArrayList<>(blocks.subList(0, MAX_EXCEPTIONS_LISTED)); + } + + return new CrashSummary(exception, blocks, moreCount, + firstGroup(ENGINE_VERSION_PATTERN, text), firstGroup(DISPLAY_VERSION_PATTERN, text), + extractActiveModules(text)); + } + + /** + * Builds one Markdown block per distinct exception found - the one that triggered this report + * first, then every other exception found across the log tabs - so a crash whose real cause is + * an earlier exception logged in a different tab (e.g. during init) isn't left out of the + * pre-filled issue just because it wasn't the in-process {@code exception} the reporter happened + * to be invoked with. + */ + private static List buildExceptionBlocks(Throwable exception, String combinedLogText) { + String primaryHeader = exception.toString().trim(); + List found = findAllExceptions(combinedLogText); + + ExceptionEntry primary = null; + for (ExceptionEntry entry : found) { + if (entry.header.equals(primaryHeader)) { + primary = entry; + break; + } + } + // Not logged anywhere - fall back to the exception object's own trace. On macOS that trace + // is a best-effort reconstruction (see the class javadoc) rather than the real crash site, + // but it's all that's available. There's no log text to pull leading context from either. + if (primary == null) { + primary = new ExceptionEntry(null, primaryHeader, framesFromThrowable(exception), ""); + } + + List blocks = new ArrayList<>(); + blocks.add(formatBlock(primary)); + for (ExceptionEntry entry : found) { + if (entry.header.equals(primaryHeader)) { + continue; + } + String block = formatBlock(entry); + if (!blocks.contains(block)) { + blocks.add(block); + } + } + return blocks; + } + + private static String formatBlock(ExceptionEntry entry) { + String label = entry.tabName != null ? entry.tabName : NO_TAB_LABEL; + String combined = entry.frames.isEmpty() ? entry.header : entry.header + "\n" + entry.frames; + String trace = truncateTrace(combined); + // Context isn't part of the trace itself, so it's not subject to truncateTrace()'s + // MAX_STACK_LINES cap - a few lines of what led up to the crash shouldn't cost trace detail. + String content = entry.context.isEmpty() ? trace : entry.context + "\n" + trace; + return "**" + label + "**\n\n```\n" + content + "\n```"; + } + + private static String truncateTrace(String combined) { + String[] lines = combined.split("\r?\n"); + StringBuilder builder = new StringBuilder(); + int limit = Math.min(lines.length, MAX_STACK_LINES); + for (int i = 0; i < limit; i++) { + builder.append(lines[i]).append('\n'); + } + if (lines.length > limit) { + builder.append("... ").append(lines.length - limit).append(" more line(s) - see the full log\n"); + } + return builder.toString().trim(); + } + + private static String framesFromThrowable(Throwable exception) { + StringWriter sink = new StringWriter(); + exception.printStackTrace(new PrintWriter(sink)); + String full = sink.toString(); + // printStackTrace()'s first line is exception.toString() - already the header - so only the + // "at ..."/"Caused by: ..." frames after it are needed here. + int newlineIndex = full.indexOf('\n'); + return newlineIndex >= 0 ? stripTrailingWhitespace(full.substring(newlineIndex + 1)) : ""; + } + + /** + * Like {@link String#trim()} but only at the end - frame lines are indented with a leading tab + * ({@code "\tat ..."}), which a plain {@code trim()} would strip from the first line along with + * the trailing newline it's actually meant to remove. + */ + private static String stripTrailingWhitespace(String s) { + int end = s.length(); + while (end > 0 && Character.isWhitespace(s.charAt(end - 1))) { + end--; + } + return s.substring(0, end); + } + + private static final class ExceptionEntry { + private final String tabName; + private final String header; + private final String frames; + private final String context; + + private ExceptionEntry(String tabName, String header, String frames, String context) { + this.tabName = tabName; + this.header = header; + this.frames = frames; + this.context = context; + } + } + + private static List findAllExceptions(String combinedLogText) { + List found = new ArrayList<>(); + + Matcher tabMatcher = LOG_TAB_PATTERN.matcher(combinedLogText); + int tabStart = -1; + String tabName = null; + while (true) { + boolean hasNext = tabMatcher.find(); + int nextStart = hasNext ? tabMatcher.start() : combinedLogText.length(); + if (tabName != null) { + collectExceptionHeaders(combinedLogText.substring(tabStart, nextStart), tabName, found); + } + if (!hasNext) { + break; + } + tabName = tabMatcher.group(1); + // tabMatcher.end() lands right after "===", before that line's own terminator - skip it + // too, so each tab's text starts at its real first content line instead of with a blank + // artifact line (which precedingLines() would otherwise count as logged context). + tabStart = tabMatcher.end(); + if (tabStart < combinedLogText.length() && combinedLogText.charAt(tabStart) == '\r') { + tabStart++; + } + if (tabStart < combinedLogText.length() && combinedLogText.charAt(tabStart) == '\n') { + tabStart++; + } + } + // No "=== tab ===" headers at all - a single combined-log caller (e.g. a direct test) rather + // than ErrorMessagePanel#getLog(); scan the whole text with no tab attribution. + if (tabName == null) { + collectExceptionHeaders(combinedLogText, null, found); + } + return found; + } + + private static void collectExceptionHeaders(String tabText, String tabName, List found) { + Matcher matcher = STACK_TRACE_HEADER_PATTERN.matcher(tabText); + while (matcher.find()) { + String header = (matcher.group(1) + (matcher.group(2) != null ? matcher.group(2) : "")).trim(); + String frames = stripTrailingWhitespace(matcher.group(3)); + String context = precedingLines(tabText, matcher.start(), CONTEXT_LINES_BEFORE); + found.add(new ExceptionEntry(tabName, header, frames, context)); + } + } + + /** + * @return up to {@code maxLines} lines of whatever was logged right before {@code beforeIndex} in + * {@code text} - what led up to a crash is often as useful for diagnosing it as the trace + * itself, and it's only available here (the reporter's own {@link #exception} carries no + * log context of its own). + */ + private static String precedingLines(String text, int beforeIndex, int maxLines) { + String[] lines = text.substring(0, beforeIndex).split("\r?\n", -1); + int end = lines.length; + // A trailing empty element only ever means the substring ended in a newline - i.e. the line + // right before the match, not a real blank log line - so it isn't context to show. + if (end > 0 && lines[end - 1].isEmpty()) { + end--; + } + int start = Math.max(0, end - maxLines); + StringBuilder builder = new StringBuilder(); + for (int i = start; i < end; i++) { + builder.append(lines[i]).append('\n'); + } + return stripTrailingWhitespace(builder.toString()); + } + + private static String firstGroup(Pattern pattern, String text) { + Matcher matcher = pattern.matcher(text); + return matcher.find() ? matcher.group(1).trim() : ""; + } + + private static List extractActiveModules(String text) { + List modules = new ArrayList<>(); + Matcher matcher = ACTIVE_MODULE_PATTERN.matcher(text); + while (matcher.find()) { + String module = matcher.group(1); + if (!modules.contains(module)) { + modules.add(module); + } + } + return modules; + } + + /** + * @return a short, single-line issue title: the exception's simple class name, plus a + * truncated message if it has one. + */ + public String buildTitle() { + StringBuilder title = new StringBuilder("Crash: ").append(exception.getClass().getSimpleName()); + String message = exception.getLocalizedMessage(); + if (message != null && !message.trim().isEmpty()) { + String trimmed = message.length() > MAX_TITLE_MESSAGE_LENGTH + ? message.substring(0, MAX_TITLE_MESSAGE_LENGTH - 3) + "..." + : message; + title.append(": ").append(trimmed); + } + return title.toString(); + } + + /** + * @param pastebinLink the uploaded log link, or {@code null} if the user skipped upload + * @return a Markdown issue body: every exception found, one labeled code block each (naming the + * log tab it was found in), then environment info, then a link to the full logs + */ + public String buildBody(URL pastebinLink) { + StringBuilder body = new StringBuilder(); + + body.append("### Exceptions\n\n"); + for (String block : exceptionBlocks) { + body.append(block).append("\n\n"); + } + if (moreExceptionsCount > 0) { + body.append("... ").append(moreExceptionsCount).append(" more - see the full log\n\n"); + } + + body.append("### Environment\n\n"); + body.append("- Terasology version: ").append(engineVersion.isEmpty() ? "unknown" : engineVersion); + if (!displayVersion.isEmpty()) { + body.append(" (").append(displayVersion).append(')'); + } + body.append('\n'); + body.append("- OS: ").append(System.getProperty("os.name")).append(' ') + .append(System.getProperty("os.version")).append(" (").append(System.getProperty("os.arch")).append(")\n"); + body.append("- Active modules:"); + if (activeModules.isEmpty()) { + body.append(" none found in logs\n"); + } else { + body.append('\n'); + int shown = Math.min(activeModules.size(), MAX_MODULES_LISTED); + for (int i = 0; i < shown; i++) { + body.append(" - ").append(activeModules.get(i)).append('\n'); + } + if (activeModules.size() > shown) { + body.append(" - ... ").append(activeModules.size() - shown).append(" more\n"); + } + } + + body.append("\n### Full logs\n\n"); + body.append(pastebinLink != null ? "[PasteBin](" + pastebinLink + ")\n" : "(not uploaded)\n"); + + return body.toString(); + } + + /** + * @param pastebinLink the uploaded log link, or {@code null} if the user skipped upload + * @return field ID to value for the issue form named by + * {@link org.terasology.crashreporter.GlobalProperties.KEY#REPORT_ISSUE_TEMPLATE} - only + * meaningful when a downstream app has configured one (see + * {@link GitHubIssueLinkBuilder#build(String, String, String, Map)}); the IDs here match + * Terasology's own {@code crash-bug-report.yml}. Fields with nothing extractable are + * omitted so the user's own blank field is left for them to fill in, rather than being + * pre-filled with something misleading like "unknown". + */ + public Map buildIssueFormFields(URL pastebinLink) { + Map fields = new LinkedHashMap<>(); + + if (!engineVersion.isEmpty()) { + String version = displayVersion.isEmpty() ? engineVersion : engineVersion + " (" + displayVersion + ")"; + fields.put("terasology_version", version); + } + fields.put("operating_system", System.getProperty("os.name") + " " + System.getProperty("os.version") + + " (" + System.getProperty("os.arch") + ")"); + fields.put("java_version", System.getProperty("java.version")); + + StringBuilder actual = new StringBuilder(); + for (String block : exceptionBlocks) { + actual.append(block).append("\n\n"); + } + if (moreExceptionsCount > 0) { + actual.append("... ").append(moreExceptionsCount).append(" more - see the full log\n"); + } + fields.put("actual_behavior", actual.toString().trim()); + + if (pastebinLink != null) { + fields.put("log_details", "[PasteBin](" + pastebinLink + ")"); + } + + if (!activeModules.isEmpty()) { + StringBuilder modules = new StringBuilder("Active modules:\n"); + int shown = Math.min(activeModules.size(), MAX_MODULES_LISTED); + for (int i = 0; i < shown; i++) { + modules.append("- ").append(activeModules.get(i)).append('\n'); + } + if (activeModules.size() > shown) { + modules.append("- ... ").append(activeModules.size() - shown).append(" more\n"); + } + fields.put("additional_context", modules.toString().trim()); + } + + return fields; + } +} diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/FinalActionsPanel.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/FinalActionsPanel.java index f9d452c..438071c 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/FinalActionsPanel.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/FinalActionsPanel.java @@ -15,12 +15,14 @@ import javax.swing.JPanel; import javax.swing.JTextArea; import javax.swing.SwingConstants; +import javax.swing.SwingUtilities; import javax.swing.border.EmptyBorder; import java.awt.BorderLayout; import java.awt.Dimension; import java.awt.Font; import java.awt.GridLayout; import java.awt.Toolkit; +import java.awt.Window; import java.awt.datatransfer.Clipboard; import java.awt.datatransfer.StringSelection; import java.awt.event.ActionEvent; @@ -35,6 +37,10 @@ public class FinalActionsPanel extends JPanel { private static final long serialVersionUID = 2639334979749507943L; + private final Throwable exception; + + private final Supplier logTextSupplier; + private final Supplier uploadedFile; private final JTextArea linkText; @@ -43,8 +49,11 @@ public class FinalActionsPanel extends JPanel { private boolean pageComplete; - public FinalActionsPanel(GlobalProperties properties, Supplier uploadedFile) { + public FinalActionsPanel(GlobalProperties properties, Throwable exception, Supplier logTextSupplier, + Supplier uploadedFile) { + this.exception = exception; + this.logTextSupplier = logTextSupplier; this.uploadedFile = uploadedFile; setLayout(new BorderLayout(0, 10)); @@ -85,7 +94,20 @@ public void actionPerformed(ActionEvent e) { @Override public void actionPerformed(ActionEvent e) { - BrowserLauncher.open(properties.get(KEY.REPORT_ISSUE_LINK)); + CrashSummary summary = CrashSummary.extract(exception, logTextSupplier.get()); + String baseUrl = properties.get(KEY.REPORT_ISSUE_LINK); + String template = properties.get(KEY.REPORT_ISSUE_TEMPLATE); + String link; + if (template != null && !template.isEmpty()) { + // The downstream app has its own issue *form* - land the summary in its real + // fields instead of overwriting the whole thing with a bespoke body. + link = GitHubIssueLinkBuilder.build(baseUrl, template, summary.buildTitle(), + summary.buildIssueFormFields(uploadedFile.get())); + } else { + link = GitHubIssueLinkBuilder.build(baseUrl, summary.buildTitle(), + summary.buildBody(uploadedFile.get())); + } + BrowserLauncher.open(link); pageComplete = true; firePropertyChange("pageComplete", !pageComplete, pageComplete); } @@ -93,6 +115,23 @@ public void actionPerformed(ActionEvent e) { githubIssueButton.setToolTipText(properties.get(KEY.REPORT_ISSUE_LINK)); gridPanel.add(githubIssueButton); + String oauthClientId = properties.get(KEY.REPORT_ISSUE_OAUTH_CLIENT_ID); + String[] ownerRepo = GitHubIssueApiClient.parseOwnerRepo(properties.get(KEY.REPORT_ISSUE_LINK)); + if (oauthClientId != null && !oauthClientId.isEmpty() && ownerRepo != null) { + JButton submitDirectlyButton = new JButton(I18N.getMessage("reportIssueDirectly")); + submitDirectlyButton.setFont(buttonFont); + submitDirectlyButton.setIcon(Resources.loadIcon(properties.get(KEY.RES_GITHUB_ICON))); + submitDirectlyButton.addActionListener(e -> { + CrashSummary summary = CrashSummary.extract(exception, logTextSupplier.get()); + Window window = SwingUtilities.getWindowAncestor(this); + new GitHubLoginDialog(window, oauthClientId, ownerRepo[0], ownerRepo[1], + summary.buildTitle(), summary.buildBody(uploadedFile.get())).setVisible(true); + pageComplete = true; + firePropertyChange("pageComplete", !pageComplete, pageComplete); + }); + gridPanel.add(submitDirectlyButton); + } + JButton forumButton = new JButton(I18N.getMessage("gotoForum")); forumButton.setIcon(Resources.loadIcon(properties.get(KEY.RES_FORUM_ICON))); forumButton.setFont(buttonFont); diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubDeviceLogin.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubDeviceLogin.java new file mode 100644 index 0000000..13332d2 --- /dev/null +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubDeviceLogin.java @@ -0,0 +1,144 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.crashreporter.pages; + +import org.apache.http.NameValuePair; +import org.apache.http.client.entity.UrlEncodedFormEntity; +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpPost; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.message.BasicNameValuePair; +import org.apache.http.util.EntityUtils; + +import java.io.IOException; +import java.io.UnsupportedEncodingException; +import java.net.URLDecoder; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +/** + * GitHub's OAuth Device Flow - no client secret, no redirect URI, made for apps like this one. + * Needs an OAuth App registered at github.com/settings/developers with Device Flow enabled; + * its client ID goes in {@link org.terasology.crashreporter.GlobalProperties.KEY#REPORT_ISSUE_OAUTH_CLIENT_ID}. + * Endpoints reply form-urlencoded by default, so no JSON parsing needed here. + */ +public final class GitHubDeviceLogin { + + private static final String DEVICE_CODE_URL = "https://github.com/login/device/code"; + private static final String TOKEN_URL = "https://github.com/login/oauth/access_token"; + private static final String SCOPE = "public_repo"; + + private GitHubDeviceLogin() { + } + + public static DeviceCode requestDeviceCode(CloseableHttpClient client, String clientId) throws IOException { + Map fields = post(client, DEVICE_CODE_URL, + param("client_id", clientId), param("scope", SCOPE)); + failOnError(fields); + return new DeviceCode(fields.get("device_code"), fields.get("user_code"), fields.get("verification_uri"), + Integer.parseInt(fields.get("expires_in")), Integer.parseInt(fields.get("interval"))); + } + + /** Blocks until authorized, denied, or expired. Call off the UI thread. */ + public static String pollForAccessToken(CloseableHttpClient client, String clientId, DeviceCode code) + throws IOException, InterruptedException { + int interval = code.intervalSeconds; + long deadline = System.currentTimeMillis() + code.expiresInSeconds * 1000L; + while (System.currentTimeMillis() < deadline) { + Thread.sleep(interval * 1000L); + Map fields = post(client, TOKEN_URL, + param("client_id", clientId), param("device_code", code.deviceCode), + param("grant_type", "urn:ietf:params:oauth:grant-type:device_code")); + String token = fields.get("access_token"); + if (token != null) { + return token; + } + String error = fields.get("error"); + if ("authorization_pending".equals(error)) { + continue; + } + if ("slow_down".equals(error)) { + interval += 5; + continue; + } + failOnError(fields); + } + throw new IOException("Device code expired"); + } + + private static void failOnError(Map fields) throws IOException { + String error = fields.get("error"); + if (error != null) { + throw new IOException(fields.getOrDefault("error_description", error)); + } + } + + private static Map post(CloseableHttpClient client, String url, NameValuePair... params) + throws IOException { + HttpPost post = new HttpPost(url); + post.setHeader("Accept", "application/x-www-form-urlencoded"); + List paramList = new ArrayList<>(); + for (NameValuePair param : params) { + paramList.add(param); + } + post.setEntity(new UrlEncodedFormEntity(paramList, StandardCharsets.UTF_8)); + try (CloseableHttpResponse response = client.execute(post)) { + String body = EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8); + return parseFormBody(body); + } + } + + private static NameValuePair param(String name, String value) { + return new BasicNameValuePair(name, value); + } + + static Map parseFormBody(String body) { + Map result = new LinkedHashMap<>(); + for (String pair : body.split("&")) { + if (pair.isEmpty()) { + continue; + } + int eq = pair.indexOf('='); + String key = eq >= 0 ? pair.substring(0, eq) : pair; + String value = eq >= 0 ? pair.substring(eq + 1) : ""; + result.put(decode(key), decode(value)); + } + return result; + } + + private static String decode(String value) { + try { + return URLDecoder.decode(value, "UTF-8"); + } catch (UnsupportedEncodingException e) { + throw new AssertionError(e); + } + } + + public static final class DeviceCode { + final String deviceCode; + final String userCode; + final String verificationUri; + final int expiresInSeconds; + final int intervalSeconds; + + DeviceCode(String deviceCode, String userCode, String verificationUri, int expiresInSeconds, int intervalSeconds) { + this.deviceCode = deviceCode; + this.userCode = userCode; + this.verificationUri = verificationUri; + this.expiresInSeconds = expiresInSeconds; + this.intervalSeconds = intervalSeconds; + } + + public String getUserCode() { + return userCode; + } + + public String getVerificationUri() { + return verificationUri; + } + } +} diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueApiClient.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueApiClient.java new file mode 100644 index 0000000..09b6db2 --- /dev/null +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueApiClient.java @@ -0,0 +1,68 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.crashreporter.pages; + +import org.apache.http.HttpStatus; +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpPost; +import org.apache.http.entity.ContentType; +import org.apache.http.entity.StringEntity; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.util.EntityUtils; +import org.json.JSONObject; + +import java.io.IOException; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * Creates a GitHub issue via the REST API - a POST body, not a URL query string, so no length + * limit (unlike {@link GitHubIssueLinkBuilder}). Needs an access token from + * {@link GitHubDeviceLogin}. + */ +public final class GitHubIssueApiClient { + + private static final Pattern OWNER_REPO_PATTERN = Pattern.compile("github\\.com/([^/]+)/([^/]+)/"); + + private GitHubIssueApiClient() { + } + + /** + * @return the created issue's URL + */ + public static URL createIssue(CloseableHttpClient client, String token, String owner, String repo, + String title, String body) throws IOException { + HttpPost post = new HttpPost("https://api.github.com/repos/" + owner + "/" + repo + "/issues"); + post.setHeader("Authorization", "Bearer " + token); + post.setHeader("Accept", "application/vnd.github+json"); + post.setHeader("X-GitHub-Api-Version", "2022-11-28"); + JSONObject requestJson = new JSONObject(); + requestJson.put("title", title); + requestJson.put("body", body); + post.setEntity(new StringEntity(requestJson.toString(), ContentType.APPLICATION_JSON)); + + try (CloseableHttpResponse response = client.execute(post)) { + String responseBody = EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8); + if (response.getStatusLine().getStatusCode() != HttpStatus.SC_CREATED) { + throw new IOException("GitHub API error " + response.getStatusLine().getStatusCode() + ": " + responseBody); + } + return new URL(new JSONObject(responseBody).getString("html_url")); + } + } + + /** + * @return {@code {owner, repo}} parsed from a link like + * {@code https://github.com/MovingBlocks/Terasology/issues/new}, or {@code null} if + * it doesn't look like a github.com repo link + */ + public static String[] parseOwnerRepo(String reportIssueLink) { + if (reportIssueLink == null) { + return null; + } + Matcher matcher = OWNER_REPO_PATTERN.matcher(reportIssueLink); + return matcher.find() ? new String[] {matcher.group(1), matcher.group(2)} : null; + } +} diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java new file mode 100644 index 0000000..9c10bda --- /dev/null +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java @@ -0,0 +1,104 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.crashreporter.pages; + +import java.io.UnsupportedEncodingException; +import java.net.URLEncoder; +import java.util.Map; + +/** + * Builds a GitHub new-issue URL, pre-filled either via the classic {@code title}/{@code body} query + * parameters (works against any repo, regardless of what issue templates it has - the safe default), + * or, when a downstream app has one, via an issue *form*'s own field IDs (see + * {@link org.terasology.crashreporter.GlobalProperties.KEY#REPORT_ISSUE_TEMPLATE}) - a + * {@code template=} query parameter plus one parameter per field ID, landing the crash summary in + * the repo's own real template instead of overwriting it with a bespoke body. + */ +public final class GitHubIssueLinkBuilder { + + // GitHub rejects the whole URL past this. See github/docs#5136, crashreporter#58. + private static final int GITHUB_URL_BYTE_LIMIT = 8191; + // Safety margin. + private static final int URL_BYTE_BUDGET = GITHUB_URL_BYTE_LIMIT - 200; + private static final String TRUNCATED_SUFFIX = "\n... truncated, see the full log"; + + private GitHubIssueLinkBuilder() { + } + + public static String build(String baseUrl, String title, String body) { + if (baseUrl == null) { + return null; + } + String separator = baseUrl.contains("?") ? "&" : "?"; + String encodedTitle = encode(title); + int budget = URL_BYTE_BUDGET - baseUrl.length() - separator.length() + - "title=".length() - encodedTitle.length() - "&body=".length(); + String encodedBody = fitToBudget(body, budget); + String query = "title=" + encodedTitle + "&body=" + (encodedBody != null ? encodedBody : ""); + return baseUrl + separator + query; + } + + /** + * @param template issue form filename under {@code .github/ISSUE_TEMPLATE/} + * @param fields field ID to value. Null/empty value: field omitted. Too long: truncated or + * dropped, whichever fits. + */ + public static String build(String baseUrl, String template, String title, Map fields) { + if (baseUrl == null) { + return null; + } + String separator = baseUrl.contains("?") ? "&" : "?"; + StringBuilder query = new StringBuilder("template=").append(encode(template)) + .append("&title=").append(encode(title)); + int budget = URL_BYTE_BUDGET - baseUrl.length() - separator.length() - query.length(); + + for (Map.Entry field : fields.entrySet()) { + String value = field.getValue(); + if (value == null || value.isEmpty()) { + continue; + } + String key = field.getKey(); + int overhead = key.length() + 2; // '&' + key + '=' + String encoded = fitToBudget(value, budget - overhead); + if (encoded == null) { + continue; + } + query.append('&').append(key).append('=').append(encoded); + budget -= overhead + encoded.length(); + } + return baseUrl + separator + query; + } + + /** + * URL-encodes {@code value}, truncating with {@link #TRUNCATED_SUFFIX} to fit {@code maxBytes}. + * Null if even the suffix doesn't fit. Truncates the raw text first, then encodes - never + * splits mid-escape. + */ + private static String fitToBudget(String value, int maxBytes) { + String encoded = encode(value); + if (encoded.length() <= maxBytes) { + return encoded; + } + String suffix = encode(TRUNCATED_SUFFIX); + if (suffix.length() > maxBytes) { + return null; + } + String truncated = value; + String withSuffix; + do { + truncated = truncated.substring(0, truncated.length() - 1); + withSuffix = encode(truncated) + suffix; + } while (!truncated.isEmpty() && withSuffix.length() > maxBytes); + return truncated.isEmpty() ? null : withSuffix; + } + + private static String encode(String value) { + try { + return URLEncoder.encode(value, "UTF-8"); + } catch (UnsupportedEncodingException e) { + // UTF-8 is a standard charset every JVM implementation is required to support. + throw new AssertionError(e); + } + } +} diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubLoginDialog.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubLoginDialog.java new file mode 100644 index 0000000..09be023 --- /dev/null +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubLoginDialog.java @@ -0,0 +1,176 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.crashreporter.pages; + +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.client.HttpClientBuilder; +import org.terasology.crashreporter.I18N; + +import javax.swing.JButton; +import javax.swing.JDialog; +import javax.swing.JLabel; +import javax.swing.JPanel; +import javax.swing.JScrollPane; +import javax.swing.JTextArea; +import javax.swing.JTextField; +import javax.swing.SwingConstants; +import javax.swing.SwingUtilities; +import javax.swing.border.EmptyBorder; +import java.awt.BorderLayout; +import java.awt.CardLayout; +import java.awt.Desktop; +import java.awt.Window; +import java.io.IOException; +import java.net.URI; +import java.net.URL; + +/** + * Logs in via {@link GitHubDeviceLogin}, lets the user review/edit the pre-filled title and body, + * then submits via {@link GitHubIssueApiClient} - no URL length limit, unlike + * {@link GitHubIssueLinkBuilder}'s browser-prefill link. + */ +public class GitHubLoginDialog extends JDialog { + + private static final long serialVersionUID = 1L; + + private final CardLayout cards = new CardLayout(); + private final JPanel content = new JPanel(cards); + private final JLabel waitingLabel = new JLabel(); + private final JTextField titleField = new JTextField(); + private final JTextArea bodyArea = new JTextArea(); + private final JLabel resultLabel = new JLabel(); + + private final String clientId; + private final String owner; + private final String repo; + + private volatile String accessToken; + + public GitHubLoginDialog(Window ownerWindow, String clientId, String owner, String repo, + String title, String body) { + super(ownerWindow, I18N.getMessage("githubLoginTitle"), ModalityType.APPLICATION_MODAL); + this.clientId = clientId; + this.owner = owner; + this.repo = repo; + + setSize(500, 400); + setLocationRelativeTo(ownerWindow); + setLayout(new BorderLayout()); + add(content, BorderLayout.CENTER); + + content.add(buildWaitingCard(), "waiting"); + content.add(buildReviewCard(title, body), "review"); + content.add(buildResultCard(), "result"); + cards.show(content, "waiting"); + + startLogin(); + } + + private JPanel buildWaitingCard() { + JPanel panel = new JPanel(new BorderLayout(10, 10)); + panel.setBorder(new EmptyBorder(20, 20, 20, 20)); + waitingLabel.setText(I18N.getMessage("githubLoginRequesting")); + waitingLabel.setHorizontalAlignment(SwingConstants.CENTER); + panel.add(waitingLabel, BorderLayout.CENTER); + JButton cancel = new JButton(I18N.getMessage("githubLoginCancel")); + cancel.addActionListener(e -> dispose()); + panel.add(cancel, BorderLayout.SOUTH); + return panel; + } + + private JPanel buildReviewCard(String title, String body) { + JPanel panel = new JPanel(new BorderLayout(10, 10)); + panel.setBorder(new EmptyBorder(10, 10, 10, 10)); + titleField.setText(title); + bodyArea.setText(body); + bodyArea.setLineWrap(true); + bodyArea.setWrapStyleWord(true); + + JPanel top = new JPanel(new BorderLayout(5, 0)); + top.add(new JLabel(I18N.getMessage("githubLoginReviewTitleLabel")), BorderLayout.WEST); + top.add(titleField, BorderLayout.CENTER); + panel.add(top, BorderLayout.NORTH); + panel.add(new JScrollPane(bodyArea), BorderLayout.CENTER); + + JButton submit = new JButton(I18N.getMessage("githubLoginSubmit")); + submit.addActionListener(e -> submitIssue()); + JButton cancel = new JButton(I18N.getMessage("githubLoginCancel")); + cancel.addActionListener(e -> dispose()); + JPanel buttons = new JPanel(); + buttons.add(cancel); + buttons.add(submit); + panel.add(buttons, BorderLayout.SOUTH); + return panel; + } + + private JPanel buildResultCard() { + JPanel panel = new JPanel(new BorderLayout(10, 10)); + panel.setBorder(new EmptyBorder(20, 20, 20, 20)); + resultLabel.setHorizontalAlignment(SwingConstants.CENTER); + panel.add(resultLabel, BorderLayout.CENTER); + JButton close = new JButton(I18N.getMessage("close")); + close.addActionListener(e -> dispose()); + panel.add(close, BorderLayout.SOUTH); + return panel; + } + + private void startLogin() { + Thread thread = new Thread(() -> { + try (CloseableHttpClient client = HttpClientBuilder.create().build()) { + GitHubDeviceLogin.DeviceCode code = GitHubDeviceLogin.requestDeviceCode(client, clientId); + SwingUtilities.invokeLater(() -> showCode(code)); + accessToken = GitHubDeviceLogin.pollForAccessToken(client, clientId, code); + SwingUtilities.invokeLater(() -> cards.show(content, "review")); + } catch (IOException | InterruptedException e) { + SwingUtilities.invokeLater(() -> showFailure(e)); + } + }, "GitHubDeviceLogin"); + thread.setDaemon(true); + thread.start(); + } + + private void showCode(GitHubDeviceLogin.DeviceCode code) { + openInBrowser(code.getVerificationUri()); + waitingLabel.setText("

" + I18N.getMessage("githubLoginWaiting", + code.getVerificationUri(), code.getUserCode()) + "
"); + } + + private void submitIssue() { + cards.show(content, "waiting"); + waitingLabel.setText(I18N.getMessage("githubLoginSubmitting")); + String title = titleField.getText(); + String body = bodyArea.getText(); + Thread thread = new Thread(() -> { + try (CloseableHttpClient client = HttpClientBuilder.create().build()) { + URL issueUrl = GitHubIssueApiClient.createIssue(client, accessToken, owner, repo, title, body); + SwingUtilities.invokeLater(() -> showSuccess(issueUrl)); + } catch (IOException e) { + SwingUtilities.invokeLater(() -> showFailure(e)); + } + }, "GitHubIssueSubmit"); + thread.setDaemon(true); + thread.start(); + } + + private void showSuccess(URL issueUrl) { + resultLabel.setText("" + I18N.getMessage("githubLoginSuccess", issueUrl) + ""); + cards.show(content, "result"); + openInBrowser(issueUrl.toString()); + } + + private void showFailure(Exception e) { + resultLabel.setText("" + I18N.getMessage("githubLoginFailed", e.getLocalizedMessage()) + ""); + cards.show(content, "result"); + } + + private static void openInBrowser(String url) { + if (Desktop.isDesktopSupported() && Desktop.getDesktop().isSupported(Desktop.Action.BROWSE)) { + try { + Desktop.getDesktop().browse(new URI(url)); + } catch (Exception e) { + e.printStackTrace(System.err); + } + } + } +} diff --git a/cr-core/src/main/resources/i18n/MessagesBundle.properties b/cr-core/src/main/resources/i18n/MessagesBundle.properties index 74aa2f8..b742a84 100644 --- a/cr-core/src/main/resources/i18n/MessagesBundle.properties +++ b/cr-core/src/main/resources/i18n/MessagesBundle.properties @@ -13,6 +13,16 @@ uploadLog2=Upload log file noUpload=You did not yet upload the log file noUploadLinkText=No file uploaded reportIssue=File an issue on GitHub +reportIssueDirectly=Submit issue directly (log in) +githubLoginTitle=GitHub Login +githubLoginRequesting=Requesting a login code... +githubLoginWaiting=Go to {0} and enter code: {1} +githubLoginReviewTitleLabel=Title +githubLoginSubmit=Submit Issue +githubLoginSubmitting=Submitting issue... +githubLoginCancel=Cancel +githubLoginSuccess=Issue created: {0} +githubLoginFailed=Login failed: {0} joinDiscord=Join Discord Server stackTrace=StackTrace logFile=Log File diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java new file mode 100644 index 0000000..afb982b --- /dev/null +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java @@ -0,0 +1,232 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.crashreporter.pages; + +import org.junit.jupiter.api.Test; + +import java.net.MalformedURLException; +import java.net.URL; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Regression tests for #53 item 3: "Report Issue" opened a blank GitHub form instead of one + * pre-filled with the crash summary. + */ +class CrashSummaryTest { + + private static final String LOG_TEXT = + "10:00:00.000 [main] INFO o.t.e.version.TerasologyVersion - " + + "[buildNumber=42, buildId=42, buildTag=Terasology-42, buildUrl=, jobName=Terasology/engine/develop, " + + "dateTime=2026-08-20, displayVersion=Aeternum, engineVersion=5.4.0-SNAPSHOT]\n" + + "10:00:00.100 [main] INFO o.t.e.core.TerasologyEngine - OS: Linux, arch: amd64, version: 6.12.85\n" + + "10:00:01.000 [main] INFO o.t.e.core.modes.loadProcesses.RegisterMods - Activating module: engine:5.4.0-SNAPSHOT\n" + + "10:00:01.010 [main] INFO o.t.e.core.modes.loadProcesses.RegisterMods - Activating module: CoreAssets:2.4.0\n" + + "10:00:01.020 [main] INFO o.t.e.core.modes.loadProcesses.RegisterMods - Activating module: CoreAssets:2.4.0\n"; + + @Test + void extractsEngineAndDisplayVersion() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + String body = summary.buildBody(null); + + assertTrue(body.contains("5.4.0-SNAPSHOT"), "Expected the engine version in the body, got: " + body); + assertTrue(body.contains("Aeternum"), "Expected the display version in the body, got: " + body); + } + + @Test + void extractsActiveModulesAndDeduplicates() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + String body = summary.buildBody(null); + + assertTrue(body.contains("engine:5.4.0-SNAPSHOT"), "Expected engine module, got: " + body); + // Occurs twice in the log (duplicate "Activating module" line) - should appear once in the body. + int occurrences = body.split("CoreAssets:2\\.4\\.0", -1).length - 1; + assertEquals(1, occurrences, "Expected the duplicate module line deduplicated, got: " + body); + } + + @Test + void missingVersionAndModulesDegradeGracefullyInsteadOfFailing() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), "no relevant lines here"); + String body = summary.buildBody(null); + + assertTrue(body.contains("unknown"), "Expected a fallback for a missing version, got: " + body); + assertTrue(body.contains("none found in logs"), "Expected a fallback for an empty module list, got: " + body); + } + + @Test + void titleUsesExceptionClassAndMessage() { + CrashSummary summary = CrashSummary.extract(new IllegalStateException("world was null"), LOG_TEXT); + + assertEquals("Crash: IllegalStateException: world was null", summary.buildTitle()); + } + + @Test + void bodyIncludesTheExceptionExtract() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("kaboom"), LOG_TEXT); + String body = summary.buildBody(null); + + assertTrue(body.contains("kaboom"), "Expected the exception message in the body, got: " + body); + assertTrue(body.contains("RuntimeException"), "Expected the exception type in the body, got: " + body); + } + + @Test + void bodyIncludesThePastebinLinkWhenUploaded() throws MalformedURLException { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + URL link = new URL("https://pastebin.com/abc123"); + + String body = summary.buildBody(link); + + assertTrue(body.contains("https://pastebin.com/abc123"), "Expected the PasteBin link in the body, got: " + body); + } + + @Test + void bodyNotesWhenUploadWasSkipped() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + + String body = summary.buildBody(null); + + assertFalse(body.contains("null"), "A skipped upload must not leak the literal string \"null\" into the body: " + body); + assertTrue(body.contains("not uploaded"), "Expected a note that upload was skipped, got: " + body); + } + + // Regression: ErrorMessagePanel#getLog() combines every log tab, not just the one that + // triggered the report, but only the in-process exception ever made it into the pre-filled + // issue - an exception logged in a different tab (e.g. an earlier init-time failure) was + // silently left out even though it was right there in the combined text. All exceptions - the + // primary one and every other one found - are listed together, one labeled code block each + // (full trace, not just a one-line header - a real fix needs the actual trace), before + // "### Environment", not split across two separate sections. + @Test + void bodyListsExceptionsFoundInOtherLogTabsWithTheirFullTraceNamingTheTab() { + String combinedLog = "=== Terasology-init.log ===\n" + LOG_TEXT + + "\n=== Terasology-game.log ===\n" + + "10:10:05.123 [main] ERROR o.t.e.core.TerasologyEngine - Uncaught exception in main loop\n" + + "java.lang.NullPointerException: world was null\n" + + "\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n"; + + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), combinedLog); + String body = summary.buildBody(null); + + assertTrue(body.contains("### Exceptions"), "Expected a single unified exceptions section, got: " + body); + assertTrue(body.contains("**Terasology-game.log**\n\n```\n" + + "10:10:05.123 [main] ERROR o.t.e.core.TerasologyEngine - Uncaught exception in main loop\n" + + "java.lang.NullPointerException: world was null\n" + + "\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n```"), + "Expected the other tab's exception as its own labeled block, with the line logged right before it and the full " + + "trace, got: " + body); + int exceptionsIndex = body.indexOf("### Exceptions"); + int environmentIndex = body.indexOf("### Environment"); + assertTrue(exceptionsIndex >= 0 && environmentIndex > exceptionsIndex, + "Expected \"### Exceptions\" before \"### Environment\", got: " + body); + } + + @Test + void bodyAttributesThePrimaryExceptionToItsOwnTabInsteadOfListingItTwice() { + RuntimeException primary = new RuntimeException("boom"); + // The crash is very often also logged (by the crashed process itself) in one of its own + // log tabs - that's the same exception, not another one, and must not be listed twice. It's + // also the *real* trace (see the class javadoc on macOS reconstruction), so it must be used + // in preference to the exception object's own (possibly fake) trace. + String combinedLog = "=== Terasology-game.log ===\n" + + primary + "\n" + + "\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n"; + + CrashSummary summary = CrashSummary.extract(primary, combinedLog); + String body = summary.buildBody(null); + + assertTrue(body.contains("**Terasology-game.log**\n\n```\njava.lang.RuntimeException: boom\n" + + "\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n```"), + "Expected the primary exception's block attributed to the tab it was found in, using that tab's real trace, got: " + + body); + int blocks = body.split("\\*\\*Terasology-game\\.log\\*\\*", -1).length - 1; + assertEquals(1, blocks, "Expected exactly one block - the primary exception must not also be listed as an \"other\" one: " + + body); + } + + @Test + void bodyIncludesOnlyTheLastFiveLinesLoggedBeforeTheException() { + StringBuilder combinedLog = new StringBuilder("=== Terasology-game.log ===\n"); + for (int i = 1; i <= 8; i++) { + combinedLog.append("log line ").append(i).append('\n'); + } + combinedLog.append("java.lang.NullPointerException: world was null\n") + .append("\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n"); + + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), combinedLog.toString()); + String body = summary.buildBody(null); + + assertFalse(body.contains("log line 1\n") || body.contains("log line 2\n") || body.contains("log line 3\n"), + "Expected only the last 5 lines of context, not all 8, got: " + body); + assertTrue(body.contains("log line 4\nlog line 5\nlog line 6\nlog line 7\nlog line 8\n" + + "java.lang.NullPointerException: world was null"), + "Expected the last 5 lines directly before the exception's header, got: " + body); + } + + @Test + void bodyFallsBackToTheExceptionsOwnTraceWhenNotFoundInAnyTab() { + RuntimeException primary = new RuntimeException("boom"); + CrashSummary summary = CrashSummary.extract(primary, LOG_TEXT); + String body = summary.buildBody(null); + + assertTrue(body.contains("**this crash**\n\n```\njava.lang.RuntimeException: boom\n"), + "Expected a fallback label and the exception's own trace when it isn't found in any log tab, got: " + body); + assertTrue(body.contains(CrashSummaryTest.class.getName()), + "Expected this test's own stack frame in the fallback trace, got: " + body); + } + + // buildIssueFormFields() feeds GitHubIssueLinkBuilder's template-based overload (see + // GitHubIssueLinkBuilderTest) - used instead of buildBody() when a downstream app has configured + // REPORT_ISSUE_TEMPLATE, landing the summary in that issue form's own fields. + @Test + void issueFormFieldsIncludeVersionOsAndTheExceptionBlocks() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + Map fields = summary.buildIssueFormFields(null); + + assertEquals("5.4.0-SNAPSHOT (Aeternum)", fields.get("terasology_version")); + assertTrue(fields.get("operating_system").contains(System.getProperty("os.name")), fields.get("operating_system")); + assertEquals(System.getProperty("java.version"), fields.get("java_version")); + assertTrue(fields.get("actual_behavior").contains("RuntimeException: boom"), fields.get("actual_behavior")); + } + + @Test + void issueFormFieldsOmitVersionWhenNotFoundInsteadOfSayingUnknown() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), "no relevant lines here"); + Map fields = summary.buildIssueFormFields(null); + + // Unlike buildBody()'s "unknown" fallback, an omitted field is left blank in the actual issue + // form for the user to fill in themselves - "unknown" pre-filled into a real form field would + // read as if the reporter deliberately couldn't tell, not as an untouched field. + assertNull(fields.get("terasology_version"), "Expected no terasology_version entry, got: " + fields); + } + + @Test + void issueFormFieldsOmitLogDetailsWhenUploadWasSkipped() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + Map fields = summary.buildIssueFormFields(null); + + assertNull(fields.get("log_details"), "Expected no log_details entry when nothing was uploaded, got: " + fields); + } + + @Test + void issueFormFieldsIncludeThePastebinLinkWhenUploaded() throws MalformedURLException { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + URL link = new URL("https://pastebin.com/abc123"); + + Map fields = summary.buildIssueFormFields(link); + + assertTrue(fields.get("log_details").contains("https://pastebin.com/abc123"), fields.get("log_details")); + } + + @Test + void issueFormFieldsIncludeActiveModulesUnderAdditionalContext() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + Map fields = summary.buildIssueFormFields(null); + + assertTrue(fields.get("additional_context").contains("engine:5.4.0-SNAPSHOT"), fields.get("additional_context")); + } +} diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubDeviceLoginTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubDeviceLoginTest.java new file mode 100644 index 0000000..3a7a12f --- /dev/null +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubDeviceLoginTest.java @@ -0,0 +1,110 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.crashreporter.pages; + +import org.apache.http.HttpEntity; +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpUriRequest; +import org.apache.http.entity.StringEntity; +import org.apache.http.impl.client.CloseableHttpClient; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +class GitHubDeviceLoginTest { + + @Test + void parseFormBodyDecodesKeysAndValues() { + Map fields = GitHubDeviceLogin.parseFormBody( + "device_code=abc&user_code=WDJB-MJHT&verification_uri=https%3A%2F%2Fgithub.com%2Flogin%2Fdevice" + + "&expires_in=900&interval=5"); + + assertEquals("abc", fields.get("device_code")); + assertEquals("WDJB-MJHT", fields.get("user_code")); + assertEquals("https://github.com/login/device", fields.get("verification_uri")); + assertEquals("900", fields.get("expires_in")); + assertEquals("5", fields.get("interval")); + } + + @Test + void parseFormBodyHandlesEmptyBody() { + assertEquals(0, GitHubDeviceLogin.parseFormBody("").size()); + } + + @Test + void requestDeviceCodeParsesTheResponse() throws IOException { + CloseableHttpClient client = mockClientReturning( + "device_code=abc&user_code=WDJB-MJHT&verification_uri=https%3A%2F%2Fgithub.com%2Flogin%2Fdevice" + + "&expires_in=900&interval=5"); + + GitHubDeviceLogin.DeviceCode code = GitHubDeviceLogin.requestDeviceCode(client, "client-id"); + + assertEquals("WDJB-MJHT", code.getUserCode()); + assertEquals("https://github.com/login/device", code.getVerificationUri()); + } + + @Test + void requestDeviceCodeThrowsOnError() throws IOException { + CloseableHttpClient client = mockClientReturning("error=access_denied&error_description=nope"); + + assertThrows(IOException.class, () -> GitHubDeviceLogin.requestDeviceCode(client, "client-id")); + } + + @Test + void pollForAccessTokenReturnsTokenOnSuccess() throws IOException, InterruptedException { + CloseableHttpClient client = mockClientReturning("access_token=tok123&token_type=bearer"); + GitHubDeviceLogin.DeviceCode code = fastDeviceCode(); + + String token = GitHubDeviceLogin.pollForAccessToken(client, "client-id", code); + + assertEquals("tok123", token); + } + + @Test + void pollForAccessTokenRetriesOnAuthorizationPending() throws IOException, InterruptedException { + CloseableHttpResponse pending = response("error=authorization_pending"); + CloseableHttpResponse granted = response("access_token=tok123"); + CloseableHttpClient client = mock(CloseableHttpClient.class); + when(client.execute(any(HttpUriRequest.class))).thenReturn(pending).thenReturn(granted); + + String token = GitHubDeviceLogin.pollForAccessToken(client, "client-id", fastDeviceCode()); + + assertEquals("tok123", token); + } + + @Test + void pollForAccessTokenThrowsOnAccessDenied() throws IOException { + CloseableHttpClient client = mockClientReturning("error=access_denied&error_description=User denied access"); + + IOException e = assertThrows(IOException.class, + () -> GitHubDeviceLogin.pollForAccessToken(client, "client-id", fastDeviceCode())); + assertEquals("User denied access", e.getMessage()); + } + + /** interval=0, one-second window - pollForAccessToken doesn't actually sleep long in a test. */ + private static GitHubDeviceLogin.DeviceCode fastDeviceCode() { + return new GitHubDeviceLogin.DeviceCode("device-code", "USER-CODE", "https://github.com/login/device", 5, 0); + } + + private static CloseableHttpClient mockClientReturning(String formBody) throws IOException { + CloseableHttpResponse resp = response(formBody); + CloseableHttpClient client = mock(CloseableHttpClient.class); + when(client.execute(any(HttpUriRequest.class))).thenReturn(resp); + return client; + } + + private static CloseableHttpResponse response(String formBody) throws IOException { + CloseableHttpResponse response = mock(CloseableHttpResponse.class); + HttpEntity entity = new StringEntity(formBody); + when(response.getEntity()).thenReturn(entity); + return response; + } +} diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueApiClientTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueApiClientTest.java new file mode 100644 index 0000000..55abeaa --- /dev/null +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueApiClientTest.java @@ -0,0 +1,70 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.crashreporter.pages; + +import org.apache.http.HttpEntity; +import org.apache.http.ProtocolVersion; +import org.apache.http.StatusLine; +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpUriRequest; +import org.apache.http.entity.StringEntity; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.message.BasicStatusLine; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.net.URL; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +class GitHubIssueApiClientTest { + + @Test + void parseOwnerRepoExtractsBothParts() { + assertArrayEquals(new String[] {"MovingBlocks", "Terasology"}, + GitHubIssueApiClient.parseOwnerRepo("https://github.com/MovingBlocks/Terasology/issues/new")); + } + + @Test + void parseOwnerRepoReturnsNullForNonGithubLink() { + assertNull(GitHubIssueApiClient.parseOwnerRepo("https://example.com/issues/new")); + assertNull(GitHubIssueApiClient.parseOwnerRepo(null)); + } + + @Test + void createIssueReturnsTheHtmlUrlOnSuccess() throws IOException { + CloseableHttpResponse resp = response(201, "{\"html_url\": \"https://github.com/MovingBlocks/Terasology/issues/123\"}"); + CloseableHttpClient client = mock(CloseableHttpClient.class); + when(client.execute(any(HttpUriRequest.class))).thenReturn(resp); + + URL issueUrl = GitHubIssueApiClient.createIssue(client, "token", "MovingBlocks", "Terasology", "t", "b"); + + assertEquals("https://github.com/MovingBlocks/Terasology/issues/123", issueUrl.toString()); + } + + @Test + void createIssueThrowsOnNon201Response() throws IOException { + CloseableHttpResponse resp = response(401, "{\"message\": \"Bad credentials\"}"); + CloseableHttpClient client = mock(CloseableHttpClient.class); + when(client.execute(any(HttpUriRequest.class))).thenReturn(resp); + + assertThrows(IOException.class, + () -> GitHubIssueApiClient.createIssue(client, "bad-token", "o", "r", "t", "b")); + } + + private static CloseableHttpResponse response(int statusCode, String jsonBody) throws IOException { + CloseableHttpResponse response = mock(CloseableHttpResponse.class); + HttpEntity entity = new StringEntity(jsonBody); + when(response.getEntity()).thenReturn(entity); + StatusLine statusLine = new BasicStatusLine(new ProtocolVersion("HTTP", 1, 1), statusCode, ""); + when(response.getStatusLine()).thenReturn(statusLine); + return response; + } +} diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java new file mode 100644 index 0000000..88d513b --- /dev/null +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java @@ -0,0 +1,125 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.crashreporter.pages; + +import org.junit.jupiter.api.Test; + +import java.util.LinkedHashMap; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class GitHubIssueLinkBuilderTest { + + @Test + void returnsNullWhenBaseUrlIsNotConfigured() { + // REPORT_ISSUE_LINK is only set by downstream apps (cr-terasology, cr-destsol, ...), not by + // cr-core's own defaults - build() must not silently produce a broken "null?title=..." link. + assertNull(GitHubIssueLinkBuilder.build(null, "t", "b")); + assertNull(GitHubIssueLinkBuilder.build(null, "template.yml", "t", new LinkedHashMap<>())); + } + + @Test + void appendsTitleAndBodyAsQueryParameters() { + String link = GitHubIssueLinkBuilder.build("https://github.com/MovingBlocks/Terasology/issues/new", + "Crash: NullPointerException", "some body text"); + + assertTrue(link.startsWith("https://github.com/MovingBlocks/Terasology/issues/new?")); + assertTrue(link.contains("title=Crash%3A+NullPointerException"), link); + assertTrue(link.contains("body=some+body+text"), link); + } + + @Test + void encodesSpecialCharactersInTitleAndBody() { + String link = GitHubIssueLinkBuilder.build("https://example.com/issues/new", "a & b", "line1\nline2"); + + assertTrue(link.contains("title=a+%26+b"), link); + assertTrue(link.contains("body=line1%0Aline2"), link); + } + + @Test + void usesAmpersandWhenBaseUrlAlreadyHasAQueryString() { + String link = GitHubIssueLinkBuilder.build("https://example.com/issues/new?template=bug", "t", "b"); + + assertEquals("https://example.com/issues/new?template=bug&title=t&body=b", link); + } + + @Test + void formBuildAppendsTemplateTitleAndEachFieldAsItsOwnQueryParameter() { + Map fields = new LinkedHashMap<>(); + fields.put("terasology_version", "5.4.0-SNAPSHOT"); + fields.put("operating_system", "Mac OS X"); + + String link = GitHubIssueLinkBuilder.build("https://github.com/MovingBlocks/Terasology/issues/new", + "crash-bug-report.yml", "Crash: NullPointerException", fields); + + assertTrue(link.contains("template=crash-bug-report.yml"), link); + assertTrue(link.contains("title=Crash%3A+NullPointerException"), link); + assertTrue(link.contains("terasology_version=5.4.0-SNAPSHOT"), link); + assertTrue(link.contains("operating_system=Mac+OS+X"), link); + } + + @Test + void formBuildOmitsFieldsWithNoValueInsteadOfPreFillingThemBlank() { + Map fields = new LinkedHashMap<>(); + fields.put("terasology_version", ""); + fields.put("java_version", null); + fields.put("operating_system", "Linux"); + + String link = GitHubIssueLinkBuilder.build("https://example.com/issues/new", "crash-bug-report.yml", "t", fields); + + assertFalse(link.contains("terasology_version="), link); + assertFalse(link.contains("java_version="), link); + assertTrue(link.contains("operating_system=Linux"), link); + } + + @Test + void bodyBuildStaysUnderGitHubsUrlByteLimit() { + // GitHub rejects the whole thing past 8191 bytes. Must truncate. + String hugeBody = "x".repeat(50_000); + + String link = GitHubIssueLinkBuilder.build("https://github.com/MovingBlocks/Terasology/issues/new", + "Crash: NullPointerException", hugeBody); + + assertTrue(link.length() < 8191, "link was " + link.length() + " bytes: " + link); + assertTrue(link.contains("truncated"), link); + } + + @Test + void formBuildStaysUnderGitHubsUrlByteLimit() { + Map fields = new LinkedHashMap<>(); + fields.put("terasology_version", "5.4.0-SNAPSHOT"); + fields.put("operating_system", "Mac OS X"); + fields.put("actual_behavior", "x".repeat(50_000)); + fields.put("additional_context", "y".repeat(50_000)); + + String link = GitHubIssueLinkBuilder.build("https://github.com/MovingBlocks/Terasology/issues/new", + "crash-bug-report.yml", "Crash: NullPointerException", fields); + + assertTrue(link.length() < 8191, "link was " + link.length() + " bytes: " + link); + // Early fields stay full, only the overflowing tail gets trimmed. + assertTrue(link.contains("terasology_version=5.4.0-SNAPSHOT"), link); + assertTrue(link.contains("operating_system=Mac+OS+X"), link); + } + + @Test + void truncationNeverSplitsAPercentEscape() { + Map fields = new LinkedHashMap<>(); + // Every char is a 3-byte "%XX" escape, so a mid-escape cut would hide here. + fields.put("actual_behavior", "&".repeat(50_000)); + + String link = GitHubIssueLinkBuilder.build("https://example.com/issues/new", "t.yml", "t", fields); + + int valueStart = link.indexOf("actual_behavior=") + "actual_behavior=".length(); + String value = link.substring(valueStart); + for (int i = 0; i < value.length(); i++) { + if (value.charAt(i) == '%') { + assertTrue(i + 2 < value.length(), "truncated escape at end of value: " + value); + } + } + } +} diff --git a/cr-terasology/src/main/resources/crashreporter.properties b/cr-terasology/src/main/resources/crashreporter.properties index 47a7a74..baaf910 100644 --- a/cr-terasology/src/main/resources/crashreporter.properties +++ b/cr-terasology/src/main/resources/crashreporter.properties @@ -1,5 +1,7 @@ SUPPORT_FORUM_LINK=http://forum.terasology.org/forum/support.20/ REPORT_ISSUE_LINK=https://github.com/MovingBlocks/Terasology/issues/new +REPORT_ISSUE_TEMPLATE=crash-bug-report.yml +REPORT_ISSUE_OAUTH_CLIENT_ID=Ov23liSKtBll7VXE8Vur JOIN_DISCORD_LINK=https://discord.gg/terasology RES_BANNER_IMAGE=icons/banner.jpg From c1437a1aa81c9017d465f4aa9047fecea2e99ddb Mon Sep 17 00:00:00 2001 From: Cervator Date: Wed, 30 Sep 2026 21:45:22 -0400 Subject: [PATCH 2/6] build: regenerate lockfiles for org.json Master now locks dependencies (#68); this PR's `org.json:json` was not in the lock state, so the merged build would fail the same way PR-68 #7 did. Co-Authored-By: Claude Fable 5.1 --- cr-core/gradle.lockfile | 1 + cr-destsol/gradle.lockfile | 1 + cr-terasology/gradle.lockfile | 1 + 3 files changed, 3 insertions(+) diff --git a/cr-core/gradle.lockfile b/cr-core/gradle.lockfile index b7d2293..fa69e8d 100644 --- a/cr-core/gradle.lockfile +++ b/cr-core/gradle.lockfile @@ -39,6 +39,7 @@ org.apiguardian:apiguardian-api:1.1.2=testCompileClasspath org.checkerframework:checker-qual:2.5.2=pmd org.checkerframework:checker-qual:3.12.0=checkstyle,compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath org.javassist:javassist:3.28.0-GA=checkstyle +org.json:json:20260814=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath org.junit.jupiter:junit-jupiter-api:5.10.1=testCompileClasspath,testRuntimeClasspath org.junit.jupiter:junit-jupiter-engine:5.10.1=testRuntimeClasspath org.junit.jupiter:junit-jupiter-params:5.10.1=testCompileClasspath,testRuntimeClasspath diff --git a/cr-destsol/gradle.lockfile b/cr-destsol/gradle.lockfile index 815da04..296dfa5 100644 --- a/cr-destsol/gradle.lockfile +++ b/cr-destsol/gradle.lockfile @@ -18,5 +18,6 @@ org.apache.httpcomponents:httpclient:4.5.13=runtimeClasspath,testRuntimeClasspat org.apache.httpcomponents:httpcore:4.4.13=runtimeClasspath,testRuntimeClasspath org.apache.httpcomponents:httpmime:4.5.13=runtimeClasspath,testRuntimeClasspath org.checkerframework:checker-qual:3.12.0=runtimeClasspath,testRuntimeClasspath +org.json:json:20260814=runtimeClasspath,testRuntimeClasspath org:jpastebin:1.0.1=runtimeClasspath,testRuntimeClasspath empty=annotationProcessor,compileClasspath,testAnnotationProcessor,testCompileClasspath diff --git a/cr-terasology/gradle.lockfile b/cr-terasology/gradle.lockfile index 5c825ee..eb60ab8 100644 --- a/cr-terasology/gradle.lockfile +++ b/cr-terasology/gradle.lockfile @@ -18,5 +18,6 @@ org.apache.httpcomponents:httpclient:4.5.13=runtimeClasspath,testRuntimeClasspat org.apache.httpcomponents:httpcore:4.4.13=runtimeClasspath,testRuntimeClasspath org.apache.httpcomponents:httpmime:4.5.13=runtimeClasspath,testRuntimeClasspath org.checkerframework:checker-qual:3.12.0=runtimeClasspath,testRuntimeClasspath +org.json:json:20260814=runtimeClasspath,testRuntimeClasspath org:jpastebin:1.0.1=runtimeClasspath,testRuntimeClasspath empty=annotationProcessor,compileClasspath,testAnnotationProcessor,testCompileClasspath From cfc3b99fd9b2835490626e7ae888d77ea7643c08 Mon Sep 17 00:00:00 2001 From: Cervator Date: Wed, 30 Sep 2026 22:49:18 -0400 Subject: [PATCH 3/6] =?UTF-8?q?fix(reporter):=20PR=20#58=20review=20?= =?UTF-8?q?=E2=80=94=20linear=20URL=20truncation,=20cancellable=20login,?= =?UTF-8?q?=20hardened=20GitHub=20responses,=20full=20cause=20chains?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fitToBudget` re-encoded the whole body per dropped character on the Swing thread, so a large crash froze the dialog. Cancel never interrupted the device-flow poller. Both worker threads caught only `IOException`, so a non-form GitHub response killed the thread with the dialog stuck on "Requesting…". The trace regex stopped at `... N more`, dropping every later `Caused by:`. Co-Authored-By: Claude Fable 5.1 --- .../crashreporter/pages/CrashSummary.java | 7 +++- .../pages/GitHubDeviceLogin.java | 30 ++++++++++++- .../pages/GitHubIssueApiClient.java | 13 +++++- .../pages/GitHubIssueLinkBuilder.java | 33 +++++++++++---- .../pages/GitHubLoginDialog.java | 42 ++++++++++++------- .../crashreporter/pages/CrashSummaryTest.java | 20 +++++++++ .../pages/GitHubDeviceLoginTest.java | 35 ++++++++++++++++ .../pages/GitHubIssueApiClientTest.java | 18 ++++++++ .../pages/GitHubIssueLinkBuilderTest.java | 31 ++++++++++++++ 9 files changed, 198 insertions(+), 31 deletions(-) diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java index 64192fb..155c405 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java @@ -50,9 +50,12 @@ public final class CrashSummary { // A log-formatted stack trace: a "some.FullyQualified.NameException[: message]" header line // immediately followed by one or more "at ..."/"Caused by: ..." frame lines - the shape every // JVM logging framework prints a Throwable in (Logback's %ex, java.util.logging, a raw - // printStackTrace()), regardless of which class emits it. + // printStackTrace()), regardless of which class emits it. "... N more" / "... N common frames + // omitted" lines count as frames too: they sit between a cause chain's links, and a pattern that + // stopped at them dropped every "Caused by:" after the first - which is the root cause. private static final Pattern STACK_TRACE_HEADER_PATTERN = Pattern.compile( - "(?m)^([\\w$]+(?:\\.[\\w$]+)+(?:Exception|Error))(:[^\\n]*)?\\n((?:[ \\t]*(?:at |Caused by:)[^\\n]*\\n?)+)"); + "(?m)^([\\w$]+(?:\\.[\\w$]+)+(?:Exception|Error))(:[^\\n]*)?\\n" + + "((?:[ \\t]*(?:at |Caused by:|\\.\\.\\. \\d+ (?:more|common frames omitted))[^\\n]*\\n?)+)"); private final Throwable exception; private final List exceptionBlocks; diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubDeviceLogin.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubDeviceLogin.java index 13332d2..8b70557 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubDeviceLogin.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubDeviceLogin.java @@ -4,6 +4,7 @@ package org.terasology.crashreporter.pages; import org.apache.http.NameValuePair; +import org.apache.http.StatusLine; import org.apache.http.client.entity.UrlEncodedFormEntity; import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpPost; @@ -39,8 +40,27 @@ public static DeviceCode requestDeviceCode(CloseableHttpClient client, String cl Map fields = post(client, DEVICE_CODE_URL, param("client_id", clientId), param("scope", SCOPE)); failOnError(fields); - return new DeviceCode(fields.get("device_code"), fields.get("user_code"), fields.get("verification_uri"), - Integer.parseInt(fields.get("expires_in")), Integer.parseInt(fields.get("interval"))); + return new DeviceCode(required(fields, "device_code"), required(fields, "user_code"), + required(fields, "verification_uri"), requiredInt(fields, "expires_in"), requiredInt(fields, "interval")); + } + + // A rate-limited or erroring endpoint answers with HTML or JSON, not the form fields this + // expects. Reporting that as an IOException keeps it on the path the dialog shows to the user; + // a NullPointerException or NumberFormatException here used to kill the login thread silently. + private static String required(Map fields, String name) throws IOException { + String value = fields.get(name); + if (value == null || value.isEmpty()) { + throw new IOException("GitHub's response is missing '" + name + "': " + fields); + } + return value; + } + + private static int requiredInt(Map fields, String name) throws IOException { + try { + return Integer.parseInt(required(fields, name)); + } catch (NumberFormatException e) { + throw new IOException("GitHub's response has a non-numeric '" + name + "': " + fields.get(name), e); + } } /** Blocks until authorized, denied, or expired. Call off the UI thread. */ @@ -88,6 +108,12 @@ private static Map post(CloseableHttpClient client, String url, post.setEntity(new UrlEncodedFormEntity(paramList, StandardCharsets.UTF_8)); try (CloseableHttpResponse response = client.execute(post)) { String body = EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8); + // The OAuth endpoints report expected conditions (authorization_pending, access_denied) + // in the form body with a 200, so only a genuinely failed request is rejected here. + StatusLine status = response.getStatusLine(); + if (status != null && status.getStatusCode() / 100 != 2) { + throw new IOException("GitHub login endpoint returned HTTP " + status.getStatusCode() + ": " + body); + } return parseFormBody(body); } } diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueApiClient.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueApiClient.java index 09b6db2..c9210be 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueApiClient.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueApiClient.java @@ -10,6 +10,7 @@ import org.apache.http.entity.StringEntity; import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.util.EntityUtils; +import org.json.JSONException; import org.json.JSONObject; import java.io.IOException; @@ -25,7 +26,9 @@ */ public final class GitHubIssueApiClient { - private static final Pattern OWNER_REPO_PATTERN = Pattern.compile("github\\.com/([^/]+)/([^/]+)/"); + // Trailing slash optional: "https://github.com/Owner/Repo" is a valid REPORT_ISSUE_LINK too, + // and requiring the slash silently hid the direct-submit button for it. + private static final Pattern OWNER_REPO_PATTERN = Pattern.compile("github\\.com/([^/?#]+)/([^/?#]+)"); private GitHubIssueApiClient() { } @@ -49,7 +52,13 @@ public static URL createIssue(CloseableHttpClient client, String token, String o if (response.getStatusLine().getStatusCode() != HttpStatus.SC_CREATED) { throw new IOException("GitHub API error " + response.getStatusLine().getStatusCode() + ": " + responseBody); } - return new URL(new JSONObject(responseBody).getString("html_url")); + try { + return new URL(new JSONObject(responseBody).getString("html_url")); + } catch (JSONException e) { + // A 201 with an unexpected body is still a failed submission from the user's point + // of view; surface it on the IOException path the dialog already handles. + throw new IOException("GitHub API returned an unexpected response: " + responseBody, e); + } } } diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java index 9c10bda..6213465 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java @@ -74,23 +74,38 @@ public static String build(String baseUrl, String template, String title, Map + * Binary search on the cut point: the encoded length grows monotonically with the prefix + * length, so about {@code log2(n)} full encodes find the longest prefix that fits. The obvious + * loop (drop one char, re-encode everything) is quadratic, and it runs on the Swing thread + * when the user clicks the button - a 30k-character crash body froze the dialog for seconds. */ - private static String fitToBudget(String value, int maxBytes) { + static String fitToBudget(String value, int maxBytes) { String encoded = encode(value); if (encoded.length() <= maxBytes) { return encoded; } String suffix = encode(TRUNCATED_SUFFIX); - if (suffix.length() > maxBytes) { + int budgetForText = maxBytes - suffix.length(); + if (budgetForText <= 0) { return null; } - String truncated = value; - String withSuffix; - do { - truncated = truncated.substring(0, truncated.length() - 1); - withSuffix = encode(truncated) + suffix; - } while (!truncated.isEmpty() && withSuffix.length() > maxBytes); - return truncated.isEmpty() ? null : withSuffix; + int low = 0; + int high = value.length(); + while (low < high) { + int mid = (low + high + 1) >>> 1; + if (encode(value.substring(0, mid)).length() <= budgetForText) { + low = mid; + } else { + high = mid - 1; + } + } + // Never cut between a surrogate pair: URLEncoder would turn the lone high surrogate into + // "%3F" ('?'), which is not a split escape but is a mangled trailing character. + if (low > 0 && Character.isHighSurrogate(value.charAt(low - 1))) { + low--; + } + return low == 0 ? null : encode(value.substring(0, low)) + suffix; } private static String encode(String value) { diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubLoginDialog.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubLoginDialog.java index 09be023..4575d80 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubLoginDialog.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubLoginDialog.java @@ -19,10 +19,8 @@ import javax.swing.border.EmptyBorder; import java.awt.BorderLayout; import java.awt.CardLayout; -import java.awt.Desktop; import java.awt.Window; import java.io.IOException; -import java.net.URI; import java.net.URL; /** @@ -46,6 +44,7 @@ public class GitHubLoginDialog extends JDialog { private final String repo; private volatile String accessToken; + private volatile Thread loginThread; public GitHubLoginDialog(Window ownerWindow, String clientId, String owner, String repo, String title, String body) { @@ -122,16 +121,36 @@ private void startLogin() { SwingUtilities.invokeLater(() -> showCode(code)); accessToken = GitHubDeviceLogin.pollForAccessToken(client, clientId, code); SwingUtilities.invokeLater(() -> cards.show(content, "review")); - } catch (IOException | InterruptedException e) { + } catch (InterruptedException e) { + // Cancel: dispose() interrupted us. Nothing to show, the dialog is gone. + Thread.currentThread().interrupt(); + } catch (IOException | RuntimeException e) { + // RuntimeException too: a rate-limited or erroring endpoint used to surface as an + // uncaught exception that killed this thread and left the dialog on "Requesting...". SwingUtilities.invokeLater(() -> showFailure(e)); } }, "GitHubDeviceLogin"); thread.setDaemon(true); + loginThread = thread; thread.start(); } + /** + * Cancel stops the device-flow poll. Without this the daemon thread kept asking GitHub for a + * token until the code expired (typically fifteen minutes), and a user who finished the browser + * step after cancelling minted a live token into a dialog nobody could see. + */ + @Override + public void dispose() { + Thread thread = loginThread; + if (thread != null) { + thread.interrupt(); + } + super.dispose(); + } + private void showCode(GitHubDeviceLogin.DeviceCode code) { - openInBrowser(code.getVerificationUri()); + BrowserLauncher.open(code.getVerificationUri()); waitingLabel.setText("
" + I18N.getMessage("githubLoginWaiting", code.getVerificationUri(), code.getUserCode()) + "
"); } @@ -145,7 +164,7 @@ private void submitIssue() { try (CloseableHttpClient client = HttpClientBuilder.create().build()) { URL issueUrl = GitHubIssueApiClient.createIssue(client, accessToken, owner, repo, title, body); SwingUtilities.invokeLater(() -> showSuccess(issueUrl)); - } catch (IOException e) { + } catch (IOException | RuntimeException e) { SwingUtilities.invokeLater(() -> showFailure(e)); } }, "GitHubIssueSubmit"); @@ -156,21 +175,12 @@ private void submitIssue() { private void showSuccess(URL issueUrl) { resultLabel.setText("" + I18N.getMessage("githubLoginSuccess", issueUrl) + ""); cards.show(content, "result"); - openInBrowser(issueUrl.toString()); + BrowserLauncher.open(issueUrl.toString()); } private void showFailure(Exception e) { + e.printStackTrace(System.err); resultLabel.setText("" + I18N.getMessage("githubLoginFailed", e.getLocalizedMessage()) + ""); cards.show(content, "result"); } - - private static void openInBrowser(String url) { - if (Desktop.isDesktopSupported() && Desktop.getDesktop().isSupported(Desktop.Action.BROWSE)) { - try { - Desktop.getDesktop().browse(new URI(url)); - } catch (Exception e) { - e.printStackTrace(System.err); - } - } - } } diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java index afb982b..2938407 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java @@ -167,6 +167,26 @@ void bodyIncludesOnlyTheLastFiveLinesLoggedBeforeTheException() { "Expected the last 5 lines directly before the exception's header, got: " + body); } + @Test + void bodyKeepsTheRootCauseOfAChainedExceptionAfterAMoreFramesLine() { + // Logback and printStackTrace() both print "... N more" between a cause chain's links. A + // frame pattern that stops there drops every "Caused by:" after the first - the root cause. + String combinedLog = "=== Terasology-game.log ===\n" + + "java.lang.IllegalStateException: wrapper\n" + + "\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n" + + "\t... 12 more\n" + + "Caused by: java.io.IOException: disk full\n" + + "\tat java.base/java.io.FileOutputStream.write(FileOutputStream.java:100)\n" + + "\t... 3 common frames omitted\n"; + + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), combinedLog); + String body = summary.buildBody(null); + + assertTrue(body.contains("Caused by: java.io.IOException: disk full"), + "Expected the root cause after the '... N more' line, got: " + body); + assertTrue(body.contains("... 3 common frames omitted"), "Expected the trailing omitted-frames line, got: " + body); + } + @Test void bodyFallsBackToTheExceptionsOwnTraceWhenNotFoundInAnyTab() { RuntimeException primary = new RuntimeException("boom"); diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubDeviceLoginTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubDeviceLoginTest.java index 3a7a12f..9699234 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubDeviceLoginTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubDeviceLoginTest.java @@ -4,10 +4,13 @@ package org.terasology.crashreporter.pages; import org.apache.http.HttpEntity; +import org.apache.http.ProtocolVersion; +import org.apache.http.StatusLine; import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpUriRequest; import org.apache.http.entity.StringEntity; import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.message.BasicStatusLine; import org.junit.jupiter.api.Test; import java.io.IOException; @@ -15,6 +18,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -58,6 +62,37 @@ void requestDeviceCodeThrowsOnError() throws IOException { assertThrows(IOException.class, () -> GitHubDeviceLogin.requestDeviceCode(client, "client-id")); } + @Test + void requestDeviceCodeReportsAGarbageBodyAsAnIOExceptionNotANullPointer() throws IOException { + // A rate-limited endpoint answers with an HTML page; parseFormBody turns it into nonsense + // keys with no expires_in, and the old Integer.parseInt(null) killed the login thread. + CloseableHttpClient client = mockClientReturning("Too many requests"); + + IOException e = assertThrows(IOException.class, () -> GitHubDeviceLogin.requestDeviceCode(client, "client-id")); + assertTrue(e.getMessage().contains("device_code"), e.getMessage()); + } + + @Test + void requestDeviceCodeReportsANonNumericIntervalAsAnIOException() throws IOException { + CloseableHttpClient client = mockClientReturning( + "device_code=abc&user_code=WDJB-MJHT&verification_uri=https%3A%2F%2Fgithub.com%2Flogin%2Fdevice" + + "&expires_in=soon&interval=5"); + + assertThrows(IOException.class, () -> GitHubDeviceLogin.requestDeviceCode(client, "client-id")); + } + + @Test + void aFailedHttpStatusIsAnIOExceptionEvenWithAFormShapedBody() throws IOException { + CloseableHttpResponse response = response("error=server_error"); + StatusLine status = new BasicStatusLine(new ProtocolVersion("HTTP", 1, 1), 503, "Service Unavailable"); + when(response.getStatusLine()).thenReturn(status); + CloseableHttpClient client = mock(CloseableHttpClient.class); + when(client.execute(any(HttpUriRequest.class))).thenReturn(response); + + IOException e = assertThrows(IOException.class, () -> GitHubDeviceLogin.requestDeviceCode(client, "client-id")); + assertTrue(e.getMessage().contains("503"), e.getMessage()); + } + @Test void pollForAccessTokenReturnsTokenOnSuccess() throws IOException, InterruptedException { CloseableHttpClient client = mockClientReturning("access_token=tok123&token_type=bearer"); diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueApiClientTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueApiClientTest.java index 55abeaa..01eb25c 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueApiClientTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueApiClientTest.java @@ -32,6 +32,24 @@ void parseOwnerRepoExtractsBothParts() { GitHubIssueApiClient.parseOwnerRepo("https://github.com/MovingBlocks/Terasology/issues/new")); } + @Test + void parseOwnerRepoAcceptsABareRepoLinkWithoutATrailingPath() { + assertArrayEquals(new String[] {"MovingBlocks", "Terasology"}, + GitHubIssueApiClient.parseOwnerRepo("https://github.com/MovingBlocks/Terasology")); + assertArrayEquals(new String[] {"MovingBlocks", "Terasology"}, + GitHubIssueApiClient.parseOwnerRepo("https://github.com/MovingBlocks/Terasology?template=x.yml")); + } + + @Test + void createIssueReportsAMalformedSuccessBodyAsAnIOException() throws IOException { + CloseableHttpResponse resp = response(201, "unexpected"); + CloseableHttpClient client = mock(CloseableHttpClient.class); + when(client.execute(any(HttpUriRequest.class))).thenReturn(resp); + + assertThrows(IOException.class, + () -> GitHubIssueApiClient.createIssue(client, "token", "o", "r", "t", "b")); + } + @Test void parseOwnerRepoReturnsNullForNonGithubLink() { assertNull(GitHubIssueApiClient.parseOwnerRepo("https://example.com/issues/new")); diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java index 88d513b..8cf6fbc 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java @@ -106,6 +106,37 @@ void formBuildStaysUnderGitHubsUrlByteLimit() { assertTrue(link.contains("operating_system=Mac+OS+X"), link); } + @Test + void fitToBudgetKeepsTheLongestPrefixThatFits() { + // 10 'x' encode to 10 bytes; the suffix is fixed. The result must land exactly on the budget, + // not one short of it - the old drop-one-char loop and a binary search can disagree here. + String suffix = "%0A...+truncated%2C+see+the+full+log"; + int budget = 10 + suffix.length(); + + String fitted = GitHubIssueLinkBuilder.fitToBudget("x".repeat(100), budget); + + assertEquals("xxxxxxxxxx" + suffix, fitted); + assertEquals(budget, fitted.length()); + } + + @Test + void fitToBudgetReturnsNullWhenNotEvenTheSuffixFits() { + assertNull(GitHubIssueLinkBuilder.fitToBudget("x".repeat(100), 5)); + } + + @Test + void truncationNeverSplitsASurrogatePair() { + // U+1F600 is one code point but two Java chars; cutting between them yields a lone high + // surrogate, which URLEncoder renders as "%3F" ('?') - a mangled trailing character. + String emoji = new String(Character.toChars(0x1F600)); + String value = emoji.repeat(2_000); + + String fitted = GitHubIssueLinkBuilder.fitToBudget(value, 1_000); + + assertFalse(fitted.contains("%3F"), "lone surrogate leaked as '?': " + fitted); + assertTrue(fitted.length() <= 1_000, "over budget: " + fitted.length()); + } + @Test void truncationNeverSplitsAPercentEscape() { Map fields = new LinkedHashMap<>(); From 0d02048f60d92b5d70ae0f9d636f6c4745f644de Mon Sep 17 00:00:00 2001 From: Cervator Date: Wed, 30 Sep 2026 23:09:13 -0400 Subject: [PATCH 4/6] refactor(reporter): move Terasology log patterns and form field IDs out of cr-core into app properties cr-core is shared with Destination Sol, whose crash reports would have read "Terasology version: unknown" and "Active modules: none found in logs". `CrashSummary.Profile` now carries the product name, log regexes and issue-form field IDs from `crashreporter.properties`; cr-core's defaults declare none, so a bare core degrades to exceptions, OS and Java. Co-Authored-By: Claude Fable 5.1 --- .../crashreporter/GlobalProperties.java | 18 ++ .../crashreporter/pages/CrashSummary.java | 191 ++++++++++++++---- .../pages/FinalActionsPanel.java | 5 +- .../crashreporter/pages/CrashSummaryTest.java | 92 +++++++-- .../main/resources/crashreporter.properties | 3 + .../main/resources/crashreporter.properties | 16 ++ 6 files changed, 268 insertions(+), 57 deletions(-) diff --git a/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java b/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java index 11b6429..00942ed 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java @@ -22,6 +22,24 @@ public enum KEY { // GitHub OAuth App client ID with Device Flow enabled. Unset by default - see // GitHubDeviceLogin's javadoc. When unset, the "submit directly" button is hidden. REPORT_ISSUE_OAUTH_CLIENT_ID, + // Field IDs of the issue form named by REPORT_ISSUE_TEMPLATE, one per thing CrashSummary + // can pre-fill. Unset means that field is left for the user. Form-specific, so these live + // with the template in the downstream app's properties, never in cr-core's defaults. + REPORT_ISSUE_FIELD_VERSION, + REPORT_ISSUE_FIELD_OS, + REPORT_ISSUE_FIELD_JAVA, + REPORT_ISSUE_FIELD_DETAILS, + REPORT_ISSUE_FIELD_LOG, + REPORT_ISSUE_FIELD_EXTRA, + + // What CrashSummary knows about the hosting application's logs. The regexes each capture + // one group from the combined log text; unset means that line is not extracted or shown. + // Log formats are app-specific and not a published API, which is why they are configured + // by the app (cr-terasology, cr-destsol) rather than hardcoded in cr-core. + CRASH_SUMMARY_PRODUCT_NAME, + CRASH_SUMMARY_VERSION_PATTERN, + CRASH_SUMMARY_DISPLAY_VERSION_PATTERN, + CRASH_SUMMARY_MODULE_PATTERN, RES_BANNER_IMAGE, RES_SERVER_ICON, diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java index 155c405..e1fac11 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java @@ -3,15 +3,21 @@ package org.terasology.crashreporter.pages; +import org.terasology.crashreporter.GlobalProperties; +import org.terasology.crashreporter.GlobalProperties.KEY; + import java.io.PrintWriter; import java.io.StringWriter; import java.net.URL; import java.util.ArrayList; +import java.util.Collections; +import java.util.EnumMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.regex.PatternSyntaxException; /** * Builds a pre-filled GitHub issue title/body from a crash: the exception itself, plus every other @@ -27,12 +33,13 @@ * the real one and is used instead; the reconstructed exception's own trace is only a fallback for * when nothing better is available. *

- * The version/module regexes here mirror two fixed, narrow log lines the engine emits at startup - - * see {@code TerasologyEngine#logEnvironmentInfo} ({@code TerasologyVersion#toString}'s - * {@code [buildNumber=..., ..., engineVersion=X, displayVersion=Y]} format) and - * {@code RegisterMods} ({@code "Activating module: :"}, once per active module). - * Log formatting is not a published API and can drift; a change there degrades this to a blank - * "unknown"/empty-list extract rather than failing the report itself. + * What counts as a version line or a module line is the hosting application's business: cr-core + * is shared by Terasology and Destination Sol, whose logs look nothing alike. A {@link Profile}, + * built from the app's {@code crashreporter.properties}, carries those regexes plus the product + * name and the issue-form field IDs; with no profile configured the summary degrades to the parts + * every JVM app has (exceptions, OS, Java) rather than mislabelling another game's crash as a + * Terasology one. Log formatting is not a published API and can drift; a pattern that stops + * matching degrades to "unknown"/an empty list rather than failing the report itself. */ public final class CrashSummary { @@ -43,9 +50,6 @@ public final class CrashSummary { private static final int CONTEXT_LINES_BEFORE = 5; private static final String NO_TAB_LABEL = "this crash"; - private static final Pattern ENGINE_VERSION_PATTERN = Pattern.compile("engineVersion=([^,\\]]*)"); - private static final Pattern DISPLAY_VERSION_PATTERN = Pattern.compile("displayVersion=([^,\\]]*)"); - private static final Pattern ACTIVE_MODULE_PATTERN = Pattern.compile("Activating module: (\\S+:\\S+)"); private static final Pattern LOG_TAB_PATTERN = Pattern.compile("(?m)^=== (.*) ===$"); // A log-formatted stack trace: a "some.FullyQualified.NameException[: message]" header line // immediately followed by one or more "at ..."/"Caused by: ..." frame lines - the shape every @@ -57,6 +61,7 @@ public final class CrashSummary { "(?m)^([\\w$]+(?:\\.[\\w$]+)+(?:Exception|Error))(:[^\\n]*)?\\n" + "((?:[ \\t]*(?:at |Caused by:|\\.\\.\\. \\d+ (?:more|common frames omitted))[^\\n]*\\n?)+)"); + private final Profile profile; private final Throwable exception; private final List exceptionBlocks; private final int moreExceptionsCount; @@ -64,8 +69,9 @@ public final class CrashSummary { private final String displayVersion; private final List activeModules; - private CrashSummary(Throwable exception, List exceptionBlocks, int moreExceptionsCount, + private CrashSummary(Profile profile, Throwable exception, List exceptionBlocks, int moreExceptionsCount, String engineVersion, String displayVersion, List activeModules) { + this.profile = profile; this.exception = exception; this.exceptionBlocks = exceptionBlocks; this.moreExceptionsCount = moreExceptionsCount; @@ -74,7 +80,12 @@ private CrashSummary(Throwable exception, List exceptionBlocks, int more this.activeModules = activeModules; } + /** Extracts with {@link Profile#GENERIC}: exceptions, OS and Java only. */ public static CrashSummary extract(Throwable exception, String combinedLogText) { + return extract(exception, combinedLogText, Profile.GENERIC); + } + + public static CrashSummary extract(Throwable exception, String combinedLogText, Profile profile) { String text = combinedLogText != null ? combinedLogText : ""; List blocks = buildExceptionBlocks(exception, text); @@ -84,9 +95,9 @@ public static CrashSummary extract(Throwable exception, String combinedLogText) blocks = new ArrayList<>(blocks.subList(0, MAX_EXCEPTIONS_LISTED)); } - return new CrashSummary(exception, blocks, moreCount, - firstGroup(ENGINE_VERSION_PATTERN, text), firstGroup(DISPLAY_VERSION_PATTERN, text), - extractActiveModules(text)); + return new CrashSummary(profile, exception, blocks, moreCount, + firstGroup(profile.versionPattern, text), firstGroup(profile.displayVersionPattern, text), + extractActiveModules(profile.modulePattern, text)); } /** @@ -256,13 +267,19 @@ private static String precedingLines(String text, int beforeIndex, int maxLines) } private static String firstGroup(Pattern pattern, String text) { + if (pattern == null) { + return ""; + } Matcher matcher = pattern.matcher(text); return matcher.find() ? matcher.group(1).trim() : ""; } - private static List extractActiveModules(String text) { + private static List extractActiveModules(Pattern pattern, String text) { List modules = new ArrayList<>(); - Matcher matcher = ACTIVE_MODULE_PATTERN.matcher(text); + if (pattern == null) { + return modules; + } + Matcher matcher = pattern.matcher(text); while (matcher.find()) { String module = matcher.group(1); if (!modules.contains(module)) { @@ -305,24 +322,29 @@ public String buildBody(URL pastebinLink) { } body.append("### Environment\n\n"); - body.append("- Terasology version: ").append(engineVersion.isEmpty() ? "unknown" : engineVersion); - if (!displayVersion.isEmpty()) { - body.append(" (").append(displayVersion).append(')'); + if (profile.extractsVersion()) { + body.append("- ").append(profile.versionLabel()).append(": ").append(engineVersion.isEmpty() ? "unknown" : engineVersion); + if (!displayVersion.isEmpty()) { + body.append(" (").append(displayVersion).append(')'); + } + body.append('\n'); } - body.append('\n'); body.append("- OS: ").append(System.getProperty("os.name")).append(' ') .append(System.getProperty("os.version")).append(" (").append(System.getProperty("os.arch")).append(")\n"); - body.append("- Active modules:"); - if (activeModules.isEmpty()) { - body.append(" none found in logs\n"); - } else { - body.append('\n'); - int shown = Math.min(activeModules.size(), MAX_MODULES_LISTED); - for (int i = 0; i < shown; i++) { - body.append(" - ").append(activeModules.get(i)).append('\n'); - } - if (activeModules.size() > shown) { - body.append(" - ... ").append(activeModules.size() - shown).append(" more\n"); + body.append("- Java: ").append(System.getProperty("java.version")).append('\n'); + if (profile.extractsModules()) { + body.append("- Active modules:"); + if (activeModules.isEmpty()) { + body.append(" none found in logs\n"); + } else { + body.append('\n'); + int shown = Math.min(activeModules.size(), MAX_MODULES_LISTED); + for (int i = 0; i < shown; i++) { + body.append(" - ").append(activeModules.get(i)).append('\n'); + } + if (activeModules.size() > shown) { + body.append(" - ... ").append(activeModules.size() - shown).append(" more\n"); + } } } @@ -337,9 +359,9 @@ public String buildBody(URL pastebinLink) { * @return field ID to value for the issue form named by * {@link org.terasology.crashreporter.GlobalProperties.KEY#REPORT_ISSUE_TEMPLATE} - only * meaningful when a downstream app has configured one (see - * {@link GitHubIssueLinkBuilder#build(String, String, String, Map)}); the IDs here match - * Terasology's own {@code crash-bug-report.yml}. Fields with nothing extractable are - * omitted so the user's own blank field is left for them to fill in, rather than being + * {@link GitHubIssueLinkBuilder#build(String, String, String, Map)}). The IDs come from + * the {@link Profile}; a part with no configured ID, or nothing extractable, is omitted + * so the user's own blank field is left for them to fill in, rather than being * pre-filled with something misleading like "unknown". */ public Map buildIssueFormFields(URL pastebinLink) { @@ -347,11 +369,11 @@ public Map buildIssueFormFields(URL pastebinLink) { if (!engineVersion.isEmpty()) { String version = displayVersion.isEmpty() ? engineVersion : engineVersion + " (" + displayVersion + ")"; - fields.put("terasology_version", version); + putField(fields, Profile.FormField.VERSION, version); } - fields.put("operating_system", System.getProperty("os.name") + " " + System.getProperty("os.version") + putField(fields, Profile.FormField.OS, System.getProperty("os.name") + " " + System.getProperty("os.version") + " (" + System.getProperty("os.arch") + ")"); - fields.put("java_version", System.getProperty("java.version")); + putField(fields, Profile.FormField.JAVA, System.getProperty("java.version")); StringBuilder actual = new StringBuilder(); for (String block : exceptionBlocks) { @@ -360,10 +382,10 @@ public Map buildIssueFormFields(URL pastebinLink) { if (moreExceptionsCount > 0) { actual.append("... ").append(moreExceptionsCount).append(" more - see the full log\n"); } - fields.put("actual_behavior", actual.toString().trim()); + putField(fields, Profile.FormField.DETAILS, actual.toString().trim()); if (pastebinLink != null) { - fields.put("log_details", "[PasteBin](" + pastebinLink + ")"); + putField(fields, Profile.FormField.LOG, "[PasteBin](" + pastebinLink + ")"); } if (!activeModules.isEmpty()) { @@ -375,9 +397,100 @@ public Map buildIssueFormFields(URL pastebinLink) { if (activeModules.size() > shown) { modules.append("- ... ").append(activeModules.size() - shown).append(" more\n"); } - fields.put("additional_context", modules.toString().trim()); + putField(fields, Profile.FormField.EXTRA, modules.toString().trim()); } return fields; } + + private void putField(Map fields, Profile.FormField field, String value) { + String id = profile.fieldId(field); + if (id != null) { + fields.put(id, value); + } + } + + /** + * What the hosting application tells CrashSummary about its logs and its issue form. Built + * from {@link GlobalProperties} by {@link #from}; cr-core's own defaults configure none of it. + */ + public static final class Profile { + + /** Which of the summary's parts a configured issue-form field ID receives. */ + public enum FormField { VERSION, OS, JAVA, DETAILS, LOG, EXTRA } + + /** No product name, no log patterns, no form fields: exceptions, OS and Java only. */ + public static final Profile GENERIC = new Profile(null, null, null, null, Collections.emptyMap()); + + private final String productName; + private final Pattern versionPattern; + private final Pattern displayVersionPattern; + private final Pattern modulePattern; + private final Map formFieldIds; + + /** + * @param productName shown as "{productName} version" in the body; null for a plain "Version" + * @param versionRegex regex whose group 1 is the version, or null to extract none + * @param displayVersionRegex regex whose group 1 is a display name for the version, or null + * @param moduleRegex regex whose group 1 is one active module, matched repeatedly, or null + * @param formFieldIds issue-form field ID per part; parts without an ID are not pre-filled + */ + public Profile(String productName, String versionRegex, String displayVersionRegex, String moduleRegex, + Map formFieldIds) { + this.productName = productName; + this.versionPattern = compileOrNull(versionRegex, KEY.CRASH_SUMMARY_VERSION_PATTERN); + this.displayVersionPattern = compileOrNull(displayVersionRegex, KEY.CRASH_SUMMARY_DISPLAY_VERSION_PATTERN); + this.modulePattern = compileOrNull(moduleRegex, KEY.CRASH_SUMMARY_MODULE_PATTERN); + this.formFieldIds = new EnumMap<>(FormField.class); + for (Map.Entry entry : formFieldIds.entrySet()) { + if (entry.getValue() != null && !entry.getValue().isEmpty()) { + this.formFieldIds.put(entry.getKey(), entry.getValue()); + } + } + } + + public static Profile from(GlobalProperties properties) { + Map fields = new EnumMap<>(FormField.class); + fields.put(FormField.VERSION, properties.get(KEY.REPORT_ISSUE_FIELD_VERSION)); + fields.put(FormField.OS, properties.get(KEY.REPORT_ISSUE_FIELD_OS)); + fields.put(FormField.JAVA, properties.get(KEY.REPORT_ISSUE_FIELD_JAVA)); + fields.put(FormField.DETAILS, properties.get(KEY.REPORT_ISSUE_FIELD_DETAILS)); + fields.put(FormField.LOG, properties.get(KEY.REPORT_ISSUE_FIELD_LOG)); + fields.put(FormField.EXTRA, properties.get(KEY.REPORT_ISSUE_FIELD_EXTRA)); + return new Profile(properties.get(KEY.CRASH_SUMMARY_PRODUCT_NAME), + properties.get(KEY.CRASH_SUMMARY_VERSION_PATTERN), + properties.get(KEY.CRASH_SUMMARY_DISPLAY_VERSION_PATTERN), + properties.get(KEY.CRASH_SUMMARY_MODULE_PATTERN), fields); + } + + // A typo in a downstream app's regex must not take the whole crash dialog down with it; + // it degrades to "not extracted", and says so once on stderr. + private static Pattern compileOrNull(String regex, KEY key) { + if (regex == null || regex.isEmpty()) { + return null; + } + try { + return Pattern.compile(regex); + } catch (PatternSyntaxException e) { + System.err.println("Ignoring invalid " + key + " regex: " + e.getMessage()); + return null; + } + } + + String versionLabel() { + return productName != null && !productName.isEmpty() ? productName + " version" : "Version"; + } + + boolean extractsVersion() { + return versionPattern != null; + } + + boolean extractsModules() { + return modulePattern != null; + } + + String fieldId(FormField field) { + return formFieldIds.get(field); + } + } } diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/FinalActionsPanel.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/FinalActionsPanel.java index 438071c..68abe41 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/FinalActionsPanel.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/FinalActionsPanel.java @@ -55,6 +55,7 @@ public FinalActionsPanel(GlobalProperties properties, Throwable exception, Suppl this.exception = exception; this.logTextSupplier = logTextSupplier; this.uploadedFile = uploadedFile; + final CrashSummary.Profile summaryProfile = CrashSummary.Profile.from(properties); setLayout(new BorderLayout(0, 10)); setBorder(new EmptyBorder(0, 10, 10, 10)); @@ -94,7 +95,7 @@ public void actionPerformed(ActionEvent e) { @Override public void actionPerformed(ActionEvent e) { - CrashSummary summary = CrashSummary.extract(exception, logTextSupplier.get()); + CrashSummary summary = CrashSummary.extract(exception, logTextSupplier.get(), summaryProfile); String baseUrl = properties.get(KEY.REPORT_ISSUE_LINK); String template = properties.get(KEY.REPORT_ISSUE_TEMPLATE); String link; @@ -122,7 +123,7 @@ public void actionPerformed(ActionEvent e) { submitDirectlyButton.setFont(buttonFont); submitDirectlyButton.setIcon(Resources.loadIcon(properties.get(KEY.RES_GITHUB_ICON))); submitDirectlyButton.addActionListener(e -> { - CrashSummary summary = CrashSummary.extract(exception, logTextSupplier.get()); + CrashSummary summary = CrashSummary.extract(exception, logTextSupplier.get(), summaryProfile); Window window = SwingUtilities.getWindowAncestor(this); new GitHubLoginDialog(window, oauthClientId, ownerRepo[0], ownerRepo[1], summary.buildTitle(), summary.buildBody(uploadedFile.get())).setVisible(true); diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java index 2938407..1208ee1 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java @@ -7,6 +7,7 @@ import java.net.MalformedURLException; import java.net.URL; +import java.util.EnumMap; import java.util.Map; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -29,18 +30,77 @@ class CrashSummaryTest { + "10:00:01.010 [main] INFO o.t.e.core.modes.loadProcesses.RegisterMods - Activating module: CoreAssets:2.4.0\n" + "10:00:01.020 [main] INFO o.t.e.core.modes.loadProcesses.RegisterMods - Activating module: CoreAssets:2.4.0\n"; + /** + * The same patterns and field IDs cr-terasology's crashreporter.properties declares - kept in + * step by hand, since cr-core's tests cannot see that module's resources. + */ + private static CrashSummary.Profile terasology() { + Map fields = new EnumMap<>(CrashSummary.Profile.FormField.class); + fields.put(CrashSummary.Profile.FormField.VERSION, "terasology_version"); + fields.put(CrashSummary.Profile.FormField.OS, "operating_system"); + fields.put(CrashSummary.Profile.FormField.JAVA, "java_version"); + fields.put(CrashSummary.Profile.FormField.DETAILS, "actual_behavior"); + fields.put(CrashSummary.Profile.FormField.LOG, "log_details"); + fields.put(CrashSummary.Profile.FormField.EXTRA, "additional_context"); + return new CrashSummary.Profile("Terasology", "engineVersion=([^,\\]]*)", "displayVersion=([^,\\]]*)", + "Activating module: (\\S+:\\S+)", fields); + } + @Test - void extractsEngineAndDisplayVersion() { + void genericProfileMentionsNoProductAndPreFillsNoFormFields() { + // cr-core alone, or an app like cr-destsol with no log patterns configured: the body must + // not claim "Terasology version: unknown" or "Active modules: none found in logs". CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); String body = summary.buildBody(null); + assertFalse(body.contains("Terasology"), "Expected no product name under the generic profile, got: " + body); + assertFalse(body.contains("version:"), "Expected no version line without a version pattern, got: " + body); + assertFalse(body.contains("Active modules"), "Expected no module list without a module pattern, got: " + body); + assertTrue(body.contains("- OS: ") && body.contains("- Java: "), "Expected OS and Java lines, got: " + body); + assertTrue(summary.buildIssueFormFields(null).isEmpty(), "Expected no form fields without configured IDs"); + } + + @Test + void profileFromCoreDefaultsIsGeneric() { + // cr-core's own crashreporter_defaults.properties declares none of the CRASH_SUMMARY_* or + // REPORT_ISSUE_FIELD_* keys, so a standalone cr-core gets exactly the generic behaviour. + CrashSummary.Profile profile = CrashSummary.Profile.from(new org.terasology.crashreporter.GlobalProperties()); + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT, profile); + + assertFalse(summary.buildBody(null).contains("version:")); + assertTrue(summary.buildIssueFormFields(null).isEmpty()); + } + + @Test + void productNameLabelsTheVersionLine() { + CrashSummary.Profile destSol = new CrashSummary.Profile("Destination Sol", "version=(\\S+)", null, null, + new EnumMap<>(CrashSummary.Profile.FormField.class)); + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), "startup version=2.1.0 ok", destSol); + + assertTrue(summary.buildBody(null).contains("- Destination Sol version: 2.1.0"), summary.buildBody(null)); + } + + @Test + void anInvalidConfiguredRegexDegradesToNotExtractedInsteadOfThrowing() { + CrashSummary.Profile broken = new CrashSummary.Profile("X", "version=(unclosed", null, null, + new EnumMap<>(CrashSummary.Profile.FormField.class)); + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT, broken); + + assertFalse(summary.buildBody(null).contains("version:")); + } + + @Test + void extractsEngineAndDisplayVersion() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT, terasology()); + String body = summary.buildBody(null); + assertTrue(body.contains("5.4.0-SNAPSHOT"), "Expected the engine version in the body, got: " + body); assertTrue(body.contains("Aeternum"), "Expected the display version in the body, got: " + body); } @Test void extractsActiveModulesAndDeduplicates() { - CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT, terasology()); String body = summary.buildBody(null); assertTrue(body.contains("engine:5.4.0-SNAPSHOT"), "Expected engine module, got: " + body); @@ -51,7 +111,7 @@ void extractsActiveModulesAndDeduplicates() { @Test void missingVersionAndModulesDegradeGracefullyInsteadOfFailing() { - CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), "no relevant lines here"); + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), "no relevant lines here", terasology()); String body = summary.buildBody(null); assertTrue(body.contains("unknown"), "Expected a fallback for a missing version, got: " + body); @@ -60,14 +120,14 @@ void missingVersionAndModulesDegradeGracefullyInsteadOfFailing() { @Test void titleUsesExceptionClassAndMessage() { - CrashSummary summary = CrashSummary.extract(new IllegalStateException("world was null"), LOG_TEXT); + CrashSummary summary = CrashSummary.extract(new IllegalStateException("world was null"), LOG_TEXT, terasology()); assertEquals("Crash: IllegalStateException: world was null", summary.buildTitle()); } @Test void bodyIncludesTheExceptionExtract() { - CrashSummary summary = CrashSummary.extract(new RuntimeException("kaboom"), LOG_TEXT); + CrashSummary summary = CrashSummary.extract(new RuntimeException("kaboom"), LOG_TEXT, terasology()); String body = summary.buildBody(null); assertTrue(body.contains("kaboom"), "Expected the exception message in the body, got: " + body); @@ -76,7 +136,7 @@ void bodyIncludesTheExceptionExtract() { @Test void bodyIncludesThePastebinLinkWhenUploaded() throws MalformedURLException { - CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT, terasology()); URL link = new URL("https://pastebin.com/abc123"); String body = summary.buildBody(link); @@ -86,7 +146,7 @@ void bodyIncludesThePastebinLinkWhenUploaded() throws MalformedURLException { @Test void bodyNotesWhenUploadWasSkipped() { - CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT, terasology()); String body = summary.buildBody(null); @@ -109,7 +169,7 @@ void bodyListsExceptionsFoundInOtherLogTabsWithTheirFullTraceNamingTheTab() { + "java.lang.NullPointerException: world was null\n" + "\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n"; - CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), combinedLog); + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), combinedLog, terasology()); String body = summary.buildBody(null); assertTrue(body.contains("### Exceptions"), "Expected a single unified exceptions section, got: " + body); @@ -136,7 +196,7 @@ void bodyAttributesThePrimaryExceptionToItsOwnTabInsteadOfListingItTwice() { + primary + "\n" + "\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n"; - CrashSummary summary = CrashSummary.extract(primary, combinedLog); + CrashSummary summary = CrashSummary.extract(primary, combinedLog, terasology()); String body = summary.buildBody(null); assertTrue(body.contains("**Terasology-game.log**\n\n```\njava.lang.RuntimeException: boom\n" @@ -157,7 +217,7 @@ void bodyIncludesOnlyTheLastFiveLinesLoggedBeforeTheException() { combinedLog.append("java.lang.NullPointerException: world was null\n") .append("\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n"); - CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), combinedLog.toString()); + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), combinedLog.toString(), terasology()); String body = summary.buildBody(null); assertFalse(body.contains("log line 1\n") || body.contains("log line 2\n") || body.contains("log line 3\n"), @@ -179,7 +239,7 @@ void bodyKeepsTheRootCauseOfAChainedExceptionAfterAMoreFramesLine() { + "\tat java.base/java.io.FileOutputStream.write(FileOutputStream.java:100)\n" + "\t... 3 common frames omitted\n"; - CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), combinedLog); + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), combinedLog, terasology()); String body = summary.buildBody(null); assertTrue(body.contains("Caused by: java.io.IOException: disk full"), @@ -204,7 +264,7 @@ void bodyFallsBackToTheExceptionsOwnTraceWhenNotFoundInAnyTab() { // REPORT_ISSUE_TEMPLATE, landing the summary in that issue form's own fields. @Test void issueFormFieldsIncludeVersionOsAndTheExceptionBlocks() { - CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT, terasology()); Map fields = summary.buildIssueFormFields(null); assertEquals("5.4.0-SNAPSHOT (Aeternum)", fields.get("terasology_version")); @@ -215,7 +275,7 @@ void issueFormFieldsIncludeVersionOsAndTheExceptionBlocks() { @Test void issueFormFieldsOmitVersionWhenNotFoundInsteadOfSayingUnknown() { - CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), "no relevant lines here"); + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), "no relevant lines here", terasology()); Map fields = summary.buildIssueFormFields(null); // Unlike buildBody()'s "unknown" fallback, an omitted field is left blank in the actual issue @@ -226,7 +286,7 @@ void issueFormFieldsOmitVersionWhenNotFoundInsteadOfSayingUnknown() { @Test void issueFormFieldsOmitLogDetailsWhenUploadWasSkipped() { - CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT, terasology()); Map fields = summary.buildIssueFormFields(null); assertNull(fields.get("log_details"), "Expected no log_details entry when nothing was uploaded, got: " + fields); @@ -234,7 +294,7 @@ void issueFormFieldsOmitLogDetailsWhenUploadWasSkipped() { @Test void issueFormFieldsIncludeThePastebinLinkWhenUploaded() throws MalformedURLException { - CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT, terasology()); URL link = new URL("https://pastebin.com/abc123"); Map fields = summary.buildIssueFormFields(link); @@ -244,7 +304,7 @@ void issueFormFieldsIncludeThePastebinLinkWhenUploaded() throws MalformedURLExce @Test void issueFormFieldsIncludeActiveModulesUnderAdditionalContext() { - CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT, terasology()); Map fields = summary.buildIssueFormFields(null); assertTrue(fields.get("additional_context").contains("engine:5.4.0-SNAPSHOT"), fields.get("additional_context")); diff --git a/cr-destsol/src/main/resources/crashreporter.properties b/cr-destsol/src/main/resources/crashreporter.properties index d4ea105..e63f010 100644 --- a/cr-destsol/src/main/resources/crashreporter.properties +++ b/cr-destsol/src/main/resources/crashreporter.properties @@ -2,5 +2,8 @@ SUPPORT_FORUM_LINK=http://forum.terasology.org/forum/destination-sol.57/ REPORT_ISSUE_LINK=https://github.com/MovingBlocks/DestinationSol/issues/new JOIN_IRC_LINK=https://webchat.freenode.net/?channels=destination-sol +# No version/module log patterns yet: the pre-filled issue carries exceptions, OS and Java only. +CRASH_SUMMARY_PRODUCT_NAME=Destination Sol + RES_BANNER_IMAGE=icons/banner_destsol.jpg RES_SERVER_ICON=icons/DestSol_icon.png diff --git a/cr-terasology/src/main/resources/crashreporter.properties b/cr-terasology/src/main/resources/crashreporter.properties index baaf910..ad67d36 100644 --- a/cr-terasology/src/main/resources/crashreporter.properties +++ b/cr-terasology/src/main/resources/crashreporter.properties @@ -4,5 +4,21 @@ REPORT_ISSUE_TEMPLATE=crash-bug-report.yml REPORT_ISSUE_OAUTH_CLIENT_ID=Ov23liSKtBll7VXE8Vur JOIN_DISCORD_LINK=https://discord.gg/terasology +# Field IDs in Terasology's .github/ISSUE_TEMPLATE/crash-bug-report.yml (MovingBlocks/Terasology#5390). +REPORT_ISSUE_FIELD_VERSION=terasology_version +REPORT_ISSUE_FIELD_OS=operating_system +REPORT_ISSUE_FIELD_JAVA=java_version +REPORT_ISSUE_FIELD_DETAILS=actual_behavior +REPORT_ISSUE_FIELD_LOG=log_details +REPORT_ISSUE_FIELD_EXTRA=additional_context + +# Two fixed lines the engine logs at startup: TerasologyVersion#toString's +# "[buildNumber=..., displayVersion=X, engineVersion=Y]" block, and RegisterMods' +# "Activating module: :" once per active module. Properties-file escaping: \\ is one backslash. +CRASH_SUMMARY_PRODUCT_NAME=Terasology +CRASH_SUMMARY_VERSION_PATTERN=engineVersion=([^,\\]]*) +CRASH_SUMMARY_DISPLAY_VERSION_PATTERN=displayVersion=([^,\\]]*) +CRASH_SUMMARY_MODULE_PATTERN=Activating module: (\\S+:\\S+) + RES_BANNER_IMAGE=icons/banner.jpg RES_SERVER_ICON=icons/server.png From bbbf7c6261886534361873721f7690cbceca4d75 Mon Sep 17 00:00:00 2001 From: Cervator Date: Thu, 1 Oct 2026 22:29:04 -0400 Subject: [PATCH 5/6] =?UTF-8?q?fix(reporter):=20PR=20#58=20review=20round?= =?UTF-8?q?=201=20=E2=80=94=20keep=20the=20log=20link=20under=20truncation?= =?UTF-8?q?,=20dispose=20on=20close,=20stop=20on=20malformed=20token=20rep?= =?UTF-8?q?lies?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit and Copilot, all ten accepted. Small form fields are reserved before a trace is fitted, and the body leads with the log link, so truncation can no longer drop the one pointer the suffix refers to. The dialog disposes on window close and ignores callbacks after it; submission has its own failure message. `Suppressed:` and same-header failures are kept; a regex without a capture group is rejected up front. Co-Authored-By: Claude Fable 5.1 --- .../crashreporter/pages/CrashSummary.java | 42 ++++++++--- .../pages/GitHubDeviceLogin.java | 3 + .../pages/GitHubIssueLinkBuilder.java | 34 +++++++-- .../pages/GitHubLoginDialog.java | 37 +++++++--- .../resources/i18n/MessagesBundle.properties | 1 + .../crashreporter/pages/CrashSummaryTest.java | 69 +++++++++++++++++++ .../pages/GitHubDeviceLoginTest.java | 9 +++ .../pages/GitHubIssueLinkBuilderTest.java | 34 +++++++-- 8 files changed, 196 insertions(+), 33 deletions(-) diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java index e1fac11..c824ca4 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java @@ -55,11 +55,12 @@ public final class CrashSummary { // immediately followed by one or more "at ..."/"Caused by: ..." frame lines - the shape every // JVM logging framework prints a Throwable in (Logback's %ex, java.util.logging, a raw // printStackTrace()), regardless of which class emits it. "... N more" / "... N common frames - // omitted" lines count as frames too: they sit between a cause chain's links, and a pattern that - // stopped at them dropped every "Caused by:" after the first - which is the root cause. + // omitted" lines count as frames too, as do "Suppressed:" sections: both sit between a cause + // chain's links, and a pattern that stopped at them dropped every "Caused by:" after the first - + // which is the root cause. private static final Pattern STACK_TRACE_HEADER_PATTERN = Pattern.compile( "(?m)^([\\w$]+(?:\\.[\\w$]+)+(?:Exception|Error))(:[^\\n]*)?\\n" - + "((?:[ \\t]*(?:at |Caused by:|\\.\\.\\. \\d+ (?:more|common frames omitted))[^\\n]*\\n?)+)"); + + "((?:[ \\t]*(?:at |Caused by:|Suppressed:|\\.\\.\\. \\d+ (?:more|common frames omitted))[^\\n]*\\n?)+)"); private final Profile profile; private final Throwable exception; @@ -128,7 +129,10 @@ private static List buildExceptionBlocks(Throwable exception, String com List blocks = new ArrayList<>(); blocks.add(formatBlock(primary)); for (ExceptionEntry entry : found) { - if (entry.header.equals(primaryHeader)) { + // Skip the one entry chosen as primary, not every entry sharing its header: two distinct + // failures can log the same type and message with different traces, and the second + // one is exactly what "list every exception" is for. Exact duplicates fall out below. + if (entry == primary) { continue; } String block = formatBlock(entry); @@ -297,9 +301,15 @@ public String buildTitle() { StringBuilder title = new StringBuilder("Crash: ").append(exception.getClass().getSimpleName()); String message = exception.getLocalizedMessage(); if (message != null && !message.trim().isEmpty()) { - String trimmed = message.length() > MAX_TITLE_MESSAGE_LENGTH - ? message.substring(0, MAX_TITLE_MESSAGE_LENGTH - 3) + "..." - : message; + String trimmed = message; + if (message.length() > MAX_TITLE_MESSAGE_LENGTH) { + int cut = MAX_TITLE_MESSAGE_LENGTH - 3; + // Never split a surrogate pair: a lone high surrogate renders as a mangled character. + if (Character.isHighSurrogate(message.charAt(cut - 1))) { + cut--; + } + trimmed = message.substring(0, cut) + "..."; + } title.append(": ").append(trimmed); } return title.toString(); @@ -313,6 +323,12 @@ public String buildTitle() { public String buildBody(URL pastebinLink) { StringBuilder body = new StringBuilder(); + // The link goes first: when this body is squeezed into a URL, GitHubIssueLinkBuilder + // truncates the tail, and the one line that must survive a truncated trace is the pointer + // to the full log. + body.append("### Full logs\n\n"); + body.append(pastebinLink != null ? "[PasteBin](" + pastebinLink + ")\n\n" : "(not uploaded)\n\n"); + body.append("### Exceptions\n\n"); for (String block : exceptionBlocks) { body.append(block).append("\n\n"); @@ -348,9 +364,6 @@ public String buildBody(URL pastebinLink) { } } - body.append("\n### Full logs\n\n"); - body.append(pastebinLink != null ? "[PasteBin](" + pastebinLink + ")\n" : "(not uploaded)\n"); - return body.toString(); } @@ -470,7 +483,14 @@ private static Pattern compileOrNull(String regex, KEY key) { return null; } try { - return Pattern.compile(regex); + Pattern pattern = Pattern.compile(regex); + // firstGroup/extractActiveModules read group(1); a regex without one compiles fine + // and then throws IndexOutOfBoundsException on the first match. + if (pattern.matcher("").groupCount() < 1) { + System.err.println("Ignoring " + key + " regex without a capture group: " + regex); + return null; + } + return pattern; } catch (PatternSyntaxException e) { System.err.println("Ignoring invalid " + key + " regex: " + e.getMessage()); return null; diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubDeviceLogin.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubDeviceLogin.java index 8b70557..19641bb 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubDeviceLogin.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubDeviceLogin.java @@ -86,6 +86,9 @@ public static String pollForAccessToken(CloseableHttpClient client, String clien continue; } failOnError(fields); + // Neither a token nor an error: not a response this flow defines. Polling on would + // leave the user staring at the dialog until the device code expires. + throw new IOException("GitHub's token response had neither access_token nor error: " + fields); } throw new IOException("Device code expired"); } diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java index 6213465..71790c6 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java @@ -5,6 +5,7 @@ import java.io.UnsupportedEncodingException; import java.net.URLEncoder; +import java.util.LinkedHashMap; import java.util.Map; /** @@ -39,10 +40,15 @@ public static String build(String baseUrl, String title, String body) { return baseUrl + separator + query; } + // A field whose encoded value is at most this many bytes is reserved before any large field is + // fitted. Versions, OS, Java and the full-log link are all a line; the exception text is not. + private static final int SMALL_FIELD_BYTES = 512; + /** * @param template issue form filename under {@code .github/ISSUE_TEMPLATE/} - * @param fields field ID to value. Null/empty value: field omitted. Too long: truncated or - * dropped, whichever fits. + * @param fields field ID to value, in output order. Null/empty value: field omitted. Small + * fields are reserved first, so an oversized trace earlier in the map cannot crowd out + * the full-log link after it; large fields are then truncated or dropped, whichever fits. */ public static String build(String baseUrl, String template, String title, Map fields) { if (baseUrl == null) { @@ -53,19 +59,35 @@ public static String build(String baseUrl, String template, String title, Map showFailure(e)); + SwingUtilities.invokeLater(() -> showFailure("githubLoginFailed", e)); } }, "GitHubDeviceLogin"); thread.setDaemon(true); @@ -136,17 +139,23 @@ private void startLogin() { } /** - * Cancel stops the device-flow poll. Without this the daemon thread kept asking GitHub for a - * token until the code expired (typically fifteen minutes), and a user who finished the browser - * step after cancelling minted a live token into a dialog nobody could see. + * Cancel stops the device-flow poll and abandons an in-flight submission. Without this the + * daemon thread kept asking GitHub for a token until the code expired (typically fifteen + * minutes), and a user who finished the browser step after cancelling minted a live token into + * a dialog nobody could see. An interrupted POST may still reach GitHub, which is why the + * success and failure callbacks also check that the dialog is still displayable before acting. */ @Override public void dispose() { - Thread thread = loginThread; + interrupt(loginThread); + interrupt(submitThread); + super.dispose(); + } + + private static void interrupt(Thread thread) { if (thread != null) { thread.interrupt(); } - super.dispose(); } private void showCode(GitHubDeviceLogin.DeviceCode code) { @@ -165,22 +174,32 @@ private void submitIssue() { URL issueUrl = GitHubIssueApiClient.createIssue(client, accessToken, owner, repo, title, body); SwingUtilities.invokeLater(() -> showSuccess(issueUrl)); } catch (IOException | RuntimeException e) { - SwingUtilities.invokeLater(() -> showFailure(e)); + SwingUtilities.invokeLater(() -> showFailure("githubSubmitFailed", e)); } }, "GitHubIssueSubmit"); thread.setDaemon(true); + submitThread = thread; thread.start(); } private void showSuccess(URL issueUrl) { + if (!isDisplayable()) { + // Cancelled while submitting, but the POST had already gone out. Don't pop a browser + // from a dialog the user closed; the issue exists under their account regardless. + System.err.println("Issue created after the dialog was closed: " + issueUrl); + return; + } resultLabel.setText("" + I18N.getMessage("githubLoginSuccess", issueUrl) + ""); cards.show(content, "result"); BrowserLauncher.open(issueUrl.toString()); } - private void showFailure(Exception e) { + private void showFailure(String messageKey, Exception e) { e.printStackTrace(System.err); - resultLabel.setText("" + I18N.getMessage("githubLoginFailed", e.getLocalizedMessage()) + ""); + if (!isDisplayable()) { + return; + } + resultLabel.setText("" + I18N.getMessage(messageKey, e.getLocalizedMessage()) + ""); cards.show(content, "result"); } } diff --git a/cr-core/src/main/resources/i18n/MessagesBundle.properties b/cr-core/src/main/resources/i18n/MessagesBundle.properties index b742a84..6fdb895 100644 --- a/cr-core/src/main/resources/i18n/MessagesBundle.properties +++ b/cr-core/src/main/resources/i18n/MessagesBundle.properties @@ -23,6 +23,7 @@ githubLoginSubmitting=Submitting issue... githubLoginCancel=Cancel githubLoginSuccess=Issue created: {0} githubLoginFailed=Login failed: {0} +githubSubmitFailed=Submitting the issue failed: {0} joinDiscord=Join Discord Server stackTrace=StackTrace logFile=Log File diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java index 1208ee1..490c587 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java @@ -247,6 +247,75 @@ void bodyKeepsTheRootCauseOfAChainedExceptionAfterAMoreFramesLine() { assertTrue(body.contains("... 3 common frames omitted"), "Expected the trailing omitted-frames line, got: " + body); } + @Test + void bodyKeepsTheRootCauseAfterASuppressedSection() { + String combinedLog = "=== Terasology-game.log ===\n" + + "java.lang.IllegalStateException: wrapper\n" + + "\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n" + + "\tSuppressed: java.io.IOException: close failed\n" + + "\t\tat java.base/java.io.FileOutputStream.close(FileOutputStream.java:300)\n" + + "Caused by: java.io.IOException: disk full\n" + + "\tat java.base/java.io.FileOutputStream.write(FileOutputStream.java:100)\n"; + + String body = CrashSummary.extract(new RuntimeException("boom"), combinedLog, terasology()).buildBody(null); + + assertTrue(body.contains("Suppressed: java.io.IOException: close failed"), body); + assertTrue(body.contains("Caused by: java.io.IOException: disk full"), body); + } + + @Test + void bodyListsTwoDistinctFailuresThatShareAHeader() { + RuntimeException primary = new RuntimeException("boom"); + // Same type and message, different traces: the second is a separate failure, not a duplicate. + String combinedLog = "=== Terasology-game.log ===\n" + + primary + "\n" + + "\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n" + + "later...\n" + + primary + "\n" + + "\tat org.terasology.engine.world.WorldRenderer.render(WorldRenderer.java:50)\n"; + + String body = CrashSummary.extract(primary, combinedLog, terasology()).buildBody(null); + + assertTrue(body.contains("TerasologyEngine.run"), body); + assertTrue(body.contains("WorldRenderer.render"), "Expected the second same-header failure listed too, got: " + body); + } + + @Test + void bodyPutsTheFullLogLinkBeforeTheExceptions() throws MalformedURLException { + String body = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT, terasology()) + .buildBody(new URL("https://pastebin.com/abc123")); + + // A URL-squeezed body is truncated at the tail, so the link must come first to survive. + assertTrue(body.indexOf("https://pastebin.com/abc123") < body.indexOf("### Exceptions"), body); + } + + @Test + void titleTruncationDoesNotSplitASurrogatePair() { + String emoji = new String(Character.toChars(0x1F600)); + // 76 ASCII chars put the emoji's two chars at indices 76-77, exactly where the cut lands. + StringBuilder message = new StringBuilder(); + for (int i = 0; i < 76; i++) { + message.append('a'); + } + message.append(emoji).append(" and much more text after it"); + + String title = CrashSummary.extract(new RuntimeException(message.toString()), LOG_TEXT, terasology()).buildTitle(); + + assertFalse(title.contains("\uD83D..."), "Expected no lone high surrogate before the ellipsis: " + title); + assertTrue(title.endsWith("..."), title); + } + + @Test + void aConfiguredRegexWithoutACaptureGroupDegradesToNotExtracted() { + CrashSummary.Profile noGroup = new CrashSummary.Profile("X", "engineVersion=\\S+", null, "Activating module: \\S+", + new EnumMap<>(CrashSummary.Profile.FormField.class)); + + String body = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT, noGroup).buildBody(null); + + assertFalse(body.contains("version:"), body); + assertFalse(body.contains("Active modules"), body); + } + @Test void bodyFallsBackToTheExceptionsOwnTraceWhenNotFoundInAnyTab() { RuntimeException primary = new RuntimeException("boom"); diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubDeviceLoginTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubDeviceLoginTest.java index 9699234..51fc99f 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubDeviceLoginTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubDeviceLoginTest.java @@ -115,6 +115,15 @@ void pollForAccessTokenRetriesOnAuthorizationPending() throws IOException, Inter assertEquals("tok123", token); } + @Test + void pollForAccessTokenStopsOnAResponseWithNeitherTokenNorError() throws IOException { + // Not a response the device flow defines. Polling on would hold the dialog until expiry. + CloseableHttpClient client = mockClientReturning("token_type=bearer"); + + assertThrows(IOException.class, + () -> GitHubDeviceLogin.pollForAccessToken(client, "client-id", fastDeviceCode())); + } + @Test void pollForAccessTokenThrowsOnAccessDenied() throws IOException { CloseableHttpClient client = mockClientReturning("error=access_denied&error_description=User denied access"); diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java index 8cf6fbc..053f65f 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java @@ -3,6 +3,7 @@ package org.terasology.crashreporter.pages; +import com.google.common.base.Strings; import org.junit.jupiter.api.Test; import java.util.LinkedHashMap; @@ -80,7 +81,7 @@ void formBuildOmitsFieldsWithNoValueInsteadOfPreFillingThemBlank() { @Test void bodyBuildStaysUnderGitHubsUrlByteLimit() { // GitHub rejects the whole thing past 8191 bytes. Must truncate. - String hugeBody = "x".repeat(50_000); + String hugeBody = Strings.repeat("x", 50_000); String link = GitHubIssueLinkBuilder.build("https://github.com/MovingBlocks/Terasology/issues/new", "Crash: NullPointerException", hugeBody); @@ -94,8 +95,8 @@ void formBuildStaysUnderGitHubsUrlByteLimit() { Map fields = new LinkedHashMap<>(); fields.put("terasology_version", "5.4.0-SNAPSHOT"); fields.put("operating_system", "Mac OS X"); - fields.put("actual_behavior", "x".repeat(50_000)); - fields.put("additional_context", "y".repeat(50_000)); + fields.put("actual_behavior", Strings.repeat("x", 50_000)); + fields.put("additional_context", Strings.repeat("y", 50_000)); String link = GitHubIssueLinkBuilder.build("https://github.com/MovingBlocks/Terasology/issues/new", "crash-bug-report.yml", "Crash: NullPointerException", fields); @@ -113,7 +114,7 @@ void fitToBudgetKeepsTheLongestPrefixThatFits() { String suffix = "%0A...+truncated%2C+see+the+full+log"; int budget = 10 + suffix.length(); - String fitted = GitHubIssueLinkBuilder.fitToBudget("x".repeat(100), budget); + String fitted = GitHubIssueLinkBuilder.fitToBudget(Strings.repeat("x", 100), budget); assertEquals("xxxxxxxxxx" + suffix, fitted); assertEquals(budget, fitted.length()); @@ -121,7 +122,7 @@ void fitToBudgetKeepsTheLongestPrefixThatFits() { @Test void fitToBudgetReturnsNullWhenNotEvenTheSuffixFits() { - assertNull(GitHubIssueLinkBuilder.fitToBudget("x".repeat(100), 5)); + assertNull(GitHubIssueLinkBuilder.fitToBudget(Strings.repeat("x", 100), 5)); } @Test @@ -129,7 +130,7 @@ void truncationNeverSplitsASurrogatePair() { // U+1F600 is one code point but two Java chars; cutting between them yields a lone high // surrogate, which URLEncoder renders as "%3F" ('?') - a mangled trailing character. String emoji = new String(Character.toChars(0x1F600)); - String value = emoji.repeat(2_000); + String value = Strings.repeat(emoji, 2_000); String fitted = GitHubIssueLinkBuilder.fitToBudget(value, 1_000); @@ -137,11 +138,30 @@ void truncationNeverSplitsASurrogatePair() { assertTrue(fitted.length() <= 1_000, "over budget: " + fitted.length()); } + @Test + void formBuildKeepsSmallLaterFieldsWhenAnEarlierFieldIsOversized() { + // The full-log link is the one field a truncated trace points at, and it comes after the + // trace in CrashSummary's field order. A greedy allocation dropped it. + Map fields = new LinkedHashMap<>(); + fields.put("actual_behavior", Strings.repeat("x", 50_000)); + fields.put("log_details", "[PasteBin](https://pastebin.com/abc123)"); + fields.put("additional_context", "Active modules:\n- engine:5.4.0"); + + String link = GitHubIssueLinkBuilder.build("https://example.com/issues/new", "t.yml", "t", fields); + + assertTrue(link.contains("log_details=%5BPasteBin%5D%28https%3A%2F%2Fpastebin.com%2Fabc123%29"), link); + assertTrue(link.contains("additional_context="), link); + assertTrue(link.contains("actual_behavior=xxx"), link); + assertTrue(link.length() < 8191, "link was " + link.length() + " bytes"); + // Output order is still the map's order, so the form reads the way CrashSummary wrote it. + assertTrue(link.indexOf("actual_behavior=") < link.indexOf("log_details="), link); + } + @Test void truncationNeverSplitsAPercentEscape() { Map fields = new LinkedHashMap<>(); // Every char is a 3-byte "%XX" escape, so a mid-escape cut would hide here. - fields.put("actual_behavior", "&".repeat(50_000)); + fields.put("actual_behavior", Strings.repeat("&", 50_000)); String link = GitHubIssueLinkBuilder.build("https://example.com/issues/new", "t.yml", "t", fields); From d3c9edcf538899736086a67ea098cbe9daff2dd7 Mon Sep 17 00:00:00 2001 From: Cervator Date: Sat, 3 Oct 2026 21:22:03 -0400 Subject: [PATCH 6/6] fix(reporter): authorize through a GitHub App with issues-only permission, not an OAuth App The OAuth App asked every reporter for `public_repo`: write access to all their public repositories, kept until revoked. The GitHub App's token can only do what the player already could on Terasology, which is open an issue. Client ID checked against the device-code endpoint; no `scope` is sent. Co-Authored-By: Claude Fable 5.1 GDD-Host: phoenix --- .../crashreporter/GlobalProperties.java | 5 +++-- .../crashreporter/pages/GitHubDeviceLogin.java | 16 +++++++++++----- .../src/main/resources/crashreporter.properties | 4 +++- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java b/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java index 00942ed..70a56df 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java @@ -19,8 +19,9 @@ public enum KEY { JOIN_DISCORD_LINK, REPORT_ISSUE_LINK, REPORT_ISSUE_TEMPLATE, - // GitHub OAuth App client ID with Device Flow enabled. Unset by default - see - // GitHubDeviceLogin's javadoc. When unset, the "submit directly" button is hidden. + // Client ID of a GitHub App (not an OAuth App) with Device Flow enabled and Issues write + // permission. Unset by default - see GitHubDeviceLogin's javadoc for why a GitHub App. + // When unset, the "submit directly" button is hidden. REPORT_ISSUE_OAUTH_CLIENT_ID, // Field IDs of the issue form named by REPORT_ISSUE_TEMPLATE, one per thing CrashSummary // can pre-fill. Unset means that field is left for the user. Form-specific, so these live diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubDeviceLogin.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubDeviceLogin.java index 19641bb..18f70cd 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubDeviceLogin.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubDeviceLogin.java @@ -23,22 +23,28 @@ /** * GitHub's OAuth Device Flow - no client secret, no redirect URI, made for apps like this one. - * Needs an OAuth App registered at github.com/settings/developers with Device Flow enabled; - * its client ID goes in {@link org.terasology.crashreporter.GlobalProperties.KEY#REPORT_ISSUE_OAUTH_CLIENT_ID}. + * Needs a GitHub App with Device Flow enabled, the "Issues: read and write" repository + * permission, and an installation on the repository that receives the reports; its client ID goes + * in {@link org.terasology.crashreporter.GlobalProperties.KEY#REPORT_ISSUE_OAUTH_CLIENT_ID}. + *

+ * A GitHub App rather than an OAuth App on purpose. The token a user grants is limited to what + * the app may do, on the repositories it is installed on, and to what that user could already do + * there - for a player with no role on the repo, opening an issue. An OAuth App has no scope + * narrower than {@code public_repo}, which is write access to every public repository the user + * owns. GitHub Apps ignore the {@code scope} parameter, so none is sent. + *

* Endpoints reply form-urlencoded by default, so no JSON parsing needed here. */ public final class GitHubDeviceLogin { private static final String DEVICE_CODE_URL = "https://github.com/login/device/code"; private static final String TOKEN_URL = "https://github.com/login/oauth/access_token"; - private static final String SCOPE = "public_repo"; private GitHubDeviceLogin() { } public static DeviceCode requestDeviceCode(CloseableHttpClient client, String clientId) throws IOException { - Map fields = post(client, DEVICE_CODE_URL, - param("client_id", clientId), param("scope", SCOPE)); + Map fields = post(client, DEVICE_CODE_URL, param("client_id", clientId)); failOnError(fields); return new DeviceCode(required(fields, "device_code"), required(fields, "user_code"), required(fields, "verification_uri"), requiredInt(fields, "expires_in"), requiredInt(fields, "interval")); diff --git a/cr-terasology/src/main/resources/crashreporter.properties b/cr-terasology/src/main/resources/crashreporter.properties index ad67d36..ca22d6f 100644 --- a/cr-terasology/src/main/resources/crashreporter.properties +++ b/cr-terasology/src/main/resources/crashreporter.properties @@ -1,7 +1,9 @@ SUPPORT_FORUM_LINK=http://forum.terasology.org/forum/support.20/ REPORT_ISSUE_LINK=https://github.com/MovingBlocks/Terasology/issues/new REPORT_ISSUE_TEMPLATE=crash-bug-report.yml -REPORT_ISSUE_OAUTH_CLIENT_ID=Ov23liSKtBll7VXE8Vur +# Client ID of the "Terasology Crash Reporter" GitHub App (Issues: read and write, installed on +# MovingBlocks/Terasology). Public by design; device flow uses no secret and no private key. +REPORT_ISSUE_OAUTH_CLIENT_ID=Iv23li6bGRhTCcQtIk0u JOIN_DISCORD_LINK=https://discord.gg/terasology # Field IDs in Terasology's .github/ISSUE_TEMPLATE/crash-bug-report.yml (MovingBlocks/Terasology#5390).