Bug 166624 – SecurityOrigin::create triplet constructor does not canonicalize port

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Michael Catanzaro]

WebCore::SecurityOrigin has four public create functions:

    WEBCORE_EXPORT static Ref<SecurityOrigin> create(const URL&);
    static Ref<SecurityOrigin> createUnique();

    WEBCORE_EXPORT static Ref<SecurityOrigin> createFromString(const String&);
    WEBCORE_EXPORT static Ref<SecurityOrigin> create(const String& protocol, const String& host, std::optional<uint16_t> port);

When a WebCore::SecurityOrigin is created using the first function, the port is canonicalized by unsetting it if it matches the default port for a protocol. That is handled by this code in the constructor:

    if (m_port && isDefaultPortForProtocol(m_port.value(), m_protocol))
        m_port = std::nullopt;

The second create function is not relevant to this bug.

The third function, createFromString, converts a String to a URL and then uses this path. So, for example, the expression SecurityOrigin::createFromString("http://example.com:80").port() would return nullopt, as it would if the port was omitted from the URL string. This is correct as per RFC 6454 because http://example.com and http://example.com:80 have the same security origin.

The issue lies with the fourth constructor. It does not perform this canonicalization, allowing two different nonequal SecurityOrigin objects to be created that represent the same security origin. That is, this constructor could be used to create a SecurityOrigin with protocol http, port 80, even though the other constructors would always canonicalize port 80 to nullopt.

I noticed this when working on exposing WebKitSecurityOrigin in the GTK+ API. I'm performing the canonicalization at the GTK+ API level for now, but I think it makes sense to do it in WebCore instead. I wonder if changing this in WebCore would expose unexpected side effects.

[Michael Catanzaro]

(In reply to comment #0) 
> The third function, createFromString, converts a String to a URL and then
> uses this path.

I meant: "and then uses the first create function"

[Michael Catanzaro]

Created attachment 297862 [details]
Patch

[Michael Catanzaro]

Patch largely just to see whether it survives EWS.

It does pass the GTK+ API tests I added in bug #163366 with my GTK-layer workaround removed from that patch, but it looks easy to add a cross-platform test so should probably do that....

[Michael Catanzaro]

EWS seems happy. Anyone know if this would be a bad idea?

I wrote a little cross-platform test. It needs to be added to XCode build system. Shame to see that WebCore tests are listed separately for each port and so most are never built at all on most ports. :(

[Michael Catanzaro]

Created attachment 297868 [details]
Patch

[Daniel Bates]

Comment on attachment 297868 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297868&action=review

> Tools/TestWebKitAPI/Tests/WebCore/SecurityOrigin.cpp:47
> +    Ref<SecurityOrigin> o1 = SecurityOrigin::create("http", "example.com", std::optional<uint16_t>(80));
> +    Ref<SecurityOrigin> o2 = SecurityOrigin::create("http", "example.com", std::optional<uint16_t>());

Can we use std::make_optional()?

[Daniel Bates]

(In reply to comment #4)
> EWS seems happy. Anyone know if this would be a bad idea?
> 
> I wrote a little cross-platform test. It needs to be added to XCode build
> system. 

Yes, we need to add it. I do not have a clean working copy at the moment. I will look to do this tomorrow morning unless someone else beats me.

[Michael Catanzaro]

(In reply to comment #6)
> Comment on attachment 297868 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297868&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebCore/SecurityOrigin.cpp:47
> > +    Ref<SecurityOrigin> o1 = SecurityOrigin::create("http", "example.com", std::optional<uint16_t>(80));
> > +    Ref<SecurityOrigin> o2 = SecurityOrigin::create("http", "example.com", std::optional<uint16_t>());
> 
> Can we use std::make_optional()?

Yeah, but I don't know if there's any advantage to doing so. At least it's more typing:

Ref<SecurityOrigin> o1 = SecurityOrigin::create("http", "example.com", std::make_optional<uint16_t>(80));
Ref<SecurityOrigin> o2 = SecurityOrigin::create("http", "example.com", std::optional<uint16_t>(std::nullopt));

I guess make_optional is more obviously useful when you don't need to explicitly specify the type, but here we do.

[Michael Catanzaro]

(In reply to comment #8)
> Yeah, but I don't know if there's any advantage to doing so. At least it's
> more typing:
> 
> Ref<SecurityOrigin> o1 = SecurityOrigin::create("http", "example.com",
> std::make_optional<uint16_t>(80));
> Ref<SecurityOrigin> o2 = SecurityOrigin::create("http", "example.com",
> std::optional<uint16_t>(std::nullopt));

Um, I forgot to add the "make_" in the second call and didn't notice until I posted it on Bugzilla. I don't think there's a way to use make_optional to create an uninitialized optional, so I think only for the first one.

[Daniel Bates]

Created attachment 297870 [details]
Patch

Added file Tools/TestWebKitAPI/Tests/WebCore/SecurityOrigin.cpp to TestWebKitAPI Xcode project. Also substituted word "unequal" for "nonequal" (sic) in WebCore ChangeLog description so that it reads well.

[Michael Catanzaro]

Looks like we need to add WEBCORE_EXPORT to the declaration of isSameOriginAs.

[Michael Catanzaro]

Created attachment 297872 [details]
Patch

[Michael Catanzaro]

(Also declared SetUp() as final instead of virtual.)

[WebKit Commit Bot]

Comment on attachment 297872 [details]
Patch

Clearing flags on attachment: 297872

Committed r210218: <http://trac.webkit.org/changeset/210218>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.