Skip to content
This repository has been archived by the owner on Oct 2, 2018. It is now read-only.

Frontend passes target to backend #119

Closed
wants to merge 13 commits into from
11 changes: 7 additions & 4 deletions backend/src/api/base.clj
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,16 @@
(player-view/get-game-as-player (persistence/fetch-game game-id) player-id))

(defn play-card-as-player
[game-id player index row-id & target]
(let [game-state (persistence/fetch-game game-id)]
([game-id player index row-id]
(play-card-as-player game-id player index row-id "nil"))
([game-id player index row-id target]
(let [game-state (persistence/fetch-game game-id)
target (load-string target)]
Copy link
Owner

Choose a reason for hiding this comment

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

load-string is a security issue, it is executing arbitrary Clojure code contained in a string received from an external connection.
A malicious user could alter the value being sent as target and run any kind of code on the system, compromising the entire backend.

Copy link
Owner

Choose a reason for hiding this comment

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

Just as a reminder: this pull request cannot be accepted until it doesn't use vulnerable things such as load-string

(if (= (:status (get-game game-id player)) messages/play)
(do
(persistence/save-game (play-card/play-card game-state (conversions/player-num game-state player) index row-id (first target)))
(persistence/save-game (play-card/play-card game-state (conversions/player-num game-state player) index row-id target))
(get-game game-id player))
{:error messages/out-of-turn})))
{:error messages/out-of-turn}))))

(defn ^:private create-empty-game
"Creates a new instance of a game"
Expand Down
3 changes: 2 additions & 1 deletion backend/src/api/handler.clj
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
game-id
player-id
(parse-int (:index action))
(parse-int (:row action))))
(parse-int (:row action))
(:target action)))

(defroutes ^:private app-routes
(context "/games" [] (defroutes game-list-routes
Expand Down
2 changes: 1 addition & 1 deletion backend/test/api/effects_test.clj
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@
(api/play-card-as-player game-id player-id 0 0)
(api/play-card-as-player game-id opponent-id 0 1)
(api/play-card-as-player game-id player-id 0 1)
(api/play-card-as-player game-id opponent-id 0 1 [:rows 0 :cards 0])
(api/play-card-as-player game-id opponent-id 0 1 "[:rows 0 :cards 0]")
(get-in (api/get-game game-id player-id)
[:rows 0 :cards 0 :power]))))
4 changes: 3 additions & 1 deletion frontend/src/game/board/board.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,18 @@ module.exports = {
})

boardState.forEach(function (row, rownum) {
row["cards"].forEach(function (cardInRow) {
row["cards"].forEach(function (cardInRow, index) {
var newCard = builder.buildCard(templates.baseCard, cardInRow);
newCard.setAttribute("rownum", rownum);
newCard.setAttribute("index", index);

if (cardInRow["owner"] === "me") {
fetchRow("#my-rows .game-row", rownum).appendChild(newCard);
} else {
fetchRow("#opp-rows .game-row", rownum).appendChild(newCard);
}
});

document.querySelectorAll("#limits .scores-row")[rownum].innerText = "(lim: " + row["limit"] + ")"
});
},
Expand Down
3 changes: 2 additions & 1 deletion frontend/src/game/board/builder.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ module.exports = {
if (status.clickedCard &&
status.clickedCard.hasAttribute("row-played") &&
!status.clickedCard.hasAttribute("target")) {
status.clickedCard.setAttribute("target", 1);
status.clickedCard.setAttribute("target",
"[:rows " + this.getAttribute("rownum") + " :cards " + this.getAttribute("index") + "]");
play.playCard(status.clickedCard);
}
});
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/game/play/hand.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ module.exports = {
status.clickedCard = card;
card.style.background = "red";
}

document.getElementById("game-status").innerHTML = config.messages["play"];
}
})
}
Expand Down
4 changes: 4 additions & 0 deletions frontend/src/game/play/play.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,13 @@ module.exports = {
status.clickedCard = card;
document.getElementById("game-status").innerHTML = "Select a target";
} else {
if (!card.hasAttribute("target")) {
card.setAttribute("target", "nil");
}
const playData = {
index: card.getAttribute("index"),
row: card.getAttribute("row-played"),
target: card.getAttribute("target"),
};
card.style.background = "green";

Expand Down
3 changes: 2 additions & 1 deletion frontend/tests/game/play.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@ test('Playing cards changes background as intended', () => {
play.playCard(card);
expect(card.style.background).toBe("green");

card = document.createElement("div");
card.setAttribute("add-power", 1);
play.playCard(card);
expect(card.style.background).toBe("yellow");

card.setAttribute("target", 1);
card.setAttribute("target", "[:rows 0 :cards 0]");
Copy link
Owner

Choose a reason for hiding this comment

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

We cannot send Clojure code. Either split the information into target-row and target-card or use a json-encoded object

Copy link
Owner

Choose a reason for hiding this comment

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

The removal of load-string makes this change now incorrect, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test only checks the card has a target, it doesn't checks if the target is correct.

play.playCard(card);
expect(card.style.background).toBe("green");
});