diff --git a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/NoticeConfigServiceImpl.java b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/NoticeConfigServiceImpl.java index bfbed03191..eab8ad014e 100644 --- a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/NoticeConfigServiceImpl.java +++ b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/NoticeConfigServiceImpl.java @@ -190,13 +190,28 @@ public class NoticeConfigServiceImpl implements NoticeConfigService, CommandLine * be resolved, and saving would persist the placeholder */ private void resolveMaskedSecrets(NoticeReceiver noticeReceiver) { + resolveMaskedSecrets(noticeReceiver, false); + } + + /** + * Bind stored secrets to their original notification type and destination for test messages. + */ + private void resolveMaskedSecretsForTest(NoticeReceiver noticeReceiver) { + resolveMaskedSecrets(noticeReceiver, true); + } + + private void resolveMaskedSecrets(NoticeReceiver noticeReceiver, boolean testMessage) { if (noticeReceiver == null || noticeReceiver.getId() == null) { return; } NoticeReceiver existing = noticeReceiverDao.findById(noticeReceiver.getId()) .orElseThrow(() -> new IllegalArgumentException( "The receiver with id " + noticeReceiver.getId() + " does not exist.")); - NoticeReceiverMaskUtil.resolveMask(noticeReceiver, existing); + if (testMessage) { + NoticeReceiverMaskUtil.resolveMaskForTest(noticeReceiver, existing); + } else { + NoticeReceiverMaskUtil.resolveMask(noticeReceiver, existing); + } } @Override @@ -339,7 +354,7 @@ public class NoticeConfigServiceImpl implements NoticeConfigService, CommandLine @Override public boolean sendTestMsg(NoticeReceiver noticeReceiver) { - resolveMaskedSecrets(noticeReceiver); + resolveMaskedSecretsForTest(noticeReceiver); Map labels = new HashMap<>(8); labels.put(CommonConstants.LABEL_INSTANCE, "127.0.0.1"); labels.put(CommonConstants.LABEL_ALERT_NAME, "CPU Usage Alert"); diff --git a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/util/NoticeReceiverMaskUtil.java b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/util/NoticeReceiverMaskUtil.java index db5fe30a43..51da9d0f52 100644 --- a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/util/NoticeReceiverMaskUtil.java +++ b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/util/NoticeReceiverMaskUtil.java @@ -18,6 +18,7 @@ package org.apache.hertzbeat.alert.util; import java.util.List; +import java.util.Objects; import java.util.function.BiConsumer; import java.util.function.Function; import org.apache.commons.lang3.StringUtils; @@ -92,6 +93,7 @@ public final class NoticeReceiverMaskUtil { * A re-entered secret or a cleared field is left untouched. * @param incoming receiver submitted by the ui, modified in place * @param existing receiver currently stored in the database + * @throws IllegalArgumentException if a submitted mask does not match the stored secret */ public static void resolveMask(NoticeReceiver incoming, NoticeReceiver existing) { if (incoming == null || existing == null) { @@ -102,15 +104,72 @@ public final class NoticeReceiverMaskUtil { String stored = field.getter().apply(existing); if (isMaskOf(submitted, stored)) { field.setter().accept(incoming, stored); + } else if (isMaskValue(submitted)) { + throw new IllegalArgumentException( + "The submitted secret mask does not match the stored secret."); } } } + /** + * Resolve masked secrets for a test message while binding them to their persisted destination. + * A caller that changes the notification type or a URL receiving authentication data + * must submit the new secret explicitly instead of reusing a stored secret mask. + * @param incoming receiver submitted for a test message, modified in place + * @param existing receiver currently stored in the database + * @throws IllegalArgumentException if a masked secret is combined with a changed destination + */ + public static void resolveMaskForTest(NoticeReceiver incoming, NoticeReceiver existing) { + if (incoming == null || existing == null) { + return; + } + + boolean containsStoredSecretMask = SECRET_FIELDS.stream() + .anyMatch(field -> isMaskOf(field.getter().apply(incoming), field.getter().apply(existing))); + if (containsStoredSecretMask && !Objects.equals(incoming.getType(), existing.getType())) { + throw new IllegalArgumentException( + "The notification type cannot be changed when reusing a masked secret."); + } + rejectChangedSecretDestination( + incoming.getHookAuthToken(), + existing.getHookAuthToken(), + incoming.getHookUrl(), + existing.getHookUrl(), + "webhook URL"); + rejectChangedSecretDestination( + incoming.getNtfyToken(), + existing.getNtfyToken(), + incoming.getNtfyServerUrl(), + existing.getNtfyServerUrl(), + "ntfy server URL"); + resolveMask(incoming, existing); + } + + private static void rejectChangedSecretDestination( + String submittedSecret, + String storedSecret, + String submittedDestination, + String storedDestination, + String destinationName) { + if (isMaskOf(submittedSecret, storedSecret) + && !Objects.equals(submittedDestination, storedDestination)) { + throw new IllegalArgumentException( + "The " + destinationName + " cannot be changed when reusing a masked secret."); + } + } + private static boolean isMaskOf(String submitted, String stored) { if (submitted == null || StringUtils.isBlank(stored)) { return false; } - return submitted.equals(SECRET_MASK) || submitted.equals(maskValue(stored)); + return submitted.equals(maskValue(stored)); + } + + private static boolean isMaskValue(String value) { + return value != null + && value.startsWith(SECRET_MASK) + && (value.length() == SECRET_MASK.length() + || value.length() == SECRET_MASK.length() + VISIBLE_SUFFIX_LENGTH); } private static String maskValue(String value) { diff --git a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/service/NoticeConfigServiceTest.java b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/service/NoticeConfigServiceTest.java index 72f21bc3c8..2371e2692c 100644 --- a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/service/NoticeConfigServiceTest.java +++ b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/service/NoticeConfigServiceTest.java @@ -279,6 +279,41 @@ class NoticeConfigServiceTest { verify(dispatcherAlarm, never()).sendNoticeMsg(any(), any(), any()); } + @Test + void sendTestMsgRejectsMaskedSecretReplayToChangedWebhookUrl() { + final NoticeReceiver stored = new NoticeReceiver(); + stored.setId(5L); + stored.setType((byte) 2); + stored.setHookUrl("https://trusted.example/hook"); + stored.setHookAuthToken("hook-auth-token-abcd"); + when(noticeReceiverDao.findById(5L)).thenReturn(Optional.of(stored)); + + final NoticeReceiver incoming = NoticeReceiverMaskUtil.mask(stored); + incoming.setHookUrl("https://attacker.example/collect"); + + assertThrows(IllegalArgumentException.class, () -> noticeConfigService.sendTestMsg(incoming)); + verify(dispatcherAlarm, never()).sendNoticeMsg(any(), any(), any()); + } + + @Test + void sendTestMsgRejectsBareMaskReplayToChangedWebhookUrl() { + final NoticeReceiver stored = new NoticeReceiver(); + stored.setId(5L); + stored.setType((byte) 2); + stored.setHookUrl("https://trusted.example/hook"); + stored.setHookAuthToken("hook-auth-token-abcd"); + when(noticeReceiverDao.findById(5L)).thenReturn(Optional.of(stored)); + + final NoticeReceiver incoming = new NoticeReceiver(); + incoming.setId(5L); + incoming.setType((byte) 2); + incoming.setHookUrl("https://attacker.example/collect"); + incoming.setHookAuthToken(NoticeReceiverMaskUtil.SECRET_MASK); + + assertThrows(IllegalArgumentException.class, () -> noticeConfigService.sendTestMsg(incoming)); + verify(dispatcherAlarm, never()).sendNoticeMsg(any(), any(), any()); + } + @Test void deleteReceiver() { final Long receiverId = 23342525L; diff --git a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/util/NoticeReceiverMaskUtilTest.java b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/util/NoticeReceiverMaskUtilTest.java index 7d42ed6e87..cdac126c46 100644 --- a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/util/NoticeReceiverMaskUtilTest.java +++ b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/util/NoticeReceiverMaskUtilTest.java @@ -23,6 +23,7 @@ import org.junit.jupiter.api.Test; import static org.apache.hertzbeat.alert.util.NoticeReceiverMaskUtil.SECRET_MASK; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; /** * Test case for {@link NoticeReceiverMaskUtil} @@ -104,7 +105,6 @@ class NoticeReceiverMaskUtilTest { NoticeReceiver incoming = NoticeReceiverMaskUtil.mask(existing); incoming.setAccessToken("new-access-token-1234"); incoming.setGotifyToken(null); - incoming.setNtfyToken(SECRET_MASK); NoticeReceiverMaskUtil.resolveMask(incoming, existing); @@ -122,13 +122,73 @@ class NoticeReceiverMaskUtilTest { } @Test - void resolveMaskIgnoresMaskWhenNothingIsStored() { + void resolveMaskRejectsMaskWhenNothingIsStored() { NoticeReceiver existing = new NoticeReceiver(); NoticeReceiver incoming = new NoticeReceiver(); incoming.setAccessToken(SECRET_MASK); + assertThrows( + IllegalArgumentException.class, + () -> NoticeReceiverMaskUtil.resolveMask(incoming, existing)); + } + + @Test + void resolveMaskRejectsBareMaskAsWildcard() { + NoticeReceiver existing = buildReceiverWithSecrets(); + NoticeReceiver incoming = new NoticeReceiver(); + incoming.setAccessToken(SECRET_MASK); + + assertThrows( + IllegalArgumentException.class, + () -> NoticeReceiverMaskUtil.resolveMask(incoming, existing)); + } + + @Test + void resolveMaskRestoresBareMaskForShortStoredSecret() { + NoticeReceiver existing = new NoticeReceiver(); + existing.setAccessToken("short-token"); + NoticeReceiver incoming = NoticeReceiverMaskUtil.mask(existing); + NoticeReceiverMaskUtil.resolveMask(incoming, existing); - assertEquals(SECRET_MASK, incoming.getAccessToken()); + assertEquals("short-token", incoming.getAccessToken()); + } + + @Test + void resolveMaskForTestRejectsMaskedWebhookSecretForChangedUrl() { + NoticeReceiver existing = buildReceiverWithSecrets(); + existing.setType((byte) 2); + NoticeReceiver incoming = NoticeReceiverMaskUtil.mask(existing); + incoming.setHookUrl("https://attacker.example/collect"); + + assertThrows( + IllegalArgumentException.class, + () -> NoticeReceiverMaskUtil.resolveMaskForTest(incoming, existing)); + assertEquals(SECRET_MASK + "abcd", incoming.getHookAuthToken()); + } + + @Test + void resolveMaskForTestRejectsMaskedNtfySecretForChangedServer() { + NoticeReceiver existing = buildReceiverWithSecrets(); + existing.setType((byte) 15); + existing.setNtfyServerUrl("https://ntfy.example"); + NoticeReceiver incoming = NoticeReceiverMaskUtil.mask(existing); + incoming.setNtfyServerUrl("https://attacker.example"); + + assertThrows( + IllegalArgumentException.class, + () -> NoticeReceiverMaskUtil.resolveMaskForTest(incoming, existing)); + assertEquals(SECRET_MASK + "NIz2", incoming.getNtfyToken()); + } + + @Test + void resolveMaskForTestRestoresSecretForUnchangedDestination() { + NoticeReceiver existing = buildReceiverWithSecrets(); + existing.setType((byte) 2); + NoticeReceiver incoming = NoticeReceiverMaskUtil.mask(existing); + + NoticeReceiverMaskUtil.resolveMaskForTest(incoming, existing); + + assertEquals("hook-auth-token-abcd", incoming.getHookAuthToken()); } }