Fix NPE in CipherTool when deployment.toml has configurations after s…

[PasinduSuraweera]

Purpose

Resolves wso2/api-manager#4561

This PR fixes a critical NullPointerException in the CipherTool that occurs when the deployment.toml file contains configuration sections defined after the [secrets] section.

Goals

• Fix the crash in ciphertool when users add custom configurations or new sections at the end of deployment.toml.
• Ensure the tool gracefully handles keys that are not present in the encryptedKeyMap instead of throwing a runtime exception.

Approach

The crash was caused by the tool attempting to encrypt keys found in sections following the [secrets] block. When the tool encountered a key from a subsequent section (e.g., [test_section]), it tried to look up its value in the encryptedKeyMap. Since the key was not a secret, the map returned null. The code then attempted to concatenate this null value (.concat(value)), resulting in a NullPointerException.

I implemented a null check for the retrieved value. If the value is null, the code now skips the line modification, preserving the configuration as-is without crashing.

User stories

As a System Administrator or DevOps Engineer, I want to be able to add custom configuration sections to the bottom of my deployment.toml file without causing the ciphertool script to crash, so that I can safely manage my environment configurations and secrets.

Release note

Fixed a NullPointerException in the CipherTool utility that occurred when deployment.toml contained configuration sections placed after the [secrets] block.

Documentation

N/A - This is a bug fix for an internal utility and does not require documentation updates.

Training

N/A

Certification

N/A

Marketing

N/A

Automation tests

• Unit tests

  N/A - Manual verification was performed.

• Integration tests

  Verified manually by:

  1. Modifying deployment.toml in WSO2 API Manager 4.6.0 to include a [test_section] after [secrets].
  2. Running ./ciphertool.bat -Dconfigure -Dpassword=wso2carbon.
  3. Confirming the tool completes successfully and secrets are encrypted.

Security checks

• Followed secure coding standards in http://wso2.com/technical-reports/wso2-secure-engineering-guidelines? yes
• Ran FindSecurityBugs plugin and verified report? yes
• Confirmed that this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets? yes

Samples

N/A

Related PRs

N/A

Migrations (if applicable)

N/A

Test environment

• JDK: Amazon Corretto 11.0.29
• OS: Windows 11
• Product: WSO2 API Manager 4.6.0

Learning

Identified that the tokenizer logic in CipherTool.java did not account for scenarios where keys might be parsed from non-secret sections if the section parsing logic allows fall-through, leading to null values in the encryption map.

Summary by CodeRabbit

• Bug Fixes
  • Enhanced deployment configuration encryption to prevent invalid null values from being inserted during the encryption process, improving overall reliability of encrypted configuration handling.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[wso2-engineering]

AI Agent Log Improvement Checklist

⚠️ Warning: AI-Generated Review Comments

• The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
• Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.

✅ Before merging this pull request:

• Review all AI-generated comments for accuracy and relevance.
• Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.

Comment	Accepted (Y/N)	Reason
#### Log Improvement Suggestion No: 1

[coderabbitai]

Walkthrough

The change introduces a null-check in CipherTool.java before replacing deployment configuration lines with encrypted values. When a value from the encryptedKeyMap is null, the line replacement is skipped, preventing NullPointerException errors during the cipher tool's configuration update flow.

Changes

Cohort / File(s)	Summary
Null-safety enhancement components/ciphertool/src/main/java/org/wso2/ciphertool/CipherTool.java	Adds null-check guard in the deployment configuration update logic to prevent NullPointerException when encrypted value is null, skipping line modification in such cases

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

• Single null-check addition in a localized area of the code
• Straightforward fix addressing a specific exception case
• No logic restructuring or API changes

Poem

🐰 A null pointer's bite caused quite the fright,
When secrets held nothing but air,
But now with a check, we prevent the wreck,
The cipher tool works without care! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly indicates the main change: fixing an NPE in CipherTool when deployment.toml has configurations after a specific section, which aligns with the core issue being addressed.
Description check	✅ Passed	The description comprehensively covers all major template sections including purpose, goals, approach, user stories, release notes, testing verification, and security checks, providing sufficient detail for understanding the fix.
Linked Issues check	✅ Passed	The PR successfully addresses the linked issue #4561 by implementing a null check to prevent NullPointerException when deployment.toml contains configuration sections after [secrets], exactly as required.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the NPE issue in CipherTool by adding a null check for retrieved values, with no extraneous modifications detected.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

components/ciphertool/src/main/java/org/wso2/ciphertool/CipherTool.java (1)

387-390: Null check prevents NPE; consider adding logging as previously suggested.

The null check correctly prevents the NullPointerException when a key is not found in the encryptedKeyMap. However, the fix silently skips the replacement, which could hide configuration issues where expected keys are missing from the encryption map.

As the previous bot review suggested, consider adding logging to improve observability:

                                String value = encryptedKeyMap.get(key.trim());
                                // Fix: Check if value exists before replacing
                                if (value != null) {
+                                   log.debug("Updating deployment configuration for key: " + key);
                                    line = key.concat("= \"").concat(value).concat("\"");
+                               } else {
+                                   log.warn("No encrypted value found for key: " + key + ". Skipping update.");
                                }

Based on past review comments.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ec79069 and 239a9f1.

📒 Files selected for processing (1)

• components/ciphertool/src/main/java/org/wso2/ciphertool/CipherTool.java (1 hunks)

[PasinduSuraweera]

Hi @arunans23, following up on this fix for Issue wso2/api-manager#4561. It fixes a NullPointerException in the CipherTool (Issue wso2/api-manager#4561) that occurs when custom sections are added to deployment.toml.

I have verified the fix locally with APIM 4.6.0.

I’d appreciate a review whenever you have a moment.
Thanks!