feat: add an option to allow review app creation from forks #882

yohann-bacha · 2023-02-03T18:27:43Z

contributes to https://github.com/Scalingo/project-items/issues/68
depends on Scalingo/go-scalingo#300

need to do a go mod vendor after Scalingo/go-scalingo#300 before merging this

cmd/integration_link.go

aurelien-reeves-scalingo · 2023-02-10T15:06:13Z

Will be ready once go-scalingo is done: Scalingo/go-scalingo#300

…forks

aurelien-reeves-scalingo

We have to make sure other review-apps related commands puts some emphasis when dealing with forks

cmd/integration_link.go

Signed-off-by: Yohann Bacha <[email protected]>

…ating integration links Signed-off-by: Yohann Bacha <[email protected]>

Signed-off-by: Yohann Bacha <[email protected]>

cmd/integration_link.go

aurelien-reeves-scalingo · 2023-02-23T09:08:22Z

cmd/integration_link.go

+	forksAllowed := false
+	err = survey.AskOne(&survey.Confirm{
+		Message: "Allow review apps to be created from forks:",
+		Help:    "Only allow automatic review apps deployments from forks if you trust the owners of those forks, as this could lead to security issues",
+		Default: forksAllowed,
+	}, &forksAllowed, nil)


I like the use of io.Warning rather than the help mesage to make sure the warning is always seen. We could do the same here.

Also we could then use the same message defined in the constant reviewAppsFromForksSecurityWarning (warningSecurityMessageForAutomaticReviewAppsFromForks)

Nice idea, it has been done

it has been done

I don't see the change here. Am I missing something?

Oh I got messed up, fixing it

aurelien-reeves-scalingo · 2023-02-23T09:13:23Z

cmd/integration_link.go

+	if forksAllowed {
+		io.Info("To bypass this security warning next time, you can provide the flag --aware-of-security-risks")
+	}


I would not encourage in any way to bypass a security notice. User could find that option on their own easily as it is properly documented

Suggested change

if forksAllowed {

io.Info("To bypass this security warning next time, you can provide the flag --aware-of-security-risks")

}

Well, I think if the user has already told he's been aware of the security risks, we could give him the way to bypass it, because he's already aware. IMHO it would be a better experience

I would be in favor of security over UX in such case

Also, the option to bypass the security warning is there mainly to prevent interactive mode in non-tty environment. Not to improve UX.

Well if you insist, I'll go with it :)

cmd/integration_link.go

Co-authored-by: aurelien-reeves-scalingo <[email protected]>

aurelien-reeves-scalingo · 2023-02-23T10:13:33Z

cmd/integration_link.go

