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

Harden KeyContent against different publicKey-file line-delimiters #1083

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

HannesWell
Copy link
Contributor

@HannesWell HannesWell commented May 19, 2022

The class KeyContent reads the entire content of a publicKey-file and returns its raw content as byte-array.
This is problematic if the publicKey-file on server- and client-site use different line endings.
This can happen for example if the file is under git version-control and git is configured to use platform-specific line-endings for text files.

This PR attempts to fix this issue by effectively ignoring line-feed and carriage-return characters when reading the file-content.
Using a BufferedReader makes the method even a bit more compact.

The additional test-case outlines the issue and only passes with the change in this PR.

@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #1083 (29643a0) into master (7232d34) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1083      +/-   ##
============================================
- Coverage     33.23%   33.20%   -0.03%     
  Complexity      356      356              
============================================
  Files          1161     1161              
  Lines         25700    25699       -1     
  Branches       1589     1588       -1     
============================================
- Hits           8541     8534       -7     
- Misses        16654    16660       +6     
  Partials        505      505              
Impacted Files Coverage Δ
...rc/org/eclipse/passage/lic/base/io/KeyContent.java 80.00% <100.00%> (-1.82%) ⬇️
...src/org/eclipse/passage/lic/equinox/GearAware.java 56.25% <0.00%> (-12.50%) ⬇️
...ssage/lbc/internal/base/acquire/FeatureGrants.java 73.52% <0.00%> (-11.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7232d34...29643a0. Read the comment docs.

@HannesWell
Copy link
Contributor Author

@eparovyshnaya can you please review this one? I already created a test case.

Copy link
Contributor

@eparovyshnaya eparovyshnaya left a comment

Choose a reason for hiding this comment

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

Thank for your effort, @HannesWell

Needs a bit of redesign.

@HannesWell HannesWell force-pushed the keyContent branch 2 times, most recently from 5b4c4e6 to 69a92c7 Compare July 16, 2022 11:55
@HannesWell
Copy link
Contributor Author

I tried to address your remarks as good as possible.

@HannesWell HannesWell force-pushed the keyContent branch 2 times, most recently from f56ba9b to 120feef Compare July 16, 2022 12:44
Copy link
Contributor

@eparovyshnaya eparovyshnaya left a comment

Choose a reason for hiding this comment

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

@HannesWell nice work!
one final touch, and it's ready for merge.

@HannesWell HannesWell force-pushed the keyContent branch 2 times, most recently from 69c9fe0 to 4930ca2 Compare July 18, 2022 17:34
Copy link
Contributor

@eparovyshnaya eparovyshnaya left a comment

Choose a reason for hiding this comment

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

Thank you, @HannesWell

@eparovyshnaya eparovyshnaya merged commit 96683f5 into eclipse-passage:master Jul 19, 2022
@HannesWell HannesWell deleted the keyContent branch July 19, 2022 09:57
@HannesWell
Copy link
Contributor Author

Thank you, too.

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