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

fix(cors) improve behavior of regex matching #4261

Closed
wants to merge 1 commit into from

Conversation

hishamhm
Copy link
Contributor

This reverts a regression introduced in #3872, in which regexes were being
forcibly anchored and matched against normalized domains, leading to a
breaking change with regard to the 0.x behavior.

In 0.x, regexes such as (.*[.])?foo\.test would accept subdomain entries
(but were subject to bug #3832 as it would also accept foo.test.evil.test);
in 1.0.2, the latter is not accepted, but the regular uses failed as well,
because regexes were translated to ^(.*[.])?foo\.test$ but were then matched
against the normalized domains in a way that always included the port.

With this commit, both configured origins and the input header value are
normalized so that default ports are not an issue for both regex and non-regex
cases. We verify if each configured origin is a regex or not, then:

  • for non-regex entries, we do plain equality matching against the normalized
    domain
  • For regex entries,
    • if the regex contains :, we do an anchored match against the normalized
      domain
    • otherwise, we do an anchored match against the host component only
      (to account for the 0.x behavior where ports were not considered)

Matching domains with regexes must be done with care (for starters,
note that dots must be escaped), so we recommend using plain-text full-domain
matching whenever possible.

This change in behavior is arguably a breaking change, but it is a compromise
between a workable behavior and backwards compatibility. Good domain-matching
regexes such as (.*[.])?foo\.test will remain matching against the host
component as intended (without being subject to #3832). Naive regexes such as
.foo.test will stop "working", but these were vulnerable to #3832 anyway. In
particular, thorough regexes such as
^https?://(.*[.])?foo\\.test(:(80|90))?$ that performed their own anchoring
remain working as well.

This reverts a regression introduced in #3872, in which regexes were being
forcibly anchored and matched against normalized domains, leading to a
breaking change with regard to the 0.x behavior.

In 0.x, regexes such as `(.*[.])?foo\.test` would accept subdomain entries
(but were subject to bug #3832 as it would also accept `foo.test.evil.test`);
in 1.0.2, the latter is not accepted, but the regular uses failed as well,
because regexes were translated to `^(.*[.])?foo\.test$` but were then matched
against the normalized domains in a way that always included the port.

With this commit, both configured origins and the input header value are
normalized so that default ports are not an issue for both regex and non-regex
cases. We verify if each configured origin is a regex or not, then:

* for non-regex entries, we do plain equality matching against the normalized
  domain
* For regex entries,
  * if the regex contains `:`, we do an anchored match against the normalized
    domain
  * otherwise, we do an anchored match against the host component only
    (to account for the 0.x behavior where ports were not considered)

Matching domains with regexes must be done with care (for starters,
note that dots must be escaped), so we recommend using plain-text full-domain
matching whenever possible.

This change in behavior is arguably a breaking change, but it is a compromise
between a workable behavior and backwards compatibility. Good domain-matching
regexes such as `(.*[.])?foo\.test` will remain matching against the host
component as intended (without being subject to #3832). Naive regexes such as
`.foo.test` will stop "working", but these were vulnerable to #3832 anyway. In
particular, thorough regexes such as
`^https?://(.*[.])?foo\\.test(:(80|90))?$` that performed their own anchoring
remain working as well.
@hishamhm hishamhm added this to the 1.0.3 milestone Jan 29, 2019
@hishamhm hishamhm requested a review from thibaultcha January 30, 2019 12:32

if accept then
assert.equal("GET,HEAD,PUT,PATCH,POST,DELETE", res.headers["Access-Control-Allow-Methods"])
assert.equal(accept == true and origin or accept, res.headers["Access-Control-Allow-Origin"])
Copy link
Member

@kikito kikito Jan 30, 2019

Choose a reason for hiding this comment

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

accept can be true, false or a domain. That was ... surprising. I initially thought this tertiary operator was unnecessary (given the if accept clause two lines above). Approved nonetheless.

@thibaultcha
Copy link
Member

Manually merged, thanks!

thibaultcha pushed a commit that referenced this pull request Jan 30, 2019
This reverts a regression introduced in #3872, in which regexes were
being forcibly anchored and matched against normalized domains, leading
to a breaking change with regard to the 0.x behavior.

In 0.x, regexes such as `(.*[.])?foo\.test` would accept subdomain
entries (but were subject to bug #3832 as it would also accept
`foo.test.evil.test`); in 1.0.2, the latter is not accepted, but the
regular uses failed as well, because regexes were translated to
`^(.*[.])?foo\.test$` but were then matched against the normalized
domains in a way that always included the port.

With this commit, both configured origins and the input header value are
normalized so that default ports are not an issue for both regex and
non-regex cases. We verify if each configured origin is a regex or not,
then:

* for non-regex entries, we do plain equality matching against the normalized
  domain
* For regex entries,
  * if the regex contains `:`, we do an anchored match against the normalized
    domain
  * otherwise, we do an anchored match against the host component only
    (to account for the 0.x behavior where ports were not considered)

Matching domains with regexes must be done with care (for starters, note
that dots must be escaped), so we recommend using plain-text full-domain
matching whenever possible.

This change in behavior is arguably a breaking change, but it is a
compromise between a workable behavior and backwards compatibility. Good
domain-matching regexes such as `(.*[.])?foo\.test` will remain matching
against the host component as intended (without being subject to #3832).
Naive regexes such as `.foo.test` will stop "working", but these were
vulnerable to #3832 anyway. In particular, thorough regexes such as
`^https?://(.*[.])?foo\\.test(:(80|90))?$` that performed their own
anchoring remain working as well.

From #4261

Signed-off-by: Thibault Charbonnier <[email protected]>
@thibaultcha thibaultcha deleted the fix/cors-regex-match branch January 30, 2019 19:11
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