select_autoescape

[MartinRyan]

address bandit b701 jinja2_autoescape_false

[kinow]

Kicking Travis because I think it was a flaky test.

[kinow]

And clicking on the check of Codacy, under Fixed issues, it's possible to confirm it was fixed:

👏 well done! Approving as soon as Travis is done.

[kinow]

Hmmm, kicked it twice, and failed both times with:

Test Summary Report
-------------------
./tests/hold-release/08-hold-family-inexact.t (Wstat: 0 Tests: 2 Failed: 1)
  Failed test:  2

@MartinRyan , checked out your branch, and tried cylc test-battery ./tests/hold-release/08-hold-family-inexact.t, and it failed too. I think it broke something... maybe looking at that test you might be able to adjust the parameters passed to the select_autoescape?

[kinow]

Did a bit more of investigation after chatting with @MartinRyan in the channel. Here's what I did:

• cylc register hold-family tests/hold-release/hold-family/
• Configured my IDE to launch cylc run --debug --verbose --no-detach --set=HOLD_MATCH="'*FF'" hold-family
• Put a break point after the Jinja2 environment is created and is about to be returned (in the file changed in this PR)
• Got the output of the processed suite.

They don't match, and looks like the new code escapes quotes and other characters that were OK before. @MartinRyan I don't know parsec/Cylc configuration well enough, so will have to wait that others chime in to confirm what should be escaped. My guess is that we should not escape these - and perhaps we would need a custom escape function as mentioned in the Jinja2 documentation (doesn't sound fun to write one though).

# file: before the change
[meta]
    title = "hold family test suite"

    description = """One task that selectively holds tasks in the first cycle
point of the suite."""

[cylc]
    UTC mode = True
    [[reference test]]
        live mode suite timeout = PT1M
[scheduling]
    initial cycle point = 20141009T00
    [[dependencies]]
        [[[R1]]]
            graph = holdrelease => foo & stop
        [[[P1D]]]
            graph = """
                foo => bar
            """
[runtime]
    [[holdrelease]]
        script = """
sleep 5
cylc hold "$CYLC_SUITE_NAME" ''*FF'.20141009T0000Z'
sleep 5
"""
    [[STUFF]]
    [[STOP]]
    [[foo,bar]]
        inherit = STUFF
        script = true
    [[stop]]
        inherit = STOP
        script = sleep 5; cylc stop $CYLC_SUITE_NAME

And notice now the cylc hold call in the script value.

# file: after the change in this PR
[meta]
    title = "hold family test suite"

    description = """One task that selectively holds tasks in the first cycle
point of the suite."""

[cylc]
    UTC mode = True
    [[reference test]]
        live mode suite timeout = PT1M
[scheduling]
    initial cycle point = 20141009T00
    [[dependencies]]
        [[[R1]]]
            graph = holdrelease => foo & stop
        [[[P1D]]]
            graph = """
                foo => bar
            """
[runtime]
    [[holdrelease]]
        script = """
sleep 5
cylc hold "$CYLC_SUITE_NAME" ''*FF'.20141009T0000Z'
sleep 5
"""
    [[STUFF]]
    [[STOP]]
    [[foo,bar]]
        inherit = STUFF
        script = true
    [[stop]]
        inherit = STOP
        script = sleep 5; cylc stop $CYLC_SUITE_NAME

[MartinRyan]

actually checking the syntax for other tests I think it should be fine to use --set=HOLD_MATCH='FF'
i.e. 04-release-family-inexact.t uses --set=RELEASE_MATCH='ST'

[hjoliver]

@kinow and @MartinRyan - for the record, you can view Jinja2-processed suite.rc files with cylc view --jinja2. E.g. to get the above output:

cylc view --stdout --set HOLD_MATCH="'*FF'" -j tests/hold-release/hold-family/

[hjoliver]

actually checking the syntax for other tests I think it should be fine to use --set=HOLD_MATCH='FF' i.e. 04-release-family-inexact.t uses --set=RELEASE_MATCH='ST'

@MartinRyan - you are correct that the original inside quotes are unnecessary here. Without them it works fine because no quotes are passed in to Jinja2 (the remaining/outside quotes are "used up" by the shell interpreter on the command line).

However, I think the fix in this PR should be ditched and just replaced by a #nosec to satisfy codacy, because there's no way use of Jinja2 in Cylc can cause an XSS vulnerability - workflow definitions are not web pages. Furthermore, if anyone does need to pass a quoted value in to a Jinja2 variable (which is conceivable, I wouldn't be surprised if it's done quite frequently) then the quotes will end up encoded as meaningless garbage in the suite definition (meaningless to Cylc).

Or have I missed something here?

[TomekTrzeciak]

Or have I missed something here?

You haven't. This has been raised already multiple times in #2908:
#2908 (comment)
#2908 (comment)

[kinow]

One file in conflict preventing the merge. And the change to 08-hold-family-inexact.t can be reversed I think?

[hjoliver]

@MartinRyan - you don't need to remove lib/jinja2/ext.py as the bundled Jinja2 library has now been deleted on master (by merge of #2990 today). So best to revert that change.

I think it is OK to keep the change to 08-hold-family-inexact.t though, as the extra quotes aren't actually needed (and are a bit ugly).

[hjoliver]

(see comment above)

[MartinRyan]

I reverted deletion of ext.py, but it's in conflict as it's deleted on master

[hjoliver]

OK, you took my "revert" request more literally than I expected! I'd do an interactive rebase to remove your original "delete" commit and the "revert" one (so its as if they never happened) and force-push the result back to GitHub.

[kinow]

I reverted deletion of ext.py, but it's in conflict as it's deleted on master

Partly my fault as this file was removed when setup.py was added. @MartinRyan, if you rebase your branch (something like git fetch --all && git rebase upstream/master or so), there should be a few conflicts that you can manually fix.

If it gets messy, just avoid pushing to remote as your can always reset your local branch (i.e. git reset --hard origin/add-autoescape) and maybe myself or somebody else can try to update your branch instead 👍

[kinow]

(@hjoliver beat me ☝️ )

[hjoliver]

(As @kinow suggests, a hard reset is an alternative to an interactive rebase with skipped commits - so long as the commits are both at the head of your branch).

[MartinRyan]

after a hard reset and force push it looks like I still need to delete ext.py

[hjoliver]

after a hard reset and force push it looks like I still need to delete ext.py

No you can just remove your commit that modifies that file (no mods to it are needed, since the file no longer exists on master).

[hjoliver]

I see you've made the good change, and the unnecessary one (tolib/jinja2) in the same commit. In that case, because it's the final commit do a soft git reset HEAD^ and re-commit just the good change ... then force push again.

[MartinRyan]

ext.py was in the original commit of this pull request...

[hjoliver]

ext.py was in the original commit of this pull request...

Ah, sorry, didn't realize you also modified that file in that commit.

In that case, interactive rebase - it let's you edit specific commits before replaying them.

[kinow]

Looks good to me 👍

[kinow]

Hmm, now it has 17 commits. I think you need to rebase or sort out what went wrong here.

[hjoliver]

Looks good now ... waiting on Travis CI

[hjoliver]

Thanks @MartinRyan 👍