feat: disable unsafe features by default and add `--UNSAFE` switch #1171

sisp · 2023-06-01T14:42:09Z

I've disabled the use of unsafe features (Jinja extensions, migrations, and tasks) by default and added a new CLI switch --UNSAFE which enables them. Templates that don't use unsafe features are unaffected by this change. But Copier will raise an error for templates that do use unsafe features unless the --UNSAFE flag is passed.

I've not added an interactive prompt that asks for consent for using unsafe features because I think it's not clear how to distinguish between interactive prompting and raising an error when --UNSAFE is not passed. For this, I think Copier would need a switch that clearly states whether interactive or non-interactive mode is desired. Currently, --defaults implies this for questions.

We could add an interactive prompt in a follow-up PR ones the path forward is clear. I'd also add a dedicated page to the docs regarding Copier's security architecture and threat model in a separate PR.

Resolves partially #1137.

codecov · 2023-06-01T14:53:15Z

Codecov Report

Merging #1171 (2337575) into master (ff5aa05) will decrease coverage by 0.14%.
The diff coverage is 100.00%.

❗ Current head 2337575 differs from pull request most recent head 0c3e859. Consider uploading reports for the commit 0c3e859 to get more accurate results

@@            Coverage Diff             @@
##           master    #1171      +/-   ##
==========================================
- Coverage   96.87%   96.73%   -0.14%     
==========================================
  Files          46       47       +1     
  Lines        3809     3925     +116     
==========================================
+ Hits         3690     3797     +107     
- Misses        119      128       +9

