Frontend passes target to backend #119
Conversation
backend/src/api/base.clj
Outdated
| (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)] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Just as a reminder: this pull request cannot be accepted until it doesn't use vulnerable things such as load-string
frontend/tests/game/play.test.js
Outdated
| 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]"); |
There was a problem hiding this comment.
We cannot send Clojure code. Either split the information into target-row and target-card or use a json-encoded object
There was a problem hiding this comment.
The removal of load-string makes this change now incorrect, right?
There was a problem hiding this comment.
This test only checks the card has a target, it doesn't checks if the target is correct.
|
Now it passes the backend and frontend tests, but not the e2e. |
* small changes * tests already done * forgot an include
* small changes * backend structure changes * tests already done * forgot an include * readme * remove changelog * some rework * english
|
@Masclins Just to confirm, you dropped this on purpose, right? |
|
I dropped this because branch was deleted and pushed unto your repository. |
This resolves #78.
But after this and #115 everything is really ugly.
Philosophy goes as following:
Cards in hand have an attribute
add-powerequal to its add-power ability or"undefined".When a card is "played", if
add-power === "undefined"the play is passed to the backend, withtarget: "nil". Otherwise it gets an attributerow-playedwith the row where it's been played.When a card on the board is clicked with a card "played" (that means it must be a card with an
add-power !== "undefined"previously dragged/clicked) the "played" card recieves an attributetarget: "[:rows 0 :cards 0]"with 0's being the appropriate indexes (even I can see the ugliness in that).On the other hand, backend recieves always a target, and
api.base/play-card-as-playerunderstand the target (a string) as a clojure code giving the path (using(let [target (load-string target)].From this pull-request (and the previous one) I see a couple of things that need work:
play()are done in many places on the frontend code. That's weird and messy.