Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Issue #2242749 by znerol, torotil, rszrama, larowlan, dawehner, penyaskito, tim.plunkett, sun, Damien Tournoud, David_Rothstein, effulgentsia: Fixed Port Form API security fix SA-CORE-2014-002 to Drupal 8.
See https://drupal.org/SA-CORE-2014-002. This is going to be a fun one.
Drupal 7 code can be found at http://drupalcode.org/project/drupal.git/commitdiff/6642fbc7001c728e2181... (and Drupal 6 code at http://drupalcode.org/project/drupal.git/commitdiff/66e94d74994fced9fafb...).
Comment | File | Size | Author |
---|---|---|---|
#44 | 2242749-form-cache-44.patch | 47.46 KB | penyaskito |
#1 | SA-CORE-2014-002.do-not-test.patch | 26.25 KB | sun |
Comments
Comment #1
sunThe D7 fix is publicly accessible http://drupalcode.org/project/drupal.git/commitdiff/6642fbc7001c728e2181...
So here's the D7 patch for reference.
Comment #2
penyaskitoWorking on this.
Comment #3
penyaskitoIt still does not work, but it is a start.
Comment #5
penyaskitoCreated UpdateBuildIdCommand. Renamed command key from updateBuildId to update_build_id for consistency. Proper cache config saving.
Still same failures.
Comment #7
penyaskitoFixed FormBuilderTest by injecting services. Had to redefine drupal_page_is_cacheable(), ugly as hell, but I guess acceptable.
Comment #8
penyaskitoArgh, this introduces a circular dependency :(
Comment #10
penyaskitoI was getting "A fatal error occurred:" with no other information, and it was because of AccessDeniedHttpException in AccessSubscriber::onKernelRequestAccessCheck. Maybe we should add a message there in a different issue.
The attached patch should fix at least some tests.
Comment #12
penyaskitoI was calling the wrong form. Locally this is down to 1 failure in FormCacheTest.
Comment #14
penyaskitoClean-up of the patch, nothing functional.
I'm not sure if the failure is expected with this change. Unassigning for now.
Comment #18
larowlanComment #19
star-szrThe main conflict looks to be #2328777: Refactor FAPI getCache()/setCache() into a standalone class, so I would think the caching changes need to be moved to Drupal\Core\Form\FormCache.
Comment #20
tim.plunkettHere's the patch using FormCache, with added unit tests.
The interdiff isn't exact, as I had to rebase first and move the code to the right class, but it should encapsulate my changes.
Comment #21
tim.plunkettReroll after #2332389: Finish adding methods to FormStateInterface
Comment #22
catchTagging with D8 upgrade path, see issue summary of #2341575: [meta] Provide a beta to beta/rc upgrade path for why.
Comment #23
penyaskitoRerolled (automerged).
Comment #25
penyaskitoFixed the errors after
drupal_page_is_cacheable()
is gone (#2263981: Introduce a robust and extensible page cache-policy framework) and #2310255: [meta] Remove ArrayAccess from FormState is in.Comment #26
larowlanusers » user ?
> 80
This doesn't appear to be the case anymore?
Can't we inject this service?
We should be injecting the request stack service here
This really sounds like we need a value object.
Returning an array of objects where the order is important is bad form in my opinion.
form_get_cache is deprecated, can we reference it's replacement instead
Missing public scope modifier
If this extends from FormBase you should have a method to get this without calling out to the static container (I think)
Comment #27
penyaskitoCovered the easy stuff from #26: 1, 2, 3, 8, 9.
For 7 I'm not sure, as we are still calling the deprecated method here, and having something else there could be confusing.
Changing the deprecated calls is out of scope here IMHO.
I will work on 4,5,6 next. Suggestions for naming the object at 6 are appreciated.
Comment #28
penyaskitotestbot anyway.
Comment #29
penyaskitoDone with service injection. Only #26.6 is pending.
Comment #31
penyaskitoOops, typo.
Comment #33
penyaskitoFixed unit tests. Misuse of requestStack as request object solved.
Comment #34
penyaskitoCreated AjaxForm class (couldn't think of a better name) for wrapping the list object. Needs phpdocs for the new class and the function wrapping those, some cleanup and such, but let's look what the testbot has to say.
Comment #35
penyaskitoCool, green! Needs work still.
Comment #36
penyaskitoFixed phpdocs. Ready for reviews :-D
Comment #37
penyaskitoSorry, right interdiff.
Comment #38
penyaskitoCalm down. New patch, new interdiff, since #34.
Comment #39
penyaskitoI messed up the last interdiff too. Look at the interdiff of #37 or the full patch at #38.
Comment #40
larowlanLooks great, just nitpicks that I can see
nitpick: whitespace
> 80
form_get_cache() is deprecated or gone?
Comment #41
larowlandawehner asked if we could directly document that this is used to replace a multi-return-value tuple
Comment #42
dawehnerI like the idea to make things a little bit more clear, though I doubt that this class is really that reusable as the name suggests. Should we instead move it into system and prefix it with File or something else?
What do you think?
Comment #43
penyaskitoWrapped comments to 80 chars and fixed nitpick at #40.
#40.3: Only changed one occurrence, as it is talking about D6. See context:
Comment #44
penyaskitoRenamed AjaxForm to FileAjaxForm (not happy with the name neither). Moved it to Drupal\system. Specified in phpdoc that it is avoiding a multi-return-value tuple.
Comment #45
larowlanif re-rolling please reword to meet 80 char requirements - eg %s/avoiding/avoids
other than that, I think this is ready - thanks @penyaskito
Comment #46
penyaskitoI would like to figure out who worked on s.d.o for giving proper attribution. There are some in the IS, but not sure if all.
Comment #47
penyaskitoOh, @larowlan already did that in the IS, thanks!
Comment #49
alexpottCommitted 3fb2ec3 and pushed to 8.0.x. Thanks!
Minor fixes on commit
Comment #51
catch#2421503: SA-CORE-2014-002 forward port only checks internal cache.