Skip to content

Fix NPE during public IP listing when a removed network or VPC ID is informed for associatenetworkid parameter #12372

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
erikbocks wants to merge 1 commit into
base: 4.20
Choose a base branch
from
+14 −4
Open

Fix NPE during public IP listing when a removed network or VPC ID is informed for associatenetworkid parameter #12372

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions server/src/main/java/com/cloud/server/ManagementServerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import javax.inject.Inject;
import javax.naming.ConfigurationException;

import com.cloud.network.vpc.VpcVO;
import org.apache.cloudstack.acl.ControlledEntity;
import org.apache.cloudstack.acl.SecurityChecker;
import org.apache.cloudstack.affinity.AffinityGroupProcessor;
Expand Down Expand Up @@ -2580,12 +2581,21 @@ public Pair<List<? extends IpAddress>, Integer> searchForIPAddresses(final ListP
}

if (associatedNetworkId != null) {
_accountMgr.checkAccess(caller, null, false, networkDao.findById(associatedNetworkId));
sc.setParameters("associatedNetworkIdEq", associatedNetworkId);
NetworkVO associatedNetwork = networkDao.findById(associatedNetworkId);

if (associatedNetwork != null) {
_accountMgr.checkAccess(caller, null, false, associatedNetwork);
sc.setParameters("associatedNetworkIdEq", associatedNetworkId);
}
}

if (vpcId != null) {
_accountMgr.checkAccess(caller, null, false, _vpcDao.findById(vpcId));
sc.setParameters("vpcId", vpcId);
VpcVO vpc = _vpcDao.findById(vpcId);

if (vpc != null) {
_accountMgr.checkAccess(caller, null, false, vpc);
sc.setParameters("vpcId", vpcId);
}
}
Comment on lines 2583 to 2599
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix addresses the NPE on lines 2584-2598, but there's a similar vulnerability later in the same method. Lines 2614-2621 also retrieve the network using findById and directly access its properties (getDataCenterId() on line 2616 and getAccountId() on line 2621) without null checks. This code path is executed when listing free IP addresses (when isAllocatedOrReserved is false and vlanType is not DirectAttached). If a removed network ID is provided, this will still result in a NullPointerException.

The same null check pattern should be applied here to prevent this additional NPE scenario.

Copilot uses AI. Check for mistakes.
Comment on lines 2583 to 2599
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current fix silently ignores removed networks and VPCs by not adding them to the search criteria when they are null. However, this behavior is inconsistent with other parts of the codebase. For example, at lines 2507-2509 and 2514-2515, when an entity is not found (null), the code throws an InvalidParameterValueException with a descriptive error message.

Consider whether the API should throw an InvalidParameterValueException when a user explicitly provides an ID for a removed or non-existent network/VPC, rather than silently returning an empty result set. This would provide clearer feedback to API consumers about why their query is not returning expected results. The current behavior could be confusing - users might think there are simply no IPs associated with that network, when in reality the network itself doesn't exist or has been removed.

Copilot uses AI. Check for mistakes.
Comment on lines 2583 to 2599
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix changes the behavior of the listPublicIpAddresses API when given a removed network or VPC ID. Consider adding test cases to verify this new behavior, specifically:

  1. Test that no NullPointerException is thrown when a User account calls listPublicIpAddresses with a removed network ID
  2. Test that no NullPointerException is thrown when a User account calls listPublicIpAddresses with a removed VPC ID
  3. Test the expected return value when a removed entity ID is provided (currently returns empty list)

The existing tests in ManagementServerImplTest.java test the setParameters method but don't cover the access control check scenario that was causing the NPE.

Copilot uses AI. Check for mistakes.

addrs = _publicIpAddressDao.search(sc, searchFilter); // Allocated
Expand Down
Loading