Copilot commented on code in PR #12372:
URL: https://github.com/apache/cloudstack/pull/12372#discussion_r2767652958
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -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);
+ }
}
Review Comment:
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.
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -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);
+ }
}
Review Comment:
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.
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -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);
+ }
}
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
