search

isomerpages / isomercms-backend

A static website builder and host for the Singapore Government
5 stars 1 forks source link

Test/image retrieval tests #741

Closed alexanderleegs closed 1 year ago

alexanderleegs commented 1 year ago

This PR introduces tests for PR #740. It removes existing tests for private repos in MediaDirectoryService and MediaFileService and moves them to the newly created media-utils test instead, to be able to better mock and handle axios calls.

alexanderleegs commented 1 year ago

actually i peeked at getMediaFileInfo and i think there are some invariants that we want to ensure that isn't immediately apparent in the test

  1. if the file type is dir, we don't do anything else.
  2. we only call getAccessToken for private images (why not private files?)
  3. we always sanitize svg if 2 is true (ie, if getAccessToken is called and it is a svg, we should sanitize); separately, do we not need to sanitize anything else?
  4. if 2 is true, we will always replace the mediaUrl with a data URI

should we be more comprehensive wrt ^

This is what I've tried to do in the media utils tests, which have 7 (now 6) main flows, private/public x regular image/svg/files

(The dirs test has been removed because it's now being done in MediaDirectoryService directly) The private image and svg tests look for the mock axios call to be called and the mediaUrl to be modified to use the generated dataUri instead, because the axios call is used only to retrieve the actual image which needs to be encoded - as we can't really determine the final encoded value, we look for the an entry starting with data: instead

The svg tests look for sanitization being added to the raw github url - github itself handles the rest. This was a point raised by our VAPT, that svgs specifically were vulnerable

Files have the same behaviour in both public and private repos, and we test that it doesn't use the private image flow - this is because files have no need for a preview image, and we don't need the mediaUrl at all

alexanderleegs commented 1 year ago

6ed1774

Yeah that makes sense, I've added a new test case in https://github.com/isomerpages/isomercms-backend/pull/741/commits/6ed1774e78bb0987c33d47d56f81333c372d38f1