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

Fix/upload artifact error windows #27802

Merged
merged 9 commits into from
Oct 30, 2023
Merged

Fix/upload artifact error windows #27802

merged 9 commits into from
Oct 30, 2023

Conversation

fuxiaohei
Copy link
Contributor

@fuxiaohei fuxiaohei commented Oct 26, 2023 •
edited
Loading

From issue #27314

When act_runner in host mode on Windows. upload_artifact@v3 actions use path.join to generate itemPath params when uploading artifact chunk. itemPath is encoded as ${artifact_name}\${artifact_path}.

It's twice query escaped from ${artifact_name}/${artifact_path} that joined by Windows slash .

So we need convert Windows slash to linux.

In #27314, runner shows logs from upload_artifact@v3 like with %255C:

[artifact-cases/test-artifact-cases]   | ::error::Unexpected response. Unable to upload chunk to http://192.168.31.230:3000/api/actions_pipeline/_apis/pipelines/workflows/6/artifacts/34d628a422db9367c869d3fb36be81f5/upload?itemPath=more-files%255Css.json

But in gitea server at the same time, But shows %5C

2023/10/27 19:29:51 ...eb/routing/logger.go:102:func1() [I] router: completed PUT /api/actions_pipeline/_apis/pipelines/workflows/6/artifacts/34d628a422db9367c869d3fb36be81f5/upload?itemPath=more-files%5Css.json for 192.168.31.230:55340, 400 Bad Request in 17.6ms @ <autogenerated>:1(actions.artifactRoutes.uploadArtifact-fm)

I found %255C is escaped by https://github.com/actions/upload-artifact/blob/main/dist/index.js#L2329.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 26, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 26, 2023
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Oct 26, 2023
@techknowlogick techknowlogick added the backport/v1.21 This PR should be backported to Gitea 1.21 label Oct 26, 2023
@KN4CK3R
Copy link
Member

KN4CK3R commented Oct 26, 2023

Isn't that something the runner should do?

@silverwind
Copy link
Member

silverwind commented Oct 26, 2023 •
edited
Loading

Double encoding would definitely be a bug in the runner.

Also util.PathJoinRel seems to be meant for joining 2 or more paths. Calling it with a single path seems wrong and I think path.Clean would achieve the same in a cleaner manner.

@wxiaoguang
Copy link
Contributor

It's not right to do url.QueryUnescape(ctx.Req.URL.Query().Get("itemPath")) (double decoding)

I think it should figure out the runner's bug first.

@fuxiaohei
Copy link
Contributor Author

It's not right to do url.QueryUnescape(ctx.Req.URL.Query().Get("itemPath")) (double decoding)

I think it should figure out the runner's bug first.

I make a mistake that confusing logs from upload_artifact@v3. I rewrite the description of this bug above.

@wxiaoguang
Copy link
Contributor

Double encoding would definitely be a bug in the runner.

Also util.PathJoinRel seems to be meant for joining 2 or more paths. Calling it with a single path seems wrong and I think path.Clean would achieve the same in a cleaner manner.

// PathJoinRel joins the path elements into a single path, each element is cleaned by path.Clean separately.

// PathJoinRelX joins the path elements into a single path like PathJoinRel,
// and covert all backslashes to slashes. (X means "extended", also means the combination of `\` and `/`).

PathJoinRelX is the right function to call.

