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

Add HSTSRedirection Middleware #23

Merged
merged 6 commits into from Jan 5, 2021

Conversation

Mattiav8
Copy link
Contributor

Hello Tim,

I'd like to show you a little implementation for http/https redirection. Please be free to give me any feedback (good or bad) and don't hesitate to tell me if anything is wrong or needs to be changed.
Thank you very much for your disposal, I really appreciate it.

@Mattiav8
Copy link
Contributor Author

Is the pull request's check failing for anything I'm doing wrong? I thought it was because the warning but I've already fix it. Thank you very much and sorry for the inconvenience.

@0xTim
Copy link
Member

0xTim commented Dec 21, 2020

I don't think so, have reran them to see.

Also, thanks for this PR! Just to say I've seen it, am very grateful and haven't forgotten! Will take a look over the holiday break in the coming days

@Mattiav8
Copy link
Contributor Author

Mattiav8 commented Dec 21, 2020 via email

@0xTim
Copy link
Member

0xTim commented Dec 21, 2020

@Mattiav8 ah I know what the problem is and it's nothing you've done. Installing packages changed a few months ago. Quickest way to fix this would be to change this line https://github.com/brokenhandsio/VaporSecurityHeaders/blob/master/.github/workflows/ci.yml#L22 to

run: apt-get update && apt-get install curl -y

The -y should enable the installation and make it work. Could you try that?

@Mattiav8
Copy link
Contributor Author

Yes, but I could do it this afternoon if you agree with it.
Thank you

@codecov
Copy link

codecov bot commented Dec 21, 2020

Codecov Report

Merging #23 (ed920b4) into master (56b250f) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #23   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        11    +1     
  Lines          213       226   +13     
=========================================
+ Hits           213       226   +13     
Impacted Files Coverage Δ
...aders/Configurations/HTTPSRedirectMiddleware.swift 100.00% <100.00%> (ø)

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 56b250f...ed920b4. Read the comment docs.

Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Thanks for this! Looks really good! Just a couple of small tweaks before it can be merged

README.md Outdated Show resolved Hide resolved
Sources/VaporSecurityHeaders/SecurityHeadersFactory.swift Outdated Show resolved Hide resolved
@Mattiav8
Copy link
Contributor Author

Mattiav8 commented Jan 5, 2021

Thank you Tim. I modified the code as requested. Comments were right and sorry not to think about it before. If you need anything else, I'm at your disposal. Thank you very much!

Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Thanks!

@0xTim 0xTim merged commit 089470c into brokenhandsio:master Jan 5, 2021
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