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.

Sign up for GitHub

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

Jump to bottom

Respect original content when creating secrets #24745

Merged
merged 5 commits into from
May 16, 2023
Merged

Respect original content when creating secrets #24745

merged 5 commits into from
May 16, 2023

Conversation

wolfogre
Copy link
Member

@wolfogre wolfogre commented May 16, 2023 •
edited
Loading

Fix #24721.

Follow what GitHub does:

  • Don't trim spaces for secrets.
  • Newline should be \n instead of \r\n.

Did some tests with:

name: secrets
on: push
jobs:
  show_secrets:
    runs-on: ubuntu-latest
    steps:
      - name: Dump secrets context
        run: echo '${{ toJSON(secrets) }}' | base64

AAAAAA:

   AAAAAA
AAAAAA

BBBBBB:




BBBBBB
BBBBBB

On GitHub:

image

On Gitea (before):

image

On Gitea (after):

image

@wolfogre wolfogre added type/bug topic/gitea-actions related to the actions of Gitea outdated/backport/v1.19 This PR should be backported to Gitea 1.19 labels May 16, 2023
@wolfogre wolfogre added this to the 1.20.0 milestone May 16, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 16, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 16, 2023
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 16, 2023
@wolfogre
Copy link
Member Author

wolfogre commented May 16, 2023 •
edited
Loading

Need help:

The newline in textarea is \r\n, how to change it to \n (like GitHub)?

image image

Update:

Since it's a standard behavior of HTML, I'll repleace \r\n at backend.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 16, 2023 •
edited
Loading

The newline in textarea is \r\n, how to change it to \n?

There is a standard.

https://html.spec.whatwg.org/multipage/forms.html#the-textarea-element

For historical reasons, the element's value is normalized in three different ways for three different purposes.
The raw value is the value as it was originally set. It is not normalized.
The API value ... It is normalized so that line breaks use U+000A LINE FEED (LF) characters.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 16, 2023 •
edited
Loading

And this:

https://www.w3.org/TR/REC-html40/interact/forms.html#h-17.13.3.2

