Copilot commented on code in PR #13209:
URL: https://github.com/apache/cloudstack/pull/13209#discussion_r3978925534
##########
server/src/main/java/com/cloud/api/ApiServlet.java:
##########
@@ -216,6 +216,7 @@ void processRequestInContext(final HttpServletRequest req,
final HttpServletResp
HttpUtils.RESPONSE_TYPE_XML,
ApiServer.JSONcontentType.value());
return;
}
+ CallContext.current().setRequestRemoteAddress(req.getServerName());
Review Comment:
req.getServerName() is derived from the incoming Host header / request URL
and can be client-controlled in many deployments; persisting it into
CallContext and using it to build password-reset links can enable host-header
injection/phishing unless it is strictly validated/whitelisted (e.g., against
configured GUI theme CNs or a dedicated allowed domain list) and includes the
correct external scheme/port.
##########
server/src/main/java/org/apache/cloudstack/user/UserPasswordResetManagerImpl.java:
##########
@@ -182,26 +189,12 @@ public void setResetTokenAndSend(UserAccount userAccount)
{
final String email = userAccount.getEmail();
final String username = userAccount.getUsername();
final String subject = "Password Reset Request";
- String domainUrl = UserPasswordResetDomainURL.value();
- if (StringUtils.isBlank(domainUrl)) {
- String mgmtServerAddr =
ManagementServerAddresses.value().split(",")[0];
- if (ServerProperties.isHttpsEnabled()) {
- domainUrl = "https://"; + mgmtServerAddr + ":" +
ServerProperties.getHttpsPort();
- } else {
- domainUrl = "http://"; + mgmtServerAddr + ":" +
ServerProperties.getHttpPort();
- }
- } else if (!domainUrl.startsWith("http://";) &&
!domainUrl.startsWith("https://";)) {
- if (ServerProperties.isHttpsEnabled()) {
- domainUrl = "https://"; + domainUrl;
- } else {
- domainUrl = "http://"; + domainUrl;
- }
- }
-
- domainUrl = domainUrl.replaceAll("/+$", "");
+ String requestDomain = CallContext.current().getRequestRemoteAddress();
+ String resetLinkDomain = getResetLinkDomain(requestDomain);
+ String formattedResetLinkDomain =
formatResetLinkDomain(resetLinkDomain);
String resetLink =
String.format("%s/client/#/user/resetPassword?username=%s&token=%s",
- domainUrl, username, resetToken);
+ formattedResetLinkDomain, username, resetToken);
Review Comment:
`requestDomain` is sourced from CallContext’s `requestRemoteAddress`, which
is currently populated with `req.getServerName()` (host only, no port). When
the UI/API is accessed on a non-default port (e.g., :8080/:8443) without a
reverse proxy, the generated reset link will omit the port for hostname-based
domains, producing a broken link.
##########
api/src/main/java/org/apache/cloudstack/context/CallContext.java:
##########
@@ -66,6 +66,7 @@ protected Stack<CallContext> initialValue() {
private final Map<String, String> apiResourcesUuids = new HashMap<>();
private Project project;
private String apiName;
+ private String requestRemoteAddress;
Review Comment:
The new CallContext field name `requestRemoteAddress` is misleading:
ApiServlet sets it from `req.getServerName()` (the server/domain being
accessed), not the client remote address. This increases the risk of future
misuse (e.g., using it for auditing/security decisions) and makes the
password-reset flow harder to reason about.
##########
server/src/main/java/org/apache/cloudstack/user/UserPasswordResetManagerImpl.java:
##########
@@ -222,6 +215,44 @@ public void setResetTokenAndSend(UserAccount userAccount) {
userAccount, userAccount.getAccountId(),
userAccount.getDomainId(), email, resetTokenExpiryTime);
}
+ private String getResetLinkDomain(String requestDomain) {
+ if (StringUtils.isNotBlank(requestDomain)) {
+ logger.debug("Searching for GUI theme with common name that
matches the request's domain: [{}]", requestDomain);
+ List<Long> commonNameDetails =
guiThemeDetailsDao.listGuiThemeIdsByCommonName(requestDomain);
+
+ if (!commonNameDetails.isEmpty()) {
+ logger.debug("GUI theme with ID {} was found; using request's
domain for password reset link.", commonNameDetails.get(0));
+ return requestDomain;
+ } else {
+ logger.debug("No GUI theme was found with a common name that
matches the request's domain.");
+ }
+ }
+
+ String configurationDomain = UserPasswordResetDomainURL.value();
+ if (StringUtils.isNotBlank(configurationDomain)) {
+ logger.debug("Defaulting reset link's domain to the [{}]
configuration value: [{}].", UserPasswordResetDomainURL.key(),
UserPasswordResetDomainURL.value());
+ return configurationDomain;
+ }
+
+ logger.debug("Using the first IP address in the [{}] configuration for
the reset password email domain because the [{}] configuration is not
defined.", ManagementServerAddresses.key(), UserPasswordResetDomainURL.key());
+ return ManagementServerAddresses.value().split(",")[0];
+ }
+
+ private String formatResetLinkDomain(String resetLinkDomain) {
+ String protocol = ServerProperties.isHttpsEnabled() ? "https" : "http";
+
+ if (InetAddressUtils.isIPv4Address(resetLinkDomain)) {
+ int port = protocol.equals("https") ?
ServerProperties.getHttpsPort() : ServerProperties.getHttpPort();
+ resetLinkDomain = resetLinkDomain + ":" + port;
+ }
+
+ if (!resetLinkDomain.startsWith("http")) {
+ resetLinkDomain = protocol + "://" + resetLinkDomain;
+ }
+
+ return resetLinkDomain.replaceAll("/+$", "");
+ }
Review Comment:
The new domain-selection and formatting logic (`getResetLinkDomain` /
`formatResetLinkDomain`) introduces multiple branches (GUI theme match, config
fallback, host fallback, IPv4 vs hostname scheme handling) but there are no
unit tests covering these behaviors, even though this component already has a
dedicated test class.
This issue also appears in the following locations of the same file:
- line 220
- line 231
- line 249
--
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]
