Skip to content

Conversation

ddevsr
Copy link
Collaborator

@ddevsr ddevsr commented Dec 26, 2022

Description
Ref: twbs/bootstrap#36325 PHP-CS-Fixer/PHP-CS-Fixer#6644

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@ddevsr ddevsr added the github_actions Pull requests that update Github_actions code label Dec 26, 2022
Copy link
Contributor

@datamweb datamweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -9,6 +9,8 @@ on:
jobs:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add top-level permissions:.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

permissions: in top-level will cover permission all jobs, And i think no need set permission per job when top-level already write exclude job check-version must set permission: read. Agree?

Copy link
Member

@kenjis kenjis Dec 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think generally we should set top-level permissions: read only.
Top level permissions: is the default, and it can be overridden in jobs.

I don't know the default setting of this organization. So if it is read write permissions, jobs without permission can write. It is not safe to supply chain attacks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can set default permission in organization, open https://github.com/organizations/codeigniter4/settings/actions and find Workflow permissions in bottom

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no permission to access the URL.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or only owner this organization can access this setting

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I have access, though I'm hesitant to make changes until we have the necessary write permissions in place everywhere first.

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default org permission should be read-only. Adding write permissions should be done on a per-workflow or per-job basis only.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, said @paulbalandan, and we must not forget that one day if we want to change permissions at the organizational level, we must include permission read at the top-level.

Btw, i am @ddevsr. Waiting review recovery account.

@kenjis
Copy link
Member

kenjis commented Dec 27, 2022

SCSS error was fixed. Please rebase when you update.

@MGatner
Copy link
Member

MGatner commented Dec 27, 2022

Thanks for spearheading this! I will have to read more in depth on the individual permissions and scopes. Please address @kenjis' questions in the meantime.

@ddevsr ddevsr requested a review from kenjis December 28, 2022 01:48
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need all the "contents: read" directives once the org change is made?

@kenjis
Copy link
Member

kenjis commented Dec 30, 2022

Do we still need all the "contents: read" directives once the org change is made?

Yes. If we would change the org setting accidentally or by any reason, all GA flows will have write permissions.

@kenjis kenjis merged commit ee15d88 into codeigniter4:develop Dec 31, 2022
@ddevsr ddevsr deleted the workflow-secure branch January 1, 2023 03:52
@MGatner
Copy link
Member

MGatner commented Jan 2, 2023

Organization repository workflow permissions have been set to "read". Please keep an eye on these workflow runs over the next few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update Github_actions code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants