-
Notifications
You must be signed in to change notification settings - Fork 9
[Backend] NodeJS - week 1 #146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
magdazelena
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments based on my discovery with #159 and past experiences.
I like the structure though, especially in the session materials!
6fbceaa to
a44dd31
Compare
|
@marcorichetta to review the progress here and summarise what's left, before we continue |
courses/backend/node/week2/README.md
Outdated
| For more research, check the following resource: | ||
|
|
||
| - [What is REST: a simple explanation for beginners](https://medium.com/extend/what-is-rest-a-simple-explanation-for-beginners-part-1-introduction-b4a072f8740f) | ||
|
|
||
| - [@NoerGitKat (lots of web app clones/examples to learn from)](https://github.com/NoerGitKat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Candidate for preparation.md
| First, you should demonstrate the SQL injection and that it for instance is possible to drop/delete the `contacts` table with the `sort` query parameter. | ||
| You can for instance demonstrate this with a screen recording and include it in the PR description. | ||
|
|
||
| After having demonstrated the SQL injection vulnerability, the goal is then to fix the issue by updating `app.js`. | ||
|
|
||
| **Hint:** the `multipleStatements: true` part in the configuration indicates how you can use the vulnerability. The configuration should not be changed though, the SQL injection should be fixed by making changes in the `/api/contacts` route. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This made sense for MySQL. Knex with sqlite doesn't accept that config.
We should review how to replicate or change the way of showing an SQL injection. There's something similar in the backend/databases module
Done!
|
Co-authored-by: Marco Richetta <[email protected]>
Co-authored-by: Marco Richetta <[email protected]>
0caae9d to
c5c14e6
Compare
|
@adamblanchard I have two doubts to clear. I went according to the plan: https://github.com/HackYourFuture-CPH/program/blob/fe5da8a4eef91d759ed80b31d9d7b1bcd8be5d83/contributing/proposals/js-node-restructure.md
I think I'm most doubtful about the above two, otherwise the week 1 could be up for review :) |
|
@magdazelena Hey! I think POST can live in week2. Since this week seems like the main focus is learning express, that makes sense. Thought about assignment: I think it could be ok to leave POST there? They did cover POST previously, and I think assignments should not only practice what they learned in the session but also stretch them to apply things they know in new ways. We could help by linking to some documentation, or writing some more hints in the assignment. What do you think? I can't really judge whether auth should be covered more. I do appreciate it's an important and big topic though, that is easy to "not understand". Also recognise it's one of those topics that can come up in interviews. If you think we could spend more time on it, we could move the postman stuff all to week2? Then it's more unified with the other POSTMAN stuff we are covering there, rather than splitting their focus for the last 30 mins here. And we'd have some more time here to dive into it? Either way, might be a good idea to move postman all to week2 anyway? I will review the rest of the PR in the meantime :) |
|
@adamblanchard I think since we do mention auth already in the module, it could make sense to make it more extensive... I think it will depend on the mentor, but in general at least a mention of different methods could be beneficial here - and then moving POST and Postman to week 2. I agree with the assignment, it has been already explained after all :) |
|
Merging this for now, this PR appears to be complete. It's also getting a bit big! Work will continue in #199 |
Description
Status
Content check or update
Week 1
module-materialsWeek 2 -> Work moved to [Backend] NodeJS - week 2 #199
Generic
Review for traces of MySQL
Review assignments for outdated material
Remove
teach-live-codingreferencesRemove homework upload references
Remove
nodemonreferences(Optional) Review relevance to Foundation/intro-to-nodejs, link overlapping material if exists
Proposal overview
https://github.com/HackYourFuture-CPH/programme/blob/main/contributing/proposals/js-node-restructure.md#backend-specialisation