From 88fe9c9695a89f2d9083507ae5b0697955a9f3f7 Mon Sep 17 00:00:00 2001 From: marcos Date: Wed, 5 Aug 2026 16:05:22 +0000 Subject: [PATCH] fix: stop a malformed API key reaching a log line HttpRequest.Builder.header() quotes the offending header value back in its IllegalArgumentException. Fuzzed on temurin-25: all 32 control characters it rejects echo the value, so a key carrying any of them ends up in whatever log catches the throw. A key file with a comment on line two survives trim() and is enough to trigger it. Ai.cleanKey now takes the first non-blank line and drops control characters, so a malformed key never forms. HttpFetcher.checkBearer rejects one anyway before the request is built, with a message naming only the position, so no future caller has to remember to redact. Its reject set is a strict superset of the JDK's. Also renames MiniMax.Msg to MiniMax.Turn: the package already has a top-level Msg, the chat-formatting helper, which the record shadowed inside MiniMax.java. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F3jSvSTrG4qsniLSr6TpC6 --- .../java/dev/marcospaulo/canalhandia/Ai.java | 41 +++++++++- .../marcospaulo/canalhandia/HttpFetcher.java | 29 +++++++ .../dev/marcospaulo/canalhandia/MiniMax.java | 21 +++-- .../marcospaulo/canalhandia/AiKeyTest.java | 78 +++++++++++++++++++ .../canalhandia/HttpFetcherTest.java | 63 +++++++++++++++ .../marcospaulo/canalhandia/MiniMaxTest.java | 4 +- 6 files changed, 224 insertions(+), 12 deletions(-) create mode 100644 src/test/java/dev/marcospaulo/canalhandia/AiKeyTest.java create mode 100644 src/test/java/dev/marcospaulo/canalhandia/HttpFetcherTest.java diff --git a/src/main/java/dev/marcospaulo/canalhandia/Ai.java b/src/main/java/dev/marcospaulo/canalhandia/Ai.java index 032431d..91d352f 100644 --- a/src/main/java/dev/marcospaulo/canalhandia/Ai.java +++ b/src/main/java/dev/marcospaulo/canalhandia/Ai.java @@ -66,16 +66,51 @@ final class Ai { return apiKey() != null; } + /** + * The first non-blank line of a key file or variable, with any control + * character removed. Null if there is nothing usable. + * + *

{@code trim()} alone leaves an interior newline — a key file + * with the key on line 1 and a comment on line 2 survives it. Such a key + * cannot go in an HTTP header, and the JDK's rejection of it quotes the + * whole header value, key included, into the exception message, which then + * reaches the server log. Cleaning at the source means that never happens; + * {@link HttpFetcher} checks again as a backstop. + * + *

