Correcting multiple X-Frame-Options header #50
Conversation
…LLOW FROM are valid values and they are mutually exclusive; that is, the header field must be set to exactly ONE of these three values. This will prevent the CSRF code from inserting it multiple times as well as duplicating it if it was already set elsewhere (e.g. IIS Header)
|
@chekm8, |
|
I was just checking back to see if there was anything additional I had to do for the pull to be reviewed/accepted. This is my first contribution and I believe the license agreement was completed successfully. |
| @@ -104,7 +104,11 @@ public TagBuilder GetFormInputElement(HttpContextBase httpContext) | |||
| // Adding X-Frame-Options header to prevent ClickJacking. See | |||
| // http://tools.ietf.org/html/draft-ietf-websec-x-frame-options-10 | |||
| // for more information. | |||
| httpContext.Response.AddHeader("X-Frame-Options", "SAMEORIGIN"); | |||
There was a problem hiding this comment.
Should this instead just be changed to:
httpContext.Response.Headers.Set("X-Frame-Options", "SAMEORIGIN");This would make sure that anti-forgery gets the header it wants by overwriting any existing value.
There was a problem hiding this comment.
Thanks for getting back to me Eilon. The "If"/Add check was to ensure that say if an administrator in IIS forced this header to a setting other than SAMEORIGIN (e.g. DENY or ALLOW FROM) it would allow them to. If you always just set it to SAMEORIGIN you are not giving the admin the option to override it.
There was a problem hiding this comment.
I was just checking back in, does the above comment make sense?
There was a problem hiding this comment.
Ok this makes sense. I guess it's debatable which one should win over the other but I don't feel strongly about that, so this should be fine.
|
BTW sorry for the long delay on this! I got sent some reminders from a couple of colleagues to get back to you on this. |
|
@dougbu can you do an extra review and merge when ready? |
| @@ -104,7 +104,11 @@ public TagBuilder GetFormInputElement(HttpContextBase httpContext) | |||
| // Adding X-Frame-Options header to prevent ClickJacking. See | |||
| // http://tools.ietf.org/html/draft-ietf-websec-x-frame-options-10 | |||
| // for more information. | |||
| httpContext.Response.AddHeader("X-Frame-Options", "SAMEORIGIN"); | |||
| var xFrameHeaderName = "X-Frame-Options"; | |||
There was a problem hiding this comment.
Oh actually one tiny comment: this should be a const string because it's not really a variable.
There was a problem hiding this comment.
Please also rename this const because the command-line build hits an FxCop error:
SA130: The variable name 'xFrameHeaderName' begins with a prefix that looks like Hungarian notation. Remove the prefix or add it to the list of allowed prefixes.
| @@ -104,7 +104,11 @@ public TagBuilder GetFormInputElement(HttpContextBase httpContext) | |||
| // Adding X-Frame-Options header to prevent ClickJacking. See | |||
| // http://tools.ietf.org/html/draft-ietf-websec-x-frame-options-10 | |||
| // for more information. | |||
| httpContext.Response.AddHeader("X-Frame-Options", "SAMEORIGIN"); | |||
| const string xFrameHeaderName = "X-Frame-Options"; | |||
There was a problem hiding this comment.
Name will still fail a build with the SA130 error. (Might be worth setting up AppVeyor for this repo.)
There was a problem hiding this comment.
@dougbu I uploaded the string const change to my repo, Do I need to initiate another pull request or anything?
Do you need further changes to get past the SA130 error?
There was a problem hiding this comment.
Yes, please renamed the const to something like frameHeader or frameHeaderName. That'll get command-line builds working.
There was a problem hiding this comment.
All set, let me know if you need anything else. Thanks.
| @@ -104,7 +104,11 @@ public TagBuilder GetFormInputElement(HttpContextBase httpContext) | |||
| // Adding X-Frame-Options header to prevent ClickJacking. See | |||
| // http://tools.ietf.org/html/draft-ietf-websec-x-frame-options-10 | |||
| // for more information. | |||
| httpContext.Response.AddHeader("X-Frame-Options", "SAMEORIGIN"); | |||
| const string frameHeaderName = "X-Frame-Options"; | |||
There was a problem hiding this comment.
Shoot. Command-line build now fails with
SA1303: Constants must start with an upper-case letter: frameHeaderName.
Please rename this again -- FrameHeaderName. (I wasn't aware this error was enabled.)
|
@dougbu Thanks, I know I can grab a nightly build but, do you guy have a estimated timeline for your next RTM release and nuget package build? I'm not really sure how that process works. |
|
@chekm8 the best place to see the future is https://github.com/aspnet/Home/wiki/Roadmap The page needs a refresh now that 2.0.0 is out. But, I recommend checking there occasionally for more detail on 2.1.0. |
According to RFC7034, only these three values, DENY, SAMEORIGIN and ALLOW FROM are valid values and they are mutually exclusive; that is, the header field must be set to exactly one of these three values. This will prevent the CSRF code from inserting it multiple times as well as duplicating it if it was already set elsewhere (e.g. IIS Header)
Addresses #7