Juice-shop: Migration to latest Sequelize

[bkimminich]

• Migrate to Sequelize 3.x
• Remove dependency on sequelize-restful
  
  
  
  • use different dynamic REST API generator
    
    _or_
  
  
  • implement REST API manually
  
  

---

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[bkimminich]

131 and #152 should automatically be solved when this issue has been addressed, as sequelize-restful would be removed in the proposed migration. New API generator or manual implementation would of course need to be more robust against "strange" requests.

[bkimminich]

Sidetask: Challenge for reporting a vulnerable library must not accept sequelize-restful as a solution any more once it has been removed from the project dependencies.

[bkimminich]

Experimenting with https://github.com/dchester/epilogue resulted in some headaches and a revert. Got either Invalid sequelize instance (_=too early?_) or when setting it up after the datacreator run routes were not registered with Express, e.g. Error: Unexpected path: /api/Feedbacks/ (_=too late?_). Also setup and config of sequelize-restful seemed much leaner.

[J12934]

Hi!
i looked into this issue and managed to update sequelize to the latest version (4.13).
I replaced sequelize-restfull with the previously mentioned epilogue package and got it working. I configured epilogue to make the generated rest api compatible with the sequelize-restful api.

There were also quite a lot of changes required due to api changes in the upgrade from sequelize 1 => 4. I managed to find most of these issue and fix them but there are still some issue regarding a change in the way associations are named. In the most recent version associations are always named upper case, the angular app assumes assumes the relationships to be named lower case.

I not quite sure what would be the best way to fix this naming issue. There a lot of references which would be renamed. The alternative is to explicitly tell sequelize how to name the relationships and ignore their standards.

I worked on this issue in this branch, the standard test suit is already passing but the e2e test are still failing due to the sequelize naming issues.

@bkimminich do you have any preferences / tips on how to tackle this issue?

[bkimminich]

Awesome! :100: I'll have a look at your branch to get an idea what this naming issue is!

[bkimminich]

@J12934 I suppose renaming all relationships is a bit tedious but better, because from then on everything is according to default of sequelize again. But, that might not be the only issue:

I looked at the Feedbacks endpoint and even the rows created by the data creator script have all an empty comment. This happens for new Feedback as well (which is why all "Inform the shop..."-challenges fail in the e2e suite). The fields UserId and rating are properly saved on the other hand.

I cannot run npm run frisby at the moment, but it should also fail like npm run protractor does.

[J12934]

Oh right totaly forgot about the frisby tests 😉
Just run them and 38 / 220 test are failing, but the error messages of these are a lot more helpfull then the e2e tests. I will use these to iron out more bugs.

Thank you!

[bkimminich]

@J12934, if you'd like me to assist, feel free to bring your fork into a separate branch on the main repo.

[J12934]

Pushed the current state to the upgrade-sequelize branch.

There are a quite a few tasks still open:

Due to epilogue:

• [x] Fix test to expect 201 after model creation / update
• [x] Investigate behaviour of related (associated) models in the generated api
• [x] Change Not Found Test to use a 404 status code
• [x] Models without a generated api (e.g. Basket) now throw a 500

Due to sequelize:

• [x] Investigate more potential differences in the sequlize api

[J12934]

Fixed some more tests.

Unfortunately i am running into inconsistent / flaky test failures. This is apparently due to sqlite database locking.

Here is a API Response of one of the inconsistent test (login api):

feedbackApiSpec (POST feedback is associated with any passed user ID)

<div id="wrapper">
    <h1>Juice Shop (Express ~4.15)</h1>
    <h2><em>500</em> SequelizeTimeoutError: SQLITE_BUSY: database is locked</h2>
    <ul id="stacktrace"><li> &nbsp; &nbsp;at Query.formatError (/Users/jannik/juice-shop/node_modules/sequelize/lib/dialects/sqlite/query.js:415:16)</li><li> &nbsp; &nbsp;at Statement.afterExecute (/Users/jannik/juice-shop/node_modules/sequelize/lib/dialects/sqlite/query.js:119:32)</li><li> &nbsp; &nbsp;at Statement.replacement (/Users/jannik/juice-shop/node_modules/sqlite3/lib/trace.js:19:31)</li></ul>
</div>

