Skip to content
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

Enforce signatures on post/patch operations #48

Merged
merged 4 commits into from
Sep 24, 2021
Merged

Conversation

carletex
Copy link
Collaborator

Fixes #44

I worked on this with @dgrcode and I've just gave it the final touches.

This PR enforces the use of signatures on challenge / challenge review submissions. Not sure if we are missing something with the signatures, but I think this should be secure enough.

Let me know what you think.

Copy link
Collaborator

@dgrcode dgrcode left a comment

Choose a reason for hiding this comment

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

I'll address all the comments besides the middleware and commit. I'll also open a new issue for the middleware improvement.

packages/backend/index.js Outdated Show resolved Hide resolved
packages/backend/utils/sign.js Show resolved Hide resolved
packages/backend/utils/sign.js Show resolved Hide resolved
packages/backend/index.js Show resolved Hide resolved
packages/react-app/src/components/ChallengeSubmission.jsx Outdated Show resolved Hide resolved
packages/react-app/src/views/ChallengeReviewView.jsx Outdated Show resolved Hide resolved
@dgrcode dgrcode merged commit 3cdd996 into master Sep 24, 2021
@dgrcode dgrcode deleted the 44/sign-challenges branch September 24, 2021 15:25
@dgrcode
Copy link
Collaborator

dgrcode commented Sep 24, 2021

I made a few changes related to the comments I added before. Just merged into master a few hours before dogfooding, which is probably not recommendable, although it'd be awesome to show this feature today.

@sogasg Could you please give it a run in your local before dogfooding? I checked everything works on my machine, but this things are mysterious some times, especially on live demos.

In case things blow up, the commit master had as HEAD before merging was 2af7e33. Feel free to
git reset --hard 2af7e33b8ba20c1d049a02f71c051a9b6c447ce8
if needed. I hope this is not necessary! 🤞

@carletex
Copy link
Collaborator Author

Thanks @dgrcode !!

Just tested locally and everything seems to work.

@sogasg Good luck at the dogfooding, let us know how it goes.

@asgeir-s
Copy link
Contributor

Awesome work!

Everything is working locally for me too :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sign messages on challenge / challenge review submission
3 participants