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

chore(deps): Update winston-cloudwatch #2726

Merged
merged 3 commits into from
Sep 7, 2021
Merged

chore(deps): Update winston-cloudwatch #2726

merged 3 commits into from
Sep 7, 2021

Conversation

timotheeg
Copy link
Contributor

Context

Snyk reports a vulnerability in dependencies of [email protected]. See report here.

Approach

Upgrade winston-cloudwatch to 3.0.2

Instantiation signature stays the same so upgrade should be backward compatible.

winston-cloudwatch@3 no longer lists aws-sdk as a direct dependency (set as a peer dependency instead), but since Form does list aws-sdk as a dependency, we're good to go.

cc @mantariksh @karrui

@timotheeg
Copy link
Contributor Author

Sigh, there is a type upgrade to do... Doing it now

@timotheeg
Copy link
Contributor Author

Looks like the type upgrade might be wrong in the typescript definition... 😢 I've opened an issue upstream here

But for now, to unblock this PR, I have added a dummy name into the constructor options with a TODO comment for future removal.

Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

just to be sure, we should probably test this on staging by making sure that public-facing email notifications (i.e. for email confirmations and verified email fields) go to the custom Cloudwatch log group rather than the main log group. we do this so that we can make email data for public users expire after 1 week instead of the standard 1 year for logs, so we don't keep public user data around for unnecessarily long (the 1 week is useful e.g. when there are outages with SGmail causing submissions to be lost because they don't reach admins' email inboxes, upon which we can check that custom log group to find email addresses of respondents).

@timotheeg
Copy link
Contributor Author

Thanks for the advice, I'll push this to staging and try it out. I hesitated to provide an empty string in name this the constructor has a default value of 'CloudWatch' 🤔

See here:
https://github.com/lazywithclass/winston-cloudwatch/blob/d77124fbba1a5986605e805c62938549aa7e1ad3/index.js#L17

Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

lgtm

@timotheeg
Copy link
Contributor Author

Verified that logs still end up in cloudwatch log below:

/aws/elasticbeanstalk/formsg-staging-alb/var/log/eb-docker/containers/eb-current-app/stdouterr.log

Tested with a dummy staging form.
image

I'll be merging now

@timotheeg timotheeg merged commit bb9749c into develop Sep 7, 2021
@timotheeg timotheeg deleted the sec/winston branch September 7, 2021 09:36
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.

3 participants