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

Allow browser headers to be set for the friendly html page #43

Merged
merged 1 commit into from
Nov 2, 2017

Conversation

matt-richardson
Copy link
Contributor

Allows us to set security related headers such as X-Frame-Options to avoid flagging up on security scanning tools

Related to OctopusDeploy/Issues#3884

Allows us to set security related headers such as `X-Frame-Options` to avoid flagging up on security scanning tools

Related to OctopusDeploy/Issues#3884
@@ -315,6 +330,39 @@ static string DownloadStringIgnoringCertificateValidation(string uri)
#endif
}

static IEnumerable<KeyValuePair<string, string>> GetHeadersIgnoringCertificateValidation(string uri)
{
#if NET40
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use HttpClient for both cases? No need to deal with ServicePointManager static shenanigans... :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would make sense...

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately that is not that case with the latest versions, OctopusDeploy/OctopusClients#121

@droyad
Copy link
Contributor

droyad commented Oct 29, 2017

Any reason to have the headers configurable instead of just including the security ones by default?

@michaelnoonan
Copy link
Contributor

It's a good point I think.

I ended up making the friendly HTML configurable back in 3.0 because the main reason this page is used is to diagnose connection issues. It's helpful to have a custom message to describe what the endpoint actually represents, a Tentacle or the Octopus Server.

In the case of security headers, I'm not sure they need to be configurable - will they be likely to change between Octopus Server, Tentacle, or other consumers of our library? If I'm an external (non-Octopus) consumer of Halibut, would I be blocked in any way by these headers being hard-coded?

@matt-richardson
Copy link
Contributor Author

My thinking was:

  • this is how the custom html page was implemented - it seemed logical to follow the same pattern
  • it would be odd to have different headers returned from the site and from the comms port. I could see potential support issues there, especially if one of them is turned off / configured to return a different value
  • if we make them configurable like this, as soon as we add them to the server, they get added to the comms port (not the case with Tentacle though)

In the case of security headers, I'm not sure they need to be configurable - will they be likely to change between Octopus Server, Tentacle, or other consumers of our library?
Not likely. Until someone demands we implement Expect_CT or similar

If I'm an external (non-Octopus) consumer of Halibut, would I be blocked in any way by these headers being hard-coded?
At a stretch, I could see someone doing something with an iframe to show the diagnostics page. (Unlikely, but possible.) Adding hard coded headers could break this.

I'm not wedded to the current solution - if either of you feeling strongly about it, I'm happy to change.

@matt-richardson matt-richardson merged commit 80372d3 into master Nov 2, 2017
@matt-richardson matt-richardson deleted the enh-browserheaders branch November 2, 2017 02:07
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