@@ -22,6 +22,8 @@ import (
 )

 var (
+	reviewAppsFromForksSecurityWarning = "Only allow automatic review apps deployments from forks if you trust the owners of those forks, as this could lead to security issues"


Sorry, one final request for changes: addition of a link to the documentation as suggested on slack

Suggested change

reviewAppsFromForksSecurityWarning = "Only allow automatic review apps deployments from forks if you trust the owners of those forks, as this could lead to security issues"

reviewAppsFromForksSecurityWarning = "Only allow automatic review apps deployments from forks if you trust the owners of those forks, as this could lead to security issues. More info here: https://doc.scalingo.com/platform/app/review-apps#addons-collaborators-and-environment-variables"

aurelien-reeves-scalingo · 2023-02-23T10:18:47Z

cmd/integration_link.go

+	forksAllowed := false
+	err = survey.AskOne(&survey.Confirm{
+		Message: "Allow review apps to be created from forks:",
+		Help:    "Only allow automatic review apps deployments from forks if you trust the owners of those forks, as this could lead to security issues",
+		Default: forksAllowed,
+	}, &forksAllowed, nil)


it has been done

I don't see the change here. Am I missing something?

aurelien-reeves-scalingo · 2023-02-23T10:22:35Z

cmd/integration_link.go

+		Default: forksAllowed,
+	}, &forksAllowed, nil)
+	if err != nil {
+		return params, err


I think we try to always wrap errors into our own, using errgo.Notef for example (errgo when our custom error library is not available).

But I see here that there are other places like you did. I don't know if that was actually on purpose or not

Well, in order to be consistent, I made this logic on all the integration_link.go file

…rity concerns as a warning Signed-off-by: Yohann Bacha <[email protected]>

aurelien-reeves-scalingo · 2023-02-23T10:35:45Z

cmd/integration_link.go

@@ -447,6 +492,19 @@ func interactiveCreate() (scalingo.SCMRepoLinkCreateParams, error) {
 		hoursBeforeDestroyOnStale := uint(hoursBeforeDestroyOnStale64)
 		params.HoursBeforeDeleteStale = &hoursBeforeDestroyOnStale
 	}
+
+	io.Warning("Only allow automatic review apps deployments from forks if you trust the owners of those forks, as this could lead to security issues")


Suggested change

io.Warning("Only allow automatic review apps deployments from forks if you trust the owners of those forks, as this could lead to security issues")

io.Warning(reviewAppsFromForksSecurityWarning)

Signed-off-by: Yohann Bacha <[email protected]>

…ink commands Signed-off-by: Yohann Bacha <[email protected]>

Signed-off-by: Yohann Bacha <[email protected]>

aurelien-reeves-scalingo

Looks good.

I'll approve after a short qualification session

aurelien-reeves-scalingo · 2023-02-24T08:49:27Z

cmd/integration_link.go

+				if err != nil {
+					errorQuit(err)
+				}
+				err = c.Set("allow-review-apps-from-forks", strconv.FormatBool(stillAllowed))


It appears we actually allow review apps from forks even if the user answered "No".

This is because CheckAndFillParams only checks if allow-review-apps-from-forks is set, not its actual value.

I am still facing the exact same issue

Did you submit a fix for that one?

To reproduce:

$ ./scalingo-cli -a test-app integration-link Application: test-app (63691985884fef00111d2a6b) Integration: GitHub (22a2daa4-b636-41c7-8b1b-9cd10e549fa5) Linker: aurelien-reeves-scalingo Repository: aurelien-reeves-scalingo/review-apps-securisation Auto Deploy: ✘ Review Apps Deploy: ✔ Destroy on Close: instantly Destroy on Stale: ✘ Automatic creation from forks: ✘ $ ./scalingo-cli -a test-app integration-link-update --allow-review-apps-from-forks /!\ Only allow automatic review apps deployments from forks if you trust the owners of those forks, as this could lead to security issues. More info here: https://doc.scalingo.com/platform/app/review-apps#addons-collaborators-and-environment-variables ? Are your sure? No -----> Your app 'test-app' integration link has been updated. $ ./scalingo-cli -a test-app integration-link Application: test-app (63691985884fef00111d2a6b) Integration: GitHub (22a2daa4-b636-41c7-8b1b-9cd10e549fa5) Linker: aurelien-reeves-scalingo Repository: aurelien-reeves-scalingo/review-apps-securisation Auto Deploy: ✘ Review Apps Deploy: ✔ Destroy on Close: instantly Destroy on Stale: ✘ Automatic creation from forks: ✔

We can see that the answer "No" to the update request did not prevent the update to be done

aurelien-reeves-scalingo

The "are you sure?" message still feels weird. We may find a way to improve it.

At the moment it looks like that:

$ scalingo -a test-app integration-link-create --allow-review-apps-from-forks https://github.com/aurelien-reeves-scalingo/review-apps-securisation
  /!\  Only allow automatic review apps deployments from forks if you trust the owners of those forks, as this could lead to security issues. More info here: https://doc.scalingo.com/platform/app/review-apps#addons-collaborators-and-environment-variables
? Are your sure? (y/N)

We may find a way to make it look more like that:

$ scalingo -a test-app integration-link-create --allow-review-apps-from-forks https://github.com/aurelien-reeves-scalingo/review-apps-securisation

  /!\  Only allow automatic review apps deployments from forks if you trust the owners of those forks, as this could lead to security issues. More info here: https://doc.scalingo.com/platform/app/review-apps#addons-collaborators-and-environment-variables

? Allow review apps from forks: (y/N)

cmd/integration_link.go

Signed-off-by: Yohann Bacha <[email protected]>

…areness Signed-off-by: Yohann Bacha <[email protected]>

aurelien-reeves-scalingo

I am still facing the issue when I am answering "No" to the confirmation, but the update is still done.

Also, the changes requested here seems to still be missing

aurelien-reeves-scalingo · 2023-02-27T16:18:05Z

cmd/integration_link.go

+				if err != nil {
+					errorQuit(err)
+				}
+				err = c.Set("allow-review-apps-from-forks", strconv.FormatBool(stillAllowed))


I am still facing the exact same issue

Did you submit a fix for that one?

To reproduce:

$ ./scalingo-cli -a test-app integration-link Application: test-app (63691985884fef00111d2a6b) Integration: GitHub (22a2daa4-b636-41c7-8b1b-9cd10e549fa5) Linker: aurelien-reeves-scalingo Repository: aurelien-reeves-scalingo/review-apps-securisation Auto Deploy: ✘ Review Apps Deploy: ✔ Destroy on Close: instantly Destroy on Stale: ✘ Automatic creation from forks: ✘ $ ./scalingo-cli -a test-app integration-link-update --allow-review-apps-from-forks /!\ Only allow automatic review apps deployments from forks if you trust the owners of those forks, as this could lead to security issues. More info here: https://doc.scalingo.com/platform/app/review-apps#addons-collaborators-and-environment-variables ? Are your sure? No -----> Your app 'test-app' integration link has been updated. $ ./scalingo-cli -a test-app integration-link Application: test-app (63691985884fef00111d2a6b) Integration: GitHub (22a2daa4-b636-41c7-8b1b-9cd10e549fa5) Linker: aurelien-reeves-scalingo Repository: aurelien-reeves-scalingo/review-apps-securisation Auto Deploy: ✘ Review Apps Deploy: ✔ Destroy on Close: instantly Destroy on Stale: ✘ Automatic creation from forks: ✔

We can see that the answer "No" to the update request did not prevent the update to be done

cmd/integration_link.go

Signed-off-by: Yohann Bacha <[email protected]>

Soulou · 2023-03-02T10:18:56Z

@aurelien-reeves-scalingo can you merge and release this week?

aurelien-reeves-scalingo · 2023-03-02T10:31:39Z

@aurelien-reeves-scalingo can you merge and release this week?

I'll release it tomorrow 👌

yohann-bacha self-assigned this Feb 3, 2023

yohann-bacha marked this pull request as draft February 3, 2023 18:27

SCedricThomas mentioned this pull request Feb 7, 2023

feat(release): add changelog entry due to cli release Scalingo/documentation#1938

Merged

aurelien-reeves-scalingo reviewed Feb 10, 2023

View reviewed changes

cmd/integration_link.go Outdated Show resolved Hide resolved

aurelien-reeves-scalingo changed the title ~~feat: added the option to enable or disable review app creation forks~~ feat: added the option to allow review app creation forks Feb 10, 2023

aurelien-reeves-scalingo reviewed Feb 10, 2023

View reviewed changes

cmd/integration_link.go Outdated Show resolved Hide resolved

aurelien-reeves-scalingo changed the title ~~feat: added the option to allow review app creation forks~~ feat: add an option to allow review app creation from forks Feb 15, 2023

yohann-bacha added 3 commits February 21, 2023 10:34

feat: added the option to enable or disable review app creation from …

c6a741d

…forks

fix: changed the option name

f8a2765

Rename the flag for setting the parameter

678c261

yohann-bacha force-pushed the feat/review-apps/add-option-to-enable-from-forks branch from b050444 to 678c261 Compare February 21, 2023 09:40

fix: help message for interactive creation

9f67acb

yohann-bacha marked this pull request as ready for review February 21, 2023 09:49

yohann-bacha requested a review from aurelien-reeves-scalingo February 21, 2023 10:34

aurelien-reeves-scalingo reviewed Feb 22, 2023

View reviewed changes

cmd/integration_link.go Show resolved Hide resolved

yohann-bacha added 3 commits February 22, 2023 16:51

feat(review-apps): security warning

7b9f785

Signed-off-by: Yohann Bacha <[email protected]>

feat(review-apps): interactive security warning when creating and upd…

0bd2178

…ating integration links Signed-off-by: Yohann Bacha <[email protected]>

fix(review-apps): linting issues

c67487b

Signed-off-by: Yohann Bacha <[email protected]>

yohann-bacha requested a review from aurelien-reeves-scalingo February 22, 2023 17:22

aurelien-reeves-scalingo requested changes Feb 23, 2023

View reviewed changes

yohann-bacha and others added 2 commits February 23, 2023 10:45

fix: apply suggestions from code review

2e3cf1d

Co-authored-by: aurelien-reeves-scalingo <[email protected]>

fix: applied reviews

bfbf686

yohann-bacha requested a review from aurelien-reeves-scalingo February 23, 2023 10:10

aurelien-reeves-scalingo reviewed Feb 23, 2023

View reviewed changes

fix(review-apps): when interactive creating, display review apps secu…

66972e0

…rity concerns as a warning Signed-off-by: Yohann Bacha <[email protected]>

yohann-bacha requested a review from aurelien-reeves-scalingo February 23, 2023 10:30

aurelien-reeves-scalingo reviewed Feb 23, 2023

View reviewed changes

yohann-bacha added 2 commits February 23, 2023 14:46

feat(review-apps): giving some context to errors

fc566d7

Signed-off-by: Yohann Bacha <[email protected]>

feat(review-apps): errors have even more context in the integration-l…

763a3e4

…ink commands Signed-off-by: Yohann Bacha <[email protected]>

feat(review-apps): more information on the warning message

94c4574

Signed-off-by: Yohann Bacha <[email protected]>

yohann-bacha requested a review from aurelien-reeves-scalingo February 23, 2023 14:42

yohann-bacha added 3 commits February 23, 2023 15:59

feat(review-apps): applying pr returns

c0d3f61

Signed-off-by: Yohann Bacha <[email protected]>

fix(review-apps): trying to fix the linter error

8def4b9

Signed-off-by: Yohann Bacha <[email protected]>

fix(review-apps): trying to fix the linter error again

60f78fa

Signed-off-by: Yohann Bacha <[email protected]>

aurelien-reeves-scalingo reviewed Feb 23, 2023

View reviewed changes

aurelien-reeves-scalingo requested changes Feb 24, 2023

View reviewed changes

cmd/integration_link.go Outdated Show resolved Hide resolved

yohann-bacha added 3 commits February 27, 2023 11:50

fix: do not prompt when review apps are not deployed

8bac5fc

feat(review-apps): added changelog entry

6d032af

Signed-off-by: Yohann Bacha <[email protected]>

fix(review-apps): change forks allowed on update depending on user aw…

ee8190d

…areness Signed-off-by: Yohann Bacha <[email protected]>

yohann-bacha requested a review from aurelien-reeves-scalingo February 27, 2023 16:07

aurelien-reeves-scalingo requested changes Feb 27, 2023

View reviewed changes

fix(review-apps): refactoring the prompt message

0878f3d

Signed-off-by: Yohann Bacha <[email protected]>

yohann-bacha requested a review from aurelien-reeves-scalingo February 27, 2023 16:59

Improve warning message and prompt

7d372a7

aurelien-reeves-scalingo approved these changes Feb 28, 2023

View reviewed changes

yohann-bacha merged commit 48813e6 into master Mar 2, 2023

yohann-bacha deleted the feat/review-apps/add-option-to-enable-from-forks branch March 2, 2023 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add an option to allow review app creation from forks #882

feat: add an option to allow review app creation from forks #882

yohann-bacha commented Feb 3, 2023 •

edited

Loading

aurelien-reeves-scalingo commented Feb 10, 2023

aurelien-reeves-scalingo left a comment

aurelien-reeves-scalingo Feb 23, 2023

yohann-bacha Feb 23, 2023

aurelien-reeves-scalingo Feb 23, 2023

yohann-bacha Feb 23, 2023 •

edited

Loading

aurelien-reeves-scalingo Feb 23, 2023

yohann-bacha Feb 23, 2023

aurelien-reeves-scalingo Feb 23, 2023

aurelien-reeves-scalingo Feb 23, 2023

yohann-bacha Feb 23, 2023

aurelien-reeves-scalingo Feb 23, 2023

yohann-bacha Feb 23, 2023

aurelien-reeves-scalingo Feb 23, 2023

aurelien-reeves-scalingo Feb 23, 2023

yohann-bacha Feb 23, 2023

aurelien-reeves-scalingo Feb 23, 2023

yohann-bacha Feb 23, 2023

aurelien-reeves-scalingo left a comment

aurelien-reeves-scalingo Feb 24, 2023 •

edited

Loading

aurelien-reeves-scalingo Feb 27, 2023

aurelien-reeves-scalingo left a comment

aurelien-reeves-scalingo left a comment

aurelien-reeves-scalingo Feb 27, 2023

Soulou commented Mar 2, 2023

aurelien-reeves-scalingo commented Mar 2, 2023

	if forksAllowed {
	io.Info("To bypass this security warning next time, you can provide the flag --aware-of-security-risks")
	}

	reviewAppsFromForksSecurityWarning = "Only allow automatic review apps deployments from forks if you trust the owners of those forks, as this could lead to security issues"
	reviewAppsFromForksSecurityWarning = "Only allow automatic review apps deployments from forks if you trust the owners of those forks, as this could lead to security issues. More info here: https://doc.scalingo.com/platform/app/review-apps#addons-collaborators-and-environment-variables"

	io.Warning("Only allow automatic review apps deployments from forks if you trust the owners of those forks, as this could lead to security issues")
	io.Warning(reviewAppsFromForksSecurityWarning)

feat: add an option to allow review app creation from forks #882

feat: add an option to allow review app creation from forks #882

Conversation

yohann-bacha commented Feb 3, 2023 • edited Loading

aurelien-reeves-scalingo commented Feb 10, 2023

aurelien-reeves-scalingo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yohann-bacha Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aurelien-reeves-scalingo left a comment

Choose a reason for hiding this comment

aurelien-reeves-scalingo Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aurelien-reeves-scalingo left a comment

Choose a reason for hiding this comment

aurelien-reeves-scalingo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Soulou commented Mar 2, 2023

aurelien-reeves-scalingo commented Mar 2, 2023

yohann-bacha commented Feb 3, 2023 •

edited

Loading

yohann-bacha Feb 23, 2023 •

edited

Loading

aurelien-reeves-scalingo Feb 24, 2023 •

edited

Loading