Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[Security] Fixed broken master build #36485

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 18, 2020
Merged

[Security] Fixed broken master build #36485

merged 1 commit into from
Apr 18, 2020

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Apr 17, 2020 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR n/a

The build failures are caused by these lines (line 100 specically):

symfony/src/Symfony/Component/Security/Http/Firewall/ContextListener.php

Lines 97 to 108 in 2460ca5

if (null !== $session) {
$usageIndexValue = $session instanceof Session ? $usageIndexReference = &$session->getUsageIndex() : 0;
$usageIndexReference = PHP_INT_MIN;
$sessionId = $request->cookies->get($session->getName());
$token = $session->get($this->sessionKey);
if ($this->sessionTrackerEnabler && \in_array($sessionId, [true, $session->getId()], true)) {
$usageIndexReference = $usageIndexValue;
} else {
$usageIndexReference = $usageIndexReference - PHP_INT_MIN + $usageIndexValue;
}
}

Since #34363, $request->cookies->get() is typehinted as string|null. On Travis with PHP=7.4, this doc typehint is transformed into PHP return type: get(): ?string.

On tests, the session cookie is set to true. See #36118 for some background on why this is necessary.

There are a couple possible solutions:

  1. Update the InputBag::get() PHPdoc to use @return scalar|null
  2. Use $request->cookie->all()[$session->getName()] in ContextListener
  3. Allow pre-configuring the session ID in MockArraySessionStorage.

I've implemented solution (1). The method is actually using is_scalar() to check if a deprecation notice should be triggered, so it is expected to return a scalar in Symfony 6.

I've had to update the DebugClassLoader to not convert this to get(): ?scalar, as that doesn't exists in PHP. I'm not sure if my changes are correct (but they work).

@wouterj wouterj force-pushed the fix-build branch 10 times, most recently from 986b2aa to 8a0dae4 Compare April 18, 2020 14:45
@wouterj wouterj marked this pull request as ready for review April 18, 2020 14:45
@wouterj wouterj changed the title [Security] Debug broken master build on travis only [HttpFoundation] Fixed broken master build Apr 18, 2020
@wouterj wouterj changed the title [HttpFoundation] Fixed broken master build [Security][HttpFoundation] Fixed broken master build Apr 18, 2020
@wouterj
Copy link
Member Author

wouterj commented Apr 18, 2020 •
edited
Loading

Hmm, seems like this very minor change introduces a new failing tests (on Travis only, but this time in all jobs, so not related to PHP return typing):

1) Symfony\Component\HttpFoundation\Tests\Session\Storage\Handler\AbstractSessionHandlerTest::testSession with data set #2 ('regenerate')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
 read\n
 doRead: abc|i:123;\n
 read\n
+doRead: abc|i:123;\n
 \n
 write\n
 doWrite: abc|i:123;\n

/home/travis/build/symfony/symfony/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/AbstractSessionHandlerTest.php:49

Looking at the test, I can't see Request or InputBag being used, so I'm again puzzled how this change affects session management. My knowledge of sessions is 0, so I think I need a bit of guidance.

@nicolas-grekas
Copy link
Member

I'm for using solution 2.
About the new failure, it's unrelated apparently - must be something else.

@wouterj wouterj changed the title [Security][HttpFoundation] Fixed broken master build [Security] Fixed broken master build Apr 18, 2020
@nicolas-grekas
Copy link
Member

I suspect the other failure to be related to the just released PHP 7.3.17, and to php/php-src@b510250 specifically.

@wouterj
Copy link
Member Author

wouterj commented Apr 18, 2020

Alright, so unrelated to this PR. Travis confirms this fixes the PHP 7.4 build for Security HTTP, so ready to merge imho :)

@nicolas-grekas
Copy link
Member

Thank you @wouterj.

@nicolas-grekas nicolas-grekas merged commit 23f5070 into symfony:master Apr 18, 2020
@nanasess nanasess mentioned this pull request Apr 21, 2020
Merged
11 tasks
nanasess added a commit to nanasess/ec-cube that referenced this pull request Apr 22, 2020
@nanasess
PHP7.4.5, 7.3.17 のバグでエラーになるためスキップ
0b25bda 
- see symfony/symfony#36485 (comment)
@ryo-endo ryo-endo mentioned this pull request May 22, 2020
Merged
11 tasks
@wouterj wouterj deleted the fix-build branch June 17, 2020 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Security Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wouterj @nicolas-grekas @carsonbot
Morty Proxy This is a proxified and sanitized view of the page, visit original site.