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

Security updates #51

Merged
merged 4 commits into from
Dec 20, 2022
Merged

Security updates #51

merged 4 commits into from
Dec 20, 2022

Conversation

atxr
Copy link
Contributor

@atxr atxr commented Nov 25, 2022

This PR brings security updates about:

  • Dependabot alerts on some dependencies used in the frontend
  • Snyk scan on the production app, which found vulnerabilities on the nginx-alpine image

This changes haven't been tested yet in production mode.
Close #39 and should resolve Dependabot alerts

This dependency is a false positive
The nthcheck, even if required in react-scipts, is never used in the
code
Moreover, react-script is only a dev feature, and not needed in
production mode
Hence, I removed it from the dependencies and add it to devDependencies
alpine is no longer mainted
nginx-alpine version used was vulnerable to latest openssl CVE
This vulnerability was discovered by running Snyk scan
Not tested yet
@atxr atxr added Security issue A potential security issue Priority: High The Issue must be addressed as soon as possible Severity: Major The bug or Issue prevent a feature from working or present a possible security issue labels Nov 25, 2022
@atxr atxr added this to the iScsc blog v0.2.0 milestone Nov 25, 2022
@atxr atxr requested review from amtoine and ctmbl November 25, 2022 14:52
@atxr atxr self-assigned this Nov 25, 2022
Copy link
Contributor

@ctmbl ctmbl left a comment

Choose a reason for hiding this comment

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

This currently doesn't work in production mode:

> sudo docker-compose --env-file .env.production up -d --build
...
 > [iscscfr-react-app 7/7] RUN npm run build:
#0 0.880
#0 0.880 > iscsc.fr-frontend@0.1.0 build
#0 0.880 > react-scripts build
#0 0.880
#0 0.889 sh: 1: react-scripts: not found
------
failed to solve: executor failed running [/bin/sh -c npm run build]: exit code: 127

@ctmbl

This comment was marked as off-topic.

@atxr
Copy link
Contributor Author

atxr commented Nov 26, 2022

Oh indeed let me fix this 🤔
EDIT: Fix 62b848b

Did you experience any issue with the SSL certificates? Or maybe you tested it on the server directly.

@atxr

This comment was marked as off-topic.

@amtoine

This comment was marked as off-topic.

@amtoine

This comment was marked as off-topic.

@amtoine amtoine closed this Nov 26, 2022
@amtoine amtoine reopened this Nov 26, 2022
@amtoine
Copy link
Member

amtoine commented Nov 26, 2022

and in the end, @atxr and @ctmbl, it's probably not the place to discuss a switch in workflows 👀

Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

that looks ok to me
you only changed two versions for the better 👌

Copy link
Contributor

@ctmbl ctmbl left a comment

Choose a reason for hiding this comment

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

@atxr

Oh indeed let me fix this thinking
EDIT: Fix 62b848b

As far as I can tell that worked, but I have 2 limitations:

  • it seems I can't test SSL certificates on local see (that will answer your question)
  • I get the same issue that forbade me to test production mode on V0.2.0: production and development mode #20, all I can tell is that I get the same behaviour, but I strongly advise you to test it yourself

And one request change:
please run npm install in frontend because package-lock.json isn't up to date, and that point makes me remember we have to deal with #31

@ctmbl
Copy link
Contributor

ctmbl commented Nov 28, 2022

@atxr @amtoine

and in the end, @atxr and @ctmbl, it's probably not the place to discuss a switch in workflows eyes

I totally agree so please continue there: #54

@ctmbl
Copy link
Contributor

ctmbl commented Dec 7, 2022

@atxr this PR is almost ready do you have some time to finish it?

@atxr
Copy link
Contributor Author

atxr commented Dec 7, 2022

@atxr this PR is almost ready do you have some time to finish it?

I can try yes, but how about the tests? Should I test directly on the server?

@ctmbl
Copy link
Contributor

ctmbl commented Dec 8, 2022

@atxr

Should I test directly on the server?

for SSL certificates it seems that we don't have much choice for now, we should however look for a better solution, maybe a subdomain to test automatically? we'll have to discuss that in another issue though
So overall my answer is yes I think, but maybe I could give you a hand? (with some supervision ofc)

please run npm install in frontend

but you should do this first 😉

Anyway, if you're lacking time right now nevermind it's not an emergency I think (even if it seems severe the blog is quite unknown right now)

Update package-lock.json in frontend
@atxr
Copy link
Contributor Author

atxr commented Dec 17, 2022

The tests were successful and I ran npm i in the frontend!
Ready to merge 🚀

@ctmbl ctmbl requested a review from amtoine December 19, 2022 17:50
@ctmbl
Copy link
Contributor

ctmbl commented Dec 19, 2022

@atxr tks it's perfect!
Once @amtoine will approve we'll be able to merge it

@amtoine
Copy link
Member

amtoine commented Dec 20, 2022

@atxr
in dc5600e, you simply run

cd frontend
npm install

and that caused changes in frontend/package-lock.json? 😋

@atxr
Copy link
Contributor Author

atxr commented Dec 20, 2022

@atxr in dc5600e, you simply run

cd frontend
npm install

and that caused changes in frontend/package-lock.json? 😋

Yes exactly, as @ctmbl requested

Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

Yes exactly, as @ctmbl requested

then it's perfect 👌

@amtoine amtoine merged commit 3c6d3f7 into iScsc:main Dec 20, 2022
@atxr atxr deleted the fix-dependabot branch March 17, 2023 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High The Issue must be addressed as soon as possible Security issue A potential security issue Severity: Major The bug or Issue prevent a feature from working or present a possible security issue
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Run Snyk to scan vulnerabilities
3 participants