Any ideas on how to avoid these issues?

[bkimminich]

Hm, never occured with sequelize-restful afaik,.. feedbackApiSpec.js is one of the tests with the most DB writes happening. The other would be userApiSpec.js, I think. Is that one flaky as well? Is only the one test case you mentioned flaky or different ones from feedbackApiSpec.js?

[J12934]

Yup at least these were the main ones that kept failing.

I found a solution to work around this.
You can pass Jest a flag to force it to run all test in order (-i), instead of trying to run them in parallel. This reduces the load on the sqlite db quite considerably.

[bkimminich]

Ok, so this seems to have been the reason for Jest not running properly on my Windows machine, because now it works as it should. I wonder why it never caused problems on Travis-CI's Linux infrastructure, but as long as it works now... :+1:

[J12934]

Did some more fixing yesterday.

We are apparently down to one last issue. 😀

This issue is due to epilogue correctly ignoring the soft deleted entries in the product table(the Christmas item) and not including them in the api. Epilogue doesn't offer a way to change this behaviour, or at least it isn't documented.

This makes the Christmas challenge currently not solvable prevents the user on putting the item in the basket.

It might not be possible to keep the Christmas challenge in its current state with epilogue 😢

Possible Solutions would be:

1. Exclude the Ordering Part from the challenge. This would not really reduce the complexity of the challenge, but would make it way less exciting.
2. Find a exploit in the Basket api to be still be able to order the Christmas item.

[bkimminich]

So, we cannot force a BasketItem into the DB with the ProductId of the Christmas product? Another option would be to change the putting stuff into the basket from API to manual SQL via a /rest route? Not sure if that's worth all the rewrite, though...

[bkimminich]

@J12934 Wenn man in routes/order.js statt auf product.id === products.christmasSpecial.id auf product.BasketItem.ProductId === products.christmasSpecial.id prüft, dann geht die Challenge wieder, aber nur, wenn man sich die ID des Produkts erschließt und einen POST an /api/BasketItems bastelt mit der entsprechenden ID. Hätte dennoch ein paar Haken:

1. sieht man im Warenkorb nur eine leere Zeile
2. sieht man auf der Rechnung ebenfalls nur eine Zeile ohne Werte

Ist also weniger nachvollziehbar, dass man es auf diesem Wege erfolgreich lösen kann.

Außerdem funktioniert der entsprechende e2e/searchSpec.js-Test dann nicht mehr. Man könnte den so umbauen, dass statt auf den "In-den-Warenkorb"-Button zu drücken ein Skript den besagten POST sendet. Allerdings muss man dazu dynamisch die ID des Produkts ermitteln, da es ja nicht zwingend das mit der id = 10 sein muss, z.B. in Instanzen mit eigener Produktliste.

[bkimminich]

And for some reason I wrote that in :de: ... :sleeping:

[J12934]

Just tried it out.
The basket api is behaving quite a strange way. The "normal" way to solve the challenge is actually still working under on condition: There has to be another item in the basket.

If the Christmas item is the only item in the basket the api will return a basket with a empty products array. If there is another item in the basket all items (including the Christmas one) are returned in the array.

This is some really weird behaviour, might actually be a bug in sequelize. We can work around it by explicitly including soft deleted entries in the order and basket api. Doing this the basket ui and the order pdf are also looking correctly.

[bkimminich]

Hm, so could we just increase the difficulty from * to ** and make finding out that another item needs to be in the basket part of the challenge? That way only the tests would need to get updated to this new prerequisite. Would that work?

[J12934]

Oh i forgot to write about the good news 😅, including the products explicitly also works around the bug. by doing it it works exactly like it did before.

[J12934]

All Tests are now green 🎉🎉🎉

[bkimminich]

Build is green, so I'll bring this into dev branch! Awesome work, @J12934! Once the project budget is above zero again, you'll get a Juice Shop Hoodie! 🎁

[J12934]

Should the references to sequelize 1.7 and sequelize-restful be removed from the backup package.json? To avoid confusion with the known vulnerable components challenge?

[bkimminich]

I updated it to the current version 5.1.0-SNAPSHOT content.

[lock[bot]]

This thread has been automatically locked because it has not had recent activity after it was closed. :lock: Please open a new issue for regressions or related bugs.