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

Handle invalid Origin header values in CorsMiddleware #60

Merged
merged 3 commits into from
Dec 8, 2023

Conversation

boesing
Copy link
Member

@boesing boesing commented Dec 8, 2023

Q A
Bugfix yes
BC Break no
New Feature yes

Description

For requests with invalid Origin URIs (i.e. when using laminas/laminas-diactoros, passing chrome-extension://asdkjasdkjasjkdasjkd will throw InvalidOriginValueException), the CorsMiddleware is throwing an unhandled exception, which ends up in HTTP 500 in mezzio applications (unless the error is catched in the error handler and whyever some1 would then return a different HTTP status code).

So to properly handle this (as it some PSR-7 implementations do allow any scheme), I've created laminas/laminas-diactoros#179.
However, after some feedback and more research, I found out that PSR-7 is actually quite "clear" on what MUST be supported and what MAY be supported:

Implementations MUST support the schemes "http" and "https" case insensitively, and MAY accommodate other schemes if required.

It also states that unsupported schemes may end up in an InvalidArgumentException:

/**
 * Return an instance with the specified scheme.
 *
 * This method MUST retain the state of the current instance, and return
 * an instance that contains the specified scheme.
 *
 * Implementations MUST support the schemes "http" and "https" case
 * insensitively, and MAY accommodate other schemes if required.
 *
 * An empty scheme is equivalent to removing the scheme.
 *
 * @param string $scheme The scheme to use with the new instance.
 * @return static A new instance with the specified scheme.
 * @throws \InvalidArgumentException for invalid or unsupported schemes.
 */
public function withScheme(string $scheme): UriInterface;

So as a conclusion, I decided to handle these problems in mezzio-cors since InvalidOriginValueException may also occur for other issues unrelated to the scheme.

So this patch handles InvalidOriginValueException in CorsMiddleware when passing the request to CorsInterface#isCorsRequest and returns an unauthorized response now.
Prior this patch, an unhandled exception (InvalidOriginValueException) was bubbling up the middleware pipeline in mezzio applications and thus, this change might not introduce a real BC break, WDYT?

I am targeting the next minor as I also introduced a way to actually fetch the RAW Origin header value from the InvalidOriginValueException so that consumers of the CorsInterface can access the header value (so does CorsMiddleware).

@boesing boesing added the Enhancement New feature or request label Dec 8, 2023
@boesing boesing added this to the 1.11.0 milestone Dec 8, 2023
…inValueException`

Signed-off-by: Maximilian Bösing <[email protected]>
For origins resulting in `InvalidOriginValueException`, we can assume that these are actual CORS requests. If these are made from unsupported origins, we should treat these as unauthorized requests.

Signed-off-by: Maximilian Bösing <[email protected]>
@boesing boesing force-pushed the bugfix/invalid-origin-values branch from 3d6b5b0 to b08de20 Compare December 8, 2023 17:41
@boesing boesing requested a review from Ocramius December 8, 2023 17:41
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

🚢

@Ocramius Ocramius self-assigned this Dec 8, 2023
@Ocramius Ocramius merged commit 425b0fd into mezzio:1.11.x Dec 8, 2023
@boesing boesing deleted the bugfix/invalid-origin-values branch December 9, 2023 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants