From b3593701ce16ef92afdb067c933169ab1734ebb0 Mon Sep 17 00:00:00 2001 From: Charles Graham Date: Wed, 8 Apr 2026 21:26:14 -0500 Subject: [PATCH 1/6] Create ErrorTraceSupport class to handle sending back stacktraces via response --- .../src/main/java/cwms/cda/ApiServlet.java | 18 +- .../cda/api/errors/ErrorTraceSupport.java | 161 ++++++++++++++++++ .../cda/api/errors/ErrorTraceSupportTest.java | 124 ++++++++++++++ 3 files changed, 295 insertions(+), 8 deletions(-) create mode 100644 cwms-data-api/src/main/java/cwms/cda/api/errors/ErrorTraceSupport.java create mode 100644 cwms-data-api/src/test/java/cwms/cda/api/errors/ErrorTraceSupportTest.java diff --git a/cwms-data-api/src/main/java/cwms/cda/ApiServlet.java b/cwms-data-api/src/main/java/cwms/cda/ApiServlet.java index 34207341f..8d26760a6 100644 --- a/cwms-data-api/src/main/java/cwms/cda/ApiServlet.java +++ b/cwms-data-api/src/main/java/cwms/cda/ApiServlet.java @@ -106,6 +106,7 @@ import cwms.cda.api.enums.UnitSystem; import cwms.cda.api.errors.ApplicationException; import cwms.cda.api.errors.CdaError; +import cwms.cda.api.errors.ErrorTraceSupport; import cwms.cda.api.location.kind.GateChangeCreateController; import cwms.cda.api.location.kind.GateChangeDeleteController; import cwms.cda.api.location.kind.GateChangeGetAllController; @@ -355,31 +356,32 @@ public void init() { ctx.header("X-XSS-Protection", "1; mode=block"); }) .exception(ApplicationException.class, (e, ctx) -> { - CdaError re = new CdaError(e.getCdaErrorMessage(), e.getSource(), e.getDetails()); + CdaError re = ErrorTraceSupport.buildError(ctx, e.getCdaErrorMessage(), + e.getSource(), e.getDetails(), e); if (e.getLoggerLevel().isPresent()) { logger.at(e.getLoggerLevel().get()).withCause(e).log(re.toString()); } ctx.status(e.getCdaHttpErrorCode()).json(re); }) .exception(UnsupportedOperationException.class, (e, ctx) -> { - final CdaError re = CdaError.notImplemented(); + final CdaError re = ErrorTraceSupport.buildError(ctx, "Not Implemented", e); logger.atWarning().withCause(e) .log("%s for request: %s", re, ctx.fullUrl()); ctx.status(HttpServletResponse.SC_NOT_IMPLEMENTED).json(re); }) .exception(BadRequestResponse.class, (e, ctx) -> { - CdaError re = new CdaError("Bad Request", - "User Input", new HashMap<>(e.getDetails())); + CdaError re = ErrorTraceSupport.buildError(ctx, "Bad Request", + "User Input", new HashMap<>(e.getDetails()), e); logger.atInfo().withCause(e).log(re.toString()); ctx.status(e.getStatus()).json(re); }) .exception(IllegalArgumentException.class, (e, ctx) -> { - CdaError re = new CdaError("Bad Request"); + CdaError re = ErrorTraceSupport.buildError(ctx, "Bad Request", e); logger.atInfo().withCause(e).log(re.toString()); ctx.status(HttpServletResponse.SC_BAD_REQUEST).json(re); }) .exception(DateTimeException.class, (e, ctx) -> { - CdaError re = new CdaError(e.getMessage()); + CdaError re = ErrorTraceSupport.buildError(ctx, e.getMessage(), e); ctx.status(HttpServletResponse.SC_BAD_REQUEST).json(re); }) .exception(DataAccessException.class, (e, ctx) -> { @@ -393,7 +395,7 @@ public void init() { // CdaError does not include the Oracle exception message b/c this block catches // all unhandled DataAccessExceptions and we don't know what is in the message // it is unknown if the message would be safe/appropriate for users to see. - CdaError errResponse = new CdaError("Database Error"); + CdaError errResponse = ErrorTraceSupport.buildError(ctx, "Database Error", e); logger.atWarning().withCause(e).log("error on request[%s]: %s", errResponse.getIncidentIdentifier(), ctx.req.getRequestURI()); ctx.status(500); @@ -401,7 +403,7 @@ public void init() { ctx.json(errResponse); }) .exception(Exception.class, (e, ctx) -> { - CdaError errResponse = new CdaError("System Error"); + CdaError errResponse = ErrorTraceSupport.buildError(ctx, "System Error", e); logger.atWarning().withCause(e).log("error on request[%s]: %s", errResponse.getIncidentIdentifier(), ctx.req.getRequestURI()); ctx.status(500); diff --git a/cwms-data-api/src/main/java/cwms/cda/api/errors/ErrorTraceSupport.java b/cwms-data-api/src/main/java/cwms/cda/api/errors/ErrorTraceSupport.java new file mode 100644 index 000000000..fbc7525e4 --- /dev/null +++ b/cwms-data-api/src/main/java/cwms/cda/api/errors/ErrorTraceSupport.java @@ -0,0 +1,161 @@ +package cwms.cda.api.errors; + +import cwms.cda.data.dao.AuthDao; +import cwms.cda.security.DataApiPrincipal; +import cwms.cda.security.Role; +import cwms.cda.spi.IdentityProvider; +import io.javalin.http.Context; +import java.io.PrintWriter; +import java.io.Serializable; +import java.io.StringWriter; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; + +public final class ErrorTraceSupport { + public static final String STACK_TRACE_KEY = "stackTrace"; + static final String ALWAYS_SHOW_STACK_TRACE_PROPERTY = "cwms.dataapi.errors.alwaysShowStackTrace"; + static final String ALWAYS_SHOW_STACK_TRACE_VARIABLE = "CWMS_DATAAPI_ERRORS_ALWAYS_SHOW_STACK_TRACE"; + static final String PRIMARY_ENVIRONMENT_PROPERTY = "cwms.dataapi.environment.name"; + static final String PRIMARY_ENVIRONMENT_VARIABLE = "CWMS_DATAAPI_ENVIRONMENT_NAME"; + static final String HOST_ENVIRONMENT_VARIABLE = "ENVIRONMENT"; + static final String LEGACY_ENVIRONMENT_PROPERTY = "cda.environment.name"; + static final String LEGACY_ENVIRONMENT_VARIABLE = "CDA_ENVIRONMENT_NAME"; + static final String WAR_CONTEXT_PROPERTY = "warContext"; + private static final String DEV_MARKER = "dev"; + private static final Role CWMS_USER_ADMINS_ROLE = new Role("CWMS User Admins"); + + private ErrorTraceSupport() { + } + + public static CdaError buildError(Context ctx, String message, Throwable cause) { + Map details = buildDetails(ctx, Collections.emptyMap(), cause); + if (details.isEmpty()) { + return new CdaError(message); + } + return new CdaError(message, details); + } + + public static CdaError buildError(Context ctx, String message, String source, + Map details, Throwable cause) { + return new CdaError(message, source, buildDetails(ctx, details, cause)); + } + + static Map buildDetails(Context ctx, Map details, + Throwable cause) { + return buildDetails(details, cause, shouldIncludeStackTrace(ctx)); + } + + static Map buildDetails(Map details, + Throwable cause, boolean includeStackTrace) { + Map merged = new HashMap<>(); + if (details != null) { + merged.putAll(details); + } + if (cause != null && includeStackTrace) { + merged.put(STACK_TRACE_KEY, stackTraceOf(cause)); + } + return Collections.unmodifiableMap(merged); + } + + static boolean shouldIncludeStackTrace(Context ctx) { + if (alwaysShowStackTraceOverrideEnabled()) { + return true; + } + if (localhostRequestOverrideEnabled(ctx)) { + return true; + } + return shouldIncludeStackTrace(resolvePrincipal(ctx).orElse(null), resolveEnvironmentName(ctx)); + } + + static boolean shouldIncludeStackTrace(DataApiPrincipal principal, String environmentName) { + if (alwaysShowStackTraceOverrideEnabled()) { + return true; + } + return hasAdminRole(principal) && environmentLooksLikeDev(environmentName); + } + + static boolean alwaysShowStackTraceOverrideEnabled() { + return Boolean.parseBoolean(firstNonBlank( + System.getProperty(ALWAYS_SHOW_STACK_TRACE_PROPERTY), + System.getenv(ALWAYS_SHOW_STACK_TRACE_VARIABLE), + "false" + )); + } + + static boolean localhostRequestOverrideEnabled(Context ctx) { + if (ctx == null || ctx.req == null) { + return false; + } + return localhostRequestOverrideEnabled(ctx.req.getServerName()); + } + + static boolean localhostRequestOverrideEnabled(String serverName) { + return serverName != null + && ("localhost".equalsIgnoreCase(serverName) + || "127.0.0.1".equals(serverName) + || "::1".equals(serverName)); + } + + static Optional resolvePrincipal(Context ctx) { + if (ctx == null) { + return Optional.empty(); + } + DataApiPrincipal attributePrincipal = ctx.attribute(AuthDao.DATA_API_PRINCIPAL); + if (attributePrincipal != null) { + return Optional.of(attributePrincipal); + } + DataApiPrincipal sessionPrincipal = ctx.sessionAttribute(IdentityProvider.PRINCIPAL_KEY); + return Optional.ofNullable(sessionPrincipal); + } + + static boolean hasAdminRole(DataApiPrincipal principal) { + return principal != null + && principal.getRoles().contains(CWMS_USER_ADMINS_ROLE); + } + + static String resolveEnvironmentName(Context ctx) { + String configuredName = firstNonBlank( + System.getProperty(PRIMARY_ENVIRONMENT_PROPERTY), + System.getenv(PRIMARY_ENVIRONMENT_VARIABLE), + System.getenv(HOST_ENVIRONMENT_VARIABLE), + System.getProperty(LEGACY_ENVIRONMENT_PROPERTY), + System.getenv(LEGACY_ENVIRONMENT_VARIABLE), + System.getProperty(WAR_CONTEXT_PROPERTY) + ); + if (configuredName != null) { + return configuredName; + } + if (ctx == null) { + return ""; + } + return firstNonBlank(ctx.contextPath(), ctx.req != null ? ctx.req.getContextPath() : null, ""); + } + + static boolean environmentLooksLikeDev(String environmentName) { + return normalizeEnvironmentName(environmentName).contains(DEV_MARKER); + } + + static String normalizeEnvironmentName(String environmentName) { + if (environmentName == null) { + return ""; + } + return environmentName.toLowerCase().replaceAll("[^a-z0-9]", ""); + } + + private static String stackTraceOf(Throwable cause) { + StringWriter sw = new StringWriter(); + cause.printStackTrace(new PrintWriter(sw)); + return sw.toString(); + } + + private static String firstNonBlank(String... values) { + for (String value : values) { + if (value != null && !value.isBlank()) { + return value; + } + } + return null; + } +} diff --git a/cwms-data-api/src/test/java/cwms/cda/api/errors/ErrorTraceSupportTest.java b/cwms-data-api/src/test/java/cwms/cda/api/errors/ErrorTraceSupportTest.java new file mode 100644 index 000000000..731999a0d --- /dev/null +++ b/cwms-data-api/src/test/java/cwms/cda/api/errors/ErrorTraceSupportTest.java @@ -0,0 +1,124 @@ +package cwms.cda.api.errors; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import cwms.cda.security.DataApiPrincipal; +import cwms.cda.security.Role; +import java.io.Serializable; +import java.util.Map; +import java.util.Set; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +class ErrorTraceSupportTest { + + @AfterEach + void clearProperties() { + System.clearProperty(ErrorTraceSupport.ALWAYS_SHOW_STACK_TRACE_PROPERTY); + System.clearProperty(ErrorTraceSupport.PRIMARY_ENVIRONMENT_PROPERTY); + System.clearProperty(ErrorTraceSupport.LEGACY_ENVIRONMENT_PROPERTY); + System.clearProperty(ErrorTraceSupport.WAR_CONTEXT_PROPERTY); + } + + @Test + void includesStackTraceForDevAdminUser() { + DataApiPrincipal principal = new DataApiPrincipal("admin-user", + Set.of(new Role("CWMS User Admins"))); + + Map details = ErrorTraceSupport.buildDetails(Map.of(), + new IllegalStateException("boom"), + ErrorTraceSupport.shouldIncludeStackTrace(principal, "CWMS_DEV-West")); + + assertTrue(details.containsKey(ErrorTraceSupport.STACK_TRACE_KEY)); + assertTrue(details.get(ErrorTraceSupport.STACK_TRACE_KEY).toString() + .contains("IllegalStateException")); + } + + @Test + void includesStackTraceForExistingUserAdminsRole() { + DataApiPrincipal principal = new DataApiPrincipal("admin-user", + Set.of(new Role("CWMS User Admins"))); + + assertTrue(ErrorTraceSupport.shouldIncludeStackTrace(principal, "spk-dev")); + } + + @Test + void omitsStackTraceOutsideDev() { + DataApiPrincipal principal = new DataApiPrincipal("admin-user", + Set.of(new Role("CWMS User Admins"))); + + Map details = ErrorTraceSupport.buildDetails(Map.of(), + new IllegalStateException("boom"), + ErrorTraceSupport.shouldIncludeStackTrace(principal, "production")); + + assertFalse(details.containsKey(ErrorTraceSupport.STACK_TRACE_KEY)); + } + + @Test + void omitsStackTraceForNonAdminUser() { + DataApiPrincipal principal = new DataApiPrincipal("normal-user", + Set.of(new Role("CWMS Users"))); + + assertFalse(ErrorTraceSupport.shouldIncludeStackTrace(principal, "dev")); + } + + @Test + void omitsStackTraceForGuestUser() { + assertFalse(ErrorTraceSupport.shouldIncludeStackTrace(null, "dev")); + } + + @Test + void normalizesEnvironmentNameBeforeDevCheck() { + assertTrue(ErrorTraceSupport.environmentLooksLikeDev("CWMS.Dev-West")); + assertTrue(ErrorTraceSupport.environmentLooksLikeDev("cwms_dev_west")); + assertFalse(ErrorTraceSupport.environmentLooksLikeDev("production")); + } + + @Test + void fallsBackToContextPathWhenEnvironmentPropertyMissing() { + assertTrue(ErrorTraceSupport.environmentLooksLikeDev("/spk-data-dev")); + } + + @Test + void environmentPropertyStillResolvesForDevChecks() { + System.setProperty(ErrorTraceSupport.PRIMARY_ENVIRONMENT_PROPERTY, "local-dev"); + + assertTrue(ErrorTraceSupport.environmentLooksLikeDev( + ErrorTraceSupport.resolveEnvironmentName(null))); + } + + @Test + void omitsStackTraceForUndefinedCwmsAdminRole() { + DataApiPrincipal principal = new DataApiPrincipal("admin-user", + Set.of(new Role("CWMS Admin"))); + + assertFalse(ErrorTraceSupport.shouldIncludeStackTrace(principal, "dev")); + } + + @Test + void overrideEnablesStackTraceWithoutAdminRole() { + DataApiPrincipal principal = new DataApiPrincipal("normal-user", + Set.of(new Role("CWMS Users"))); + System.setProperty(ErrorTraceSupport.ALWAYS_SHOW_STACK_TRACE_PROPERTY, "true"); + + assertTrue(ErrorTraceSupport.shouldIncludeStackTrace(principal, "production")); + } + + @Test + void overrideAddsStackTraceToDetails() { + System.setProperty(ErrorTraceSupport.ALWAYS_SHOW_STACK_TRACE_PROPERTY, "true"); + + Map details = ErrorTraceSupport.buildDetails(Map.of(), + new IllegalStateException("boom"), + ErrorTraceSupport.shouldIncludeStackTrace(null, "production")); + + assertTrue(details.containsKey(ErrorTraceSupport.STACK_TRACE_KEY)); + } + + @Test + void localhostRequestEnablesStackTraceWithoutAuth() { + assertTrue(ErrorTraceSupport.localhostRequestOverrideEnabled("localhost")); + assertTrue(ErrorTraceSupport.localhostRequestOverrideEnabled("127.0.0.1")); + assertFalse(ErrorTraceSupport.localhostRequestOverrideEnabled("example.com")); + } +} From 982fa8a35c925f01ad9bb67881012a3085ffda25 Mon Sep 17 00:00:00 2001 From: Charles Graham Date: Wed, 8 Apr 2026 21:46:36 -0500 Subject: [PATCH 2/6] Cleanup endpoint specific errors to use ErrorTraceSupport, preserving behavior and keeping it consistent --- .../java/cwms/cda/api/BasinController.java | 3 ++- .../cda/api/BinaryTimeSeriesController.java | 5 +++-- .../java/cwms/cda/api/LocationController.java | 13 +++++------- .../cda/api/TextTimeSeriesController.java | 5 +++-- .../cwms/cda/api/TimeSeriesController.java | 5 +++-- ...eSeriesIdentifierDescriptorController.java | 3 ++- .../cwms/cda/api/rating/RatingController.java | 21 ++++++++++++------- 7 files changed, 31 insertions(+), 24 deletions(-) diff --git a/cwms-data-api/src/main/java/cwms/cda/api/BasinController.java b/cwms-data-api/src/main/java/cwms/cda/api/BasinController.java index c8bdf4f4a..d66174f98 100644 --- a/cwms-data-api/src/main/java/cwms/cda/api/BasinController.java +++ b/cwms-data-api/src/main/java/cwms/cda/api/BasinController.java @@ -42,6 +42,7 @@ import com.codahale.metrics.Timer; import cwms.cda.api.enums.UnitSystem; import cwms.cda.api.errors.CdaError; +import cwms.cda.api.errors.ErrorTraceSupport; import cwms.cda.data.dao.JooqDao; import cwms.cda.data.dao.basinconnectivity.BasinDao; import cwms.cda.data.dto.CwmsId; @@ -143,7 +144,7 @@ public void getAll(@NotNull Context ctx) { ctx.result(result); ctx.status(HttpServletResponse.SC_OK); } catch (SQLException ex) { - CdaError error = new CdaError("Error retrieving all basins"); + CdaError error = ErrorTraceSupport.buildError(ctx, "Error retrieving all basins", ex); LOGGER.atSevere().withCause(ex).log("Error retrieving all basins"); ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(error); } diff --git a/cwms-data-api/src/main/java/cwms/cda/api/BinaryTimeSeriesController.java b/cwms-data-api/src/main/java/cwms/cda/api/BinaryTimeSeriesController.java index 8f6f1d0e1..980151ac9 100644 --- a/cwms-data-api/src/main/java/cwms/cda/api/BinaryTimeSeriesController.java +++ b/cwms-data-api/src/main/java/cwms/cda/api/BinaryTimeSeriesController.java @@ -45,6 +45,7 @@ import com.codahale.metrics.MetricRegistry; import com.codahale.metrics.Timer; import cwms.cda.api.errors.CdaError; +import cwms.cda.api.errors.ErrorTraceSupport; import cwms.cda.data.dao.binarytimeseries.TimeSeriesBinaryDao; import cwms.cda.data.dto.binarytimeseries.BinaryTimeSeries; import cwms.cda.formatters.ContentType; @@ -163,8 +164,8 @@ public void getAll(@NotNull Context ctx) { ctx.status(HttpServletResponse.SC_OK); } catch (URISyntaxException | UnsupportedEncodingException ex) { - CdaError re = - new CdaError("Failed to process request: " + ex.getLocalizedMessage()); + CdaError re = ErrorTraceSupport.buildError(ctx, + "Failed to process request: " + ex.getLocalizedMessage(), ex); logger.atSevere().withCause(ex).log("%s", re); ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } diff --git a/cwms-data-api/src/main/java/cwms/cda/api/LocationController.java b/cwms-data-api/src/main/java/cwms/cda/api/LocationController.java index f1610e113..13188ea6f 100644 --- a/cwms-data-api/src/main/java/cwms/cda/api/LocationController.java +++ b/cwms-data-api/src/main/java/cwms/cda/api/LocationController.java @@ -43,6 +43,7 @@ import cwms.cda.api.enums.UnitSystem; import cwms.cda.api.errors.CdaError; import cwms.cda.api.errors.DeleteConflictException; +import cwms.cda.api.errors.ErrorTraceSupport; import cwms.cda.data.dao.LocationsDao; import cwms.cda.data.dao.LocationsDaoImpl; import cwms.cda.data.dto.Location; @@ -200,10 +201,6 @@ public void getAll(@NotNull Context ctx) { ctx.status(HttpServletResponse.SC_OK); - } catch (Exception ex) { - CdaError re = new CdaError("failed to process request"); - logger.atSevere().withCause(ex).log("%s", re.toString()); - ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } } @@ -261,7 +258,7 @@ public void getOne(@NotNull Context ctx, @NotNull String locationId) { addDeprecatedContentTypeWarning(ctx, contentType); } catch (IOException ex) { String errorMsg = "Error retrieving " + locationId; - CdaError re = new CdaError(errorMsg); + CdaError re = ErrorTraceSupport.buildError(ctx, errorMsg, ex); ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); logger.atSevere().withCause(ex).log("%s", errorMsg); } @@ -303,7 +300,7 @@ public void create(@NotNull Context ctx) { "Created Location", locationFromBody.getName()); ctx.status(HttpServletResponse.SC_CREATED).json(re); } catch (IOException ex) { - CdaError re = new CdaError("failed to process request"); + CdaError re = ErrorTraceSupport.buildError(ctx, "failed to process request", ex); logger.atSevere().withCause(ex).log("%s", re.toString()); ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } @@ -357,8 +354,8 @@ public void update(@NotNull Context ctx, @NotNull String locationId) { "Updated Location", updatedLocation.getName())); } } catch (IOException ex) { - CdaError re = - new CdaError("Failed to process request: " + ex.getLocalizedMessage()); + CdaError re = ErrorTraceSupport.buildError(ctx, + "Failed to process request: " + ex.getLocalizedMessage(), ex); logger.atSevere().withCause(ex).log("%s", re.toString()); ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } diff --git a/cwms-data-api/src/main/java/cwms/cda/api/TextTimeSeriesController.java b/cwms-data-api/src/main/java/cwms/cda/api/TextTimeSeriesController.java index 43893dd3a..521f4e945 100644 --- a/cwms-data-api/src/main/java/cwms/cda/api/TextTimeSeriesController.java +++ b/cwms-data-api/src/main/java/cwms/cda/api/TextTimeSeriesController.java @@ -30,6 +30,7 @@ import com.codahale.metrics.MetricRegistry; import com.codahale.metrics.Timer; import cwms.cda.api.errors.CdaError; +import cwms.cda.api.errors.ErrorTraceSupport; import cwms.cda.data.dao.texttimeseries.TimeSeriesTextDao; import cwms.cda.data.dto.texttimeseries.TextTimeSeries; import cwms.cda.formatters.ContentType; @@ -144,8 +145,8 @@ public void getAll(@NotNull Context ctx) { ctx.status(HttpServletResponse.SC_OK); } catch (URISyntaxException | UnsupportedEncodingException ex) { - CdaError re = - new CdaError("Failed to process request: " + ex.getLocalizedMessage()); + CdaError re = ErrorTraceSupport.buildError(ctx, + "Failed to process request: " + ex.getLocalizedMessage(), ex); logger.atSevere().withCause(ex).log("%s", re); ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } diff --git a/cwms-data-api/src/main/java/cwms/cda/api/TimeSeriesController.java b/cwms-data-api/src/main/java/cwms/cda/api/TimeSeriesController.java index 6a7841ff2..580e82cba 100644 --- a/cwms-data-api/src/main/java/cwms/cda/api/TimeSeriesController.java +++ b/cwms-data-api/src/main/java/cwms/cda/api/TimeSeriesController.java @@ -47,6 +47,7 @@ import com.codahale.metrics.Timer; import cwms.cda.api.enums.UnitSystem; import cwms.cda.api.errors.CdaError; +import cwms.cda.api.errors.ErrorTraceSupport; import cwms.cda.api.errors.NotFoundException; import cwms.cda.data.dao.JooqDao; import cwms.cda.data.dao.StoreRule; @@ -222,7 +223,7 @@ public void create(@NotNull Context ctx) { dao.create(timeSeries, createAsLrts, storeRule, overrideProtection, vd); ctx.status(HttpServletResponse.SC_OK); } catch (DataAccessException | IOException ex) { - CdaError re = new CdaError("Internal Error"); + CdaError re = ErrorTraceSupport.buildError(ctx, "Internal Error", ex); logger.atSevere().withCause(ex).log("%s", re.toString()); ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } @@ -638,7 +639,7 @@ public void update(@NotNull Context ctx, @NotNull String id) { ctx.status(HttpServletResponse.SC_OK); } catch (DataAccessException | IOException ex) { - CdaError re = new CdaError("Internal Error"); + CdaError re = ErrorTraceSupport.buildError(ctx, "Internal Error", ex); logger.atSevere().withCause(ex).log("%s", re.toString()); ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } diff --git a/cwms-data-api/src/main/java/cwms/cda/api/TimeSeriesIdentifierDescriptorController.java b/cwms-data-api/src/main/java/cwms/cda/api/TimeSeriesIdentifierDescriptorController.java index 142a430c6..f9958a431 100644 --- a/cwms-data-api/src/main/java/cwms/cda/api/TimeSeriesIdentifierDescriptorController.java +++ b/cwms-data-api/src/main/java/cwms/cda/api/TimeSeriesIdentifierDescriptorController.java @@ -31,6 +31,7 @@ import com.codahale.metrics.MetricRegistry; import com.codahale.metrics.Timer; import cwms.cda.api.errors.CdaError; +import cwms.cda.api.errors.ErrorTraceSupport; import cwms.cda.data.dao.JooqDao; import cwms.cda.data.dao.TimeSeriesIdentifierDescriptorDao; import cwms.cda.data.dto.TimeSeriesIdentifierDescriptor; @@ -343,7 +344,7 @@ public void delete(@NotNull Context ctx, @NotNull String timeseriesId) { ctx.status(HttpServletResponse.SC_OK); } catch (DataAccessException ex) { - CdaError re = new CdaError("Internal Error"); + CdaError re = ErrorTraceSupport.buildError(ctx, "Internal Error", ex); logger.atSevere().withCause(ex).log("%s", re.toString()); ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } diff --git a/cwms-data-api/src/main/java/cwms/cda/api/rating/RatingController.java b/cwms-data-api/src/main/java/cwms/cda/api/rating/RatingController.java index 365816242..763acee65 100644 --- a/cwms-data-api/src/main/java/cwms/cda/api/rating/RatingController.java +++ b/cwms-data-api/src/main/java/cwms/cda/api/rating/RatingController.java @@ -62,6 +62,7 @@ import com.fasterxml.jackson.dataformat.xml.XmlMapper; import cwms.cda.api.Controllers; import cwms.cda.api.errors.CdaError; +import cwms.cda.api.errors.ErrorTraceSupport; import cwms.cda.data.dao.JsonRatingUtils; import cwms.cda.data.dao.RatingDao; import cwms.cda.data.dao.RatingSetDao; @@ -171,11 +172,13 @@ public void create(@NotNull Context ctx) { StatusResponse re = new StatusResponse(RatingDao.extractOfficeFromXml(ratingSet), "Rating Set successfully stored to CWMS."); ctx.status(HttpServletResponse.SC_CREATED).json(re); } catch (IOException ex) { - CdaError re = new CdaError("Failed to process request to update RatingSet"); + CdaError re = ErrorTraceSupport.buildError(ctx, + "Failed to process request to update RatingSet", ex); logger.atSevere().withCause(ex).log("%s", re); ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } catch (RatingException ex) { - CdaError re = new CdaError("Failed to process request to update RatingSet: " + ex.getLocalizedMessage()); + CdaError re = ErrorTraceSupport.buildError(ctx, + "Failed to process request to update RatingSet: " + ex.getLocalizedMessage(), ex); logger.atSevere().withCause(ex).log("%s", re); ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } @@ -492,14 +495,14 @@ private String getRatingSetString(Context ctx, RatingSet.DatabaseLoadMethod meth ctx.status(HttpCode.NOT_FOUND); } } catch (RatingException e) { - CdaError re = - new CdaError("Failed to process request to retrieve RatingSet"); + CdaError re = ErrorTraceSupport.buildError(ctx, + "Failed to process request to retrieve RatingSet", e); logger.atSevere().withCause(e).log("%s", re); ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); ctx.json(re); } catch (IOException e) { - CdaError re = - new CdaError("Failed to process request to retrieve RatingSet"); + CdaError re = ErrorTraceSupport.buildError(ctx, + "Failed to process request to retrieve RatingSet", e); logger.atSevere().withCause(e).log("%s", re); ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } @@ -580,11 +583,13 @@ public void update(@NotNull Context ctx, @NotNull String ratingId) { StatusResponse re = new StatusResponse(RatingDao.extractOfficeFromXml(ratingSet), "Updated RatingSet"); ctx.status(HttpServletResponse.SC_OK).json(re); } catch (IOException ex) { - CdaError re = new CdaError("Failed to process request to update RatingSet"); + CdaError re = ErrorTraceSupport.buildError(ctx, + "Failed to process request to update RatingSet", ex); logger.atSevere().withCause(ex).log("%s", re); ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } catch (RatingException ex) { - CdaError re = new CdaError("Failed to process request to update RatingSet: " + ex.getLocalizedMessage()); + CdaError re = ErrorTraceSupport.buildError(ctx, + "Failed to process request to update RatingSet: " + ex.getLocalizedMessage(), ex); logger.atSevere().withCause(ex).log("%s", re); ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } From a2f50dc880422bd10097bd997d1c3d33da6ab70b Mon Sep 17 00:00:00 2001 From: Charles Graham Date: Wed, 8 Apr 2026 21:47:11 -0500 Subject: [PATCH 3/6] Allow generic 500 errors to bubble up to ApiServlet --- .../src/main/java/cwms/cda/api/ParametersController.java | 6 ------ .../main/java/cwms/cda/api/SpecifiedLevelController.java | 7 ------- .../src/main/java/cwms/cda/api/TimeZoneController.java | 7 ------- .../src/main/java/cwms/cda/api/UnitsController.java | 6 ------ .../java/cwms/cda/api/rating/RatingMetadataController.java | 5 ----- .../java/cwms/cda/api/rating/RatingSpecController.java | 5 ----- .../java/cwms/cda/api/rating/RatingTemplateController.java | 5 ----- 7 files changed, 41 deletions(-) diff --git a/cwms-data-api/src/main/java/cwms/cda/api/ParametersController.java b/cwms-data-api/src/main/java/cwms/cda/api/ParametersController.java index b3a103d45..b4fa06aae 100644 --- a/cwms-data-api/src/main/java/cwms/cda/api/ParametersController.java +++ b/cwms-data-api/src/main/java/cwms/cda/api/ParametersController.java @@ -28,12 +28,10 @@ import io.javalin.plugin.openapi.annotations.OpenApiParam; import io.javalin.plugin.openapi.annotations.OpenApiResponse; import java.util.List; -import com.google.common.flogger.FluentLogger; import javax.servlet.http.HttpServletResponse; import org.jooq.DSLContext; public class ParametersController implements CrudHandler { - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private final MetricRegistry metrics; private final Histogram requestResultSize; @@ -121,10 +119,6 @@ public void getAll(Context ctx) { ctx.result(results); addDeprecatedContentTypeWarning(ctx, contentType); requestResultSize.update(results.length()); - } catch (Exception ex) { - CdaError re = new CdaError("Failed to process request"); - logger.atSevere().withCause(ex).log("%s", re); - ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } } diff --git a/cwms-data-api/src/main/java/cwms/cda/api/SpecifiedLevelController.java b/cwms-data-api/src/main/java/cwms/cda/api/SpecifiedLevelController.java index 31638a5d4..db2ac2b9f 100644 --- a/cwms-data-api/src/main/java/cwms/cda/api/SpecifiedLevelController.java +++ b/cwms-data-api/src/main/java/cwms/cda/api/SpecifiedLevelController.java @@ -41,7 +41,6 @@ import javax.servlet.http.HttpServletResponse; import java.util.List; -import com.google.common.flogger.FluentLogger; import static com.codahale.metrics.MetricRegistry.name; import static cwms.cda.api.Controllers.*; @@ -49,7 +48,6 @@ public class SpecifiedLevelController implements CrudHandler { - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private static final String TAG = "Levels"; private final MetricRegistry metrics; @@ -110,11 +108,6 @@ public void getAll(Context ctx) { ctx.result(result); requestResultSize.update(result.length()); ctx.status(HttpServletResponse.SC_OK); - } catch (Exception ex) { - CdaError re = - new CdaError("Failed to process request: " + ex.getLocalizedMessage()); - logger.atSevere().withCause(ex).log("%s", re); - ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } } diff --git a/cwms-data-api/src/main/java/cwms/cda/api/TimeZoneController.java b/cwms-data-api/src/main/java/cwms/cda/api/TimeZoneController.java index 44cf6d9ff..fa1ca5c6f 100644 --- a/cwms-data-api/src/main/java/cwms/cda/api/TimeZoneController.java +++ b/cwms-data-api/src/main/java/cwms/cda/api/TimeZoneController.java @@ -30,13 +30,10 @@ import io.javalin.plugin.openapi.annotations.OpenApiParam; import io.javalin.plugin.openapi.annotations.OpenApiResponse; -import com.google.common.flogger.FluentLogger; import javax.servlet.http.HttpServletResponse; import org.jooq.DSLContext; public class TimeZoneController implements CrudHandler { - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - private final MetricRegistry metrics; private final Histogram requestResultSize; @@ -121,10 +118,6 @@ public void getAll(Context ctx) { requestResultSize.update(results.length()); ctx.status(HttpServletResponse.SC_OK); ctx.result(results); - } catch (Exception ex) { - logger.atSevere().withCause(ex).log("Failed to process request"); - ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); - ctx.result("Failed to process request"); } } diff --git a/cwms-data-api/src/main/java/cwms/cda/api/UnitsController.java b/cwms-data-api/src/main/java/cwms/cda/api/UnitsController.java index 11f1b2aa8..89fb497ea 100644 --- a/cwms-data-api/src/main/java/cwms/cda/api/UnitsController.java +++ b/cwms-data-api/src/main/java/cwms/cda/api/UnitsController.java @@ -27,12 +27,10 @@ import io.javalin.plugin.openapi.annotations.OpenApiParam; import io.javalin.plugin.openapi.annotations.OpenApiResponse; import java.util.List; -import com.google.common.flogger.FluentLogger; import javax.servlet.http.HttpServletResponse; import org.jooq.DSLContext; public class UnitsController implements CrudHandler { - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private final MetricRegistry metrics; private final Histogram requestResultSize; @@ -115,10 +113,6 @@ public void getAll(Context ctx) { ctx.result(results); addDeprecatedContentTypeWarning(ctx, contentType); requestResultSize.update(results.length()); - } catch (Exception ex) { - logger.atSevere().withCause(ex).log("Failed to process request"); - ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); - ctx.result("Failed to process request"); } } diff --git a/cwms-data-api/src/main/java/cwms/cda/api/rating/RatingMetadataController.java b/cwms-data-api/src/main/java/cwms/cda/api/rating/RatingMetadataController.java index ce95a2f17..bd684d0e9 100644 --- a/cwms-data-api/src/main/java/cwms/cda/api/rating/RatingMetadataController.java +++ b/cwms-data-api/src/main/java/cwms/cda/api/rating/RatingMetadataController.java @@ -167,11 +167,6 @@ public void getAll(Context ctx) { requestResultSize.update(result.length()); ctx.status(HttpServletResponse.SC_OK); - } catch (Exception ex) { - CdaError re = - new CdaError("Failed to process request: " + ex.getLocalizedMessage()); - logger.atSevere().withCause(ex).log("%s", re); - ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } } diff --git a/cwms-data-api/src/main/java/cwms/cda/api/rating/RatingSpecController.java b/cwms-data-api/src/main/java/cwms/cda/api/rating/RatingSpecController.java index 63f157b45..41eb577b5 100644 --- a/cwms-data-api/src/main/java/cwms/cda/api/rating/RatingSpecController.java +++ b/cwms-data-api/src/main/java/cwms/cda/api/rating/RatingSpecController.java @@ -138,11 +138,6 @@ public void getAll(Context ctx) { ctx.result(result); requestResultSize.update(result.length()); ctx.status(HttpServletResponse.SC_OK); - } catch (Exception ex) { - CdaError re = - new CdaError("Failed to process request: " + ex.getLocalizedMessage()); - logger.atSevere().withCause(ex).log("%s", re); - ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } } diff --git a/cwms-data-api/src/main/java/cwms/cda/api/rating/RatingTemplateController.java b/cwms-data-api/src/main/java/cwms/cda/api/rating/RatingTemplateController.java index a56b6ed50..6f55c1039 100644 --- a/cwms-data-api/src/main/java/cwms/cda/api/rating/RatingTemplateController.java +++ b/cwms-data-api/src/main/java/cwms/cda/api/rating/RatingTemplateController.java @@ -134,11 +134,6 @@ public void getAll(Context ctx) { String result = Formats.format(contentType, ratingTemplates); ctx.result(result); requestResultSize.update(result.length()); - } catch (Exception ex) { - CdaError re = - new CdaError("Failed to process request: " + ex.getLocalizedMessage()); - logger.atSevere().withCause(ex).log("%s", re); - ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re); } } From cfbc4d09c0ab6cdff0b0e77ec9b9ef7c2cbb4ce3 Mon Sep 17 00:00:00 2001 From: Charles Graham Date: Wed, 8 Apr 2026 22:16:03 -0500 Subject: [PATCH 4/6] Update compose readme to include the changes --- docker-compose.README.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/docker-compose.README.md b/docker-compose.README.md index 5a0be00da..006bd1b7d 100644 --- a/docker-compose.README.md +++ b/docker-compose.README.md @@ -45,3 +45,26 @@ The following users and permissions are available: Traefik uses port 8081 by default, if this conflicts with existing services on your machine it can be changed by setting the APP_PORT variable. + +## Local error trace debugging + +The API supports conditional stack traces in JSON error responses through +`cwms.cda.api.errors.ErrorTraceSupport`. + +For local Docker Compose or localhost testing, stack traces can be enabled in either of these ways: + +1. Set the explicit override: + + `CWMS_DATAAPI_ERRORS_ALWAYS_SHOW_STACK_TRACE=true` + +2. Run with an environment name that normalizes to a value containing `dev`, for example: + + `ENVIRONMENT=development` + +In addition, localhost requests are treated as local debug requests. When enabled, error responses +include `details.stackTrace` along with the existing `incidentIdentifier`. + +This is intended for local development and shared `development` environments. In shared `dev` (not localhost), +stack traces are gated by authentication and the `CWMS User Admins` role. + +Response stack traces should *not* be enabled broadly for production traffic. From bffb28683521ba2df41746e39687696c8edaa4f9 Mon Sep 17 00:00:00 2001 From: Charles Graham Date: Wed, 8 Apr 2026 22:43:38 -0500 Subject: [PATCH 5/6] Add list of stack traces split by new line for visiblity in browser --- .../java/cwms/cda/api/errors/ErrorTraceSupport.java | 13 ++++++++++++- .../cwms/cda/api/errors/ErrorTraceSupportTest.java | 6 ++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/cwms-data-api/src/main/java/cwms/cda/api/errors/ErrorTraceSupport.java b/cwms-data-api/src/main/java/cwms/cda/api/errors/ErrorTraceSupport.java index fbc7525e4..5fa83d871 100644 --- a/cwms-data-api/src/main/java/cwms/cda/api/errors/ErrorTraceSupport.java +++ b/cwms-data-api/src/main/java/cwms/cda/api/errors/ErrorTraceSupport.java @@ -8,13 +8,16 @@ import java.io.PrintWriter; import java.io.Serializable; import java.io.StringWriter; +import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Optional; public final class ErrorTraceSupport { public static final String STACK_TRACE_KEY = "stackTrace"; + public static final String STACK_TRACE_LINES_KEY = "stackTraceLines"; static final String ALWAYS_SHOW_STACK_TRACE_PROPERTY = "cwms.dataapi.errors.alwaysShowStackTrace"; static final String ALWAYS_SHOW_STACK_TRACE_VARIABLE = "CWMS_DATAAPI_ERRORS_ALWAYS_SHOW_STACK_TRACE"; static final String PRIMARY_ENVIRONMENT_PROPERTY = "cwms.dataapi.environment.name"; @@ -54,7 +57,9 @@ static Map buildDetails(Map details, merged.putAll(details); } if (cause != null && includeStackTrace) { - merged.put(STACK_TRACE_KEY, stackTraceOf(cause)); + String stackTrace = stackTraceOf(cause); + merged.put(STACK_TRACE_KEY, stackTrace); + merged.put(STACK_TRACE_LINES_KEY, stackTraceLinesOf(stackTrace)); } return Collections.unmodifiableMap(merged); } @@ -150,6 +155,12 @@ private static String stackTraceOf(Throwable cause) { return sw.toString(); } + private static ArrayList stackTraceLinesOf(String stackTrace) { + ArrayList lines = new ArrayList<>(); + Collections.addAll(lines, stackTrace.split("\\R")); + return lines; + } + private static String firstNonBlank(String... values) { for (String value : values) { if (value != null && !value.isBlank()) { diff --git a/cwms-data-api/src/test/java/cwms/cda/api/errors/ErrorTraceSupportTest.java b/cwms-data-api/src/test/java/cwms/cda/api/errors/ErrorTraceSupportTest.java index 731999a0d..66f8d9432 100644 --- a/cwms-data-api/src/test/java/cwms/cda/api/errors/ErrorTraceSupportTest.java +++ b/cwms-data-api/src/test/java/cwms/cda/api/errors/ErrorTraceSupportTest.java @@ -1,6 +1,7 @@ package cwms.cda.api.errors; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertTrue; import cwms.cda.security.DataApiPrincipal; import cwms.cda.security.Role; @@ -30,8 +31,11 @@ void includesStackTraceForDevAdminUser() { ErrorTraceSupport.shouldIncludeStackTrace(principal, "CWMS_DEV-West")); assertTrue(details.containsKey(ErrorTraceSupport.STACK_TRACE_KEY)); + assertTrue(details.containsKey(ErrorTraceSupport.STACK_TRACE_LINES_KEY)); assertTrue(details.get(ErrorTraceSupport.STACK_TRACE_KEY).toString() .contains("IllegalStateException")); + assertTrue(assertInstanceOf(Iterable.class, details.get(ErrorTraceSupport.STACK_TRACE_LINES_KEY)) + .iterator().next().toString().contains("IllegalStateException")); } @Test @@ -52,6 +56,7 @@ void omitsStackTraceOutsideDev() { ErrorTraceSupport.shouldIncludeStackTrace(principal, "production")); assertFalse(details.containsKey(ErrorTraceSupport.STACK_TRACE_KEY)); + assertFalse(details.containsKey(ErrorTraceSupport.STACK_TRACE_LINES_KEY)); } @Test @@ -113,6 +118,7 @@ void overrideAddsStackTraceToDetails() { ErrorTraceSupport.shouldIncludeStackTrace(null, "production")); assertTrue(details.containsKey(ErrorTraceSupport.STACK_TRACE_KEY)); + assertTrue(details.containsKey(ErrorTraceSupport.STACK_TRACE_LINES_KEY)); } @Test From 9bcae0408de0c7f90b366cb55e21f1343cb7991a Mon Sep 17 00:00:00 2001 From: Charles Graham Date: Thu, 9 Apr 2026 18:50:06 -0500 Subject: [PATCH 6/6] check togglz flag instead of env/property, default it off --- .../cda/api/errors/ErrorTraceSupport.java | 72 ++++--------------- .../java/cwms/cda/features/CdaFeatures.java | 4 +- .../src/main/resources/features.properties | 3 +- .../cda/api/errors/ErrorTraceSupportTest.java | 68 +++--------------- .../CdaFeatureManagerProviderTest.java | 18 +++++ docker-compose.README.md | 20 ++---- 6 files changed, 51 insertions(+), 134 deletions(-) diff --git a/cwms-data-api/src/main/java/cwms/cda/api/errors/ErrorTraceSupport.java b/cwms-data-api/src/main/java/cwms/cda/api/errors/ErrorTraceSupport.java index 5fa83d871..98f2d0cc1 100644 --- a/cwms-data-api/src/main/java/cwms/cda/api/errors/ErrorTraceSupport.java +++ b/cwms-data-api/src/main/java/cwms/cda/api/errors/ErrorTraceSupport.java @@ -1,6 +1,7 @@ package cwms.cda.api.errors; import cwms.cda.data.dao.AuthDao; +import cwms.cda.features.CdaFeatures; import cwms.cda.security.DataApiPrincipal; import cwms.cda.security.Role; import cwms.cda.spi.IdentityProvider; @@ -11,21 +12,14 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; -import java.util.List; import java.util.Map; import java.util.Optional; +import org.togglz.core.context.FeatureContext; +import org.togglz.core.manager.FeatureManager; public final class ErrorTraceSupport { public static final String STACK_TRACE_KEY = "stackTrace"; public static final String STACK_TRACE_LINES_KEY = "stackTraceLines"; - static final String ALWAYS_SHOW_STACK_TRACE_PROPERTY = "cwms.dataapi.errors.alwaysShowStackTrace"; - static final String ALWAYS_SHOW_STACK_TRACE_VARIABLE = "CWMS_DATAAPI_ERRORS_ALWAYS_SHOW_STACK_TRACE"; - static final String PRIMARY_ENVIRONMENT_PROPERTY = "cwms.dataapi.environment.name"; - static final String PRIMARY_ENVIRONMENT_VARIABLE = "CWMS_DATAAPI_ENVIRONMENT_NAME"; - static final String HOST_ENVIRONMENT_VARIABLE = "ENVIRONMENT"; - static final String LEGACY_ENVIRONMENT_PROPERTY = "cda.environment.name"; - static final String LEGACY_ENVIRONMENT_VARIABLE = "CDA_ENVIRONMENT_NAME"; - static final String WAR_CONTEXT_PROPERTY = "warContext"; private static final String DEV_MARKER = "dev"; private static final Role CWMS_USER_ADMINS_ROLE = new Role("CWMS User Admins"); @@ -65,28 +59,14 @@ static Map buildDetails(Map details, } static boolean shouldIncludeStackTrace(Context ctx) { - if (alwaysShowStackTraceOverrideEnabled()) { - return true; - } if (localhostRequestOverrideEnabled(ctx)) { return true; } - return shouldIncludeStackTrace(resolvePrincipal(ctx).orElse(null), resolveEnvironmentName(ctx)); - } - - static boolean shouldIncludeStackTrace(DataApiPrincipal principal, String environmentName) { - if (alwaysShowStackTraceOverrideEnabled()) { - return true; - } - return hasAdminRole(principal) && environmentLooksLikeDev(environmentName); + return shouldIncludeStackTrace(resolvePrincipal(ctx).orElse(null), stackTraceFeatureEnabled()); } - static boolean alwaysShowStackTraceOverrideEnabled() { - return Boolean.parseBoolean(firstNonBlank( - System.getProperty(ALWAYS_SHOW_STACK_TRACE_PROPERTY), - System.getenv(ALWAYS_SHOW_STACK_TRACE_VARIABLE), - "false" - )); + static boolean shouldIncludeStackTrace(DataApiPrincipal principal, boolean stackTraceFeatureEnabled) { + return stackTraceFeatureEnabled && hasAdminRole(principal); } static boolean localhostRequestOverrideEnabled(Context ctx) { @@ -120,33 +100,13 @@ static boolean hasAdminRole(DataApiPrincipal principal) { && principal.getRoles().contains(CWMS_USER_ADMINS_ROLE); } - static String resolveEnvironmentName(Context ctx) { - String configuredName = firstNonBlank( - System.getProperty(PRIMARY_ENVIRONMENT_PROPERTY), - System.getenv(PRIMARY_ENVIRONMENT_VARIABLE), - System.getenv(HOST_ENVIRONMENT_VARIABLE), - System.getProperty(LEGACY_ENVIRONMENT_PROPERTY), - System.getenv(LEGACY_ENVIRONMENT_VARIABLE), - System.getProperty(WAR_CONTEXT_PROPERTY) - ); - if (configuredName != null) { - return configuredName; - } - if (ctx == null) { - return ""; - } - return firstNonBlank(ctx.contextPath(), ctx.req != null ? ctx.req.getContextPath() : null, ""); - } - - static boolean environmentLooksLikeDev(String environmentName) { - return normalizeEnvironmentName(environmentName).contains(DEV_MARKER); - } - - static String normalizeEnvironmentName(String environmentName) { - if (environmentName == null) { - return ""; + static boolean stackTraceFeatureEnabled() { + try { + FeatureManager featureManager = FeatureContext.getFeatureManager(); + return featureManager.isActive(CdaFeatures.INCLUDE_ERROR_STACK_TRACES); + } catch (Throwable ignore) { + return false; } - return environmentName.toLowerCase().replaceAll("[^a-z0-9]", ""); } private static String stackTraceOf(Throwable cause) { @@ -161,12 +121,4 @@ private static ArrayList stackTraceLinesOf(String stackTrace) { return lines; } - private static String firstNonBlank(String... values) { - for (String value : values) { - if (value != null && !value.isBlank()) { - return value; - } - } - return null; - } } diff --git a/cwms-data-api/src/main/java/cwms/cda/features/CdaFeatures.java b/cwms-data-api/src/main/java/cwms/cda/features/CdaFeatures.java index fa2a662ef..c8bcbd3dd 100644 --- a/cwms-data-api/src/main/java/cwms/cda/features/CdaFeatures.java +++ b/cwms-data-api/src/main/java/cwms/cda/features/CdaFeatures.java @@ -5,5 +5,7 @@ public enum CdaFeatures implements Feature { @Label("Use object-storage backed Blob DAO in BlobController") - USE_OBJECT_STORAGE_BLOBS + USE_OBJECT_STORAGE_BLOBS, + @Label("Include stack traces in JSON error responses for authorized debug requests") + INCLUDE_ERROR_STACK_TRACES } diff --git a/cwms-data-api/src/main/resources/features.properties b/cwms-data-api/src/main/resources/features.properties index 94eeb1556..c7c091f5d 100644 --- a/cwms-data-api/src/main/resources/features.properties +++ b/cwms-data-api/src/main/resources/features.properties @@ -1 +1,2 @@ -USE_OBJECT_STORAGE_BLOBS=false \ No newline at end of file +USE_OBJECT_STORAGE_BLOBS=false +INCLUDE_ERROR_STACK_TRACES=false diff --git a/cwms-data-api/src/test/java/cwms/cda/api/errors/ErrorTraceSupportTest.java b/cwms-data-api/src/test/java/cwms/cda/api/errors/ErrorTraceSupportTest.java index 66f8d9432..dce8e4352 100644 --- a/cwms-data-api/src/test/java/cwms/cda/api/errors/ErrorTraceSupportTest.java +++ b/cwms-data-api/src/test/java/cwms/cda/api/errors/ErrorTraceSupportTest.java @@ -8,27 +8,18 @@ import java.io.Serializable; import java.util.Map; import java.util.Set; -import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; class ErrorTraceSupportTest { - @AfterEach - void clearProperties() { - System.clearProperty(ErrorTraceSupport.ALWAYS_SHOW_STACK_TRACE_PROPERTY); - System.clearProperty(ErrorTraceSupport.PRIMARY_ENVIRONMENT_PROPERTY); - System.clearProperty(ErrorTraceSupport.LEGACY_ENVIRONMENT_PROPERTY); - System.clearProperty(ErrorTraceSupport.WAR_CONTEXT_PROPERTY); - } - @Test - void includesStackTraceForDevAdminUser() { + void includesStackTraceWhenFeatureEnabledForAdminUser() { DataApiPrincipal principal = new DataApiPrincipal("admin-user", Set.of(new Role("CWMS User Admins"))); Map details = ErrorTraceSupport.buildDetails(Map.of(), new IllegalStateException("boom"), - ErrorTraceSupport.shouldIncludeStackTrace(principal, "CWMS_DEV-West")); + ErrorTraceSupport.shouldIncludeStackTrace(principal, true)); assertTrue(details.containsKey(ErrorTraceSupport.STACK_TRACE_KEY)); assertTrue(details.containsKey(ErrorTraceSupport.STACK_TRACE_LINES_KEY)); @@ -39,21 +30,21 @@ void includesStackTraceForDevAdminUser() { } @Test - void includesStackTraceForExistingUserAdminsRole() { + void featureEnablesStackTraceForUserAdminsRole() { DataApiPrincipal principal = new DataApiPrincipal("admin-user", Set.of(new Role("CWMS User Admins"))); - assertTrue(ErrorTraceSupport.shouldIncludeStackTrace(principal, "spk-dev")); + assertTrue(ErrorTraceSupport.shouldIncludeStackTrace(principal, true)); } @Test - void omitsStackTraceOutsideDev() { + void omitsStackTraceWhenFeatureDisabled() { DataApiPrincipal principal = new DataApiPrincipal("admin-user", Set.of(new Role("CWMS User Admins"))); Map details = ErrorTraceSupport.buildDetails(Map.of(), new IllegalStateException("boom"), - ErrorTraceSupport.shouldIncludeStackTrace(principal, "production")); + ErrorTraceSupport.shouldIncludeStackTrace(principal, false)); assertFalse(details.containsKey(ErrorTraceSupport.STACK_TRACE_KEY)); assertFalse(details.containsKey(ErrorTraceSupport.STACK_TRACE_LINES_KEY)); @@ -64,32 +55,12 @@ void omitsStackTraceForNonAdminUser() { DataApiPrincipal principal = new DataApiPrincipal("normal-user", Set.of(new Role("CWMS Users"))); - assertFalse(ErrorTraceSupport.shouldIncludeStackTrace(principal, "dev")); + assertFalse(ErrorTraceSupport.shouldIncludeStackTrace(principal, true)); } @Test void omitsStackTraceForGuestUser() { - assertFalse(ErrorTraceSupport.shouldIncludeStackTrace(null, "dev")); - } - - @Test - void normalizesEnvironmentNameBeforeDevCheck() { - assertTrue(ErrorTraceSupport.environmentLooksLikeDev("CWMS.Dev-West")); - assertTrue(ErrorTraceSupport.environmentLooksLikeDev("cwms_dev_west")); - assertFalse(ErrorTraceSupport.environmentLooksLikeDev("production")); - } - - @Test - void fallsBackToContextPathWhenEnvironmentPropertyMissing() { - assertTrue(ErrorTraceSupport.environmentLooksLikeDev("/spk-data-dev")); - } - - @Test - void environmentPropertyStillResolvesForDevChecks() { - System.setProperty(ErrorTraceSupport.PRIMARY_ENVIRONMENT_PROPERTY, "local-dev"); - - assertTrue(ErrorTraceSupport.environmentLooksLikeDev( - ErrorTraceSupport.resolveEnvironmentName(null))); + assertFalse(ErrorTraceSupport.shouldIncludeStackTrace(null, true)); } @Test @@ -97,28 +68,7 @@ void omitsStackTraceForUndefinedCwmsAdminRole() { DataApiPrincipal principal = new DataApiPrincipal("admin-user", Set.of(new Role("CWMS Admin"))); - assertFalse(ErrorTraceSupport.shouldIncludeStackTrace(principal, "dev")); - } - - @Test - void overrideEnablesStackTraceWithoutAdminRole() { - DataApiPrincipal principal = new DataApiPrincipal("normal-user", - Set.of(new Role("CWMS Users"))); - System.setProperty(ErrorTraceSupport.ALWAYS_SHOW_STACK_TRACE_PROPERTY, "true"); - - assertTrue(ErrorTraceSupport.shouldIncludeStackTrace(principal, "production")); - } - - @Test - void overrideAddsStackTraceToDetails() { - System.setProperty(ErrorTraceSupport.ALWAYS_SHOW_STACK_TRACE_PROPERTY, "true"); - - Map details = ErrorTraceSupport.buildDetails(Map.of(), - new IllegalStateException("boom"), - ErrorTraceSupport.shouldIncludeStackTrace(null, "production")); - - assertTrue(details.containsKey(ErrorTraceSupport.STACK_TRACE_KEY)); - assertTrue(details.containsKey(ErrorTraceSupport.STACK_TRACE_LINES_KEY)); + assertFalse(ErrorTraceSupport.shouldIncludeStackTrace(principal, true)); } @Test diff --git a/cwms-data-api/src/test/java/cwms/cda/features/CdaFeatureManagerProviderTest.java b/cwms-data-api/src/test/java/cwms/cda/features/CdaFeatureManagerProviderTest.java index 7a2f68a2a..8506eec9e 100644 --- a/cwms-data-api/src/test/java/cwms/cda/features/CdaFeatureManagerProviderTest.java +++ b/cwms-data-api/src/test/java/cwms/cda/features/CdaFeatureManagerProviderTest.java @@ -61,6 +61,23 @@ void testUseObjectStorageBlobsFeature() throws IOException { assertTrue(manager.isActive(CdaFeatures.USE_OBJECT_STORAGE_BLOBS)); } + @Test + void testIncludeErrorStackTracesFeature() throws IOException { + File tempFile = Files.createTempFile("features", ".properties").toFile(); + tempFile.deleteOnExit(); + + try (FileWriter writer = new FileWriter(tempFile)) { + writer.write(CdaFeatures.INCLUDE_ERROR_STACK_TRACES.name() + " = true"); + } + + System.setProperty(CdaFeatureManagerProvider.PROPERTIES_FILE, tempFile.getAbsolutePath()); + + CdaFeatureManagerProvider provider = new CdaFeatureManagerProvider(); + FeatureManager manager = provider.getFeatureManager(); + + assertTrue(manager.isActive(CdaFeatures.INCLUDE_ERROR_STACK_TRACES)); + } + @Test void testFeatureDisabledByDefault() throws IOException { File tempFile = Files.createTempFile("features_disabled", ".properties").toFile(); @@ -73,5 +90,6 @@ void testFeatureDisabledByDefault() throws IOException { FeatureManager manager = provider.getFeatureManager(); assertFalse(manager.isActive(CdaFeatures.USE_OBJECT_STORAGE_BLOBS)); + assertFalse(manager.isActive(CdaFeatures.INCLUDE_ERROR_STACK_TRACES)); } } diff --git a/docker-compose.README.md b/docker-compose.README.md index 006bd1b7d..5dde9ba3a 100644 --- a/docker-compose.README.md +++ b/docker-compose.README.md @@ -51,20 +51,14 @@ be changed by setting the APP_PORT variable. The API supports conditional stack traces in JSON error responses through `cwms.cda.api.errors.ErrorTraceSupport`. -For local Docker Compose or localhost testing, stack traces can be enabled in either of these ways: +Stack-trace exposure is controlled through the Togglz feature flag +`INCLUDE_ERROR_STACK_TRACES`. -1. Set the explicit override: +For shared environments, enable that feature in the active `features.properties` and the API will +include traces only for authenticated users with the `CWMS User Admins` role. - `CWMS_DATAAPI_ERRORS_ALWAYS_SHOW_STACK_TRACE=true` - -2. Run with an environment name that normalizes to a value containing `dev`, for example: - - `ENVIRONMENT=development` - -In addition, localhost requests are treated as local debug requests. When enabled, error responses -include `details.stackTrace` along with the existing `incidentIdentifier`. - -This is intended for local development and shared `development` environments. In shared `dev` (not localhost), -stack traces are gated by authentication and the `CWMS User Admins` role. +For local Docker Compose or other localhost testing, requests to `localhost`, `127.0.0.1`, and +`::1` are still treated as local debug requests even when the feature is disabled. When enabled, +error responses include `details.stackTrace` along with the existing `incidentIdentifier`. Response stack traces should *not* be enabled broadly for production traffic.