fix(formfill,saas,saml): follow-ups from the merged PR review

This commit is contained in:
Anthony Stirling
2026-08-30 00:06:24 +01:00
parent 34694c6f5e
commit 25c0c16461
19 changed files with 294 additions and 47 deletions
+4 -4
View File
@@ -54,10 +54,10 @@ jobs:
- name: Install Task
uses: go-task/setup-task@01a4adf9db2d14c1de7a560f09170b6e0df736aa # v2.1.0
- name: Check Java formatting (Spotless)
# Runs once per matrix combination - pick the cheapest leg
# (core - no proprietary, no saas) so we don't wait for the
# heavier flavors just to fail formatting.
if: matrix.jdk-version == 25 && matrix.flavor == 'core'
# Runs once per matrix combination, on the saas leg because that is the
# only flavor whose settings.gradle includes every module. On the core
# leg :proprietary and :saas are absent, so their Java is never checked.
if: matrix.jdk-version == 25 && matrix.flavor == 'saas'
id: spotless-check
run: task backend:format:check
continue-on-error: true
@@ -14,8 +14,11 @@ import org.opensaml.saml.saml2.core.AuthnStatement;
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
import org.springframework.core.convert.converter.Converter;
import org.springframework.security.core.authority.SimpleGrantedAuthority;
import org.springframework.security.saml2.core.Saml2Error;
import org.springframework.security.saml2.core.Saml2ErrorCodes;
import org.springframework.security.saml2.provider.service.authentication.OpenSaml5AuthenticationProvider.ResponseToken;
import org.springframework.security.saml2.provider.service.authentication.Saml2Authentication;
import org.springframework.security.saml2.provider.service.authentication.Saml2AuthenticationException;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
@@ -64,7 +67,12 @@ public class CustomSaml2ResponseAuthenticationConverter
List<Assertion> assertions = responseToken.getResponse().getAssertions();
if (assertions == null || assertions.isEmpty()) {
log.error("SAML response contains no assertions");
return null;
// Returning null makes ProviderManager report "no provider found" and loses the
// SAML error code the failure handler renders.
throw new Saml2AuthenticationException(
new Saml2Error(
Saml2ErrorCodes.MALFORMED_RESPONSE_DATA,
"No assertions found in response."));
}
Assertion assertion = assertions.getFirst();
Map<String, List<Object>> attributes = extractAttributes(assertion);
@@ -0,0 +1,76 @@
package stirling.software.proprietary.workflow.util;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import org.junit.jupiter.api.Test;
import stirling.software.proprietary.workflow.dto.ParticipantResponse;
import stirling.software.proprietary.workflow.model.WorkflowParticipant;
import tools.jackson.databind.ObjectMapper;
/**
* Locks the wet-signature extraction contract across the Jackson 2 to Jackson 3 move (#7444).
*
* <p>Jackson 3 defaults FAIL_ON_UNKNOWN_PROPERTIES to false, where Jackson 2 defaulted it to true.
* Under Jackson 2 an unrecognised key threw, and the catch-all in {@code extractWetSignatures}
* discarded every signature for that participant. These tests pin the current behaviour so a future
* mapper built with FAIL_ON_UNKNOWN_PROPERTIES re-enabled cannot silently reintroduce that loss.
*/
class WorkflowMapperWetSignatureTest {
private final ObjectMapper objectMapper = new ObjectMapper();
private WorkflowParticipant participantWithMetadata(Map<String, Object> signature) {
Map<String, Object> metadata = new HashMap<>();
metadata.put("wetSignatures", List.of(signature));
WorkflowParticipant participant = new WorkflowParticipant();
participant.setId(7L);
participant.setParticipantMetadata(metadata);
return participant;
}
private Map<String, Object> signature() {
Map<String, Object> signature = new HashMap<>();
signature.put("type", "draw");
signature.put("data", "data:image/png;base64,AAAA");
signature.put("page", 2);
signature.put("x", 10.5);
signature.put("y", 20.5);
signature.put("width", 100.0);
signature.put("height", 40.0);
return signature;
}
@Test
void extractsAKnownSignature() {
ParticipantResponse response =
WorkflowMapper.toParticipantResponse(
participantWithMetadata(signature()), objectMapper, false);
assertNotNull(response);
assertEquals(1, response.getWetSignatures().size());
assertEquals("draw", response.getWetSignatures().getFirst().getType());
assertEquals(2, response.getWetSignatures().getFirst().getPage());
}
@Test
void keepsSignaturesWhenMetadataCarriesAnUnknownKey() {
Map<String, Object> signature = signature();
signature.put("unknownKeyFromAnOlderClient", "ignored");
ParticipantResponse response =
WorkflowMapper.toParticipantResponse(
participantWithMetadata(signature), objectMapper, false);
assertNotNull(response);
assertEquals(1, response.getWetSignatures().size());
assertEquals("draw", response.getWetSignatures().getFirst().getType());
}
}
@@ -26,9 +26,6 @@ import org.springframework.web.bind.annotation.RestController;
import org.springframework.web.server.ResponseStatusException;
import org.springframework.web.servlet.mvc.method.annotation.StreamingResponseBody;
import tools.jackson.core.JacksonException;
import tools.jackson.databind.ObjectMapper;
import io.swagger.v3.oas.annotations.Hidden;
import io.swagger.v3.oas.annotations.tags.Tag;
@@ -53,6 +50,9 @@ import stirling.software.saas.payg.model.JobSource;
import stirling.software.saas.payg.model.ProcessType;
import stirling.software.saas.util.AuthenticationUtils;
import tools.jackson.core.JacksonException;
import tools.jackson.databind.ObjectMapper;
@RestController
@Profile("saas")
@RequestMapping("/api/v1/ai/create")
@@ -14,9 +14,6 @@ import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;
import org.springframework.web.server.ResponseStatusException;
import tools.jackson.core.JacksonException;
import tools.jackson.databind.ObjectMapper;
import io.swagger.v3.oas.annotations.Hidden;
import io.swagger.v3.oas.annotations.tags.Tag;
@@ -29,6 +26,9 @@ import stirling.software.saas.ai.service.AiCreateSessionService;
import stirling.software.saas.payg.cap.RequiresFeature;
import stirling.software.saas.payg.model.FeatureGate;
import tools.jackson.core.JacksonException;
import tools.jackson.databind.ObjectMapper;
@RestController
@Profile("saas")
@RequestMapping("/api/v1/ai/create/internal")
@@ -12,12 +12,12 @@ import org.springframework.beans.factory.annotation.Value;
import org.springframework.context.annotation.Profile;
import org.springframework.stereotype.Service;
import tools.jackson.databind.ObjectMapper;
import lombok.extern.slf4j.Slf4j;
import stirling.software.saas.config.SupabaseConfigurationProperties;
import tools.jackson.databind.ObjectMapper;
/**
* Reports per-tenant overage to Stripe Billing Meters via the Supabase {@code meter-usage} Edge
* Function. Only credits consumed above the free tier flow through {@link #reportUsageToStripe}.
@@ -13,13 +13,13 @@ import java.util.regex.Pattern;
import org.springframework.core.io.ClassPathResource;
import org.springframework.stereotype.Service;
import tools.jackson.databind.JsonNode;
import tools.jackson.databind.ObjectMapper;
import jakarta.annotation.PostConstruct;
import lombok.extern.slf4j.Slf4j;
import tools.jackson.databind.JsonNode;
import tools.jackson.databind.ObjectMapper;
/**
* Loads the versioned legal-document registry from {@code legal/manifest.json} on startup and
* serves document metadata + rendered markdown from the classpath.
@@ -63,8 +63,7 @@ public class LegalDocumentRegistry {
d.path("parts"),
objectMapper
.getTypeFactory()
.constructCollectionType(
List.class, String.class));
.constructCollectionType(List.class, String.class));
documents.put(
id,
new LegalDocumentMeta(
@@ -20,8 +20,6 @@ import org.springframework.stereotype.Component;
import org.springframework.web.method.HandlerMethod;
import org.springframework.web.servlet.HandlerInterceptor;
import tools.jackson.databind.ObjectMapper;
import io.micrometer.core.instrument.Counter;
import io.micrometer.core.instrument.MeterRegistry;
@@ -40,6 +38,9 @@ import stirling.software.saas.payg.cap.RequiresFeature;
import stirling.software.saas.payg.model.FeatureGate;
import stirling.software.saas.util.AuthenticationUtils;
import tools.jackson.core.JacksonException;
import tools.jackson.databind.ObjectMapper;
/**
* Hot-path entitlement check. Runs after {@code PaygChargeInterceptor} in the MVC chain and short-
* circuits the request before any handler work happens when the team's snapshot is missing one of
@@ -357,9 +358,11 @@ public class EntitlementGuard implements HandlerInterceptor {
response.setHeader(HttpHeaders.CONTENT_LENGTH, Integer.toString(payload.length));
response.getOutputStream().write(payload);
response.getOutputStream().flush();
} catch (IOException e) {
} catch (IOException | JacksonException e) {
// Container will fall back to its default error page — we did set the status code,
// so the client still sees the right HTTP code even if the body fails to write.
// JacksonException is unchecked and not an IOException, so serialization failures
// would otherwise escape preHandle and turn a 402 into a 500.
log.warn("EntitlementGuard write response body failed", e);
errorsCounter.increment();
}
@@ -18,8 +18,6 @@ import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;
import tools.jackson.databind.ObjectMapper;
import io.swagger.v3.oas.annotations.Hidden;
import jakarta.servlet.http.HttpServletRequest;
@@ -43,6 +41,8 @@ import stirling.software.saas.procurement.pricing.QuoteLineItem;
import stirling.software.saas.procurement.service.ProcurementService;
import stirling.software.saas.util.AuthenticationUtils;
import tools.jackson.databind.ObjectMapper;
/**
* The enterprise procurement journey for a linked team: read the deal snapshot, start/extend a
* (mock-licensed) trial, build a server-priced quote, and accept it. Stripe checkout itself is a
@@ -10,8 +10,6 @@ import java.util.Map;
import org.springframework.stereotype.Service;
import tools.jackson.databind.ObjectMapper;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
@@ -22,6 +20,8 @@ import stirling.software.saas.procurement.pricing.ProcurementPricingService;
import stirling.software.saas.procurement.pricing.QuoteConfig;
import stirling.software.saas.procurement.pricing.QuoteLineItem;
import tools.jackson.databind.ObjectMapper;
/**
* Builds the full Stirling Enterprise Agreement for a specific quote: the static MSA (Part A) and
* DPA (Part C) from the {@link LegalDocumentRegistry}, with the dynamic Order Form (Part B)
@@ -16,13 +16,13 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
import org.springframework.context.annotation.Profile;
import org.springframework.stereotype.Service;
import tools.jackson.databind.JsonNode;
import tools.jackson.databind.ObjectMapper;
import lombok.extern.slf4j.Slf4j;
import stirling.software.saas.procurement.config.KeygenConfigurationProperties;
import tools.jackson.databind.JsonNode;
import tools.jackson.databind.ObjectMapper;
/**
* Real {@link EnterpriseLicenseService}: manages the team's enterprise licence directly against the
* Keygen API (the "call Keygen from Java" direction), rather than via the Supabase edge functions
@@ -10,9 +10,6 @@ import org.springframework.context.annotation.Profile;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
import tools.jackson.core.JacksonException;
import tools.jackson.databind.ObjectMapper;
import lombok.extern.slf4j.Slf4j;
import stirling.software.common.model.enumeration.TeamRole;
@@ -38,6 +35,9 @@ import stirling.software.saas.procurement.repository.ProcurementQuoteRepository;
import stirling.software.saas.service.SaasTeamService;
import stirling.software.saas.util.LogRedactionUtils;
import tools.jackson.core.JacksonException;
import tools.jackson.databind.ObjectMapper;
/**
* Orchestrates a linked team's procurement journey: start a (mock-licensed) trial, build a
* server-priced quote, and accept it. Stripe checkout itself lives in a Supabase edge function the
@@ -28,9 +28,6 @@ import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.security.oauth2.jwt.Jwt;
import org.springframework.web.method.HandlerMethod;
import tools.jackson.databind.JsonNode;
import tools.jackson.databind.ObjectMapper;
import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
@@ -45,6 +42,9 @@ import stirling.software.saas.payg.model.FeatureGate;
import stirling.software.saas.payg.model.FeatureSet;
import stirling.software.saas.security.EnhancedJwtAuthenticationToken;
import tools.jackson.databind.JsonNode;
import tools.jackson.databind.ObjectMapper;
/**
* Pure-Mockito tests for {@link EntitlementGuard}. Covers the four decision-matrix cells: anonymous
* billable → 401, anonymous manual → pass, authenticated FULL → pass, authenticated DEGRADED for a
@@ -18,6 +18,7 @@ import {
import { useTranslation } from "react-i18next";
import { useFieldValue } from "@app/tools/formFill/FormFillContext";
import type { FormField } from "@app/tools/formFill/types";
import { isFieldChecked } from "@app/tools/formFill/checkboxState";
function FieldInputInner({
field,
@@ -69,9 +70,7 @@ function FieldInputInner({
case "checkbox": {
const exportVal = field.widgets && field.widgets[0]?.exportValue;
const isChecked = exportVal
? value === exportVal || value === "Yes"
: !!value && value !== "Off";
const isChecked = isFieldChecked(field.widgets, value);
const onValue = exportVal || "Yes";
return (
<Checkbox
@@ -29,6 +29,7 @@ import type {
WidgetCoordinates,
ButtonAction,
} from "@app/tools/formFill/types";
import { isWidgetChecked } from "@app/tools/formFill/checkboxState";
/**
* Execute PDF JavaScript in a minimally sandboxed context.
@@ -338,10 +339,7 @@ function WidgetInputInner({
);
case "checkbox": {
// Checkbox is checked when value matches exportValue if present, or is non-empty and not 'Off'
const isChecked = widget.exportValue
? value === widget.exportValue || value === "Yes"
: !!value && value !== "Off";
const isChecked = isWidgetChecked(widget, value);
// When toggling on, use the widget's exportValue (e.g. 'Red', 'Blue', 'Pass') or fall back to 'Yes'
const onValue = widget.exportValue || "Yes";
return (
@@ -106,7 +106,10 @@ export function FormSaveBar({
...styles,
position: "absolute",
top: "1rem",
left: "1rem",
right: "1rem",
display: "flex",
justifyContent: "flex-end",
zIndex: 100,
pointerEvents: "none",
}}
@@ -117,10 +120,9 @@ export function FormSaveBar({
withBorder
style={{
pointerEvents: "auto",
// Fill the viewport on small screens instead of overflowing the
// left edge with a fixed 320px+ minimum width.
width: "min(420px, calc(100vw - 2rem))",
maxWidth: "100%",
// Resolved against the PDF pane, not the viewport: the pane is
// inset by the tool rail and any open sidebar, and clips overflow.
width: "min(420px, 100%)",
overflow: "hidden",
}}
>
@@ -0,0 +1,70 @@
import { describe, expect, it } from "vitest";
import {
isFieldChecked,
isGenericOn,
isWidgetChecked,
} from "@app/tools/formFill/checkboxState";
describe("isWidgetChecked", () => {
it("ticks only the widget whose export value matches", () => {
expect(isWidgetChecked({ exportValue: "Red" }, "Red")).toBe(true);
expect(isWidgetChecked({ exportValue: "Blue" }, "Red")).toBe(false);
});
it("accepts the on-state spellings the backend accepts", () => {
for (const value of ["Yes", "true", "1", "on", "checked", "CHECKED"]) {
expect(isWidgetChecked({ exportValue: "Red" }, value)).toBe(true);
}
});
it("falls back to non-Off when the widget has no export value", () => {
expect(isWidgetChecked({ exportValue: null }, "Yes")).toBe(true);
expect(isWidgetChecked({ exportValue: null }, "Off")).toBe(false);
expect(isWidgetChecked({ exportValue: null }, "off")).toBe(false);
expect(isWidgetChecked({ exportValue: null }, "")).toBe(false);
});
it("treats a missing value as unchecked", () => {
expect(isWidgetChecked({ exportValue: "Red" }, undefined)).toBe(false);
expect(isWidgetChecked(undefined, null)).toBe(false);
});
});
describe("isFieldChecked", () => {
const widgets = [{ exportValue: "Red" }, { exportValue: "Blue" }];
it("ticks when any kid matches, not only the first", () => {
expect(isFieldChecked(widgets, "Blue")).toBe(true);
expect(isFieldChecked(widgets, "Red")).toBe(true);
});
it("stays unticked for a state no kid carries", () => {
expect(isFieldChecked(widgets, "Green")).toBe(false);
expect(isFieldChecked(widgets, "Off")).toBe(false);
});
it("accepts legacy on-values against a field that has export values", () => {
expect(isFieldChecked(widgets, "true")).toBe(true);
});
it("falls back to non-Off when no kid declares an export value", () => {
expect(isFieldChecked([{ exportValue: null }], "Yes")).toBe(true);
expect(isFieldChecked([{ exportValue: null }], "Off")).toBe(false);
});
});
describe("isGenericOn", () => {
it("is false for empty, whitespace and Off in any case", () => {
expect(isGenericOn("")).toBe(false);
expect(isGenericOn(" ")).toBe(false);
expect(isGenericOn("Off")).toBe(false);
expect(isGenericOn("OFF")).toBe(false);
expect(isGenericOn(null)).toBe(false);
});
it("is true for any other non-empty state", () => {
expect(isGenericOn("Yes")).toBe(true);
expect(isGenericOn("Red")).toBe(true);
});
});
@@ -0,0 +1,60 @@
/**
* One definition of "is this checkbox on", shared by the sidebar row, the page
* overlay and the PDFium save path.
*
* Must stay in sync with FormUtils.isChecked (app/common) — the backend accepts
* these spellings regardless of the widget's appearance state, so a UI that only
* accepted the widget's own export value would render unticked boxes that the
* backend still saves ticked.
*/
const LEGACY_ON_VALUES = new Set(["yes", "true", "1", "on", "checked"]);
/** The PDF "off" appearance state. Compared case-insensitively, as the backend does. */
const OFF_STATE = "off";
interface WidgetOnState {
exportValue?: string | null;
}
function isLegacyOn(value: string): boolean {
return LEGACY_ON_VALUES.has(value.trim().toLowerCase());
}
/** True when a field value that names no export state still means "on". */
export function isGenericOn(value: string | null | undefined): boolean {
if (!value) return false;
const trimmed = value.trim();
return trimmed !== "" && trimmed.toLowerCase() !== OFF_STATE;
}
/**
* Whether one widget renders ticked for the field's current value.
*
* A checkbox field's kid widgets can carry different on-states, so the answer is
* per widget: only the kid whose export value matches is ticked.
*/
export function isWidgetChecked(
widget: WidgetOnState | null | undefined,
value: string | null | undefined,
): boolean {
if (!value) return false;
const exportValue = widget?.exportValue;
if (exportValue) return value === exportValue || isLegacyOn(value);
return isGenericOn(value);
}
/**
* Whether a field reads as ticked anywhere, for UI that has no widget in hand.
*
* Matches against every kid rather than the first: a field whose checked kid is
* not kid 0 would otherwise render unticked.
*/
export function isFieldChecked(
widgets: ReadonlyArray<WidgetOnState> | null | undefined,
value: string | null | undefined,
): boolean {
if (!value) return false;
if (widgets?.some((w) => isWidgetChecked(w, value))) return true;
const hasExportValues = widgets?.some((w) => Boolean(w.exportValue));
return hasExportValues ? isLegacyOn(value) : isGenericOn(value);
}
@@ -14,6 +14,7 @@
* for both providers.
*/
import { PDF_FORM_FIELD_TYPE } from "@app/services/pdfiumService";
import type { WrappedPdfiumModule } from "@embedpdf/pdfium";
import { FPDF_ANNOT_WIDGET, FLAT_PRINT } from "@app/utils/pdfiumBitmapUtils";
import type {
FormField,
@@ -23,6 +24,26 @@ import type {
} from "@app/tools/formFill/types";
import type { IFormDataProvider } from "@app/tools/formFill/providers/types";
import type { PDFDict } from "@cantoo/pdf-lib";
import { isWidgetChecked } from "@app/tools/formFill/checkboxState";
/** Reads one widget annotation's /AP on-state name, or null when it has none. */
function readAnnotExportValue(
m: WrappedPdfiumModule,
formEnvPtr: number,
annotPtr: number,
): string | null {
try {
const len = m.FPDFAnnot_GetFormFieldExportValue(formEnvPtr, annotPtr, 0, 0);
if (len <= 0) return null;
const buf = m.pdfium.wasmExports.malloc(len);
m.FPDFAnnot_GetFormFieldExportValue(formEnvPtr, annotPtr, buf, len);
const out = readUtf16(m, buf, len);
m.pdfium.wasmExports.free(buf);
return out || null;
} catch {
return null;
}
}
interface PDFAcroField {
dict: PDFDict;
@@ -92,7 +113,8 @@ function toFormField(
// Derive value string
let value = f.value;
if (type === "checkbox") {
value = f.isChecked ? f.widgets[0]?.exportValue || "Yes" : "Off";
const checkedWidget = f.widgets.find((w) => w.isChecked) ?? f.widgets[0];
value = f.isChecked ? checkedWidget?.exportValue || "Yes" : "Off";
} else if (type === "radio") {
// Use widget index as the canonical radio value.
// This avoids issues with duplicate exportValues across widgets
@@ -615,7 +637,17 @@ export class PdfiumFormProvider implements IFormDataProvider {
formEnvPtr,
annotPtr,
);
const shouldBeChecked = value !== "" && value !== "Off";
// Per widget, not per field: a checkbox field's kid widgets can
// carry different on-states, and only the matching kid is ticked.
const widgetExportValue = readAnnotExportValue(
m,
formEnvPtr,
annotPtr,
);
const shouldBeChecked = isWidgetChecked(
{ exportValue: widgetExportValue },
value,
);
if (isCurrentlyChecked !== shouldBeChecked) {
const ENTER_KEY = 13;
m.FORM_SetFocusedAnnot(formEnvPtr, annotPtr);