diff --git a/fleetd/src/main/java/dev/ltms/fleet/auth/Authz.java b/fleetd/src/main/java/dev/ltms/fleet/auth/Authz.java index d59d9e8..854b46f 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/auth/Authz.java +++ b/fleetd/src/main/java/dev/ltms/fleet/auth/Authz.java @@ -75,6 +75,9 @@ public final class Authz { * {@code SEND} is refused, as if no terminal were a configured lead or collaborator — the * same decision {@link #NO_KNOWN_LEAD_OR_COLLABORATOR} gives explicitly. Every other action's * result is identical to the four-argument form's, since none of them consult the classifier. + * + *

Its default classifier denies every collaborator, so a caller enforcing authorization + * must use the four-argument form instead. */ public static boolean permits(Principal caller, Action action, String targetSession) { return permits(caller, action, targetSession, NO_KNOWN_LEAD_OR_COLLABORATOR); diff --git a/fleetd/src/test/java/dev/ltms/fleet/msg/MessageServicePollUsageTest.java b/fleetd/src/test/java/dev/ltms/fleet/msg/MessageServicePollUsageTest.java new file mode 100644 index 0000000..3bf33ad --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/msg/MessageServicePollUsageTest.java @@ -0,0 +1,280 @@ +package dev.ltms.fleet.msg; + +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Stream; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Pins that no file under {@code src/main/java} calls the fail-open + * {@link MessageService#poll(String)} overload. That overload skips the ownership check in + * {@code MessageService}'s {@code ownsTicket} entirely, so a caller of it can read any session's + * ticket. Every production caller must go through {@link MessageService#poll(String, String)} + * and pass a {@code callerTerminal} explicitly, even when it is {@code null}. + * + *

This reads each file's own source text rather than reflecting on compiled bytecode, because + * the risk is a future one-word edit at a call site, not a missing overload. + * + *

The scan below finds a violation by its receiver, {@code messages.poll(}, rather than the + * bare method name, so it does not mistake {@link java.util.Queue#poll()} for a violation. That + * anchor only covers a {@code MessageService} reached through a variable or field named + * {@code messages}, so {@link #everyMessageServiceDeclarationIsNamedMessages} pins the naming + * convention the anchor depends on: a declaration under any other name would be invisible to the + * scan above, and must turn this second check red instead of passing silently. + */ +class MessageServicePollUsageTest { + + private static final Path PRODUCTION_SOURCE = Path.of("src/main/java"); + + @Test + void noProductionFileCallsTheSingleArgumentPollOverload() throws IOException { + List violations = new ArrayList<>(); + List twoArgSites = new ArrayList<>(); + int filesScanned = scanForPollCalls(PRODUCTION_SOURCE, violations, twoArgSites); + + // CONTROL: the scan actually walked files -- a wrong root would otherwise report "no + // violations found" having looked at nothing. + assertTrue(filesScanned > 0, "control failed: the scan under " + PRODUCTION_SOURCE + + " visited zero .java files -- the path is wrong, so the absence of violations " + + "below proves nothing"); + + assertTrue(violations.isEmpty(), "found a call to the fail-open MessageService.poll(String) " + + "overload, which skips the ownership check entirely -- pass a callerTerminal " + + "explicitly (even if null) through poll(String, String) instead: " + violations); + + // CONTROL: the arity parser actually finds the two genuine two-argument call sites (the + // MCP handler in FleetMcp and the REST handler in FleetApp). If this drops, the parser + // itself is broken, not the production code -- a broken parser (or a scan root that + // reaches no real source) must fail loudly here rather than pass vacuously above. + assertEquals(2, twoArgSites.size(), "control failed: expected exactly the two known " + + "two-argument messages.poll(...) call sites, found: " + twoArgSites); + assertTrue(twoArgSites.stream().anyMatch(s -> s.contains("FleetMcp.java")), + "control failed: did not find the FleetMcp.java messages.poll(ticket, callerTerminal) " + + "site among: " + twoArgSites); + assertTrue(twoArgSites.stream().anyMatch(s -> s.contains("FleetApp.java")), + "control failed: did not find the FleetApp.java messages.poll(...) site among: " + + twoArgSites); + } + + /** + * The {@code messages.poll(} anchor above only sees a {@code MessageService} reached through + * a variable, field, or parameter named {@code messages}. This asserts that every such + * declaration under {@code src/main/java} uses that name, so a differently named declaration + * -- invisible to the scan above -- fails loudly here instead of letting that scan pass on a + * call site it never looked at. + */ + @Test + void everyMessageServiceDeclarationIsNamedMessages() throws IOException { + List names = new ArrayList<>(); + int filesScanned = scanForDeclarationNames(PRODUCTION_SOURCE, names); + + // CONTROL: the scan actually walked files -- a wrong root would otherwise report "every + // declaration is named messages" having looked at nothing. + assertTrue(filesScanned > 0, "control failed: the scan under " + PRODUCTION_SOURCE + + " visited zero .java files -- the path is wrong, so the result below proves nothing"); + + // CONTROL: the declaration pattern actually finds real declarations. Zero means the + // pattern is broken, not that every MessageService variable, field, or parameter vanished. + assertTrue(names.size() > 0, "control failed: found zero MessageService declarations under " + + PRODUCTION_SOURCE + " -- the declaration pattern is broken, update it before " + + "trusting the naming check below"); + + List other = names.stream().filter(n -> !n.equals("messages")).distinct().toList(); + assertTrue(other.isEmpty(), "found a MessageService declaration not named \"messages\": " + + other + " -- the messages.poll( scan above only looks for that name, so a call " + + "through a differently named variable or field is invisible to it; either rename " + + "the declaration or widen that scan's anchor to cover it"); + } + + private static int scanForPollCalls(Path root, List violations, List twoArgSites) + throws IOException { + List files = javaFiles(root); + for (Path file : files) { + scanFileForPollCalls(file, violations, twoArgSites); + } + return files.size(); + } + + private static int scanForDeclarationNames(Path root, List names) throws IOException { + List files = javaFiles(root); + Pattern declaration = Pattern.compile("MessageService\\s+([A-Za-z_][A-Za-z0-9_]*)"); + for (Path file : files) { + if (file.getFileName().toString().equals("MessageService.java")) { + continue; // the type's own declaration, not a caller holding a reference to it + } + String stripped = stripComments(Files.readString(file)); + Matcher m = declaration.matcher(stripped); + while (m.find()) { + int j = m.end(); + while (j < stripped.length() && Character.isWhitespace(stripped.charAt(j))) j++; + if (j < stripped.length() && stripped.charAt(j) == '(') { + continue; // a method named like the convention, e.g. "MessageService messages()" + } + names.add(m.group(1)); + } + } + return files.size(); + } + + private static List javaFiles(Path root) throws IOException { + try (Stream paths = Files.walk(root)) { + return paths.filter(p -> p.toString().endsWith(".java")).toList(); + } + } + + /** + * Replaces {@code //} and {@code /* *}{@code /} comment text with nothing, leaving code, + * string/char literals and line breaks untouched -- so a comment that merely mentions + * {@code MessageService} in prose can never be read as a declaration. + */ + private static String stripComments(String source) { + StringBuilder out = new StringBuilder(source.length()); + boolean inString = false; + boolean inChar = false; + int i = 0; + while (i < source.length()) { + char c = source.charAt(i); + if (inString) { + out.append(c); + if (c == '\\' && i + 1 < source.length()) { out.append(source.charAt(i + 1)); i += 2; continue; } + if (c == '"') inString = false; + i++; + continue; + } + if (inChar) { + out.append(c); + if (c == '\\' && i + 1 < source.length()) { out.append(source.charAt(i + 1)); i += 2; continue; } + if (c == '\'') inChar = false; + i++; + continue; + } + if (c == '"') { inString = true; out.append(c); i++; continue; } + if (c == '\'') { inChar = true; out.append(c); i++; continue; } + if (c == '/' && i + 1 < source.length() && source.charAt(i + 1) == '/') { + while (i < source.length() && source.charAt(i) != '\n') i++; + continue; // leaves the newline itself for the next iteration to append + } + if (c == '/' && i + 1 < source.length() && source.charAt(i + 1) == '*') { + i += 2; + while (i < source.length() && !(source.charAt(i) == '*' && i + 1 < source.length() + && source.charAt(i + 1) == '/')) { + if (source.charAt(i) == '\n') out.append('\n'); + i++; + } + i += 2; + continue; + } + out.append(c); + i++; + } + return out.toString(); + } + + private static void scanFileForPollCalls(Path file, List violations, List twoArgSites) + throws IOException { + String source = Files.readString(file); + String needle = "messages.poll("; + int from = 0; + int idx; + while ((idx = source.indexOf(needle, from)) >= 0) { + int argsStart = idx + needle.length(); + String args = extractBalancedArgs(source, argsStart, file, idx); + int closeParenIndex = argsStart + args.length(); + from = closeParenIndex + 1; + + if (args.isBlank()) { + continue; // MessageService has no zero-argument poll() -- nothing to classify + } + String site = file + ":" + lineOf(source, idx); + if (topLevelCommaCount(args) == 0) { + violations.add(site + " -- messages.poll(" + args.trim() + ")"); + } else { + twoArgSites.add(site); + } + } + } + + /** + * The text between {@code messages.poll(} and its matching close paren: balanced over nested + * calls, and never split by a paren or comma sitting inside a string or char literal. + */ + private static String extractBalancedArgs(String source, int start, Path file, int callIndex) { + int depth = 1; + boolean inString = false; + boolean inChar = false; + int i = start; + while (i < source.length()) { + char c = source.charAt(i); + if (inString) { + if (c == '\\') { i += 2; continue; } + if (c == '"') inString = false; + } else if (inChar) { + if (c == '\\') { i += 2; continue; } + if (c == '\'') inChar = false; + } else if (c == '"') { + inString = true; + } else if (c == '\'') { + inChar = true; + } else if (c == '(') { + depth++; + } else if (c == ')') { + depth--; + if (depth == 0) return source.substring(start, i); + } + i++; + } + throw new IllegalStateException( + "unbalanced parentheses scanning " + file + ":" + lineOf(source, callIndex)); + } + + /** + * Commas at paren/bracket/brace depth zero, skipping string and char literals -- the argument + * separators a human reader would see, not every comma character in the text. + */ + private static int topLevelCommaCount(String args) { + int depth = 0; + int commas = 0; + boolean inString = false; + boolean inChar = false; + int i = 0; + while (i < args.length()) { + char c = args.charAt(i); + if (inString) { + if (c == '\\') { i += 2; continue; } + if (c == '"') inString = false; + } else if (inChar) { + if (c == '\\') { i += 2; continue; } + if (c == '\'') inChar = false; + } else if (c == '"') { + inString = true; + } else if (c == '\'') { + inChar = true; + } else if (c == '(' || c == '[' || c == '{') { + depth++; + } else if (c == ')' || c == ']' || c == '}') { + depth--; + } else if (c == ',' && depth == 0) { + commas++; + } + i++; + } + return commas; + } + + private static int lineOf(String source, int index) { + int line = 1; + for (int i = 0; i < index; i++) { + if (source.charAt(i) == '\n') line++; + } + return line; + } +}