Bug 208924 – Make sure a preflight fails if response headers are invalid

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[youenn fablet]

Make sure a preflight fails if response headers are invalid

[youenn fablet]

Created attachment 393260 [details]
Patch

[youenn fablet]

Created attachment 393351 [details]
Patch

[youenn fablet]

Created attachment 393362 [details]
Patch

[youenn fablet]

Ping review

[Alex Christensen]

Comment on attachment 393362 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Minor refactoring to return Expected/Optional for erro rhandlng instead of passing an out parameter.

error

> Source/WebCore/loader/CrossOriginAccessControl.cpp:273
> +    CrossOriginPreflightResultCache::singleton().appendEntry(securityOrigin.toString(), request.url(), entry.moveToUniquePtr());

Do we want to do this if there was an error?  We didn't before.

> Source/WebCore/loader/CrossOriginPreflightResultCache.h:54
> +    bool allowsCrossOriginMethod(const String&, StoredCredentialsPolicy, String& errorDescription) const;
> +    bool allowsCrossOriginHeaders(const HTTPHeaderMap&, StoredCredentialsPolicy, String& errorDescription) const;

These should return Expected<void, String> instead of having an out parameter.

[youenn fablet]

CrossOriginPreflightResultCache::singleton().appendEntry(securityOrigin.toString(), request.url(), entry.moveToUniquePtr());
> 
> Do we want to do this if there was an error?  We didn't before.

Yes, I think so, this is explained in the change log

> > Source/WebCore/loader/CrossOriginPreflightResultCache.h:54
> > +    bool allowsCrossOriginMethod(const String&, StoredCredentialsPolicy, String& errorDescription) const;
> > +    bool allowsCrossOriginHeaders(const HTTPHeaderMap&, StoredCredentialsPolicy, String& errorDescription) const;
> 
> These should return Expected<void, String> instead of having an out
> parameter.

Sure, this is preexisting and could be dealt with as a follow-up.
This patch moves these method from public to private so that at least we do not make this pattern more widespread.

[youenn fablet]

Created attachment 393838 [details]
Patch for landing

[youenn fablet]

> Sure, this is preexisting and could be dealt with as a follow-up.
> This patch moves these method from public to private so that at least we do
> not make this pattern more widespread.

https://bugs.webkit.org/show_bug.cgi?id=209224

[WebKit Commit Bot]

Comment on attachment 393838 [details]
Patch for landing

Clearing flags on attachment: 393838

Committed r258631: <https://trac.webkit.org/changeset/258631>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60584210>