Takes the first line rather than deleting the newline and joining, so + * a trailing comment line cannot be silently welded onto the key to make a + * different, wrong one — that would turn a readable failure into a puzzling + * authentication error. + */ + static String cleanKey(String raw) { + if (raw == null) { + return null; + } + for (String line : raw.split("\\R")) { + StringBuilder out = new StringBuilder(line.length()); + for (int i = 0; i < line.length(); i++) { + char c = line.charAt(i); + if (c >= 0x20 && c != 0x7F) { + out.append(c); + } + } + String key = out.toString().trim(); + if (!key.isEmpty()) { + return key; + } + } + return null; + } + private String apiKey() { String fromEnv = System.getenv(KEY_ENV); if (fromEnv != null && !fromEnv.isBlank()) { - return fromEnv.trim(); + return cleanKey(fromEnv); } Path file = plugin.getDataFolder().toPath().resolve(KEY_FILE); try { if (Files.isReadable(file)) { - String key = Files.readString(file, StandardCharsets.UTF_8).trim(); - return key.isEmpty() ? null : key; + return cleanKey(Files.readString(file, StandardCharsets.UTF_8)); } } catch (IOException e) { plugin.getLogger().warning("Não consegui ler " + KEY_FILE + ": " + e.getMessage()); diff --git a/src/main/java/dev/marcospaulo/canalhandia/HttpFetcher.java b/src/main/java/dev/marcospaulo/canalhandia/HttpFetcher.java index 0ad6834..f3c9ba5 100644 --- a/src/main/java/dev/marcospaulo/canalhandia/HttpFetcher.java +++ b/src/main/java/dev/marcospaulo/canalhandia/HttpFetcher.java @@ -36,9 +36,38 @@ final class HttpFetcher implements Fetcher { return body(http.send(request, HttpResponse.BodyHandlers.ofString())); } + /** + * Rejects a bearer token that cannot go in a header, before it reaches one. + * + *

{@code HttpRequest.Builder.header()} validates the value itself, but + * its {@code IllegalArgumentException} quotes the offending value back — + * measured on temurin-25: {@code invalid header value: "Bearer sk-…"}. That + * throw happens before any I/O, so it reaches callers as an ordinary call + * failure and lands the credential in whatever log catches it. A key with a + * stray second line is enough to trigger it. + * + *

Checking here rather than at the call site means no future caller has + * to remember to redact. The message deliberately names only the position + * of the offending character, never any part of the key. + */ + static void checkBearer(String bearer) throws IOException { + if (bearer == null || bearer.isEmpty()) { + throw new IOException("Chave de API vazia."); + } + for (int i = 0; i < bearer.length(); i++) { + char c = bearer.charAt(i); + if (c < 0x20 || c == 0x7F) { + throw new IOException("Chave de API inválida: caractere de controle na posição " + + i + " (de " + bearer.length() + "). " + + "Verifique se o arquivo da chave tem uma única linha, sem quebra de linha."); + } + } + } + @Override public String postJson(String url, String json, String bearer) throws IOException, InterruptedException { + checkBearer(bearer); HttpRequest request = HttpRequest.newBuilder(URI.create(url)) .timeout(Duration.ofSeconds(timeoutSeconds)) .header("Content-Type", "application/json") diff --git a/src/main/java/dev/marcospaulo/canalhandia/MiniMax.java b/src/main/java/dev/marcospaulo/canalhandia/MiniMax.java index 9215daa..0431658 100644 --- a/src/main/java/dev/marcospaulo/canalhandia/MiniMax.java +++ b/src/main/java/dev/marcospaulo/canalhandia/MiniMax.java @@ -59,8 +59,15 @@ final class MiniMax { */ private static final double TERM_TEMPERATURE = 0.0; - /** One message in the request. */ - record Msg(String role, String content) { + /** + * One message in the request. + * + *

Named {@code Turn} and not {@code Msg} because the package already has + * a top-level {@link Msg}, the chat-formatting helper. A nested record of + * that name would shadow it inside this file, so a later {@code Msg.error} + * here would resolve to the record instead. + */ + record Turn(String role, String content) { } private final Fetcher fetcher; @@ -103,8 +110,8 @@ final class MiniMax { */ String searchTerm(String key, String model, String question) { JsonObject body = base(model, List.of( - new Msg("system", "Você escolhe qual artigo da Minecraft Wiki consultar."), - new Msg("user", question)), TERM_TOKENS, TERM_TEMPERATURE); + new Turn("system", "Você escolhe qual artigo da Minecraft Wiki consultar."), + new Turn("user", question)), TERM_TOKENS, TERM_TEMPERATURE); body.add("tools", JsonParser.parseString(TOOLS).getAsJsonArray()); body.add("tool_choice", JsonParser.parseString(TOOL_CHOICE).getAsJsonObject()); @@ -146,7 +153,7 @@ final class MiniMax { * spends the whole allowance thinking and returns nothing at all. Measured * twice at 400 tokens, which is why answers are given 1200. */ - String answer(String key, String model, List messages, int maxTokens, double temperature) { + String answer(String key, String model, List messages, int maxTokens, double temperature) { JsonObject message = message(post(key, base(model, messages, maxTokens, temperature))); if (message == null) { return null; @@ -160,9 +167,9 @@ final class MiniMax { return content.getAsString(); } - private JsonObject base(String model, List messages, int maxTokens, double temperature) { + private JsonObject base(String model, List messages, int maxTokens, double temperature) { JsonArray array = new JsonArray(); - for (Msg msg : messages) { + for (Turn msg : messages) { JsonObject object = new JsonObject(); object.addProperty("role", msg.role()); object.addProperty("content", msg.content()); diff --git a/src/test/java/dev/marcospaulo/canalhandia/AiKeyTest.java b/src/test/java/dev/marcospaulo/canalhandia/AiKeyTest.java new file mode 100644 index 0000000..bae5bfe --- /dev/null +++ b/src/test/java/dev/marcospaulo/canalhandia/AiKeyTest.java @@ -0,0 +1,78 @@ +package dev.marcospaulo.canalhandia; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * The key-cleaning half of {@link Ai}. The rest of that class needs a running + * server; {@code cleanKey} is pure and is where the credential is made safe to + * put in a header. + */ +class AiKeyTest { + + private static final char LF = 10; + private static final char CR = 13; + + @Test + void keepsAWellFormedKeyUnchanged() { + assertEquals("sk-abc123", Ai.cleanKey("sk-abc123")); + } + + @Test + void stripsSurroundingWhitespaceAndTrailingNewline() { + assertEquals("sk-abc123", Ai.cleanKey(" sk-abc123 " + LF)); + assertEquals("sk-abc123", Ai.cleanKey("sk-abc123" + CR + LF)); + } + + /** The case trim() misses: a second line in the key file. */ + @Test + void takesTheFirstLineRatherThanWeldingTheSecondOn() { + assertEquals("sk-abc123", Ai.cleanKey("sk-abc123" + LF + "# comentário")); + assertEquals("sk-abc123", Ai.cleanKey("sk-abc123" + LF + LF + "lixo")); + } + + @Test + void skipsLeadingBlankLines() { + assertEquals("sk-abc123", Ai.cleanKey(LF + " " + LF + "sk-abc123")); + } + + @Test + void removesControlCharactersFromWithinTheLine() { + assertEquals("sk-abc", Ai.cleanKey("sk-" + (char) 0 + "abc" + (char) 0x7F)); + } + + @Test + void nothingUsableIsNull() { + assertNull(Ai.cleanKey(null)); + assertNull(Ai.cleanKey("")); + assertNull(Ai.cleanKey(" " + LF + " ")); + } + + /** + * The contract that matters: whatever comes out is something + * {@link HttpFetcher} will accept, so the JDK never gets to quote it back. + */ + /** + * The contract that matters, asserted end to end: anything {@code cleanKey} + * hands back is something {@link HttpFetcher} accepts, so a real key can + * never reach the JDK's header validator and be quoted back into a log. + */ + @Test + void whateverSurvivesCleaningIsAcceptedAsABearer() { + String[] messy = { + "sk-abc123", + " sk-abc " + LF, + "sk-a" + LF + "b", + "sk" + (char) 9 + "-x", + "sk-x" + CR + LF + "# nota", + LF + "sk-y", + }; + for (String raw : messy) { + String key = Ai.cleanKey(raw); + assertNotNull(key, "nothing survived cleaning of " + raw); + assertDoesNotThrow(() -> HttpFetcher.checkBearer(key), + "HttpFetcher rejected a key cleanKey had approved"); + } + } +} diff --git a/src/test/java/dev/marcospaulo/canalhandia/HttpFetcherTest.java b/src/test/java/dev/marcospaulo/canalhandia/HttpFetcherTest.java new file mode 100644 index 0000000..f6274f9 --- /dev/null +++ b/src/test/java/dev/marcospaulo/canalhandia/HttpFetcherTest.java @@ -0,0 +1,63 @@ +package dev.marcospaulo.canalhandia; + +import org.junit.jupiter.api.Test; + +import java.io.IOException; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * Only the bearer check is exercised here. It runs before the request is built + * and before any I/O, so these tests reach no network: the URL below is a + * reserved-for-invalid TLD and would fail loudly if one were ever attempted. + */ +class HttpFetcherTest { + + private static final String URL = "https://nunca.invalid/v1/chat"; + + /** + * A key file with a second line survives {@code trim()}. The resulting + * header value makes the JDK throw + * {@code invalid header value: "Bearer sk-…"} — the credential, in an + * exception message, in whatever log catches it. + */ + @Test + void rejectsABearerWithANewlineBeforeBuildingTheRequest() { + String key = "sk-canalhandia-segredo" + (char) 10 + "# comentário"; + IOException thrown = assertThrows(IOException.class, + () -> new HttpFetcher(5).postJson(URL, "{}", key)); + assertFalse(thrown.getMessage().contains("segredo"), + "key leaked into the exception: " + thrown.getMessage()); + assertTrue(thrown.getMessage().contains("controle"), thrown.getMessage()); + } + + @Test + void rejectsCarriageReturnAndNulAndDelete() { + for (char c : new char[]{13, 0, 0x7F, 9}) { + String key = "sk-segredo" + c + "x"; + IOException thrown = assertThrows(IOException.class, + () -> new HttpFetcher(5).postJson(URL, "{}", key), + "accepted a bearer containing char " + (int) c); + assertFalse(thrown.getMessage().contains("segredo"), thrown.getMessage()); + } + } + + @Test + void rejectsAnEmptyOrNullBearer() { + assertThrows(IOException.class, () -> new HttpFetcher(5).postJson(URL, "{}", "")); + assertThrows(IOException.class, () -> new HttpFetcher(5).postJson(URL, "{}", null)); + } + + /** + * The message must be actionable without quoting the key, so it names the + * position and the likely cause instead. + */ + @Test + void theMessageSaysWhereTheProblemIsWithoutQuotingTheKey() { + IOException thrown = assertThrows(IOException.class, + () -> new HttpFetcher(5).postJson(URL, "{}", "abc" + (char) 10 + "def")); + assertTrue(thrown.getMessage().contains("3"), thrown.getMessage()); + assertFalse(thrown.getMessage().contains("abc"), thrown.getMessage()); + assertFalse(thrown.getMessage().contains("def"), thrown.getMessage()); + } +} diff --git a/src/test/java/dev/marcospaulo/canalhandia/MiniMaxTest.java b/src/test/java/dev/marcospaulo/canalhandia/MiniMaxTest.java index a361901..b8d1fd3 100644 --- a/src/test/java/dev/marcospaulo/canalhandia/MiniMaxTest.java +++ b/src/test/java/dev/marcospaulo/canalhandia/MiniMaxTest.java @@ -119,7 +119,7 @@ class MiniMaxTest { void answerSendsNoToolsAndCarriesItsOwnBudget() { Recorder recorder = new Recorder("{\"choices\":[{\"message\":{\"content\":\"ok\"}}]}"); new MiniMax(recorder).answer("k", "MiniMax-M2.7", - List.of(new MiniMax.Msg("system", "regras"), new MiniMax.Msg("user", "oi")), + List.of(new MiniMax.Turn("system", "regras"), new MiniMax.Turn("user", "oi")), 1200, 0.3); JsonObject body = JsonParser.parseString(recorder.json).getAsJsonObject(); @@ -203,7 +203,7 @@ class MiniMaxTest { Recorder recorder = new Recorder("{\"base_resp\":{\"status_code\":1004," + "\"status_msg\":\"invalid api key\"}}"); MiniMax refused = new MiniMax(recorder, null, warnings::add); - assertNull(refused.answer(key, "m", List.of(new MiniMax.Msg("user", "oi")), 1200, 0.3)); + assertNull(refused.answer(key, "m", List.of(new MiniMax.Turn("user", "oi")), 1200, 0.3)); assertNull(refused.searchTerm(key, "m", "oi")); MiniMax broken = new MiniMax(new Fetcher() {