Flag	Coverage Δ
unittests	`96.73% <100.00%> (-0.14%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tests/test_answersfile_templating.py	`100.00% <ø> (ø)`
tests/test_copy.py	`100.00% <ø> (ø)`
copier/cli.py	`100.00% <100.00%> (ø)`
copier/errors.py	`95.55% <100.00%> (+0.55%)`	⬆️
copier/main.py	`95.64% <100.00%> (+0.26%)`	⬆️
tests/test_cleanup.py	`100.00% <100.00%> (ø)`
tests/test_demo_update_tasks.py	`100.00% <100.00%> (ø)`
tests/test_jinja2_extensions.py	`100.00% <100.00%> (ø)`
tests/test_migrations.py	`100.00% <100.00%> (ø)`
tests/test_tasks.py	`100.00% <100.00%> (ø)`
... and 3 more

... and 1 file with indirect coverage changes

yajo

Neat! Thanks, looks very good. Actually it looks so good that I'd gladly forget the idea of interactive prompting for unsafety. You're right in that it could be confused with the usual questionary.

Some questions below, though.

BTW would you mind amending the commit and adding a BREAKING CHANGE: trailer so it helps the autogeneration of changelog when I launch the next release? See the example.

tests/test_unsafe.py

BREAKING CHANGE: Copier raises an error when a template uses unsafe features unless the `--UNSAFE` switch is passed

sisp · 2023-06-01T17:37:18Z

BTW would you mind amending the commit and adding a BREAKING CHANGE: trailer so it helps the autogeneration of changelog when I launch the next release? See the example.

Sure, no problem. 🙂 I've amended the commit and added a BREAKING CHANGE: trailer.

yajo

Please just one little extra request.

Besides, please mind the commits. There's one fix: support python 3.7. The fix commits appear in the changelog, and that wouldn't make sense because python 3.7 was already supported before this change.

Well, if you prefer, just forget about commits and I'll just squash on merge, rewording it. I wanted to tell just in case you were wondering why I usually squash PRs... 😆 I know you're the kind of person that likes to understand even these kind of details. 😉

yajo · 2023-06-01T17:42:25Z

docs/configuring.md

+Therefore, these features are disabled by default and Copier will raise an error when
+they are found in a template. In this case, please verify that no malicious code gets
+executed by any of the used features. When you're sufficiently confident or willing to
+take the risk, set `unsafe=True` or pass the CLI switch `--UNSAFE`.


I think it would be useful to explain that these kind of failures give an exit code 2 on CLI.

Like this? 0c3e859

sisp · 2023-06-01T18:01:42Z

Besides, please mind the commits. There's one fix: support python 3.7. The fix commits appear in the changelog, and that wouldn't make sense because python 3.7 was already supported before this change.

Well, if you prefer, just forget about commits and I'll just squash on merge, rewording it. I wanted to tell just in case you were wondering why I usually squash PRs... laughing I know you're the kind of person that likes to understand even these kind of details. wink

Haha, yes, indeed I do. But my logic has been this: The first commit should have a proper change type (here, feat:). All subsequent commits in this PR will get change types according to their change relative to the first commit in the PR. So because I messed up Python 3.7 support in this PR, I fixed it. I always assume such PRs to be squashed, so the first commit message (or rather PR title) is used for the final commit message in the master branch. Is this wrong? 🤔

yajo · 2023-06-01T18:12:42Z

No, it's not wrong, don't worry!

It's just simpler for me to hit "Rebase and merge" than to rewrite the squash message.

But it's really the most irrelevant thing, so don't worry about that. Thanks for the PR!

sisp · 2023-06-01T18:48:36Z

I'm still curious to understand the change type thing better. How would would you do it?

Amend the first commit and force-push?
Always use the change type of the first commit for any subsequent commit in the PR independent of its actual change type?
Something else?

🤔

yajo · 2023-06-02T14:33:19Z

For me, the rule is: does the commit type appear in the release notes and is it meaningful there? Then add a commit. Otherwise, squash. El jue, 1 jun 2023, 19:48, Sigurd Spieckermann ***@***.***> escribió:

…

I'm still curious to understand the change type ting better. How would would you do it? 1. Amend the first commit and force-push? 2. Always use the change type of the first commit for any subsequent commit in the PR independent of its actual change type? 3. Something else? 🤔 — Reply to this email directly, view it on GitHub <#1171 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHNXDP35VVQKQZSUFEN3ILXJDPY5ANCNFSM6AAAAAAYXBCYVE> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

sisp · 2023-06-02T14:43:09Z

It's still not clear to me ... 😕 What would you have done, e.g., for the fix: support Python 3.7 commit? Would you have amended the first commit? Or would you have used a different change type (feat:)?

yajo · 2023-06-02T15:32:59Z

Does the commit type (fix) appear in the changelog? Yes. Does it make sense there? No, because python 3.7 was already supported before. Then, I'd squash it. El vie, 2 jun 2023, 15:43, Sigurd Spieckermann ***@***.***> escribió:

…

It's still not clear to me ... 😕 What would you have done, e.g., for the fix: support Python 3.7 <19fe0c8> commit? Would you have amended the first commit? Or would you have used a different change type (feat:)? — Reply to this email directly, view it on GitHub <#1171 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHNXDI6OXIGGZQR4HNIPMLXJH3YRANCNFSM6AAAAAAYXBCYVE> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

sisp · 2023-06-02T16:03:38Z

Sorry to be so persistent. With squashing, there's no problem then. But it seems you'd prefer no squashing. Then, the only alternative is to amend the first commit after a review that requests changes. Somehow I don't think that's a good workflow because it's less clear how a PR has evolved until it got merged. And committing review suggestions via the GitHub UI also creates new commits. I think squash-merging is the way to go for almost all PRs, and then subsequent commits don't contribute to the changelog no matter what their change types are.

yajo · 2023-06-03T05:40:59Z

Haha yes indeed. I do that most of the time. In the case of this PR, it made sense to have both feat commits in the changelog, but the fix commit didn't. In any case, this really isn't an extremely important problem. Don't worry about it too much and contribute the way it's easiest for you. 😊 El vie., 2 jun. 2023 17:03, Sigurd Spieckermann ***@***.***> escribió:

…

Sorry to be so persistent. With squashing, there's no problem then. But it seems you'd prefer no squashing. Then, the only alternative is to amend the first commit after a review that requests changes. Somehow I don't think that's a good workflow because it's less clear how a PR has evolved until it got merged. And committing review suggestions via the GitHub UI also creates new commits. I think squash-merging is the way to go for almost all PRs, and then subsequent commits don't contribute to the changelog no matter what their change types are. — Reply to this email directly, view it on GitHub <#1171 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHNXDLCKBRYVCRYIL3PYLTXJIFGJANCNFSM6AAAAAAYXBCYVE> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

pawamoy · 2023-06-04T16:48:46Z

Personally I use a fixup commit instead of amending: git commit --fixup SHA_OF_COMMIT_TO_FIX.
This way we still have the commit history in the branch / PR, the GitHub notifications do not lead to 404 pages, and I squash the commits when merging. It also makes it very easy to squash them locally, if needed: git rebase -i --autosquash master (or something like HEAD~N instead of master, where N is a number of commits).

otherwise the following message is displayed: > Template uses unsafe feature: tasks Introduced in copier v8.0.0 (copier-org/copier#1171)

yajo linked an issue Jun 1, 2023 that may be closed by this pull request

Reduce template dangers #1137

Closed

yajo requested changes Jun 1, 2023

View reviewed changes

tests/test_unsafe.py Show resolved Hide resolved

tests/test_unsafe.py Outdated Show resolved Hide resolved

sisp added 3 commits June 1, 2023 19:33

feat: disable unsafe features by default and add --UNSAFE switch

09dec88

BREAKING CHANGE: Copier raises an error when a template uses unsafe features unless the `--UNSAFE` switch is passed

fix: support Python 3.7

19fe0c8

feat: use special return code for disallowing unsafe template features

2337575

sisp force-pushed the feat/unsafe branch from 7854c58 to 2337575 Compare June 1, 2023 17:35

yajo approved these changes Jun 1, 2023

View reviewed changes

yajo requested changes Jun 1, 2023

View reviewed changes

docs: mention exit code when unsafe features are detected

0c3e859

sisp changed the title ~~feat: disable unsafe features by default and --UNSAFE switch~~ feat: disable unsafe features by default and add --UNSAFE switch Jun 1, 2023

yajo enabled auto-merge (squash) June 1, 2023 18:14

yajo merged commit 83f44cb into copier-org:master Jun 1, 2023

sisp deleted the feat/unsafe branch June 1, 2023 18:46

SirTakobi added a commit to SirTakobi/oca-addons-repo-template that referenced this pull request Jul 11, 2023

Allow unsafe tasks

1bdcf68

otherwise the following message is displayed: > Template uses unsafe feature: tasks Introduced in copier v8.0.0 (copier-org/copier#1171)

SirTakobi mentioned this pull request Jul 11, 2023

Allow unsafe tasks OCA/oca-addons-repo-template#202

Merged

yajo pushed a commit to OCA/oca-addons-repo-template that referenced this pull request Sep 6, 2023

docs: remind to allow unsafe tasks (#202)

942ca52

otherwise the following message is displayed: > Template uses unsafe feature: tasks Introduced in copier v8.0.0 (copier-org/copier#1171)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: disable unsafe features by default and add `--UNSAFE` switch #1171

feat: disable unsafe features by default and add `--UNSAFE` switch #1171

sisp commented Jun 1, 2023 •

edited

Loading

codecov bot commented Jun 1, 2023 •

edited

Loading

yajo left a comment

sisp commented Jun 1, 2023

yajo left a comment

yajo Jun 1, 2023

sisp Jun 1, 2023

sisp commented Jun 1, 2023

yajo commented Jun 1, 2023

sisp commented Jun 1, 2023 •

edited

Loading

yajo commented Jun 2, 2023 via email

sisp commented Jun 2, 2023

yajo commented Jun 2, 2023 via email

sisp commented Jun 2, 2023

yajo commented Jun 3, 2023 via email

pawamoy commented Jun 4, 2023 •

edited

Loading

feat: disable unsafe features by default and add --UNSAFE switch #1171

feat: disable unsafe features by default and add --UNSAFE switch #1171

Conversation

sisp commented Jun 1, 2023 • edited Loading

codecov bot commented Jun 1, 2023 • edited Loading

Codecov Report

yajo left a comment

Choose a reason for hiding this comment

sisp commented Jun 1, 2023

yajo left a comment

Choose a reason for hiding this comment

yajo Jun 1, 2023

Choose a reason for hiding this comment

sisp Jun 1, 2023

Choose a reason for hiding this comment

sisp commented Jun 1, 2023

yajo commented Jun 1, 2023

sisp commented Jun 1, 2023 • edited Loading

yajo commented Jun 2, 2023 via email

sisp commented Jun 2, 2023

yajo commented Jun 2, 2023 via email

sisp commented Jun 2, 2023

yajo commented Jun 3, 2023 via email

pawamoy commented Jun 4, 2023 • edited Loading

feat: disable unsafe features by default and add `--UNSAFE` switch #1171

feat: disable unsafe features by default and add `--UNSAFE` switch #1171

sisp commented Jun 1, 2023 •

edited

Loading

codecov bot commented Jun 1, 2023 •

edited

Loading

sisp commented Jun 1, 2023 •

edited

Loading

pawamoy commented Jun 4, 2023 •

edited

Loading