Skip to content

Dwn 39926 input validation #31

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

Merged
merged 7 commits into from
Feb 1, 2023
Merged

Dwn 39926 input validation #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b6f361b
DWN-39926 : validate whitelist scopes are alphabet characters
hsmith-gresham Jan 3, 2023
0bea906
DWN-39926 : amend exception name in log messages
hsmith-gresham Jan 4, 2023
32240a5
DWN-39926 : use put rather than addAttribute for updating whitelists
hsmith-gresham Jan 4, 2023
9325917
DWN-39926 : validate create and update scope directly
hsmith-gresham Jan 18, 2023
42b6aa5
DWN-39926 : use patterns to avoid multiple compilation
hsmith-gresham Jan 23, 2023
9119ddd
DWN-39926 : validate scopes on manage client page
hsmith-gresham Jan 26, 2023
46b0312
DWN-39926 : pass whole exception not just the message
hsmith-gresham Jan 26, 2023
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
28 changes: 27 additions & 1 deletion openid-connect-server/src/main/java/org/mitre/oauth2/web/ScopeAPI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@
package org.mitre.oauth2.web;

import java.util.Set;
import java.util.regex.Pattern;

import org.mitre.oauth2.model.SystemScope;
import org.mitre.oauth2.service.SystemScopeService;
import org.mitre.openid.connect.exception.ScopeException;
import org.mitre.openid.connect.view.HttpCodeView;
import org.mitre.openid.connect.view.JsonEntityView;
import org.mitre.openid.connect.view.JsonErrorView;
Expand All @@ -33,6 +35,7 @@
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpStatus;
import org.springframework.http.MediaType;
import org.springframework.security.access.method.P;
import org.springframework.security.access.prepost.PreAuthorize;
import org.springframework.stereotype.Controller;
import org.springframework.ui.ModelMap;
Expand All @@ -54,6 +57,8 @@ public class ScopeAPI {

public static final String URL = RootController.API_URL + "/scopes";

private static final String characterMatcher = "[a-zA-Z]+";
private static final Pattern pattern = Pattern.compile(characterMatcher);
@Autowired
private SystemScopeService scopeService;

Expand Down Expand Up @@ -101,7 +106,14 @@ public String updateScope(@PathVariable("id") Long id, @RequestBody String json,
SystemScope existing = scopeService.getById(id);

SystemScope scope = gson.fromJson(json, SystemScope.class);

try {
validateScope(scope);
} catch (ScopeException e) {
logger.error("updateScope failed due to ScopeException", e);
m.put(HttpCodeView.CODE, HttpStatus.BAD_REQUEST);
m.put(JsonErrorView.ERROR_MESSAGE, "Could not update scope. The server encountered a scope exception. Contact a system administrator for assistance.");
return JsonErrorView.VIEWNAME;
}
if (existing != null && scope != null) {

if (existing.getId().equals(scope.getId())) {
Expand Down Expand Up @@ -138,6 +150,14 @@ public String createScope(@RequestBody String json, ModelMap m) {
SystemScope scope = gson.fromJson(json, SystemScope.class);

SystemScope alreadyExists = scopeService.getByValue(scope.getValue());
try {
validateScope(scope);
} catch (ScopeException e) {
logger.error("createScope failed due to ScopeException", e);
m.put(HttpCodeView.CODE, HttpStatus.BAD_REQUEST);
m.put(JsonErrorView.ERROR_MESSAGE, "Could not create scope. The server encountered a scope exception. Contact a system administrator for assistance.");
return JsonErrorView.VIEWNAME;
}
if (alreadyExists != null) {
//Error, cannot save a scope with the same value as an existing one
logger.error("Error: attempting to save a scope with a value that already exists: " + scope.getValue());
Expand All @@ -163,6 +183,12 @@ public String createScope(@RequestBody String json, ModelMap m) {
}
}

private void validateScope(SystemScope scope) throws ScopeException {
if (!pattern.matcher(scope.getValue()).matches()) {
throw new ScopeException(scope.getValue());
}
}

@PreAuthorize("hasRole('ROLE_ADMIN')")
@RequestMapping(value = "/{id}", method = RequestMethod.DELETE)
public String deleteScope(@PathVariable("id") Long id, ModelMap m) {
Expand Down
25 changes: 25 additions & 0 deletions openid-connect-server/src/main/java/org/mitre/openid/connect/exception/ScopeException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/**
* <copyright>
* <p>
* Copyright (c) 2010-2023 Gresham Technologies plc. All rights reserved.
*
* </copyright>
*/
package org.mitre.openid.connect.exception;

/**
* @author hwsmith
*/
public class ScopeException extends Exception {

private final String invalidScope;

public ScopeException(String invalidScope) {
this.invalidScope = invalidScope;
}

public String getMessage() {
return "The scope " + invalidScope + " is invalid as it contains non-alphabet characters";
}

}
30 changes: 26 additions & 4 deletions openid-connect-server/src/main/java/org/mitre/openid/connect/web/ClientAPI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import java.sql.SQLIntegrityConstraintViolationException;
import java.text.ParseException;
import java.util.Collection;
import java.util.Set;
import java.util.regex.Pattern;

import javax.persistence.PersistenceException;

Expand All @@ -33,9 +35,8 @@
import org.mitre.oauth2.model.PKCEAlgorithm;
import org.mitre.oauth2.service.ClientDetailsEntityService;
import org.mitre.oauth2.web.AuthenticationUtilities;
import org.mitre.openid.connect.exception.ScopeException;
import org.mitre.openid.connect.exception.ValidationException;
import org.mitre.openid.connect.model.CachedImage;
import org.mitre.openid.connect.service.ClientLogoLoadingService;
import org.mitre.openid.connect.view.ClientEntityViewForAdmins;
import org.mitre.openid.connect.view.ClientEntityViewForUsers;
import org.mitre.openid.connect.view.HttpCodeView;
Expand All @@ -45,10 +46,8 @@
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpStatus;
import org.springframework.http.MediaType;
import org.springframework.http.ResponseEntity;
import org.springframework.security.access.prepost.PreAuthorize;
import org.springframework.security.core.Authentication;
import org.springframework.security.oauth2.common.util.OAuth2Utils;
Expand Down Expand Up @@ -130,6 +129,9 @@ public class ClientAPI {

public static final String URL = RootController.API_URL + "/clients";

private static final String characterMatcher = "[a-zA-Z]+";
private static final Pattern pattern = Pattern.compile(characterMatcher);

@Autowired
private ClientDetailsEntityService clientService;

Expand Down Expand Up @@ -256,6 +258,12 @@ public String apiAddClient(@RequestBody String jsonString, Model m, Authenticati
json = parser.parse(jsonString).getAsJsonObject();
client = gson.fromJson(json, ClientDetailsEntity.class);
client = validateSoftwareStatement(client);
validateScopes(client.getScope());
} catch (ScopeException e) {
logger.error("apiAddClient failed due to ScopeException", e);
m.addAttribute(HttpCodeView.CODE, HttpStatus.BAD_REQUEST);
m.addAttribute(JsonErrorView.ERROR_MESSAGE, "Could not save new client. The server encountered a scope exception. Contact a system administrator for assistance.");
return JsonErrorView.VIEWNAME;
} catch (JsonSyntaxException e) {
logger.error("apiAddClient failed due to JsonSyntaxException", e);
m.addAttribute(HttpCodeView.CODE, HttpStatus.BAD_REQUEST);
Expand Down Expand Up @@ -369,6 +377,12 @@ public String apiUpdateClient(@PathVariable("id") Long id, @RequestBody String j
json = parser.parse(jsonString).getAsJsonObject();
client = gson.fromJson(json, ClientDetailsEntity.class);
client = validateSoftwareStatement(client);
validateScopes(client.getScope());
} catch (ScopeException e) {
logger.error("apiUpdateClient failed due to ScopeException", e);
m.addAttribute(HttpCodeView.CODE, HttpStatus.BAD_REQUEST);
m.addAttribute(JsonErrorView.ERROR_MESSAGE, "Could not update client. The server encountered a scope exception. Contact a system administrator for assistance.");
return JsonErrorView.VIEWNAME;
} catch (JsonSyntaxException e) {
logger.error("apiUpdateClient failed due to JsonSyntaxException", e);
m.addAttribute(HttpCodeView.CODE, HttpStatus.BAD_REQUEST);
Expand Down Expand Up @@ -460,6 +474,14 @@ public String apiUpdateClient(@PathVariable("id") Long id, @RequestBody String j
}
}

private void validateScopes(Set<String> scopes) throws ScopeException {
for (String s : scopes) {
if (!pattern.matcher(s).matches()) {
throw new ScopeException(s);
}
}
}

/**
* Delete a client
* @param id
Expand Down
27 changes: 25 additions & 2 deletions openid-connect-server/src/main/java/org/mitre/openid/connect/web/WhitelistAPI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@

import java.security.Principal;
import java.util.Collection;
import java.util.Set;
import java.util.regex.Pattern;

import org.mitre.openid.connect.exception.ScopeException;
import org.mitre.openid.connect.model.WhitelistedSite;
import org.mitre.openid.connect.service.WhitelistedSiteService;
import org.mitre.openid.connect.view.HttpCodeView;
Expand Down Expand Up @@ -56,6 +59,8 @@
public class WhitelistAPI {

public static final String URL = RootController.API_URL + "/whitelist";
private static final String characterMatcher = "[a-zA-Z]+";
private static final Pattern pattern = Pattern.compile(characterMatcher);

@Autowired
private WhitelistedSiteService whitelistService;
Expand Down Expand Up @@ -100,7 +105,12 @@ public String addNewWhitelistedSite(@RequestBody String jsonString, ModelMap m,
try {
json = parser.parse(jsonString).getAsJsonObject();
whitelist = gson.fromJson(json, WhitelistedSite.class);

validateWhitelistScopes(whitelist.getAllowedScopes());
} catch (ScopeException e) {
logger.error("addNewWhitelistedSite failed due to ScopeException", e);
m.addAttribute(HttpCodeView.CODE, HttpStatus.BAD_REQUEST);
m.addAttribute(JsonErrorView.ERROR_MESSAGE, "Could not save new whitelisted site. The server encountered a scopes exception. Contact a system administrator for assistance.");
return JsonErrorView.VIEWNAME;
} catch (JsonParseException e) {
logger.error("addNewWhitelistedSite failed due to JsonParseException", e);
m.addAttribute(HttpCodeView.CODE, HttpStatus.BAD_REQUEST);
Expand Down Expand Up @@ -137,7 +147,12 @@ public String updateWhitelistedSite(@PathVariable("id") Long id, @RequestBody St
try {
json = parser.parse(jsonString).getAsJsonObject();
whitelist = gson.fromJson(json, WhitelistedSite.class);

validateWhitelistScopes(whitelist.getAllowedScopes());
} catch (ScopeException e) {
logger.error("updateWhitelistedSite failed due to ScopeException", e);
m.put(HttpCodeView.CODE, HttpStatus.BAD_REQUEST);
m.put(JsonErrorView.ERROR_MESSAGE, "Could not update whitelisted site. The server encountered a scope exception. Contact a system administrator for assistance.");
return JsonErrorView.VIEWNAME;
} catch (JsonParseException e) {
logger.error("updateWhitelistedSite failed due to JsonParseException", e);
m.put(HttpCodeView.CODE, HttpStatus.BAD_REQUEST);
Expand Down Expand Up @@ -167,6 +182,14 @@ public String updateWhitelistedSite(@PathVariable("id") Long id, @RequestBody St
}
}

private void validateWhitelistScopes(Set<String> scopes) throws ScopeException {
for (String s : scopes) {
if (!pattern.matcher(s).matches()) {
throw new ScopeException(s);
}
}
}

/**
* Delete a whitelisted site
*
Expand Down