Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Branch name validation too strict #247

Closed
simonmumenthaler opened this issue Aug 6, 2021 · 18 comments
Closed

Branch name validation too strict #247

simonmumenthaler opened this issue Aug 6, 2021 · 18 comments
Labels
bug Something isn't working effort/small This issue will take less than a day of effort to fix p2

Comments

@simonmumenthaler
Copy link

We name our branches with a specific pattern that starts with a # followed by digits and other chars like #2-bit-ref-r.

But when executing the configure-aws-credentials action it fails since the name does not match the required regex pattern.

1 validation error detected: Value 'refs/heads/#2-bit-ref-r' at 'tags.7.member.value' failed to satisfy constraint: Member must satisfy regular expression pattern: [\p{L}\p{Z}\p{N}_.:/=+\-@]*

Since for github it's a valid branch name it should also be accepted by this action.

@simonmumenthaler simonmumenthaler changed the title Action fails because the validation of the branch name fails Branch name validation too strict Aug 6, 2021
@dejwsz
Copy link

dejwsz commented Aug 10, 2021

I have the same thing today for one of our branches

@dejwsz
Copy link

dejwsz commented Aug 10, 2021

In our case we had such error:
Error: 1 validation error detected: Value 'refs/heads/CFA-94/Image_uploading_prototype_(company_logo)_' at 'tags.7.member.value' failed to satisfy constraint: Member must satisfy regular expression pattern: [\p{L}\p{Z}\p{N}_.:/=+\-@]*

@dejwsz
Copy link

dejwsz commented Aug 10, 2021

After we changed the branch name it went through without any problems.

@paragbhingre paragbhingre self-assigned this Aug 12, 2021
@paragbhingre
Copy link
Contributor

@simonmumenthaler Thank you for reporting this issue with us. I tried reproducing this issue on my end and couldn't see the error that you are getting with the same branch name as yours i.e. #2-bit-ref-r
Could you please provide me some more information regarding this issue such as some screen shots, steps to reproduce, environment etc.

@simonmumenthaler
Copy link
Author

simonmumenthaler commented Aug 13, 2021

@paragbhingre Thank you for investigating this problem.

I receive the error in different repos. The action works when running on master but fails on our #\d+-[a-z\d-]+ branches.

The config I use for the action:

name: Delete Orphan Stacks
on:
  schedule:
    - cron: '15 2 * * 1'
  workflow_dispatch: 
jobs:
  delete-stack:
    runs-on: ubuntu-latest
    steps:
      - name: Configure AWS Credentials for eu-central-1
        uses: aws-actions/configure-aws-credentials@v1
        with:
          aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }}
          aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
          role-to-assume: 'arn:aws:iam::************:role/ScAccess'
          role-duration-seconds: 3600
          aws-region: eu-central-1
      - name: Delete Stacks in eu-central-1
        uses: shiftcode/[email protected]
        with:
          githubToken: ${{ secrets.GH_TOKEN_REPO_READ }}
          stackNamePrefix: 'pai-website'
          ignoreStacks: '["xx1"]'

Output when running on #001-dev:
image

dgholz added a commit to dgholz/configure-aws-credentials that referenced this issue Sep 14, 2021
@dgholz
Copy link

dgholz commented Sep 14, 2021

I hit this, I tried overriding GITHUB_REF to a sanitised value but found out that GitHub throws away updates to the GITHUB_* env vars:

https://docs.github.com/en/[email protected]/actions/reference/environment-variables#naming-conventions-for-environment-variables

When you set a custom environment variable, you cannot use any of the default environment variable names listed above with the prefix GITHUB_. If you attempt to override the value of one of these default environment variables, the assignment is ignored.

Rather than disabling session tagging altogether, I made #259 to apply the same sanitation to the branch name that the workflow name gets.

@paragbhingre
Copy link
Contributor

@dgholz Thank you for your contribution, I was just wondering if changing the name of the branch have any side effects on the workflow? As in if we modify the name of the branch then does it affect/break the workflow at any point as real name of the branch is different than the one we are providing after modifying it?

@dgholz
Copy link

dgholz commented Sep 16, 2021

