Skip to content

Commit

Permalink
Security bugfix for XSS window.location.href
Browse files Browse the repository at this point in the history
  • Loading branch information
oharsta committed Oct 18, 2023
1 parent b3a8847 commit ddf368e
Show file tree
Hide file tree
Showing 11 changed files with 67 additions and 20 deletions.
5 changes: 3 additions & 2 deletions client/src/utils/Login.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {isEmpty} from "./Utils";
import {isEmpty, sanitizeURL} from "./Utils";


export function login(config, force = false, hash = null) {
let params = force ? `?force=true` : "";
Expand All @@ -11,5 +12,5 @@ export function login(config, force = false, hash = null) {
serverUrl = local ? "http://localhost:8080" :
`${window.location.protocol}//${window.location.host}`
}
window.location.href = `${serverUrl}/api/v1/users/login${params}`;
window.location.href = sanitizeURL(`${serverUrl}/api/v1/users/login${params}`);
}
5 changes: 5 additions & 0 deletions client/src/utils/Utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,8 @@ export function pseudoGuid() {
export const splitListSemantically = (arr, lastSeparator) => {
return [arr.slice(0, -1).join(", "), arr.slice(-1)[0]].join(arr.length < 2 ? "" : ` ${lastSeparator} `);
}

export const sanitizeURL = url => {
const protocol = new URL(url).protocol;
return ["https", "http"].includes(protocol) ? url : "about:blank";
}
5 changes: 5 additions & 0 deletions client/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -9249,6 +9249,11 @@ uri-js@^4.2.2:
dependencies:
punycode "^2.1.0"

urisanity@^0.1.2:
version "0.1.2"
resolved "https://registry.yarnpkg.com/urisanity/-/urisanity-0.1.2.tgz#abe4010666ee745a3a20c49dadac633078f2a3e4"
integrity sha512-0tDbl+8E8oRUYWJhX3N4HtGiSfpEzpCoSPirvQg1nN8U6f/JTRGlCUCFadmc04mCoyLga//11JpoZFYuaVM30g==

url-parse@^1.5.3:
version "1.5.10"
resolved "https://registry.yarnpkg.com/url-parse/-/url-parse-1.5.10.tgz#9d3c2f736c1d75dd3bd2be507dcc111f1e2ea9c1"
Expand Down
16 changes: 12 additions & 4 deletions server/src/main/java/access/api/RoleController.java
Original file line number Diff line number Diff line change
@@ -1,16 +1,21 @@
package access.api;

import access.config.Config;
import access.exception.InvalidInputException;
import access.exception.NotAllowedException;
import access.exception.NotFoundException;
import access.logging.AccessLogger;
import access.logging.Event;
import access.manage.Manage;
import access.model.*;
import access.model.Authority;
import access.model.Role;
import access.model.RoleExists;
import access.model.User;
import access.provision.ProvisioningService;
import access.repository.RoleRepository;
import access.provision.scim.GroupURN;
import access.repository.RoleRepository;
import access.security.UserPermissions;
import access.validation.URLFormatValidator;
import io.swagger.v3.oas.annotations.Parameter;
import io.swagger.v3.oas.annotations.security.SecurityRequirement;
import org.apache.commons.logging.Log;
Expand All @@ -19,7 +24,6 @@
import org.springframework.http.MediaType;
import org.springframework.http.ResponseEntity;
import org.springframework.transaction.annotation.Transactional;
import org.springframework.util.StringUtils;
import org.springframework.validation.annotation.Validated;
import org.springframework.web.bind.annotation.*;

Expand All @@ -40,6 +44,7 @@ public class RoleController {
private final RoleRepository roleRepository;
private final Manage manage;
private final ProvisioningService provisioningService;
private final URLFormatValidator urlFormatValidator = new URLFormatValidator();

public RoleController(Config config,
RoleRepository roleRepository,
Expand Down Expand Up @@ -67,7 +72,7 @@ public ResponseEntity<List<Role>> rolesByApplication(@Parameter(hidden = true) U
.filter(userRole -> userRole.getAuthority().hasEqualOrHigherRights(Authority.MANAGER))
.map(userRole -> userRole.getRole().getManageId())
.collect(Collectors.toSet());
roles.addAll(roleRepository.findByManageIdIn(manageIdentifiers)) ;
roles.addAll(roleRepository.findByManageIdIn(manageIdentifiers));
return ResponseEntity.ok(manage.deriveRemoteApplications(roles));
}

Expand Down Expand Up @@ -132,6 +137,9 @@ public ResponseEntity<Void> deleteRole(@PathVariable("id") Long id, @Parameter(h
}

private ResponseEntity<Role> saveOrUpdate(Role role, User user) {
if (!urlFormatValidator.isValid(role.getLandingPage())) {
throw new InvalidInputException();
}
Map<String, Object> provider = manage.providerById(role.getManageType(), role.getManageId());
UserPermissions.assertManagerRole(provider, user);
boolean isNew = role.getId() == null;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package access.exception;

import org.springframework.http.HttpStatus;
import org.springframework.web.bind.annotation.ResponseStatus;

@ResponseStatus(HttpStatus.BAD_REQUEST)
public class InvalidInputException extends RuntimeException {
}
19 changes: 18 additions & 1 deletion server/src/test/java/access/api/RoleControllerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,23 @@ void create() throws Exception {
assertNotNull(result.get("id"));
}

@Test
void createInvalidLandingPage() throws Exception {
AccessCookieFilter accessCookieFilter = openIDConnectFlow("/api/v1/users/login", MANAGE_SUB);
Role role = new Role("New", "New desc", "javascript:alert(42)", "1", EntityType.SAML20_SP, 365, false, false);

given()
.when()
.filter(accessCookieFilter.cookieFilter())
.accept(ContentType.JSON)
.header(accessCookieFilter.csrfToken().getHeaderName(), accessCookieFilter.csrfToken().getToken())
.contentType(ContentType.JSON)
.body(role)
.post("/api/v1/roles")
.then()
.statusCode(400);
}

@Test
void createProvisionException() throws Exception {
AccessCookieFilter accessCookieFilter = openIDConnectFlow("/api/v1/users/login", MANAGE_SUB);
Expand Down Expand Up @@ -175,7 +192,7 @@ void rolesByApplicationInstitutionAdmin() throws Exception {
super.stubForManageProviderByOrganisationGUID(ORGANISATION_GUID);

AccessCookieFilter accessCookieFilter = openIDConnectFlow("/api/v1/users/me", INSTITUTION_ADMIN,
m -> {
m -> {
m.put("eduperson_entitlement",
List.of(
"urn:mace:surfnet.nl:surfnet.nl:sab:role:SURFconextverantwoordelijke",
Expand Down
6 changes: 3 additions & 3 deletions welcome/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,22 @@
"js-cookie": "^3.0.1",
"luxon": "^3.3.0",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"react-copy-to-clipboard": "^5.0.1",
"react-dom": "^18.2.0",
"react-router": "^6.11.1",
"react-router-dom": "^6.11.2",
"react-tooltip": "^5.11.2",
"timeago.js": "^4.0.2",
"zustand": "^4.3.8"
},
"devDependencies": {
"@babel/plugin-proposal-private-property-in-object": "^7.21.11",
"@testing-library/jest-dom": "^5.16.5",
"@testing-library/react": "^13.4.0",
"@testing-library/user-event": "^13.5.0",
"http-proxy-middleware": "^2.0.6",
"react-scripts": "5.0.1",
"sass": "^1.62.1",
"@babel/plugin-proposal-private-property-in-object": "^7.21.11"
"sass": "^1.62.1"
},
"resolutions": {
"nth-check": "^2.1.1",
Expand Down
4 changes: 3 additions & 1 deletion welcome/src/components/RoleCard.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import Logo from "./Logo";
import I18n from "../locale/I18n";
import {MoreLessText} from "./MoreLessText";
import {Button, Card, CardType, Chip, ChipType} from "@surfnet/sds";
import {sanitizeURL} from "../utils/Utils";


export const RoleCard = ({role, index, isNew = false, skipLaunch= false}) => {

Expand All @@ -19,7 +21,7 @@ export const RoleCard = ({role, index, isNew = false, skipLaunch= false}) => {
</section>
{!skipLaunch && <div className={"launch"}>
<Button txt={I18n.t("proceed.launch")} onClick={() => {
window.location.href = role.landingPage;
window.location.href = sanitizeURL(role.landingPage);
}}/>
</div>}

Expand Down
5 changes: 2 additions & 3 deletions welcome/src/pages/Proceed.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import "../styles/circle.scss";
import DOMPurify from "dompurify";
import {Loader, Modal, Toaster, ToasterType, Tooltip} from "@surfnet/sds";
import {useAppStore} from "../stores/AppStore";
import {isEmpty, stopEvent} from "../utils/Utils";
import {isEmpty, sanitizeURL, stopEvent} from "../utils/Utils";
import {getParameterByName} from "../utils/QueryParameters";
import {invitationByHash, logout} from "../api";
import {login} from "../utils/Login";
Expand All @@ -15,7 +15,6 @@ import {User} from "../components/User";
import HighFive from "../icons/high-five.svg";
import {useNavigate} from "react-router-dom";


export const Proceed = () => {

const {user, invitation, config} = useAppStore(state => state);
Expand Down Expand Up @@ -105,7 +104,7 @@ export const Proceed = () => {
if (isEmpty(inviteRedeemUrl)) {
setShowModal(false);
} else {
window.location.href = inviteRedeemUrl
window.location.href = sanitizeURL(inviteRedeemUrl);
}
}

Expand Down
4 changes: 2 additions & 2 deletions welcome/src/utils/Login.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {isEmpty} from "./Utils";
import {isEmpty, sanitizeURL} from "./Utils";

export function login(config, force = false, hash = null) {
let params = "?app=welcome&"
Expand All @@ -14,5 +14,5 @@ export function login(config, force = false, hash = null) {
serverWelcomeUrl = local ? "http://localhost:8080" :
`${window.location.protocol}//${window.location.host}`
}
window.location.href = `${serverWelcomeUrl}/api/v1/users/login${params}`;
window.location.href = sanitizeURL(`${serverWelcomeUrl}/api/v1/users/login${params}`);
}
10 changes: 6 additions & 4 deletions welcome/src/utils/Utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,12 @@ export function isEmpty(obj) {
return false;
}

export function pseudoGuid() {
return (crypto.randomUUID && crypto.randomUUID()) || Math.round((new Date().getTime() * Math.random() * 1000)).toString()
}

export const splitListSemantically = (arr, lastSeparator) => {
return [arr.slice(0, -1).join(", "), arr.slice(-1)[0]].join(arr.length < 2 ? "" : ` ${lastSeparator} `);
}

export const sanitizeURL = url => {
const protocol = new URL(url).protocol;
return ["https", "http"].includes(protocol) ? url : "about:blank";
}

0 comments on commit ddf368e

Please sign in to comment.