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

test(routes/api/tasks): limit file permissions #89

Merged
merged 2 commits into from
Mar 13, 2025
Merged

Conversation

Fdawgs
Copy link
Member

@Fdawgs Fdawgs commented Mar 11, 2025

Closes https://github.com/fastify/demo/security/code-scanning/2

Checklist

Signed-off-by: Frazer Smith <frazer.dev@icloud.com>
@@ -629,7 +629,7 @@ describe('Tasks api (logged user only)', () => {
const largeTestImagePath = path.join(tmpDir, 'large-test-image.jpg')

const largeBuffer = Buffer.alloc(1024 * 1024 * 1.5, 'a') // Max file size in bytes is 1 MB
fs.writeFileSync(largeTestImagePath, largeBuffer)
fs.writeFileSync(largeTestImagePath, largeBuffer, { mode: 0o600 }) // 0600 permissions (read/write for owner only)
Copy link
Member

Choose a reason for hiding this comment

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

Can you plz explain why you think this is a good idea? Even for a test file?
And can you plz write a comment a little bite more elaborated as the demo has an educational purpose.

Copy link
Member Author

Choose a reason for hiding this comment

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

CodeQL was reporting this as a security issue and it was an easy fix.

File permissions should follow the principle of least privilege.

As this is an educational demo as you said, I think it's good to show that test files should also follow security best practices. :)

Snyk has a good guide on this.

Copy link
Member

@jean-michelet jean-michelet Mar 12, 2025

Choose a reason for hiding this comment

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

As this is an educational demo as you said, I think it's good to show that test files should also follow security best practices. :)

I meant that it would be nice to add the explanations in the code as a comment.
Maybe the link to Snyk article is a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

We often share links in the code base to help understand code implementation.

Copy link
Member

@jean-michelet jean-michelet left a comment

Choose a reason for hiding this comment

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

Can you plz add the link to the article n the code if you think it is relevant?

Signed-off-by: Frazer Smith <frazer.dev@icloud.com>
@Fdawgs Fdawgs merged commit ef40c33 into main Mar 13, 2025
8 checks passed
@Fdawgs Fdawgs deleted the Fdawgs-patch-1 branch March 13, 2025 20:29
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.

2 participants