Issue #1341, security issues with path expressions

[Edward K. Ream]

#1338 fixed a serious bug that could lead to the unexpected execution of code.  g.computeFileUrl was expanding path expressions of the form {{text}} within arbitrary body text!

I am continuing to explore the implications for Leo's code base. I have just created #1341, which suggests that calls to g.os_path_expandExpression should be strictly limited to the context in which they were originally proposed, namely to compute paths in @<file> nodes.

At present, g.os_path_finalize and g.os_path_finalize_join expand path expressions(!!).  I think this is wrong, and dangerous. I'm pretty sure this code arose as a misguided attempt on my part to support path expressions in @<file> nodes elegantly.

g.os_path_finalize and g.os_path_finalize_join are used throughout Leo. I don't believe it is wise to have these utility functions expand what looks like path expressions! Elegance must take a back seat to safety.

I am alerting you all to this issue because fixing it may cause other problems.  For example, plugins now get the "benefits" of automatic expansion of path expressions.  One option would be to leave plugins unchanged, and wait for complaints from users ;-)

Your comments, please.

Edward

[Edward K. Ream]

On Saturday, September 21, 2019 at 2:04:22 AM UTC-5, Edward K. Ream wrote:

#1341 ..suggests that calls to g.os_path_expandExpression should be strictly limited to the context in which they were originally proposed, namely to compute paths in @<file> nodes.

The culprit is the overly-clever distinction between c.os_path_finalize* and g.os_path_finalize*. The commander methods expand path expressions; the global functions do not.

Leo's contains 27 nodes that call c.os_path_finalize*:

- 7 nodes are directly involved with path computations in leoAtFile.py.
- 5 nodes are legitimately involved in path computations elsewhere.  Some should partially change.

- The remaining 15, including 3 ShadowController methods, probably should call g.os_path_finalize* instead.

Two nodes in plugins call c.os_path_finalize*.  Both these should call g.os_path_finalize* instead.

Proposed fix

- The c.os_path_finalize* methods will be deprecated. They will call g.os_path_finalize*, with a deprecation warning.

- All calls to c.os_path_finalize* should be changed to calls to g.os_path_finalize*, with a #1341 comment.

- 12 nodes, all in Leo's core, should explicitly call g.os_path_expandExpression in addition to g.os_path_finalize*.

Summary

Imo, this is an important change, worth the risks inherent in any significant change to Leo's code base.

This is a variation of the evils of extra kwargs that Vitalije has properly warned against.

Edward

[Matt Wilkie]

#1338 fixed a serious bug that could lead to the unexpected execution of code.  g.computeFileUrl was expanding path expressions of the form {{text}} within arbitrary body text!

Eeep! Good catch. I'm glad you noticed it.

-matt

[Edward K. Ream]

On Saturday, September 21, 2019 at 7:48:11 AM UTC-5, Edward K. Ream wrote:

#1341 ..suggests that calls to g.os_path_expandExpression should be strictly limited to the context in which they were originally proposed, namely to compute paths in @<file> nodes.

This issue is now complete, and has been closed.  #1341 discusses all the changes.  Along the way I made a few more simplifications.

The "path" branch has been merged into devel.  We want to test this code now, as widely as possible.

The @shadow logic needs more testing.  It is difficult to know exactly when path expressions were expanded.  Let me know if you run into problems here.

Edward

[Edward K. Ream]

On Saturday, September 21, 2019 at 4:40:35 PM UTC-5, Edward K. Ream wrote:

The @shadow logic needs more testing. 

I have reopened #1341. There appear to be problems with path expressions in both @shadow and @file.  Clearly, I did not do enough testing.  I'll create new unit tests.

Edward

[Edward K. Ream]

On Sunday, September 22, 2019 at 4:51:19 AM UTC-5, Edward K. Ream wrote:

> I have reopened #1341. There appear to be problems with path expressions in both @shadow and @file.

Fixed at cf755e6 in devel. There were straightforward changes to pyflakes.finalize and at.readOneAtShadowNode.

I have considerably more confidence in the code now. Please report any further problems immediately.

Edward