- * The dispatcher never deserializes the payload into a {@link CommandPayload}; it stores - * the raw JSON and hands it to the agent so classification has to hang off the wire - * string rather than the payload type. + * Each constant carries the {@link CommandPayload} record that defines its payload shape. + * {@code CommandPayloadCodec} uses that mapping to deserialize and Bean-Validate the + * incoming JSON before anything is stored or forwarded, so a bound declared on a payload + * component is enforced on the wire without any further wiring. */ public enum CommandKind { - CHROME_RELOAD("chrome.reload", CommandRisk.BENIGN), - CONFIG_REFRESH("config.refresh", CommandRisk.BENIGN), + CHROME_RELOAD("chrome.reload", CommandRisk.BENIGN, ChromeReloadPayload.class), + CONFIG_REFRESH("config.refresh", CommandRisk.BENIGN, ConfigRefreshPayload.class), - KIOSK_RESTART("kiosk.restart", CommandRisk.DISRUPTIVE), - SYSTEM_REBOOT("system.reboot", CommandRisk.DISRUPTIVE), + KIOSK_RESTART("kiosk.restart", CommandRisk.DISRUPTIVE, KioskRestartPayload.class), + SYSTEM_REBOOT("system.reboot", CommandRisk.DISRUPTIVE, SystemRebootPayload.class), - CHROME_SCREENSHOT("chrome.screenshot", CommandRisk.INSPECT), - LOGS_TAIL("logs.tail", CommandRisk.INSPECT); + CHROME_SCREENSHOT("chrome.screenshot", CommandRisk.INSPECT, ChromeScreenshotPayload.class), + LOGS_TAIL("logs.tail", CommandRisk.INSPECT, LogsTailPayload.class); private final String wireName; private final CommandRisk risk; + private final Class payloadType; - CommandKind(String wireName, CommandRisk risk) { + CommandKind(String wireName, CommandRisk risk, Class payloadType) { this.wireName = wireName; this.risk = risk; + this.payloadType = payloadType; } /** The dotted discriminator sent over the wire, e.g. {@code chrome.reload}. */ @@ -47,6 +51,11 @@ public CommandRisk getRisk() { return risk; } + /** The record defining this kind's payload shape; the target type for validated deserialization. */ + public Class getPayloadType() { + return payloadType; + } + public Permission getRequiredPermission() { return risk.getRequiredPermission(); } diff --git a/src/main/java/com/ibrasoft/lensbridge/model/minbar/board/commands/CommandPayload.java b/src/main/java/com/ibrasoft/lensbridge/model/minbar/board/commands/CommandPayload.java index 7a69a5f..82bdba9 100644 --- a/src/main/java/com/ibrasoft/lensbridge/model/minbar/board/commands/CommandPayload.java +++ b/src/main/java/com/ibrasoft/lensbridge/model/minbar/board/commands/CommandPayload.java @@ -10,6 +10,18 @@ *
* The discriminator is {@code kind} (e.g. {@code "chrome.reload"}). Stored as JSON in * {@code device_commands.payload_json} and re-parsed when delivering to the agent. + *
+ * This hierarchy is the single source of truth for payload shape on the issue path. + * {@code CommandPayloadCodec} resolves the record from the request's {@code kind} via + * {@link CommandKind#getPayloadType()}, deserializes into it while rejecting unknown + * properties, runs Jakarta Bean Validation, and re-serializes the validated object as + * what gets persisted and pushed to the agent. Nothing an operator typed reaches the + * device verbatim, so a constraint annotation added to a component here is enforced on + * the wire with no further wiring. Declare bounds as constraint annotations rather than + * as compact-constructor checks: the codec reports the former as a clean 400. + *
+ * The closed {@code @JsonSubTypes} registry below is a deliberate anti-gadget defence. + * Never replace it with {@code Id.CLASS}/{@code MINIMAL_CLASS} or enable default typing. */ @JsonTypeInfo( use = JsonTypeInfo.Id.NAME, diff --git a/src/main/java/com/ibrasoft/lensbridge/model/minbar/board/commands/LogsTailPayload.java b/src/main/java/com/ibrasoft/lensbridge/model/minbar/board/commands/LogsTailPayload.java index d163b8b..553cc0e 100644 --- a/src/main/java/com/ibrasoft/lensbridge/model/minbar/board/commands/LogsTailPayload.java +++ b/src/main/java/com/ibrasoft/lensbridge/model/minbar/board/commands/LogsTailPayload.java @@ -1,11 +1,20 @@ package com.ibrasoft.lensbridge.model.minbar.board.commands; -/** Tail the last {@code lines} entries from the agent + kiosk journald units. Capped at 500. */ -public record LogsTailPayload(Integer lines) implements CommandPayload { - public LogsTailPayload { - if (lines != null && (lines < 1 || lines > 500)) { - throw new IllegalArgumentException("lines must be between 1 and 500"); - } - } +import jakarta.validation.constraints.Max; +import jakarta.validation.constraints.Min; + +/** + * Tail the last {@code lines} entries from the agent + kiosk journald units. + *
+ * The bound lives on the component as a constraint annotation and nowhere else. + * {@code CommandPayloadCodec} runs Bean Validation on every issued payload, so this is + * what the device actually receives — see {@link CommandPayload}. Null means "agent + * default"; the agent picks its own line count. + */ +public record LogsTailPayload( + @Min(value = 1, message = "lines must be between 1 and 500") + @Max(value = 500, message = "lines must be between 1 and 500") + Integer lines +) implements CommandPayload { @Override public String kind() { return "logs.tail"; } } diff --git a/src/main/java/com/ibrasoft/lensbridge/service/agent/CommandDispatcher.java b/src/main/java/com/ibrasoft/lensbridge/service/agent/CommandDispatcher.java index f4b8899..aaa5232 100644 --- a/src/main/java/com/ibrasoft/lensbridge/service/agent/CommandDispatcher.java +++ b/src/main/java/com/ibrasoft/lensbridge/service/agent/CommandDispatcher.java @@ -13,6 +13,7 @@ import com.ibrasoft.lensbridge.model.minbar.board.DeviceCommand; import com.ibrasoft.lensbridge.model.minbar.board.DeviceCommandStatus; import com.ibrasoft.lensbridge.model.minbar.board.commands.CommandKind; +import com.ibrasoft.lensbridge.model.minbar.board.commands.CommandPayload; import com.ibrasoft.lensbridge.repository.sql.DeviceCommandRepository; import com.ibrasoft.lensbridge.repository.sql.DeviceRepository; import com.ibrasoft.lensbridge.service.agent.events.DeviceEventPublisher; @@ -62,6 +63,7 @@ public class CommandDispatcher { private final AgentSessionRegistry registry; private final ObjectMapper objectMapper; private final DeviceEventPublisher events; + private final CommandPayloadCodec payloadCodec; @Transactional public CommandIssuedResponse issue(UUID deviceId, String issuedBy, IssueCommandRequest request) { @@ -73,10 +75,19 @@ public CommandIssuedResponse issue(UUID deviceId, String issuedBy, IssueCommandR throw new ResponseStatusException(HttpStatus.CONFLICT, "Device is revoked"); } - JsonNode payload = request.payload() == null ? NullNode.getInstance() : request.payload(); + // Bind the wire JSON to the sealed CommandPayload subtype for this kind, validate it, and + // store what came back rather than what was sent. The raw JsonNode used to be stringified + // verbatim, which meant LogsTailPayload's 1..500 bound -- and every other constraint in + // that hierarchy -- was never evaluated on the only path that issues commands: the + // records existed but nothing deserialized into them. Every bound was therefore delegated + // to agent code that does not live in this repository. Re-serializing the validated + // object also drops anything the client tacked on beyond the record's components. String payloadJson; try { + CommandPayload payload = payloadCodec.parse(kind, request.payload()); payloadJson = objectMapper.writeValueAsString(payload); + } catch (ResponseStatusException e) { + throw e; } catch (Exception e) { throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Invalid payload"); } diff --git a/src/main/java/com/ibrasoft/lensbridge/service/agent/CommandPayloadCodec.java b/src/main/java/com/ibrasoft/lensbridge/service/agent/CommandPayloadCodec.java new file mode 100644 index 0000000..dc96c66 --- /dev/null +++ b/src/main/java/com/ibrasoft/lensbridge/service/agent/CommandPayloadCodec.java @@ -0,0 +1,167 @@ +package com.ibrasoft.lensbridge.service.agent; + +import com.fasterxml.jackson.databind.DeserializationFeature; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.ObjectReader; +import com.fasterxml.jackson.databind.node.ObjectNode; +import com.ibrasoft.lensbridge.model.minbar.board.commands.CommandKind; +import com.ibrasoft.lensbridge.model.minbar.board.commands.CommandPayload; +import jakarta.validation.ConstraintViolation; +import jakarta.validation.Validator; +import lombok.extern.slf4j.Slf4j; +import org.springframework.http.HttpStatus; +import org.springframework.stereotype.Component; +import org.springframework.web.server.ResponseStatusException; + +import java.util.Comparator; +import java.util.Set; +import java.util.stream.Collectors; + +/** + * Turns the raw {@code payload} JSON of an issue request into a validated + * {@link CommandPayload}, and back into the JSON that is persisted and pushed to the device. + *
+ * This exists because the issue path used to stringify whatever JSON the operator sent and + * hand it to the kiosk agent verbatim: the sealed {@code CommandPayload} hierarchy — bounds + * and all — was never on that path, so its constraints only looked authoritative. + * Everything a payload can express now has to survive four gates before it leaves the server: + *
+ * The discriminator is stripped on the way out: the frame already carries {@code kind} at
+ * top level, and keeping the stored payload free of it preserves the shape the agent has
+ * always been handed.
+ */
+@Component
+@Slf4j
+public class CommandPayloadCodec {
+
+ /** Discriminator property configured on {@link CommandPayload}'s {@code @JsonTypeInfo}. */
+ private static final String KIND_PROPERTY = "kind";
+
+ private final ObjectMapper objectMapper;
+ private final ObjectReader strictReader;
+ private final Validator validator;
+
+ public CommandPayloadCodec(ObjectMapper objectMapper, Validator validator) {
+ this.objectMapper = objectMapper;
+ this.validator = validator;
+ // A dedicated reader: the shared mapper has FAIL_ON_UNKNOWN_PROPERTIES off, which is
+ // the right default for lenient DTOs and exactly wrong for a payload bound for a device.
+ this.strictReader = objectMapper
+ .reader()
+ .with(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
+ .forType(CommandPayload.class);
+ }
+
+ /**
+ * Parses and validates {@code raw} as the payload for {@code kind}.
+ *
+ * @throws ResponseStatusException 400 with a message safe to hand back to the caller
+ */
+ public CommandPayload parse(CommandKind kind, JsonNode raw) {
+ ObjectNode body = asObject(kind, raw);
+ checkDiscriminator(kind, body);
+ body.put(KIND_PROPERTY, kind.getWireName());
+
+ CommandPayload payload;
+ try {
+ payload = strictReader.readValue(body);
+ } catch (Exception e) {
+ log.warn("Rejected {} payload: {}", kind.getWireName(), e.getMessage());
+ throw badRequest("Invalid payload for command " + kind.getWireName() + ": " + rootCauseMessage(e));
+ }
+
+ // Belt and braces: the Jackson registry and CommandKind are pinned together by
+ // CommandKindTest, so a mismatch here means one of them was edited without the other.
+ if (!kind.getPayloadType().isInstance(payload)) {
+ log.error("Payload for {} deserialized to {}, expected {}",
+ kind.getWireName(), payload.getClass().getSimpleName(), kind.getPayloadType().getSimpleName());
+ throw badRequest("Payload does not match command kind " + kind.getWireName());
+ }
+
+ Set The dispatcher used to stringify {@code IssueCommandRequest.payload()} — a raw
+ * {@code JsonNode} — and hand it to the device verbatim. The sealed {@link CommandPayload}
+ * hierarchy was never deserialized on that path, so {@link LogsTailPayload}'s 1..500 bound and
+ * every other constraint in it were dead code that merely looked authoritative. Every bound was
+ * in practice delegated to agent code that does not live in this repository.
+ *
+ * These tests pin the rule that the sealed hierarchy is now the single source of truth: what
+ * gets stored and forwarded is the re-serialized validated object, not what the client sent.
+ */
+class CommandPayloadCodecTest {
+
+ private ObjectMapper mapper;
+ private CommandPayloadCodec codec;
+
+ @BeforeEach
+ void setUp() {
+ mapper = new ObjectMapper();
+ codec = new CommandPayloadCodec(mapper, Validation.buildDefaultValidatorFactory().getValidator());
+ }
+
+ private JsonNode json(String raw) throws Exception {
+ return mapper.readTree(raw);
+ }
+
+ @Test
+ void acceptsAnInBoundsLogsTailPayload() throws Exception {
+ CommandPayload payload = codec.parse(CommandKind.LOGS_TAIL, json("{\"lines\":200}"));
+
+ assertThat(payload).isInstanceOf(LogsTailPayload.class);
+ assertThat(((LogsTailPayload) payload).lines()).isEqualTo(200);
+ }
+
+ @Test
+ void rejectsTheOutOfBoundsLineCountThatUsedToReachTheDevice() throws Exception {
+ // The exact payload from the audit: accepted and forwarded verbatim before this change.
+ assertThatThrownBy(() -> codec.parse(CommandKind.LOGS_TAIL, json("{\"lines\":999999999}")))
+ .isInstanceOf(ResponseStatusException.class)
+ .hasMessageContaining("lines must be between 1 and 500");
+ }
+
+ @Test
+ void rejectsBoundaryViolationsOnBothSides() throws Exception {
+ assertThatThrownBy(() -> codec.parse(CommandKind.LOGS_TAIL, json("{\"lines\":0}")))
+ .isInstanceOf(ResponseStatusException.class);
+ assertThatThrownBy(() -> codec.parse(CommandKind.LOGS_TAIL, json("{\"lines\":501}")))
+ .isInstanceOf(ResponseStatusException.class);
+ assertThat(((LogsTailPayload) codec.parse(CommandKind.LOGS_TAIL, json("{\"lines\":1}"))).lines())
+ .isEqualTo(1);
+ assertThat(((LogsTailPayload) codec.parse(CommandKind.LOGS_TAIL, json("{\"lines\":500}"))).lines())
+ .isEqualTo(500);
+ }
+
+ @Test
+ void rejectsUnknownPropertiesRatherThanForwardingThem() throws Exception {
+ // The shared ObjectMapper has FAIL_ON_UNKNOWN_PROPERTIES off, which is right for lenient
+ // DTOs and exactly wrong for something bound for a physical device.
+ assertThatThrownBy(() ->
+ codec.parse(CommandKind.LOGS_TAIL, json("{\"lines\":10,\"anything\":\"else\"}")))
+ .isInstanceOf(ResponseStatusException.class);
+ }
+
+ @Test
+ void rejectsAPayloadWhoseDiscriminatorContradictsTheWireKind() throws Exception {
+ assertThatThrownBy(() ->
+ codec.parse(CommandKind.LOGS_TAIL, json("{\"kind\":\"system.reboot\"}")))
+ .isInstanceOf(ResponseStatusException.class);
+ }
+
+ @Test
+ void zeroArgCommandsAcceptAnAbsentOrEmptyPayload() throws Exception {
+ assertThat(codec.parse(CommandKind.CHROME_RELOAD, null)).isNotNull();
+ assertThat(codec.parse(CommandKind.SYSTEM_REBOOT, json("{}"))).isNotNull();
+ assertThat(codec.parse(CommandKind.CHROME_SCREENSHOT, mapper.nullNode())).isNotNull();
+ }
+
+ @Test
+ void rejectsANonObjectPayload() throws Exception {
+ assertThatThrownBy(() -> codec.parse(CommandKind.LOGS_TAIL, json("\"lines=500\"")))
+ .isInstanceOf(ResponseStatusException.class);
+ assertThatThrownBy(() -> codec.parse(CommandKind.LOGS_TAIL, json("[1,2,3]")))
+ .isInstanceOf(ResponseStatusException.class);
+ }
+}
diff --git a/src/test/java/com/ibrasoft/lensbridge/service/agent/DeviceCommandReaperTest.java b/src/test/java/com/ibrasoft/lensbridge/service/agent/DeviceCommandReaperTest.java
index fd83adf..0a988b1 100644
--- a/src/test/java/com/ibrasoft/lensbridge/service/agent/DeviceCommandReaperTest.java
+++ b/src/test/java/com/ibrasoft/lensbridge/service/agent/DeviceCommandReaperTest.java
@@ -1,6 +1,7 @@
package com.ibrasoft.lensbridge.service.agent;
import com.fasterxml.jackson.databind.ObjectMapper;
+import jakarta.validation.Validation;
import com.ibrasoft.lensbridge.model.minbar.board.DeviceCommand;
import com.ibrasoft.lensbridge.model.minbar.board.DeviceCommandStatus;
import com.ibrasoft.lensbridge.repository.sql.DeviceCommandRepository;
@@ -37,9 +38,11 @@ class DeviceCommandReaperTest {
void setUp() {
commandRepo = mock(DeviceCommandRepository.class);
events = mock(DeviceEventPublisher.class);
+ ObjectMapper mapper = new ObjectMapper();
CommandDispatcher dispatcher = new CommandDispatcher(
commandRepo, mock(DeviceRepository.class), new AgentSessionRegistry(),
- new ObjectMapper(), events);
+ mapper, events,
+ new CommandPayloadCodec(mapper, Validation.buildDefaultValidatorFactory().getValidator()));
reaper = new DeviceCommandReaper(commandRepo, dispatcher, events);
when(commandRepo.findByStatusAndExpiresAtBefore(any(), any())).thenReturn(List.of());