Skip to content

add function Handler for convenience#14

Merged
kr merged 1 commit into
mainfrom
handler
Aug 6, 2018
Merged

add function Handler for convenience#14
kr merged 1 commit into
mainfrom
handler

Conversation

@kr

@kr kr commented Aug 6, 2018

Copy link
Copy Markdown
Owner

This function signature makes it easier to do the most
common usage patterns I'm aware of, after several years
of seeing this package used in the wild.

http.Handle("/", secureheader.Handler(mux))
http.ListenAndServe(addr, nil)

and

http.Handle("/a", a)
http.Handle("/b", b)
http.ListenAndServe(addr, secureheader.Handler(nil))

and

var h http.Handler
h = mux
h = middleware3(h)
h = secureheader.Handler(h)
h = middleware1(h)
http.ListenAndServe(addr, h)

This function signature makes it easier to do the most
common usage patterns I'm aware of, after several years
of seeing this package used in the wild.

	http.Handle("/", secureheader.Handler(mux))

and

	http.Handle("/a", a)
	http.Handle("/b", b)
	http.ListenAndServe(addr, secureheader.Handler(nil))

and

	var h http.Handler
	h = mux
	h = middleware3(h)
	h = secureheader.Handler(h)
	h = middleware1(h)
	http.ListenAndServe(addr, h)
@kr

kr commented Aug 6, 2018

Copy link
Copy Markdown
Owner Author

PTAL

@croaky croaky left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

Comment thread example_test.go
secureheader.DefaultConfig.HSTSIncludeSubdomains = false
secureheader.DefaultConfig.FrameOptions = false
http.ListenAndServe(":80", secureheader.DefaultConfig)
h := secureheader.Handler(http.DefaultServeMux)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Articles like this mention http.DefaultServeMux as a potential security gotcha. Would it be better to use something else here in the context of a security-focused library?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Yeah maybe. For the purpose of this PR, I just wanted to update the form of the examples, but not change their behavior. Opened #15 to track this.

@ryansmith3136 ryansmith3136 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛋 comfy.
lgtm

@kr
kr merged commit 9ede934 into main Aug 6, 2018
@kr
kr deleted the handler branch August 6, 2018 20:42
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.

3 participants