I think changing the branch name is safe. The modified branch name is only used on the session tag, and that's just a 'nice to know' bit of information when looking at them in the console/API.

dgholz added a commit to dgholz/configure-aws-credentials that referenced this issue Jan 4, 2022
@peterwoodworth peterwoodworth added needs-triage This issue still needs to be triaged bug Something isn't working p2 effort/small This issue will take less than a day of effort to fix in-progress This issue is being actively worked on and removed investigating needs-triage This issue still needs to be triaged labels Sep 30, 2022
@peterwoodworth
Copy link
Contributor

I mentioned this in the PR but I'm having trouble reproducing this issue when running from branches containing the # character. Maybe there are more steps to reproduce this than just running this action on a branch with certain characters?

@peterwoodworth peterwoodworth removed the in-progress This issue is being actively worked on label Oct 5, 2022
@peterwoodworth peterwoodworth added the needs-reproduction This issue needs reproduction. label Oct 7, 2022
@dgholz
Copy link

dgholz commented Oct 18, 2022

You're having trouble reproducing because you're using the OIDC token instead of explicit secret access key/access key ID, and the code specifically throws away the session tags when the OIDC token is used

delete assumeRoleRequest.Tags;

dgholz added a commit to dgholz/configure-aws-credentials that referenced this issue Oct 18, 2022
@peterwoodworth
Copy link
Contributor

Thanks for getting back @dgholz,

I did use access keys! You can see from my comment in the PR that I was testing using access keys because of this OIDC functionality. I can try to test again later this week, is there anything else you think i could have misconfigured based on my workflow file?

@dgholz
Copy link

dgholz commented Oct 19, 2022

I see you passed the access keys, but because of

    permissions:
      id-token: write

they are ignored, and the OIDC token is used instead. Take that out and you will reproduce it.

I know it seem impossible from reading the code, since the GitHub OIDC token should only be used if the access key is not set. But that's the only difference I can see between your workflow and my reproduction https://github.com/dgholz/aws-credentials-repro/actions/runs/3282098461/jobs/5405027659#step:2:12

@peterwoodworth
Copy link
Contributor

Thanks for the suggestion, I tried it out and still couldn't get it to fail - Specifying permissions doesn't necessarily mean the action will try to use OIDC. If an access key is provided, that signals to the action to use access keys as authentication

return roleToAssume && process.env.ACTIONS_ID_TOKEN_REQUEST_TOKEN && !accessKeyId && !webIdentityTokenFile

However, I finally figured out what I was doing wrong. I was trying to use a pull request from the branch to test it, and it turns out that a PR is considered its own branch, at least for this. TIL. I checked CloudTrail and saw this for the session tag:

            {
                "key": "Branch",
                "value": "refs/pull/45/merge"
            }

So, my bad for hesitating this long due to my own error 😅. Are you willing to reopen the PR for this or make a new one @dgholz? I'm not able to reopen it 🙂

@peterwoodworth peterwoodworth removed the needs-reproduction This issue needs reproduction. label Oct 19, 2022
@dgholz
Copy link

dgholz commented Oct 20, 2022

Are you willing to reopen the PR for this or make a new one @dgholz? I'm not able to reopen it

Most certainly. I think I did something dumb and force-pushed the branch (after rebasing), which stopped it from being re-openable. I've reset the branch, so I think you can re-open it now.

@Woodz
Copy link

Woodz commented May 5, 2023

I have just experienced this when trying to call TagLogGroup with cf:version=bugfix/#46-add-region-runner-tags-to-tms-address-book-api. It looks like this validation is enforced within AWS and the documentation at https://docs.aws.amazon.com/tag-editor/latest/userguide/tagging.html#tag-conventions mentions that hash symbol (#) is not supported in tags, which causes problems when the tag is based on the branch name

@jonathanberube-zylo
Copy link

I have also experienced this issue with a git branch name of AM-22&AM-32|alert_tooltip. While the & and | characters are not crucial to our workflow, it would be nice to make sure that the action is compatible with valid git branch names.

@peterwoodworth
Copy link
Contributor

This should be fixed in v3, thanks for the patience everyone

@github-actions
Copy link

** Note **
Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working effort/small This issue will take less than a day of effort to fix p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants