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

Improvement: Response headers handled in a case sensitive way #3

Closed
bcachet opened this issue Mar 27, 2018 · 3 comments
Closed

Improvement: Response headers handled in a case sensitive way #3

bcachet opened this issue Mar 27, 2018 · 3 comments

Comments

@bcachet
Copy link
Contributor

bcachet commented Mar 27, 2018

Hello,

In Request module, you parse headers (__responseHeaderCallback) in a case-sensitive way.
You do not enforce CURL to use HTTP 1.1 Headers in HTTP2.0 are lower case.
You can endup in not parsing the headers correctly.

protected function  __responseHeaderCallback(&$curl, &$data)
	{
		if (($strlen = strlen($data)) <= 2)
		{
			return $strlen;
		}

		if (substr($data, 0, 4) == 'HTTP')
		{
			$this->response->code = (int)substr($data, 9, 3);

			return $strlen;
		}

		list($header, $value) = explode(': ', trim($data), 2);

		switch (strtolower($header))
		{
			case 'last-modified':
				$this->response->setHeader('time', strtotime($value));
				break;

			case 'content-length':
				$this->response->setHeader('size', (int)$value);
				break;

			case 'content-type':
				$this->response->setHeader('type', $value);
				break;

			case 'etag':
				$this->response->setHeader('hash', $value{0} == '"' ? substr($value, 1, -1) : $value);
				break;

			default:
				if (preg_match('/^x-amz-meta-.*$/', $header))
				{
					$this->setHeader($header, is_numeric($value) ? (int)$value : $value);
				}
				break;
		}

		return $strlen;
	}

Hope it helps

@nikosdion
Copy link
Member

Could you please make a Pull Request with your changes?

@bcachet
Copy link
Contributor Author

bcachet commented Mar 27, 2018

Here it is: #4

@nikosdion
Copy link
Member

PR gh-4 has been already merged. This issue wasn't closed because the PR didn't reference the issue (yep, you can only link issues to PRs manually). So I'm closing this issue as fixed.

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

No branches or pull requests

2 participants