Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove path variable when retrieving the error log file. #1139

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

chandrashekar-s
Copy link
Collaborator

@chandrashekar-s chandrashekar-s commented Aug 6, 2024

Description of what I changed

Currently to retrieve the error log file associated with the latest pipeline run, we pass the log file path in the request as a path parameter, this is prone to vulnerability attacks as there are no checks to restrict the paths only to the data-warehouse directory.

This PR contains changes to remove the path parameter altogether and only retrieves the error log associated with the latest pipeline run if exists.

E2E test

Tested the changed api and it works as expected. Also, the path parameter to accept file path is no more valid.

TESTED:

relied on e2e tests.

Checklist: I completed these to help reviewers :)

  • I have read and will follow the review process.

  • I am familiar with Google Style Guides for the language I have coded in.

    No? Please take some time and review Java and Python style guides.

  • My IDE is configured to follow the Google code styles.

    No? Unsure? -> configure your IDE.

  • I have added tests to cover my changes. (If you refactored existing code that was well tested you do not have to add tests)

  • I ran mvn clean package right before creating this pull request and added all formatting changes to my commit.

  • All new and existing tests passed.

  • My pull request is based on the latest changes of the master branch.

    No? Unsure? -> execute command git pull --rebase upstream master

@chandrashekar-s chandrashekar-s requested a review from bashir2 August 6, 2024 07:00
@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 52.50%. Comparing base (c92ff49) to head (4e73b17).

Files Patch % Lines
.../java/com/google/fhir/analytics/ApiController.java 0.00% 8 Missing ⚠️
...ava/com/google/fhir/analytics/PipelineManager.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1139      +/-   ##
============================================
- Coverage     52.59%   52.50%   -0.09%     
  Complexity      706      706              
============================================
  Files            95       95              
  Lines          5751     5757       +6     
  Branches        765      767       +2     
============================================
- Hits           3025     3023       -2     
- Misses         2434     2443       +9     
+ Partials        292      291       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@bashir2 bashir2 left a comment

Choose a reason for hiding this comment

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

Thanks @chandrashekar-s for fixing this.

@chandrashekar-s chandrashekar-s merged commit ffc55f5 into google:master Aug 19, 2024
6 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants