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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jSvSTrG4qsniLSr6TpC6
This commit is contained in:
@@ -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.
|
||||
*
|
||||
* <p>{@code trim()} alone leaves an <em>interior</em> 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.
|
||||
*
|
||||
* <p>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());
|
||||
|
||||
@@ -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.
|
||||
*
|
||||
* <p>{@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.
|
||||
*
|
||||
* <p>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")
|
||||
|
||||
@@ -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.
|
||||
*
|
||||
* <p>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<Msg> messages, int maxTokens, double temperature) {
|
||||
String answer(String key, String model, List<Turn> 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<Msg> messages, int maxTokens, double temperature) {
|
||||
private JsonObject base(String model, List<Turn> 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());
|
||||
|
||||
@@ -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");
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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());
|
||||
}
|
||||
}
|
||||
@@ -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() {
|
||||
|
||||
Reference in New Issue
Block a user