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 security access headers for ocdav requests #780

Merged
merged 1 commit into from
May 28, 2020

Conversation

karakayasemi
Copy link
Contributor

Adds necessary security headers for ocdav requests.

For owncloud/ocis-reva#66

@karakayasemi karakayasemi requested a review from labkode as a code owner May 27, 2020 22:02
@karakayasemi karakayasemi force-pushed the add-access-headers branch 2 times, most recently from 284fc34 to 1c5f32e Compare May 27, 2020 22:08
@@ -134,3 +138,8 @@ func (s *svc) handleGet(w http.ResponseWriter, r *http.Request, ns string) {
log.Error().Err(err).Msg("error finishing copying data to response")
}
}

// Rawurlencode https://www.php2golang.com/method/function.rawurlencode.html
func Rawurlencode(str string) string {
Copy link
Member

Choose a reason for hiding this comment

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

Why the url.QueryEspace is not enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oc10 encodes filename in Content-Disposition header with PHP's rawurlencode function. The difference between rawurlencode and url.QueryEscape is url.QueryEscape encodes spaces as + instead of %20. I added this function to ensure consistency with oc10.

I found url.PathEscape is also working in same way. So, now url.PathEscape has been used instead of this function with the last commit.

Copy link
Member

Choose a reason for hiding this comment

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

@karakayasemi in that case, as youre' doing path.Base there no need to call url.PathEscape as there isn't a path. Base gives you the basename of the path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. Redundant url.PathEscape has been removed.

@labkode
Copy link
Member

labkode commented May 28, 2020

Hi @karakayasemi ,

take a look at the following page for default security configurations:
https://ssl-config.mozilla.org/#server=nginx&version=1.17.7&config=intermediate&openssl=1.1.1d&guideline=5.4

If we are providing some defaults, let's make sure they are aligned with the propositions above.

@karakayasemi
Copy link
Contributor Author

Hi @karakayasemi ,

take a look at the following page for default security configurations:
https://ssl-config.mozilla.org/#server=nginx&version=1.17.7&config=intermediate&openssl=1.1.1d&guideline=5.4

If we are providing some defaults, let's make sure they are aligned with the propositions above.

The only conflicting part that I see is the age of Strict-Transport-Security. I aligned it according to the example config.

@karakayasemi karakayasemi force-pushed the add-access-headers branch 2 times, most recently from 2be076c to 56b8838 Compare May 28, 2020 12:25
@labkode labkode merged commit 7d74bf8 into cs3org:master May 28, 2020
@karakayasemi karakayasemi deleted the add-access-headers branch May 28, 2020 12:37
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