1279324 - Access-Control-Allow-Origin header doesn't correctly handle multiple values when one of the values is empty

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Decky Coss (:deckycoss)]

According to the CORS spec (https://www.w3.org/TR/cors/#resource-sharing-check-0), a response should not have more than one value for Access-Control-Allow-Origin. However, the CORS origin web-platform-test shows that the check for multiple values currently fails when there are two values, and one of those values is the empty string: http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/cors/origin.htm.ini

This seems to happen because nsHttpHeaderArray::MergeHeader will merge "*" and "" without a comma separator, regardless of the order they are defined in. Then, because nsCORSListenerProxy checks the result of the merge instead of the individually defined values, it will find "*", which is a valid value.

Comment 1

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Currently assigned to Decky.

Comment 2

[Decky Coss (:deckycoss)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=717bfb589abd

Comment 3

[Decky Coss (:deckycoss)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2cefc56a439

Comment 4

[Decky Coss (:deckycoss)]

Attachment: Bug 1279324: use VisitOriginalResponseHeaders to check for multiple Access-Control-Allow-Origin values;

Review commit: https://reviewboard.mozilla.org/r/60084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60084/

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8764046 [details]
Bug 1279324: use VisitOriginalResponseHeaders to check for multiple Access-Control-Allow-Origin values;

https://reviewboard.mozilla.org/r/60084/#review57060

r=me

::: netwerk/protocol/http/nsCORSListenerProxy.cpp:509
(Diff revision 1)
> +
> +  uint32_t headerCount = 0;
> +
> +  ~CheckOriginHeader()
> +  {}
> +//

Nit: Even though the default is private, please explicitly define private: and have all private members be declared underneath the public: ones.

Please remove the // and please prefix headerCount with m - mHeaderCount;

Please also add an explicit constructor and have mHeaderCounter be initialized in the initilization list.

CheckOriginHeader::CheckOriginHeader()
: mHeaderCount(0)
{}

::: netwerk/protocol/http/nsCORSListenerProxy.cpp:516
(Diff revision 1)
> +  NS_DECL_ISUPPORTS
> +
> +  NS_IMETHOD
> +  VisitHeader(const nsACString & aHeader, const nsACString & aValue) override
> +  {
> +    ErrorResult result;

nit: remove ErrorResult result; it's not used, right?

::: netwerk/protocol/http/nsHttpHeaderArray.cpp:149
(Diff revision 1)
>          }
>          nsresult rv = MergeHeader(header, entry, value, variety);
>          if (NS_FAILED(rv)) {
>              return rv;
>          }
> +        // nsresult rv;

nit: please remove the commented out nsresult rv;

Comment 6

[Decky Coss (:deckycoss)]

Comment on attachment 8764046 [details]
Bug 1279324: use VisitOriginalResponseHeaders to check for multiple Access-Control-Allow-Origin values;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60084/diff/1-2/

Comment 7

[Decky Coss (:deckycoss)]

(In reply to Decky Coss (:deckycoss) from comment #6)
> Comment on attachment 8764046 [details]
> Bug 1279324: use VisitOriginalResponseHeaders to check for multiple
> Access-Control-Allow-Origin values;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/60084/diff/1-2/

one thing i should mention is that at nsCORSListenerProxy.cpp:520, i return NS_ERROR_DOM_BAD_URI when the visitor detects too many header values. i don't know that this is the best error code to return, but i wasn't sure what else to use.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Decky Coss (:deckycoss) from comment #7)
> one thing i should mention is that at nsCORSListenerProxy.cpp:520, i return
> NS_ERROR_DOM_BAD_URI when the visitor detects too many header values. i
> don't know that this is the best error code to return, but i wasn't sure
> what else to use.

I looked at that; given that surrounding errors also bail with NS_ERROR_DOM_BAD_URI makes me think this is the right error to return.

Comment 9

[Decky Coss (:deckycoss)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3bb55cb9963

Comment 10

[Decky Coss (:deckycoss)]

Comment on attachment 8764046 [details]
Bug 1279324: use VisitOriginalResponseHeaders to check for multiple Access-Control-Allow-Origin values;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60084/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5387a7d0920
use VisitOriginalResponseHeaders to check for multiple Access-Control-Allow-Origin values; r=ckerschb

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e5387a7d0920