Control names and values are escaped. Space characters are replaced by '+', and then reserved characters are escaped as described in [RFC1738], section 2.2: Non-alphanumeric characters are replaced by '%HH', a percent sign and two hexadecimal digits representing the ASCII code of the character. Line breaks are represented as "CR LF" pairs (i.e., '%0D%0A').

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 16, 2023
@wolfogre wolfogre marked this pull request as ready for review May 16, 2023 04:11
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 16, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 16, 2023
@lunny lunny merged commit d81659d into go-gitea:main May 16, 2023
@GiteaBot GiteaBot mentioned this pull request May 16, 2023
Merged
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request May 16, 2023
@wolfogre @GiteaBot
Respect original content when creating secrets (go-gitea#24745)
9974fe8 
Fix go-gitea#24721.

Follow what GitHub does:
- Don't trim spaces for secrets.
- Newline should be `\n` instead of `\r\n`.

Did some tests with:

```yaml
name: secrets
on: push
jobs:
  show_secrets:
    runs-on: ubuntu-latest
    steps:
      - name: Dump secrets context
        run: echo '${{ toJSON(secrets) }}' | base64
```

`AAAAAA`:
```text
   AAAAAA
AAAAAA


```
`BBBBBB`:
```text



BBBBBB
BBBBBB   
```


On GitHub:

<img width="675" alt="image"
src="https://github.com/go-gitea/gitea/assets/9418365/0ec60652-c2a3-47bb-9f9d-7e81665355a8">


On Gitea (before):

<img width="673" alt="image"
src="https://github.com/go-gitea/gitea/assets/9418365/cce818bf-5edc-4656-86e1-2c81c304cdb2">

On Gitea (after):

<img width="673" alt="image"
src="https://github.com/go-gitea/gitea/assets/9418365/0b3b15af-4d48-4bab-a334-4738a1b0eb4a">
@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels May 16, 2023
silverwind pushed a commit that referenced this pull request May 16, 2023
@GiteaBot @wolfogre
Respect original content when creating secrets (#24745) (#24746)
e4f200e 
Backport #24745 by @wolfogre

Fix #24721.

Follow what GitHub does:
- Don't trim spaces for secrets.
- Newline should be `\n` instead of `\r\n`.

Did some tests with:

```yaml
name: secrets
on: push
jobs:
  show_secrets:
    runs-on: ubuntu-latest
    steps:
      - name: Dump secrets context
        run: echo '${{ toJSON(secrets) }}' | base64
```

`AAAAAA`:
```text
   AAAAAA
AAAAAA


```
`BBBBBB`:
```text



BBBBBB
BBBBBB   
```


On GitHub:

<img width="675" alt="image"
src="https://github.com/go-gitea/gitea/assets/9418365/0ec60652-c2a3-47bb-9f9d-7e81665355a8">


On Gitea (before):

<img width="673" alt="image"
src="https://github.com/go-gitea/gitea/assets/9418365/cce818bf-5edc-4656-86e1-2c81c304cdb2">

On Gitea (after):

<img width="673" alt="image"
src="https://github.com/go-gitea/gitea/assets/9418365/0b3b15af-4d48-4bab-a334-4738a1b0eb4a">

Co-authored-by: Jason Song <i@wolfogre.com>
@delvh
Copy link
Member

delvh commented May 16, 2023 •
edited
Loading

Hmm… I don't know why this PR was merged, and why it is necessary in the first place.
I mean, I think it's a much better user experience that trailing spaces are cut as they can appear when copying a secret.
On the other hand, secrets with trailing spaces are such an uncommon case that I don't think we should support them.

So, for me, the logical choice would be to restore the previous behavior to avoid user complaints.
This depends of course if we want secrets to be

  1. usable for everyone but not user-friendly
    or
  2. easily usable for 99% of developers.

I'm in favor of option 2.

@delvh
Copy link
Member

delvh commented May 16, 2023

Oh, by the way, this PR was certainly not a bug but expected behavior. So, if at all it's an enhancement, although I'd still doubt that (see above).
I think I even commented something of the form

I like that we trim trailing whitespace around Secrets unlike GitHub

on the original secrets PR.

@silverwind
Copy link
Member

I generall prefer to trim all input fields as well, it's such a common user error that I think it warrants it. In case a user really wants untrimmed values, a option could be added to the user settings to disable the trimming.

zjjhot added a commit to zjjhot/gitea that referenced this pull request May 18, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
d507440 
* giteaofficial/main:
  Enable two vue eslint rules (go-gitea#24780)
  Add two eslint plugins (go-gitea#24776)
  Mark `models/fixtures` as generated (go-gitea#24775)
  Fix TestMinioStorageIterator skip message (go-gitea#24765)
  Fix missed table name on iterate lfs meta objects (go-gitea#24768)
  Revert "Fix missed table name on iterate lfs meta objects" (go-gitea#24764)
  Make the color of zero-contribution-squares in the activity heatmap more subtle (go-gitea#24758)
  Fix missed table name on iterate lfs meta objects
  Skip TestMinioStorageIterator on CI (go-gitea#24762)
  Support no label/assignee filter and batch clearing labels/assignees (go-gitea#24707)
  Support for status check pattern (go-gitea#24633)
  Updates to doc (go-gitea#24757)
  Ignore build for docs only (go-gitea#24761)
  Fix team members API endpoint pagination (go-gitea#24754)
  Make mailer SMTP check have timed context (go-gitea#24751)
  Add @garymoon to MAINTAINERS (go-gitea#24752)
  Skip TestRepoCommitsStatusParallel on CI (go-gitea#24741)
  Respect original content when creating secrets (go-gitea#24745)
Codeberg-org pushed a commit to Codeberg-org/gitea that referenced this pull request Jun 3, 2023
@GiteaBot @earl-warren
Respect original content when creating secrets (go-gitea#24745) (go-g…
f6919e3 
…itea#24746)

Backport go-gitea#24745 by @wolfogre

Fix go-gitea#24721.

Follow what GitHub does:
- Don't trim spaces for secrets.
- Newline should be `\n` instead of `\r\n`.

Did some tests with:

```yaml
name: secrets
on: push
jobs:
  show_secrets:
    runs-on: ubuntu-latest
    steps:
      - name: Dump secrets context
        run: echo '${{ toJSON(secrets) }}' | base64
```

`AAAAAA`:
```text
   AAAAAA
AAAAAA

```
`BBBBBB`:
```text

BBBBBB
BBBBBB
```

On GitHub:

<img width="675" alt="image"
src="https://github.com/go-gitea/gitea/assets/9418365/0ec60652-c2a3-47bb-9f9d-7e81665355a8">

On Gitea (before):

<img width="673" alt="image"
src="https://github.com/go-gitea/gitea/assets/9418365/cce818bf-5edc-4656-86e1-2c81c304cdb2">

On Gitea (after):

<img width="673" alt="image"
src="https://github.com/go-gitea/gitea/assets/9418365/0b3b15af-4d48-4bab-a334-4738a1b0eb4a">

Co-authored-by: Jason Song <i@wolfogre.com>
(cherry picked from commit e4f200e)
@wxiaoguang wxiaoguang mentioned this pull request Jun 5, 2023
Merged
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. outdated/backport/v1.19 This PR should be backported to Gitea 1.19 size/S Denotes a PR that changes 10-29 lines, ignoring generated files. topic/gitea-actions related to the actions of Gitea type/bug
Projects
None yet
Milestone
1.20.0
Development

Successfully merging this pull request may close these issues.

Action Secrets remove the last new line
6 participants
@wolfogre @wxiaoguang @delvh @silverwind @lunny @GiteaBot