Mark webhook-channel configs as encrypted

[TomasLongo]

Mark webhook-channel configs as encrypted

This PR supports the effort to encrypt notification channel configurations in the OpenSearch Notification-Plugin.

It checks if the url for a webhook channel is encrypted and skips url-validation if this is the case.

Caveats

This introduces external knowledge into the utility project (How encrypted urls are prefixed). An alternative would be, creating dedicated types for encrypted webhook configs inside the notification project.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• New functionality includes testing.
• New functionality has been documented.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit c764168)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 Security concerns Sensitive information exposure: The "enc:" prefix convention used to bypass URL validation could potentially be abused. Any caller can pass a URL starting with "enc:" to skip validation without the URL actually being encrypted. This could allow storing malformed or malicious URLs in the system under the guise of being encrypted, potentially leading to SSRF or other injection issues when the URL is later used.

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Magic String The prefix "enc:" is hardcoded as a magic string in all four model classes (Chime, Slack, MicrosoftTeams, Webhook). This should be extracted into a shared constant to avoid duplication and reduce the risk of typos or inconsistency across classes. if (!url.startsWith("enc:")) { validateUrl(url) } Bypass Risk The encrypted URL check only verifies that the URL starts with "enc:". There is no validation that the remainder of the string is actually encrypted content. A user could pass "enc:" followed by an empty string or arbitrary plaintext, bypassing URL validation entirely without any real encryption. Consider adding a minimum length check or format validation for the encrypted payload. if (!url.startsWith("enc:")) { validateUrl(url) } Inconsistent Approach The PR description mentions a flag to mark a webhook as encrypted, but the implementation uses a URL prefix convention ("enc:") instead of an explicit boolean flag. This implicit approach couples the encryption marker to the URL field format, which may cause issues if the encryption scheme changes in the future or if the prefix needs to be updated. if (!url.startsWith("enc:")) { validateUrl(url) }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to c764168
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Extract magic encryption prefix to shared constant The "enc:" prefix check is a hardcoded magic string repeated across four files. If the prefix ever changes, all four model classes must be updated consistently. Consider extracting this constant to a shared location (e.g., a companion object or a constants file) and referencing it everywhere to avoid inconsistency bugs. src/main/kotlin/org/opensearch/commons/notifications/model/Chime.kt [29-31] -if (!url.startsWith("enc:")) { +if (!url.startsWith(ENCRYPTED_PREFIX)) { validateUrl(url) } +// In a shared constants file or companion object: +// const val ENCRYPTED_PREFIX = "enc:" Suggestion importance[1-10]: 5 __ Why: Extracting the "enc:" magic string to a shared constant is a valid maintainability improvement, as it's repeated across four files. However, this is a minor style/maintainability concern rather than a critical bug.	Low
	Validate encrypted URL value is non-empty Bypassing URL validation for any string that starts with "enc:" could allow malformed or malicious input to be stored without any validation. At minimum, you should verify that the encrypted value is non-trivially long or matches an expected format (e.g., a minimum length or a regex pattern) to prevent trivially invalid encrypted values from being accepted. src/main/kotlin/org/opensearch/commons/notifications/model/Chime.kt [29-31] if (!url.startsWith("enc:")) { validateUrl(url) +} else { + require(url.length > "enc:".length) { "Encrypted URL value cannot be empty" } } Suggestion importance[1-10]: 4 __ Why: The suggestion to add a minimum length check for encrypted URLs is a reasonable defensive measure, but the existing require(!Strings.isNullOrEmpty(url)) check already prevents empty strings. The "enc:" prefix alone (4 chars) would still pass the null/empty check, so the additional validation has some merit but is a minor edge case.	Low

---

Previous suggestions

Suggestions up to commit 4fe3b64

Category	Suggestion	Impact
Possible issue	Ensure serialization handles new encrypted field The isEncrypted field is added at the end of the constructor, but since headerParams and method already have defaults, callers using named parameters are fine. However, serialization/deserialization (e.g., from JSON/XContent) may break if isEncrypted is not handled in the corresponding read/write methods. Ensure the XContent reader and writer for Webhook are updated to include isEncrypted, otherwise deserialized objects will always have isEncrypted = false regardless of stored data. src/main/kotlin/org/opensearch/commons/notifications/model/Webhook.kt [28-31] +val url: String, +val headerParams: Map<String, String> = mapOf(), +val method: HttpMethodType = HttpMethodType.POST, +val isEncrypted: Boolean = false - Suggestion importance[1-10]: 6 __ Why: The existing_code and improved_code are identical, but the suggestion raises a valid concern about XContent serialization not being updated for the new isEncrypted field, which could cause silent data loss during deserialization. This is a legitimate issue worth investigating, though it's a verification request rather than a concrete fix.	Low
General	Verify null/empty check applies to encrypted URLs When isEncrypted is true, the URL may be an encrypted/obfuscated string, so the non-empty check on url is still valid. However, you should also consider whether an encrypted URL could legitimately be an empty string or null — if so, the require check should also be conditional. More critically, if isEncrypted is true and the URL is empty, the error message "URL is null or empty" may be misleading. Consider keeping the null/empty check unconditional but adjusting the logic if encrypted URLs have different constraints. src/main/kotlin/org/opensearch/commons/notifications/model/Chime.kt [29-32] +require(!Strings.isNullOrEmpty(url)) { "URL is null or empty" } +if (!isEncrypted) { + validateUrl(url) +} - Suggestion importance[1-10]: 2 __ Why: The existing_code and improved_code are identical, meaning no actual code change is proposed. The suggestion only asks to verify behavior, and the current logic (always checking non-empty, only validating URL format when not encrypted) is reasonable and correct.	Low

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit fd075d2.

Path	Line	Severity	Description
src/main/kotlin/org/opensearch/commons/notifications/model/Chime.kt	30	high	URL validation is bypassed for any URL prefixed with 'enc:'. This pattern is applied consistently across Chime, Slack, MicrosoftTeams, and Webhook models. While the stated intent is to support encrypted/stored URLs, the bypass is purely prefix-based with no cryptographic verification — any user-supplied string starting with 'enc:' skips validation entirely. If the consuming code passes these values to HTTP clients without further validation or decryption, this could allow SSRF attacks or requests to arbitrary internal endpoints. Maintainers should verify that a corresponding decryption layer enforces URL validity before use.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 1 | Medium: 0 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

Persistent review updated to latest commit c764168

[TomasLongo]

Addressing the security issue raised by the AI-Review:

If this change is read in isolation, it is right. However, the change in the notifications-plugin uses the AES-256-GCM algorithm which checks if the encrypted ur was manipulated. Decryption would fail if the content after enc: would not actually be some encrypted data.

Since the current change is mainly driven by creating backwarcompatibility with not encrypted webhook urls, the alternative approach mentioned in the PR description might be worth considering to create a tailored mechanic for encrypted webhook-urls.