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.

Sign up for GitHub

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

Jump to bottom

Feature/protect file extensions #164

Merged
merged 8 commits into from
Oct 26, 2022
Merged

Feature/protect file extensions #164

merged 8 commits into from
Oct 26, 2022

Conversation

markadrake
Copy link
Contributor

Protect File Extensions

I deployed the Umbraco CMS to a secure enterprise environment and encountered an issue. Our security policies blocked the static asset bundles produced by Smidge. After troubleshooting, we discovered that network requests were being blocked because the file extensions were unknown. Smidge's file naming pattern changes the file extension.

With this PR, I hope to remedy the above issue by opting in through a configuration value protectFileExtensions.

Please review my PR and I appreciate any critical feedback. I am not a C# developer.

  1. Adds a new configuration option, protectFileExtensions.
  2. Requires opt-in, so no existing behavior should be modified.
  3. After opt-in, the naming scheme will preserve the file extension (.css or .js).
  4. Adds two additional tests for verifying the path URL when protectFileExtensions is set to true.

Sample Settings

image

Previously, without protectFileExtensions

image

Now, with protectFileExtensions

image

@markadrake
Copy link
Contributor Author

markadrake commented Oct 6, 2022 •
edited
Loading

@Shazwazza,

I ran into various errors after running Smidge.Web.

  1. The application failed to start because of a missing directory: ~/Smidge/Static.
  2. Startup.cs:197 adds a non-existent file to a bundle. The application handles this error and continues to run.

Please let me know if you would like me to check in the changes I made to remedy these issues. Respectively:

  1. Created the missing folder.
  2. Removed lines 197-200.

PhyxionNL
PhyxionNL suggested changes Oct 6, 2022
View reviewed changes
src/Smidge.Core/SmidgeConfig.cs Outdated
@@ -34,6 +34,8 @@ public SmidgeConfig()
public string Version => _config["version"] ?? "1";

public string DataFolder => (_config["dataFolder"] ?? "Smidge").Replace('/', Path.DirectorySeparatorChar);

public bool ProtectFileExtensions => bool.Parse((_config["protectFileExtensions"] ?? "false"));
Copy link
Contributor

Choose a reason for hiding this comment

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

This name is odd to me as there's nothing to protect. I think KeepFileExtensions would be a better name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also formatting is off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PhyxionNL I've renamed everything to keepFileExtensions, including the 2 new tests as Keep_File_Extensions.

I noticed the tabs vs.spaces on line 38. I'm going to save checking this in until I have addressed your related feedback.

Thank you!

src/Smidge.Core/CompositeFiles/DefaultUrlManager.cs Outdated
@@ -160,8 +162,8 @@ public ParsedUrlPath ParsePath(string input)
private string GetCompositeUrl(string fileKey, string fileExtension, string cacheBusterValue)
{
//Create a delimited URL query string

const string handler = "~/{0}/{1}{2}.v{3}";

Copy link
Contributor

Choose a reason for hiding this comment

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

Undo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the new line on 165. Had to do this outside of Visual Studio.

src/Smidge.Core/CompositeFiles/DefaultUrlManager.cs Outdated
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Undo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the new line on 176. Had to do this outside of Visual Studio.

@markadrake
Copy link
Contributor Author

Please note that protectFileExtensions has been renamed to keepFileExtensions.

appsettings.json:

image

Smidge.Tests:

image

@markadrake markadrake requested a review from PhyxionNL October 7, 2022 17:12
@markadrake
Copy link
Contributor Author

markadrake commented Oct 11, 2022 •
edited
Loading

@Shazwazza or @PhyxionNL, I apologize for my persistence with this issue. Could either of you provide me some additional feedback, as I do have some time I can dedicate to working and addressing any problems or concerns? I do appreciate it. I appreciate your help,

All the best,

@markadrake
Copy link
Contributor Author

@Shazwazza if there is any feedback at all, I'm happy to try and implement the changes. Hope all is good sir!

@Shazwazza
Copy link
Owner

Sorry for the delay, having a look now

@Shazwazza Shazwazza merged commit e38b7dc into Shazwazza:master Oct 26, 2022
@Shazwazza
Copy link
Owner

Looks awesome, thanks so much 🎉

I'll ship a release asap

@Shazwazza
Copy link
Owner

This has been pushed :) https://github.com/Shazwazza/Smidge/releases/tag/v4.2.0

@markadrake markadrake deleted the feature/protect_file_extensions branch October 28, 2022 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PhyxionNL PhyxionNL Awaiting requested review from PhyxionNL

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@markadrake @Shazwazza @PhyxionNL