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 AllowedHosts #6142

Merged
merged 3 commits into from
May 1, 2018
Merged

Add AllowedHosts #6142

merged 3 commits into from
May 1, 2018

Conversation

guardrex
Copy link
Collaborator

Fixes #5793

@Tratcher @scottaddie In the new table, you only need to look at the first row for AllowedHosts. Nothing else in the table changes on this PR.

@Tratcher By "normalized form" in the API comments, it means "conventional" format, correct? ... because the spec doesn't refer to "normalized" ... it only speaks to "conventional" formats.

Also, leading zeros are fine in IPv6 "conventional" addresses ... they're ok here as well (i.e., Kestrel fills them in perhaps)?

@guardrex guardrex requested review from Tratcher and scottaddie April 30, 2018 20:24
::: moniker range=">= aspnetcore-2.1"
| Option | Description |
| ------ | ----------- |
| `AllowedHosts` | Restricts hosts by the `X-Forwarded-Host` header to the values provided. Port numbers must be excluded. If the list is empty, all hosts are allowed. A top level wildcard `*` allows all non-empty hosts. Subdomain wildcards are permitted but don't match the root (naked) domain [for example, `*.contoso.com` matches the subdomain `foo.contoso.com` but not the root (naked) domain `contoso.com`]. Unicode host names are allowed but are converted to [Punycode](https://tools.ietf.org/html/rfc3492) for matching. [IPv6 addresses](https://tools.ietf.org/html/rfc4291) must include bounding brackets and be in [conventional form](https://tools.ietf.org/html/rfc4291#section-2.2) (for example, `[ABCD:EF01:2345:6789:ABCD:EF01:2345:6789]`). Failure to restrict the allowed hosts may allow an attacker to spoof links generated by the service.<br><br>The default value is an empty [IList\<string>](/dotnet/api/system.collections.generic.ilist-1). |
Copy link
Member

Choose a reason for hiding this comment

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

  • top level --> top-level
  • Delete the square braces around the following, and make it a sentence:

[for example, *.contoso.com matches the subdomain foo.contoso.com but not the root (naked) domain contoso.com]

Copy link
Member

Choose a reason for hiding this comment

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

naked?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's one of the terms. Bare or apex are the others, but they're not as common.

@guardrex
Copy link
Collaborator Author

Since the bracketed part of that item is part of one requirement, let's try an in-cell list. Let me know if it breaks, since I can't see it.

https://review.docs.microsoft.com/en-us/aspnet/core/host-and-deploy/proxy-load-balancer?view=aspnetcore-1.0&branch=pr-en-us-6142

@guardrex
Copy link
Collaborator Author

I see a new DocFX is out tho ... Version 2.35.2 ... I'll give that try and see if more bits are working locally.

@scottaddie
Copy link
Member

Here's how it displays:
image

Can you also remove the code fencing from AllowedHosts?

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

Values are compared using ordinal-ignore-case. IPv6 addresses are not special cased to check for logical equality between different formats and no canonicalization will be performed for you.

::: moniker range=">= aspnetcore-2.1"
| Option | Description |
| ------ | ----------- |
| `AllowedHosts` | Restricts hosts by the `X-Forwarded-Host` header to the values provided. Port numbers must be excluded. If the list is empty, all hosts are allowed. A top level wildcard `*` allows all non-empty hosts. Subdomain wildcards are permitted but don't match the root (naked) domain [for example, `*.contoso.com` matches the subdomain `foo.contoso.com` but not the root (naked) domain `contoso.com`]. Unicode host names are allowed but are converted to [Punycode](https://tools.ietf.org/html/rfc3492) for matching. [IPv6 addresses](https://tools.ietf.org/html/rfc4291) must include bounding brackets and be in [conventional form](https://tools.ietf.org/html/rfc4291#section-2.2) (for example, `[ABCD:EF01:2345:6789:ABCD:EF01:2345:6789]`). Failure to restrict the allowed hosts may allow an attacker to spoof links generated by the service.<br><br>The default value is an empty [IList\<string>](/dotnet/api/system.collections.generic.ilist-1). |
Copy link
Member

Choose a reason for hiding this comment

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

naked?

@Tratcher
Copy link
Member

Naked?

@guardrex
Copy link
Collaborator Author

@scottaddie U sure ... if it's a property, I thought we were fencing them. Well ... truly ... I've been using API references (links) without styling, but that's when it's possible to do so. I can't with the 2.1 stuff. I don't think that matters here, so just confirm that you want it removed given that it is a property.

@Tratcher ...

Values are compared using ordinal-ignore-case. IPv6 addresses are not special cased to check for logical equality between different formats and no canonicalization will be performed for you.

In which case, would you say it's inaccurate for the API comments to remark that "normalized form" is ok? ... I mean I guess it's just unclear what that means in context of the spec. Anyway ... On the next commit, I'll get an updated line in there for that along the lines of what you just said.

YES! "naked!" lol I hate for it to sound so seedy ... many devs know the "root" domain as the "naked domain" down in SoCal. Probably a bunch of whacky devs down there who like the word. I'll change it to "root domain" ... I think our "naked"-domain peeps will know that it's the same thing. 😄

@guardrex
Copy link
Collaborator Author

guardrex commented Apr 30, 2018

@scottaddie Plz take one more look to make sure everything is good. I'm done; so if it's 👍, feel free to :shipit:.

@scottaddie
Copy link
Member

@guardrex Normally, I'd have you keep the code-fencing. In this case, all the other properties aren't fenced. For the sake of consistency, and to avoid extra work, it's best to remove.

@scottaddie scottaddie merged commit eea51cc into master May 1, 2018
@scottaddie scottaddie deleted the guardrex/allowed-hosts-update branch May 1, 2018 15:16
@guardrex
Copy link
Collaborator Author

guardrex commented May 1, 2018

For the sake of consistency

Yes, but the rest aren't code fenced by virtue of being styled for links. I didn't think we had a repo rule that provided an exception like this. After all, where does it stop? ... Do we not code fence in lists, too? ... What about in paragraphs under the same conditions? I see consistency the other way: I think introducing an exception is problematic from a consistency POV. Anyway ... doesn't matter. I'll swing back in here later and get the link as soon as the bits go public.

to avoid extra work

I have to come back anyway to put the API link in, so the work is the same basically.

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.

4 participants