@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 Oct 28, 2023
@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 Oct 30, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 30, 2023
@lunny lunny enabled auto-merge (squash) October 30, 2023 08:03
@lunny lunny merged commit ec0c682 into go-gitea:main Oct 30, 2023
24 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Oct 30, 2023
@GiteaBot GiteaBot mentioned this pull request Oct 30, 2023
Merged
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Oct 30, 2023
@fuxiaohei @wxiaoguang @GiteaBot
Fix/upload artifact error windows (go-gitea#27802)
0966909 
From issue go-gitea#27314

When act_runner in `host` mode on Windows. `upload_artifact@v3` actions
use `path.join` to generate `itemPath` params when uploading artifact
chunk. `itemPath` is encoded as `${artifact_name}\${artifact_path}`.

<del>It's twice query escaped from ${artifact_name}/${artifact_path}
that joined by Windows slash \.</del>

**So we need convert Windows slash to linux**.

In go-gitea#27314, runner shows logs
from `upload_artifact@v3` like with `%255C`:

```
[artifact-cases/test-artifact-cases]   | ::error::Unexpected response. Unable to upload chunk to http://192.168.31.230:3000/api/actions_pipeline/_apis/pipelines/workflows/6/artifacts/34d628a422db9367c869d3fb36be81f5/upload?itemPath=more-files%255Css.json
```

But in gitea server at the same time, But shows `%5C`

```
2023/10/27 19:29:51 ...eb/routing/logger.go:102:func1() [I] router: completed PUT /api/actions_pipeline/_apis/pipelines/workflows/6/artifacts/34d628a422db9367c869d3fb36be81f5/upload?itemPath=more-files%5Css.json for 192.168.31.230:55340, 400 Bad Request in 17.6ms @ <autogenerated>:1(actions.artifactRoutes.uploadArtifact-fm)
```

I found `%255C` is escaped by
`https://github.com/actions/upload-artifact/blob/main/dist/index.js#L2329`.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@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 Oct 30, 2023
lunny pushed a commit that referenced this pull request Oct 30, 2023
@GiteaBot
Fix/upload artifact error windows (#27802) (#27840)
3c03b7d
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 31, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
54d5771 
* giteaofficial/main:
  Fix package webhook (go-gitea#27839)
  Add user secrets API integration tests (go-gitea#27832)
  Fix wrong relative path on obtain token from command line (go-gitea#27850)
  doc: actions/act-runner: document obtaining a runner registration token from gitea CLI (go-gitea#27845)
  Fix/upload artifact error windows (go-gitea#27802)
  Always use whole user name as link (go-gitea#27815)
  Fix display member unit in the menu bar if there are no hidden members in public org (go-gitea#27795)
  Add Index to pull_auto_merge.doer_id (go-gitea#27811)
  Bump workflows in github actions (go-gitea#27836)
  Allow pull requests Manually Merged option to be used by non-admins (go-gitea#27780)
fuxiaohei added a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
@fuxiaohei @wxiaoguang
Fix/upload artifact error windows (go-gitea#27802)
dc74a41 
From issue go-gitea#27314

When act_runner in `host` mode on Windows. `upload_artifact@v3` actions
use `path.join` to generate `itemPath` params when uploading artifact
chunk. `itemPath` is encoded as `${artifact_name}\${artifact_path}`.

<del>It's twice query escaped from ${artifact_name}/${artifact_path}
that joined by Windows slash \.</del>

**So we need convert Windows slash to linux**.

In go-gitea#27314, runner shows logs
from `upload_artifact@v3` like with `%255C`:

```
[artifact-cases/test-artifact-cases]   | ::error::Unexpected response. Unable to upload chunk to http://192.168.31.230:3000/api/actions_pipeline/_apis/pipelines/workflows/6/artifacts/34d628a422db9367c869d3fb36be81f5/upload?itemPath=more-files%255Css.json
```

But in gitea server at the same time, But shows `%5C`

```
2023/10/27 19:29:51 ...eb/routing/logger.go:102:func1() [I] router: completed PUT /api/actions_pipeline/_apis/pipelines/workflows/6/artifacts/34d628a422db9367c869d3fb36be81f5/upload?itemPath=more-files%5Css.json for 192.168.31.230:55340, 400 Bad Request in 17.6ms @ <autogenerated>:1(actions.artifactRoutes.uploadArtifact-fm)
```

I found `%255C` is escaped by
`https://github.com/actions/upload-artifact/blob/main/dist/index.js#L2329`.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jan 28, 2024
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 backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.
7 participants
@fuxiaohei @KN4CK3R @silverwind @wxiaoguang @lunny @techknowlogick @GiteaBot