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

Erxes security #4974

Merged
merged 10 commits into from
Feb 20, 2024
Merged

Erxes security #4974

merged 10 commits into from
Feb 20, 2024

Conversation

Gerelsukh
Copy link
Collaborator

@Gerelsukh Gerelsukh commented Feb 19, 2024

ISSUE

Context

Your context here. Additionally, any screenshots. Delete this line.

// Delete the below section once completed

PR Checklist

  • Description is clearly stated under Context section
  • Screenshots and the additional verifications are attached

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

PR Type: Refactoring

PR Summary: This pull request introduces changes to how files are accessed and served in the application. Specifically, it refactors the endpoint for fetching import files by changing from using a header to specify the file name to using a path parameter. Additionally, it updates the corresponding service call in the workers package to align with this new approach.

Decision: Comment

📝 Type: 'Refactoring' - not supported yet.
  • Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.

General suggestions:

  • Given the security concern raised about sanitizing the 'fileName' parameter to prevent directory traversal vulnerabilities, it's crucial to implement input validation or sanitization before using the 'fileName' in file path construction.
  • The PR title 'Erxes security' is somewhat vague and does not fully capture the essence of the changes made. A more descriptive title, such as 'Refactor file import endpoint for enhanced security', might provide clearer insight into the PR's purpose at a glance.
  • The PR description lacks detail and does not follow the provided checklist, specifically the 'Context' section which is left unfilled. Providing a more comprehensive context and rationale behind the changes, along with any potential screenshots if applicable, would greatly improve the PR's clarity and the reviewers' understanding of the changes.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

packages/core/src/index.ts Outdated Show resolved Hide resolved
@dulguun0225 dulguun0225 self-assigned this Feb 20, 2024
Copy link

sonarcloud bot commented Feb 20, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@dulguun0225 dulguun0225 merged commit b608aa1 into dev Feb 20, 2024
2 of 3 checks passed
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.

None yet

2 participants