[xwiki-devs] Questions about fixing getRenderedContent() bug
Hi, Im trying to fix http://jira.xwiki.org/jira/browse/XWIKI-4274 Basically if you do $xwiki.getDocument("someDoc").getRenderedContent() it'll get executed in the context of the current doc which I believe is wrong especially since other signatures of getRenderedContent() execute in the target document's context. I have fixed this locally but found that admin.vm for example is assuming that getRenderedContent() will get executed in the context of the calling doc (i.e. XWiki.Import when doing an import for example). FYI the chain flow is admin.vm -- getRenderedContent() --> XWiki.AdminSheet --> XWiki.AdminImportSheet --> importinline.vm, which requires the current doc to be XWiki.Import (to get/put attachments from/to it). I can fix this easily using a new getRenderedContent signature I've introduced. However I'm wondering if we have other places that incorrectly use getRenderedContent() and assume it won't be rendered in the context of the target document. Is this change too dangerous to make? If not know, we'll need to it quickly (2.1M1?) since it's an important bug IMO. WDYT? Thanks -Vincent
BTW I think it's wrong that we set the XWiki.Import doc in the XWiki.AdminSheet page. It should be set internally as the current in XWiki.AdminImportSheet. There's no reason AdminSheet should know about it IMO. WDYT? Thanks -Vincent On Sep 4, 2009, at 10:07 AM, Vincent Massol wrote:
Hi,
Im trying to fix http://jira.xwiki.org/jira/browse/XWIKI-4274
Basically if you do $xwiki.getDocument("someDoc").getRenderedContent() it'll get executed in the context of the current doc which I believe is wrong especially since other signatures of getRenderedContent() execute in the target document's context.
I have fixed this locally but found that admin.vm for example is assuming that getRenderedContent() will get executed in the context of the calling doc (i.e. XWiki.Import when doing an import for example). FYI the chain flow is admin.vm -- getRenderedContent() --> XWiki.AdminSheet --> XWiki.AdminImportSheet --> importinline.vm, which requires the current doc to be XWiki.Import (to get/put attachments from/to it). I can fix this easily using a new getRenderedContent signature I've introduced.
However I'm wondering if we have other places that incorrectly use getRenderedContent() and assume it won't be rendered in the context of the target document.
Is this change too dangerous to make? If not know, we'll need to it quickly (2.1M1?) since it's an important bug IMO.
WDYT?
Thanks -Vincent
On Sep 4, 2009, at 10:07 AM, Vincent Massol wrote:
Hi,
Im trying to fix http://jira.xwiki.org/jira/browse/XWIKI-4274
Basically if you do $xwiki.getDocument("someDoc").getRenderedContent() it'll get executed in the context of the current doc which I believe is wrong especially since other signatures of getRenderedContent() execute in the target document's context.
I have fixed this locally but found that admin.vm for example is assuming that getRenderedContent() will get executed in the context of the calling doc (i.e. XWiki.Import when doing an import for example). FYI the chain flow is admin.vm -- getRenderedContent() --> XWiki.AdminSheet --> XWiki.AdminImportSheet --> importinline.vm, which requires the current doc to be XWiki.Import (to get/put attachments from/to it). I can fix this easily using a new getRenderedContent signature I've introduced.
However I'm wondering if we have other places that incorrectly use getRenderedContent() and assume it won't be rendered in the context of the target document.
Is this change too dangerous to make? If not know, we'll need to it quickly (2.1M1?) since it's an important bug IMO.
WDYT?
I think it's safer to wait for 2.1M1. I've attached the patch to the issue and detailed what still needs to be done. Thanks -Vincent
On Fri, Sep 4, 2009 at 10:07, Vincent Massol<vincent@massol.net> wrote:
Hi,
Im trying to fix http://jira.xwiki.org/jira/browse/XWIKI-4274
Basically if you do $xwiki.getDocument("someDoc").getRenderedContent() it'll get executed in the context of the current doc which I believe is wrong especially since other signatures of getRenderedContent() execute in the target document's context.
I have fixed this locally but found that admin.vm for example is assuming that getRenderedContent() will get executed in the context of the calling doc (i.e. XWiki.Import when doing an import for example). FYI the chain flow is admin.vm -- getRenderedContent() --> XWiki.AdminSheet --> XWiki.AdminImportSheet --> importinline.vm, which requires the current doc to be XWiki.Import (to get/put attachments from/to it). I can fix this easily using a new getRenderedContent signature I've introduced.
However I'm wondering if we have other places that incorrectly use getRenderedContent() and assume it won't be rendered in the context of the target document.
Is this change too dangerous to make? If not know, we'll need to it quickly (2.1M1?) since it's an important bug IMO.
Yes it's a dangerous fix I think, getRenderedContent() is really not an hidden method so changing its behavior that much even if it's a fix is dangerous anyway.
WDYT?
Thanks -Vincent _______________________________________________ devs mailing list devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs
-- Thomas Mortagne
On Sep 4, 2009, at 10:30 AM, Thomas Mortagne wrote:
On Fri, Sep 4, 2009 at 10:07, Vincent Massol<vincent@massol.net> wrote:
Hi,
Im trying to fix http://jira.xwiki.org/jira/browse/XWIKI-4274
Basically if you do $xwiki.getDocument("someDoc").getRenderedContent() it'll get executed in the context of the current doc which I believe is wrong especially since other signatures of getRenderedContent() execute in the target document's context.
I have fixed this locally but found that admin.vm for example is assuming that getRenderedContent() will get executed in the context of the calling doc (i.e. XWiki.Import when doing an import for example). FYI the chain flow is admin.vm -- getRenderedContent() --> XWiki.AdminSheet --> XWiki.AdminImportSheet --> importinline.vm, which requires the current doc to be XWiki.Import (to get/put attachments from/to it). I can fix this easily using a new getRenderedContent signature I've introduced.
However I'm wondering if we have other places that incorrectly use getRenderedContent() and assume it won't be rendered in the context of the target document.
Is this change too dangerous to make? If not know, we'll need to it quickly (2.1M1?) since it's an important bug IMO.
Yes it's a dangerous fix I think, getRenderedContent() is really not an hidden method so changing its behavior that much even if it's a fix is dangerous anyway.
But would you be ok to make this change in 2.1M1? Or would you prefer we live with it and instead document that it doesn't execute in the document's contex (and instead add a new method that does)? It's much simpler for sure but keeps adding to the mess. We could decide to keep adding to the mess and fix it when we rewrite that part in some far future. Thanks -Vincent
On Fri, Sep 4, 2009 at 10:39, Vincent Massol<vincent@massol.net> wrote:
On Sep 4, 2009, at 10:30 AM, Thomas Mortagne wrote:
On Fri, Sep 4, 2009 at 10:07, Vincent Massol<vincent@massol.net> wrote:
Hi,
Im trying to fix http://jira.xwiki.org/jira/browse/XWIKI-4274
Basically if you do $xwiki.getDocument("someDoc").getRenderedContent() it'll get executed in the context of the current doc which I believe is wrong especially since other signatures of getRenderedContent() execute in the target document's context.
I have fixed this locally but found that admin.vm for example is assuming that getRenderedContent() will get executed in the context of the calling doc (i.e. XWiki.Import when doing an import for example). FYI the chain flow is admin.vm -- getRenderedContent() --> XWiki.AdminSheet --> XWiki.AdminImportSheet --> importinline.vm, which requires the current doc to be XWiki.Import (to get/put attachments from/to it). I can fix this easily using a new getRenderedContent signature I've introduced.
However I'm wondering if we have other places that incorrectly use getRenderedContent() and assume it won't be rendered in the context of the target document.
Is this change too dangerous to make? If not know, we'll need to it quickly (2.1M1?) since it's an important bug IMO.
Yes it's a dangerous fix I think, getRenderedContent() is really not an hidden method so changing its behavior that much even if it's a fix is dangerous anyway.
But would you be ok to make this change in 2.1M1?
Or would you prefer we live with it and instead document that it doesn't execute in the document's contex (and instead add a new method that does)? It's much simpler for sure but keeps adding to the mess. We could decide to keep adding to the mess and fix it when we rewrite that part in some far future.
Actually i was mainly thinking that we will rewrite all that in the new model anyway so for me one way or another it's temporary code so i think i would be more in favor of not changing the behavior of the current getRenderedContent() and instead add new one for backward compatibility.
Thanks -Vincent
_______________________________________________ devs mailing list devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs
-- Thomas Mortagne
Greetings from the far future! No, the new model is sadly not here yet, but we have a shiny new UI :) Bumping this old thread since I`ve stumbled upon this issue recently and it took me a while to understand what the actual problem is (thanks Thomas!) and I feel it's something we really need to get fixed as soon as possible. Thomas was suggesting we add this on the list for the 7.x Roadmap and I`m obviously +1 for that. There is the matter of backwards compatibility, that we need to still address, as either to break it or keep it. My take would be to break it, since I consider it to be bad code to depend on this broken behavior in the first place and that huge security hole we keep dragging along needs to be plugged abruptly. WDYT? Thanks, Eduard On Fri, Sep 4, 2009 at 11:56 AM, Thomas Mortagne <thomas.mortagne@xwiki.com> wrote:
On Fri, Sep 4, 2009 at 10:39, Vincent Massol<vincent@massol.net> wrote:
On Sep 4, 2009, at 10:30 AM, Thomas Mortagne wrote:
On Fri, Sep 4, 2009 at 10:07, Vincent Massol<vincent@massol.net> wrote:
Hi,
Im trying to fix http://jira.xwiki.org/jira/browse/XWIKI-4274
Basically if you do $xwiki.getDocument("someDoc").getRenderedContent() it'll get executed in the context of the current doc which I believe is wrong especially since other signatures of getRenderedContent() execute in the target document's context.
I have fixed this locally but found that admin.vm for example is assuming that getRenderedContent() will get executed in the context of the calling doc (i.e. XWiki.Import when doing an import for example). FYI the chain flow is admin.vm -- getRenderedContent() --> XWiki.AdminSheet --> XWiki.AdminImportSheet --> importinline.vm, which requires the current doc to be XWiki.Import (to get/put attachments from/to it). I can fix this easily using a new getRenderedContent signature I've introduced.
However I'm wondering if we have other places that incorrectly use getRenderedContent() and assume it won't be rendered in the context of the target document.
Is this change too dangerous to make? If not know, we'll need to it quickly (2.1M1?) since it's an important bug IMO.
Yes it's a dangerous fix I think, getRenderedContent() is really not an hidden method so changing its behavior that much even if it's a fix is dangerous anyway.
But would you be ok to make this change in 2.1M1?
Or would you prefer we live with it and instead document that it doesn't execute in the document's contex (and instead add a new method that does)? It's much simpler for sure but keeps adding to the mess. We could decide to keep adding to the mess and fix it when we rewrite that part in some far future.
Actually i was mainly thinking that we will rewrite all that in the new model anyway so for me one way or another it's temporary code so i think i would be more in favor of not changing the behavior of the current getRenderedContent() and instead add new one for backward compatibility.
Thanks -Vincent
_______________________________________________ devs mailing list devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs
-- Thomas Mortagne _______________________________________________ devs mailing list devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs
On 27 Nov 2014 at 18:46:50, Eduard Moraru (enygma2002@gmail.com(mailto:enygma2002@gmail.com)) wrote:
Greetings from the far future! No, the new model is sadly not here yet, but we have a shiny new UI :)
Bumping this old thread since I`ve stumbled upon this issue recently and it took me a while to understand what the actual problem is (thanks Thomas!) and I feel it's something we really need to get fixed as soon as possible.
Thomas was suggesting we add this on the list for the 7.x Roadmap and I`m obviously +1 for that.
There is the matter of backwards compatibility, that we need to still address, as either to break it or keep it. My take would be to break it, since I consider it to be bad code to depend on this broken behavior in the first place and that huge security hole we keep dragging along needs to be plugged abruptly.
WDYT?
Since this is available from our scripting api I don’t think we should break it, or we can be sure we’re going to break some extensions. An alternative is to move the getRenderedContent() to legacy and implement a new method and use that new method in our code. The only issue with this is the naming of the new method and the closeness with the other getRenderedContent(…) signatures… We could also introduce new methods to replace all the getRenderedContent(…) methods and move them all to legacy. Thanks -Vincent
Thanks, Eduard
On Fri, Sep 4, 2009 at 11:56 AM, Thomas Mortagne wrote:
On Fri, Sep 4, 2009 at 10:39, Vincent Massol wrote:
On Sep 4, 2009, at 10:30 AM, Thomas Mortagne wrote:
On Fri, Sep 4, 2009 at 10:07, Vincent Massol wrote:
Hi,
Im trying to fix http://jira.xwiki.org/jira/browse/XWIKI-4274
Basically if you do $xwiki.getDocument("someDoc").getRenderedContent() it'll get executed in the context of the current doc which I believe is wrong especially since other signatures of getRenderedContent() execute in the target document's context.
I have fixed this locally but found that admin.vm for example is assuming that getRenderedContent() will get executed in the context of the calling doc (i.e. XWiki.Import when doing an import for example). FYI the chain flow is admin.vm -- getRenderedContent() --> XWiki.AdminSheet --> XWiki.AdminImportSheet --> importinline.vm, which requires the current doc to be XWiki.Import (to get/put attachments from/to it). I can fix this easily using a new getRenderedContent signature I've introduced.
However I'm wondering if we have other places that incorrectly use getRenderedContent() and assume it won't be rendered in the context of the target document.
Is this change too dangerous to make? If not know, we'll need to it quickly (2.1M1?) since it's an important bug IMO.
Yes it's a dangerous fix I think, getRenderedContent() is really not an hidden method so changing its behavior that much even if it's a fix is dangerous anyway.
But would you be ok to make this change in 2.1M1?
Or would you prefer we live with it and instead document that it doesn't execute in the document's contex (and instead add a new method that does)? It's much simpler for sure but keeps adding to the mess. We could decide to keep adding to the mess and fix it when we rewrite that part in some far future.
Actually i was mainly thinking that we will rewrite all that in the new model anyway so for me one way or another it's temporary code so i think i would be more in favor of not changing the behavior of the current getRenderedContent() and instead add new one for backward compatibility.
Thanks -Vincent
participants (4)
-
Eduard Moraru -
Thomas Mortagne -
Vincent Massol -
vincent＠massol.net