Skip to content

Correcting multiple X-Frame-Options header #50

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Sep 21, 2017
Merged

Correcting multiple X-Frame-Options header #50

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d20956a
According to RFC7034, only these three values, DENY, SAMEORIGIN and A…
chekm8 Jun 19, 2017
3048853
Changed var to const string per request.
chekm8 Sep 21, 2017
46bb648
Changed const name to avoid SA130 error
chekm8 Sep 21, 2017
f7a6ea7
Changing to correct cost naming per standard
chekm8 Sep 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/System.Web.WebPages/Helpers/AntiXsrf/AntiForgeryWorker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just checking back in, does the above comment 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.

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.

const string xFrameHeaderName = "X-Frame-Options";
Copy link
Contributor

Choose a reason for hiding this comment

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

Name will still fail a build with the SA130 error. (Might be worth setting up AppVeyor for this repo.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, please renamed the const to something like frameHeader or frameHeaderName. That'll get command-line builds working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All set, let me know if you need anything else. Thanks.

if (httpContext.Response.Headers[xFrameHeaderName] == null)
{
httpContext.Response.AddHeader(xFrameHeaderName, "SAMEORIGIN");
}
}

// <input type="hidden" name="__AntiForgeryToken" value="..." />
Expand Down